Skip to content

Commit baacbe0

Browse files
sirtimidclaude
andcommitted
fix(ocap-kernel): free retired exports and harden GC delivery
Follow-up to the c-list accounting fix, addressing defects found in review. An owner that stops naming its own export left the object behind. Both the delivered `retireExport` and the `retireExports`/`abandonExports` syscalls tore down the owner's c-list entry but left `owner` and `refCount` in place, with no path that could ever reclaim them: `cleanupTerminatedVat` finds krefs by walking the owner's c-list, and the collector only revisits krefs in `maybeFreeKrefs`. The records leaked, and the next collection to visit such a kref read the owner's deleted entry through `getRequired` and took the run loop down with it. New `orphanKernelObject` drops the owner mapping and hands the object to the collector, which already knows how to retire an orphan. `collectGarbage` also treats an owner with no c-list entry as orphaned rather than trusting the mapping. Reporting a dead run loop belongs to #1005, which landed on main first. It is what makes the audit usable at all: `assertRefCountsIfAuditing` throws from inside a crank, so with the failure logged and swallowed a violation's sole symptom was a test hanging to its timeout with no mention of reference counts. The `kernel-test` case here asserts that shape — the caller is told the run loop died, and the audit error rides along as the `cause`. Also: GC action delivery survives a vanished endpoint or a failed delivery instead of stopping the loop; `launchVat` tears down a worker whose kernel-side registration failed rather than stranding it; `RefCountViolation` discriminates on `kind` instead of sentinel-matching `stored`; and the store context's auditing flag no longer shares a name with `auditRefCounts()`. Tests cover the crash path, the orphan-and-collect sequence, retiring stragglers, GC-action robustness, and that a violation reaches a caller. The `item.target` charge and both `deliver|notify` early returns now have assertions that fail if the fix is reverted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 7e61f1c commit baacbe0

19 files changed

Lines changed: 485 additions & 86 deletions

packages/kernel-test/src/garbage-collection.test.ts

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -238,17 +238,30 @@ describe('Garbage Collection', () => {
238238
});
239239

