fix(vats): fix promise space reset() misbehavior

packages/vats/src/core/promise-space.js

@@ -1,7 +1,8 @@
 // @ts-check
+import { E } from '@endo/far';
 import { assertKey } from '@agoric/store';
 import { canBeDurable } from '@agoric/vat-data';
-import { isPromise, makePromiseKit } from '@endo/promise-kit';
+import { makePromiseKit } from '@endo/promise-kit';
 
 /**
  * @typedef {{
@@ -59,11 +60,7 @@ export const makeStoreHooks = (store, log = noop) => {
   return harden({
     ...logHooks,
     onResolve: (name, valueP) => {
-      if (isPromise(valueP)) {
-        void valueP.then(value => save(name, value));
-      } else {
-        save(name, valueP);
-      }
+      void E.when(valueP, value => save(name, value));
     },
     onReset: name => {
       if (store.has(name)) {
@@ -77,7 +74,7 @@ export const makeStoreHooks = (store, log = noop) => {
  * Make { produce, consume } where for each name, `consume[name]` is a promise
  * and `produce[name].resolve` resolves it.
  *
- * Note: repeated resolve()s are noops.
+ * Note: repeated resolve()s without an intervening reset() are noops.
  *
  * @template {Record<string, unknown>} [T=Record<string, unknown>]
  * @param {{ log?: typeof console.log } & (
@@ -95,102 +92,97 @@ export const makePromiseSpace = (optsOrLog = {}) => {
   const { onAddKey, onSettled, onResolve, onReset } = hooks;
 
   /**
-   * @typedef {PromiseRecord<any> & {
-   *   reset: (reason?: unknown) => void,
-   *   isSettling: boolean,
-   * }} PromiseState
+   * @typedef {{ pk: PromiseRecord<any>, isSettling: boolean }} PromiseState
    */
   /** @type {Map<string, PromiseState>} */
   const nameToState = new Map();
   /** @type {Set<string>} */
   const remaining = new Set();
 
