Alert on disabled modules, and retry repos that were unreachable at startup #27

Merged

julian merged 5 commits from feat/external-module-retry into main 2026-08-11 15:44:43 +02:00

Conversation 5 Commits 5 Files changed 10 +1341 -70

julian commented 2026-08-11 11:45:30 +02:00

Owner

Copy link

Closes #26 — options 3 (notify) and 4 (background retry + late load). Option 1 (serve the existing checkout when a fetch fails) and option 2 (retry on the startup path) are deliberately not here; see below.

Option 3 — a failed module says so

Every entry in host.failed now raises a notification after Deno.serve (packages/server/src/modules/alerts.ts). The two open questions in the issue both resolved to "follow the precedent that already exists":

• module: "core" — PushService already raises its delivery alert that way. The module a row names is the one that would have to render it, and a module that never loaded renders nothing.
• link — left null. Every client resolves link module-relative (/m/<module>/… on the web, module/<name>?path=… on Android), and /system is a shell route none of them can reach that way. Pointing there would need a link-contract change across core and the mobile repo, so the message names the page instead and the palette finds it. Worth doing separately if the bell grows more core-owned rows.

Keyed by repo slug (credential-free, and stable across a name that changes from slug to manifest name once a clone works), so a restart loop updates one row rather than stacking one per boot. Error text goes through redactSecrets.

Option 4 — retry, and load into the running server

startExternalModuleRetries (modules/external-retry.ts) runs after Deno.serve, never before: a second of backoff on the startup path is a second the whole UI is unreachable. Backoff 5s → 15s → 45s → 2m → 5m → 10m, then it gives up and says so on the same notification row. On success the module is loaded into the running ModuleHost and the alert becomes an info-level "recovered".

Only network-shaped clone failures are retried. stage: "clone" covers both "cannot resolve host" and "the remote said 403", exactly as the issue notes, so the split is made on git's stderr (isTransientGitError) — an allow-list of network fragments, so anything the remote actually answered is left alone.

The app-assembly change

This is the part the issue called out as a change to how the app is assembled, and it is: module routers are no longer mounted into the app directly. A hono router refuses new routes once its matcher is built (SmartRouter.add throws), and anything registered after the /api/* 404 would be shadowed by it anyway. So /api/mod/* delegates to a child app that owns the full path and is rebuilt whenever ModuleHost fires onModuleLoaded.

The child is handed the original Request. Rewriting it — which is what app.mount does — would break every module WebSocket, because Deno.upgradeWebSocket only accepts the object Deno.serve produced. Docker's exec/attach terminals live on exactly this path.

Verification

deno fmt --check, deno lint, deno task check, 97 server tests green. Four new tests in packages/server/tests/external_modules_test.ts, including one that builds the app with no modules, answers a request through it (so the matcher is built), then loads a module and gets 200 from its route.

End-to-end against a real server, with a git daemon started only after boot:

09:41:43 ERROR external module skipped   errno=Connection refused
09:41:43 INFO  listening                 port 8099
09:41:50 INFO  retrying external module  attempt 1     ← still refused
09:42:05 INFO  retrying external module  attempt 2
09:42:05 INFO  module loaded             origin=external
09:42:05 INFO  external module loaded after retry      attempts=2

• GET /api/mod/testmod/ping → 200 {"pong":"testmod"} (route mounted after serve)
• ws://…/api/mod/testmod/ws → echoes, so upgrades survive the indirection
• notification: error "Module … is not running … Retrying in the background." → info "Module "testmod" recovered", one row throughout
• /api/core/system flips failed → loaded, stale failure record gone

Not in scope, on purpose

• Option 1 (fall back to the existing checkout on a failed fetch). Independent, and the issue is right that it is the bigger reduction in how often this bites. It needs a ModuleFailure shape that does not disable the module, which this PR does not introduce.
• Frontend hot-add. The shell fetches /api/core/modules once at boot, so a late module's API is live immediately and its pages appear on the next load. The recovery notification says so.
• ctx.tasks.onInterrupted for a late module misses this boot's frozen batch — tasks.start() hands it back once. Registration otherwise works after start (declareSchedule already handles it). Noted in docs/modules.md.

Closes #26 — options **3** (notify) and **4** (background retry + late load). Option 1 (serve the existing checkout when a fetch fails) and option 2 (retry on the startup path) are deliberately not here; see below. ## Option 3 — a failed module says so Every entry in `host.failed` now raises a notification after `Deno.serve` (`packages/server/src/modules/alerts.ts`). The two open questions in the issue both resolved to "follow the precedent that already exists": - **`module: "core"`** — `PushService` already raises its delivery alert that way. The module a row names is the one that would have to render it, and a module that never loaded renders nothing. - **`link`** — left `null`. Every client resolves `link` module-relative (`/m/<module>/…` on the web, `module/<name>?path=…` on Android), and `/system` is a shell route none of them can reach that way. Pointing there would need a link-contract change across core *and* the mobile repo, so the message names the page instead and the palette finds it. Worth doing separately if the bell grows more core-owned rows. Keyed by repo slug (credential-free, and stable across a `name` that changes from slug to manifest name once a clone works), so a restart loop updates one row rather than stacking one per boot. Error text goes through `redactSecrets`. ## Option 4 — retry, and load into the running server `startExternalModuleRetries` (`modules/external-retry.ts`) runs after `Deno.serve`, never before: a second of backoff on the startup path is a second the whole UI is unreachable. Backoff `5s → 15s → 45s → 2m → 5m → 10m`, then it gives up and says so on the same notification row. On success the module is loaded into the running `ModuleHost` and the alert becomes an info-level "recovered". Only network-shaped clone failures are retried. `stage: "clone"` covers both "cannot resolve host" and "the remote said 403", exactly as the issue notes, so the split is made on git's stderr (`isTransientGitError`) — an allow-list of network fragments, so anything the remote actually answered is left alone. ### The app-assembly change This is the part the issue called out as a change to how the app is assembled, and it is: **module routers are no longer mounted into the app directly.** A hono router refuses new routes once its matcher is built (`SmartRouter.add` throws), and anything registered after the `/api/*` 404 would be shadowed by it anyway. So `/api/mod/*` delegates to a child app that owns the full path and is rebuilt whenever `ModuleHost` fires `onModuleLoaded`. The child is handed the **original** `Request`. Rewriting it — which is what `app.mount` does — would break every module WebSocket, because `Deno.upgradeWebSocket` only accepts the object `Deno.serve` produced. Docker's exec/attach terminals live on exactly this path. ## Verification `deno fmt --check`, `deno lint`, `deno task check`, 97 server tests green. Four new tests in `packages/server/tests/external_modules_test.ts`, including one that builds the app with **no** modules, answers a request through it (so the matcher is built), then loads a module and gets `200` from its route. End-to-end against a real server, with a `git daemon` started only after boot: ``` 09:41:43 ERROR external module skipped errno=Connection refused 09:41:43 INFO listening port 8099 09:41:50 INFO retrying external module attempt 1 ← still refused 09:42:05 INFO retrying external module attempt 2 09:42:05 INFO module loaded origin=external 09:42:05 INFO external module loaded after retry attempts=2 ``` - `GET /api/mod/testmod/ping` → `200 {"pong":"testmod"}` (route mounted after serve) - `ws://…/api/mod/testmod/ws` → echoes, so upgrades survive the indirection - notification: `error "Module … is not running … Retrying in the background."` → `info "Module "testmod" recovered"`, one row throughout - `/api/core/system` flips `failed` → `loaded`, stale failure record gone ## Not in scope, on purpose - **Option 1 (fall back to the existing checkout on a failed fetch).** Independent, and the issue is right that it is the bigger reduction in how often this bites. It needs a `ModuleFailure` shape that does not disable the module, which this PR does not introduce. - **Frontend hot-add.** The shell fetches `/api/core/modules` once at boot, so a late module's API is live immediately and its pages appear on the next load. The recovery notification says so. - **`ctx.tasks.onInterrupted`** for a late module misses this boot's frozen batch — `tasks.start()` hands it back once. Registration otherwise works after start (`declareSchedule` already handles it). Noted in `docs/modules.md`.

julian added 1 commit 2026-08-11 11:45:30 +02:00

feat(modules): alert on disabled modules, and retry unreachable repos ... 04a522604f

A DNS failure at container start made an external module disappear for the
container's lifetime: `prepareExternalModules` recorded a `ModuleFailure`,
logged one line, and nothing ever tried again. The only surface that showed
it was `/system`, which is deliberately unlisted and therefore only answers
"why did this break" to someone who already knows something did.

Two independent changes (options 3 and 4 of #26):

Every recorded module failure now raises a notification as module "core"
(`modules/alerts.ts`), keyed by repo slug so a restart loop updates one row
instead of stacking them. No `link` — clients resolve it module-relative
(`/m/<module>/…`), and the page this points at is `/system`, a shell route
nothing can reach that way, so the message names it instead. Follows the
existing push-delivery alert, which is already raised as "core".

A clone that failed on the network is retried after `Deno.serve` on a bounded
backoff (5s → 10min, six attempts) and the module is loaded into the RUNNING
host when one succeeds; the alert becomes a "recovered" one on the same row.
Only that shape is retried: `stage: "clone"` covers both an unresolvable host
and a rejected credential, and the two are separable only in git's stderr
(`isTransientGitError`). Retrying is never done on the startup path — every
second of backoff there is a second the whole UI is unreachable.

That means the module set is no longer final when the app is assembled, which
`createApp` could not express: a hono router refuses new routes once its
matcher is built, and anything registered after the `/api/*` 404 would be
shadowed by it anyway. `/api/mod/*` now delegates to a child app that owns the
full path and is rebuilt whenever `ModuleHost` fires `onModuleLoaded`. The
child is handed the ORIGINAL Request — rewriting it (what `app.mount` does)
would break module WebSockets, since `Deno.upgradeWebSocket` only accepts the
object `Deno.serve` produced. Verified against a live server: a module cloned
on the second retry answers both REST and WS on `/api/mod/<name>/…`.

`prepareExternalModules` is now a loop over `prepareExternalModule`, which
returns its failure instead of throwing so startup and retry share one path.

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

julian requested review from thisilike 2026-08-11 11:48:14 +02:00

thisilike requested changes 2026-08-11 14:15:33 +02:00

Dismissed

thisilike left a comment

Copy link

Reviewed the diff main…feat/external-module-retry (04a5226). Verified the PR's claims locally: deno task check clean, 97 server tests pass.

The design is sound. The child-app indirection for /api/mod/* is the right call, and the WebSocket reasoning (hand the child c.req.raw, never app.mount) is correct — csrfProtect doesn't touch the body, nothing reads c.env, and the outer security-header middleware still applies, so the indirection is behaviour-preserving. The findings below are all in the retry state machine, which is also the part with no test.

Blocking

1. A duplicate name is reported as a recovery — packages/server/src/modules/external-retry.ts:112

await host.loadExternal(srcDir, name, { source, commit });
if (host.loaded.some((m) => m.name === name)) { /* recovered */ }

