feat(ocap-kernel): launch subcluster vats in parallel - #983
Conversation
Previously #launchVatsForSubcluster awaited each vat's initVat
handshake serially, making startup O(sum of vat init times).
Now all vats launch concurrently via Promise.all. Each vat gets a
kernel promise pre-allocated for its root object; the bootstrap
message is queued immediately targeting the bootstrap vat's
(unresolved) promise. KernelRouter parks the send until that promise
resolves. As each vat's handshake completes its root promise is
resolved via resolvePromises('kernel', ...), making startup O(max).
The bootstrap vat receives kernel promises for all other vats' roots
in its bootstrap() call, so it can pipeline calls to those vats while
they are still initializing.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Benchmark: serial vs parallel subcluster startupMeasured on real worker processes (
Serial scaled linearly (~865 ms × N). Parallel is bounded by the slowest vat; the ~120 ms overhead above 1-vat for the 5-vat case is kernel promise allocation and resolution bookkeeping. Raw numbersSerial (HEAD^) Parallel (this PR) |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…nchVatsForSubcluster Both loops iterate vatEntries identically. Building roots[vatName] from the locally-scoped kpid in the same pass eliminates the second loop, the ! non-null assertion, and the eslint-disable comment that accompanied it. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ise.all result Instead of mutating an outer variable via a .then() side-effect, collect the resolved root refs as the return value of Promise.all and extract the bootstrap vat's ref by index. Promise.all preserves insertion order, so the index is stable. config.bootstrap is guaranteed present (validated before this method is called), so the as-KRef cast is correct. Also removes the dead-code guard that followed — if Promise.all resolves, every vat (including bootstrap) launched successfully, so rootRefs[idx] is always defined. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
When a vat in a multi-vat subcluster fails to launch, its pre-allocated kernel promise was left unresolved. Any bootstrap message pipelined through that promise (E(roots.failingPeer).method()) would park forever. Add a .catch() to the launchVat chain that calls resolvePromises with rejected=true and a VAT_TERMINATED kernel error, then re-throws so the outer Promise.all still rejects and launchSubcluster surfaces the error. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…r rejection Add a peer-rejection-bootstrap vat that calls E(peer).ping() and logs whether the call resolves or rejects, making peer-vat-launch failures observable from the bootstrap vat. Add an integration test that launches a two-vat cluster where the peer vat (error-build-throw) always fails to build its root object. The test asserts that launchSubcluster rejects and that, after the kernel run loop drains, the bootstrap vat has logged a rejection message containing 'VAT_TERMINATED' — confirming that the rejected kernel promise propagates to the bootstrap vat via E() pipelining. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…omise] failures Switch #launchVatsForSubcluster from Promise.all with pre-allocated kp<N> refs to Promise.allSettled. Wait for all vats to settle, then send bootstrap with real ko<N> refs for succeeded vats and immediately-rejected kp<N> for failed peers. The previous approach sent bootstrap before any vat launched, so bootstrap received pending kp<N> refs. If bootstrap returned a value pipelined through those refs, the result CapData contained kp<N> slots, and kunser() would fail with "value is not durable: '[Promise]'" when trying to deserialize them. The new approach preserves concurrent vat launch (allSettled, not allSettled- then-serial) while ensuring bootstrap always gets concrete refs. Failed peer vats still get an immediately-rejected kernel promise so bootstrap can observe the failure via E(roots.peer).method() pipelining. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…t launch With concurrent subcluster startup (PR #983), ko<N> refs are assigned in the order vat workers respond — not the order vats are declared. Tests that hardcoded ko4/ko5/ko6 for alice/bob/carol broke because those refs are now non-deterministic. - Add `vatRootKrefs: Record<string, KRef>` to `SubclusterLaunchResult` so callers can look up the root KRef of any vat by name - persistence.test.ts: use `rootKref` from `launchSubcluster` instead of hardcoded ko4 for the multi-vat coordinator test - resume.test.ts: use `vatRootKrefs.alice/bob/carol` instead of ko4/ko5/ko6 - rejection.test.ts: sort before comparing `getVatIds()` output; Map insertion order is non-deterministic with concurrent launch Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The local Fail tag is declared as returning Error (not never) to work around a TypeScript control-flow analysis bug. Using `x ?? Fail` therefore widens the type to `T | Error`, breaking tsc on CI even though ts-jest's transpile-only mode accepts it. Replace each occurrence with an explicit undefined guard and throw. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…edVat When BOYD (bringOutYourDead) runs for an importing vat before terminateAllVats, dropImports/retireImports decrements the exported object's refcount to (0,0). cleanupTerminatedVat then tries to decrement the baseline (1,1) that was set at object creation, causing an underflow. Guard: check the current reachable count before the baseline decrement and skip it if reachable is already zero. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
…ction
vatName and service names come from ClusterConfig (user-provided). Using
a plain {} as the accumulator allowed prototype pollution if a name like
'__proto__' or 'constructor' was supplied. Object.create(null) removes
the prototype chain, eliminating the risk.
vatRootKrefs is spread into a plain {} at the return site so callers
that use toStrictEqual continue to work.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Validate that no vat name shadows Object.prototype built-ins (e.g. __proto__, constructor) before using vatName as a property key on the roots/vatRootKrefs maps. Adds // lgtm[js/remote-property-injection] suppressions on the three write sites as a belt-and-suspenders measure for CodeQL, which does not track the null-prototype path through the TypeScript cast. Object.create(null) was considered but rejected: @endo/marshal requires Object.prototype-chained objects for CopyRecord serialization. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
GitHub Code Scanning requires '// lgtm [rule]' with a space, not '// lgtm[rule]' without one. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Accumulate roots and vatRootKrefs into entry arrays and call Object.fromEntries at the end, eliminating the obj[taintedKey] = value write pattern that CodeQL's js/remote-property-injection rule flags. @endo/marshal requires Object.prototype-chained CopyRecords, so Object.create(null) was not viable. The // lgtm suppression syntax is also not active on this repo. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…e arg - Move the Object.prototype name check into the main loop body, eliminating a separate pre-pass over vatEntries. - Drop the 'vatRoot' iface arg from kslot(kpid): kslot ignores iface for kp-prefixed refs (returns makeStandinPromise early), so the arg was dead code that implied it had an effect. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Design note:
|
![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 2 potential issues.
❌ 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 6647a0c. Configure here.
| !(vatName in Object.prototype) || Fail`invalid vat name '${vatName}'`; | ||
| if (result.status === 'fulfilled') { | ||
| rootsEntries.push([vatName, kslot(result.value)]); | ||
| vatRootKrefsEntries.push([vatName, result.value]); |
There was a problem hiding this comment.
Missing vatRoot slot iface
Low Severity
Successful vat roots passed into bootstrap are built with kslot(result.value), which defaults the iface to 'undefined'. The previous serial path used kslot(rootRef, 'vatRoot'), so root remotes are now branded Alleged: undefined instead of Alleged: vatRoot.
Reviewed by Cursor Bugbot for commit 6647a0c. Configure here.
| // chance to observe and handle the failures. | ||
| if (firstPeerFailure !== undefined) { | ||
| throw firstPeerFailure; | ||
| } |
There was a problem hiding this comment.
Peer failure leaves orphaned vats
Medium Severity
When a non-bootstrap vat fails, #launchVatsForSubcluster still delivers bootstrap, then throws firstPeerFailure. The launchSubcluster catch path only destroys IO channels and deletes the subcluster record, so successfully launched vats keep running and keep persisted vat configs while losing subcluster membership.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 6647a0c. Configure here.
2 participants
Explanation
Subcluster startup was O(sum of vat init times) because
#launchVatsForSubclusterused a serialfor...ofloop — each vat'sinitVatRPC handshake had to complete before the next one began.This PR changes startup to O(max vat init time) by launching all vats concurrently via
Promise.all. The mechanism:await, one kernel promise (kp<N>) is pre-allocated per vat and marked as kernel-decided.kp<N>.KernelRouterparks the send on the promise viaenqueuePromiseMessage.initVathandshake completes, its root kernel promise is resolved viaresolvePromises('kernel', ...), and the run loop forwards any queued messages.bootstrap(roots, services)call — it can pipeline calls to peers while they are still initializing.Changes
SubclusterManager.ts— rewrote#launchVatsForSubclusterto pre-allocate kernel promises, queue bootstrap immediately, and launch all vats withPromise.all.SubclusterManager.test.ts— updated mocks (initKernelPromise,setPromiseDecider,resolvePromises) and assertions to match the new flow.Kernel.test.ts— addedresolvePromises = vi.fn()to theKernelQueuemock class.Checklist
yarn workspace @metamask/ocap-kernel test:dev:quiet)yarn workspace @metamask/ocap-kernel build)Note
Medium Risk
Changes core subcluster launch ordering, bootstrap wiring, and promise rejection paths; mistakes could break startup or bootstrap peer discovery, though coverage is broad.
Overview
Subcluster startup now launches every vat in parallel (
Promise.allSettled) instead of one-by-one, so init time tracks the slowest vat rather than the sum of all vats. Kernel service resolution runs before launches; bootstrap is queued only after all launch attempts finish.When a non-bootstrap peer fails, its entry in
bootstrap(roots, …)is a rejected kernel promise (VAT_TERMINATED) so the bootstrap vat can observe the failure via pipelined calls;launchSubclusterstill rejects afterward with the peer error.SubclusterLaunchResultgainsvatRootKrefs(per-name rootko<N>for successful vats).Integration tests stop assuming fixed
ko4/ko5/ko6order and use returned refs; a new peer-rejection integration test and bootstrap vat cover failed-peer propagation. Terminated-vat export cleanup skips baseline refcount decrement when GC already zeroed reachability.Reviewed by Cursor Bugbot for commit 6647a0c. Bugbot is set up for automated code reviews on this repo. Configure here.