fix(modules): the retry rescue names its own stage, and /system hears about it #45

julian · 2026-08-17T00:01:03+02:00

julian commented

2026-08-17 00:01:03 +02:00

Closes #30.

The two contradictions

schedule's catch in packages/server/src/modules/external-retry.ts:

attemptOnce(failure, ref, attempt).catch(async (e) => {
  log.error("module retry failed", { error: e as Error });
  if (stopped) return;
  await alertModuleFailure(
    notifications,
    { ...failure, error: (e as Error).message },
    { attempts: attempt + 1, stopped: "permanent" },
  ).catch(…);
});

It spread the previous failure, so the alert carried that attempt's stage — always clone, because that is the only shape this loop takes. A throw out of host.loadExternal reached the operator as a clone problem.
It never touched host.failed. Every escape this catch can see lands after attemptOnce already cleared the pre-retry record — since #27 moved the mkdirs inside prepareExternalModule's try, nothing above that clear can reject — so the host held nothing, and /system reported the repo as unknown, the status that otherwise means the loader skipped the entry, while the bell called the module permanently stopped. /system is the page that alert sends the operator to.

The taxonomy call

The issue flags that no ModuleFailureStage means "the retry machinery itself broke". It gets one: retry.

The alternative the issue offers — leave the stage out of the rescue message — does not actually settle it. ModuleFailure.stage is not optional and /system renders it, so the record would still assert clone or some other lie; only the bell would go quiet about it. The point of the fix is that the two agree, so the honest answer is a stage that says what happened.

It slots in as its own thing rather than reusing backend: nothing in the retry path is supposed to reject at all — prepareExternalModule carries its failure in the outcome, load() disables the module — so this names an escape from both, and the realistic cause is a full or read-only /data rather than anything about the module.

isRetryableFailure gates on stage === "clone", so a retry failure is never itself retried. That is right: nothing about a full /data resolves on a backoff.

The fix

const rescued: ModuleFailure = {
  name: failure.name,
  origin: failure.origin,
  configuredAs: failure.configuredAs,
  source: failure.source,
  stage: "retry",
  error: (e as Error).message,
};
host.clearExternalFailure(ref.url);
host.recordFailure(rescued);

The record makes the host say what the bell says. The clear is defence against an escape from above attemptOnce's own clear — unreachable today, cheap to keep.

Still meant to be unreachable overall: the two Deno.mkdir calls moved inside prepareExternalModule's try in #27. This only makes it honest when it fires.

Test

One added to packages/server/tests/external_modules_test.ts. Nothing in the retry path is supposed to reject, so reaching the catch means making something in it throw — host.loadExternal is stubbed to reject with no space left, which is the shape of the real cause. It asserts both halves:

the alert reads retry: no space left. Stopped after 1 attempt …, not clone:;
host.failed holds exactly one record, stage retry, with that URL and message — where before, attemptOnce had already cleared the pre-retry record, so /system showed the module as neither running nor failed while the bell called it permanently stopped.

Verified it fails with the source change stashed.

Elsewhere

Adding a union member breaks nothing: system.ts passes ModuleFailureStage straight through into the report and the shell renders it as a string — there is no exhaustive switch on it anywhere.

Gates: deno task check, deno lint packages/server/, deno fmt --check, packages/server/tests/ (111 passed).

CLAUDE.md's stage list gains it, with why it is the odd one.