load() throws duplicate module name "X" precisely when a module named X is already in loaded (host.ts:296). So on a name collision this probe is true by definition: the log says "external module loaded after retry", the bell gets the info-level "recovered" row, and host.failed simultaneously holds the fresh duplicate failure. /system says failed, the bell says fixed.

The comment at :121 names this case ("or the name collides") but the branch can't be reached for it. Check host.failed.some((f) => f.configuredAs === ref.url) after the load instead of probing loaded.

2. The re-alert suppression does not suppress — external-retry.ts:100

outcome.failure.error !== previous.error assumes git's stderr is stable across attempts. It isn't — curl embeds timing:

Failed to connect to git.example.com port 443 after 2 ms: Connection refused

That is this PR's own fixture (tests/external_modules_test.ts:33). The text differs on nearly every attempt, so the alert re-raises on all six. NotificationStore.create on an existing key runs UPDATE … created_at = now (notifications/store.ts:92), so the row jumps to the top of the bell each time and re-publishes created, which wakes every registered device through PushService. That is exactly the behaviour the comment says it avoids.

Compare something stable — stage plus the matched transient fragment — or only alert on the final attempt.

3. A hanging clone, which is the case this feature exists for, is never retried — external.ts:197 + isTransientGitError

git() aborts at 120 s via AbortSignal.timeout. The kill yields a non-zero exit with partial or empty stderr, so the message is git clone failed: plus nothing that matches TRANSIENT_GIT_ERRORS — isRetryableFailure returns false and no retry is ever scheduled.

A half-up VPN or a packet-dropping firewall times out; it does not answer Connection refused. The doc comment on BACKOFF_MS names "a VPN link that is seconds away from working" as a target case, and that case is the one that falls through. Detect the abort explicitly (out.signal, or a flag on the thrown error) rather than pattern-matching stderr for it.

Should fix

4. The retry loop has no test

The four new tests cover isTransientGitError, isRetryableFailure, alert dedupe and the late mount. None of them calls startExternalModuleRetries. ExternalRetryDeps.backoffMs (external-retry.ts:23) is documented as "override the schedule (tests)" and no test uses it — the seam is dead code as it stands. All three blockers above live inside the one function nothing exercises.

5. Unbounded notification body — alerts.ts:60

At stage frontend-build, failure.error is the entire builder stdout+stderr (external.ts:220); a vite failure is kilobytes. That goes straight into the notification message, into DuckDB, and over SSE to every admin. Existing code truncates in the same situation: push/fcm.ts:149 (.slice(0, 300)), docker/backend/update.ts:239 (.slice(0, 200)).

6. Wrong give-up text — alerts.ts:47

again is false both at the last attempt and when the error stopped being transient. So attempt 2 coming back 403 produces "Gave up after 2 attempts — restart once the repository is reachable." The repository is reachable; the credential is wrong. Split the two exits.

7. Retry eligibility is computed twice — main.ts:200 and external-retry.ts:131

Two independent evaluations of isRetryableFailure over host.failed, plus the ref lookup repeated in three places. If they ever drift, the bell promises "Retrying in the background" for a module that nothing retries. Have startExternalModuleRetries report which failures it took and drive the alert flag from that.

Nits

• tests/external_modules_test.ts — the comment "and the registry the shell reads names it too" sits directly above assertEquals(registry.length, 0). It states the opposite of the assertion, and the assertion holds before the load as well, so it asserts nothing.
• external-retry.ts:86 — passes deps.log rather than the retry child logger to prepareExternalModule, so retry-time clone logs are indistinguishable from startup ones.
• external-retry.ts:110-119 — no stopped check after await host.loadExternal(…). A module can register() scheduler jobs and task definitions after scheduler.stop() / tasks.stop() during shutdown, and alertModuleRecovered can write to a closing DB. Deno.exit(0) covers it in practice; the guard is one line.
• CLAUDE.md — "Deno.serve is the last thing main.ts does" is no longer true after this PR: the alert loop, the retry start and the signal handlers all follow it.

