Storage review follow-ups: boot-only prune, prune-then-persist, cap + persistence tests, named columns #44

julian · 2026-08-16T18:46:04+02:00

julian commented

2026-08-16 18:46:04 +02:00

Closes #43 — all six follow-ups from the approving review on #38.

What

1. The 8 MB cap now has a test pinning the UTF-8 measurement (storage_test.ts). 5 Mi "ä" characters: ~10 MiB in UTF-8 (over the cap), ~5 M UTF-16 code units (under it) — so a check rewritten against string .length fails the test, not just a removed check.

2. The poll-path prune runs only on the boot-seeded map (mod.ts). A seededFromStorage flag is set when rehydration seeds a non-empty map and consumed by the first full, non-empty listing — the same predicate that gates haveFullListing, so an empty first poll (an engine still coming up; reachable only in the bare-process dev shape, since the containerised deployment always lists OpsDeck's own container) neither wipes the seeded map nor unlocks the scoped-refresh prune. A poll landing inside an operator's own compose down / up -d window can no longer drop live verdicts. Tests pin the trade-off from both sides: a poll whose listing is missing exactly the downed project keeps the verdict, and an empty full listing defers instead of wiping — in the boot prune and in the flag.

3. A pruning poll persists the pruned map (new persistUpdates helper, shared with the sweep). Before, only sweeps persisted, so a host whose registry sweeps keep failing re-seeded the same dead verdicts on every restart. Persist stays after publish on every path. New test: a poll that prunes a ghost writes a map without it.

4. The boot comment no longer overstates — it now states the condition its "corrects within one poll" argument depends on: ctx.scheduler.every gives an immediate first tick only at intervals ≤ 5 min; a larger INTERVAL_MS becomes a real schedule whose firstRun: "immediate" does not fire when the schedule row already exists.

5. Sweep persistence is tested end-to-end (mod_test.ts). What storage.set was called with is captured and fed into a second register() — the sweep's set key and boot's get key are checked against each other, so a typo in either "updates" literal now fails a test instead of leaving the feature dead with a green suite.

6. The module_kv insert names its columns (storage.ts, and the corrupt-row fixture in storage_test.ts), so a widening migration 11 fails the statement at parse instead of silently shifting the positional binding.

Boot-only pruning narrows the down/up hole to one shot; it does not remove the class. A stack an operator left down across an OpsDeck restart is absent from the first full listing, so that one prune drops its verdicts and persists the loss until the next sweep — and a stack removed out of band while OpsDeck runs keeps a phantom verdict for up to CHECK_INTERVAL_MS, healed sooner only by the action path. Both directions are stated in the module comment; that trade is the one #43 asked for. The listing capability remains #39.

Tests

mod_test.ts grew from 1 to 8 tests (shared engine/storage mocks extracted, env dance deduped into realDataTest). Every guard clause is mutation-tested: the eight mod-side mutations from both review rounds were each re-run against the current head and each kills at least one test. deno fmt --check, deno lint, deno task check clean. Server + module suites green except the 6 pre-existing Windows-path failures (#20). Reviewed by cavecrew-reviewer: no findings.

🤖 Generated with Claude Code

Closes #43 — all six follow-ups from the approving review on #38. ## What **1. The 8 MB cap now has a test pinning the UTF-8 measurement** (`storage_test.ts`). 5 Mi `"ä"` characters: ~10 MiB in UTF-8 (over the cap), ~5 M UTF-16 code units (under it) — so a check rewritten against string `.length` fails the test, not just a removed check. **2. The poll-path prune runs only on the boot-seeded map** (`mod.ts`). A `seededFromStorage` flag is set when rehydration seeds a non-empty map and consumed by the first **full, non-empty** listing — the same predicate that gates `haveFullListing`, so an empty first poll (an engine still coming up; reachable only in the bare-process dev shape, since the containerised deployment always lists OpsDeck's own container) neither wipes the seeded map nor unlocks the scoped-refresh prune. A poll landing inside an operator's own `compose down` / `up -d` window can no longer drop live verdicts. Tests pin the trade-off from both sides: a poll whose listing is missing exactly the downed project keeps the verdict, and an empty full listing defers instead of wiping — in the boot prune and in the flag. **3. A pruning poll persists the pruned map** (new `persistUpdates` helper, shared with the sweep). Before, only sweeps persisted, so a host whose registry sweeps keep failing re-seeded the same dead verdicts on every restart. Persist stays after publish on every path. New test: a poll that prunes a ghost writes a map without it. **4. The boot comment no longer overstates** — it now states the condition its "corrects within one poll" argument depends on: `ctx.scheduler.every` gives an immediate first tick only at intervals ≤ 5 min; a larger `INTERVAL_MS` becomes a real schedule whose `firstRun: "immediate"` does not fire when the schedule row already exists. **5. Sweep persistence is tested end-to-end** (`mod_test.ts`). What `storage.set` was called with is captured and fed into a second `register()` — the sweep's set key and boot's get key are checked against each other, so a typo in either `"updates"` literal now fails a test instead of leaving the feature dead with a green suite. **6. The `module_kv` insert names its columns** (`storage.ts`, and the corrupt-row fixture in `storage_test.ts`), so a widening migration 11 fails the statement at parse instead of silently shifting the positional binding. Boot-only pruning **narrows the down/up hole to one shot; it does not remove the class.** A stack an operator left `down` across an OpsDeck restart is absent from the first full listing, so that one prune drops its verdicts and persists the loss until the next sweep — and a stack removed out of band while OpsDeck runs keeps a phantom verdict for up to `CHECK_INTERVAL_MS`, healed sooner only by the action path. Both directions are stated in the module comment; that trade is the one #43 asked for. The listing capability remains #39. ## Tests `mod_test.ts` grew from 1 to 8 tests (shared engine/storage mocks extracted, env dance deduped into `realDataTest`). Every guard clause is mutation-tested: the eight mod-side mutations from both review rounds were each re-run against the current head and each kills at least one test. `deno fmt --check`, `deno lint`, `deno task check` clean. Server + module suites green except the 6 pre-existing Windows-path failures (#20). Reviewed by cavecrew-reviewer: no findings. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

julian added 1 commit

2026-08-16 18:46:04 +02:00

fix: close the storage review follow-ups (#43 )

Build and Deploy / verify (pull_request) Successful in 1m41s

Details

Build and Deploy / build (pull_request) Has been skipped

Details

Dependency Check / dependencies (pull_request) Successful in 1m1s

Details

6ecb0a38ab

- storage: the 8 MB cap has a test pinning the UTF-8 measurement (over
  the cap in bytes, under it in UTF-16 code units), and the module_kv
  insert names its columns so a widening migration fails at parse
  instead of silently shifting the positional binding
- docker: the poll-path prune runs only on the boot-seeded map — a poll
  landing inside an operator's own compose down/up window can no longer
  drop live verdicts. A poll that does prune a ghost persists the pruned
  map, so a host whose sweeps keep failing cannot re-seed the same dead
  verdicts on every restart. The boot comment now states the <=5 min
  interval condition its "corrects within one poll" argument depends on
- mod_test: a sweep's persisted verdicts are fed back into a second
  register(), checking the set/get key literals against each other; two
  new tests pin the boot-only prune trade-off and the prune-then-persist

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

julian requested review from thisilike

2026-08-16 18:46:07 +02:00

thisilike requested changes

2026-08-16 21:11:25 +02:00

Dismissed

thisilike left a comment

Re-reviewed at 6ecb0a3 in a detached worktree, checking the claims rather than the description.

Verified independently

The four new tests are load-bearing, not decoration. Mutated the head and re-ran: drop the seededFromStorage guard (prune every poll) → "a live verdict survives a poll that lands inside compose down/up" fails; drop if (prunedGhosts) await persistUpdates() → the persistence test fails; typo the "updates" set key → the sweep/boot key test fails; new TextEncoder().encode(json).length → json.length → the UTF-8 cap test fails. Four mutations, four kills. Items 1, 2, 3 and 5 are genuinely pinned.
Item 6 is correct. The named columns match migration 10 (packages/server/src/metrics/schema.ts:206) in order — module, key, value, updated_at_ms.
Item 4's comment is accurate, not just plausible: INTERVAL_AS_SCHEDULE_MS is 5 min (host.ts:171), and firstRun: "immediate" fires only inside if (!existing) (tasks/service.ts:1334-1337) with the declaration at catchUp: "skip".
Item 1's arithmetic holds. 5 Mi "ä" is 10 MiB in UTF-8 (over the 8 MiB cap) and 5,242,880 UTF-16 code units (under it), and JSON.stringify does not escape non-ASCII, so the value really does reach the encoder as two bytes per character.
deno fmt --check, deno lint, deno task check clean. Full deno task test on Linux: 472 passed, 0 failed (the description's 6 Windows-path failures do not appear here; the 2 failures I hit first were my own gpg signing config in external_modules_test.ts, not this branch).

What I don't buy

One blocking finding inline at mod.ts:278, with its follow-on at mod.ts:324. Short version: the prune this PR reduces to a single shot is not gated on a full listing, and a scoped refresh from any action's finally can consume it before the first poll — reproduced against this head, verdicts for every other project dropped from memory and written back to module_kv.

Separately, the description's "the down/up hole item 2 describes for per-poll pruning is gone by construction now" is too strong. The hole is narrowed to one prune, not removed by construction: that prune still keys off "absent from the current container listing", so an operator whose stack is down across an OpsDeck restart loses live verdicts exactly as item 2 described — and now the loss is persisted, bounded by the same next-sweep the item objected to. Worth saying plainly in the comment instead of claiming the class is gone.

Also worth stating out loud somewhere durable: the accepted cost of boot-only pruning is that a stack removed out of band while OpsDeck runs keeps a phantom verdict for up to CHECK_INTERVAL_MS instead of one poll. Only the action path heals it (refreshUpdates(project)). That is the trade issue #43 asked for, so it is fine — but the module comment currently argues only the other direction.

Nit, no action needed: the test-4 comment "the updates tick polls stacks first (nothing listed yet)" — the poll does list webContainer; the condition is !state.updatedAt.

Everything else here is good work, and the mutation results say the test suite will now actually defend it. Changes requested for the one guard.

Re-reviewed at `6ecb0a3` in a detached worktree, checking the claims rather than the description. ## Verified independently - **The four new tests are load-bearing, not decoration.** Mutated the head and re-ran: drop the `seededFromStorage` guard (prune every poll) → *"a live verdict survives a poll that lands inside compose down/up"* fails; drop `if (prunedGhosts) await persistUpdates()` → the persistence test fails; typo the `"updates"` set key → the sweep/boot key test fails; `new TextEncoder().encode(json).length` → `json.length` → the UTF-8 cap test fails. Four mutations, four kills. Items 1, 2, 3 and 5 are genuinely pinned. - **Item 6 is correct.** The named columns match migration 10 (`packages/server/src/metrics/schema.ts:206`) in order — `module, key, value, updated_at_ms`. - **Item 4's comment is accurate**, not just plausible: `INTERVAL_AS_SCHEDULE_MS` is 5 min (`host.ts:171`), and `firstRun: "immediate"` fires only inside `if (!existing)` (`tasks/service.ts:1334-1337`) with the declaration at `catchUp: "skip"`. - **Item 1's arithmetic holds.** 5 Mi `"ä"` is 10 MiB in UTF-8 (over the 8 MiB cap) and 5,242,880 UTF-16 code units (under it), and `JSON.stringify` does not escape non-ASCII, so the value really does reach the encoder as two bytes per character. - `deno fmt --check`, `deno lint`, `deno task check` clean. Full `deno task test` on Linux: **472 passed, 0 failed** (the description's 6 Windows-path failures do not appear here; the 2 failures I hit first were my own gpg signing config in `external_modules_test.ts`, not this branch). ## What I don't buy One blocking finding inline at `mod.ts:278`, with its follow-on at `mod.ts:324`. Short version: the prune this PR reduces to a single shot is not gated on a *full* listing, and a scoped refresh from any action's `finally` can consume it before the first poll — reproduced against this head, verdicts for every other project dropped from memory **and** written back to `module_kv`. Separately, the description's *"the down/up hole item 2 describes for per-poll pruning is gone by construction now"* is too strong. The hole is narrowed to one prune, not removed by construction: that prune still keys off "absent from the current container listing", so an operator whose stack is `down` across an OpsDeck restart loses live verdicts exactly as item 2 described — and now the loss is persisted, bounded by the same next-sweep the item objected to. Worth saying plainly in the comment instead of claiming the class is gone. Also worth stating out loud somewhere durable: the accepted cost of boot-only pruning is that a stack removed **out of band** while OpsDeck runs keeps a phantom verdict for up to `CHECK_INTERVAL_MS` instead of one poll. Only the action path heals it (`refreshUpdates(project)`). That is the trade issue #43 asked for, so it is fine — but the module comment currently argues only the other direction. Nit, no action needed: the test-4 comment "the updates tick polls stacks first (nothing listed yet)" — the poll does list `webContainer`; the condition is `!state.updatedAt`. Everything else here is good work, and the mutation results say the test suite will now actually defend it. Changes requested for the one guard.

packages/modules/docker/backend/mod.ts Outdated

					
				@ -247,2 +276,3 @@

				      // `state.stacks` is the full set even on a scoped refresh

				      // (mergeScopedStacks merges into it), so pruning here is safe.

				      state.updates = pruneUpdates(state.updates, state.stacks);

				      if (seededFromStorage) {

Blocking: this guard is missing && !project.

refreshStacks(project) is reachable before the first full poll. Every composeAction re-reads its own project in a finally (actions.ts:133), including when the run threw, and stack-lifecycle calls it directly (actions.ts:181). At that point state.stacks is [], mergeScopedStacks returns only the refreshed project (merge.ts:20 filters listed by project), and the one prune this PR still allows runs against that partial set.

Reproduced against this head, with no tick fired — i.e. the window before the first stacks poll:

seeded storage: nginx:1.26 (project web) + postgres:16 (project db)
run stack-lifecycle {project: "web", action: "stop"}   // throws unknown-stack; the finally still refreshes

updates after: ["nginx:1.26"]
persisted:     ["nginx:1.26"]

postgres:16 is gone from memory and from module_kv, restored only by the next sweep — up to CHECK_INTERVAL_MS away, or never on exactly the host whose sweeps keep failing that item 3 is written for.

The comment two lines up ("state.stacks is the full set even on a scoped refresh (mergeScopedStacks merges into it), so pruning here is safe") is only true once a full poll has run. This PR moves the prune into the window where it hasn't, and keeps the sentence.

How wide is the window? At the default INTERVAL_MS it is one microtask plus one listStacks, so small. Above 5 min it is the case this PR's own new comment documents: the schedule row already exists, firstRun: "immediate" does not fire, and the window is up to a whole interval — long enough for an operator to click something.

To be fair to the diff: the wipe itself predates it (the old code pruned on the same scoped call, and refreshUpdates(project) persists the result either way). What is new is that this is now the module's only prune and it is aimed squarely at the boot window.

Fix: if (seededFromStorage && !project). A scoped call then leaves the flag set for the first full poll — the listing the prune was actually reasoned about.

**Blocking: this guard is missing `&& !project`.** `refreshStacks(project)` is reachable before the first full poll. Every `composeAction` re-reads its own project in a `finally` (`actions.ts:133`), including when the run threw, and `stack-lifecycle` calls it directly (`actions.ts:181`). At that point `state.stacks` is `[]`, `mergeScopedStacks` returns only the refreshed project (`merge.ts:20` filters `listed` by `project`), and the one prune this PR still allows runs against that partial set. Reproduced against this head, with no tick fired — i.e. the window before the first `stacks` poll: ``` seeded storage: nginx:1.26 (project web) + postgres:16 (project db) run stack-lifecycle {project: "web", action: "stop"} // throws unknown-stack; the finally still refreshes updates after: ["nginx:1.26"] persisted: ["nginx:1.26"] ``` `postgres:16` is gone from memory *and* from `module_kv`, restored only by the next sweep — up to `CHECK_INTERVAL_MS` away, or never on exactly the host whose sweeps keep failing that item 3 is written for. The comment two lines up ("`state.stacks` is the full set even on a scoped refresh (mergeScopedStacks merges into it), so pruning here is safe") is only true once a full poll has run. This PR moves the prune into the window where it hasn't, and keeps the sentence. How wide is the window? At the default `INTERVAL_MS` it is one microtask plus one `listStacks`, so small. Above 5 min it is the case this PR's own new comment documents: the schedule row already exists, `firstRun: "immediate"` does not fire, and the window is up to a whole interval — long enough for an operator to click something. To be fair to the diff: the wipe itself predates it (the old code pruned on the same scoped call, and `refreshUpdates(project)` persists the result either way). What is new is that this is now the module's *only* prune and it is aimed squarely at the boot window. Fix: `if (seededFromStorage && !project)`. A scoped call then leaves the flag set for the first full poll — the listing the prune was actually reasoned about.

packages/modules/docker/backend/mod.ts

					
				@ -286,0 +321,4 @@

				    // A pruned ghost must not outlive this boot in storage: only sweeps

				    // persist otherwise, so on a host whose sweeps keep failing the row

				    // would re-seed the same dead verdicts on every restart, forever.

				    if (prunedGhosts) await persistUpdates();

Related, non-blocking: this line is what makes a bad boot prune durable.

The prune trusts whatever the first listing returns, and it can return less than the truth in two ways: the scoped call above, and an engine that answers the first poll with an empty or partial list (a stack the operator left down across the restart, or a daemon still coming up). Verdicts drop, and this line writes the drop back to module_kv — so the loss survives the very restart the persistence exists for. That is the mirror image of the ghost this PR removes, and it is not covered by a test.

Cheapest guard is on the prune, not here: if (seededFromStorage && !project && state.stacks.length > 0). An empty listing at boot is far more often a hiccup than a host with nothing on it, and a host with genuinely nothing running loses nothing it will miss.

The stricter version, if it is worth it: key the prune off the engine event window rather than absence from the listing — eventsByName is already fetched at the top of refreshStacks and carries the destroy events, which is the signal "this stack is gone" as opposed to "this stack is not listed right now".

Related, non-blocking: this line is what makes a bad boot prune durable. The prune trusts whatever the first listing returns, and it can return less than the truth in two ways: the scoped call above, and an engine that answers the first poll with an empty or partial list (a stack the operator left `down` across the restart, or a daemon still coming up). Verdicts drop, and this line writes the drop back to `module_kv` — so the loss survives the very restart the persistence exists for. That is the mirror image of the ghost this PR removes, and it is not covered by a test. Cheapest guard is on the prune, not here: `if (seededFromStorage && !project && state.stacks.length > 0)`. An empty listing at boot is far more often a hiccup than a host with nothing on it, and a host with genuinely nothing running loses nothing it will miss. The stricter version, if it is worth it: key the prune off the engine event window rather than absence from the listing — `eventsByName` is already fetched at the top of `refreshStacks` and carries the destroy events, which is the signal "this stack is gone" as opposed to "this stack is not listed right now".

julian referenced this pull request from a commit

2026-08-16 22:29:59 +02:00

fix(docker): never prune or sweep verdicts against a partial listing

julian added 1 commit

2026-08-16 22:29:59 +02:00

fix(docker): never prune or sweep verdicts against a partial listing

Build and Deploy / verify (pull_request) Successful in 1m34s

Details

Build and Deploy / build (pull_request) Has been skipped

Details

Dependency Check / dependencies (pull_request) Successful in 1m6s

Details

7ab05fae42

The one-shot boot prune was reachable from a scoped refresh: every
composeAction re-reads its own project in a `finally`, and before the
first full poll `state.stacks` holds nothing but that project — the
prune then dropped every other project's verdicts and persisted the
loss. The same partial-listing wipe had two more paths the requested
one-liner would not have closed: the scoped `refreshUpdates(project)`
right behind it prunes the merged map against `state.stacks` and always
persists, and the sweep tick guarded on `!state.updatedAt`, which a
scoped refresh sets, so a sweep could rebuild the whole verdict map
from a one-project listing.

One flag closes all three: `haveFullListing`, set by a full
refreshStacks. The boot prune now requires a full, non-empty listing
(`seededFromStorage && !project && state.stacks.length > 0` — an empty
first listing is an engine still coming up far more often than a bare
host, and skipping costs nothing since a full sweep rebuilds the map
anyway). A scoped check merges unpruned until a full listing has
landed, and leaves the one-shot boot prune armed for the first full
poll. The sweep tick keys off the flag instead of `updatedAt`.

The prune comment now states the accepted trade in both directions
(#43): boot-only pruning narrows the down-across-restart hole to one
shot, it does not remove the class; and a stack removed out of band
while OpsDeck runs keeps its phantom verdict for up to
CHECK_INTERVAL_MS, healed sooner only by the action path.

Three new tests pin the paths (mod_test.ts now captures the defined
actions so the real composeAction `finally` is exercised): a scoped
action refresh before the first poll neither wipes nor consumes the
boot prune, a sweep firing after only a scoped refresh polls the full
host first, and an empty first listing defers the prune. Also fixes
the test-4 comment (the poll did list webContainer; the tick condition
was what made the full poll run first).

Closes the changes requested on #44.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

julian commented

2026-08-16 22:30:17 +02:00

Addressed in 7ab05fa.

The blocking guard — taken, and it turned out not to be enough on its own. Re-running your repro with only seededFromStorage && !project in place still wipes postgres:16 from memory and module_kv: the same finally that runs the scoped refreshStacks follows it with refreshUpdates("web"), which prunes the merged map against the same partial state.stacks and persists unconditionally. And the sweep tick guarded on !state.updatedAt, which the scoped refresh sets — so a sweep firing before any full poll would rebuild the verdict map from a one-project listing (the checked map is built from state.stacks too) and wipe through a third door.

All three paths are the same defect — treating state.stacks as the whole host before a full listing has landed — so the fix is one flag, haveFullListing, set by a full refreshStacks:

the boot prune requires seededFromStorage && !project && state.stacks.length > 0 (your suggested guard, empty-listing clause included);
a scoped check merges unpruned until a full listing has landed, and only a prune that saw a full listing clears seededFromStorage — the one shot stays armed for the listing it was reasoned about;
the sweep tick keys off !haveFullListing instead of !state.updatedAt.

The description overclaim and the missing other-direction trade — the prune comment now says both plainly: boot-only pruning narrows the down-across-restart hole to one shot rather than removing the class (and the loss is persisted until the next sweep), and a stack removed out of band while OpsDeck runs keeps its phantom verdict for up to CHECK_INTERVAL_MS, healed sooner only by the action path.

The test-4 comment nit — fixed; it now describes the actual condition.

Tests. ctxFor now captures ctx.tasks.defined actions, so the new tests drive the real composeAction finally (the thrown unknown-stack run included) rather than calling the refresh helpers directly. Three new tests: a scoped action refresh before the first poll neither wipes another project's verdicts (memory or storage) nor consumes the boot prune; a sweep firing after only a scoped refresh polls the full host first; an empty first listing defers the prune to the next non-empty full poll. Mutation check on this head: dropping !project/the length guard, always-pruning in refreshUpdates, reverting the tick guard to !state.updatedAt, and clearing seededFromStorage unconditionally each fail at least one test.

deno fmt --check, deno lint, deno task check clean; server + docker suites 325 passed (322 + the 3 new), same 6 pre-existing Windows-path failures (#20). Reviewed by cavecrew-reviewer: no findings.

🤖 Generated with Claude Code

Addressed in `7ab05fa`. **The blocking guard — taken, and it turned out not to be enough on its own.** Re-running your repro with only `seededFromStorage && !project` in place still wipes `postgres:16` from memory and `module_kv`: the same `finally` that runs the scoped `refreshStacks` follows it with `refreshUpdates("web")`, which prunes the merged map against the same partial `state.stacks` and persists unconditionally. And the sweep tick guarded on `!state.updatedAt`, which the scoped refresh sets — so a sweep firing before any full poll would rebuild the verdict map from a one-project listing (the `checked` map is built from `state.stacks` too) and wipe through a third door. All three paths are the same defect — treating `state.stacks` as the whole host before a full listing has landed — so the fix is one flag, `haveFullListing`, set by a full `refreshStacks`: - the boot prune requires `seededFromStorage && !project && state.stacks.length > 0` (your suggested guard, empty-listing clause included); - a scoped check merges unpruned until a full listing has landed, and only a prune that saw a full listing clears `seededFromStorage` — the one shot stays armed for the listing it was reasoned about; - the sweep tick keys off `!haveFullListing` instead of `!state.updatedAt`. **The description overclaim and the missing other-direction trade** — the prune comment now says both plainly: boot-only pruning *narrows* the down-across-restart hole to one shot rather than removing the class (and the loss is persisted until the next sweep), and a stack removed out of band while OpsDeck runs keeps its phantom verdict for up to `CHECK_INTERVAL_MS`, healed sooner only by the action path. **The test-4 comment nit** — fixed; it now describes the actual condition. **Tests.** `ctxFor` now captures `ctx.tasks.define`d actions, so the new tests drive the real `composeAction` `finally` (the thrown `unknown-stack` run included) rather than calling the refresh helpers directly. Three new tests: a scoped action refresh before the first poll neither wipes another project's verdicts (memory or storage) nor consumes the boot prune; a sweep firing after only a scoped refresh polls the full host first; an empty first listing defers the prune to the next non-empty full poll. Mutation check on this head: dropping `!project`/the length guard, always-pruning in `refreshUpdates`, reverting the tick guard to `!state.updatedAt`, and clearing `seededFromStorage` unconditionally each fail at least one test. `deno fmt --check`, `deno lint`, `deno task check` clean; server + docker suites 325 passed (322 + the 3 new), same 6 pre-existing Windows-path failures (#20). Reviewed by cavecrew-reviewer: no findings. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

julian requested review from thisilike

2026-08-16 22:30:19 +02:00

thisilike requested changes

2026-08-21 20:35:03 +02:00

Dismissed

thisilike left a comment

Re-reviewed at 7ab05fa in a detached worktree, mutation-testing every claim rather than reading the follow-up comment.

Verified independently

The haveFullListing work is right, and the follow-up comment's mutation list checks out. Each mutation, and the test that actually kills it:

mutation	test that fails
tick guard back to `!state.updatedAt`	a sweep firing after only a scoped refresh still polls the full host first
always prune in `refreshUpdates`	a scoped action refresh before the first poll neither wipes nor consumes the boot prune
clear `seededFromStorage` unconditionally	same
drop `!project` from the boot guard	same
drop `state.stacks.length > 0`	an empty first listing defers the boot prune instead of wiping
drop `if (prunedGhosts) await persistUpdates()`	a poll that prunes a ghost also persists…
`TextEncoder().encode(json).length` → `json.length`	storage: the size cap is measured in UTF-8 bytes

Seven mutations, seven kills — one short of the set, see below.

The scoped tests' premise holds even with a filter-blind mock. I went looking for a false pass here: engineFetch ignores the compose-project filter, so refreshStacks("web") receives both containers. It still produces a genuinely partial state.stacks, because mergeScopedStacks filters listed by project itself (merge.ts:20). Confirmed separately with a filter-aware mock — same result.
Item 6 is correct. (module, key, value, updated_at_ms) matches migration 10 (metrics/schema.ts:206-212) in order, and module_kv has no other insert site (storage.ts + the updated fixture).
Item 4's comment is accurate to the code. intervalMs <= INTERVAL_AS_SCHEDULE_MS = 5 min inclusive (host.ts:409, 171), plain timers get their immediate tick from queueMicrotask(tick) (scheduler.ts:40), and firstRun: "immediate" only fires inside if (!existing) (tasks/service.ts:1334-1337) with the declaration at catchUp: "skip" — so after a restart the >5 min case really does wait a full interval.
The trade-offs are now stated in the comment, both directions — the persisted down-across-restart loss and the up-to-CHECK_INTERVAL_MS phantom for an out-of-band removal. That was my last review's second point; taken.
deno fmt --check, deno lint, deno task check clean. Full suite 475 passed, 0 failed, and deterministic: docker + storage tests identical across 8 consecutive runs. (One earlier full run reported a single failure I could not reproduce in four subsequent runs and could not pin to a test; not attributable to this branch.)

What I don't buy

1. Blocking — the single mutation that survives is the one this PR is named for. Remove seededFromStorage && from the boot-prune guard, so it prunes on every full poll — the exact behaviour issue #43 item 2 objected to and this PR's headline change — and the suite is still fully green: 475 passed | 0 failed. The test the file's own header nominates as the pin ("ONLY the first … (third test)") empties the entire container listing, so it's state.stacks.length > 0 that blocks the prune, not the boot-only flag. Note what is missing from the mutation list in your comment: dropping !project, dropping the length clause, always-pruning, clearing the flag unconditionally — but not dropping the flag from the guard itself. Two tokens fix the test; exact patch and both-directions verification inline at mod_test.ts:257.

That matters more than usual here because items 1 and 5 of the issue are explicitly "the check can be silently reintroduced" findings. Shipping the fix for those alongside an unpinned headline guard is the same failure one level up.

2. haveFullListing is set by an empty full listing, which reopens the wipe. The prune comment argues an empty listing is untrustworthy; forty lines earlier the flag trusts it anyway, and the flag is what gates the refreshUpdates prune. Empty first poll → next scoped action refresh prunes against a one-project state.stacks and persists it; I reproduced postgres:16 disappearing from both memory and the module_kv write on this head. Narrow — it needs a listing that can actually be empty, i.e. OpsDeck as a bare process rather than a container — but it is the same defect class through the one door left open, and it is one clause. Repro and patch inline at mod.ts:252.

Nit, no action: the description still says "mod_test.ts grew from 1 to 4 tests"; it's 7. The follow-up comment covers it.

The mechanism is right and the guards are well argued — this is one test line and one predicate away from an approve.

Re-reviewed at `7ab05fa` in a detached worktree, mutation-testing every claim rather than reading the follow-up comment. ## Verified independently **The `haveFullListing` work is right, and the follow-up comment's mutation list checks out.** Each mutation, and the test that actually kills it: | mutation | test that fails | | --- | --- | | tick guard back to `!state.updatedAt` | *a sweep firing after only a scoped refresh still polls the full host first* | | always prune in `refreshUpdates` | *a scoped action refresh before the first poll neither wipes nor consumes the boot prune* | | clear `seededFromStorage` unconditionally | same | | drop `!project` from the boot guard | same | | drop `state.stacks.length > 0` | *an empty first listing defers the boot prune instead of wiping* | | drop `if (prunedGhosts) await persistUpdates()` | *a poll that prunes a ghost also persists…* | | `TextEncoder().encode(json).length` → `json.length` | *storage: the size cap is measured in UTF-8 bytes* | Seven mutations, seven kills — one short of the set, see below. - **The scoped tests' premise holds even with a filter-blind mock.** I went looking for a false pass here: `engineFetch` ignores the compose-project filter, so `refreshStacks("web")` receives both containers. It still produces a genuinely partial `state.stacks`, because `mergeScopedStacks` filters `listed` by project itself (`merge.ts:20`). Confirmed separately with a filter-aware mock — same result. - **Item 6 is correct.** `(module, key, value, updated_at_ms)` matches migration 10 (`metrics/schema.ts:206-212`) in order, and `module_kv` has no other insert site (`storage.ts` + the updated fixture). - **Item 4's comment is accurate to the code.** `intervalMs <= INTERVAL_AS_SCHEDULE_MS` = 5 min inclusive (`host.ts:409`, `171`), plain timers get their immediate tick from `queueMicrotask(tick)` (`scheduler.ts:40`), and `firstRun: "immediate"` only fires inside `if (!existing)` (`tasks/service.ts:1334-1337`) with the declaration at `catchUp: "skip"` — so after a restart the >5 min case really does wait a full interval. - **The trade-offs are now stated in the comment, both directions** — the persisted down-across-restart loss and the up-to-`CHECK_INTERVAL_MS` phantom for an out-of-band removal. That was my last review's second point; taken. - `deno fmt --check`, `deno lint`, `deno task check` clean. Full suite **475 passed, 0 failed**, and deterministic: docker + storage tests identical across 8 consecutive runs. (One earlier full run reported a single failure I could not reproduce in four subsequent runs and could not pin to a test; not attributable to this branch.) ## What I don't buy **1. Blocking — the single mutation that survives is the one this PR is named for.** Remove `seededFromStorage &&` from the boot-prune guard, so it prunes on every full poll — the exact behaviour issue #43 item 2 objected to and this PR's headline change — and the suite is still fully green: `475 passed | 0 failed`. The test the file's own header nominates as the pin ("ONLY the first … (third test)") empties the *entire* container listing, so it's `state.stacks.length > 0` that blocks the prune, not the boot-only flag. Note what is missing from the mutation list in your comment: dropping `!project`, dropping the length clause, always-pruning, clearing the flag unconditionally — but not dropping the flag from the guard itself. Two tokens fix the test; exact patch and both-directions verification inline at `mod_test.ts:257`. That matters more than usual here because items 1 and 5 of the issue are explicitly *"the check can be silently reintroduced"* findings. Shipping the fix for those alongside an unpinned headline guard is the same failure one level up. **2. `haveFullListing` is set by an empty full listing, which reopens the wipe.** The prune comment argues an empty listing is untrustworthy; forty lines earlier the flag trusts it anyway, and the flag is what gates the `refreshUpdates` prune. Empty first poll → next scoped action refresh prunes against a one-project `state.stacks` and persists it; I reproduced `postgres:16` disappearing from both memory and the `module_kv` write on this head. Narrow — it needs a listing that can actually be empty, i.e. OpsDeck as a bare process rather than a container — but it is the same defect class through the one door left open, and it is one clause. Repro and patch inline at `mod.ts:252`. Nit, no action: the description still says *"`mod_test.ts` grew from 1 to 4 tests"*; it's 7. The follow-up comment covers it. The mechanism is right and the guards are well argued — this is one test line and one predicate away from an approve.

packages/modules/docker/backend/mod.ts Outdated

					
				@ -218,6 +249,7 @@ export default async function register(ctx: ModuleContext): Promise<void> {

				      ? mergeScopedStacks(state.stacks, listed, project)

				      : listed;

				    state.updatedAt = Date.now();

				    if (!project) haveFullListing = true;

haveFullListing is set by any full listing, empty ones included — and 47
lines below, the prune comment argues an empty listing must not be trusted
("far more often an engine still coming up"). Both cannot be right, and the
flag is the one that unlocks the wipe: after an empty first poll, the next
scoped action refresh prunes the merged map against a one-project
state.stacks at mod.ts:391 and persists it.

Reproduced on this head (probe test, filter-aware engine mock so the scoped
listing really is scoped; storage seeded with nginx:1.26 + postgres:16):

1. stacks tick, engine answers []      -> haveFullListing = true,
                                          boot prune correctly skipped
2. host catches up (web + db running)
3. stack-lifecycle{project:"web"} -> finally: refreshStacks("web")
                                            + refreshUpdates("web")

PROBE2 memory wiped: true persisted: true
PROBE2 persisted map keys: [ "nginx:1.26" ]

postgres:16 is gone from state.updates and from what went into
module_kv — the same defect haveFullListing was added to close, reached
through the door the code itself calls untrustworthy.

Reachability is narrow and I'll say so: in the containerised deployment the
listing can never be empty (self is in it), so this needs OpsDeck running as
a bare process against the socket — the documented local-dev shape — on a
host whose stacks are not up yet. The fix is one clause:

-    if (!project) haveFullListing = true;
+    if (!project && listed.length > 0) haveFullListing = true;

Same predicate the prune already uses, so the two stop disagreeing. The cost
is a sweep tick re-polling on a genuinely empty host, which is free. If you'd
rather keep the flag as-is, then the honest alternative is to drop the
state.stacks.length > 0 clause and its test as well — but not one without
the other, and I'd take the guard.

`haveFullListing` is set by *any* full listing, empty ones included — and 47 lines below, the prune comment argues an empty listing must not be trusted ("far more often an engine still coming up"). Both cannot be right, and the flag is the one that unlocks the wipe: after an empty first poll, the next scoped action refresh prunes the merged map against a one-project `state.stacks` at `mod.ts:391` and persists it. Reproduced on this head (probe test, filter-aware engine mock so the scoped listing really is scoped; storage seeded with `nginx:1.26` + `postgres:16`): ``` 1. stacks tick, engine answers [] -> haveFullListing = true, boot prune correctly skipped 2. host catches up (web + db running) 3. stack-lifecycle{project:"web"} -> finally: refreshStacks("web") + refreshUpdates("web") PROBE2 memory wiped: true persisted: true PROBE2 persisted map keys: [ "nginx:1.26" ] ``` `postgres:16` is gone from `state.updates` **and** from what went into `module_kv` — the same defect `haveFullListing` was added to close, reached through the door the code itself calls untrustworthy. Reachability is narrow and I'll say so: in the containerised deployment the listing can never be empty (self is in it), so this needs OpsDeck running as a bare process against the socket — the documented local-dev shape — on a host whose stacks are not up yet. The fix is one clause: ```diff - if (!project) haveFullListing = true; + if (!project && listed.length > 0) haveFullListing = true; ``` Same predicate the prune already uses, so the two stop disagreeing. The cost is a sweep tick re-polling on a genuinely empty host, which is free. If you'd rather keep the flag as-is, then the honest alternative is to drop the `state.stacks.length > 0` clause and its test as well — but not one without the other, and I'd take the guard.

packages/modules/docker/backend/mod_test.ts Outdated

					
				@ -163,0 +254,4 @@

				    );

				    await register(ctx);

				    await ticks.get("stacks")!(); // boot prune: nginx runs, verdict kept

				    containers = []; // the down/up window

Blocking: this test does not pin the thing the PR is named for.

The file header names this test as the pin for boot-only pruning ("ONLY the
first ... (third test)"). It isn't. Delete seededFromStorage && from the
guard at mod.ts:299 — i.e. prune on every full poll, exactly the
behaviour item 2 exists to prevent — and the entire suite stays green:

$ deno task test   # with `if (!project && state.stacks.length > 0)`
ok | 475 passed | 0 failed

Because the window here empties the whole listing, state.stacks.length > 0
is what blocks the prune, not seededFromStorage. And an empty full listing
is not what a compose down looks like: listStacks uses all=true with no
self-filter (engine.ts:66), so OpsDeck's own container is always in the
list — that is why /stacks computes selfStack from SELF_ID at all. The
realistic shape is one project missing.

Two tokens:

-    let containers: unknown[] = [webContainer];
+    let containers: unknown[] = [webContainer, dbContainer];
     ...
-    containers = []; // the down/up window
+    containers = [dbContainer]; // web mid `compose down`

Verified both directions on this head: passes as written on 7ab05fa, and
with seededFromStorage && removed it fails with
a poll during compose down dropped a live verdict. dbContainer and its
local digest are already in the fixtures, so nothing else moves.

**Blocking: this test does not pin the thing the PR is named for.** The file header names this test as the pin for boot-only pruning ("ONLY the first ... (third test)"). It isn't. Delete `seededFromStorage &&` from the guard at `mod.ts:299` — i.e. prune on *every* full poll, exactly the behaviour item 2 exists to prevent — and the entire suite stays green: ``` $ deno task test # with `if (!project && state.stacks.length > 0)` ok | 475 passed | 0 failed ``` Because the window here empties the whole listing, `state.stacks.length > 0` is what blocks the prune, not `seededFromStorage`. And an empty full listing is not what a `compose down` looks like: `listStacks` uses `all=true` with no self-filter (`engine.ts:66`), so OpsDeck's own container is always in the list — that is why `/stacks` computes `selfStack` from `SELF_ID` at all. The realistic shape is *one* project missing. Two tokens: ```diff - let containers: unknown[] = [webContainer]; + let containers: unknown[] = [webContainer, dbContainer]; ... - containers = []; // the down/up window + containers = [dbContainer]; // web mid `compose down` ``` Verified both directions on this head: passes as written on `7ab05fa`, and with `seededFromStorage &&` removed it fails with `a poll during compose down dropped a live verdict`. `dbContainer` and its local digest are already in the fixtures, so nothing else moves.

julian referenced this pull request from a commit

2026-08-21 21:54:12 +02:00

fix(docker): an empty listing is not a full one, and the test now pins the flag

julian added 1 commit

2026-08-21 21:54:12 +02:00

fix(docker): an empty listing is not a full one, and the test now pins the flag

Build and Deploy / verify (pull_request) Successful in 2m21s

Details

Build and Deploy / build (pull_request) Has been skipped

Details

Dependency Check / dependencies (pull_request) Successful in 35s

Details

7d260daeb2

Review round 2 on #44, both findings taken as prescribed:

- `haveFullListing` now requires a non-empty full listing — the same
  predicate the boot prune uses, so the flag and the prune stop
  disagreeing about whether an empty listing can be trusted. Before, a
  first poll answered by an engine still coming up set the flag, and the
  next scoped action refresh pruned the merged verdict map against a
  one-project `state.stacks` and persisted the wipe. Reachable only when
  OpsDeck runs as a bare process (in the container its own entry is
  always listed); new test pins it either way.

- The compose-down test keeps the other project in the listing, so the
  window is one project missing — what a `compose down` actually looks
  like — and only `seededFromStorage` stands between the second poll and
  the prune. Removing the flag from the guard now fails the test instead
  of surviving the suite.

All eight mod-side mutations from both review rounds re-run and killed.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

julian commented

2026-08-21 21:55:01 +02:00

Both findings taken as prescribed, in 7d260da.

1. The compose-down test now pins the flag, not the length clause. Your two-token patch, verbatim: the fixture starts at [webContainer, dbContainer] and the window is containers = [dbContainer] — one project missing, which per your all=true/SELF_ID point is what a compose down actually looks like. The second poll's listing is full and non-empty, so only seededFromStorage stands between it and the prune. Re-ran your surviving mutation: with seededFromStorage && removed from the guard, the test fails with a poll during compose down dropped a live verdict; restored, it passes. The test's comment now states why the listing must stay non-empty.

2. haveFullListing now requires a non-empty listing. Your clause, verbatim: if (!project && listed.length > 0) haveFullListing = true; — the same predicate the boot prune uses, so the two stop disagreeing. Took the guard over dropping the length clause, as you would. Your PROBE2 repro is now a regression test, "an empty full listing does not unlock the scoped-refresh prune": storage seeded with nginx:1.26 + postgres:16, empty first full poll, host catches up, stack-lifecycle{project:"web"} fires its finally refresh pair — asserts postgres:16 survives in memory and in the module_kv write. Reverting the clause fails it. The comment at the flag's set site names the bare-process reachability, mirroring your note.

Mutation matrix re-run against this head — all six mod-side mutations from your table plus the two new ones, each killed:

mutation	test that fails
drop `seededFromStorage &&` from the boot guard (your survivor)	a live verdict survives a poll that lands inside compose down/up
revert `listed.length > 0` on the flag	an empty full listing does not unlock the scoped-refresh prune
tick guard back to `!state.updatedAt`	a sweep firing after only a scoped refresh still polls the full host first
always prune in `refreshUpdates`	scoped action refresh… + empty full listing…
clear `seededFromStorage` unconditionally	a scoped action refresh before the first poll neither wipes nor consumes the boot prune
drop `!project` from the boot guard	scoped action refresh… + empty full listing…
drop `state.stacks.length > 0`	an empty first listing defers the boot prune instead of wiping + empty full listing…
disable `if (prunedGhosts) await persistUpdates()`	a poll that prunes a ghost also persists…

(The ninth from your table, encode(json).length → json.length in storage.ts, is untouched by this diff — file and test unchanged since 7ab05fa.)

Nit taken: the description now says 8 tests, and its "gone by construction" claim is replaced with the narrowed-to-one-shot statement, both directions.

deno fmt --check, deno lint, deno task check clean; server + module suites green except the 6 pre-existing Windows-path failures (#20). cavecrew-reviewer on the delta: no findings.

🤖 Generated with Claude Code

Both findings taken as prescribed, in `7d260da`. **1. The compose-down test now pins the flag, not the length clause.** Your two-token patch, verbatim: the fixture starts at `[webContainer, dbContainer]` and the window is `containers = [dbContainer]` — one project missing, which per your `all=true`/`SELF_ID` point is what a `compose down` actually looks like. The second poll's listing is full and non-empty, so only `seededFromStorage` stands between it and the prune. Re-ran your surviving mutation: with `seededFromStorage &&` removed from the guard, the test fails with `a poll during compose down dropped a live verdict`; restored, it passes. The test's comment now states why the listing must stay non-empty. **2. `haveFullListing` now requires a non-empty listing.** Your clause, verbatim: `if (!project && listed.length > 0) haveFullListing = true;` — the same predicate the boot prune uses, so the two stop disagreeing. Took the guard over dropping the length clause, as you would. Your PROBE2 repro is now a regression test, *"an empty full listing does not unlock the scoped-refresh prune"*: storage seeded with `nginx:1.26` + `postgres:16`, empty first full poll, host catches up, `stack-lifecycle{project:"web"}` fires its `finally` refresh pair — asserts `postgres:16` survives in memory **and** in the `module_kv` write. Reverting the clause fails it. The comment at the flag's set site names the bare-process reachability, mirroring your note. **Mutation matrix re-run against this head** — all six mod-side mutations from your table plus the two new ones, each killed: | mutation | test that fails | | --- | --- | | drop `seededFromStorage &&` from the boot guard *(your survivor)* | *a live verdict survives a poll that lands inside compose down/up* | | revert `listed.length > 0` on the flag | *an empty full listing does not unlock the scoped-refresh prune* | | tick guard back to `!state.updatedAt` | *a sweep firing after only a scoped refresh still polls the full host first* | | always prune in `refreshUpdates` | *scoped action refresh…* + *empty full listing…* | | clear `seededFromStorage` unconditionally | *a scoped action refresh before the first poll neither wipes nor consumes the boot prune* | | drop `!project` from the boot guard | *scoped action refresh…* + *empty full listing…* | | drop `state.stacks.length > 0` | *an empty first listing defers the boot prune instead of wiping* + *empty full listing…* | | disable `if (prunedGhosts) await persistUpdates()` | *a poll that prunes a ghost also persists…* | (The ninth from your table, `encode(json).length` → `json.length` in `storage.ts`, is untouched by this diff — file and test unchanged since `7ab05fa`.) Nit taken: the description now says 8 tests, and its *"gone by construction"* claim is replaced with the narrowed-to-one-shot statement, both directions. `deno fmt --check`, `deno lint`, `deno task check` clean; server + module suites green except the 6 pre-existing Windows-path failures (#20). cavecrew-reviewer on the delta: no findings. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

julian requested review from thisilike

2026-08-21 21:55:04 +02:00

thisilike approved these changes

2026-08-21 23:11:12 +02:00

thisilike left a comment

Re-reviewed at 7d260da in a detached worktree. Both findings are fixed as prescribed, and I re-ran the mutation matrix myself rather than taking the one in your comment.

Both findings verified fixed

1. The compose-down test now pins the flag. Second poll's listing is [dbContainer] — full and non-empty — so state.stacks.length > 0 no longer shadows the guard. Confirmed against the mutation that survived last round: with seededFromStorage && removed, a live verdict survives a poll that lands inside compose down/up fails with a poll during compose down dropped a live verdict. Restored, green.

2. haveFullListing now agrees with the prune about empty listings. The clause is in, and the regression test is not vacuous — it asserts savedValue !== null before checking the written map, so it proves the scoped refresh actually ran instead of only proving nothing changed. Reverting the clause kills it.

Mutation matrix, re-run against this head

Ten mutations: your eight, plus two of my own looking for a new survivor.

mutation	test that fails
drop `seededFromStorage &&` from the boot guard (last round's survivor)	a live verdict survives a poll that lands inside compose down/up
revert `listed.length > 0` on the flag	an empty full listing does not unlock the scoped-refresh prune
tick guard back to `!state.updatedAt`	a sweep firing after only a scoped refresh still polls the full host first
always prune in `refreshUpdates`	scoped action refresh… + empty full listing…
clear `seededFromStorage` unconditionally	a scoped action refresh before the first poll neither wipes nor consumes the boot prune
drop `!project` from the boot guard	scoped action refresh… + empty full listing…
drop `state.stacks.length > 0`	an empty first listing defers the boot prune… + empty full listing…
disable `if (prunedGhosts) await persistUpdates()`	a poll that prunes a ghost also persists…
mine: let a scoped non-empty listing set `haveFullListing` (drop `!project`)	three tests
mine: `prunedGhosts = true` — persist on every full poll	survives

The survivor is not a correctness guard, so I'm not asking for a test: it costs a module_kv write per 30 s poll instead of one per boot, on the serialized DuckDB connection behind whatever metrics flush is queued. Noting it only so the matrix isn't read as exhaustive.

encode(json).length → json.length still dies on the UTF-8 cap test; storage.ts and its test are byte-identical to 7ab05fa, as you said.

Checks

deno fmt --check, deno lint, deno task check clean. Suite 476 passed. Determinism: docker + storage tests identical across 13 consecutive runs; the full suite was clean on 20 of 21 runs, with one failure I could not capture or reproduce in 20 further attempts. Same rate and same non-reproducibility I saw on 7ab05fa, so I read it as a pre-existing flake somewhere in the suite rather than anything this branch does — worth a note if it ever bites CI.

Residual, out of scope — not blocking, worth an issue

The flag gates the prune, not the rebuild. An unscoped refreshUpdates still replaces state.updates with checked, which is built from whatever state.stacks holds — so the sweep tick over an empty listing empties the map and persists it, haveFullListing notwithstanding:

register() seeded with nginx:1.26 + postgres:16, engine answers []
await ticks.get("updates")!()   // full poll (empty) + unscoped sweep

PROBE3 memory: []  disk: []

Pre-existing, not a regression: at the merge base db57f4d the same line read pruneUpdates(project ? merged : checked, state.stacks) followed by an unconditional storage.set, so this wiped there too — this PR strictly narrows the class rather than widening it. And the fix isn't mechanical: a host that genuinely emptied must eventually drop its verdicts, so "skip when the listing is empty" is a policy call about how long a phantom may outlive its stack, not an obvious guard. Same reachability caveat as finding 2 (bare-process shape; containerised always lists self). Follow-up issue, alongside #39.

Good work — the guards are consistent now, and every one of them is defended by a test that dies without it.

Re-reviewed at `7d260da` in a detached worktree. Both findings are fixed as prescribed, and I re-ran the mutation matrix myself rather than taking the one in your comment. ## Both findings verified fixed **1. The compose-down test now pins the flag.** Second poll's listing is `[dbContainer]` — full and non-empty — so `state.stacks.length > 0` no longer shadows the guard. Confirmed against the mutation that survived last round: with `seededFromStorage &&` removed, *a live verdict survives a poll that lands inside compose down/up* fails with `a poll during compose down dropped a live verdict`. Restored, green. **2. `haveFullListing` now agrees with the prune about empty listings.** The clause is in, and the regression test is not vacuous — it asserts `savedValue !== null` before checking the written map, so it proves the scoped refresh actually ran instead of only proving nothing changed. Reverting the clause kills it. ## Mutation matrix, re-run against this head Ten mutations: your eight, plus two of my own looking for a new survivor. | mutation | test that fails | | --- | --- | | drop `seededFromStorage &&` from the boot guard *(last round's survivor)* | *a live verdict survives a poll that lands inside compose down/up* | | revert `listed.length > 0` on the flag | *an empty full listing does not unlock the scoped-refresh prune* | | tick guard back to `!state.updatedAt` | *a sweep firing after only a scoped refresh still polls the full host first* | | always prune in `refreshUpdates` | *scoped action refresh…* + *empty full listing…* | | clear `seededFromStorage` unconditionally | *a scoped action refresh before the first poll neither wipes nor consumes the boot prune* | | drop `!project` from the boot guard | *scoped action refresh…* + *empty full listing…* | | drop `state.stacks.length > 0` | *an empty first listing defers the boot prune…* + *empty full listing…* | | disable `if (prunedGhosts) await persistUpdates()` | *a poll that prunes a ghost also persists…* | | **mine:** let a *scoped* non-empty listing set `haveFullListing` (drop `!project`) | three tests | | **mine:** `prunedGhosts = true` — persist on every full poll | **survives** | The survivor is not a correctness guard, so I'm not asking for a test: it costs a `module_kv` write per 30 s poll instead of one per boot, on the serialized DuckDB connection behind whatever metrics flush is queued. Noting it only so the matrix isn't read as exhaustive. `encode(json).length` → `json.length` still dies on the UTF-8 cap test; `storage.ts` and its test are byte-identical to `7ab05fa`, as you said. ## Checks `deno fmt --check`, `deno lint`, `deno task check` clean. Suite **476 passed**. Determinism: docker + storage tests identical across 13 consecutive runs; the full suite was clean on 20 of 21 runs, with one failure I could not capture or reproduce in 20 further attempts. Same rate and same non-reproducibility I saw on `7ab05fa`, so I read it as a pre-existing flake somewhere in the suite rather than anything this branch does — worth a note if it ever bites CI. ## Residual, out of scope — not blocking, worth an issue The flag gates the *prune*, not the *rebuild*. An unscoped `refreshUpdates` still replaces `state.updates` with `checked`, which is built from whatever `state.stacks` holds — so the sweep tick over an empty listing empties the map and persists it, `haveFullListing` notwithstanding: ``` register() seeded with nginx:1.26 + postgres:16, engine answers [] await ticks.get("updates")!() // full poll (empty) + unscoped sweep PROBE3 memory: [] disk: [] ``` Pre-existing, not a regression: at the merge base `db57f4d` the same line read `pruneUpdates(project ? merged : checked, state.stacks)` followed by an unconditional `storage.set`, so this wiped there too — this PR strictly narrows the class rather than widening it. And the fix isn't mechanical: a host that genuinely emptied *must* eventually drop its verdicts, so "skip when the listing is empty" is a policy call about how long a phantom may outlive its stack, not an obvious guard. Same reachability caveat as finding 2 (bare-process shape; containerised always lists self). Follow-up issue, alongside #39. Good work — the guards are consistent now, and every one of them is defended by a test that dies without it.

julian merged commit 1471b35058 into main

2026-08-21 23:20:29 +02:00

julian referenced this pull request from a commit

2026-08-21 23:20:30 +02:00

Merge pull request 'Storage review follow-ups: boot-only prune, prune-then-persist, cap + persistence tests, named columns' (#44) from fix/storage-review-nits into main

julian referenced this pull request

2026-08-21 23:21:29 +02:00

tests: one extra DuckDB handle in the test worker can segfault the suite at teardown (exit 139) #54

julian referenced this pull request

2026-08-21 23:21:37 +02:00

docker updates: an unscoped sweep over an empty listing still empties and persists the verdict map #55

Sign in to join this conversation.