Repository navigation
fix(ocap-kernel): record a vat's death in one synchronous step - #1093
Merged
Merged
Conversation
3 of 4 tasks
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c9cc6fb. Configure here.
This was referenced Sep 15, 2026
sirtimid
added this pull request to stack #1108
September 15, 2026 22:38
2 of 4 tasks
sirtimid
force-pushed
the
sirtimid/retire-vat-synchronously
branch
from
September 25, 2026 18:14
c9cc6fb to
95824dc
Compare
Contributor
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
rekmarks-consensys-1
left a comment
Contributor
There was a problem hiding this comment.
This looks good except for two essay-length docstrings, resulting from—I suspect—a common Claudish failure mode where it dumps its train of thought into documentation instead of trying to protect the reader's attention.
A vat's death is four writes: the promises it was deciding rejected, its root
unpinned, its config and store dropped, the terminated mark set. They were
spread across `stopVat` and `VatHandle.terminate` with awaits between them, so
a failure part-way left a state nothing recovers from. The sharpest is marked
terminated while `vatConfig` survives — only `deleteVat` removes that row,
while the cleanup the mark schedules sweeps `${vatId}.` keys, which never match
`vatConfig.${vatId}` — and such a vat reads as active again the moment cleanup
drops the mark.
`#retireVat` does all four with no await between them, before the worker is
touched, and the mark happens even if an earlier write throws. Killing the
worker is deliberately not part of it: that can fail, and a store that says the
vat is dead is worth more than one still waiting to find out. It runs in a
`finally`, so a record that fails cannot leave a worker running either.
`stopVat` no longer insists on a live handle when terminating, which is what
lets a vat left behind by a failed relaunch be retired at all, and `launchVat`
tears its worker down when the kernel-side registration fails.
`VatHandle.terminate` is now only the handle's own business: reject the callers
waiting on commands the worker will never answer, then close the channel.
Supersedes #1030 by @grypez.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The mark makes the vat eligible for `nextTerminatedVatCleanup`, which deletes its decider promises' c-list entries on the stated understanding that its caller has already rejected them. Marking in a `finally` therefore turned a failed rejection loop into subscribers that hang for good, and the retry that would have finished the job hit the already-terminated guard and did nothing. The mark is now simply the last step, so a failure leaves a vat that has not been retired, with its c-list intact and the step available to be tried again. `deleteVat` sits immediately before it: the two must not be separated. `stopVat` no longer lets a channel that will not close stand in for a store left half-written, and `launchVat`'s first catch stops the worker that `platformServices.launch` has already spawned — `runVat` records no handle to reach it by, so nothing else could. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Moving the store side of a death out of `VatHandle.terminate` left the one caller that had nothing else to fall back on: the catch on the handle's own drain, which is how the kernel hears that a channel has failed. Left as it was, such a vat kept its handle, its `vatConfig` row and the promises it was deciding, so the kernel went on delivering over a channel that was gone and the next boot brought it back. The handle now takes an `onStreamFailure` and does the part that is its own — rejecting the callers waiting on commands the worker will never answer, since a crank blocked on one of them is what the retirement would wait behind — and `VatManager` retires the vat exactly as a requested termination would. Between cranks, like `terminateVat`: `#retireVat` writes synchronously, so run inside an open delivery savepoint the whole death is undone by any crank that goes on to abort, while the handle it dropped stays dropped. Reports are matched against the handle that made them, since a restart puts a new handle under the same vat id, and a vat the kernel is itself stopping reports the same way. The terminated mark moves ahead of `deleteVat`, which drops the `vatConfig` row `stopVat` reads to decide there is still a vat here to retire: a mark not reached by then could never be set, and the c-list it gates never swept. Every way a retirement can now fail leaves either the mark set, or the row that lets the whole step be tried again — so the idempotence guard covers only the writes that may happen once, and `deleteVat` runs again to carry an interrupted retirement to the end. `cleanupTerminatedVat` discards the `vatConfig` row too, so a vat marked but not discarded cannot read as active again once the sweep drops its mark. `runVat` stops a worker left over from a failed handshake, which `launchVat` alone used to do and only for its own launches, and which asked both runtimes to stop a worker they had never registered. Its unconditional `markVatAsTerminated` goes: `stopVat` sets the mark early enough that asserting it afterwards could only add it where `stopVat` stopped short of `deleteVat`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…cannot say Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
sirtimid
force-pushed
the
sirtimid/retire-vat-synchronously
branch
from
October 1, 2026 08:42
95824dc to
2125267
Compare
…be retried `#stopVat` finds a handle-less vat by its `vatConfig` row, and `deleteVat` dropped that row first. A failure on the vat store or subcluster write after it left those behind for good: a retry threw `VatNotFoundError`. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
sirtimid
added a commit
that referenced
this pull request
Oct 6, 2026
#1100's change, ported onto #1098 and the stream-failure mechanism #1093 put on main. - A channel that closes without an error counts as lost, as one that fails already did: a worker that exits closes the channel, and a worker thread emits no port event at all. The Node runtime keeps an `exit` listener once the worker is online, which closes the channel, and `terminate` tolerates a worker that has already gone. - A lost vat's handle comes off the books at once, so #1098's lookup skips deliveries to it, and its death goes through the queued `terminateVat` in a crank of its own. A dead run loop gets the death recorded directly. - `Kernel.stop` closes every channel through the platform services, so it tells the manager first rather than have that read as every vat dying. - One vat that will not start no longer fails `initializeAllVats`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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
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.