Closes #30. ## The two contradictions `schedule`'s catch in `packages/server/src/modules/external-retry.ts`: ```ts attemptOnce(failure, ref, attempt).catch(async (e) => { log.error("module retry failed", { error: e as Error }); if (stopped) return; await alertModuleFailure( notifications, { ...failure, error: (e as Error).message }, { attempts: attempt + 1, stopped: "permanent" }, ).catch(…); }); ``` 1. It spread the **previous** failure, so the alert carried that attempt's stage — always `clone`, because that is the only shape this loop takes. A throw out of `host.loadExternal` reached the operator as a clone problem. 2. It never touched `host.failed`. Every escape this catch can see lands after `attemptOnce` already cleared the pre-retry record — since #27 moved the `mkdir`s inside `prepareExternalModule`'s try, nothing above that clear can reject — so the host held nothing, and `/system` reported the repo as `unknown`, the status that otherwise means the loader skipped the entry, while the bell called the module permanently stopped. `/system` is the page that alert sends the operator to. ## The taxonomy call The issue flags that no `ModuleFailureStage` means "the retry machinery itself broke". It gets one: **`retry`**. The alternative the issue offers — leave the stage out of the rescue message — does not actually settle it. `ModuleFailure.stage` is not optional and `/system` renders it, so the record would still assert `clone` or some other lie; only the bell would go quiet about it. The point of the fix is that the two agree, so the honest answer is a stage that says what happened. It slots in as its own thing rather than reusing `backend`: nothing in the retry path is supposed to reject at all — `prepareExternalModule` carries its failure in the outcome, `load()` disables the module — so this names an escape from *both*, and the realistic cause is a full or read-only `/data` rather than anything about the module. `isRetryableFailure` gates on `stage === "clone"`, so a `retry` failure is never itself retried. That is right: nothing about a full `/data` resolves on a backoff. ## The fix ```ts const rescued: ModuleFailure = { name: failure.name, origin: failure.origin, configuredAs: failure.configuredAs, source: failure.source, stage: "retry", error: (e as Error).message, }; host.clearExternalFailure(ref.url); host.recordFailure(rescued); ``` The record makes the host say what the bell says. The clear is defence against an escape from above `attemptOnce`'s own clear — unreachable today, cheap to keep. Still meant to be unreachable overall: the two `Deno.mkdir` calls moved inside `prepareExternalModule`'s try in #27. This only makes it honest when it fires. ## Test One added to `packages/server/tests/external_modules_test.ts`. Nothing in the retry path is supposed to reject, so reaching the catch means making something in it throw — `host.loadExternal` is stubbed to reject with `no space left`, which is the shape of the real cause. It asserts both halves: - the alert reads `retry: no space left. Stopped after 1 attempt …`, not `clone:`; - `host.failed` holds exactly one record, stage `retry`, with that URL and message — where before, `attemptOnce` had already cleared the pre-retry record, so `/system` showed the module as neither running nor failed while the bell called it permanently stopped. Verified it fails with the source change stashed. ## Elsewhere Adding a union member breaks nothing: `system.ts` passes `ModuleFailureStage` straight through into the report and the shell renders it as a string — there is no exhaustive switch on it anywhere. Gates: `deno task check`, `deno lint packages/server/`, `deno fmt --check`, `packages/server/tests/` (111 passed). `CLAUDE.md`'s stage list gains it, with why it is the odd one.

julian added 1 commit

2026-08-17 00:01:03 +02:00

fix(modules): the retry rescue names its own stage, and /system hears about it

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

Details

Build and Deploy / build (pull_request) Has been skipped

Details

Dependency Check / dependencies (pull_request) Successful in 1m10s

Details

9bf71f9239

Follow-up from the review of #27. The belt-and-braces catch in
`startExternalModuleRetries` contradicted itself twice if it ever fired.

It spread the PREVIOUS failure, so the notification carried that attempt's
stage — always `clone`, because that is the only shape this loop takes — no
matter where the escaped error came from. A throw out of `host.loadExternal`
was reported to the operator as a clone problem.

And it never touched `host.failed`. Depending on where the throw landed the
host held either the stale pre-retry record or nothing at all, while the bell
said the module was permanently stopped — and `/system` is the page that alert
sends the operator to.

There was no `ModuleFailureStage` for "the retry machinery itself broke", which
is the taxonomy call this needed. It gets one: `retry`. Omitting the stage from
the rescue message instead would have left `/system` asserting one anyway,
since `ModuleFailure.stage` is not optional and the system page renders it —
and the whole point here is that the two agree. `isRetryableFailure` gates on
`stage === "clone"`, so a `retry` failure is never itself retried, which is
right: nothing about a full `/data` resolves on a backoff.

The rescue now builds a fresh failure from the identity fields, clears any
record for that URL and records this one, so either prior state ends as the
one record the bell describes.

Still meant to be unreachable — the two `Deno.mkdir` calls moved inside
`prepareExternalModule`'s try in #27. This only makes it honest when it fires.

Closes #30

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

julian requested review from thisilike

2026-08-17 00:01:09 +02:00

thisilike approved these changes

2026-08-21 16:36:47 +02:00

thisilike left a comment

Re-review, and I checked the claims rather than reading them.

Ran on the PR head merged with current main (2844b43) — the branch sits on fb29e2c, and main has since gained ctx.storage, which touches host.ts. Merges clean, no semantic conflict:

deno fmt --check, deno lint (repo-wide, as CI runs it), deno task check: all clean.
packages/server/tests/: 120 passed (the 111 you quote, plus the 9 storage tests main added).
Reverted external-retry.ts to base and re-ran the new test: it fails, with exactly clone: no space left. Stopped after 1 attempt — …. The claim holds.
Halved the revert both ways, because one assertion covering for the other is the usual way a two-part fix ships half-tested: dropping only the stage: "retry" change fails the message assertion; dropping only recordFailure fails host.failed.length. Both halves are independently covered.

