fix(xsnap): do not leak through vat termination race

[mhofman]

refs: #5507

Description

• Fixes the promise memory leak discovered through the diagnostics added in feat(cosmic-swingset): Add memory stats to new commit block slog event #5637

Security Considerations

None

Documentation Considerations

We should have some documentation for developers regarding the memory leak risks of promises.

Testing Considerations

Tested locally with the loadgen. The snapshot does not seem to accumulate any significant program objects.

[dckc]

I started to take a look... it's more than I can look at urgently. I don't object to this going forward, though it seems to introduce significant technical-debt to re-sync endo (in place of the patch). I'd prefer to see an issue about cleaning that up.

[mhofman]

@dckc the patches will become redundant and be removed at the next endo sync. I've used this approach before to incorporate endo changes into agoric-sdk without going through a full blown version bump, and it has worked well. cc @kriskowal

[dckc]

@dckc the patches will become redundant and be removed at the next endo sync.

Yes; I'm asking that we make an issue now for that work later.

Just like... hm... I don't see an issue for #5576 . So maybe that's not the norm. Oh well.

[turadg]

Reviewed just the non-Endo changes since those have separate review.

This patch approach seems fine to me. It's the norm when syncing Endo deps to remove stale patches. (These will fail otherwise.)

The snapshot does not seem to accumulate any significant program objects.

I can't tell whether this is evidence that the motivating observation is gone.

Approving because this appears to be strictly an improvement, whether it fully solves the problem or not.

[mhofman]

Ugh sorry I didn't mean to add @turadg or @dtribble as reviewers. I had a bad push which included some of master into an out of date base branch. (I assume code-owners triggered this)

[warner]

I'm not qualified to review the implementation of racePromises (maybe @erights or @kriskowal ?), but the use of it within xsnap.js looks good to me.

[mhofman]

I re-integrated the patches of endo PR that now landed, and rebased to not layer on top of the measurement work.

@kriskowal depending on how #5576 goes, let decide whether when to merge this.

[mhofman]

@Mergifyio refresh

[mergify]

refresh

✅ Pull request refreshed