Skip to content

feat!: v2.0.0 — AbortSignal, Symbol.asyncDispose, error modes, cleanupTimeout - #50

Open
voxpelli wants to merge 87 commits into
mainfrom
claude/merge-branches-to-master-nixCB
Open

feat!: v2.0.0 — AbortSignal, Symbol.asyncDispose, error modes, cleanupTimeout#50
voxpelli wants to merge 87 commits into
mainfrom
claude/merge-branches-to-master-nixCB

Conversation

@voxpelli

@voxpelli voxpelli commented May 8, 2026

Copy link
Copy Markdown
Owner

Summary

The full 2.0.0 release: reconciles three feature branches, then layers on review-driven hardening across five deep review cycles plus a final two-agent go/no-go audit (the last three cycles: multi-angle finder fleets + adversarial design verifiers + architects + primary-source research + judges settling design contradictions + an integration prototyper executing core fixes before they landed, with every confirmed finding probe-verified against native for await before fixing), a benchmark suite with measurable perf wins, memory-retention fixes, full JSDoc type tightening, a restructured README + advanced-semantics doc, and a modernised toolchain. ~78 commits, 141 specs.

Features

Cancellation — options.signal

  • Accept an optional AbortSignal. When aborted, the next pending or freshly-called iterator.next() rejects with signal.reason exactly once; subsequent calls return { done: true, value: undefined }.
  • Pre-aborted signals are handled (the source is never read). The source's .return() runs — and is awaited — as part of abort delivery. External abort wins over queued/not-yet-captured errors; an abort left undelivered when the consumer explicitly closes the iterator is suppressed (later next() resolves done, matching native AsyncGenerator).

Per-callback signal — callback(item, { signal })

  • The callback receives { signal } as a second argument that is always present (even with no options.signal) and aborts on iterator close — return() / throw() / Symbol.asyncDispose / source exhaustion / external abort / first fail-fast error. Forward it into fetch/undici to cancel in-flight IO on shutdown. (On natural source exhaustion the abort fires during end-of-stream cleanup, after in-flight work drained — pinned.)
  • Existing one-arg callbacks keep working.

Symbol.asyncDispose

  • The returned iterator implements Symbol.asyncDispose for await using; equivalent to iterator.return() and idempotent.

Error mode — options.errors

  • 'fail-eventually' (default) keeps the historical semantics, precisely documented: after the first error no new items are pulled; in-flight items drain (their values surface); then the captured error throws (identity preserved) or an AggregateError for ≥2.
  • 'fail-fast' mirrors Promise.all: first error short-circuits, the next .next() rejects with the original error, the source's .return() runs once. Also like Promise.all, exactly one error owns the rejection: a second racing error — or an error that lost the shutdown race to a synchronous abort — is discarded, never delivered twice.

cleanupTimeout — wedged-source escape hatch

  • New option capping how long close/abort waits for source .return() to settle. Default undefined keeps the unbounded await (matching native AsyncGenerator). Validated up to Node's TIMEOUT_MAX (values above it would silently clamp to 1 ms).

mergeIterables parity + direct return

  • Forwards all options to bufferedAsyncMap; returns the underlying iterator directly, so it carries Symbol.asyncDispose at runtime and the precise return type.
  • Eager validation of the input array and its elements. This is load-bearing, not ceremony (probe-verified): without it a string element silently char-splits into the merge ('abc''a','b','c' interleaved, no error — boxed and cross-realm Strings included), and a non-iterable element surfaces only at consume time as an unbranded engine TypeError with no index, in default mode after every healthy source drained. Validation happens in the wrapper because element failures inside the callback are — by design — stream errors routed through the errors mode; the callability check is GetMethod-shaped so it can never disagree with what yield * does at iteration.

Correctness hardening (review waves 1–3 + final audit)