-  /** @param {string} name */
+  /** @type {(name: string) => PromiseState} */
   const provideState = name => {
-    /** @type {PromiseState} */
-    let state;
-    const currentState = nameToState.get(name);
-    if (currentState) {
-      state = currentState;
-    } else {
+    let state = nameToState.get(name);
+    if (!state) {
       onAddKey(name);
+      remaining.add(name);
       const pk = makePromiseKit();
-
       pk.promise
         .finally(() => {
           remaining.delete(name);
           onSettled(name, remaining);
         })
         .catch(() => {});
-
-      const settling = () => {
-        assert(state);
-        state = harden({ ...state, isSettling: true });
-        nameToState.set(name, state);
-      };
-
-      const resolve = value => {
-        settling();
-        onResolve(name, value);
-        pk.resolve(value);
-      };
-      const reject = reason => {
-        settling();
-        pk.reject(reason);
-      };
-
-      const reset = (reason = undefined) => {
-        onReset(name);
-        if (!state.isSettling) {
-          if (!reason) {
-            // Reuse the old promise; don't reject it.
-            return;
-          }
-          reject(reason);
-        }
-        // Now publish a new promise.
-        nameToState.delete(name);
-        remaining.delete(name);
-      };
-
-      state = harden({
-        isSettling: false,
-        resolve,
-        reject,
-        reset,
-        promise: pk.promise,
-      });
+      state = harden({ pk, isSettling: false });
       nameToState.set(name, state);
-      remaining.add(name);
     }
     return state;
   };
 
+  // we must tolerate these producer methods being retrieved both
+  // before and after the consumer is retrieved, and also both before
+  // and after reset() is invoked, so they only close over 'name' and
+  // not over any state variables
+
+  const makeProducer = name => {
+    const resolve = value => {
+      const old = provideState(name);
+      if (old.isSettling) {
+        // First attempt to settle always wins.
+        return;
+      }
+      onResolve(name, value);
+      nameToState.set(name, harden({ ...old, isSettling: true }));
+      old.pk.resolve(value);
+    };
+    const reject = reason => {
+      const old = provideState(name);
+      if (old.isSettling) {
+        // First attempt to settle always wins.
+        return;
+      }
+      nameToState.set(name, harden({ ...old, isSettling: true }));
+      old.pk.reject(reason);
+    };
+    const reset = (reason = undefined) => {
+      onReset(name);
+      const old = provideState(name);
+      if (!old.isSettling) {
+        // we haven't produced a value yet, and there might be
+        // consumers still watching old.pk.promise
+        if (reason === undefined) {
+          // so just let them wait for the new value: resetting an
+          // unresolved item is a no-op
+          return;
+        }
+        // reject those watchers; new watchers will wait for the new
+        // value through the replacement promise
+        reject(reason);
+      }
+      // delete the state, so new callers will get a new promise kit
+      nameToState.delete(name);
+      remaining.delete(name);
+    };
+
+    return harden({ resolve, reject, reset });
+  };
+
   /** @type {PromiseSpaceOf<T>['consume']} */
   // @ts-expect-error cast
-  const consume = new Proxy(
-    {},
-    {
-      get: (_target, name) => {
-        assert.typeof(name, 'string');
-        const kit = provideState(name);
-        return kit.promise;
-      },
+  const consume = new Proxy(harden({}), {
+    get: (_target, name) => {
+      assert.typeof(name, 'string');
+      return provideState(name).pk.promise;
     },
-  );
+  });
 
   /** @type {PromiseSpaceOf<T>['produce']} */
   // @ts-expect-error cast
-  const produce = new Proxy(
-    {},
-    {
-      get: (_target, name) => {
-        assert.typeof(name, 'string');
-        const { reject, resolve, reset } = provideState(name);
-        return harden({ reject, resolve, reset });
-      },
+  const produce = new Proxy(harden({}), {
+    get: (_target, name) => {
+      assert.typeof(name, 'string');
+      return makeProducer(name);
     },
-  );
+  });
 
   return harden({ produce, consume });
 };

packages/vats/test/test-promise-space.js

@@ -38,7 +38,7 @@ test('makePromiseSpace', async t => {
   await checkAlice(reusedAlice, `Hi, I'm Alice 3!`);
 });
 
-test('makePromiseSpace backed by store', async t => {
+test('makePromiseSpace copied into store', async t => {
   /** @type {MapStore<string, string>} */
   const store = makeScalarBigMapStore('stuff', { durable: true });
   {
@@ -64,6 +64,7 @@ test('makePromiseSpace backed by store', async t => {
     produce.chocolate.resolve(doesNotResolve);
     const nonPassable = harden(() => {});
     produce.strawberry.resolve(nonPassable);
+    await null;
     const actual = Object.fromEntries(store.entries());
     t.deepEqual(
       actual,
@@ -78,3 +79,77 @@ test('makePromiseSpace backed by store', async t => {
     produce.strawberry.resolve('ignored already resolved');
   }
 });
+
+test('resolve after reset', async t => {
+  /** @type {MapStore<string, string>} */
+  const store = makeScalarBigMapStore('stuff', { durable: true });
+
+  const { consume, produce } = makePromiseSpace({ store });
+
+  // sample resolve/consume early, then use after reset(): #7709
+
+  // for foo, we produce first
+  {
+    // reset before resolving the first time
+    const { resolve, reset } = produce.foo;
+    const foo1 = consume.foo;
+    reset();
+    resolve(1);
+    t.is(await foo1, 1);
+    const foo2 = consume.foo;
+    t.is(await foo2, 1);
+  }
+
+  {
+    const { resolve, reset } = produce.foo;
+    const foo1 = consume.foo;
+    reset();
+    resolve(2);
+    t.is(await foo1, 1); // captured before reset()
+    const foo2 = consume.foo;
+    t.is(await foo2, 2);
+  }
+
+  {
+    const foo1 = consume.foo;
+    produce.foo.reset();
+    const foo2 = consume.foo;
+    produce.foo.resolve(3);
+    const foo3 = consume.foo;
+    t.is(await foo1, 2); // captured before reset()
+    t.is(await foo2, 3);
+    t.is(await foo3, 3);
+  }
+
+  // for 'bar', we consume first
+  {
+    const bar1 = consume.bar;
+    const { resolve, reset } = produce.bar;
+    reset();
+    resolve(1);
+    t.is(await bar1, 1);
+    const bar2 = consume.bar;
+    t.is(await bar2, 1);
+  }
+
+  {
+    const { resolve, reset } = produce.bar;
+    const bar1 = consume.bar;
+    reset();
+    resolve(2);
+    t.is(await bar1, 1); // captured before reset()
+    const bar2 = consume.bar;
+    t.is(await bar2, 2);
+  }
+
+  {
+    const bar1 = consume.bar;
+    produce.bar.reset();
+    const bar2 = consume.bar;
+    produce.bar.resolve(3);
+    const bar3 = consume.bar;
+    t.is(await bar1, 2); // captured before reset()
+    t.is(await bar2, 3);
+    t.is(await bar3, 3);
+  }
+});