vat termination leaks promises

[warner]

Describe the bug

While writing tests for remediation of #9039 , I noticed a second problem. When a vat is terminated, and the kernel rejects its remaining outstanding (vat-decided) promises, we delete the late vat's c-list entry without decrementing the corresponding refcount. That means these promises will be left with a final refcount of 1, not 0, causing the promise (and it's rejected value, e.g. the DisconnectionObject) to be retained forever.

To Reproduce

I'm modifying SwingSet/test/vat-admin/terminate/terminate.test.js to demonstrate the problem, by gleaning the kpid of a dead-vat -decided promise, and then examining the kvStore afterwards to see whether or not the kernel promise table entry is still present.

Impact

On chain, we've only ever deleted one vat (a provisioning vat, during the initial bootstrap run at chain launch, back in 2023). So the impact is minimal so far. But we're intending to terminate some large vats in the near future, so we should get this fixed before we perform those terminations.

Cause

When the kernel terminates a vat, we run a function named terminateVat() in kernel.js . Originally, before we landed slow-termination, this built a list of deadPromises by scanning for all promises decided by the vatID, then used kernelKeeper.cleanupAfterTerminatedVat() to delete all c-list entries (and vatstore), then submit each dead kpid to resolveToError(). This resolveToError calls doResolve with a rejection value, which is the same function that is used by syscall.resolve.

The critical difference is that when a vat does syscall.resolve(), the vatTranslator (which converts the VatSyscallObject to a KernelSyscallObject, translating vrefs to krefs) would also retire the c-list entries immediately after translation, using vatKeeper.deleteCListEntriesForKernelSlots(). This function decrements the entry's refcount just before deleting the kvStore keys, allowing the usual maybeFreeKrefs processing to clean things up.

But terminateVat goes directly to resolveToError/doResolve without going through a vref/kref translator, so nothing ever calls deleteCListEntriesForKernelSlots. It calls cleanupAfterTerminatedVat, and that function does decrement refcounts for kernel objects, but it does not do anything special for kernel promises, it merely deletes the forward (vNN.c.kpNN) and reverse kvStore entries without touching refcounts.

When we landed slow termination, we retained this pattern. All the dead vat's promises are rejected immediately, inside terminateVat(), but we leave c-list handling to the slow/background kernelKeeper.cleanupAfterTerminatedVat(), which behaves just like the old version, and doesn't perform refcount handling on promise c-list entries.

Likely Fix

One path would be to properly retire the promise c-list entries during terminateVat(). We would change it to use vatKeeper.deleteCListEntriesForKernelSlots(deadPromises). I'd have to think about the right ordering: should be retire those entries before or after doing resolveToError on all of them? The vat might have been self-subscribed to some of those promises, so resolveToError will enqueue notifications to the dead vat. I'm pretty sure those will be safely ignored when they reach the front of the run-queue, but we'd need to test it.

A second path would be to change cleanupAfterTerminatedVat() to have a 'promise' phase, probably just after the 'import' and 'export' phases, but before the 'kv' phase. Currently, the vNN.c.kpNN promise c-list entries are bulk-deleted during the kv phase, because I didn't think those deletions had side-effects (I thought the refcounts were decremented immediately, during terminateVat(), so it's safe to delete them at a faster rate. This would be in keeping with our slow-deletion patterns: all userspace-visible consequences of the termination happen immediately, during terminateVat(), but non-userspace-visible consequences (GC drops/retires) are allowed to happen slowly, afterwards.

I'm inclined to take the second path, although it probably means more work: adding a new phase involves cosmic-swingset code to make that limit be configurable.

[warner]

I took a quick look at the mainnet DB. We have two vats which are already unused, which we'll terminate as soon as the demolition perimeter is secure: v45-auctioneer and v44-auctioneer.governor. These currently have 6 and 0 outstanding promises, respectively.

We're also planning to cease using (and later terminate) v29-ATOM-USD_price_feed and v46-scaledPriceAuthority-ATOM. These are still in use, so they might have more outstanding promises than they will once we stop talking to them, but they currently have 6 and 12 promises outstanding.

So, the overall size of the leak would not be huge.

[warner]

Oh, I think it's worse than that.. even externally-decided promises, known to the dead vat (and probably subscribed to), will be leaked, when we delete the c-list entry without decrementing the refcount.

That means the second approach is definitely the better one. At termination time, the kernel walks all the promises decided by the vat, for rejection, but that's not a good place to walk the entire c-list. The slow-cleanup handler is a much better place for that kind of scan.