fix(#597): harden supervisor timer callbacks against stale extension ctx - #606
Merged
Conversation
Pi throws 'This extension ctx is stale after session replacement or
reload' from assertActive when an extension uses a captured 'pi' handle
after ctx.newSession(), ctx.fork(), ctx.switchSession(), or ctx.reload().
Background timers in supervisor.ts (startHeartbeat, startEventTailer)
capture 'pi' in a closure and call pi.sendMessage() at arbitrary times.
When the captured handle goes stale between timer ticks, an uncaught
throw from pi.sendMessage() became a process-fatal uncaughtException
that killed the entire Pi process during normal batch startup.
This implements the three-part fix outlined in the issue.
1. New isStaleExtensionCtx(err) helper
Recognizes Pi's distinctive stale-ctx error by message-substring
match ('This extension ctx is stale'). Defensive against
non-Error throws (null, undefined, strings, numbers) which are not
stale-ctx errors and should propagate to the caller's normal error
path. The error class is not exported by @earendil-works/pi-coding-
agent so substring match on the documented message text is the
stable contract.
2. New safeSendMessageFromTimer(pi, msg, opts) wrapper
Replaces direct pi.sendMessage(...) at timer call sites. Returns:
* true on success
* false when the call was swallowed due to stale ctx (caller
should clear its own interval)
* true on other unexpected errors (logged via console.error so
they're diagnosable, but not propagated; a background timer
in a long-running process should keep ticking rather than
crashing the host on a transient failure)
Importantly: the wrapper NEVER throws. That's the entire point.
3. Rewires the two known-affected call sites
a) startHeartbeat takeover branch (was line 3736, now 3781):
the pi.sendMessage call that emits 'supervisor-yield' on
sessionId mismatch.
b) startEventTailer's notify() callback (was line 4447, now
4486): the pi.sendMessage call that surfaces every
supervisor-event notification.
In (b), when the wrapper returns false (stale ctx detected), the
notify callback also calls stopEventTailer() so the next tick
doesn't repeat the failed call. Heartbeat already cleared its
interval explicitly in the takeover branch before the
sendMessage call, so the wrapper's false return is effectively a
no-op there.
4. Defensive timer teardown at top of activateSupervisor
The supervisor state is a mutable singleton that persists across
activate/deactivate cycles. In re-activation paths (a previous
activation that didn't go through deactivateSupervisor cleanly,
or session churn after takeover), state.heartbeatTimer and
state.eventTailer may still reference running timers that
captured a now-stale pi handle. Just reassigning
state.heartbeatTimer = startHeartbeat(...) would orphan the
previous timer (still ticking, still holding the stale pi).
Now activateSupervisor explicitly calls stopEventTailer() +
clearInterval(state.heartbeatTimer) at the top before installing
the new timers. stopEventTailer is already idempotent so this is
safe on fresh state.
Tests
8.14 — isStaleExtensionCtx identifies Pi's exact message text
8.15 — isStaleExtensionCtx rejects unrelated errors and non-Error
values (null, undefined, strings, numbers, plain objects)
and the string-throw edge case (covers if Pi ever throws a
plain string carrying the marker phrase)
8.16 — safeSendMessageFromTimer returns true and forwards args
unchanged on success
8.17 — safeSendMessageFromTimer NEVER throws on stale-ctx + returns
false (the core invariant; if this regresses, pi process
crashes return)
8.18 — safeSendMessageFromTimer logs via console.error but does NOT
throw on unrelated errors; returns true (don't stop the
timer for unrelated errors)
8.19 — startHeartbeat takeover branch uses safeSendMessageFromTimer
and has zero remaining bare pi.sendMessage() calls
8.20 — startEventTailer notify callback uses the safe wrapper +
calls stopEventTailer when wrapper returns false
8.21 — activateSupervisor calls the teardown BEFORE startHeartbeat,
uses clearInterval + state.heartbeatTimer = null (not just
reassignment)
Validation
npm run typecheck pass
npm run lint 286 warnings / 671 infos (identical to main)
npm run format:check pass (1 biome auto-format on test file)
Full test suite 3722/3723 pass, 1 skipped, zero new failures
Supervisor tests 122/122 pass (8 new for #597)
Closes #597
…followup Sage's review of the initial #597 fix caught a blocking finding: the heartbeat takeover branch calls deactivateSupervisor(pi, state), and deactivateSupervisor itself calls presentBatchSummary(pi, ...) when state.pendingSummaryDeps is set. presentBatchSummary had an UNWRAPPED pi.sendMessage at line 2023 \u2014 the exact uncaughtException pattern #597 was meant to prevent, just one frame deeper. Quote from Sage: 'startHeartbeat now wraps its direct pi.sendMessage, but then calls deactivateSupervisor(pi, state) (timer context). deactivateSupervisor can call presentBatchSummary when state.pendingSummaryDeps is set, and presentBatchSummary does a raw pi.sendMessage(...). If pi is stale, this throws and can still terminate the process. I verified this behavior with a direct runtime repro.' Sage was right. Confirmed. Fix presentBatchSummary's sole pi.sendMessage now goes through the safeSendMessageFromTimer wrapper introduced in the original PR. The function is a terminal best-effort presentation operation \u2014 if pi has gone stale, the message can't be delivered anyway; silently drop it rather than crashing the host. This is a strict generalization of the original fix and changes no behavior on the happy path. Function hoisting handles the forward reference (the wrapper is defined ~1700 lines later, but ES module function declarations hoist). Behavioral regression test (8.22) Sage's second finding: 'Call-site rewiring tests are source- substring assertions, not behavioral timer-path tests. They won't catch regressions in runtime behavior.' Adds an actual behavioral test that wires up the exact failure scenario: 1. Construct a SupervisorState in the active-with-pending-summary configuration (mirrors what the heartbeat takeover branch reaches in production) 2. Provide a stale-pi mock whose sendMessage throws Pi's distinctive 'This extension ctx is stale ...' error 3. Call deactivateSupervisor(stalePi, state) directly 4. Assert no exception escapes 5. Assert deactivation still completes its bookkeeping (state.active false, pendingSummaryDeps cleared) \u2014 confirms we didn't just swallow the error mid-flight I verified the test correctly catches the regression by temporarily reverting the presentBatchSummary fix: 8.22 fails on the unwrapped version, passes on the wrapped version. Findings explicitly NOT addressed Sage flagged 'standardize timer-error logging surface with existing extension diagnostics style' as out-of-scope follow-up. Agreed \u2014 console.error is functional for this PR; switching to a structured log channel is its own polish pass. Sage's 'withSession' question \u2014 'withSession is not a direct replacement for these passive background timers. The try/catch hardening pattern is appropriate here.' Sage explicitly endorsed the chosen approach. Confirmed not pursued. Validation npm run typecheck pass npm run lint 286 warnings / 671 infos (identical to main) npm run format:check pass (1 biome auto-format on test file) Full test suite 3723/3724 pass, 1 skipped, zero new failures Supervisor tests 123/123 pass (1 new for #597 \u2014 behavioral) Mutation-test of 8.22 confirmed 8.22 fails when fix is reverted
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #597 — reported by @beettlle. Process-crashing bug during normal batch startup.
Background
Pi throws 'This extension ctx is stale after session replacement or reload' from its
assertActiveguard when an extension uses a capturedpi: ExtensionAPIhandle afterctx.newSession(),ctx.fork(),ctx.switchSession(), orctx.reload().Two background timers in
supervisor.tscapturepiin their closures and callpi.sendMessage()at arbitrary times:startHeartbeat— 30s interval, callspi.sendMessagein the takeover-detection branchstartEventTailer.notify— fires per event, callspi.sendMessagefor each notificationWhen the captured
piwent stale between ticks, the throw became a process-fataluncaughtExceptionthat killed the entire Pi process. The reporter caught this on macOS with Pi 0.77.0; the bug is still present onmain(Pi 0.79.6).What this PR does
1. New
isStaleExtensionCtx(err)helperRecognizes Pi's distinctive stale-ctx error by message-substring match. The error class is not exported by
@earendil-works/pi-coding-agent, so substring match on the documented message text is the stable contract. Defensive against non-Error throws (null, undefined, strings) — only the specific Pi case is matched.2. New
safeSendMessageFromTimer(pi, msg, opts)wrapperThe core never-throw invariant:
trueon successfalseon stale-ctx (caller stops its own interval)trueon other unexpected errors (logged viaconsole.error, NOT propagated) — transient sendMessage failures are unlikely to repeat; only Pi specifically telling us 'you're dead, stop' (stale-ctx) gets the timer-stop treatment3. Rewires three sendMessage call sites
startHeartbeattakeover-detection branchstartEventTailer.notifycallbackpresentBatchSummary(added in Sage review followup)startHeartbeat → deactivateSupervisor → presentBatchSummarywhenstate.pendingSummaryDepsis set. Caught by Sage's review as a blocking finding — the original PR addressed only the direct call sites and missed this indirect path4. Defensive teardown at top of
activateSupervisorstopEventTailer(state.eventTailer)+clearInterval(state.heartbeatTimer)+state.heartbeatTimer = nullbefore installing new timers. Closes the re-activation gap where session churn could leave orphan timers holding stalepireferences.Tests — 9 new (8.14–8.22)
isStaleExtensionCtxidentifies Pi's exact error message textisStaleExtensionCtxrejects unrelated errors and non-Error values (null, undefined, strings, numbers, objects); accepts plain-string throws carrying the marker phrasesafeSendMessageFromTimerreturns true and forwards args unchanged on successsafeSendMessageFromTimerNEVER throws on stale-ctx + returns falsesafeSendMessageFromTimerlogs viaconsole.errorbut does NOT throw on unrelated errors; returns truestartHeartbeattakeover branch uses the safe wrapper; zero remaining barepi.sendMessage()callsstartEventTailer.notifyuses the safe wrapper + callsstopEventTaileron stale-ctx returnactivateSupervisorcalls teardown BEFOREstartHeartbeat, usesclearInterval+state.heartbeatTimer = null(not just reassignment)pendingSummaryDepsset +deactivateSupervisorcall does NOT throw. Mutation-verified to fail when thepresentBatchSummarywrap is reverted. (Sage's behavioral-test request.)Sage review
Sage flagged a blocking finding on the original PR: the heartbeat takeover branch's call chain reaches
presentBatchSummaryviadeactivateSupervisorwhenstate.pendingSummaryDepsis set, andpresentBatchSummaryhad an unwrappedpi.sendMessage. Sage verified the issue with a direct runtime repro. Fixed in commit3812491with the wrappedpresentBatchSummary+ behavioral regression test 8.22. Sage's tests-as-source-substring-assertions critique was also addressed by 8.22.Sage's two non-blocking notes were retained as deliberate decisions:
withSessionas Pi's alternative pattern — Sage agreed it's not a direct replacement for passive background timers; try/catch hardening is appropriateconsole.errorlogging surface — Sage marked as out-of-scope follow-upValidation
npm run typechecknpm run lintmainnpm run format:checkpresentBatchSummaryfix is revertedRecommendation
Patch release
v0.30.4after merge. This is a real process-crashing bug for supervised batches under realistic session-churn conditions.