The reasoning, not just the tests:

No exhaustive switch on the stage anywhere — system.ts passes it into the report, SystemPage.svelte types it string | null and renders it. The union member is as free as you say.
isRetryableFailure gates on clone, and the seeding loop runs once at startup, so a retry record can never re-enter the loop or be re-alerted by main.ts (which alerts over a copy, before the first backoff elapses).
load() wraps everything in one try, including the onLoad fan-out, and each listener has its own catch. So a module cannot end up in loaded and recorded retry — the "still meant to be unreachable" framing survives.
ref.url === failure.configuredAs is guaranteed by isRetryableFailure plus the find above, so clear-then-record targets the same identity failureKey derives the alert key from.
Clear and record before the awaited alert is also what makes the test's until deterministic. Right order.

Two prose problems, and they are the same problem twice — a sentence asserting a state the code cannot be in. Inline. Code stays as written; fix the words before merge.

Aside, not this PR: seedRepo runs git commit without -c commit.gpgsign=false, so on a checkout with global commit signing the three repo-seeding tests die with gpg failed to sign the data before asserting anything — I had to point GIT_CONFIG_GLOBAL at a stub config to run them at all. Two of the three predate this PR; the new one inherits it. Worth a follow-up.

Re-review, and I checked the claims rather than reading them. Ran on the PR head **merged with current `main` (`2844b43`)** — the branch sits on `fb29e2c`, and main has since gained `ctx.storage`, which touches `host.ts`. Merges clean, no semantic conflict: - `deno fmt --check`, `deno lint` (repo-wide, as CI runs it), `deno task check`: all clean. - `packages/server/tests/`: **120 passed** (the 111 you quote, plus the 9 storage tests main added). - Reverted `external-retry.ts` to base and re-ran the new test: it fails, with exactly `clone: no space left. Stopped after 1 attempt — …`. The claim holds. - Halved the revert both ways, because one assertion covering for the other is the usual way a two-part fix ships half-tested: dropping only the `stage: "retry"` change fails the message assertion; dropping only `recordFailure` fails `host.failed.length`. Both halves are independently covered. The reasoning, not just the tests: - No exhaustive switch on the stage anywhere — `system.ts` passes it into the report, `SystemPage.svelte` types it `string | null` and renders it. The union member is as free as you say. - `isRetryableFailure` gates on `clone`, and the seeding loop runs once at startup, so a `retry` record can never re-enter the loop or be re-alerted by `main.ts` (which alerts over a copy, before the first backoff elapses). - `load()` wraps everything in one try, including the `onLoad` fan-out, and each listener has its own catch. So a module cannot end up in `loaded` *and* recorded `retry` — the "still meant to be unreachable" framing survives. - `ref.url === failure.configuredAs` is guaranteed by `isRetryableFailure` plus the `find` above, so clear-then-record targets the same identity `failureKey` derives the alert key from. - Clear and record *before* the awaited alert is also what makes the test's `until` deterministic. Right order. Two prose problems, and they are the same problem twice — a sentence asserting a state the code cannot be in. Inline. Code stays as written; fix the words before merge. Aside, not this PR: `seedRepo` runs `git commit` without `-c commit.gpgsign=false`, so on a checkout with global commit signing the three repo-seeding tests die with `gpg failed to sign the data` before asserting anything — I had to point `GIT_CONFIG_GLOBAL` at a stub config to run them at all. Two of the three predate this PR; the new one inherits it. Worth a follow-up.

CLAUDE.md Outdated

					
				@ -283,0 +282,4 @@

				(which carries its failure in the outcome) and `load()` (which disables the

				module) — and the rescue records it on the host too, or the bell would call a

				module permanently stopped that `/system` does not list at all. Container builds

				stamp `OPSDECK_VERSION`/`OPSDECK_COMMIT` via Docker build args; a checkout

/system does list it. moduleReports has a third status for exactly this shape: a configured entry with neither a load nor a failure comes out as status: "unknown", whose own comment reads "seeing it means the loader itself skipped an entry". So the pre-fix symptom was not a missing row — it was a row blaming the loader, which is a different wrong answer and the one an operator would actually have been staring at. The test comment gets this right ("neither running nor failed"); this sentence overstates it into an absence.

`/system` does list it. `moduleReports` has a third status for exactly this shape: a configured entry with neither a load nor a failure comes out as `status: "unknown"`, whose own comment reads "seeing it means the loader itself skipped an entry". So the pre-fix symptom was not a missing row — it was a row blaming the loader, which is a *different* wrong answer and the one an operator would actually have been staring at. The test comment gets this right ("neither running nor failed"); this sentence overstates it into an absence.