240240
/**
241-
* Give an importer a chance to notice a dropped object and tell the kernel.
241+
* Give an importer a chance to notice a dropped object and tell the kernel,
242+
* then keep cranking until the resulting GC actions have all been consumed.
242243
*
243244
* @param vatId - The vat to reap.
244245
* @param rootKRef - That vat's root, to poke with cranks afterwards.
245246
*/
246247
async function reapAndSettle(vatId: VatId, rootKRef: KRef): Promise<void> {
247248
kernel.reapVats((id) => id === vatId);
248-
for (let i = 0; i < 3; i++) {
249+
// BOYD has to reach the vat, the vat has to answer, and the kernel has to
250+
// act on the answer — but a round can queue more work, so loop until the
251+
// queue is actually empty rather than guessing at a crank count.
252+
const maxRounds = 10;
253+
for (let round = 0; round < maxRounds; round++) {
249254
await kernel.queueMessage(rootKRef, 'noop', []);
250255
await waitUntilQuiescent(500);
256+
if ([...kernelStore.getGCActions()].length === 0) {
257+
return;
258+
}
251259
}
260+
throw Error(
261+
`GC actions still pending after ${maxRounds} rounds: ${[
262+
...kernelStore.getGCActions(),
263+
].join(', ')}`,
264+
);
252265
}
253266

254267
it('survives until both importers let go', async () => {
Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
import { makeSQLKernelDatabase } from '@metamask/kernel-store/sqlite/nodejs';
2+
import { makeKernelStore } from '@metamask/ocap-kernel';
3+
import type { KRef, VatId } from '@metamask/ocap-kernel';
4+
import { expect, describe, it } from 'vitest';
5+
6+
import {
7+
getBundleSpec,
8+
makeKernel,
9+
makeMockLogger,
10+
runTestVats,
11+
} from './utils.ts';
12+
13+
/**
14+
* The per-crank audit throws from inside the run loop, which nothing restarts.
15+
* Unless that failure is reported to whoever is waiting on the kernel, the only
16+
* symptom is a test that hangs until its timeout, with no mention of reference
17+
* counts anywhere — which would make the audit worthless as a build gate.
18+
*/
19+
describe('reference count audit', () => {
20+
it('reports a violation to kernel callers rather than hanging', async () => {
21+
const kernelDatabase = await makeSQLKernelDatabase({
22+
dbFilename: ':memory:',
23+
});
24+
const kernelStore = makeKernelStore(kernelDatabase);
25+
const kernel = await makeKernel(kernelDatabase, true, makeMockLogger());
26+
await runTestVats(kernel, {
27+
bootstrap: 'exporter',
28+
forceReset: true,
29+
vats: {
30+
exporter: {
31+
bundleSpec: getBundleSpec('exporter-vat'),
32+
parameters: { name: 'Exporter' },
33+
},
34+
},
35+
});
36+
37+
const exporterVatId = kernel.getVats()[0]?.id as VatId;
38+
const exporterKRef = kernelStore.getRootObject(exporterVatId) as KRef;
39+
40+
kernelStore.setObjectRefCount(exporterKRef, {
41+
reachable: 7,
42+
recognizable: 9,
43+
});
44+
45+
// The crank carrying this message settles its result before the
46+
// end-of-crank audit runs, so this one may still succeed.
47+
await kernel
48+
.queueMessage(exporterKRef, 'createObject', ['x'])
49+
.catch(() => undefined);
50+
51+
// What a caller is told directly is that the run loop is gone; the audit
52+
// failure that killed it rides along as the `cause`. That chain is the part
53+
// that has to survive, since "run loop died" on its own names nothing.
54+
const failure = (await kernel
55+
.queueMessage(exporterKRef, 'createObject', ['y'])
56+
.catch((error) => error)) as Error;
57+
58+
expect(failure.message).toMatch(/Kernel run loop died/u);
59+
expect(String(failure.cause)).toMatch(
60+
/reference count invariant violated/u,
61+
);
62+
}, 30000);
63+
});

packages/ocap-kernel/CHANGELOG.md

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,8 +38,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
3838

3939
- Reference-count auditing: `auditRefCounts`, `recomputeRefCounts`, `formatRefCountViolations`, `assertRefCountsIfAuditing`, and `setRefCountAuditing` on the kernel store, plus a `Kernel.make` option `auditRefCounts` that verifies every kref's counts against the references the kernel actually holds at the end of each crank ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010))
4040
- Reports drift in both directions: counts too low (a live capability can be collected) and counts too high with no holder (a leak)
41-
- Exports the `RefCountViolation` type
41+
- Exports the `RefCountViolation` type, a union over `kind: 'mismatch' | 'dangling'`
4242
- Add `setReachableFlag` to the kernel store, the counterpart to `clearReachableFlag` ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010))
43+
- Add `orphanKernelObject` to the kernel store, which drops an object's owner mapping and hands it to the collector ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010))
4344

4445
### Changed
4546

@@ -79,6 +80,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
7980
- A root is addressable while its vat lives whether or not anyone imports it; the old `(1, 1)` birth baseline was standing in for this
8081
- Garbage-collection action delivery now moves the kernel's own c-list: `dropExports` clears the owner's reachable flag and `retireExports`/`retireImports` tear the entry down ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010))
8182
- Previously the owner's flag never cleared, so the same action could be re-derived, and retired entries outlived the objects they named
83+
- Orphan a kernel object when its owner stops naming it (a delivered `retireExport`, or a `retireExports`/`abandonExports` syscall), so its `owner` and `refCount` records no longer outlive the c-list entry they were reachable through — they leaked, and the next collection to visit such a kref killed the run loop ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010))
84+
- `queueMessage` now rejects with the error that stopped the run loop, and messages already in flight are rejected rather than left pending forever ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010))
85+
- Garbage-collection action delivery survives a vanished endpoint or a failed delivery instead of stopping the run loop ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010))
86+
- Tear down a vat whose worker launched but whose kernel-side registration then failed ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010))
8287
- Charge a delivered message's target reference against the run-queue item's own target rather than the routed target, so a message routed through a resolved promise no longer decrements an object nobody charged while leaking the promise ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010))
8388
- Release a queued notification's reference before the paths that decide there is nothing to deliver, and stop decrementing references on promises retired alongside it that nobody had taken ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010))
8489
- Transfer, rather than duplicate, the references a message carries when it is queued on an unresolved promise and later re-enqueued on resolution ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010))

packages/ocap-kernel/src/KernelRouter.test.ts

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@ describe('KernelRouter', () => {
6565
clearReachableFlag: vi.fn(),
6666
deleteCListEntry: vi.fn(),
6767
forgetKref: vi.fn(),
68+
orphanKernelObject: vi.fn(),
6869
createCrankSavepoint: vi.fn(),
6970
} as unknown as KernelStore;
7071

@@ -317,6 +318,38 @@ describe('KernelRouter', () => {
317318
]);
318319
});
319320

