From 5214237c8df40f98156519bff270a5927a83bc7e Mon Sep 17 00:00:00 2001 From: Brian Warner Date: Tue, 27 Aug 2024 15:06:52 -0700 Subject: [PATCH 1/3] test: augment collections test to exercise clear/has/re-init This was broken because of #8756 . The modified tests are marked as failing, until the fix is landed. --- .../test/collections.test.js | 20 +++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/packages/swingset-liveslots/test/collections.test.js b/packages/swingset-liveslots/test/collections.test.js index 9a70d129b95..9bebc238920 100644 --- a/packages/swingset-liveslots/test/collections.test.js +++ b/packages/swingset-liveslots/test/collections.test.js @@ -145,6 +145,14 @@ function exerciseMapOperations(t, collectionName, testStore) { `key "[Alleged: something missing]" not found in collection "${collectionName}"`, ), ); + if (collectionName === 'map') { + // strong map, so we can .clear + testStore.clear(); + for (const [key, _value] of stuff) { + t.false(testStore.has(key)); + } + fillBasicMapStore(testStore); + } } function exerciseSetOperations(t, collectionName, testStore) { @@ -172,9 +180,17 @@ function exerciseSetOperations(t, collectionName, testStore) { `key "[Alleged: something missing]" not found in collection "${collectionName}"`, ), ); + if (collectionName === 'set') { + // strong set, so we can .clear + testStore.clear(); + for (const [key, _value] of stuff) { + t.false(testStore.has(key)); + } + fillBasicSetStore(testStore); + } } -test('basic map operations', t => { +test.failing('basic map operations', t => { exerciseMapOperations( t, 'map', @@ -190,7 +206,7 @@ test('basic weak map operations', t => { ); }); -test('basic set operations', t => { +test.failing('basic set operations', t => { exerciseSetOperations( t, 'set', From 97e81f17d1e060f56114ddd7fc124f90df0695cb Mon Sep 17 00:00:00 2001 From: Brian Warner Date: Tue, 16 Jan 2024 15:50:37 -0800 Subject: [PATCH 2/3] 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, which skipped the refcount processing of keys, and removal of the "ordinal-assignment" record) * recognition records (vom.ir. keys) weren't created or removed for Remotable-style keys in weak collections * Presence-style vrefs were not submitted for retirement processing when deleting a weak collection As a result: * `collection.clear()` left the "ordinal-assignment"` records in the vatstore, causing subsequent .has(oldkey) to incorrectly return true, and .init(oldkey, newvalue) to throw a "already registered" error, as well as consuming DB space longer than necessary. * 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. * 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 * Allowing a weak virtual/durable collection to be garbage collected did not inform the kernel that the vat can no longer recognize the Presence-style keys, consuming c-list entries longer than necessary. This commit fixes those bugs, which fixes the immediate cause of: * fixes #8756 * fixes #7355 * fixes #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. This commit marks the new tests as expected to pass again. It adds one new (failing) test to demonstrate the lack of remediation code. Thanks to @mhofman and @gibson042 for recommendations. --- packages/swingset-liveslots/src/boyd-gc.js | 438 ++++++++----- .../src/collectionManager.js | 70 ++- .../src/virtualReferences.js | 101 ++- .../test/clear-collection.test.js | 586 ++++++++++++++++++ .../test/collections.test.js | 4 +- .../test/gc-before-finalizer.test.js | 28 +- .../test/liveslots-mock-gc.test.js | 99 +++ .../test/weakset-dropped-remotable.test.js | 50 ++ 8 files changed, 1164 insertions(+), 212 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/boyd-gc.js b/packages/swingset-liveslots/src/boyd-gc.js index 8f6c341039c..1b6d8e1b13d 100644 --- a/packages/swingset-liveslots/src/boyd-gc.js +++ b/packages/swingset-liveslots/src/boyd-gc.js @@ -1,65 +1,188 @@ import { parseVatSlot } from './parseVatSlots.js'; +/* + Theory of Operation (vref logical objects) + + Liveslots coordinates access to "logical objects", sometimes known + as "vref objects" because each one is named with a distinct vref + identifier (o+1, o-2, o+d3/4, etc). The SwingSet kernel coordinates + access to the objects between vats, using a kref identifier (ko56), + but vats never see krefs. + + When vat code (written in JavaScript) needs to interact with a + logical object, it needs some JS `Object` to pass in arguments, or + to invoke methods upon, or to receive in the arguments of a method + call. We use Presence objects for imported vrefs, Remotable/heap + objects for locally-created (potentially-exported) ephemeral + objects, and Representatives for virtual/durable objects initially + created by calling the Kind's maker function. Liveslots is + responsible for recognizing these `Object` objects when they appear + at its doorstep (e.g. in a remote method send, or when assigned to + the `state` of a virtual object), and knowing which vref is being + referenced. Liveslots is also the one to create all Presences and + Representatives (vat code creates the Remotable/heap objects, by + calling `Far()`). + + For garbage-collection purposes, liveslots tracks the "reachable" + and "recognizable" status for these logical objects. A logical + object is either VREF-REACHABLE, VREF-RECOGNIZABLE, or + nothing. Strong references from a VREF-REACHABLE object makes the + target also VREF-REACHABLE. Weak references from a VREF-REACHABLE + makes the target at least VREF-RECOGNIZABLE, although it might be + VREF-REACHABLE if there is also a strong reference to it. + + Weak collections provide weak access to their keys, and strong + access to their values. Strong collections provide strong access to + both keys and values. Virtual/durable objects provide strong access + to their `state`. The "reachable set" is the transitive closure of + strong edges, given a set of anchors. There will also be a + "recognizable set", with weak edges from some members of the + reachable set. + + Liveslots needs to keep track of the reachable or recognizable + status of logical objects to limit storage consumption. It must + preserve all reachable data, but it can delete data that can no + longer be reached. Liveslots also needs to coordinate status updates + with the kernel, to enable the kernel to do the same. + + The logical object for an imported vref (known as a "Presence-style + vref", o-NN) can be kept alive by either the existence of an actual + Presence `Object`, or by a (strong) virtual/durable collection that + uses the vref as its key or inside its value, or by a (weak) + collection that uses the vref inside its value, or by a + virtual/durable object that uses the vref inside its `state`. We + call the Presence the "RAM pillar", and the virtual/durable + references the "vdata pillar". The "vdata pillar" is tracked as a + record in the vatstore DB. We say the logical object is + VREF-REACHABLE if either pillar is up, and it becomes + VREF-UNREACHABLE if all pillars are down. + + The logical object for a virtual/durable object (known as + "Representative-style", o+vKK/II or o+dKK/II) can be kept alive by + any of three pillars: the Representative (RAM pillar), a + virtual/durable collection or object state (vdata pillar), or by + virtue of being exported to the kernel (export pillar). Sending a + Representative in a message to some other vat will cause the vref to + be exported, and the kernel will tell us if/when that other vat ever + drops their strong reference, which will then drop the export + pillar. The "export status" for a vref is one of EXPORT-REACHABLE, + EXPORT-RECOGNIZABLE, or nothing. + + The third category of vrefs (o+NN) are for locally-created ephmeral + objects, created by calling `Far()`. Liveslots frequently calls + these "Remotable-style vrefs", although "heap" might be a less + ambiguous term. They have only the RAM pillar: the lifetime of the + logical object is the same as that of the "Remotable" heap value. + + Once all pillars for a logical object have dropped, liveslots needs + to delete the logical object. For virtual/durable objects, this + means deleting the `state` data, and since the state can hold + references to other objects, it means decrementing refcounts and + dropping vdata pillars, which might trigger more deletions. For + logical objects that have been imported from the kernel, it must + also notify the kernel of the transition from "reachable" status to + "unreachable", so the kernel can propagate the event outwards to the + upstream vat that exported the object, deleting table entries all + the way. + + Objects which are unreachable by our vat might still be reachable by + others, or by something in the exporting vat, so becoming + unreachable is not the end of the story. The logical object might + still be *recognizable* by our vat, as a key in one or more weak + collections. While in this VREF-RECOGNIZABLE state, three things + might happen: + + * The owner, or someone who still has access, may send it to us in a + message. The vat code receives a newly-minted Presence or + Representative object, and now the logical object is + VREF-REACHABLE once more. + * The owner may declare the object "retired", meaning they've + deleted it. We should drop our matching WeakMap entries, and free + their data (which might make other objects become + unreachable). This is triggered by "dispatch.retireImports". + * We might delete our last WeakMap that recognized the vref, + allowing us to tell the kernel that we don't care about the vref + anymore, so it can remove the bookkeeping records. This uses + "possiblyRetiredSet", and may invoke "syscall.retireImports". + + We track recognizability status using "recognizer records". When the + recognizer is a virtual/durable collection or object, the record is + stored in the vatstore DB. + + The kernel tracks the vat's imported vrefs with an "import status", + one of IMPORT-REACHABLE, IMPORT-RECOGNIZABLE, or nothing. This + status is stored in the c-list entry, and is not visible to the + vat. It changes when the vat receives a vref in a delivery, or + performs syscall.dropImports or syscall.retireImports, or receives a + dispatch.retireImports. + /* Theory of Operation (imports/Presences) - Imports are in one of 5 states: UNKNOWN, REACHABLE, UNREACHABLE, - COLLECTED, FINALIZED. Note that there's no actual state machine with those - values, and we can't observe all of the transitions from JavaScript, but - we can describe what operations could cause a transition, and what our - observations allow us to deduce about the state: - - * UNKNOWN moves to REACHABLE when a crank introduces a new import - * userspace holds a reference only in REACHABLE - * REACHABLE moves to UNREACHABLE only during a userspace crank - * UNREACHABLE moves to COLLECTED when GC runs, which queues the finalizer - * COLLECTED moves to FINALIZED when a new turn runs the finalizer - * liveslots moves from FINALIZED to UNKNOWN by syscalling dropImports - - convertSlotToVal either imports a vref for the first time, or - re-introduces a previously-seen vref. It transitions from: - - * UNKNOWN to REACHABLE by creating a new Presence - * UNREACHABLE to REACHABLE by re-using the old Presence that userspace - forgot about - * COLLECTED/FINALIZED to REACHABLE by creating a new Presence - - Our tracking tables hold data that depends on the current state: - - * slotToVal holds a WeakRef in [REACHABLE, UNREACHABLE, COLLECTED] - * that WeakRef .deref()s into something in [REACHABLE, UNREACHABLE] - * possiblyDeadSet holds the vref only in FINALIZED - * (TODO) re-introduction could remove the vref from possiblyDeadSet - - Each state thus has a set of perhaps-measurable properties: - - * UNKNOWN: slotToVal[baseRef] is missing, baseRef not in possiblyDeadSet - * REACHABLE: slotToVal has live weakref, userspace can reach - * UNREACHABLE: slotToVal has live weakref, userspace cannot reach - * COLLECTED: slotToVal[baseRef] has dead weakref - * FINALIZED: slotToVal[baseRef] is missing, baseRef is in possiblyDeadSet - - Our finalizer callback is queued by the engine's transition from - UNREACHABLE to COLLECTED, but the baseRef might be re-introduced before the - callback has a chance to run. There might even be multiple copies of the - finalizer callback queued. So the callback must deduce the current state - and only perform cleanup (i.e. delete the slotToVal entry and add the - baseRef to possiblyDeadSet) in the COLLECTED state. - - Our general rule is "trust the finalizer". The GC code below - considers a Presence to be reachable (the vref's "RAM pillar" - remains "up") until it moves to the FINALIZED state. We do this to - avoid race conditions between some other pillar dropping (and a - BOYD sampling the WeakRef) while it is in the COLLECTED state. If - we treated COLLECTED as unreachable,, then the subsequent - finalizer callback would examine the vref a second time, - potentially causing a vat-fatal double syscall.dropImports. This - rule may change if/when we use FinalizationRegistry better, by - explicitly de-registering the vref when we drop it, which JS - guarantees will remove and pending callback from the queue. This - may help us avoid probing the WeakRef during BOYD (instead relying - upon the fired/not-fired state of the FR), since that probing can - cause engines to retain objects longer than necessary. + This describes the states that a Presence `Object` might be in, and + how we track changes in that status. The Presence forms the "RAM + pillar" that may support the logical object (vref). + + A Presence object is in one of 5 states: UNKNOWN, REACHABLE, + UNREACHABLE, COLLECTED, FINALIZED. Note that there's no actual state + machine with those values, and we can't observe all of the + transitions from JavaScript, but we can describe what operations + could cause a transition, and what our observations allow us to + deduce about the state: + + * UNKNOWN moves to REACHABLE when a crank introduces a new import + * userspace holds a reference only in REACHABLE + * REACHABLE moves to UNREACHABLE only during a userspace crank + * UNREACHABLE moves to COLLECTED when engine GC runs, which queues the finalizer + * COLLECTED moves to FINALIZED when a new turn runs the finalizer + * FINALIZED moves to UNKNOWN when liveslots sends a dropImports syscall + + convertSlotToVal either imports a vref for the first time, or + re-introduces a previously-seen vref. It transitions from: + + * UNKNOWN to REACHABLE by creating a new Presence + * UNREACHABLE to REACHABLE by re-using the old Presence that userspace + forgot about + * COLLECTED/FINALIZED to REACHABLE by creating a new Presence + + Our tracking tables hold data that depends on the current state: + + * slotToVal holds a WeakRef only in [REACHABLE, UNREACHABLE, COLLECTED] + * that WeakRef .deref()s into something only in [REACHABLE, UNREACHABLE] + * possiblyDeadSet holds the vref only in FINALIZED + * (TODO) re-introduction could remove the vref from possiblyDeadSet + + Each state thus has a set of perhaps-measurable properties: + + * UNKNOWN: slotToVal[baseRef] is missing, baseRef not in possiblyDeadSet + * REACHABLE: slotToVal[baseRef] has live weakref, userspace can reach + * UNREACHABLE: slotToVal[baseRef] has live weakref, userspace cannot reach + * COLLECTED: slotToVal[baseRef] has dead weakref + * FINALIZED: slotToVal[baseRef] is missing, baseRef is in possiblyDeadSet + + Our finalizer callback is queued by the engine's transition from + UNREACHABLE to COLLECTED, but the baseRef might be re-introduced + before the callback has a chance to run. There might even be + multiple copies of the finalizer callback queued. So the callback + must deduce the current state and only perform cleanup (i.e. delete + the slotToVal entry and add the baseRef to possiblyDeadSet) in the + COLLECTED state. + + Our general rule is "trust the finalizer". The GC code below + considers a Presence to be reachable (the vref's "RAM pillar" + remains "up") until it moves to the FINALIZED state. We do this to + avoid race conditions between some other pillar dropping (and a BOYD + sampling the WeakRef) while it is in the COLLECTED state. If we + treated COLLECTED as the RAM pillar being "down"), then the + subsequent finalizer callback would examine the vref a second time, + potentially causing a vat-fatal double syscall.dropImports. This + rule may change if/when we use FinalizationRegistry better, by + explicitly de-registering the vref when we drop it, which JS + guarantees will remove and pending callback from the queue. This may + help us avoid probing the WeakRef during BOYD (instead relying upon + the fired/not-fired state of the FR), since that probing can cause + engines to retain objects longer than necessary. + */ /* @@ -74,26 +197,28 @@ import { parseVatSlot } from './parseVatSlots.js'; vat can reach, because the vref might be held in virtual/durable data ("vdata") while the in-RAM `Presence` object was dropped. Likewise the in-RAM `Representative` can be dropped while - the o+dKK/II vref is kept REACHABLE by either vdata or an export to - the kernel. We *do* always have a Remotable for every o+NN vref that - the vat knows about, because Remotables are ephemeral. - - We aren't recording any information about the kernel-facing import - status (c-list state) for Presence-style vrefs (o-NN), so we rely - upon the invariant that you only add a vref to possiblyDeadSet if it - was REACHABLE first. That way, possiblyDeadSet.has(vref) means that - the import status was REACHABLE. Likewise, code should only add to - possiblyRetiredSet if the vref was at least RECOGNIZABLE - beforehand. This helps us avoid a vat-fatal duplicate dropImports or - retireImports syscall. + the o+dKK/II vref is kept VREF-REACHABLE by either vdata or an + export to the kernel. We *do* always have a Remotable for every o+NN + vref that the vat knows about, because Remotables are ephemeral. + + The vat does not record any information about the kernel-facing + import status (c-list state) for Presence-style vrefs (o-NN), and + cannot ask the kernel for it, so we rely upon the invariant that you + only add a vref to possiblyDeadSet if it was VREF-REACHABLE + first. That way, possiblyDeadSet.has(vref) means that the c-list + import status was IMPORT-REACHABLE. Likewise, code should only add + to possiblyRetiredSet if the vref was at least VREF-RECOGNIZABLE + beforehand, meaning the c-list status was at least + IMPORT-RECOGNIZABLE. This helps us avoid a vat-fatal duplicate + dropImports or retireImports syscall. For imports, the lifetime is controlled by the upstream vat: we - might drop REACHABLE today and maintain RECOGNIZABLE for days until - the object is finally retired. For exports *we* control the - lifetime, so when we determine an export is no longer REACHABLE, we - delete it and retire the vref immediately, and it does not - observably linger in the RECOGNIZABLE state. This simplifies our - tracking, and allows the deletion of Remotables and + might drop VREF-REACHABLE today and maintain VREF-RECOGNIZABLE for + days until the object is finally retired. For exports *we* control + the lifetime, so when we determine an export is no longer + VREF-REACHABLE, we delete it and retire the vref immediately, and it + does not observably linger in the VREF-RECOGNIZABLE state. This + simplifies our tracking, and allows the deletion of Remotables and Representative-type vrefs to be idempotent. Each vref's reachability status is determined by a set of @@ -118,19 +243,19 @@ import { parseVatSlot } from './parseVatSlots.js'; away. We don't do this for Representatives because it would violate our "don't use RAM for inactive virtual objects" rule. - When an imported Presence becomes UNREACHABLE, it might still be - RECOGNIZABLE, by virtue of being the key of one or more weak - collections. If not, it might transit from REACHABLE directly to - NONE. The code that reacts to the UNREACHABLE transition must check - for recognizers, and do a retireImports right away if there are - none. Otherwise, recognizer will remain until either the kernel - retires the object (dispatch.retireImports), or the weak collection - is deleted, in which case possiblyRetiredSet will be updated with - the vref that might no longer be recognized. There will be a race - between us ceasing to recognize the vref (which should trigger a - syscall.retireImports), and the kernel telling us the object has - been deleted (via dispatch.retireImports). Either one must inhibit - the other. + When an imported Presence becomes VREF-UNREACHABLE, it might still + be VREF-RECOGNIZABLE, by virtue of being the key of one or more weak + collections. If not, it might transit from VREF-REACHABLE directly + to NONE. The code that reacts to the VREF-UNREACHABLE transition + must check for recognizers, and do a retireImports right away if + there are none. Otherwise, recognizer will remain until either the + kernel retires the object (dispatch.retireImports), or the weak + collection is deleted, in which case possiblyRetiredSet will be + updated with the vref that might no longer be recognized. There will + be a race between us ceasing to recognize the vref (which should + trigger a syscall.retireImports), and the kernel telling us the + object has been deleted (via dispatch.retireImports). Either one + must inhibit the other. possiblyRetiredSet only cares about Presence-style vrefs, because they represent imports, whose lifetime is not under our control. The @@ -139,17 +264,18 @@ import { parseVatSlot } from './parseVatSlots.js'; them. We use slotToVal.has(vref) everywhere for our "is it still - reachable" check, which returns true for the REACHABLE / UNREACHABLE - / COLLECTED states, and false for the FINALIZED state. In contrast, - getValForSlot(vref) returns false for both COLLECTED and - FINALIZED. We want COLLECTED to qualify as "still reachable" because - it means there's still a finalizer callback queued, which will be - run eventually, and we need that callback to not trigger a duplicate - drop. We use slotToVal.has() in the possiblyRetiredSet loop (to - inhibit retirement of imported vrefs whose Presences are in the - COLLECTED state, and which just lost a recognizer), because - getValForSlot would allow such COLLECTED vrefs to be retired, even - before the finalizer had fired and could do a dropImports. + reachable" check, which returns true for the Presence's REACHABLE / + UNREACHABLE / COLLECTED states, and false for the FINALIZED + state. In contrast, getValForSlot(vref) returns false for both + COLLECTED and FINALIZED. We want COLLECTED to qualify as "still + reachable" because it means there's still a finalizer callback + queued, which will be run eventually, and we need that callback to + not trigger a duplicate drop. We use slotToVal.has() in the + possiblyRetiredSet loop (to inhibit retirement of imported vrefs + whose Presences are in the COLLECTED state, and which just lost a + recognizer), because getValForSlot would allow such COLLECTED vrefs + to be retired, even before the finalizer had fired and could do a + dropImports. When we decide to delete a virtual object, we will delete its `state`, decrementing the refcounts of any objects therein, which @@ -213,6 +339,7 @@ import { parseVatSlot } from './parseVatSlots.js'; * outer loop * gcAndFinalize * sort possiblyDeadSet, examine each by type + * all: remove from possiblyDeadSet * presence (vref): * if unreachable: * dropImports @@ -221,10 +348,9 @@ import { parseVatSlot } from './parseVatSlots.js'; * if unreachable: * retireExports if kernelRecognizableRemotables * vrm.ceaseRecognition - * VOM (baseref) + * VOM (baseRef) * if unreachable: * deleteVirtualObject (and retireExports retirees) - * all: remove from possiblyDeadSet * repeat loop if gcAgain or possiblyDeadSet.size > 0 * now sort and process possiblyRetiredSet. for each: * ignore unless presence @@ -240,10 +366,10 @@ import { parseVatSlot } from './parseVatSlots.js'; faceted `behavior` argument), each time userspace create a new instance, we create a full "cohort" of facets, passing a record of Representative objects (one per facet) back to the caller. Each - facet gets is own vref, but they all share a common prefix, known as - the "baseRef". For example, `o+d44/2` is a BaseRef for a cohort, the - second instance created of the `o+d44` Kind, whose individual facets - would have vrefs of `o+d44/2:0` and `o+d44/2:1`. + facet gets its own vref, but they all share a common prefix, known + as the "baseRef". For example, `o+d44/2` is a BaseRef for a cohort, + the second instance created of the `o+d44` Kind, whose individual + facets would have vrefs of `o+d44/2:0` and `o+d44/2:1`. We use a WeakMap to ensure that holding any facet will keep all the others alive, so the cohort lives and dies as a group. The GC @@ -254,17 +380,21 @@ import { parseVatSlot } from './parseVatSlots.js'; `valToSlot` is keyed by the facets, and its values are the individual facet's vref. + For Presence- and Remotable- style objects, the baseRef is just the + vref (i.e., every baseRef is either a cohort-identifying prefix or + an isolated-object vref, and every vref either has a baseRef prefix + and identifies one facet of a cohort or has no such prefix and + identifies an isolated object). + Most of the GC-related APIs that appear here take vrefs, but the exceptions are: - * slotToVal is keyed by baseRef - * possiblyDeadSet holds baseRefs, that's what our WeakRefs track + * slotToVal is keyed by BaseRef + * possiblyDeadSet holds BaseRefs, that's what our WeakRefs track * vrm.isVirtualObjectReachable takes baseRef * vrm.deleteVirtualObject takes baseRef, returns [bool, retireees=vrefs] * vrm.ceaseRecognition takes either baseRef or vref (if given a baseRef, it will process all the facets) - For Presence- and Remotable- style objects, the vref and baseref are - the same. */ export const makeBOYDKit = ({ @@ -276,20 +406,26 @@ export const makeBOYDKit = ({ possiblyDeadSet, possiblyRetiredSet, }) => { - // Presence (o-NN) lifetimes are controlled by the upstream vat, or - // the kernel. If the vref was in possiblyDeadSet, then it *was* - // reachable before, so we can safely presume the kernel to think we - // can reach it. + // Representative (o+dNN/II or o+vNN/II) lifetimes are also + // controlled by us. We allow the Representative object to go away + // without deleting the vref, so we must track all three pillars: + // Representative (RAM), export, and vdata. When we decide the vref + // is unreachable, we must delete the virtual object's state, as + // well as retiring the object (by telling the kernel it has been + // retired, if the kernel cares, and removing any local recognition + // records). - const checkImportPresence = vref => { - // RAM pillar || vdata pillar - const isReachable = slotToVal.has(vref) || vrm.isPresenceReachable(vref); - // use slotToVal.has, not getSlotForVal(), to avoid duplicate drop - let dropImport; + const checkExportRepresentative = baseRef => { + // RAM pillar || (vdata pillar || export pillar) + const isReachable = + slotToVal.has(baseRef) || vrm.isVirtualObjectReachable(baseRef); + let gcAgain = false; + let exportsToRetire = []; if (!isReachable) { - dropImport = vref; + // again, we own the object, so we retire it immediately + [gcAgain, exportsToRetire] = vrm.deleteVirtualObject(baseRef); } - return { dropImport }; + return { gcAgain, exportsToRetire }; }; // Remotable (o+NN) lifetimes are controlled by us: we delete/retire @@ -302,7 +438,7 @@ export const makeBOYDKit = ({ // recognition records, and to inform the kernel with a // retireExports if kernelRecognizableRemotables says that the // kernel still cares. - + // // note: we track export status for remotables in the // kernelRecognizableRemotables set, not vom.es.VREF records. We // don't currently track recognition records with @@ -329,26 +465,20 @@ export const makeBOYDKit = ({ return { gcAgain, exportsToRetire }; }; - // Representative (o+dNN/II or o+vNN/II) lifetimes are also - // controlled by us. We allow the Representative object to go away - // without deleting the vref, so we must track all three pillars: - // Representative (RAM), export, and vdata. When we decide the vref - // is unreachable, we must delete the virtual object's state, as - // well as retiring the object (by telling the kernel it has been - // retired, if the kernel cares, and removing any local recognition - // records). + // Presence (o-NN) lifetimes are controlled by the upstream vat, or + // the kernel. If the vref was in possiblyDeadSet, then it *was* + // reachable before, so we can safely presume the kernel to think we + // can reach it. - const checkExportRepresentative = baseRef => { - // RAM pillar || (vdata pillar || export pillar) - const isReachable = - slotToVal.has(baseRef) || vrm.isVirtualObjectReachable(baseRef); - let gcAgain = false; - let exportsToRetire = []; + const checkImportPresence = vref => { + // RAM pillar || vdata pillar + // use slotToVal.has, not getSlotForVal(), to avoid duplicate drop + const isReachable = slotToVal.has(vref) || vrm.isPresenceReachable(vref); + let dropImport; if (!isReachable) { - // again, we own the object, so we retire it immediately - [gcAgain, exportsToRetire] = vrm.deleteVirtualObject(baseRef); + dropImport = vref; } - return { gcAgain, exportsToRetire }; + return { dropImport }; }; const scanForDeadObjects = async () => { @@ -407,7 +537,7 @@ export const makeBOYDKit = ({ // Deleting virtual object state, or freeing weak-keyed // collection entries, might shake loose more // objects. possiblyDeadSet and possiblyRetiredSet are added - // when vdata vrefs are decreffed to zero, and gcAgain means that + // when a vdata vref decrefs to zero, and gcAgain means that // something in RAM might now be free. In both cases we should // do another pass, including gcAndFinalize(), until we've // cleared everything we can. @@ -432,22 +562,20 @@ export const makeBOYDKit = ({ possiblyRetiredSet.delete(vref); const parsed = parseVatSlot(vref); assert.equal(parsed.type, 'object', vref); - // not allocated by us? that makes it a Presence-type - if (!parsed.allocatedByVat) { - // if we're dropping the vref, checkImportPresence() already - // did our isReachable check, so we can safely skip it (and - // save a vatstore syscall) - const isReachable = - !importsToDrop.has(vref) && - (slotToVal.has(vref) || vrm.isPresenceReachable(vref)); - // note: the old version used getValForSlot, which treats - // COLLECTED as unreachable, which would allow a retirement at - // the wrong time (before the finalizer fires and does the - // dropImport) - const isRecognizable = isReachable || vrm.isVrefRecognizable(vref); - if (!isRecognizable) { - importsToRetire.add(vref); - } + // ignore non-Presences + if (parsed.allocatedByVat) continue; + + // if we're dropping the vref, checkImportPresence() already + // did our isReachable check, so we can safely skip it (and + // save a vatstore syscall) + const isReachable = + !importsToDrop.has(vref) && + // Use slotToVal.has, not getValForSlot(), to avoid retirement + // before the finalizer fires and does dropImport + (slotToVal.has(vref) || vrm.isPresenceReachable(vref)); + const isRecognizable = isReachable || vrm.isVrefRecognizable(vref); + if (!isRecognizable) { + importsToRetire.add(vref); } } diff --git a/packages/swingset-liveslots/src/collectionManager.js b/packages/swingset-liveslots/src/collectionManager.js index 8aa7ddd6e57..db692165919 100644 --- a/packages/swingset-liveslots/src/collectionManager.js +++ b/packages/swingset-liveslots/src/collectionManager.js @@ -4,7 +4,6 @@ import { zeroPad, makeEncodePassable, makeDecodePassable, - isEncodedRemotable, compareRank, } from '@endo/marshal'; import { @@ -66,6 +65,12 @@ function prefixc(collectionID, dbEntryKey) { return `vc.${collectionID}.${dbEntryKey}`; } +export const collectionMetaKeys = new Set([ + '|entryCount', + '|nextOrdinal', + '|schemata', +]); + /** * @typedef {object} SchemaCacheValue * @property {Pattern} keyShape @@ -293,6 +298,20 @@ export function makeCollectionManager( return `${dbKeyPrefix}${dbEntryKey}`; } + // A "vref" is a string like "o-4" or "o+d44/2:0" + // An "EncodedKey" is the output of encode-passable: + // * strings become `s${string}`, like "foo" -> "sfoo" + // * small positive BigInts become `p${len}:${digits}`, like 47n -> "p2:47" + // * refs are assigned an "ordinal" and use `r${fixedLengthOrdinal}:${vref}` + // * e.g. vref(o-4) becomes "r0000000001:o-4" + // A "DBKey" is used to index the vatstore. DBKeys for collection + // entries join a collection prefix and an EncodedKey. Some + // possible DBKeys for entries of collection "5", using collection + // prefix "vc.5.", are: + // * "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 +327,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 +367,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)) { @@ -553,26 +579,38 @@ export function makeCollectionManager( */ function clearInternalFull() { let doMoreGC = false; - const [coverStart, coverEnd] = getRankCover(M.any(), encodeKey); - const start = prefix(coverStart); - const end = prefix(coverEnd); - - // this yields all keys for which (start <= key < end) - for (const dbKey of enumerateKeysStartEnd(syscall, start, end)) { - const value = JSON.parse(syscall.vatstoreGet(dbKey)); - doMoreGC = - value.slots.map(vrm.removeReachableVref).some(b => b) || doMoreGC; - syscall.vatstoreDelete(dbKey); - if (isEncodedRemotable(dbKey)) { - const keyVref = vrefFromDBKey(dbKey); + + // visit every DB entry associated with the collection, which + // (due to sorting) will be collection entries first, and then a + // mixture of ordinal-assignment mappings and size-independent + // metadata (both of which start with "|"). + for (const dbKey of enumerateKeysWithPrefix(syscall, dbKeyPrefix)) { + const encodedKey = dbKeyToEncodedKey(dbKey); + + // preserve general metadata ("|entryCount" and friends are + // cleared by our caller) + if (collectionMetaKeys.has(encodedKey)) continue; + + if (encodedKey.startsWith('|')) { + // ordinal assignment; decref or de-recognize its vref + const keyVref = encodedKey.substring(1); + parseVatSlot(keyVref); if (hasWeakKeys) { vrm.removeRecognizableVref(keyVref, `${collectionID}`, true); } else { doMoreGC = vrm.removeReachableVref(keyVref) || doMoreGC; } - syscall.vatstoreDelete(prefix(`|${keyVref}`)); + } else { + // a collection entry; decref slots from its value + const value = JSON.parse(syscall.vatstoreGet(dbKey)); + doMoreGC = + value.slots.map(vrm.removeReachableVref).some(b => b) || doMoreGC; } + + // in either case, delete the DB entry + syscall.vatstoreDelete(dbKey); } + return doMoreGC; } diff --git a/packages/swingset-liveslots/src/virtualReferences.js b/packages/swingset-liveslots/src/virtualReferences.js index 09f78ff2ac9..ffe0daca3bb 100644 --- a/packages/swingset-liveslots/src/virtualReferences.js +++ b/packages/swingset-liveslots/src/virtualReferences.js @@ -500,26 +500,51 @@ 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). + * + * Each voAwareWeakMap/Set must have a distinct recognizer, so we + * can remove the key from the right ones. The recognizer is held + * strongly by the recognition record, so it must not be the + * voAwareWeakMap/Set itself (which would inhibit GC). + * + * 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 +560,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 +589,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 +623,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 00000000000..313f0d941df --- /dev/null +++ b/packages/swingset-liveslots/test/clear-collection.test.js @@ -0,0 +1,586 @@ +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)); +}); + +// This test is marked as failing because we do not have any +// remediation code for #8756. Collections which were cleared before +// the fix will be corrupted, such that the old keys appear to still +// be present, even after the fix has been applied. This test +// demonstrates that we can still *not* handle the following sequence: +// +// * (in old version, without fix for #8756): +// * const c = makeScalarBigMapStore(); +// * const key = Far(); // or any remotable +// * c.add(key, 'value'); +// * c.clear(); +// * (then in new version, with fix) +// * assert.equal(c.has(key), false); +// * c.init(key, 'new value'); + +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. See if this will 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'); + + // other likely problems: store.keys() will report the old key, + // store.get(oldkey) will probably crash +}); diff --git a/packages/swingset-liveslots/test/collections.test.js b/packages/swingset-liveslots/test/collections.test.js index 9bebc238920..c8dd6e0d9f9 100644 --- a/packages/swingset-liveslots/test/collections.test.js +++ b/packages/swingset-liveslots/test/collections.test.js @@ -190,7 +190,7 @@ function exerciseSetOperations(t, collectionName, testStore) { } } -test.failing('basic map operations', t => { +test('basic map operations', t => { exerciseMapOperations( t, 'map', @@ -206,7 +206,7 @@ test('basic weak map operations', t => { ); }); -test.failing('basic set operations', t => { +test('basic set operations', t => { exerciseSetOperations( t, 'set', diff --git a/packages/swingset-liveslots/test/gc-before-finalizer.test.js b/packages/swingset-liveslots/test/gc-before-finalizer.test.js index b0b5f7c7c19..92a0955fb3d 100644 --- a/packages/swingset-liveslots/test/gc-before-finalizer.test.js +++ b/packages/swingset-liveslots/test/gc-before-finalizer.test.js @@ -114,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(); @@ -160,7 +155,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(); @@ -193,10 +188,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. @@ -206,29 +200,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 d7cff5d0967..1ab290c621b 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 00000000000..76618297370 --- /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; +}); From 685e62982feea8cf69e60d5c0cbbad887b7bd49d Mon Sep 17 00:00:00 2001 From: Brian Warner Date: Fri, 30 Aug 2024 17:01:34 -0700 Subject: [PATCH 3/3] test: note liveslots bug in collection.clear() and getSize() refs #10007 --- .../src/collectionManager.js | 1 + .../test/collections.test.js | 27 +++++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/packages/swingset-liveslots/src/collectionManager.js b/packages/swingset-liveslots/src/collectionManager.js index db692165919..8ee66541b48 100644 --- a/packages/swingset-liveslots/src/collectionManager.js +++ b/packages/swingset-liveslots/src/collectionManager.js @@ -624,6 +624,7 @@ export function makeCollectionManager( } } if (!hasWeakKeys && !isDeleting) { + // TODO: broken, see #10007 syscall.vatstoreSet(prefix('|entryCount'), '0'); } return doMoreGC; diff --git a/packages/swingset-liveslots/test/collections.test.js b/packages/swingset-liveslots/test/collections.test.js index c8dd6e0d9f9..d63192b025c 100644 --- a/packages/swingset-liveslots/test/collections.test.js +++ b/packages/swingset-liveslots/test/collections.test.js @@ -503,6 +503,20 @@ test('map clear', t => { t.is(testStore.getSize(), 0); }); +// see #10007 +test.failing('map clear with pattern', t => { + const testStore = makeScalarBigMapStore('cmap', { keyShape: M.any() }); + testStore.init('a', 'ax'); + testStore.init('b', 'bx'); + testStore.init('c', 'cx'); + console.log(`M is`, M); + testStore.clear(M.eq('c')); + t.true(testStore.has('a')); + t.true(testStore.has('b')); + t.false(testStore.has('c')); + t.is(testStore.getSize(), 2); +}); + test('set clear', t => { const testStore = makeScalarBigSetStore('cset', { keyShape: M.any() }); testStore.add('a'); @@ -515,6 +529,19 @@ test('set clear', t => { t.is(testStore.getSize(), 0); }); +// see #10007 +test.failing('set clear with pattern', t => { + const testStore = makeScalarBigSetStore('cset', { keyShape: M.any() }); + testStore.add('a'); + testStore.add('b'); + testStore.add('c'); + testStore.clear(M.eq('c')); + t.true(testStore.has('a')); + t.true(testStore.has('b')); + t.false(testStore.has('c')); + t.is(testStore.getSize(), 2); +}); + test('map fail on concurrent modification', t => { const primeMap = makeScalarBigMapStore('fmap', { keyShape: M.number(),