Skip to content
Merged
Show file tree
Hide file tree
Changes from 13 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`, 18 tests + 1 GC test skipped under non-`--expose-gc`).
Comment thread
doudouOUC marked this conversation as resolved.
Outdated
- All warningHandler tests pass (`warningHandler.test.ts`, 9 tests).
- Existing combinedAbortSignal tests pass against the deprecation shim (8 tests).
- All agent runtime / followup / openaiContentGenerator / hooks tests pass.
- Migration completeness: `grep -rn "new AbortController" packages/core/src --include="*.ts" | grep -v test | grep -v abortController.ts` returns **empty**.
- TypeScript strict-mode typecheck passes for both `packages/core` and `packages/cli`.
- Prettier check passes on all modified files.

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

## How to capture the artifacts for the PR body

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

```sh
$ grep -rn "new AbortController" packages/core/src --include="*.ts" \
| grep -v test | grep -v abortController.ts
(no output)
```

Every production `new AbortController()` call in `packages/core/src` (outside
the helper module itself and outside test files) has been migrated to use
`createAbortController` / `createChildAbortController`. See
`migration-completeness.txt` for the captured grep.
Comment thread
doudouOUC marked this conversation as resolved.
Outdated

## 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,3 @@
$ grep -rn "new AbortController" packages/core/src --include="*.ts" \
| grep -v test | grep -v abortController.ts
(no output)
23 changes: 23 additions & 0 deletions docs/verification/abort-controller-refactor/scripts/02-lite.sh
Original file line number Diff line number Diff line change
@@ -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"
Loading
Loading