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