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 7 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 <noreply@anthropic.com>
`#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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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. |
…ranger The kernel-test integration test drives the brick end to end but over the in-memory `directNetwork`; it leaves one link unproven — that a peer the victim never granted anything can reach `handleRemoteMessage` at all over a real transport. This closes that gap with two real kernels and a real relay, nothing stubbed. The attacker never redeems anything: it hands the victim a well-formed ocap URL whose object id is fabricated and cannot decrypt. Redemption fails, but `remoteFor` establishes the attacker's handle to the victim before the request is even sent, and the request itself makes the victim auto-create its own remote for the attacker — so one `reapRemotes` then puts a single BOYD on the wire with no capability having crossed in either direction. The reap queue is read from the victim's database after it is shut down, both to see what the victim's own store committed rather than this test's stale cache and to prove the reap is the persisted kind that survives into the next incarnation. The victim's loop is parked before the BOYD is sent — a reap does not wake it — so it cannot eat its own reap while the remote still exists. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.What this fixes
One
bringOutYourDeadfrom a peer is enough to stop a kernel ever bootingagain, assuming only that it restarts at some point.
bringOutYourDeadis anordinary arm of the remote protocol — any connected peer can send one,
unsolicited, needing no authority beyond being able to reach the victim. The
kernel answers by scheduling a reap against the remote it came from, in the
persisted reap queue.
scheduleReapdoes not wake a parked run loop, so an idle kernel holds thatreap indefinitely, and carries it into its next incarnation. That incarnation
starts its run loop inside
Kernel.make— before an embedder can callinitRemoteComms, which is what restores remote handles — and reaps are takenahead of the run queue, so the loop's first act is to deliver one addressed to a
remote that does not exist yet:
The kernel is dead before
Kernel.makereturns, and it stays dead on every bootafter that: the crank that dies is rolled back, which restores the reap to the
queue for the next boot to dequeue and die on too. Recovery means hand-editing
reapQueueout of the database.#deliverNotify,#deliverGCActionand#deliverBringOutYourDeadall lookedtheir endpoint up bare, and an endpoint named by persisted state but absent from
the running kernel threw
VatNotFoundError(orRemote not found) from insidethe crank. A
sendalready tolerated this, rejecting its caller withENDPOINT_UNREACHABLE(#1007); none of these three has a caller to reject, so anabsent endpoint is something to skip, not a kernel fault.
Why a reap is the delivery that reaches this
Because nothing filters it. A GC action is dropped by
shouldProcessActiononcethe endpoint has no c-list entry, and a
notifyshort-circuits on the samecheck — but a reap carries no kref and is handed back with no liveness check at
all, and nothing purges the reap queue when its endpoint goes away.
The same crash is also reachable while a terminated vat awaits cleanup, which
happens one vat per crank:
deleteVattakes a terminated vat's config andsubcluster membership but leaves its c-lists and reachable flags in place until
cleanupTerminatedVatgets to it. The callers that reach this are the boot-timeorphan prune (
SubclusterManager.deleteSubcluster) and the run loop's owntermination callback, both of which mark vats terminated without draining. Not
via
terminateSubcluster, which callscollectGarbageafter eachterminateVatand so drains every pending cleanup before returning — an earlier revision of
this description claimed otherwise and was wrong.
No missing bundle required for any of it — that is the third PR in this stack.
Two halves of a skip that are not interchangeable
Converting a throw into a return inside a crank is the general hazard here: every
store write upstream of the old throw site goes from provisional to committed. So
a bare skip is wrong in two ways, each pinned by a test.
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.
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.
One more, found by Bugbot on the first cut
Guarding only the endpoint lookup left the same crash one line further on. The
run loop calls
nextTerminatedVatCleanupbetweenprocessGCActionSetselectingan action and this delivery, and cleaning a vat takes its whole c-list — so an
action selected in that crank can arrive after its entries are gone, and
krefsToErefsthrows on the unmapped kref, out of the crank, exactly as theunguarded lookup did. Nothing is owed in that case; the cleanup did 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 — a running endpoint missing a c-list entry is a real
disagreement and still throws.
No prior relationship is required
The victim need not have granted the peer anything, nor ever have heard of it.
An inbound message from an unknown peer auto-creates and persists the remote
endpoint it came from (
RemoteManager.remoteFor→establishRemote), and theinbound path gates on nothing that identifies the peer: the libp2p protocol
handler accepts any dialer, the handshake exchanges incarnation ids only, and
there is no inbound rate limit. The precondition is reachability alone.
Two properties bound the blast radius. A peer cannot crash a running kernel:
while the victim is up the handle exists and the reap delivers harmlessly, so the
damage is latent and lands on the next restart. And it is not amplifiable —
scheduleReapdedupes per endpoint, so a peer spammingbringOutYourDeadaddsone queue entry.
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
KernelRouter.test.ts— the failing repro on its own: all three deliverytypes skipped rather than thrown, both halves above, the invalid id, and the
log level, with a control asserting endpoints that are running still receive
their deliveries.
store/methods/gc.test.ts— pins that a reap outlives the vat it was scheduledfor, since nothing purges the queue.
kernel-test/src/remote-comms.test.ts— the end-to-end brick over thein-memory network: a peer's
bringOutYourDeadleaves a persisted reap thatkills two successive boots on
main, and both boots come up with this PR.kernel-node-runtime/test/e2e/remote-comms.test.ts— the same reachabilityover real libp2p, from a stranger: two real kernels and a real relay,
nothing stubbed, no ocap URL ever successfully redeemed, so the victim grants
the attacker nothing — and a single unsolicited
bringOutYourDeadstill landsa persisted reap.
@metamask/ocap-kernel,@ocap/kernel-testand@metamask/kernel-node-runtimegreen (one unrelated pre-existing flake in the e2e disconnect suite).
lintandbuildclean.Checklist