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

Previously, the reset() feature misbehaved in a common use pattern,
where both the `reset` and `resolve` facets were extracted at the same
time. The state was nominally held in `nameToState.get(name)`, but was
sometimes used by closed-over `state` and `pk` variables. Which one
you got depended upon when the Proxy `get` trap was called. If
`resolve` was fetched early and retained across a `reset()` call, then
`resolve()` would resolve the *old* promise. Later, when `consume` was
used, the space would create a new Promise to satisfy the request,
which would never be resolved.

This changes the implementation to strictly keep/use the state in
`nameToState`, and allow all resolve/reject/reset methods to work the
same way no matter when they were retrieved (they close over `name`
but not any state).

closes #7709

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

@@ -1,4 +1,5 @@
 // @ts-check
+import { Fail } from '@agoric/assert';
 import { assertKey } from '@agoric/store';
 import { canBeDurable } from '@agoric/vat-data';
 import { isPromise, makePromiseKit } from '@endo/promise-kit';
@@ -77,7 +78,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,75 +96,69 @@ 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 {
+    if (!nameToState.has(name)) {
       onAddKey(name);
+      remaining.add(name);
       const pk = makePromiseKit();
-
       pk.promise
         .finally(() => {
-          remaining.delete(name);
           onSettled(name, remaining);
         })
         .catch(() => {});
+      nameToState.set(name, harden({ pk, isSettling: false }));
+    }
+    return nameToState.get(name) || Fail`provideState(${name})`;
+  };
 
-      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);
+  // 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 => {
+      onResolve(name, value);
+      const old = provideState(name);
+      nameToState.set(name, harden({ ...old, isSettling: true }));
+      old.pk.resolve(value);
+      remaining.delete(name);
+    };
+    const reject = reason => {
+      const old = provideState(name);
+      nameToState.set(name, harden({ ...old, isSettling: true }));
+      old.pk.reject(reason);
+      remaining.delete(name);
+    };
+    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) {
+          // so just let them wait for the new value: resetting an
+          // unresolved item is a no-op
+          return;
         }
-        // Now publish a new promise.
-        nameToState.delete(name);
-        remaining.delete(name);
-      };
+        // 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);
+    };
 
-      state = harden({
-        isSettling: false,
-        resolve,
-        reject,
-        reset,
-        promise: pk.promise,
-      });
-      nameToState.set(name, state);
-      remaining.add(name);
-    }
-    return state;
+    return harden({ resolve, reject, reset });
   };
 
   /** @type {PromiseSpaceOf<T>['consume']} */
@@ -173,8 +168,7 @@ export const makePromiseSpace = (optsOrLog = {}) => {
     {
       get: (_target, name) => {
         assert.typeof(name, 'string');
-        const kit = provideState(name);
-        return kit.promise;
+        return provideState(name).pk.promise;
       },
     },
   );
@@ -186,8 +180,7 @@ export const makePromiseSpace = (optsOrLog = {}) => {
     {
       get: (_target, name) => {
         assert.typeof(name, 'string');
-        const { reject, resolve, reset } = provideState(name);
-        return harden({ reject, resolve, reset });
+        return makeProducer(name);
       },
     },
   );

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 });
   {
@@ -78,3 +78,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);
+  }
+});