The final review cycles empirically reproduced and fixed twenty-one bugs — most matching native-AsyncGenerator/for await semantics the docs promise:

  • Reject-proof buffer slots, end to end: synchronously-throwing callbacks and next() bodies used to reject raw buffer slots — bypassing both error modes, rejecting every subsequent next() forever, leaking the source, and crashing the process via unhandledRejection on early close. Waves 2–3 extend this to hostile iterator results (the CVE-2024-7652 defect class): results with throwing done/value getters or has-trap Proxies surface as ordinary stream errors with identity preserved; a Proxy whose has-trap lies about the internal error tag can no longer silently truncate iteration and leak the source (the tag is brand-verified); spec-legal callable IteratorResults are accepted like native for await; a callback result whose [Symbol.asyncIterator]() returns a non-object surfaces as a branded TypeError instead of livelocking the event loop (falsy) or leaking internal variable names (truthy). All failures flow through tagged internal envelopes.
  • No post-done over-pull (found by the final adversarial go/no-go audit; reproduces byte-for-byte on 1.x, so inherited, not a regression): once the source has resolved { done: true }, refills never call its next() again — native parity. Pre-fix, up to bufferSize post-done pulls turned defensive sources ("cursor already closed") into spurious AggregateErrors, a never-settling post-done next() wedged the consumer forever, and in fail-fast mode a spurious post-done error could win the race against a slow in-flight value and silently drop it. The construction-time speculative prefetch (the library's documented contract) is unchanged and now documented under bufferSize.
  • Spec-faithful dispatch everywhere (ECMA-262 GetMethod / GetIteratorFromMethod, matching for await): the input's Symbol.asyncIterator is read exactly once and the captured method invoked — a stateful getter can't desync probing from invocation; a null/undefined member falls back to sync iteration; a non-callable member gets a branded TypeError with no sync fallback; the captured method returning a non-object throws immediately at construction (native parity) instead of a deferred unbranded AggregateError; the callback-result fan-out dispatch does a single [[Get]] (no has-probe) so sync and async callbacks returning the same object behave identically.
  • Close-path integrity: the external-signal abort listener detaches on iterator close (previously each closed iterator stayed retained by a long-lived signal — 5000 leaked listeners observed); markAsEnded is await-idempotent (every closer awaits the first closer's cleanup, so await using scopes can't exit while the source's finally is still running); return(thenable) closes the iterator synchronously; an abort racing the fail-eventually drain-throw can no longer produce two consecutive rejections — asserted through a sound oracle (the per-callback signal's immutable reason as the commit-point record); iterators that produced malformed results are still .return()ed during cleanup, exactly once each.
  • Validation edges: bufferSize values above ~7000 no longer crash construction with a stack overflow (iterative refill); every input-validation rejection carries an accurate per-case message (the protocols hint appears only where the sync fallback genuinely exists; strings get their own guidance).

Docs

  • README restructured around a why-first, lean-reference shape: a new "Why" opener positioning against sequential for await and array-bound helpers (with explicit when-NOT-to-use cases); concrete, complete usage examples (paginated-API fan-in lead example, async-generator fan-out, a first mergeIterables example); pre-release-relative perf history cut in favour of consumer-relevant findings.
  • New ADVANCED.md holds the precise contracts — prefetch/speculation model, iterator-protocol details, abort delivery and precedence, error-mode mechanics, memory guarantees — each linked per-section from the README and pointing at the spec file that pins it. Shipped in the npm files list so links resolve inside node_modules too.

Performance & memory

mitata benchmark suite (npm run bench) guarding:

  • Per-pull abort "park" instead of a long-lived race promise — removes per-item PromiseReaction retention (nodejs/node#51452); pinned by test/memory.spec.js.
  • Load-balancer skipped when no sub-iterators exist.
  • Two stable envelope hidden-classes (lean hot-path value shape + shared terminal shape) and hoisted per-item closures — the full hardening nets out faster than its baseline (flat-callback −13%, fan-out −11%), and the wave-2/3 exception-proofing was A/B-verified neutral with an identical deopt profile.

Types & toolchain

  • Standalone BufferedAsyncIterableIterator typedef (exported); its assignability to AsyncIterableIterator is now enforced at compile time by a type-contract spec, not a comment.
  • Strict type-coverage gate at 99%; @voxpelli/tsconfig v16 / TypeScript 5.9 / node22 preset; exports map for node16/nodenext resolution; bench:smoke CI job.

Breaking changes

  • Node engine: >=18.6.0^22.16.0 || >=24.0.0 (native Symbol.asyncDispose; Node 20 EOL, 23 non-LTS). CI matrix: 22 + 24. TypeScript consumers need TS ≥5.9 (declared in engines).
  • Callback signature widened to callback(item, { signal }) — runtime-compatible.
  • bufferSize validation tightened: 0, negatives, floats, NaN, Infinity now throw at construction (previously they silently behaved like 1, and Infinity crashed later).
  • cleanupTimeout rejects values above 2147483647 (Node's timer maximum).
  • Malformed-result errors changed shape and timing: a source/sub-iterator next() resolving a non-object now surfaces through the configured error mode (drain-time in default mode, possibly AggregateError-wrapped) with new messages, instead of rejecting the racing next() immediately with TypeError('Expected an object value').
  • Validation messages reworded: the 1.x 'Expected asyncIterable to have a Symbol.asyncIterator function' string is replaced by three accurate per-case messages (non-callable member / string input / neither protocol usable). Don't match on the old string.
  • mergeIterables requires an array input, validates elements eagerly at call time (bad or non-callable-protocol elements throw immediately with an indexed TypeError instead of surfacing as deferred stream errors), rejects string elements (including cross-realm boxed Strings), and starts prefetching at construction (1.x was fully lazy).
  • Input dispatch follows for await: dual-protocol inputs are consumed via their async iterator; a nullish Symbol.asyncIterator member falls back to sync iteration instead of throwing; a Symbol.asyncIterator method returning a non-object throws a branded TypeError at construction.
  • The source is never pulled after done: 1.x could call next() on an exhausted source up to bufferSize extra times; 2.0.0 stops at the first observed { done: true } (speculative concurrent prefetch before done resolves remains, as documented).

Tests & review

  • 141 specs; lib/ at 100% coverage. Five full review cycles ending in a final 9-angle sweep (3 correctness angles, 3 cleanup angles, altitude, conventions, full-branch cohesion) plus a two-agent go/no-go audit (constructive release-readiness + adversarial differential fuzzing vs native for await, race storms, livelock and memory probes — both returned GO; its single actionable finding, the post-done over-pull, is fixed above). Every fix commit's regression specs were verified failing pre-fix.

release-please will cut 2.0.0 from the feat!: history on merge (the commit-override block below feeds the generated changelog).

BEGIN_COMMIT_OVERRIDE
feat!: AbortSignal cancellation, Symbol.asyncDispose, error modes, cleanupTimeout

feat: options.signal — abort delivery with exactly-once rejection semantics
feat: per-callback { signal } second argument, always present, aborting on iterator close
feat: Symbol.asyncDispose on the returned iterator for await using
feat: options.errors: 'fail-eventually' | 'fail-fast' error modes
feat: options.cleanupTimeout escape hatch for wedged sources
fix: exception-proof buffer pipeline against sync throws and hostile iterator results (spoofed tags, throwing getters, callable results)
fix: GetMethod-aligned dispatch for inputs, callback results and mergeIterables elements, with eager branded validation
fix: never pull the source again once it has reported done
fix: detach the external abort listener on close; await-idempotent cleanup
fix: iterative buffer refill (no stack overflow at large bufferSize)
fix: accurate per-case input-validation messages
docs: why-first README restructure plus ADVANCED.md contract reference

BREAKING CHANGE: Node engine floor is now ^22.16.0 || >=24.0.0 (native Symbol.asyncDispose required; Node 18/20 and non-LTS 23 dropped). TypeScript consumers need TS ≥5.9.
BREAKING CHANGE: bufferSize is strictly validated — 0, negatives, floats, NaN and Infinity throw at construction.
BREAKING CHANGE: cleanupTimeout rejects values above Node's timer maximum (2147483647).
BREAKING CHANGE: malformed source/sub-iterator results surface through the configured error mode with new messages instead of immediately rejecting the racing next().
BREAKING CHANGE: input-validation TypeErrors are reworded per case; the 1.x 'Expected asyncIterable to have a Symbol.asyncIterator function' string is gone.
BREAKING CHANGE: mergeIterables requires an array input, validates elements eagerly with indexed TypeErrors, rejects string elements, and prefetches at construction.
BREAKING CHANGE: input dispatch follows for await — dual-protocol inputs use their async iterator; a nullish Symbol.asyncIterator member falls back to sync iteration; a non-object iterator from the method throws at construction.
BREAKING CHANGE: the source is never pulled again once it has reported done (1.x made up to bufferSize post-done next() calls).
END_COMMIT_OVERRIDE

https://claude.ai/code/session_01FSyTJjbP823pjYfueuSsdu

  • Breaking changes

    • Requires Node.js ^22.16.0 || >=24.0.0 and TypeScript 5.9+.
    • Applies stricter option, iterator-protocol, and mergeIterables() input validation.
    • Aligns iterator dispatch and malformed-result handling with native for await semantics.
    • mergeIterables() returns a BufferedAsyncIterableIterator directly.
  • Features

    • Adds external and per-callback AbortSignal support.
    • Adds Symbol.asyncDispose and configurable cleanupTimeout.
    • Adds fail-fast and fail-eventually error modes.
    • Adds typed public iterator contracts and conditional TypeScript exports.
    • Supports ordered delivery for async-generator callback results.
  • Correctness and resource handling

    • Prevents source pulls after completion.
    • Handles hostile iterator results, synchronous throws, thenables, and non-Error failures.
    • Improves cleanup idempotence, pending-close coordination, listener detachment, abort precedence, and unhandled-rejection prevention.
    • Reduces memory retention and adds bounded-heap regression coverage.
  • Documentation and tooling

    • Restructures README.md and adds ADVANCED.md for API and iterator semantics.
    • Updates package metadata, Node.js configuration, type coverage enforcement, and development guidance.
    • Adds throughput, nested-iterator, and abort benchmarks.
    • Expands the test suite to 141 specs with full library coverage.
    • CI tests Node.js 22 and 24 and runs a benchmark smoke check.

claude added 10 commits May 8, 2026 14:58
Previously, once a callback or source threw, the iterator would record
only the first error and silently drop any subsequent ones still in the
buffer. The captured error was then thrown when the buffer drained.

Capture all errors into an array. On drain, throw the original error
when only one was captured (identity-preserving), or throw an
AggregateError containing all captured errors when there were two or
more.

The existing generator-map rejection test relied on the dropped-errors
behaviour and is updated to unwrap AggregateError when present.
Aliases the new dispose method to the existing return() cleanup path so
`await using it = bufferedAsyncMap(...)` runs source.return(), clears
buffers, and is idempotent on repeat dispose/return calls.

Bumps the supported Node range to >=22.0.0 so the well-known
Symbol.asyncDispose is always available natively (Node 18 and 20 are
both EOL as of May 2026), and updates the tsconfig preset and
@types/node devDep to match.
Each call to bufferedAsyncMap now mints an internal AbortController
whose signal is passed as the second argument to the user callback —
`callback(item, { signal })`. The internal controller is aborted from
inside markAsEnded() so iterator.return(), iterator.throw(), and
Symbol.asyncDispose all surface as `signal.aborted === true` to any
in-flight callback within one microtask, giving callbacks a
fast-path to bail out of long-running fetches/loops on shutdown.

Existing one-arg callbacks keep working — JavaScript ignores the extra
argument — so this widening is non-breaking.

Mirrors the pattern from mcollina/hwp; the consumer-supplied signal
option layered on top arrives in the next commit.
Adds opts.signal: AbortSignal so consumers can cancel iteration without
hand-wiring signal.addEventListener('abort', () => it.return()).

Contract:
- Pre-aborted signal: source.next() is never called and the first
  iterator.next() rejects synchronously with signal.reason.
- Mid-iteration abort: the next pending or freshly-called iterator.next()
  rejects exactly once with signal.reason; subsequent iterator.next()
  calls return { done: true, value: undefined }.
- After abort: the source iterator's .return() runs once via the
  existing markAsEnded() path, and in-flight callbacks observe
  signal.aborted === true on the second-arg signal within one microtask.
- signal.reason is preserved by identity, including non-Error reasons.
- Abort wins over a buffered value resolving in the same tick.
- Holds in both ordered and unordered modes and across sub-iterator
  callbacks.

Implementation:
- Validates options.signal is undefined or AbortSignal at construction
  time.
- Links external → internal AbortController by hand (simple addEventListener,
  no AbortSignal.any) and short-circuits if the iterator was already
  closed via return()/throw()/dispose so a late abort is a no-op.
- nextValue() races the buffered await against an abort sentinel and
  threads abort-state through a dedicated handleAbortIfPending() helper
  so the "reject once, then done forever" contract is centralised.
- The currentStep .next() chain is now then(nextValue, nextValue) so a
  rejection on one .next() does not poison every subsequent call —
  required for the post-abort done semantics.
Adds opts.errors: 'fail-eventually' | 'fail-fast' (default
'fail-eventually', preserving existing semantics).

In 'fail-fast' mode the first error from the callback or the source
short-circuits iteration: the next iterator.next() rejects with the
original error (no AggregateError wrapping), subsequent iterator.next()
calls return { done: true }, source.next() is never called again,
source.return() is called once, and in-flight callbacks observe
signal.aborted === true on the second-arg signal within one microtask.

Implementation reuses commit 4's abort state machine: the captured
error is routed through abortReason and internalAC.abort(err), so the
"reject once, then done forever" contract is identical to external
abort.

Precedence rules (also tested):
- fail-fast + external abort fired before any error  → external reason wins.
- fail-fast + callback error before any external abort → fail-fast wins.
- fail-eventually + external abort fired with errors queued → external
  reason wins; AggregateError discarded.

The default flip to 'fail-fast' and the proposed 'isolate' envelope
mode are deferred to a future major release.
Removes the long-standing describe.skip and rewrites the spec on top of
sinon useFakeTimers (matching return.spec.js), asserting that:

- iterator.throw(err) rejects with err and the next iterator.next()
  returns { done: true, value: undefined }.
- The source iterator's .return() is called exactly once via the
  shared markAsEnded() cleanup path.
- In-flight callbacks observe signal.aborted === true on the second-arg
  signal within one microtask of throw() — confirming the throw path
  reuses the same abort propagation as return()/dispose/external abort.

No production-code change.
Documents the three new public surfaces:

- options.signal: AbortSignal — with a runnable AbortController + setTimeout
  example, an explicit "cancels consumption, not in-flight work" caveat,
  and guidance to forward the per-callback signal into fetch/undici.
- options.errors: 'fail-eventually' | 'fail-fast' — explains the
  AggregateError shape of the default mode, the Promise.all-style
  semantics of fail-fast, and the precedence rule that external abort
  wins over queued/captured errors.
- Symbol.asyncDispose — covers `await using` usage, idempotency, and the
  Node 22+ requirement.

Updates the bufferedAsyncMap signature/options sections to surface the
new fields and the widened (item, { signal }) callback shape.
Both TODO comments (one calling out hwp's AbortController pattern as
inspiration, one wondering if an AbortController could improve
markAsEnded cleanup) are now resolved by the per-callback signal and
external-signal commits.
chai-quantifiers ships its own type declarations (its package.json sets
"types": "src/index.d.ts"), so the separate @types/chai-quantifiers
package is unused. knip has been flagging this; removing it lets the
pre-push check chain pass cleanly.
Captures the JSDoc-as-source convention, the npm test pre-push gate,
the bufferedAsyncMap state machine (internalAC, abortReason,
capturedErrors, fillQueue/nextValue split, markAsEnded as single
cleanup path), the public-API contracts worth preserving, and the
IIFE + clock.runAllAsync test pattern future contributors need to
avoid fake-timer deadlocks.
@voxpelli
voxpelli changed the base branch from copilot/harden-implementation-type-safety to main May 8, 2026 20:58
@voxpelli
voxpelli force-pushed the claude/merge-branches-to-master-nixCB branch from ba54b28 to 1b45e12 Compare May 8, 2026 21:06
Layers minimal refactors on top of the abort-signal feature commits:

- Add normalizeError helper (lib/misc.js) and use it in all 5 fillQueue /
  fail-fast catch sites
- Extract isValueObject helper in lib/type-checks.js (DRY for isIterable /
  isAsyncIterable)
- Add bounds check to ordered-insertion while loop in fillQueue so the
  BufferPromise type cast is honest past array end
- Inline null-safety on the IteratorResult shape check
  (`!result || typeof result !== 'object'`); typeof null === 'object' would
  have let null through to the next-line property accesses

Drops the larger-scope tweaks from the harden / efficiency branches that
weren't earning their keep:

- @voxpelli/typed-utils runtime dep (kept the lib zero-dep; the only
  material correctness win — null check on IteratorResult — is inlined)
- yieldArrayWithItem generator (replaces a 1-element array allocation in
  the common case with a generator allocation; not a measurable win and
  less readable than the spread)
- guardedArrayIncludes (the type cast in isPartOfArray is honest enough)

BREAKING CHANGE: engine requirement bumped from >=18.6.0 to >=22.0.0 (native
Symbol.asyncDispose is required). Callback signature widened from
`(item)` to `(item, { signal })`; existing one-arg callbacks keep working
since JS ignores extra args, but TypeScript consumers that pass a callback
type with a strict single-parameter signature may need to update the type.
@voxpelli
voxpelli force-pushed the claude/merge-branches-to-master-nixCB branch from 1b45e12 to 1d87f8e Compare May 8, 2026 21:16
claude added 6 commits May 9, 2026 10:51
- Add normalizeError(err, defaultMessage) to lib/misc.js entry, with a
  reuse hint to avoid open-coding the err-instanceof-Error pattern.
- Add isObject to lib/type-checks.js entry; note it closes the
  typeof null === 'object' hole and is what isAsyncIterable / isIterable
  / the IteratorResult shape check are now built on.
…variants

- Hoist raceAbort() into a single abortPromise created at construction so
  the {once:true} listener count does not grow per consumer pull. Flatten
  the per-call Promise.race so buffered promises and abortPromise live in
  one input array.
- Use IteratorYieldResult<R> as the cast type for the yielded result
  (still {value} at runtime — the lib type's done is optional false, and
  two existing specs assert on the loose shape).
- Rename local normalisedErr -> normalizedErr to match the helper name.
- Document the load-bearing invariants inline (why internalAC is
  unconditional, why abortPromise is shared, why return() bypasses the
  currentStep chain) and expand JSDoc on bufferedAsyncMap, markAsEnded,
  fillQueue, nextValue, and handleAbortIfPending so future readers don't
  have to re-derive them from tests.

Refresh CLAUDE.md to reflect the shared abortPromise and add an
"Implementation invariants worth preserving" block plus a "Style notes"
note (zero-runtime-dep / one-listener-per-call / unconditional-internalAC,
helper/local spelling convention).
Autofix from npx eslint --fix; pre-existing warning unrelated to the
preceding logic change.
- Rename internalAC -> internalAbortController in index.js and CLAUDE.md
  (the variable was short for AbortController; the abbreviation overloaded
  with "AC X.Y" acceptance-criteria refs and was hard to read).
- Strip "AC X.Y(.Y)?(+...)?: " prefix from 41 it() descriptions across
  test/abort.spec.js, test/dispose.spec.js, test/errors.spec.js,
  test/errors-fail-fast.spec.js, test/per-task-signal.spec.js, and
  test/throw.spec.js. Acceptance-criteria numbering isn't persisted
  outside the test labels themselves so the prefixes don't add value.
- Replace AC-numbered prose in index.js comments and the CLAUDE.md
  invariants block with descriptive references to the spec files.
mergeIterables previously only accepted bufferSize. Widen its options
bag to forward signal, errors, and ordered through to bufferedAsyncMap.
Pure pass-through — no new code paths, no test breakage. Existing
"should process iterables in parallel" still passes unchanged because
it doesn't specify ordered.

Also lands the sensible follow-ups from the /review pass:

- More specific "Expected ... iterator next() result to be an object"
  TypeError messages so the stack distinguishes subiterator vs source
  iterator protocol violations. test/values.spec.js updated.
- README: document the new mergeIterables options; clarify that
  in-flight callbacks see signal.aborted within one microtask of
  iterator close (but Promises can't be cancelled); add an explicit
  "Requirements: Node.js >=22.0.0" line. Fix "simultanoeus" /
  "ordinare" typos.
- test/values.spec.js: three new specs covering signal abort,
  fail-fast error mode, and ordered:true merging.
…ofix sort

- .github/workflows/nodejs.yml: node-versions 18,20,21 -> 22,24. The
  previous matrix was below the engines floor (>=22.0.0) and only passed
  because `[Symbol.asyncDispose]` becomes the string key "undefined"
  when the global symbol is absent — so the dispose specs never
  exercised the real path. Node 22 (current LTS) and Node 24 are now in
  matrix.
- index.js: sort the mergeIterables type and inner option object so
  `errors` precedes `ordered`/`signal` alphabetically; lands the
  perfectionist/sort-objects auto-fix.
- test/values.spec.js: rewrite the "ordered: true" mergeIterables spec
  with asymmetric timing (first source 10x slower than second). Under
  the default ordered:false this would interleave second-* values
  between first-1 and first-2; under ordered:true the first iterable
  drains completely first. The previous symmetric-timing version would
  have passed even if `ordered` had been silently dropped.
@voxpelli
voxpelli marked this pull request as ready for review May 12, 2026 11:59
@voxpelli
voxpelli requested a review from Copilot May 12, 2026 11:59

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR extends bufferedAsyncMap (and the mergeIterables wrapper) with cancellation and improved error surfacing by adding AbortSignal support, a configurable fail-fast error mode, and Symbol.asyncDispose for deterministic cleanup.

Changes:

  • Add options.signal cancellation, a per-callback { signal } argument, and iterator support for Symbol.asyncDispose.
  • Add errors: 'fail-fast' | 'fail-eventually' mode and normalize/aggregate error reporting semantics.
  • Update docs, Node engine/CI targets, and expand test coverage for abort, disposal, per-task signals, and fail-fast behavior.

Reviewed changes

Copilot reviewed 14 out of 15 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tsconfig.json Updates TS config baseline (extends Node 20 config).
package.json Bumps Node engine to ≥22 and updates type deps.
index.js Implements abort handling, per-task signal propagation, fail-fast mode, and Symbol.asyncDispose.
lib/type-checks.js Adds isObject() and refactors iterable guards to use it.
lib/misc.js Adds normalizeError() helper for non-Error rejections.
README.md Documents new options (signal, errors), per-task cancellation, and resource management semantics.
.github/workflows/nodejs.yml Updates CI Node matrix to 22/24.
CLAUDE.md Adds repo guidance and documents the iterator state machine/contracts.
test/values.spec.js Adjusts expectations for single-error vs AggregateError behavior and updated TypeError messages.
test/throw.spec.js Enables/updates throw() tests and adds signal/cleanup assertions.
test/per-task-signal.spec.js Adds coverage ensuring per-task signals exist and abort on close paths.
test/errors.spec.js Adds coverage for fail-eventually error identity vs AggregateError.
test/errors-fail-fast.spec.js Adds comprehensive fail-fast behavior, source cleanup, and abort interaction tests.
test/dispose.spec.js Adds tests for Symbol.asyncDispose cleanup/idempotency.
test/abort.spec.js Adds abort semantics tests including “reject once then done”, cleanup, and race coverage.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread index.js
Comment thread README.md Outdated
claude added 2 commits May 12, 2026 12:15
Previously handleAbortIfPending threw synchronously when an abort was
pending fresh delivery, propagating out of nextValue (async) as a
rejected promise before the await markAsEnded() lines ran. For direct
.next() consumers this meant source.return() wasn't called until either
a subsequent .next() or an explicit iterator.return() — the for await
loop's implicit cleanup-on-exception papered over the issue, but the
abort-delivery contract was not actually carrying its own cleanup.

Restructure handleAbortIfPending to return a discriminated descriptor
({kind:'throw', reason} | {kind:'done'} | undefined) instead of
throwing. The two call sites in nextValue (early-abort check and
post-race branch) now run markAsEnded() before propagating the throw,
matching the fail-fast branch's ordering.

Pin the new contract in test/abort.spec.js by asserting
returnSpy.calledOnce after the first rejecting .next() resolves, before
the drain call that previously masked the issue.

Also reword README §"Resource management": Symbol.asyncDispose is
equivalent to iterator.return() for cleanup, not literally aliased (the
dispose method has its own body returning Promise<void> per the
AsyncDisposable contract).

Resolves copilot-pull-request-reviewer comments on PR #50.
- .knip.jsonc: drop the stale `entry` array (no benchmark/ dir; index.js
  is auto-detected from package.json exports) — knip no longer prints
  configuration hints on every check run.
- index.js: remove the stale `// TODO: Add "throw"` comment that sat
  directly above the implemented throw() method; document the invariant
  behind the currently-unreachable ordered-insertion loop body so the
  coverage gap reads as intentional.
- test: cover normalizeError's non-Error branch via a callback that
  rejects with a non-Error value; lib/misc.js is now at 100%.

PR description rewritten and the two copilot-pull-request-reviewer
threads resolved separately on GitHub.
@voxpelli voxpelli changed the title Add AbortSignal support and fail-fast error handling Add AbortSignal, Symbol.asyncDispose and fail-fast error handling (v2.0.0) May 14, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Compliance Checks Passed!

@voxpelli voxpelli changed the title Add AbortSignal, Symbol.asyncDispose and fail-fast error handling (v2.0.0) feat: add AbortSignal, Symbol.asyncDispose and fail-fast error handling (v2.0.0) May 14, 2026
@voxpelli voxpelli changed the title feat: add AbortSignal, Symbol.asyncDispose and fail-fast error handling (v2.0.0) feat!: add AbortSignal, Symbol.asyncDispose and fail-fast error handling (v2.0.0) May 14, 2026
claude added 13 commits July 7, 2026 18:37
deliverAbort was handleAbortIfPending's only caller, and the
{ kind: 'throw' | 'done' } descriptor protocol existed solely to keep the
claim non-throwing so cleanup could run before the reason propagated.
With the two folded together the claim ordering is visible in one
function: claim synchronously (delivered = true plus the isDone
suppression read, no await before it, so concurrent pulls can't both
win), await markAsEnded(), then throw the claimed reason or resolve
done. Behaviour is unchanged — the abort sweep, suppression and
exactly-once specs all pin it.

Also updates the construction-time listener comment and the CLAUDE.md
dispatcher paragraph that described the deleted descriptor protocol.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FSyTJjbP823pjYfueuSsdu
…ures

- Replace the hand-rolled outcome-accumulator IIFEs in
  errors-fail-fast.spec.js (5 sites), abort.spec.js (exactly-once spec +
  the drain-race sweep's collection loop) and values.spec.js (merge
  fail-fast spec) with collectNextOutcomes /
  expectSingleRejectionThenDone from test/utils.js. The sweep keeps its
  custom per-offset identity assertions (the helper's messages lack the
  offset context); the exactly-once spec keeps its stronger
  rejection-lands-first check on top of the helper.
- fromArray becomes a re-export of lib/misc.js's makeIterableAsync —
  the fixture was an exact twin of the library's own sync-to-async shim.
- The very-large-bufferSize spec drops from 100_000 to 20_000: ~3x the
  measured ~7000-frame recursion crash point at roughly a tenth of the
  runtime, same regression power. The "deliberately NOT exercised"
  caveat block stays.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FSyTJjbP823pjYfueuSsdu
Review-round follow-ups, all consequence-free for runtime behavior:

- index.js: the mergeIterables docblock falsely claimed one-shot getter
  members are unsupported "same as the main input" — the main input
  captures its member on a single read and invokes it (pinned by the
  reads === 1 spec); elements are deliberately stricter. Also add the
  convention-mandated drain-boolean comment to drainOrContinue's
  markAsEnded(true) call site.
- CLAUDE.md: drop the deleted isIterable from the lib-helpers inventory.
- benchmark/fixtures.js: syncRange's docblock described the removed
  isIterable dispatch branch; it now names the captured-Symbol.iterator
  shim actually under measurement.
- test/utils.js: expectSingleRejectionThenDone now rejects extra
  enumerable keys on post-rejection results, restoring the strictness of
  the deep-equal assertions it replaced (a leaked internal envelope with
  done/value looking right must still fail).
- test/values.spec.js: finish the merge fail-fast spec's migration —
  expectSingleRejectionThenDone pins exactly-once + done-forever, which
  the hand-rolled tail never asserted.
- test/hostile-results.spec.js: reuse promisableTimeout instead of an
  open-coded setTimeout promise, and correct the fixture docblock's
  "all timer-free" claim (the file uses one real 20 ms delay; fake
  timers would deadlock it).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FSyTJjbP823pjYfueuSsdu
Two classification defects in the fillOneSlot arms, both probe-confirmed
against native for-await:

- A Proxy result whose has-trap answers true for every key spoofed the
  private ERR-symbol check and was classified as an internal error
  envelope carrying an undefined error: iteration ended silently
  mid-stream (no rejection, values dropped) and the still-open source was
  never .return()ed. The ERR tag is now brand-verified — kind 2 requires
  the same-realm Error every catch handler attaches via normalizeError
  (which wraps cross-realm throwables, so the check is loss-free for
  internal envelopes). A spoofed tag falls through to the done/value
  reads, matching what native for-await does with such a proxy: falsy
  done, yield the (undefined) value, keep pulling. A proxy that
  additionally forges an Error under the symbol read surfaces a loud
  stream error — equivalent to a rejecting next(); no silent path
  remains. Throwing has/get traps keep their existing kind-1 routing.

- ECMA-262 only requires Type(IteratorResult) is Object, which includes
  function objects; isObject rejected callables as malformed where
  native for-await yields their value. Both arms now classify with the
  new isSpecObject from lib/type-checks.js (callables included), and
  isObject becomes an internal (unexported) building block since no
  module-external consumer remains.

The twin classification blocks gain explicit keep-in-sync
cross-references. bufferSize-scaling benches at or below prior baselines.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FSyTJjbP823pjYfueuSsdu
The dispatch that decides whether a callback result fans out as a nested
iterable used an `in`-presence probe plus a separate re-read invoke,
producing three inconsistent outcomes for the same shapes (all
probe-confirmed):

- `{ [Symbol.asyncIterator]: undefined }` from a sync callback threw an
  unbranded TypeError that leaked an internal variable name, while the
  identical object from an async callback was yielded as a value.
- A callable member returning `null` yielded the hybrid as data (the
  wave-2 livelock fix), while one returning `42` surfaced later as an
  unbranded 'Unknown subiterator error'.

It now follows GetMethod: one [[Get]] of the member (a has-trap is never
consulted, matching for-await); callable -> the captured method is
invoked and a non-object return is a branded stream TypeError (for-await
parity, per maintainer decision — this supersedes the wave-2
yield-as-value pin while keeping its no-livelock guarantee); nullish or
non-callable -> the result is plain data and is yielded, from sync and
async callbacks alike.

Spec updates: the null-iterator pin now asserts the branded rejection;
new pins for the truthy non-object iterator, the sync/async consistency,
and the throwing-has hybrid (only [[Get]] is consulted, so it yields;
a throwing member READ still routes through the errors mode).

Accepted residual: a get-trap-throwing proxy reaches this dispatch only
from a sync callback (the envelope flag is probed pre-await); from an
async callback it is yielded unread. Non-silent on both paths;
eliminating it would need an envelope-shape change.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FSyTJjbP823pjYfueuSsdu
GetIteratorFromMethod parity: when the captured Symbol.asyncIterator
method returns a non-object, native for-await throws an immediate
TypeError ('Result of the Symbol.asyncIterator method is not an
object'). The pipeline instead accepted it at construction and the
failure surfaced later as a deferred, unbranded AggregateError wrapping
'Unknown iterator error' (probe-confirmed). Construction now throws the
branded TypeError on the spot.

The sync arm is deliberately not eagerly checked: its captured method is
invoked lazily inside the makeIterableAsync shim (an eager invocation
would add a construction-time side effect) and for-of brands its own
TypeError at that point.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FSyTJjbP823pjYfueuSsdu
The three input-validation sites shared one string ('Expected
asyncIterable to have a Symbol.asyncIterator function') that was
misleading for every case it covered:

- a plain object lacking both protocols was told only about
  Symbol.asyncIterator, even though adding Symbol.iterator would have
  been accepted -> now 'Expected asyncIterable to have a callable
  Symbol.asyncIterator or Symbol.iterator';
- a string input HAS a callable Symbol.iterator, so any protocol-naming
  message is false for it -> now a dedicated message mirroring the
  mergeIterables string guidance ('spread it first if iterating
  characters is intended');
- a non-nullish non-callable Symbol.asyncIterator member deliberately
  has NO sync fallback (GetMethod), so naming both protocols would
  mislead -> now 'Expected the Symbol.asyncIterator member to be a
  function'.

All four existing pins updated; the string rejection gains its first
direct pin.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FSyTJjbP823pjYfueuSsdu
- README mergeIterables: the quoted eager-validation error prefix was
  stale ('Expected input[1] to be ...'); quote the actual callability
  message.
- README bufferSize: claim what the regression spec pins (20 000) —
  'hundreds of thousands' exceeded any tested size after the spec was
  right-sized.
- bufferedAsyncMap JSDoc: qualify the per-callback signal's
  source-exhaustion bullet — the abort fires during end-of-stream
  cleanup after in-flight work drained, so callbacks never observe
  aborted === true mid-flight in that case (matching the pinned spec);
  the hover-doc previously implied a reachable fast-path there.
- test/abort.spec.js: retitle the 'behaves identically to commit 3'
  spec — the branch-internal breadcrumb is meaningless post-merge; the
  title now states the actual pin.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FSyTJjbP823pjYfueuSsdu
fillQueue's refill loop kept calling the main iterator's next() after a
done result had already resolved and been classified — up to bufferSize
extra pulls that native for-await would never make. Against a defensive
source ("cursor already closed") those post-done pulls surfaced as
spurious captured errors (an AggregateError of identical errors at
default options); a post-done next() returning a never-settling promise
wedged the consumer forever; and in fail-fast mode a spurious post-done
error could win the unordered race against a slow in-flight value and
silently drop it.

fillOneSlot now signals whether it filled a slot, and returns false
without pulling when the dispatch pick falls through to a main iterator
that has already reported done — fillQueue stops refilling and the
buffered slots drain on their own. Sub-iterators need no equivalent
guard (a done sub leaves the rotation in its own classification arm),
and the construction-time speculative fill is unchanged: up to
bufferSize concurrent pulls before the first result resolves remain the
library's documented prefetch contract (README now spells out the
implication for sources that throw on concurrent or trailing next()).

The over-pull reproduces byte-for-byte on 1.x, so this is an inherited
gap rather than a 2.0.0 regression — but the fail-fast value-drop
manifestation is new with the errors option, hence fixed here. Found by
the adversarial go/no-go review pass via live differential probes
against native for await.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FSyTJjbP823pjYfueuSsdu
- New "Why" opener: the gap between sequential for-await and array-bound
  concurrency helpers, what bounded buffering/backpressure buys, and two
  explicit reach-for-something-else cases.
- Usage examples are now concrete and complete: a paginated-API fan-in as
  the lead example, an async-generator fan-out over directories, and a
  first mergeIterables example (previously undocumented by example).
- The deep contract notes (speculative prefetch model, iterator-protocol
  details, abort delivery/precedence, error-mode mechanics, memory
  guarantees) move to ADVANCED.md, each README section keeping a short
  summary plus an anchored link. Section anchors referenced from code
  comments (Cancellation, Errors, Resource management) are unchanged.
  (Root-level file rather than docs/ — the boilerplate .gitignore
  reserves /docs.)
- The Performance section drops the pre-release-relative optimisation
  history (internal baselines no consumer ever saw); the three
  consumer-relevant findings stay, plus a new long-stream memory bullet.
- Similar modules gains p-map and web/Node streams with one-line
  positioning against each.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FSyTJjbP823pjYfueuSsdu
The README links to it per section; including it in files means the
links have a local target inside node_modules, not only on GitHub.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FSyTJjbP823pjYfueuSsdu
Both files live at the repo root, so ../README.md 404s on GitHub and npm.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FSyTJjbP823pjYfueuSsdu
Three explorer agents (new-consumer, reference-accuracy, ecosystem
conventions) proposed 23 improvements; an adversarial gatekeeper
re-verified every premise and killed or trimmed 9 of them (TOC, second
lead example, inline recipes, error-message appendix and other re-bloat
of the just-leaned README). The 14 survivors:

- declaration.tsconfig.json: removeComments false — the base
  @voxpelli/tsconfig strips ALL JSDoc from the emitted index.d.ts, so
  editor hover showed bare signatures; the published declarations now
  carry the documented contracts (verified by emitting).
- index.js: @param/@returns one-liners with inline defaults on the two
  exported functions, now that they reach consumers.
- README accuracy: the API summary no longer reads as "only the first
  bufferSize items"; the ordered bullet is defined non-circularly
  (delivery order, concurrency unchanged); the least-targeted
  load-balancing claim is qualified as unordered-only; string inputs
  noted as eagerly rejected ("ordinary iterable" overclaimed); the
  p-map Similar-modules bullet corrected — pMapIterable does accept
  async-iterable input, so the honest differentiators are
  strict-ordering-only, no fan-out/merge, no dispose/abort contract.
- README structure: one-line npm-install fence at the top of Usage;
  mergeIterables options deduplicated down to `ordered` plus a
  cross-link (removing already-drifted bufferSize wording); the
  thin-wrapper sentence now states the exact bufferedAsyncMap
  equivalence.
- ADVANCED.md: new "Ordered mode" section (delivery-vs-dispatch,
  generator-value contiguity, abort/error parity — all spec-pinned);
  concurrent next() request-queue paragraph; the same-realm note
  extended to non-Error throw normalization (.cause carries the
  original).
- package.json: description replaced with the README tagline (the old
  one predated v2); keywords expanded with the niche's actual search
  vocabulary.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FSyTJjbP823pjYfueuSsdu
claude added 2 commits July 10, 2026 21:41
The Performance section stated unconditionally that the internal
Promise.race grows with the buffer, but ordered mode races only the
head entry against the park — O(1) per pull regardless of bufferSize.
The options-list and ADVANCED.md wordings were already qualified;
this brings the third mention in line.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FSyTJjbP823pjYfueuSsdu
The ordered-mode docs stated that callbacks run concurrently up to
bufferSize without qualification, but that only holds for plain-value
callbacks. Async-generator callbacks run strictly one at a time in
ordered mode: the buffer always feeds from the current sub-iterator, so
bufferSize only buffers the undispatched generator objects and does not
step them concurrently.

- ADVANCED.md "Ordered mode": scope the concurrency claim to plain-value
  callbacks and spell out the generator exception (dispatch, not just
  delivery), with the "return a value instead of yielding" workaround.
- README.md: qualify the `ordered` option note and add a Performance
  bullet pointing at the same limitation.
- test/values.spec.js: pin the serialization directly (maxInFlight === 1
  for a generator callback under ordered:true) so the behaviour is
  deliberate rather than only implied by the nested-timing spec.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FSyTJjbP823pjYfueuSsdu
@voxpelli

voxpelli commented Aug 1, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Aug 1, 2026

Copy link
Copy Markdown
✅ Action performed

Full review finished.

@coderabbitai

coderabbitai Bot commented Aug 1, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The PR expands bufferedAsyncMap and mergeIterables with cancellation semantics, cleanup deadlines, configurable error modes, per-callback abort signals, protocol validation, ordered execution, async disposal support, and comprehensive test and benchmark coverage. Node.js 22 is now the minimum, with 22 and 24 tested in CI.

Changes

Buffered iterator behavior and API expansion

Layer / File(s) Summary
Library setup, helpers, and package exports
index.js, lib/misc.js, lib/type-checks.js, package.json, tsconfig.json, declaration.tsconfig.json, test/type-contract.spec.js
Type guards now recognize spec objects and function-valued async iterables. Error normalization preserves Error instances and attaches non-nullish causes. Package exports conditional types. Node.js 22 is minimum; type coverage raised to 99%.
Core iterator logic and state machine
index.js
Implements buffering, abort coordination, cleanup timeouts, error routing, and per-pull state. Normalizes iterator steps safely. Validates sub-iterator results. Coordinates concurrent pulls and cleanup.
Abort signaling, cancellation, and cleanup
test/abort.spec.js, test/per-task-signal.spec.js
External abort signals and per-callback abort signals are created and propagated. Cleanup timeouts bound teardown duration. Abort listeners are attached and detached correctly. Stale aborts are suppressed after close.
Error handling: fail-fast and fail-eventually modes
test/errors-fail-fast.spec.js, test/errors.spec.js
Fail-fast stops immediately; fail-eventually aggregates errors into AggregateError. Errors from callbacks and sources are normalized and routed. Abort errors take precedence. Ordered and unordered modes apply both strategies.
Iterator protocol: return(), throw(), and async disposal
test/return.spec.js, test/throw.spec.js, test/dispose.spec.js
The return() method awaits values and closes the source. The throw() method closes before rejecting. Symbol.asyncDispose enables await using scope cleanup. Concurrent operations resolve in correct order.
Input validation and protocol safety against hostile results
test/basic.spec.js, test/hostile-results.spec.js
Eager validation rejects primitive and boxed strings, non-objects, and invalid iterator members. Malformed iterator results, Proxies, spoofed error envelopes, and throwing getters are detected and safely normalized without unhandled rejections or cleanup races.
Test utilities and comprehensive value/merge behavior
test/utils.js, test/values.spec.js, test/yield.spec.js
Reusable async iterator stubs and outcome-collection helpers support error unwrapping and rejection assertions. Comprehensive tests cover callback result routing, ordered concurrency, and mergeIterables validation and option forwarding.
Memory and garbage-collection regression
test/memory.spec.js
Memory test confirms unbounded buffering does not leak retained promises or abort listeners. Uses exposed GC and enforces 3 MB growth limit over 20,000 pulls.
Documentation
README.md, ADVANCED.md, CLAUDE.md
README expands usage examples, option reference, cancellation, error modes, and resource management. ADVANCED.md specifies edge cases, memory guarantees, and iterator semantics. CLAUDE.md guides contributors on architecture, testing, and benchmarking.
Benchmarks, CI matrix, and validation tooling
benchmark/*.js, .github/workflows/nodejs.yml, .mocharc.json, .knip.jsonc
Throughput, nested-iterator, and abort benchmarks measure overhead and cleanup costs. Node.js 22/24 CI includes benchmark smoke test. Mocha enables expose-gc. Knip validates entry points.

Estimated code review effort: 4 (Complex) | ~75 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant Iterator as bufferedAsyncMap/<br>mergeIterables
  participant SourceIterator
  participant Callback
  participant Cleanup
  Caller->>Iterator: next() to consume value
  Iterator->>SourceIterator: pull next item
  SourceIterator-->>Iterator: { value, done }
  Iterator->>Callback: invoke callback with item<br>and AbortSignal
  Callback-->>Iterator: Promise or AsyncIterable
  Iterator-->>Caller: buffered value
  Caller->>Iterator: return() or throw()<br>or await using disposal
  Iterator->>Cleanup: close SourceIterator<br>with timeout
  Cleanup-->>Iterator: cleanup settled<br>or deadline reached
  Iterator-->>Caller: completion or rejection
Loading

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the 2.0.0 release and its main API changes, including cancellation, disposal, error modes, and cleanup timeouts.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/merge-branches-to-master-nixCB

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🧹 Nitpick comments (5)
test/type-contract.spec.js (1)

31-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Prefer @ts-expect-error over @ts-ignore here.

mapped is annotated as AsyncIterableIterator<number>, which has no Symbol.asyncDispose member, so the suppression is needed today. @ts-ignore stays silent if the TypeScript lib later adds Symbol.asyncDispose to AsyncIterableIterator. @ts-expect-error fails the build at that point, which is exactly the signal this file exists to produce. The rest of the suite already uses @ts-expect-error for intentional type violations.

♻️ Proposed change
-    // `@ts-ignore` — Symbol.asyncDispose is not part of AsyncIterableIterator
+    // `@ts-expect-error` -- Symbol.asyncDispose is not part of AsyncIterableIterator
     chai.expect(mapped[Symbol.asyncDispose]).to.be.a('function');
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/type-contract.spec.js` around lines 31 - 32, Replace the `@ts-ignore`
directive above the mapped[Symbol.asyncDispose] assertion with `@ts-expect-error`,
keeping the existing assertion and AsyncIterableIterator annotation unchanged.
test/errors.spec.js (1)

64-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Wrap this drain in an IIFE to match the suite convention.

Lines 64-71 run the for await inline and call clock.runAllAsync() afterwards at line 73. This works only because fromArray and the sinon stubs settle on microtasks. If the source or callback ever gains a timer, the loop suspends with no running clock and the test hangs. Lines 43-48 and 109-113 in this same file already use the IIFE shape. Lines 88-95 and 132-138 have the same inline structure.

I raise this as one root cause with the same pattern in test/errors-fail-fast.spec.js; see the consolidated comment.

As per coding guidelines: "wrap async flows needing fake-timer advancement in an IIFE rather than using an inline for await block".

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/errors.spec.js` around lines 64 - 73, Wrap the async drain loop in the
error test’s try block inside an async IIFE, matching the existing patterns in
this suite, so the loop can suspend while clock.runAllAsync advances fake
timers. Preserve the current bufferedAsyncMap invocation, callback, bufferSize,
and error capture behavior, and apply the same inline for-await restructuring to
the corresponding flow in errors-fail-fast.spec.js.

Source: Coding guidelines

test/per-task-signal.spec.js (1)

160-164: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Also assert that the signal aborts after exhaustion completes.

The test proves the negative half of the contract: callbacks do not observe aborted === true mid-flight. index.js lines 157-160 also promise the positive half — on natural source exhaustion the abort fires during end-of-stream cleanup. Nothing here pins that. A regression that stopped aborting on exhaustion would leave in-flight callbacks without a shutdown signal and would pass this suite.

♻️ Proposed addition
     await clock.runAllAsync();
     await flow;
 
     result.should.have.length(2);
+    // The other half of the contract: the abort fires during end-of-stream
+    // cleanup, after in-flight work has drained.
+    chai.expect(lastSignal?.aborted).to.equal(true);
   });
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/per-task-signal.spec.js` around lines 160 - 164, Add an assertion to the
test around the existing await clock.runAllAsync() and await flow sequence
confirming the signal is aborted after natural source exhaustion completes. Keep
the existing mid-flight non-aborted assertions and result length check, and
verify the post-exhaustion state through the test’s existing signal reference.
test/values.spec.js (1)

988-991: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use the imported unwrapCapturedError helper here.

This block open-codes the exact AggregateError unwrap that unwrapCapturedError performs. The file already imports the helper at line 20 and uses it at lines 596, 745, and 947. Keeping one copy means a future change to the drain-throw convention lands in one place.

♻️ Proposed refactor
-    const captured = outcome.rejectedWith instanceof AggregateError
-      ? outcome.rejectedWith.errors[0]
-      : outcome.rejectedWith;
-    captured.should.equal(malformedError);
+    unwrapCapturedError(outcome.rejectedWith).should.equal(malformedError);

As per coding guidelines: "Reuse helpers from test/utils.js".

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/values.spec.js` around lines 988 - 991, Replace the inline
AggregateError handling in the captured error assertion with the imported
unwrapCapturedError helper, passing outcome.rejectedWith and preserving the
existing malformedError equality assertion.

Source: Coding guidelines

test/hostile-results.spec.js (1)

399-439: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

The 20 ms real delay makes this spec wall-clock dependent.

This file runs without fake timers on purpose, and the comment explains why. The consequence is that the only ordering lever in this spec is a real 20 ms sleep. That is enough today, but on a loaded CI runner the main pull can resolve later than intended and the "buffer is empty when the sub-iterator registers" precondition can shift, which turns a deduplication regression into a pass instead of a failure. A deterministic gate (an externally resolved promise released after the done-terminals are consumed) would pin the same window without depending on timing.

As per coding guidelines: "use fake timers with clock.runAllAsync() or clock.tickAsync(ms) for deterministic timing."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/hostile-results.spec.js` around lines 399 - 439, The hostile-results
spec should replace the real 20 ms delay in the source iterator’s first next()
with a deterministic externally controlled promise gate. Release that gate only
after the done-terminals have been consumed, preserving the empty-buffer and
four in-flight sub-pull ordering while removing wall-clock dependence; keep the
existing assertion that subReturn is called once.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@lib/type-checks.js`:
- Around line 19-26: Update isAsyncIterable to use isSpecObject instead of the
narrower isObject guard, so function values with a callable Symbol.asyncIterator
member are recognized consistently with the callback-result contract and
consume-time handling. Preserve the existing Symbol.asyncIterator membership
check.

In `@package.json`:
- Around line 38-44: Add the generated lib/**/*.d.ts and corresponding
declaration map files to the package files list so declarations emitted by
declaration.tsconfig.json accompany the published lib/**/*.js modules imported
by index.js. Preserve the existing package entries and ensure the published type
paths resolve correctly.

In `@README.md`:
- Line 255: Update the README’s plain for-await-of cleanup statement to clarify
that iterator.return() is invoked only on early exits such as break or throw,
not after natural exhaustion; if discussing normal-completion cleanup, describe
it separately through the relevant Symbol.asyncDispose or await using behavior.

In `@test/errors-fail-fast.spec.js`:
- Around line 205-243: In test/errors-fail-fast.spec.js at lines 205-243,
192-199, and 303-310, and test/errors.spec.js at lines 64-73, 88-95, and
132-138, replace each inline for-await drain with an async IIFE, start it,
advance the fake clock with clock.runAllAsync(), then await the drain; preserve
the existing error assertions and iterator cleanup checks.

In `@test/hostile-results.spec.js`:
- Around line 51-52: Add a JSDoc `@returns` annotation to the
nonCallableMemberShape helper, describing that it returns the data object
containing a non-callable Symbol.asyncIterator member. Keep the helper
implementation unchanged.

In `@test/memory.spec.js`:
- Around line 36-47: Increase the Mocha timeout for the long-running memory test
around the iterator.next() loops so 22,000 awaited calls can complete reliably
on slow CI; configure it at the test or suite level using the existing memory
spec structure, and add fake timers only if the test’s asynchronous operations
require deterministic time control.

---

Nitpick comments:
In `@test/errors.spec.js`:
- Around line 64-73: Wrap the async drain loop in the error test’s try block
inside an async IIFE, matching the existing patterns in this suite, so the loop
can suspend while clock.runAllAsync advances fake timers. Preserve the current
bufferedAsyncMap invocation, callback, bufferSize, and error capture behavior,
and apply the same inline for-await restructuring to the corresponding flow in
errors-fail-fast.spec.js.

In `@test/hostile-results.spec.js`:
- Around line 399-439: The hostile-results spec should replace the real 20 ms
delay in the source iterator’s first next() with a deterministic externally
controlled promise gate. Release that gate only after the done-terminals have
been consumed, preserving the empty-buffer and four in-flight sub-pull ordering
while removing wall-clock dependence; keep the existing assertion that subReturn
is called once.

In `@test/per-task-signal.spec.js`:
- Around line 160-164: Add an assertion to the test around the existing await
clock.runAllAsync() and await flow sequence confirming the signal is aborted
after natural source exhaustion completes. Keep the existing mid-flight
non-aborted assertions and result length check, and verify the post-exhaustion
state through the test’s existing signal reference.

In `@test/type-contract.spec.js`:
- Around line 31-32: Replace the `@ts-ignore` directive above the
mapped[Symbol.asyncDispose] assertion with `@ts-expect-error`, keeping the
existing assertion and AsyncIterableIterator annotation unchanged.

In `@test/values.spec.js`:
- Around line 988-991: Replace the inline AggregateError handling in the
captured error assertion with the imported unwrapCapturedError helper, passing
outcome.rejectedWith and preserving the existing malformedError equality
assertion.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: ce67d78a-573d-4da7-a314-7888ba898a8b

📥 Commits

Reviewing files that changed from the base of the PR and between bb7edf5 and ee2a29f.

📒 Files selected for processing (31)
  • .github/workflows/nodejs.yml
  • .knip.jsonc
  • .mocharc.json
  • ADVANCED.md
  • CLAUDE.md
  • README.md
  • benchmark/abort.js
  • benchmark/fixtures.js
  • benchmark/index.js
  • benchmark/nested.js
  • benchmark/throughput.js
  • declaration.tsconfig.json
  • index.js
  • lib/misc.js
  • lib/type-checks.js
  • package.json
  • test/abort.spec.js
  • test/basic.spec.js
  • test/dispose.spec.js
  • test/errors-fail-fast.spec.js
  • test/errors.spec.js
  • test/hostile-results.spec.js
  • test/memory.spec.js
  • test/per-task-signal.spec.js
  • test/return.spec.js
  • test/throw.spec.js
  • test/type-contract.spec.js
  • test/utils.js
  • test/values.spec.js
  • test/yield.spec.js
  • tsconfig.json

Comment thread lib/type-checks.js Outdated
Comment thread package.json
Comment thread README.md Outdated
Comment thread test/errors-fail-fast.spec.js
Comment thread test/hostile-results.spec.js Outdated
Comment thread test/memory.spec.js
claude added 3 commits August 4, 2026 02:19
Six inline `for await` drains advanced the fake clock only after the loop
had already returned, so any timer in the source or callback would deadlock
the test. They pass today because their sources happen to resolve without
needing the clock, but the fail-fast source spec drains a timer-driven
yieldValuesOverTime and is one bufferSize change away from hanging. Wrap
each in an async IIFE and advance the clock concurrently, matching the
pattern already used elsewhere in both files.

Also give the memory spec an explicit 30s timeout: 22k awaited pulls plus
repeated forced GC finish in ~200ms locally but have no margin against
Mocha's 2s default on a loaded CI runner, where a timeout would read as a
memory regression rather than a slow box.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FSyTJjbP823pjYfueuSsdu
`for await … of` runs IteratorClose only on abrupt completion — break,
return or throw — not on natural exhaustion, so the README's "when the loop
completes" overstated it. State the early-exit cases precisely and explain
why normal completion needs no `.return()`: a source that ran to exhaustion
has already run its own cleanup, and this iterator runs its end-of-stream
cleanup on that same path.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FSyTJjbP823pjYfueuSsdu
The callback-result fan-out gate used an object-only check while the
consume-time GetMethod read that follows it uses isSpecObject, which accepts
callables. A callback returning a function that carries a callable
Symbol.asyncIterator therefore lost its fan-out: the dispatch gate rejected
it and it was delivered as a plain value, even though `for await` iterates
such a function happily — ECMA "Type(x) is Object" includes callables, and
the fan-out is itself a GetIteratorFromMethod position.

Build isAsyncIterable on isSpecObject so both ends agree. Only the exotic
callable-carrying-Symbol.asyncIterator case changes: a function without the
member still fails the `in` check, and plain objects and async generators
are unaffected. This leaves isObject with no callers, so it goes.

BREAKING CHANGE: a callback result that is a function carrying a callable
Symbol.asyncIterator is now fanned out as a nested async iterable instead of
being delivered as a plain value.

Also adds the missing JSDoc @returns on nonCallableMemberShape, which was
the only lint warning left in the repo.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FSyTJjbP823pjYfueuSsdu
claude added 2 commits August 4, 2026 14:55
…tile traps

Four defects surfaced by adversarial review, each reproduced before fixing:

The catch-envelope brand was spoofable: kind-2 classification accepted any
result where `result[ERR] instanceof Error`, and a Proxy get-trap can answer
the private symbol with a fresh same-realm Error. The spoof dropped the real
value, surfaced the planted error, and — classified as a protocol-closed
rejection — skipped the source's cleanup .return() entirely. The brand is
now a module-local WeakSet populated only at the two mint sites; foreign
objects can never be members, and a confirmed member's ERR read touches no
traps. Falls through to the done/value reads exactly like for-await.

The fan-out gate probed with `Symbol.asyncIterator in value` — a [[Has]] —
while GetMethod (and the consume-time re-read) consult only [[Get]], so a
proxy whose has/get traps disagree was delivered as a plain value where
for-await iterates it. isAsyncIterable now does the single [[Get]], and the
consume-time re-read is extracted into subIteratorFromResolved — one shared
implementation for every dispatch mode, ending the verbatim twin copy.

doCleanup read the foreign `return` member twice (truthy check + reinvoke),
so a stateful getter passed the check and then invoked nothing — cleanup
silently skipped. The member is read once and the captured method invoked.
Close targets are also Set-deduped so one iterator reachable through two
lists gets its .return() exactly once, matching the pinned contract.

Also: the empty string now gets the string-specific validation message (the
falsy guard swallowed it as "no input"), and the mergeIterables validation
docblock records its residual deferred case (callable member returning a
non-object — invoke-checking it would be a construction side effect).

All pinned in test/hostile-results.spec.js / test/basic.spec.js, verified
failing pre-fix.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FSyTJjbP823pjYfueuSsdu
…bench note

README said "after the first error … no further items are pulled", but pulls
only stop once the error is CAPTURED — consumed, which with ordered: true can
be long after it occurred — matching what ADVANCED.md already states. The
cleanupTimeout bullet now names the 2^31-1 ceiling and that Infinity is
rejected (omit the option for unbounded waiting). CLAUDE.md's envelope-brand
description follows the WeakSet mechanism. The fail-eventually bench comment
claimed every error lands in capturedErrors; refills stop after the first
capture, so the aggregate is bufferSize-sized by design.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FSyTJjbP823pjYfueuSsdu

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
benchmark/abort.js (1)

50-52: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Assert that failure-path benchmarks actually reject.

These benchmarks only call .catch(doNotOptimize). If abort or error delivery regresses and the operation resolves, the catch handler is skipped and Mitata records a successful run. The benchmark then measures normal completion instead of the documented abort or error path. Assert rejection for the pre-aborted, mid-stream abort, fail-fast, and fail-eventually cases with assert.rejects or an equivalent explicit postcondition.

Also applies to: 57-65, 69-70, 79-80

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@benchmark/abort.js` around lines 50 - 52, Update the failure-path benchmarks
around the pre-aborted, mid-stream abort, fail-fast, and fail-eventually cases
to assert that the awaited iterator operation rejects, using assert.rejects or
an equivalent explicit rejection postcondition instead of only catching errors
with doNotOptimize. Keep the benchmark operations and covered scenarios
unchanged while ensuring successful resolution causes the benchmark to fail.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@benchmark/abort.js`:
- Around line 50-52: Update the failure-path benchmarks around the pre-aborted,
mid-stream abort, fail-fast, and fail-eventually cases to assert that the
awaited iterator operation rejects, using assert.rejects or an equivalent
explicit rejection postcondition instead of only catching errors with
doNotOptimize. Keep the benchmark operations and covered scenarios unchanged
while ensuring successful resolution causes the benchmark to fail.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: bea7ae73-71f0-4d44-b2db-5e4fe30893e3

📥 Commits

Reviewing files that changed from the base of the PR and between b988bb7 and 8723e2d.

📒 Files selected for processing (7)
  • CLAUDE.md
  • README.md
  • benchmark/abort.js
  • index.js
  • lib/type-checks.js
  • test/basic.spec.js
  • test/hostile-results.spec.js
🚧 Files skipped from review as they are similar to previous changes (6)
  • lib/type-checks.js
  • CLAUDE.md
  • test/hostile-results.spec.js
  • test/basic.spec.js
  • README.md
  • index.js

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants