diff --git a/CHANGELOG.md b/CHANGELOG.md index 44cf91ef9..999a829f6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- **The maturity soak clock no longer resets when a container is recreated.** `getResultSignature` (`app/store/container.ts`) decided whether a recreated container inherits its stashed `updateDetectedAt` / `firstSeenAt` / `maturityGatePendingSince`, and it included `created` even when a digest was present. Registries don't all derive `created` from the digest, so it could differ between the pre-delete snapshot and the post-recreate rescan without the update candidate changing — discarding the stash and silently restarting the soak right after an update landed, which is exactly when clock continuity matters. Same defect class as [#565](https://github.com/CodesWhat/drydock/issues/565), surviving in the recreate path that [#568](https://github.com/CodesWhat/drydock/pull/568) never touched. +- **Notification dedup no longer fires a duplicate `once: true` notification when a manual recheck bypasses the registry poll cache.** `computeResultHash` (`app/store/notification-history.ts`) hashed `suggestedTag` and `created` alongside `tag`/`digest`/`updateKind`, so a recheck that returned a drifted `created`/`suggestedTag` for the same candidate produced a different hash, made `hasAlreadyNotifiedForResult` report "not yet notified", and re-fired notifications that were supposed to be one-shot. The hash now derives from the same candidate-identity definition PR #568 established for the maturity soak (`tag` + `digest`, with `created` participating only when no digest is available), via a single shared helper (`getCandidateIdentityFields` in `app/model/container.ts`) now used by `hasCandidateIdentityChanged`, `getResultSignature`, and `computeResultHash` alike — previously three independently hand-rolled comparators that disagreed with each other. Upgrading may produce one extra notification for updates already in flight, since history persisted under the old hash formula won't match the new one; this is a one-time false positive, not a regression. +- **A security scan finishing no longer reverts an update the watcher detected while it was running.** `runBulkScan` (`app/api/container/bulk-security.ts`) and `persistAndBroadcast` (`app/api/container/security.ts`) both captured a container snapshot before starting the scan, then wrote that whole snapshot back with only `security` changed. Vulnerability scans run for seconds to minutes, so a watcher poll during that window could legitimately detect a new update and have `result`, `updateAvailable`, `updateDetectedAt`, and `maturityGatePendingSince` silently rolled back by the scan's write-back. The next poll re-detected the same update and stamped a fresh timestamp, discarding the elapsed soak time. Both paths now re-fetch the live record at write-back time and merge only `security` onto it, matching what the scan scheduler already did, and skip the write entirely if the container is gone. +- **Containers with no available update are no longer ranked or filtered as if they had one.** `getContainerUpdateAge` (`app/api/container/update-age.ts`) fell back to `firstSeenAt` / `publishedAt` / `updateDetectedAt` without checking `updateAvailable`, so a container gated by `maturityMode` could still appear under `?maturity=mature`, `?maturity=hot`, and `?sort=age` while its own `updateEligibility` blockers in the same response said it was blocked. + ## [1.6.0-rc.6] — 2026-07-26 ### Added diff --git a/app/api/container.test.ts b/app/api/container.test.ts index c3abe30c3..ea168b5b9 100644 --- a/app/api/container.test.ts +++ b/app/api/container.test.ts @@ -1724,6 +1724,14 @@ describe('Container Router', () => { return res; } + beforeEach(() => { + // Write-back re-fetches the current store record via getContainerRaw + // rather than trusting the pre-scan snapshot (see security.ts + // persistAndBroadcast). None of these tests simulate a concurrent + // watcher poll, so mirror whatever getContainer was set up to return. + storeContainer.getContainerRaw.mockImplementation(() => storeContainer.getContainer()); + }); + test('should return 404 when container not found', async () => { storeContainer.getContainer.mockReturnValue(undefined); const res = await callScanContainer('missing'); diff --git a/app/api/container.ts b/app/api/container.ts index 31c25f209..74b188cde 100644 --- a/app/api/container.ts +++ b/app/api/container.ts @@ -237,6 +237,7 @@ const bulkSecurityHandlers = createBulkSecurityHandlers({ storeContainer: { getAllContainers: () => storeContainer.getContainers({}), getContainer: (id) => storeContainer.getContainer(id), + getContainerRaw: (id) => storeContainer.getContainerRaw(id), updateContainer: (c) => storeContainer.updateContainer(c), }, getSecurityConfiguration, diff --git a/app/api/container/bulk-security.test.ts b/app/api/container/bulk-security.test.ts index f4110a261..e924978a2 100644 --- a/app/api/container/bulk-security.test.ts +++ b/app/api/container/bulk-security.test.ts @@ -40,6 +40,7 @@ function createHarness(options: { containers?: Record[] } = {}) const storeContainer = { getAllContainers: vi.fn(() => [...containers]), getContainer: vi.fn((id: string) => containers.find((c) => c.id === id)), + getContainerRaw: vi.fn((id: string) => containers.find((c) => c.id === id)), updateContainer: vi.fn((c: any) => { const idx = containers.findIndex((existing) => existing.id === c.id); if (idx >= 0) { @@ -875,6 +876,78 @@ describe('api/container/bulk-security', () => { expect(harness.deps.updateDigestScanCache).toHaveBeenCalledWith('sha256:abc', scan, ''); }); + + test('does not revert a concurrent watcher update detected while the scan was in flight', async () => { + const initialContainer = createContainer({ + id: 'c1', + name: 'nginx', + updateAvailable: true, + result: { tag: 'a', digest: 'sha256:AAA' }, + updateDetectedAt: 'T0', + }); + const harness = createHarness({ containers: [initialContainer] }); + + let releaseScan: ((scan: ReturnType) => void) | undefined; + const deferredScan = new Promise>((resolve) => { + releaseScan = resolve; + }); + harness.deps.scanImageForVulnerabilities.mockReturnValue(deferredScan); + + await callScanAll(harness.handlers); + + // Let the background scan get past the pre-scan snapshot capture and + // into the (still-pending) scan. + await vi.waitFor(() => { + expect(harness.deps.scanImageForVulnerabilities).toHaveBeenCalled(); + }); + + // Simulate a concurrent watcher poll that legitimately detected a + // newer update and wrote it directly into the store while the scan + // was still in flight. + const liveRecord = { + ...initialContainer, + result: { tag: 'b', digest: 'sha256:BBB' }, + updateDetectedAt: 'T1', + }; + harness.storeContainer.getContainerRaw.mockReturnValue(liveRecord); + + const scan = createScanResult(); + releaseScan?.(scan); + await waitForCycleComplete(harness.deps); + + expect(harness.storeContainer.updateContainer).toHaveBeenCalledTimes(1); + const persisted = harness.storeContainer.updateContainer.mock.calls[0][0]; + expect(persisted.result).toEqual({ tag: 'b', digest: 'sha256:BBB' }); + expect(persisted.updateDetectedAt).toBe('T1'); + expect(persisted.security.scan).toEqual(scan); + }); + + test('skips the write when the container vanished while the scan was in flight', async () => { + const harness = createHarness({ + containers: [{ id: 'c1', name: 'nginx' }], + }); + + let releaseScan: ((scan: ReturnType) => void) | undefined; + const deferredScan = new Promise>((resolve) => { + releaseScan = resolve; + }); + harness.deps.scanImageForVulnerabilities.mockReturnValue(deferredScan); + + await callScanAll(harness.handlers); + + await vi.waitFor(() => { + expect(harness.deps.scanImageForVulnerabilities).toHaveBeenCalled(); + }); + + // Container was removed (or recreated under a new id) mid-scan. + harness.storeContainer.getContainerRaw.mockReturnValue(undefined); + + releaseScan?.(createScanResult()); + await waitForCycleComplete(harness.deps); + + expect(harness.storeContainer.updateContainer).not.toHaveBeenCalled(); + expect(harness.deps.broadcastScanCompleted).toHaveBeenCalledWith('c1', 'passed'); + }); }); describe('rate limiter wiring', () => { diff --git a/app/api/container/bulk-security.ts b/app/api/container/bulk-security.ts index 13deb0a4b..2ff896e26 100644 --- a/app/api/container/bulk-security.ts +++ b/app/api/container/bulk-security.ts @@ -25,6 +25,7 @@ interface BulkSecurityAlertPayload { interface BulkSecurityStoreApi { getAllContainers: () => Container[]; getContainer: (id: string) => Container | undefined; + getContainerRaw: (id: string) => Container | undefined; updateContainer: (container: Container) => Container; } @@ -179,13 +180,25 @@ async function runBulkScan( scannedCount += 1; try { - deps.storeContainer.updateContainer({ - ...container, - security: { - ...(container.security || {}), - scan: scanResult, - }, - }); + // Re-fetch the current store record at write-back time rather than + // spreading the stale `container` snapshot captured when the scan + // was queued. The scan can take seconds to minutes, so by the time + // we write back, a watcher poll may have legitimately advanced + // `result`/`updateAvailable`/`updateDetectedAt`/`maturityGatePendingSince`. + // Spreading the stale snapshot would silently revert those fields + // and reset the maturity soak clock. If the record is gone + // (container removed or recreated under a new id) skip the write + // rather than resurrecting a zombie record. + const current = deps.storeContainer.getContainerRaw(containerId); + if (current) { + deps.storeContainer.updateContainer({ + ...current, + security: { + ...(current.security || {}), + scan: scanResult, + }, + }); + } } catch (persistErr: unknown) { deps.log.info( `Bulk scan persistence failed for container ${containerId} (${deps.getErrorMessage(persistErr)})`, diff --git a/app/api/container/crud.test.ts b/app/api/container/crud.test.ts index 30b24b32a..6f10748a2 100644 --- a/app/api/container/crud.test.ts +++ b/app/api/container/crud.test.ts @@ -684,9 +684,9 @@ describe('api/container/crud', () => { test('supports sort=age and returns oldest updates first', () => { const harness = createHarness({ containers: [ - createContainer({ id: 'c1', updateAge: 2_000 }), - createContainer({ id: 'c2', updateAge: 8_000 }), - createContainer({ id: 'c3', updateAge: 4_000 }), + createContainer({ id: 'c1', updateAvailable: true, updateAge: 2_000 }), + createContainer({ id: 'c2', updateAvailable: true, updateAge: 8_000 }), + createContainer({ id: 'c3', updateAvailable: true, updateAge: 4_000 }), ], }); @@ -1328,19 +1328,23 @@ describe('api/container/crud', () => { containers: [ createContainer({ id: 'c-min', + updateAvailable: true, firstSeenAt: '2026-03-05T00:00:00.000Z', result: { publishedAt: '2026-03-12T00:00:00.000Z' }, }), createContainer({ id: 'c-first', + updateAvailable: true, firstSeenAt: '2026-03-07T00:00:00.000Z', }), createContainer({ id: 'c-published', + updateAvailable: true, result: { publishedAt: '2026-03-09T00:00:00.000Z' }, }), createContainer({ id: 'c-detected', + updateAvailable: true, updateDetectedAt: '2026-03-11T00:00:00.000Z', }), createContainer({ @@ -1376,14 +1380,17 @@ describe('api/container/crud', () => { containers: [ createContainer({ id: 'c-hot', + updateAvailable: true, firstSeenAt: '2026-03-14T00:00:00.000Z', }), createContainer({ id: 'c-mature', + updateAvailable: true, result: { publishedAt: '2026-03-07T00:00:00.000Z' }, }), createContainer({ id: 'c-established', + updateAvailable: true, updateDetectedAt: '2026-02-01T00:00:00.000Z', }), ], @@ -1425,6 +1432,7 @@ describe('api/container/crud', () => { }), createContainer({ id: 'c-hot', + updateAvailable: true, firstSeenAt: '2026-03-14T00:00:00.000Z', }), ], @@ -1439,6 +1447,93 @@ describe('api/container/crud', () => { } }); + test('excludes maturity-gated containers (updateAvailable: false) from the age fallback', () => { + vi.useFakeTimers(); + vi.setSystemTime(new Date('2026-03-15T00:00:00.000Z')); + + try { + const harness = createHarness({ + containers: [ + createContainer({ + id: 'c-gated', + updateAvailable: false, + firstSeenAt: '2026-01-01T00:00:00.000Z', + }), + createContainer({ + id: 'c-hot', + updateAvailable: true, + firstSeenAt: '2026-03-14T00:00:00.000Z', + }), + ], + }); + + // A container with no available update must never surface an age, + // so it cannot be caught by a maturity filter it was never gated + // into, nor ranked by ?sort=age as if it had a real pending update. + const maturityRes = callGetContainers(harness.handlers, { maturity: 'hot' }); + expect( + maturityRes.json.mock.calls[0][0].data.map((container: { id: string }) => container.id), + ).toEqual(['c-hot']); + + const establishedRes = callGetContainers(harness.handlers, { maturity: 'established' }); + expect(establishedRes.json.mock.calls[0][0].data).toEqual([]); + + const sortRes = callGetContainers(harness.handlers, { sort: 'age' }); + expect( + sortRes.json.mock.calls[0][0].data.map((container: { id: string }) => container.id), + ).toEqual(['c-hot', 'c-gated']); + } finally { + vi.useRealTimers(); + } + }); + + test('excludes gated containers carrying a materialized updateAge from the age fallback', () => { + vi.useFakeTimers(); + vi.setSystemTime(new Date('2026-03-15T00:00:00.000Z')); + + try { + // buildFallbackContainerReport (watchers/providers/docker/docker-helpers.ts) + // spreads the container, which turns the self-gating `updateAge` getter into + // a plain frozen number, and only then sets updateAvailable = false. That + // shape reaches this code with a stale finite age attached to a container + // that has no available update, so the availability gate has to run before + // the cached age is read rather than after it. + const harness = createHarness({ + containers: [ + createContainer({ + id: 'c-gated-cached-age', + updateAvailable: false, + updateAge: 73 * 24 * 3600 * 1000, + firstSeenAt: '2026-01-01T00:00:00.000Z', + }), + createContainer({ + id: 'c-hot', + updateAvailable: true, + firstSeenAt: '2026-03-14T00:00:00.000Z', + }), + ], + }); + + const establishedRes = callGetContainers(harness.handlers, { maturity: 'established' }); + expect(establishedRes.json.mock.calls[0][0].data).toEqual([]); + + const matureRes = callGetContainers(harness.handlers, { maturity: 'mature' }); + expect(matureRes.json.mock.calls[0][0].data).toEqual([]); + + const hotRes = callGetContainers(harness.handlers, { maturity: 'hot' }); + expect( + hotRes.json.mock.calls[0][0].data.map((container: { id: string }) => container.id), + ).toEqual(['c-hot']); + + const sortRes = callGetContainers(harness.handlers, { sort: 'age' }); + expect( + sortRes.json.mock.calls[0][0].data.map((container: { id: string }) => container.id), + ).toEqual(['c-hot', 'c-gated-cached-age']); + } finally { + vi.useRealTimers(); + } + }); + test('uses watcher/name/id fallbacks when sorting by age with equal ages', () => { const harness = createHarness({ containers: [ diff --git a/app/api/container/filters.test.ts b/app/api/container/filters.test.ts index 16e39dc9c..92bfcb33a 100644 --- a/app/api/container/filters.test.ts +++ b/app/api/container/filters.test.ts @@ -43,6 +43,7 @@ describe('api/container/filters', () => { const containers = Array.from({ length: 12 }, (_, index) => ({ id: `c${index + 1}`, name: `container-${index + 1}`, + updateAvailable: true, firstSeenAt: `2024-01-${String(index + 1).padStart(2, '0')}T00:00:00.000Z`, })); diff --git a/app/api/container/maturity-filter.test.ts b/app/api/container/maturity-filter.test.ts index 7f502b556..cb2d9bbb0 100644 --- a/app/api/container/maturity-filter.test.ts +++ b/app/api/container/maturity-filter.test.ts @@ -11,9 +11,17 @@ describe('api/container/maturity-filter', () => { test('applyContainerMaturityFilter returns only hot containers', () => { const containers = [ - { id: 'c1', updateAge: 60_000 } as unknown as Container, - { id: 'c2', updateAge: 9 * 24 * 60 * 60 * 1000 } as unknown as Container, - { id: 'c3', updateAge: 35 * 24 * 60 * 60 * 1000 } as unknown as Container, + { id: 'c1', updateAvailable: true, updateAge: 60_000 } as unknown as Container, + { + id: 'c2', + updateAvailable: true, + updateAge: 9 * 24 * 60 * 60 * 1000, + } as unknown as Container, + { + id: 'c3', + updateAvailable: true, + updateAge: 35 * 24 * 60 * 60 * 1000, + } as unknown as Container, ]; const filtered = applyContainerMaturityFilter(containers, 'hot'); diff --git a/app/api/container/security.test.ts b/app/api/container/security.test.ts index 01551be30..308cacff7 100644 --- a/app/api/container/security.test.ts +++ b/app/api/container/security.test.ts @@ -86,6 +86,7 @@ function createHarness( const storeContainer = { getContainer: vi.fn(() => container), + getContainerRaw: vi.fn(() => container), updateContainer: vi.fn((value) => value), }; const deps = { @@ -1073,4 +1074,154 @@ describe('api/container/security', () => { expect(harness.deps.updateDigestScanCache).not.toHaveBeenCalled(); }); }); + + describe('concurrent write-back safety', () => { + test('does not revert a concurrent watcher update detected while the scan was in flight', async () => { + const initialContainer = createContainer({ + result: { tag: 'a', digest: 'sha256:AAA' }, + updateDetectedAt: 'T0', + }); + const harness = createHarness({ container: initialContainer }); + + let releaseScan: ((scan: ReturnType) => void) | undefined; + const deferredScan = new Promise>((resolve) => { + releaseScan = resolve; + }); + harness.deps.scanImageForVulnerabilities.mockReturnValue(deferredScan); + + const scanContainerPromise = callScanContainer(harness.handlers); + + // Let the handler get past the pre-scan snapshot capture and into the + // (still-pending) scan. + await vi.waitFor(() => { + expect(harness.deps.scanImageForVulnerabilities).toHaveBeenCalled(); + }); + + // Simulate a concurrent watcher poll that legitimately detected a + // newer update and wrote it directly into the store while our scan + // was still in flight. + const liveRecord = { + ...initialContainer, + result: { tag: 'b', digest: 'sha256:BBB' }, + updateDetectedAt: 'T1', + }; + harness.storeContainer.getContainerRaw.mockReturnValue(liveRecord); + + const scan = createScanResult(); + releaseScan?.(scan); + const res = await scanContainerPromise; + + expect(harness.storeContainer.updateContainer).toHaveBeenCalledTimes(1); + const persisted = harness.storeContainer.updateContainer.mock.calls[0][0]; + expect(persisted.result).toEqual({ tag: 'b', digest: 'sha256:BBB' }); + expect(persisted.updateDetectedAt).toBe('T1'); + expect(persisted.security.scan).toEqual(scan); + expect(res.status).toHaveBeenCalledWith(200); + }); + + test('discards update-image results when the candidate moved while the scan was in flight', async () => { + // scanUpdateImage resolves the update image from the PRE-scan + // container.result.tag, so its results describe that candidate. The + // write-back re-fetch makes result live again, and applying the patch + // unchanged would attach the old candidate's CVEs and signature to the + // new pending update. + const initialContainer = createContainer({ + result: { tag: '2.0.0', digest: 'sha256:AAA' }, + updateDetectedAt: 'T0', + }); + const harness = createHarness({ + container: initialContainer, + securityConfiguration: { + enabled: true, + scanner: 'trivy', + signature: { verify: true }, + sbom: { enabled: false, formats: [] }, + }, + }); + + const currentScan = createScanResult({ image: CURRENT_IMAGE }); + const updateScan = createScanResult({ image: UPDATE_IMAGE }); + let releaseUpdateScan: ((scan: ReturnType) => void) | undefined; + const deferredUpdateScan = new Promise>((resolve) => { + releaseUpdateScan = resolve; + }); + harness.deps.scanImageForVulnerabilities + .mockResolvedValueOnce(currentScan) + .mockReturnValueOnce(deferredUpdateScan); + harness.deps.verifyImageSignature + .mockResolvedValueOnce(createSignatureResult(CURRENT_IMAGE)) + .mockResolvedValueOnce(createSignatureResult(UPDATE_IMAGE)); + + const scanContainerPromise = callScanContainer(harness.handlers); + + await vi.waitFor(() => { + expect(harness.deps.scanImageForVulnerabilities).toHaveBeenCalledTimes(2); + }); + + harness.storeContainer.getContainerRaw.mockReturnValue({ + ...initialContainer, + result: { tag: '2.1.0', digest: 'sha256:BBB' }, + updateDetectedAt: 'T1', + }); + + releaseUpdateScan?.(updateScan); + const res = await scanContainerPromise; + + expect(harness.storeContainer.updateContainer).toHaveBeenCalledTimes(1); + const persisted = harness.storeContainer.updateContainer.mock.calls[0][0]; + expect(persisted.result).toEqual({ tag: '2.1.0', digest: 'sha256:BBB' }); + expect(persisted.security.updateScan).toBeUndefined(); + expect(persisted.security.updateSignature).toBeUndefined(); + expect(persisted.security.updateSbom).toBeUndefined(); + // The current-image results describe the running image, not the + // candidate, so they must survive. + expect(persisted.security.scan).toEqual(currentScan); + expect(res.status).toHaveBeenCalledWith(200); + }); + + test('keeps update-image results when the candidate is unchanged at write-back', async () => { + const initialContainer = createContainer({ + result: { tag: '2.0.0', digest: 'sha256:AAA' }, + }); + const harness = createHarness({ container: initialContainer }); + + const currentScan = createScanResult({ image: CURRENT_IMAGE }); + const updateScan = createScanResult({ image: UPDATE_IMAGE }); + harness.deps.scanImageForVulnerabilities + .mockResolvedValueOnce(currentScan) + .mockResolvedValueOnce(updateScan); + harness.storeContainer.getContainerRaw.mockReturnValue({ ...initialContainer }); + + await callScanContainer(harness.handlers); + + const persisted = harness.storeContainer.updateContainer.mock.calls[0][0]; + expect(persisted.security.updateScan).toEqual(updateScan); + }); + + test('skips the write and returns 404 when the container vanished while the scan was in flight', async () => { + const harness = createHarness(); + + let releaseScan: ((scan: ReturnType) => void) | undefined; + const deferredScan = new Promise>((resolve) => { + releaseScan = resolve; + }); + harness.deps.scanImageForVulnerabilities.mockReturnValue(deferredScan); + + const scanContainerPromise = callScanContainer(harness.handlers); + + await vi.waitFor(() => { + expect(harness.deps.scanImageForVulnerabilities).toHaveBeenCalled(); + }); + + // Container was removed (or recreated under a new id) mid-scan. + harness.storeContainer.getContainerRaw.mockReturnValue(undefined); + + releaseScan?.(createScanResult()); + const res = await scanContainerPromise; + + expect(harness.storeContainer.updateContainer).not.toHaveBeenCalled(); + expect(res.status).toHaveBeenCalledWith(404); + expect(res.json).toHaveBeenCalledWith({ error: 'Container not found' }); + }); + }); }); diff --git a/app/api/container/security.ts b/app/api/container/security.ts index e8d6388b5..532ef1f84 100644 --- a/app/api/container/security.ts +++ b/app/api/container/security.ts @@ -1,7 +1,11 @@ import type { Request, Response } from 'express'; import type { SecurityConfiguration, SecuritySbomFormat } from '../../configuration/index.js'; import type { SecurityScanCycleCompleteEventPayload } from '../../event/index.js'; -import type { Container, ContainerSecurityState } from '../../model/container.js'; +import { + type Container, + type ContainerSecurityState, + hasCandidateIdentityChanged, +} from '../../model/container.js'; import { getTrivyDatabaseStatus as getTrivyDatabaseStatusDefault, type TrivyDatabaseStatus, @@ -19,6 +23,7 @@ import { getPathParamValue } from './request-helpers.js'; interface SecurityStoreContainerApi { getContainer: (id: string) => Container | undefined; + getContainerRaw: (id: string) => Container | undefined; updateContainer: (container: Container) => Container; } @@ -398,11 +403,53 @@ function persistAndBroadcast(options: { res: Response; }): void { const { context, id, container, securityPatch, status, res } = options; + + // Re-fetch the current store record at write-back time rather than + // merging onto the pre-scan `container` snapshot the caller captured. + // Current/update-image scans (plus optional signature/SBOM checks) can + // run for seconds to minutes, so by the time we persist, a watcher poll + // may have legitimately advanced result/updateAvailable/updateDetectedAt/ + // maturityGatePendingSince. Spreading the stale snapshot would silently + // revert those fields and reset the maturity soak clock. + const current = context.storeContainer.getContainerRaw(id); + if (!current) { + // Container removed or recreated under a new id while the scan was + // in flight — skip the write rather than resurrecting a zombie + // record with the stale pre-scan snapshot. + context.broadcastScanCompleted(id, status); + sendErrorResponse(res, 404, 'Container not found'); + return; + } + + // `scanUpdateImage` computes updateScan/updateSignature/updateSbom against the + // PRE-scan candidate (`container.result.tag`). Re-fetching `current` above makes + // `result` live again, so if a watcher advanced the candidate mid-scan those three + // keys would now be attached to a different image than the one they describe — + // reporting the old candidate's CVEs, signature, and SBOM against the new pending + // update. Before this write-back re-fetch existed the snapshot and the patch were + // at least consistently stale together; making `result` live is what splits them. + // + // When the candidate has moved, clear the three update-image keys instead of + // applying them. Anything already stored describes the superseded candidate too, + // so clearing is correct in both directions and matches how `scanUpdateImage` + // already clears them when no update is available. The next scan repopulates them + // against the current candidate. Current-image `scan`/`signature`/`sbom` are + // unaffected: those describe the running image, not the update candidate. + const candidateMoved = hasCandidateIdentityChanged(container.result, current.result); + const patchToApply = candidateMoved + ? { ...securityPatch, updateScan: undefined, updateSignature: undefined, updateSbom: undefined } + : securityPatch; + if (candidateMoved) { + context.log.info( + `Update candidate for container ${id} changed during the scan; discarding update-image scan results`, + ); + } + const containerToStore = { - ...container, + ...current, security: { - ...(container.security || {}), - ...securityPatch, + ...(current.security || {}), + ...patchToApply, }, }; const updatedContainer = context.storeContainer.updateContainer(containerToStore); diff --git a/app/api/container/update-age.ts b/app/api/container/update-age.ts index cd8b57451..1c25e792f 100644 --- a/app/api/container/update-age.ts +++ b/app/api/container/update-age.ts @@ -1,6 +1,25 @@ import type { Container } from '../../model/container.js'; export function getContainerUpdateAge(container: Container): number | undefined { + // Match the model layer's own gate (model/container.ts getRawUpdateAge): + // no available update means no age to report, full stop. A falsy check + // (not `=== false`) is intentional: `updateAvailable` left unset (never + // evaluated) and `updateAvailable: false` (evaluated and gated, e.g. by + // maturityMode) both mean "nothing to age" here, same as upstream. + // + // This MUST come before reading `container.updateAge` below. On a validated + // container `updateAge` is a live getter that re-runs getRawUpdateAge and is + // therefore self-gating, but any spread or structuredClone materializes it + // into a plain number frozen at copy time. `buildFallbackContainerReport` + // (watchers/providers/docker/docker-helpers.ts) does exactly that: it + // spreads the container, then sets `updateAvailable = false`. The result + // carries a stale finite `updateAge` alongside `updateAvailable: false`, and + // reading the cache first would hand that age straight to maturity filtering + // and age sorting. + if (!container.updateAvailable) { + return undefined; + } + const age = container.updateAge; if (typeof age === 'number' && Number.isFinite(age)) { return age; diff --git a/app/model/container.ts b/app/model/container.ts index 127a5a84f..cfb6e3ed4 100644 --- a/app/model/container.ts +++ b/app/model/container.ts @@ -911,6 +911,46 @@ function hasResultChanged( ); } +export interface CandidateIdentityFields { + tag: string | undefined; + digest: string | undefined; + created: string | undefined; +} + +/** + * Canonical "update candidate identity" for a result: the fields that + * actually define which candidate a result points at, as opposed to + * display-only metadata that can drift between scans without the candidate + * itself changing. + * + * This is the single source of truth for that notion across the codebase — + * `hasCandidateIdentityChanged` (same-container-ID recheck path, #568), + * `getResultSignature` in `app/store/container.ts` (container-recreate path, + * companion to #568), and `computeResultHash` in + * `app/store/notification-history.ts` (notification dedup) all derive from + * this. Three independently hand-rolled comparators previously disagreed + * with each other; do not add a fourth. + * + * `tag` and `digest` always participate. `created` participates only when + * `digest` is undefined: with a digest present the candidate is already + * uniquely identified, and `created` is display-only metadata that + * registries don't all derive identically from the digest, so it can differ + * between scans (e.g. pre-delete snapshot vs. post-recreate rescan, or a + * manual recheck bypassing the poll cache) without the candidate changing. + * Without a digest, `created` is the only available immutable discriminator + * (legacy manifests) and must participate. + * @param result + * @returns {CandidateIdentityFields} + */ +export function getCandidateIdentityFields(result: Container['result']): CandidateIdentityFields { + const digest = result?.digest; + return { + tag: result?.tag, + digest, + created: digest === undefined ? result?.created : undefined, + }; +} + /** * Check whether the update candidate's identity changed, i.e. the tag or * digest a recheck would actually promote. Unlike hasResultChanged, this @@ -927,13 +967,12 @@ export function hasCandidateIdentityChanged( currentResult: Container['result'], otherResult: Container['result'], ): boolean { - if (currentResult?.tag !== otherResult?.tag || currentResult?.digest !== otherResult?.digest) { - return true; - } + const current = getCandidateIdentityFields(currentResult); + const other = getCandidateIdentityFields(otherResult); return ( - currentResult?.digest === undefined && - otherResult?.digest === undefined && - currentResult?.created !== otherResult?.created + current.tag !== other.tag || + current.digest !== other.digest || + current.created !== other.created ); } diff --git a/app/store/container.test.ts b/app/store/container.test.ts index 3be30fc74..237bd9bd2 100644 --- a/app/store/container.test.ts +++ b/app/store/container.test.ts @@ -4942,6 +4942,65 @@ describe('updateLifecycleCache carry-forward', () => { expect(inserted.updateDetectedAt).toBe(twelveHoursAgo); }); + // Recreate-path companion to the #565/#568 same-container-ID fix: #568 taught + // `hasCandidateIdentityChanged` that `created` is display-only metadata once a + // digest is present (it can drift between scans without the candidate itself + // changing), but never touched `getResultSignature`, which gates this recreate + // path. A container deleted and recreated with the SAME tag+digest but a + // drifted `created` therefore mismatched the stashed signature, discarded the + // cache entry, and silently reset the soak clock right after an update landed. + test('#recreate-companion-568: preserves updateDetectedAt/firstSeenAt/maturityGatePendingSince across recreation when only created drifts (digest present)', () => { + const twelveHoursAgo = new Date(Date.now() - 12 * 3600 * 1000).toISOString(); + const oldFixture = makeDigestUpdateFixture({ + id: 'lifecycle-created-drift-old', + result: { tag: 'version', digest: 'sha256:new', created: '2026-06-01T00:00:00.000Z' }, + updateDetectedAt: twelveHoursAgo, + firstSeenAt: twelveHoursAgo, + maturityGatePendingSince: twelveHoursAgo, + }); + const collection = createFilterableCollection([{ data: oldFixture }]); + const db = { getCollection: () => collection, addCollection: () => null }; + container.createCollections(db); + container.deleteContainer('lifecycle-created-drift-old', { replacementExpected: true }); + + // Same tag + same digest as the stashed entry — only the registry-reported + // `created` metadata drifted between the pre-delete snapshot and the + // post-recreate rescan. + const newFixture = makeDigestUpdateFixture({ + id: 'lifecycle-created-drift-new', + result: { tag: 'version', digest: 'sha256:new', created: '2026-06-02T00:00:00.000Z' }, + }); + const inserted = container.insertContainer(newFixture); + expect(inserted.updateDetectedAt).toBe(twelveHoursAgo); + expect(inserted.firstSeenAt).toBe(twelveHoursAgo); + expect(inserted.maturityGatePendingSince).toBe(twelveHoursAgo); + }); + + test('#recreate-companion-568: still resets the soak across recreation when the tag genuinely changes (digest present, created constant)', () => { + const twelveHoursAgo = new Date(Date.now() - 12 * 3600 * 1000).toISOString(); + const oldFixture = makeDigestUpdateFixture({ + id: 'lifecycle-tag-change-old', + result: { tag: 'version', digest: 'sha256:new', created: '2026-06-01T00:00:00.000Z' }, + updateDetectedAt: twelveHoursAgo, + firstSeenAt: twelveHoursAgo, + maturityGatePendingSince: twelveHoursAgo, + }); + const collection = createFilterableCollection([{ data: oldFixture }]); + const db = { getCollection: () => collection, addCollection: () => null }; + container.createCollections(db); + container.deleteContainer('lifecycle-tag-change-old', { replacementExpected: true }); + + // Same digest and created, but a genuinely different tag — must NOT inherit. + const newFixture = makeDigestUpdateFixture({ + id: 'lifecycle-tag-change-new', + result: { tag: 'newversion', digest: 'sha256:new', created: '2026-06-01T00:00:00.000Z' }, + }); + const inserted = container.insertContainer(newFixture); + expect(inserted.updateDetectedAt).not.toBe(twelveHoursAgo); + expect(inserted.firstSeenAt).not.toBe(twelveHoursAgo); + expect(inserted.maturityGatePendingSince).not.toBe(twelveHoursAgo); + }); + test('BLOCKER-1: firstSeenAt is restored from cache independently when incoming has updateDetectedAt but no firstSeenAt', () => { const twelveHoursAgo = new Date(Date.now() - 12 * 3600 * 1000).toISOString(); const incomingDetectedAt = '2026-01-01T00:00:00.000Z'; diff --git a/app/store/container.ts b/app/store/container.ts index 049f0b31b..61986eb62 100644 --- a/app/store/container.ts +++ b/app/store/container.ts @@ -19,6 +19,7 @@ import { } from '../event/index.js'; import { deriveContainerIdentityKey, + getCandidateIdentityFields, hasCandidateIdentityChanged, hasRawUpdate, isRollbackContainerName, @@ -114,13 +115,21 @@ function toCacheKey(watcher, name) { return `${watcher}::${name}`; } -function getResultSignature( - c: { result?: { tag?: unknown; digest?: unknown; created?: unknown } } | undefined, -): string { +/** + * Signature of the update candidate a recreated container's stashed lifecycle + * entry is compared against (see `deleteContainer`'s `replacementExpected` + * stash and its `insertContainer` consumer). Defines "same candidate" via the + * shared `getCandidateIdentityFields` (#568), which is also what + * `hasCandidateIdentityChanged` (same-container-ID recheck path) and + * `computeResultHash` (notification dedup) derive from — see that function's + * docstring for why `created` only participates when no digest is available. + */ +function getResultSignature(c: { result?: container.ContainerResult } | undefined): string { + const fields = getCandidateIdentityFields(c?.result); return JSON.stringify({ - tag: c?.result?.tag ?? null, - digest: c?.result?.digest ?? null, - created: c?.result?.created ?? null, + tag: fields.tag ?? null, + digest: fields.digest ?? null, + created: fields.created ?? null, }); } @@ -1412,7 +1421,13 @@ export function getContainer(id: string) { /** * Get container by id without redacting sensitive env values. - * Only used by the env reveal endpoint. + * + * Used by the env reveal endpoint, and by every write-back path that persists + * a container it fetched earlier (security scans, the scan scheduler). Those + * callers must NOT use `getContainer`: it masks sensitive env to + * `'[REDACTED]'`, and while `updateContainer` restores classified details + * rather than storing the mask, re-fetching raw keeps the write-back honest + * instead of relying on that guard. * @param id */ export function getContainerRaw(id: string) { diff --git a/app/store/notification-history.test.ts b/app/store/notification-history.test.ts index b17a83be4..3aa80a874 100644 --- a/app/store/notification-history.test.ts +++ b/app/store/notification-history.test.ts @@ -39,7 +39,27 @@ describe('notification-history store', () => { ); }); - test('returns a different hash when any result field changes', () => { + // Regression test for a duplicate-notification bug: a manual recheck bypasses the + // registry poll cache, so suggestedTag/created can come back drifted even though the + // candidate (tag + digest) is unchanged. The hash must NOT change here, or + // hasAlreadyNotifiedForResult sees a "new" result and a once:true trigger fires again + // for the same update. This replaces a prior version of this test that asserted the + // opposite (a `created` change producing a different hash), which pinned the bug. + test('returns the same hash when only display-only metadata drifts (digest present)', () => { + const base = { + result: { tag: '2.0', digest: 'sha256:abc', created: '2026-04-15', suggestedTag: '2.0' }, + updateKind: { kind: 'tag', remoteValue: '2.0' }, + } as any; + const drifted = { + ...base, + result: { ...base.result, created: '2026-04-20', suggestedTag: '2.1' }, + }; + expect(notificationHistory.computeResultHash(drifted)).toBe( + notificationHistory.computeResultHash(base), + ); + }); + + test('returns a different hash when the candidate identity or updateKind changes', () => { const base = { result: { tag: '2.0', digest: 'sha256:abc', created: '2026-04-15' }, updateKind: { kind: 'tag', remoteValue: '2.0' }, @@ -60,13 +80,27 @@ describe('notification-history store', () => { expect( notificationHistory.computeResultHash({ ...base, - result: { ...base.result, created: '2026-04-16' }, + updateKind: { ...base.updateKind, remoteValue: '2.1' }, }), ).not.toBe(baseHash); expect( notificationHistory.computeResultHash({ ...base, - updateKind: { ...base.updateKind, remoteValue: '2.1' }, + updateKind: { ...base.updateKind, kind: 'digest' }, + }), + ).not.toBe(baseHash); + }); + + test('treats created as the sole discriminator when no digest is present (legacy manifest path)', () => { + const base = { + result: { tag: 'latest', created: '2026-04-15' }, + updateKind: { kind: 'tag', remoteValue: 'latest' }, + } as any; + const baseHash = notificationHistory.computeResultHash(base); + expect( + notificationHistory.computeResultHash({ + ...base, + result: { ...base.result, created: '2026-04-16' }, }), ).not.toBe(baseHash); }); diff --git a/app/store/notification-history.ts b/app/store/notification-history.ts index d6d8d4e95..b2e8563a8 100644 --- a/app/store/notification-history.ts +++ b/app/store/notification-history.ts @@ -1,6 +1,6 @@ import crypto from 'node:crypto'; import type Loki from 'lokijs'; -import type { Container } from '../model/container.js'; +import { type Container, getCandidateIdentityFields } from '../model/container.js'; import { initCollection } from './util.js'; export type NotificationEventKind = @@ -48,17 +48,23 @@ function buildKey( /** * Compute a stable hash of the fields that define "a notification about this exact update." - * Mirrors the fields used by `hasResultChanged()` so a hash change corresponds exactly to - * what humans would call "a different update". + * Mirrors the candidate identity used by `hasCandidateIdentityChanged()` (#568) — tag and + * digest, with `created` participating only when no digest is available — plus `updateKind`, + * so a hash change corresponds exactly to what humans would call "a different update". + * + * Excludes `suggestedTag` and (when a digest is present) `created` on purpose: those are + * display-only metadata that can drift between scans — most notably on a manual recheck that + * bypasses the registry poll cache — without the candidate itself changing. Hashing them + * caused `hasAlreadyNotifiedForResult` to see a "new" result and fire a duplicate `once: true` + * notification for the same update. */ export function computeResultHash(container: Pick): string { - const result = container.result ?? {}; const updateKind = container.updateKind ?? {}; + const fields = getCandidateIdentityFields(container.result); const payload = { - tag: (result as { tag?: unknown }).tag ?? null, - suggestedTag: (result as { suggestedTag?: unknown }).suggestedTag ?? null, - digest: (result as { digest?: unknown }).digest ?? null, - created: (result as { created?: unknown }).created ?? null, + tag: fields.tag ?? null, + digest: fields.digest ?? null, + created: fields.created ?? null, kind: (updateKind as { kind?: unknown }).kind ?? null, remoteValue: (updateKind as { remoteValue?: unknown }).remoteValue ?? null, }; diff --git a/scripts/release-docs-identity.test.mjs b/scripts/release-docs-identity.test.mjs index 61c687e88..1649607da 100644 --- a/scripts/release-docs-identity.test.mjs +++ b/scripts/release-docs-identity.test.mjs @@ -29,11 +29,10 @@ test('public release surfaces identify the v1.6 release candidate', () => { const changelog = read('CHANGELOG.md'); const escapedRcVersion = escapeRegExp(RC_VERSION); + // RC_VERSION is either a prerelease (e.g. 1.6.0-rc.6) or, at the GA cut, the plain base + // version (e.g. 1.6.0). Both shapes drive every assertion below directly from the + // constant — no shape-specific literal is hand-maintained, and no shape throws. const rcSuffixMatch = /^(.*-rc\.)(\d+)$/u.exec(RC_VERSION); - if (!rcSuffixMatch) { - throw new Error(`RC_VERSION must end in -rc., got: ${RC_VERSION}`); - } - const [, rcPrefix, rcNumber] = rcSuffixMatch; assert.match( readme, @@ -46,17 +45,34 @@ test('public release surfaces identify the v1.6 release candidate', () => { assert.match(agentApi, new RegExp(`"version": "${escapedRcVersion}"`, 'u')); assert.match(portwingApi, new RegExp(`"version": "${escapedRcVersion}"`, 'u')); assert.match(portwingApi, new RegExp(`"drydockVersion": "${escapedRcVersion}"`, 'u')); - assert.match( - quickstart, - new RegExp(`\\| \`${escapedRcVersion}\` \\| Immutable release candidate`, 'u'), - ); - assert.doesNotMatch( - quickstart, - new RegExp( - `\\| \`${escapeRegExp(rcPrefix)}(?!${rcNumber}\\b)\\d+\` \\| Immutable release candidate`, - 'u', - ), - ); + + if (rcSuffixMatch) { + // Prerelease shape: the quickstart tag matrix must show this exact candidate as an + // "Immutable release candidate" row, and must not still show any OTHER rc number under + // the same prefix (a stale/un-bumped candidate row left behind by a partial release cut). + const [, rcPrefix, rcNumber] = rcSuffixMatch; + assert.match( + quickstart, + new RegExp(`\\| \`${escapedRcVersion}\` \\| Immutable release candidate`, 'u'), + ); + assert.doesNotMatch( + quickstart, + new RegExp( + `\\| \`${escapeRegExp(rcPrefix)}(?!${rcNumber}\\b)\\d+\` \\| Immutable release candidate`, + 'u', + ), + ); + } else { + // GA shape: the quickstart tag matrix must list this exact GA tag as its own row (the + // backtick+pipe delimiters mean an un-bumped `X.Y.Z-rc.N` row does NOT satisfy this), + // and no release-candidate row may still be present for this release line. + assert.match(quickstart, new RegExp(`\\| \`${escapedRcVersion}\` \\|`, 'u')); + assert.doesNotMatch( + quickstart, + new RegExp(`\\| \`${escapedRcVersion}-rc\\.\\d+\` \\| Immutable release candidate`, 'u'), + ); + } + assert.ok(changelog.includes(`## [${RC_VERSION}] — ${RC_DATE}`)); assert.ok( changelog.includes(