From 1451b34ff552646c430d3879f1fc6be84430eb64 Mon Sep 17 00:00:00 2001 From: scttbnsn <80784472+scttbnsn@users.noreply.github.com> Date: Tue, 4 Aug 2026 15:03:24 -0400 Subject: [PATCH 1/2] =?UTF-8?q?=F0=9F=90=9B=20fix(update-eligibility):=20s?= =?UTF-8?q?often=20agent-mismatch=20during=20trigger=20registration=20wind?= =?UTF-8?q?ow?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes: #605 AgentClient._doHandshake() deregisters an agent's components before awaiting the /api/triggers fetch and re-register. A read during that window found zero triggers for the agent and computeUpdateEligibility raised agent-mismatch as a hard blocker, disabling the Update button for a condition that was purely transient and self-corrected once registration finished. - makeBlocker() accepts an optional severity override - UpdateEligibilityContext gains isAgentPendingRegistration(agentName), wired into the three eligibility call sites (container list, SSE enrichment, manual update request) via the agent manager's getAgent(...)?.isConnected - agent-mismatch downgrades to soft when the mismatched trigger's agent is still connecting; unaffected otherwise --- CHANGELOG.md | 1 + app/agent/AgentClient.test.ts | 51 ++++++++++ app/agent/AgentClient.ts | 116 +++++++++++++--------- app/api/container/handlers/list.test.ts | 73 ++++++++++++++ app/api/container/handlers/list.ts | 2 + app/api/sse-container-enrichment.test.ts | 105 ++++++++++++++++++-- app/api/sse-container-enrichment.ts | 3 + app/model/update-eligibility.test.ts | 118 +++++++++++++++++++++++ app/model/update-eligibility.ts | 68 +++++++++---- app/updates/request-update.test.ts | 24 +++++ app/updates/request-update.ts | 6 ++ 11 files changed, 495 insertions(+), 72 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f3cf36899..8cb2096df 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **"Agent Mismatch" no longer appears in the container list/SSE display during the brief window an agent's docker/dockercompose trigger is still (re)registering** ([#605](https://github.com/CodesWhat/drydock/issues/605)). Eligibility is recomputed live on every read, and `AgentClient._doHandshake()` deregisters the agent's components before awaiting the `/api/triggers` fetch and re-register. A read in that window found zero triggers for the agent and `computeUpdateEligibility` raised a hard `agent-mismatch` blocker, disabling the Update button, even though nothing was actually misconfigured — the condition self-corrected once registration finished. `agent-mismatch` now downgrades to a soft blocker (button stays enabled) on display surfaces whenever the container's own agent is mid-registration, per the new `AgentClient.isRegisteringComponents` flag (true only for the deregister→re-register span, not the whole reconnect backoff). Update **admission** (`app/updates/request-update.ts`) is unaffected and stays hard/fail-closed throughout, so an update can never be enqueued through a wrong-agent trigger during that window. - **WebSocket log streams no longer reject anonymous-auth sessions** ([#636](https://github.com/CodesWhat/drydock/issues/636)). Both WS upgrade paths — the system log stream and the container log stream — gated on `isAuthenticatedSession()` requiring `session.passport.user`, which `passport-anonymous` never sets, so under `DD_ANONYMOUS_AUTH_CONFIRM=true` the log stream WebSocket always rejected the upgrade even though every REST endpoint worked. `isAuthenticatedSession` now also accepts the session when anonymous authentication is the registered mode. - **Maturity clock: swallowed auth errors surfaced, per-container threshold respected** ([#604](https://github.com/CodesWhat/drydock/issues/604)). `getImagePublishedAt` failures — including GHCR/LSCR 401/403 auth errors — now log at `warn` instead of `debug`, so the maturity gate's silent fallback from the registry `publishedAt` to `updateDetectedAt` is no longer invisible. `getRawUpdateMaturityLevel` (`app/model/container.ts`) and `getContainerMaturityLevel` (`app/api/container/maturity-filter.ts`) now resolve each container's own `updatePolicy.maturityMinAgeDays` before falling back to the global `DD_UI_MATURITY_THRESHOLD_DAYS`, matching the gate's own `isUpdateSuppressed`/`isMaturityGatePending` logic so the hot/mature badge can no longer disagree with the gate in the same API response. - **Container start/stop/restart/rollback return an explicit 501 instead of an ambiguous 404 for agent containers without lifecycle transport** ([#637](https://github.com/CodesWhat/drydock/issues/637)). `POST /:id/start|stop|restart` and `POST /:id/rollback` returned a bare 404 `No docker trigger found for this container` whenever the lookup missed, indistinguishable from "container not found" — for agent-owned containers this was the only signal the UI got. That lookup miss now returns 501 naming the likely cause (the agent's connection typically hasn't advertised `usesControllerDockerTransport`) when `container.agent` is set; non-agent containers still get the existing 404. This complements the native-transport support that shipped in rc.11 via [#651](https://github.com/CodesWhat/drydock/pull/651), which closed #637's core gap — this is the remaining explicit-error half. diff --git a/app/agent/AgentClient.test.ts b/app/agent/AgentClient.test.ts index b26504a68..b02a8ddc0 100644 --- a/app/agent/AgentClient.test.ts +++ b/app/agent/AgentClient.test.ts @@ -743,6 +743,50 @@ describe('AgentClient', () => { expect(client.isConnected).toBe(true); }); + test('sets isRegisteringComponents for the deregister -> re-register span and resets after completion (#605)', async () => { + const observedDuringDeregister: boolean[] = []; + const observedDuringWatcherFetch: boolean[] = []; + const observedDuringTriggerFetch: boolean[] = []; + + vi.mocked(registry.deregisterAgentComponents).mockImplementationOnce(async () => { + observedDuringDeregister.push(client.isRegisteringComponents); + }); + + axios.get + .mockResolvedValueOnce({ data: [] }) // containers + .mockImplementationOnce(async () => { + observedDuringWatcherFetch.push(client.isRegisteringComponents); + return { data: [] }; + }) + .mockImplementationOnce(async () => { + observedDuringTriggerFetch.push(client.isRegisteringComponents); + return { data: [] }; + }); + + storeContainer.getContainers.mockReturnValue([]); + + expect(client.isRegisteringComponents).toBe(false); + await client.handshake(); + + expect(observedDuringDeregister).toEqual([true]); + expect(observedDuringWatcherFetch).toEqual([true]); + expect(observedDuringTriggerFetch).toEqual([true]); + expect(client.isRegisteringComponents).toBe(false); + }); + + test('resets isRegisteringComponents to false when a mid-handshake step throws (#605)', async () => { + axios.get.mockResolvedValueOnce({ data: [] }); // containers succeeds + vi.mocked(registry.deregisterAgentComponents).mockRejectedValueOnce( + new Error('deregister failed'), + ); + storeContainer.getContainers.mockReturnValue([]); + + await expect(client.handshake()).rejects.toThrow('deregister failed'); + + expect(client.isRegisteringComponents).toBe(false); + expect(client.isConnected).toBe(false); + }); + test('should emit agent-connected when transitioning to connected state', async () => { axios.get .mockResolvedValueOnce({ data: [] }) @@ -1120,6 +1164,13 @@ describe('AgentClient', () => { expect(spy).toHaveBeenCalled(); }); + test('should reset isRegisteringComponents to false on disconnect (#605)', () => { + client.isConnected = true; + client.isRegisteringComponents = true; + client.scheduleReconnect(1000); + expect(client.isRegisteringComponents).toBe(false); + }); + test('should not schedule duplicate reconnects', () => { const spy = vi.spyOn(client, 'startSse').mockImplementation(() => {}); client.scheduleReconnect(1000); diff --git a/app/agent/AgentClient.ts b/app/agent/AgentClient.ts index 499c4ddf6..118a54c4b 100644 --- a/app/agent/AgentClient.ts +++ b/app/agent/AgentClient.ts @@ -276,6 +276,18 @@ export class AgentClient { // Parsed once at construction when authmode is 'ed25519'; undefined in token mode. private readonly ed25519PrivateKey?: KeyObject; public isConnected: boolean; + /** + * True only for the span inside `_doHandshake()` between deregistering this + * agent's components and finishing their re-registration (watchers, then + * triggers). Eligibility display surfaces (container list, SSE enrichment) + * read this to soften `agent-mismatch` / `no-update-trigger-configured` to a + * soft blocker during that transient window — see issue #605. Always reset + * in a `finally` so a handshake failure, or a disconnect via + * `scheduleReconnect()`, reverts to the hard-blocker default. Admission + * (`app/updates/request-update.ts`) never reads this field and stays + * fail-closed throughout. + */ + public isRegisteringComponents: boolean; public info: AgentClientRuntimeInfo; private reconnectTimer: NodeJS.Timeout | null; private reconnectAttempts: number; @@ -310,6 +322,7 @@ export class AgentClient { } this.isConnected = false; + this.isRegisteringComponents = false; this.info = {}; this.reconnectTimer = null; this.reconnectAttempts = 0; @@ -945,53 +958,62 @@ export class AgentClient { const containers = response.data; this.log.info(`Handshake successful. Received ${containers.length} containers.`); - // Unregister existing components for this agent - await registry.deregisterAgentComponents(this.name); - - // Fetch and register watchers + // isRegisteringComponents is true for the entire deregister → re-register + // span below, including the container-inventory apply that happens + // between watcher and trigger registration. The `finally` guarantees it + // reverts to false whether registration succeeds or this method throws. + this.isRegisteringComponents = true; try { - const responseWatchers = await axios.get( - `${this.baseUrl}/api/watchers`, - this.buildRequestConfig('GET', '/api/watchers'), - ); - await this.registerAgentWatchersTransactional(responseWatchers.data); - // Only transfer update-enrichment ownership after every controller-side - // watcher/delegate has registered successfully. - this.setControllerDockerTransportWatchers(responseWatchers.data); - this.seedWatcherSnapshotCacheFromHandshake(responseWatchers.data); - } catch (error: unknown) { - this.log.warn(`Failed to fetch/register watchers: ${getErrorMessage(error)}`); - } - - // Apply inventory only after watcher registration. Controller-transport - // descriptors change which fields are authoritative: Portwing owns live - // runtime state, while Drydock's native watcher owns update enrichment. - await this.processAuthoritativeContainers(containers); - // A zero-container handshake is ambiguous: it could mean the agent has - // no running containers, or its in-memory store is fresh-empty after a - // restart while docker still has running containers. Defer the prune - // until the first authoritative watcher snapshot arrives — that path is - // unambiguous because the snapshot is only emitted after a successful - // enumeration with no enrichment errors (#362, #386 / d02080ae). - // Pruning here would wipe last-known state for an agent that's about to - // re-populate it in seconds via its first watch cycle. - if (containers.length > 0) { - this.pruneOldContainers(containers); - } else if (this.hasConnectedOnce) { - this.log.warn( - 'Handshake returned 0 containers; preserving last-known state until the first watch cycle completes', - ); - } + // Unregister existing components for this agent + await registry.deregisterAgentComponents(this.name); - // Fetch and register triggers - try { - const responseTriggers = await axios.get( - `${this.baseUrl}/api/triggers`, - this.buildRequestConfig('GET', '/api/triggers'), - ); - await this.registerAgentComponents('trigger', responseTriggers.data); - } catch (error: unknown) { - this.log.warn(`Failed to fetch/register triggers: ${getErrorMessage(error)}`); + // Fetch and register watchers + try { + const responseWatchers = await axios.get( + `${this.baseUrl}/api/watchers`, + this.buildRequestConfig('GET', '/api/watchers'), + ); + await this.registerAgentWatchersTransactional(responseWatchers.data); + // Only transfer update-enrichment ownership after every controller-side + // watcher/delegate has registered successfully. + this.setControllerDockerTransportWatchers(responseWatchers.data); + this.seedWatcherSnapshotCacheFromHandshake(responseWatchers.data); + } catch (error: unknown) { + this.log.warn(`Failed to fetch/register watchers: ${getErrorMessage(error)}`); + } + + // Apply inventory only after watcher registration. Controller-transport + // descriptors change which fields are authoritative: Portwing owns live + // runtime state, while Drydock's native watcher owns update enrichment. + await this.processAuthoritativeContainers(containers); + // A zero-container handshake is ambiguous: it could mean the agent has + // no running containers, or its in-memory store is fresh-empty after a + // restart while docker still has running containers. Defer the prune + // until the first authoritative watcher snapshot arrives — that path is + // unambiguous because the snapshot is only emitted after a successful + // enumeration with no enrichment errors (#362, #386 / d02080ae). + // Pruning here would wipe last-known state for an agent that's about to + // re-populate it in seconds via its first watch cycle. + if (containers.length > 0) { + this.pruneOldContainers(containers); + } else if (this.hasConnectedOnce) { + this.log.warn( + 'Handshake returned 0 containers; preserving last-known state until the first watch cycle completes', + ); + } + + // Fetch and register triggers + try { + const responseTriggers = await axios.get( + `${this.baseUrl}/api/triggers`, + this.buildRequestConfig('GET', '/api/triggers'), + ); + await this.registerAgentComponents('trigger', responseTriggers.data); + } catch (error: unknown) { + this.log.warn(`Failed to fetch/register triggers: ${getErrorMessage(error)}`); + } + } finally { + this.isRegisteringComponents = false; } this.isConnected = true; @@ -1070,6 +1092,10 @@ export class AgentClient { const reconnectDelay = delay ?? this.getNextReconnectDelayMs(); const wasConnected = this.isConnected; this.isConnected = false; + // A disconnect is never a "still registering" state — it's a hard loss of + // the agent. Reset unconditionally so a disconnect that races a still-running + // _doHandshake() cannot leave eligibility softened after the connection drops. + this.isRegisteringComponents = false; if (wasConnected) { void emitAgentDisconnected({ agentName: this.name, diff --git a/app/api/container/handlers/list.test.ts b/app/api/container/handlers/list.test.ts index e47cd1109..151645a90 100644 --- a/app/api/container/handlers/list.test.ts +++ b/app/api/container/handlers/list.test.ts @@ -1587,6 +1587,79 @@ describe('buildEligibilityContext cross-agent scoping (issue #411)', () => { (result as any).updateEligibility.blockers.find((b: any) => b.reason === 'active-operation'), ).toBeDefined(); }); + + describe('agent-mismatch severity during registration window (#605)', () => { + function mismatchedDockerTrigger() { + return { + type: 'docker', + agent: 'agent-a', + configuration: { threshold: 'all' }, + getId: () => 'docker.update', + isTriggerIncluded: () => true, + isTriggerExcluded: () => false, + }; + } + + test('downgrades to soft when the container agent is still completing registration', () => { + const container = createContainerWithUpdate({ agent: 'agent-b' }); + const getAgent = vi.fn().mockReturnValue({ isRegisteringComponents: true }); + const context: CrudHandlerContext = { + ...createMockContext(), + getTriggers: vi + .fn() + .mockReturnValue({ 'docker.update': mismatchedDockerTrigger() } as never), + getAgent, + }; + + const result = attachUpdateEligibility(context, container); + + const blocker = (result as any).updateEligibility.blockers.find( + (b: any) => b.reason === 'agent-mismatch', + ); + expect(blocker).toBeDefined(); + expect(blocker.severity).toBe('soft'); + expect(getAgent).toHaveBeenCalledWith('agent-b'); + }); + + test('stays hard when the agent is not pending registration', () => { + const container = createContainerWithUpdate({ agent: 'agent-b' }); + const context: CrudHandlerContext = { + ...createMockContext(), + getTriggers: vi + .fn() + .mockReturnValue({ 'docker.update': mismatchedDockerTrigger() } as never), + getAgent: vi.fn().mockReturnValue(undefined), + }; + + const result = attachUpdateEligibility(context, container); + + const blocker = (result as any).updateEligibility.blockers.find( + (b: any) => b.reason === 'agent-mismatch', + ); + expect(blocker).toBeDefined(); + expect(blocker.severity).toBe('hard'); + }); + + test('falls back to an empty agent name when the container has no agent', () => { + const container = createContainerWithUpdate(); + const getAgent = vi.fn().mockReturnValue(undefined); + const context: CrudHandlerContext = { + ...createMockContext(), + getTriggers: vi + .fn() + .mockReturnValue({ 'docker.update': mismatchedDockerTrigger() } as never), + getAgent, + }; + + const result = attachUpdateEligibility(context, container); + + const blocker = (result as any).updateEligibility.blockers.find( + (b: any) => b.reason === 'agent-mismatch', + ); + expect(blocker).toBeDefined(); + expect(getAgent).toHaveBeenCalledWith(''); + }); + }); }); describe('createGetContainersHandler', () => { diff --git a/app/api/container/handlers/list.ts b/app/api/container/handlers/list.ts index b7f5dc3b5..278a9cc46 100644 --- a/app/api/container/handlers/list.ts +++ b/app/api/container/handlers/list.ts @@ -208,6 +208,8 @@ export function attachInProgressUpdateOperation( function buildEligibilityContext(context: CrudHandlerContext): UpdateEligibilityContext { return { triggers: context.getTriggers ? context.getTriggers() : undefined, + isAgentPendingRegistration: (agentName) => + context.getAgent(agentName ?? '')?.isRegisteringComponents === true, getActiveOperation: (container: Container) => { const byId = context.updateOperationStore.getActiveOperationByContainerId(container.id); // Scoped by agent+watcher so cross-agent same-named ops don't affect eligibility (issue #411). diff --git a/app/api/sse-container-enrichment.test.ts b/app/api/sse-container-enrichment.test.ts index 4a3bf8778..1478e3ee2 100644 --- a/app/api/sse-container-enrichment.test.ts +++ b/app/api/sse-container-enrichment.test.ts @@ -1,16 +1,25 @@ -var { mockGetState, mockGetActiveOperationByContainerId, mockGetActiveOperationByContainerName } = - vi.hoisted(() => { - return { - mockGetState: vi.fn(() => ({ trigger: {}, watcher: {} })), - mockGetActiveOperationByContainerId: vi.fn(() => undefined), - mockGetActiveOperationByContainerName: vi.fn(() => undefined), - }; - }); +var { + mockGetState, + mockGetActiveOperationByContainerId, + mockGetActiveOperationByContainerName, + mockGetAgent, +} = vi.hoisted(() => { + return { + mockGetState: vi.fn(() => ({ trigger: {}, watcher: {} })), + mockGetActiveOperationByContainerId: vi.fn(() => undefined), + mockGetActiveOperationByContainerName: vi.fn(() => undefined), + mockGetAgent: vi.fn(() => undefined), + }; +}); vi.mock('../registry/index.js', () => ({ getState: mockGetState, })); +vi.mock('../agent/manager.js', () => ({ + getAgent: mockGetAgent, +})); + vi.mock('../store/update-operation.js', () => ({ getActiveOperationByContainerId: mockGetActiveOperationByContainerId, getActiveOperationByContainerName: mockGetActiveOperationByContainerName, @@ -23,9 +32,11 @@ describe('enrichContainerLifecyclePayloadWithEligibility', () => { mockGetState.mockClear(); mockGetActiveOperationByContainerId.mockClear(); mockGetActiveOperationByContainerName.mockClear(); + mockGetAgent.mockClear(); mockGetState.mockReturnValue({ trigger: {}, watcher: {} }); mockGetActiveOperationByContainerId.mockReturnValue(undefined); mockGetActiveOperationByContainerName.mockReturnValue(undefined); + mockGetAgent.mockReturnValue(undefined); }); describe('malformed payload guard', () => { @@ -336,4 +347,82 @@ describe('enrichContainerLifecyclePayloadWithEligibility', () => { ).toBe(true); }); }); + + describe('agent-mismatch severity during registration window (#605)', () => { + function mismatchedDockerTrigger() { + return { + type: 'docker', + agent: 'agent-a', + configuration: { threshold: 'all' }, + getId: () => 'docker.update', + isTriggerIncluded: () => true, + isTriggerExcluded: () => false, + }; + } + + const updatePayload = (): any => ({ + id: 'c1', + name: 'mysql', + image: { tag: { value: '9.6.0' } }, + result: { tag: '9.7.0' }, + }); + + test('downgrades to soft when the container agent is still completing registration', () => { + mockGetState.mockReturnValueOnce({ + trigger: { 'docker.update': mismatchedDockerTrigger() }, + watcher: {}, + }); + mockGetAgent.mockReturnValueOnce({ isRegisteringComponents: true }); + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const result = enrichContainerLifecyclePayloadWithEligibility({ + ...updatePayload(), + agent: 'agent-b', + }) as any; + + const blocker = result.updateEligibility.blockers.find( + (b: { reason: string }) => b.reason === 'agent-mismatch', + ); + expect(blocker).toBeDefined(); + expect(blocker.severity).toBe('soft'); + expect(mockGetAgent).toHaveBeenCalledWith('agent-b'); + }); + + test('stays hard when the agent is not pending registration', () => { + mockGetState.mockReturnValueOnce({ + trigger: { 'docker.update': mismatchedDockerTrigger() }, + watcher: {}, + }); + mockGetAgent.mockReturnValueOnce(undefined); + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const result = enrichContainerLifecyclePayloadWithEligibility({ + ...updatePayload(), + agent: 'agent-b', + }) as any; + + const blocker = result.updateEligibility.blockers.find( + (b: { reason: string }) => b.reason === 'agent-mismatch', + ); + expect(blocker).toBeDefined(); + expect(blocker.severity).toBe('hard'); + }); + + test('falls back to an empty agent name when the container has no agent', () => { + mockGetState.mockReturnValueOnce({ + trigger: { 'docker.update': mismatchedDockerTrigger() }, + watcher: {}, + }); + mockGetAgent.mockReturnValueOnce(undefined); + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const result = enrichContainerLifecyclePayloadWithEligibility(updatePayload()) as any; + + const blocker = result.updateEligibility.blockers.find( + (b: { reason: string }) => b.reason === 'agent-mismatch', + ); + expect(blocker).toBeDefined(); + expect(mockGetAgent).toHaveBeenCalledWith(''); + }); + }); }); diff --git a/app/api/sse-container-enrichment.ts b/app/api/sse-container-enrichment.ts index 0af942311..d8f44fc10 100644 --- a/app/api/sse-container-enrichment.ts +++ b/app/api/sse-container-enrichment.ts @@ -1,3 +1,4 @@ +import { getAgent } from '../agent/manager.js'; import type { ContainerLifecycleEventPayload } from '../event/index.js'; import type { Container } from '../model/container.js'; import { @@ -18,6 +19,8 @@ function buildEligibilityContext(container: Container): UpdateEligibilityContext triggers: registryState.trigger, isSelfUpdateAvailable: isSelfUpdateAvailable(container), maintenanceWindowOpen: getContainerMaintenanceWindowOpen(container, registryState.watcher), + isAgentPendingRegistration: (agentName) => + getAgent(agentName ?? '')?.isRegisteringComponents === true, getActiveOperation: (c: Container) => { const byId = getActiveOperationByContainerId(c.id); // Scoped by agent+watcher so cross-agent same-named ops don't pollute enrichment (issue #411). diff --git a/app/model/update-eligibility.test.ts b/app/model/update-eligibility.test.ts index 0206c11f4..f25196b05 100644 --- a/app/model/update-eligibility.test.ts +++ b/app/model/update-eligibility.test.ts @@ -1006,6 +1006,72 @@ describe('computeUpdateEligibility', () => { const blocker = result.blockers.find((b) => b.reason === 'no-update-trigger-configured'); expect(blocker).toBeDefined(); }); + + describe('severity during the trigger registration window (#605)', () => { + test('downgrades to soft when container is agent-owned and its agent is pending registration', () => { + const container = makeContainerWithTagUpdate({ agent: 'agent-b' }); + const isAgentPendingRegistration = vi.fn().mockReturnValue(true); + const result = computeUpdateEligibility( + container, + makeContext({ + triggers: undefined, + now: FIXED_NOW, + isAgentPendingRegistration, + }), + ); + const blocker = result.blockers.find((b) => b.reason === 'no-update-trigger-configured'); + expect(blocker).toBeDefined(); + expect(blocker?.severity).toBe('soft'); + expect(isAgentPendingRegistration).toHaveBeenCalledWith('agent-b'); + }); + + test('stays hard when container is agent-owned but its agent is not pending registration', () => { + const container = makeContainerWithTagUpdate({ agent: 'agent-b' }); + const isAgentPendingRegistration = vi.fn().mockReturnValue(false); + const result = computeUpdateEligibility( + container, + makeContext({ + triggers: undefined, + now: FIXED_NOW, + isAgentPendingRegistration, + }), + ); + const blocker = result.blockers.find((b) => b.reason === 'no-update-trigger-configured'); + expect(blocker).toBeDefined(); + expect(blocker?.severity).toBe('hard'); + }); + + test('stays hard for non-agent containers even when isAgentPendingRegistration would return true', () => { + const container = makeContainerWithTagUpdate(); // no agent property + const isAgentPendingRegistration = vi.fn().mockReturnValue(true); + const result = computeUpdateEligibility( + container, + makeContext({ + triggers: undefined, + now: FIXED_NOW, + isAgentPendingRegistration, + }), + ); + const blocker = result.blockers.find((b) => b.reason === 'no-update-trigger-configured'); + expect(blocker).toBeDefined(); + expect(blocker?.severity).toBe('hard'); + expect(isAgentPendingRegistration).not.toHaveBeenCalled(); + }); + + test('stays hard for agent-owned containers when no isAgentPendingRegistration callback is supplied', () => { + const container = makeContainerWithTagUpdate({ agent: 'agent-b' }); + const result = computeUpdateEligibility( + container, + makeContext({ + triggers: undefined, + now: FIXED_NOW, + }), + ); + const blocker = result.blockers.find((b) => b.reason === 'no-update-trigger-configured'); + expect(blocker).toBeDefined(); + expect(blocker?.severity).toBe('hard'); + }); + }); }); describe('threshold-not-reached', () => { @@ -1381,6 +1447,58 @@ describe('computeUpdateEligibility', () => { ); expect(result.blockers.find((b) => b.reason === 'agent-mismatch')).toBeUndefined(); }); + + describe('severity during the trigger registration window (#605)', () => { + test('downgrades to soft when isAgentPendingRegistration returns true for the container agent', () => { + const trigger = makeTrigger({ agent: 'agent-a' }); + const container = makeContainerWithTagUpdate({ agent: 'agent-b' }); + const isAgentPendingRegistration = vi.fn().mockReturnValue(true); + const result = computeUpdateEligibility( + container, + makeContext({ + triggers: { 'docker.update': trigger as never }, + now: FIXED_NOW, + isAgentPendingRegistration, + }), + ); + const blocker = result.blockers.find((b) => b.reason === 'agent-mismatch'); + expect(blocker).toBeDefined(); + expect(blocker?.severity).toBe('soft'); + expect(isAgentPendingRegistration).toHaveBeenCalledWith('agent-b'); + }); + + test('stays hard when isAgentPendingRegistration returns false for the container agent', () => { + const trigger = makeTrigger({ agent: 'agent-a' }); + const container = makeContainerWithTagUpdate({ agent: 'agent-b' }); + const isAgentPendingRegistration = vi.fn().mockReturnValue(false); + const result = computeUpdateEligibility( + container, + makeContext({ + triggers: { 'docker.update': trigger as never }, + now: FIXED_NOW, + isAgentPendingRegistration, + }), + ); + const blocker = result.blockers.find((b) => b.reason === 'agent-mismatch'); + expect(blocker).toBeDefined(); + expect(blocker?.severity).toBe('hard'); + }); + + test('stays hard when no isAgentPendingRegistration callback is supplied on the context', () => { + const trigger = makeTrigger({ agent: 'agent-a' }); + const container = makeContainerWithTagUpdate({ agent: 'agent-b' }); + const result = computeUpdateEligibility( + container, + makeContext({ + triggers: { 'docker.update': trigger as never }, + now: FIXED_NOW, + }), + ); + const blocker = result.blockers.find((b) => b.reason === 'agent-mismatch'); + expect(blocker).toBeDefined(); + expect(blocker?.severity).toBe('hard'); + }); + }); }); describe('active-operation', () => { diff --git a/app/model/update-eligibility.ts b/app/model/update-eligibility.ts index 697ba850a..7aeebc6fd 100644 --- a/app/model/update-eligibility.ts +++ b/app/model/update-eligibility.ts @@ -106,8 +106,11 @@ function updateScanMatchesCandidate(container: Container, updateScan: UpdateSecu ); } -function makeBlocker(blocker: Omit): UpdateBlocker { - return { ...blocker, severity: BLOCKER_SEVERITY[blocker.reason] }; +function makeBlocker( + blocker: Omit, + severityOverride?: UpdateBlockerSeverity, +): UpdateBlocker { + return { ...blocker, severity: severityOverride ?? BLOCKER_SEVERITY[blocker.reason] }; } export interface UpdateEligibility { @@ -134,6 +137,20 @@ export interface UpdateEligibilityContext { * eligibility model. Manual UI/API update requests leave it undefined and are never gated by it. */ maintenanceWindowOpen?: boolean; + /** + * Optional. Called with the container's own agent name when the `agent-mismatch` or + * `no-update-trigger-configured` branch is about to fire. Returning `true` (e.g. because + * the agent's client is mid-registration, per `AgentClient.isRegisteringComponents`) + * downgrades that blocker to `soft` instead of the reason's default `hard` severity, so the + * transient window between `AgentClient._doHandshake()` deregistering an agent's components + * and finishing their re-registration doesn't disable manual updates on display surfaces. + * + * This only softens *display* eligibility (container list, SSE enrichment). Admission + * (`app/updates/request-update.ts`) never wires this callback in, so a hard agent-mismatch + * always blocks the actual update request — an update can never be enqueued through a + * wrong-agent trigger during the registration window. See issue #605. + */ + isAgentPendingRegistration?: (agentName: string | undefined) => boolean; } /** @@ -442,32 +459,45 @@ export function computeUpdateEligibility( ); if (!typeOnlyTrigger) { - // 11. no-update-trigger-configured — no docker/dockercompose trigger exists at all + // 11. no-update-trigger-configured — no docker/dockercompose trigger exists at all. + // AgentClient._doHandshake() deregisters an agent's components before re-registering, + // so an agent-owned container can transiently see zero triggers of any kind during that + // window too. Apply the same #605 downgrade as agent-mismatch below. + const isPendingRegistration = container.agent + ? (context.isAgentPendingRegistration?.(container.agent) ?? false) + : false; blockers.push( - makeBlocker({ - reason: 'no-update-trigger-configured', - message: 'No docker or dockercompose action trigger is configured for this container.', - actionable: true, - actionHint: 'Configure `DD_ACTION_DOCKER_*` or `DD_ACTION_DOCKERCOMPOSE_*`.', - }), + makeBlocker( + { + reason: 'no-update-trigger-configured', + message: 'No docker or dockercompose action trigger is configured for this container.', + actionable: true, + actionHint: 'Configure `DD_ACTION_DOCKER_*` or `DD_ACTION_DOCKERCOMPOSE_*`.', + }, + isPendingRegistration ? 'soft' : undefined, + ), ); } else if (!candidateTrigger) { // A docker trigger exists but it's not compatible with this container's agent. // 10. agent-mismatch (detected here because full lookup failed but type-only succeeded) const t = typeOnlyTrigger; const triggerAgent = t.agent; + const isPendingRegistration = context.isAgentPendingRegistration?.(container.agent) ?? false; blockers.push( - makeBlocker({ - reason: 'agent-mismatch', - message: `Update trigger runs on agent '${triggerAgent ?? ''}'; container is on agent '${container.agent ?? ''}'.`, - actionable: true, - actionHint: 'Configure an update trigger for the target agent.', - details: { - triggerAgent, - containerAgent: container.agent, - triggerId: t.getId?.(), + makeBlocker( + { + reason: 'agent-mismatch', + message: `Update trigger runs on agent '${triggerAgent ?? ''}'; container is on agent '${container.agent ?? ''}'.`, + actionable: true, + actionHint: 'Configure an update trigger for the target agent.', + details: { + triggerAgent, + containerAgent: container.agent, + triggerId: t.getId?.(), + }, }, - }), + isPendingRegistration ? 'soft' : undefined, + ), ); } else { const t = candidateTrigger; diff --git a/app/updates/request-update.test.ts b/app/updates/request-update.test.ts index 2eb9a9c26..554591faf 100644 --- a/app/updates/request-update.test.ts +++ b/app/updates/request-update.test.ts @@ -877,6 +877,30 @@ describe('request-update', () => { }); }); + test('stays a hard agent-mismatch rejection even when the agent is still completing registration (#605)', async () => { + // Admission never wires isAgentPendingRegistration in — only display surfaces + // (container list, SSE enrichment) soften agent-mismatch during the agent's + // component re-registration window. A manual update request must still be + // rejected so it can never be enqueued through a wrong-agent trigger. + const trigger = { + type: 'docker', + trigger: vi.fn(), + agent: 'edge-1', + configuration: { threshold: 'all' }, + getId: () => 'docker.update', + isTriggerIncluded: () => true, + isTriggerExcluded: () => false, + }; + mockGetState.mockReturnValue({ trigger: { 'docker.update': trigger } }); + + await expect( + enqueueContainerUpdate(createContainerWithRawUpdate({ agent: 'edge-2' })), + ).rejects.toMatchObject>({ + statusCode: 404, + message: expect.stringContaining("Update trigger runs on agent 'edge-1'"), + }); + }); + test('rejects with 409 when self-update-unavailable blocker fires (drydock self-container, socket absent)', async () => { const trigger = { type: 'docker', diff --git a/app/updates/request-update.ts b/app/updates/request-update.ts index a6c9b4f56..69b1f8910 100644 --- a/app/updates/request-update.ts +++ b/app/updates/request-update.ts @@ -292,6 +292,12 @@ function prepareContainerUpdateRequest( // // The raw-candidate check above is the source of truth for "an update exists" // when a soft gate deliberately makes updateAvailable false. + // + // isAgentPendingRegistration is deliberately NOT wired in here. That softening + // exists only for display surfaces (container list, SSE enrichment) — admission + // stays hard/fail-closed so a hard agent-mismatch can never be bypassed to enqueue + // an update through a wrong-agent trigger during the component re-registration + // window. See issue #605. const eligibility = computeUpdateEligibility(container, { triggers: registry.getState().trigger, getActiveOperation: () => undefined, From 487c16befc8f0b57dcea00f59f9d61615142cd4e Mon Sep 17 00:00:00 2001 From: scttbnsn <80784472+scttbnsn@users.noreply.github.com> Date: Tue, 4 Aug 2026 18:30:57 -0400 Subject: [PATCH 2/2] =?UTF-8?q?=F0=9F=90=9B=20fix(agent):=20set=20isRegist?= =?UTF-8?q?eringComponents=20during=20edge=20component=20sync?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 🐛 fix: handleComponentSync() now wraps its deregister → re-register sequence in the same isRegisteringComponents try/finally as _doHandshake(), so eligibility display stays soft during an awaited edge component sync - ✅ test: cover the flag through the sync sequence, including reset on watcher-registration failure - ✅ test: pin the registration-pending admission path — enqueueContainerUpdate still hard-rejects agent-mismatch while the agent is mid-registration Fixes: #605 --- app/agent/AgentClient.test.ts | 47 +++++++++++++++++++++++++++++- app/agent/AgentClient.ts | 27 +++++++++++------ app/updates/request-update.test.ts | 46 +++++++++++++++++++---------- 3 files changed, 94 insertions(+), 26 deletions(-) diff --git a/app/agent/AgentClient.test.ts b/app/agent/AgentClient.test.ts index b02a8ddc0..9a43e436a 100644 --- a/app/agent/AgentClient.test.ts +++ b/app/agent/AgentClient.test.ts @@ -7626,10 +7626,23 @@ describe('AgentClient', () => { }); describe('handleComponentSync (edge agent public shim)', () => { - test('deregisters agent components and re-registers watchers and triggers', async () => { + test('keeps isRegisteringComponents true through edge component replacement and resets it after success (#605)', async () => { const watchers = [{ type: 'docker', name: 'local', configuration: {} }]; const triggers = [{ type: 'mock', name: 'update', configuration: {} }]; + const observedSteps: string[] = []; + vi.mocked(registry.deregisterAgentComponents).mockImplementationOnce(async () => { + observedSteps.push(`deregister:${client.isRegisteringComponents}`); + }); + vi.mocked(registry.registerComponent) + .mockImplementationOnce(async (component) => { + observedSteps.push(`${component.kind}:${client.isRegisteringComponents}`); + }) + .mockImplementationOnce(async (component) => { + observedSteps.push(`${component.kind}:${client.isRegisteringComponents}`); + }); + + expect(client.isRegisteringComponents).toBe(false); await client.handleComponentSync(watchers, triggers); expect(registry.deregisterAgentComponents).toHaveBeenCalledWith('test-agent'); @@ -7639,6 +7652,38 @@ describe('AgentClient', () => { expect(registry.registerComponent).toHaveBeenCalledWith( expect.objectContaining({ kind: 'trigger', provider: 'mock', name: 'update' }), ); + expect(observedSteps).toEqual(['deregister:true', 'watcher:true', 'trigger:true']); + expect(client.isRegisteringComponents).toBe(false); + }); + + test('resets isRegisteringComponents when edge watcher registration throws (#605)', async () => { + const observedDuringDeregister: boolean[] = []; + let observedDuringWatcherRegistration = false; + + vi.mocked(registry.deregisterAgentComponents) + .mockImplementationOnce(async () => { + observedDuringDeregister.push(client.isRegisteringComponents); + }) + .mockImplementationOnce(async () => { + observedDuringDeregister.push(client.isRegisteringComponents); + }); + vi.mocked(registry.registerComponent).mockImplementationOnce(async () => { + observedDuringWatcherRegistration = client.isRegisteringComponents; + throw new Error('watcher registration failed'); + }); + + await expect( + client.handleComponentSync( + [{ type: 'docker', name: 'local', configuration: {} }], + [{ type: 'mock', name: 'update', configuration: {} }], + ), + ).rejects.toThrow('watcher registration failed'); + + expect(observedDuringDeregister).toEqual([true, true]); + expect(observedDuringWatcherRegistration).toBe(true); + expect(registry.deregisterAgentComponents).toHaveBeenCalledTimes(2); + expect(registry.registerComponent).toHaveBeenCalledTimes(1); + expect(client.isRegisteringComponents).toBe(false); }); test('works with empty watchers and triggers (no-op)', async () => { diff --git a/app/agent/AgentClient.ts b/app/agent/AgentClient.ts index 118a54c4b..082397988 100644 --- a/app/agent/AgentClient.ts +++ b/app/agent/AgentClient.ts @@ -277,9 +277,11 @@ export class AgentClient { private readonly ed25519PrivateKey?: KeyObject; public isConnected: boolean; /** - * True only for the span inside `_doHandshake()` between deregistering this - * agent's components and finishing their re-registration (watchers, then - * triggers). Eligibility display surfaces (container list, SSE enrichment) + * True only while this agent's components are being replaced — the span + * inside `_doHandshake()` (and the equivalent edge-path + * `handleComponentSync()`) between deregistering and finishing the + * re-registration (watchers, then triggers). + * Eligibility display surfaces (container list, SSE enrichment) * read this to soften `agent-mismatch` / `no-update-trigger-configured` to a * soft blocker during that transient window — see issue #605. Always reset * in a `finally` so a handshake failure, or a disconnect via @@ -2183,12 +2185,19 @@ export class AgentClient { watchers: AgentComponentDescriptor[], triggers: AgentComponentDescriptor[], ): Promise { - this.setControllerDockerTransportWatchers([]); - await registry.deregisterAgentComponents(this.name); - await this.registerAgentWatchersTransactional(watchers); - this.setControllerDockerTransportWatchers(watchers); - this.seedWatcherSnapshotCacheFromHandshake(watchers); - await this.registerAgentComponents('trigger', triggers); + // Same deregister → re-register window as _doHandshake(): keep transient + // eligibility blockers soft while components are being replaced. + this.isRegisteringComponents = true; + try { + this.setControllerDockerTransportWatchers([]); + await registry.deregisterAgentComponents(this.name); + await this.registerAgentWatchersTransactional(watchers); + this.setControllerDockerTransportWatchers(watchers); + this.seedWatcherSnapshotCacheFromHandshake(watchers); + await this.registerAgentComponents('trigger', triggers); + } finally { + this.isRegisteringComponents = false; + } } /** diff --git a/app/updates/request-update.test.ts b/app/updates/request-update.test.ts index 554591faf..09ea0b33b 100644 --- a/app/updates/request-update.test.ts +++ b/app/updates/request-update.test.ts @@ -12,18 +12,29 @@ const { mockLogWarn, mockStatSync, mockGetUpdateMode, -} = vi.hoisted(() => ({ - mockGetOperationById: vi.fn(), - mockGetActiveOperationByContainerId: vi.fn(), - mockGetActiveOperationByContainerName: vi.fn(), - mockGetRecentTerminalSucceededOperationByContainerName: vi.fn(() => undefined), - mockHasOtherActiveOperationByContainerName: vi.fn(() => false), - mockInsertOperation: vi.fn(), - mockMarkOperationTerminal: vi.fn(), - mockGetState: vi.fn(() => ({ trigger: {}, watcher: {} })), - mockLogWarn: vi.fn(), - mockStatSync: vi.fn(() => ({ isSocket: () => false })), - mockGetUpdateMode: vi.fn(() => 'auto' as const), + mockGetAgent, + agentFixture, +} = vi.hoisted(() => { + const agentFixture = { name: 'edge-2', isRegisteringComponents: false }; + return { + mockGetOperationById: vi.fn(), + mockGetActiveOperationByContainerId: vi.fn(), + mockGetActiveOperationByContainerName: vi.fn(), + mockGetRecentTerminalSucceededOperationByContainerName: vi.fn(() => undefined), + mockHasOtherActiveOperationByContainerName: vi.fn(() => false), + mockInsertOperation: vi.fn(), + mockMarkOperationTerminal: vi.fn(), + mockGetState: vi.fn(() => ({ trigger: {}, watcher: {} })), + mockLogWarn: vi.fn(), + mockStatSync: vi.fn(() => ({ isSocket: () => false })), + mockGetUpdateMode: vi.fn(() => 'auto' as const), + mockGetAgent: vi.fn(), + agentFixture, + }; +}); + +vi.mock('../agent/manager.js', () => ({ + getAgent: mockGetAgent, })); vi.mock('../store/settings.js', () => ({ @@ -106,6 +117,8 @@ describe('request-update', () => { mockGetState.mockReturnValue({ trigger: {}, watcher: {} }); mockStatSync.mockReturnValue({ isSocket: () => false }); mockGetUpdateMode.mockReturnValue('auto'); + agentFixture.isRegisteringComponents = false; + mockGetAgent.mockReturnValue(undefined); mockInsertOperation.mockImplementation((operation) => ({ id: operation.id || 'op-1', ...operation, @@ -878,10 +891,10 @@ describe('request-update', () => { }); test('stays a hard agent-mismatch rejection even when the agent is still completing registration (#605)', async () => { - // Admission never wires isAgentPendingRegistration in — only display surfaces - // (container list, SSE enrichment) soften agent-mismatch during the agent's - // component re-registration window. A manual update request must still be - // rejected so it can never be enqueued through a wrong-agent trigger. + agentFixture.isRegisteringComponents = true; + mockGetAgent.mockImplementation((name) => + name === agentFixture.name ? agentFixture : undefined, + ); const trigger = { type: 'docker', trigger: vi.fn(), @@ -899,6 +912,7 @@ describe('request-update', () => { statusCode: 404, message: expect.stringContaining("Update trigger runs on agent 'edge-1'"), }); + expect(mockGetAgent).not.toHaveBeenCalled(); }); test('rejects with 409 when self-update-unavailable blocker fires (drydock self-container, socket absent)', async () => {