From 72741b8f30452a35fada1371cd5b3f8324fc1496 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 and retirement bugs Liveslots was suffering from the following bugs: * vrefs weren't properly encoded into DB keys and "encodePassable" keys when deleting collections (such that `isEncodedRemotable()` always answered false) * recognition records (vom.ir. keys) weren't created or removed for Remotable-style keys in weak collections * Remotable-style vrefs were not submitted for retirement processing when deleting a weak collection As a result: * Invoking the `.clear()` method on a virtual/durable collection, or dereferencing the collection itself (and allowing it to be garbage collected), did not free any Remotables, virtual/durable objects (Representatives), or imported Presences used as keys. This could cause data in other weak collections (or other vats) to be retained longer than necessary. * Allowing a weak virtual/durable collection to be garbage collected did not inform the kernel that the vat can no longer recognize the Remotable/Representative/Presence-style keys, consuming c-list entries longer than necessary. * `collection.clear()` left the "ordinal-assignment"` records in the vatstore, causing subsequent .has() to incorrectly return true, and .init() to throw a "already registered" error, as well as consuming DB space longer than necessary. * retiring (deleting) a Remotable used as a key in a weak virtual/durable collection did not free the corresponding value, causing data (in local and/or remote vats) to be retained longer than necessary This commit fixes those bugs, which fixes the immediate cause of issues #8756 , #7355 , and #9956 . As a change to liveslots, full deployment requires both restarting the kernel with this new code, *and* triggering a vat upgrade of all vats. Once that is done, no new collection entries will suffer the problems listed above. However, this commit makes no attempt to remediate any existing data corruption. See #8759 for plans to build a tool that can audit the virtual-data reference graph and detect the consequences of these bugs in pre-existing kernel databases, and see the issues listed above for notes on efforts to build remediation tools. --- .../src/collectionManager.js | 27 +- .../src/virtualReferences.js | 102 +++- .../test/clear-collection.test.js | 568 ++++++++++++++++++ .../test/gc-before-finalizer.test.js | 35 +- .../test/liveslots-mock-gc.test.js | 99 +++ .../test/weakset-dropped-remotable.test.js | 50 ++ 6 files changed, 837 insertions(+), 44 deletions(-) create mode 100644 packages/swingset-liveslots/test/clear-collection.test.js create mode 100644 packages/swingset-liveslots/test/weakset-dropped-remotable.test.js diff --git a/packages/swingset-liveslots/src/collectionManager.js b/packages/swingset-liveslots/src/collectionManager.js index 8aa7ddd6e573..9041d06039d1 100644 --- a/packages/swingset-liveslots/src/collectionManager.js +++ b/packages/swingset-liveslots/src/collectionManager.js @@ -293,6 +293,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); @@ -308,10 +319,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`. @@ -347,10 +359,16 @@ 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) { + assert(dbKey.startsWith(dbKeyPrefix), dbKey); + return dbKey.substring(dbKeyPrefix.length); + } + function has(key) { const { keyShape } = getSchema(); if (!matches(key, keyShape)) { @@ -563,8 +581,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/src/virtualReferences.js b/packages/swingset-liveslots/src/virtualReferences.js index 09f78ff2ac9d..c1a21187cfc0 100644 --- a/packages/swingset-liveslots/src/virtualReferences.js +++ b/packages/swingset-liveslots/src/virtualReferences.js @@ -500,26 +500,52 @@ export function makeVirtualReferenceManager( } /** - * A vref is "recognizable" when it is used as the key of a weak Map - * or Set: that Map/Set can be used to query whether a future - * specimen matches the original or not, without holding onto the - * original. + * A vref is "recognizable" when it is used as the key of a weak + * collection, like a virtual/durable WeakMapStore or WeakSetStore, + * or the ephemeral voAwareWeakMap/Set that we impose upon userspace + * as "WeakMap/WeakSet". The collection can be used to query whether + * a future specimen matches the original or not, without holding + * onto the original. * - * This 'vrefRecognizers' is a Map from those vrefs to the set of - * recognizing weak collections, for virtual keys and non-virtual - * collections. Specifically, the vrefs correspond to imported - * Presences or virtual-object Representatives (Remotables do not - * participate: they are keyed by the actual Remotable object, not - * its vref). The collections are either a VirtualObjectAwareWeakMap - * or a VirtualObjectAwareWeakSet. We remove the entry when the key - * is removed from the collection, and when the entire collection is - * deleted. + * We need "recognition records" to map from the vref to the + * collection that can recognize it. When the vref is retired, we + * use the record to find all the collections from which we need to + * delete entries, so we can release the matching values. This might + * happen because the vref was for a Presence and the kernel just + * told us the upstream vat has deleted it (dispatch.retireImports), + * or because it was for a locally-managed object (an ephemeral + * Remotable or a virtual/durable Representative) and we decided to + * delete it. * - * It is critical that each collection have exactly one recognizer that is - * unique to that collection, because the recognizers themselves will be - * tracked by their object identities, but the recognizer cannot be the - * collection itself else it would prevent the collection from being garbage - * collected. + * The virtual/durable collections track their "recognition records" + * in the vatstore, in keys like "vom.ir.${vref}|${collectionID}". + * These records do not contribute to our RAM usage. + * + * voAwareWeakMap and voAwareWeakSet store their recognition records + * in RAM, using this Map named 'vrefRecognizers'. Each key is a + * vref, and the value is a Set of recognizers. Each recognizer is + * the internal 'virtualObjectMap' in which the collection maps from + * vref to value. These in-RAM collections only use virtualObjectMap + * to track Presence-style (imports) and Representative-style + * (virtual/durable) vrefs: any Remotable-style keys are stored in + * the collection's internal (real) WeakMap under the Remotable + * object itself (because the engine handles the bookkeeping, and + * there is no virtual data in the value that we need to clean up at + * deletion time). + * + * It is critical that each collection have exactly one recognizer + * that is unique to that collection, because the recognizers + * themselves will be tracked by their object identities, but the + * recognizer cannot be the collection itself else it would prevent + * the collection from being garbage collected. + * + * When an individual entry is deleted from the weak collection, we + * must also delete the recognition record. When the collection + * itself is deleted (i.e. because nothing was referencing it), we + * must both delete all recognition records and also notify the + * kernel about any Presence-style vrefs that we can no longer + * recognize (syscall.retireImports). The kernel doesn't care about + * Remotable- or Representative- style vrefs, only the imports. * * TODO: all the "recognizers" in principle could be, and probably should be, * reduced to deleter functions. However, since the VirtualObjectAware @@ -535,14 +561,24 @@ export function makeVirtualReferenceManager( /** @type {Map>} */ const vrefRecognizers = new Map(); + /** + * @param {*} value The vref-bearing object used as the collection key + * @param {string|Recognizer} recognizer The collectionID or virtualObjectMap for the collection + * @param {boolean} [recognizerIsVirtual] true for virtual/durable Stores, false for voAwareWeakMap/Set + */ function addRecognizableValue(value, recognizer, recognizerIsVirtual) { const vref = getSlotForVal(value); if (vref) { const { type, allocatedByVat, virtual, durable } = parseVatSlot(vref); - if (type === 'object' && (!allocatedByVat || virtual || durable)) { + if (type === 'object') { + // recognizerSet (voAwareWeakMap/Set) doesn't track Remotables + const notRemotable = !allocatedByVat || virtual || durable; + if (recognizerIsVirtual) { + assert.typeof(recognizer, 'string'); syscall.vatstoreSet(`vom.ir.${vref}|${recognizer}`, '1'); - } else { + } else if (notRemotable) { + assert.typeof(recognizer, 'object'); let recognizerSet = vrefRecognizers.get(vref); if (!recognizerSet) { recognizerSet = new Set(); @@ -554,18 +590,33 @@ export function makeVirtualReferenceManager( } } + /** + * @param {string} vref The vref or the object used as the collection key + * @param {string|Recognizer} recognizer The collectionID or virtualObjectMap for the collection + * @param {boolean} [recognizerIsVirtual] true for virtual/durable Stores, false for voAwareWeakMap/Set + */ function removeRecognizableVref(vref, recognizer, recognizerIsVirtual) { const { type, allocatedByVat, virtual, durable } = parseVatSlot(vref); - if (type === 'object' && (!allocatedByVat || virtual || durable)) { + if (type === 'object') { + // addToPossiblyDeadSet only needs Presence-style vrefs + const isPresence = !allocatedByVat; + // recognizerSet (voAwareWeakMap/Set) doesn't track Remotables + const notRemotable = !allocatedByVat || virtual || durable; + if (recognizerIsVirtual) { + assert.typeof(recognizer, 'string'); syscall.vatstoreDelete(`vom.ir.${vref}|${recognizer}`); - } else { + if (isPresence) { + addToPossiblyRetiredSet(vref); + } + } else if (notRemotable) { + assert.typeof(recognizer, 'object'); const recognizerSet = vrefRecognizers.get(vref); assert(recognizerSet && recognizerSet.has(recognizer)); recognizerSet.delete(recognizer); if (recognizerSet.size === 0) { vrefRecognizers.delete(vref); - if (!allocatedByVat) { + if (isPresence) { addToPossiblyRetiredSet(vref); } } @@ -573,6 +624,11 @@ export function makeVirtualReferenceManager( } } + /** + * @param {*} value The vref-bearing object used as the collection key + * @param {string|Recognizer} recognizer The collectionID or virtualObjectMap for the collection + * @param {boolean} [recognizerIsVirtual] true for virtual/durable Stores, false for voAwareWeakMap/Set + */ function removeRecognizableValue(value, recognizer, recognizerIsVirtual) { const vref = getSlotForVal(value); if (vref) { diff --git a/packages/swingset-liveslots/test/clear-collection.test.js b/packages/swingset-liveslots/test/clear-collection.test.js new file mode 100644 index 000000000000..acd9d925cffc --- /dev/null +++ b/packages/swingset-liveslots/test/clear-collection.test.js @@ -0,0 +1,568 @@ +import test from 'ava'; + +import { Far } from '@endo/marshal'; +import { kser, kslot } from '@agoric/kmarshal'; +import { makeLiveSlots } from '../src/liveslots.js'; +import { parseVatSlot } from '../src/parseVatSlots.js'; +import { buildSyscall } from './liveslots-helpers.js'; +import { makeMessage, makeStartVat, makeBringOutYourDead } from './util.js'; +import { makeMockGC } from './mock-gc.js'; + +const getPrefixedKeys = (map, prefix) => { + const keys = []; + for (const key of map.keys()) { + if (key.startsWith(prefix)) { + keys.push(key.substring(prefix.length)); + } + } + return keys; +}; + +const collectionMetaKeys = new Set([ + '|nextOrdinal', + '|entryCount', + '|schemata', +]); + +const scanCollection = (kvStore, collectionID) => { + const collectionPrefix = `vc.${collectionID}.`; + const ordinalAssignments = []; + const entries = []; + const metaKeys = []; + let totalKeys = 0; + for (const key of getPrefixedKeys(kvStore, collectionPrefix)) { + totalKeys += 1; + if (key.startsWith('|')) { + if (collectionMetaKeys.has(key)) { + metaKeys.push(key); + } else { + ordinalAssignments.push(key); + } + } else { + entries.push(key); + } + } + const keyVrefs = []; + const refcounts = {}; + for (const ordinalKey of ordinalAssignments) { + const vref = ordinalKey.substring(1); + keyVrefs.push(vref); + const rcKey = `vom.rc.${vref}`; + refcounts[vref] = kvStore.get(rcKey); + } + return { + totalKeys, + metaKeys, + ordinalAssignments, + entries, + keyVrefs, + refcounts, + }; +}; + +const GC = ['dropImports', 'retireImports', 'retireExports']; + +const doTest = async (t, mode) => { + assert(['strong-clear', 'strong-delete', 'weak-delete'].includes(mode)); + const kvStore = new Map(); + const { syscall, log } = buildSyscall({ kvStore }); + const gcTools = makeMockGC(); + const COUNT = 5; + + // we'll either call holder.clear() to exercise manual clearing, or + // gcTools.kill(holder) to exercise the collection itself being + // deleted + + let holder; + + function build(vatPowers) { + const { defineKind, makeScalarBigSetStore } = vatPowers.VatData; + const make = defineKind('target', () => ({}), {}); + 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, testHooks } = ls; + const { valToSlot } = testHooks; + await dispatch(makeStartVat(kser())); + log.length = 0; + + const rootA = 'o+0'; + + await dispatch(makeMessage(rootA, 'create', [])); + log.length = 0; + + const holderVref = valToSlot.get(holder); + const collectionID = Number(parseVatSlot(holderVref).subid); + const populated = scanCollection(kvStore, collectionID); + t.is(populated.ordinalAssignments.length, COUNT); + t.is(populated.entries.length, COUNT); + t.is(populated.keyVrefs.length, COUNT); + t.true(populated.keyVrefs.every(vref => populated.refcounts[vref] === '1')); + + // 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; + t.is(scanCollection(kvStore, collectionID).totalKeys, populated.totalKeys); + + if (mode === 'strong-clear') { + // 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. The metadata is retained, because the + // collection has been cleared, not deleted. + await dispatch(makeMessage(rootA, 'clear', [])); + } else if (mode === 'strong-delete') { + gcTools.kill(holder); + gcTools.flushAllFRs(); + await dispatch(makeBringOutYourDead()); + // that should clear everything, both the holder and the referenced + // targets + } + + const scan2 = scanCollection(kvStore, collectionID); + + // all entries should be gone + t.is(scan2.ordinalAssignments.length, 0); + t.is(scan2.entries.length, 0); + t.is(scan2.keyVrefs.length, 0); + + if (mode === 'strong-clear') { + // but the collection itself is still present + t.is(scan2.metaKeys.length, populated.metaKeys.length); + for (const vref of populated.keyVrefs) { + const rcKey = `vom.rc.${vref}`; + const rc = kvStore.get(rcKey); + // the target refcounts should be zero (= undefined) + t.is(rc, undefined); + // but the data should still be present + const dataKey = `vom.${vref}`; + const data = kvStore.get(dataKey); + t.is(data, '{}'); + } + // and we need one more BOYD to notice the zero refcounts and + // delete the data + await dispatch(makeBringOutYourDead()); + } else if (mode === 'strong-delete') { + // the collection should be gone + t.is(scan2.metaKeys.length, 0); + t.is(scan2.totalKeys, 0); + } + + // all the targets should be collected now + for (const vref of populated.keyVrefs) { + const rcKey = `vom.rc.${vref}`; + const rc = kvStore.get(rcKey); + // the target refcounts should be zero (= undefined) + t.is(rc, undefined); + const dataKey = `vom.${vref}`; + const data = kvStore.get(dataKey); + t.is(data, undefined); + } + + // none of the Presences were exported, so no GC syscalls + const gcCalls1 = log.filter(l => GC.includes(l.type)); + t.deepEqual(gcCalls1, []); +}; + +// When a virtual collection's keys are the only reference to a +// virtual object, collection.clear() should let them be deleted. Bug +// #8756 caused the keys to be retained by mistake. + +test('collection.clear() deletes keys', async t => { + await doTest(t, 'strong-clear'); +}); + +// Allowing GC to delete a strong collection should delete/release the +// keys too + +test('deleting a strong collection will delete the keys', async t => { + await doTest(t, 'strong-delete'); +}); + +// Allowing GC to delete a weak collection should retire the keys, and +// delete/release the contents. + +test('deleting a weak collection will retire the keys', async t => { + const kvStore = new Map(); + const { syscall, log } = buildSyscall({ kvStore }); + 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')); + } + + let recognizer; + + // 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 notify the kernel that we aren't + // recognizing the keys (syscall.retireImports) + function build(vatPowers) { + const { makeScalarBigWeakSetStore } = vatPowers.VatData; + 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); + } + }, + }); + return root; + } + + const ls = makeLiveSlots(syscall, 'vatA', {}, {}, gcTools, undefined, () => ({ + buildRootObject: build, + })); + const { dispatch, testHooks } = ls; + const { valToSlot } = testHooks; + await dispatch(makeStartVat(kser())); + log.length = 0; + + const rootA = 'o+0'; + + await dispatch(makeMessage(rootA, 'create', [allKslots])); + log.length = 0; + + const recognizerVref = valToSlot.get(recognizer); + const collectionID = Number(parseVatSlot(recognizerVref).subid); + + // all the Presences should be recognized by the collection, but not + // referenced + const populated = scanCollection(kvStore, collectionID); + t.is(populated.ordinalAssignments.length, COUNT); + t.is(populated.entries.length, COUNT); + t.is(populated.keyVrefs.length, COUNT); + t.true( + populated.keyVrefs.every(vref => populated.refcounts[vref] === undefined), + ); + // and there should be recognizer (.ir) entries for each vref|collection pair + t.true( + populated.keyVrefs.every(vref => + kvStore.has(`vom.ir.${vref}|${collectionID}`), + ), + ); + + // collect the Presences, which was the only remaining reachability + // pillar, leaving just the recognizers + gcTools.flushAllFRs(); + await dispatch(makeBringOutYourDead()); + // no changes to the collection + t.deepEqual(scanCollection(kvStore, collectionID), populated); + // but the Presence vrefs should be dropped + const gcCalls1 = log.filter(l => GC.includes(l.type)); + t.deepEqual(gcCalls1, [{ type: 'dropImports', slots: allVrefs }]); + log.length = 0; + + // now free the whole collection + gcTools.kill(recognizer); + gcTools.flushAllFRs(); + await dispatch(makeBringOutYourDead()); + + // the collection should be gone + const scan2 = scanCollection(kvStore, collectionID); + t.is(scan2.totalKeys, 0); + + // and the .ir entries + t.true( + populated.keyVrefs.every( + vref => !kvStore.has(`vom.ir.${vref}|${collectionID}`), + ), + ); + + // and the kernel should be notified that we don't care anymore + const gcCalls2 = log.filter(l => GC.includes(l.type)); + t.deepEqual(gcCalls2, [{ type: 'retireImports', slots: allVrefs }]); +}); + +// Allowing GC to delete a voAwareWeakSet (or Map) should retire the +// keys, and delete/release the contents. + +test('deleting a voAwareWeakSet will retire the keys', async t => { + const kvStore = new Map(); + const { syscall, log } = buildSyscall({ kvStore }); + 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')); + } + + let recognizer; + + // 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 notify the kernel that we aren't + // recognizing the keys (syscall.retireImports) + function build(vatPowers) { + recognizer = new vatPowers.WeakSet(); + 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); + } + }, + }); + return root; + } + + const ls = makeLiveSlots(syscall, 'vatA', {}, {}, gcTools, undefined, () => ({ + buildRootObject: build, + })); + const { dispatch, testHooks } = ls; + const { vrefRecognizers } = testHooks; + await dispatch(makeStartVat(kser())); + log.length = 0; + + const rootA = 'o+0'; + + await dispatch(makeMessage(rootA, 'create', [allKslots])); + log.length = 0; + + // the WeakSet has no vref, and doesn't store anything like ".ir" + // entries in vatstore, but we can snoop on its internal + // tables. vrefRecognizers is a Map, keyed by vref, with an entry + // for every vref that is tracked by any voAwareWeakMap/Set. The + // value is a Set of virtualObjectMaps, the internal/hidden Set used + // by voAwareWeakMap/Sets. + + const vrefKeys = [...vrefRecognizers.keys()].sort(); + + // we should be tracking all the presences + t.is(vrefKeys.length, COUNT); + // each vref should have a single recognizer + t.true(vrefKeys.every(vref => vrefRecognizers.get(vref).size === 1)); + // that single recognizer should be the virtualObjectMap for our voAwareWeakSet + const virtualObjectMap = [...vrefRecognizers.get(vrefKeys[0])][0]; + // they should all point to the same one + t.true( + vrefKeys.every( + vref => [...vrefRecognizers.get(vref)][0] === virtualObjectMap, + ), + ); + + // collect the Presences, which was the only remaining reachability + // pillar, leaving just the recognizers + gcTools.flushAllFRs(); + await dispatch(makeBringOutYourDead()); + // no changes to the collection + t.is(vrefKeys.length, COUNT); + t.true(vrefKeys.every(vref => vrefRecognizers.get(vref).size === 1)); + t.true( + vrefKeys.every( + vref => [...vrefRecognizers.get(vref)][0] === virtualObjectMap, + ), + ); + + // but the Presence vrefs should be dropped + const gcCalls1 = log.filter(l => GC.includes(l.type)); + t.deepEqual(gcCalls1, [{ type: 'dropImports', slots: allVrefs }]); + log.length = 0; + + // now free the whole collection + gcTools.kill(recognizer); + gcTools.flushAllFRs(); + await dispatch(makeBringOutYourDead()); + + // the collection should be gone + t.is(vrefRecognizers.size, 0); + + // and the kernel should be notified that we don't care anymore + const gcCalls2 = log.filter(l => GC.includes(l.type)); + t.deepEqual(gcCalls2, [{ type: 'retireImports', slots: allVrefs }]); +}); + +// explore remediation/leftover problems from bugs #7355, #8756, #9956 +// where the DB has corrupted data leftover from before they were fixed + +test('missing recognition record during delete', async t => { + const kvStore = new Map(); + const { syscall, log } = buildSyscall({ kvStore }); + const gcTools = makeMockGC(); + + let recognizer; + let target; + + // liveslots didn't always add "vom.ir." recognition-records for + // Remotable-style keys, nor remove them when the key was + // deleted. So a kernel which adds a key, upgrades to the current + // (fixed) version, then attempts to delete the key, will not see + // the record it is expecting. Make sure this doesn't cause + // problems. + + function build(vatPowers) { + const { makeScalarBigWeakSetStore } = vatPowers.VatData; + recognizer = makeScalarBigWeakSetStore('recognizer'); + target = Far('target', {}); + const root = Far('root', { + store() { + recognizer.add(target); + }, + delete() { + recognizer.delete(target); + }, + }); + return root; + } + + const ls = makeLiveSlots(syscall, 'vatA', {}, {}, gcTools, undefined, () => ({ + buildRootObject: build, + })); + const { dispatch, testHooks } = ls; + const { valToSlot } = testHooks; + await dispatch(makeStartVat(kser())); + log.length = 0; + + const rootA = 'o+0'; + + await dispatch(makeMessage(rootA, 'store')); + log.length = 0; + + const targetVref = valToSlot.get(target); + const recognizerVref = valToSlot.get(recognizer); + const collectionID = Number(parseVatSlot(recognizerVref).subid); + const ordinalAssignmentKey = `vc.${collectionID}.|${targetVref}`; + const ordinalNumber = kvStore.get(ordinalAssignmentKey); + t.is(ordinalNumber, '1'); + const dataKey = `vc.${collectionID}.r0000000001:${targetVref}`; + const value = kvStore.get(dataKey); + t.deepEqual(JSON.parse(value), { body: '#null', slots: [] }); + + // the correct recognition record key + const rrKey = `vom.ir.${targetVref}|${collectionID}`; + + // our fixed version creates one + t.is(kvStore.get(rrKey), '1'); + + // now simulate data from the broken version, by deleting the + // recognition record + kvStore.delete(rrKey); + + // check that deleting the same Remotable doesn't break + await dispatch(makeMessage(rootA, 'delete')); + t.false(kvStore.has(ordinalAssignmentKey)); + t.false(kvStore.has(dataKey)); +}); + +test.failing('leftover ordinal-assignment record during init', async t => { + const kvStore = new Map(); + const { syscall, log } = buildSyscall({ kvStore }); + const gcTools = makeMockGC(); + + let store; + let target; + /** @type {any} */ + let result; + + // liveslots didn't always remove the "vc.${collectionID}.|${vref}" + // ordinal-assignment records when clearing or deleting a + // collection. So a kernel which adds a key, upgrades to the current + // (fixed) version, then clears the collection, will have a leftover + // record. Make sure this doesn't cause problems when iterating keys + // or re-adding the same key later. + + function build(vatPowers) { + const { makeScalarBigMapStore } = vatPowers.VatData; + store = makeScalarBigMapStore('store'); + target = Far('target', {}); + const root = Far('root', { + store() { + try { + store.init(target, 123); + result = 'ok'; + } catch (e) { + result = e; + } + }, + clear() { + store.clear(); + }, + has() { + result = store.has(target); + }, + }); + return root; + } + + const ls = makeLiveSlots(syscall, 'vatA', {}, {}, gcTools, undefined, () => ({ + buildRootObject: build, + })); + const { dispatch, testHooks } = ls; + const { valToSlot } = testHooks; + await dispatch(makeStartVat(kser())); + log.length = 0; + + const rootA = 'o+0'; + + result = undefined; + await dispatch(makeMessage(rootA, 'store')); + t.is(result, 'ok'); + + const targetVref = valToSlot.get(target); + const storeVref = valToSlot.get(store); + const collectionID = Number(parseVatSlot(storeVref).subid); + const ordinalAssignmentKey = `vc.${collectionID}.|${targetVref}`; + const ordinalNumber = kvStore.get(ordinalAssignmentKey); + t.is(ordinalNumber, '1'); + const dataKey = `vc.${collectionID}.r0000000001:${targetVref}`; + const value = kvStore.get(dataKey); + t.deepEqual(JSON.parse(value), { body: '#123', slots: [] }); + + result = undefined; + await dispatch(makeMessage(rootA, 'clear')); + + // now simulate data from the broken version, by restoring the + // ordinal-assignment record, as if the code failed to delete it + + kvStore.set(ordinalAssignmentKey, '1'); + + // problem 1: store.has() should report "false", but incorrectly + // returns "true" + + result = undefined; + await dispatch(makeMessage(rootA, 'has')); + t.is(result, false); + + // problem 2: store.init() to re-add the old key should succeed, but + // incorrectly fails (because the store thinks the key is already + // present) + + result = undefined; + await dispatch(makeMessage(rootA, 'store')); + t.is(result, 'ok'); +}); diff --git a/packages/swingset-liveslots/test/gc-before-finalizer.test.js b/packages/swingset-liveslots/test/gc-before-finalizer.test.js index 2e073b4cd82c..cc460114febe 100644 --- a/packages/swingset-liveslots/test/gc-before-finalizer.test.js +++ b/packages/swingset-liveslots/test/gc-before-finalizer.test.js @@ -7,7 +7,12 @@ import { makeStartVat, makeMessage, makeBringOutYourDead } from './util.js'; import { makeMockGC } from './mock-gc.js'; const justGC = log => - log.filter(l => l.type === 'dropImports' || l.type === 'retireImports'); + log.filter( + l => + l.type === 'dropImports' || + l.type === 'retireImports' || + l.type === 'retireExports', + ); test('presence in COLLECTED state is not dropped yet', async t => { const { syscall, log } = buildSyscall(); @@ -109,12 +114,7 @@ test('presence in COLLECTED state is not dropped yet', async t => { ]); }); -// disabled until #9956 and #9959 are fixed, which interfere with this -// test - -/* - -test.failing('presence in COLLECTED state is not retired early', async t => { +test('presence in COLLECTED state is not retired early', async t => { const { syscall, log } = buildSyscall(); const gcTools = makeMockGC(); @@ -154,7 +154,7 @@ test.failing('presence in COLLECTED state is not retired early', async t => { let myWeakStore; function buildRootObject(vatPowers) { - const { VatData, WeakSet } = vatPowers; + const { VatData } = vatPowers; const { makeScalarBigMapStore, makeScalarBigWeakSetStore } = VatData; const store = makeScalarBigMapStore(); myWeakStore = makeScalarBigWeakSetStore(); @@ -187,10 +187,9 @@ test.failing('presence in COLLECTED state is not retired early', async t => { t.is(possiblyDeadSet.size, 0); t.is(possiblyRetiredSet.size, 0); - console.log(`-- starting`); // step 2: delete vdata ref to weakstore, make myPresence COLLECTED await dispatch(makeMessage('o+0', 'dropWeakStore', [])); - gcTools.kill(myPresence) + gcTools.kill(myPresence); log.length = 0; // weakstore is possiblyDead (NARRATORS VOICE: it's dead). Presence // is not, because the finalizer hasn't run. @@ -200,29 +199,31 @@ test.failing('presence in COLLECTED state is not retired early', async t => { // the empty weakref is still there t.true(slotToVal.has('o-1')); - // step 3: BOYD. It will collect myWeakStore on the first pass, // whose deleter should clear all entries, which will add its // recognized vrefs to possiblyRetiredSet. The post-pass will check // o-1 for reachability with slotToVal.has, and because that says it // is reachable, it will not be retired (even though it has no - // recognizer by now) - console.log(`-- doing first BOYD`); + // recognizer by now). + // + // *If* scanForDeadObjects() were mistakenly using getValForSlot() + // *instead of slotToVal.has(), we would see a retireImports here, + // *which would be a vat-fatal error, because we haven't seen a + // *dropImports yet. await dispatch(makeBringOutYourDead()); + // I tested this manually, by modifying boyd-gc.js *to use + // getValForSlot, and observed that this deepEqual(justGC(log), []) + // failed: it had an unexpected retireImports t.deepEqual(justGC(log), []); log.length = 0; // eventually, the finalizer runs gcTools.flushAllFRs(); - console.log(`-- doing second BOYD`); // *now* a BOYD will drop+retire await dispatch(makeBringOutYourDead()); - console.log(log); t.deepEqual(justGC(log), [ { type: 'dropImports', slots: ['o-1'] }, { type: 'retireImports', slots: ['o-1'] }, ]); }); - -*/ diff --git a/packages/swingset-liveslots/test/liveslots-mock-gc.test.js b/packages/swingset-liveslots/test/liveslots-mock-gc.test.js index d7cff5d0967f..1ab290c621b3 100644 --- a/packages/swingset-liveslots/test/liveslots-mock-gc.test.js +++ b/packages/swingset-liveslots/test/liveslots-mock-gc.test.js @@ -11,6 +11,7 @@ import { makeStartVat, makeBringOutYourDead, makeResolve, + makeRetireImports, } from './util.js'; import { makeMockGC } from './mock-gc.js'; @@ -465,3 +466,101 @@ for (const firstType of ['object', 'collection']) { } // test('double-free', doublefreetest, { firstType: 'object', lastType: 'collection', order: 'first->last' }); + +test('retirement', async t => { + const { syscall, fakestore, log } = buildSyscall(); + const gcTools = makeMockGC(); + + // A is a weak collection, with one entry, whose key is B (a + // Presence). We drop the RAM pillar for B and do a BOYD, which + // should provoke a syscall.dropImports. Then, when we delete A (by + // dropping the RAM pillar), the next BOYD should see a + // `syscall.retireImports`. + + let weakmapA; + let presenceB; + + function buildRootObject(vatPowers) { + const { VatData } = vatPowers; + const { makeScalarBigWeakMapStore } = VatData; + + weakmapA = makeScalarBigWeakMapStore(); + + return Far('root', { + add: p => { + presenceB = p; + weakmapA.init(presenceB, 'value'); + }, + }); + } + + const makeNS = () => ({ buildRootObject }); + const ls = makeLiveSlots(syscall, 'vatA', {}, {}, gcTools, undefined, makeNS); + const { dispatch, testHooks } = ls; + const { valToSlot } = testHooks; + + await dispatch(makeStartVat(kser())); + log.length = 0; + const weakmapAvref = valToSlot.get(weakmapA); + const { subid } = parseVatSlot(weakmapAvref); + const collectionID = String(subid); + + const rootA = 'o+0'; + const presenceBvref = 'o-1'; + await dispatch(makeMessage(rootA, 'add', [kslot(presenceBvref)])); + log.length = 0; + + // the fact that weakmapA can recognize presenceA is recorded in a + // vatstore key + const recognizerKey = `vom.ir.${presenceBvref}|${collectionID}`; + t.is(fakestore.get(recognizerKey), '1'); + + // tell mockGC that userspace has dropped presenceB + gcTools.kill(presenceB); + gcTools.flushAllFRs(); + + await dispatch(makeBringOutYourDead()); + const priorKey = `vom.ir.${presenceBvref}|`; + + t.deepEqual(log.splice(0), [ + // when a Presence is dropped, scanForDeadObjects can't drop the + // underlying vref import until it knows that virtual data isn't + // holding a reference, so we expect a refcount check + { type: 'vatstoreGet', key: `vom.rc.${presenceBvref}`, result: undefined }, + + // the vref is now in importsToDrop, but since this commonly means + // it can be retired too, scanForDeadObjects goes ahead and checks + // for recognizers + { type: 'vatstoreGetNextKey', priorKey, result: recognizerKey }, + + // it found a recognizer, so the vref cannot be retired + // yet. scanForDeadObjects finishes the BOYD by emitting the + // dropImports, but should keep watching for an opportunity to + // retire it too + { type: 'dropImports', slots: [presenceBvref] }, + ]); + + // now tell mockGC that we're dropping the weakmap too + gcTools.kill(weakmapA); + gcTools.flushAllFRs(); + + // this will provoke the deletion of the collection and all its + // data. It should *also* trigger a syscall.retireImports of the + // no-longer-recognizable key + await dispatch(makeBringOutYourDead()); + const retires = log.filter(e => e.type === 'retireImports'); + + t.deepEqual(retires, [{ type: 'retireImports', slots: [presenceBvref] }]); + + // If the bug is present, the vat won't send `syscall.retireImports` + // to the kernel. In a full system, that means the kernel can + // eventually send a `dispatch.retireImports` into the vat, if/when + // the object's hosting vat decides to drop it. Make sure that won't + // cause a crash. + + if (!retires.length) { + console.log(`testing kernel's dispatch.retireImports`); + await dispatch(makeRetireImports(presenceBvref)); + console.log(`dispatch.retireImports did not crash`); + } +}); diff --git a/packages/swingset-liveslots/test/weakset-dropped-remotable.test.js b/packages/swingset-liveslots/test/weakset-dropped-remotable.test.js new file mode 100644 index 000000000000..766182973704 --- /dev/null +++ b/packages/swingset-liveslots/test/weakset-dropped-remotable.test.js @@ -0,0 +1,50 @@ +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 { makeStartVat, makeMessage, makeBringOutYourDead } from './util.js'; +import { makeMockGC } from './mock-gc.js'; + +// Test for https://github.com/Agoric/agoric-sdk/issues/9956 + +test('delete remotable key from weakset', async t => { + const { syscall, log } = buildSyscall(); + const gcTools = makeMockGC(); + const rem = Far('remotable', {}); + + function buildRootObject(vatPowers) { + const { VatData } = vatPowers; + const { makeScalarBigWeakMapStore } = VatData; + const wms = makeScalarBigWeakMapStore(); + return Far('root', { + store: p => { + wms.init(rem, p); + gcTools.kill(p); + }, + }); + } + + const makeNS = () => ({ buildRootObject }); + const ls = makeLiveSlots(syscall, 'vatA', {}, {}, gcTools, undefined, makeNS); + const { dispatch } = ls; + await dispatch(makeStartVat(kser())); + + await dispatch(makeMessage('o+0', 'store', [kslot('o-1')])); + + // pretend the Remotable was dropped from RAM + log.length = 0; + gcTools.kill(rem); + gcTools.flushAllFRs(); + await dispatch(makeBringOutYourDead()); + + // that ought to emit a drop and retire for the presence + const gcCalls = log.filter( + l => l.type === 'dropImports' || l.type === 'retireImports', + ); + t.deepEqual(gcCalls, [ + { type: 'dropImports', slots: ['o-1'] }, + { type: 'retireImports', slots: ['o-1'] }, + ]); + log.length = 0; +});