Skip to content

fix: re-arm the approver inbox with exponential backoff (R1.5) - #97

Merged
stormer78 merged 1 commit into
mainfrom
fix/approver-inbox-reconnect-backoff
Jul 19, 2026
Merged

fix: re-arm the approver inbox with exponential backoff (R1.5)#97
stormer78 merged 1 commit into
mainfrom
fix/approver-inbox-reconnect-backoff

Conversation

@stormer78

Copy link
Copy Markdown
Contributor

First of the audit findings. The R1.5 defect CLAUDE.md warns about was fixed on the worker inbound path — but the approver inbox, added later as a deliberately parallel pool, kept it verbatim.

The bug

offscreen.ts:1377-1384 (before):

onClose: () => {
  approverPool.delete(key);
  if (approverIdentities.has(vtaDid)) {
    setTimeout(() => void getApproverWarmSession(vtaDid).catch(() => undefined), 2000);
  }
},

One fixed 2s retry, scheduled only from onClose, failure swallowed by .catch(() => undefined). If that retry failed, no session ever opened — so onClose could not fire again and nothing re-armed. A mediator outage longer than ~2s left the approver's inbox permanently dead until the operator happened to re-run the biometric unlock.

That is the inbox which receives task-consent/request. A dead listener there means gated actions silently never get their human check — the security control simply doesn't happen, and nothing reports that it didn't (R1.5, R7.2).

The fix

Adds ReconnectScheduler to core:

  • retry forever — cap the delay, never the attempt count
  • re-arm on every failure, including first-connect (no open ⇒ no close, which is the hole the old design had)
  • never stack timers per key; reset to base on success
  • optional shouldRetry gate so an operator lock beats a retry already in flight
  • injectable timers, so the loop is testable without real waiting

The worker path deliberately keeps its own copy. That pool is documented as isolated so it "can never disturb the working worker inbound path" — adopting the shared scheduler there is a change to that path, not this one, and belongs in its own PR.

Verification

9 new tests: re-arm after failure, doubling + cap, 60s-outage recovery, reset-on-success, timer non-stacking, lock-beats-retry-in-flight, cancel-during-flight, per-key independence.

Critically, I mutation-tested them — reintroduced the original defect (removed the re-arm) and confirmed 4 of 9 fail, including "a mediator down for 60s recovers on its own". They detect the bug rather than merely passing alongside it.

Full suite 180/180 (was 171), clean build and lint, MV3 single-bundle guard holds.

Not verified: not exercised against a live mediator in a loaded extension. The logic is unit-tested against a fake clock; the wiring into createApproverWarmSession.onClose is not. A real approver flow with the mediator stopped and restarted would confirm end-to-end.

Follow-ups from the same audit (not in this PR)

  1. register-gateway.ts:71-75 throws on status before reading the body, on a consent-capable endpoint — R3.7, same bug rest-channel.ts was fixed for
  2. rest-channel.ts:91 consumes the body, so errorFromResponse re-reads a used stream and always loses the server's code — R3.7 violated inside the R3.7 fix (proven with a repro)
  3. Nine fetch call sites lack timeouts, hidden from grep by injected-fetch indirection; getVtaBearer is worst since it runs before every REST request — R1.2
  4. Persist-before-ack is violated structurally in vti-didcomm-js — needs cross-repo coordination (R4.1/R1.6)

Pre-merge checklist

  • No new bare fetch(); timeouts (R1.2) — no network code added
  • No lock held across a network await (R1.3) — n/a
  • No local state committed before its remote effect (R2.1) — n/a
  • Every retry bounded + backed off; non-idempotent ops not blind-retried (R1.4) — delay is bounded at 60s; the retried op is idempotent session establishment
  • Accept/poll/listen loops survive transient errors (R1.5) — this is the fix
  • Acks/deletes only after durable handoff (R1.6) — untouched; see follow-up 4
  • New/changed wire types (R3.*) — none
  • Config absence = most restrictive (R5.*) — shouldRetry defaults to not resurrecting a locked identity
  • Logs/status claim only what was verified (R6.*) — failed connects now log a warning instead of being swallowed
  • "Process dies on the next line" answered (R2.1) — scheduler state is in-memory; an offscreen teardown drops pending retries, and reconcileInbound/unlock re-establishes on restart, same as before
  • Deviations flagged with rule numbers — worker path intentionally not unified; rationale in code comment

