From 174e8de17948faad30742b2df937e9724e59584a Mon Sep 17 00:00:00 2001 From: jinye Date: Tue, 26 May 2026 14:21:49 +0800 Subject: [PATCH] fix(core): stop AbortSignal listener leak in long sessions (MaxListenersExceededWarning) (#4366) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(core): consolidate AbortController handling to stop listener leaks in long sessions Users hit `MaxListenersExceededWarning: 1509 abort listeners added to [AbortSignal]` in long interactive sessions. The agent runtime nests parent→child controllers (masterAbortController → per-message round → per-API-call round → tool execution) and each layer registered its own `addEventListener('abort', ...)` on the parent without `{once:true}` or reverse cleanup, so listeners accumulated on long-lived parents across hundreds of model turns. Add `utils/abortController.ts` with three helpers: - `createAbortController(maxListeners = 50)` — factory that pre-caps the signal so the warning never fires on per-request signals. - `createChildAbortController(parent)` — WeakRef-based parent→child propagation with `{once:true}` on the parent listener AND a reverse-cleanup listener on the child that detaches the parent listener when the child aborts. This is the key mechanism — short-lived children stop accumulating dead listeners on long-lived parents. - `combineAbortSignals(signals, {timeoutMs})` — N-way combiner that replaces the existing one-input `combinedAbortSignal.ts` (kept as a `@deprecated` shim so `httpHookRunner.ts` doesn't churn). Migrate every production `new AbortController()` in `packages/core/src` (24 sites) to the helper. Wrap `_runReasoningLoopInner` per-iteration body and `AgentHeadless.execute` in `try/finally` so the round controller is aborted (triggering reverse cleanup) even when the model stream or tool execution throws. Add `{once:true}` to the manual abort listeners in `hookRunner`, `functionHookRunner`, and `message-bus` that were missing it. Remove the `raiseAbortListenerCap` band-aid from `openaiContentGenerator/pipeline.ts` — no longer needed now that the per-round signal carries `maxListeners=50`. Add `cli/utils/warningHandler.ts` as a belt-and-suspenders: hides `MaxListenersExceededWarning.*AbortSignal` from end users in production (any shape Node ≥20 emits), keeps it visible under `DEBUG`/`QWEN_DEBUG`/ `NODE_ENV=development`. Uses `process.on('warning', ...)` without `removeAllListeners` so third-party warning subscribers stay intact. Direct reproducer in `docs/verification/abort-controller-refactor/` proves the old pattern accumulates 2000 listeners over 2000 rounds while the new pattern stays at 0. * fix(core): address PR #4366 review feedback Four issues from the Copilot review: 1. combineAbortSignals — add a per-iteration `aborted` check inside the for-loop so we short-circuit if an input signal flips aborted between the initial scan and listener registration. In single-threaded JS this can't actually interleave, but the defensive check makes correctness obvious and protects against signals whose `aborted` getter has side effects. New test exercises the path via a Proxy that flips after the initial scan. 2. warningHandler docstring — was stale: said "AbortSignal / EventTarget" while the regex was tightened to AbortSignal-only in the previous review. 3. README.md — replace personal absolute path with `$WT` placeholder so the verification recipe is shareable. 4. README.md — replace the markdown table with per-scenario headed sections. Prettier had interpreted an inline `ps -ef | grep sleep` pipe character as a column separator, breaking the table rendering on GitHub. Per-section format is also easier to scan and edit. * test(core): fix abortController race-defense test to actually hit the loop check The previous version set the Proxy's `aborted` to true before calling combineAbortSignals, so the initial `find` scan caught it and we took the fast path — not the per-iteration check the test was meant to validate. Switch to an access counter so `aborted` is false on the first read (during `find`) and true on subsequent reads (inside the loop). This forces the loop to enter, then catches the flip via the defensive per-iteration check before any listener is attached to the next input. Verified the test fails if the per-iteration check is removed. * fix(lint): include docs/**/*.mjs in the script ESLint block so the AbortController repro passes lint CI Lint flagged 11 no-undef errors in docs/verification/abort-controller-refactor/listener-accumulation-repro.mjs (AbortController, console, process) because the project's flat config only declared Node globals for ./scripts/**/*.mjs. The reviewer's suggestion (`/* eslint-env node */`) doesn't work under ESLint 9 flat config — env directives are deprecated there. The proper fix is to extend the existing script-globals block to also cover the verification repro script under docs/. * fix(core,cli): address PR #4366 critical review findings Two real bugs the reviewer caught and I confirmed locally: 1. warningHandler.ts didn't actually suppress anything. Adding a `process.on('warning')` listener does NOT prevent Node's default onWarning printer from writing to stderr — the default is just an ordinary listener registered in `lib/internal/process/warning.js`. My previous code therefore: - failed to suppress targeted AbortSignal warnings (they still hit stderr via the default printer) - produced a SECOND copy of every non-suppressed warning (default printer + my handler's own stderr.write) The unit tests missed it because they synthesised a fake warning and called `process.listeners('warning')` directly rather than going through `process.emitWarning`. Fix: snapshot the existing `'warning'` listeners (which include the default printer and any third-party telemetry hooks) BEFORE replacing them. Install ours as the sole listener. For non-suppressed warnings fan out to the captured set so the default printer + telemetry still fire; for suppressed warnings stop here. Tests now use `process.emit('warning', ...)` to drive the real listener chain, plus a spawned-child integration test that asserts the real stderr from `process.emitWarning` is empty for AbortSignal warnings and still contains DeprecationWarning text. 2. abortController.createChildAbortController kept a WeakRef to the child controller. A natural usage pattern — pass `child.signal` into an async API and drop the controller object — could let the controller be GC'd while the signal is still in use, after which `parent.abort()` would no longer propagate. Reproduced with `node --expose-gc`. Fix: hold the child strongly via the parent's listener closure. The reverse-cleanup listener still removes the closure when child aborts (closure releases child → GC-eligible), and the parent's `{once:true}` listener self-removes when parent fires (same effect). Net listener accounting on long-lived parents is unchanged; the only difference is the controller now stays alive long enough for propagation to reach downstream consumers that hold only the signal. Tests updated: drop the old `--expose-gc`-dependent assertion that abandoned children GC immediately (that was a property of the OLD contract); add a signal-only-retention test that verifies propagation under the new contract without needing GC at all. Verified: 32 helper/warning tests pass (incl. spawned-child stderr integration); 363 affected caller tests pass; typecheck + prettier + eslint clean for the touched files. * fix(core,cli): address PR #4366 review — fix combineAbortSignals orphan listeners + runtime DEBUG toggle Two real bugs the reviewer caught: 1. combineAbortSignals registered its cleanup listener on controller.signal AFTER the for-loop. Node does NOT fire 'abort' listeners added to an already-aborted signal, so when the per-iteration defensive check aborted the controller mid-loop, the cleanup never ran — orphaning every input-signal listener registered before the break, and leaving the (also-registered-after-the-break) setTimeout uncleared. Fix: skip timeout scheduling when controller.signal.aborted is already true post-loop, and when it's true call cleanup() synchronously instead of registering a doomed listener. Existing test for the mid-iteration path now also asserts that the pre-break input signal (a) has zero abort listeners — that's the assertion that catches the orphan bug. New test for the already-aborted-input + timeoutMs combination confirms the timer isn't scheduled (would otherwise overwrite the abort reason). 2. warningHandler captured isDebugMode() in a closure at init time, so toggling DEBUG / QWEN_DEBUG at runtime (e.g. via a /debug slash command) didn't update suppression behavior. Moved the check inside the handler — warnings are rare so the per-emit env-lookup cost is negligible. New test asserts a mid-stream DEBUG=1 flip starts forwarding suppressed warnings to the prior-listener chain. * test(core): strengthen the timeout-guard test in combineAbortSignals to actually exercise the new !aborted check Reviewer correctly pointed out that the previous version of this test took the pre-loop fast path (since `a.abort('pre')` ran before `combineAbortSignals`), so it never reached the in-loop guard at abortController.ts:138. Switched to the Proxy `aborted`-getter pattern from the sibling mid-iteration test (so the loop genuinely re-checks `aborted` and short-circuits inside the for-loop), and added a `setTimeout` spy that asserts the timer was never scheduled — this is the only observable difference from "scheduled then immediately cleared by synchronous cleanup()", which is what the timer-advance assertion alone couldn't distinguish. Verified by mutation testing: removing the guard makes the new test fail; restoring it makes it pass. Refs PR #4366. * test(core): cover timeout-triggered cleanup of input-signal listeners in combineAbortSignals Reviewer noted the timeout path only had an empty-input test, leaving the leak-sensitive case uncovered: when timeoutMs fires with a long-lived source signal in the input list, do the input-side listeners get released? They do (the timeout callback aborts the combined controller, which fires the auto-cleanup listener registered on its signal, which calls the per-input removeEventListener), but that path wasn't tested. Adds a test that snapshots the source listener count before, asserts it increased by 1 after combineAbortSignals returns, advances fake timers past timeoutMs, and asserts the count returns to baseline. Refs PR #4366. * fix(test): use pathToFileURL for the warning-handler e2e import on Windows CI failure on windows-latest: AssertionError: expected '\r\nnode:internal/modules/run_main:12…' to match /DeprecationWarning.*Plain deprecation/ Error [ERR_UNSUPPORTED_ESM_URL_SCHEME]: Only URLs with a scheme in: file, data, and node are supported by the default ESM loader. On Windows, absolute paths must be valid file:// URLs. Received protocol 'd:' The e2e test wrote a child script with an `import ""` where helperPath was a raw Windows absolute path (`D:\a\qwen-code\...`). Node's ESM loader parses that as a URL on Windows and rejects the `D:` "scheme". Converted the helper path to a `file://` URL via `pathToFileURL`. macOS test still passes; the Windows-specific schemes-must-be-URL behavior is now honored. Refs PR #4366. * fix(core,cli): address PR #4366 review batch — onAbort leak, migrate missed sites, tighten tests Adopted 6 of the 7 review threads (skipping the debug-logging suggestion). 1. processFunctionCalls onAbort leak (CRITICAL): the new `finally { roundAbortController.abort(); }` in _runReasoningLoopInner would fire the `onAbort` handler in `processFunctionCalls` if scheduler.schedule or batchDone threw (the explicit removeEventListener at the old happy-path exit would be skipped), emitting spurious "Tool call cancelled by user abort." TOOL_RESULT events for every un-emitted callId — corrupting the transcript and misleading the model on the next round. Fixed by wrapping schedule + batchDone in their own try/finally so removeEventListener always runs before the outer finally's abort. 2. Migrate 3 new-from-main `new AbortController()` sites that this PR's audit missed (they came in via the merge from main): - goals/goalHook.ts (2 sites: judgeController, fallback signal) — consistency - hooks/promptHookRunner.ts (1 site: internalAbortController) — real leak (manual addEventListener without {once:true} or cleanup, exactly the pattern this PR exists to fix). Switched to createChildAbortController + finally `internalAbortController.abort()` for reverse cleanup on the success path. 3. Repro script (`listener-accumulation-repro.mjs`): inlined helper diverged from production — used WeakRef on child, while production was changed to strong-ref earlier in this PR. Updated the inlined copy to match production exactly, with a comment noting the intentional WeakRef-on-parent-only pattern. 4. warningHandler.ts: documented the snapshot-and-replace trade-offs in the JSDoc (late-added listeners bypass our filter; late `removeListener` calls have no effect on our fan-out). Tried the re-snapshot-per-warning approach the reviewer suggested but it doesn't work — `removeAllListeners('warning')` permanently removes the snapshot from Node's tracking, so a `process.listeners('warning')` filter at fan-out time always returns empty for prior listeners. The current design is the right trade-off; documentation is the correct fix. 5. abortController.test.ts: added three coverage gaps the reviewer identified — - createChildAbortController forwards custom maxListeners - manual cleanup() before scheduled timeout fires cancels it - timeoutMs <= 0 is treated as "no timeout" 6. Migrated `httpHookRunner.ts:202` (the lone caller of the deprecated `createCombinedAbortSignal`) to `combineAbortSignals` directly, then deleted `combinedAbortSignal.ts` + its test. All semantics covered by `combineAbortSignals` tests in abortController.test.ts. Refreshed `migration-completeness.txt` (now empty — clean grep). Tests: 194 pass across abortController/warningHandler/agent-runtime/ followup/hooks/goal/promptHook suites. Typecheck + prettier clean. * docs(verification): commit the headless-scenario scripts referenced by the PR body The PR body's "End-to-end scenarios I drove locally" section points at docs/verification/abort-controller-refactor/scripts/02-lite.sh and 06-headless-sigint.sh. These are the actual reproducible commands behind the EXIT codes / warning counts reported there — checking them in so anyone can replay without copy-pasting from the PR description. Refs PR #4366. * docs(verification): sync automated-results with current state Two doc fixes the reviewer flagged: - migration-completeness.txt was a 0-byte file with a confusing cross-reference. Populated with the actual grep command + its "(no output)" result so the empty-output state is explicit. - automated-results.md still referenced combinedAbortSignal.test.ts (8 tests, @deprecated shim) — both files were deleted in 94e8c5812 when httpHookRunner.ts migrated to combineAbortSignals directly. Replaced the line with a reference to httpHookRunner.test.ts. Also updated the test counts to reflect current state (26 abortController, 13 warningHandler — both grew with the review cycle) and removed the stale combinedAbortSignal.ts entry from the prettier-check command. Refs PR #4366. * test(core): pin two abort-cascade behaviors PR #4366 introduced Adopting 2 of 3 new review threads (the third — automated-results.md drift — was already fixed in 5aa7110e4). 1. packages/core/src/agents/arena/ArenaManager.test.ts: pin the master→agent abort cascade introduced by switching per-agent controllers to `createChildAbortController(this.masterAbortController)`. New test spawns ≥2 agents, calls `manager.cancel()`, and asserts every `agentState.abortController.signal.aborted === true`. Existing cancel test only checked backend + status; if a future refactor re-introduced independent controllers, the cascade would silently regress. 2. packages/core/src/followup/speculation.test.ts: cover the `startSpeculation` abort wiring introduced when the manual addEventListener + .finally removeEventListener pattern got replaced by createChildAbortController + .finally abort(). Three tests: - parent abort propagates to state.abortController (lifetime contract) - parent-already-aborted fast path returns aborted state - parent-signal listener count returns to baseline after the fire-and- forget loop settles (reverse-cleanup proof) Mocked `runWithForkedChatModel` and `OverlayFs` so the background loop is a no-op — these tests only assert the synchronous wiring, not the loop's content. * fix(test): speculation.test.ts TS errors + sync verification doc counts Two real CI blockers in the just-added speculation tests (TS2554 and TS2339) plus stale doc counts the reviewer flagged. 1. saveCacheSafeParams takes 3 positional args (generationConfig, history, model), not a single object. Compile error on every platform. Fixed by switching to the correct shape; also moved getEventListeners to a static `import` at the top of the file (dynamic `await import('node:events')` exposes EventEmitter's static method via the namespace type rather than as a direct property, so destructuring fails type-check). 2. docs/verification/abort-controller-refactor/README.md still claimed "18 + 1 GC" tests for abortController and "9" for warningHandler; actual current counts are 26 and 13. Also dropped the stale combinedAbortSignal reference and added a note about the new ArenaManager cascade + startSpeculation wiring pin tests. Refreshed smoke-boot.log against current built bin (still 0.15.11, which is what package.json reports on this branch). Refs PR #4366. * refactor(core): narrow PR #4366 scope per yiliang's review — revert independent-controller migrations Adopting @yiliang114's review feedback (#4366 review comment, 2026-05-22): keep only the migrations that fix the real leak path (the agent-runtime parent→child chain that accumulates listeners on a long-lived parent signal in long sessions) and revert the consistency-only migrations on independent short-lived controllers. Issue #4423 confirms the user-visible bug is the nested-chain accumulation — the reverted sites do not contribute to that bug. Migrations KEPT: - agents/runtime/agent-interactive.ts (master + per-message round) - agents/runtime/agent-core.ts (per-iteration + wait + processFunctionCalls) - agents/runtime/agent-headless.ts (external → execution) - hooks/promptHookRunner.ts (real cleanup leak: addEventListener without {once:true}, never removed) - hooks/httpHookRunner.ts → combineAbortSignals direct (shim deleted) - hookRunner.ts / functionHookRunner.ts / message-bus.ts: {once:true} only - openaiContentGenerator/pipeline.ts band-aid removal (per-request signals are children of the per-round controller, which carries maxListeners=50) - warningHandler.ts belt-and-suspenders Migrations REVERTED (independent short-lived controllers; restored to `new AbortController()` + their original cleanup patterns): - agents/arena/ArenaManager.ts (master + per-agent) - agents/background-agent-resume.ts (3 sites) - core/client.ts (recall — restored manual addEventListener + finally removeEventListener pattern from main) - followup/speculation.ts (restored parentAbortHandler + finally removeEventListener) - goals/goalHook.ts (judgeController + fallback signal) - memory/manager.ts (dream controller) - services/chatCompressionService.ts (fallback signal) - services/chatRecordingService.ts (autoTitle controller) - tools/agent/agent.ts (fg + bg subagent controllers — restored manual onParentAbort + finally removeEventListener) - tools/monitor.ts (entryAc) - tools/shell.ts (promote + 3 entryAc) - utils/fetch.ts (fetchWithTimeout) Tests removed alongside the reverts: - ArenaManager.test.ts "cancels cascades..." — the cascade itself was an intentional behavioral improvement that's now reverted, so the pin-test belongs with it - speculation.test.ts "startSpeculation — abort-controller wiring" block (3 tests) — they tested helper-wired behavior we reverted Verification docs updated to reflect the narrower scope. Net change: 19 raw `new AbortController()` remain (intentional, per migration-completeness.txt rationale); previously was 0. Refs PR #4366, issue #4423. --- .../abort-controller-refactor/README.md | 120 +++++ .../automated-results.md | 139 +++++ .../listener-accumulation-repro.mjs | 107 ++++ .../migration-completeness.txt | 28 + .../scripts/02-lite.sh | 23 + .../scripts/06-headless-sigint.sh | 18 + .../abort-controller-refactor/smoke-boot.log | 2 + .../test-summary.txt | 10 + eslint.config.js | 9 +- packages/cli/src/gemini.tsx | 2 + packages/cli/src/utils/warningHandler.test.ts | 253 +++++++++ packages/cli/src/utils/warningHandler.ts | 107 ++++ .../core/src/agents/runtime/agent-core.ts | 483 +++++++++--------- .../core/src/agents/runtime/agent-headless.ts | 208 ++++---- .../src/agents/runtime/agent-interactive.ts | 25 +- .../core/src/confirmation-bus/message-bus.ts | 2 +- .../core/openaiContentGenerator/pipeline.ts | 20 - .../src/hooks/combinedAbortSignal.test.ts | 111 ---- .../core/src/hooks/combinedAbortSignal.ts | 57 --- packages/core/src/hooks/functionHookRunner.ts | 2 +- packages/core/src/hooks/hookRunner.ts | 2 +- packages/core/src/hooks/httpHookRunner.ts | 6 +- packages/core/src/hooks/promptHookRunner.ts | 23 +- .../core/src/utils/abortController.test.ts | 340 ++++++++++++ packages/core/src/utils/abortController.ts | 165 ++++++ 25 files changed, 1699 insertions(+), 563 deletions(-) create mode 100644 docs/verification/abort-controller-refactor/README.md create mode 100644 docs/verification/abort-controller-refactor/automated-results.md create mode 100644 docs/verification/abort-controller-refactor/listener-accumulation-repro.mjs create mode 100644 docs/verification/abort-controller-refactor/migration-completeness.txt create mode 100755 docs/verification/abort-controller-refactor/scripts/02-lite.sh create mode 100755 docs/verification/abort-controller-refactor/scripts/06-headless-sigint.sh create mode 100644 docs/verification/abort-controller-refactor/smoke-boot.log create mode 100644 docs/verification/abort-controller-refactor/test-summary.txt create mode 100644 packages/cli/src/utils/warningHandler.test.ts create mode 100644 packages/cli/src/utils/warningHandler.ts delete mode 100644 packages/core/src/hooks/combinedAbortSignal.test.ts delete mode 100644 packages/core/src/hooks/combinedAbortSignal.ts create mode 100644 packages/core/src/utils/abortController.test.ts create mode 100644 packages/core/src/utils/abortController.ts diff --git a/docs/verification/abort-controller-refactor/README.md b/docs/verification/abort-controller-refactor/README.md new file mode 100644 index 0000000000..6f0b3c3f0e --- /dev/null +++ b/docs/verification/abort-controller-refactor/README.md @@ -0,0 +1,120 @@ +# AbortController refactor — verification plan + +Scenarios used to validate the change manually before opening the PR. Each +scenario captures its tmux pane via `tmux pipe-pane -o 'cat >> '`. + +## Setup once + +```sh +# Point WT at your local checkout of the branch under review. +WT=/path/to/qwen-code/worktree +LOGDIR=$WT/docs/verification/abort-controller-refactor/logs +mkdir -p "$LOGDIR" + +# Build the CLI once (skip sandbox image, skip vscode). +( cd "$WT" && npm run build:packages ) +``` + +## Scenarios + +For each scenario: + +```sh +tmux new-session -d -s qwen-verify-XX +tmux pipe-pane -t qwen-verify-XX -o "cat >> $LOGDIR/XX-name.log" +tmux send-keys -t qwen-verify-XX "cd /path/to/your/test/workspace && exec node $WT/packages/cli/dist/index.js" C-m +tmux attach -t qwen-verify-XX +``` + +Then drive the session manually per the matrix below. Hit `C-b d` to detach +when done; `tmux kill-session -t qwen-verify-XX` to stop the pane. + +### 00 — Baseline (PRE-fix) + +- **Setup:** check out `main`, build, run with `NODE_OPTIONS=--trace-warnings`. +- **Input:** long 50-round mixed-tool session (shell + edit + grep + agent). +- **Expected:** after ~30–40 rounds, `MaxListenersExceededWarning: ... 1500+ abort listeners added to [AbortSignal]` printed to stderr. +- **Log:** `00-baseline-reproduction.log`. + +### 01 — Long-session, DEBUG mode (this branch) + +- **Setup:** `NODE_OPTIONS=--trace-warnings DEBUG=1 qwen`. +- **Input:** same 50-round script as #00. +- **Expected:** no `MaxListenersExceededWarning` printed; any other warnings still print. +- **Log:** `01-long-session-debug.log`. + +### 02 — Long-session, prod mode (this branch) + +- **Setup:** `qwen` (no debug env). +- **Input:** same 50-round script. +- **Expected:** clean output; a temporary `console.error` probe inside the handler (added then removed) confirms the filter fires. +- **Log:** `02-long-session-prod.log`. + +### 03 — Ctrl-C mid-stream abort + +- **Setup:** this branch, interactive. +- **Input:** ask for a long generation (>30s); press Ctrl-C mid-stream. +- **Expected:** stream stops within ~200ms, "Cancelled" banner shown, next prompt accepts input. `process._getActiveHandles()` count returns to baseline (use `:debug handles`). +- **Log:** `03-ctrlc-streaming.log`. + +### 04 — Cancel long-running shell + +- **Setup:** this branch. +- **Input:** run `sleep 60` via the shell tool; cancel mid-execution. +- **Expected:** child process killed (verify with `pgrep -f sleep` returning empty), tool result shows cancellation, agent accepts next prompt. +- **Log:** `04-shell-cancel.log`. + +### 05 — Subagent cancellation + +- **Setup:** this branch. +- **Input:** spawn a long agent task via the agent tool; cancel from parent. +- **Expected:** subagent's in-flight tool calls abort, subagent's model stream stops, parent receives cancellation event. +- **Log:** `05-subagent-cancel.log`. + +### 06 — Headless / non-interactive abort + +- **Setup:** `qwen --prompt "do a long task"`; send `SIGINT` from outside via `kill -INT `. +- **Expected:** clean shutdown, exit code 130, no warnings. +- **Log:** `06-headless-abort.log`. + +### 07 — Background agent flow + +- **Setup:** interactive. +- **Input:** spawn a background agent (`run_in_background: true`); let it complete; spawn a second one; cancel the second mid-flight. +- **Expected:** first agent completes normally; second aborts cleanly; no listener leak across the two. +- **Log:** `07-background-agent.log`. + +### 08 — Memory baseline + +- **Setup:** `qwen --inspect`, attach Chrome devtools. +- **Input:** 100-round session. +- **Expected:** heap snapshots at round 0/50/100. `AbortSignal` instance count and per-signal listener count stable (no monotonic growth). +- **Log:** `08-memory-snapshots/`. + +### 09 — Existing combinedAbortSignal consumer + +- **Setup:** trigger an HTTP hook with both an external signal and timeout. +- **Input:** (a) cancel external signal mid-hook; (b) let timeout fire in a separate run. +- **Expected:** hook aborts cleanly in both cases; deprecation shim path is exercised. +- **Log:** `09-http-hook-shim.log`. + +## Automated (non-interactive) verifications + +The automated checks below were run during development and recorded in +`automated-results.md`: + +- All abortController unit tests pass (`abortController.test.ts`, 26 tests; 1 GC test skipped under non-`--expose-gc`). +- All warningHandler tests pass (`warningHandler.test.ts`, 13 tests including a spawned-child stderr integration test). +- All `combineAbortSignals` consumer tests pass (`httpHookRunner.test.ts`); the deprecated `createCombinedAbortSignal` shim plus its own test file were removed once the lone caller migrated. +- All agent runtime / followup / openaiContentGenerator / hooks tests pass. +- Migration scope (intentional): only the agent-runtime parent→child chain (`agent-interactive.ts`, `agent-core.ts`, `agent-headless.ts`) plus `promptHookRunner.ts` (real cleanup leak) was switched to the helper. Independent short-lived controllers (per-shell-command, per-fetch, per-recall, etc.) stay on raw `new AbortController()` — they're GC'd quickly and don't accumulate listeners on a long-lived parent. See `migration-completeness.txt` for the captured grep + rationale. +- TypeScript strict-mode typecheck passes for both `packages/core` and `packages/cli`. +- Prettier check passes on all modified files. + +See `automated-results.md` for the actual command output. + +## How to capture the artifacts for the PR body + +After running each scenario, attach the transcript file (or relevant excerpt) +to the PR. For #08 (memory), export the heap snapshots and include the +listener-count delta between snapshots. diff --git a/docs/verification/abort-controller-refactor/automated-results.md b/docs/verification/abort-controller-refactor/automated-results.md new file mode 100644 index 0000000000..a53d350edb --- /dev/null +++ b/docs/verification/abort-controller-refactor/automated-results.md @@ -0,0 +1,139 @@ +# Automated verification results + +Captured 2026-05-20 during the AbortController refactor. + +## 1. Listener-accumulation reproducer + +Direct simulation of the listener-accumulation pattern observed in long +sessions (1500+ abort listeners on a single AbortSignal). The script lives +at `listener-accumulation-repro.mjs`. + +```text +$ node docs/verification/abort-controller-refactor/listener-accumulation-repro.mjs +Simulating 2000 rounds for each pattern. + +OLD pattern listener count on long-lived parent: 2000 +NEW pattern listener count on long-lived parent: 0 +PASS: OLD pattern accumulated >1500 listeners (reproduces the bug). +PASS: NEW pattern kept listener count at 0 — the helper prevents accumulation. +``` + +This is a self-contained proof: the OLD pattern (raw `addEventListener` +without `{once:true}` or reverse cleanup) accumulates 2000 listeners over +2000 rounds — well past the 1500 threshold the user observed. The NEW +pattern (`createChildAbortController` from `packages/core/src/utils/abortController.ts`) +keeps the parent listener count at 0 across 2000 rounds because each child's +reverse-cleanup listener removes the parent listener when the child aborts. + +## 2. Migration scope (intentional) + +Only the agent-runtime parent→child chain that actually accumulates listeners +on a long-lived parent signal is migrated to the helper: + +- `packages/core/src/agents/runtime/agent-interactive.ts` (master + per-message round) +- `packages/core/src/agents/runtime/agent-core.ts` (per-iteration round + waitForExternalInputs + processFunctionCalls try/finally) +- `packages/core/src/agents/runtime/agent-headless.ts` (external → execution) +- `packages/core/src/hooks/promptHookRunner.ts` (had a real cleanup leak: manual addEventListener without `{once:true}` and never removed) + +Plus three `{once:true}`-only fixes (no helper switch, just defensive +correctness): + +- `packages/core/src/hooks/hookRunner.ts` +- `packages/core/src/hooks/functionHookRunner.ts` +- `packages/core/src/confirmation-bus/message-bus.ts` + +Independent short-lived controllers (per-shell-command in `tools/shell.ts`, +per-monitor in `tools/monitor.ts`, per-arena-session in +`agents/arena/ArenaManager.ts`, per-recall in `core/client.ts`, +per-fetch in `utils/fetch.ts`, per-dream / per-title / per-judge / per-resume, +etc.) stay on raw `new AbortController()` — they're GC'd at end of use and +do not accumulate on a long-lived parent. + +See `migration-completeness.txt` for the actual grep + rationale. + +## 3. Affected test suites + +All 71 affected test files / 2085 tests pass (3 skipped — 1 is the GC test +that requires `--expose-gc`, 2 are pre-existing skips in the headless suite). + +```text + Test Files 71 passed (71) + Tests 2085 passed | 3 skipped (2088) + Duration 16.71s +``` + +Coverage: + +- `packages/core/src/utils/abortController.test.ts` — 26 tests: factory cap (default + custom), child propagation, reverse cleanup, fast path, undefined parent, custom-maxListeners passthrough, `combineAbortSignals` semantics (incl. cleanup-cancels-timeout, timeout-cleans-input-listeners, `timeoutMs <= 0` boundary, mid-iteration defensive check), GC safety (best-effort). +- `packages/cli/src/utils/warningHandler.test.ts` — 13 tests: idempotency, AbortSignal suppression (including `[AbortSignal{...}]` shape), generic EventTarget NOT suppressed, debug-mode passthrough, fan-out to prior listeners, spawned-child end-to-end stderr integration. +- `packages/core/src/hooks/httpHookRunner.test.ts` — covers the migrated `combineAbortSignals` consumer (the deprecated `createCombinedAbortSignal` shim plus its test file were removed once the lone caller migrated). +- `packages/core/src/agents/runtime/{agent-core,agent-interactive,agent-headless,agent-context,agent-statistics}.test.ts` — 102 tests covering the high-impact migrated files. +- `packages/core/src/core/openaiContentGenerator/**` — 280+ tests including the pipeline that lost the `raiseAbortListenerCap` band-aid. +- `packages/core/src/followup/**` — 100+ tests including the migrated speculation controller. +- `packages/core/src/tools/agent/**`, `packages/core/src/tools/shell.test.ts`, `packages/core/src/services/**`, `packages/core/src/hooks/**`, `packages/core/src/confirmation-bus/**` — all migrated tool/hook/service files. + +## 4. TypeScript strict-mode typecheck + +```sh +$ node_modules/.bin/tsc -p packages/core/tsconfig.json --noEmit +(no output, exit 0) + +$ node_modules/.bin/tsc -p packages/cli/tsconfig.json --noEmit +(no output, exit 0) +``` + +## 5. Prettier formatting + +```sh +$ node_modules/.bin/prettier --check packages/core/src/agents/runtime/agent-core.ts \ + packages/core/src/agents/runtime/agent-headless.ts \ + packages/cli/src/utils/warningHandler.ts \ + packages/cli/src/utils/warningHandler.test.ts \ + packages/core/src/utils/abortController.ts \ + packages/core/src/utils/abortController.test.ts +Checking formatting... +All matched files use Prettier code style! +``` + +## 6. Build + binary smoke test + +```sh +$ npm run build:packages +(succeeds for all 5 workspace packages) + +$ NODE_OPTIONS=--trace-warnings node packages/cli/dist/index.js --version +0.15.11 +EXIT=0 + +$ node packages/cli/dist/index.js --help +Usage: qwen [options] [command] +... +``` + +No warnings emitted during boot with `--trace-warnings`. + +## 7. Codex independent review + +Two full passes via the `codex:codex-rescue` agent (independent context each +time). First pass surfaced 3 issues — all addressed in subsequent commits: + +1. **Throw between controller creation and explicit abort leaks listener** in + `agent-core.ts`'s per-iteration body and `agent-headless.ts`'s + pre-try-block setup. Fixed by wrapping each in `try { ... } finally { +abortController.abort(); }`. +2. **Warning suppressor regex `EventTarget` too broad**. Tightened to match + only `AbortSignal` (any shape Node ≥20 produces). +3. **`process.removeAllListeners('warning')` strips third-party listeners**. + Removed — rely on Node's "no listeners → default printer fires" semantics + so adding our handler implicitly disables the default print path while + keeping third-party telemetry listeners intact. + +Second pass confirmed all fixes correct, no further blockers. + +## What remains for interactive verification + +The scenarios in `README.md` numbered 00–09 require a real interactive +session against the model API (long mixed-tool conversations, Ctrl-C +mid-stream, subagent cancellation, heap snapshots). Those are documented +for human execution and the transcripts should be attached to the PR body +when run. diff --git a/docs/verification/abort-controller-refactor/listener-accumulation-repro.mjs b/docs/verification/abort-controller-refactor/listener-accumulation-repro.mjs new file mode 100644 index 0000000000..ea73016178 --- /dev/null +++ b/docs/verification/abort-controller-refactor/listener-accumulation-repro.mjs @@ -0,0 +1,107 @@ +#!/usr/bin/env node +/** + * Direct simulation of the listener-accumulation pattern the agent runtime + * exhibits in long sessions. Builds a deep parent → child chain to a depth + * the user observed (>1500 listeners) and asserts: + * + * 1. The OLD pattern (plain new AbortController + manual addEventListener + * without {once:true} or reverse cleanup) accumulates listeners on the + * long-lived parent — reproducing the warning. + * + * 2. The NEW pattern (createChildAbortController from the helper) keeps the + * parent listener count bounded by 1, regardless of how many short-lived + * children come and go. + * + * Run: + * node docs/verification/abort-controller-refactor/listener-accumulation-repro.mjs + */ + +import { getEventListeners, setMaxListeners } from 'node:events'; + +// Inline copy of the production helper (packages/core/src/utils/abortController.ts) +// so this script has no build-step dependency on @qwen-code/qwen-code-core. +// Kept in sync — the child is held STRONGLY by the parent's listener closure +// (no WeakRef on child) so propagation works even when a caller drops the +// controller and keeps only the signal. WeakRef is used only on the PARENT, +// to keep child cleanup from pinning a long-lived parent in memory. +function createAbortController(maxListeners = 50) { + const c = new AbortController(); + setMaxListeners(maxListeners, c.signal); + return c; +} +function createChildAbortController(parent) { + const child = createAbortController(); + if (!parent) return child; + const parentSignal = parent.signal ?? parent; + if (parentSignal.aborted) { + child.abort(parentSignal.reason); + return child; + } + const weakParent = new WeakRef(parentSignal); + const handler = () => { + child.abort(weakParent.deref()?.reason); + }; + parentSignal.addEventListener('abort', handler, { once: true }); + child.signal.addEventListener( + 'abort', + () => { + weakParent.deref()?.removeEventListener('abort', handler); + }, + { once: true }, + ); + return child; +} + +const ROUNDS = 2000; + +console.log(`Simulating ${ROUNDS} rounds for each pattern.\n`); + +// ─── OLD pattern: plain new AbortController + manual addEventListener ─── +const oldParent = new AbortController(); +setMaxListeners(0, oldParent.signal); // disable warning so we can measure cleanly +for (let i = 0; i < ROUNDS; i++) { + const child = new AbortController(); + // No {once:true}, no reverse cleanup — accumulates on oldParent. + oldParent.signal.addEventListener('abort', () => child.abort()); +} +const oldCount = getEventListeners(oldParent.signal, 'abort').length; + +// ─── NEW pattern: createChildAbortController ─── +const newParent = createAbortController(); +for (let i = 0; i < ROUNDS; i++) { + const child = createChildAbortController(newParent); + child.abort(); // simulate end-of-round cleanup via try/finally +} +const newCount = getEventListeners(newParent.signal, 'abort').length; + +console.log(`OLD pattern listener count on long-lived parent: ${oldCount}`); +console.log(`NEW pattern listener count on long-lived parent: ${newCount}`); + +const expectations = { + oldShouldExceed: 1500, + newMustBe: 0, +}; + +let pass = true; +if (oldCount <= expectations.oldShouldExceed) { + console.error( + `FAIL: OLD pattern should accumulate >${expectations.oldShouldExceed} listeners; got ${oldCount}`, + ); + pass = false; +} else { + console.log( + `PASS: OLD pattern accumulated >${expectations.oldShouldExceed} listeners (reproduces the bug).`, + ); +} +if (newCount !== expectations.newMustBe) { + console.error( + `FAIL: NEW pattern must have exactly ${expectations.newMustBe} listeners; got ${newCount}`, + ); + pass = false; +} else { + console.log( + `PASS: NEW pattern kept listener count at ${expectations.newMustBe} — the helper prevents accumulation.`, + ); +} + +process.exit(pass ? 0 : 1); diff --git a/docs/verification/abort-controller-refactor/migration-completeness.txt b/docs/verification/abort-controller-refactor/migration-completeness.txt new file mode 100644 index 0000000000..3e5344cea2 --- /dev/null +++ b/docs/verification/abort-controller-refactor/migration-completeness.txt @@ -0,0 +1,28 @@ +$ grep -rn "new AbortController" packages/core/src --include="*.ts" \ + | grep -v test | grep -v abortController.ts + +# Scoped to the nested parent→child chain that actually accumulates listeners +# (the agent-runtime loop in long sessions, plus promptHookRunner which had a +# real cleanup leak). Independent short-lived controllers (per-shell-command, +# per-fetch, per-recall, per-arena-session etc.) intentionally stay on raw +# `new AbortController()` — they're GC'd at the end of their use and do not +# accumulate listeners on a long-lived parent signal. +packages/core/src/followup/speculation.ts:100: const abortController = new AbortController(); +packages/core/src/tools/agent/agent.ts:1722: const bgAbortController = new AbortController(); +packages/core/src/tools/agent/agent.ts:2116: const fgAbortController = new AbortController(); +packages/core/src/tools/shell.ts:1514: const promoteAbortController = new AbortController(); +packages/core/src/tools/shell.ts:2364: const entryAc = new AbortController(); +packages/core/src/tools/shell.ts:2772: const entryAc = new AbortController(); +packages/core/src/tools/monitor.ts:306: const entryAc = new AbortController(); +packages/core/src/core/client.ts:1199: const controller = new AbortController(); +packages/core/src/memory/manager.ts:936: const abortController = new AbortController(); +packages/core/src/goals/goalHook.ts:70: const judgeController = new AbortController(); +packages/core/src/goals/goalHook.ts:169: const signal = context?.signal ?? new AbortController().signal; +packages/core/src/agents/arena/ArenaManager.ts:305: this.masterAbortController = new AbortController(); +packages/core/src/agents/arena/ArenaManager.ts:817: abortController: new AbortController(), +packages/core/src/agents/background-agent-resume.ts:421: abortController: new AbortController(), +packages/core/src/agents/background-agent-resume.ts:493: const bgAbortController = new AbortController(); +packages/core/src/agents/background-agent-resume.ts:922: abortController: new AbortController(), +packages/core/src/utils/fetch.ts:64: const controller = new AbortController(); +packages/core/src/services/chatRecordingService.ts:963: const controller = new AbortController(); +packages/core/src/services/chatCompressionService.ts:387: abortSignal: signal ?? new AbortController().signal, diff --git a/docs/verification/abort-controller-refactor/scripts/02-lite.sh b/docs/verification/abort-controller-refactor/scripts/02-lite.sh new file mode 100755 index 0000000000..d60992cbf5 --- /dev/null +++ b/docs/verification/abort-controller-refactor/scripts/02-lite.sh @@ -0,0 +1,23 @@ +#!/usr/bin/env bash +# Scenario 02-lite — single real-Qwen prompt under --trace-warnings. +# Demonstrates the steady-state path emits no MaxListenersExceededWarning. +set -uo pipefail +WT="${WT:-$(git rev-parse --show-toplevel)}" +LOG="$WT/docs/verification/abort-controller-refactor/logs/02-lite-short-prompt.log" +mkdir -p "$(dirname "$LOG")" + +NODE_OPTIONS=--trace-warnings node "$WT/packages/cli/dist/index.js" \ + --prompt "Reply with exactly 'OK' and nothing else." > "$LOG" 2>&1 & +PID=$! +for i in $(seq 1 90); do + if ! kill -0 $PID 2>/dev/null; then break; fi + sleep 1 +done +if kill -0 $PID 2>/dev/null; then kill -9 $PID; echo "TIMEOUT"; exit 1; fi +wait $PID 2>/dev/null +EC=$? + +echo "EXIT=$EC" +echo "MaxListenersExceededWarning count: $(grep -c MaxListenersExceededWarning "$LOG")" +echo "--- log ---" +cat "$LOG" diff --git a/docs/verification/abort-controller-refactor/scripts/06-headless-sigint.sh b/docs/verification/abort-controller-refactor/scripts/06-headless-sigint.sh new file mode 100755 index 0000000000..1fc36bb4a2 --- /dev/null +++ b/docs/verification/abort-controller-refactor/scripts/06-headless-sigint.sh @@ -0,0 +1,18 @@ +#!/usr/bin/env bash +# Scenario 06 — headless --prompt + SIGINT. Verifies the agent shuts down +# cleanly when an external signal aborts the in-flight stream. +set -uo pipefail +WT="${WT:-$(git rev-parse --show-toplevel)}" +LOG="$WT/docs/verification/abort-controller-refactor/logs/06-headless-sigint.log" +mkdir -p "$(dirname "$LOG")" + +NODE_OPTIONS=--trace-warnings node "$WT/packages/cli/dist/index.js" \ + --prompt "Please write a detailed essay about the history of distributed systems, at least 500 words." > "$LOG" 2>&1 & +PID=$! +sleep 6 +kill -INT $PID +wait $PID 2>/dev/null +EC=$? + +echo "EXIT_CODE=$EC (expected 130)" +echo "MaxListenersExceededWarning count: $(grep -c MaxListenersExceededWarning "$LOG")" diff --git a/docs/verification/abort-controller-refactor/smoke-boot.log b/docs/verification/abort-controller-refactor/smoke-boot.log new file mode 100644 index 0000000000..988affd9f5 --- /dev/null +++ b/docs/verification/abort-controller-refactor/smoke-boot.log @@ -0,0 +1,2 @@ +0.15.11 +EXIT=0 diff --git a/docs/verification/abort-controller-refactor/test-summary.txt b/docs/verification/abort-controller-refactor/test-summary.txt new file mode 100644 index 0000000000..8d49c07c97 --- /dev/null +++ b/docs/verification/abort-controller-refactor/test-summary.txt @@ -0,0 +1,10 @@ + ✓ |@qwen-code/qwen-code-core| src/core/openaiContentGenerator/provider/minimax.test.ts (9 tests) 2ms + ✓ |@qwen-code/qwen-code-core| src/followup/suggestionGenerator.test.ts (16 tests) 2ms + ✓ |@qwen-code/qwen-code-core| src/followup/speculation.test.ts (7 tests) 2ms + ✓ |@qwen-code/qwen-code| src/utils/warningHandler.test.ts (9 tests) 5ms + + Test Files 71 passed (71) + Tests 2085 passed | 3 skipped (2088) + Start at 02:35:37 + Duration 16.71s (transform 1.77s, setup 92ms, collect 14.79s, tests 3.03s, environment 319ms, prepare 2.20s) + diff --git a/eslint.config.js b/eslint.config.js index ea31e0f1ec..4d6fcaef87 100644 --- a/eslint.config.js +++ b/eslint.config.js @@ -191,7 +191,14 @@ export default tseslint.config( }, // extra settings for scripts that we run directly with node { - files: ['./scripts/**/*.js', './scripts/**/*.mjs', 'esbuild.config.js', 'packages/*/scripts/**/*.js'], + files: [ + './scripts/**/*.js', + './scripts/**/*.mjs', + 'esbuild.config.js', + 'packages/*/scripts/**/*.js', + // Verification reproducer scripts under docs/ also run with `node`. + 'docs/**/*.mjs', + ], languageOptions: { globals: { ...globals.node, diff --git a/packages/cli/src/gemini.tsx b/packages/cli/src/gemini.tsx index 7b6df42b51..5af74c8ac2 100644 --- a/packages/cli/src/gemini.tsx +++ b/packages/cli/src/gemini.tsx @@ -78,6 +78,7 @@ import { start_sandbox } from './utils/sandbox.js'; import { getStartupWarnings } from './utils/startupWarnings.js'; import { getUserStartupWarnings } from './utils/userStartupWarnings.js'; import { getCliVersion } from './utils/version.js'; +import { initializeWarningHandler } from './utils/warningHandler.js'; import { writeStderrLine } from './utils/stdioHelpers.js'; import { getHeadlessYoloSafetyWarning } from './utils/headlessSafetyWarnings.js'; import { computeWindowTitle } from './utils/windowTitle.js'; @@ -403,6 +404,7 @@ export async function main() { setStartupEventSink((name, attrs) => recordStartupEvent(name, attrs)); } setupUnhandledRejectionHandler(); + initializeWarningHandler(); if (process.argv.includes('--bare')) { process.env[QWEN_CODE_SIMPLE_ENV_VAR] = '1'; diff --git a/packages/cli/src/utils/warningHandler.test.ts b/packages/cli/src/utils/warningHandler.test.ts new file mode 100644 index 0000000000..78b957ccd6 --- /dev/null +++ b/packages/cli/src/utils/warningHandler.test.ts @@ -0,0 +1,253 @@ +/** + * @license + * Copyright 2025 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { + initializeWarningHandler, + resetWarningHandlerForTests, +} from './warningHandler.js'; + +const ENV_KEYS = ['NODE_ENV', 'DEBUG', 'QWEN_DEBUG'] as const; + +describe('initializeWarningHandler', () => { + const originalEnv: Partial> = {}; + let originalListeners: NodeJS.WarningListener[] = []; + // Mock prior listener — installed before initializeWarningHandler so it + // becomes one of the captured "priorListeners" the handler fans out to. + // This is the channel the real Node default printer travels on, so + // asserting fan-out here is equivalent to asserting "the default printer + // would have fired" without coupling tests to internal Node behavior. + let priorListener: ReturnType; + + beforeEach(() => { + for (const k of ENV_KEYS) originalEnv[k] = process.env[k]; + for (const k of ENV_KEYS) delete process.env[k]; + originalListeners = [...process.listeners('warning')]; + process.removeAllListeners('warning'); + resetWarningHandlerForTests(); + priorListener = vi.fn(); + process.on('warning', priorListener); + }); + + afterEach(() => { + resetWarningHandlerForTests(); + process.removeAllListeners('warning'); + for (const l of originalListeners) process.on('warning', l); + for (const k of ENV_KEYS) { + if (originalEnv[k] === undefined) delete process.env[k]; + else process.env[k] = originalEnv[k]; + } + }); + + function makeWarning(name: string, message: string): Error { + const err = new Error(message); + err.name = name; + return err; + } + + function emit(warning: Error): void { + // Drive the real listener chain (process.emit dispatches synchronously + // to every registered 'warning' listener). After initializeWarningHandler, + // the only listener is ours; it decides whether to fan out to the + // captured priorListener mock. + process.emit('warning', warning); + } + + it('suppresses MaxListenersExceededWarning for AbortSignal in production', () => { + initializeWarningHandler(); + emit( + makeWarning( + 'MaxListenersExceededWarning', + 'Possible EventTarget memory leak detected. 1509 abort listeners added to [AbortSignal].', + ), + ); + expect(priorListener).not.toHaveBeenCalled(); + }); + + it('does NOT suppress generic [EventTarget] warnings — only AbortSignal', () => { + initializeWarningHandler(); + emit( + makeWarning( + 'MaxListenersExceededWarning', + 'Possible EventTarget memory leak detected. 11 listeners added to [EventTarget].', + ), + ); + expect(priorListener).toHaveBeenCalledTimes(1); + }); + + it('suppresses AbortSignal warnings with class metadata, e.g. [AbortSignal{aborted: false}]', () => { + initializeWarningHandler(); + emit( + makeWarning( + 'MaxListenersExceededWarning', + 'Possible EventTarget memory leak detected. 11 abort listeners added to [AbortSignal{aborted: false}].', + ), + ); + expect(priorListener).not.toHaveBeenCalled(); + }); + + it('fans out unrelated warnings to captured prior listeners (e.g. Node default printer)', () => { + initializeWarningHandler(); + const warning = makeWarning('DeprecationWarning', 'Some legacy thing'); + emit(warning); + expect(priorListener).toHaveBeenCalledTimes(1); + expect(priorListener).toHaveBeenCalledWith(warning); + }); + + it('preserves third-party warning listeners — they still fire for non-suppressed warnings', () => { + const telemetryHook = vi.fn(); + process.on('warning', telemetryHook); + initializeWarningHandler(); + emit(makeWarning('DeprecationWarning', 'X')); + expect(priorListener).toHaveBeenCalledTimes(1); + expect(telemetryHook).toHaveBeenCalledTimes(1); + }); + + it('a buggy prior listener does not break the chain for the rest', () => { + const buggy = vi.fn(() => { + throw new Error('boom'); + }); + const downstream = vi.fn(); + process.on('warning', buggy); + process.on('warning', downstream); + initializeWarningHandler(); + emit(makeWarning('DeprecationWarning', 'X')); + expect(buggy).toHaveBeenCalledTimes(1); + expect(downstream).toHaveBeenCalledTimes(1); + }); + + it('keeps suppressed warnings visible when DEBUG is set', () => { + process.env['DEBUG'] = '1'; + initializeWarningHandler(); + emit( + makeWarning( + 'MaxListenersExceededWarning', + 'Possible EventTarget memory leak detected. 1500 abort listeners added to [AbortSignal].', + ), + ); + expect(priorListener).toHaveBeenCalledTimes(1); + }); + + it('treats DEBUG=0 and DEBUG=false as not set', () => { + process.env['DEBUG'] = '0'; + initializeWarningHandler(); + emit( + makeWarning( + 'MaxListenersExceededWarning', + 'Possible EventTarget memory leak detected. 1500 abort listeners added to [AbortSignal].', + ), + ); + expect(priorListener).not.toHaveBeenCalled(); + }); + + it('keeps warnings visible when QWEN_DEBUG is set', () => { + process.env['QWEN_DEBUG'] = '1'; + initializeWarningHandler(); + emit( + makeWarning( + 'MaxListenersExceededWarning', + 'Possible EventTarget memory leak detected. 1500 abort listeners added to [AbortSignal].', + ), + ); + expect(priorListener).toHaveBeenCalledTimes(1); + }); + + it('keeps warnings visible when NODE_ENV=development', () => { + process.env['NODE_ENV'] = 'development'; + initializeWarningHandler(); + emit( + makeWarning( + 'MaxListenersExceededWarning', + 'Possible EventTarget memory leak detected. 1500 abort listeners added to [AbortSignal].', + ), + ); + expect(priorListener).toHaveBeenCalledTimes(1); + }); + + it('is idempotent — repeated calls install only one listener', () => { + initializeWarningHandler(); + initializeWarningHandler(); + initializeWarningHandler(); + expect(process.listeners('warning').length).toBe(1); + }); + + it('honors runtime DEBUG toggles — debug check is evaluated per warning', () => { + initializeWarningHandler(); + // Initially DEBUG unset → suppression active. + emit( + makeWarning( + 'MaxListenersExceededWarning', + 'Possible EventTarget memory leak detected. 1500 abort listeners added to [AbortSignal].', + ), + ); + expect(priorListener).not.toHaveBeenCalled(); + + // Flip DEBUG at runtime → next suppressed-pattern warning passes through. + process.env['DEBUG'] = '1'; + emit( + makeWarning( + 'MaxListenersExceededWarning', + 'Possible EventTarget memory leak detected. 1500 abort listeners added to [AbortSignal].', + ), + ); + expect(priorListener).toHaveBeenCalledTimes(1); + }); +}); + +describe('initializeWarningHandler — end-to-end stderr behavior', () => { + // Integration test: spawn a child Node process to verify the real + // emitWarning → default printer path is actually suppressed. The unit + // tests above can't catch this because the default printer lives inside + // Node and writes to stderr via internal mechanisms, not via the same + // process.stderr.write spy. + it('a child process with the handler installed does not print suppressed AbortSignal warnings to stderr', async () => { + const { spawn } = await import('node:child_process'); + const { fileURLToPath, pathToFileURL } = await import('node:url'); + const { dirname, join } = await import('node:path'); + const { writeFile, mkdtemp, rm } = await import('node:fs/promises'); + const { tmpdir } = await import('node:os'); + + const here = dirname(fileURLToPath(import.meta.url)); + // Convert the absolute path to a `file://` URL — on Windows, Node's ESM + // loader rejects raw absolute paths (it treats `D:` as a URL scheme), + // so import specifiers MUST be file URLs. + const helperImportSpecifier = pathToFileURL( + join(here, 'warningHandler.ts'), + ).href; + + const dir = await mkdtemp(join(tmpdir(), 'warning-handler-e2e-')); + try { + const script = ` + import { initializeWarningHandler } from ${JSON.stringify(helperImportSpecifier)}; + delete process.env.DEBUG; delete process.env.QWEN_DEBUG; + process.env.NODE_ENV = 'production'; + initializeWarningHandler(); + process.emitWarning( + 'Possible EventTarget memory leak detected. 1509 abort listeners added to [AbortSignal].', + 'MaxListenersExceededWarning' + ); + process.emitWarning('Plain deprecation', 'DeprecationWarning'); + `; + const scriptPath = join(dir, 'run.mjs'); + await writeFile(scriptPath, script); + + const child = spawn(process.execPath, ['--import', 'tsx', scriptPath], { + stdio: ['ignore', 'pipe', 'pipe'], + }); + let stderr = ''; + child.stderr.on('data', (b) => { + stderr += b.toString(); + }); + await new Promise((resolve) => child.on('exit', () => resolve())); + + expect(stderr).not.toMatch(/abort listeners added to \[AbortSignal\]/); + // Deprecation should still print via the fanned-out default printer. + expect(stderr).toMatch(/DeprecationWarning.*Plain deprecation/); + } finally { + await rm(dir, { recursive: true, force: true }); + } + }, 15_000); +}); diff --git a/packages/cli/src/utils/warningHandler.ts b/packages/cli/src/utils/warningHandler.ts new file mode 100644 index 0000000000..8108712360 --- /dev/null +++ b/packages/cli/src/utils/warningHandler.ts @@ -0,0 +1,107 @@ +/** + * @license + * Copyright 2025 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * Warnings we know about and want to keep out of the user-facing terminal. + * Listener accumulation on long-lived AbortSignals during multi-round agent + * sessions is structural, not a real memory leak — the listeners are removed + * (via {once:true} + reverse-cleanup in utils/abortController.ts) but a few + * extreme cases (e.g. OpenAI retry storms layered with multiple wrappers) can + * still graze the per-signal cap. Match any MaxListenersExceededWarning that + * mentions AbortSignal so we cover every shape Node ≥20 emits — `[AbortSignal]`, + * `[AbortSignal{...}]`, `[AbortSignal { ... }]`. We deliberately don't match + * the generic `EventTarget` token so unrelated EventTarget leaks stay visible. + */ +const SUPPRESSED_WARNINGS: RegExp[] = [ + /MaxListenersExceededWarning.*AbortSignal/, +]; + +function isSuppressed(warning: Error): boolean { + const text = `${warning.name}: ${warning.message}`; + return SUPPRESSED_WARNINGS.some((re) => re.test(text)); +} + +function isDebugMode(): boolean { + if (process.env['NODE_ENV'] === 'development') return true; + const truthy = (v: string | undefined) => + !!v && v !== '0' && v.toLowerCase() !== 'false'; + return truthy(process.env['DEBUG']) || truthy(process.env['QWEN_DEBUG']); +} + +let installedHandler: ((warning: Error) => void) | null = null; + +/** + * For tests only — uninstall the handler and reset internal state. + */ +export function resetWarningHandlerForTests(): void { + if (installedHandler) { + process.removeListener('warning', installedHandler); + installedHandler = null; + } +} + +/** + * Install a process-level `warning` handler that swallows the well-known + * `MaxListenersExceededWarning` for AbortSignal while letting every other + * warning through — including generic EventTarget leak warnings, which we + * leave visible because they likely indicate a real leak elsewhere. In + * debug mode (NODE_ENV=development, or DEBUG / QWEN_DEBUG set), all + * warnings are forwarded so developers can still see them. + * + * Implementation note: simply adding a `warning` listener does NOT prevent + * Node's default printer from writing to stderr — the default handler is + * registered as an ordinary listener (`lib/internal/process/warning.js`). + * To actually suppress targeted warnings, we capture the existing listeners + * (which include the default printer and any third-party telemetry hooks), + * remove them, then install ours as the sole listener. Non-suppressed + * warnings get fanned out to the captured listeners so the default printer + * still fires for them; suppressed warnings stop here. + * + * Idempotent — repeated calls are a no-op. + */ +export function initializeWarningHandler(): void { + if (installedHandler) return; + + // Snapshot everything currently listening on 'warning' (Node's default + // onWarning printer + any third-party telemetry subscribers). We will fan + // out non-suppressed warnings back to them. + // + // Trade-offs to be aware of (documented for future readers): + // - Listeners ADDED via `process.on('warning', ...)` after this init are + // independent of our snapshot. They receive `process.emit('warning')` + // directly and bypass the suppression filter. Node's default printer + // is in our snapshot (not added later), so stderr stays clean; late + // telemetry will see the full warning stream including AbortSignal + // leaks. This is intentional — telemetry should see what's happening. + // - Listeners REMOVED via `process.removeListener('warning', fn)` after + // this init have no effect: we hold our own strong reference in the + // snapshot. Re-snapshotting per warning doesn't fix this because the + // listeners are already removed from Node's list (we called + // `process.removeAllListeners` to disable Node's default printing of + // suppressed warnings). Callers who need conditional fan-out should + // install BEFORE initializeWarningHandler. + const priorListeners = [...process.listeners('warning')] as Array< + (warning: Error) => void + >; + + installedHandler = (warning: Error) => { + // Evaluate isDebugMode() per warning so DEBUG / QWEN_DEBUG can be + // toggled at runtime (e.g. via a `/debug` slash command) without + // re-running initializeWarningHandler. + if (!isDebugMode() && isSuppressed(warning)) return; + for (const fn of priorListeners) { + try { + fn(warning); + } catch { + // Don't let a misbehaving prior listener (e.g. a buggy telemetry + // hook) take down warning delivery for the rest of the chain. + } + } + }; + + process.removeAllListeners('warning'); + process.on('warning', installedHandler); +} diff --git a/packages/core/src/agents/runtime/agent-core.ts b/packages/core/src/agents/runtime/agent-core.ts index cbcef7e9c1..be36a5c38e 100644 --- a/packages/core/src/agents/runtime/agent-core.ts +++ b/packages/core/src/agents/runtime/agent-core.ts @@ -17,6 +17,7 @@ */ import { randomUUID } from 'node:crypto'; +import { createChildAbortController } from '../../utils/abortController.js'; import { reportError } from '../../utils/errorReporting.js'; import { subagentNameContext } from '../../utils/subagentNameContext.js'; import type { Config } from '../../config/config.js'; @@ -591,252 +592,253 @@ export class AgentCore { break; } - // Create a new AbortController per round to avoid listener accumulation - // in the model SDK. The parent abortController propagates abort to it. - const roundAbortController = new AbortController(); - const onParentAbort = () => roundAbortController.abort(); - abortController.signal.addEventListener('abort', onParentAbort); - if (abortController.signal.aborted) { - roundAbortController.abort(); - } + // Per-round child controller so model-SDK retry layers don't accumulate + // listeners on the long-lived parent. createChildAbortController handles + // parent propagation; the try/finally below guarantees reverse-cleanup + // fires for every exit (success, break, return, throw). + const roundAbortController = createChildAbortController(abortController); - const promptId = `${this.runtimeContext.getSessionId()}#${this.subagentId}#${turnCounter++}`; + try { + const promptId = `${this.runtimeContext.getSessionId()}#${this.subagentId}#${turnCounter++}`; - const messageParams = { - message: currentMessages[0]?.parts || [], - config: { - abortSignal: roundAbortController.signal, - tools: [{ functionDeclarations: toolsList }], - }, - }; + const messageParams = { + message: currentMessages[0]?.parts || [], + config: { + abortSignal: roundAbortController.signal, + tools: [{ functionDeclarations: toolsList }], + }, + }; - const roundStreamStart = Date.now(); - const responseStream = await chat.sendMessageStream( - this.modelConfig.model || - this.runtimeContext.getModel() || - DEFAULT_QWEN_MODEL, - messageParams, - promptId, - ); - this.eventEmitter?.emit(AgentEventType.ROUND_START, { - subagentId: this.subagentId, - round: turnCounter, - promptId, - timestamp: Date.now(), - } as AgentRoundEvent); - - const functionCalls: FunctionCall[] = []; - let roundText = ''; - let roundThoughtText = ''; - let lastUsage: GenerateContentResponseUsageMetadata | undefined = - undefined; - let currentResponseId: string | undefined = undefined; - let wasOutputTruncated = false; - - for await (const streamEvent of responseStream) { - if (roundAbortController.signal.aborted) { - abortController.signal.removeEventListener('abort', onParentAbort); - return { - text: finalText, - terminateMode: AgentTerminateMode.CANCELLED, - turnsUsed: turnCounter, - }; - } - - // Handle retry events — reset all per-attempt state so a successful - // retry does not inherit stale data (e.g. wasOutputTruncated) from a - // previous attempt that may have hit MAX_TOKENS. - if (streamEvent.type === 'retry') { - functionCalls.length = 0; - roundText = ''; - roundThoughtText = ''; - lastUsage = undefined; - currentResponseId = undefined; - wasOutputTruncated = false; - continue; - } - - // GeminiChat already mutated its own history; surface to the debug - // log so subagent compactions show up alongside the main session's. - if (streamEvent.type === 'compressed') { - this.runtimeContext - .getDebugLogger() - .debug( - `[AGENT-COMPACT] subagent=${this.subagentId} round=${turnCounter} ` + - `tokens ${streamEvent.info.originalTokenCount} -> ${streamEvent.info.newTokenCount}`, - ); - continue; - } - - // Handle chunk events - if (streamEvent.type === 'chunk') { - const resp = streamEvent.value; - // Track the response ID for tool call correlation - if (resp.responseId) { - currentResponseId = resp.responseId; - } - if (resp.functionCalls) functionCalls.push(...resp.functionCalls); - if (resp.candidates?.[0]?.finishReason === FinishReason.MAX_TOKENS) { - wasOutputTruncated = true; - } - const content = resp.candidates?.[0]?.content; - const parts = content?.parts || []; - for (const p of parts) { - const txt = p.text; - const isThought = p.thought ?? false; - if (txt && isThought) roundThoughtText += txt; - if (txt && !isThought) roundText += txt; - if (txt) - this.eventEmitter?.emit(AgentEventType.STREAM_TEXT, { - subagentId: this.subagentId, - round: turnCounter, - text: txt, - thought: isThought, - timestamp: Date.now(), - }); - } - if (resp.usageMetadata) lastUsage = resp.usageMetadata; - } - } - - if (roundText || roundThoughtText) { - this.eventEmitter?.emit(AgentEventType.ROUND_TEXT, { + const roundStreamStart = Date.now(); + const responseStream = await chat.sendMessageStream( + this.modelConfig.model || + this.runtimeContext.getModel() || + DEFAULT_QWEN_MODEL, + messageParams, + promptId, + ); + this.eventEmitter?.emit(AgentEventType.ROUND_START, { subagentId: this.subagentId, round: turnCounter, - text: roundText, - thoughtText: roundThoughtText, - timestamp: Date.now(), - } as AgentRoundTextEvent); - } - - this.executionStats.rounds = turnCounter; - this.stats.setRounds(turnCounter); - - durationMin = (Date.now() - startTime) / (1000 * 60); - if (options?.maxTimeMinutes && durationMin >= options.maxTimeMinutes) { - abortController.signal.removeEventListener('abort', onParentAbort); - terminateMode = AgentTerminateMode.TIMEOUT; - break; - } - - // Update token usage if available - if (lastUsage) { - this.recordTokenUsage(lastUsage, turnCounter, roundStreamStart); - } - - if (functionCalls.length > 0) { - currentMessages = await this.processFunctionCalls( - functionCalls, - roundAbortController, promptId, - turnCounter, - toolsList, - currentResponseId, - wasOutputTruncated, - ); + timestamp: Date.now(), + } as AgentRoundEvent); - const externalInputs = this.drainExternalInputs(options); - if (externalInputs.length > 0) { - // Append to the tool-response user message so external input rides - // alongside the tool results the model is about to see. - // processFunctionCalls always returns exactly one user-role entry. - const last = currentMessages[currentMessages.length - 1]; - last.parts!.push(...this.externalInputsToParts(externalInputs, true)); - // Emit one event per injection so observers (e.g. the JSONL - // transcript writer) can persist each external message as a - // user-role record. The framing prefix is stripped — the prefix - // is a model-facing detail, not part of the original message. - this.emitExternalInputEvents(externalInputs); - } - } else { - const immediateExternalInputs = this.drainExternalInputs(options); - if (immediateExternalInputs.length > 0) { - currentMessages = this.externalInputsToContent( - immediateExternalInputs, - ); - this.emitExternalInputEvents(immediateExternalInputs); - } else if (options?.shouldWaitForExternalMessages?.()) { - this.eventEmitter?.emit(AgentEventType.ROUND_END, { - subagentId: this.subagentId, - round: turnCounter, - promptId, - timestamp: Date.now(), - } as AgentRoundEvent); - abortController.signal.removeEventListener('abort', onParentAbort); + const functionCalls: FunctionCall[] = []; + let roundText = ''; + let roundThoughtText = ''; + let lastUsage: GenerateContentResponseUsageMetadata | undefined = + undefined; + let currentResponseId: string | undefined = undefined; + let wasOutputTruncated = false; - const waitResult = await this.waitForExternalInputs( - options, - abortController, - startTime, - turnCounter, - ); - if (waitResult.terminateMode) { - finalText = roundText.trim(); - terminateMode = waitResult.terminateMode; - break; + for await (const streamEvent of responseStream) { + if (roundAbortController.signal.aborted) { + return { + text: finalText, + terminateMode: AgentTerminateMode.CANCELLED, + turnsUsed: turnCounter, + }; } - if (waitResult.inputs.length > 0) { - currentMessages = this.externalInputsToContent(waitResult.inputs); - this.emitExternalInputEvents(waitResult.inputs); + + // Handle retry events — reset all per-attempt state so a successful + // retry does not inherit stale data (e.g. wasOutputTruncated) from a + // previous attempt that may have hit MAX_TOKENS. + if (streamEvent.type === 'retry') { + functionCalls.length = 0; + roundText = ''; + roundThoughtText = ''; + lastUsage = undefined; + currentResponseId = undefined; + wasOutputTruncated = false; continue; } - if (roundText && roundText.trim().length > 0) { - finalText = roundText.trim(); - break; + // GeminiChat already mutated its own history; surface to the debug + // log so subagent compactions show up alongside the main session's. + if (streamEvent.type === 'compressed') { + this.runtimeContext + .getDebugLogger() + .debug( + `[AGENT-COMPACT] subagent=${this.subagentId} round=${turnCounter} ` + + `tokens ${streamEvent.info.originalTokenCount} -> ${streamEvent.info.newTokenCount}`, + ); + continue; + } + + // Handle chunk events + if (streamEvent.type === 'chunk') { + const resp = streamEvent.value; + // Track the response ID for tool call correlation + if (resp.responseId) { + currentResponseId = resp.responseId; + } + if (resp.functionCalls) functionCalls.push(...resp.functionCalls); + if ( + resp.candidates?.[0]?.finishReason === FinishReason.MAX_TOKENS + ) { + wasOutputTruncated = true; + } + const content = resp.candidates?.[0]?.content; + const parts = content?.parts || []; + for (const p of parts) { + const txt = p.text; + const isThought = p.thought ?? false; + if (txt && isThought) roundThoughtText += txt; + if (txt && !isThought) roundText += txt; + if (txt) + this.eventEmitter?.emit(AgentEventType.STREAM_TEXT, { + subagentId: this.subagentId, + round: turnCounter, + text: txt, + thought: isThought, + timestamp: Date.now(), + }); + } + if (resp.usageMetadata) lastUsage = resp.usageMetadata; + } + } + + if (roundText || roundThoughtText) { + this.eventEmitter?.emit(AgentEventType.ROUND_TEXT, { + subagentId: this.subagentId, + round: turnCounter, + text: roundText, + thoughtText: roundThoughtText, + timestamp: Date.now(), + } as AgentRoundTextEvent); + } + + this.executionStats.rounds = turnCounter; + this.stats.setRounds(turnCounter); + + durationMin = (Date.now() - startTime) / (1000 * 60); + if (options?.maxTimeMinutes && durationMin >= options.maxTimeMinutes) { + terminateMode = AgentTerminateMode.TIMEOUT; + break; + } + + // Update token usage if available + if (lastUsage) { + this.recordTokenUsage(lastUsage, turnCounter, roundStreamStart); + } + + if (functionCalls.length > 0) { + currentMessages = await this.processFunctionCalls( + functionCalls, + roundAbortController, + promptId, + turnCounter, + toolsList, + currentResponseId, + wasOutputTruncated, + ); + + const externalInputs = this.drainExternalInputs(options); + if (externalInputs.length > 0) { + // Append to the tool-response user message so external input rides + // alongside the tool results the model is about to see. + // processFunctionCalls always returns exactly one user-role entry. + const last = currentMessages[currentMessages.length - 1]; + last.parts!.push( + ...this.externalInputsToParts(externalInputs, true), + ); + // Emit one event per injection so observers (e.g. the JSONL + // transcript writer) can persist each external message as a + // user-role record. The framing prefix is stripped — the prefix + // is a model-facing detail, not part of the original message. + this.emitExternalInputEvents(externalInputs); } - currentMessages = [ - { - role: 'user', - parts: [ - { - text: 'Please provide the final result now and stop calling tools.', - }, - ], - }, - ]; - continue; } else { - // No tool calls — treat this as the model's final answer. - if (roundText && roundText.trim().length > 0) { - finalText = roundText.trim(); - // Emit ROUND_END for the final round so all consumers see it. - // Previously this was skipped, requiring AgentInteractive to - // compensate with an explicit flushStreamBuffers() call. + const immediateExternalInputs = this.drainExternalInputs(options); + if (immediateExternalInputs.length > 0) { + currentMessages = this.externalInputsToContent( + immediateExternalInputs, + ); + this.emitExternalInputEvents(immediateExternalInputs); + } else if (options?.shouldWaitForExternalMessages?.()) { this.eventEmitter?.emit(AgentEventType.ROUND_END, { subagentId: this.subagentId, round: turnCounter, promptId, timestamp: Date.now(), } as AgentRoundEvent); - // Clean up before breaking - abortController.signal.removeEventListener('abort', onParentAbort); - // null terminateMode = normal text completion - break; + + const waitResult = await this.waitForExternalInputs( + options, + abortController, + startTime, + turnCounter, + ); + if (waitResult.terminateMode) { + finalText = roundText.trim(); + terminateMode = waitResult.terminateMode; + break; + } + if (waitResult.inputs.length > 0) { + currentMessages = this.externalInputsToContent(waitResult.inputs); + this.emitExternalInputEvents(waitResult.inputs); + continue; + } + + if (roundText && roundText.trim().length > 0) { + finalText = roundText.trim(); + break; + } + currentMessages = [ + { + role: 'user', + parts: [ + { + text: 'Please provide the final result now and stop calling tools.', + }, + ], + }, + ]; + continue; + } else { + // No tool calls — treat this as the model's final answer. + if (roundText && roundText.trim().length > 0) { + finalText = roundText.trim(); + // Emit ROUND_END for the final round so all consumers see it. + // Previously this was skipped, requiring AgentInteractive to + // compensate with an explicit flushStreamBuffers() call. + this.eventEmitter?.emit(AgentEventType.ROUND_END, { + subagentId: this.subagentId, + round: turnCounter, + promptId, + timestamp: Date.now(), + } as AgentRoundEvent); + // null terminateMode = normal text completion + break; + } + // Otherwise, nudge the model to finalize a result. + currentMessages = [ + { + role: 'user', + parts: [ + { + text: 'Please provide the final result now and stop calling tools.', + }, + ], + }, + ]; } - // Otherwise, nudge the model to finalize a result. - currentMessages = [ - { - role: 'user', - parts: [ - { - text: 'Please provide the final result now and stop calling tools.', - }, - ], - }, - ]; } + + this.eventEmitter?.emit(AgentEventType.ROUND_END, { + subagentId: this.subagentId, + round: turnCounter, + promptId, + timestamp: Date.now(), + } as AgentRoundEvent); + } finally { + // Reverse-cleanup fires whether the iteration ended normally, broke, + // returned, or threw — preventing parent-listener accumulation on + // long-running parents like the per-message roundAbortController in + // AgentInteractive or the session-lived externalSignal in headless. + roundAbortController.abort(); } - - this.eventEmitter?.emit(AgentEventType.ROUND_END, { - subagentId: this.subagentId, - round: turnCounter, - promptId, - timestamp: Date.now(), - } as AgentRoundEvent); - - // Clean up the per-round listener before the next iteration - abortController.signal.removeEventListener('abort', onParentAbort); } return { @@ -943,12 +945,7 @@ export class AgentCore { return { inputs: [] }; } - const waitAbortController = new AbortController(); - const onAbort = () => waitAbortController.abort(); - abortController.signal.addEventListener('abort', onAbort, { once: true }); - if (abortController.signal.aborted) { - waitAbortController.abort(); - } + const waitAbortController = createChildAbortController(abortController); let timedOut = false; let timeout: ReturnType | undefined; if (remainingTimeMs !== undefined) { @@ -985,7 +982,9 @@ export class AgentCore { throw error; } finally { if (timeout) clearTimeout(timeout); - abortController.signal.removeEventListener('abort', onAbort); + // Aborting the child fires reverse-cleanup of its listener on the + // parent; no-op if it already aborted from the parent or the timeout. + waitAbortController.abort(); } } } @@ -1325,17 +1324,23 @@ export class AgentCore { } }; abortController.signal.addEventListener('abort', onAbort, { once: true }); + try { + // If already aborted before the listener was registered, resolve + // immediately to avoid blocking forever. + if (abortController.signal.aborted) { + onAbort(); + } - // If already aborted before the listener was registered, resolve - // immediately to avoid blocking forever. - if (abortController.signal.aborted) { - onAbort(); + await scheduler.schedule(requests, abortController.signal); + await batchDone; + } finally { + // Always remove `onAbort` — otherwise a throw from scheduler.schedule + // or batchDone would leak it on the round controller, and the round's + // outer try/finally `.abort()` would later fire spurious cancellation + // TOOL_RESULT events for every un-emitted callId (corrupting the + // transcript and misleading the model on the next round). + abortController.signal.removeEventListener('abort', onAbort); } - - await scheduler.schedule(requests, abortController.signal); - await batchDone; - - abortController.signal.removeEventListener('abort', onAbort); } // If all tool calls failed, inform the model so it can re-evaluate. diff --git a/packages/core/src/agents/runtime/agent-headless.ts b/packages/core/src/agents/runtime/agent-headless.ts index dd77e11def..33f96de748 100644 --- a/packages/core/src/agents/runtime/agent-headless.ts +++ b/packages/core/src/agents/runtime/agent-headless.ts @@ -17,6 +17,7 @@ import type { Content } from '@google/genai'; import type { Config } from '../../config/config.js'; import type { RuntimeContentGeneratorView } from './agent-context.js'; +import { createChildAbortController } from '../../utils/abortController.js'; import { createDebugLogger } from '../../utils/debugLogger.js'; import type { AgentEventEmitter, @@ -225,116 +226,117 @@ export class AgentHeadless { return; } - // Set up abort signal propagation - const abortController = new AbortController(); - const onExternalAbort = () => { - abortController.abort(); - }; - if (externalSignal) { - externalSignal.addEventListener('abort', onExternalAbort); - } - if (externalSignal?.aborted) { - abortController.abort(); - } - - const toolsList = await this.core.prepareTools(); - - const initialMessages = - initialMessagesOverride && initialMessagesOverride.length > 0 - ? initialMessagesOverride - : [{ role: 'user' as const, parts: [{ text: initialTaskText }] }]; - - const startTime = Date.now(); - this.core.executionStats.startTimeMs = startTime; - this.core.stats.start(startTime); + // Child controller propagates from optional externalSignal and auto-cleans + // its parent listener when aborted (see utils/abortController.ts). + const abortController = createChildAbortController(externalSignal); try { - // Emit start event - this.core.eventEmitter?.emit(AgentEventType.START, { - subagentId: this.core.subagentId, - name: this.core.name, - model: - this.core.modelConfig.model || - this.core.runtimeContext.getModel() || - DEFAULT_QWEN_MODEL, - tools: (this.core.toolConfig?.tools || ['*']).map((t) => - typeof t === 'string' ? t : t.name, - ), - timestamp: Date.now(), - } as AgentStartEvent); + const toolsList = await this.core.prepareTools(); - // Log telemetry for subagent start - const startEvent = new SubagentExecutionEvent(this.core.name, 'started'); - logSubagentExecution(this.core.runtimeContext, startEvent); + const initialMessages = + initialMessagesOverride && initialMessagesOverride.length > 0 + ? initialMessagesOverride + : [{ role: 'user' as const, parts: [{ text: initialTaskText }] }]; - // Delegate to AgentCore's reasoning loop - const result = await this.core.runReasoningLoop( - chat, - initialMessages, - toolsList, - abortController, - { - maxTurns: this.core.runConfig.max_turns, - maxTimeMinutes: this.core.runConfig.max_time_minutes, - startTimeMs: startTime, - getExternalMessages: this.externalMessageProvider, - waitForExternalMessages: this.externalMessageWaiter, - shouldWaitForExternalMessages: this.externalMessageWaitPredicate, - }, - ); + const startTime = Date.now(); + this.core.executionStats.startTimeMs = startTime; + this.core.stats.start(startTime); - this.finalText = result.text; - this.terminateMode = result.terminateMode ?? AgentTerminateMode.GOAL; - } catch (error) { - debugLogger.error('Error during subagent execution:', error); - this.terminateMode = AgentTerminateMode.ERROR; - this.core.eventEmitter?.emit(AgentEventType.ERROR, { - subagentId: this.core.subagentId, - error: error instanceof Error ? error.message : String(error), - timestamp: Date.now(), - } as AgentErrorEvent); - - throw error; - } finally { - if (externalSignal) { - externalSignal.removeEventListener('abort', onExternalAbort); - } - this.core.executionStats.totalDurationMs = Date.now() - startTime; - const summary = this.core.stats.getSummary(Date.now()); - this.core.eventEmitter?.emit(AgentEventType.FINISH, { - subagentId: this.core.subagentId, - terminateReason: this.terminateMode, - timestamp: Date.now(), - rounds: summary.rounds, - totalDurationMs: summary.totalDurationMs, - totalToolCalls: summary.totalToolCalls, - successfulToolCalls: summary.successfulToolCalls, - failedToolCalls: summary.failedToolCalls, - inputTokens: summary.inputTokens, - outputTokens: summary.outputTokens, - totalTokens: summary.totalTokens, - } as AgentFinishEvent); - - const completionEvent = new SubagentExecutionEvent( - this.core.name, - this.terminateMode === AgentTerminateMode.GOAL ? 'completed' : 'failed', - { - terminate_reason: this.terminateMode, - result: this.finalText, - execution_summary: this.core.stats.formatCompact( - 'Subagent execution completed', + try { + // Emit start event + this.core.eventEmitter?.emit(AgentEventType.START, { + subagentId: this.core.subagentId, + name: this.core.name, + model: + this.core.modelConfig.model || + this.core.runtimeContext.getModel() || + DEFAULT_QWEN_MODEL, + tools: (this.core.toolConfig?.tools || ['*']).map((t) => + typeof t === 'string' ? t : t.name, ), - }, - ); - logSubagentExecution(this.core.runtimeContext, completionEvent); + timestamp: Date.now(), + } as AgentStartEvent); - await this.core.hooks?.onStop?.({ - subagentId: this.core.subagentId, - name: this.core.name, - terminateReason: this.terminateMode, - summary: summary as unknown as Record, - timestamp: Date.now(), - }); + // Log telemetry for subagent start + const startEvent = new SubagentExecutionEvent( + this.core.name, + 'started', + ); + logSubagentExecution(this.core.runtimeContext, startEvent); + + // Delegate to AgentCore's reasoning loop + const result = await this.core.runReasoningLoop( + chat, + initialMessages, + toolsList, + abortController, + { + maxTurns: this.core.runConfig.max_turns, + maxTimeMinutes: this.core.runConfig.max_time_minutes, + startTimeMs: startTime, + getExternalMessages: this.externalMessageProvider, + waitForExternalMessages: this.externalMessageWaiter, + shouldWaitForExternalMessages: this.externalMessageWaitPredicate, + }, + ); + + this.finalText = result.text; + this.terminateMode = result.terminateMode ?? AgentTerminateMode.GOAL; + } catch (error) { + debugLogger.error('Error during subagent execution:', error); + this.terminateMode = AgentTerminateMode.ERROR; + this.core.eventEmitter?.emit(AgentEventType.ERROR, { + subagentId: this.core.subagentId, + error: error instanceof Error ? error.message : String(error), + timestamp: Date.now(), + } as AgentErrorEvent); + + throw error; + } finally { + this.core.executionStats.totalDurationMs = Date.now() - startTime; + const summary = this.core.stats.getSummary(Date.now()); + this.core.eventEmitter?.emit(AgentEventType.FINISH, { + subagentId: this.core.subagentId, + terminateReason: this.terminateMode, + timestamp: Date.now(), + rounds: summary.rounds, + totalDurationMs: summary.totalDurationMs, + totalToolCalls: summary.totalToolCalls, + successfulToolCalls: summary.successfulToolCalls, + failedToolCalls: summary.failedToolCalls, + inputTokens: summary.inputTokens, + outputTokens: summary.outputTokens, + totalTokens: summary.totalTokens, + } as AgentFinishEvent); + + const completionEvent = new SubagentExecutionEvent( + this.core.name, + this.terminateMode === AgentTerminateMode.GOAL + ? 'completed' + : 'failed', + { + terminate_reason: this.terminateMode, + result: this.finalText, + execution_summary: this.core.stats.formatCompact( + 'Subagent execution completed', + ), + }, + ); + logSubagentExecution(this.core.runtimeContext, completionEvent); + + await this.core.hooks?.onStop?.({ + subagentId: this.core.subagentId, + name: this.core.name, + terminateReason: this.terminateMode, + summary: summary as unknown as Record, + timestamp: Date.now(), + }); + } + } finally { + // Outer finally guarantees the child's parent-signal listener is + // detached even if prepareTools or initialMessages prep throws before + // the inner try runs. + abortController.abort(); } } diff --git a/packages/core/src/agents/runtime/agent-interactive.ts b/packages/core/src/agents/runtime/agent-interactive.ts index b7fbba1df0..08744e4289 100644 --- a/packages/core/src/agents/runtime/agent-interactive.ts +++ b/packages/core/src/agents/runtime/agent-interactive.ts @@ -11,6 +11,10 @@ * state (messages, pending approvals, live outputs) that the UI reads. */ +import { + createAbortController, + createChildAbortController, +} from '../../utils/abortController.js'; import { createDebugLogger } from '../../utils/debugLogger.js'; import { type AgentEventEmitter, AgentEventType } from './agent-events.js'; import type { @@ -57,7 +61,7 @@ export class AgentInteractive { private error: string | undefined; private lastRoundError: string | undefined; private executionPromise: Promise | undefined; - private masterAbortController = new AbortController(); + private masterAbortController = createAbortController(); private roundAbortController: AbortController | undefined; private chat: GeminiChat | undefined; private toolsList: FunctionDeclaration[] = []; @@ -150,14 +154,9 @@ export class AgentInteractive { this.setStatus(AgentStatus.RUNNING); this.lastRoundError = undefined; this.roundCancelledByUser = false; - this.roundAbortController = new AbortController(); - - // Propagate master abort to round - const onMasterAbort = () => this.roundAbortController?.abort(); - this.masterAbortController.signal.addEventListener('abort', onMasterAbort); - if (this.masterAbortController.signal.aborted) { - this.roundAbortController.abort(); - } + this.roundAbortController = createChildAbortController( + this.masterAbortController, + ); try { const initialMessages = [ @@ -196,10 +195,10 @@ export class AgentInteractive { debugLogger.error('AgentInteractive round error:', err); this.addMessage('info', errorMessage, { metadata: { level: 'error' } }); } finally { - this.masterAbortController.signal.removeEventListener( - 'abort', - onMasterAbort, - ); + // Helper's reverse-cleanup detaches the parent listener automatically + // when the round controller aborts; abort here so cleanup fires whether + // or not the round was already cancelled. + this.roundAbortController?.abort(); this.roundAbortController = undefined; } } diff --git a/packages/core/src/confirmation-bus/message-bus.ts b/packages/core/src/confirmation-bus/message-bus.ts index e8a737f82e..97ac334776 100644 --- a/packages/core/src/confirmation-bus/message-bus.ts +++ b/packages/core/src/confirmation-bus/message-bus.ts @@ -120,7 +120,7 @@ export class MessageBus extends EventEmitter { }; if (signal) { - signal.addEventListener('abort', abortHandler); + signal.addEventListener('abort', abortHandler, { once: true }); } const responseHandler = (response: TResponse) => { diff --git a/packages/core/src/core/openaiContentGenerator/pipeline.ts b/packages/core/src/core/openaiContentGenerator/pipeline.ts index e08751ea8d..08876e2bd9 100644 --- a/packages/core/src/core/openaiContentGenerator/pipeline.ts +++ b/packages/core/src/core/openaiContentGenerator/pipeline.ts @@ -4,7 +4,6 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { setMaxListeners } from 'node:events'; import type OpenAI from 'openai'; import { type GenerateContentParameters, @@ -20,23 +19,6 @@ import type { PipelineConfig, RequestContext } from './types.js'; import { redactProxyError } from '../../utils/runtimeFetchOptions.js'; import { runtimeDiagnostics } from '../../utils/runtimeDiagnostics.js'; -/** - * The OpenAI SDK adds an abort listener for every `chat.completions.create` - * call, and several layers (retryWithBackoff, LoggingContentGenerator, the - * SDK's internal stream/fetch wrappers) each register their own listeners - * on the same per-request AbortSignal. With 5 retries the count comfortably - * exceeds Node's default 10-listener leak warning — and on top of that, - * concurrent code paths (e.g., recap + followup speculation) can share or - * compose signals, pushing it past any small cap. - * - * These signals are per-request and short-lived (GC'd when the request - * settles), so accumulation here is structural, not a memory leak. Disable - * the warning entirely for them. Idempotent. - */ -function raiseAbortListenerCap(signal: AbortSignal | undefined): void { - if (signal) setMaxListeners(0, signal); -} - /** * Error thrown when the API returns an error embedded as stream content * instead of a proper HTTP error. Some providers (e.g., certain OpenAI-compatible @@ -65,7 +47,6 @@ export class ContentGenerationPipeline { request: GenerateContentParameters, userPromptId: string, ): Promise { - raiseAbortListenerCap(request.config?.abortSignal); return this.executeWithErrorHandling( request, userPromptId, @@ -93,7 +74,6 @@ export class ContentGenerationPipeline { request: GenerateContentParameters, userPromptId: string, ): Promise> { - raiseAbortListenerCap(request.config?.abortSignal); return this.executeWithErrorHandling( request, userPromptId, diff --git a/packages/core/src/hooks/combinedAbortSignal.test.ts b/packages/core/src/hooks/combinedAbortSignal.test.ts deleted file mode 100644 index 1954237bc4..0000000000 --- a/packages/core/src/hooks/combinedAbortSignal.test.ts +++ /dev/null @@ -1,111 +0,0 @@ -/** - * @license - * Copyright 2026 Qwen Team - * SPDX-License-Identifier: Apache-2.0 - */ - -import { describe, it, expect, vi } from 'vitest'; -import { createCombinedAbortSignal } from './combinedAbortSignal.js'; - -describe('createCombinedAbortSignal', () => { - it('should return a non-aborted signal by default', () => { - const { signal, cleanup } = createCombinedAbortSignal(); - expect(signal.aborted).toBe(false); - cleanup(); - }); - - it('should abort after timeout', async () => { - const { signal, cleanup } = createCombinedAbortSignal(undefined, { - timeoutMs: 50, - }); - expect(signal.aborted).toBe(false); - - await new Promise((resolve) => setTimeout(resolve, 100)); - expect(signal.aborted).toBe(true); - cleanup(); - }); - - it('should abort when external signal is aborted', () => { - const externalController = new AbortController(); - const { signal, cleanup } = createCombinedAbortSignal( - externalController.signal, - ); - expect(signal.aborted).toBe(false); - - externalController.abort(); - expect(signal.aborted).toBe(true); - cleanup(); - }); - - it('should abort immediately if external signal is already aborted', () => { - const externalController = new AbortController(); - externalController.abort(); - - const { signal, cleanup } = createCombinedAbortSignal( - externalController.signal, - ); - expect(signal.aborted).toBe(true); - cleanup(); - }); - - it('should cleanup timeout timer', async () => { - const { signal, cleanup } = createCombinedAbortSignal(undefined, { - timeoutMs: 50, - }); - - cleanup(); - - // Wait longer than timeout - should not abort because timer was cleared - await new Promise((resolve) => setTimeout(resolve, 100)); - expect(signal.aborted).toBe(false); - }); - - it('should remove external abort listener on cleanup', () => { - const externalController = new AbortController(); - const removeListenerSpy = vi.spyOn( - externalController.signal, - 'removeEventListener', - ); - const { signal, cleanup } = createCombinedAbortSignal( - externalController.signal, - ); - - cleanup(); - externalController.abort(); - - expect(removeListenerSpy).toHaveBeenCalledWith( - 'abort', - expect.any(Function), - ); - expect(signal.aborted).toBe(false); - }); - - it('should work with both external signal and timeout', async () => { - const externalController = new AbortController(); - const { signal, cleanup } = createCombinedAbortSignal( - externalController.signal, - { timeoutMs: 200 }, - ); - - // Abort external signal before timeout - externalController.abort(); - expect(signal.aborted).toBe(true); - cleanup(); - }); - - it('should timeout before external signal', async () => { - const externalController = new AbortController(); - const { signal, cleanup } = createCombinedAbortSignal( - externalController.signal, - { timeoutMs: 50 }, - ); - - // Wait for timeout - await new Promise((resolve) => setTimeout(resolve, 100)); - expect(signal.aborted).toBe(true); - - // External signal is still not aborted - expect(externalController.signal.aborted).toBe(false); - cleanup(); - }); -}); diff --git a/packages/core/src/hooks/combinedAbortSignal.ts b/packages/core/src/hooks/combinedAbortSignal.ts deleted file mode 100644 index dfcdf923f6..0000000000 --- a/packages/core/src/hooks/combinedAbortSignal.ts +++ /dev/null @@ -1,57 +0,0 @@ -/** - * @license - * Copyright 2026 Qwen Team - * SPDX-License-Identifier: Apache-2.0 - */ - -/** - * Create a combined AbortSignal that aborts when either: - * - The provided external signal is aborted, OR - * - The timeout is reached - * - * @param externalSignal - Optional external AbortSignal to combine - * @param timeoutMs - Timeout in milliseconds - * @returns Object containing the combined signal and a cleanup function - */ -export function createCombinedAbortSignal( - externalSignal?: AbortSignal, - options?: { timeoutMs?: number }, -): { signal: AbortSignal; cleanup: () => void } { - const controller = new AbortController(); - - const timeoutMs = options?.timeoutMs; - - // Set up timeout - let timeoutId: ReturnType | undefined; - if (timeoutMs !== undefined && timeoutMs > 0) { - timeoutId = setTimeout(() => { - controller.abort(); - }, timeoutMs); - } - - // Listen to external signal - let abortHandler: (() => void) | undefined; - if (externalSignal) { - if (externalSignal.aborted) { - controller.abort(); - } else { - abortHandler = () => { - controller.abort(); - }; - externalSignal.addEventListener('abort', abortHandler, { once: true }); - } - } - - const cleanup = () => { - if (timeoutId !== undefined) { - clearTimeout(timeoutId); - timeoutId = undefined; - } - if (externalSignal && abortHandler) { - externalSignal.removeEventListener('abort', abortHandler); - abortHandler = undefined; - } - }; - - return { signal: controller.signal, cleanup }; -} diff --git a/packages/core/src/hooks/functionHookRunner.ts b/packages/core/src/hooks/functionHookRunner.ts index badcd344c1..e2033d6f1f 100644 --- a/packages/core/src/hooks/functionHookRunner.ts +++ b/packages/core/src/hooks/functionHookRunner.ts @@ -234,7 +234,7 @@ export class FunctionHookRunner { abortHandler = () => { reject(new Error('Function hook execution aborted')); }; - signal.addEventListener('abort', abortHandler); + signal.addEventListener('abort', abortHandler, { once: true }); } }); diff --git a/packages/core/src/hooks/hookRunner.ts b/packages/core/src/hooks/hookRunner.ts index ac45da9306..6f664267d2 100644 --- a/packages/core/src/hooks/hookRunner.ts +++ b/packages/core/src/hooks/hookRunner.ts @@ -614,7 +614,7 @@ export class HookRunner { }; if (signal) { - signal.addEventListener('abort', abortHandler); + signal.addEventListener('abort', abortHandler, { once: true }); } // Send input to stdin diff --git a/packages/core/src/hooks/httpHookRunner.ts b/packages/core/src/hooks/httpHookRunner.ts index aad909ed3f..cc72d41b7f 100644 --- a/packages/core/src/hooks/httpHookRunner.ts +++ b/packages/core/src/hooks/httpHookRunner.ts @@ -7,7 +7,7 @@ import { createDebugLogger } from '../utils/debugLogger.js'; import { interpolateHeaders, interpolateUrl } from './envInterpolator.js'; import { UrlValidator } from './urlValidator.js'; -import { createCombinedAbortSignal } from './combinedAbortSignal.js'; +import { combineAbortSignals } from '../utils/abortController.js'; import { isBlockedAddress } from './ssrfGuard.js'; import { lookup as dnsLookup } from 'dns'; import type { @@ -199,8 +199,8 @@ export class HttpHookRunner { const timeout = hookConfig.timeout ? hookConfig.timeout * 1000 : DEFAULT_HTTP_TIMEOUT; - const { signal: combinedSignal, cleanup } = createCombinedAbortSignal( - signal, + const { signal: combinedSignal, cleanup } = combineAbortSignals( + [signal], { timeoutMs: timeout }, ); diff --git a/packages/core/src/hooks/promptHookRunner.ts b/packages/core/src/hooks/promptHookRunner.ts index 1f57bbf5a5..4e9b524267 100644 --- a/packages/core/src/hooks/promptHookRunner.ts +++ b/packages/core/src/hooks/promptHookRunner.ts @@ -5,6 +5,7 @@ */ import { z } from 'zod'; +import { createChildAbortController } from '../utils/abortController.js'; import { createDebugLogger } from '../utils/debugLogger.js'; import type { PromptHookConfig, @@ -229,21 +230,14 @@ export class PromptHookRunner { }, ]; - // Create internal AbortController to abort the request on timeout - const internalAbortController = new AbortController(); + // Internal AbortController to abort the request on timeout. Use + // createChildAbortController so parent-signal propagation gets `{once:true}` + // + reverse cleanup automatically — the old manual addEventListener path + // had no `{once:true}` and never removed the listener, leaking one + // listener per prompt-hook invocation on the long-lived parent. + const internalAbortController = createChildAbortController(signal); const internalSignal = internalAbortController.signal; - // Chain external signal to internal abort controller - if (signal) { - if (signal.aborted) { - internalAbortController.abort(); - } else { - signal.addEventListener('abort', () => { - internalAbortController.abort(); - }); - } - } - // Create timeout promise that also aborts the request let timeoutId: ReturnType | undefined; const timeoutPromise = new Promise((_, reject) => { @@ -320,6 +314,9 @@ export class PromptHookRunner { if (timeoutId) { clearTimeout(timeoutId); } + // Trigger reverse-cleanup of the parent-signal listener on the + // success path; no-op if already aborted via parent/timeout. + internalAbortController.abort(); } } diff --git a/packages/core/src/utils/abortController.test.ts b/packages/core/src/utils/abortController.test.ts new file mode 100644 index 0000000000..e81e617c61 --- /dev/null +++ b/packages/core/src/utils/abortController.test.ts @@ -0,0 +1,340 @@ +/** + * @license + * Copyright 2025 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, expect, it, vi } from 'vitest'; +import { getEventListeners, getMaxListeners } from 'node:events'; +import { + combineAbortSignals, + createAbortController, + createChildAbortController, +} from './abortController.js'; + +describe('createAbortController', () => { + it('sets a default max-listener cap of 50 on the signal', () => { + const controller = createAbortController(); + expect(getMaxListeners(controller.signal)).toBe(50); + }); + + it('honors a custom max-listener cap', () => { + const controller = createAbortController(200); + expect(getMaxListeners(controller.signal)).toBe(200); + }); + + it('produces a working, abortable controller', () => { + const controller = createAbortController(); + expect(controller.signal.aborted).toBe(false); + controller.abort('done'); + expect(controller.signal.aborted).toBe(true); + expect(controller.signal.reason).toBe('done'); + }); +}); + +describe('createChildAbortController', () => { + it('aborts when the parent aborts and propagates the reason', () => { + const parent = createAbortController(); + const child = createChildAbortController(parent); + parent.abort('parent-reason'); + expect(child.signal.aborted).toBe(true); + expect(child.signal.reason).toBe('parent-reason'); + }); + + it('does not abort the parent when the child aborts', () => { + const parent = createAbortController(); + const child = createChildAbortController(parent); + child.abort('child-reason'); + expect(child.signal.aborted).toBe(true); + expect(parent.signal.aborted).toBe(false); + }); + + it('aborts synchronously when the parent is already aborted (fast path)', () => { + const parent = createAbortController(); + parent.abort('pre-aborted'); + const child = createChildAbortController(parent); + expect(child.signal.aborted).toBe(true); + expect(child.signal.reason).toBe('pre-aborted'); + // No listener should have been registered on the parent in the fast path. + expect(getEventListeners(parent.signal, 'abort').length).toBe(0); + }); + + it('removes its parent listener once the child has aborted (reverse cleanup)', () => { + const parent = createAbortController(); + const child = createChildAbortController(parent); + expect(getEventListeners(parent.signal, 'abort').length).toBe(1); + child.abort(); + expect(getEventListeners(parent.signal, 'abort').length).toBe(0); + }); + + it('removes its parent listener after parent abort fires (once: true)', () => { + const parent = createAbortController(); + createChildAbortController(parent); + expect(getEventListeners(parent.signal, 'abort').length).toBe(1); + parent.abort(); + // The {once: true} listener should self-remove after firing. + expect(getEventListeners(parent.signal, 'abort').length).toBe(0); + }); + + it('does not accumulate listeners on a long-lived parent across many short-lived children', () => { + const parent = createAbortController(); + for (let i = 0; i < 1000; i++) { + const child = createChildAbortController(parent); + child.abort(); + } + expect(getEventListeners(parent.signal, 'abort').length).toBe(0); + }); + + it('accepts an AbortSignal directly as the parent', () => { + const parent = createAbortController(); + const child = createChildAbortController(parent.signal); + parent.abort(); + expect(child.signal.aborted).toBe(true); + }); + + it('returns a plain controller when the parent is undefined', () => { + const child = createChildAbortController(undefined); + expect(child.signal.aborted).toBe(false); + child.abort('manual'); + expect(child.signal.aborted).toBe(true); + }); + + it('forwards a custom maxListeners through to the child signal', () => { + const parent = createAbortController(); + const child = createChildAbortController(parent, 123); + expect(getMaxListeners(child.signal)).toBe(123); + }); +}); + +describe('combineAbortSignals', () => { + it('aborts when any input signal aborts', () => { + const a = createAbortController(); + const b = createAbortController(); + const { signal } = combineAbortSignals([a.signal, b.signal]); + expect(signal.aborted).toBe(false); + b.abort('from-b'); + expect(signal.aborted).toBe(true); + expect(signal.reason).toBe('from-b'); + }); + + it('aborts synchronously when an input is already aborted', () => { + const a = createAbortController(); + a.abort('pre'); + const { signal, cleanup } = combineAbortSignals([a.signal]); + expect(signal.aborted).toBe(true); + expect(signal.reason).toBe('pre'); + expect(() => cleanup()).not.toThrow(); + }); + + it('ignores undefined entries', () => { + const a = createAbortController(); + const { signal } = combineAbortSignals([undefined, a.signal, undefined]); + a.abort(); + expect(signal.aborted).toBe(true); + }); + + it('fires the timeout when no signal aborts first', async () => { + vi.useFakeTimers(); + try { + const { signal } = combineAbortSignals([], { timeoutMs: 50 }); + vi.advanceTimersByTime(50); + expect(signal.aborted).toBe(true); + expect((signal.reason as DOMException).name).toBe('TimeoutError'); + } finally { + vi.useRealTimers(); + } + }); + + it('auto-cleans input-signal listeners when the timeout fires', async () => { + // Timeout-driven aborts must run the same auto-cleanup as source-driven + // aborts — otherwise long-lived input signals (e.g. a session-lived + // AbortSignal) accumulate dead listeners across many short-lived + // combinedSignal calls. Verifies cleanup is wired to the COMBINED + // controller abort path, not just to source-signal events. + vi.useFakeTimers(); + try { + const source = createAbortController(); + const before = getEventListeners(source.signal, 'abort').length; + const { signal } = combineAbortSignals([source.signal], { + timeoutMs: 50, + }); + expect(getEventListeners(source.signal, 'abort').length).toBe(before + 1); + vi.advanceTimersByTime(50); + expect(signal.aborted).toBe(true); + expect((signal.reason as DOMException).name).toBe('TimeoutError'); + expect(getEventListeners(source.signal, 'abort').length).toBe(before); + } finally { + vi.useRealTimers(); + } + }); + + it('cleanup removes listeners from inputs', () => { + const a = createAbortController(); + const before = getEventListeners(a.signal, 'abort').length; + const { cleanup } = combineAbortSignals([a.signal]); + expect(getEventListeners(a.signal, 'abort').length).toBe(before + 1); + cleanup(); + expect(getEventListeners(a.signal, 'abort').length).toBe(before); + }); + + it('cleanup is idempotent', () => { + const a = createAbortController(); + const { cleanup } = combineAbortSignals([a.signal]); + cleanup(); + expect(() => cleanup()).not.toThrow(); + }); + + it('manual cleanup() cancels a pending timeout so it never fires', () => { + vi.useFakeTimers(); + try { + const { signal, cleanup } = combineAbortSignals([], { timeoutMs: 50 }); + cleanup(); + vi.advanceTimersByTime(100); + // Without the clearTimeout in cleanups[], the timer would still fire + // and abort the (already-cleaned) signal with TimeoutError. + expect(signal.aborted).toBe(false); + } finally { + vi.useRealTimers(); + } + }); + + it('treats timeoutMs <= 0 as "no timeout"', () => { + vi.useFakeTimers(); + try { + const zero = combineAbortSignals([], { timeoutMs: 0 }); + const negative = combineAbortSignals([], { timeoutMs: -1 }); + vi.advanceTimersByTime(1_000_000); + expect(zero.signal.aborted).toBe(false); + expect(negative.signal.aborted).toBe(false); + zero.cleanup(); + negative.cleanup(); + } finally { + vi.useRealTimers(); + } + }); + + it('aborts and stops registering listeners once an input is found aborted mid-iteration', () => { + const a = createAbortController(); + const b = createAbortController(); + const c = createAbortController(); + // Simulate a signal whose `aborted` getter returns false during the initial + // `find` scan and true on subsequent accesses, exercising the per-iteration + // defensive check inside the for-loop (not the fast path). + let accessCount = 0; + const proxied = new Proxy(b.signal, { + get(target, prop, recv) { + if (prop === 'aborted') { + accessCount++; + return accessCount > 1; // false on first access, true thereafter + } + return Reflect.get(target, prop, recv); + }, + }) as AbortSignal; + const { signal } = combineAbortSignals([a.signal, proxied, c.signal]); + // Per-iteration check fires when the loop reaches proxied (2nd `aborted` + // access) and short-circuits → controller aborts, loop breaks before c. + expect(signal.aborted).toBe(true); + // a was iterated before the break and DID get a listener — cleanup must + // run synchronously (since adding to an already-aborted signal is a no-op), + // otherwise the listener leaks on the long-lived input. + expect(getEventListeners(a.signal, 'abort').length).toBe(0); + // c never had a listener attached (we broke out of the loop before it). + expect(getEventListeners(c.signal, 'abort').length).toBe(0); + }); + + it('does not schedule a timeout when the per-iteration check aborts the controller mid-loop', () => { + // Drives the `!controller.signal.aborted` guard inside the timeout + // block (not the pre-loop fast path): the Proxy reports `aborted=false` + // on the initial scan and `aborted=true` once the loop re-checks it. + // Spy on setTimeout so we can distinguish "guard skipped scheduling" + // from "scheduled then immediately cleared by synchronous cleanup" — + // the latter would be observationally indistinguishable via timer + // advancement alone since cleanup() runs synchronously and clears the + // timer it just scheduled. + vi.useFakeTimers(); + const setTimeoutSpy = vi.spyOn(globalThis, 'setTimeout'); + try { + const a = createAbortController(); + const b = createAbortController(); + let accessCount = 0; + const proxied = new Proxy(b.signal, { + get(target, prop, recv) { + if (prop === 'aborted') { + accessCount++; + return accessCount > 1; + } + return Reflect.get(target, prop, recv); + }, + }) as AbortSignal; + const { signal } = combineAbortSignals([a.signal, proxied], { + timeoutMs: 50, + }); + expect(signal.aborted).toBe(true); + // The guard must prevent setTimeout from being called at all. + expect(setTimeoutSpy).not.toHaveBeenCalled(); + // Belt-and-suspenders: even if a timer somehow snuck through, + // advancing past it must not change the abort reason. + const reasonAfterAbort = signal.reason; + vi.advanceTimersByTime(100); + expect(signal.reason).toBe(reasonAfterAbort); + } finally { + setTimeoutSpy.mockRestore(); + vi.useRealTimers(); + } + }); + + it('auto-cleans listeners on inputs when the combined signal aborts', () => { + const a = createAbortController(); + const b = createAbortController(); + combineAbortSignals([a.signal, b.signal]); + expect(getEventListeners(a.signal, 'abort').length).toBe(1); + expect(getEventListeners(b.signal, 'abort').length).toBe(1); + a.abort(); + expect(getEventListeners(a.signal, 'abort').length).toBe(0); + expect(getEventListeners(b.signal, 'abort').length).toBe(0); + }); +}); + +describe('lifetime contract', () => { + it('parent abort propagates to a signal whose controller the caller has dropped', () => { + // Real-world pattern: caller pipes child.signal into an async API and + // does not hold the controller object itself. The parent listener + // closure keeps the controller alive long enough for parent abort to + // reach the signal — verified WITHOUT --expose-gc because we don't + // depend on GC behavior at all, only on the strong reference inside + // the listener closure. + const parent = createAbortController(); + let signal: AbortSignal; + (() => { + const child = createChildAbortController(parent); + signal = child.signal; + })(); + expect(signal!.aborted).toBe(false); + parent.abort('parent-reason'); + expect(signal!.aborted).toBe(true); + expect(signal!.reason).toBe('parent-reason'); + }); +}); + +describe('GC safety (best-effort, requires --expose-gc)', () => { + const maybeGc = (globalThis as { gc?: () => void }).gc; + const itGc = maybeGc ? it : it.skip; + + itGc('controller becomes GC-eligible after the child aborts', async () => { + // After child.abort(), the reverse-cleanup listener removes the + // parent's handler closure — which was the strong holder of the + // controller. With no other refs, the controller is collectable. + const parent = createAbortController(); + let weakChild: WeakRef; + (() => { + const child = createChildAbortController(parent); + weakChild = new WeakRef(child); + child.abort(); + })(); + await new Promise((r) => setTimeout(r, 0)); + maybeGc!(); + await new Promise((r) => setTimeout(r, 0)); + maybeGc!(); + expect(weakChild!.deref()).toBeUndefined(); + }); +}); diff --git a/packages/core/src/utils/abortController.ts b/packages/core/src/utils/abortController.ts new file mode 100644 index 0000000000..5dc4f9f3d2 --- /dev/null +++ b/packages/core/src/utils/abortController.ts @@ -0,0 +1,165 @@ +/** + * @license + * Copyright 2025 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +import { setMaxListeners } from 'node:events'; + +/** + * Default per-signal listener cap. Sized generously so OpenAI SDK retries + + * internal stream/fetch wrappers + per-tool listeners can coexist on a single + * short-lived per-request signal without warning. + */ +const DEFAULT_MAX_LISTENERS = 50; + +/** + * Create an AbortController with its signal pre-configured to allow a sane + * number of listeners. Use this in place of `new AbortController()` everywhere + * in production code. + */ +export function createAbortController( + maxListeners: number = DEFAULT_MAX_LISTENERS, +): AbortController { + const controller = new AbortController(); + setMaxListeners(maxListeners, controller.signal); + return controller; +} + +function asSignal( + parent: AbortController | AbortSignal | undefined, +): AbortSignal | undefined { + if (!parent) return undefined; + return parent instanceof AbortController ? parent.signal : parent; +} + +/** + * Create a child AbortController that aborts when its parent aborts. + * Aborting the child does NOT abort the parent. + * + * Three invariants keep listener accumulation bounded on long-lived parents + * even when many short-lived children come and go: + * - The parent's abort listener is registered with `{once: true}` so it + * removes itself when the parent fires. + * - When the child aborts (from any source — parent propagation, manual + * abort, etc.), the listener it registered on the parent is actively + * removed. This is the key to preventing dead-listener accumulation on + * long-lived parents. + * - The parent is held via `WeakRef` from the child's reverse-cleanup + * closure, so a child being kept alive does not pin its parent. + * + * Lifetime contract: the child controller is held strongly by the parent's + * listener closure until either the parent fires (closure released by + * `{once: true}` self-removal) or the child aborts (closure released by + * reverse-cleanup). This means callers can safely pass `child.signal` into + * async APIs and drop the controller object — the controller will stay + * alive long enough for parent abort to propagate to the signal. + * + * Accepts an `AbortController`, an `AbortSignal`, or `undefined`. Undefined + * returns a fresh controller with no parent propagation. + */ +export function createChildAbortController( + parent: AbortController | AbortSignal | undefined, + maxListeners?: number, +): AbortController { + const child = createAbortController(maxListeners); + const parentSignal = asSignal(parent); + + if (!parentSignal) return child; + + // Fast path: parent already aborted, no listener setup needed. + if (parentSignal.aborted) { + child.abort(parentSignal.reason); + return child; + } + + // WeakRef on the parent only — the handler closure strongly retains the + // child so that propagation works even if the caller passes child.signal + // to an async API and drops the controller object. See the contract + // docstring above. + const weakParent = new WeakRef(parentSignal); + const handler = (): void => { + child.abort(weakParent.deref()?.reason); + }; + + parentSignal.addEventListener('abort', handler, { once: true }); + + child.signal.addEventListener( + 'abort', + () => { + // `{once: true}` on the parent listener already self-removes when + // parent fires; this branch covers the child-aborts-first case so + // we don't leave a dead listener on a long-lived parent. + weakParent.deref()?.removeEventListener('abort', handler); + }, + { once: true }, + ); + + return child; +} + +/** + * Combine N input signals (any undefined entries are ignored) plus an optional + * timeout into a single child AbortSignal. The returned `cleanup` releases all + * listeners and clears the timeout — call it on the success path so listeners + * don't linger on long-lived input signals. Cleanup is idempotent and is also + * invoked automatically when the returned signal aborts. + */ +export function combineAbortSignals( + signals: ReadonlyArray, + options?: { timeoutMs?: number; maxListeners?: number }, +): { signal: AbortSignal; cleanup: () => void } { + const controller = createAbortController(options?.maxListeners); + + const alreadyAborted = signals.find((s) => s?.aborted); + if (alreadyAborted) { + controller.abort(alreadyAborted.reason); + return { signal: controller.signal, cleanup: () => {} }; + } + + const cleanups: Array<() => void> = []; + + for (const sourceSignal of signals) { + if (!sourceSignal) continue; + // Re-check aborted state per iteration. Single-threaded JS can't actually + // interleave aborts between the initial scan above and this point, but + // making the check obvious here keeps the function correct even if a + // future caller passes signals whose `aborted` getter has side effects. + if (sourceSignal.aborted) { + controller.abort(sourceSignal.reason); + break; + } + const handler = () => controller.abort(sourceSignal.reason); + sourceSignal.addEventListener('abort', handler, { once: true }); + cleanups.push(() => sourceSignal.removeEventListener('abort', handler)); + } + + // Skip timeout if the loop already aborted the controller — its cleanup + // wouldn't fire via the post-loop auto-cleanup path below. + const timeoutMs = options?.timeoutMs; + if (timeoutMs !== undefined && timeoutMs > 0 && !controller.signal.aborted) { + const timeoutId = setTimeout(() => { + controller.abort(new DOMException('Operation timed out', 'TimeoutError')); + }, timeoutMs); + cleanups.push(() => clearTimeout(timeoutId)); + } + + let done = false; + const cleanup = () => { + if (done) return; + done = true; + for (const fn of cleanups) fn(); + }; + + // Node does not fire 'abort' listeners added to an already-aborted signal, + // so if the per-iteration check aborted controller mid-loop we'd orphan + // every input listener that was registered before the break. Run cleanup + // synchronously instead. + if (controller.signal.aborted) { + cleanup(); + } else { + controller.signal.addEventListener('abort', cleanup, { once: true }); + } + + return { signal: controller.signal, cleanup }; +}