Commit f02677d
fix(ocap-kernel): make c-list import accounting symmetric (#1020)
Closes #1006. Replaces #1010, which carried this plus three unrelated
fixes; it is split into four PRs, this one first.
## The defect
Creating an import c-list entry changed no refcount; tearing one down
decremented both `reachable` and `recognizable`. `initKernelObject`
compensated by minting every object at `(1, 1)`, which is exactly right
for **one** importer — the only topology our tests exercised. There is
no `setReachableFlag` in the repo; it was never ported.
That single unit was also claimed by two parties: importer-side
(`object.ts`: born at 1 "on the assumption that the new object
corresponds to an object that has just been imported") and owner-side
(`vat.ts`: "the baseline decrement below corresponds to the implicit
reference `exportFromEndpoint` installed…"). Both an importer's drop and
the owner's termination were entitled to spend it.
All four symptoms in the issue reproduced against the real store before
the fix, and are covered by regression tests now.
### main has since grown a second compensation for this
While this was in review, #983 landed this in `cleanupTerminatedVat`:
```js
// Skip baseline decrement if GC already zeroed reachable via dropImports.
const { reachable } = getObjectRefCount(kref);
if (reachable > 0) {
decrementRefCount(kref, 'cleanup|export|baseline');
}
```
That is a guard against the phantom baseline, at the same site this PR
deletes the baseline decrement outright. This branch removes it; the
condition is moot once no phantom unit exists. #983's parallel-launch
tests pass unchanged under the audit.
## Approach
Followed the issue's proposed path, in order.
**Step 1 — the invariant checker, first.**
`store/methods/refcount-audit.ts` recomputes each kref's counts from
ground truth — c-list entries and their reachable flags, run-queue and
promise-queue messages, promise resolution values, pins — and reports
drift **in both directions**: counts too low, which lets a live
capability be collected, and counts too high, which keeps a dead one
alive (the issue's symptom 4 would pass an underflow-only check). It
compares 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. The credits mirror `incrementRefCount` case for case.
Enabled per kernel via `Kernel.make({ auditRefCounts: true })`, run
after every crank, and **on for every kernel `kernel-test` builds**. The
audit reports by throwing, which kills the run loop, and the kernel
hands run loop death to `onRunLoopFailure` rather than rethrowing it —
so `kernel-test` passes a handler that fails the test, and a violation
on a GC-only crank or after a test's last assertion fails the build too.
**Step 2 — restore the increment, rebase the baseline.**
`initKernelObject` → `(0, 0)`; `addCListEntry` takes the entry's
reference, mirroring `deleteCListEntry`; new `setReachableFlag`;
owner-side baseline decrements deleted. `collectGarbage` is already a
faithful port of `processRefcounts`, so this hands it the inputs it was
written for.
**Step 3 — remove the compensations.** This is where the checker earned
its keep. It found four more unbalanced paths the phantom baseline had
been absorbing:
- `#deliverSend` charged the target against the **routed** kref, not the
run-queue item's own. For a message routed through a resolved promise
those differ, so it decremented an object nobody charged and leaked the
promise.
- `#deliverNotify` released its reference only on the success path,
leaking it on both early returns, and decremented promises retired
alongside it that nobody had taken.
- A message queued on an unresolved promise duplicated every reference
it carried when re-enqueued on resolution.
- `resolve|kpid` incremented with no matching release. (I had assumed
`resolve|decider` cancelled it; that releases the distinct
unsettled-promise reference.)
Two things the baseline was silently standing in for, now explicit:
- **Vat roots are pinned** for their vat's lifetime, released on
termination. A root is addressable whether or not anyone imports it —
SwingSet pins static vat roots for exactly this reason. `pinVatRoot`
already existed and was never called internally.
- **GC action delivery moves the kernel's own c-list**: `dropExports`
clears the owner's flag, `retireExports`/`retireImports` tear the entry
down. `krefsToExistingErefs` → `krefsToErefs`, which throws rather than
silently dropping an unmapped kref.
## Migration
**There is none, and none is planned at this version: a store written
before this change must be reset.**
`kernel-store` has no schema version and no migration path, so such a
store opens under this code with every object still at `(1, 1)`, no pin
recorded for any vat root, and its pin and retention records in a layout
this code does not read. Both consequences land on the crank path,
against an existing user's database:
- the second importer's `dropImports` throws `"ko1" underflow -1,1` from
inside `performDropImports`;
- `initializeAllVats` uses `runVat`, which does not pin, and relies on
the persisted pin a legacy store does not have — so the last importer's
drop can retire a live vat's root.
`recomputeRefCounts` rebuilds the counts from ground truth, but it
cannot restore the root pins, so it is a diagnostic for a drifted store
rather than an upgrade path. Reach it by calling `makeKernelStore` over
the kernel's own database; `RefCountViolation` is now exported from the
package root.
## Judgment call worth review
**The `gc.ts:169` assert is not re-enabled.** The issue asks for it; I
believe it would fire legitimately. Left as a comment explaining why,
and the audit covers the same ground from outside.
## Changes since review
@grypez's seven in-scope items and @FUDCo's, one commit each.
1. **`incrementRefCount` guards at the primitive.** It now `Fail`s on a
missing object row, symmetric with the decrement's guard — the guard was
at two call sites, so `pinObject`, `resolve|slot` and every other path
could still resurrect a deleted object. The call-site guards stay: they
refuse before an eref is allocated or a ledger entry is written, and
name what was attempted.
2. **The audit actually fails the build.** `kernel-test` passes an
`onRunLoopFailure` that reports the failure to `afterEach`/`afterAll`
hooks, so the test fails with the message naming the drifted kref. An
async rethrow was the first attempt and is worse: under `endoify-node`
it exits the worker with `process.exit unexpectedly called with "-1"`
and the real error nowhere in sight. `io.test.ts` and
`endowment-globals.test.ts` build kernels directly and are audited now
too. Verified by injecting a double increment into `pinObject`: two
`cluster-launch` tests fail with the violation, where before they
passed.
3. **The audit compares the raw refcount row** instead of reading it
back through `getObjectRefCount`, which `Fail`s on `reachable >
recognizable` — one of the two drifts it exists to report. A malformed
row is now reported as it stands.
4. **Both headline fixes are pinned by tests.** A send routed through a
promise that fulfilled to an object, where the queued and routed targets
differ; and the notify release on both early returns, plus a batch
retiring a sibling promise.
5. **`resolvePromises` charges `data.slots` after the state and decider
checks**, so an illegal `syscall.resolve` leaves nothing behind.
6. **Migration decision stated above.**
7. **Changelog:** the rename moved to `### Changed` as its own
**BREAKING** bullet, the "counts too high (a leak)" claim corrected to
name the blind spot, `undoOcapURLRetention` added, and the blank lines
my formatting commit put inside the #984 entry reverted.
8. **Retentions and pins are counted per object**, not listed in one
row. Which objects get URLs is the holder's choice, so neither list was
bounded by anything the kernel controls, and each issuance rewrote the
whole row. A count per object is one write per issuance and keeps the
per-issuance semantics: overlapping issuances share the one pin. And
ending a retention is two operations, not one — `undoOcapURLRetention`
unwinds a single failed issuance, `releaseOcapURLRetentions` drops the
object's whole retention for a disavowal. `getPinnedObjects` names each
object once; `getPinCount` gives the count.
The follow-ups from the reviews that are not this PR's — the
settled-promise requeue, `unpinVatRoot`, `addCListEntry` idempotency,
`incRefCount`/`decRefCount`, and wiring revocation to
`releaseOcapURLRetentions` — are noted and will be raised separately.
## What moved to the other PRs in this stack
This is the first of four. The rest are being prepared now and will be
linked here as they open; #1010, #1011, #1012 and #1018 stay open until
then, so nothing looks dropped.
- **GC-delivery hardening** (retired-export freeing, the disowning
guard, remote-GC starvation, the restarting-vat GC release) — stacked
directly on this PR.
- **Crank-rollback and transaction-boundary semantics** — merges #1012
with #1018 and this branch's rollback commit into one PR. They had to be
one: #1012 rewrote `rollbackCrank`'s `finally` into a `try/catch`, this
branch changed `ctx.savepoints` from `string[]` to `{name,
maybeFreeKrefs}[]` on the same lines, and composed naively the rethrow
fires *before* the `maybeFreeKrefs` restore — a hole neither PR could
see alone.
- **Vat-lifecycle consistency** — was #1019, rebasing onto the end of
the stack.
Reviewing in order is worthwhile; each one's diff is much smaller than
#1010's was.
## Testing
`yarn lint` clean, `yarn build` 31/31. `@MetaMask/ocap-kernel` and
`@ocap/kernel-test` fully green, with `auditRefCounts` on for every
kernel `kernel-test` builds and a violation now failing the test that
provoked it.
## Checklist
- [x] I've updated the test suite for new or updated code as appropriate
- [x] I've updated documentation (JSDoc, `README.md`, `CHANGELOG.md`) as
appropriate
<!-- CURSOR_SUMMARY -->
---
> [!NOTE]
> **High Risk**
> Touches core capability GC, refcount invariants, and persistent store
layout with a mandatory reset for existing databases; incorrect
accounting can collect live objects or leak capabilities.
>
> **Overview**
> Fixes **#1006** by making import c-list creation/release symmetric:
new objects start at **`(0, 0)`**, **`addCListEntry`** takes a reference
(with **`setReachableFlag`** for re-handoffs), and owner-side baseline
decrements are removed. **Vat roots are pinned** for the vat lifetime;
**GC deliveries** now update the kernel’s own c-list (`dropExports` /
retire paths).
>
> Adds **reference-count auditing** (`auditRefCounts`,
`recomputeRefCounts`, …) and optional **`Kernel.make({ auditRefCounts:
true })`** checks after each crank; **`kernel-test`** enables this via
**`makeAuditedKernelOptions`** so drift fails tests through
**`onRunLoopFailure`**.
>
> Corrects several refcount leaks: send delivery charges
**`item.target`** (not the routed object), promise requeue **transfers**
refs, notify releases early, promise-queue messages are charged/released
consistently, **`resolvePromises`** only increments slots after legal
resolve, and **`getPromisesByDecider`** scans the real
**`${endpoint}.c.`** layout.
>
> **Ocap URL issuance** retains targets (per-URL issuance counts,
**`pinned.${kref}`** pin counts); **`krefsToExistingErefs`** →
**`krefsToErefs`** (throws if unmapped); **`incrementRefCount`** refuses
deleted krefs.
>
> **BREAKING:** existing stores must be **reset** (no migration);
tests/assertions updated for new baselines (e.g. createObject refcounts,
v3 root pin in e2e).
>
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
2f9ef11. Bugbot is set up for automated
code reviews on this repo. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>1 parent 37db357 commit f02677d
48 files changed
Lines changed: 2714 additions & 434 deletions
File tree
- packages
- extension/test/e2e
- kernel-test/src
- ocap-kernel
- src
- remotes/kernel
- store
- methods
- vats
- test
Some content is hidden
Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
252 | 252 | | |
253 | 253 | | |
254 | 254 | | |
255 | | - | |
256 | 255 | | |
257 | 256 | | |
258 | 257 | | |
| |||
282 | 281 | | |
283 | 282 | | |
284 | 283 | | |
| 284 | + | |
| 285 | + | |
| 286 | + | |
| 287 | + | |
| 288 | + | |
| 289 | + | |
| 290 | + | |
| 291 | + | |
| 292 | + | |
| 293 | + | |
| 294 | + | |
| 295 | + | |
| 296 | + | |
| 297 | + | |
| 298 | + | |
285 | 299 | | |
286 | 300 | | |
287 | 301 | | |
| |||
307 | 321 | | |
308 | 322 | | |
309 | 323 | | |
| 324 | + | |
| 325 | + | |
| 326 | + | |
| 327 | + | |
| 328 | + | |
| 329 | + | |
| 330 | + | |
| 331 | + | |
310 | 332 | | |
311 | 333 | | |
312 | 334 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
12 | 12 | | |
13 | 13 | | |
14 | 14 | | |
15 | | - | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
16 | 20 | | |
17 | 21 | | |
18 | 22 | | |
| |||
38 | 42 | | |
39 | 43 | | |
40 | 44 | | |
| 45 | + | |
41 | 46 | | |
42 | 47 | | |
43 | 48 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
21 | 21 | | |
22 | 22 | | |
23 | 23 | | |
| 24 | + | |
| 25 | + | |
24 | 26 | | |
25 | 27 | | |
26 | | - | |
| 28 | + | |
27 | 29 | | |
28 | 30 | | |
29 | 31 | | |
| |||
40 | 42 | | |
41 | 43 | | |
42 | 44 | | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
43 | 54 | | |
44 | 55 | | |
45 | 56 | | |
| |||
81 | 92 | | |
82 | 93 | | |
83 | 94 | | |
84 | | - | |
85 | | - | |
86 | | - | |
87 | | - | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
88 | 100 | | |
89 | 101 | | |
90 | 102 | | |
| |||
116 | 128 | | |
117 | 129 | | |
118 | 130 | | |
119 | | - | |
120 | | - | |
121 | | - | |
122 | | - | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
123 | 135 | | |
124 | 136 | | |
125 | 137 | | |
| |||
201 | 213 | | |
202 | 214 | | |
203 | 215 | | |
| 216 | + | |
| 217 | + | |
| 218 | + | |
| 219 | + | |
| 220 | + | |
| 221 | + | |
| 222 | + | |
| 223 | + | |
| 224 | + | |
| 225 | + | |
| 226 | + | |
| 227 | + | |
| 228 | + | |
| 229 | + | |
| 230 | + | |
| 231 | + | |
| 232 | + | |
| 233 | + | |
| 234 | + | |
| 235 | + | |
| 236 | + | |
| 237 | + | |
| 238 | + | |
| 239 | + | |
| 240 | + | |
| 241 | + | |
| 242 | + | |
| 243 | + | |
| 244 | + | |
| 245 | + | |
| 246 | + | |
| 247 | + | |
| 248 | + | |
| 249 | + | |
| 250 | + | |
| 251 | + | |
| 252 | + | |
| 253 | + | |
| 254 | + | |
| 255 | + | |
| 256 | + | |
| 257 | + | |
| 258 | + | |
| 259 | + | |
| 260 | + | |
| 261 | + | |
| 262 | + | |
| 263 | + | |
| 264 | + | |
| 265 | + | |
| 266 | + | |
| 267 | + | |
| 268 | + | |
| 269 | + | |
| 270 | + | |
| 271 | + | |
| 272 | + | |
| 273 | + | |
| 274 | + | |
| 275 | + | |
| 276 | + | |
| 277 | + | |
| 278 | + | |
| 279 | + | |
| 280 | + | |
| 281 | + | |
| 282 | + | |
| 283 | + | |
| 284 | + | |
| 285 | + | |
| 286 | + | |
| 287 | + | |
| 288 | + | |
| 289 | + | |
| 290 | + | |
| 291 | + | |
| 292 | + | |
| 293 | + | |
| 294 | + | |
| 295 | + | |
| 296 | + | |
| 297 | + | |
| 298 | + | |
| 299 | + | |
| 300 | + | |
| 301 | + | |
| 302 | + | |
| 303 | + | |
| 304 | + | |
| 305 | + | |
| 306 | + | |
| 307 | + | |
| 308 | + | |
| 309 | + | |
| 310 | + | |
| 311 | + | |
| 312 | + | |
| 313 | + | |
| 314 | + | |
| 315 | + | |
| 316 | + | |
| 317 | + | |
| 318 | + | |
| 319 | + | |
| 320 | + | |
| 321 | + | |
| 322 | + | |
| 323 | + | |
| 324 | + | |
| 325 | + | |
| 326 | + | |
| 327 | + | |
204 | 328 | | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
6 | 6 | | |
7 | 7 | | |
8 | 8 | | |
9 | | - | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
10 | 14 | | |
11 | 15 | | |
12 | 16 | | |
| |||
79 | 83 | | |
80 | 84 | | |
81 | 85 | | |
| 86 | + | |
82 | 87 | | |
83 | 88 | | |
84 | 89 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
176 | 176 | | |
177 | 177 | | |
178 | 178 | | |
179 | | - | |
| 179 | + | |
| 180 | + | |
180 | 181 | | |
181 | 182 | | |
182 | 183 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
11 | 11 | | |
12 | 12 | | |
13 | 13 | | |
14 | | - | |
15 | | - | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
16 | 69 | | |
17 | 70 | | |
18 | 71 | | |
| |||
93 | 146 | | |
94 | 147 | | |
95 | 148 | | |
| 149 | + | |
96 | 150 | | |
97 | 151 | | |
98 | 152 | | |
| |||
0 commit comments