Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
08ed287
fix(core): consolidate AbortController handling to stop listener leak…
doudouOUC May 20, 2026
aff30bc
fix(core): address PR #4366 review feedback
doudouOUC May 21, 2026
8e42dc7
test(core): fix abortController race-defense test to actually hit the…
doudouOUC May 21, 2026
0dc689a
Merge remote-tracking branch 'origin/main' into worktree-joyful-honki…
doudouOUC May 21, 2026
082db37
fix(lint): include docs/**/*.mjs in the script ESLint block so the Ab…
doudouOUC May 21, 2026
832355e
fix(core,cli): address PR #4366 critical review findings
doudouOUC May 21, 2026
874d4a9
fix(core,cli): address PR #4366 review — fix combineAbortSignals orph…
doudouOUC May 21, 2026
733c85c
test(core): strengthen the timeout-guard test in combineAbortSignals …
doudouOUC May 21, 2026
c840346
test(core): cover timeout-triggered cleanup of input-signal listeners…
doudouOUC May 21, 2026
d95a18e
fix(test): use pathToFileURL for the warning-handler e2e import on Wi…
doudouOUC May 21, 2026
94e8c58
fix(core,cli): address PR #4366 review batch — onAbort leak, migrate …
doudouOUC May 21, 2026
31784f4
docs(verification): commit the headless-scenario scripts referenced b…
doudouOUC May 21, 2026
5aa7110
docs(verification): sync automated-results with current state
doudouOUC May 21, 2026
b4f36d4
test(core): pin two abort-cascade behaviors PR #4366 introduced
doudouOUC May 21, 2026
e31432f
fix(test): speculation.test.ts TS errors + sync verification doc counts
doudouOUC May 21, 2026
9323bc3
refactor(core): narrow PR #4366 scope per yiliang's review — revert i…
doudouOUC May 25, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
120 changes: 120 additions & 0 deletions docs/verification/abort-controller-refactor/README.md
Original file line number Diff line number Diff line change
@@ -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 >> <log>'`.

## Setup once

```sh
# Point WT at your local checkout of the branch under review.
WT=/path/to/qwen-code/worktree
LOGDIR=$WT/docs/verification/abort-controller-refactor/logs
mkdir -p "$LOGDIR"

# Build the CLI once (skip sandbox image, skip vscode).
( cd "$WT" && npm run build:packages )
```

## Scenarios

For each scenario:

```sh
tmux new-session -d -s qwen-verify-XX
tmux pipe-pane -t qwen-verify-XX -o "cat >> $LOGDIR/XX-name.log"
tmux send-keys -t qwen-verify-XX "cd /path/to/your/test/workspace && exec node $WT/packages/cli/dist/index.js" C-m
tmux attach -t qwen-verify-XX
```

Then drive the session manually per the matrix below. Hit `C-b d` to detach
when done; `tmux kill-session -t qwen-verify-XX` to stop the pane.

### 00 — Baseline (PRE-fix)

- **Setup:** check out `main`, build, run with `NODE_OPTIONS=--trace-warnings`.
- **Input:** long 50-round mixed-tool session (shell + edit + grep + agent).
- **Expected:** after ~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 <pid>`.
- **Expected:** clean shutdown, exit code 130, no warnings.
- **Log:** `06-headless-abort.log`.

### 07 — Background agent flow

- **Setup:** interactive.
- **Input:** spawn a background agent (`run_in_background: true`); let it complete; spawn a second one; cancel the second mid-flight.
- **Expected:** first agent completes normally; second aborts cleanly; no listener leak across the two.
- **Log:** `07-background-agent.log`.

### 08 — Memory baseline

- **Setup:** `qwen --inspect`, attach Chrome devtools.
- **Input:** 100-round session.
- **Expected:** heap snapshots at round 0/50/100. `AbortSignal` instance count and per-signal listener count stable (no monotonic growth).
- **Log:** `08-memory-snapshots/`.

### 09 — Existing combinedAbortSignal consumer

- **Setup:** trigger an HTTP hook with both an external signal and timeout.
- **Input:** (a) cancel external signal mid-hook; (b) let timeout fire in a separate run.
- **Expected:** hook aborts cleanly in both cases; deprecation shim path is exercised.
- **Log:** `09-http-hook-shim.log`.

## Automated (non-interactive) verifications

The automated checks below were run during development and recorded in
`automated-results.md`:

- All abortController unit tests pass (`abortController.test.ts`, 26 tests; 1 GC test skipped under non-`--expose-gc`).
- All warningHandler tests pass (`warningHandler.test.ts`, 13 tests including a spawned-child stderr integration test).
- All `combineAbortSignals` consumer tests pass (`httpHookRunner.test.ts`); the deprecated `createCombinedAbortSignal` shim plus its own test file were removed once the lone caller migrated.
- All agent runtime / followup / openaiContentGenerator / hooks tests pass.
- Migration scope (intentional): only the agent-runtime parent→child chain (`agent-interactive.ts`, `agent-core.ts`, `agent-headless.ts`) plus `promptHookRunner.ts` (real cleanup leak) was switched to the helper. Independent short-lived controllers (per-shell-command, per-fetch, per-recall, etc.) stay on raw `new AbortController()` — they're GC'd quickly and don't accumulate listeners on a long-lived parent. See `migration-completeness.txt` for the captured grep + rationale.
- TypeScript strict-mode typecheck passes for both `packages/core` and `packages/cli`.
- Prettier check passes on all modified files.

See `automated-results.md` for the actual command output.

## How to capture the artifacts for the PR body

After running each scenario, attach the transcript file (or relevant excerpt)
to the PR. For #08 (memory), export the heap snapshots and include the
listener-count delta between snapshots.
139 changes: 139 additions & 0 deletions docs/verification/abort-controller-refactor/automated-results.md
Original file line number Diff line number Diff line change
@@ -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.

Comment thread
doudouOUC marked this conversation as resolved.
## 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.
Original file line number Diff line number Diff line change
@@ -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();
Comment thread
doudouOUC marked this conversation as resolved.
setMaxListeners(maxListeners, c.signal);
return c;
}
function createChildAbortController(parent) {
const child = createAbortController();
if (!parent) return child;
const parentSignal = parent.signal ?? parent;
if (parentSignal.aborted) {
Comment thread
doudouOUC marked this conversation as resolved.
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);
Original file line number Diff line number Diff line change
@@ -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,
Loading
Loading