diff --git a/packages/SwingSet/src/kernel/kernel.js b/packages/SwingSet/src/kernel/kernel.js index fe07b958c64..eac3ba99d02 100644 --- a/packages/SwingSet/src/kernel/kernel.js +++ b/packages/SwingSet/src/kernel/kernel.js @@ -980,10 +980,7 @@ export default function buildKernel( const abandonedObjects = [ ...kernelKeeper.enumerateNonDurableObjectExports(vatID), ]; - for (const { kref, vref } of abandonedObjects) { - /** @see translateAbandonExports in {@link ./vatTranslator.js} */ - vatKeeper.deleteCListEntry(kref, vref); - /** @see abandonExports in {@link ./kernelSyscall.js} */ + for (const { kref } of abandonedObjects) { kernelKeeper.orphanKernelObject(kref, vatID); } diff --git a/packages/SwingSet/src/kernel/kernelSyscall.js b/packages/SwingSet/src/kernel/kernelSyscall.js index fa1ce165daf..573b2b351b7 100644 --- a/packages/SwingSet/src/kernel/kernelSyscall.js +++ b/packages/SwingSet/src/kernel/kernelSyscall.js @@ -178,24 +178,13 @@ export function makeKernelSyscallHandler(tools) { function retireExports(koids) { Array.isArray(koids) || Fail`retireExports given non-Array ${koids}`; - const newActions = []; - for (const koid of koids) { - const importers = kernelKeeper.getImporters(koid); - for (const vatID of importers) { - newActions.push(`${vatID} retireImport ${koid}`); - } - // TODO: decref and delete any #2069 auxdata - kernelKeeper.deleteKernelObject(koid); - } - kernelKeeper.addGCActions(newActions); + kernelKeeper.retireKernelObjects(koids); return OKNULL; } function abandonExports(vatID, koids) { Array.isArray(koids) || Fail`abandonExports given non-Array ${koids}`; for (const koid of koids) { - // note that this is effectful and also performed outside of a syscall - // by processUpgradeVat in {@link ./kernel.js} kernelKeeper.orphanKernelObject(koid, vatID); } return OKNULL; diff --git a/packages/SwingSet/src/kernel/state/kernelKeeper.js b/packages/SwingSet/src/kernel/state/kernelKeeper.js index c6d84d97d27..b4204833e05 100644 --- a/packages/SwingSet/src/kernel/state/kernelKeeper.js +++ b/packages/SwingSet/src/kernel/state/kernelKeeper.js @@ -112,7 +112,6 @@ export const CURRENT_SCHEMA_VERSION = 2; // $vatSlot is one of: o+$NN/o-$NN/p+$NN/p-$NN/d+$NN/d-$NN // v$NN.c.$vatSlot = $kernelSlot = ko$NN/kp$NN/kd$NN // v$NN.vs.$key = string -// v$NN.meter = m$NN // XXX does this exist? // old (v0): v$NN.reapInterval = $NN or 'never' // old (v0): v$NN.reapCountdown = $NN or 'never' // v$NN.reapDirt = JSON({ deliveries, gcKrefs, computrons }) // missing keys treated as zero @@ -621,7 +620,9 @@ export default function makeKernelKeeper( function getObjectRefCount(kernelSlot) { const data = kvStore.get(`${kernelSlot}.refCount`); - data || Fail`getObjectRefCount(${kernelSlot}) was missing`; + if (!data) { + return { reachable: 0, recognizable: 0 }; + } const [reachable, recognizable] = commaSplit(data).map(Number); reachable <= recognizable || Fail`refmismatch(get) ${kernelSlot} ${reachable},${recognizable}`; @@ -709,13 +710,32 @@ export default function makeKernelKeeper( return owner; } + function retireKernelObjects(koids) { + Array.isArray(koids) || Fail`retireExports given non-Array ${koids}`; + const newActions = []; + for (const koid of koids) { + const importers = getImporters(koid); + for (const vatID of importers) { + newActions.push(`${vatID} retireImport ${koid}`); + } + deleteKernelObject(koid); + } + addGCActions(newActions); + } + function orphanKernelObject(kref, oldVat) { + // termination orphans all exports, upgrade orphans non-durable + // exports, and syscall.abandonExports orphans specific ones const ownerKey = `${kref}.owner`; const ownerVat = kvStore.get(ownerKey); ownerVat === oldVat || Fail`export ${kref} not owned by old vat`; kvStore.delete(ownerKey); + const { vatSlot: vref } = getReachableAndVatSlot(oldVat, kref); + kvStore.delete(`${oldVat}.c.${kref}`); + kvStore.delete(`${oldVat}.c.${vref}`); + addMaybeFreeKref(kref); // note that we do not delete the object here: it will be - // collected if/when all other references are dropped + // retired if/when all other references are dropped } function deleteKernelObject(koid) { @@ -931,6 +951,14 @@ export default function makeKernelKeeper( * */ function cleanupAfterTerminatedVat(vatID, budget = undefined) { + // this is called from terminateVat, which is called from either: + // * end of processDeliveryMessage, if crankResults.terminate + // * device-vat-admin (when vat-v-a does adminNode.terminateVat) + // (which always happens inside processDeliveryMessage) + // so we're always followed by a call to processRefcounts, at + // end-of-delivery in processDeliveryMessage, after checking + // crankResults.terminate + insistVatID(vatID); let cleanups = 0; const work = { @@ -999,19 +1027,8 @@ export default function makeKernelKeeper( const vref = stripPrefix(clistPrefix, k); assert(vref.startsWith('o+'), vref); const kref = kvStore.get(k); - // we must delete the c-list entry, but we don't need to - // manipulate refcounts like the way vatKeeper/deleteCListEntry - // does, so just delete the keys directly - // vatKeeper.deleteCListEntry(kref, vref); - kvStore.delete(`${vatID}.c.${kref}`); - kvStore.delete(`${vatID}.c.${vref}`); - // if this object became unreferenced, processRefcounts will - // delete it, so don't be surprised. TODO: this results in an - // extra get() for each delete(), see if there's a way to avoid - // this. TODO: think carefully about other such races. - if (kvStore.has(`${kref}.owner`)) { - orphanKernelObject(kref, vatID); - } + // note: adds to maybeFreeKrefs, deletes c-list and .owner + orphanKernelObject(kref, vatID); work.exports += 1; if (spend()) { logWork(); @@ -1484,14 +1501,43 @@ export default function makeKernelKeeper( return false; } + // TODO (#9888): change the processRefcounts maybeFreeKrefs + // iteration to handle krefs that get added multiple times (while + // we're iterating), because we might do different work the second + // time around. The concerning scenario is: + // + // * kp2 has resolution data which references ko1 + // * somehow both kp2 and ko1 end up on maybeFreeKrefs, but ko1.reachable=1 + // (via kp2) + // * our iterator visits ko1 first, sees it is still reachable, ignores it + // * then the iterator visits kp2, decrefs its kp.data.slots + // * this pushes ko1 back onto maybeFreeKrefs + // * we need to examine ko1 again, so it can be released + // + // It could also happen if/when we implement #2069 auxdata: + // + // * ko2 has auxdata that references ko1 + // * both ko1 and ko2 end up on maybeFreeKrefs, but ko1.reachable = 1 + // (via the ko2 auxdata) + // * our iterator visits ko1 first, sees it is still reachable, ignores it + // * then the iterator visits ko2, does deleteKernelObject + // * this frees the auxdata, which pushes ko1 back onto maybeFreeKrefs + // * we need to examine ko1 again, so it can be released + // + // We should use something like an ordered Set, and a loop that does: + // * pop the first kref off + // * processes it (maybe adding more krefs) + // * repeats until the thing is empty + // Or maybe make a copy of maybeFreeKrefs at the start, clear the + // original, and wrap this in a loop that keeps going until + // maybeFreeKrefs is still empty at the end. Be sure to imagine a + // very deep linked list: don't just process it twice, keep + // processing until there's nothing left to do, otherwise we'll be + // leaving work for the next delivery. + function processRefcounts() { if (enableKernelGC) { - const actions = getGCActions(); // local cache - // TODO (else buggy): change the iteration to handle krefs that get - // added multiple times (while we're iterating), because we might do - // different work the second time around. Something like an ordered - // Set, and a loop that: pops the first kref off, processes it (maybe - // adding more krefs), repeats until the thing is empty. + const actions = new Set(); for (const kref of maybeFreeKrefs.values()) { const { type } = parseKernelSlot(kref); if (type === 'promise') { @@ -1499,6 +1545,7 @@ export default function makeKernelKeeper( const kp = getKernelPromise(kpid); if (kp.refCount === 0) { let idx = 0; + // TODO (#9889) don't assume promise is settled for (const slot of kp.data.slots) { // Note: the following decrement can result in an addition to the // maybeFreeKrefs set, which we are in the midst of iterating. @@ -1509,32 +1556,74 @@ export default function makeKernelKeeper( deleteKernelPromise(kpid); } } + if (type === 'object') { const { reachable, recognizable } = getObjectRefCount(kref); if (reachable === 0) { - const ownerVatID = ownerOfKernelObject(kref); - if (ownerVatID) { + // We avoid ownerOfKernelObject(), which will report + // 'undefined' if the owner is dead (and being slowly + // deleted). Message delivery should use that, but not us. + const ownerKey = `${kref}.owner`; + let ownerVatID = kvStore.get(ownerKey); + const terminated = terminatedVats.includes(ownerVatID); + + // Some objects that are still owned, but the owning vat + // might still alive, or might be terminated and in the + // process of being deleted. These two clauses are + // mutually exclusive. + + if (ownerVatID && !terminated) { const vatKeeper = provideVatKeeper(ownerVatID); - const isReachable = vatKeeper.getReachableFlag(kref); - if (isReachable) { + const vatConsidersReachable = vatKeeper.getReachableFlag(kref); + if (vatConsidersReachable) { // the reachable count is zero, but the vat doesn't realize it actions.add(`${ownerVatID} dropExport ${kref}`); } if (recognizable === 0) { - // TODO: rethink this - // assert.equal(isReachable, false, `${kref} is reachable but not recognizable`); + // TODO: rethink this assert + // assert.equal(vatConsidersReachable, false, `${kref} is reachable but not recognizable`); actions.add(`${ownerVatID} retireExport ${kref}`); } - } else if (recognizable === 0) { - // unreachable, unrecognizable, orphaned: delete the - // empty refcount here, since we can't send a GC - // action without an ownerVatID - deleteKernelObject(kref); + } else if (ownerVatID && terminated) { + // When we're slowly deleting a vat, and one of its + // exports becomes unreferenced, we obviously must not + // send dropExports or retireExports into the dead vat. + // We fast-forward the abandonment that slow-deletion + // would have done, then treat the object as orphaned. + + const { vatSlot } = getReachableAndVatSlot(ownerVatID, kref); + // delete directly, not orphanKernelObject(), which + // would re-submit to maybeFreeKrefs + kvStore.delete(ownerKey); + kvStore.delete(`${ownerVatID}.c.${kref}`); + kvStore.delete(`${ownerVatID}.c.${vatSlot}`); + // now fall through to the orphaned case + ownerVatID = undefined; + } + + // Now handle objects which were orphaned. NOTE: this + // includes objects which were owned by a terminated (but + // not fully deleted) vat, where `ownerVatID` was cleared + // in the last line of that previous clause (the + // fall-through case). Don't try to change this `if + // (!ownerVatID)` into an `else if`: the two clauses are + // *not* mutually-exclusive. + + if (!ownerVatID) { + // orphaned and unreachable, so retire it. If the kref + // is recognizable, then we need retireKernelObjects() + // to scan for importers and send retireImports (and + // delete), else we can call deleteKernelObject directly + if (recognizable) { + retireKernelObjects([kref]); + } else { + deleteKernelObject(kref); + } } } } } - setGCActions(actions); + addGCActions([...actions]); } maybeFreeKrefs.clear(); } @@ -1824,6 +1913,7 @@ export default function makeKernelKeeper( kernelObjectExists, getImporters, orphanKernelObject, + retireKernelObjects, deleteKernelObject, pinObject, diff --git a/packages/SwingSet/src/kernel/vatTranslator.js b/packages/SwingSet/src/kernel/vatTranslator.js index 4f1d1e2aa08..d781a41cf38 100644 --- a/packages/SwingSet/src/kernel/vatTranslator.js +++ b/packages/SwingSet/src/kernel/vatTranslator.js @@ -481,9 +481,6 @@ function makeTranslateVatSyscallToKernelSyscall(vatID, kernelKeeper) { assert.equal(allocatedByVat, true); // abandon *exports*, not imports // kref must already be in the clist const kref = mapVatSlotToKernelSlot(vref, gcSyscallMapOpts); - // note that this is effectful and also performed outside of a syscall - // by processUpgradeVat in {@link ./kernel.js} - vatKeeper.deleteCListEntry(kref, vref); return kref; }); kdebug(`syscall[${vatID}].abandonExports(${krefs.join(' ')})`); diff --git a/packages/SwingSet/test/abandon-export.test.js b/packages/SwingSet/test/abandon-export.test.js index 55d3762e22e..d619b93df9a 100644 --- a/packages/SwingSet/test/abandon-export.test.js +++ b/packages/SwingSet/test/abandon-export.test.js @@ -5,15 +5,21 @@ import { test } from '../tools/prepare-test-env-ava.js'; import buildKernel from '../src/kernel/index.js'; import { initializeKernel } from '../src/controller/initializeKernel.js'; import { extractMethod } from '../src/lib/kdebug.js'; +import makeKernelKeeper, { + CURRENT_SCHEMA_VERSION, +} from '../src/kernel/state/kernelKeeper.js'; import { makeKernelEndowments, buildDispatch } from './util.js'; import { kser, kunser, kslot } from '@agoric/kmarshal'; const makeKernel = async () => { const endowments = makeKernelEndowments(); - const { kvStore } = endowments.kernelStorage; - await initializeKernel({}, endowments.kernelStorage); + const { kernelStorage } = endowments; + const { kvStore } = kernelStorage; + await initializeKernel({}, kernelStorage); const kernel = buildKernel(endowments, {}, {}); - return { kernel, kvStore }; + const kernelKeeper = makeKernelKeeper(kernelStorage, CURRENT_SCHEMA_VERSION); + kernelKeeper.loadStats(); + return { kernel, kvStore, kernelKeeper }; }; /** @@ -43,7 +49,7 @@ const assertSingleEntryLog = (t, log, type, details = {}) => { return entry; }; -const makeTestVat = async (t, kernel, name) => { +const makeTestVat = async (kernel, name, kernelKeeper) => { const { log, dispatch } = buildDispatch(); let syscall; const setup = providedSyscall => { @@ -56,21 +62,29 @@ const makeTestVat = async (t, kernel, name) => { const kref = kernel.getRootObject(vatID); kernel.pinObject(kref); const flushDeliveries = async () => { - // Make a dummy delivery so the kernel will call processRefcounts(). - // This isn't normally needed, but we're directly making syscalls - // outside of a delivery. - // If that ever stops working, we can update `makeTestVat` to return - // functions supporting more true-to-production vat behavior like - // ```js - // enqueueSyscall('send', target, kser([...args]), resultVPID); - // enqueueSyscall('subscribe', resultVPID); - // flushSyscalls(); // issues queued syscalls inside a dummy delivery - // ``` - kernel.queueToKref(kref, 'flush', []); + // This test use a really lazy+sketchy approach to triggering + // syscalls and the refcount bookkeeping that we want to exercise. + // + // The kernel only runs processRefcounts() after a real crank + // (i.e. inside processDeliveryMessage and + // processAcceptanceMessage) but we want to avoid cluttering + // delivery logs of our vats with dummy activity, so we avoid + // making dispatches *into* the vat. + // + // The hack is to grab the vat's `syscall` object and invoke it + // directly, from *outside* a crank. Then, to trigger + // `processRefcounts()`, we inject a `{ type: 'negated-gc-action' + // }` onto the run-queue, which is handled by + // processDeliveryMessage but doesn't actually deliver anything. + // + // A safer approach would be to define the vat's dispatch() to + // listen for some messages that trigger each of the syscalls we + // want to invoke + // + kernelKeeper.addToRunQueue({ type: 'negated-gc-action' }); await kernel.run(); - assertFirstLogEntry(t, log, 'deliver', { method: 'flush' }); }; - return { vatID, kref, dispatch, log, syscall, flushDeliveries }; + return { vatID, kref, log, syscall, flushDeliveries }; }; async function doAbandon(t, reachable) { @@ -78,7 +92,7 @@ async function doAbandon(t, reachable) { // vatB abandons it // vatA should retain the object // sending to the abandoned object should get an error - const { kernel, kvStore } = await makeKernel(); + const { kernel, kvStore, kernelKeeper } = await makeKernel(); await kernel.start(); const { @@ -87,13 +101,14 @@ async function doAbandon(t, reachable) { log: logA, syscall: syscallA, flushDeliveries: flushDeliveriesA, - } = await makeTestVat(t, kernel, 'vatA'); + } = await makeTestVat(kernel, 'vatA', kernelKeeper); const { vatID: vatB, kref: bobKref, log: logB, syscall: syscallB, - } = await makeTestVat(t, kernel, 'vatB'); + flushDeliveries: flushDeliveriesB, + } = await makeTestVat(kernel, 'vatB', kernelKeeper); await kernel.run(); // introduce B to A, so it can send 'holdThis' later @@ -142,16 +157,24 @@ async function doAbandon(t, reachable) { // now have vatB abandon the export syscallB.abandonExports([targetForBob]); - await flushDeliveriesA(); - - // no GC messages for either vat - t.deepEqual(logA, []); - t.deepEqual(logB, []); - + await flushDeliveriesB(); targetOwner = kvStore.get(`${targetKref}.owner`); targetRefCount = kvStore.get(`${targetKref}.refCount`); - t.is(targetOwner, undefined); - t.is(targetRefCount, expectedRefCount); // unchanged + + if (reachable) { + // no GC messages for either vat + t.deepEqual(logA, []); + t.deepEqual(logB, []); + t.is(targetOwner, undefined); + t.is(targetRefCount, expectedRefCount); // unchanged + } else { + // 'target' was orphaned and unreachable, kernel will delete it, + // so A will get a retireImports now + assertSingleEntryLog(t, logA, 'retireImports', { vrefs: [targetForAlice] }); + t.deepEqual(logB, []); + t.is(targetOwner, undefined); + t.is(targetRefCount, undefined); + } if (reachable) { // vatA can send a message, but it will reject @@ -180,17 +203,11 @@ async function doAbandon(t, reachable) { await flushDeliveriesA(); // vatB should not get a dispatch.dropImports t.deepEqual(logB, []); - // the object still exists, now only recognizable - targetRefCount = kvStore.get(`${targetKref}.refCount`); - expectedRefCount = '0,1'; - t.is(targetRefCount, expectedRefCount); // merely recognizable + // the kernel automatically retires the orphaned + // now-merely-recognizable object + assertSingleEntryLog(t, logA, 'retireImports', { vrefs: [targetForAlice] }); + // which deletes it } - - // now vatA retires the object too - syscallA.retireImports([targetForAlice]); - await flushDeliveriesA(); - // vatB should not get a dispatch.retireImports - t.deepEqual(logB, []); // the object no longer exists targetRefCount = kvStore.get(`${targetKref}.refCount`); expectedRefCount = undefined; diff --git a/packages/SwingSet/test/gc-dead-vat/bootstrap.js b/packages/SwingSet/test/gc-dead-vat/bootstrap.js index 50668b2902b..7bf0c47039b 100644 --- a/packages/SwingSet/test/gc-dead-vat/bootstrap.js +++ b/packages/SwingSet/test/gc-dead-vat/bootstrap.js @@ -1,5 +1,6 @@ import { Far, E } from '@endo/far'; import { makePromiseKit } from '@endo/promise-kit'; +import { makeScalarBigWeakSetStore } from '@agoric/vat-data'; async function sendExport(doomedRoot) { const exportToDoomed = Far('exportToDoomed', {}); @@ -11,6 +12,7 @@ export function buildRootObject() { let doomedRoot; const pin = []; const pk1 = makePromiseKit(); + const wh = makeScalarBigWeakSetStore('weak-holder'); return Far('root', { async bootstrap(vats, devices) { const vatMaker = E(vats.vatAdmin).createVatAdminService(devices.vatAdmin); @@ -19,6 +21,8 @@ export function buildRootObject() { await sendExport(doomedRoot); const doomedExport1Presence = await E(doomedRoot).getDoomedExport1(); pin.push(doomedExport1Presence); + const doomedExport3Presence = await E(doomedRoot).getDoomedExport3(); + wh.add(doomedExport3Presence); }, async stash() { // Give vat-doomed a target that doesn't resolve one() right away. diff --git a/packages/SwingSet/test/gc-dead-vat/vat-doomed.js b/packages/SwingSet/test/gc-dead-vat/vat-doomed.js index f3ad636b7c0..19acca8758b 100644 --- a/packages/SwingSet/test/gc-dead-vat/vat-doomed.js +++ b/packages/SwingSet/test/gc-dead-vat/vat-doomed.js @@ -4,6 +4,7 @@ export function buildRootObject(vatPowers) { const pin = []; const doomedExport1 = Far('doomedExport1', {}); const doomedExport2 = Far('doomedExport2', {}); + const doomedExport3 = Far('doomedExport3', {}); return Far('root', { accept(exportToDoomedPresence) { pin.push(exportToDoomedPresence); @@ -14,6 +15,9 @@ export function buildRootObject(vatPowers) { stashDoomedExport2(target) { E(E(target).one()).neverCalled(doomedExport2); }, + getDoomedExport3() { + return doomedExport3; + }, terminate() { vatPowers.exitVat('completion'); }, diff --git a/packages/SwingSet/test/gc-kernel-orphan.test.js b/packages/SwingSet/test/gc-kernel-orphan.test.js new file mode 100644 index 00000000000..d6b30126493 --- /dev/null +++ b/packages/SwingSet/test/gc-kernel-orphan.test.js @@ -0,0 +1,463 @@ +// @ts-nocheck +/* global WeakRef, FinalizationRegistry */ + +import anylogger from 'anylogger'; +// eslint-disable-next-line import/order +import { test } from '../tools/prepare-test-env-ava.js'; + +import { assert } from '@endo/errors'; +import { kser, kunser, kslot } from '@agoric/kmarshal'; +import { initSwingStore } from '@agoric/swing-store'; +import { waitUntilQuiescent } from '@agoric/internal/src/lib-nodejs/waitUntilQuiescent.js'; +import buildKernel from '../src/kernel/index.js'; +import { initializeKernel } from '../src/controller/initializeKernel.js'; + +function makeConsole(tag) { + const log = anylogger(tag); + const cons = {}; + for (const level of ['debug', 'log', 'info', 'warn', 'error']) { + cons[level] = log[level]; + } + return harden(cons); +} + +function writeSlogObject(o) { + function bigintReplacer(_, arg) { + if (typeof arg === 'bigint') { + return Number(arg); + } + return arg; + } + 0 && console.log(JSON.stringify(o, bigintReplacer)); +} + +function makeEndowments() { + return { + waitUntilQuiescent, + kernelStorage: initSwingStore().kernelStorage, + runEndOfCrank: () => {}, + makeConsole, + writeSlogObject, + WeakRef, + FinalizationRegistry, + }; +} + +async function makeKernel() { + const endowments = makeEndowments(); + const { kvStore } = endowments.kernelStorage; + await initializeKernel({}, endowments.kernelStorage); + const kernel = buildKernel(endowments, {}, {}); + return { kernel, kvStore }; +} + +const orphanTest = test.macro(async (t, cause, when, amyState) => { + assert(['reachable', 'recognizable', 'none'].includes(amyState)); + assert(['abandon', 'terminate'].includes(cause)); + // There is a third case (vatA gets upgraded, and the object wasn't + // durable), but we can't upgrade vats created by createTestVat(). + assert(['before', 'after'].includes(when)); + const retireImmediately = false; + const { kernel, kvStore } = await makeKernel(); + await kernel.start(); + + const vrefs = {}; // track vrefs within vats + + // Our two root objects (alice and bob) are pinned so they don't disappear + // while the test is talking to them. So we make alice introduce "amy" as a + // new object that's doomed to be collected. Bob first drops amy, then + // retires her, provoking first a dropExports then a retireImports on + // alice. + + vrefs.aliceForA = 'o+100'; + vrefs.amyForA = 'o+101'; + function setupA(syscall, _state, _helpers, _vatPowers) { + let exportingAmy = false; + function dispatch(vd) { + if (vd[0] === 'startVat') { + return; // skip startVat + } + let method; + if (vd[0] === 'message') { + const methargs = kunser(vd[2].methargs); + [method] = methargs; + if (method === 'init') { + // initial conditions: bob holds a reference to amy + vrefs.bobForA = vd[2].methargs.slots[0]; + syscall.send( + vrefs.bobForA, + kser(['accept-amy', [kslot(vrefs.amyForA)]]), + ); + exportingAmy = true; + } + if (method === 'abandon') { + if (exportingAmy) { + syscall.abandonExports([vrefs.amyForA]); + } + } + if (method === 'terminate') { + syscall.exit(false, kser('terminated')); + } + } + if (vd[0] === 'dropExports') { + if (retireImmediately) { + // pretend there are no local strongrefs, and as soon as liveslots + // drops it's claim, the object goes away completely + syscall.retireExports(vd[1]); + } + } + if (vd[0] === 'retireExports') { + exportingAmy = false; + } + } + return dispatch; + } + await kernel.createTestVat('vatA', setupA); + const vatA = kernel.vatNameToID('vatA'); + const alice = kernel.addExport(vatA, vrefs.aliceForA); + + let amyRetiredToB = false; + function setupB(syscall, _state, _helpers, _vatPowers) { + function dispatch(vd) { + if (vd[0] === 'startVat') { + return; // skip startVat + } + let method; + if (vd[0] === 'message') { + [method] = kunser(vd[2].methargs); + if (method === 'accept-amy') { + vrefs.amyForB = vd[2].methargs.slots[0]; + } + if (method === 'drop') { + syscall.dropImports([vrefs.amyForB]); + } + if (method === 'drop and retire') { + syscall.dropImports([vrefs.amyForB]); + syscall.retireImports([vrefs.amyForB]); + } + if (method === 'retire') { + // it would be an error for bob to do this before or without + // dropImports + syscall.retireImports([vrefs.amyForB]); + } + } + if (vd[0] === 'retireImports') { + t.deepEqual(vd[1], [vrefs.amyForB]); + amyRetiredToB = true; + } + } + return dispatch; + } + await kernel.createTestVat('vatB', setupB); + const vatB = kernel.vatNameToID('vatB'); + vrefs.bobForB = 'o+200'; + const bob = kernel.addExport(vatB, vrefs.bobForB); + + // we always start with bob importing an object from alice + kernel.queueToKref(alice, 'init', [kslot(bob)], 'none'); + await kernel.run(); + + const amy = kvStore.get(`${vatA}.c.${vrefs.amyForA}`); + t.is(amy, 'ko22'); // probably + + if (when === 'before') { + // the object is abandoned before vatB drops anything + if (cause === 'abandon') { + kernel.queueToKref(alice, 'abandon', [], 'none'); + } else if (cause === 'terminate') { + kernel.queueToKref(alice, 'terminate', [], 'none'); + } + await kernel.run(); + t.is(kvStore.get(`${amy}.owner`), undefined); + } + + t.is(kvStore.get(`${amy}.refCount`), '1,1'); + t.is(kvStore.get(`${vatB}.c.${amy}`), `R ${vrefs.amyForB}`); + + // vatB now drops/retires/neither the "amy" object + + if (amyState === 'reachable') { + // no change + } else if (amyState === 'recognizable') { + kernel.queueToKref(bob, 'drop', [], 'none'); + await kernel.run(); + if (when === 'before') { + // dropping an abandoned object should also retire it + t.true(amyRetiredToB); // fixed by #7212 + t.is(kvStore.get(`${vatB}.c.${amy}`), undefined); + t.is(kvStore.get(`${amy}.refCount`), undefined); + } else { + // dropping a living object should merely drop it + t.is(kvStore.get(`${vatB}.c.${amy}`), `_ ${vrefs.amyForB}`); + t.is(kvStore.get(`${amy}.refCount`), '0,1'); + } + } else if (amyState === 'none') { + kernel.queueToKref(bob, 'drop and retire', [], 'none'); + await kernel.run(); + t.is(kvStore.get(`${vatB}.c.${amy}`), undefined); + t.is(kvStore.get(`${amy}.refCount`), undefined); + } + + if (when === 'after') { + // the object is abandoned *after* vatB drops anything + if (cause === 'abandon') { + kernel.queueToKref(alice, 'abandon', [], 'none'); + } else if (cause === 'terminate') { + kernel.queueToKref(alice, 'terminate', [], 'none'); + } + await kernel.run(); + } + + t.is(kvStore.get(`${amy}.owner`), undefined); + + if (amyState === 'reachable') { + // amy should remain defined and reachable, just lacking an owner + t.is(kvStore.get(`${vatB}.c.${amy}`), `R ${vrefs.amyForB}`); + t.is(kvStore.get(`${amy}.refCount`), '1,1'); + } else if (amyState === 'recognizable') { + // an unreachable koid should be retired upon abandonment + t.true(amyRetiredToB); // fixed by #7212 + t.is(kvStore.get(`${vatB}.c.${amy}`), undefined); + t.is(kvStore.get(`${amy}.refCount`), undefined); + } else if (amyState === 'none') { + // no change + } +}); + +for (const cause of ['abandon', 'terminate']) { + for (const when of ['before', 'after']) { + for (const amyState of ['reachable', 'recognizable', 'none']) { + test( + `orphan test ${cause}-${when}-${amyState}`, + orphanTest, + cause, + when, + amyState, + ); + } + } +} + +// exercise a failure case I saw in the zoe tests (zoe - +// secondPriceAuction - valid inputs): +// * vatB is exporting an object ko120 to vatA +// * vatA does E(ko120).wake(), then drops the reference +// (vatA can still recognize ko120) +// * vatA gets a BOYD, does syscall.dropImport(ko120) +// * ko120.refCount = 1,2 (0,1 from vatA, 1,1 from the run-queue wake()) +// * wake() is delivered to vatB +// * removing it from the run-queue drops refCount to 0,1 +// * and adds ko120 to maybeFreeKrefs +// * wake() provokes vatB to syscall.exit +// * post-crank terminateVat() orphans ko120, then retires it +// * deleting both .owner and .refCount +// * post-er-crank processRefcounts sees ko120 in maybeFreeKrefs +// * tries to look up .refCount, fails, panics + +// the fix was to change kernelKeeper.getRefCounts to handle a missing +// koNN.refCounts by just returning 0,0 + +test('termination plus maybeFreeKrefs - dropped', async t => { + const { kernel, kvStore } = await makeKernel(); + await kernel.start(); + + const vrefs = {}; // track vrefs within vats + + // vatB exports an object to vatA, vatA sends it a message and drops + // it (but doesn't retire it), vatB will self-terminate upon + // receiving that message, creating two places that try to retire it + + // The order of events will be: + // * 'terminate' translated, drops reachable to zero, adds to maybeFreeKrefs + // * 'terminate' delivered, delivery marks vat for termination + // * post-delivery crankResults.terminate check marks vat as terminated + // (but slow-deletion means nothing is deleted on that crank) + // * post-delivery does processRefCounts() + // * that processes ko22/billy, sees 0,1, owner=v2, v2 is terminated + // so it orphans ko22 (removes from vatB c-list and clears .owner) + // and falls through to (owner=undefined) case + // which sees recognizable=1 and retires the object + // (i.e. pushes retireImport gcAction and deletes .owner and .refCounts) + // * next crank starts cleanup, walks c-list, orphans ko21/bob + // which adds ko21 to maybeFreeKrefs + // * post-cleanup processRefCounts() does ko21, sees 1,1, owner=undefined + // does nothing, since vatA still holds an (orphaned) reference + // * cleanup finishes + + // Our two root objects (alice and bob) are pinned so they don't + // disappear while the test is talking to them, so vatB exports + // "billy". + + vrefs.aliceForA = 'o+100'; + vrefs.bobForB = 'o+200'; + vrefs.billyForB = 'o+201'; + let billyKref; + + let vatA; + function setupA(syscall, _state, _helpers, _vatPowers) { + function dispatch(vd) { + if (vd[0] === 'startVat') { + return; // skip startVat + } + // console.log(`deliverA:`, JSON.stringify(vd)); + if (vd[0] === 'message') { + const methargs = kunser(vd[2].methargs); + const [method] = methargs; + if (method === 'call-billy') { + t.is(vd[2].methargs.slots.length, 1); + vrefs.billyForA = vd[2].methargs.slots[0]; + t.is(vrefs.billyForA, 'o-50'); // probably + billyKref = kvStore.get(`${vatA}.c.${vrefs.billyForA}`); + syscall.send( + vrefs.billyForA, + kser(['terminate', [kslot(vrefs.billyForA, 'billy-A')]]), + ); + syscall.dropImports([vrefs.billyForA]); + } + } + } + return dispatch; + } + await kernel.createTestVat('vatA', setupA); + vatA = kernel.vatNameToID('vatA'); + const alice = kernel.addExport(vatA, vrefs.aliceForA); + + function setupB(syscall, _state, _helpers, _vatPowers) { + function dispatch(vd) { + if (vd[0] === 'startVat') { + return; // skip startVat + } + // console.log(`deliverB:`, JSON.stringify(vd)); + if (vd[0] === 'message') { + const [method] = kunser(vd[2].methargs); + if (method === 'init') { + vrefs.aliceForB = vd[2].methargs.slots[0]; + syscall.send( + vrefs.aliceForB, + kser(['call-billy', [kslot(vrefs.billyForB, 'billy-B')]]), + ); + } + if (method === 'terminate') { + t.is(vd[2].methargs.slots.length, 1); + assert.equal(vd[2].methargs.slots[0], vrefs.billyForB); + syscall.exit(false, kser('reason')); + } + } + } + return dispatch; + } + await kernel.createTestVat('vatB', setupB); + const vatB = kernel.vatNameToID('vatB'); + const bob = kernel.addExport(vatB, vrefs.bobForB); + + // this triggers everything, the bug was a kernel crash + kernel.queueToKref(bob, 'init', [kslot(alice, 'alice')], 'none'); + await kernel.run(); + + t.is(kvStore.get(`${billyKref}.owner`), undefined); + t.is(kvStore.get(`${billyKref}.refCounts`), undefined); + t.is(kvStore.get(`${vatA}.c.${billyKref}`), undefined); + t.is(kvStore.get(`${vatA}.c.${vrefs.billyForA}`), undefined); + t.is(kvStore.get(`${vatB}.c.${billyKref}`), undefined); + t.is(kvStore.get(`${vatB}.c.${vrefs.billyForB}`), undefined); +}); + +// like above, but the object doesn't remain recognizable +test('termination plus maybeFreeKrefs - retired', async t => { + const { kernel, kvStore } = await makeKernel(); + await kernel.start(); + + const vrefs = {}; // track vrefs within vats + + // vatB exports an object to vatA, vatA sends it a message and drops + // and retires it, vatB will self-terminate upon receiving that + // message. The order of events will be: + // * 'terminate' translated, drops refcount to zero, adds to maybeFreeKrefs + // * 'terminate' delivered, delivery marks vat for termination + // * post-delivery crankResults.terminate check marks vat as terminated + // (but slow-deletion means nothing is deleted on that crank) + // * post-delivery does processRefCounts() + // * that processes ko22/billy, sees 0,0, owner=v2, v2 is terminated + // so it orphans ko22 (removes from vatB c-list and clears .owner) + // and falls through to (owner=undefined) case + // which sees recognizable=0 and deletes the object (just .refCount now) + // * next crank starts cleanup, walks c-list, orphans ko21/bob + // which adds ko21 to maybeFreeKrefs + // * post-cleanup processRefCounts() does ko21, sees 1,1, owner=undefined + // does nothing, since vatA still holds an (orphaned) reference + // * cleanup finishes + + vrefs.aliceForA = 'o+100'; + vrefs.bobForB = 'o+200'; + vrefs.billyForB = 'o+201'; + let billyKref; + + let vatA; + function setupA(syscall, _state, _helpers, _vatPowers) { + function dispatch(vd) { + if (vd[0] === 'startVat') { + return; // skip startVat + } + console.log(`deliverA:`, JSON.stringify(vd)); + if (vd[0] === 'message') { + const methargs = kunser(vd[2].methargs); + const [method] = methargs; + if (method === 'call-billy') { + t.is(vd[2].methargs.slots.length, 1); + vrefs.billyForA = vd[2].methargs.slots[0]; + t.is(vrefs.billyForA, 'o-50'); // probably + billyKref = kvStore.get(`${vatA}.c.${vrefs.billyForA}`); + syscall.send( + vrefs.billyForA, + kser(['terminate', [kslot(vrefs.billyForA, 'billy-A')]]), + ); + syscall.dropImports([vrefs.billyForA]); + syscall.retireImports([vrefs.billyForA]); + } + } + } + return dispatch; + } + await kernel.createTestVat('vatA', setupA); + vatA = kernel.vatNameToID('vatA'); + const alice = kernel.addExport(vatA, vrefs.aliceForA); + + function setupB(syscall, _state, _helpers, _vatPowers) { + function dispatch(vd) { + if (vd[0] === 'startVat') { + return; // skip startVat + } + console.log(`deliverB:`, JSON.stringify(vd)); + if (vd[0] === 'message') { + const [method] = kunser(vd[2].methargs); + if (method === 'init') { + vrefs.aliceForB = vd[2].methargs.slots[0]; + syscall.send( + vrefs.aliceForB, + kser(['call-billy', [kslot(vrefs.billyForB, 'billy-B')]]), + ); + } + if (method === 'terminate') { + t.is(vd[2].methargs.slots.length, 1); + assert.equal(vd[2].methargs.slots[0], vrefs.billyForB); + syscall.exit(false, kser('reason')); + } + } + } + return dispatch; + } + await kernel.createTestVat('vatB', setupB); + const vatB = kernel.vatNameToID('vatB'); + const bob = kernel.addExport(vatB, vrefs.bobForB); + + // this triggers everything, the bug was a kernel crash + kernel.queueToKref(bob, 'init', [kslot(alice, 'alice')], 'none'); + await kernel.run(); + + t.is(kvStore.get(`${billyKref}.owner`), undefined); + t.is(kvStore.get(`${billyKref}.refCounts`), undefined); + t.is(kvStore.get(`${vatA}.c.${billyKref}`), undefined); + t.is(kvStore.get(`${vatA}.c.${vrefs.billyForA}`), undefined); + t.is(kvStore.get(`${vatB}.c.${billyKref}`), undefined); + t.is(kvStore.get(`${vatB}.c.${vrefs.billyForB}`), undefined); +}); diff --git a/packages/SwingSet/test/gc-kernel.test.js b/packages/SwingSet/test/gc-kernel.test.js index eef51e76a4f..91b3a0bc26a 100644 --- a/packages/SwingSet/test/gc-kernel.test.js +++ b/packages/SwingSet/test/gc-kernel.test.js @@ -1091,12 +1091,15 @@ test('terminated vat', async t => { // we'll watch for this to be deleted when the vat is terminated // console.log(`pinKref`, pinKref); - // find the highest export: doomedExport1 / doomedExport1Presence + // find the two highest exports: doomedExport[13] / doomedExport[13]Presence let exports = Object.keys(vrefs).filter(vref => vref.startsWith('o+')); sortVrefs(exports); - const doomedExport1Vref = exports[exports.length - 1]; + const doomedExport1Vref = exports[exports.length - 2]; + const doomedExport3Vref = exports[exports.length - 1]; t.is(doomedExport1Vref, 'o+10'); // arbitrary + t.is(doomedExport3Vref, 'o+11'); // arbitrary const doomedExport1Kref = vrefs[doomedExport1Vref]; + const doomedExport3Kref = vrefs[doomedExport3Vref]; // this should also be deleted // console.log(`doomedExport1Kref`, doomedExport1Kref); @@ -1105,6 +1108,8 @@ test('terminated vat', async t => { t.is(owners[pinKref], bootstrapVat); t.deepEqual(refcounts[doomedExport1Kref], [1, 1]); t.is(owners[doomedExport1Kref], doomedVat); + t.deepEqual(refcounts[doomedExport3Kref], [0, 1]); + t.is(owners[doomedExport3Kref], doomedVat); // Tell bootstrap to give a promise to the doomed vat. The doomed vat will // send a second export in a message to this promise, so the only @@ -1117,7 +1122,7 @@ test('terminated vat', async t => { exports = Object.keys(vrefs).filter(vref => vref.startsWith('o+')); sortVrefs(exports); const doomedExport2Vref = exports[exports.length - 1]; - t.is(doomedExport2Vref, 'o+11'); // arbitrary + t.is(doomedExport2Vref, 'o+12'); // arbitrary const doomedExport2Kref = vrefs[doomedExport2Vref]; [refcounts, owners] = getRefCountsAndOwners(); t.deepEqual(refcounts[doomedExport2Kref], [1, 1]); // from promise queue @@ -1137,6 +1142,9 @@ test('terminated vat', async t => { t.deepEqual(refcounts[doomedExport2Kref], [1, 1]); t.falsy(owners[doomedExport1Kref]); t.falsy(owners[doomedExport2Kref]); + // but the merely-recognizable export should be deleted + t.falsy(owners[doomedExport3Kref]); + t.deepEqual(refcounts[doomedExport3Kref], undefined); // send a message to the orphan, to wiggle refcounts some more const r = c.queueToVatRoot('bootstrap', 'callOrphan', [], 'panic'); @@ -1157,12 +1165,8 @@ test('terminated vat', async t => { c.queueToVatRoot('bootstrap', 'drop', [], 'panic'); await c.run(); - // TODO: however, for some reason neither Node.js nor XS actually drops - // 'doomedExport1Presence', despite it clearly going out of scope. I don't - // know why. Until we can find way to make it drop, this check is commented - // out. [refcounts, owners] = getRefCountsAndOwners(); - // t.is(refcounts[doomedExport1Kref], undefined); + t.is(refcounts[doomedExport1Kref], undefined); t.falsy(owners[doomedExport1Kref]); t.is(refcounts[doomedExport2Kref], undefined); diff --git a/packages/SwingSet/test/upgrade/upgrade.test.js b/packages/SwingSet/test/upgrade/upgrade.test.js index 77b7c781b87..aca1efbbd89 100644 --- a/packages/SwingSet/test/upgrade/upgrade.test.js +++ b/packages/SwingSet/test/upgrade/upgrade.test.js @@ -707,20 +707,11 @@ test('non-durable exports are abandoned by upgrade of non-liveslots vat', async '1,1', 'strong observation of abandoned object retains ref counts', ); - // TODO: Expect and verify observer receipt of dispatch.retireExports - // and corresponding removal of weakKref ref counts. - // https://github.com/Agoric/agoric-sdk/issues/7212 t.is( kvStore.get(`${weakKref}.refCount`), - '0,1', - 'weak observation of abandoned object retains ref counts before #7212', + undefined, + 'unreachable abandoned object is forgotten', ); - // t.is( - // kvStore.get(`${weakKref}.refCount`), - // undefined, - // 'unreachable abandoned object is forgotten', - // ); - // const observerLog = await messageToVat('observer', 'getDispatchLog'); }); // No longer valid as of removing stopVat per #6650