A vat's death is four store writes: reject the promises it was deciding, unpin its root, drop its config and vat store, set the terminated mark. They were spread across
VatManager.stopVatandVatHandle.terminate, with awaits between them, so a crank could land in the middle and see a half-dead vat.A new private
#retireVatdoes all four in one synchronous step, before the worker is touched. The worker is torn down even if a write fails, and the caller hears about the store failure rather than a channel that would not close.Having the death in one place leads to these changes:
stopVatno longer needs a live handle to terminate. A failed relaunch leaves a vat persisted but not running.terminateVatthrewVatNotFoundErrorfor one, so it could not be retired at all, andterminateSubclusterstopped part-way on reaching one. A vat that is neither running nor persisted still throws.platformServices.launchspawns the worker before the handshake, and a failed handshake leaves no handle to stop it through.runVatnow stops it, soinitializeAllVatsandrestartVatno longer strand workers (onlylaunchVatcleaned up before). A launch that failed before a worker existed no longer asks the runtime to stop one, which logged a misleading second error.VatHandle.terminateonly does the handle's own work: reject pending commands, then close the channel.removeVatFromSubclustertolerates a vat in no subcluster, sodeleteVatcan be retried.A channel that fails on its own
The drain's catch in
VatHandleused to callterminate(true, …)and get the store teardown for free. With that moved out, the handle takes a requiredonStreamFailure. It rejects its own pending commands, andVatManager.#retireLostVatretires the vat the same way a requested termination would. Two details matter:#vats.deleteVattwice.There is nobody to retry this path, so a store failure ends in a log line saying to
terminateVatby hand."Channel fails" is narrower than it sounds. Nothing listens for a worker that dies outright (
NodeWorkerReaderregisters only'message', andPlatformServices.launchdrops'error'/'exit'once the worker is online), so the kernel only sees a worker that is still sending bad frames. Wiring up'exit'/'error'is left for later.Write order
The mark goes after the rejections and before
deleteVat:nextTerminatedVatCleanup, which deletes the decider promises' c-list entries assuming they were already rejected.stopVatrecords a vat's death before killing its worker, so a failed record strands a live worker #1067 asked for the mark in afinally; all three review agents showed that marking after failed rejections hangs those promises' subscribers for good.deleteVat, becausedeleteVatdrops thevatConfigrow that a retry uses to find the vat. With the mark last, a failure on that write left a vat that could never be marked.As a result, every failure leaves either the mark set or the row that allows a retry. The idempotence guard covers only the writes that must happen once, and
deleteVatreruns.deleteVatitself now dropsvatConfiglast, so a failure on the vat store or subcluster write leaves the row a retry needs. IfdeleteVatitself throws, the vat is marked and itsvatConfigrow survives.cleanupTerminatedVatnow discards that row, so the sweep cannot bring the vat back to life.Synchronous is not atomic.
terminateVatruns between cranks with no open transaction, so the four writes are four commits. This PR removes the interleaving and the dead ends, not the partial-failure window. Closing that window is the Tier 3 question of routing control-plane termination through the run loop.Superseding #1030
Supersedes #1030 by @grypez, which fixed the same "persisted but not running" defect with a dedicated
#retirePersistedVatbranch. This PR routes it through the one path instead. Carried over: his defect narrative, hisremoveVatFromSubclusterchange and test, and his persisted-but-not-running test cases.Changes
VatManager.#retireVatrecords a vat's death in one synchronous stepstopVattolerates a missing handle when terminating, and reports the store failure over the channel failureVatHandle.terminatekeeps only the handle's own work; a new requiredonStreamFailurereports a failed channelVatManager.#retireLostVatretires such a vat between cranks, ignoring reports from a handle it no longer keepsrunVatstops the worker a failed handshake leaves behind;launchVatdrops its own teardown and its now-redundant trailing markcleanupTerminatedVatdiscards the vat'svatConfigrowdeleteVatdrops thevatConfigrow lastKernel's in-crank termination lambda drops its now-redundant trailing markremoveVatFromSubclustertolerates a vat in no subclusterTesting
VatManager.test.ts:stopVat's synchronous prefix. They are read out of the mocks before the returned promise is awaited, so the test checks that no yield point comes between them, not just their order.deleteVatleaves it marked, and a retry finishes the job without repeating the one-time writes.VatHandle.test.tschecks that pending commands are rejected before the channel closes, using a stream whoseendnever settles. It also checks that a failed channel is reported with its commands already rejected, and that a restart leaves them alone.store/index.test.tschecks that adeleteVatfailing on the vat store keeps thevatConfigrow, and that a second call finishes.kernel-test'svat-lifecycle.test.tsadds two cases against a real kernel and SQLite store:vatConfigrecord survives termination and cleanup.NodejsPlatformServicestrigger.Each fix was mutation-checked on its own, and each mutation failed only its own cases.
Full
@metamask/ocap-kernelsuite green.@ocap/kernel-test'svat-lifecycle,subclusters,persistence,garbage-collection,resumeandorphaned-ephemeral-exowere green against a freshdistbefore the rebase.Closes #1063
Closes #1067
🤖 Generated with Claude Code