Follow-ups from PR #38 review: ctx.storage / docker update persistence nits #43

New issue

Closed

opened 2026-08-16 18:21:03 +02:00 by julian · 0 comments

julian commented

2026-08-16 18:21:03 +02:00

Owner

Non-blocking findings from the approving review on #38 (review at 033d6b0). None block correctness today; collected here so they don't get lost.

8 MB cap has no test — including the UTF-8 fix. Replacing the size check with json.length > MAX_VALUE_BYTES * 1000 leaves all nine storage tests green, so the UTF-16→UTF-8 measurement fix can be silently reintroduced. Add: 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.
Per-poll prune trades a phantom for a hole. pruneUpdates on every refreshStacks tick keys off currently listed containers, so a poll landing during a manual docker compose down / up -d window drops those verdicts until the next sweep (up to CHECK_INTERVAL_MS). Scoped refreshUpdates(project) heals the action path; an operator's own compose run has nothing. Airtight variant if wanted: let seededFromStorage = stored != null, prune only while set, clear on first successful prune or sweep — covers the boot ghost without a way to lose a live verdict.
Pruned map is never persisted. Only refreshUpdates writes storage, so a poll-pruned ghost stays in the module_kv row and is re-seeded (then re-pruned) every boot. Self-correcting, but a host whose registry sweeps keep failing accumulates dead verdicts in /data and resurrects them on every restart.
mod.ts:71 comment overstates. "The immediate stacks tick below" is only unconditional while INTERVAL_MS ≤ 5 min — above that, ctx.scheduler.every becomes a real schedule (host.ts:409) where firstRun: "immediate" doesn't fire if the schedule row already exists (service.ts:1334-1337, catchUp: "skip"), leaving the rehydrated map unpruned/un-re-derived for a whole interval. Fix the comment (or the gap).
Nothing tests that a sweep persists anything. In mod_test storage.set is a noop and storage.get returns a literal, so the "updates" key literals at mod.ts:77 and mod.ts:344 are never checked against each other — a typo in either leaves the feature dead with a green suite. Capture what set was called with, feed it into a second ctxFor, assert the verdicts come back.
Positional insert in storage.ts (INSERT INTO module_kv VALUES (?, ?, ?, ?), also storage_test.ts:106). A migration 11 adding a column breaks the insert at runtime. Name the four columns.

Related: #39 (storage listing capability, carries the never-auto-delete-on-load-failure constraint).

Non-blocking findings from the approving review on #38 (review at `033d6b0`). None block correctness today; collected here so they don't get lost. 1. **8 MB cap has no test — including the UTF-8 fix.** Replacing the size check with `json.length > MAX_VALUE_BYTES * 1000` leaves all nine storage tests green, so the UTF-16→UTF-8 measurement fix can be silently reintroduced. Add: `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. **Per-poll prune trades a phantom for a hole.** `pruneUpdates` on every `refreshStacks` tick keys off currently listed containers, so a poll landing during a manual `docker compose down` / `up -d` window drops those verdicts until the next sweep (up to `CHECK_INTERVAL_MS`). Scoped `refreshUpdates(project)` heals the action path; an operator's own compose run has nothing. Airtight variant if wanted: `let seededFromStorage = stored != null`, prune only while set, clear on first successful prune or sweep — covers the boot ghost without a way to lose a live verdict. 3. **Pruned map is never persisted.** Only `refreshUpdates` writes storage, so a poll-pruned ghost stays in the `module_kv` row and is re-seeded (then re-pruned) every boot. Self-correcting, but a host whose registry sweeps keep failing accumulates dead verdicts in `/data` and resurrects them on every restart. 4. **`mod.ts:71` comment overstates.** "The immediate stacks tick below" is only unconditional while `INTERVAL_MS` ≤ 5 min — above that, `ctx.scheduler.every` becomes a real schedule (`host.ts:409`) where `firstRun: "immediate"` doesn't fire if the schedule row already exists (`service.ts:1334-1337`, `catchUp: "skip"`), leaving the rehydrated map unpruned/un-re-derived for a whole interval. Fix the comment (or the gap). 5. **Nothing tests that a sweep persists anything.** In `mod_test` `storage.set` is a noop and `storage.get` returns a literal, so the `"updates"` key literals at `mod.ts:77` and `mod.ts:344` are never checked against each other — a typo in either leaves the feature dead with a green suite. Capture what `set` was called with, feed it into a second `ctxFor`, assert the verdicts come back. 6. **Positional insert in `storage.ts`** (`INSERT INTO module_kv VALUES (?, ?, ?, ?)`, also `storage_test.ts:106`). A migration 11 adding a column breaks the insert at runtime. Name the four columns. Related: #39 (storage listing capability, carries the never-auto-delete-on-load-failure constraint).

julian referenced this issue from a commit

2026-08-16 18:45:49 +02:00

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

julian referenced this issue from a pull request that will close it,

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

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

julian referenced this issue from a commit

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

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

julian closed this issue

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