feat(badges): the catalog and its icons survive a restart #2

Merged
julian merged 4 commits from feat/issue-1-persist-badges into main 2026-08-21 16:54:23 +02:00
Owner

Closes #1. Unblocked by OpsDeck/core#37, which landed as core#38.

What changed

The catalog (91 entries, ~17 KB of protobuf) and every fetched icon were in module memory only. Both now go through ctx.storage:

key holds
badge-catalog the parsed entries plus fetchedAtMs
badge-icon:<guid>:<variant> { url, data }, the bytes base64

BadgeCatalog takes the store as a small structural port rather than importing the SDK type, so the backend keeps its imports type-only and the class stays drivable from a Map. Every persistence step is best effort — a storage failure must not cost the module badges it already holds in memory — and refresh() still throws so a failed fetch is a task row rather than a swallowed warning.

Encoding is chunked: String.fromCharCode(...bytes) on a 512 KB icon spreads half a million arguments and overflows the stack.

Reading back what somebody else wrote

Every stored row is validated, never cast. A row this version cannot use reads as "nothing stored" — a state the code already handled. That covers a value that is not base64, an empty one, one over a cap the module has since lowered, and a guid in the wrong case (GUID_RE is /i, every lookup is lowercased, so an uppercase entry would hydrate, count towards size, and resolve nothing — validCatalog now lowercases).

Two things the row deliberately does not carry:

  • The content type. It is derived from the variant, never taken from the response and never stored. A remote header persisted verbatim is replayed forever: a 200 HTML error page from the CDN would be served same-origin as text/html, and a value carrying a CRLF would make new Response throw inside the route — past the .catch() on icon(), on a resolved Icon nothing evicts, which is a permanent 500 on that badge. Two variants, two known types; the header has nothing to say.
  • A trusted key. badge-icon:<guid>:<variant> does not name the bytes' input, so the row stores the url it fetched and a row whose URL the catalog has moved on from is refetched. Without that, a badge whose art changes serves the old file forever, where before persistence a restart fixed it. The in-memory map is keyed by URL for the same reason — a process outlives several refreshes, and a memory hit would otherwise overrule the disk check.

Boot

register is synchronous again and hydration runs beside it, not in front of it: a ctx.storage read queues behind the metrics flush, and register is what gates the port opening. The cost is that a poll landing first renders that client's badges nameless — one poll, against the most-of-a-day a missing catalog used to mean. A stale disk read cannot clobber a refresh() that won the race.

firstRun: "immediate" is restored, and the eight lines claiming it fired a second run are gone: it does not. A newly declared interval schedule is already due (nextRunMs = now, measured from a last run that does not exist), so "immediate" and "scheduled" both fire exactly one run on the first boot — measured on core origin/main, one run either way, in a fresh data dir. Whatever produced two rows on the first attempt, it was not this flag.

