From cbbf1d9ff0f261e14747adae04df6e69ebb99358 Mon Sep 17 00:00:00 2001 From: scttbnsn <80784472+scttbnsn@users.noreply.github.com> Date: Sun, 26 Jul 2026 16:30:11 -0400 Subject: [PATCH 1/8] =?UTF-8?q?=F0=9F=90=9B=20fix(store):=20keep=20the=20s?= =?UTF-8?q?oak=20clock=20across=20container=20recreates?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit getResultSignature guards whether a recreated container inherits its stashed updateDetectedAt, firstSeenAt, and maturityGatePendingSince. It hashed tag + digest + created unconditionally, so a drifting created value discarded the stash and reset the maturity soak to now, right after an update landed. This is the same defect class as #565, surviving in a path #568 never touched. #568 fixed hasCandidateIdentityChanged, which guards the same-container-ID recheck; the recreate path has its own comparison and kept the old behavior. created now participates only when no digest is available, matching hasCandidateIdentityChanged exactly: with a digest present the candidate is already uniquely identified and created is display-only metadata that registries don't all derive from the digest, so it can differ between the pre-delete snapshot and the post-recreate rescan without the candidate changing. Without a digest it stays the sole immutable discriminator. Regression test fails without the source change and passes with it; the companion test pins that a genuine tag change still resets the clock, so the fix cannot degrade into never resetting. --- app/store/container.test.ts | 59 +++++++++++++++++++++++++++++++++++++ app/store/container.ts | 24 +++++++++++++-- 2 files changed, 81 insertions(+), 2 deletions(-) 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..b1e026ed2 100644 --- a/app/store/container.ts +++ b/app/store/container.ts @@ -114,13 +114,33 @@ function toCacheKey(watcher, name) { return `${watcher}::${name}`; } +/** + * Signature of the update candidate a recreated container's stashed lifecycle + * entry is compared against (see `deleteContainer`'s `replacementExpected` + * stash and its `insertContainer` consumer). Must define "same candidate" + * identically to `hasCandidateIdentityChanged` (#568), which restarts the + * maturity soak on the same-container-ID recheck path: tag/digest are the + * candidate identity, and `created` only participates when no digest is + * available (legacy manifests, where it's the sole immutable discriminator). + * + * When a digest IS present, `created` is display-only metadata that can drift + * between the pre-delete snapshot and the post-recreate rescan (registries + * don't all cover it by the digest hash — e.g. a "last pushed"/tag-metadata + * field vs. the image config `created`) without the candidate itself + * changing. Including it unconditionally here (as this function did before + * this fix) silently reset the soak clock on every such recreate, right after + * an update lands — the same defect class as #565/#568, surviving in the + * recreate path because #568 only touched `hasCandidateIdentityChanged` + * (the same-ID recheck path), never this function. + */ function getResultSignature( c: { result?: { tag?: unknown; digest?: unknown; created?: unknown } } | undefined, ): string { + const digest = c?.result?.digest; return JSON.stringify({ tag: c?.result?.tag ?? null, - digest: c?.result?.digest ?? null, - created: c?.result?.created ?? null, + digest: digest ?? null, + created: digest === undefined ? (c?.result?.created ?? null) : null, }); } From ed1bfe27ca0111edb0b6d774e19d4d2c1b8ed15b Mon Sep 17 00:00:00 2001 From: scttbnsn <80784472+scttbnsn@users.noreply.github.com> Date: Sun, 26 Jul 2026 16:50:39 -0400 Subject: [PATCH 2/8] =?UTF-8?q?=F0=9F=94=84=20refactor(model):=20consolida?= =?UTF-8?q?te=20candidate-identity=20comparison=20into=20one=20helper?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit hasCandidateIdentityChanged (model/container.ts), getResultSignature (store/container.ts, just fixed by cbbf1d9f), and computeResultHash (store/notification-history.ts) each hand-rolled the same "is this the same update candidate" comparison and had drifted out of sync with each other — the exact defect class #565/#568 exist to fix. Export getCandidateIdentityFields from model/container.ts as the single definition: tag + digest always participate, created only when digest is undefined. Rewire all three consumers to derive from it. Behavior of hasCandidateIdentityChanged and getResultSignature is unchanged (their existing tests pass untouched). --- CHANGELOG.md | 5 +++ app/model/container.ts | 51 +++++++++++++++++++++++--- app/store/container.ts | 33 ++++++----------- app/store/notification-history.test.ts | 40 ++++++++++++++++++-- app/store/notification-history.ts | 22 +++++++---- 5 files changed, 112 insertions(+), 39 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 44cf91ef9..5976a6fa2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,11 @@ 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. + ## [1.6.0-rc.6] — 2026-07-26 ### Added diff --git a/app/model/container.ts b/app/model/container.ts index 127a5a84f..da67c59b3 100644 --- a/app/model/container.ts +++ b/app/model/container.ts @@ -911,6 +911,48 @@ 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 +969,10 @@ 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.ts b/app/store/container.ts index b1e026ed2..3d5545e42 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, @@ -117,30 +118,18 @@ function toCacheKey(watcher, name) { /** * Signature of the update candidate a recreated container's stashed lifecycle * entry is compared against (see `deleteContainer`'s `replacementExpected` - * stash and its `insertContainer` consumer). Must define "same candidate" - * identically to `hasCandidateIdentityChanged` (#568), which restarts the - * maturity soak on the same-container-ID recheck path: tag/digest are the - * candidate identity, and `created` only participates when no digest is - * available (legacy manifests, where it's the sole immutable discriminator). - * - * When a digest IS present, `created` is display-only metadata that can drift - * between the pre-delete snapshot and the post-recreate rescan (registries - * don't all cover it by the digest hash — e.g. a "last pushed"/tag-metadata - * field vs. the image config `created`) without the candidate itself - * changing. Including it unconditionally here (as this function did before - * this fix) silently reset the soak clock on every such recreate, right after - * an update lands — the same defect class as #565/#568, surviving in the - * recreate path because #568 only touched `hasCandidateIdentityChanged` - * (the same-ID recheck path), never this function. + * 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?: { tag?: unknown; digest?: unknown; created?: unknown } } | undefined, -): string { - const digest = c?.result?.digest; +function getResultSignature(c: { result?: container.ContainerResult } | undefined): string { + const fields = getCandidateIdentityFields(c?.result); return JSON.stringify({ - tag: c?.result?.tag ?? null, - digest: digest ?? null, - created: digest === undefined ? (c?.result?.created ?? null) : null, + tag: fields.tag ?? null, + digest: fields.digest ?? null, + created: fields.created ?? null, }); } 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..f3a27e566 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 { getCandidateIdentityFields, type Container } 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, }; From 2d83bc7c22cc4ed8595a5bdad87df7e09ab2c791 Mon Sep 17 00:00:00 2001 From: scttbnsn <80784472+scttbnsn@users.noreply.github.com> Date: Sun, 26 Jul 2026 16:46:55 -0400 Subject: [PATCH 3/8] =?UTF-8?q?=F0=9F=90=9B=20fix(api):=20stop=20maturity-?= =?UTF-8?q?gated=20containers=20leaking=20through=20age=20fallback?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit getContainerUpdateAge()'s firstSeenAt/publishedAt/updateDetectedAt fallback ignored updateAvailable entirely, unlike the model layer's getRawUpdateAge() which already short-circuits to undefined when no update is available. Callers in maturity-filter.ts and sorting.ts had no guard of their own, so a container actively blocked by maturityMode=mature could still surface under ?maturity=mature/hot and be ranked by ?sort=age, contradicting its own updateEligibility blockers in the same response. Guard with a plain falsy check (not === false) so updateAvailable left unset (never evaluated) and updateAvailable: false (evaluated and gated) are both treated as "nothing to age", matching the existing convention in model/container.ts. Updates the pinned crud.test.ts fallback test to set updateAvailable: true so it keeps covering the fallback's legitimate case, and adds a regression test proving a gated container (updateAvailable: false) is excluded from both the maturity filter and the age sort. --- app/api/container/crud.test.ts | 48 +++++++++++++++++++++++++++++++ app/api/container/filters.test.ts | 1 + app/api/container/update-age.ts | 9 ++++++ 3 files changed, 58 insertions(+) diff --git a/app/api/container/crud.test.ts b/app/api/container/crud.test.ts index 30b24b32a..0d87692f9 100644 --- a/app/api/container/crud.test.ts +++ b/app/api/container/crud.test.ts @@ -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,46 @@ 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('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/update-age.ts b/app/api/container/update-age.ts index cd8b57451..d81993537 100644 --- a/app/api/container/update-age.ts +++ b/app/api/container/update-age.ts @@ -6,6 +6,15 @@ export function getContainerUpdateAge(container: Container): number | undefined return age; } + // 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 deliberate — `updateAvailable` left unset (never + // evaluated) and `updateAvailable: false` (evaluated and gated, e.g. by + // maturityMode) both mean "nothing to age" here, same as upstream. + if (!container.updateAvailable) { + return undefined; + } + // Fallback for containers not processed through validate() — includes // updateDetectedAt as a third date source that the model layer omits. const firstSeenAtMs = Date.parse(container.firstSeenAt || ''); From 26dbf41eb61e3856ce059f96468e523e65a1759f Mon Sep 17 00:00:00 2001 From: scttbnsn <80784472+scttbnsn@users.noreply.github.com> Date: Sun, 26 Jul 2026 16:58:08 -0400 Subject: [PATCH 4/8] =?UTF-8?q?=F0=9F=90=9B=20fix(security):=20stop=20scan?= =?UTF-8?q?=20write-back=20from=20reverting=20the=20maturity=20soak=20cloc?= =?UTF-8?q?k?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit runBulkScan() and persistAndBroadcast() each captured a Container snapshot before awaiting a vulnerability scan (seconds to minutes for Trivy/Grype plus optional SBOM/signature checks), then wrote back by spreading that stale snapshot with only `security` changed. If a watcher poll legitimately detected a real update while the scan was in flight, updateContainer()'s merge doesn't preserve `result` from the live record, so the write-back silently reverted `result`, `updateDetectedAt`, and `maturityGatePendingSince`. The next poll re-detected the same update and stamped a new timestamp, discarding all elapsed maturity soak time — silently, with nothing logged. Mirror the pattern already used by security/scheduler.ts (scanDigestGroup): re-fetch the current record via storeContainer.getContainerRaw(id) at write-back time and merge only the `security` field onto that live record. If the record is gone (container removed or recreated under a new id) skip the write instead of resurrecting a zombie record. - bulk-security.ts: runBulkScan's per-container write-back now reads getContainerRaw(containerId) and skips silently when gone (mirrors the existing broadcastScanCompleted-then-continue error handling). - security.ts: persistAndBroadcast now reads getContainerRaw(id); when gone it broadcasts completion and returns 404 instead of writing. - container.ts: wires getContainerRaw into the bulk-security dependency composition (security.ts already receives the whole store module, which exposes it). Rewrites the persistence assertions in bulk-security.test.ts and security.test.ts to the fetch-then-merge pattern and adds a regression test per file: start a scan via a deferred promise, mutate the store record mid-flight to simulate a concurrent watcher poll, resolve the scan, and assert the live update fields survive and only `security` changed. Both fail before this fix. Also adds a vanished-container regression test per file, and syncs api/container.test.ts's scanContainer suite to mirror getContainerRaw with getContainer since none of those tests simulate concurrent mutation. --- app/api/container.test.ts | 8 +++ app/api/container.ts | 1 + app/api/container/bulk-security.test.ts | 73 +++++++++++++++++++++++++ app/api/container/bulk-security.ts | 27 ++++++--- app/api/container/security.test.ts | 72 ++++++++++++++++++++++++ app/api/container/security.ts | 25 ++++++++- 6 files changed, 196 insertions(+), 10 deletions(-) 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/security.test.ts b/app/api/container/security.test.ts index 01551be30..d4c74e64c 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,75 @@ 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('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..8c452bc2b 100644 --- a/app/api/container/security.ts +++ b/app/api/container/security.ts @@ -19,6 +19,7 @@ import { getPathParamValue } from './request-helpers.js'; interface SecurityStoreContainerApi { getContainer: (id: string) => Container | undefined; + getContainerRaw: (id: string) => Container | undefined; updateContainer: (container: Container) => Container; } @@ -397,11 +398,29 @@ function persistAndBroadcast(options: { status: ContainerSecurityScan['status']; res: Response; }): void { - const { context, id, container, securityPatch, status, res } = options; + const { context, id, 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; + } + const containerToStore = { - ...container, + ...current, security: { - ...(container.security || {}), + ...(current.security || {}), ...securityPatch, }, }; From ccee28dc3f91e4f8f165a09ca24485ef8e2a8ac6 Mon Sep 17 00:00:00 2001 From: scttbnsn <80784472+scttbnsn@users.noreply.github.com> Date: Sun, 26 Jul 2026 16:14:38 -0400 Subject: [PATCH 5/8] =?UTF-8?q?=F0=9F=94=A7=20chore(release):=20make=20ide?= =?UTF-8?q?ntity=20tests=20handle=20the=20GA=20version=20shape?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit release-docs-identity.test.mjs threw "RC_VERSION must end in -rc." as soon as the version constant became a plain GA version (1.6.0, no -rc.N suffix), which would have broken the v1.6 GA cut. The guard only existed to build the quickstart negative-lookahead that catches a stale rc-number row. Replace the throw with a branch on whether RC_VERSION actually has an -rc.N suffix: - Prerelease shape: unchanged behavior — exact "Immutable release candidate" row match plus the same-prefix negative lookahead for a stale rc number. - GA shape: assert the quickstart tag matrix lists the 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 — not a no-op), and assert no release-candidate row remains for this release line. All other assertions in the file (badge, site-config, updates heading, API version samples, changelog links) were already shape-independent and needed no change. release-identity.test.mjs required no change: its `${RC_VERSION}0` boundary check already works for both shapes, since the boundary regex rejects any extra trailing version character regardless of whether the base string has an rc suffix. Verified the GA branch is non-vacuous by setting RC_VERSION to 1.6.0, fixing only the README badge, and confirming that specific assertion passed while later assertions against still-un-bumped files kept failing on real content mismatches. --- scripts/release-docs-identity.test.mjs | 43 +++++++++++++++++--------- 1 file changed, 28 insertions(+), 15 deletions(-) diff --git a/scripts/release-docs-identity.test.mjs b/scripts/release-docs-identity.test.mjs index 61c687e88..9bd12bba0 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,31 @@ 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, /\| `[^`]*-rc\.\d+` \| Immutable release candidate/u); + } + assert.ok(changelog.includes(`## [${RC_VERSION}] — ${RC_DATE}`)); assert.ok( changelog.includes( From c20393c58012edeb4103908b8baa83f585963f67 Mon Sep 17 00:00:00 2001 From: scttbnsn <80784472+scttbnsn@users.noreply.github.com> Date: Sun, 26 Jul 2026 17:01:05 -0400 Subject: [PATCH 6/8] =?UTF-8?q?=F0=9F=93=9D=20docs(changelog):=20document?= =?UTF-8?q?=20the=20scan=20write-back=20and=20update-age=20fixes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The two fixes cherry-picked from fix/v1.6-scan-writeback landed without CHANGELOG entries. Adds both under [Unreleased] alongside the two identity-drift entries they belong with — all four are the same defect class and ship as one candidate. Also corrects getContainerRaw's docstring, which still claimed it was "only used by the env reveal endpoint" — the scan scheduler already used it, and the two write-back paths fixed here now do too. --- CHANGELOG.md | 2 ++ app/api/container/update-age.ts | 2 +- app/store/container.ts | 8 +++++++- 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5976a6fa2..999a829f6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **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 diff --git a/app/api/container/update-age.ts b/app/api/container/update-age.ts index d81993537..65fde69a7 100644 --- a/app/api/container/update-age.ts +++ b/app/api/container/update-age.ts @@ -8,7 +8,7 @@ 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 deliberate — `updateAvailable` left unset (never + // (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. if (!container.updateAvailable) { diff --git a/app/store/container.ts b/app/store/container.ts index 3d5545e42..61986eb62 100644 --- a/app/store/container.ts +++ b/app/store/container.ts @@ -1421,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) { From f2f6dd717d4cb5d3f06a6cf4dc9713c1413d0a7f Mon Sep 17 00:00:00 2001 From: scttbnsn <80784472+scttbnsn@users.noreply.github.com> Date: Sun, 26 Jul 2026 17:06:09 -0400 Subject: [PATCH 7/8] =?UTF-8?q?=F0=9F=8E=A8=20style(app):=20apply=20biome?= =?UTF-8?q?=20import-order=20and=20line-width=20fixes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Formatting-only fallout from the getCandidateIdentityFields consolidation: type-vs-value import ordering in notification-history.ts and two line-width wraps in model/container.ts. No behavior change. --- app/model/container.ts | 8 ++++---- app/store/notification-history.ts | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/model/container.ts b/app/model/container.ts index da67c59b3..cfb6e3ed4 100644 --- a/app/model/container.ts +++ b/app/model/container.ts @@ -942,9 +942,7 @@ export interface CandidateIdentityFields { * @param result * @returns {CandidateIdentityFields} */ -export function getCandidateIdentityFields( - result: Container['result'], -): CandidateIdentityFields { +export function getCandidateIdentityFields(result: Container['result']): CandidateIdentityFields { const digest = result?.digest; return { tag: result?.tag, @@ -972,7 +970,9 @@ export function hasCandidateIdentityChanged( const current = getCandidateIdentityFields(currentResult); const other = getCandidateIdentityFields(otherResult); return ( - current.tag !== other.tag || current.digest !== other.digest || current.created !== other.created + current.tag !== other.tag || + current.digest !== other.digest || + current.created !== other.created ); } diff --git a/app/store/notification-history.ts b/app/store/notification-history.ts index f3a27e566..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 { getCandidateIdentityFields, type Container } from '../model/container.js'; +import { type Container, getCandidateIdentityFields } from '../model/container.js'; import { initCollection } from './util.js'; export type NotificationEventKind = From cebf61e7f15f5fee486148c12c6ce5f7db45618b Mon Sep 17 00:00:00 2001 From: scttbnsn <80784472+scttbnsn@users.noreply.github.com> Date: Sun, 26 Jul 2026 17:47:57 -0400 Subject: [PATCH 8/8] =?UTF-8?q?=F0=9F=90=9B=20fix(api):=20address=20review?= =?UTF-8?q?=20findings=20on=20the=20identity-drift=20batch?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three of CodeRabbit's four findings on #607 were real. 1. update-age.ts: the updateAvailable gate ran AFTER reading the cached container.updateAge, so it only covered the fallback path. On a validated container updateAge is a live getter that re-runs getRawUpdateAge and self-gates, but any spread or structuredClone materializes it into a plain frozen number. buildFallbackContainerReport (docker-helpers.ts:63) spreads the container and then sets updateAvailable = false on the next line, producing a stale finite age on a container with no available update. Gate moved above the cache read. 2. security.ts: scanUpdateImage computes updateScan/updateSignature/ updateSbom against the pre-scan container.result.tag. The write-back re-fetch added earlier in this branch makes result live again, so a candidate that moved mid-scan would get the previous candidate's CVEs, signature, and SBOM attached to it. Before the re-fetch the snapshot and the patch were at least consistently stale together; making result live is what split them. persistAndBroadcast now compares candidates with hasCandidateIdentityChanged and clears the three update-image keys when they diverge, matching how scanUpdateImage already clears them when no update is available. Current-image scan/signature/sbom still apply, since they describe the running image, not the candidate. 3. release-docs-identity.test.mjs: the GA branch rejected an "Immutable release candidate" row for ANY version line while its own comment claimed it was scoped to this release line. Scoped the regex to ${RC_VERSION}-rc.N. Re-verified it still fails against the real un-bumped quickstart rather than passing vacuously. The fourth (duplicate `const db` in container.test.ts) was not real: the two declarations are in separate test() callbacks, so they are different function scopes. The module parses and the full suite runs green. Each new test was run against the pre-fix source and observed to fail. Also sets updateAvailable: true on the sort=age fixture, which set updateAge with no updateAvailable and only passed because the cached read came first. --- app/api/container/crud.test.ts | 53 ++++++++++++++- app/api/container/maturity-filter.test.ts | 14 +++- app/api/container/security.test.ts | 79 +++++++++++++++++++++++ app/api/container/security.ts | 34 +++++++++- app/api/container/update-age.ts | 20 ++++-- scripts/release-docs-identity.test.mjs | 5 +- 6 files changed, 190 insertions(+), 15 deletions(-) diff --git a/app/api/container/crud.test.ts b/app/api/container/crud.test.ts index 0d87692f9..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 }), ], }); @@ -1487,6 +1487,53 @@ describe('api/container/crud', () => { } }); + 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/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 d4c74e64c..308cacff7 100644 --- a/app/api/container/security.test.ts +++ b/app/api/container/security.test.ts @@ -1119,6 +1119,85 @@ describe('api/container/security', () => { 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(); diff --git a/app/api/container/security.ts b/app/api/container/security.ts index 8c452bc2b..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, @@ -398,7 +402,7 @@ function persistAndBroadcast(options: { status: ContainerSecurityScan['status']; res: Response; }): void { - const { context, id, securityPatch, status, res } = options; + 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. @@ -417,11 +421,35 @@ function persistAndBroadcast(options: { 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 = { ...current, security: { ...(current.security || {}), - ...securityPatch, + ...patchToApply, }, }; const updatedContainer = context.storeContainer.updateContainer(containerToStore); diff --git a/app/api/container/update-age.ts b/app/api/container/update-age.ts index 65fde69a7..1c25e792f 100644 --- a/app/api/container/update-age.ts +++ b/app/api/container/update-age.ts @@ -1,20 +1,30 @@ import type { Container } from '../../model/container.js'; export function getContainerUpdateAge(container: Container): number | undefined { - const age = container.updateAge; - if (typeof age === 'number' && Number.isFinite(age)) { - return age; - } - // 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; + } + // Fallback for containers not processed through validate() — includes // updateDetectedAt as a third date source that the model layer omits. const firstSeenAtMs = Date.parse(container.firstSeenAt || ''); diff --git a/scripts/release-docs-identity.test.mjs b/scripts/release-docs-identity.test.mjs index 9bd12bba0..1649607da 100644 --- a/scripts/release-docs-identity.test.mjs +++ b/scripts/release-docs-identity.test.mjs @@ -67,7 +67,10 @@ test('public release surfaces identify the v1.6 release candidate', () => { // 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, /\| `[^`]*-rc\.\d+` \| Immutable release candidate/u); + assert.doesNotMatch( + quickstart, + new RegExp(`\\| \`${escapedRcVersion}-rc\\.\\d+\` \\| Immutable release candidate`, 'u'), + ); } assert.ok(changelog.includes(`## [${RC_VERSION}] — ${RC_DATE}`));