POC broken alt cancellation 3032

packages/cancel/DESIGN.md

@@ -33,7 +33,7 @@ import { makeCancelKit } from '@endo/cancel';
 const { cancelled, cancel } = makeCancelKit();
 
 // Synchronous check
-if (cancelled.cancelled) {
+if (isKnownCancelled(cancelled)) {
   return; // Skip unnecessary work
 }
 

packages/cancel/README.md

@@ -36,12 +36,12 @@ import { makeCancelKit } from '@endo/cancel';
 const { cancelled, cancel } = makeCancelKit();
 
 // Check synchronously if cancelled
-console.log(cancelled.cancelled); // undefined
+console.log(isKnownCancelled(cancelled)); // undefined
 
 // Trigger cancellation
 cancel(Error('Operation timed out'));
 
-console.log(cancelled.cancelled); // true
+console.log(isKnownCancelled(cancelled)); // true
 
 // The promise rejects when cancelled
 cancelled.catch(error => console.log(error.message)); // "Operation timed out"

packages/cancel/src/cancel-kit.js

@@ -4,6 +4,67 @@
  * @import { Cancelled, Cancel, CancelKit } from './types.js'
  */
 
+const { apply } = Reflect;
+
+/**
+ * @type {WeakSet<Promise>}
+ */
+const knownCancelledSet = new WeakSet();
+harden(knownCancelledSet);
+
+const thenMethod = Promise.prototype.then;
+
+// TODO more accurate typing of `when`. It is intended to emulate E.when.
+/**
+ * Since this package has no dependencies, it likely needs to work reliably in
+ * a non-ses environment too.
+ *
+ * @template {any} V
+ * @template {any} T
+ * @param {Promise<V>} promise
+ * @param {(v: V) => T} [onFulfilled]
+ * @param {(reason: Error) => T } [onRejected]
+ * @returns {Promise<T>}
+ */
+const when = (promise, onFulfilled = undefined, onRejected = undefined) =>
+  apply(thenMethod, promise, [onFulfilled, onRejected]);
+
+/**
+ * Says whether a promise, if interpreted as a cancellation token, is known
+ * to be cancelled. If the promise was made by the CancelKit, the answer is
+ * always immediately accurate.
+ *
+ * For other promises, we interpret any rejected state as cancelled.
+ * For these, the answer is false the first time, since we do not yet know.
+ * But once the promise settles, we will eventually
+ * have an accurate answer. Thus, if the promise is well behaved,
+ * once the answer is ever `true` it should remain `true` forever.
+ * A passable promise is necessarily well behaved, so for those, this
+ * monotonicity is guaranteed.
+ *
+ * XXX SECURITY BUG DO NOT MERGE THIS EXPERIMENT
+ *
+ * The fact that this always says false the first time we ask about an
+ * unrelated promise is a global communications channel.
+ *
+ * @param {Promise} promise
+ * @returns {boolean}
+ */
+export const isKnownCancelled = promise => {
+  if (knownCancelledSet.has(promise)) {
+    return true;
+  }
+  when(
+    promise,
+    _v => {},
+    _reason => {
+      knownCancelledSet.add(promise);
+    },
+  );
+  return false;
+};
+harden(isKnownCancelled);
+
 /**
  * Creates a cancellation kit containing a cancellation token and a cancel function.
  *
@@ -20,47 +81,39 @@
  * @param {Cancelled} [parentCancelled] - Optional parent cancellation token
  * @returns {CancelKit}
  */
-export const makeCancelKit = parentCancelled => {
-  /** @type {undefined | true} */
-  let cancelledState;
-
+export const makeCancelKit = (parentCancelled = undefined) => {
   /** @type {Cancel | undefined} */
   let internalReject;
 
-  const promise = new Promise((_resolve, reject) => {
+  /** @type {Cancelled} */
+  const cancelled = new Promise((_resolve, reject) => {
     internalReject = reject;
   });
 
-  // Prevent unhandled rejection warnings when the promise is not awaited
-  promise.catch(() => {});
-
-  // Define the cancelled getter on the promise
-  Object.defineProperty(promise, 'cancelled', {
-    get() {
-      return cancelledState;
-    },
-    enumerable: false,
-    configurable: false,
-  });
+  // Propagate cancellation from parent if provided
+  if (parentCancelled) {
+    if (isKnownCancelled(parentCancelled)) {
+      knownCancelledSet.add(cancelled);
+    }
+  }
 
-  /** @type {Cancelled} */
-  const cancelled = /** @type {Cancelled} */ (promise);
+  // Prevent unhandled rejection warnings when the promise is not awaited
+  when(cancelled, undefined, () => {});
 
   /** @type {Cancel} */
   const cancel = reason => {
-    if (cancelledState === undefined) {
-      cancelledState = true;
-      const error = reason || Error('Cancelled');
-      if (internalReject) {
-        internalReject(error);
-        internalReject = undefined;
-      }
+    const error = reason || Error('Cancelled');
+    if (internalReject) {
+      internalReject(error);
+      internalReject = undefined;
+      knownCancelledSet.add(cancelled);
     }
   };
 
   // Propagate cancellation from parent if provided
   if (parentCancelled) {
-    parentCancelled.then(
+    when(
+      parentCancelled,
       () => {},
       reason => cancel(reason),
     );

packages/cancel/src/to-abort.js

@@ -1,5 +1,7 @@
 /// <reference types="ses"/>
 
+import { isKnownCancelled } from './cancel-kit.js';
+
 /**
  * @import { Cancelled } from './types.js'
  */
@@ -19,7 +21,7 @@ export const toAbortSignal = cancelled => {
   );
 
   // If already cancelled, abort immediately
-  if (cancelled.cancelled) {
+  if (isKnownCancelled(cancelled)) {
     controller.abort(Error('Cancelled'));
   }
 

packages/cancel/src/types.js

@@ -1,8 +1,8 @@
 /**
- * A cancellation token that is a Promise<never> with a synchronous
- * `cancelled` getter for local observation.
+ * A cancellation token that is a Promise<never> intended to be used as a
+ * cancellation token
  *
- * @typedef {Promise<never> & { readonly cancelled: undefined | true }} Cancelled
+ * @typedef {Promise<never>} Cancelled
  */
 
 /**

packages/cancel/test/index.test.js

@@ -9,6 +9,7 @@ import { toAbortSignal } from '../to-abort.js';
 import { fromAbortSignal } from '../from-abort.js';
 import { delay } from '../delay.js';
 import { makeDelay } from '../delay-lite.js';
+import { isKnownCancelled } from '../src/cancel-kit.js';
 
 // ==================== makeCancelKit tests ====================
 
@@ -20,16 +21,16 @@ test('makeCancelKit returns cancelled and cancel', t => {
   t.is(typeof kit.cancel, 'function', 'cancel is a function');
 });
 
-test('cancelled starts as undefined', t => {
+test('starts as not cancelled', t => {
   const { cancelled } = makeCancelKit();
-  t.is(cancelled.cancelled, undefined);
+  t.is(isKnownCancelled(cancelled), false);
 });
 
 test('cancelled becomes true after cancel is called', t => {
   const { cancelled, cancel } = makeCancelKit();
-  t.is(cancelled.cancelled, undefined);
+  t.is(isKnownCancelled(cancelled), false);
   cancel();
-  t.is(cancelled.cancelled, true);
+  t.is(isKnownCancelled(cancelled), true);
 });
 
 test('cancelled promise rejects after cancel', async t => {
@@ -48,23 +49,23 @@ test('cancelled promise rejects with default error if no reason', async t => {
 test('cancel is idempotent', t => {
   const { cancelled, cancel } = makeCancelKit();
   cancel();
-  t.is(cancelled.cancelled, true);
+  t.is(isKnownCancelled(cancelled), true);
   cancel(); // Should not throw
-  t.is(cancelled.cancelled, true);
+  t.is(isKnownCancelled(cancelled), true);
 });
 
 test('makeCancelKit propagates cancellation from parent', async t => {
   const { cancelled: parentCancelled, cancel: cancelParent } = makeCancelKit();
   const { cancelled: childCancelled } = makeCancelKit(parentCancelled);
 
-  t.is(childCancelled.cancelled, undefined);
+  t.is(isKnownCancelled(childCancelled), false);
 
   cancelParent(Error('parent cancelled'));
 
   // Give time for propagation
   await Promise.resolve();
 
-  t.is(childCancelled.cancelled, true);
+  t.is(isKnownCancelled(childCancelled), true);
   await t.throwsAsync(childCancelled, { message: 'parent cancelled' });
 });
 
@@ -75,8 +76,8 @@ test('makeCancelKit child can cancel independently of parent', t => {
 
   cancelChild(Error('child cancelled'));
 
-  t.is(childCancelled.cancelled, true);
-  t.is(parentCancelled.cancelled, undefined); // Parent not affected
+  t.is(isKnownCancelled(childCancelled), true);
+  t.is(isKnownCancelled(parentCancelled), false); // Parent not affected
 });
 
 // ==================== allMap tests ====================
@@ -113,7 +114,7 @@ test('allMap provides cancellation token to callback', async t => {
     values,
     (_value, _index, cancelled) => {
       t.true(cancelled instanceof Promise);
-      t.is(cancelled.cancelled, undefined);
+      t.is(isKnownCancelled(cancelled), false);
     },
     parentCancelled,
   );
@@ -141,7 +142,13 @@ test('allMap cancels on first rejection', async t => {
   );
 
   // After allMap rejects, the cancellation token should be triggered
-  t.is(capturedCancelled?.cancelled, true, 'cancellation was triggered');
+  // TODO TypeScript understands the type implications of `&&` but not the more
+  // accurate `??`.
+  t.is(
+    capturedCancelled && isKnownCancelled(capturedCancelled),
+    true,
+    'cancellation was triggered',
+  );
 });
 
 test('allMap respects external cancellation', async t => {
@@ -156,7 +163,7 @@ test('allMap respects external cancellation', async t => {
     values,
     async (_value, _index, cancelled) => {
       await Promise.resolve();
-      if (cancelled.cancelled) {
+      if (isKnownCancelled(cancelled)) {
         observedCancellation = true;
       }
       return 1;
@@ -198,7 +205,13 @@ test('anyMap cancels remaining after first success', async t => {
 
   t.is(result, 'first');
   // After anyMap succeeds, the cancellation token should be triggered
-  t.is(capturedCancelled?.cancelled, true, 'cancellation was triggered');
+  // TODO TypeScript understands the type implications of `&&` but not the more
+  // accurate `??`.
+  t.is(
+    capturedCancelled && isKnownCancelled(capturedCancelled),
+    true,
+    'cancellation was triggered',
+  );
 });
 
 test('anyMap rejects with AggregateError if all fail', async t => {
@@ -233,7 +246,7 @@ test('anyMap provides cancellation token to callback', async t => {
   const values = [1];
   await anyMap(values, (_value, _index, cancelled) => {
     t.true(cancelled instanceof Promise);
-    t.is(cancelled.cancelled, undefined);
+    t.is(isKnownCancelled(cancelled), false);
     return 'done';
   });
 });
@@ -250,7 +263,7 @@ test('anyMap respects external cancellation', async t => {
     values,
     async (_value, _index, cancelled) => {
       await Promise.resolve();
-      if (cancelled.cancelled) {
+      if (isKnownCancelled(cancelled)) {
         observedCancellation = true;
       }
       return 'done';
@@ -299,27 +312,27 @@ test('fromAbortSignal returns a Cancelled token', t => {
   const cancelled = fromAbortSignal(controller.signal);
 
   t.true(cancelled instanceof Promise);
-  t.is(cancelled.cancelled, undefined);
+  t.is(isKnownCancelled(cancelled), false);
 });
 
 test('fromAbortSignal triggers when signal aborts', async t => {
   const controller = new AbortController();
   const cancelled = fromAbortSignal(controller.signal);
 
-  t.is(cancelled.cancelled, undefined);
+  t.is(isKnownCancelled(cancelled), false);
   controller.abort(Error('test abort'));
 
   // Give time for the abort to propagate
   await Promise.resolve();
-  t.is(cancelled.cancelled, true);
+  t.is(isKnownCancelled(cancelled), true);
 });
 
 test('fromAbortSignal handles already aborted signal', t => {
   const controller = new AbortController();
   controller.abort(Error('already aborted'));
 
   const cancelled = fromAbortSignal(controller.signal);
-  t.is(cancelled.cancelled, true);
+  t.is(isKnownCancelled(cancelled), true);
 });
 
 test('fromAbortSignal promise rejects with abort reason', async t => {

packages/pass-style/src/safe-promise.js

@@ -31,12 +31,9 @@ const confirmPromiseOwnKeys = (pr, reject) => {
    *   * An overriding `toStringTag` non-enumerable data property
    *     with a string value.
    *   * Those own properties that might be added by Node's async_hooks.
-   *   * A `cancelled` non-enumerable getter for cancellation tokens.
    */
   const unknownKeys = keys.filter(
-    key =>
-      key !== 'cancelled' &&
-      (typeof key !== 'symbol' || !hasOwn(Promise.prototype, key)),
+    key => typeof key !== 'symbol' || !hasOwn(Promise.prototype, key),
   );
 
   if (unknownKeys.length !== 0) {
@@ -85,20 +82,6 @@ const confirmPromiseOwnKeys = (pr, reject) => {
             reject`Own @@toStringTag must not be enumerable: ${q(tagDesc)}`))
       );
     }
-    if (key === 'cancelled') {
-      // Allow a non-enumerable `cancelled` getter for cancellation tokens.
-      // This property is local-only and will not be passed over CapTP.
-      const cancelledDesc = getOwnPropertyDescriptor(pr, 'cancelled');
-      assert(cancelledDesc !== undefined);
-      return (
-        (hasOwn(cancelledDesc, 'get') ||
-          (reject &&
-            reject`Own 'cancelled' must be an accessor property, not a data property: ${q(cancelledDesc)}`)) &&
-        (!cancelledDesc.enumerable ||
-          (reject &&
-            reject`Own 'cancelled' must not be enumerable: ${q(cancelledDesc)}`))
-      );
-    }
     const val = pr[key];
     if (val === undefined || typeof val === 'number') {
       return true;