From 08ed287f4facf5c9abdfaba10c47670f1cce923e Mon Sep 17 00:00:00 2001 From: doudouOUC Date: Thu, 21 May 2026 02:37:14 +0800 Subject: [PATCH 01/15] fix(core): consolidate AbortController handling to stop listener leaks in long sessions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../abort-controller-refactor/README.md | 64 +++ .../automated-results.md | 127 +++++ .../listener-accumulation-repro.mjs | 109 +++++ .../migration-completeness.txt | 0 .../abort-controller-refactor/smoke-boot.log | 2 + .../test-summary.txt | 10 + packages/cli/src/gemini.tsx | 2 + packages/cli/src/utils/warningHandler.test.ts | 153 ++++++ packages/cli/src/utils/warningHandler.ts | 72 +++ .../core/src/agents/arena/ArenaManager.ts | 8 +- .../src/agents/background-agent-resume.ts | 7 +- .../core/src/agents/runtime/agent-core.ts | 449 +++++++++--------- .../core/src/agents/runtime/agent-headless.ts | 216 ++++----- .../src/agents/runtime/agent-interactive.ts | 25 +- .../core/src/confirmation-bus/message-bus.ts | 2 +- packages/core/src/core/client.ts | 3 +- .../core/openaiContentGenerator/pipeline.ts | 20 - packages/core/src/followup/speculation.ts | 16 +- .../core/src/hooks/combinedAbortSignal.ts | 52 +- packages/core/src/hooks/functionHookRunner.ts | 2 +- packages/core/src/hooks/hookRunner.ts | 2 +- packages/core/src/memory/manager.ts | 3 +- .../src/services/chatCompressionService.ts | 3 +- .../core/src/services/chatRecordingService.ts | 3 +- packages/core/src/tools/agent/agent.ts | 28 +- packages/core/src/tools/monitor.ts | 3 +- packages/core/src/tools/shell.ts | 7 +- .../core/src/utils/abortController.test.ts | 193 ++++++++ packages/core/src/utils/abortController.ts | 163 +++++++ packages/core/src/utils/fetch.ts | 3 +- 30 files changed, 1294 insertions(+), 453 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 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 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 00000000000..b14f62250e5 --- /dev/null +++ b/docs/verification/abort-controller-refactor/README.md @@ -0,0 +1,64 @@ +# 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 +# From repo root, the worktree path: +WT=/Users/jinye.djy/Projects/qwen-code/.claude/worktrees/joyful-honking-melody +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 /Users/jinye.djy/Projects/qwen-code/.claude/worktrees/joyful-honking-melody/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. + +| # | Scenario | Setup | Input prompt | Expected user output | Log file | +| --- | ------------------------------------- | ----------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------ | ---------------------------------------------------------------------------------------------------------------------------------------------------------------- | ---------------------------------------------------------------------------------------- | --------------------- | +| 00 | Baseline (PRE-fix) | Check out `main`, build, run with `NODE_OPTIONS=--trace-warnings` | Long 50-round mixed-tool session (shell + edit + grep + agent) | After ~30-40 rounds: `MaxListenersExceededWarning: ... 1500+ abort listeners added to [AbortSignal]` printed to stderr | `00-baseline-reproduction.log` | +| 01 | Long-session, DEBUG mode | This branch, `NODE_OPTIONS=--trace-warnings DEBUG=1 qwen` | Same 50-round script as #00 | No `MaxListenersExceededWarning` printed; any other warnings still print | `01-long-session-debug.log` | +| 02 | Long-session, prod mode | This branch, `qwen` (no debug env) | Same 50-round script | Clean output; temporary `console.error` probe inside the handler (added then removed) confirms filter fires | `02-long-session-prod.log` | +| 03 | Ctrl-C mid-stream abort | This branch, interactive | Ask for a long generation (>30s). Press Ctrl-C mid-stream. | Stream stops within ~200ms, "Cancelled" banner shown, next prompt accepts input. `process._getActiveHandles()` count returns to baseline (use `:debug handles`). | `03-ctrlc-streaming.log` | +| 04 | Cancel long-running shell | This branch | `Run \`sleep 60\` via the shell tool`. Cancel mid-execution. | Child process killed (`ps -ef | grep sleep` returns nothing), tool result shows cancellation, agent accepts next prompt. | `04-shell-cancel.log` | +| 05 | Subagent cancellation | This branch | Spawn a long agent task via the agent tool. Cancel from parent. | Subagent's in-flight tool calls abort, subagent's model stream stops, parent receives cancellation event. | `05-subagent-cancel.log` | +| 06 | Headless / non-interactive abort | `qwen --prompt "do a long task"`, send `SIGINT` from outside | (sent via `kill -INT `) | Clean shutdown, exit code 130, no warnings. | `06-headless-abort.log` | +| 07 | Background agent flow | Interactive | Spawn a background agent (`run_in_background: true`). Let it complete. Spawn a second one. Cancel the second mid-flight. | First agent completes normally; second aborts cleanly; no listener leak across the two. | `07-background-agent.log` | +| 08 | Memory baseline | `qwen --inspect` + attach Chrome devtools | 100-round session | Heap snapshots at round 0/50/100. `AbortSignal` instance count and per-signal listener count stable (no monotonic growth). | `08-memory-snapshots/` | +| 09 | Existing combinedAbortSignal consumer | Trigger an HTTP hook with both an external signal and timeout. | (a) Cancel external signal mid-hook (b) Let timeout fire in a separate run | Hook aborts cleanly in both cases; deprecation shim path is exercised. | `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`, 18 tests + 1 GC test skipped under non-`--expose-gc`). +- All warningHandler tests pass (`warningHandler.test.ts`, 9 tests). +- Existing combinedAbortSignal tests pass against the deprecation shim (8 tests). +- All agent runtime / followup / openaiContentGenerator / hooks tests pass. +- Migration completeness: `grep -rn "new AbortController" packages/core/src --include="*.ts" | grep -v test | grep -v abortController.ts` returns **empty**. +- 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 00000000000..4ee3c63d246 --- /dev/null +++ b/docs/verification/abort-controller-refactor/automated-results.md @@ -0,0 +1,127 @@ +# 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 completeness + +```sh +$ grep -rn "new AbortController" packages/core/src --include="*.ts" \ + | grep -v test | grep -v abortController.ts +(no output) +``` + +Every production `new AbortController()` call in `packages/core/src` (outside +the helper module itself and outside test files) has been migrated to use +`createAbortController` / `createChildAbortController`. See +`migration-completeness.txt` for the captured grep. + +## 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` — 19 tests: factory cap, child propagation, reverse cleanup, fast path, undefined parent, `combineAbortSignals` semantics, GC safety. +- `packages/cli/src/utils/warningHandler.test.ts` — 9 tests: idempotency, AbortSignal suppression (including `[AbortSignal{...}]` shape), generic EventTarget NOT suppressed, debug-mode passthrough. +- `packages/core/src/hooks/combinedAbortSignal.test.ts` — 8 existing tests pass against the new `@deprecated` shim, proving wire compatibility with `httpHookRunner.ts:202`. +- `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 \ + packages/core/src/hooks/combinedAbortSignal.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 00000000000..2df834e91a4 --- /dev/null +++ b/docs/verification/abort-controller-refactor/listener-accumulation-repro.mjs @@ -0,0 +1,109 @@ +#!/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 so this script has no build-step +// dependency on the @qwen-code/qwen-code-core package. +function createAbortController(maxListeners = 50) { + const c = new AbortController(); + setMaxListeners(maxListeners, c.signal); + return c; +} +function propagateAbort(weakChild) { + const parent = this.deref(); + weakChild.deref()?.abort(parent?.reason); +} +function removeAbortHandler(weakHandler) { + const parent = this.deref(); + const handler = weakHandler.deref(); + if (parent && handler) parent.removeEventListener('abort', handler); +} +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 weakChild = new WeakRef(child); + const weakParent = new WeakRef(parentSignal); + const handler = propagateAbort.bind(weakParent, weakChild); + parentSignal.addEventListener('abort', handler, { once: true }); + child.signal.addEventListener( + 'abort', + removeAbortHandler.bind(weakParent, new WeakRef(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 00000000000..e69de29bb2d 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 00000000000..988affd9f50 --- /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 00000000000..8d49c07c975 --- /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/packages/cli/src/gemini.tsx b/packages/cli/src/gemini.tsx index b8da6949822..b1c249cf400 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 { computeWindowTitle } from './utils/windowTitle.js'; import { @@ -402,6 +403,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 00000000000..a4b2c89adc3 --- /dev/null +++ b/packages/cli/src/utils/warningHandler.test.ts @@ -0,0 +1,153 @@ +/** + * @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[] = []; + // Spy on stderr.write; typed loosely because the overloads aren't worth + // re-stating just to satisfy the strict generic of vi.spyOn's return type. + // eslint-disable-next-line @typescript-eslint/no-explicit-any + let stderrSpy: any; + + 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(); + stderrSpy = vi + .spyOn(process.stderr, 'write') + .mockImplementation(() => true); + }); + + 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]; + } + stderrSpy.mockRestore(); + }); + + function makeWarning(name: string, message: string): Error { + const err = new Error(message); + err.name = name; + return err; + } + + function emit(warning: Error): void { + for (const l of process.listeners('warning')) { + (l as (w: Error) => void)(warning); + } + } + + it('suppresses MaxListenersExceededWarning for AbortSignal in production', () => { + initializeWarningHandler(); + emit( + makeWarning( + 'MaxListenersExceededWarning', + 'Possible EventTarget memory leak detected. 1509 abort listeners added to [AbortSignal].', + ), + ); + expect(stderrSpy).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(stderrSpy).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(stderrSpy).not.toHaveBeenCalled(); + }); + + it('forwards unrelated warnings to stderr', () => { + initializeWarningHandler(); + emit(makeWarning('DeprecationWarning', 'Some legacy thing')); + expect(stderrSpy).toHaveBeenCalledTimes(1); + const written = (stderrSpy.mock.calls[0]?.[0] ?? '').toString(); + expect(written).toContain('DeprecationWarning'); + expect(written).toContain('Some legacy thing'); + }); + + 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(stderrSpy).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(stderrSpy).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(stderrSpy).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(stderrSpy).toHaveBeenCalledTimes(1); + }); + + it('is idempotent — repeated calls install only one listener', () => { + initializeWarningHandler(); + initializeWarningHandler(); + initializeWarningHandler(); + expect(process.listeners('warning').length).toBe(1); + }); +}); diff --git a/packages/cli/src/utils/warningHandler.ts b/packages/cli/src/utils/warningHandler.ts new file mode 100644 index 00000000000..7ba3fec7f7d --- /dev/null +++ b/packages/cli/src/utils/warningHandler.ts @@ -0,0 +1,72 @@ +/** + * @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 / EventTarget while letting + * every other warning through. In debug mode (NODE_ENV=development, or + * DEBUG / QWEN_DEBUG set), all warnings are forwarded to stderr so + * developers can still see them. + * + * Idempotent — repeated calls are a no-op. + */ +export function initializeWarningHandler(): void { + if (installedHandler) return; + + const debug = isDebugMode(); + + installedHandler = (warning: Error) => { + if (!debug && isSuppressed(warning)) return; + const text = warning.stack ?? `${warning.name}: ${warning.message}`; + process.stderr.write(`(node) ${text}\n`); + }; + + // Adding a listener (instead of removeAllListeners) leaves any third-party + // warning subscribers in place. Node's default printer only fires when + // there are zero listeners — so installing our handler implicitly disables + // the default print path, and we take over forwarding to stderr for the + // non-suppressed cases. + process.on('warning', installedHandler); +} diff --git a/packages/core/src/agents/arena/ArenaManager.ts b/packages/core/src/agents/arena/ArenaManager.ts index 382c02cc24d..3c0737d8eba 100644 --- a/packages/core/src/agents/arena/ArenaManager.ts +++ b/packages/core/src/agents/arena/ArenaManager.ts @@ -10,6 +10,10 @@ import { GitWorktreeService } from '../../services/gitWorktreeService.js'; import { Storage } from '../../config/storage.js'; import type { Config } from '../../config/config.js'; import { getCoreSystemPrompt } from '../../core/prompts.js'; +import { + createAbortController, + createChildAbortController, +} from '../../utils/abortController.js'; import { createDebugLogger } from '../../utils/debugLogger.js'; import { isNodeError } from '../../utils/errors.js'; import { atomicWriteJSON } from '../../utils/atomicFileWrite.js'; @@ -302,7 +306,7 @@ export class ArenaManager { this.worktreeDirName = await this.deriveWorktreeDirName(this.sessionId); this.startedAt = Date.now(); this.sessionStatus = ArenaSessionStatus.INITIALIZING; - this.masterAbortController = new AbortController(); + this.masterAbortController = createAbortController(); const sourceRepoPath = this.config.getWorkingDir(); const arenaSettings = this.config.getAgentsSettings().arena; @@ -814,7 +818,7 @@ export class ArenaManager { model, status: AgentStatus.INITIALIZING, worktree, - abortController: new AbortController(), + abortController: createChildAbortController(this.masterAbortController), agentSessionId: `${this.sessionId}#${agentId}`, stats: { rounds: 0, diff --git a/packages/core/src/agents/background-agent-resume.ts b/packages/core/src/agents/background-agent-resume.ts index a4906f24013..4808e37f8f7 100644 --- a/packages/core/src/agents/background-agent-resume.ts +++ b/packages/core/src/agents/background-agent-resume.ts @@ -9,6 +9,7 @@ import * as path from 'node:path'; import type { Content, FunctionDeclaration, Part } from '@google/genai'; import type { Config } from '../config/config.js'; import * as jsonl from '../utils/jsonl-utils.js'; +import { createAbortController } from '../utils/abortController.js'; import { createDebugLogger } from '../utils/debugLogger.js'; import { AgentEventEmitter, @@ -407,7 +408,7 @@ export class BackgroundAgentResumeService { startTime: Number.isFinite(parsedStartTime) ? parsedStartTime : Date.now(), - abortController: new AbortController(), + abortController: createAbortController(), prompt: recovery.initialPrompt, outputFile, metaPath, @@ -479,7 +480,7 @@ export class BackgroundAgentResumeService { return undefined; } - const bgAbortController = new AbortController(); + const bgAbortController = createAbortController(); registry.register({ ...existing, @@ -899,7 +900,7 @@ export class BackgroundAgentResumeService { const pausedEntry: BackgroundTaskEntry = { ...latest, status: 'paused', - abortController: new AbortController(), + abortController: createAbortController(), endTime: undefined, result: undefined, error: options.error, diff --git a/packages/core/src/agents/runtime/agent-core.ts b/packages/core/src/agents/runtime/agent-core.ts index d0c7704ce11..781ea07e88e 100644 --- a/packages/core/src/agents/runtime/agent-core.ts +++ b/packages/core/src/agents/runtime/agent-core.ts @@ -16,6 +16,7 @@ * and how to interpret the results. */ +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'; @@ -586,252 +587,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(); - } - - const promptId = `${this.runtimeContext.getSessionId()}#${this.subagentId}#${turnCounter++}`; - - const messageParams = { - message: currentMessages[0]?.parts || [], - config: { - abortSignal: roundAbortController.signal, - tools: [{ functionDeclarations: toolsList }], - }, - }; + // 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 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; - } + try { + const promptId = `${this.runtimeContext.getSessionId()}#${this.subagentId}#${turnCounter++}`; - // 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; - } - } + const messageParams = { + message: currentMessages[0]?.parts || [], + config: { + abortSignal: roundAbortController.signal, + tools: [{ functionDeclarations: toolsList }], + }, + }; - 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, + promptId, 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; - } + } 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) { + return { + text: finalText, + terminateMode: AgentTerminateMode.CANCELLED, + turnsUsed: turnCounter, + }; + } - // Update token usage if available - if (lastUsage) { - this.recordTokenUsage(lastUsage, turnCounter, roundStreamStart); - } + // 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 (functionCalls.length > 0) { - currentMessages = await this.processFunctionCalls( - functionCalls, - roundAbortController, - promptId, - turnCounter, - toolsList, - currentResponseId, - wasOutputTruncated, - ); + // 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; + } - 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); + // 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; + } } - } 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, { + + if (roundText || roundThoughtText) { + this.eventEmitter?.emit(AgentEventType.ROUND_TEXT, { subagentId: this.subagentId, round: turnCounter, - promptId, + text: roundText, + thoughtText: roundThoughtText, timestamp: Date.now(), - } as AgentRoundEvent); - abortController.signal.removeEventListener('abort', onParentAbort); + } as AgentRoundTextEvent); + } + + this.executionStats.rounds = turnCounter; + this.stats.setRounds(turnCounter); - const waitResult = await this.waitForExternalInputs( - options, - abortController, - startTime, + 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, ); - 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; + 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); - // Clean up the per-round listener before the next iteration - abortController.signal.removeEventListener('abort', onParentAbort); + 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(); + } } return { @@ -938,12 +940,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) { @@ -980,7 +977,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(); } } } diff --git a/packages/core/src/agents/runtime/agent-headless.ts b/packages/core/src/agents/runtime/agent-headless.ts index dd77e11defb..33f96de748b 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); - - // 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 { - 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', + 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); + + 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); - - 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(), - }); + timestamp: Date.now(), + } as AgentStartEvent); + + // 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 b7fbba1df06..08744e42893 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 e8a737f82e4..97ac334776c 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/client.ts b/packages/core/src/core/client.ts index 65dc9fe395f..4ce56b78e4c 100644 --- a/packages/core/src/core/client.ts +++ b/packages/core/src/core/client.ts @@ -16,6 +16,7 @@ import { SpanStatusCode } from '@opentelemetry/api'; // Config import { ApprovalMode, type Config } from '../config/config.js'; +import { createAbortController } from '../utils/abortController.js'; import { createDebugLogger } from '../utils/debugLogger.js'; import { recordStartupEvent } from '../utils/startupEventSink.js'; import { microcompactHistory } from '../services/microcompaction/microcompact.js'; @@ -970,7 +971,7 @@ export class GeminiClient { messageType === SendMessageType.Cron ) { if (this.config.getManagedAutoMemoryEnabled()) { - const recallAbortController = new AbortController(); + const recallAbortController = createAbortController(); const rawRecallPromise = this.config .getMemoryManager() .recall(this.config.getProjectRoot(), partToString(request), { diff --git a/packages/core/src/core/openaiContentGenerator/pipeline.ts b/packages/core/src/core/openaiContentGenerator/pipeline.ts index c814527d610..579645cadcb 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, @@ -19,23 +18,6 @@ import { TaggedThinkingParser } from './taggedThinkingParser.js'; import type { PipelineConfig, RequestContext } from './types.js'; import { redactProxyError } from '../../utils/runtimeFetchOptions.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 @@ -64,7 +46,6 @@ export class ContentGenerationPipeline { request: GenerateContentParameters, userPromptId: string, ): Promise { - raiseAbortListenerCap(request.config?.abortSignal); return this.executeWithErrorHandling( request, userPromptId, @@ -92,7 +73,6 @@ export class ContentGenerationPipeline { request: GenerateContentParameters, userPromptId: string, ): Promise> { - raiseAbortListenerCap(request.config?.abortSignal); return this.executeWithErrorHandling( request, userPromptId, diff --git a/packages/core/src/followup/speculation.ts b/packages/core/src/followup/speculation.ts index f26d7c9cb12..297c4730957 100644 --- a/packages/core/src/followup/speculation.ts +++ b/packages/core/src/followup/speculation.ts @@ -26,6 +26,7 @@ import { createForkedChat, runForkedAgent, } from '../utils/forkedAgent.js'; +import { createChildAbortController } from '../utils/abortController.js'; import { getFilterReason, SUGGESTION_PROMPT } from './suggestionGenerator.js'; // --------------------------------------------------------------------------- @@ -96,7 +97,7 @@ export async function startSpeculation( throw new Error('CacheSafeParams not available for speculation'); } - const abortController = new AbortController(); + const abortController = createChildAbortController(parentSignal); // If parent was already aborted, return aborted state without starting loop if (parentSignal?.aborted) { @@ -112,13 +113,6 @@ export async function startSpeculation( }; } - // Link to parent signal with cleanup to prevent memory leak (#20) - let parentAbortHandler: (() => void) | undefined; - if (parentSignal) { - parentAbortHandler = () => abortController.abort(); - parentSignal.addEventListener('abort', parentAbortHandler, { once: true }); - } - const overlayFs = new OverlayFs(config.getCwd()); const startTime = Date.now(); @@ -176,10 +170,8 @@ export async function startSpeculation( await overlayFs.cleanup(); }) .finally(() => { - // Clean up parent signal listener (#20) - if (parentSignal && parentAbortHandler) { - parentSignal.removeEventListener('abort', parentAbortHandler); - } + // Reverse-cleanup detaches the parent-signal listener (#20). + abortController.abort(); }); return state; diff --git a/packages/core/src/hooks/combinedAbortSignal.ts b/packages/core/src/hooks/combinedAbortSignal.ts index dfcdf923f67..bf3c7501dcd 100644 --- a/packages/core/src/hooks/combinedAbortSignal.ts +++ b/packages/core/src/hooks/combinedAbortSignal.ts @@ -4,54 +4,18 @@ * SPDX-License-Identifier: Apache-2.0 */ +import { combineAbortSignals } from '../utils/abortController.js'; + /** - * 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 + * @deprecated Use {@link combineAbortSignals} from `utils/abortController.js`. + * Thin wrapper preserved so existing callers (httpHookRunner) keep working + * during the abort-controller helper rollout. */ 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 }; + return combineAbortSignals([externalSignal], { + timeoutMs: options?.timeoutMs, + }); } diff --git a/packages/core/src/hooks/functionHookRunner.ts b/packages/core/src/hooks/functionHookRunner.ts index badcd344c1e..e2033d6f1fe 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 33789766749..93fedec70fb 100644 --- a/packages/core/src/hooks/hookRunner.ts +++ b/packages/core/src/hooks/hookRunner.ts @@ -593,7 +593,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/memory/manager.ts b/packages/core/src/memory/manager.ts index ca0bb5aa27e..98eb36d74f8 100644 --- a/packages/core/src/memory/manager.ts +++ b/packages/core/src/memory/manager.ts @@ -39,6 +39,7 @@ import { randomUUID } from 'node:crypto'; import type { Content, Part } from '@google/genai'; import type { Config } from '../config/config.js'; import { Storage } from '../config/storage.js'; +import { createAbortController } from '../utils/abortController.js'; import { createDebugLogger } from '../utils/debugLogger.js'; import { logMemoryDream, @@ -933,7 +934,7 @@ export class MemoryManager { // missing-controller defensive warn-and-return-false path and the // model gets a phantom failure on a brand-new dream. Registering // first means any reentrant cancel sees a complete state. - const abortController = new AbortController(); + const abortController = createAbortController(); this.dreamAbortControllers.set(record.id, abortController); this.dreamInFlightByKey.set(dedupeKey, record.id); this.storeWith(record, { diff --git a/packages/core/src/services/chatCompressionService.ts b/packages/core/src/services/chatCompressionService.ts index a8f310f0309..9c02cc7dfb6 100644 --- a/packages/core/src/services/chatCompressionService.ts +++ b/packages/core/src/services/chatCompressionService.ts @@ -10,6 +10,7 @@ import type { GeminiChat } from '../core/geminiChat.js'; import { type ChatCompressionInfo, CompressionStatus } from '../core/turn.js'; import { DEFAULT_TOKEN_LIMIT } from '../core/tokenLimits.js'; import { getCompressionPrompt } from '../core/prompts.js'; +import { createAbortController } from '../utils/abortController.js'; import { runSideQuery } from '../utils/sideQuery.js'; import { logChatCompression } from '../telemetry/loggers.js'; import { makeChatCompressionEvent } from '../telemetry/types.js'; @@ -375,7 +376,7 @@ export class ChatCompressionService { config: { thinkingConfig: { includeThoughts: true }, }, - abortSignal: signal ?? new AbortController().signal, + abortSignal: signal ?? createAbortController().signal, promptId, }); const summary = summaryResult.text; diff --git a/packages/core/src/services/chatRecordingService.ts b/packages/core/src/services/chatRecordingService.ts index aecbb1e900f..d749e4f3b83 100644 --- a/packages/core/src/services/chatRecordingService.ts +++ b/packages/core/src/services/chatRecordingService.ts @@ -17,6 +17,7 @@ import { createModelContent, } from '@google/genai'; import * as jsonl from '../utils/jsonl-utils.js'; +import { createAbortController } from '../utils/abortController.js'; import { getGitBranch } from '../utils/gitUtils.js'; import { createDebugLogger } from '../utils/debugLogger.js'; import type { AttributionSnapshot } from './commitAttribution.js'; @@ -935,7 +936,7 @@ export class ChatRecordingService { if (!this.config.getFastModel()) return; this.autoTitleAttempts++; - const controller = new AbortController(); + const controller = createAbortController(); this.autoTitleController = controller; void (async () => { diff --git a/packages/core/src/tools/agent/agent.ts b/packages/core/src/tools/agent/agent.ts index c511cc276d3..6d0bce0cba8 100644 --- a/packages/core/src/tools/agent/agent.ts +++ b/packages/core/src/tools/agent/agent.ts @@ -58,6 +58,10 @@ import type { AgentUsageEvent, } from '../../agents/runtime/agent-events.js'; import { BuiltinAgentRegistry } from '../../subagents/builtin-agents.js'; +import { + createAbortController, + createChildAbortController, +} from '../../utils/abortController.js'; import { createDebugLogger } from '../../utils/debugLogger.js'; import { PermissionMode } from '../../hooks/types.js'; import type { StopHookOutput } from '../../hooks/types.js'; @@ -1226,9 +1230,9 @@ class AgentToolInvocation extends BaseToolInvocation { } } - // Create an independent AbortController — background agents - // survive ESC cancellation of the parent's current turn. - const bgAbortController = new AbortController(); + // Independent AbortController — background agents survive ESC + // cancellation of the parent's current turn, so no parent linkage. + const bgAbortController = createAbortController(); // Background agents have no UI, so interactive permission prompts must be // auto-denied rather than auto-approved (YOLO). PermissionRequest hooks @@ -1579,17 +1583,10 @@ class AgentToolInvocation extends BaseToolInvocation { } // ── Foreground (synchronous) execution path ──────────────── - // Compose a child AbortController so the dialog's per-agent cancel - // can abort just this subagent without aborting the parent turn. - // Parent abort still propagates down (so ESC at the parent kills - // the subagent), but child abort does NOT propagate up. - const fgAbortController = new AbortController(); - const onParentAbort = () => fgAbortController.abort(); - if (signal?.aborted) { - fgAbortController.abort(); - } else { - signal?.addEventListener('abort', onParentAbort, { once: true }); - } + // Child AbortController so the dialog's per-agent cancel aborts only + // this subagent. Parent abort still propagates down (ESC at the parent + // kills the subagent); child abort does NOT propagate up. + const fgAbortController = createChildAbortController(signal); const fgHookOpts = { ...hookOpts, signal: fgAbortController.signal }; const runFramed = () => @@ -1706,7 +1703,8 @@ class AgentToolInvocation extends BaseToolInvocation { } finally { this.eventEmitter.off(AgentEventType.TOOL_CALL, onFgToolCall); this.eventEmitter.off(AgentEventType.USAGE_METADATA, onFgUsageMetadata); - signal?.removeEventListener('abort', onParentAbort); + // Reverse cleanup detaches our parent-signal listener. + fgAbortController.abort(); cleanupOwnedMonitorNotifications(); // Foreground entries leave the registry as soon as the tool-call // returns — the parent's tool-result is the durable record. Doing diff --git a/packages/core/src/tools/monitor.ts b/packages/core/src/tools/monitor.ts index 1311d804013..c92fd6bddf5 100644 --- a/packages/core/src/tools/monitor.ts +++ b/packages/core/src/tools/monitor.ts @@ -41,6 +41,7 @@ import { normalizeMonitorCommand as normalizeMonitorShellCommand, splitCommands, } from '../utils/shell-utils.js'; +import { createAbortController } from '../utils/abortController.js'; import { createDebugLogger } from '../utils/debugLogger.js'; import { isSubpaths } from '../utils/paths.js'; import type { MonitorEntry } from '../services/monitorRegistry.js'; @@ -300,7 +301,7 @@ class MonitorToolInvocation extends BaseToolInvocation< // Independent AbortController — pressing Ctrl+C on the current turn // should NOT kill a long-running monitor the user intentionally started. - const entryAc = new AbortController(); + const entryAc = createAbortController(); const entry: MonitorEntry = { monitorId, diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts index 1a05ef27e4c..d413a1ca913 100644 --- a/packages/core/src/tools/shell.ts +++ b/packages/core/src/tools/shell.ts @@ -49,6 +49,7 @@ import { stripShellWrapper, } from '../utils/shell-utils.js'; import { parse } from 'shell-quote'; +import { createAbortController } from '../utils/abortController.js'; import { createDebugLogger } from '../utils/debugLogger.js'; import { isShellCommandReadOnlyAST, @@ -1449,7 +1450,7 @@ export class ShellToolInvocation extends BaseToolInvocation< // ShellExecutionService detects the discriminated reason and // returns `result.promoted: true` instead of killing the child — // see #3842 / #3886 for the foundation. - const promoteAbortController = new AbortController(); + const promoteAbortController = createAbortController(); let combinedSignal = AbortSignal.any([ signal, promoteAbortController.signal, @@ -2134,7 +2135,7 @@ export class ShellToolInvocation extends BaseToolInvocation< // SIGTERM → SIGKILL ourselves (mirroring the kill semantics // `ShellExecutionService.execute()`'s abort handler uses for the // non-promote path) and to mark the registry entry `cancelled`. - const entryAc = new AbortController(); + const entryAc = createAbortController(); const cancelChild = async () => { const pid = result.pid; if (pid !== undefined) { @@ -2341,7 +2342,7 @@ export class ShellToolInvocation extends BaseToolInvocation< // The `signal` parameter is still honored for the synchronous early // return below (don't even spawn if the agent already aborted), but // we deliberately do not forward it. - const entryAc = new AbortController(); + const entryAc = createAbortController(); const outputStream = fs.createWriteStream(outputPath, { flags: 'w' }); // Without an 'error' listener, a write failure (disk full, permission diff --git a/packages/core/src/utils/abortController.test.ts b/packages/core/src/utils/abortController.test.ts new file mode 100644 index 00000000000..7d9e5bafbc8 --- /dev/null +++ b/packages/core/src/utils/abortController.test.ts @@ -0,0 +1,193 @@ +/** + * @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); + }); +}); + +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('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('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('GC safety (best-effort, requires --expose-gc)', () => { + const maybeGc = (globalThis as { gc?: () => void }).gc; + const itGc = maybeGc ? it : it.skip; + + itGc( + 'does not retain abandoned children through the parent listener', + async () => { + const parent = createAbortController(); + let weakChild: WeakRef; + (() => { + const child = createChildAbortController(parent); + weakChild = new WeakRef(child); + })(); + // Yield to allow finalizers; then GC. + await new Promise((r) => setTimeout(r, 0)); + maybeGc!(); + await new Promise((r) => setTimeout(r, 0)); + maybeGc!(); + expect(weakChild!.deref()).toBeUndefined(); + // Firing parent now should not throw (handler dereferences a dead WeakRef). + expect(() => parent.abort()).not.toThrow(); + }, + ); +}); diff --git a/packages/core/src/utils/abortController.ts b/packages/core/src/utils/abortController.ts new file mode 100644 index 00000000000..1a5d9bea45e --- /dev/null +++ b/packages/core/src/utils/abortController.ts @@ -0,0 +1,163 @@ +/** + * @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; +} + +/** + * Propagate abort from a weakly-referenced parent to a weakly-referenced child. + * Module-scope (not a per-call closure) to keep allocation cheap. + */ +function propagateAbort( + this: WeakRef, + weakChild: WeakRef, +): void { + const parent = this.deref(); + weakChild.deref()?.abort(parent?.reason); +} + +/** + * Remove an abort handler from a weakly-referenced parent signal. + * No-op if either side has been GC'd or the parent already fired (`{once:true}`). + */ +function removeAbortHandler( + this: WeakRef, + weakHandler: WeakRef<(...args: unknown[]) => void>, +): void { + const parent = this.deref(); + const handler = weakHandler.deref(); + if (parent && handler) { + parent.removeEventListener('abort', handler); + } +} + +/** + * Create a child AbortController that aborts when its parent aborts. + * Aborting the child does NOT abort the parent. + * + * Three invariants make this safe under long-running parents with many + * short-lived children: + * - 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. + * - Both parent and child are held via `WeakRef`, so the parent does not + * strongly retain abandoned children; a child that is dropped without + * being aborted can still be GC'd. + * + * Caveat: if the parent controller is held ONLY through the child's WeakRef, + * it can be GC'd before its `abort` fires. In practice every parent in this + * codebase is held strongly by a long-lived owner (e.g. `this.master...`) + * that outlives the child, so this is safe. + * + * 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; + } + + const weakChild = new WeakRef(child); + const weakParent = new WeakRef(parentSignal); + const handler = propagateAbort.bind(weakParent, weakChild); + + parentSignal.addEventListener('abort', handler, { once: true }); + + child.signal.addEventListener( + 'abort', + removeAbortHandler.bind(weakParent, new WeakRef(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; + const handler = () => controller.abort(sourceSignal.reason); + sourceSignal.addEventListener('abort', handler, { once: true }); + cleanups.push(() => sourceSignal.removeEventListener('abort', handler)); + } + + const timeoutMs = options?.timeoutMs; + if (timeoutMs !== undefined && timeoutMs > 0) { + 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(); + }; + + controller.signal.addEventListener('abort', cleanup, { once: true }); + + return { signal: controller.signal, cleanup }; +} diff --git a/packages/core/src/utils/fetch.ts b/packages/core/src/utils/fetch.ts index eb358a8f3fd..cd97df5c2b6 100644 --- a/packages/core/src/utils/fetch.ts +++ b/packages/core/src/utils/fetch.ts @@ -4,6 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ +import { createAbortController } from './abortController.js'; import { getErrorMessage, isNodeError } from './errors.js'; import { URL } from 'node:url'; @@ -61,7 +62,7 @@ export async function fetchWithTimeout( timeout: number, headers?: Record, ): Promise { - const controller = new AbortController(); + const controller = createAbortController(); const timeoutId = setTimeout(() => controller.abort(), timeout); try { From aff30bc1b2a76cbc62d427024f8a0bcf3d26bb2e Mon Sep 17 00:00:00 2001 From: doudouOUC Date: Thu, 21 May 2026 08:00:04 +0800 Subject: [PATCH 02/15] fix(core): address PR #4366 review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../abort-controller-refactor/README.md | 86 +++++++++++++++---- packages/cli/src/utils/warningHandler.ts | 9 +- .../core/src/utils/abortController.test.ts | 22 +++++ packages/core/src/utils/abortController.ts | 8 ++ 4 files changed, 106 insertions(+), 19 deletions(-) diff --git a/docs/verification/abort-controller-refactor/README.md b/docs/verification/abort-controller-refactor/README.md index b14f62250e5..3d14b6f14f3 100644 --- a/docs/verification/abort-controller-refactor/README.md +++ b/docs/verification/abort-controller-refactor/README.md @@ -6,8 +6,8 @@ scenario captures its tmux pane via `tmux pipe-pane -o 'cat >> '`. ## Setup once ```sh -# From repo root, the worktree path: -WT=/Users/jinye.djy/Projects/qwen-code/.claude/worktrees/joyful-honking-melody +# 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" @@ -22,25 +22,81 @@ 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 /Users/jinye.djy/Projects/qwen-code/.claude/worktrees/joyful-honking-melody/packages/cli/dist/index.js' C-m +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. -| # | Scenario | Setup | Input prompt | Expected user output | Log file | -| --- | ------------------------------------- | ----------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------ | ---------------------------------------------------------------------------------------------------------------------------------------------------------------- | ---------------------------------------------------------------------------------------- | --------------------- | -| 00 | Baseline (PRE-fix) | Check out `main`, build, run with `NODE_OPTIONS=--trace-warnings` | Long 50-round mixed-tool session (shell + edit + grep + agent) | After ~30-40 rounds: `MaxListenersExceededWarning: ... 1500+ abort listeners added to [AbortSignal]` printed to stderr | `00-baseline-reproduction.log` | -| 01 | Long-session, DEBUG mode | This branch, `NODE_OPTIONS=--trace-warnings DEBUG=1 qwen` | Same 50-round script as #00 | No `MaxListenersExceededWarning` printed; any other warnings still print | `01-long-session-debug.log` | -| 02 | Long-session, prod mode | This branch, `qwen` (no debug env) | Same 50-round script | Clean output; temporary `console.error` probe inside the handler (added then removed) confirms filter fires | `02-long-session-prod.log` | -| 03 | Ctrl-C mid-stream abort | This branch, interactive | Ask for a long generation (>30s). Press Ctrl-C mid-stream. | Stream stops within ~200ms, "Cancelled" banner shown, next prompt accepts input. `process._getActiveHandles()` count returns to baseline (use `:debug handles`). | `03-ctrlc-streaming.log` | -| 04 | Cancel long-running shell | This branch | `Run \`sleep 60\` via the shell tool`. Cancel mid-execution. | Child process killed (`ps -ef | grep sleep` returns nothing), tool result shows cancellation, agent accepts next prompt. | `04-shell-cancel.log` | -| 05 | Subagent cancellation | This branch | Spawn a long agent task via the agent tool. Cancel from parent. | Subagent's in-flight tool calls abort, subagent's model stream stops, parent receives cancellation event. | `05-subagent-cancel.log` | -| 06 | Headless / non-interactive abort | `qwen --prompt "do a long task"`, send `SIGINT` from outside | (sent via `kill -INT `) | Clean shutdown, exit code 130, no warnings. | `06-headless-abort.log` | -| 07 | Background agent flow | Interactive | Spawn a background agent (`run_in_background: true`). Let it complete. Spawn a second one. Cancel the second mid-flight. | First agent completes normally; second aborts cleanly; no listener leak across the two. | `07-background-agent.log` | -| 08 | Memory baseline | `qwen --inspect` + attach Chrome devtools | 100-round session | Heap snapshots at round 0/50/100. `AbortSignal` instance count and per-signal listener count stable (no monotonic growth). | `08-memory-snapshots/` | -| 09 | Existing combinedAbortSignal consumer | Trigger an HTTP hook with both an external signal and timeout. | (a) Cancel external signal mid-hook (b) Let timeout fire in a separate run | Hook aborts cleanly in both cases; deprecation shim path is exercised. | `09-http-hook-shim.log` | +### 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 diff --git a/packages/cli/src/utils/warningHandler.ts b/packages/cli/src/utils/warningHandler.ts index 7ba3fec7f7d..564e6af76ba 100644 --- a/packages/cli/src/utils/warningHandler.ts +++ b/packages/cli/src/utils/warningHandler.ts @@ -45,10 +45,11 @@ export function resetWarningHandlerForTests(): void { /** * Install a process-level `warning` handler that swallows the well-known - * `MaxListenersExceededWarning` for AbortSignal / EventTarget while letting - * every other warning through. In debug mode (NODE_ENV=development, or - * DEBUG / QWEN_DEBUG set), all warnings are forwarded to stderr so - * developers can still see them. + * `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 to stderr so developers can still see them. * * Idempotent — repeated calls are a no-op. */ diff --git a/packages/core/src/utils/abortController.test.ts b/packages/core/src/utils/abortController.test.ts index 7d9e5bafbc8..2c70ffbce2a 100644 --- a/packages/core/src/utils/abortController.test.ts +++ b/packages/core/src/utils/abortController.test.ts @@ -155,6 +155,28 @@ describe('combineAbortSignals', () => { expect(() => cleanup()).not.toThrow(); }); + 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 flips to true after the initial + // alreadyAborted scan — the second pass through the loop should catch it + // and short-circuit instead of attaching a listener that never fires. + let aborted = false; + const proxied = new Proxy(b.signal, { + get(target, prop, recv) { + if (prop === 'aborted') return aborted; + return Reflect.get(target, prop, recv); + }, + }) as AbortSignal; + // Flip the proxy "aborted" right after the initial find() pass. + aborted = true; + const { signal } = combineAbortSignals([a.signal, proxied, c.signal]); + expect(signal.aborted).toBe(true); + // c should never have had a listener attached (we broke out of the loop). + expect(getEventListeners(c.signal, 'abort').length).toBe(0); + }); + it('auto-cleans listeners on inputs when the combined signal aborts', () => { const a = createAbortController(); const b = createAbortController(); diff --git a/packages/core/src/utils/abortController.ts b/packages/core/src/utils/abortController.ts index 1a5d9bea45e..b59863d73ba 100644 --- a/packages/core/src/utils/abortController.ts +++ b/packages/core/src/utils/abortController.ts @@ -137,6 +137,14 @@ export function combineAbortSignals( 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)); From 8e42dc73ed082a122925dbc178652280d74b4352 Mon Sep 17 00:00:00 2001 From: doudouOUC Date: Thu, 21 May 2026 08:07:16 +0800 Subject: [PATCH 03/15] test(core): fix abortController race-defense test to actually hit the loop check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../core/src/utils/abortController.test.ts | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/packages/core/src/utils/abortController.test.ts b/packages/core/src/utils/abortController.test.ts index 2c70ffbce2a..aab7bca80d1 100644 --- a/packages/core/src/utils/abortController.test.ts +++ b/packages/core/src/utils/abortController.test.ts @@ -159,21 +159,23 @@ describe('combineAbortSignals', () => { const a = createAbortController(); const b = createAbortController(); const c = createAbortController(); - // Simulate a signal whose `aborted` getter flips to true after the initial - // alreadyAborted scan — the second pass through the loop should catch it - // and short-circuit instead of attaching a listener that never fires. - let aborted = false; + // 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') return aborted; + if (prop === 'aborted') { + accessCount++; + return accessCount > 1; // false on first access, true thereafter + } return Reflect.get(target, prop, recv); }, }) as AbortSignal; - // Flip the proxy "aborted" right after the initial find() pass. - aborted = true; 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); - // c should never have had a listener attached (we broke out of the loop). expect(getEventListeners(c.signal, 'abort').length).toBe(0); }); From 082db37a825f7f40bf6bda8d72baa8fe52a63e82 Mon Sep 17 00:00:00 2001 From: doudouOUC Date: Thu, 21 May 2026 10:23:07 +0800 Subject: [PATCH 04/15] fix(lint): include docs/**/*.mjs in the script ESLint block so the AbortController repro passes lint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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/. --- eslint.config.js | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/eslint.config.js b/eslint.config.js index ea31e0f1ec7..4d6fcaef87f 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, From 832355e9f6c3b5160f29715e6a9bef38f52d39de Mon Sep 17 00:00:00 2001 From: doudouOUC Date: Thu, 21 May 2026 11:22:59 +0800 Subject: [PATCH 05/15] fix(core,cli): address PR #4366 critical review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- packages/cli/src/utils/warningHandler.test.ts | 121 ++++++++++++++---- packages/cli/src/utils/warningHandler.ts | 34 +++-- .../core/src/utils/abortController.test.ts | 57 ++++++--- packages/core/src/utils/abortController.ts | 62 ++++----- 4 files changed, 184 insertions(+), 90 deletions(-) diff --git a/packages/cli/src/utils/warningHandler.test.ts b/packages/cli/src/utils/warningHandler.test.ts index a4b2c89adc3..46cafb65965 100644 --- a/packages/cli/src/utils/warningHandler.test.ts +++ b/packages/cli/src/utils/warningHandler.test.ts @@ -15,10 +15,12 @@ const ENV_KEYS = ['NODE_ENV', 'DEBUG', 'QWEN_DEBUG'] as const; describe('initializeWarningHandler', () => { const originalEnv: Partial> = {}; let originalListeners: NodeJS.WarningListener[] = []; - // Spy on stderr.write; typed loosely because the overloads aren't worth - // re-stating just to satisfy the strict generic of vi.spyOn's return type. - // eslint-disable-next-line @typescript-eslint/no-explicit-any - let stderrSpy: any; + // 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]; @@ -26,9 +28,8 @@ describe('initializeWarningHandler', () => { originalListeners = [...process.listeners('warning')]; process.removeAllListeners('warning'); resetWarningHandlerForTests(); - stderrSpy = vi - .spyOn(process.stderr, 'write') - .mockImplementation(() => true); + priorListener = vi.fn(); + process.on('warning', priorListener); }); afterEach(() => { @@ -39,7 +40,6 @@ describe('initializeWarningHandler', () => { if (originalEnv[k] === undefined) delete process.env[k]; else process.env[k] = originalEnv[k]; } - stderrSpy.mockRestore(); }); function makeWarning(name: string, message: string): Error { @@ -49,9 +49,11 @@ describe('initializeWarningHandler', () => { } function emit(warning: Error): void { - for (const l of process.listeners('warning')) { - (l as (w: Error) => void)(warning); - } + // 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', () => { @@ -62,7 +64,7 @@ describe('initializeWarningHandler', () => { 'Possible EventTarget memory leak detected. 1509 abort listeners added to [AbortSignal].', ), ); - expect(stderrSpy).not.toHaveBeenCalled(); + expect(priorListener).not.toHaveBeenCalled(); }); it('does NOT suppress generic [EventTarget] warnings — only AbortSignal', () => { @@ -73,7 +75,7 @@ describe('initializeWarningHandler', () => { 'Possible EventTarget memory leak detected. 11 listeners added to [EventTarget].', ), ); - expect(stderrSpy).toHaveBeenCalledTimes(1); + expect(priorListener).toHaveBeenCalledTimes(1); }); it('suppresses AbortSignal warnings with class metadata, e.g. [AbortSignal{aborted: false}]', () => { @@ -84,16 +86,37 @@ describe('initializeWarningHandler', () => { 'Possible EventTarget memory leak detected. 11 abort listeners added to [AbortSignal{aborted: false}].', ), ); - expect(stderrSpy).not.toHaveBeenCalled(); + 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('forwards unrelated warnings to stderr', () => { + 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', 'Some legacy thing')); - expect(stderrSpy).toHaveBeenCalledTimes(1); - const written = (stderrSpy.mock.calls[0]?.[0] ?? '').toString(); - expect(written).toContain('DeprecationWarning'); - expect(written).toContain('Some legacy thing'); + emit(makeWarning('DeprecationWarning', 'X')); + expect(buggy).toHaveBeenCalledTimes(1); + expect(downstream).toHaveBeenCalledTimes(1); }); it('keeps suppressed warnings visible when DEBUG is set', () => { @@ -105,7 +128,7 @@ describe('initializeWarningHandler', () => { 'Possible EventTarget memory leak detected. 1500 abort listeners added to [AbortSignal].', ), ); - expect(stderrSpy).toHaveBeenCalledTimes(1); + expect(priorListener).toHaveBeenCalledTimes(1); }); it('treats DEBUG=0 and DEBUG=false as not set', () => { @@ -117,7 +140,7 @@ describe('initializeWarningHandler', () => { 'Possible EventTarget memory leak detected. 1500 abort listeners added to [AbortSignal].', ), ); - expect(stderrSpy).not.toHaveBeenCalled(); + expect(priorListener).not.toHaveBeenCalled(); }); it('keeps warnings visible when QWEN_DEBUG is set', () => { @@ -129,7 +152,7 @@ describe('initializeWarningHandler', () => { 'Possible EventTarget memory leak detected. 1500 abort listeners added to [AbortSignal].', ), ); - expect(stderrSpy).toHaveBeenCalledTimes(1); + expect(priorListener).toHaveBeenCalledTimes(1); }); it('keeps warnings visible when NODE_ENV=development', () => { @@ -141,7 +164,7 @@ describe('initializeWarningHandler', () => { 'Possible EventTarget memory leak detected. 1500 abort listeners added to [AbortSignal].', ), ); - expect(stderrSpy).toHaveBeenCalledTimes(1); + expect(priorListener).toHaveBeenCalledTimes(1); }); it('is idempotent — repeated calls install only one listener', () => { @@ -151,3 +174,53 @@ describe('initializeWarningHandler', () => { expect(process.listeners('warning').length).toBe(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 } = 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)); + const helperPath = join(here, 'warningHandler.ts'); + + const dir = await mkdtemp(join(tmpdir(), 'warning-handler-e2e-')); + try { + const script = ` + import { initializeWarningHandler } from ${JSON.stringify(helperPath)}; + 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 index 564e6af76ba..99b8c806af8 100644 --- a/packages/cli/src/utils/warningHandler.ts +++ b/packages/cli/src/utils/warningHandler.ts @@ -49,7 +49,16 @@ export function resetWarningHandlerForTests(): void { * 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 to stderr so developers can still see them. + * 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. */ @@ -58,16 +67,25 @@ export function initializeWarningHandler(): void { const debug = isDebugMode(); + // Snapshot everything currently listening on 'warning' (Node's default + // onWarning printer + any external telemetry subscribers). We will fan + // out non-suppressed warnings back to them. + const priorListeners = [...process.listeners('warning')] as Array< + (warning: Error) => void + >; + installedHandler = (warning: Error) => { if (!debug && isSuppressed(warning)) return; - const text = warning.stack ?? `${warning.name}: ${warning.message}`; - process.stderr.write(`(node) ${text}\n`); + 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. + } + } }; - // Adding a listener (instead of removeAllListeners) leaves any third-party - // warning subscribers in place. Node's default printer only fires when - // there are zero listeners — so installing our handler implicitly disables - // the default print path, and we take over forwarding to stderr for the - // non-suppressed cases. + process.removeAllListeners('warning'); process.on('warning', installedHandler); } diff --git a/packages/core/src/utils/abortController.test.ts b/packages/core/src/utils/abortController.test.ts index aab7bca80d1..f520b34cda8 100644 --- a/packages/core/src/utils/abortController.test.ts +++ b/packages/core/src/utils/abortController.test.ts @@ -191,27 +191,46 @@ describe('combineAbortSignals', () => { }); }); +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( - 'does not retain abandoned children through the parent listener', - async () => { - const parent = createAbortController(); - let weakChild: WeakRef; - (() => { - const child = createChildAbortController(parent); - weakChild = new WeakRef(child); - })(); - // Yield to allow finalizers; then GC. - await new Promise((r) => setTimeout(r, 0)); - maybeGc!(); - await new Promise((r) => setTimeout(r, 0)); - maybeGc!(); - expect(weakChild!.deref()).toBeUndefined(); - // Firing parent now should not throw (handler dereferences a dead WeakRef). - expect(() => parent.abort()).not.toThrow(); - }, - ); + 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 index b59863d73ba..977a1bf47bf 100644 --- a/packages/core/src/utils/abortController.ts +++ b/packages/core/src/utils/abortController.ts @@ -33,53 +33,27 @@ function asSignal( return parent instanceof AbortController ? parent.signal : parent; } -/** - * Propagate abort from a weakly-referenced parent to a weakly-referenced child. - * Module-scope (not a per-call closure) to keep allocation cheap. - */ -function propagateAbort( - this: WeakRef, - weakChild: WeakRef, -): void { - const parent = this.deref(); - weakChild.deref()?.abort(parent?.reason); -} - -/** - * Remove an abort handler from a weakly-referenced parent signal. - * No-op if either side has been GC'd or the parent already fired (`{once:true}`). - */ -function removeAbortHandler( - this: WeakRef, - weakHandler: WeakRef<(...args: unknown[]) => void>, -): void { - const parent = this.deref(); - const handler = weakHandler.deref(); - if (parent && handler) { - parent.removeEventListener('abort', handler); - } -} - /** * Create a child AbortController that aborts when its parent aborts. * Aborting the child does NOT abort the parent. * - * Three invariants make this safe under long-running parents with many - * short-lived children: + * 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. - * - Both parent and child are held via `WeakRef`, so the parent does not - * strongly retain abandoned children; a child that is dropped without - * being aborted can still be GC'd. + * - The parent is held via `WeakRef` from the child's reverse-cleanup + * closure, so a child being kept alive does not pin its parent. * - * Caveat: if the parent controller is held ONLY through the child's WeakRef, - * it can be GC'd before its `abort` fires. In practice every parent in this - * codebase is held strongly by a long-lived owner (e.g. `this.master...`) - * that outlives the child, so this is safe. + * 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. @@ -99,15 +73,25 @@ export function createChildAbortController( return child; } - const weakChild = new WeakRef(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 = propagateAbort.bind(weakParent, weakChild); + const handler = (): void => { + child.abort(weakParent.deref()?.reason); + }; parentSignal.addEventListener('abort', handler, { once: true }); child.signal.addEventListener( 'abort', - removeAbortHandler.bind(weakParent, new WeakRef(handler)), + () => { + // `{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 }, ); From 874d4a984a3cb48e7297524fe2131ed65857f501 Mon Sep 17 00:00:00 2001 From: doudouOUC Date: Thu, 21 May 2026 12:48:23 +0800 Subject: [PATCH 06/15] =?UTF-8?q?fix(core,cli):=20address=20PR=20#4366=20r?= =?UTF-8?q?eview=20=E2=80=94=20fix=20combineAbortSignals=20orphan=20listen?= =?UTF-8?q?ers=20+=20runtime=20DEBUG=20toggle?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- packages/cli/src/utils/warningHandler.test.ts | 22 +++++++++++++++++++ packages/cli/src/utils/warningHandler.ts | 8 ++++--- .../core/src/utils/abortController.test.ts | 22 +++++++++++++++++++ packages/core/src/utils/abortController.ts | 14 ++++++++++-- 4 files changed, 61 insertions(+), 5 deletions(-) diff --git a/packages/cli/src/utils/warningHandler.test.ts b/packages/cli/src/utils/warningHandler.test.ts index 46cafb65965..4437613ce6b 100644 --- a/packages/cli/src/utils/warningHandler.test.ts +++ b/packages/cli/src/utils/warningHandler.test.ts @@ -173,6 +173,28 @@ describe('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', () => { diff --git a/packages/cli/src/utils/warningHandler.ts b/packages/cli/src/utils/warningHandler.ts index 99b8c806af8..fc51114a36a 100644 --- a/packages/cli/src/utils/warningHandler.ts +++ b/packages/cli/src/utils/warningHandler.ts @@ -65,8 +65,6 @@ export function resetWarningHandlerForTests(): void { export function initializeWarningHandler(): void { if (installedHandler) return; - const debug = isDebugMode(); - // Snapshot everything currently listening on 'warning' (Node's default // onWarning printer + any external telemetry subscribers). We will fan // out non-suppressed warnings back to them. @@ -75,7 +73,11 @@ export function initializeWarningHandler(): void { >; installedHandler = (warning: Error) => { - if (!debug && isSuppressed(warning)) return; + // Evaluate isDebugMode() per warning so DEBUG / QWEN_DEBUG can be + // toggled at runtime (e.g. via a `/debug` slash command) without + // re-running initializeWarningHandler. Warnings are rare; the cost is + // a couple of env lookups. + if (!isDebugMode() && isSuppressed(warning)) return; for (const fn of priorListeners) { try { fn(warning); diff --git a/packages/core/src/utils/abortController.test.ts b/packages/core/src/utils/abortController.test.ts index f520b34cda8..bc90732f897 100644 --- a/packages/core/src/utils/abortController.test.ts +++ b/packages/core/src/utils/abortController.test.ts @@ -176,9 +176,31 @@ describe('combineAbortSignals', () => { // 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 if the loop already aborted the controller', () => { + vi.useFakeTimers(); + try { + const a = createAbortController(); + a.abort('pre'); + const { signal } = combineAbortSignals([a.signal], { timeoutMs: 50 }); + expect(signal.aborted).toBe(true); + // Advancing past the timeout must not change the abort reason — the + // controller was already aborted before the timer ran, and if we had + // scheduled one anyway it would have replaced `pre` with TimeoutError. + vi.advanceTimersByTime(100); + expect(signal.reason).toBe('pre'); + } finally { + vi.useRealTimers(); + } + }); + it('auto-cleans listeners on inputs when the combined signal aborts', () => { const a = createAbortController(); const b = createAbortController(); diff --git a/packages/core/src/utils/abortController.ts b/packages/core/src/utils/abortController.ts index 977a1bf47bf..5dc4f9f3d2f 100644 --- a/packages/core/src/utils/abortController.ts +++ b/packages/core/src/utils/abortController.ts @@ -134,8 +134,10 @@ export function combineAbortSignals( 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) { + if (timeoutMs !== undefined && timeoutMs > 0 && !controller.signal.aborted) { const timeoutId = setTimeout(() => { controller.abort(new DOMException('Operation timed out', 'TimeoutError')); }, timeoutMs); @@ -149,7 +151,15 @@ export function combineAbortSignals( for (const fn of cleanups) fn(); }; - controller.signal.addEventListener('abort', cleanup, { once: true }); + // 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 }; } From 733c85ce576cdb1b9635a505573a9f2f09598779 Mon Sep 17 00:00:00 2001 From: doudouOUC Date: Thu, 21 May 2026 13:48:43 +0800 Subject: [PATCH 07/15] test(core): strengthen the timeout-guard test in combineAbortSignals to actually exercise the new !aborted check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../core/src/utils/abortController.test.ts | 38 +++++++++++++++---- 1 file changed, 31 insertions(+), 7 deletions(-) diff --git a/packages/core/src/utils/abortController.test.ts b/packages/core/src/utils/abortController.test.ts index bc90732f897..485c2273e5b 100644 --- a/packages/core/src/utils/abortController.test.ts +++ b/packages/core/src/utils/abortController.test.ts @@ -184,19 +184,43 @@ describe('combineAbortSignals', () => { expect(getEventListeners(c.signal, 'abort').length).toBe(0); }); - it('does not schedule a timeout if the loop already aborted the controller', () => { + 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(); - a.abort('pre'); - const { signal } = combineAbortSignals([a.signal], { timeoutMs: 50 }); + 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); - // Advancing past the timeout must not change the abort reason — the - // controller was already aborted before the timer ran, and if we had - // scheduled one anyway it would have replaced `pre` with TimeoutError. + // 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('pre'); + expect(signal.reason).toBe(reasonAfterAbort); } finally { + setTimeoutSpy.mockRestore(); vi.useRealTimers(); } }); From c840346348cfd790724f6fbff528148a6225e6ad Mon Sep 17 00:00:00 2001 From: doudouOUC Date: Thu, 21 May 2026 14:16:23 +0800 Subject: [PATCH 08/15] 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. --- .../core/src/utils/abortController.test.ts | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/packages/core/src/utils/abortController.test.ts b/packages/core/src/utils/abortController.test.ts index 485c2273e5b..e81c4778fba 100644 --- a/packages/core/src/utils/abortController.test.ts +++ b/packages/core/src/utils/abortController.test.ts @@ -139,6 +139,29 @@ describe('combineAbortSignals', () => { } }); + 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; From d95a18e3df87c70c2fbaa5530aa8cb68546413d5 Mon Sep 17 00:00:00 2001 From: doudouOUC Date: Thu, 21 May 2026 15:47:26 +0800 Subject: [PATCH 09/15] fix(test): use pathToFileURL for the warning-handler e2e import on Windows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- packages/cli/src/utils/warningHandler.test.ts | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/packages/cli/src/utils/warningHandler.test.ts b/packages/cli/src/utils/warningHandler.test.ts index 4437613ce6b..78b957ccd62 100644 --- a/packages/cli/src/utils/warningHandler.test.ts +++ b/packages/cli/src/utils/warningHandler.test.ts @@ -205,18 +205,23 @@ describe('initializeWarningHandler — end-to-end stderr behavior', () => { // 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 } = await import('node:url'); + 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)); - const helperPath = join(here, 'warningHandler.ts'); + // 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(helperPath)}; + import { initializeWarningHandler } from ${JSON.stringify(helperImportSpecifier)}; delete process.env.DEBUG; delete process.env.QWEN_DEBUG; process.env.NODE_ENV = 'production'; initializeWarningHandler(); From 94e8c5812154046d441762721df115568922c8bc Mon Sep 17 00:00:00 2001 From: doudouOUC Date: Thu, 21 May 2026 18:55:43 +0800 Subject: [PATCH 10/15] =?UTF-8?q?fix(core,cli):=20address=20PR=20#4366=20r?= =?UTF-8?q?eview=20batch=20=E2=80=94=20onAbort=20leak,=20migrate=20missed?= =?UTF-8?q?=20sites,=20tighten=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../listener-accumulation-repro.mjs | 26 ++-- packages/cli/src/utils/warningHandler.ts | 20 +++- .../core/src/agents/runtime/agent-core.ts | 24 ++-- packages/core/src/goals/goalHook.ts | 5 +- .../src/hooks/combinedAbortSignal.test.ts | 111 ------------------ .../core/src/hooks/combinedAbortSignal.ts | 21 ---- packages/core/src/hooks/httpHookRunner.ts | 6 +- packages/core/src/hooks/promptHookRunner.ts | 23 ++-- .../core/src/utils/abortController.test.ts | 35 ++++++ 9 files changed, 95 insertions(+), 176 deletions(-) delete mode 100644 packages/core/src/hooks/combinedAbortSignal.test.ts delete mode 100644 packages/core/src/hooks/combinedAbortSignal.ts diff --git a/docs/verification/abort-controller-refactor/listener-accumulation-repro.mjs b/docs/verification/abort-controller-refactor/listener-accumulation-repro.mjs index 2df834e91a4..ea730161787 100644 --- a/docs/verification/abort-controller-refactor/listener-accumulation-repro.mjs +++ b/docs/verification/abort-controller-refactor/listener-accumulation-repro.mjs @@ -18,22 +18,17 @@ import { getEventListeners, setMaxListeners } from 'node:events'; -// Inline copy of the production helper so this script has no build-step -// dependency on the @qwen-code/qwen-code-core package. +// 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 propagateAbort(weakChild) { - const parent = this.deref(); - weakChild.deref()?.abort(parent?.reason); -} -function removeAbortHandler(weakHandler) { - const parent = this.deref(); - const handler = weakHandler.deref(); - if (parent && handler) parent.removeEventListener('abort', handler); -} function createChildAbortController(parent) { const child = createAbortController(); if (!parent) return child; @@ -42,13 +37,16 @@ function createChildAbortController(parent) { child.abort(parentSignal.reason); return child; } - const weakChild = new WeakRef(child); const weakParent = new WeakRef(parentSignal); - const handler = propagateAbort.bind(weakParent, weakChild); + const handler = () => { + child.abort(weakParent.deref()?.reason); + }; parentSignal.addEventListener('abort', handler, { once: true }); child.signal.addEventListener( 'abort', - removeAbortHandler.bind(weakParent, new WeakRef(handler)), + () => { + weakParent.deref()?.removeEventListener('abort', handler); + }, { once: true }, ); return child; diff --git a/packages/cli/src/utils/warningHandler.ts b/packages/cli/src/utils/warningHandler.ts index fc51114a36a..81087123601 100644 --- a/packages/cli/src/utils/warningHandler.ts +++ b/packages/cli/src/utils/warningHandler.ts @@ -66,8 +66,23 @@ export function initializeWarningHandler(): void { if (installedHandler) return; // Snapshot everything currently listening on 'warning' (Node's default - // onWarning printer + any external telemetry subscribers). We will fan + // 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 >; @@ -75,8 +90,7 @@ export function initializeWarningHandler(): 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. Warnings are rare; the cost is - // a couple of env lookups. + // re-running initializeWarningHandler. if (!isDebugMode() && isSuppressed(warning)) return; for (const fn of priorListeners) { try { diff --git a/packages/core/src/agents/runtime/agent-core.ts b/packages/core/src/agents/runtime/agent-core.ts index 3339ab00d21..be36a5c38e2 100644 --- a/packages/core/src/agents/runtime/agent-core.ts +++ b/packages/core/src/agents/runtime/agent-core.ts @@ -1324,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/goals/goalHook.ts b/packages/core/src/goals/goalHook.ts index ea6fd8657b7..a35564e1a7c 100644 --- a/packages/core/src/goals/goalHook.ts +++ b/packages/core/src/goals/goalHook.ts @@ -21,6 +21,7 @@ import { type ActiveGoal, } from './activeGoalStore.js'; import { judgeGoal } from './goalJudge.js'; +import { createAbortController } from '../utils/abortController.js'; import { createDebugLogger } from '../utils/debugLogger.js'; const debugLogger = createDebugLogger('GOAL_HOOK'); @@ -67,7 +68,7 @@ async function judgeGoalWithTimeout( // without this `judgeGoal`'s `generateContent` keeps running in the // background — leaking one request per timeout that accumulates across // goal-loop iterations. - const judgeController = new AbortController(); + const judgeController = createAbortController(); const linkedSignal = AbortSignal.any([args.signal, judgeController.signal]); try { return await Promise.race([ @@ -166,7 +167,7 @@ export function createGoalStopHookCallback(args: { return { continue: true }; } - const signal = context?.signal ?? new AbortController().signal; + const signal = context?.signal ?? createAbortController().signal; const verdict = await judgeGoalWithTimeout(config, { condition, lastAssistantText, diff --git a/packages/core/src/hooks/combinedAbortSignal.test.ts b/packages/core/src/hooks/combinedAbortSignal.test.ts deleted file mode 100644 index 1954237bc4c..00000000000 --- 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 bf3c7501dcd..00000000000 --- a/packages/core/src/hooks/combinedAbortSignal.ts +++ /dev/null @@ -1,21 +0,0 @@ -/** - * @license - * Copyright 2026 Qwen Team - * SPDX-License-Identifier: Apache-2.0 - */ - -import { combineAbortSignals } from '../utils/abortController.js'; - -/** - * @deprecated Use {@link combineAbortSignals} from `utils/abortController.js`. - * Thin wrapper preserved so existing callers (httpHookRunner) keep working - * during the abort-controller helper rollout. - */ -export function createCombinedAbortSignal( - externalSignal?: AbortSignal, - options?: { timeoutMs?: number }, -): { signal: AbortSignal; cleanup: () => void } { - return combineAbortSignals([externalSignal], { - timeoutMs: options?.timeoutMs, - }); -} diff --git a/packages/core/src/hooks/httpHookRunner.ts b/packages/core/src/hooks/httpHookRunner.ts index aad909ed3f6..cc72d41b7f6 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 1f57bbf5a55..4e9b524267a 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 index e81c4778fba..e81e617c618 100644 --- a/packages/core/src/utils/abortController.test.ts +++ b/packages/core/src/utils/abortController.test.ts @@ -98,6 +98,12 @@ describe('createChildAbortController', () => { 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', () => { @@ -178,6 +184,35 @@ describe('combineAbortSignals', () => { 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(); From 31784f4c6c36d9c5da6d0263cbf1215ff6d2c6f7 Mon Sep 17 00:00:00 2001 From: doudouOUC Date: Thu, 21 May 2026 19:35:55 +0800 Subject: [PATCH 11/15] docs(verification): commit the headless-scenario scripts referenced by the PR body MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../scripts/02-lite.sh | 23 +++++++++++++++++++ .../scripts/06-headless-sigint.sh | 18 +++++++++++++++ 2 files changed, 41 insertions(+) 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 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 00000000000..d60992cbf54 --- /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 00000000000..1fc36bb4a2d --- /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")" From 5aa7110e4018428276fe0bbab6ced64e3d5ddeb2 Mon Sep 17 00:00:00 2001 From: doudouOUC Date: Thu, 21 May 2026 19:56:56 +0800 Subject: [PATCH 12/15] docs(verification): sync automated-results with current state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../abort-controller-refactor/automated-results.md | 9 ++++----- .../abort-controller-refactor/migration-completeness.txt | 3 +++ 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/docs/verification/abort-controller-refactor/automated-results.md b/docs/verification/abort-controller-refactor/automated-results.md index 4ee3c63d246..ebe5627b055 100644 --- a/docs/verification/abort-controller-refactor/automated-results.md +++ b/docs/verification/abort-controller-refactor/automated-results.md @@ -51,9 +51,9 @@ that requires `--expose-gc`, 2 are pre-existing skips in the headless suite). Coverage: -- `packages/core/src/utils/abortController.test.ts` — 19 tests: factory cap, child propagation, reverse cleanup, fast path, undefined parent, `combineAbortSignals` semantics, GC safety. -- `packages/cli/src/utils/warningHandler.test.ts` — 9 tests: idempotency, AbortSignal suppression (including `[AbortSignal{...}]` shape), generic EventTarget NOT suppressed, debug-mode passthrough. -- `packages/core/src/hooks/combinedAbortSignal.test.ts` — 8 existing tests pass against the new `@deprecated` shim, proving wire compatibility with `httpHookRunner.ts:202`. +- `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. @@ -77,8 +77,7 @@ $ node_modules/.bin/prettier --check packages/core/src/agents/runtime/agent-core 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 \ - packages/core/src/hooks/combinedAbortSignal.ts + packages/core/src/utils/abortController.test.ts Checking formatting... All matched files use Prettier code style! ``` diff --git a/docs/verification/abort-controller-refactor/migration-completeness.txt b/docs/verification/abort-controller-refactor/migration-completeness.txt index e69de29bb2d..59e53924046 100644 --- a/docs/verification/abort-controller-refactor/migration-completeness.txt +++ b/docs/verification/abort-controller-refactor/migration-completeness.txt @@ -0,0 +1,3 @@ +$ grep -rn "new AbortController" packages/core/src --include="*.ts" \ + | grep -v test | grep -v abortController.ts +(no output) From b4f36d43c92374704846f69d8705411e57772cd3 Mon Sep 17 00:00:00 2001 From: doudouOUC Date: Thu, 21 May 2026 20:21:50 +0800 Subject: [PATCH 13/15] test(core): pin two abort-cascade behaviors PR #4366 introduced MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../src/agents/arena/ArenaManager.test.ts | 35 ++++++ .../core/src/followup/speculation.test.ts | 100 +++++++++++++++++- 2 files changed, 133 insertions(+), 2 deletions(-) diff --git a/packages/core/src/agents/arena/ArenaManager.test.ts b/packages/core/src/agents/arena/ArenaManager.test.ts index 744c059a81b..0fdd9424e43 100644 --- a/packages/core/src/agents/arena/ArenaManager.test.ts +++ b/packages/core/src/agents/arena/ArenaManager.test.ts @@ -541,6 +541,41 @@ index 111..222 100644 expect(manager.getSessionStatus()).toBe(ArenaSessionStatus.CANCELLED); }); + it('cancel cascades the master abort to every spawned agent controller', async () => { + // Pins the per-agent abort cascade introduced in PR #4366 + // (per-agent controllers are now `createChildAbortController(master)`). + // If a future refactor reverts to independent controllers, this test + // catches the regression — existing cancel tests only check backend + // and status, not signal propagation. + const manager = new ArenaManager(mockConfig as never); + mockBackend.setAutoExit(false); + + const startPromise = manager.start({ + ...createValidStartOptions(), + timeoutSeconds: 30, + }); + + await waitForCondition( + () => mockBackend.spawnAgent.mock.calls.length >= 2, + ); + + // Before cancel: at least 2 agents, none should be aborted. + const beforeStates = manager.getAgentStates(); + expect(beforeStates.length).toBeGreaterThanOrEqual(2); + for (const agent of beforeStates) { + expect(agent.abortController.signal.aborted).toBe(false); + } + + await manager.cancel(); + + // After cancel: every agent's signal must be aborted. + for (const agent of beforeStates) { + expect(agent.abortController.signal.aborted).toBe(true); + } + + await startPromise; + }); + it('cleanup should release backend and worktree resources after start', async () => { const manager = new ArenaManager(mockConfig as never); diff --git a/packages/core/src/followup/speculation.test.ts b/packages/core/src/followup/speculation.test.ts index e1361bceab8..972303ef9cd 100644 --- a/packages/core/src/followup/speculation.test.ts +++ b/packages/core/src/followup/speculation.test.ts @@ -4,9 +4,33 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { describe, it, expect } from 'vitest'; -import { ensureToolResultPairing } from './speculation.js'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { ensureToolResultPairing, startSpeculation } from './speculation.js'; import type { Content } from '@google/genai'; +import { + saveCacheSafeParams, + clearCacheSafeParams, +} from '../utils/forkedAgent.js'; + +// Stub the forked-agent + overlay infrastructure so startSpeculation's +// fire-and-forget background loop is a no-op. The tests below only assert +// the synchronous abort-controller wiring set up by startSpeculation; the +// loop's actual execution is covered elsewhere. +vi.mock('../utils/forkedAgent.js', async () => { + const actual = await vi.importActual< + typeof import('../utils/forkedAgent.js') + >('../utils/forkedAgent.js'); + return { + ...actual, + runWithForkedChatModel: vi.fn(async () => ({ messages: [] })), + }; +}); + +vi.mock('./overlayFs.js', () => ({ + OverlayFs: vi.fn().mockImplementation(() => ({ + cleanup: vi.fn().mockResolvedValue(undefined), + })), +})); describe('ensureToolResultPairing', () => { it('returns empty array unchanged', () => { @@ -111,3 +135,75 @@ describe('ensureToolResultPairing', () => { expect(result).toEqual(messages); }); }); + +describe('startSpeculation — abort-controller wiring', () => { + // Minimal Config stub — startSpeculation only calls config.getCwd() in the + // synchronous path (OverlayFs ctor), and that's already mocked above. + const fakeConfig = { + getCwd: () => '/tmp/test-speculation-cwd', + getApprovalMode: () => 'default', + getFastModel: () => undefined, + } as unknown as import('../config/config.js').Config; + + beforeEach(() => { + // startSpeculation throws if cache-safe params aren't set; install a stub. + saveCacheSafeParams({ + model: 'fake-model', + history: [], + tools: [], + } as unknown as import('../utils/forkedAgent.js').CacheSafeParams); + }); + + afterEach(() => { + clearCacheSafeParams(); + }); + + it('parent abort propagates to the speculation controller (lifetime contract)', async () => { + const parent = new AbortController(); + const state = await startSpeculation( + fakeConfig, + 'do a thing', + parent.signal, + ); + // The mocked runWithForkedChatModel resolves immediately, but the fire- + // and-forget .then().finally() chain runs async. The abortController in + // the returned state is wired before the background loop starts. + expect(state.abortController).toBeTruthy(); + expect(state.abortController!.signal.aborted).toBe(false); + parent.abort('parent reason'); + expect(state.abortController!.signal.aborted).toBe(true); + expect(state.abortController!.signal.reason).toBe('parent reason'); + }); + + it('fast-path: parent already aborted returns aborted state without entering the loop', async () => { + const parent = new AbortController(); + parent.abort('pre'); + const state = await startSpeculation( + fakeConfig, + 'do a thing', + parent.signal, + ); + expect(state.status).toBe('aborted'); + expect(state.abortController!.signal.aborted).toBe(true); + expect(state.abortController!.signal.reason).toBe('pre'); + }); + + it('child controller never strong-pins the parent listener after settling', async () => { + const { getEventListeners } = await import('node:events'); + const parent = new AbortController(); + const before = getEventListeners(parent.signal, 'abort').length; + const state = await startSpeculation( + fakeConfig, + 'do a thing', + parent.signal, + ); + // While the loop is running, child has a listener on parent. + expect(getEventListeners(parent.signal, 'abort').length).toBe(before + 1); + // Let the background promise settle (mocked loop resolves immediately, then + // the .finally() calls abortController.abort() which triggers reverse cleanup). + await new Promise((r) => setImmediate(r)); + await new Promise((r) => setImmediate(r)); + expect(state.abortController!.signal.aborted).toBe(true); + expect(getEventListeners(parent.signal, 'abort').length).toBe(before); + }); +}); From e31432ff10d8e48f5f97241d2c97edf0427210b6 Mon Sep 17 00:00:00 2001 From: doudouOUC Date: Thu, 21 May 2026 20:49:42 +0800 Subject: [PATCH 14/15] 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. --- .../abort-controller-refactor/README.md | 8 ++++---- packages/core/src/followup/speculation.test.ts | 15 ++++++++------- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/docs/verification/abort-controller-refactor/README.md b/docs/verification/abort-controller-refactor/README.md index 3d14b6f14f3..f9d0d02caa9 100644 --- a/docs/verification/abort-controller-refactor/README.md +++ b/docs/verification/abort-controller-refactor/README.md @@ -103,10 +103,10 @@ when done; `tmux kill-session -t qwen-verify-XX` to stop the pane. The automated checks below were run during development and recorded in `automated-results.md`: -- All abortController unit tests pass (`abortController.test.ts`, 18 tests + 1 GC test skipped under non-`--expose-gc`). -- All warningHandler tests pass (`warningHandler.test.ts`, 9 tests). -- Existing combinedAbortSignal tests pass against the deprecation shim (8 tests). -- All agent runtime / followup / openaiContentGenerator / hooks tests pass. +- 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 — including new pinning tests for the master→agent abort cascade (`ArenaManager.test.ts`) and `startSpeculation` parent-signal wiring (`speculation.test.ts`). - Migration completeness: `grep -rn "new AbortController" packages/core/src --include="*.ts" | grep -v test | grep -v abortController.ts` returns **empty**. - TypeScript strict-mode typecheck passes for both `packages/core` and `packages/cli`. - Prettier check passes on all modified files. diff --git a/packages/core/src/followup/speculation.test.ts b/packages/core/src/followup/speculation.test.ts index 972303ef9cd..46bb285b68d 100644 --- a/packages/core/src/followup/speculation.test.ts +++ b/packages/core/src/followup/speculation.test.ts @@ -5,8 +5,9 @@ */ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { getEventListeners } from 'node:events'; import { ensureToolResultPairing, startSpeculation } from './speculation.js'; -import type { Content } from '@google/genai'; +import type { Content, GenerateContentConfig } from '@google/genai'; import { saveCacheSafeParams, clearCacheSafeParams, @@ -147,11 +148,12 @@ describe('startSpeculation — abort-controller wiring', () => { beforeEach(() => { // startSpeculation throws if cache-safe params aren't set; install a stub. - saveCacheSafeParams({ - model: 'fake-model', - history: [], - tools: [], - } as unknown as import('../utils/forkedAgent.js').CacheSafeParams); + // saveCacheSafeParams takes 3 positional args: (generationConfig, history, model). + saveCacheSafeParams( + { systemInstruction: '' } as unknown as GenerateContentConfig, + [], + 'fake-model', + ); }); afterEach(() => { @@ -189,7 +191,6 @@ describe('startSpeculation — abort-controller wiring', () => { }); it('child controller never strong-pins the parent listener after settling', async () => { - const { getEventListeners } = await import('node:events'); const parent = new AbortController(); const before = getEventListeners(parent.signal, 'abort').length; const state = await startSpeculation( From 9323bc38391909844e9461b4de97d3d83e2727dc Mon Sep 17 00:00:00 2001 From: doudouOUC Date: Mon, 25 May 2026 19:18:28 +0800 Subject: [PATCH 15/15] =?UTF-8?q?refactor(core):=20narrow=20PR=20#4366=20s?= =?UTF-8?q?cope=20per=20yiliang's=20review=20=E2=80=94=20revert=20independ?= =?UTF-8?q?ent-controller=20migrations?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 | 4 +- .../automated-results.md | 33 ++++-- .../migration-completeness.txt | 27 ++++- .../src/agents/arena/ArenaManager.test.ts | 35 ------ .../core/src/agents/arena/ArenaManager.ts | 8 +- .../src/agents/background-agent-resume.ts | 7 +- packages/core/src/core/client.ts | 26 +++-- .../core/src/followup/speculation.test.ts | 103 +----------------- packages/core/src/followup/speculation.ts | 16 ++- packages/core/src/goals/goalHook.ts | 5 +- packages/core/src/memory/manager.ts | 3 +- .../src/services/chatCompressionService.ts | 3 +- .../core/src/services/chatRecordingService.ts | 3 +- packages/core/src/tools/agent/agent.ts | 28 ++--- packages/core/src/tools/monitor.ts | 3 +- packages/core/src/tools/shell.ts | 7 +- packages/core/src/utils/fetch.ts | 3 +- 17 files changed, 110 insertions(+), 204 deletions(-) diff --git a/docs/verification/abort-controller-refactor/README.md b/docs/verification/abort-controller-refactor/README.md index f9d0d02caa9..6f0b3c3f0e6 100644 --- a/docs/verification/abort-controller-refactor/README.md +++ b/docs/verification/abort-controller-refactor/README.md @@ -106,8 +106,8 @@ The automated checks below were run during development and recorded in - 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 — including new pinning tests for the master→agent abort cascade (`ArenaManager.test.ts`) and `startSpeculation` parent-signal wiring (`speculation.test.ts`). -- Migration completeness: `grep -rn "new AbortController" packages/core/src --include="*.ts" | grep -v test | grep -v abortController.ts` returns **empty**. +- 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. diff --git a/docs/verification/abort-controller-refactor/automated-results.md b/docs/verification/abort-controller-refactor/automated-results.md index ebe5627b055..a53d350edb6 100644 --- a/docs/verification/abort-controller-refactor/automated-results.md +++ b/docs/verification/abort-controller-refactor/automated-results.md @@ -25,18 +25,31 @@ pattern (`createChildAbortController` from `packages/core/src/utils/abortControl 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 completeness +## 2. Migration scope (intentional) -```sh -$ grep -rn "new AbortController" packages/core/src --include="*.ts" \ - | grep -v test | grep -v abortController.ts -(no output) -``` +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. -Every production `new AbortController()` call in `packages/core/src` (outside -the helper module itself and outside test files) has been migrated to use -`createAbortController` / `createChildAbortController`. See -`migration-completeness.txt` for the captured grep. +See `migration-completeness.txt` for the actual grep + rationale. ## 3. Affected test suites diff --git a/docs/verification/abort-controller-refactor/migration-completeness.txt b/docs/verification/abort-controller-refactor/migration-completeness.txt index 59e53924046..3e5344cea28 100644 --- a/docs/verification/abort-controller-refactor/migration-completeness.txt +++ b/docs/verification/abort-controller-refactor/migration-completeness.txt @@ -1,3 +1,28 @@ $ grep -rn "new AbortController" packages/core/src --include="*.ts" \ | grep -v test | grep -v abortController.ts -(no output) + +# 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/packages/core/src/agents/arena/ArenaManager.test.ts b/packages/core/src/agents/arena/ArenaManager.test.ts index 0fdd9424e43..744c059a81b 100644 --- a/packages/core/src/agents/arena/ArenaManager.test.ts +++ b/packages/core/src/agents/arena/ArenaManager.test.ts @@ -541,41 +541,6 @@ index 111..222 100644 expect(manager.getSessionStatus()).toBe(ArenaSessionStatus.CANCELLED); }); - it('cancel cascades the master abort to every spawned agent controller', async () => { - // Pins the per-agent abort cascade introduced in PR #4366 - // (per-agent controllers are now `createChildAbortController(master)`). - // If a future refactor reverts to independent controllers, this test - // catches the regression — existing cancel tests only check backend - // and status, not signal propagation. - const manager = new ArenaManager(mockConfig as never); - mockBackend.setAutoExit(false); - - const startPromise = manager.start({ - ...createValidStartOptions(), - timeoutSeconds: 30, - }); - - await waitForCondition( - () => mockBackend.spawnAgent.mock.calls.length >= 2, - ); - - // Before cancel: at least 2 agents, none should be aborted. - const beforeStates = manager.getAgentStates(); - expect(beforeStates.length).toBeGreaterThanOrEqual(2); - for (const agent of beforeStates) { - expect(agent.abortController.signal.aborted).toBe(false); - } - - await manager.cancel(); - - // After cancel: every agent's signal must be aborted. - for (const agent of beforeStates) { - expect(agent.abortController.signal.aborted).toBe(true); - } - - await startPromise; - }); - it('cleanup should release backend and worktree resources after start', async () => { const manager = new ArenaManager(mockConfig as never); diff --git a/packages/core/src/agents/arena/ArenaManager.ts b/packages/core/src/agents/arena/ArenaManager.ts index 3c0737d8eba..382c02cc24d 100644 --- a/packages/core/src/agents/arena/ArenaManager.ts +++ b/packages/core/src/agents/arena/ArenaManager.ts @@ -10,10 +10,6 @@ import { GitWorktreeService } from '../../services/gitWorktreeService.js'; import { Storage } from '../../config/storage.js'; import type { Config } from '../../config/config.js'; import { getCoreSystemPrompt } from '../../core/prompts.js'; -import { - createAbortController, - createChildAbortController, -} from '../../utils/abortController.js'; import { createDebugLogger } from '../../utils/debugLogger.js'; import { isNodeError } from '../../utils/errors.js'; import { atomicWriteJSON } from '../../utils/atomicFileWrite.js'; @@ -306,7 +302,7 @@ export class ArenaManager { this.worktreeDirName = await this.deriveWorktreeDirName(this.sessionId); this.startedAt = Date.now(); this.sessionStatus = ArenaSessionStatus.INITIALIZING; - this.masterAbortController = createAbortController(); + this.masterAbortController = new AbortController(); const sourceRepoPath = this.config.getWorkingDir(); const arenaSettings = this.config.getAgentsSettings().arena; @@ -818,7 +814,7 @@ export class ArenaManager { model, status: AgentStatus.INITIALIZING, worktree, - abortController: createChildAbortController(this.masterAbortController), + abortController: new AbortController(), agentSessionId: `${this.sessionId}#${agentId}`, stats: { rounds: 0, diff --git a/packages/core/src/agents/background-agent-resume.ts b/packages/core/src/agents/background-agent-resume.ts index bfa032e8521..1987335c48e 100644 --- a/packages/core/src/agents/background-agent-resume.ts +++ b/packages/core/src/agents/background-agent-resume.ts @@ -9,7 +9,6 @@ import * as path from 'node:path'; import type { Content, FunctionDeclaration, Part } from '@google/genai'; import type { Config } from '../config/config.js'; import * as jsonl from '../utils/jsonl-utils.js'; -import { createAbortController } from '../utils/abortController.js'; import { createDebugLogger } from '../utils/debugLogger.js'; import { AgentEventEmitter, @@ -419,7 +418,7 @@ export class BackgroundAgentResumeService { startTime: Number.isFinite(parsedStartTime) ? parsedStartTime : Date.now(), - abortController: createAbortController(), + abortController: new AbortController(), prompt: recovery.initialPrompt, outputFile, metaPath, @@ -491,7 +490,7 @@ export class BackgroundAgentResumeService { return undefined; } - const bgAbortController = createAbortController(); + const bgAbortController = new AbortController(); registry.register({ ...existing, @@ -920,7 +919,7 @@ export class BackgroundAgentResumeService { ...latest, isBackgrounded: true, status: 'paused', - abortController: createAbortController(), + abortController: new AbortController(), endTime: undefined, result: undefined, error: options.error, diff --git a/packages/core/src/core/client.ts b/packages/core/src/core/client.ts index 8e4edefd2c2..85d98495f59 100644 --- a/packages/core/src/core/client.ts +++ b/packages/core/src/core/client.ts @@ -15,7 +15,6 @@ import type { // Config import { ApprovalMode, type Config } from '../config/config.js'; -import { createChildAbortController } from '../utils/abortController.js'; import { createDebugLogger } from '../utils/debugLogger.js'; import { recordStartupEvent } from '../utils/startupEventSink.js'; import { microcompactHistory } from '../services/microcompaction/microcompact.js'; @@ -1197,12 +1196,19 @@ export class GeminiClient { // turn arrived before it settled). Abort it before installing the // new handle so the orphan doesn't keep running indefinitely. this.cancelPendingMemoryPrefetch(); - // Child controller bridges the caller's signal into the prefetch - // controller (so a user abort on the parent turn also kills the - // recall side-query) AND auto-detaches its parent-listener via - // reverse cleanup once this controller aborts — preventing long - // sessions from accumulating listeners on the parent signal. - const controller = createChildAbortController(signal); + const controller = new AbortController(); + // Bridge the caller's signal into the prefetch controller so a user + // abort (Ctrl-C / Esc) on the parent turn also terminates the + // recall side-query. `{ once: true }` lets the listener clean itself + // up after firing; we still call removeEventListener on the promise's + // finally to cover the normal-completion case so a long-lived parent + // signal doesn't accumulate listeners across many turns. + const onParentAbort = () => controller.abort(); + if (signal.aborted) { + controller.abort(); + } else { + signal.addEventListener('abort', onParentAbort, { once: true }); + } const promise = this.config .getMemoryManager() .recall(this.config.getProjectRoot(), partToString(request), { @@ -1238,11 +1244,7 @@ export class GeminiClient { }; void promise.finally(() => { handle.settledAt = Date.now(); - // Aborting the (already-resolved) child controller triggers its - // reverse-cleanup listener, which detaches the parent-signal - // listener registered by createChildAbortController. No-op if the - // controller already aborted (e.g. parent fired first). - controller.abort(); + signal.removeEventListener('abort', onParentAbort); }); this.pendingMemoryPrefetch = handle; } diff --git a/packages/core/src/followup/speculation.test.ts b/packages/core/src/followup/speculation.test.ts index 46bb285b68d..e1361bceab8 100644 --- a/packages/core/src/followup/speculation.test.ts +++ b/packages/core/src/followup/speculation.test.ts @@ -4,34 +4,9 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; -import { getEventListeners } from 'node:events'; -import { ensureToolResultPairing, startSpeculation } from './speculation.js'; -import type { Content, GenerateContentConfig } from '@google/genai'; -import { - saveCacheSafeParams, - clearCacheSafeParams, -} from '../utils/forkedAgent.js'; - -// Stub the forked-agent + overlay infrastructure so startSpeculation's -// fire-and-forget background loop is a no-op. The tests below only assert -// the synchronous abort-controller wiring set up by startSpeculation; the -// loop's actual execution is covered elsewhere. -vi.mock('../utils/forkedAgent.js', async () => { - const actual = await vi.importActual< - typeof import('../utils/forkedAgent.js') - >('../utils/forkedAgent.js'); - return { - ...actual, - runWithForkedChatModel: vi.fn(async () => ({ messages: [] })), - }; -}); - -vi.mock('./overlayFs.js', () => ({ - OverlayFs: vi.fn().mockImplementation(() => ({ - cleanup: vi.fn().mockResolvedValue(undefined), - })), -})); +import { describe, it, expect } from 'vitest'; +import { ensureToolResultPairing } from './speculation.js'; +import type { Content } from '@google/genai'; describe('ensureToolResultPairing', () => { it('returns empty array unchanged', () => { @@ -136,75 +111,3 @@ describe('ensureToolResultPairing', () => { expect(result).toEqual(messages); }); }); - -describe('startSpeculation — abort-controller wiring', () => { - // Minimal Config stub — startSpeculation only calls config.getCwd() in the - // synchronous path (OverlayFs ctor), and that's already mocked above. - const fakeConfig = { - getCwd: () => '/tmp/test-speculation-cwd', - getApprovalMode: () => 'default', - getFastModel: () => undefined, - } as unknown as import('../config/config.js').Config; - - beforeEach(() => { - // startSpeculation throws if cache-safe params aren't set; install a stub. - // saveCacheSafeParams takes 3 positional args: (generationConfig, history, model). - saveCacheSafeParams( - { systemInstruction: '' } as unknown as GenerateContentConfig, - [], - 'fake-model', - ); - }); - - afterEach(() => { - clearCacheSafeParams(); - }); - - it('parent abort propagates to the speculation controller (lifetime contract)', async () => { - const parent = new AbortController(); - const state = await startSpeculation( - fakeConfig, - 'do a thing', - parent.signal, - ); - // The mocked runWithForkedChatModel resolves immediately, but the fire- - // and-forget .then().finally() chain runs async. The abortController in - // the returned state is wired before the background loop starts. - expect(state.abortController).toBeTruthy(); - expect(state.abortController!.signal.aborted).toBe(false); - parent.abort('parent reason'); - expect(state.abortController!.signal.aborted).toBe(true); - expect(state.abortController!.signal.reason).toBe('parent reason'); - }); - - it('fast-path: parent already aborted returns aborted state without entering the loop', async () => { - const parent = new AbortController(); - parent.abort('pre'); - const state = await startSpeculation( - fakeConfig, - 'do a thing', - parent.signal, - ); - expect(state.status).toBe('aborted'); - expect(state.abortController!.signal.aborted).toBe(true); - expect(state.abortController!.signal.reason).toBe('pre'); - }); - - it('child controller never strong-pins the parent listener after settling', async () => { - const parent = new AbortController(); - const before = getEventListeners(parent.signal, 'abort').length; - const state = await startSpeculation( - fakeConfig, - 'do a thing', - parent.signal, - ); - // While the loop is running, child has a listener on parent. - expect(getEventListeners(parent.signal, 'abort').length).toBe(before + 1); - // Let the background promise settle (mocked loop resolves immediately, then - // the .finally() calls abortController.abort() which triggers reverse cleanup). - await new Promise((r) => setImmediate(r)); - await new Promise((r) => setImmediate(r)); - expect(state.abortController!.signal.aborted).toBe(true); - expect(getEventListeners(parent.signal, 'abort').length).toBe(before); - }); -}); diff --git a/packages/core/src/followup/speculation.ts b/packages/core/src/followup/speculation.ts index 82393d95e6b..a6f06d73ca9 100644 --- a/packages/core/src/followup/speculation.ts +++ b/packages/core/src/followup/speculation.ts @@ -27,7 +27,6 @@ import { runForkedAgent, runWithForkedChatModel, } from '../utils/forkedAgent.js'; -import { createChildAbortController } from '../utils/abortController.js'; import { getFilterReason, SUGGESTION_PROMPT } from './suggestionGenerator.js'; // --------------------------------------------------------------------------- @@ -98,7 +97,7 @@ export async function startSpeculation( throw new Error('CacheSafeParams not available for speculation'); } - const abortController = createChildAbortController(parentSignal); + const abortController = new AbortController(); // If parent was already aborted, return aborted state without starting loop if (parentSignal?.aborted) { @@ -114,6 +113,13 @@ export async function startSpeculation( }; } + // Link to parent signal with cleanup to prevent memory leak (#20) + let parentAbortHandler: (() => void) | undefined; + if (parentSignal) { + parentAbortHandler = () => abortController.abort(); + parentSignal.addEventListener('abort', parentAbortHandler, { once: true }); + } + const overlayFs = new OverlayFs(config.getCwd()); const startTime = Date.now(); @@ -171,8 +177,10 @@ export async function startSpeculation( await overlayFs.cleanup(); }) .finally(() => { - // Reverse-cleanup detaches the parent-signal listener (#20). - abortController.abort(); + // Clean up parent signal listener (#20) + if (parentSignal && parentAbortHandler) { + parentSignal.removeEventListener('abort', parentAbortHandler); + } }); return state; diff --git a/packages/core/src/goals/goalHook.ts b/packages/core/src/goals/goalHook.ts index a35564e1a7c..ea6fd8657b7 100644 --- a/packages/core/src/goals/goalHook.ts +++ b/packages/core/src/goals/goalHook.ts @@ -21,7 +21,6 @@ import { type ActiveGoal, } from './activeGoalStore.js'; import { judgeGoal } from './goalJudge.js'; -import { createAbortController } from '../utils/abortController.js'; import { createDebugLogger } from '../utils/debugLogger.js'; const debugLogger = createDebugLogger('GOAL_HOOK'); @@ -68,7 +67,7 @@ async function judgeGoalWithTimeout( // without this `judgeGoal`'s `generateContent` keeps running in the // background — leaking one request per timeout that accumulates across // goal-loop iterations. - const judgeController = createAbortController(); + const judgeController = new AbortController(); const linkedSignal = AbortSignal.any([args.signal, judgeController.signal]); try { return await Promise.race([ @@ -167,7 +166,7 @@ export function createGoalStopHookCallback(args: { return { continue: true }; } - const signal = context?.signal ?? createAbortController().signal; + const signal = context?.signal ?? new AbortController().signal; const verdict = await judgeGoalWithTimeout(config, { condition, lastAssistantText, diff --git a/packages/core/src/memory/manager.ts b/packages/core/src/memory/manager.ts index 98eb36d74f8..ca0bb5aa27e 100644 --- a/packages/core/src/memory/manager.ts +++ b/packages/core/src/memory/manager.ts @@ -39,7 +39,6 @@ import { randomUUID } from 'node:crypto'; import type { Content, Part } from '@google/genai'; import type { Config } from '../config/config.js'; import { Storage } from '../config/storage.js'; -import { createAbortController } from '../utils/abortController.js'; import { createDebugLogger } from '../utils/debugLogger.js'; import { logMemoryDream, @@ -934,7 +933,7 @@ export class MemoryManager { // missing-controller defensive warn-and-return-false path and the // model gets a phantom failure on a brand-new dream. Registering // first means any reentrant cancel sees a complete state. - const abortController = createAbortController(); + const abortController = new AbortController(); this.dreamAbortControllers.set(record.id, abortController); this.dreamInFlightByKey.set(dedupeKey, record.id); this.storeWith(record, { diff --git a/packages/core/src/services/chatCompressionService.ts b/packages/core/src/services/chatCompressionService.ts index 8a1242fda41..f704ee10fed 100644 --- a/packages/core/src/services/chatCompressionService.ts +++ b/packages/core/src/services/chatCompressionService.ts @@ -10,7 +10,6 @@ import type { GeminiChat } from '../core/geminiChat.js'; import { type ChatCompressionInfo, CompressionStatus } from '../core/turn.js'; import { DEFAULT_TOKEN_LIMIT } from '../core/tokenLimits.js'; import { getCompressionPrompt } from '../core/prompts.js'; -import { createAbortController } from '../utils/abortController.js'; import { runSideQuery } from '../utils/sideQuery.js'; import { logChatCompression } from '../telemetry/loggers.js'; import { makeChatCompressionEvent } from '../telemetry/types.js'; @@ -385,7 +384,7 @@ export class ChatCompressionService { config: { thinkingConfig: { includeThoughts: true }, }, - abortSignal: signal ?? createAbortController().signal, + abortSignal: signal ?? new AbortController().signal, promptId, }); const summary = summaryResult.text; diff --git a/packages/core/src/services/chatRecordingService.ts b/packages/core/src/services/chatRecordingService.ts index 9a838c393f4..8145bf73099 100644 --- a/packages/core/src/services/chatRecordingService.ts +++ b/packages/core/src/services/chatRecordingService.ts @@ -17,7 +17,6 @@ import { createModelContent, } from '@google/genai'; import * as jsonl from '../utils/jsonl-utils.js'; -import { createAbortController } from '../utils/abortController.js'; import { getGitBranch } from '../utils/gitUtils.js'; import { createDebugLogger } from '../utils/debugLogger.js'; import type { AttributionSnapshot } from './commitAttribution.js'; @@ -961,7 +960,7 @@ export class ChatRecordingService { if (!fastModel) return; this.autoTitleAttempts++; - const controller = createAbortController(); + const controller = new AbortController(); this.autoTitleController = controller; void (async () => { diff --git a/packages/core/src/tools/agent/agent.ts b/packages/core/src/tools/agent/agent.ts index 942ff8c7f78..ba871d3c4a0 100644 --- a/packages/core/src/tools/agent/agent.ts +++ b/packages/core/src/tools/agent/agent.ts @@ -67,10 +67,6 @@ import type { AgentUsageEvent, } from '../../agents/runtime/agent-events.js'; import { BuiltinAgentRegistry } from '../../subagents/builtin-agents.js'; -import { - createAbortController, - createChildAbortController, -} from '../../utils/abortController.js'; import { createDebugLogger } from '../../utils/debugLogger.js'; import { PermissionMode } from '../../hooks/types.js'; import type { StopHookOutput } from '../../hooks/types.js'; @@ -1721,9 +1717,9 @@ class AgentToolInvocation extends BaseToolInvocation { } } - // Independent AbortController — background agents survive ESC - // cancellation of the parent's current turn, so no parent linkage. - const bgAbortController = createAbortController(); + // Create an independent AbortController — background agents + // survive ESC cancellation of the parent's current turn. + const bgAbortController = new AbortController(); // Background agents have no UI, so interactive permission prompts must be // auto-denied rather than auto-approved (YOLO). PermissionRequest hooks @@ -2113,10 +2109,17 @@ class AgentToolInvocation extends BaseToolInvocation { } // ── Foreground (synchronous) execution path ──────────────── - // Child AbortController so the dialog's per-agent cancel aborts only - // this subagent. Parent abort still propagates down (ESC at the parent - // kills the subagent); child abort does NOT propagate up. - const fgAbortController = createChildAbortController(signal); + // Compose a child AbortController so the dialog's per-agent cancel + // can abort just this subagent without aborting the parent turn. + // Parent abort still propagates down (so ESC at the parent kills + // the subagent), but child abort does NOT propagate up. + const fgAbortController = new AbortController(); + const onParentAbort = () => fgAbortController.abort(); + if (signal?.aborted) { + fgAbortController.abort(); + } else { + signal?.addEventListener('abort', onParentAbort, { once: true }); + } const fgHookOpts = { ...hookOpts, signal: fgAbortController.signal }; const runFramed = () => @@ -2313,8 +2316,7 @@ class AgentToolInvocation extends BaseToolInvocation { } this.eventEmitter.off(AgentEventType.TOOL_CALL, onFgToolCall); this.eventEmitter.off(AgentEventType.USAGE_METADATA, onFgUsageMetadata); - // Reverse cleanup detaches our parent-signal listener. - fgAbortController.abort(); + signal?.removeEventListener('abort', onParentAbort); cleanupOwnedMonitorNotifications(); // Release the JSONL writer's listeners and close the fd before // patching the meta sidecar — closing first guarantees the diff --git a/packages/core/src/tools/monitor.ts b/packages/core/src/tools/monitor.ts index d7ad8344538..a1eb2b3ac9d 100644 --- a/packages/core/src/tools/monitor.ts +++ b/packages/core/src/tools/monitor.ts @@ -41,7 +41,6 @@ import { normalizeMonitorCommand as normalizeMonitorShellCommand, splitCommands, } from '../utils/shell-utils.js'; -import { createAbortController } from '../utils/abortController.js'; import { createDebugLogger } from '../utils/debugLogger.js'; import { isSubpaths } from '../utils/paths.js'; import { @@ -304,7 +303,7 @@ class MonitorToolInvocation extends BaseToolInvocation< // Independent AbortController — pressing Ctrl+C on the current turn // should NOT kill a long-running monitor the user intentionally started. - const entryAc = createAbortController(); + const entryAc = new AbortController(); const registration: MonitorTaskRegistration = { monitorId, diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts index e16c6d66c5c..095c2a4b41e 100644 --- a/packages/core/src/tools/shell.ts +++ b/packages/core/src/tools/shell.ts @@ -53,7 +53,6 @@ import { stripShellWrapper, } from '../utils/shell-utils.js'; import { parse } from 'shell-quote'; -import { createAbortController } from '../utils/abortController.js'; import { createDebugLogger } from '../utils/debugLogger.js'; import { isShellCommandReadOnlyAST, @@ -1512,7 +1511,7 @@ export class ShellToolInvocation extends BaseToolInvocation< // ShellExecutionService detects the discriminated reason and // returns `result.promoted: true` instead of killing the child — // see #3842 / #3886 for the foundation. - const promoteAbortController = createAbortController(); + const promoteAbortController = new AbortController(); let combinedSignal = AbortSignal.any([ signal, promoteAbortController.signal, @@ -2362,7 +2361,7 @@ export class ShellToolInvocation extends BaseToolInvocation< // SIGTERM → SIGKILL ourselves (mirroring the kill semantics // `ShellExecutionService.execute()`'s abort handler uses for the // non-promote path) and to mark the registry entry `cancelled`. - const entryAc = createAbortController(); + const entryAc = new AbortController(); const cancelChild = async () => { const pid = result.pid; if (pid !== undefined) { @@ -2770,7 +2769,7 @@ export class ShellToolInvocation extends BaseToolInvocation< // The `signal` parameter is still honored for the synchronous early // return below (don't even spawn if the agent already aborted), but // we deliberately do not forward it. - const entryAc = createAbortController(); + const entryAc = new AbortController(); const outputStream = fs.createWriteStream(outputPath, { flags: 'w' }); // Without an 'error' listener, a write failure (disk full, permission diff --git a/packages/core/src/utils/fetch.ts b/packages/core/src/utils/fetch.ts index cd97df5c2b6..eb358a8f3fd 100644 --- a/packages/core/src/utils/fetch.ts +++ b/packages/core/src/utils/fetch.ts @@ -4,7 +4,6 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { createAbortController } from './abortController.js'; import { getErrorMessage, isNodeError } from './errors.js'; import { URL } from 'node:url'; @@ -62,7 +61,7 @@ export async function fetchWithTimeout( timeout: number, headers?: Record, ): Promise { - const controller = createAbortController(); + const controller = new AbortController(); const timeoutId = setTimeout(() => controller.abort(), timeout); try {