From 874196c477036964634b5e3b8af93fb57279ef18 Mon Sep 17 00:00:00 2001
From: Brian Warner <warner@lothar.com>
Date: Fri, 20 Dec 2024 15:44:58 -0800
Subject: [PATCH] fix(liveslots): avoid slotToVal memory leak for watched
 promises

Liveslots has a bug (#10757) which leaks slotToVal entries when a
tracked Promise is still being held in virtual data (e.g. a
merely-virtual MapStore) at the time it becomes settled. This is
triggered by `watchPromise` because of the order in which we attach
two handlers: one which notices the resolution and is inhibited from
deleting the slotToVal entry, and a second which removes the Promise
from the (virtual) `promiseRegistrations` collection (thus enabling
the deletion). For any watched Promise that is resolved, we leave a
`slotToVal` entry (with an empty WeakRef) in RAM until the end of the
incarnation.

This PR does not fix the underlying bug, but it rearranges the handler
order to avoid triggering it.

closes #10756
refs #10706
---
 .../swingset-liveslots/src/watchedPromises.js    | 16 +++++++++++++---
 .../test/watch-promise.test.js                   |  2 +-
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/packages/swingset-liveslots/src/watchedPromises.js b/packages/swingset-liveslots/src/watchedPromises.js
index 799171f5d99..30771fc0686 100644
--- a/packages/swingset-liveslots/src/watchedPromises.js
+++ b/packages/swingset-liveslots/src/watchedPromises.js
@@ -235,13 +235,23 @@ export function makeWatchedPromiseManager({
       } else {
         watchedPromiseTable.init(vpid, harden([[watcher, ...args]]));
 
+        promiseRegistrations.init(vpid, p);
+
+        // pseudoThen registers a settlement callback that will remove
+        // this promise from promiseRegistrations and
+        // watchedPromiseTable. To avoid triggering
+        // https://github.com/Agoric/agoric-sdk/issues/10757 and
+        // preventing slotToVal cleanup, the `pseudoThen()` should
+        // precede `maybeExportPromise()`. This isn't foolproof, but
+        // does mitigate in advance of a proper fix. See #10756 for
+        // details of this particular mitigation, and #10757 for the
+        // deeper bug.
+        pseudoThen(p, vpid);
+
         // Ensure that this vat's promises are rejected at termination.
         if (maybeExportPromise(vpid)) {
           syscall.subscribe(vpid);
         }
-
-        promiseRegistrations.init(vpid, p);
-        pseudoThen(p, vpid);
       }
     });
   }
diff --git a/packages/swingset-liveslots/test/watch-promise.test.js b/packages/swingset-liveslots/test/watch-promise.test.js
index 52c4d033599..6ceecbf48f5 100644
--- a/packages/swingset-liveslots/test/watch-promise.test.js
+++ b/packages/swingset-liveslots/test/watch-promise.test.js
@@ -26,7 +26,7 @@ const build = vatPowers => {
   });
 };
 
-test.failing('watched local promises should not leak slotToVal entries', async t => {
+test('watched local promises should not leak slotToVal entries', async t => {
   const { dispatchMessage, testHooks } = await setupTestLiveslots(
     t,
     build,