packages/server/src/modules/external-retry.ts

					
				@ -117,0 +120,4 @@

				        // reported as a clone problem. The escape can come from anywhere in

				        // the path, and naming where it came from is the point of a stage.

				        const rescued: ModuleFailure = {

				          name: failure.name,

Nit, no change requested: when the escape comes out of loadExternal the manifest name is known (outcome.module.name) and this reports the repo slug instead, so the title stays Module "<slug>" is not running for a module whose manifest read fine. Harmless — failureKey is slug-based either way, and the row the operator was already looking at said the slug too — and the rescue has no access to the outcome without restructuring attemptOnce.

Nit, no change requested: when the escape comes out of `loadExternal` the manifest name is known (`outcome.module.name`) and this reports the repo slug instead, so the title stays `Module "<slug>" is not running` for a module whose manifest read fine. Harmless — `failureKey` is slug-based either way, and the row the operator was already looking at said the slug too — and the rescue has no access to the outcome without restructuring `attemptOnce`.

packages/server/src/modules/external-retry.ts

					
				@ -117,0 +132,4 @@

				        // at all, while the bell says the module is permanently stopped —

				        // and `/system` is the page that alert sends the operator to. Clear

				        // then record, so either state ends as this one record.

				        host.clearExternalFailure(ref.url);

"either the stale pre-retry record or nothing at all" — only the second is reachable. prepareExternalModule cannot reject before attemptOnce's own clear on line 162: since #27 moved the mkdirs inside its try, everything that can fail is in there, and slugify/log.child above the try are pure string work. Every escape this catch can actually see is therefore post-clear, so the state is always "nothing at all".

Verified rather than reasoned: delete this clearExternalFailure(ref.url) line and all 10 tests in the file still pass. Keep the call — belt and braces is the whole spirit of this handler — but the comment asserts a host state that cannot occur, which is the same class of lie the PR exists to remove. Either name it as defensive against an escape that would have to come from above line 162, or drop the two-states framing.

"either the stale pre-retry record or nothing at all" — only the second is reachable. `prepareExternalModule` cannot reject before `attemptOnce`'s own clear on line 162: since #27 moved the `mkdir`s inside its try, everything that can fail is in there, and `slugify`/`log.child` above the try are pure string work. Every escape this catch can actually see is therefore post-clear, so the state is always "nothing at all". Verified rather than reasoned: delete this `clearExternalFailure(ref.url)` line and all 10 tests in the file still pass. Keep the call — belt and braces is the whole spirit of this handler — but the comment asserts a host state that cannot occur, which is the same class of lie the PR exists to remove. Either name it as defensive against an escape that would have to come from above line 162, or drop the two-states framing.

julian added 1 commit

2026-08-21 16:44:53 +02:00

docs(modules): the rescue's prose says what the host and /system actually hold

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

Details

Build and Deploy / build (pull_request) Has been skipped

Details

Dependency Check / dependencies (pull_request) Successful in 1m11s

Details

bac6cc8e10

Two sentences asserted states the code cannot be in, which is the class of
lie this PR exists to remove.

The source comment claimed the host held "either the stale pre-retry record
or nothing at all". Only the second is reachable: since #27 moved the
`mkdir`s inside `prepareExternalModule`'s try, nothing above `attemptOnce`'s
own `clearExternalFailure` can reject, so every escape this catch sees is
post-clear. The `clearExternalFailure` call stays as defence against an
escape from above that clear — now named as such rather than justified by an
unreachable state.

`CLAUDE.md` said `/system` "does not list at all" a module the bell calls
permanently stopped. It does list it: `moduleReports` gives a configured
entry with neither a load nor a failure `status: "unknown"`, the status that
otherwise means the loader skipped the entry. The pre-fix symptom was a row
blaming the loader, not a missing row.

Comments only; no behaviour change. Gates: `deno task check`,
`deno lint packages/server/`, `deno fmt --check`, `packages/server/tests/`
(111 passed).

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

julian merged commit 9dbcb06e6b into main

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

julian deleted branch fix/issue-30-retry-rescue-stage

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

julian referenced this pull request from a commit

2026-08-21 16:45:26 +02:00

fix(modules): the retry rescue names its own stage, and /system hears about it (#45)

julian referenced this pull request

2026-08-21 21:42:00 +02:00

feat(storage): see what a module is holding, and drop what outlived it #46

julian referenced this pull request

2026-08-21 21:42:10 +02:00