From 8f6921ae9532ddf7bed12faa9ce5db1cacc3d8de Mon Sep 17 00:00:00 2001
From: Kris Kowal <kris@agoric.com>
Date: Mon, 1 Jan 2024 22:32:24 -0800
Subject: [PATCH 1/3] feat(daemon): Thread cancellation context into worklets

---
 packages/daemon/src/daemon.js  | 22 ++++++++++++++++++++--
 packages/daemon/src/types.d.ts |  2 +-
 packages/daemon/src/worker.js  | 14 ++++++++------
 3 files changed, 29 insertions(+), 9 deletions(-)

diff --git a/packages/daemon/src/daemon.js b/packages/daemon/src/daemon.js
index 3dc5053175..d84565609a 100644
--- a/packages/daemon/src/daemon.js
+++ b/packages/daemon/src/daemon.js
@@ -46,6 +46,14 @@ const makeInspector = (type, number, record) =>
     list: () => Object.keys(record),
   });
 
+const makeFarContext = context =>
+  Far('Context', {
+    cancel: context.cancel,
+    whenCancelled: () => context.cancelled,
+    whenDisposed: () => context.disposed,
+    addDisposalHook: context.onCancel,
+  });
+
 /**
  * @param {import('./types.js').DaemonicPowers} powers
  * @param {Promise<number>} webletPortP
@@ -284,7 +292,12 @@ const makeDaemonCore = async (
       // eslint-disable-next-line no-use-before-define
       provideValueForFormulaIdentifier(guestFormulaIdentifier)
     );
-    const external = E(workerDaemonFacet).makeUnconfined(specifier, guestP);
+    const external = E(workerDaemonFacet).makeUnconfined(
+      specifier,
+      guestP,
+      // TODO fix type
+      /** @type {any} */ (makeFarContext(context)),
+    );
     return { external, internal: undefined };
   };
 
@@ -327,7 +340,12 @@ const makeDaemonCore = async (
       // eslint-disable-next-line no-use-before-define
       provideValueForFormulaIdentifier(guestFormulaIdentifier)
     );
-    const external = E(workerDaemonFacet).makeBundle(readableBundleP, guestP);
+    const external = E(workerDaemonFacet).makeBundle(
+      readableBundleP,
+      guestP,
+      // TODO fix type
+      /** @type {any} */ (makeFarContext(context)),
+    );
     return { external, internal: undefined };
   };
 
diff --git a/packages/daemon/src/types.d.ts b/packages/daemon/src/types.d.ts
index c8a2db33a0..96272dc921 100644
--- a/packages/daemon/src/types.d.ts
+++ b/packages/daemon/src/types.d.ts
@@ -227,7 +227,7 @@ export interface Context {
 }
 
 export interface FarContext {
-  cancel: (reason: string) => Promise<never>;
+  cancel: (reason: Error) => Promise<void>;
   whenCancelled: () => Promise<never>;
   whenDisposed: () => Promise<void>;
 }
