fixup! reviewer discussion

packages/async-flow/src/async-flow.js

@@ -1,4 +1,4 @@
-import { annotateError, Fail, makeError, X } from '@endo/errors';
+import { annotateError, Fail, makeError, q, X } from '@endo/errors';
 import { E } from '@endo/eventual-send';
 import { M } from '@endo/patterns';
 import { makeScalarWeakMapStore } from '@agoric/store';
@@ -257,13 +257,50 @@ export const prepareAsyncFlowTools = (outerZone, outerOptions = {}) => {
             const { flow } = facets;
 
             const flowState = flow.getFlowState();
-            if (flowState === 'Done' || flowState === 'Failed') {
-              return;
-            }
-            if (hasMembrane(flow)) {
-              getMembrane(flow).wake();
-            } else {
-              flow.restart();
+            switch (flowState) {
+              case 'Done':
+              case 'Failed': {
+                return;
+              }
+              case 'Running':
+              case 'Replaying': {
+                // Safe to call membrane wake for a replaying or running flow
+                // because it is idempotent. membrane.wake already has reentrancy
+                // protection. Aside from harmless reentrancy, calling
+                // membrane.wake won't cause it to do anything that it would
+                // not have done on its own.
+                //
+                // An interesting edge case is that when the guest proceeds
+                // from a top-level doReturn or doThrow, while we're still in
+                // the guest turn, if somehow flow.wake were to be called then,
+                // and if the next thing in the replay log was a `doCall`
+                // (a future feature), then the `doCall` would call the guest
+                // while it was still in the middle of a "past" turn. However,
+                // this cannot happen because `flow` is host-side. For it to
+                // be called while the guest is active, the membrane's
+                // `callStack` would not be empty. membrane.wake checks and
+                // already throws an error in that case.
+                //
+                // More important, during a replay, no guest action can actually
+                // call host functions at all. Rather, the host is fully
+                // emulated from the log. So this case cannot arise.
+                //
+                // This analysis *assumes* that the guest function has no access
+                // to the flow outside the membrane, i.e., the "closed guest"
+                // assumption.
+                getMembrane(flow).wake();
+                return;
+              }
+              case 'Sleeping': {
+                flow.restart();
+                return;
+              }
+              default: {
+                // Should be a at-ts-expect-error that this case is unreachable
+                // which TS clearly knows anyway because it thinks the following
+                // `flowState` variable in this context has type `never`.
+                throw Fail`unexpected flowState ${q(flowState)}`;
+              }
             }
           },
           getOutcome() {
@@ -289,6 +326,7 @@ export const prepareAsyncFlowTools = (outerZone, outerOptions = {}) => {
             const { state, facets } = this;
             const { bijection, log } = state;
             const { flow } = facets;
+            !state.isDone || Fail`Cannot reset a done flow`;
 
             if (failures.has(flow)) {
               failures.delete(flow);
@@ -299,8 +337,6 @@ export const prepareAsyncFlowTools = (outerZone, outerOptions = {}) => {
             }
             log.reset();
             bijection.reset();
-
-            state.isDone = false;
           },
           complete() {
             const { state, facets } = this;

packages/async-flow/src/log-store.js

@@ -63,6 +63,10 @@ export const prepareLogStore = zone => {
       reset() {
         const { self } = this;
         tmp.resetFor(self);
+
+        // TODO: Should we resolve replayDoneKit here, in case we're
+        // transitioning to a Failed state, so that any pending watchers
+        // can exit?
       },
       dispose() {
         const { state, self } = this;

packages/async-flow/src/replay-membrane.js

@@ -42,8 +42,8 @@ export const makeReplayMembrane = (
   const doFulfill = (hostVow, hostFulfillment) => {
     const guestPromise = hostToGuest(hostVow);
     const status = guestPromiseMap.get(guestPromise);
-    if (status === 'settled') {
-      return;
+    if (!status || status === 'settled') {
+      Fail`doFulfill should only be called on a registered unresolved promise`;
     }
     const guestFulfillment = hostToGuest(hostFulfillment);
     status.resolve(guestFulfillment);
@@ -61,8 +61,8 @@ export const makeReplayMembrane = (
   const doReject = (hostVow, hostReason) => {
     const guestPromise = hostToGuest(hostVow);
     const status = guestPromiseMap.get(guestPromise);
-    if (status === 'settled') {
-      return;
+    if (!status || status === 'settled') {
+      Fail`doReject should only be called on a registered unresolved promise`;
     }
     const guestReason = hostToGuest(hostReason);
     status.reject(guestReason);
@@ -184,14 +184,18 @@ export const makeReplayMembrane = (
       throw panic(fatalError);
     }
 
-    if (outcome.kind === 'return') {
-      return outcome.result;
-    } else {
-      outcome.kind === 'throw' ||
+    switch (outcome.kind) {
+      case 'return': {
+        return outcome.result;
+      }
+      case 'throw': {
+        throw outcome.problem;
+      }
+      default: {
         // @ts-expect-error TS correctly knows this case would be outside
         // the type. But that's what we want to check.
-        Fail`unexpected outcome kind ${q(outcome.kind)}`;
-      throw outcome.problem;
+        throw Fail`unexpected outcome kind ${q(outcome.kind)}`;
+      }
     }
   };
 
@@ -213,7 +217,7 @@ export const makeReplayMembrane = (
       };
       if (optVerb) {
         defineProperties(guestMethod, {
-          name: { value: String(optVerb) },
+          name: { value: String(hRem[optVerb].name || optVerb) },
           length: { value: Number(hRem[optVerb].length || 0) },
         });
       } else {
@@ -263,21 +267,21 @@ export const makeReplayMembrane = (
     void when(
       hVow,
       async hostFulfillment => {
-        await log.promiseReplayDone();
-        if (guestPromiseMap.get(promise) !== 'settled') {
+        await log.promiseReplayDone(); // should never reject
+        if (!stopped && guestPromiseMap.get(promise) !== 'settled') {
           /** @type {LogEntry} */
           const entry = harden(['doFulfill', hVow, hostFulfillment]);
           log.pushEntry(entry);
-          interpretOne(topDispatch, entry);
+          interpretOne(topDispatch, entry); // does its own panic
         }
       },
       async hostReason => {
-        await log.promiseReplayDone();
-        if (guestPromiseMap.get(promise) !== 'settled') {
+        await log.promiseReplayDone(); // should never reject
+        if (!stopped && guestPromiseMap.get(promise) !== 'settled') {
           /** @type {LogEntry} */
           const entry = harden(['doReject', hVow, hostReason]);
           log.pushEntry(entry);
-          interpretOne(topDispatch, entry);
+          interpretOne(topDispatch, entry); // does its own panic
         }
       },
     );