What the flag genuinely cannot cover is a later boot: core ignores it once a schedule row exists, so storage cleared or corrupted under a current window left the module without names or icons for up to 12 h — with "immediate" as much as without it, which is why the previous "known trade-off" traded nothing for nothing. restoreBadges() now closes it: nothing hydrated, a schedule row that already carries a lastRunMs, and that lastRunMs older than this boot (so firstRun's own fire is not mistaken for a missed window) means storage went away under a live window, and the module starts one run itself. A fresh install has no row and is left alone.

Verification

No test framework in this repo, so: a probe driving BadgeCatalog against a Map-backed store with fetch stubbed, and boots of a real host — the module copied into a detached core worktree at origin/main (2844b43), one data dir per scenario.

Probe (31 checks, all pass): refresh writes and a second instance hydrates with zero fetches; a 70 KB icon round-trips byte-identical through storage (so the chunked base64 path is exercised); content types come out image/svg+xml / image/png with the CDN answering text/html; a poisoned contentType row can no longer reach new Response; a moved URL refetches and rewrites the row, in a fresh process and in one that has already served the old art; not-base64, empty, over-cap, no-url and not-an-object rows all refetch without throwing; an uppercase guid resolves a name and an icon; an off-prefix URL is refused before any fetch (the SSRF guard survives hydration); a store that rejects get/set, and no store at all, cost nothing; a slow disk read does not replace a fresher refresh.

Host:

  1. cold start — one badge-catalog run (origin: schedule, {"badges":91}), 19,902 bytes of catalog written, no recovery run.
  2. restartbadge catalog restored {"badges":91,"ageHours":0}, still one run in the table. GET /badge/<guid> 200, content-type: image/svg+xml, 6,880 bytes, and the stored row carries the URL and no contentType.
  3. served from storage — replaced the stored bytes with a marker SVG the content server would never return; the next boot served the marker, so the read never reached the network.
  4. moved URL — rewrote the stored row's url to a _OLD.svg the catalog does not name, keeping the marker bytes. The request refetched the real icon (6,880 bytes) and rewrote the row to the current URL — the marker was not served.
  5. storage cleared under a live window — deleted the module's module_kv rows with the schedule row still current: badge catalog gone from storage — refreshing now, one run with origin: module, badges back in the same boot.
  6. no servers configured — hydration and recovery both no-op, nothing logged as an error. An unknown guid still 404s.

deno check backend/mod.ts in that worktree reports exactly one error, the pre-existing query.ts Timeout/number one.

Unrelated and pre-existing: every badge-catalog run logs run is terminal but its work has not returned from core, on this branch and on the previous head alike.

Version 0.4.10 → 0.5.0.

Closes #1. Unblocked by OpsDeck/core#37, which landed as core#38. ## What changed The catalog (91 entries, ~17 KB of protobuf) and every fetched icon were in module memory only. Both now go through `ctx.storage`: | key | holds | | --- | --- | | `badge-catalog` | the parsed entries plus `fetchedAtMs` | | `badge-icon:<guid>:<variant>` | `{ url, data }`, the bytes base64 | `BadgeCatalog` takes the store as a small structural port rather than importing the SDK type, so the backend keeps its imports type-only and the class stays drivable from a `Map`. Every persistence step is best effort — a storage failure must not cost the module badges it already holds in memory — and `refresh()` still throws so a failed fetch is a task row rather than a swallowed warning. Encoding is chunked: `String.fromCharCode(...bytes)` on a 512 KB icon spreads half a million arguments and overflows the stack. ## Reading back what somebody else wrote Every stored row is validated, never cast. A row this version cannot use reads as "nothing stored" — a state the code already handled. That covers a value that is not base64, an empty one, one over a cap the module has since lowered, and a `guid` in the wrong case (`GUID_RE` is `/i`, every lookup is lowercased, so an uppercase entry would hydrate, count towards `size`, and resolve nothing — `validCatalog` now lowercases). Two things the row deliberately does not carry: - **The content type.** It is derived from the variant, never taken from the response and never stored. A remote header persisted verbatim is replayed forever: a 200 HTML error page from the CDN would be served same-origin as `text/html`, and a value carrying a CRLF would make `new Response` throw inside the route — past the `.catch()` on `icon()`, on a resolved `Icon` nothing evicts, which is a permanent 500 on that badge. Two variants, two known types; the header has nothing to say. - **A trusted key.** `badge-icon:<guid>:<variant>` does not name the bytes' input, so the row stores the `url` it fetched and a row whose URL the catalog has moved on from is refetched. Without that, a badge whose art changes serves the old file forever, where before persistence a restart fixed it. The in-memory map is keyed by URL for the same reason — a process outlives several refreshes, and a memory hit would otherwise overrule the disk check. ## Boot `register` is synchronous again and hydration runs beside it, not in front of it: a `ctx.storage` read queues behind the metrics flush, and `register` is what gates the port opening. The cost is that a poll landing first renders that client's badges nameless — one poll, against the most-of-a-day a missing catalog used to mean. A stale disk read cannot clobber a `refresh()` that won the race. `firstRun: "immediate"` is restored, and the eight lines claiming it fired a second run are gone: it does not. A newly declared interval schedule is already due (`nextRunMs = now`, measured from a last run that does not exist), so `"immediate"` and `"scheduled"` both fire exactly one run on the first boot — measured on core `origin/main`, one run either way, in a fresh data dir. Whatever produced two rows on the first attempt, it was not this flag. What the flag genuinely cannot cover is a later boot: core ignores it once a schedule row exists, so storage cleared or corrupted under a current window left the module without names or icons for up to 12 h — with `"immediate"` as much as without it, which is why the previous "known trade-off" traded nothing for nothing. `restoreBadges()` now closes it: nothing hydrated, a schedule row that already carries a `lastRunMs`, and that `lastRunMs` older than this boot (so `firstRun`'s own fire is not mistaken for a missed window) means storage went away under a live window, and the module starts one run itself. A fresh install has no row and is left alone. ## Verification No test framework in this repo, so: a probe driving `BadgeCatalog` against a `Map`-backed store with `fetch` stubbed, and boots of a real host — the module copied into a detached core worktree at `origin/main` (`2844b43`), one data dir per scenario. Probe (31 checks, all pass): refresh writes and a second instance hydrates with zero fetches; a 70 KB icon round-trips byte-identical through storage (so the chunked base64 path is exercised); content types come out `image/svg+xml` / `image/png` with the CDN answering `text/html`; a poisoned `contentType` row can no longer reach `new Response`; a moved URL refetches and rewrites the row, in a fresh process and in one that has already served the old art; not-base64, empty, over-cap, no-url and not-an-object rows all refetch without throwing; an uppercase guid resolves a name and an icon; an off-prefix URL is refused before any fetch (the SSRF guard survives hydration); a store that rejects `get`/`set`, and no store at all, cost nothing; a slow disk read does not replace a fresher refresh. Host: 1. **cold start** — one `badge-catalog` run (`origin: schedule`, `{"badges":91}`), 19,902 bytes of catalog written, no recovery run. 2. **restart** — `badge catalog restored {"badges":91,"ageHours":0}`, still one run in the table. `GET /badge/<guid>` 200, `content-type: image/svg+xml`, 6,880 bytes, and the stored row carries the URL and no `contentType`. 3. **served from storage** — replaced the stored bytes with a marker SVG the content server would never return; the next boot served the marker, so the read never reached the network. 4. **moved URL** — rewrote the stored row's `url` to a `_OLD.svg` the catalog does not name, keeping the marker bytes. The request refetched the real icon (6,880 bytes) and rewrote the row to the current URL — the marker was not served. 5. **storage cleared under a live window** — deleted the module's `module_kv` rows with the schedule row still current: `badge catalog gone from storage — refreshing now`, one run with `origin: module`, badges back in the same boot. 6. **no servers configured** — hydration and recovery both no-op, nothing logged as an error. An unknown guid still 404s. `deno check backend/mod.ts` in that worktree reports exactly one error, the pre-existing `query.ts` `Timeout`/`number` one. Unrelated and pre-existing: every `badge-catalog` run logs `run is terminal but its work has not returned` from core, on this branch and on the previous head alike. Version 0.4.10 → 0.5.0.
The myTeamSpeak badge catalog (91 entries, ~17 KB of protobuf) and every
fetched icon lived in module memory, so each boot threw them away: the
catalog came back from badges-content.teamspeak.com and every icon variant
was re-downloaded on first use. Between boot and that refresh, every client's
badges rendered nameless.

Core ships `ctx.storage` now, so both are persisted. The catalog goes under
one key with the time it was fetched and is rehydrated before anything can
ask for a badge name; icons go one key per guid and variant, base64 because
that store holds JSON. Encoding is chunked — `String.fromCharCode(...bytes)`
on a 512 KB icon spreads half a million arguments and overflows the stack.

Everything read back is validated rather than cast: what some earlier version
of this module wrote is not necessarily what this one expects, and a row it
cannot use reads as "nothing stored", which is a state this code already
handled. That covers a value that is not base64, one that is empty, and one
over a cap the module has since lowered — all of them mean refetch. The
promise the icon map caches can no longer reject, either: a cached rejection
would replay to every later request for that badge for the life of the
process, and failures here are meant to be retried, not remembered.

`register` is async now, because whether the catalog needs fetching at boot
is a question only the persisted copy can answer, and the host awaits it.

The refresh schedule's `firstRun` drops from "immediate" to "scheduled".
Core already computes `nextRunMs = now` for a newly declared interval
schedule, so "immediate" fired a second run on the first boot — measured:
two runs, two fetches of the same 91 entries. One now. Later boots are
covered by the stored catalog plus the window core carries in the schedule
row: fresh enough to serve, or already due and fired without being asked.

Verified against a real host over four boots: a cold start fetches once and
writes 19,916 bytes of catalog; a restart logs "badge catalog restored" with
91 entries and starts no run; an icon whose stored bytes were replaced with a
marker came back AS the marker, proving the read never reached the network,
while an unstored variant still fetched; and a row corrupted to invalid
base64 refetched and re-stored rather than failing the request.

Closes #1
thisilike left a comment

Re-review of 94de7f1 from a detached worktree at the PR head, checked against core origin/main (2844b43, which contains #38). Everything below was run, not read.

What I could confirm

Drove BadgeCatalog directly against a Map-backed store with fetch stubbed (the structural port pays off here):

  • refresh() writes badge-catalog; a second instance's hydrate() restores it with zero fetches. Icons are written once and served from storage in the next process, byte-identical — with a 70 KB icon, so the chunked base64 path is actually exercised. The chunking is correct.
  • corrupt (!!!not base64!!!), empty, and over-cap rows all refetch, re-store, and never throw.
  • a store that rejects set costs nothing (catalog and icon still served); a store that rejects get reads as "nothing stored".
  • the SSRF guard survives hydration: a stored url outside CONTENT_PREFIX is refused by icon() before any fetch. (validCatalog itself does not check the prefix — the guard is entirely downstream, worth knowing before someone "simplifies" icon().)
  • a host older than core#38 — which sdk: "^1" still admits — has no ctx.storage; the optional constructor argument means that degrades instead of crashing. Good.
  • the deno check claim is honest. Copied into a core origin/main checkout, deno check backend/mod.ts reports exactly one error, the pre-existing query.ts Timeout/number one.

So the persistence mechanism does what the title says. The problems are in the reasoning around it, and two of them are durable-wrong-state bugs.

1. The firstRun section is wrong, and I cannot reproduce the double run

firstRun is consulted only in the if (!existing) branch (tasks/service.ts:1334-1338), and the fire it performs calls updateScheduleFire with nextRunMs = now + everyMs (service.ts:1395-1404) — before start() reaches its first #tickSchedules() (service.ts:328), because #applyPendingSchedules() is awaited (service.ts:297). Modules are loaded before tasks.start() (packages/server/main.ts:151/:160 vs :168), so the declaration is always in the pending batch.

Core's own test asserts this: tasks_test.ts:1290 "an interval schedule survives restarts more frequent than itself" declares every: "6h" with firstRun: "immediate" and asserts runs === 1, "the first boot fires once".

I replicated the badge declaration verbatim (12h, catchUp: "skip", onSelfConflict: drop, a 60 ms "fetch") on core origin/main, both declaration paths:

declaration firstRun fetches boot 1 fetches boot 2
before start() (built-in + external, i.e. production) immediate 1 0
before start() scheduled 1 0
after start() (the external-retry path) immediate 1
svc.define("teamspeak", { id: "badge-catalog", role: "admin", lock: () => "badge-catalog",
  onSelfConflict: { mode: "drop" }, capabilities: { cancel: false, retry: "new-run" },
  run: async (h) => { await h.step("fetch", async () => { fetches++; await tick(60); }); return { badges: 91 }; } });
svc.declareSchedule("teamspeak", { action: "badge-catalog", every: "12h", catchUp: "skip", firstRun });
await svc.start(); await tick(400);

Two consequences:

  • The diff line is a no-op. For an interval schedule, next is already max(lastRunMs + everyMs, now) = now when no row exists, so immediate and scheduled fire the same single run. Fine as churn — but the eight-line comment above it states a mechanism core does not have, in a file where the comments are the load-bearing part. Either restore the flag or replace the comment with what actually happens.
  • The "Known trade-off" is not caused by this change. The up-to-12h wait after storage is cleared exists identically with firstRun: "immediate", because firstRun is ignored whenever a schedule row already exists — which is the exact case described (storage gone, schedule row current). Verified: boot 2 fetches 0 with immediate. So the PR trades nothing for nothing and documents a regression it did not introduce.

What did produce your two successful runs? Both succeeded with {"badges": 91} under onSelfConflict: drop means they did not overlap. If the run ids were 1 and 2 in one fresh data dir, I would like to see their started_at_ms — if they are ~12h apart, that was a normal second window and nothing was ever firing twice.

2. README documents a recovery path that does not exist

To drop them, use the module storage section on OpsDeck's /system page

There is no such section, and no HTTP surface for module_kv at all. On core origin/main, ModuleKv is reachable only through the in-process ctx.storage facade (modules/host.ts:395-402); the only other references to module_kv are storage.ts and the schema. /system's storage block is metrics-DB information — dbPath, dbBytes, rollup watermarks, retention (system.ts:136-146, SystemPage.svelte:226-241) — read-only, not per module, no delete.

Same claim in badges.ts:236 ("an operator having cleared the module's storage from /system").

Today an operator's only options are the Run button (rebuilds the catalog; cannot touch an icon row) or SQL against /data/opsdeck.duckdb. Say that, or add the surface to core first. This matters because it is the escape hatch the trade-off section leans on.

3. A stored contentType is served verbatim, and can be a permanent 500

fetchIcon never checks the content type — it takes whatever the response header says — and now that value is persisted and replayed forever. validIcon only checks typeof i.contentType === "string".

Verified: with a row whose contentType is image/svg+xml\r\nX-Injected: 1, icon() resolves happily and the route's new Response(..., { headers: { "content-type": icon.contentType } }) throws:

TypeError: Invalid header value: "image/svg+xml
X-Injected: 1"

That throw is in the handler, after the new .catch() — which only covers what happens behind icon(), not the response built from what it returned. And the poisoned Icon is cached in this.icons (resolved, not rejected, so nothing deletes it), so it is a 500 on that badge for the life of the process: exactly the failure mode this PR says it eliminated, reintroduced one layer up. The description's "a 404 … whatever went wrong behind it" is overstated.

A CRLF cannot arrive over HTTP, so the realistic sources are a foreign or older row — but the plain-wrong-type case is reachable today: a 200 HTML error page from the CDN is stored as an icon and served same-origin as text/html, permanently.

The fix is one line and the code already contains it: derive the type from the variant (svgimage/svg+xml, png64image/png) instead of trusting and storing a remote header, or validate contentType against /^[\w.+-]+\/[\w.+-]+$/ in validIcon.

4. The icon key omits the URL, and nothing expires

badge-icon:<guid>:<variant> does not cover the input the bytes came from. If a badge's url changes in a later catalog, refresh() replaces the entry, loadIcon still finds the old row under the same key, and the stale art is served forever — where previously a restart fixed it. README's "an icon URL names immutable content" is an assumption about someone else's CDN, and it is the only thing standing between this cache and a permanently wrong answer.

Cheap fix: put url in StoredIcon and refetch when it differs from info.url + VARIANT_SUFFIX[variant]. That also gives you the invalidation hook the change currently lacks entirely.

Non-blocking

  1. The recovery gap has a fix you say does not exist. ctx.schedules.list() (sdk/tasks.ts:721) tells the module whether a schedule row already exists. hydrate() null and a row with lastRunMs set = storage was cleared under a current window — start one run there. A fresh install has no row, so the "double fetch on every fresh install" you rejected does not apply, and onSelfConflict: drop covers the overlap anyway. That closes the up-to-12h blind spot without the flag.

  2. await badges.hydrate() sits in front of Deno.serve. Core's guidance is explicit: a ctx.storage read queues behind whatever the metrics flush is doing, so "never put one on a latency-sensitive path" — and nothing is served until Deno.serve. Worse, the awaited value is used for a log line and nothing else, so the comment on register ("whether a refresh is needed at boot is a question only the persisted copy can answer") describes a decision the code does not make: firstRun is a constant. The real reason to await is that the first poll should see badge names — say that, or make it void badges.hydrate().then(...) and accept one nameless poll.

  3. validCatalog accepts an uppercase GUID that can never resolve. GUID_RE is /i, hydrate() keys the map with the raw stored guid, and name() looks up lowercased. Verified: a stored catalog with an uppercase guid hydrates, reports size: 1, returns "" for the name and null for the icon — and because hydrate() returned non-null, nothing refetches. refresh() cannot produce this (parseBadgeList lowercases), so it needs a foreign writer — but it is the same class the validation exists to prevent: a row this version cannot use reading as usable. guid.toLowerCase() in validCatalog.

  4. Nothing can ever collect retired icon rows. ctx.storage has delete but no enumeration, so an icon whose badge left the catalog is unreachable garbage — not even findable by the module. If it is worth solving, keep the written keys in the catalog row; if not, the README sentence should say the rows are permanent (see #2).

  5. The description says 0.4.9 → 0.5.0; the diff is 0.4.10 → 0.5.0.

Blocking on 1-4. The storage work itself is sound and I would take it as soon as the false rationale is gone and the two durable-wrong-state paths (content type, icon key) are closed.

Re-review of `94de7f1` from a detached worktree at the PR head, checked against core `origin/main` (`2844b43`, which contains #38). Everything below was run, not read. ## What I could confirm Drove `BadgeCatalog` directly against a `Map`-backed store with `fetch` stubbed (the structural port pays off here): - `refresh()` writes `badge-catalog`; a second instance's `hydrate()` restores it with **zero** fetches. Icons are written once and served from storage in the next process, byte-identical — with a 70 KB icon, so the chunked base64 path is actually exercised. The chunking is correct. - corrupt (`!!!not base64!!!`), empty, and over-cap rows all refetch, re-store, and never throw. - a store that rejects `set` costs nothing (catalog and icon still served); a store that rejects `get` reads as "nothing stored". - the SSRF guard survives hydration: a stored `url` outside `CONTENT_PREFIX` is refused by `icon()` before any fetch. (`validCatalog` itself does not check the prefix — the guard is entirely downstream, worth knowing before someone "simplifies" `icon()`.) - a host older than core#38 — which `sdk: "^1"` still admits — has no `ctx.storage`; the optional constructor argument means that degrades instead of crashing. Good. - the `deno check` claim is honest. Copied into a core `origin/main` checkout, `deno check backend/mod.ts` reports exactly one error, the pre-existing `query.ts` `Timeout`/`number` one. So the persistence mechanism does what the title says. The problems are in the reasoning around it, and two of them are durable-wrong-state bugs. ## 1. The `firstRun` section is wrong, and I cannot reproduce the double run `firstRun` is consulted **only** in the `if (!existing)` branch (`tasks/service.ts:1334-1338`), and the fire it performs calls `updateScheduleFire` with `nextRunMs = now + everyMs` (`service.ts:1395-1404`) — before `start()` reaches its first `#tickSchedules()` (`service.ts:328`), because `#applyPendingSchedules()` is awaited (`service.ts:297`). Modules are loaded before `tasks.start()` (`packages/server/main.ts:151`/`:160` vs `:168`), so the declaration is always in the pending batch. Core's own test asserts this: `tasks_test.ts:1290` "an interval schedule survives restarts more frequent than itself" declares `every: "6h"` with `firstRun: "immediate"` and asserts `runs === 1`, "the first boot fires once". I replicated the badge declaration verbatim (12h, `catchUp: "skip"`, `onSelfConflict: drop`, a 60 ms "fetch") on core `origin/main`, both declaration paths: | declaration | firstRun | fetches boot 1 | fetches boot 2 | | --- | --- | --- | --- | | before `start()` (built-in + external, i.e. production) | `immediate` | 1 | 0 | | before `start()` | `scheduled` | 1 | 0 | | after `start()` (the external-retry path) | `immediate` | 1 | — | ```js svc.define("teamspeak", { id: "badge-catalog", role: "admin", lock: () => "badge-catalog", onSelfConflict: { mode: "drop" }, capabilities: { cancel: false, retry: "new-run" }, run: async (h) => { await h.step("fetch", async () => { fetches++; await tick(60); }); return { badges: 91 }; } }); svc.declareSchedule("teamspeak", { action: "badge-catalog", every: "12h", catchUp: "skip", firstRun }); await svc.start(); await tick(400); ``` Two consequences: - **The diff line is a no-op.** For an interval schedule, `next` is already `max(lastRunMs + everyMs, now)` = `now` when no row exists, so `immediate` and `scheduled` fire the same single run. Fine as churn — but the eight-line comment above it states a mechanism core does not have, in a file where the comments are the load-bearing part. Either restore the flag or replace the comment with what actually happens. - **The "Known trade-off" is not caused by this change.** The up-to-12h wait after storage is cleared exists identically with `firstRun: "immediate"`, because `firstRun` is ignored whenever a schedule row already exists — which is the exact case described (storage gone, schedule row current). Verified: boot 2 fetches 0 with `immediate`. So the PR trades nothing for nothing and documents a regression it did not introduce. What did produce your two successful runs? Both `succeeded` with `{"badges": 91}` under `onSelfConflict: drop` means they did not overlap. If the run ids were 1 and 2 in one fresh data dir, I would like to see their `started_at_ms` — if they are ~12h apart, that was a normal second window and nothing was ever firing twice. ## 2. README documents a recovery path that does not exist > To drop them, use the module storage section on OpsDeck's `/system` page There is no such section, and no HTTP surface for `module_kv` at all. On core `origin/main`, `ModuleKv` is reachable only through the in-process `ctx.storage` facade (`modules/host.ts:395-402`); the only other references to `module_kv` are `storage.ts` and the schema. `/system`'s `storage` block is metrics-DB information — `dbPath`, `dbBytes`, rollup watermarks, retention (`system.ts:136-146`, `SystemPage.svelte:226-241`) — read-only, not per module, no delete. Same claim in `badges.ts:236` ("an operator having cleared the module's storage from /system"). Today an operator's only options are the Run button (rebuilds the catalog; cannot touch an icon row) or SQL against `/data/opsdeck.duckdb`. Say that, or add the surface to core first. This matters because it is the escape hatch the trade-off section leans on. ## 3. A stored `contentType` is served verbatim, and can be a permanent 500 `fetchIcon` never checks the content type — it takes whatever the response header says — and now that value is **persisted** and replayed forever. `validIcon` only checks `typeof i.contentType === "string"`. Verified: with a row whose `contentType` is `image/svg+xml\r\nX-Injected: 1`, `icon()` resolves happily and the route's `new Response(..., { headers: { "content-type": icon.contentType } })` throws: ``` TypeError: Invalid header value: "image/svg+xml X-Injected: 1" ``` That throw is in the handler, *after* the new `.catch()` — which only covers what happens behind `icon()`, not the response built from what it returned. And the poisoned `Icon` is cached in `this.icons` (resolved, not rejected, so nothing deletes it), so it is a 500 on that badge for the life of the process: exactly the failure mode this PR says it eliminated, reintroduced one layer up. The description's "a 404 … whatever went wrong behind it" is overstated. A CRLF cannot arrive over HTTP, so the realistic sources are a foreign or older row — but the plain-wrong-type case is reachable today: a 200 HTML error page from the CDN is stored as an icon and served same-origin as `text/html`, permanently. The fix is one line and the code already contains it: derive the type from the variant (`svg` → `image/svg+xml`, `png64` → `image/png`) instead of trusting and storing a remote header, or validate `contentType` against `/^[\w.+-]+\/[\w.+-]+$/` in `validIcon`. ## 4. The icon key omits the URL, and nothing expires `badge-icon:<guid>:<variant>` does not cover the input the bytes came from. If a badge's `url` changes in a later catalog, `refresh()` replaces the entry, `loadIcon` still finds the old row under the same key, and the stale art is served **forever** — where previously a restart fixed it. README's "an icon URL names immutable content" is an assumption about someone else's CDN, and it is the only thing standing between this cache and a permanently wrong answer. Cheap fix: put `url` in `StoredIcon` and refetch when it differs from `info.url + VARIANT_SUFFIX[variant]`. That also gives you the invalidation hook the change currently lacks entirely. ## Non-blocking 5. **The recovery gap has a fix you say does not exist.** `ctx.schedules.list()` (`sdk/tasks.ts:721`) tells the module whether a schedule row already exists. `hydrate()` null **and** a row with `lastRunMs` set = storage was cleared under a current window — start one run there. A fresh install has no row, so the "double fetch on every fresh install" you rejected does not apply, and `onSelfConflict: drop` covers the overlap anyway. That closes the up-to-12h blind spot without the flag. 6. **`await badges.hydrate()` sits in front of `Deno.serve`.** Core's guidance is explicit: a `ctx.storage` read queues behind whatever the metrics flush is doing, so "never put one on a latency-sensitive path" — and nothing is served until `Deno.serve`. Worse, the awaited value is used for a log line and nothing else, so the comment on `register` ("whether a refresh is needed at boot is a question only the persisted copy can answer") describes a decision the code does not make: `firstRun` is a constant. The real reason to await is that the first poll should see badge names — say that, or make it `void badges.hydrate().then(...)` and accept one nameless poll. 7. **`validCatalog` accepts an uppercase GUID that can never resolve.** `GUID_RE` is `/i`, `hydrate()` keys the map with the raw stored guid, and `name()` looks up lowercased. Verified: a stored catalog with an uppercase guid hydrates, reports `size: 1`, returns `""` for the name and `null` for the icon — and because `hydrate()` returned non-null, nothing refetches. `refresh()` cannot produce this (`parseBadgeList` lowercases), so it needs a foreign writer — but it is the same class the validation exists to prevent: a row this version cannot *use* reading as usable. `guid.toLowerCase()` in `validCatalog`. 8. **Nothing can ever collect retired icon rows.** `ctx.storage` has `delete` but no enumeration, so an icon whose badge left the catalog is unreachable garbage — not even findable by the module. If it is worth solving, keep the written keys in the catalog row; if not, the README sentence should say the rows are permanent (see #2). 9. The description says 0.4.9 → 0.5.0; the diff is 0.4.10 → 0.5.0. Blocking on 1-4. The storage work itself is sound and I would take it as soon as the false rationale is gone and the two durable-wrong-state paths (content type, icon key) are closed.
README.md Outdated
@ -142,0 +149,4 @@
Nothing expires these rows — the catalog is a full replacement on each
refresh, and an icon URL names immutable content. To drop them, use the
module storage section on OpsDeck's `/system` page; the next refresh
Owner

This instructs an operation the shipped UI cannot perform: core has no module-storage section on /system and no module_kv endpoint — the storage block there is metrics-DB info (system.ts:136-146, SystemPage.svelte:226-241). Today the only ways out are the Run button (catalog only, cannot drop an icon row) or SQL against /data/opsdeck.duckdb.

Either state that, or land the surface in core first — this is the escape hatch the PR's trade-off section relies on.

This instructs an operation the shipped UI cannot perform: core has no module-storage section on `/system` and no `module_kv` endpoint — the `storage` block there is metrics-DB info (`system.ts:136-146`, `SystemPage.svelte:226-241`). Today the only ways out are the Run button (catalog only, cannot drop an icon row) or SQL against `/data/opsdeck.duckdb`. Either state that, or land the surface in core first — this is the escape hatch the PR's trade-off section relies on.
@ -124,0 +192,4 @@
const badges = c.badges.filter((b): b is BadgeInfo =>
!!b && typeof b === "object" &&
typeof (b as BadgeInfo).guid === "string" &&
GUID_RE.test((b as BadgeInfo).guid) &&
Owner

GUID_RE is case-insensitive, but hydrate() keys the map with the raw stored guid while name() looks up lowercased. A stored catalog with an uppercase guid therefore validates, hydrates, reports size: 1 — and resolves nothing: name() returns "", icon() returns null, and since hydrate() was non-null nothing refetches. Verified.

refresh() can't produce it (parseBadgeList lowercases), so this needs a foreign writer — but it is precisely the case this validation exists to catch: a row this version cannot use reading back as usable. guid.toLowerCase() here.

`GUID_RE` is case-insensitive, but `hydrate()` keys the map with the raw stored guid while `name()` looks up lowercased. A stored catalog with an uppercase guid therefore validates, hydrates, reports `size: 1` — and resolves nothing: `name()` returns `""`, `icon()` returns null, and since `hydrate()` was non-null nothing refetches. Verified. `refresh()` can't produce it (`parseBadgeList` lowercases), so this needs a foreign writer — but it is precisely the case this validation exists to catch: a row this version cannot use reading back as usable. `guid.toLowerCase()` here.
@ -124,0 +206,4 @@
if (typeof i.contentType !== "string" || typeof i.data !== "string") {
return null;
}
return { contentType: i.contentType, data: i.data };
Owner

contentType is validated as "a string" and nothing else, and it comes from an unchecked remote response header — fetchIcon never looks at the type at all. Persisting it makes any junk permanent.

Verified with a row carrying image/svg+xml\r\nX-Injected: 1: icon() resolves fine, then the route's new Response(..., { headers: { "content-type": ... } }) throws TypeError: Invalid header value. That is after the new .catch() on icon(), and the poisoned Icon stays in this.icons (resolved, so nothing evicts it) — a permanent 500 on that badge, the same shape this PR set out to remove. The reachable-today variant is a 200 HTML error page from the CDN being stored and served same-origin as text/html.

Derive the type from the variant (the fallback in fetchIcon already computes it) or regex-validate it here.

`contentType` is validated as "a string" and nothing else, and it comes from an unchecked remote response header — `fetchIcon` never looks at the type at all. Persisting it makes any junk permanent. Verified with a row carrying `image/svg+xml\r\nX-Injected: 1`: `icon()` resolves fine, then the route's `new Response(..., { headers: { "content-type": ... } })` throws `TypeError: Invalid header value`. That is after the new `.catch()` on `icon()`, and the poisoned `Icon` stays in `this.icons` (resolved, so nothing evicts it) — a permanent 500 on that badge, the same shape this PR set out to remove. The reachable-today variant is a 200 HTML error page from the CDN being stored and served same-origin as `text/html`. Derive the type from the variant (the fallback in `fetchIcon` already computes it) or regex-validate it here.
@ -136,0 +233,4 @@
* Load the persisted catalog, if there is one, and say when it was
* fetched. Null means there is nothing usable stored and the caller should
* arrange a refresh a cold start, a layout this version no longer reads,
* or an operator having cleared the module's storage from /system.
Owner

There is no way to clear a module's storage from /system. ModuleKv has no HTTP surface at all on core origin/main — only the in-process facade (modules/host.ts:395-402); /system's storage block is the metrics DB (path, size, rollups, retention). Same claim in the README.

There is no way to clear a module's storage from `/system`. `ModuleKv` has no HTTP surface at all on core `origin/main` — only the in-process facade (`modules/host.ts:395-402`); `/system`'s `storage` block is the metrics DB (path, size, rollups, retention). Same claim in the README.
@ -167,0 +303,4 @@
info: BadgeInfo,
variant: BadgeVariant,
): Promise<Icon | null> {
const key = iconKey(info.guid, variant);
Owner

The key does not cover the input. If a badge's url changes in a later catalog, refresh() replaces the entry but loadIcon still hits this row and serves the old art forever — previously a restart fixed that. "An icon URL names immutable content" (README) is an assumption about someone else's CDN, and nothing here can invalidate.

Put url in StoredIcon and refetch on mismatch.

The key does not cover the input. If a badge's `url` changes in a later catalog, `refresh()` replaces the entry but `loadIcon` still hits this row and serves the old art forever — previously a restart fixed that. "An icon URL names immutable content" (README) is an assumption about someone else's CDN, and nothing here can invalidate. Put `url` in `StoredIcon` and refetch on mismatch.
backend/mod.ts Outdated
@ -565,0 +570,4 @@
// badge name — a boot that starts empty renders every client's badges
// nameless until the next refresh lands, which with a 12 h cadence is most
// of a day
const catalogFetchedAtMs = await badges.hydrate();
Owner

Two things here.

  1. This is a ctx.storage read in front of Deno.serve — core is explicit that a storage read queues behind the metrics flush and must not sit on a latency-sensitive path, and nothing is served until the port opens.
  2. The register comment says the persisted copy answers "whether a refresh is needed at boot". It does not: firstRun is a constant below and catalogFetchedAtMs is used for a log line only. The honest reason to await is that the first poll should see names — either say that, or drop the await (void badges.hydrate().then(...)) and accept one nameless poll.
Two things here. 1. This is a `ctx.storage` read in front of `Deno.serve` — core is explicit that a storage read queues behind the metrics flush and must not sit on a latency-sensitive path, and nothing is served until the port opens. 2. The `register` comment says the persisted copy answers "whether a refresh is needed at boot". It does not: `firstRun` is a constant below and `catalogFetchedAtMs` is used for a log line only. The honest reason to await is that the first poll should see names — either say that, or drop the await (`void badges.hydrate().then(...)`) and accept one nameless poll.
backend/mod.ts Outdated
@ -835,0 +854,4 @@
// change exists to remove. Every later boot is covered by the stored
// catalog plus the window core carries in the schedule row: fresh
// enough to serve, or already due and fired without being asked.
firstRun: "scheduled",
Owner

This comment describes a mechanism core does not have, and the line is a no-op.

firstRun is only read in the if (!existing) branch (tasks/service.ts:1334-1338), and the fire it performs advances nextRunMs to now + everyMs (:1395-1404) before start() reaches its first tick (:328, after the awaited #applyPendingSchedules() at :297). Modules load before tasks.start() (server/main.ts:151/160 vs 168), so the declaration is always in that batch.

Core's own test asserts one fire for exactly this shape: tasks_test.ts:1290, every: "6h" + firstRun: "immediate"runs === 1. I replicated this declaration verbatim on core origin/main, both before and after start(): 1 fetch on boot 1, 0 on boot 2, identical for immediate and scheduled.

Also note the flag is ignored once a schedule row exists — so the up-to-12h gap the description lists as the price of this change is present with "immediate" too. Restore the flag or rewrite the comment; see review point 1.

This comment describes a mechanism core does not have, and the line is a no-op. `firstRun` is only read in the `if (!existing)` branch (`tasks/service.ts:1334-1338`), and the fire it performs advances `nextRunMs` to `now + everyMs` (`:1395-1404`) before `start()` reaches its first tick (`:328`, after the awaited `#applyPendingSchedules()` at `:297`). Modules load before `tasks.start()` (`server/main.ts:151`/`160` vs `168`), so the declaration is always in that batch. Core's own test asserts one fire for exactly this shape: `tasks_test.ts:1290`, `every: "6h"` + `firstRun: "immediate"` → `runs === 1`. I replicated this declaration verbatim on core `origin/main`, both before and after `start()`: 1 fetch on boot 1, 0 on boot 2, identical for `immediate` and `scheduled`. Also note the flag is ignored once a schedule row exists — so the up-to-12h gap the description lists as the price of this change is present with `"immediate"` too. Restore the flag or rewrite the comment; see review point 1.
Answers the review of #2.

The stored icon row now carries the URL its bytes came from, and both the
in-memory map and the disk read refetch when the catalog names a different
one. `badge-icon:<guid>:<variant>` does not name the input, so without this
a badge whose art moves in a later catalog served the old file forever —
where before persistence a restart fixed it.

The content type is derived from the variant instead of taken from the
response header and persisted. A remote header written to disk is replayed
for good: a 200 HTML error page from the CDN would be served same-origin as
`text/html`, and a value carrying a CRLF makes `new Response` throw inside
the route — past the `.catch()` on `icon()`, on a resolved Icon nothing
evicts, which is a permanent 500 on that badge. Two variants, two known
types.

`validCatalog` lowercases the guid: `GUID_RE` is case-insensitive and every
lookup is lowercased, so an uppercase entry from a foreign writer hydrated,
counted towards `size`, and resolved nothing while reporting success.

Hydration moves off the boot path (`register` is synchronous again): a
`ctx.storage` read queues behind the metrics flush and `register` gates the
port opening. A stale disk read can no longer replace a `refresh()` that
won the race.

`firstRun: "immediate"` is restored and the comment claiming it fired a
second run is gone — it does not: a newly declared interval schedule is
already due, so both settings fire exactly one run on the first boot. What
the flag cannot cover is a later boot, because core ignores it once a row
exists; `restoreBadges()` covers that instead, starting one run when
nothing hydrated and the schedule row already fired before this boot.

README: there is no module-storage section on /system and no HTTP surface
for `module_kv` at all — say what the recovery paths really are, and that
an orphaned icon row is permanent.
Author
Owner

Fixed in 09348a1. Everything below was measured the way you measured it — a probe driving BadgeCatalog against a Map-backed store with fetch stubbed, plus boots of a real host from a detached core worktree at origin/main (2844b43), one data dir per scenario. The description is rewritten; the old firstRun and "known trade-off" sections are gone rather than softened.

1 — the firstRun section was wrong. Confirmed, and the flag is restored. I reproduced your result before changing anything: fresh data dir, one badge-catalog run either way, nextRunMs = now for a schedule with no row. "immediate" is back because it says at the declaration what the arithmetic in tasks/service.ts:1307 happens to give, and it stops being a coincidence in another repo. The comment now says what core actually does and that the flag is read only when there is no row.

I cannot reconstruct the two runs I reported. That data dir is gone, so I have no started_at_ms to show you, and I am not going to defend a measurement I cannot reproduce — every fresh boot I ran this week fires once. Treat the original claim as withdrawn.

2 — README documented a recovery path that does not exist. Removed, in the README and in badges.ts. It now says what is true: module_kv has no HTTP surface, /system's storage block is the metrics DB, the Run button rebuilds the catalog and cannot touch an icon row, and dropping one means SQL against /data/opsdeck.duckdb. That also absorbs 8: an orphaned icon row is stated to be permanent, since ctx.storage has no enumeration and the module cannot find it either.

3 — the stored contentType. The type is now derived from the variant and never stored: fetchIcon ignores the response header entirely, StoredIcon has no contentType field, and validIcon rejects anything else. Your CRLF row is in the probe — it now resolves to image/svg+xml and new Response builds cleanly. The CDN-serves-text/html case is covered by the same line.

4 — the icon key omitted the URL. StoredIcon carries the url it fetched, and a row the catalog has moved on from refetches and is rewritten. Verified on the host: with the stored row pointed at a _OLD.svg and holding marker bytes, the request returned the real 6,880-byte icon and the row came back with the current URL.

While testing that I found the same bug one layer up, which your comment implies but does not name: this.icons was keyed guid:variant, so a process that outlived the refresh which moved the art kept serving it from memory whatever the disk said. That map is keyed by URL now. In-process probe: catalog moves the badge, next icon() returns the new bytes, no restart.

5 — the recovery gap. Taken. restoreBadges() in mod.ts: nothing hydrated, a schedule row that already carries a lastRunMs, and that lastRunMs older than this boot means storage went away under a live window, so the module starts one run itself. The last condition is the one your sketch needs — firstRun fires during #applyPendingSchedules() and stamps lastRunMs while the hydrate read is still queued, so without it a fresh install can look exactly like a cleared one. Host check: cleared module_kv with the schedule row current gives badge catalog gone from storage — refreshing now and one run at origin: module; a fresh data dir gives one run at origin: schedule and no warning.

6 — await badges.hydrate() in front of Deno.serve. Gone; register is synchronous again. Hydration and the recovery decision run beside boot, and I took the nameless-poll cost you offered rather than writing a better comment for the await. That needed a guard I did not have: a disk read landing after a refresh() would have replaced the fresher catalog with the older one, so hydrate() no longer overwrites a non-empty map.

7 — uppercase guid. validCatalog lowercases. Probe: the stored uppercase entry now resolves both a name and an icon.

9 — version. The diff was right, the description was wrong; it says 0.4.10 → 0.5.0 now.

Two notes on things I did not change. validCatalog still does not check CONTENT_PREFIX — the guard stays entirely in icon(), and the probe asserts a hydrated off-prefix URL is refused before any fetch, so a later simplification of icon() fails a check rather than opening a hole. And every badge-catalog run logs core's run is terminal but its work has not returned; I saw it on the previous head as well, so it is not from this branch, but it may be worth a core issue.

deno check backend/mod.ts in that worktree: one error, the pre-existing query.ts Timeout/number one.

Fixed in `09348a1`. Everything below was measured the way you measured it — a probe driving `BadgeCatalog` against a `Map`-backed store with `fetch` stubbed, plus boots of a real host from a detached core worktree at `origin/main` (`2844b43`), one data dir per scenario. The description is rewritten; the old `firstRun` and "known trade-off" sections are gone rather than softened. **1 — the `firstRun` section was wrong.** Confirmed, and the flag is restored. I reproduced your result before changing anything: fresh data dir, one `badge-catalog` run either way, `nextRunMs = now` for a schedule with no row. `"immediate"` is back because it says at the declaration what the arithmetic in `tasks/service.ts:1307` happens to give, and it stops being a coincidence in another repo. The comment now says what core actually does and that the flag is read only when there is no row. I cannot reconstruct the two runs I reported. That data dir is gone, so I have no `started_at_ms` to show you, and I am not going to defend a measurement I cannot reproduce — every fresh boot I ran this week fires once. Treat the original claim as withdrawn. **2 — README documented a recovery path that does not exist.** Removed, in the README and in `badges.ts`. It now says what is true: `module_kv` has no HTTP surface, `/system`'s `storage` block is the metrics DB, the Run button rebuilds the catalog and cannot touch an icon row, and dropping one means SQL against `/data/opsdeck.duckdb`. That also absorbs **8**: an orphaned icon row is stated to be permanent, since `ctx.storage` has no enumeration and the module cannot find it either. **3 — the stored `contentType`.** The type is now derived from the variant and never stored: `fetchIcon` ignores the response header entirely, `StoredIcon` has no `contentType` field, and `validIcon` rejects anything else. Your CRLF row is in the probe — it now resolves to `image/svg+xml` and `new Response` builds cleanly. The CDN-serves-`text/html` case is covered by the same line. **4 — the icon key omitted the URL.** `StoredIcon` carries the `url` it fetched, and a row the catalog has moved on from refetches and is rewritten. Verified on the host: with the stored row pointed at a `_OLD.svg` and holding marker bytes, the request returned the real 6,880-byte icon and the row came back with the current URL. While testing that I found the same bug one layer up, which your comment implies but does not name: `this.icons` was keyed `guid:variant`, so a process that outlived the refresh which moved the art kept serving it from memory whatever the disk said. That map is keyed by URL now. In-process probe: catalog moves the badge, next `icon()` returns the new bytes, no restart. **5 — the recovery gap.** Taken. `restoreBadges()` in `mod.ts`: nothing hydrated, a schedule row that already carries a `lastRunMs`, and that `lastRunMs` older than this boot means storage went away under a live window, so the module starts one run itself. The last condition is the one your sketch needs — `firstRun` fires during `#applyPendingSchedules()` and stamps `lastRunMs` while the hydrate read is still queued, so without it a fresh install can look exactly like a cleared one. Host check: cleared `module_kv` with the schedule row current gives `badge catalog gone from storage — refreshing now` and one run at `origin: module`; a fresh data dir gives one run at `origin: schedule` and no warning. **6 — `await badges.hydrate()` in front of `Deno.serve`.** Gone; `register` is synchronous again. Hydration and the recovery decision run beside boot, and I took the nameless-poll cost you offered rather than writing a better comment for the await. That needed a guard I did not have: a disk read landing after a `refresh()` would have replaced the fresher catalog with the older one, so `hydrate()` no longer overwrites a non-empty map. **7 — uppercase guid.** `validCatalog` lowercases. Probe: the stored uppercase entry now resolves both a name and an icon. **9 — version.** The diff was right, the description was wrong; it says 0.4.10 → 0.5.0 now. Two notes on things I did not change. `validCatalog` still does not check `CONTENT_PREFIX` — the guard stays entirely in `icon()`, and the probe asserts a hydrated off-prefix URL is refused before any fetch, so a later simplification of `icon()` fails a check rather than opening a hole. And every `badge-catalog` run logs core's `run is terminal but its work has not returned`; I saw it on the previous head as well, so it is not from this branch, but it may be worth a core issue. `deno check backend/mod.ts` in that worktree: one error, the pre-existing `query.ts` `Timeout`/`number` one.
thisilike approved these changes 2026-08-21 16:39:57 +02:00
thisilike left a comment

Re-review of 09348a1, same method as last time: detached worktree at the head, copied into a core worktree at origin/main (2844b43), a probe driving BadgeCatalog against a Map-backed store, and real boots against one data dir — plus a harness against core's own TaskService for the boot-ordering questions. Every blocker is closed, and closed in the code rather than in the prose.

Re-verified independently

1 — firstRun. Withdrawn claim accepted, and the comment now matches core. Fresh data dir on the real host: one run, origin: schedule, catalog 19,902 bytes — the same number you report.

2 — the /system sentence. Gone from both places, and the replacement is accurate: I dropped the rows with SQL against module_kv in /data/opsdeck.duckdb a dozen times while testing, which is exactly what the README now tells an operator to do. The orphan-is-permanent statement is right — ctx.storage has get/set/delete and no enumeration.

3 — content type. row keys: data,url; the CDN answering content-type: text/html; charset=utf-8 yields image/svg+xml for svg and image/png for png64. There is no longer a field for a CRLF to live in. On the host: 200 image/svg+xml 6880 and 200 image/png 3607, unknown guid 404.

4 — the icon key. Verified on the host, not just in the probe: I rewrote a stored row to hold a 95-byte marker SVG under a _OLD.svg URL. The request returned the real 6,880-byte icon and the row came back pointing at the current URL — the marker never reached the response. Keying this.icons by URL as well is the right catch; a warm process now picks up moved art without a restart (probe: catalog moves the badge, next icon() returns the new bytes).

5 — restoreBadges(). Works, including the case your sketch and mine both missed. Host, module_kv deleted with the schedule row current, four separate boots: badge catalog gone from storage — refreshing now, one run at origin: module, badges back in the same boot, and a fresh data dir still gives exactly one run at origin: schedule with no warning. The lastRunMs >= bootMs guard is doing real work — without it the fresh-install case is indistinguishable.

The collision case behaves as you claim: with storage cleared and the window already due (next_run_ms in the past, last_run_ms 13 h old), core's tick lands on #13 is still live and is recorded skipped. One fetch, not two.

6 — hydration beside boot. register is synchronous, the read cannot clobber a fresher refresh() (probe: FRESH survives a slow read carrying STALE). The nameless-poll cost is bounded the way the README says: names are baked into the tree at poll time, and treeHash hashes the tree including them, so the poll after hydration is a change and pushes. One poll at POLL_SEC.

7 — uppercase guid. Resolves a name and an icon now. 9 — description matches the diff.

Rows an older build wrote ({contentType, data}, no url), not-base64, empty, over-cap, and a row that is not an object all refetch and rewrite; store get/set rejections and no store at all still cost nothing; a hydrated off-prefix URL is refused before any fetch. deno check backend/mod.ts: one error, the pre-existing query.ts one.

Your core-log note is real, and here is the cause

#execute sets state.handlerLive = true, and on the success path awaits #finish(state, "succeeded", …)finally clears handlerLive only after that await returns (tasks/service.ts:978-1009). So #finish's state.handlerLive && run.lockKey branch (:1198) fires for every successful run of any action that declares a lock: a claim is parked in #claims, the warning is logged, and the next line is work behind a released run returned; lock freed. Nothing to do with this module — badge-catalog just happens to take a lock. I see it on every run in every boot I made, including ones with no badge work involved in the ordering. Worth the core issue with that pointer; the fix is to clear handlerLive before finishing, not after.

Follow-ups, none blocking

  1. this.icons is now unbounded. Keying by URL fixed the correctness bug and removed the bound: the old guid:variant key could hold at most two entries per badge, a URL key holds one per URL the process has ever seen, and the bytes go with it. Probe: one badge, one variant, five moves → 6 live entries. Trivial in practice, and trivial to close — refresh() knows the full URL set, so it can drop map keys the new catalog does not name.

  2. hydrate() reports a row it did not apply. When the guard skips the assignment it still returns the stored fetchedAtMs, so restoreBadges logs badge catalog restored {ageHours: 40} for a catalog that is not the one being served. Measured with a snapshotting store: serving FRESH, reported 40 h. The early return is right; the log is not. Returning null when the guard skipped would say "someone else got there first".

  3. The recovery run starts before tasks.start(), which is safe by ordering rather than by construction. register runs before await tasks.start() in packages/server/main.ts, so ctx.tasks.start("badge-catalog") can insert a live run before core's start() reads liveRuns(). Harness against the real TaskService: a module-started run that is live at that moment is frozen as interrupted / "process restarted", published that way, and offered to onInterrupted, after which the handler's own #finish overwrites the row with succeeded — a false alarm and a flapping status, from a run that was fine.

    I could not make it happen on a real boot: ~12 attempts across three module layouts (teamspeak alone, plus a cold external, plus a cached external) never logged froze runs left by a previous process, because your path has two queued storage reads and a gate validation in front of the insert while liveRuns() is enqueued the moment start() is called, and the fetch is only ~100 ms. So: latent, not live. Since the safety rests entirely on that ordering, it is worth a sentence in the comment on restoreBadges — the next person to move the ctx.schedules.list() read (or to hydrate lazily) has no way to know they are holding it up.

Approving. The blockers are gone, the two durable-wrong-state paths are closed, and the description now describes the code.

Re-review of `09348a1`, same method as last time: detached worktree at the head, copied into a core worktree at `origin/main` (`2844b43`), a probe driving `BadgeCatalog` against a `Map`-backed store, and real boots against one data dir — plus a harness against core's own `TaskService` for the boot-ordering questions. Every blocker is closed, and closed in the code rather than in the prose. ## Re-verified independently **1 — `firstRun`.** Withdrawn claim accepted, and the comment now matches core. Fresh data dir on the real host: one run, `origin: schedule`, catalog `19,902` bytes — the same number you report. **2 — the `/system` sentence.** Gone from both places, and the replacement is accurate: I dropped the rows with SQL against `module_kv` in `/data/opsdeck.duckdb` a dozen times while testing, which is exactly what the README now tells an operator to do. The orphan-is-permanent statement is right — `ctx.storage` has get/set/delete and no enumeration. **3 — content type.** `row keys: data,url`; the CDN answering `content-type: text/html; charset=utf-8` yields `image/svg+xml` for `svg` and `image/png` for `png64`. There is no longer a field for a CRLF to live in. On the host: `200 image/svg+xml 6880` and `200 image/png 3607`, unknown guid `404`. **4 — the icon key.** Verified on the host, not just in the probe: I rewrote a stored row to hold a 95-byte marker SVG under a `_OLD.svg` URL. The request returned the real 6,880-byte icon and the row came back pointing at the current URL — the marker never reached the response. Keying `this.icons` by URL as well is the right catch; a warm process now picks up moved art without a restart (probe: catalog moves the badge, next `icon()` returns the new bytes). **5 — `restoreBadges()`.** Works, including the case your sketch and mine both missed. Host, `module_kv` deleted with the schedule row current, four separate boots: `badge catalog gone from storage — refreshing now`, one run at `origin: module`, badges back in the same boot, and a fresh data dir still gives exactly one run at `origin: schedule` with no warning. The `lastRunMs >= bootMs` guard is doing real work — without it the fresh-install case is indistinguishable. The collision case behaves as you claim: with storage cleared *and* the window already due (`next_run_ms` in the past, `last_run_ms` 13 h old), core's tick lands on `#13 is still live` and is recorded `skipped`. One fetch, not two. **6 — hydration beside boot.** `register` is synchronous, the read cannot clobber a fresher `refresh()` (probe: `FRESH` survives a slow read carrying `STALE`). The nameless-poll cost is bounded the way the README says: names are baked into the tree at poll time, and `treeHash` hashes the tree including them, so the poll after hydration is a change and pushes. One poll at `POLL_SEC`. **7 — uppercase guid.** Resolves a name and an icon now. **9 —** description matches the diff. Rows an older build wrote (`{contentType, data}`, no `url`), not-base64, empty, over-cap, and a row that is not an object all refetch and rewrite; store `get`/`set` rejections and no store at all still cost nothing; a hydrated off-prefix URL is refused before any fetch. `deno check backend/mod.ts`: one error, the pre-existing `query.ts` one. ## Your core-log note is real, and here is the cause `#execute` sets `state.handlerLive = true`, and on the success path awaits `#finish(state, "succeeded", …)` — `finally` clears `handlerLive` only *after* that await returns (`tasks/service.ts:978-1009`). So `#finish`'s `state.handlerLive && run.lockKey` branch (`:1198`) fires for every successful run of any action that declares a `lock`: a claim is parked in `#claims`, the warning is logged, and the next line is `work behind a released run returned; lock freed`. Nothing to do with this module — `badge-catalog` just happens to take a lock. I see it on every run in every boot I made, including ones with no badge work involved in the ordering. Worth the core issue with that pointer; the fix is to clear `handlerLive` before finishing, not after. ## Follow-ups, none blocking 1. **`this.icons` is now unbounded.** Keying by URL fixed the correctness bug and removed the bound: the old `guid:variant` key could hold at most two entries per badge, a URL key holds one per URL the process has ever seen, and the bytes go with it. Probe: one badge, one variant, five moves → 6 live entries. Trivial in practice, and trivial to close — `refresh()` knows the full URL set, so it can drop map keys the new catalog does not name. 2. **`hydrate()` reports a row it did not apply.** When the guard skips the assignment it still returns the stored `fetchedAtMs`, so `restoreBadges` logs `badge catalog restored {ageHours: 40}` for a catalog that is not the one being served. Measured with a snapshotting store: serving `FRESH`, reported 40 h. The early return is right; the log is not. Returning null when the guard skipped would say "someone else got there first". 3. **The recovery run starts before `tasks.start()`, which is safe by ordering rather than by construction.** `register` runs before `await tasks.start()` in `packages/server/main.ts`, so `ctx.tasks.start("badge-catalog")` can insert a live run before core's `start()` reads `liveRuns()`. Harness against the real `TaskService`: a module-started run that is live at that moment is frozen as `interrupted` / "process restarted", published that way, and offered to `onInterrupted`, after which the handler's own `#finish` overwrites the row with `succeeded` — a false alarm and a flapping status, from a run that was fine. I could not make it happen on a real boot: ~12 attempts across three module layouts (teamspeak alone, plus a cold external, plus a cached external) never logged `froze runs left by a previous process`, because your path has two queued storage reads and a gate validation in front of the insert while `liveRuns()` is enqueued the moment `start()` is called, and the fetch is only ~100 ms. So: latent, not live. Since the safety rests entirely on that ordering, it is worth a sentence in the comment on `restoreBadges` — the next person to move the `ctx.schedules.list()` read (or to hydrate lazily) has no way to know they are holding it up. Approving. The blockers are gone, the two durable-wrong-state paths are closed, and the description now describes the code.
@ -136,0 +276,4 @@
// The read above queues behind whatever the metrics flush is doing, so a
// refresh can land while it waits. What came off disk is then the older
// of the two and must not replace what is already being served.
if (this.byGuid.size === 0) {
Owner

When the guard skips the assignment this still returns the stored fetchedAtMs, so the caller logs badge catalog restored {ageHours: N} for a catalog it is not serving. Measured against a store whose read snapshots at call time: serving FRESH, reported 40 h.

The early return in restoreBadges is correct either way — returning null when the guard skipped would just stop the log from claiming a restore that did not happen.

When the guard skips the assignment this still returns the stored `fetchedAtMs`, so the caller logs `badge catalog restored {ageHours: N}` for a catalog it is not serving. Measured against a store whose read snapshots at call time: serving `FRESH`, reported 40 h. The early return in `restoreBadges` is correct either way — returning null when the guard skipped would just stop the log from claiming a restore that did not happen.
@ -156,0 +312,4 @@
// stored row carries one: a catalog that moves a badge's art would
// otherwise be overruled by whatever the last one put in this map, for
// the life of a process that outlives several refreshes
const key = info.url + VARIANT_SUFFIX[variant];
Owner

Keying by URL is the right fix for the correctness bug, and it removes the bound the old key had: guid:variant was at most two entries per badge, a URL key is one per URL this process has ever seen — bytes included. Probe: one badge, one variant, five catalog moves → 6 live entries, none reachable again.

Nothing urgent (badge art moves rarely, and a container restart clears it), but refresh() holds the complete URL set, so dropping map keys the new catalog does not name is a two-line close.

Keying by URL is the right fix for the correctness bug, and it removes the bound the old key had: `guid:variant` was at most two entries per badge, a URL key is one per URL this process has ever seen — bytes included. Probe: one badge, one variant, five catalog moves → 6 live entries, none reachable again. Nothing urgent (badge art moves rarely, and a container restart clears it), but `refresh()` holds the complete URL set, so dropping map keys the new catalog does not name is a two-line close.
@ -565,0 +605,4 @@
});
// onSelfConflict is drop, so a run core started meanwhile wins and this
// one is recorded skipped rather than queued behind it
await ctx.tasks.start("badge-catalog");
Owner

This is the one thing left that rests on timing rather than construction: register runs before await tasks.start() in packages/server/main.ts, so this insert can land before core's start() reads liveRuns().

Harness against the real TaskService: a module-started run that is live at that moment is frozen interrupted / "process restarted", published as such and offered to onInterrupted, then overwritten by the handler's own succeeded. A false alarm from a run that was fine.

I could not trigger it live — ~12 boots across three module layouts, no froze runs left by a previous process — because the two queued storage reads and the gate validation in front of this insert let liveRuns() win the queue, and the fetch only takes ~100 ms. Latent, not live. Worth stating in the comment that the ordering is what makes it safe, since nothing enforces it.

This is the one thing left that rests on timing rather than construction: `register` runs before `await tasks.start()` in `packages/server/main.ts`, so this insert can land before core's `start()` reads `liveRuns()`. Harness against the real `TaskService`: a module-started run that is live at that moment is frozen `interrupted` / "process restarted", published as such and offered to `onInterrupted`, then overwritten by the handler's own `succeeded`. A false alarm from a run that was fine. I could not trigger it live — ~12 boots across three module layouts, no `froze runs left by a previous process` — because the two queued storage reads and the gate validation in front of this insert let `liveRuns()` win the queue, and the fetch only takes ~100 ms. Latent, not live. Worth stating in the comment that the ordering is what makes it safe, since nothing enforces it.
Three follow-ups from the review, none of them blocking, all in code the
PR itself introduced.

Keying `icons` by URL fixed a correctness bug and removed the bound the old
`guid:variant` key had: one entry per URL the process has ever seen, bytes
included, none of them reachable again. `refresh()` holds the complete URL
set, so it now drops the keys the new catalog does not name — one badge, one
variant, five moves goes from 6 live entries to 1.

`hydrate()` returned the stored `fetchedAtMs` even when the guard skipped the
assignment, so `restoreBadges` logged `badge catalog restored {ageHours: 40}`
for a catalog it was not serving. It returns null there instead; the caller
already checks `size` before treating null as an empty store, so the recovery
run is not reachable from that path.

The recovery run's own safety rests on ordering — `register` returns before
core's `tasks.start()` reads `liveRuns()`, and only the queued reads in front
of `ctx.tasks.start` keep the insert behind it, or the run is frozen
`interrupted` and published before its own success overwrites it. Nothing
enforces that, so the comment now says it.

Verified: probe against a `Map`-backed store, 15 checks — pruning keeps the
current art servable and leaves an unchanged catalog alone; a slow read
carrying a stale catalog reports null while the fresher one is served; a cold
hydrate still reports its age, restores the entries, and a second process
serves the icon from storage with no fetch. `deno check backend/mod.ts` in a
core `main` worktree: one error, the pre-existing `query.ts` one.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Author
Owner

c407abf takes the three non-blocking follow-ups before merge — all of them in code this PR introduced.

1 — this.icons unbounded. refresh() holds the complete URL set, so it drops the map keys the new catalog does not name. Probe: one badge, one variant, five moves is 1 live entry, not 6; the current art is still served from the map afterwards, and a refresh that moves nothing prunes nothing (both variants survive).

2 — hydrate() reporting a row it did not apply. It returns null when the guard skips the assignment. The caller was already right for this: restoreBadges checks badges.size > 0 before treating null as an empty store, so the recovery run is not reachable from that path — the only change is that the log no longer announces a restore of a catalog nobody is serving. Probe: a slow read carrying a 40 h-old catalog reports null while the fresh one is served; a cold hydrate still reports its fetchedAtMs, restores the entries, and a second process serves the icon from storage with zero fetches.

3 — the ordering under ctx.tasks.start. Said in the comment: register returns before core's tasks.start() reads liveRuns(), and what keeps the insert behind that read is the queued hydrate() storage read and the schedules.list() above it — take those off the path and the run is frozen interrupted, published, then overwritten by its own success. Now the next person to move them has been told.

Probe: 15 checks, all pass. deno check backend/mod.ts in a core main worktree (9dbcb06, which now carries core#45): one error, the pre-existing query.ts Timeout/number one. No version bump — still 0.5.0.

Filed nothing yet for the core log note (handlerLive cleared after #finish rather than before, so every successful run of a locked action warns). Happy to open it with your pointer.

`c407abf` takes the three non-blocking follow-ups before merge — all of them in code this PR introduced. **1 — `this.icons` unbounded.** `refresh()` holds the complete URL set, so it drops the map keys the new catalog does not name. Probe: one badge, one variant, five moves is 1 live entry, not 6; the current art is still served from the map afterwards, and a refresh that moves nothing prunes nothing (both variants survive). **2 — `hydrate()` reporting a row it did not apply.** It returns null when the guard skips the assignment. The caller was already right for this: `restoreBadges` checks `badges.size > 0` before treating null as an empty store, so the recovery run is not reachable from that path — the only change is that the log no longer announces a restore of a catalog nobody is serving. Probe: a slow read carrying a 40 h-old catalog reports null while the fresh one is served; a cold hydrate still reports its `fetchedAtMs`, restores the entries, and a second process serves the icon from storage with zero fetches. **3 — the ordering under `ctx.tasks.start`.** Said in the comment: `register` returns before core's `tasks.start()` reads `liveRuns()`, and what keeps the insert behind that read is the queued `hydrate()` storage read and the `schedules.list()` above it — take those off the path and the run is frozen `interrupted`, published, then overwritten by its own success. Now the next person to move them has been told. Probe: 15 checks, all pass. `deno check backend/mod.ts` in a core `main` worktree (`9dbcb06`, which now carries core#45): one error, the pre-existing `query.ts` `Timeout`/`number` one. No version bump — still 0.5.0. Filed nothing yet for the core log note (`handlerLive` cleared after `#finish` rather than before, so every successful run of a locked action warns). Happy to open it with your pointer.
julian merged commit 6eae71db13 into main 2026-08-21 16:54:23 +02:00
julian deleted branch feat/issue-1-persist-badges 2026-08-21 16:54:23 +02:00
Sign in to join this conversation.
No reviewers
No labels
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
OpsDeck/module_teamspeak!2
No description provided.