Skip to content

Commit 6618411

Browse files
sirtimidclaude
andcommitted
test(ocap-kernel): pin each refcount audit credit source to a literal
The clean-audit cases prove each rule agrees with whatever the store did, which stays true if a rule and the code it mirrors are wrong by the same constant. Six of eight rules could have drifted and the suite would have stayed green. Each of the ten credit sources now pins its count and holder labels to literals and asserts drift in both directions: too low collects a live capability, too high leaks it. That closes the two coverage gaps as a side effect — a run-queue send's result promise, and a message parked on an unresolved promise, neither of which any test reached. Also states what the audit can and cannot find, which matters because its ground truth *is* the holder set: a count that disagrees with its holders is caught either way, but a holder that should have been torn down and wasn't justifies its own count at any value, so a leaked reference is invisible to it by construction. That is exactly the case the retained settled-promise c-list entry leaves behind, so the CHANGELOG no longer claims the audit would catch it. The `auditRefCounts` JSDoc no longer scopes the option as "intended for tests and debugging": it stands in for the invariant `collectGarbage` cannot assert, and is off by default only because it walks the whole store. `kernel-test`'s audit test asserts both hops of the failure now. #1005 changed the shape: callers are told the run loop died and the audit error rides along as the `cause`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 991de29 commit 6618411

5 files changed

Lines changed: 217 additions & 7 deletions

File tree

packages/kernel-test/src/refcount-audit.test.ts

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -48,8 +48,16 @@ describe('reference count audit', () => {
4848
.queueMessage(exporterKRef, 'createObject', ['x'])
4949
.catch(() => undefined);
5050

51-
await expect(
52-
kernel.queueMessage(exporterKRef, 'createObject', ['y']),
53-
).rejects.toThrow(/reference count invariant violated/u);
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+
);
5462
}, 30000);
5563
});

packages/ocap-kernel/CHANGELOG.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
3434
- Caveat: a legitimate string argument that begins with `@@` followed by alphanumerics will be misinterpreted as a marker; wrap such literals inside an object
3535

3636
- 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))
37-
- Reports drift in both directions: counts too low (a live capability can be collected) and counts too high with no holder (a leak)
37+
- Reports drift in both directions: counts too low (a live capability can be collected) and counts too high with no holder (an orphaned count). It compares counts against the holders it finds, so a holder that should have been torn down but wasn't justifies its own count and is not detectable this way
3838
- Exports the `RefCountViolation` type, a union over `kind: 'mismatch' | 'dangling'`
3939
- Add `setReachableFlag` to the kernel store, the counterpart to `clearReachableFlag` ([#1010](https://github.com/MetaMask/ocap-kernel/pull/1010))
4040
- 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))