Reviewed the diff `main…feat/external-module-retry` (04a5226). Verified the PR's claims locally: `deno task check` clean, 97 server tests pass. The design is sound. The child-app indirection for `/api/mod/*` is the right call, and the WebSocket reasoning (hand the child `c.req.raw`, never `app.mount`) is correct — `csrfProtect` doesn't touch the body, nothing reads `c.env`, and the outer security-header middleware still applies, so the indirection is behaviour-preserving. The findings below are all in the retry state machine, which is also the part with no test. ## Blocking ### 1. A duplicate name is reported as a recovery — `packages/server/src/modules/external-retry.ts:112` ```ts await host.loadExternal(srcDir, name, { source, commit }); if (host.loaded.some((m) => m.name === name)) { /* recovered */ } ``` `load()` throws `duplicate module name "X"` precisely *when a module named X is already in `loaded`* (`host.ts:296`). So on a name collision this probe is true by definition: the log says "external module loaded after retry", the bell gets the info-level "recovered" row, and `host.failed` simultaneously holds the fresh duplicate failure. `/system` says failed, the bell says fixed. The comment at :121 names this case ("or the name collides") but the branch can't be reached for it. Check `host.failed.some((f) => f.configuredAs === ref.url)` after the load instead of probing `loaded`. ### 2. The re-alert suppression does not suppress — `external-retry.ts:100` `outcome.failure.error !== previous.error` assumes git's stderr is stable across attempts. It isn't — curl embeds timing: ``` Failed to connect to git.example.com port 443 after 2 ms: Connection refused ``` That is this PR's own fixture (`tests/external_modules_test.ts:33`). The text differs on nearly every attempt, so the alert re-raises on all six. `NotificationStore.create` on an existing key runs `UPDATE … created_at = now` (`notifications/store.ts:92`), so the row jumps to the top of the bell each time and re-publishes `created`, which wakes every registered device through `PushService`. That is exactly the behaviour the comment says it avoids. Compare something stable — stage plus the matched transient fragment — or only alert on the final attempt. ### 3. A hanging clone, which is the case this feature exists for, is never retried — `external.ts:197` + `isTransientGitError` `git()` aborts at 120 s via `AbortSignal.timeout`. The kill yields a non-zero exit with partial or empty stderr, so the message is `git clone failed:` plus nothing that matches `TRANSIENT_GIT_ERRORS` — `isRetryableFailure` returns false and no retry is ever scheduled. A half-up VPN or a packet-dropping firewall times out; it does not answer `Connection refused`. The doc comment on `BACKOFF_MS` names "a VPN link that is seconds away from working" as a target case, and that case is the one that falls through. Detect the abort explicitly (`out.signal`, or a flag on the thrown error) rather than pattern-matching stderr for it. ## Should fix ### 4. The retry loop has no test The four new tests cover `isTransientGitError`, `isRetryableFailure`, alert dedupe and the late mount. None of them calls `startExternalModuleRetries`. `ExternalRetryDeps.backoffMs` (`external-retry.ts:23`) is documented as "override the schedule (tests)" and no test uses it — the seam is dead code as it stands. All three blockers above live inside the one function nothing exercises. ### 5. Unbounded notification body — `alerts.ts:60` At stage `frontend-build`, `failure.error` is the entire builder stdout+stderr (`external.ts:220`); a vite failure is kilobytes. That goes straight into the notification `message`, into DuckDB, and over SSE to every admin. Existing code truncates in the same situation: `push/fcm.ts:149` (`.slice(0, 300)`), `docker/backend/update.ts:239` (`.slice(0, 200)`). ### 6. Wrong give-up text — `alerts.ts:47` `again` is false both at the last attempt *and* when the error stopped being transient. So attempt 2 coming back `403` produces "Gave up after 2 attempts — restart once the repository is reachable." The repository is reachable; the credential is wrong. Split the two exits. ### 7. Retry eligibility is computed twice — `main.ts:200` and `external-retry.ts:131` Two independent evaluations of `isRetryableFailure` over `host.failed`, plus the ref lookup repeated in three places. If they ever drift, the bell promises "Retrying in the background" for a module that nothing retries. Have `startExternalModuleRetries` report which failures it took and drive the alert flag from that. ## Nits - `tests/external_modules_test.ts` — the comment "and the registry the shell reads names it too" sits directly above `assertEquals(registry.length, 0)`. It states the opposite of the assertion, and the assertion holds before the load as well, so it asserts nothing. - `external-retry.ts:86` — passes `deps.log` rather than the `retry` child logger to `prepareExternalModule`, so retry-time clone logs are indistinguishable from startup ones. - `external-retry.ts:110-119` — no `stopped` check after `await host.loadExternal(…)`. A module can `register()` scheduler jobs and task definitions after `scheduler.stop()` / `tasks.stop()` during shutdown, and `alertModuleRecovered` can write to a closing DB. `Deno.exit(0)` covers it in practice; the guard is one line. - `CLAUDE.md` — "`Deno.serve` is the last thing `main.ts` does" is no longer true after this PR: the alert loop, the retry start and the signal handlers all follow it.

julian referenced this pull request from a commit 2026-08-11 14:37:05 +02:00

fix(modules): make the retry state machine tell the truth

julian added 1 commit 2026-08-11 14:37:05 +02:00

fix(modules): make the retry state machine tell the truth ... 3bc2c38884

Review of #27 found three faults in the retry loop, all in the part that
had no test.

A name collision was reported as a recovery: load() rejects a duplicate
name precisely because a module of that name is in `loaded`, so probing
`loaded` after the load was true by definition. The bell said recovered
while /system still showed the repo failed. Ask the host for the failure
record instead.

The re-alert suppression compared git's whole message, which is not
stable across attempts — curl writes the connect duration into it
("after 2 ms"), so every attempt looked like a new fault, moved the row
back to the top of the bell and woke every registered device. Compare
stage plus the matched network fragment.

A clone that hangs was never retried at all, which is the case the
backoff's own comment names (a half-up VPN). The 120s abort left a
non-zero exit and empty stderr, so nothing matched. git() now owns its
controller and says so itself.

Also: split "gave up" from "stopped, the remote answered" — the second
told the operator to wait for a repository that was already reachable;
truncate the error in the notification body (a frontend-build failure
carried the whole builder log into DuckDB and every device); pass the
retry child logger down; guard the post-load path with the stop flag;
and have startExternalModuleRetries report the repos it took so the
startup alert stops evaluating eligibility a second time.

Tests: three cover the loop itself (recovery into the running host, a
name collision, and running out of attempts with exactly one alert
write) against a real local clone. 100 server tests green.

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

julian commented 2026-08-11 14:37:38 +02:00

Author

Owner

Copy link

All seven acted on in 3bc2c38, plus the nits. Thanks — every blocker was real, and the three of them share the cause you named: the one function nothing exercised.

Blocking

1. Duplicate name reported as a recovery. Correct, and the probe was true by definition. The success path now asks the host what happened rather than looking the name up in loaded:

await host.loadExternal(srcDir, name, { source, commit });
if (stopped) return;
const failure = host.failed.find((f) => f.configuredAs === ref.url);
if (!failure) { /* recovered */ }

A collision now ends on the failure alert with stopped: "permanent", so /system and the bell say the same thing. Test: a clone into a name collision is not a recovery loads a module under the name first, then lets the retry clone into it.

2. The suppression did not suppress. Also correct, and it was this PR's own fixture that proved it. isTransientGitError is now built on transientGitErrorKind, which returns the matched fragment, and the comparison is over stage + kind:

function failureSignature(failure: ModuleFailure): string {
  return `${failure.stage}:${transientGitErrorKind(failure.error) ?? failure.error}`;
}

Asserted by counting created publishes on the bus rather than by inspecting the row: two refusals of git://127.0.0.1:1/…, one write. On the old comparison that test sees two.

3. A hanging clone was never retried. The one that mattered most — the case BACKOFF_MS's own comment advertises. git() now owns its controller instead of AbortSignal.timeout, and states the reason itself, since the kill leaves nothing to infer it from:

if (abort.signal.aborted) {
  throw new Error(`git ${args[0]} ${GIT_TIMEOUT_FRAGMENT} ${GIT_TIMEOUT_MS / 1000}s: no answer from the remote`);
}

GIT_TIMEOUT_FRAGMENT heads TRANSIENT_GIT_ERRORS, so the classification stays in one list. The detection is the abort flag; the fragment is only how the fact reaches ModuleFailure, which carries a string.

Should fix

4. No test for the loop. Three now, all driving startExternalModuleRetries through the backoffMs seam, which is no longer dead:

• a repo that comes back is loaded into the running host — a real git init repo on disk, cloned by the real prepare path, loaded into a live ModuleHost; asserts the module runs, host.failed is empty and the row flipped to the recovery.
• a clone into a name collision is not a recovery — blocker 1.
• the same fault twice is one row; running out of attempts says so — blocker 2 and the exhausted exit.

No network: the success path clones a local path, the failure path is a refused loopback connect.

5. Unbounded body. Truncated at 300 characters (with whitespace collapsed, which also fixes the newline git's own errors carry into the bell). Asserted against a 5 kB builder error.

6. Wrong give-up text. Split into stopped: "exhausted" | "permanent" — the second reads "Stopped after 2 attempts — this is not a network failure and will not clear on its own", which also covers the collision from blocker 1, not just a rejected credential.

7. Computed twice. startExternalModuleRetries returns { taken, stop } and main.ts drives the alert off retries.taken.has(...). It starts before the alert loop now, which is safe by a whole backoff and is the only ordering where a single decision is possible.

Nits

All four: the retry child logger goes to prepareExternalModule (ext:retry:<slug> in the logs below), if (stopped) return after the load, the CLAUDE.md sentence rewritten to say what actually follows Deno.serve, and the registry assertion made real — the test module registers a UI schema, so /api/core/modules is asserted empty before the load and naming the module after it.

Verification

deno fmt --check, deno lint, deno task check, 100 server tests green (97 + 3).

Live boot against an unreachable repo, checking the part the unit tests cannot (the main.ts wiring):

12:35:51 ERROR [ext:127.0.0.1-1-nope]       external module skipped   errno=Connection refused
12:35:51 INFO  [opsdeck]                    listening                 port=8099
12:35:56 INFO  [ext:retry]                  retrying external module  attempt=1
12:35:56 INFO  [ext:retry:127.0.0.1-1-nope] cloning
12:35:56 ERROR [ext:retry:127.0.0.1-1-nope] external module skipped   errno=Connection refused

/api/core/notifications holds one row — "clone: … errno=Connection refused. Retrying in the background. Details on the system page.", on one line, and unchanged by attempt 1 — and /api/core/system holds one failed entry for the same repo throughout.

All seven acted on in `3bc2c38`, plus the nits. Thanks — every blocker was real, and the three of them share the cause you named: the one function nothing exercised. ## Blocking **1. Duplicate name reported as a recovery.** Correct, and the probe was true by definition. The success path now asks the host what happened rather than looking the name up in `loaded`: ```ts await host.loadExternal(srcDir, name, { source, commit }); if (stopped) return; const failure = host.failed.find((f) => f.configuredAs === ref.url); if (!failure) { /* recovered */ } ``` A collision now ends on the failure alert with `stopped: "permanent"`, so `/system` and the bell say the same thing. Test: `a clone into a name collision is not a recovery` loads a module under the name first, then lets the retry clone into it. **2. The suppression did not suppress.** Also correct, and it was this PR's own fixture that proved it. `isTransientGitError` is now built on `transientGitErrorKind`, which returns the matched fragment, and the comparison is over `stage + kind`: ```ts function failureSignature(failure: ModuleFailure): string { return `${failure.stage}:${transientGitErrorKind(failure.error) ?? failure.error}`; } ``` Asserted by counting `created` publishes on the bus rather than by inspecting the row: two refusals of `git://127.0.0.1:1/…`, one write. On the old comparison that test sees two. **3. A hanging clone was never retried.** The one that mattered most — the case `BACKOFF_MS`'s own comment advertises. `git()` now owns its controller instead of `AbortSignal.timeout`, and states the reason itself, since the kill leaves nothing to infer it from: ```ts if (abort.signal.aborted) { throw new Error(`git ${args[0]} ${GIT_TIMEOUT_FRAGMENT} ${GIT_TIMEOUT_MS / 1000}s: no answer from the remote`); } ``` `GIT_TIMEOUT_FRAGMENT` heads `TRANSIENT_GIT_ERRORS`, so the classification stays in one list. The detection is the abort flag; the fragment is only how the fact reaches `ModuleFailure`, which carries a string. ## Should fix **4. No test for the loop.** Three now, all driving `startExternalModuleRetries` through the `backoffMs` seam, which is no longer dead: - `a repo that comes back is loaded into the running host` — a real `git init` repo on disk, cloned by the real prepare path, loaded into a live `ModuleHost`; asserts the module runs, `host.failed` is empty and the row flipped to the recovery. - `a clone into a name collision is not a recovery` — blocker 1. - `the same fault twice is one row; running out of attempts says so` — blocker 2 and the exhausted exit. No network: the success path clones a local path, the failure path is a refused loopback connect. **5. Unbounded body.** Truncated at 300 characters (with whitespace collapsed, which also fixes the newline git's own errors carry into the bell). Asserted against a 5 kB builder error. **6. Wrong give-up text.** Split into `stopped: "exhausted" | "permanent"` — the second reads "Stopped after 2 attempts — this is not a network failure and will not clear on its own", which also covers the collision from blocker 1, not just a rejected credential. **7. Computed twice.** `startExternalModuleRetries` returns `{ taken, stop }` and `main.ts` drives the alert off `retries.taken.has(...)`. It starts before the alert loop now, which is safe by a whole backoff and is the only ordering where a single decision is possible. ## Nits All four: the retry child logger goes to `prepareExternalModule` (`ext:retry:<slug>` in the logs below), `if (stopped) return` after the load, the CLAUDE.md sentence rewritten to say what actually follows `Deno.serve`, and the registry assertion made real — the test module registers a UI schema, so `/api/core/modules` is asserted empty before the load and naming the module after it. ## Verification `deno fmt --check`, `deno lint`, `deno task check`, **100** server tests green (97 + 3). Live boot against an unreachable repo, checking the part the unit tests cannot (the `main.ts` wiring): ``` 12:35:51 ERROR [ext:127.0.0.1-1-nope] external module skipped errno=Connection refused 12:35:51 INFO [opsdeck] listening port=8099 12:35:56 INFO [ext:retry] retrying external module attempt=1 12:35:56 INFO [ext:retry:127.0.0.1-1-nope] cloning 12:35:56 ERROR [ext:retry:127.0.0.1-1-nope] external module skipped errno=Connection refused ``` `/api/core/notifications` holds one row — `"clone: … errno=Connection refused. Retrying in the background. Details on the system page."`, on one line, and unchanged by attempt 1 — and `/api/core/system` holds one `failed` entry for the same repo throughout.

julian requested review from thisilike 2026-08-11 14:38:10 +02:00

julian added 1 commit 2026-08-11 15:01:16 +02:00

ci: give the toolchain image git ... 14c1af5f26

The external-module tests spawn the real `git` — the code under test shells
out to `git clone`, and stubbing that would leave the clone path untested —
but the pinned deno image ships no git, so all three retry tests died with
`NotFound: Failed to spawn 'git'` and the third then timed out, since a spawn
failure is not the network-shaped stderr `isTransientGitError` looks for.

.forgejo/deno.sh now derives a thin image that adds git, keyed to the ref it
already resolves from docker/Dockerfile, so there is still exactly one pin and
a toolchain bump rebuilds rather than reusing a stale layer. The Dockerfile
goes to the daemon's stdin: a second one in the tree is a second FROM for
tools/dep-check to watch, and this one has to stay a variable to inherit the
pin.

Also un-skips docker/backend/commit_test.ts, which probes for git and had been
setting `ignore: !hasGit` on every CI run since it was written: 387 passed /
0 ignored, against 380 passed / 3 failed / 4 ignored before.

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

thisilike requested changes 2026-08-11 15:11:44 +02:00

Dismissed

thisilike left a comment

Copy link

Re-reviewed at 14c1af5 (3bc2c38 + 14c1af5). Every finding from the previous round is addressed, and addressed properly rather than papered over:

• taken replaces the second eligibility evaluation, and main.ts now drives the "Retrying in the background." promise off the loop that keeps it;
• failureSignature keys on the matched fragment instead of git's message, so curl's after 2 ms no longer reads as a new fault — and the test asserts exactly one bell write across two identical refusals, which is the property that matters;
• the recovery check asks host.failed instead of probing loaded, with a test that constructs the collision;
• briefly() caps at 300 chars, and it runs after redactSecrets, which is the order that matters;
• exhausted vs permanent are separate exits with separate text;
• git() owns its AbortController so the hang is legible downstream.

Verified locally: deno task check clean, 100 server tests pass. buildSystemInfo already redacts every failure field, so "Details on the system page." does not point at a leak.

What is left below is the tail. None of it blocks the design; #1 and #2 are real defects in paths this PR now makes reachable.

Medium

1. runBuilder still throws away its own kill — packages/server/src/modules/external.ts:256

The bug git() was just fixed for, thirteen lines further down and untouched:

signal: AbortSignal.timeout(300_000),
…
if (out.code !== 0) throw new Error(`frontend build failed:\n${stderr}${stdout}`)

A hung vite build is killed, .output() resolves with a non-zero code and empty piped output, and the message is frontend build failed: followed by nothing. That string is now a notification body, so the bell renders

frontend-build: frontend build failed:. Details on the system page.

with no reason in it at all. briefly() was added for the opposite failure of this same path — kilobytes of stack — and this is the other end of it. Same fix as git(): own controller, state the timeout in the message.

2. A throw inside attemptOnce kills the chain while the bell still promises a retry — external-retry.ts:97

attemptOnce(failure, ref, attempt).catch((e) =>
  log.error("module retry failed", { error: e as Error })
);

Log only — no reschedule, no alert update. prepareExternalModule is documented as never throwing, and inside its try that holds, but the two Deno.mkdir calls sit outside it (external.ts:89-92). ENOSPC, a read-only /data, an EACCES after a permissions change: the rejection escapes prepareExternalModule entirely, the chain ends, and the notification reads "Retrying in the background." forever with nothing running.

That is the same broken promise the taken refactor exists to eliminate, reached by a different route. Either move the mkdirs inside the try (they become a clone-stage failure, which the loop already handles) or have the .catch flip the alert to stopped.

3. main.ts iterates host.failed with await in the body while the retry loop splices it

Starting the retries before the alert loop is the right call for taken, but it also means clearExternalFailure (host.ts:249-255) can now splice the array that

for (const failure of host.failed) {
  await alertModuleFailure(notifications, failure, { … });
}

is iterating. backoff[0] is 5 s so it does not bite today, but entries can be skipped, and a fast recovery's info row can be overwritten by the startup error row — which was impossible under the old ordering. for (const failure of [...host.failed]) closes it.

4. CI: the derived image's cache key ignores the recipe — .forgejo/deno.sh:50-62

CI_IMAGE="opsdeck-deno-ci:t$(printf '%s' "$IMAGE" | tr -c '[:alnum:]._-' '-' | tail -c 33)"
if ! docker image inspect "$CI_IMAGE" >/dev/null 2>&1; then
  docker build -q -t "$CI_IMAGE" - >/dev/null <<EOF

The tag is keyed to the base ref alone. Change the RUN line — add curl, drop safe.directory, swap the package set — with the toolchain unchanged, and a long-lived runner keeps serving the old image because docker image inspect succeeds. The comment defends the FROM pin at length; the layer sitting on top of it has no cache key at all. Fold a hash of the heredoc into the tag.

Related, smaller: set -eu makes this build fatal for every deno.sh caller, so deps.yml — which runs test --allow-read tools/dep-check/ and has no use for git — now fails whenever the Debian mirrors are unreachable from the runner. The unpinned apt-get install git itself is consistent with docker/Dockerfile:28-31, so no objection there; the difference is that one runs per release and this one runs per cold runner.

Low

• Two config entries for one URL start two concurrent chains into one work tree. config.ts:117 does not dedupe, slugify maps both to the same srcDir, and taken being a Set hides the duplication while schedule() still runs once per failure — concurrent git fetch/checkout in one tree is index.lock contention. external-retry.ts:173 also takes the first matching ref, so a #dev failure would retry with the #main ref. Guarding the schedule loop with the same Set covers both.
• No jitter in BACKOFF_MS. Every taken repo fires at exactly 5 s / 15 s / 45 s. One forge, N repos, N simultaneous clones per round.
• git():233 checks abort.signal.aborted before the exit code. A clone that finishes in the same tick the 120 s timer fires is reported as a timeout. Conservative rather than wrong, but out.code !== 0 && abort.signal.aborted costs nothing.
• GIT_TIMEOUT_FRAGMENT is "timed out after", and it is first in TRANSIENT_GIT_ERRORS. curl also writes Operation timed out after 30001 milliseconds, so a genuine remote timeout is now labelled with the self-kill fragment. Both are transient, so the retry decision is right and only the logged signature misleads — but the fragment is serving as both classifier and identity, and something like git-selfkill: would keep those apart.

Re-reviewed at 14c1af5 (`3bc2c38` + `14c1af5`). Every finding from the previous round is addressed, and addressed properly rather than papered over: - `taken` replaces the second eligibility evaluation, and `main.ts` now drives the "Retrying in the background." promise off the loop that keeps it; - `failureSignature` keys on the matched fragment instead of git's message, so curl's `after 2 ms` no longer reads as a new fault — and the test asserts exactly one bell write across two identical refusals, which is the property that matters; - the recovery check asks `host.failed` instead of probing `loaded`, with a test that constructs the collision; - `briefly()` caps at 300 chars, and it runs *after* `redactSecrets`, which is the order that matters; - `exhausted` vs `permanent` are separate exits with separate text; - `git()` owns its `AbortController` so the hang is legible downstream. Verified locally: `deno task check` clean, 100 server tests pass. `buildSystemInfo` already redacts every failure field, so "Details on the system page." does not point at a leak. What is left below is the tail. None of it blocks the design; #1 and #2 are real defects in paths this PR now makes reachable. ## Medium ### 1. `runBuilder` still throws away its own kill — `packages/server/src/modules/external.ts:256` The bug `git()` was just fixed for, thirteen lines further down and untouched: ```ts signal: AbortSignal.timeout(300_000), … if (out.code !== 0) throw new Error(`frontend build failed:\n${stderr}${stdout}`) ``` A hung vite build is killed, `.output()` resolves with a non-zero code and empty piped output, and the message is `frontend build failed:` followed by nothing. That string is now a notification body, so the bell renders ``` frontend-build: frontend build failed:. Details on the system page. ``` with no reason in it at all. `briefly()` was added for the opposite failure of this same path — kilobytes of stack — and this is the other end of it. Same fix as `git()`: own controller, state the timeout in the message. ### 2. A throw inside `attemptOnce` kills the chain while the bell still promises a retry — `external-retry.ts:97` ```ts attemptOnce(failure, ref, attempt).catch((e) => log.error("module retry failed", { error: e as Error }) ); ``` Log only — no reschedule, no alert update. `prepareExternalModule` is documented as never throwing, and inside its `try` that holds, but the two `Deno.mkdir` calls sit *outside* it (`external.ts:89-92`). ENOSPC, a read-only `/data`, an EACCES after a permissions change: the rejection escapes `prepareExternalModule` entirely, the chain ends, and the notification reads "Retrying in the background." forever with nothing running. That is the same broken promise the `taken` refactor exists to eliminate, reached by a different route. Either move the mkdirs inside the try (they become a `clone`-stage failure, which the loop already handles) or have the `.catch` flip the alert to `stopped`. ### 3. `main.ts` iterates `host.failed` with `await` in the body while the retry loop splices it Starting the retries before the alert loop is the right call for `taken`, but it also means `clearExternalFailure` (`host.ts:249-255`) can now `splice` the array that ```ts for (const failure of host.failed) { await alertModuleFailure(notifications, failure, { … }); } ``` is iterating. `backoff[0]` is 5 s so it does not bite today, but entries can be skipped, and a fast recovery's info row can be overwritten by the startup error row — which was impossible under the old ordering. `for (const failure of [...host.failed])` closes it. ### 4. CI: the derived image's cache key ignores the recipe — `.forgejo/deno.sh:50-62` ```sh CI_IMAGE="opsdeck-deno-ci:t$(printf '%s' "$IMAGE" | tr -c '[:alnum:]._-' '-' | tail -c 33)" if ! docker image inspect "$CI_IMAGE" >/dev/null 2>&1; then docker build -q -t "$CI_IMAGE" - >/dev/null <<EOF ``` The tag is keyed to the base ref alone. Change the `RUN` line — add curl, drop `safe.directory`, swap the package set — with the toolchain unchanged, and a long-lived runner keeps serving the old image because `docker image inspect` succeeds. The comment defends the FROM pin at length; the layer sitting on top of it has no cache key at all. Fold a hash of the heredoc into the tag. Related, smaller: `set -eu` makes this build fatal for *every* `deno.sh` caller, so `deps.yml` — which runs `test --allow-read tools/dep-check/` and has no use for git — now fails whenever the Debian mirrors are unreachable from the runner. The unpinned `apt-get install git` itself is consistent with `docker/Dockerfile:28-31`, so no objection there; the difference is that one runs per release and this one runs per cold runner. ## Low - **Two config entries for one URL start two concurrent chains into one work tree.** `config.ts:117` does not dedupe, `slugify` maps both to the same `srcDir`, and `taken` being a Set hides the duplication while `schedule()` still runs once per failure — concurrent `git fetch`/`checkout` in one tree is `index.lock` contention. `external-retry.ts:173` also takes the *first* matching ref, so a `#dev` failure would retry with the `#main` ref. Guarding the schedule loop with the same Set covers both. - **No jitter in `BACKOFF_MS`.** Every taken repo fires at exactly 5 s / 15 s / 45 s. One forge, N repos, N simultaneous clones per round. - **`git():233` checks `abort.signal.aborted` before the exit code.** A clone that finishes in the same tick the 120 s timer fires is reported as a timeout. Conservative rather than wrong, but `out.code !== 0 && abort.signal.aborted` costs nothing. - **`GIT_TIMEOUT_FRAGMENT` is `"timed out after"`, and it is first in `TRANSIENT_GIT_ERRORS`.** curl also writes `Operation timed out after 30001 milliseconds`, so a genuine remote timeout is now labelled with the self-kill fragment. Both are transient, so the retry decision is right and only the logged signature misleads — but the fragment is serving as both classifier and identity, and something like `git-selfkill:` would keep those apart.

julian referenced this pull request from a commit 2026-08-11 15:19:40 +02:00

fix(modules): kill the same way twice, and keep one retry chain per repo

julian added 2 commits 2026-08-11 15:19:40 +02:00

fix(modules): kill the same way twice, and keep one retry chain per repo ... a92334a75d

Second review round on #27. `git()` learned to state its own kill last
round; `runBuilder` thirteen lines below it did not, and its message is
now a notification body: a hung vite build is killed, the piped output is
empty, and the bell renders "frontend-build: frontend build failed:." with
no reason in it. Same fix — own controller, state the timeout.

A rejection inside `attemptOnce` ended the chain while the bell still
promised a retry. `prepareExternalModule` is documented as never throwing
and inside its try that held, but the two `Deno.mkdir` calls sat outside
it, so a full or read-only /data escaped it entirely. They move inside
(ENOSPC/EACCES becomes an ordinary clone-stage failure the loop already
handles) and the `.catch` flips the alert to `stopped` rather than only
logging, so no route out of the loop leaves "Retrying in the background."
standing with nothing running.

The startup alert loop iterates a copy of `host.failed`: the retry loop
starts first and `clearExternalFailure` splices that array, which with an
awaited write per entry would skip whatever follows. Nothing recovers
inside the loop at a 5s first backoff — the copy is what keeps that
timing from being load-bearing.

Also: one chain per URL, since two config entries for one repo slugify to
one work tree and would fight over index.lock (the ref lookup takes the
first entry, and the guard keeps the chain and its ref the same one);
±20% jitter, so N repos on one forge do not all reconnect in the same
instant; `git()` checks the exit code before the abort flag, so a clone
finishing in the tick the timer fires is not reported as a timeout; and
the self-kill fragment is "killed after no answer for" rather than "timed
out after", which curl also writes for a remote that went quiet — both
are transient, so only the logged identity was wrong.

Tests: an unwritable data dir stops the chain instead of promising a
retry, and two config entries for one repo produce one chain and one
alert write. 102 server tests green.

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

ci: key the derived CI image to its recipe, and let it fail softly ... 8340b6767a

The tag was keyed to the base ref alone, so changing the RUN line — another
package, a different git config — kept a long-lived runner on the image built
from the old recipe: `docker image inspect` succeeds and nothing rebuilds. The
tag is now a hash of the whole heredoc, which still contains the FROM, so a
toolchain bump misses as before.

`set -eu` also made this build fatal for every deno.sh caller, and most have no
use for git — deps.yml only reads files. An unreachable Debian mirror now falls
back to the toolchain as pinned with a warning, and the suites that actually
shell out to git fail on git instead of on apt.

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

julian commented 2026-08-11 15:20:09 +02:00

Author

Owner

Copy link

All eight acted on in a92334a (server) + 8340b67 (CI).

Medium

1. runBuilder threw away its own kill. Fixed the same way git() was, and it was the same bug: own AbortController, and the message states the timeout because the killed process leaves nothing to infer it from.

if (out.code === 0) return;
const output = `${dec.decode(out.stderr)}${dec.decode(out.stdout)}`.trim();
if (abort.signal.aborted) {
  throw new Error(`frontend build timed out after ${BUILD_TIMEOUT_MS / 1000}s${output ? `:\n${output}` : ""}`);
}

Whatever partial output there is stays on the message, so briefly() is still the thing that bounds it rather than the kill.

2. A throw inside attemptOnce ended the chain. Both halves of your either/or, because they close different holes:

• the two Deno.mkdir calls move inside prepareExternalModule's try, so ENOSPC / read-only /data / EACCES becomes an ordinary clone-stage failure — non-transient, so the loop exits through stopped: "permanent" like any other;
• the .catch now flips the alert to stopped instead of only logging, so a rejection from anywhere else still ends with the bell saying what happened rather than promising a retry forever. Guarded by the stopped flag, since shutdown must not write to a closing DB.

Test: a data dir that cannot be written stops the chain rather than promising a retry — a regular file where the data directory should be, which is ENOTDIR for every uid including root, so it needs neither a full disk nor a chmod that CI's root would ignore.

3. host.failed mutated under the alert loop. for (const failure of [...host.failed]). The comment says what you did — 5 s of backoff means nothing recovers inside that loop today, and the copy is what keeps that timing from being load-bearing.

4. The derived image's cache key. The tag is a hash of the whole heredoc now:

RECIPE_KEY=$(printf '%s' "$RECIPE" | { sha256sum 2>/dev/null || cksum; } | tr -cd '[:alnum:]' | cut -c1-16)
CI_IMAGE="opsdeck-deno-ci:t$RECIPE_KEY"

The recipe still contains the FROM, so a toolchain bump misses exactly as before; changing the RUN line now misses too.

On the set -eu half: the build is no longer fatal for callers that never wanted git. A failure warns and falls back to the toolchain as pinned, so an unreachable Debian mirror fails deno task ci on git — the thing that actually needs it — instead of failing deps.yml, which only reads files.

Low

• Two entries, one repo. if (taken.has(failure.configuredAs)) continue; in the schedule loop. One chain per URL, and since the ref lookup takes the first entry for that URL, the chain that runs and the ref it uses are now the same entry. Test asserts one chain and one alert write from two recorded failures.
• Jitter. ±20% on every delay.
• Abort flag before exit code. out.code !== 0 && abort.signal.aborted — a clone that finishes in the tick the timer fires keeps its result.
• Fragment doing two jobs. GIT_TIMEOUT_FRAGMENT = "timed out after" → GIT_SELFKILL_FRAGMENT = "killed after no answer for". curl's Operation timed out after 30001 milliseconds stays matched by "operation timed out", so the retry decision is unchanged and the two stop sharing an identity. Both strings are now in the transient-fragment test.

Verification

deno fmt --check, deno lint, deno task check, 102 server tests green (100 + 2).

.forgejo/deno.sh --version end to end: new tag built, git version 2.47.3 inside it, and the tag changes when the RUN line does.

All eight acted on in `a92334a` (server) + `8340b67` (CI). ## Medium **1. `runBuilder` threw away its own kill.** Fixed the same way `git()` was, and it was the same bug: own `AbortController`, and the message states the timeout because the killed process leaves nothing to infer it from. ```ts if (out.code === 0) return; const output = `${dec.decode(out.stderr)}${dec.decode(out.stdout)}`.trim(); if (abort.signal.aborted) { throw new Error(`frontend build timed out after ${BUILD_TIMEOUT_MS / 1000}s${output ? `:\n${output}` : ""}`); } ``` Whatever partial output there is stays on the message, so `briefly()` is still the thing that bounds it rather than the kill. **2. A throw inside `attemptOnce` ended the chain.** Both halves of your either/or, because they close different holes: - the two `Deno.mkdir` calls move inside `prepareExternalModule`'s `try`, so ENOSPC / read-only `/data` / EACCES becomes an ordinary `clone`-stage failure — non-transient, so the loop exits through `stopped: "permanent"` like any other; - the `.catch` now flips the alert to `stopped` instead of only logging, so a rejection from anywhere else still ends with the bell saying what happened rather than promising a retry forever. Guarded by the `stopped` flag, since shutdown must not write to a closing DB. Test: `a data dir that cannot be written stops the chain rather than promising a retry` — a regular file where the data directory should be, which is ENOTDIR for every uid including root, so it needs neither a full disk nor a chmod that CI's root would ignore. **3. `host.failed` mutated under the alert loop.** `for (const failure of [...host.failed])`. The comment says what you did — 5 s of backoff means nothing recovers inside that loop today, and the copy is what keeps that timing from being load-bearing. **4. The derived image's cache key.** The tag is a hash of the whole heredoc now: ```sh RECIPE_KEY=$(printf '%s' "$RECIPE" | { sha256sum 2>/dev/null || cksum; } | tr -cd '[:alnum:]' | cut -c1-16) CI_IMAGE="opsdeck-deno-ci:t$RECIPE_KEY" ``` The recipe still contains the FROM, so a toolchain bump misses exactly as before; changing the `RUN` line now misses too. On the `set -eu` half: the build is no longer fatal for callers that never wanted git. A failure warns and falls back to the toolchain as pinned, so an unreachable Debian mirror fails `deno task ci` on git — the thing that actually needs it — instead of failing `deps.yml`, which only reads files. ## Low - **Two entries, one repo.** `if (taken.has(failure.configuredAs)) continue;` in the schedule loop. One chain per URL, and since the ref lookup takes the first entry for that URL, the chain that runs and the ref it uses are now the same entry. Test asserts one chain and one alert write from two recorded failures. - **Jitter.** ±20% on every delay. - **Abort flag before exit code.** `out.code !== 0 && abort.signal.aborted` — a clone that finishes in the tick the timer fires keeps its result. - **Fragment doing two jobs.** `GIT_TIMEOUT_FRAGMENT = "timed out after"` → `GIT_SELFKILL_FRAGMENT = "killed after no answer for"`. curl's `Operation timed out after 30001 milliseconds` stays matched by `"operation timed out"`, so the retry decision is unchanged and the two stop sharing an identity. Both strings are now in the transient-fragment test. ## Verification `deno fmt --check`, `deno lint`, `deno task check`, **102** server tests green (100 + 2). `.forgejo/deno.sh --version` end to end: new tag built, `git version 2.47.3` inside it, and the tag changes when the `RUN` line does.

julian requested review from thisilike 2026-08-11 15:22:23 +02:00

thisilike approved these changes 2026-08-11 15:39:06 +02:00

thisilike left a comment

Copy link

Re-reviewed at 8340b67 (a92334a + 8340b67). Everything from the last round is fixed, and the fixes went to the cause rather than the symptom:

• runBuilder owns its AbortController and names the timeout, so a killed vite build no longer produces frontend build failed: and nothing;
• the two Deno.mkdir calls moved inside prepareExternalModule's try, which makes the function honest about never throwing instead of the .catch compensating for it — and the .catch flips the alert to stopped anyway, which is the right belt-and-braces;
• main.ts iterates a copy of host.failed, with the timing written down as not load-bearing;
• one chain per URL, so two entries for one repo cannot fight over index.lock;
• jitter on the backoff;
• git() checks the exit code before the abort flag, so a clone that lands in the same tick as the timer keeps its work;
• GIT_SELFKILL_FRAGMENT is "killed after no answer for", which keeps curl's own Operation timed out after 30001 milliseconds a distinct fault — and the test now asserts both.

The new tests are the good kind: the ENOSPC-shaped one uses a regular file where the data directory should be (ENOTDIR for root too, no privileges needed), and the dedupe one counts bus writes rather than inspecting internals. deno task check clean, 102 tests pass.

Approving. Two follow-ups below; neither blocks this.

1. A crash-killed clone wedges the work tree permanently — external.ts:178-186 (syncRepo)

syncRepo picks clone-vs-fetch from the presence of <srcDir>/.git, never from its validity. Measured:

kill	leftover
SIGTERM — what AbortController sends	none; git's own handler removes the junk dir
SIGKILL	. .. .git

So the new 120 s self-kill is clean, which was worth confirming given this PR is what makes that path reachable. SIGKILL is the other case: an OOM kill, docker stop past the grace period, a host reset mid-clone. With a partial .git on disk, every git command in syncRepo answers

fatal: not a git repository (or any parent up to mount point /)

That matches no fragment in TRANSIENT_GIT_ERRORS, so isRetryableFailure is false, the retry loop never takes it, and the module stays disabled on that boot and every boot after it — with no action named in the notification that would clear it.

Strictly pre-existing: the wedge predates this branch. Worth a follow-up anyway, because this PR is the one that makes "the container heals itself" the contract, and this is the single clone-stage failure it can never heal. Probing git rev-parse --git-dir instead of stat(.git), and removing srcDir before falling back to a fresh clone, closes it.

2. The soft CI fallback re-opens the silent-skip hole this file complains about — .forgejo/deno.sh:66-73

deno.sh:38-41 documents that without git, docker/backend/commit_test.ts "silently never ran at all", and that test still guards itself (commit_test.ts:17,25 — ignore: !hasGit). The new fallback warns on stderr and sets CI_IMAGE=$IMAGE, so an unreachable Debian mirror puts that suite straight back to silently skipping with CI green.

The new external-module tests do fail loudly — they shell out to git unguarded — so something catches it today. That is the accident of which suite happens to exist, not a property of the design. Either have the wrapper export a "git was expected" variable that turns hasGit skips into failures, or keep the soft fallback only for the callers that declare no git dependency.

Smaller

• external-retry.ts:108-123 — the rescue alert spreads the previous failure, so any escaped error carries that attempt's stage (clone:) whatever its origin, and host.failed is not updated, so /system and the bell would disagree. With the mkdirs moved this should now be unreachable, which is the right amount of defense; it just contradicts itself if it ever fires.
• .forgejo/deno.sh — nothing removes superseded opsdeck-deno-ci:* tags, so every recipe or toolchain change leaves a full Deno image behind on a long-lived runner.
• RECIPE_KEY — { sha256sum 2>/dev/null || cksum; } yields a different tag on a runner without sha256sum, so a mixed fleet builds the same recipe twice; cksum is also a CRC32 where the comment reasons about a hash.

Checked and fine: the unquoted heredoc collapses the \-continued RUN into a single line, which is a valid Dockerfile and unchanged in behaviour from before this commit.

Re-reviewed at 8340b67 (`a92334a` + `8340b67`). Everything from the last round is fixed, and the fixes went to the cause rather than the symptom: - `runBuilder` owns its `AbortController` and names the timeout, so a killed vite build no longer produces `frontend build failed:` and nothing; - the two `Deno.mkdir` calls moved inside `prepareExternalModule`'s try, which makes the function honest about never throwing instead of the `.catch` compensating for it — and the `.catch` flips the alert to `stopped` anyway, which is the right belt-and-braces; - `main.ts` iterates a copy of `host.failed`, with the timing written down as *not* load-bearing; - one chain per URL, so two entries for one repo cannot fight over `index.lock`; - jitter on the backoff; - `git()` checks the exit code before the abort flag, so a clone that lands in the same tick as the timer keeps its work; - `GIT_SELFKILL_FRAGMENT` is `"killed after no answer for"`, which keeps curl's own `Operation timed out after 30001 milliseconds` a distinct fault — and the test now asserts both. The new tests are the good kind: the ENOSPC-shaped one uses a regular file where the data directory should be (ENOTDIR for root too, no privileges needed), and the dedupe one counts bus writes rather than inspecting internals. `deno task check` clean, 102 tests pass. Approving. Two follow-ups below; neither blocks this. ## 1. A crash-killed clone wedges the work tree permanently — `external.ts:178-186` (`syncRepo`) `syncRepo` picks clone-vs-fetch from the *presence* of `<srcDir>/.git`, never from its validity. Measured: | kill | leftover | |---|---| | SIGTERM — what `AbortController` sends | none; git's own handler removes the junk dir | | SIGKILL | `. .. .git` | So the new 120 s self-kill is clean, which was worth confirming given this PR is what makes that path reachable. SIGKILL is the other case: an OOM kill, `docker stop` past the grace period, a host reset mid-clone. With a partial `.git` on disk, every git command in `syncRepo` answers ``` fatal: not a git repository (or any parent up to mount point /) ``` That matches no fragment in `TRANSIENT_GIT_ERRORS`, so `isRetryableFailure` is false, the retry loop never takes it, and the module stays disabled on that boot and every boot after it — with no action named in the notification that would clear it. Strictly pre-existing: the wedge predates this branch. Worth a follow-up anyway, because this PR is the one that makes "the container heals itself" the contract, and this is the single clone-stage failure it can never heal. Probing `git rev-parse --git-dir` instead of `stat(.git)`, and removing `srcDir` before falling back to a fresh clone, closes it. ## 2. The soft CI fallback re-opens the silent-skip hole this file complains about — `.forgejo/deno.sh:66-73` `deno.sh:38-41` documents that without git, `docker/backend/commit_test.ts` "silently never ran at all", and that test still guards itself (`commit_test.ts:17,25` — `ignore: !hasGit`). The new fallback warns on stderr and sets `CI_IMAGE=$IMAGE`, so an unreachable Debian mirror puts that suite straight back to silently skipping with CI green. The new external-module tests do fail loudly — they shell out to git unguarded — so something catches it today. That is the accident of which suite happens to exist, not a property of the design. Either have the wrapper export a "git was expected" variable that turns `hasGit` skips into failures, or keep the soft fallback only for the callers that declare no git dependency. ## Smaller - `external-retry.ts:108-123` — the rescue alert spreads the *previous* failure, so any escaped error carries that attempt's stage (`clone:`) whatever its origin, and `host.failed` is not updated, so `/system` and the bell would disagree. With the mkdirs moved this should now be unreachable, which is the right amount of defense; it just contradicts itself if it ever fires. - `.forgejo/deno.sh` — nothing removes superseded `opsdeck-deno-ci:*` tags, so every recipe or toolchain change leaves a full Deno image behind on a long-lived runner. - `RECIPE_KEY` — `{ sha256sum 2>/dev/null || cksum; }` yields a different tag on a runner without `sha256sum`, so a mixed fleet builds the same recipe twice; `cksum` is also a CRC32 where the comment reasons about a hash. Checked and fine: the unquoted heredoc collapses the `\`-continued `RUN` into a single line, which is a valid Dockerfile and unchanged in behaviour from before this commit.

julian referenced this pull request 2026-08-11 15:44:12 +02:00

A crash-killed clone wedges the module work tree permanently #28

julian referenced this pull request 2026-08-11 15:44:21 +02:00

CI: .forgejo/deno.sh follow-ups — silent git skip, stale derived images, non-deterministic recipe key #29

julian referenced this pull request 2026-08-11 15:44:33 +02:00

The retry loop's rescue alert reports the previous attempt's stage, and /system never learns #30

julian merged commit d37e579b4d into main 2026-08-11 15:44:43 +02:00

julian deleted branch feat/external-module-retry 2026-08-11 15:44:43 +02:00

julian referenced this pull request from a commit 2026-08-11 15:44:45 +02:00

Alert on disabled modules, and retry repos that were unreachable at startup (#27)

thisilike referenced this pull request 2026-08-11 19:25:30 +02:00

feat(ui): the components an external module turned out to need #31

julian referenced this pull request 2026-08-16 13:31:24 +02:00

fix(modules): re-clone a work tree git cannot use, instead of wedging on it #40

julian referenced this pull request from a commit 2026-08-16 13:54:35 +02:00

ci(deno.sh): losing git is an error, one digest, and no image left behind

julian referenced this pull request 2026-08-16 13:54:53 +02:00

ci(deno.sh): losing git is an error, one digest, and no image left behind #42

julian referenced this pull request from a commit 2026-08-17 00:00:48 +02:00

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

julian referenced this pull request 2026-08-17 00:01:03 +02:00

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

thisilike referenced this pull request 2026-08-21 16:36:47 +02:00

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

julian referenced this pull request from a commit 2026-08-21 16:44:52 +02:00

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

Sign in to join this conversation.

Reviewers

No reviewers

thisilike

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

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/core!27

No description provided.