The worker inbound path got a proper re-arming backoff (2s→60s, doubling,
re-arms on any failure). The approver inbox — added later as a deliberately
parallel pool — never did, and kept the original defect verbatim:

    onClose: () => {
      approverPool.delete(key);
      if (approverIdentities.has(vtaDid)) {
        setTimeout(() => void getApproverWarmSession(vtaDid).catch(() => undefined), 2000);
      }
    }

One fixed 2s retry, scheduled only from `onClose`, with its failure swallowed.
If that retry failed, no session ever opened — so `onClose` could not fire
again and nothing re-armed. A mediator outage longer than ~2s left the
approver's inbox permanently dead until the operator happened to re-run the
biometric unlock.

That inbox is the one that receives `task-consent/request`. A dead listener
there means gated actions silently never get their human check — exactly the
failure R1.5 exists to prevent, and a security control rather than a
convenience (R7.2).

Adds `ReconnectScheduler` to core: retry forever, cap the delay not the
attempt count, re-arm on every failure including first-connect (where no open
means no close), never stack timers per key, reset to base on success, and an
optional `shouldRetry` gate so an operator lock beats a retry already in
flight. Timers are injectable so the loop is testable without real waiting.

The worker path deliberately keeps its own copy for now. That pool is
documented as isolated so it "can never disturb the working worker inbound
path" — adopting the shared scheduler there is a change to that path, not to
this one, and belongs in its own PR.

Verified: 9 new tests covering re-arm, doubling+cap, 60s-outage recovery,
reset-on-success, timer non-stacking, lock-beats-retry, cancel-during-flight
and per-key independence. Confirmed they FAIL (4/9) against a mutant with the
re-arm removed, so they detect the original bug rather than merely passing.
Full suite 180/180, clean build and lint, MV3 single-bundle guard holds.

Signed-off-by: Glenn Gore <glenn.g@affinidi.com>
@stormer78
stormer78 merged commit c6f4542 into main Jul 19, 2026
3 checks passed
@stormer78
stormer78 deleted the fix/approver-inbox-reconnect-backoff branch July 19, 2026 03:14
stormer78 added a commit that referenced this pull request Jul 19, 2026
CLAUDE.md was untracked, so every contributor had their own copy or none.
Checks it in, and corrects the parts that had gone stale — its named defects
were the ones most likely to be trusted verbatim, and all three were fixed:

- R3.7's `consentRequiredFrom` example was fixed (it now matches
  `details.reason`). Replaced with the rule that actually bites now: a
  Response body reads once, so an already-parsed body must use
  `errorFromBody`, not a re-read of the spent Response (#99).
- R1.5's "one 2s retry from onClose" applied to the worker inbound path
  (fixed in #88) and then to the approver inbox (fixed in #97). Replaced with
  the invariant — cap the delay not the attempt count, re-arm on every
  failure including first-connect — and a pointer to `ReconnectScheduler`.
- R1.2's `handleApiGet`/`handleApiPost` example was fixed in #88 and is now
  the compliant reference. Replaced with the thing that actually hides these:
  fetch is injected, so `grep "fetch("` finds almost nothing and the timeout
  belongs at the injection point (#98).

Promotes the one genuinely open defect to its own section: R1.6
persist-before-ack, which is not fixable from this repo — vti-didcomm-js acks
before dispatching to `onMessage`, and the wallet persists only the message
id, so an offscreen teardown mid-prompt loses a task-consent request for good.

Adds a repo-mechanics section for the traps that cost time this week: build
`core` before typechecking dependents, lint is `tsc -b` (never `-b --noEmit`,
TS6310), cross-workspace imports need a `references` entry, what CI asserts,
stub with real `Response` objects, and Node unreffing the `AbortSignal.timeout`
timer (passes locally, fails in CI).

Signed-off-by: Glenn Gore <glenn.g@affinidi.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant