liveslots leaks slotToVal entries for Promises when settled before refcount=0

[warner]

This is the underlying bug that causes #10756. We have a quick workaround for that consequence, so it is tracked separately, but this ticket is for the deeper issue that may take more time to resolve.

Liveslots needs to track the mapping from vref (really vpid) to Promise object, at least until the Promise is settled (fulfilled or rejected), so that serialization and deserialization will get the right vpid or Promise. Unlike with object-style vrefs (Presences, Representatives, Far()/"Remotable"s), we don't try detect when the Promise object becomes unreachable by user code (with a FinalizationRegistry). It would be nice to drop the c-list entry when the vat no longer cares about the Promise, but we don't get enough information from the JS engine to do that safely: userspace might have called .then to add a callback and then dropped the Promise, in which case liveslots must remain subscribed so it can call the fulfill/reject functions at the right time.

So our rule is that we retain the Promise (and thus a stable vpid-Promise mapping) until it settles, after which deserialization is allowed to return any correctly-settled Promise, and serialization is not obligated to return the original vpid. (If we could somehow hook .then, then GCing Promises might be possible, as would stable serialization of settled Promises, ala #2381, which is related to auxdata in #2069).

The vpid-to-Promise mapping is held in the slotToVal, which (for the benefit of objects) has values which are WeakRefs (so it's really a vpid-to-WeakRef-to-Promise mapping). This lets liveslots deserialize correctly/stably while the Presence/Representative is still around, but create a replacement Presence/Representative when necessary. We use WeakRef values for all vrefs, both objects and promises, which means we need some strong reference to keep the Promises alive.

To manage that, liveslots uses a pair of (strong) vpid-to-Promise Maps: exportedVPIDs and importedVPIDs, for "exported" promises (decided by this vat, first appeared in data of outbound syscall.send or syscall.resolve), and "imported" promises (decided by the kernel, first appeared in the data of inbound dispatch.deliver or dispatch.notify). These keep the Promise alive until settled, and (for importedVPIDs) holds the resolve/reject functions needed to service a dispatch.notify when the kernel notifies the vat of the vpid being settled.

We remove the vpid/Promise pair from those Maps when the promise settles. However, the valToSlot entry is not necessarily removed at that time. If the vpid also appears in virtual-data, we need to retain the ability to deserialize that data later, and obtain an equivalent Promise.

(note that such Promises can only appear in the state of a merely-virtual object, or in the value of a merely-virtual collection. Fully durably objects or collections cannot hold Promises, because Promises are not durable. The durable promise watcher has one such (possibly the only such) collection)

For simplicity, we retain the actual Promise in this case. We do this with a check of vrm.getReachablePromiseRefCount() inside unregisterUnreferencedVPID:

agoric-sdk/packages/swingset-liveslots/src/liveslots.js

Lines 966 to 969 in 10b789b

	if (vrm.getReachablePromiseRefCount(p) === 0) {
	// unregister
	valToSlot.delete(p);
	slotToVal.delete(vpid);

This unregisterUnreferencedVPID is called at the (end of the handler)[https://github.com/Agoric/agoric-sdk/blob/10b789bc98ae3fa44cf4ab00ba4ec6e480e1f15f/packages/swingset-liveslots/src/liveslots.js#L1020] .then-attached by followForKernel (for exported Promises), and by dispatch.notify (for imported Promises), just before those functions delete the strong reference with exportedVPIDs.delete(vpid) or importedVPIDs.delete(vpid).

If the Promise settles, we can+should drop the strong reference, but if it still has a virtual-data reference, we can+should retain the slotToVal mapping (through the WeakRef), so that new deserializations of the old vpid map to the pre-existing (settled) Promise. If we dropped slotToVal while vdata still referenced the vpid, any deserialization of that vdata would need to create a new Promise, without knowing that it should be settled, and without knowing what data it should be fulfilled/rejected to. The kernel knows about settled promises, so if one reappears in dispatch.deliver, the kernel will follow up with a dispatch.notify, but within a vat, the virtual-data layer does not remember the resolution state of each Promise. So we need to retain the original Promise until nothing inside the vat remembers the vpid.

This means we have a two-pillar retention rule for the Promise objects (like our three-pillar "RAM, vdata, export" rule for retaining Presences and Representatives). In this case the two pillars are "settlement" and "referenced by vdata". If the Promise is not yet settled, we must retain the original (because it's the one whose fulfill/reject callbacks will be invoked). Or if vdata references the vpid, we must retain the original (because it might be settled, and that original Promise object is the only thing that remembers how and to what).

Two pillars means the state diagram has four states: both standing, first standing, second standing, and neither standing, and we retain the Promise until neither are standing. To implement that, you need the code that processes the true-to-false transition of the first piller to check on the status of the second pillar, and vice versa.

For us, that means:

• when the promise becomes settled, check the vdata pillar, and delete slotToVal if it is down (refcount === 0)
• when the vdata refcount becomes zero, check the settlement status, and delete slotToVal if it is settled

The bug is that we don't implement that second case. The refcount decrement happens in removeReachableVref, and the Promise case is in the second clause:

agoric-sdk/packages/swingset-liveslots/src/virtualReferences.js

Lines 427 to 430 in 10b789b

	if (oldRefCount === 1) {
	remotableRefCounts.delete(p);
	droppedMemoryReference = true; // true for promises too
	} else {

Currently, if the refcount reaches zero (i.e. oldRefCount === 1, so the new refcount will be zero), we delete the refcount-tracking Map entry, we set droppedMemoryReference to signal liveslots to loop back around and see if the released RAM might have shaken loose additional objects, but we don't perform any check of settlement status, and we don't make any attempt to delete slotToVal.

So, if a Promise gets added to virtual data, then settled (either by the kernel or by the vat), then removed from virtual data, we'll leave the slotToVal entry in place. The WeakRef that it points to will be empty (because there are no longer any strong references to the Promise), but we still spend four XS slots on the Map entry and the WeakRef object. That vpid will never be referenced again: the vat no longer has a Promise object, nor vdata which references the vpid, and the kernel knows it was resolved and will remove it from the c-list (and if the same kpid is delivered again later, it will be assigned a new vpid).

Directions For A Fix

Roughly, we need removeReachableVref to implement the missing state-machine edge. This probably requires code that keeps track of the resolution status of each Promise, so it can tell if the settlement has happened yet or not. But settlement cannot be checked synchronously: the best we can do is add a .then handler and record a note when it settles. This introduces a race condition between the .then handler that will perform the "first pillar has dropped" edge (in unregisterUnreferencedVPID), and the one that updates the flag used by the "second pillar has dropped" edge (in removeReachableVref).

We'll need to think more carefully about how this fix should fit in.

Other Thoughts

Also, in watchedPromises.js, maybe promiseRegistrations should be a plain Map, instead of a virtual MapStore, but maybe we really do need to keep the valToSlot mapping pinned, to keep the mapping stable.. this needs more analysis. See #10778

[warner]

@mhofman and I had a good chat about this:

The first pillar is really "known to the kernel" rather than "settled". For vpids that are known to the kernel (in the c-list), we reliably remove it from the c-list at the same time we share the settlement across the vat/kernel boundary (the kernel removes it after translating, but before invoking dispatch.notify, and the kernel also removes it immediately after translating a syscall.resolve). But if a promise is assigned a vpid without sending it to the kernel, we don't really care whether it's settled or not.

Fortunately, "known to the kernel" is well-captured by the vpid's presence in exportedVPIDs or importedVPIDs, which can be queried synchronously.

So the solution plan is:

• rename unregisterUnreferencedVPID to something like vpidBecomesUnknownToKernel or removeKernelAwarenessOfVPID
• add a parallel function like vpidBecomesUnreferencedByVData, like:

function vpidBecomesUnreferencedByVData(vpid) {
  if (!exportedVPIDs.has(vpid) && !importedVPIDs.has(vpid)) {
    const p = slotToVal.get(vpid).deref();
    assert(p);
    slotToVal.delete(vpid);
    valToSlot.delete(p);
  }
}

• include vpidBecomesUnreferencedByVData in the endowments to makeVirtualReferenceManager
• call it from removeReachableVref in the promise / refcount-becomes-zero case
  • note that it might be appropriate to refrain from setting droppedMemoryReference = true if vpidBecomesUnreferencedByVData decided to keep it in slotToVal, which could reduce the number of GC loops we do slightly

The unit test for this needs to exercise the four edges of the 2-pillar square. It should be possible to write these tests in packages/swingset-liveslots/, without using the fakeGC code, because we never care about RAM-reachability of the promise.

The tests should also exercise the "add to vdata, remove from vdata, then export to kernel" case. Here, the promise may be assigned one vpid when added to vdata, then that mapping is removed when it leaves vdata, then a second vpid is assigned when exported to kernel. (actually, we should test doing the add+remove twice, and see that we get new VPIDs each time). As long as the kernel doesn't know about the VPID (and there are no promiseWatchers), it doesn't need to be stable.