From f21826caed3b946085c6a6e0feb4c121e3dcf23f Mon Sep 17 00:00:00 2001 From: Brian Warner Date: Tue, 16 Jan 2024 15:50:37 -0800 Subject: [PATCH] fix(liveslots): collection deletion didn't free key objects Fix collection deletion. Previously, objects (passables/vrefs) used as collection keys were not de-referenced when the collection was dropped/deleted or unconditionally cleared (`collection.clear()` without keyPatt or valuePatt). Strong collections would leak a reachable-refcount to the key object, and weak collections would leak a recognizable-refcount, leading to the object being kept alive (or kept unretired) forever. In addition, each entry would leak a vatstore key until the collection was finally deleted. The code in `clearInternalFull()` confused dbKeys, encodedKeys, and vrefs, and gave the wrong kind of key to `isEncodedRemotable()`, so the answer was always "no". fixes #8756 --- .../src/collectionManager.js | 26 ++- .../test/storeGC/test-clear-collection.js | 192 ++++++++++++++++++ 2 files changed, 214 insertions(+), 4 deletions(-) create mode 100644 packages/swingset-liveslots/test/storeGC/test-clear-collection.js diff --git a/packages/swingset-liveslots/src/collectionManager.js b/packages/swingset-liveslots/src/collectionManager.js index aa9a465f695b..7d1bab76b104 100644 --- a/packages/swingset-liveslots/src/collectionManager.js +++ b/packages/swingset-liveslots/src/collectionManager.js @@ -285,6 +285,17 @@ export function makeCollectionManager( return `${dbKeyPrefix}${dbEntryKey}`; } + // a "vref" is a string like "o-4" + // an "EncodedKey" is the output of encode-passable: + // * strings become `s${string}`, like "foo" -> "sfoo" + // * positive BigInts become `p${len}:${digits}`, 47n -> "p2:47" + // * refs are assigned an "ordinal" and encode as "r0000000001:o-4" + // a "DBKey" is used to index the vatstore + // * DBKeys for collection entries join a collection prefix to an EncodedKey + // * "foo" -> "vc.5.sfoo" + // * 47n -> "vc.5.p2:47" + // * vref(o-4) -> "vc.5.r0000000001:o-4" + const encodeRemotable = remotable => { // eslint-disable-next-line no-use-before-define const ordinal = getOrdinal(remotable); @@ -300,10 +311,11 @@ export function makeCollectionManager( // the resulting function will encode only `Key` arguments. const encodeKey = makeEncodePassable({ encodeRemotable }); - const vrefFromDBKey = dbKey => dbKey.substring(BIGINT_TAG_LEN + 2); + const vrefFromEncodedKey = encodedKey => + encodedKey.substring(1 + BIGINT_TAG_LEN + 1); const decodeRemotable = encodedKey => - convertSlotToVal(vrefFromDBKey(encodedKey)); + convertSlotToVal(vrefFromEncodedKey(encodedKey)); // `makeDecodePassable` has three named options: // `decodeRemotable`, `decodeError`, and `decodePromise`. @@ -339,10 +351,15 @@ export function makeCollectionManager( } function dbKeyToKey(dbKey) { + // convert e.g. vc.5.r0000000001:o+v10/1 to r0000000001:o+v10/1 const dbEntryKey = dbKey.substring(dbKeyPrefix.length); return decodeKey(dbEntryKey); } + function dbKeyToEncodedKey(dbKey) { + return dbKey.substring(dbKeyPrefix.length); + } + function has(key) { const { keyShape } = getSchema(); if (!matches(key, keyShape)) { @@ -555,8 +572,9 @@ export function makeCollectionManager( doMoreGC = value.slots.map(vrm.removeReachableVref).some(b => b) || doMoreGC; syscall.vatstoreDelete(dbKey); - if (isEncodedRemotable(dbKey)) { - const keyVref = vrefFromDBKey(dbKey); + const encodedKey = dbKeyToEncodedKey(dbKey); + if (isEncodedRemotable(encodedKey)) { + const keyVref = vrefFromEncodedKey(encodedKey); if (hasWeakKeys) { vrm.removeRecognizableVref(keyVref, `${collectionID}`, true); } else { diff --git a/packages/swingset-liveslots/test/storeGC/test-clear-collection.js b/packages/swingset-liveslots/test/storeGC/test-clear-collection.js new file mode 100644 index 000000000000..efcddb3b08a5 --- /dev/null +++ b/packages/swingset-liveslots/test/storeGC/test-clear-collection.js @@ -0,0 +1,192 @@ +// @ts-nocheck +import test from 'ava'; + +import { Far } from '@endo/marshal'; +import { kser, kslot } from '@agoric/kmarshal'; +import { makeLiveSlots } from '../../src/liveslots.js'; +import { buildSyscall } from '../liveslots-helpers.js'; +import { makeMessage, makeStartVat, makeBringOutYourDead } from '../util.js'; +import { makeMockGC } from '../mock-gc.js'; + +// When a virtual collection is the only reference to a virtual +// object, collection.clear() should let them be deleted. Bug #8756 +// caused them to be retained by mistake. + +test('collection.clear() deletes contents', async t => { + const { syscall, log } = buildSyscall(); + const gcTools = makeMockGC(); + const COUNT = 5; + + function build(vatPowers) { + const { defineKind, makeScalarBigSetStore } = vatPowers.VatData; + const make = defineKind('target', () => ({}), {}); + const holder = makeScalarBigSetStore('holder'); + const root = Far('root', { + create() { + for (let i = 0; i < COUNT; i += 1) { + // vrefs are all `o+v10/${N}`, N=1..10 + const target = make(); + holder.add(target); + // we immediately delete the presence, but the finalizers + // won't run until gcTools.flushAllFRs() + gcTools.kill(target); + } + }, + clear() { + holder.clear(); + }, + }); + return root; + } + + const ls = makeLiveSlots(syscall, 'vatA', {}, {}, gcTools, undefined, () => ({ + buildRootObject: build, + })); + const { dispatch } = ls; + await dispatch(makeStartVat(kser())); + log.length = 0; + + const rootA = 'o+0'; + + await dispatch(makeMessage(rootA, 'create', [])); + log.length = 0; + + // Collect the representatives, leaving only the virtual-data + // pillar. This BOYD finds non-zero virtual-data refcounts for all + // five VOs, so they are not deleted. + gcTools.flushAllFRs(); + const boyd1 = await dispatch(makeBringOutYourDead()); + t.is(boyd1.possiblyDeadSet, 0); + t.is(boyd1.possiblyRetiredSet, 0); + log.length = 0; + + // clearing the collections should delete both the data key and the + // ordinal key for each entry, but it won't delete the values, that + // is deferred until BOYD + await dispatch(makeMessage(rootA, 'clear', [])); + + // The .clear() will do an initial get(vc.5.) to start the iterator, + // then for each collection entry it will getNextKey()/get(), then a + // delete() of the data row, a get()+delete() of the refcount, and a + // delete() of the ordinal row (vc.5|o+v10/1). Then we'll see an + // extra getNextKey() (which stops the iteration). At the end of the + // loop, it will write out the new "|entryCount" value with one last + // set(). Total: 1*COUNT+1 getNextKey, 2*COUNT get, 3*COUNT delete, + // plus 1 set. + t.is(log.filter(l => l.type === 'vatstoreGetNextKey').length, 1 * COUNT + 1); + t.is(log.filter(l => l.type === 'vatstoreGet').length, 2 * COUNT + 1); + t.is(log.filter(l => l.type === 'vatstoreDelete').length, 3 * COUNT); + t.is(log.filter(l => l.type === 'vatstoreSet').length, 1); + t.is(log.length, 6 * COUNT + 3); + log.length = 0; + + // this ought to delete the VOs. bug #8756 + await dispatch(makeBringOutYourDead()); + + // We expect to see the objects get deleted. The scanForDeadObjects + // does a get(rc)/get(es) on each to decide the object is really + // dead. Then deleting the objects does a get(vom) to see what + // references need to be decremented, then a redundant + // get(rc)/get(es), and a delete(rc)/delete(es) (redundant, in this + // case, since the RAM pillar was the only one left, and rc/es keys + // didn't exist). A delete(vom) is emitted for each, but held in the + // data cache until end of crank. Then the retirement check does a + // getNextKey(ir) scan. + // + // Total: 5*COUNT get, 1*COUNT getNextKey, 3*COUNT delete + t.is(log.filter(l => l.type === 'vatstoreGet').length, 5 * COUNT); + t.is(log.filter(l => l.type === 'vatstoreSet').length, 0 * COUNT); + t.is(log.filter(l => l.type === 'vatstoreGetNextKey').length, 1 * COUNT); + t.is(log.filter(l => l.type === 'vatstoreDelete').length, 3 * COUNT); + t.is(log.length, 9 * COUNT); +}); + +test('weak collection deletion will retire contents', async t => { + const { syscall, log } = buildSyscall(); + const gcTools = makeMockGC(); + const COUNT = 5; + const allVrefs = []; + const allKslots = []; + for (let i = 0; i < COUNT; i += 1) { + const vref = `o-${i + 1}`; + allVrefs.push(vref); + allKslots.push(kslot(vref, 'imported')); + } + + // Import a bunch of Presences and hold them in a weakset. Drop the + // imports, but retain recognition, until we drop the weakset, which + // should delete the collection and retire the keys. + function build(vatPowers) { + const { makeScalarBigWeakSetStore } = vatPowers.VatData; + const recognizer = makeScalarBigWeakSetStore('recognizer'); + const root = Far('root', { + create(presences) { + for (const p of presences) { + recognizer.add(p); + // we immediately delete the presence, but the finalizers + // won't run until gcTools.flushAllFRs() + gcTools.kill(p); + } + }, + drop() { + gcTools.kill(recognizer); + }, + }); + return root; + } + + const ls = makeLiveSlots(syscall, 'vatA', {}, {}, gcTools, undefined, () => ({ + buildRootObject: build, + })); + const { dispatch } = ls; + await dispatch(makeStartVat(kser())); + log.length = 0; + + const rootA = 'o+0'; + + await dispatch(makeMessage(rootA, 'create', [allKslots])); + log.length = 0; + + // Collect the representatives, leaving only the virtual-data + // pillar. This BOYD finds non-zero virtual-data refcounts for all + // five VOs, so they are not deleted. + gcTools.flushAllFRs(); + await dispatch(makeBringOutYourDead()); + t.deepEqual(log.at(-1), { type: 'dropImports', slots: allVrefs }); + log.length = 0; + + // dropping the collection makes it UNREACHABLE but it won't be + // COLLECTED until BOYD + await dispatch(makeMessage(rootA, 'drop', [])); + t.is(log.length, 0); + + // this will delete the collection and should retire the imports + gcTools.flushAllFRs(); + await dispatch(makeBringOutYourDead()); + + // Collection deletion starts with scanForDeadObjects doing a + // get(rc)+get(es) on the collection vref, to decide if it's really + // dead. Then, the deletion phase does another get(rc)+get(es), then + // iterates over rows. This starts with the weird get(vc.5.), then + // for each entry, we see getNextKey(), get+delete of the data row, + // delete(ir), and delete(reverse-mapping). One more getNextKey() + // ends the iteration. Then we get(schemata) to see if the + // keyshape/valueshape has references that need deletion. Then we + // find and delete metadata: + + // * getNextKey, delete(|nextOrdinal) + // * getNextKey (deletion of |schemata is deferred) + // * getNextKey ends the iteration + // Then delete(rc)+delete(es) for the collection vref, even though + // they didn't exist. Then getNextKey to scan for ir records + // (none). Then finally delete(|schemata) + // + // Total: 5+1*COUNT+1 get, 1*COUNT+1+3+1 getNextKey, 3*COUNT+4 delete + + t.is(log.filter(l => l.type === 'vatstoreGetNextKey').length, COUNT + 5); + t.is(log.filter(l => l.type === 'vatstoreGet').length, COUNT + 6); + t.is(log.filter(l => l.type === 'vatstoreDelete').length, 3 * COUNT + 4); + t.is(log.filter(l => l.type === 'vatstoreSet').length, 0); + t.is(log.length, 5 * COUNT + 15); + log.length = 0; +});