diff --git a/packages/daemon/src/worker.js b/packages/daemon/src/worker.js
index e2eec34095..78154b64a9 100644
--- a/packages/daemon/src/worker.js
+++ b/packages/daemon/src/worker.js
@@ -65,22 +65,24 @@ export const makeWorkerFacet = ({ cancel }) => {
 
     /**
      * @param {string} specifier
-     * @param {unknown} powersP
+     * @param {Promise<unknown>} powersP
+     * @param {Promise<unknown>} contextP
      */
-    makeUnconfined: async (specifier, powersP) => {
+    makeUnconfined: async (specifier, powersP, contextP) => {
       // Windows absolute path includes drive letter which is confused for
       // protocol specifier. So, we reformat the specifier to include the
       // file protocol.
       const specifierUrl = normalizeFilePath(specifier);
       const namespace = await import(specifierUrl);
-      return namespace.make(powersP);
+      return namespace.make(powersP, contextP);
     },
 
     /**
      * @param {import('@endo/eventual-send').ERef<import('./types.js').EndoReadable>} readableP
-     * @param {unknown} powersP
+     * @param {Promise<unknown>} powersP
+     * @param {Promise<unknown>} contextP
      */
-    makeBundle: async (readableP, powersP) => {
+    makeBundle: async (readableP, powersP, contextP) => {
       const bundleText = await E(readableP).text();
       const bundle = JSON.parse(bundleText);
 
@@ -90,7 +92,7 @@ export const makeWorkerFacet = ({ cancel }) => {
       const namespace = await importBundle(bundle, {
         endowments,
       });
-      return namespace.make(powersP);
+      return namespace.make(powersP, contextP);
     },
   });
 };

From 5f9067335b30e1243b92da3d1a704fe92e57f2e9 Mon Sep 17 00:00:00 2001
From: Erik Marks <rekmarks@protonmail.com>
Date: Wed, 14 Feb 2024 23:28:31 -0800
Subject: [PATCH 2/3] test(daemon): Add cancellation context tests

Adds unit tests for using the cancellation context in both confined
and unconfined plugins / caplets. Deletes some unused args and fixes
some type issues discovered in the course of work. Also, adds docstrings
to the Context interface in types.d.ts.

In the course of implementation, a bug was discovered. The added tests
only pass due to the addition of a delay that should not be necessary.
The following applies the same to both the confined and unconfined
cases.

The tests are structured as follows:

```
1 const result = E(host).evaluate(
2   'worker',
3   'E(caplet).awaitCancellation()',
4   ['caplet'],
5   ['context-consumer'],
6 );
7 await E(host).cancel('context-consumer');
8 t.is(await result, 'cancelled');
```

The test endo.log reveals the cause:

```
Making make-unconfined:1f440...
Making least-authority:abc51...
Cancelled:
* make-unconfined:1f440...
Making eval:c9ae8...
Making make-unconfined:1f440...
```

Despite receiving the `evaluate` request first and the cancellation
request second, the host processes these requests in reverse order. The
result is that the test hangs. As a temporary fix, a timeout is added on
line 6. This bug will be fixed in a future commit.
---
 packages/daemon/src/daemon-node-powers.js |  2 +-
 packages/daemon/src/daemon.js             |  4 ++
 packages/daemon/src/types.d.ts            | 43 ++++++++++-
 packages/daemon/test/context-consumer.js  | 14 ++++
 packages/daemon/test/test-endo.js         | 88 +++++++++++++++++++++--
 5 files changed, 141 insertions(+), 10 deletions(-)
 create mode 100644 packages/daemon/test/context-consumer.js

diff --git a/packages/daemon/src/daemon-node-powers.js b/packages/daemon/src/daemon-node-powers.js
index 3240d1ed8a..42073b6641 100644
--- a/packages/daemon/src/daemon-node-powers.js
+++ b/packages/daemon/src/daemon-node-powers.js
@@ -234,7 +234,7 @@ export const makeSocketPowers = ({ net }) => {
     );
 
   /** @type {import('./types.js').SocketPowers['connectPort']} */
-  const connectPort = ({ port, host, cancelled }) =>
+  const connectPort = ({ port, host }) =>
     new Promise((resolve, reject) => {
       const conn = net.connect(port, host, err => {
         if (err) {
diff --git a/packages/daemon/src/daemon.js b/packages/daemon/src/daemon.js
index d84565609a..e4e84ff275 100644
--- a/packages/daemon/src/daemon.js
+++ b/packages/daemon/src/daemon.js
@@ -46,6 +46,10 @@ const makeInspector = (type, number, record) =>
     list: () => Object.keys(record),
   });
 
+/**
+ * @param {import('./types.js').Context} context - The context to make far.
+ * @returns {import('./types.js').FarContext} The far context.
+ */
 const makeFarContext = context =>
   Far('Context', {
     cancel: context.cancel,
diff --git a/packages/daemon/src/types.d.ts b/packages/daemon/src/types.d.ts
index 96272dc921..0da63437ab 100644
--- a/packages/daemon/src/types.d.ts
+++ b/packages/daemon/src/types.d.ts
@@ -217,12 +217,50 @@ export interface Topic<
   subscribe(): Stream<TRead, TWrite, TReadReturn, TWriteReturn>;
 }
 
+/**
+ * The cancellation context of a live value associated with a formula.
+ */
 export interface Context {
-  cancel: (reason?: unknown, logPrefix?: string) => Promise<void>;
+  /**
+   * Cancel the value, preparing it for garbage collection. Cancellation
+   * propagates to all values that depend on this value.
+   *
+   * @param reason - The reason for the cancellation.
+   * @param logPrefix - The prefix to use within the log.
+   * @returns A promise that is resolved when the value is cancelled and
+   * can be garbage collected.
+   */
+  cancel: (reason?: Error, logPrefix?: string) => Promise<void>;
+
+  /**
+   * A promise that is rejected when the context is cancelled.
+   * Once rejected, the cancelled value may initiate any teardown procedures.
+   */
   cancelled: Promise<never>;
+
+  /**
+   * A promise that is resolved when the context is disposed. This occurs
+   * after the `cancelled` promise is rejected, and after all disposal hooks
+   * have been run.
+   * Once resolved, the value may be garbage collected at any time.
+   */
   disposed: Promise<void>;
+
+  /**
+   * @param formulaIdentifier - The formula identifier of the value whose
+   * cancellation should cause this value to be cancelled.
+   */
   thisDiesIfThatDies: (formulaIdentifier: string) => void;
+
+  /**
+   * @param formulaIdentifier - The formula identifier of the value that should
+   * be cancelled if this value is cancelled.
+   */
   thatDiesIfThisDies: (formulaIdentifier: string) => void;
+
+  /**
+   * @param hook - A hook to run when the value is cancelled.
+   */
   onCancel: (hook: () => void | Promise<void>) => void;
 }
 
@@ -230,6 +268,7 @@ export interface FarContext {
   cancel: (reason: Error) => Promise<void>;
   whenCancelled: () => Promise<never>;
   whenDisposed: () => Promise<void>;
+  addDisposalHook: Context['onCancel'];
 }
 
 export interface InternalExternal<External = unknown, Internal = unknown> {
@@ -322,7 +361,7 @@ export interface Mail {
   listSpecial(): Array<string>;
   listAll(): Array<string>;
   reverseLookupFormulaIdentifier(formulaIdentifier: string): Array<string>;
-  cancel(petName: string, reason: unknown): Promise<void>;
+  cancel(petName: string, reason: Error): Promise<void>;
   // Mail operations:
   listMessages(): Promise<Array<Message>>;
   followMessages(): Promise<FarRef<Reader<Message>>>;
diff --git a/packages/daemon/test/context-consumer.js b/packages/daemon/test/context-consumer.js
new file mode 100644
index 0000000000..6d8a7c19e7
--- /dev/null
+++ b/packages/daemon/test/context-consumer.js
@@ -0,0 +1,14 @@
+import { E, Far } from '@endo/far';
+
+export const make = async (_powers, context) => {
+  return Far('Context consumer', {
+    async awaitCancellation() {
+      try {
+        await E(context).whenCancelled();
+      } catch {
+        return 'cancelled';
+      }
+      throw new Error('should have been cancelled');
+    },
+  });
+};
diff --git a/packages/daemon/test/test-endo.js b/packages/daemon/test/test-endo.js
index c0abd773b4..7f51c2cb53 100644
--- a/packages/daemon/test/test-endo.js
+++ b/packages/daemon/test/test-endo.js
@@ -566,11 +566,11 @@ test('guest facet receives a message for host', async t => {
   await stop(locator);
 });
 
-test('direct termination', async t => {
+test('direct cancellation', async t => {
   const { promise: cancelled, reject: cancel } = makePromiseKit();
   t.teardown(() => cancel(Error('teardown')));
 
-  const locator = makeLocator('tmp', 'termination-direct');
+  const locator = makeLocator('tmp', 'cancellation-direct');
 
   await start(locator);
   t.teardown(() => stop(locator));
@@ -643,16 +643,14 @@ test('direct termination', async t => {
       ['counter'],
     ),
   );
-
-  t.pass();
 });
 
 // See: https://github.com/endojs/endo/issues/2074
-test.failing('indirect termination', async t => {
+test.failing('indirect cancellation', async t => {
   const { promise: cancelled, reject: cancel } = makePromiseKit();
   t.teardown(() => cancel(Error('teardown')));
 
-  const locator = makeLocator('tmp', 'termination-indirect');
+  const locator = makeLocator('tmp', 'cancellation-indirect');
 
   await start(locator);
   t.teardown(() => stop(locator));
@@ -732,7 +730,7 @@ test('cancel because of requested capability', async t => {
   const { promise: cancelled, reject: cancel } = makePromiseKit();
   t.teardown(() => cancel(Error('teardown')));
 
-  const locator = makeLocator('tmp', 'termination-via-request');
+  const locator = makeLocator('tmp', 'cancellation-via-request');
 
   await start(locator);
   t.teardown(() => stop(locator));
@@ -816,6 +814,82 @@ test('cancel because of requested capability', async t => {
   );
 });
 
+test('unconfined service can respond to cancellation', async t => {
+  const { promise: cancelled, reject: cancel } = makePromiseKit();
+  t.teardown(() => cancel(Error('teardown')));
+
+  const locator = makeLocator('tmp', 'cancellation-unconfined-response');
+
+  await start(locator);
+  t.teardown(() => stop(locator));
+
+  const { getBootstrap } = await makeEndoClient(
+    'client',
+    locator.sockPath,
+    cancelled,
+  );
+  const bootstrap = getBootstrap();
+  const host = E(bootstrap).host();
+  await E(host).provideWorker('worker');
+
+  const capletPath = path.join(dirname, 'test', 'context-consumer.js');
+  const capletLocation = url.pathToFileURL(capletPath).href;
+  await E(host).makeUnconfined(
+    'worker',
+    capletLocation,
+    'NONE',
+    'context-consumer',
+  );
+
+  const result = E(host).evaluate(
+    'worker',
+    'E(caplet).awaitCancellation()',
+    ['caplet'],
+    ['context-consumer'],
+  );
+  // TODO:cancel This should not be necessary.
+  // eslint-disable-next-line no-undef
+  await new Promise(resolve => setTimeout(resolve, 100));
+  await E(host).cancel('context-consumer');
+  t.is(await result, 'cancelled');
+});
+
+test('confined service can respond to cancellation', async t => {
+  const { promise: cancelled, reject: cancel } = makePromiseKit();
+  t.teardown(() => cancel(Error('teardown')));
+
+  const locator = makeLocator('tmp', 'cancellation-confined-response');
+
+  await start(locator);
+  t.teardown(() => stop(locator));
+
+  const { getBootstrap } = await makeEndoClient(
+    'client',
+    locator.sockPath,
+    cancelled,
+  );
+  const bootstrap = getBootstrap();
+  const host = E(bootstrap).host();
+  await E(host).provideWorker('worker');
+
+  const capletPath = path.join(dirname, 'test', 'context-consumer.js');
+  await doMakeBundle(host, capletPath, bundleName =>
+    E(host).makeBundle('worker', bundleName, 'NONE', 'context-consumer'),
+  );
+
+  const result = E(host).evaluate(
+    'worker',
+    'E(caplet).awaitCancellation()',
+    ['caplet'],
+    ['context-consumer'],
+  );
+  // TODO:cancel This should not be necessary.
+  // eslint-disable-next-line no-undef
+  await new Promise(resolve => setTimeout(resolve, 100));
+  await E(host).cancel('context-consumer');
+  t.is(await result, 'cancelled');
+});
+
 test('make a host', async t => {
   const { promise: cancelled, reject: cancel } = makePromiseKit();
   t.teardown(() => cancel(Error('teardown')));

From 8a5245382bbcca46b45f560da26c248489c56f3e Mon Sep 17 00:00:00 2001
From: Erik Marks <rekmarks@protonmail.com>
Date: Tue, 20 Feb 2024 21:36:54 -0800
Subject: [PATCH 3/3] fix(endo): Synchronize host cancellation with formula
 graph

Synchronizes the host's `cancel()` with the formula graph by awaiting
the formula graph mutex in a new daemon method, `cancelValue()`. Modifies
the mutex implementation to permit calling `enqueue()` without
specifying function to call.
---
 packages/daemon/src/daemon.js     | 10 +++++++++-
 packages/daemon/src/mail.js       | 10 +++-------
 packages/daemon/src/mutex.js      |  5 ++---
 packages/daemon/src/types.d.ts    | 10 ++++++++++
 packages/daemon/test/test-endo.js |  6 ------
 5 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/packages/daemon/src/daemon.js b/packages/daemon/src/daemon.js
index e4e84ff275..f66407a7ca 100644
--- a/packages/daemon/src/daemon.js
+++ b/packages/daemon/src/daemon.js
@@ -652,6 +652,14 @@ const makeDaemonCore = async (
     return controller;
   };
 
+  /** @type {import('./types.js').CancelValue} */
+  const cancelValue = async (formulaIdentifier, reason) => {
+    await formulaGraphMutex.enqueue();
+    const controller = provideControllerForFormulaIdentifier(formulaIdentifier);
+    console.log('Cancelled:');
+    return controller.context.cancel(reason);
+  };
+
   /** @type {import('./types.js').ProvideValueForFormulaIdentifier} */
   const provideValueForFormulaIdentifier = formulaIdentifier => {
     const controller = /** @type {import('./types.js').Controller<>} */ (
@@ -1087,8 +1095,8 @@ const makeDaemonCore = async (
   const makeMailbox = makeMailboxMaker({
     getFormulaIdentifierForRef,
     provideValueForFormulaIdentifier,
-    provideControllerForFormulaIdentifier,
     provideControllerForFormulaIdentifierAndResolveHandle,
+    cancelValue,
   });
 
   const makeIdentifiedGuestController = makeGuestMaker({
diff --git a/packages/daemon/src/mail.js b/packages/daemon/src/mail.js
index a98f8657cd..914969c984 100644
--- a/packages/daemon/src/mail.js
+++ b/packages/daemon/src/mail.js
@@ -11,15 +11,15 @@ const { quote: q } = assert;
 /**
  * @param {object} args
  * @param {import('./types.js').ProvideValueForFormulaIdentifier} args.provideValueForFormulaIdentifier
- * @param {import('./types.js').ProvideControllerForFormulaIdentifier} args.provideControllerForFormulaIdentifier
  * @param {import('./types.js').GetFormulaIdentifierForRef} args.getFormulaIdentifierForRef
  * @param {import('./types.js').ProvideControllerForFormulaIdentifierAndResolveHandle} args.provideControllerForFormulaIdentifierAndResolveHandle
+ * @param {import('./types.js').CancelValue} args.cancelValue
  */
 export const makeMailboxMaker = ({
   getFormulaIdentifierForRef,
   provideValueForFormulaIdentifier,
-  provideControllerForFormulaIdentifier,
   provideControllerForFormulaIdentifierAndResolveHandle,
+  cancelValue,
 }) => {
   /**
    * @param {object} args
@@ -81,11 +81,7 @@ export const makeMailboxMaker = ({
       if (formulaIdentifier === undefined) {
         throw new TypeError(`Unknown pet name: ${q(petName)}`);
       }
-      // Behold, recursion:
-      const controller =
-        provideControllerForFormulaIdentifier(formulaIdentifier);
-      console.log('Cancelled:');
-      return controller.context.cancel(reason);
+      return cancelValue(formulaIdentifier, reason);
     };
 
     /** @type {import('./types.js').Mail['list']} */
diff --git a/packages/daemon/src/mutex.js b/packages/daemon/src/mutex.js
index f222fc54f5..44fdad7bd4 100644
--- a/packages/daemon/src/mutex.js
+++ b/packages/daemon/src/mutex.js
@@ -1,7 +1,7 @@
 import { makeQueue } from '@endo/stream';
 
 /**
- * @returns {{ lock: () => Promise<void>, unlock: () => void, enqueue: (asyncFn: () => Promise<any>) => Promise<any> }}
+ * @returns {import('./types.js').Mutex}
  */
 export const makeMutex = () => {
   /** @type {import('@endo/stream').AsyncQueue<void>} */
@@ -17,8 +17,7 @@ export const makeMutex = () => {
   return {
     lock,
     unlock,
-    // helper for correct usage
-    enqueue: async asyncFn => {
+    enqueue: async (asyncFn = /** @type {any} */ (async () => {})) => {
       await lock();
       try {
         return await asyncFn();
diff --git a/packages/daemon/src/types.d.ts b/packages/daemon/src/types.d.ts
index 0da63437ab..90d48ebee0 100644
--- a/packages/daemon/src/types.d.ts
+++ b/packages/daemon/src/types.d.ts
@@ -291,6 +291,10 @@ export type ProvideControllerForFormulaIdentifier = (
 export type ProvideControllerForFormulaIdentifierAndResolveHandle = (
   formulaIdentifier: string,
 ) => Promise<Controller>;
+export type CancelValue = (
+  formulaIdentifier: string,
+  reason: Error,
+) => Promise<void>;
 
 /**
  * A handle is used to create a pointer to a formula without exposing it directly.
@@ -632,3 +636,9 @@ export type DaemonicPowers = {
   persistence: DaemonicPersistencePowers;
   control: DaemonicControlPowers;
 };
+
+type Mutex = {
+  lock: () => Promise<void>;
+  unlock: () => void;
+  enqueue: <T>(asyncFn?: () => Promise<T>) => Promise<T>;
+};
diff --git a/packages/daemon/test/test-endo.js b/packages/daemon/test/test-endo.js
index 7f51c2cb53..3fad922c82 100644
--- a/packages/daemon/test/test-endo.js
+++ b/packages/daemon/test/test-endo.js
@@ -847,9 +847,6 @@ test('unconfined service can respond to cancellation', async t => {
     ['caplet'],
     ['context-consumer'],
   );
-  // TODO:cancel This should not be necessary.
-  // eslint-disable-next-line no-undef
-  await new Promise(resolve => setTimeout(resolve, 100));
   await E(host).cancel('context-consumer');
   t.is(await result, 'cancelled');
 });
@@ -883,9 +880,6 @@ test('confined service can respond to cancellation', async t => {
     ['caplet'],
     ['context-consumer'],
   );
-  // TODO:cancel This should not be necessary.
-  // eslint-disable-next-line no-undef
-  await new Promise(resolve => setTimeout(resolve, 100));
   await E(host).cancel('context-consumer');
   t.is(await result, 'cancelled');
 });