fix ReadableStream.from() cancel behavior per WPT spec

src/workerd/api/tests/streams-test.js

@@ -1,4 +1,4 @@
-import { strictEqual, ok, deepStrictEqual } from 'node:assert';
+import { strictEqual, ok, deepStrictEqual, rejects } from 'node:assert';
 
 const enc = new TextEncoder();
 
@@ -466,6 +466,117 @@ export const readableStreamFromNoopAsyncGen = {
   },
 };
 
+// Tests for ReadableStream.from() cancel behavior per WPT spec
+export const readableStreamFromCancelRejectsWhenReturnRejects = {
+  async test() {
+    const rejectError = new Error('return error');
+    const iterable = {
+      next() {
+        return Promise.resolve({ value: undefined, done: true });
+      },
+      return() {
+        return Promise.reject(rejectError);
+      },
+      [Symbol.asyncIterator]() {
+        return this;
+      },
+    };
+
+    const rs = ReadableStream.from(iterable);
+    const reader = rs.getReader();
+
+    await rejects(reader.cancel(), (err) => err === rejectError);
+  },
+};
+
+export const readableStreamFromCancelRejectsWhenReturnThrows = {
+  async test() {
+    const throwError = new Error('return throws');
+    const iterable = {
+      next() {
+        return Promise.resolve({ value: undefined, done: true });
+      },
+      return() {
+        throw throwError;
+      },
+      [Symbol.asyncIterator]() {
+        return this;
+      },
+    };
+
+    const rs = ReadableStream.from(iterable);
+    const reader = rs.getReader();
+
+    await rejects(reader.cancel(), (err) => err === throwError);
+  },
+};
+
+export const readableStreamFromCancelRejectsWhenReturnNotMethod = {
+  async test() {
+    const iterable = {
+      next() {
+        return Promise.resolve({ value: undefined, done: true });
+      },
+      return: 42, // exists but not callable
+      [Symbol.asyncIterator]() {
+        return this;
+      },
+    };
+
+    const rs = ReadableStream.from(iterable);
+    const reader = rs.getReader();
+
+    await rejects(reader.cancel(), {
+      name: 'TypeError',
+      message: /return/,
+    });
+  },
+};
+
+export const readableStreamFromCancelRejectsWhenReturnNonObject = {
+  async test() {
+    const iterable = {
+      next() {
+        return Promise.resolve({ value: undefined, done: true });
+      },
+      return() {
+        return Promise.resolve(42); // fulfills with non-object
+      },
+      [Symbol.asyncIterator]() {
+        return this;
+      },
+    };
+
+    const rs = ReadableStream.from(iterable);
+    const reader = rs.getReader();
+
+    await rejects(reader.cancel(), {
+      name: 'TypeError',
+    });
+  },
+};
+
+export const readableStreamFromCancelResolvesWhenReturnMissing = {
+  async test() {
+    const iterable = {
+      next() {
+        return Promise.resolve({ value: undefined, done: true });
+      },
+      // no return method
+      [Symbol.asyncIterator]() {
+        return this;
+      },
+    };
+
+    const rs = ReadableStream.from(iterable);
+    const reader = rs.getReader();
+
+    // Should resolve without error when return() is missing
+    await reader.cancel();
+    await reader.closed;
+  },
+};
+
 export const abortWriterAfterGc = {
   async test() {
     function getWriter() {

src/workerd/jsg/iterator.h

@@ -205,8 +205,17 @@ class AsyncGenerator final {
   }
 
   // If nothing is returned, the generator is complete.
+  // Per GetMethod spec (https://262.ecma-international.org/#sec-getmethod), if the 'return'
+  // property exists but is not callable, we throw a TypeError.
   Promise<kj::Maybe<T>> return_(Lock& js, kj::Maybe<T> maybeValue = kj::none) {
     KJ_IF_SOME(active, maybeActive) {
+      // Per GetMethod spec: if property exists but is not callable, throw TypeError
+      if (active.returnExistsButNotCallable) {
+        maybeActive = kj::none;
+        return js.rejectedPromise<kj::Maybe<T>>(
+            js.typeError("property 'return' is not a function"_kj));
+      }
+
       KJ_IF_SOME(return_, active.maybeReturn) {
         auto& selfRef = KJ_ASSERT_NONNULL(maybeSelfRef);
         return js.tryCatch([&] {
@@ -217,17 +226,13 @@ class AsyncGenerator final {
             }
             return js.resolvedPromise(kj::mv(result.value));
           }, [ref = selfRef.addRef()](Lock& js, Value exception) {
-            Promise<kj::Maybe<T>> retPromise = nullptr;
-            if (ref->runIfAlive([&](AsyncGenerator& self) {
-              retPromise = self.throw_(js, kj::mv(exception));
-            })) {
-              return kj::mv(retPromise);
-            }
+            // Per spec, rejections from return() should be propagated directly
+            ref->runIfAlive([&](AsyncGenerator& self) { self.maybeActive = kj::none; });
             return js.rejectedPromise<kj::Maybe<T>>(kj::mv(exception));
           });
         }, [&](Value exception) {
           maybeActive = kj::none;
-          return throw_(js, kj::mv(exception));
+          return js.rejectedPromise<kj::Maybe<T>>(kj::mv(exception));
         });
       }
       maybeActive = kj::none;
@@ -276,13 +281,19 @@ class AsyncGenerator final {
     kj::Maybe<NextSignature> maybeNext;
     kj::Maybe<ReturnSignature> maybeReturn;
     kj::Maybe<ThrowSignature> maybeThrow;
+    // Per GetMethod spec, if property exists but is not callable, we should throw TypeError.
+    // We track this state to defer the error to when return_() is actually called.
+    bool returnExistsButNotCallable = false;
 
     template <typename TypeWrapper>
     Active(Lock& js, JsObject object, TypeWrapper*)
         : maybeNext(tryGetGeneratorFunction<NextSignature, TypeWrapper>(js, object, "next"_kj)),
           maybeReturn(
               tryGetGeneratorFunction<ReturnSignature, TypeWrapper>(js, object, "return"_kj)),
           maybeThrow(tryGetGeneratorFunction<ThrowSignature, TypeWrapper>(js, object, "throw"_kj)) {
+      // Check if return property exists but isn't callable (per GetMethod spec)
+      returnExistsButNotCallable =
+          maybeReturn == kj::none && !object.get(js, "return"_kj).isNullOrUndefined();
     }
     Active(Active&&) = default;
     Active& operator=(Active&&) = default;

src/wpt/streams-test.ts

@@ -422,19 +422,7 @@ export default {
       'Floating point arithmetic must manifest near 0 (total ends up zero)',
     ],
   },
-  'readable-streams/from.any.js': {
-    comment: 'See comments on tests',
-    disabledTests: [
-      // A hanging promise was cancelled
-      'ReadableStream.from: cancel() rejects when return() rejects',
-      'ReadableStream.from: cancel() rejects when return() fulfills with a non-object',
-    ],
-    expectedFailures: [
-      // TODO(soon): This one is a bit pedantic. We ignore the case where return() is not
-      // a method whereas the spec expects us to return a rejected promise in this case.
-      'ReadableStream.from: cancel() rejects when return() is not a method',
-    ],
-  },
+  'readable-streams/from.any.js': {},
   'readable-streams/garbage-collection.any.js': {
     comment: 'See comments on individual tests',
     disabledTests: [