Module-scoped persistent storage (ctx.storage) + docker update state survives restarts #38
thisilike
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/module-storage"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #37.
What
Core:
ctx.storage— module-scoped persistent KV.module_kvtable (migration 10) in the existing DuckDB database; all access goes through the one serializedDbqueue.ModuleKvstore (server/src/modules/storage.ts): JSON values, 8 MB cap measured in UTF-8 bytes, refuses values with no JSON form (a stored"undefined"would read back as silent data loss), corrupt rows read as null.buildContextcloses over the module name — same scoping pattern as metrics/scheduler; a module cannot name another module's keys.ctx.storage.get/set/delete, documented indocs/modules.md.Docker: first consumer. The sweep schedule already survived a container recreate (
task_schedulesrow), but the sweep result lived in module memory — so after a recreate the UI showed no updates until the next window, up toCHECK_INTERVAL_MS(default 6 h) later. Now:refreshUpdatespersists{updates, checkedAt}(packUpdates).register()rehydrates viaunpackUpdates, which validates per entry instead of casting — a stale layout reads as "nothing stored", one malformed row doesn't discard the rest.updateAvailablefrom local digests against the storedremoteDigest, so a pull during downtime corrects itself within one poll. Registry sweep cadence is untouched — no boot-time re-sweep.Tests
14 new tests (
server/tests/storage_test.ts,docker/backend/persist_test.ts): roundtrip, module scoping, reopen-the-database persistence, JSON-form refusal, corrupt-row handling, per-entry unpack validation.deno task checkclean; full server + docker suites: no new failures (the 6 pre-existing Windows-path failures on main are untouched — that's #20).Reviewed by cavecrew-reviewer; its one finding (size cap measured UTF-16 code units, not bytes) is fixed in this diff.
🤖 Generated with Claude Code
Reviewed at
e03ef83in a detached worktree.deno fmt --check,deno lint,deno task checkclean; full suite 466 passed / 0 failed here (the Windows-path failures the description mentions are Windows-only, they don't reproduce on Linux). The core store itself is well-built — 8 MB measured in UTF-8 bytes,JSON.stringifyreturningundefinedrefused rather than written, a corrupt row reading as null, per-entry unpack validation, scoping owned by the host facade instead of promised by the caller. The tests cover the reopen-the-database case, which is the one that actually proves the feature.One blocker, and it is the correctness argument the issue and the PR description both rest on.
Blocking: nothing prunes the rehydrated map until the next full sweep
Issue #37 says correctness is already handled because "
refreshStacksre-derivesupdateAvailable… andpruneUpdatesdrops images that no longer run". Only the first half is true.pruneUpdateshas exactly one call site —mod.ts:316, insiderefreshUpdates, the expensive registry sweep.refreshStacksnever prunes; its re-derivation loop (mod.ts:229-237)continues on any image it can't find locally, which is precisely the ghost case:Before this PR that was unreachable, because
state.updateswas only ever assigned fromrefreshUpdates, which prunes on the way out. Now the map is seeded at boot from storage and nothing prunes it until the next sweep — up toCHECK_INTERVAL_MS, default 6 h.Concrete failure: a stack is removed (or its images retagged) while OpsDeck is down — the recreate window this feature exists to cover. On boot the verdicts for those images come back with
updateAvailable: true, no local digest, and no prune.StacksPage.svelte:459counts the map globally, not per running container:so the header reads
updates checked <6 h ago> — 2 update(s) availablewhile no stack card shows a single pending update, and it stays that way for a whole check interval. That is the same "badge lies about the host" symptom the PR is fixing, in the other direction.Fix is one line on the path that already runs every 30 s — after the re-derivation block in
refreshStacks:state.stacksis the full set even on a scoped refresh (mergeScopedStacksmerges into it), so it is safe there too. Worth a test: rehydrate a map holding an image no stack runs, tickrefreshStacks, assert it is gone.Non-blocking
mod.ts:330-342— the cache write is on the publish path.await ctx.storage.set(...)sits between the metric write andpublishStacksIfChanged(), so a DuckDB queue round-trip (behind whatever metrics flush is in front of it) now delays the live push of the verdicts the sweep just produced. The comment correctly says failing to cache must not fail the run; the same logic says it must not hold up the publish. Publish first, persist after.persist.ts:38-42— validation stops at the required fields.upgrades,upgradeOptions,newerTag,tagsErrorpass through unchecked, andupgradeOptionsis consumed by the pin chooser as{patch: string[], minor: string[], major: string[]}. A row from a layout that shaped those differently is exactly the case the doc comment promises to reject ("a stale layout has to read as nothing stored"), and it currently reaches the frontend. Cheap to check the arrays.storage.ts— no per-module bound and no way to enumerate. The cap is per value; a module can write unbounded keys, and there is nolist/deleteAll, so rows for a module that was removed or renamed live in/dataforever with nothing able to see them.updated_at_msis written and never read, so today it is decoration. Not a reason to hold the PR — and please do not auto-delete on load failure, a module that failed to clone this boot must not lose its state — but a listing capability is the thing that makes this maintainable later.storage_test.ts—rig()never removes its temp dir, anddb.close()is not in afinally. A failing assertion therefore leaks an open DuckDB handle and Deno's sanitizer reports the leak instead of the assertion, which is the harder failure to read.setvalidates an empty key;getanddeletedo not. Minor asymmetry, butget("")silently answering null is a caller bug that stays invisible.ctxscopes by module name (router, metrics, scheduler, SSE topics,exec(),dockerFetch);storagebelongs in that list, and the "runs through the one serialized DuckDB queue" constraint belongs where the metrics pipeline section states it.Everything above except the first item is fine as follow-up. The prune gap should land in this PR — it is the premise the design was accepted on.
@ -66,0 +76,4 @@try {const stored = unpackUpdates(await ctx.storage.get("updates"));if (stored) {state.updates = stored.updates;Blocking. What comes back here is never pruned against what actually runs.
pruneUpdatesis only ever called fromrefreshUpdates(mod.ts:316), the registry sweep — the thing this feature deliberately does not run at boot.refreshStacksre-derives verdicts butcontinues past any image it cannot find locally (mod.ts:231), so an image removed during the downtime keeps its storedupdateAvailable: trueuntil the next sweep, up toCHECK_INTERVAL_MS(6 h).Issue #37 assumed this was already handled ("
pruneUpdatesdrops images that no longer run"); it is not, on the poll path.StacksPage.svelte:459counts the map globally, so the header says "N update(s) available" while no stack shows one.Add
state.updates = pruneUpdates(state.updates, state.stacks)at the end of the re-derivation block inrefreshStacks—state.stacksis the full set even on a scoped refresh.@ -303,1 +339,4 @@});}}publishStacksIfChanged();The persist is awaited before
publishStacksIfChanged(), so the live push of the verdicts now waits on a DuckDB queue round-trip that may be queued behind a metrics flush.The comment above has the right principle — caching for the next boot must not fail the run that produced the verdicts — and it applies to latency too: publish first, then persist.
@ -0,0 +39,4 @@(u.localDigest === null || typeof u.localDigest === "string") &&(u.remoteDigest === null || typeof u.remoteDigest === "string") &&(u.updateAvailable === null || typeof u.updateAvailable === "boolean") &&typeof u.checkedAt === "number";Validation covers the required fields only.
upgrades,upgradeOptions,newerTagandtagsErrorpass through untouched, andupgradeOptionsis read by the pin chooser as{patch: string[], minor: string[], major: string[]}— a row written by a layout that shaped it differently is exactly what the doc comment promises to reject, and it reaches the frontend instead.Checking the three arrays is a few lines and keeps the "a stale layout reads as nothing stored" contract honest.
@ -0,0 +26,4 @@* verdict map or a badge catalog, and the biggest legitimate consumer in* sight (per-image update state for a large host) is well under a megabyte.*/const MAX_VALUE_BYTES = 8 * 1024 * 1024;The cap is per value, so nothing bounds a module's total footprint: unbounded keys are allowed, there is no
list/deleteAll, and rows belonging to a module that was removed or renamed stay in/datawith no surface able to see them.updated_at_msis written and never read, which is the column that would let you do something about it later.Not a blocker, and please don't auto-delete on module load failure — a module that failed to clone this boot must not lose its state. An enumeration capability is what makes this cleanable.
Review addressed in
033d6b0. Point by point:Blocker — prune gap on the poll path: fixed exactly where you pointed.
refreshStacksnow runsstate.updates = pruneUpdates(state.updates, state.stacks)after the re-derivation block (inside the!fakebranch — fake mode never rehydrates). And the test you asked for exists: newdocker/backend/mod_test.tsrunsregister()against a mockedModuleContext, seeds storage with a verdict for an image no stack runs, ticks the captured stacks callback, and asserts the ghost is gone from/stacks— while the surviving verdict is re-derived (updateAvailableflips false when the local digest matches the stored remote), which pins the "pull during downtime corrects itself" half of the design too.Non-blocking, all taken in this PR except the last:
refreshUpdatesnow callspublishStacksIfChanged()before the awaitedctx.storage.set(...)— the live push no longer waits on the DuckDB queue.persist.tsoptional fields:isUpdateInfonow checksnewerTag,newestTag,tagsError,error,upgrades, andupgradeOptionsincluding the three string-array bands the pin chooser indexes. New test case feeds it bands-as-scalars, a non-string in a band, a missing band, and non-string scalars — every mis-shaped row drops, the well-formed one survives.storage_test.tsleak: rewritten around akvTestwrapper — everyDbhandle closes and the temp dir is removed in afinally, so a failing assertion reports itself instead of a sanitizer leak. The reopen test still closes its first handle explicitly (single-writer), double-close is guarded.getanddeletenow refuse""the same waysetdid; one sharedrequireKey, test covers all three.ctx.storageadded to the module-scoping list, and the metrics section now states that storage rides the one DuckDB queue — publish first, persist after.updated_at_ms: deferred as you suggested — filed as #39, including your "never auto-delete on load failure" constraint verbatim so it doesn't get lost.Verified at
033d6b0:deno fmt --check,deno lint,deno task checkclean; docker + server suites 326 passed, the same 6 pre-existing Windows-path failures as main (#20). Reviewed by cavecrew-reviewer before push: no findings.Re-reviewed at
033d6b0in a detached worktree, checking the claims rather than the comment that makes them. The blocker is genuinely fixed — approving.Verification (mine, not the description's)
deno fmt --check,deno lint,deno task check,deno task check:svelte(0 errors) all clean.deno task test: 468 tests, 466 pass. The two failures areexternal_modules_test.ts'sseedRepohittinggpg: signing failed: Operation cancelled— my machine'scommit.gpgsign, not this diff. Still no Windows-path failures on Linux; the "6 pre-existing failures" the description keeps citing are environmental (#20), not a baseline anyone else shares.mod.ts:248→mod_test.ts:153fails.isUpgradeOptions→return true→persist_testfails.requireKey→ no-op →storage_testfails.The blocker is properly fixed
mod.ts:248prunes after the re-derivation loop, inside!fake, before the publish — andpublishStacksIfChangedhashesupdatesas well asstacks(mod.ts:169), so the prune actually reaches connected clients instead of only correcting server memory.mergeScopedStacksdoes keepstate.stackswhole on a scoped refresh, so the placement is safe there as claimed.mod_test.tsdrives the realregister()and the realrefreshStacksagainst a mocked context rather than a re-implementation of the logic, and it pins the other half of the design too — nginx's verdict flips to false off the local digest.persist.tsoptional-field validation, thekvTestfinally,get/deleteempty-key symmetry, publish-before-persist, CLAUDE.md, #39 with the never-auto-delete constraint carried over — all present, all as described.New, none blocking
1. The 8 MB cap has no test at all — including the UTF-8 fix that was the pre-review's one finding. I replaced the entire check with
json.length > MAX_VALUE_BYTES * 1000and all nine storage tests still passed. The measurement bug that got fixed can be reintroduced silently. Three lines close it:assertRejects(() => kv.set("docker", "k", "ä".repeat(5 * 1024 * 1024)))— over the cap in UTF-8 bytes, under it in UTF-16 code units, so it pins the fix and not merely the cap.2. Pruning on every poll trades a phantom for a hole. I asked for it, so I will own the trade-off.
pruneUpdateskeys off the containers the engine currently lists. A poll landing between a manualdocker compose downandup -d(containers absent fromcontainers/json?all=true, a few seconds out of every 30) drops those images' verdicts, and nothing restores them until the next sweep — up to 6 h, the same interval this PR exists to close, in the other direction. On the action path the scopedrefreshUpdates(project)heals it; on an operator's own compose run, nothing does. Narrower window and a less alarming direction than the phantom, so ship it. If you want it airtight later, the boot gap is all this ever needed to cover:let seededFromStorage = stored != null, prune while it is set, clear it on the first successful prune or sweep — the ghost case without a permanent way to lose a live verdict.3. The pruned map is never persisted. Only
refreshUpdateswrites storage, so after a poll prunes a ghost themodule_kvrow still holds it; the next boot re-seeds the ghost and re-prunes it a tick later. Self-correcting, and consistent with "safe to serve stale" — but the persisted set only ever shrinks at sweep time, so a host whose registry sweeps keep failing accumulates dead verdicts in/dataand resurrects them on every restart.4.
mod.ts:71— "the immediate stacks tick below" is not unconditional.ctx.scheduler.everyonly stays a plain timer with an immediatequeueMicrotasktick while the interval is ≤ 5 min (host.ts:409). SetINTERVAL_MSabove that and the stacks poll becomes a real schedule, wherefirstRun: "immediate"fires only if the schedule row does not already exist (service.ts:1334-1337, withcatchUp: "skip") — which after a restart it does. The rehydrated map is then unpruned and un-re-derived for a whole interval.INTERVAL_MSis undocumented and unlikely to be set there, so this is a comment that overstates rather than a bug — but the entire "safe to serve stale" argument rests on that one sentence.5. Nothing tests that a sweep persists anything.
mod_test'sstorage.setis a noop andstorage.getreturns a literal, so the two"updates"string literals atmod.ts:77andmod.ts:344are never checked against each other — a typo in either leaves the feature dead with a green suite. Capture whatsetwas called with, feed it into a secondctxFor, assert the verdicts come back.6.
INSERT INTO module_kv VALUES (?, ?, ?, ?)is positional (storage.ts, and again atstorage_test.ts:106). A migration 11 that adds a column to that table breaks the insert at runtime. Name the four columns.