qwen-code/docs/verification/abort-controller-refactor/README.md
jinye 174e8de179
fix(core): stop AbortSignal listener leak in long sessions (MaxListenersExceededWarning) (#4366)
* 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 "<helperPath>"` 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.
2026-05-26 14:21:49 +08:00

120 lines
5.5 KiB
Markdown
Raw Permalink Blame History

This file contains ambiguous Unicode characters

This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.

# 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 >> <log>'`.
## 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 ~3040 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 <pid>`.
- **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.