fix(ocap-kernel): report a dead run loop instead of a healthy kernel - #1005
Merged
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
Coverage Report
File Coverage |
grypez
self-requested a review
August 5, 2026 16:52
The run loop's error was logged and swallowed, so the kernel kept answering getStatus with the record it returns when healthy while nothing on the run queue was ever processed again, and every queueMessage promise hung forever. KernelQueue now records the failure, rejects the message results waiting on it, and fails later enqueueMessage calls. Kernel reports runLoop status in getStatus (without waiting for a crank that may never end) and hands the failure to a new onRunLoopFailure option. endCrank settles its waiters even if releasing savepoints throws. The daemon logs the failure and exits non-zero. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review of the previous commit found that a delivery which throws (rather than returning {abort:true}) left endCrank's savepoint release to commit the half-finished crank: the dequeued item was lost, refcounts stuck, and promises resolved mid-crank stayed resolved with their notifies unflushed. The restart that commit recommends resumed from that state.
Also refuse run queue ingress (enqueueSend, enqueueNotify, resolvePromises) once the loop is dead, so a remote peer's delivery rolls back unacknowledged and it retries instead of trusting a black hole; bound the daemon's post-failure shutdown at 10s so a stalled kernel.stop() can't leave a pid file that blocks the next start; surface runLoop in the kernel panel and log it in the browser worker; keep a thrown non-Error as the wrapper's cause.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review found getStatus sampled the run loop status before awaiting waitForCrank, so a loop that died during that wait — the likeliest moment — was reported as running. Read it after instead. Collapse the two internal run-loop fields into one discriminated value so a failure recorded for a never-started loop is unrepresentable; name and export OnRunLoopFailure in place of four inline copies; export RunLoopStatusStruct; make the union arms type() so a client shipped against them tolerates a newer kernel adding a field; contain an async handler's rejection. Adds tests tying RunLoopStatusStruct to what getRunLoopStatus emits (they were two independent declarations of one shape, and a mismatch fails every getStatus RPC), pinning that a healthy getStatus still waits for the crank, and covering the makeKernel option passthrough via module mocking. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
PR review found two blockers. Guarding KernelQueue's mutators broke teardown: VatHandle.terminate and RemoteManager reject the promises a dead endpoint was deciding via resolvePromises, and terminateAllVats has no per-vat catch, so terminateAllVats and reset — the recovery actions a failed status invites — would throw and leave vats half-removed. The check now sits in RemoteHandle where remote deliveries actually enter, which still rolls back unacknowledged so the peer retries. Second blocker: the daemon's post-failure watchdog cleared its kill timer in a .finally, disarming it on the failed-shutdown path it exists for; a thrown kernel.stop() left live vat workers holding the event loop open with the pid file already removed, an orphan on kernel.sqlite invisible to both interlocks. Also: keep the original failure as the cause when a rollback fails, create a savepoint before recording its name so a failed create stops masking the real death reason, tolerate thenables in the failure handler, and drop type-defeating casts from the banner test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…off this A private-field call is still a member call, so this.#onRunLoopFailure(failure) handed a non-arrow handler the whole hardened kernel as its receiver — reset, terminateAllVats, queueMessage — on a boundary whose business is one Error. No escalation today, since every supplier already holds the kernel, but an unintended authority grant. Also fixes a struct test that passed for the wrong reason (remoteComms: undefined is invalid on its own account, so the missing runLoop was never what failed) and documents that rolling back the killing crank means a restart re-dequeues the same item. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…gaps Mutation testing found three surviving mutants: deleting the per-crank reset of #crankRolledBack, the savepoint-recording order in createCrankSavepoint, and RunLoopBanner's wiring into App all left the suite green. Each now has a test verified to fail without its production line. Also fixes a real hole the Cursor bot caught: when rollbackCrank's database call threw, the savepoint stayed listed, so endCrank's release committed the very crank being abandoned — persisting the half-finished state while the status reported the rollback had failed. Adds the missing coverage for the thenable containment branch, asserts teardown does its work rather than merely not throwing, and condenses the changelog entries. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
PR review found that the abort path recorded its rollback only after the call succeeded, so a throwing rollbackCrank left the flag unset and the run loop's catch asked again — against the savepoint rollbackCrank had already discarded in its own finally. The second attempt's "no such savepoint" then became the reported cause of death, and since only error.message crosses the wire, the database error that actually killed the kernel reached neither getStatus nor daemon.log. The flag now means "attempted", set in a finally, which is exactly what the crank.ts change makes correct. runLoop becomes required on KernelStatus. exactOptional left the type saying "may be absent" while validation demanded the key, and optional cannot fix that here: it widens the property to | undefined and an RPC result must satisfy Json. Required is the only self-consistent option, so get-status and RunLoopBanner stop documenting contradictory intents about an older kernel's reply. The daemon's post-failure handler moves to its own module and gets tested: 93 lines shaped around process.exit had no coverage at all, and the watchdog needed fake timers, which lockdown's frozen Date rules out (hence the mock shim). Failures now log through stringify, which keeps the cause chain that error.stack drops. Strengthens the double-start test to assert the running loop survives a refused second run(), and corrects comments that overstated what they guarded: teardown enqueues rather than drains, and a stalled kernel.stop() leaves an orphan the pid interlock can still see. Not fixed, deliberately: a crank that rolls back after flushing its buffer leaves a caller holding a fulfilled promise for work that replays. main already settled subscriptions mid-crank via resolvePromises(immediate) and already rolled back on abort, so this is pre-existing in kind; the fix is to defer subscription settlement past the commit, which is too broad for a review pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
KernelQueue's tests mock the store, so rollbackCrank is a vi.fn() and every claim about what the rollback actually does to SQLite went unverified. The new kernel-test suite exercises the real thing: the dequeued item returns to the run queue (the "a restart re-dequeues it" claim), the length cache is recomputed rather than left stale at zero, endCrank's unconditional release does not commit the crank the rollback abandoned, the connection is still writable afterwards and a later crank still commits, and a savepoint that was never created is refused. Verified by mutation: removing the cache invalidation fails 2 of 6, removing refreshRunQueue fails 1, and reverting the savepoint-forgetting fix fails 5 — the last being the consequence the unit tests cannot see, since they assert the bookkeeping rather than that leaving it listed really commits. Both socket e2e tests round-tripped getStatus while asserting only vats and subclusters, and neither transport validates results — sendCommand and sendJsonRpc are raw JSON-RPC, not RpcClient. So now that runLoop is required, a kernel that stopped emitting it would break every RpcClient consumer, the UI panel included, while both tests stayed green. They now assert a live daemon reports the loop running. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`assertRunLoopAlive` called `terminateAllVats` and `reset` "the recovery a failed status invites", but neither restarts the run loop: nothing clears a `failed` state and `run` refuses a second call, so a kernel that has failed stays failed for the life of the instance. They are cleanup. Name them as such, and say the same on `reset` itself, which is where someone looking for a way out would land. Agoric's swingset takes the same position — `panic` is never cleared and every later `run`/`step` re-throws it — so this is the intended design, not a gap. Also record why the browser worker deliberately stays up instead of closing itself. The old comment claimed it "has no exit to take", which is untrue and made the choice look forced: `self.close()` would remove the only diagnostic without buying any recovery, since nothing respawns the worker, the vat iframes belong to the offscreen document and would outlive it, and the panel keeps its last successful status when polling fails. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…abandoned `rollbackSavepoint` only reached its stack bookkeeping and `rollbackIfNeeded` after `ROLLBACK TO` returned, so a throwing rollback left the savepoint listed and the transaction open with nothing to ever commit or abort it. Every later write on the connection then joined that transaction, reported success, and vanished on close — invisible in the daemon, which exits and lets SQLite unwind it, but permanent in the browser worker, which deliberately stays up. Discard the whole transaction instead. That is no wider than the caller asked for: the transaction begins with the outermost savepoint, so it holds only the work the rollback was already abandoning, which for a crank is the same boundary. The rollback failure is still what gets thrown, even when aborting fails too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The failure log ran ahead of `setExitCode`, the watchdog and the shutdown, and the daemon's transport is `appendFileSync` — so a full disk threw, the kernel swallowed it, and the whole remediation was dropped. The daemon stayed up serving a dead kernel: the outage this handler exists to end. Logging is now best-effort and the exit code is set first. The test that appeared to cover this was tautological for the same reason: the mocked logger threw on its first call, so the shutdown was never reached and the trailing `.catch` was never exercised. Replaced with tests that let the first log succeed, and one that makes `exit` throw so the `.catch` is load-bearing. Extract `makeDaemonRunLoopWiring` for the lifecycle state `daemon-entry` owned inline. Deleting the started flag, the post-`initIdentity` check or the pre-start replay left the whole suite green, because `daemon-entry` shuts the process down as a side effect of being imported and so has no unit test at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…o the panel `bringOutYourDead` and `launchSubcluster` both queue work only the run loop consumes, and neither was refused: a peer's reap was acknowledged and never performed, and a launch spawned a worker per vat in the config before rejecting at the bootstrap message, leaking every one of them. The `#runLoopState` comment claiming every ingress point refused work was wrong, and disagreed with `assertRunLoopAlive`'s own doc; it now points there instead. Only `error.message` crossed the wire, so in a double failure the panel showed "...could not be rolled back" and the error that actually killed the kernel was unreachable from the one consumer built to report it. Add `detail` to the failed arm, rendered under the banner's headline. `RpcClient` now includes the struct failures too, so a union mismatch names the branch and key rather than only the union — the field most likely to fail on version skew is now a required union. `run` re-threw the raw value, so a non-`Error` throw was normalized separately here and in `Kernel`, leaving the embedder's handler and `getStatus` describing two distinct objects. Throw the recorded failure and drop the second copy. Deriving the internal state from the wire type caught `detail` missing from `getRunLoopStatus` at compile time. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
sirtimid
force-pushed
the
sirtimid/detect-run-loop-death
branch
from
August 5, 2026 18:19
9c37220 to
0293803
Compare
The startup abort this branch added unwound through cleanup that produced the exact orphan the rest of it exists to prevent. `kernel.stop()` was fired and forgotten, so the synchronous `kernelDatabase.close()` on the next line landed first; `stop` then threw on `recordLastActiveTime`, two steps short of `terminateAll`, and its rejection went to a bare `.catch`. `Kernel.make` starts a worker thread per persisted vat before the run loop runs, so there are live workers by the time a death can be reported — and a worker thread keeps the parent's event loop open, so `main().catch` setting `process.exitCode` never took effect. Socket gone, pid file already removed: an orphan invisible to both start-time interlocks, which is what the changelog claimed this branch fixed. Await the stop, bounded, before closing the database, then exit rather than setting a code. The bound is not optional: `stop` waits for the current crank, and a loop that died mid-crank may never end it. Extracted as `cleanUpFailedStartup` for the same reason `makeDaemonRunLoopWiring` was — `daemon-entry` shuts the process down as a side effect of being imported, so nothing in it can be tested in place. `process.exit` in `main().catch` also covers the already-running interlock, which throws after `makeKernel` has opened the database and launched every persisted vat, and does no cleanup at all. Hoist `report` to module scope so both paths log best-effort; the transport is `appendFileSync`, and a full disk must not take the cleanup with it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3af30a6. Configure here.
`main().catch` logged unguarded before the `process.exit(1)` the previous commit added, and the daemon's transport is `appendFileSync`, so a full disk threw and that exit was never reached. The process still died, but only by accident: the throw escaped to `unhandledRejection`, whose handler logs too and so threw again, and Node aborts when its own exception handler fails. Exit code 7 rather than 1, and the `exit` fingerprint — the last-ditch record #966 added — lost with it. Verified both the old path and the fix against a worker thread standing in for a vat. The four fatal handlers had the same shape for the same reason, each logging in front of its own exit. `report` already existed for exactly this and was already used on every run-loop failure path; export it as `logBestEffort` rather than write a second one, and put it in front of all five exits. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
4 tasks
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
2 participants
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 #985
When the kernel's run loop died,
Kernel.#initlogged the error and swallowed it. The loop stopped, but the daemon stayed up, the control socket kept answering, andgetStatus()kept returning the same record it returns for a healthy kernel — while everyqueueMessagepromise hung forever. A total outage, undetectable from outside the process.This makes the failure impossible to miss and impossible to mistake for health, and makes the state it leaves behind safe to restart from.
Changes
KernelQueuerecords the death in a single discriminated#runLoopState, so a failure recorded for a loop that never started can't be represented. In-flight message results reject with the killing error as theircauseinstead of hanging, and laterqueueMessagecalls reject immediately.Kernel.getStatus()reportsrunLoop: { state, error? }, read after the crank wait — an in-flight crank is exactly when the loop is likeliest to die — and skips that wait entirely when already failed, so a dead kernel can still answer.{abort: true}, which already rolled back) leftendCrank's savepoint release to commit a half-finished crank: the dequeued item gone for good, refcounts stuck, promises resolved with their notifies unflushed. Note the trade-off: the killing item is no longer consumed, so a restart re-dequeues it — integrity over "the commit carries you past it".RemoteHandleingress boundary. Because that runs inside a savepoint that rolls back without advancing the received-sequence number, the peer retries and then gives up rather than being acknowledged by a black hole. The check deliberately sits there and not on the queue's mutators, because teardown legitimately drains queue state after death — guardingresolvePromisesbroketerminateAllVatsandreset. Those are cleanup, not recovery: nothing revives a failed kernel in place.onRunLoopFailureoption (Kernel.make, threaded throughmakeKernel), called with the error so an embedder that outlives the kernel can act. Invoked off a local rather than offthis, so a non-arrow handler isn't handed the whole kernel as its receiver.kernel.stop()that hangs or throws otherwise leaves live vat workers holding the event loop open — soexitCodenever takes effect — with the socket gone and the pid file already cleaned up: an orphan onkernel.sqliteinvisible to both interlocks. Both paths out of startup get this, not just the post-startup handler: the abort atassertSurvivedStartuppreviously unwound through cleanup that firedkernel.stop()and forgot it, so the synchronousclose()on the next line won andstopthrew onrecordLastActiveTime, two steps short ofterminateAll. It now awaits the stop before closing, andmain().catchexits rather than setting a code. Extracted ascleanUpFailedStartupso it is testable at all —daemon-entryshuts the process down as a side effect of being imported. Logging in front of any of these exits is best-effort (logBestEffort, exported from the same module and already used on every run-loop path), the four fatal handlers included: the transport isappendFileSync, so a full disk would otherwise leave the exit unreached, and the process then died only because Node aborts when its own exception handler fails — code 7,exitfingerprint lost.runLoopis failed, since every other panel keeps rendering its last known contents; the browser worker logs and deliberately stays up — closing itself would remove that banner without buying any recovery, since nothing respawns it.endCranksettles itswaitForCrankwaiters even when releasing savepoints throws, so a database error can't strandgetStatus/stop/reset/clearStorage;createCrankSavepointrecords a name only once the database has the savepoint, so a failed create stops masking the real death reason.How this compares to Agoric's swingset
Swingset is the reference implementation for this, so here is where we match it and where we don't.
Same. A dead kernel stays dead: their
panic()sets a flag that is never cleared, and every laterrun()/step()re-throws it. Two tiers of failure — vat-fatal rolls back and kills the vat, kernel-fatal stops everything. And the kernel itself never exits; the host decides.Different:
run()processes what's queued and returns, and the host calls it in a loop — so a panic just throws to the caller. Ours loops forever and nobody is holding it, so there is no caller to throw to. HenceonRunLoopFailure.Testing
KernelQueuetests cover each run-loop state, in-flight rejection, post-death queueing, the crank rollback (including not rolling back twice after an abort, and reporting both failures when the rollback itself fails), and that teardown still drains after death.Kerneltests kill the loop and assert the reported status, the embedder notification, and that a loop dying during the crank wait is not reported as running.RemoteHandletests assert a refused delivery leaves the sequence number unadvanced, by retrying the same seq and confirming it isn't dropped as a duplicate.get-statustests tieRunLoopStatusStructto whatgetRunLoopStatus()actually emits — two independent declarations of one shape whose divergence would fail everygetStatusRPC. PlusendCranksettling on a release failure, themakeKerneloption passthrough, and the banner's render conditions.cleanUpFailedStartuptests assert the ordering that was the bug — the database closes only afterstop's continuation has run — plus the timeout when the kernel never stops, that the close and the pid removal still happen whenstoprejects, and that a throwing log transport doesn't take the cleanup with it.Lint, build, and the full test suite pass. Note
@ocap/kernel-test'scluster-launchandgarbage-collectiontests time out intermittently under full parallel load; I verified this reproduces onmainunchanged and is unrelated to this branch.Reviewer notes
Two things worth a second opinion: rolling back the killing crank trades availability for integrity as described above (a poison item now survives restart — swingset behaves the same way; see below), and
runLoopis optional in the TypeScript type but required on the wire, becauseexactOptionalonly permits an absent key insideobject()andKernelStatusStructis atype(). Pre-existing forremoteComms; documented rather than changed.One pre-existing gap left alone: the already-running interlock throws after
makeKernelhas opened the database and launched a worker thread for every persisted vat, briefly running the live daemon's vats a second time, and it does no cleanup.main().catchnow exiting bounds that rather than fixing it — moving the check ahead ofmakeKernelis the real fix and is out of scope here.🤖 Generated with Claude Code
Note
High Risk
Touches core kernel run-loop semantics, persistent SQLite transactions, daemon process lifecycle, and a breaking required
runLoopfield ongetStatusfor all RPC clients.Overview
Fixes the case where a dead kernel run loop still looked healthy:
getStatusnow requiresrunLoop(idle/running/failedwitherrorand fulldetailchain), in-flightqueueMessageresults reject, new work is refused at ingress (messages, remote deliveries,launchSubcluster), and the killing crank is rolled back so restart is consistent.Adds
onRunLoopFailurethroughKernel.make/makeKernel: the daemon logs, shuts down within 10s, removes the pid file, andprocess.exit(1)so vat workers cannot keep a zombie process; the browser worker logs and stays up so status polling can show failure. Daemon startup usesmakeDaemonRunLoopWiring,cleanUpFailedStartup, and best-effort logging on fatal paths.Supporting fixes:
rollbackSavepointabandons the transaction ifROLLBACK TOfails (node + wasm SQLite); crank helpersendCrank/rollbackCrank/createCrankSavepointstay consistent on DB errors;RpcClientvalidation errors include union branch details; kernel UI shows a run loop failure banner; e2e tests assertrunLoop: { state: 'running' }on live daemons.Reviewed by Cursor Bugbot for commit b5c129d. Bugbot is set up for automated code reviews on this repo. Configure here.