fix(ocap-kernel): keep an absent endpoint from killing the run loop - #1029
fix(ocap-kernel): keep an absent endpoint from killing the run loop#1029grypez wants to merge 6 commits into
Conversation
…loop A vat's ownership entries outlive it. `deleteVat` takes its config and subcluster membership when it is terminated, but its c-lists and reachable flags stay in the store until `cleanupTerminatedVat` gets to it — and that runs one vat per crank, so terminating a subcluster of N vats leaves a window N cranks wide in which the kernel still addresses a vat that has no handle. `#deliverNotify`, `#deliverGCAction` and `#deliverBringOutYourDead` look their endpoint up bare. In that window the lookup throws `VatNotFoundError` from inside the crank, which escapes it and kills the run loop for good. Because the crank is rolled back the item is restored to the queue, so the next boot dequeues it and dies too. Reachable as soon as a refcount for one of the vat's exports hits zero, a promise it subscribed to resolves, or a reap queued for it comes round. The remaining cases are what a skip has to get right, and neither is obvious from the delivery site alone: Releasing the kernel's own half of a GC action does not depend on the endpoint being there to be told. Skip it and a dropped export stays flagged reachable, so the same action is derived again on the next sweep — the comment already in `#deliverGCAction` says as much. `#deliverNotify` translates the resolution and its slots before it looks the endpoint up, and both translations import if needed. Committing those mints c-list rows and takes references in an endpoint that can never release them; today the throw and its rollback are the only reason they don't survive. Co-Authored-By: Claude Opus 5 <[email protected]>
`#deliverNotify`, `#deliverGCAction` and `#deliverBringOutYourDead` now tolerate an endpoint that is named by persisted state but not running, instead of throwing `VatNotFoundError` out of the crank and killing the run loop for good. `send` already did, rejecting its caller with `ENDPOINT_UNREACHABLE`; none of these three has a caller to reject, so an absent endpoint is something to skip. Two halves of a skip are not interchangeable, and getting either wrong undoes the point of the fix. A GC action still performs the kernel's own release — clearing the reachable flag, or tearing the c-list entry down. That half does not depend on the endpoint being there to be told, and an endpoint that cannot hear the action is precisely the case where a re-derived action would repeat on every sweep forever. `#deliverNotify` looks its endpoint up before translating the resolution rather than after. Both translations import if needed, so committing them mints c-list rows and takes references in an endpoint that can never release them. The throw and its rollback were the only reason they did not already survive; skipping without moving the lookup would have made them permanent. The lookup does not swallow an id that names neither a vat nor a remote. A missing vat and a missing remote are ordinary; that is corrupt state or a kernel bug, and GC actions pass `insistEndpointId` before they are ever queued. Skips are reported at warn rather than on the per-delivery trace channel, since a delivery dropped on the floor is not routine traffic and is the only trace of a vat that has quietly stopped doing anything. Co-Authored-By: Claude Opus 5 <[email protected]>
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||
The test to look at first
Parameterized over all three housekeeping deliveries. It asserts the delivery is Added in Before — The After — The other five, and why each is thereThe three above prove the kernel survives. These prove it survives correctly —
And one control that passes at both commits — |
The reachability this guard rests on, which the changelog and the JSDoc had attributed to the wrong caller. `terminateSubcluster` does not leave a terminated vat addressable: it calls `collectGarbage` after each `terminateVat`, and that drains every pending cleanup in a loop, so the c-lists are gone before the run loop sees another crank — and `shouldProcessAction` then filters out any GC action naming that vat. The reap queue is not filtered that way, and nothing purges it when a vat dies. `nextReapAction` shifts an endpoint off and hands back a `bringOutYourDead` with no liveness check at all, so a reap scheduled by `reapVats` before a vat was terminated arrives after it — through `terminateSubcluster` as readily as any other route. Reaps are taken ahead of the run queue, so it blocks everything behind it too. The window does exist for the callers that mark a vat terminated without draining — the boot-time orphan prune, and the run loop's own termination callback — which is what the corrected wording now says. Purging the queue on termination is worth doing on its own, and is tracked separately; the delivery-side guard is needed either way. Co-Authored-By: Claude Opus 5 <[email protected]>
Correction: the reachability claim above named the wrong callerAn earlier revision of this description said terminating a subcluster of N vats
while (this.#kernelStore.nextTerminatedVatCleanup()) { }So the c-lists are gone before the run loop sees another crank, and The reachability that does holdThe reap queue, which needs no window at all. So The cleanup window still exists, just not for that caller — the boot-time Follow-upThat the reap queue is never purged is arguably its own defect: even with this The changelog entry and the |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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 6b38190. Configure here.
Reported by Bugbot on #1029. Guarding only the endpoint lookup left the same crash one line further on. `processGCActionSet` selects an action while the endpoint still has a c-list entry for each of its krefs, but the run loop calls `nextTerminatedVatCleanup` between that selection and the delivery, and cleaning a vat takes its whole c-list. An action selected in that crank therefore arrives after its entries are gone, and `krefsToErefs` reports an unmapped kref by throwing — out of the crank, killing the run loop exactly as the unguarded lookup did, and re-dequeued on the next boot to kill that one too. Nothing is owed in that case: the cleanup performed the kernel's half on its way past. So the krefs are filtered to those the endpoint still holds, and an action left with none is skipped whole. The filter applies only when the endpoint is gone. Cleanup runs for a terminated vat and a terminated vat has no handle, so a running endpoint cannot be in this state; one that is missing a c-list entry is a real disagreement and still throws. Co-Authored-By: Claude Opus 5 <[email protected]>
The issue demonstrated on
|
6996006 to
c603cc4
Compare
Replaced the demonstration; the earlier one was contrivedThe previous version scheduled a reap and then immediately terminated a vat. In its place,
On Two things the test asserts rather than assumesBoth are races I hit while building it, and either one silently turns the test
Honest scoping
Three earlier reachability claims in this PR were wrong and have been corrected
The changelog and the |
`bringOutYourDead` is an ordinary arm of the remote protocol; any peer can
send one, unsolicited, and it needs no authority beyond being connected.
The kernel answers by scheduling a reap against the remote it came from,
in the persisted reap queue.
`scheduleReap` does not wake a parked run loop, so an idle kernel holds
that reap indefinitely, and carries it into its next incarnation. That
incarnation starts its run loop inside `Kernel.make` — before an embedder
can call `initRemoteComms`, which is what restores remote handles — and
reaps are taken ahead of the run queue, so the first thing the loop does
is deliver one addressed to a remote that does not exist yet.
So one message from a peer is enough to stop a kernel ever booting again,
assuming only that it restarts at some point. On main:
expected [ { state: 'failed', ... }, ... ] to strictly equal
[ { state: 'running' }, ... ]
Error: Remote not found: r1
at RemoteManager.getRemote → #getEndpoint → #deliverBringOutYourDead
→ deliver → #runLoop → KernelQueue.run
at #init (Kernel.ts) → Kernel.make
Both boots are asserted together so the failure reports both, because the
point is that the second is no better than the first: the crank that dies
is rolled back, which puts the reap back on the queue.
Two things the test pins rather than assumes. The peer is given local work
so its own loop cranks and sends the request, and nothing touches the
victim afterwards — a victim that cranks eats its own reap while the
remote still exists, and the scenario evaporates. And the reap queue is
read from the database after shutdown, so a run that loses that race fails
as a setup failure rather than passing.
Only the victim needs a file-backed database, since only it restarts.
Co-Authored-By: Claude Opus 5 <[email protected]>
c603cc4 to
5d33bc3
Compare
Reframed: one message from a peer, and the kernel never boots againThe test is now
On Both boots are asserted together on purpose: the second being no better than the With this PR both boots come up, and the log shows the guard on that path: Bounds of the claim
What the test pins rather than assumesBoth are races that would otherwise turn it green for the wrong reason:
Only the victim needs a file-backed database, since only it restarts. |
1 participant
First of three. Splits #1025, which carried all three of these; each is
reachable on
mainon its own, so they are worth reviewing separately.The defect
A vat's ownership entries outlive it.
deleteVattakes a terminated vat'sconfig and subcluster membership, but its c-lists and reachable flags stay in
the store until
cleanupTerminatedVatgets to it — and that runs one vat percrank, off the top of the run loop. Terminate a subcluster of N vats and you
get a window N cranks wide in which the kernel still addresses a vat that has no
handle.
#deliverNotify,#deliverGCActionand#deliverBringOutYourDeadlook theirendpoint up bare. In that window the lookup throws
VatNotFoundErrorfrominside the crank, which escapes it and kills the run loop for good. Because the
crank is rolled back the item is restored to the queue, so the next boot
dequeues it and dies too.
Reached by a peer's routine remote GC — demonstrated end to end. A kernel
answers a peer's
bringOutYourDeadby scheduling a reap against the remote itcame from. That queue is persisted, and
scheduleReapdoes not wake a parked runloop, so on an otherwise idle kernel the reap simply sits there. Shut that kernel
down and it carries the reap into the next incarnation — which starts its run
loop inside
Kernel.make, before an embedder can callinitRemoteComms, whichis what restores remote handles. Reaps are taken ahead of the run queue, so the
loop's first act is to deliver one addressed to a remote that does not exist yet:
Dead before
Kernel.makereturns, and it stays dead: the crank is rolled back,so the reap goes back on the queue to kill the next boot too. Covered by
'survives a peer reap that outlived the incarnation it was queued in'inkernel-test/src/remote-comms.test.ts, in the two-kernel harness alongside#1028's revocation test.
A reap is the delivery that reaches this most easily, because nothing filters
it. A GC action is dropped by
shouldProcessActiononce the endpoint has noc-list entry, and a
notifyshort-circuits on the same check — but a reapcarries no kref and is handed back with no liveness check at all, and nothing
purges the queue when its endpoint goes away.
Also reachable while a terminated vat awaits cleanup, which happens one vat
per crank, since
deleteVattakes its config and subcluster membership butleaves its c-lists and reachable flags in place. The callers that reach this are
the boot-time orphan prune (
SubclusterManager.deleteSubcluster) and the runloop's own termination callback, both of which mark vats terminated without
draining.
Not via
terminateSubcluster, which callscollectGarbageafter eachterminateVatand so drains every pending cleanup before returning — an earlierrevision of this description claimed otherwise and was wrong.
No missing bundle required for any of it — that is the third PR in this stack.
sendalready tolerated this, rejecting its caller withENDPOINT_UNREACHABLE(#1007). None of these three has a caller to reject, so an absent endpoint is
something to skip.
Two halves of a skip that are not interchangeable
A GC action still performs the kernel's own release. #1020 moved
clearReachableFlag/deleteCListEntryinto the delivery path, with a commentnoting that otherwise "the same action gets derived again". That half does not
depend on the endpoint being there to be told, and an endpoint that cannot hear
the action is precisely the case where a re-derived action would repeat on every
sweep forever. So it runs whether or not the delivery does.
#deliverNotifylooks its endpoint up before translating the resolution.Both translations import if needed, so committing them mints c-list rows and
takes references in an endpoint that can never release them. The throw and its
rollback were the only reason they did not already survive; skipping without
moving the lookup would have made them permanent. This is the general hazard in
converting a throw into a return inside a crank — every store write upstream of
the old throw site goes from provisional to committed.
Also: the lookup does not swallow an id that names neither a vat nor a remote.
That is corrupt state or a kernel bug, and GC actions pass
insistEndpointIdbefore they are ever queued. Skips are reported at
warnrather than on the@@@@per-delivery trace channel, since a dropped delivery is the only trace ofa vat that has quietly stopped doing anything.
Relationship to #1023
#1023 reaches the same conclusion independently, via
#resolveEndpoint, and itsversion discriminates "gone for good" from "in flux" rather than narrowing on id
shape — which is better. If #1023 lands first this PR should be reduced to
whatever it does not already cover; on my read that is the kernel-side release
on a skipped GC action, and the invalid-endpoint-id case. Flagging rather than
resolving it here, since the two were developed in parallel.
Testing
First commit is the failing repro on its own: 8 tests, covering all three
delivery types, both halves above, the invalid id, and the log level. A control
asserts endpoints that are running still receive their deliveries.
@metamask/ocap-kerneland@ocap/kernel-testgreen, withauditRefCountsonfor every kernel
kernel-testbuilds.lintandbuildclean.Checklist
Note
Medium Risk
Changes core run-queue delivery and GC/notify side effects; mistakes could affect refcount or GC behavior, though behavior is heavily specified in new tests including reboot scenarios.
Overview
Fixes a permanent run-loop death when housekeeping work (
notify, GC actions, orbringOutYourDead) targets an endpoint that persisted state names but is not running yet (terminated vat awaiting cleanup, or a remote not restored until afterKernel.make).KernelRouternow resolves those deliveries via#getEndpointIfRunning: missing vat/remote endpoints are skipped with a warn instead of throwing inside the crank (which rolled back and re-queued the same item on every boot). Invalid endpoint ids that are neither vat nor remote still throw.Skipped GC still runs the kernel’s half (clear reachable / delete c-list) when entries exist; skipped notify checks the endpoint before cap-data translation so skipped deliveries do not mint c-list rows. GC actions whose c-list was already torn down in the same crank are skipped without calling
krefsToErefs.Tests add
KernelRouterunit coverage, a store case for reaps after vat termination, and an integration test that a peer’s persisted reap no longer bricks kernel restart.Reviewed by Cursor Bugbot for commit 5d33bc3. Bugbot is set up for automated code reviews on this repo. Configure here.