packages/ocap-kernel/src/Kernel.ts

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -110,8 +110,12 @@ export class Kernel {
110110
* @param options.allowedGlobalNames - Optional list of allowed global names for vat endowments.
111111
* @param options.auditRefCounts - If true, verify every kref's reference
112112
* counts against the references the kernel actually holds at the end of each
113-
* crank, and throw on any mismatch. Intended for tests and debugging; the
114-
* audit walks the whole store.
113+
* crank, and throw on any mismatch. This is the check standing in for the
114+
* accounting invariant `collectGarbage` still cannot assert (see the comment
115+
* on its `retireExport` branch), so it is not optional
116+
* instrumentation: it is off by default only because it walks the whole store
117+
* every crank. Any kernel whose accounting is under test wants it on, and
118+
* every kernel `kernel-test` builds enables it.
115119
* @param options.onRunLoopFailure - Optional handler called if the run loop dies.
116120
*/
117121
// eslint-disable-next-line no-restricted-syntax

packages/ocap-kernel/src/store/methods/refcount-audit.test.ts

Lines changed: 190 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import type { KernelDatabase } from '@metamask/kernel-store';
12
import { describe, it, expect, beforeEach } from 'vitest';
23

34
import { makeMapKernelDatabase } from '../../../test/storage.ts';
@@ -6,6 +7,7 @@ import { makeKernelStore } from '../index.ts';
67

78
describe('reference count audit', () => {
89
let kernelStore: ReturnType<typeof makeKernelStore>;
10+
let kdb: KernelDatabase;
911

1012
/**
1113
* Register and initialize an endpoint so it can hold c-list entries.
@@ -19,8 +21,35 @@ describe('reference count audit', () => {
1921
}
2022
}
2123

24+
/**
25+
* Overwrite a kref's stored count, going around the store's own arithmetic so
26+
* that drift can be introduced in either direction regardless of what the
27+
* current count happens to be.
28+
*
29+
* @param kref - The kref whose count to overwrite.
30+
* @param counts - The count text, in the store's encoding.
31+
*/
32+
function setStoredCount(kref: KRef, counts: string): void {
33+
kdb.kernelKVStore.set(`${kref}.refCount`, counts);
34+
}
35+
36+
/**
37+
* Shift every component of a count by the same amount.
38+
*
39+
* @param counts - The count text, in the store's encoding.
40+
* @param delta - How far to shift each component.
41+
* @returns The shifted count text.
42+
*/
43+
function shift(counts: string, delta: number): string {
44+
return counts
45+
.split(',')
46+
.map((part) => `${Number(part) + delta}`)
47+
.join(',');
48+
}
49+
2250
beforeEach(() => {
23-
kernelStore = makeKernelStore(makeMapKernelDatabase());
51+
kdb = makeMapKernelDatabase();
52+
kernelStore = makeKernelStore(kdb);
2453
kernelStore.markInitialized();
2554
givenVats('v1', 'v2', 'v3');
2655
});
@@ -172,6 +201,166 @@ describe('reference count audit', () => {
172201
});
173202
});
174203

204+
// The clean-audit cases above prove each rule agrees with whatever the store
205+
// did, which stays true if a rule and the code it mirrors are wrong by the
206+
// same constant. These pin each credit source to a literal count and holder
207+
// label, and check drift in both directions: too low collects a live
208+
// capability, too high leaks it.
209+
describe('each credit source, on its own', () => {
210+
const sources: {
211+
what: string;
212+
hold: () => KRef;
213+
expected: string;
214+
holders: string[];
215+
}[] = [
216+
{
217+
what: 'an object import a vat still reaches',
218+
hold: () => {
219+
const kref = kernelStore.exportFromEndpoint('v1', 'o+1');
220+
kernelStore.translateRefKtoE('v2', kref, true);
221+
return kref;
222+
},
223+
expected: '1,1',
224+
holders: ['v2 c-list import o-1'],
225+
},
226+
{
227+
what: 'an object import a vat has dropped but not retired',
228+
hold: () => {
229+
const kref = kernelStore.exportFromEndpoint('v1', 'o+1');
230+
kernelStore.translateRefKtoE('v2', kref, true);
231+
kernelStore.clearReachableFlag('v2', kref);
232+
return kref;
233+
},
234+
expected: '0,1',
235+
holders: ['v2 c-list import o-1'],
236+
},
237+
{
238+
what: 'a pinned object',
239+
hold: () => {
240+
const kref = kernelStore.exportFromEndpoint('v1', 'o+1');
241+
kernelStore.pinObject(kref);
242+
return kref;
243+
},
244+
expected: '1,1',
245+
holders: ['pin'],
246+
},
247+
{
248+
what: "a run-queue send's target and slot",
249+
hold: () => {
250+
const kref = kernelStore.exportFromEndpoint('v1', 'o+1');
251+
kernelStore.enqueueRun({
252+
type: 'send',
253+
target: kref,
254+
message: { methargs: { body: '#[]', slots: [kref] }, result: null },
255+
});
256+
kernelStore.incrementRefCount(kref, 'queue|target');
257+
kernelStore.incrementRefCount(kref, 'queue|slot');
258+
return kref;
259+
},
260+
expected: '2,2',
261+
holders: ['run queue #1 send target', 'run queue #1 send slot'],
262+
},
263+
{
264+
what: "a run-queue send's result promise",
265+
hold: () => {
266+
const target = kernelStore.exportFromEndpoint('v1', 'o+1');
267+
const kpid = kernelStore.initKernelPromise()[0];
268+
kernelStore.enqueueRun({
269+
type: 'send',
270+
target,
271+
message: { methargs: { body: '#[]', slots: [] }, result: kpid },
272+
});
273+
kernelStore.incrementRefCount(target, 'queue|target');
274+
kernelStore.incrementRefCount(kpid, 'queue|result');
275+
return kpid;
276+
},
277+
expected: '2',
278+
holders: ['unsettled promise', 'run queue #1 send result'],
279+
},
280+
{
281+
what: 'a queued notification',
282+
hold: () => {
283+
const kpid = kernelStore.exportFromEndpoint('v1', 'p+1');
284+
kernelStore.enqueueRun({ type: 'notify', endpointId: 'v2', kpid });
285+
kernelStore.incrementRefCount(kpid, 'notify');
286+
return kpid;
287+
},
288+
expected: '3',
289+
holders: [
290+
'unsettled promise',
291+
'run queue #1 notify',
292+
'v1 c-list export p+1',
293+
],
294+
},
295+
{
296+
// `enqueuePromiseMessage` takes the references itself, which is the
297+
// point of the transfer-don't-duplicate fix; incrementing here too
298+
// would be the double-count it exists to prevent.
299+
what: 'a message parked on an unresolved promise',
300+
hold: () => {
301+
const target = kernelStore.exportFromEndpoint('v1', 'o+1');
302+
const kpid = kernelStore.initKernelPromise()[0];
303+
kernelStore.enqueuePromiseMessage(kpid, {
304+
methargs: { body: '#[]', slots: [target] },
305+
result: null,
306+
});
307+
return kpid;
308+
},
309+
expected: '2',
310+
holders: ['unsettled promise', 'kp1 queue #1 target'],
311+
},
312+
{
313+
what: 'a promise nobody has settled yet',
314+
hold: () => kernelStore.initKernelPromise()[0],
315+
expected: '1',
316+
holders: ['unsettled promise'],
317+
},
318+
{
319+
what: "a settled promise's resolution slot",
320+
hold: () => {
321+
const koid = kernelStore.exportFromEndpoint('v1', 'o+1');
322+
const kpid = kernelStore.exportFromEndpoint('v1', 'p+1');
323+
kernelStore.incrementRefCount(koid, 'resolve|slot');
324+
kernelStore.resolveKernelPromise(kpid, false, {
325+
body: '#"$0"',
326+
slots: [koid],
327+
});
328+
return koid;
329+
},
330+
expected: '1,1',
331+
holders: ['kp1 resolution slot'],
332+
},
333+
{
334+
what: "a promise's own c-list entries",
335+
hold: () => {
336+
const kpid = kernelStore.exportFromEndpoint('v1', 'p+1');
337+
kernelStore.translateRefKtoE('v2', kpid, true);
338+
return kpid;
339+
},
340+
expected: '3',
341+
holders: [
342+
'unsettled promise',
343+
'v1 c-list export p+1',
344+
'v2 c-list import p-1',
345+
],
346+
},
347+
];
348+
349+
it.each(sources)('credits $what exactly', ({ hold, expected, holders }) => {
350+
const kref = hold();
351+
352+
expect(kernelStore.auditRefCounts()).toStrictEqual([]);
353+
354+
for (const delta of [1, -1]) {
355+
const stored = shift(expected, delta);
356+
setStoredCount(kref, stored);
357+
expect(kernelStore.auditRefCounts()).toStrictEqual([
358+
{ kind: 'mismatch', kref, stored, expected, holders },
359+
]);
360+
}
361+
});
362+
});
363+
175364
describe('assertRefCountsIfAuditing', () => {
176365
it('does nothing while auditing is off', () => {
177366
const kref = kernelStore.exportFromEndpoint('v1', 'o+1');

packages/ocap-kernel/src/store/methods/refcount-audit.ts

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -256,6 +256,15 @@ export function getRefCountAuditMethods(ctx: StoreContext) {
256256
* Compare every kref's stored reference counts against the references the
257257
* kernel can be seen to hold.
258258
*
259+
* What this can and cannot find is worth being precise about, because the
260+
* ground truth here *is* the holder set. A count that disagrees with its
261+
* holders is caught in either direction: too low, and a live capability can be
262+
* collected; too high with no holder left, and the count itself is orphaned.
263+
* But a holder that should have been torn down and wasn't justifies its own
264+
* count — at any value — so a leaked *reference* is invisible to this by
265+
* construction. A c-list entry that outlives what it names is the case that
266+
* matters: see the settled-promise TODO in `KernelRouter`.
267+
*
259268
* @returns The krefs whose counts disagree with ground truth, in kref order.
260269
*/
261270
function auditRefCounts(): RefCountViolation[] {

0 commit comments

Comments
 (0)