321+
it('charges the promise, not the object it resolved to', async () => {
322+
const promiseId = 'kp123';
323+
const resolvedObject = 'ko456';
324+
(
325+
kernelStore.getKernelPromise as unknown as MockInstance
326+
).mockReturnValueOnce({
327+
state: 'fulfilled',
328+
value: { body: '#"$0"', slots: [resolvedObject] },
329+
});
330+
(kernelStore.getOwner as unknown as MockInstance).mockReturnValue('v1');
331+
332+
await kernelRouter.deliver({
333+
type: 'send',
334+
target: promiseId,
335+
message: {
336+
methargs: { body: 'method args', slots: [] },
337+
result: null,
338+
},
339+
});
340+
341+
// The run queue item was charged against the promise it named, so that
342+
// is what has to be released — not whatever routing resolved it to.
343+
expect(kernelStore.decrementRefCount).toHaveBeenCalledWith(
344+
promiseId,
345+
'deliver|send|target',
346+
);
347+
expect(kernelStore.decrementRefCount).not.toHaveBeenCalledWith(
348+
resolvedObject,
349+
'deliver|send|target',
350+
);
351+
});
352+
320353
it('splats message when promise resolves to a non-object', async () => {
321354
// Setup a fulfilled promise that doesn't resolve to an object
322355
const promiseId = 'kp123';
@@ -580,6 +613,12 @@ describe('KernelRouter', () => {
580613
// Verify no notification was delivered to the vat
581614
expect(endpointHandle.deliverNotify).not.toHaveBeenCalled();
582615
expect(result).toStrictEqual({ didDelivery: endpointId });
616+
// Nothing was delivered, but the queued notification is gone either
617+
// way, so its reference has to be released on this path too.
618+
expect(kernelStore.decrementRefCount).toHaveBeenCalledWith(
619+
kpid,
620+
'deliver|notify',
621+
);
583622
});
584623

585624
it('returns didDelivery when no kpids to retire', async () => {
@@ -618,6 +657,10 @@ describe('KernelRouter', () => {
618657
// Verify no notification was delivered to the vat
619658
expect(endpointHandle.deliverNotify).not.toHaveBeenCalled();
620659
expect(result).toStrictEqual({ didDelivery: endpointId });
660+
expect(kernelStore.decrementRefCount).toHaveBeenCalledWith(
661+
kpid,
662+
'deliver|notify',
663+
);
621664
});
622665

623666
it('throws if notification is for an unresolved promise', async () => {
@@ -715,6 +758,60 @@ describe('KernelRouter', () => {
715758
]);
716759
},
717760
);
761+
762+
it('orphans the object when delivering retireExports', async () => {
763+
await kernelRouter.deliver({
764+
type: 'retireExports',
765+
endpointId: 'v1',
766+
krefs: ['ko1', 'ko2'],
767+
});
768+
769+
// The owner has given up the last name for the object, so the kernel's
770+
// record of who owns it must go too or it outlives every reference.
771+
expect(
772+
(kernelStore.orphanKernelObject as unknown as MockInstance).mock
773+
.calls,
774+
).toStrictEqual([['ko1'], ['ko2']]);
775+
});
776+
777+
it('leaves ownership alone when delivering retireImports', async () => {
778+
await kernelRouter.deliver({
779+
type: 'retireImports',
780+
endpointId: 'v1',
781+
krefs: ['ko1'],
782+
});
783+
784+
expect(kernelStore.orphanKernelObject).not.toHaveBeenCalled();
785+
});
786+
787+
it('skips the action when the endpoint has vanished', async () => {
788+
getEndpoint.mockImplementationOnce(() => {
789+
throw Error('vat v1 not found');
790+
});
791+
792+
const result = await kernelRouter.deliver({
793+
type: 'retireImports',
794+
endpointId: 'v1',
795+
krefs: ['ko1'],
796+
});
797+
798+
expect(result).toStrictEqual({ didDelivery: 'v1' });
799+
expect(kernelStore.deleteCListEntry).not.toHaveBeenCalled();
800+
});
801+
802+
it('survives a failed delivery', async () => {
803+
(
804+
endpointHandle.deliverRetireImports as unknown as MockInstance
805+
).mockRejectedValueOnce(Error('endpoint went away mid-delivery'));
806+
807+
const result = await kernelRouter.deliver({
808+
type: 'retireImports',
809+
endpointId: 'v1',
810+
krefs: ['ko1'],
811+
});
812+
813+
expect(result).toStrictEqual({ didDelivery: 'v1' });
814+
});
718815
});
719816

720817
describe('bringOutYourDead', () => {

packages/ocap-kernel/src/KernelRouter.ts

Lines changed: 43 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -405,10 +405,12 @@ export class KernelRouter {
405405
this.#kernelStore.translateCapDataKtoE(endpointId, tPromise.value),
406406
]);
407407
}
408-
// TODO(#1006 follow-up): SwingSet also tears down the c-list entry for each
409-
// promise in the batch here, since the endpoint can never refer to a
410-
// settled promise by that eref again. Left alone for now because the
411-
// debug UI discovers exported ocap URLs by scanning these entries.
408+
// TODO: SwingSet also tears down the c-list entry for each promise in the
409+
// batch here, since the endpoint can never refer to a settled promise by
410+
// that eref again. Left alone for now because the debug UI discovers
411+
// exported ocap URLs by scanning these entries. The cost of keeping them is
412+
// that a settled promise reached this way holds a count forever, so it is
413+
// never collected and its resolution slots are never released.
412414
const endpoint = this.#getEndpoint(endpointId);
413415
return await endpoint.deliverNotify(resolutions);
414416
}
@@ -424,29 +426,57 @@ export class KernelRouter {
424426
this.#logger?.log(
425427
`@@@@ deliver ${endpointId} ${type} ${JSON.stringify(krefs)}`,
426428
);
427-
const endpoint = this.#getEndpoint(endpointId);
429+
let endpoint: EndpointHandle;
430+
try {
431+
endpoint = this.#getEndpoint(endpointId);
432+
} catch (error) {
433+
// The endpoint was selected for this action while its c-list still
434+
// existed, but it has since gone away (terminated, and cleaned up in the
435+
// same crank). Nothing left to tell; its c-list goes with it.
436+
this.#logger?.error(
437+
`Skipping ${type} for vanished endpoint ${endpointId}:`,
438+
error,
439+
);
440+
return { didDelivery: endpointId };
441+
}
428442
const erefs = this.#kernelStore.krefsToErefs(endpointId, krefs);
429443
// Telling an endpoint to let go is also the kernel letting go. Otherwise a
430444
// dropped export stays flagged reachable, so the same action gets derived
431445
// again, and retired entries outlive the objects they name.
432446
krefs.forEach((kref, index) => {
433447
if (type === 'dropExports') {
434448
this.#kernelStore.clearReachableFlag(endpointId, kref);
435-
} else {
436-
this.#kernelStore.deleteCListEntry(
437-
endpointId,
438-
kref,
439-
erefs[index] as ERef,
440-
);
449+
return;
450+
}
451+
// `erefs` is parallel to `krefs`: krefsToErefs throws rather than
452+
// returning a short array, so every index is populated.
453+
this.#kernelStore.deleteCListEntry(
454+
endpointId,
455+
kref,
456+
erefs[index] as ERef,
457+
);
458+
if (type === 'retireExports') {
459+
// Retiring an export is the owner giving up the last name for the
460+
// object, so the kernel's record of who owns it goes too.
461+
this.#kernelStore.orphanKernelObject(kref);
441462
}
442463
});
443464
const method =
444465
`deliver${(type[0] as string).toUpperCase()}${type.slice(1)}` as
445466
| 'deliverDropExports'
446467
| 'deliverRetireExports'
447468
| 'deliverRetireImports';
448-
const crankResult = await endpoint[method](erefs);
449-
return crankResult;
469+
try {
470+
return await endpoint[method](erefs);
471+
} catch (error) {
472+
// The kernel has already let go above, which is the part that matters for
473+
// accounting. Don't let a failed notification take down the run loop.
474+
this.#logger?.error(
475+
`Delivery of ${type} to ${endpointId} failed:`,
476+
error,
477+
);
478+
return { didDelivery: endpointId };
479+
}
450480
}
451481

