does finalizeDroppedCollection() make syscalls sensitive to GC?

[warner]

What is the Problem Being Solved?

While studying the liveslots virtual-collection code, I ran across finalizeDroppedCollection:

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

Lines 612 to 614 in 669ad6d

	function finalizeDroppedCollection(descriptor) {
	descriptor.collectionDeleter(descriptor);
	}

This is wired to a FinalizationRegistry during initialization (in startVat):

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

Lines 32 to 34 in 669ad6d

	const droppedCollectionRegistry = new FinalizationRegistry(
	finalizeDroppedCollection,
	);

and can do things like call voAwareWeakSetDeleter:

agoric-sdk/packages/SwingSet/src/liveslots/virtualObjectManager.js

Lines 355 to 371 in 669ad6d

	function voAwareWeakSetDeleter(descriptor) {
	for (const vref of descriptor.vset.values()) {
	vrm.removeRecognizableVref(vref, descriptor.vset);
	}
	}
	class VirtualObjectAwareWeakSet {
	constructor() {
	actualWeakSets.set(this, new WeakSet());
	const vset = new Set();
	virtualObjectSets.set(this, vset);
	vrm.droppedCollectionRegistry.register(this, {
	collectionDeleter: voAwareWeakSetDeleter,
	vset,
	});
	}

which can do syscalls:

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

Lines 514 to 519 in 669ad6d

	function removeRecognizableVref(vref, recognizer, recognizerIsVirtual) {
	const { type, allocatedByVat, virtual } = parseVatSlot(vref);
	if (type === 'object' && (!allocatedByVat || virtual)) {
	if (recognizerIsVirtual) {
	syscall.vatstoreDelete(`vom.ir.${vref}|${recognizer}`);
	} else {

I'm worried that this creates an additional syscall sensitivity to GC, like #6360 , because finalizeDroppedCollection does not go through a possiblyDeadSet (which doesn't get serviced until BOYD).

The task is to audit this pathway with whatever tools or techniques we come up with for #6360, determine whether it's an actual sensitivity, if so decide whether we care deeply enough to fix it, and then fix it. The fix will probably involve some kind of BOYD/possiblyDeadSet delay, where it doesn't matter when the finalizer runs, and nothing that could trigger a syscall (or influence other syscalls) can happen until a BOYD.

Things to be wary of:

• metering sensitivity of the finalizer running
• finalizeDroppedCollection reducing virtual-reference refcounts (the "vref pillar") of other objects before BOYD, allowing them to be collected when e.g. their export pillar is dropped, which would cause different syscalls.
• remember that finalizers run at the end of each delivery (we control XS enough to defer them during a delivery, but they run for all deliveries, not just BOYD

cc @FUDCo

Description of the Design

Security Considerations

Test Plan

[warner]

cc @mhofman

[mhofman]

I finally looked at the code and concur this finalizer can have observable side effects.

• metering sensitivity of the finalizer running

As far as I understand, finalizers are run after the promise queue is drained and before setImmediate calls back. Since I don't see any metering disablement for finalizers, it seems that all of them (virtual objects and vref manager) run metered, which would further make consensus execution sensitive to worker gc, including organic. I filed #6795.

[warner]

I just did another pass and I agree: metering will be affected, but not syscalls.

• virtualReferences.js has its own FinalizationRegistry, named droppedCollectionRegistry
• it is only used by VirtualObjectAwareWeakMap/Set, when a new WeakMap/WeakSet is created
  • the descriptor it registers points to the Map/Set's voAwareWeakMapDeleter
  • so the only collections being watched by this FR are VO-aware WeakMap/Sets (which don't have vrefs, and don't virtualize their data)
• when such a collection is FINALIZED, that Deleter will call vrm.removeRecognizableVref(vref, map/set) for each vref
• removeRecognizableVref takes a third recognizerIsVirtual argument, which will always be false here (the third argument isn't even provided)
  • that skips the branch which uses vom.ir.${vref}|${recognizer} vatstore keys (i.e. virtualized recognition-graph edges)
  • instead, it manipulates some in-RAM Sets and Maps which keep track of which vo-aware WeakMap/Sets are paying attention
  • it deletes any now-empty ones
  • and may submit the vref to addToPossiblyRetiredSet(vref), indicating that a RAM-recognizability pillar has just been dropped
• addToPossiblyRetiredSet() adds the vref to a set which is not processed until BOYD

So I'm pretty sure that no syscalls can happen because of this pathway. It's not as obvious as I'd like, because removeRecognizableVref is serving two clients: one that uses a FinalizationRegistry (this one), and another that behaves less GC-sensitively but which calls it in a way that uses vatstoreDelete.

The next time we consider refactoring in this file, we should consider splitting removeRecognizableVref into two separate functions, instead of using a third recognizerIsVirtual argument to distinguish the cases.

Based on that, and since we have #6795 to track the metering variations (which come from all finalizers, not just finalizeDroppedCollection), I'm going to close this one.