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

New issue

Closed

opened 2026-08-11 15:44:33 +02:00 by julian · 0 comments

julian commented

2026-08-11 15:44:33 +02:00

Owner

Follow-up from the review of #27 (approved; not a blocker).

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

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(…);
});

Two ways it contradicts itself if it ever fires:

It spreads the previous failure, so the notification carries that attempt's stage — always clone: — whatever the origin of the escaped error. A throw out of host.loadExternal would be reported as a clone problem.
It does not touch host.failed. Depending on where the throw happened, host.failed either still holds the stale pre-retry record or (if clearExternalFailure already ran) holds nothing at all, while the bell says the module is permanently stopped. /system and the bell disagree.

This should be unreachable now that the two Deno.mkdir calls moved inside prepareExternalModule's try, and keeping the belt-and-braces catch is right. It should just be honest when it fires: record a failure on the host so /system matches, and either carry a stage that reflects where the throw came from or leave the stage out of the rescue message rather than asserting clone.

Note there is no ModuleFailureStage that means "the retry machinery itself broke" (host.ts:121-126), so this needs a small call on the taxonomy — which is why it is a follow-up and not an inline fix.

Follow-up from the review of #27 (approved; not a blocker). `schedule`'s catch in `packages/server/src/modules/external-retry.ts:108-123`: ```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(…); }); ``` Two ways it contradicts itself if it ever fires: 1. It spreads the **previous** failure, so the notification carries that attempt's stage — always `clone:` — whatever the origin of the escaped error. A throw out of `host.loadExternal` would be reported as a clone problem. 2. It does not touch `host.failed`. Depending on where the throw happened, `host.failed` either still holds the stale pre-retry record or (if `clearExternalFailure` already ran) holds nothing at all, while the bell says the module is permanently stopped. `/system` and the bell disagree. This should be unreachable now that the two `Deno.mkdir` calls moved inside `prepareExternalModule`'s try, and keeping the belt-and-braces catch is right. It should just be honest when it fires: record a failure on the host so `/system` matches, and either carry a stage that reflects where the throw came from or leave the stage out of the rescue message rather than asserting `clone`. Note there is no `ModuleFailureStage` that means "the retry machinery itself broke" (`host.ts:121-126`), so this needs a small call on the taxonomy — which is why it is a follow-up and not an inline fix.

julian referenced this issue from a commit

2026-08-11 15:44:45 +02:00

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

julian referenced this issue 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 issue from a pull request that will close it,

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

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

julian closed this issue

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

julian referenced this issue from a commit