452482
/**

packages/ocap-kernel/src/garbage-collection/gc-handlers.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,5 +84,9 @@ export function performExportCleanup(
8484
}
8585
}
8686
kernelStore.forgetKref(endpointId, kref);
87+
// The owner no longer names the object, so nothing can reach it through
88+
// this endpoint again. Drop the owner mapping too, or the kernel's record
89+
// of the object outlives the only c-list entry it was reachable through.
90+
kernelStore.orphanKernelObject(kref);
8791
}
8892
}

packages/ocap-kernel/src/store/index.test.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -149,6 +149,7 @@ describe('kernel store', () => {
149149
'markVatAsTerminated',
150150
'nextReapAction',
151151
'nextTerminatedVatCleanup',
152+
'orphanKernelObject',
152153
'pinObject',
153154
'provideIncarnationId',
154155
'recomputeRefCounts',

packages/ocap-kernel/src/store/index.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -154,7 +154,7 @@ export function makeKernelStore(kdb: KernelDatabase, logger?: Logger) {
154154
subclusters: provideCachedStoredValue('subclusters', '[]'),
155155
nextSubclusterId: provideCachedStoredValue('nextSubclusterId', '1'),
156156
vatToSubclusterMap: provideCachedStoredValue('vatToSubclusterMap', '{}'),
157-
auditRefCounts: false,
157+
refCountAuditingEnabled: false,
158158
// Logging
159159
logger: logger?.subLogger({ tags: ['kernel-store'] }),
160160
};

0 commit comments

Comments
 (0)