Skip to content

fix(test): wait for interactive PTY sessions to end during cleanup - #11001

Open
qwen-code-dev-bot wants to merge 1 commit into
mainfrom
autofix/issue-10990
Open

fix(test): wait for interactive PTY sessions to end during cleanup#11001
qwen-code-dev-bot wants to merge 1 commit into
mainfrom
autofix/issue-10990

Conversation

@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

What this PR does

Makes the interactive test harness wait for each terminal session it ends, instead of signalling it and moving on. The rig already remembered every pseudo-terminal child it spawned and terminated the ones a test never closed; it now also blocks until each of those children has actually exited, within a bound that sits above the CLI's own shutdown ceiling so a child that refuses to die cannot hang teardown. The bound's timer leaves nothing behind that would keep the worker's event loop alive.

The regression test that covers this was strengthened in place rather than duplicated. Its stand-in for the CLI now behaves like the real one — it traps the termination signal and exits only after a delay — and it announces that it has finished booting before the harness is allowed to signal it. The test asserts on the wait itself, so it goes red when either half of the guard is removed: the signal, or the wait for it to take effect.

Why it's needed

The E2E Interactive - OpenTUI renderer (bun) leg keeps reddening main without naming a single failing test. It did so in six of the last nine runs, and one of those six was the very commit that landed #10971 to fix it — so that repair narrowed the class but did not close it. The failing step runs just as long as a healthy one, which says the suite completes and the process dies afterwards; and the log carries no failure line at all, which is what makes the detector file per commit instead of per test. A run that passes everything and still exits non-zero is an unhandled error.

#10971 correctly identified the mechanism: a session a test never closed stays alive to the end of the run, still forwarding every terminal byte into the worker's standard output because the same environment enables that verbose forwarding, and once vitest tears the worker down the reader end of that pipe is gone — the next write raises EPIPE, which Node escalates to an uncaught exception. What it missed is that signalling a session is not the same as ending it. The CLI traps the hangup signal for any interactive session, whatever the renderer, and exits only after an asynchronous shutdown chain has drained: chat-recording flush, MCP subprocess stop, telemetry shutdown, session-usage persisting, all bounded by a five-second wall clock. So the harness returned from teardown with the child still alive and still writing, and the window #10971 set out to close stayed open.

Both halves were measured rather than assumed. Against the real bundle, the child is still alive at the instant the kill call returns, and exits 83ms later with exit code 129 — the CLI's own code for a hangup it handled itself. And in a whole-leg run on the parent commit, watching the process table caught a CLI child being reparented to init at the moment its vitest worker exited, which is precisely the instant an EPIPE is fatal rather than harmless; its lifetime and its sibling's line up with the two test durations in the file that starts sessions and never closes them. After this change the same measurement finds no orphans at all, across two whole-leg runs, with an identical set of passing tests.

The reason the earlier witness did not catch this is worth recording, because it is the reason the fix shipped green once already. Its stand-in had no signal handler, so it died instantly on the default action, and it asserted through a poll with a ten-second timeout — a poll that is perfectly happy for the child to outlive teardown by up to ten seconds. The property that actually matters, "gone by the time teardown returns", was never pinned by anything.

Reviewer Test Plan

How to verify

The load-bearing claim is that no interactive session is still alive when teardown returns, and the regression test in the rig's own test file is the whole of it. On this branch it passes in about a second. Check out the parent commit, apply only the test change, and it fails reporting that teardown returned before the child exited — zero milliseconds measured against a 750ms floor. It needs no model credentials, no bun, and no network, because the stand-in is a short script rather than the CLI.

To confirm both halves of the guard are witnessed, delete each in turn and re-run that file. Removing the wait fails on the duration floor. Removing the kill fails on the survival poll, after the bound and the poll have both expired. Restoring either returns it to green.

The wider suite should be unchanged: run the interactive leg and compare against main, expecting the same ten files collected and the same eighteen tests passing with the same two skips, and no new ones. This matters most for the sessions that end by themselves — the Ctrl+C exit case and the mid-turn quit cases — since teardown now waits on children those tests already terminated, and for those the wait resolves immediately because the exit has already been observed.

It is also worth watching the process table while the leg runs, which is how the defect was caught. On main a CLI child outlives its worker and is reparented to init; on this branch none is.

The cost is small and measurable: the wait is each session's real shutdown, 35–42ms in measurement, and the one file that leaks a session grew by 38ms. Whole-leg wall clock is dominated by a live-model compression file whose individual tests swing between 71s and 107s run to run, so compare per-file timings rather than the total when judging whether this change slowed anything.

The OpenTUI leg itself is the final check and needs bun; it could not be run where this change was prepared. Because the failure is intermittent — the leg passed at the commit immediately after the one this issue was filed against, before any of this work — a single green run proves little on its own. The meaningful signal is whether the "exit code 1, no failing test" shape stops recurring over a run of merges.

Evidence (Before & After)

Non-UI change, so no screenshots. The measured before/after is the process table during a whole interactive-leg run:

  • Before (parent commit): one CLI child reparented to init — ppid 2130 → 1 — at the last sample of the run, i.e. at worker teardown. Whole-leg result: 9 passed | 1 skipped (10) files, 18 passed | 2 skipped (20) tests, exit 0.
  • After (this branch, two separate whole-leg runs): 0 orphans, 0 survivors fifteen seconds after the run. Whole-leg result identical: 9 passed | 1 skipped (10) files, 18 passed | 2 skipped (20) tests, exit 0.
  • Direct measurement against the real bundle: alive immediately after kill(): true, then exited after 83ms exitCode=129 signal=0, where 129 is the CLI's own handled-hangup exit code.
  • Regression test, wait removed: 1 failed | 6 passed (7)cleanup() returned before the interactive CLI child exited: expected 0 to be greater than or equal to 750. Regression test, kill removed: 1 failed | 6 passed (7)Matcher did not succeed in time. Both restored: 7 passed (7).

Tested on

OS Status
🍏 macOS ⚠️
🪟 Windows ⚠️
🐧 Linux

Environment (optional)

Linux (Node 22) inside a container, QWEN_SANDBOX=false, QWEN_E2E_RENDERER=ink, against the bundled dist/cli.js, with the runner-environment variable unset so unhandled errors stay fatal exactly as they are on the OpenTUI leg. The opentui leg was not run: bun is unavailable there and the renderer matrix throws without it. The defect and the fix are renderer-independent — the signal handler is installed for any interactive session, gated only on interactivity — but the leg that reddens is the one that could not be executed here.

Risk & Scope

  • Main risk or tradeoff: teardown now blocks until each leaked session exits, so a child that never dies would add the full bound to that test's teardown, and hook time counts against the test timeout. The bound is deliberately above the CLI's own five-second shutdown ceiling, the measured reality is tens of milliseconds, and a child that ignores the signal entirely is not one this suite produces. Before the change such a child leaked silently and could redden the whole run without naming a test; now the worst case is bounded and visible. Sessions a test already closed cost nothing, because their exit has been observed before teardown runs.
  • Not validated / out of scope: the OpenTUI leg under bun, and the sandbox:none shards, which need the self-hosted pool. Also deliberately untouched is whether github-hosted Linux should keep treating unhandled errors as fatal — that leg is the only Linux lane with the exemption off, so any other unhandled error is fatal there alone, and whether that is the right signal policy is a maintainer call, as fix(test): end interactive PTY sessions a test never closed #10971 also judged. This change removes one proven source; it is not a guarantee the leg stops reddening. Separately noted and not implemented: one interactive file carries its own copy of the launcher and never applies the renderer overlay, so on the OpenTUI leg it drives the CLI under node with the default renderer rather than under bun with the pinned one, sitting outside the guarantee the renderer matrix exists to enforce. It is not implicated here — it closes its own session and waits — and moving it onto bun would change what that file exercises. The second job named in the issue, a sandbox:none shard, was the documented transient shared-host pressure class whose one-shot retry was starved by a per-leg build that main has since removed; the same shard failing afterwards is tracked by Main CI failed: E2E Tests on d4e3e4fc8747 #10994.
  • Breaking changes / migration notes: none. The change is confined to the test harness and adds no production behaviour.

Linked Issues

Fixes #10990

中文说明

这个 PR 做了什么

让交互式测试框架等待它结束的每一个终端会话,而不是发个信号就走。rig 本来就会记住它生成的每一个伪终端子进程,并终止那些测试没有关闭的;现在它还会阻塞等待这些子进程真正退出,上界设在高于 CLI 自身关闭天花板的位置,因此一个拒绝死掉的子进程不会把 teardown 挂死。该上界使用的 timer 不会留下任何撑住 worker 事件循环的东西。

覆盖这一点的回归测试是就地加强的,而不是另写一个。它替代 CLI 的替身现在行为与真实 CLI 一致 —— 捕获终止信号,并且只在一段延迟之后退出 —— 并且在框架被允许向它发信号之前,先宣告自己已经启动完毕。测试断言的是"等待"本身,因此移除守卫的任意一半都会让它变红:发信号,或等待信号生效。

为什么需要它

E2E Interactive - OpenTUI renderer (bun) 这个 leg 一直在让 main 变红,却不指出任何一个失败的测试。最近九次运行里有六次如此,而这六次中有一次正是为修复它而合入 #10971 的那个 commit —— 所以那次修复收窄了这一类问题,却没有关闭它。失败步骤的耗时与健康步骤相当,说明套件是跑完了、之后进程才死掉;而日志里完全没有失败行,这正是检测器按 commit 而不是按测试来记录的原因。一个所有测试都通过却仍以非零码退出的 run,是 unhandled error。

#10971 正确识别了机制:测试没有关闭的会话会一直活到 run 结束,并且因为同样的环境设置开启了冗长转发,它仍在把每一个终端字节转发进 worker 的标准输出;一旦 vitest 拆除 worker,该管道的读取端就消失了 —— 下一次写入产生 EPIPE,Node 将其升级为未捕获异常。它漏掉的是:向会话发信号并不等于结束会话。CLI 对任何交互式会话都会捕获 hangup 信号,无论使用哪个渲染器,并且只有在一条异步关闭链排空之后才退出:chat-recording flush、MCP 子进程停止、telemetry shutdown、session-usage 持久化,全部由一个五秒的墙钟上界约束。因此框架从 teardown 返回时子进程仍然活着、仍在写入,#10971 想要关闭的那个窗口依然开着。

两部分都是实测得到的,不是假设。针对真实 bundle,子进程在 kill 调用返回的那一刻仍然活着,并在 83 毫秒后以退出码 129 结束 —— 那是 CLI 自己处理 hangup 时使用的退出码。而在父提交上的一次整 leg 运行中,监视进程表抓到了一个 CLI 子进程在其 vitest worker 退出的那一刻被 reparent 给 init,而那恰恰是 EPIPE 致命而非无害的瞬间;它的存活时长与它同胞进程的时长,与那个"启动会话却从不关闭"的文件里两个测试的耗时对得上。改动之后,同样的测量在两次整 leg 运行中都没有发现任何孤儿进程,且通过的测试集合完全一致。

早先那个 witness 为什么没抓到,值得记录下来,因为这正是一次修复已经"绿着"上线的原因。它的替身没有信号 handler,因此会以默认动作立刻死掉;而且它通过一个十秒超时的 poll 来断言 —— 这个 poll 完全乐意接受子进程比 teardown 多活最多十秒。真正要紧的性质"到 teardown 返回时已经消失",从来没有任何东西把它固定下来。

Reviewer 测试计划

如何验证

承重的主张是:teardown 返回时没有任何交互式会话仍然活着,而 rig 自己测试文件里的回归测试就是它的全部。在本分支上它大约一秒通过。切到父提交,只应用测试改动,它会失败并报告 teardown 在子进程退出之前就返回了 —— 实测 0 毫秒,对照 750 毫秒的下限。它不需要模型凭据、不需要 bun、不需要网络,因为替身是一段短脚本而不是 CLI。

要确认守卫的两半都有 witness,逐个删除并重跑该文件。移除"等待"会在耗时下限上失败。移除"发信号"会在存活 poll 上失败,且是在上界与 poll 都到期之后。恢复任意一个都会回到绿色。

更大的套件应当保持不变:运行 interactive leg 并与 main 对比,期望收集到同样的十个文件、通过同样的十八个测试、跳过同样的两个,且没有新增跳过。这一点对那些本应自行结束的会话最为重要 —— Ctrl+C 退出用例,以及 mid-turn 的 quit 用例 —— 因为 teardown 现在会等待这些测试已经终止过的子进程,而对它们来说等待会立刻解除,因为退出早已被观察到。

也值得在该 leg 运行期间观察进程表,这正是缺陷被抓到的方式。在 main 上,一个 CLI 子进程比它的 worker 活得更久并被 reparent 给 init;在本分支上一个都没有。

代价很小且可测:等待就是每个会话真实的关闭耗时,实测 35–42 毫秒,而唯一泄漏会话的那个文件增长了 38 毫秒。整 leg 的墙钟时间由一个真实模型的压缩文件主导,它的单个测试在不同 run 之间会在 71 秒到 107 秒之间摆动,所以判断本改动是否拖慢了任何东西时,请对比各文件耗时而不是总时长。

OpenTUI leg 本身是最终检查,需要 bun;在准备这一改动的环境里无法运行。由于失败是间歇性的 —— 该 leg 在本 issue 所针对 commit 的下一个 commit 上、在这些工作开始之前就通过了 —— 单独一次绿色运行说明不了太多。有意义的信号是:"退出码 1、无失败测试"这个形态是否在若干次合并之后不再复现。

证据(前后对比)

非 UI 改动,因此没有截图。测得的前后对比是整 interactive leg 运行期间的进程表:

  • 修复前(父提交):一个 CLI 子进程被 reparent 给 init —— ppid 2130 → 1 —— 出现在整个 run 的最后一次采样,也就是 worker 拆除时。整 leg 结果:9 passed | 1 skipped (10) 个文件、18 passed | 2 skipped (20) 个测试、exit 0。
  • 修复后(本分支,两次独立的整 leg 运行):0 个孤儿,run 结束十五秒后 0 个残留。整 leg 结果完全相同:9 passed | 1 skipped (10) 个文件、18 passed | 2 skipped (20) 个测试、exit 0。
  • 针对真实 bundle 的直接测量:alive immediately after kill(): true,随后 exited after 83ms exitCode=129 signal=0,其中 129 是 CLI 自己处理 hangup 的退出码。
  • 回归测试,移除等待:1 failed | 6 passed (7) —— cleanup() returned before the interactive CLI child exited: expected 0 to be greater than or equal to 750。回归测试,移除 kill:1 failed | 6 passed (7) —— Matcher did not succeed in time。两者都恢复后:7 passed (7)

测试环境

OS Status
🍏 macOS ⚠️
🪟 Windows ⚠️
🐧 Linux

环境(可选)

容器内的 Linux(Node 22),QWEN_SANDBOX=falseQWEN_E2E_RENDERER=ink,针对打包后的 dist/cli.js 运行,并 unset runner-environment 变量,使 unhandled error 保持致命,与 OpenTUI leg 完全一致。opentui leg 未运行:该环境中没有 bun,渲染器矩阵在缺少它时会抛错。缺陷与修复都与渲染器无关 —— 信号 handler 对任何交互式会话都会安装,只以"是否交互式"为条件 —— 但变红的恰恰是这里无法执行的那个 leg。

风险与范围

  • 主要风险或取舍:teardown 现在会阻塞到每个泄漏的会话退出为止,因此一个永不死掉的子进程会给该测试的 teardown 增加完整的上界时长,而 hook 时间是计入测试超时的。上界刻意设在高于 CLI 自身五秒关闭天花板的位置,实测情况是数十毫秒,而完全忽略信号的子进程并不是本套件会产生的。改动之前,这样的子进程会静默泄漏,并可能在不指出任何测试的情况下让整个 run 变红;现在最坏情况是有界且可见的。测试已经自行关闭的会话不产生任何代价,因为它们的退出在 teardown 运行之前就已被观察到。
  • 未验证 / 范围之外:bun 下的 OpenTUI leg,以及需要 self-hosted 池的 sandbox:none 各 shard。同样刻意未触碰的是:github-hosted Linux 是否应继续把 unhandled error 判为致命 —— 该 leg 是唯一关闭豁免的 Linux 通道,因此任何其它 unhandled error 都只在它这里是致命的,而这是否是正确的信号策略属于维护者的决定,fix(test): end interactive PTY sessions a test never closed #10971 也做了同样判断。本改动移除了一个被证明的来源;它不保证该 leg 不再变红。另外记录但未实现:有一个交互式文件自带一份启动器副本,且从不套用渲染器 overlay,因此在 OpenTUI leg 上它是用 node 加默认渲染器驱动 CLI,而不是用 bun 加被钉住的渲染器,落在渲染器矩阵本要保证的范围之外。它与本次问题无关 —— 它自己关闭会话并等待 —— 而把它搬到 bun 上会改变该文件实际验证的内容。issue 中指出的第二个 job,一个 sandbox:none shard,属于已记录在案的共享宿主压力瞬时类别,它的一次性重试被 per-leg 构建耗尽了预算,而 main 此后已移除该构建;同一 shard 在此之后仍然失败,由 Main CI failed: E2E Tests on d4e3e4fc8747 #10994 跟踪。
  • 破坏性变更 / 迁移说明:无。改动仅限于测试框架,不新增任何生产行为。

关联 Issue

Fixes #10990

…10990)

Cleanup signalled each leaked session but returned without waiting for it
to go away. The CLI traps SIGHUP and exits only once runExitCleanup() has
drained, a chain it bounds at 5s, so kill() returns with the child still
alive and still forwarding PTY bytes into the worker's stdout — measured at
83ms for a booted session, exiting with the CLI's SIGHUP code 129.

That is the window #10969 was meant to close. A full interactive leg run on
the parent commit shows a CLI child reparented to init at the moment its
vitest worker exited; the same run after this change orphans none, with an
identical result set. The wait costs each session's real drain (35-42ms
measured) and is bounded above the CLI's own 5s ceiling.

The witness now pins the wait itself. Its stand-in traps SIGHUP and exits
after a delay like the real CLI, and reports itself booted first: signalling
a child that has not installed its handler ends it on the default action,
which measured nothing. Deleting the wait turns it red at 0ms against a
750ms floor; deleting the kill turns it red on the survival poll.
@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator Author

E2E report — issue #10990 (Main CI failed: E2E Tests on b7815a7)

What the issue names, and what each half turned out to be

Run 33829764813 at b7815a7e1a reddened two jobs. Neither printed a FAIL line, which is why the detector filed per commit instead of naming a test — it matches ^FAIL\s+ after stripping ANSI and the Actions timestamp, and found nothing.

E2E Interactive - OpenTUI renderer (bun) — this is a recurring failure, not a one-off. That leg failed in runs 33797332289, 33806428062, 33813966397, 33820259657 (the commit carrying the #10969 repair itself), 33829764813 and 33830499451, interleaved with passes at 33795521868, 33811905769 and 33831058473. So #10971 narrowed the class but did not close it. The failing step ran 196s against a healthy 86–192s, i.e. the suite ran to completion and the process then died. This half is what the change below repairs.

E2E Test (Linux) - sandbox:none - shard 2/3 — the job's own annotation states the outcome: sandbox:none shard failed on ecs-qwen-hk4-28 after 2334s of the 3600s job budget — not enough left for a retry. That is the transient shared-host pressure class the workflow already documents and already has a one-shot retry for; the retry never ran because the budget gate needs ≤2100s elapsed, and this tree still built on the leg (step 11 Build project alone took 12m40s, so setup consumed 16m29s of the 60-minute job). main has since taken #10894, which builds once on a hosted runner and unpacks on every leg: at d4e3e4fc87 the same job's setup fell to 2m36s, attempt 1 failed at 1444s, and the retry did fire (retrying once (transient shared-host pressure class)). Both attempts still failed there, and that run is tracked separately by #10994, which already carries autofix/in-progress. No code change in this PR touches that half, and .github/ is deliberately left alone.

Root cause of the interactive-leg failure

#10971 made TestRig.cleanup() kill every session runInteractive() spawned. It signals, but it does not wait — and signalling is not the same as ending.

The CLI installs process.on('SIGHUP', …) for any interactive session regardless of renderer, and node-pty's kill() defaults to SIGHUP. That handler runs an exit-cleanup chain — chat-recording flush, config.shutdown() (which stops MCP subprocesses), telemetry shutdown, session-usage persisting — bounded by a 5s wall clock, and only then calls process.exit(129). So cleanup() returned while the child was still alive and still forwarding PTY bytes into the worker's process.stdout under the VERBOSE/KEEP_OUTPUT this leg sets. Vitest then tore the worker down around a live child, which is exactly the EPIPE-on-a-destroyed-stdout-pipe path #10969 described.

Measured against the real bundle rather than inferred:

PROBE alive immediately after kill(): true
PROBE exited after 83ms exitCode=129 signal=0

exitCode=129 is the CLI's own getSignalExitCode('SIGHUP'), which confirms the trapped-graceful path and not the default action.

A full interactive-leg run on the parent commit, watching the process table, caught the consequence directly — a CLI child whose vitest worker exited underneath it and was reparented to init:

[t=83]    3678    2130      82 node   node …/dist/cli.js --no-chat-recording --yolo
[t=84]    3678       1      84 node   node …/dist/cli.js --no-chat-recording --yolo

Its lifetime (84s) and its sibling's (76s) match that file's two test durations in the same run (84.5s and 77.8s), so these are the sessions context-compress-interactive.test.ts starts and never closes. The orphan appeared at the last sample of the run — the exact moment the worker tears down, which is when an EPIPE is fatal rather than harmless.

Why #10971's witness did not catch this: its stand-in was setInterval(() => {}, 1000), which has no SIGHUP handler and so dies on the default action instantly, and it asserted with expect.poll(…, { timeout: 10_000 }) — a poll that tolerates the child surviving cleanup() for up to ten seconds. The property that matters, "dead by the time cleanup() returns", was never pinned.

The change

cleanup() now waits for each session it signalled to actually exit, bounded above the CLI's own 5s drain ceiling so a pathological child cannot hang teardown. The wait resolves on node-pty's exit event; the bound's timer is cleared and unrefed so a won race leaves no handle holding the worker's event loop open — a lingering timer here would recreate the very "worker cannot exit" condition being fixed.

The witness was strengthened in place rather than added alongside, so it fails on both halves of the guard: the stand-in now traps SIGHUP and exits 750ms later like the real CLI, and reports itself booted first. That second part was necessary, not decorative — signalling a child before node has installed its handler ends it on the default action in ~3ms, which measured nothing and let the first version of this test fail against a correct fix.

Verification

Every command below was actually run in this checkout (161c784514), on Linux/Node 22.23.2 inside the Qwen sandbox container.

Required checks:

  • npm run build — passed (BUILD_EXIT=0)
  • npm run typecheck — passed (exit 0, including typecheck:integration)
  • npm run lint — passed (exit 0; eslint . --ext .ts,.tsx && eslint integration-tests)
  • npx prettier --check integration-tests/test-helper.ts integration-tests/test-helper.test.ts — passed ("All matched files use Prettier code style!")
  • Focused vitest, vitest run --root ./integration-tests test-helper.test --retry=07 passed (7), exit 0
  • Full interactive leg in the CI command shape (QWEN_E2E_RENDERER=ink QWEN_SANDBOX=false KEEP_OUTPUT=true VERBOSE=true vitest run --root ./integration-tests interactive --exclude '**/interactive/cron-interactive.test.ts' --exclude '**/channel-plugin.test.ts', RUNNER_ENVIRONMENT unset so dangerouslyIgnoreUnhandledErrors is false exactly as on the OpenTUI leg) — run three times, all exit 0 with an identical result set of 9 passed | 1 skipped (10) files and 18 passed | 2 skipped (20) tests: once on the parent commit as the baseline, once after the fix, and once after the fix against the freshly rebuilt bundle
  • vitest run --root ./integration-tests context-compress-interactive --retry=02 passed | 1 skipped (3), exit 0

Mutation probes (each guard has its own witness; the file was restored from a byte copy after each, and the restore was re-run to green):

  • Removed await settleWithin(exited, INTERACTIVE_EXIT_GRACE_MS) from cleanup() → witness FAILED: cleanup() returned before the interactive CLI child exited: expected 0 to be greater than or equal to 750. Restored → 7 passed.
  • Removed the ptyProcess.kill() block from cleanup() → witness FAILED: Matcher did not succeed in time after 20211ms (the 10s grace plus the 10s survival poll). Restored → 7 passed.

Orphan and survivor measurement, whole-leg runs with a 2s process-table sampler:

  • Parent commit: 1 CLI child reparented to init (pid 3678, ppid 2130 → 1) at worker teardown.
  • After the fix: 0 orphans and 0 survivors 15s after the run, on two separate whole-leg runs.

Cost of the wait, measured by instrumenting cleanup() (instrumentation removed afterwards, not committed):

PROBE cleanup waited 37ms for pid 7596
PROBE cleanup waited 42ms for pid 7598
PROBE cleanup waited 35ms for pid 7735

Independently corroborated by per-file timings: hooks-command.test.ts went 2509ms → 2547ms (+38ms, the measured wait). The whole-leg duration moved 166s → 190s → 308s across the three runs, and that swing is entirely context-compress-interactive.test.ts (162.3s → 187.1s → 303.9s) while every other file stayed inside ±250ms. That file drives live /compress model calls; its individual tests measured 71s, 90s, 107s and 106s across runs, so the variance is model latency, not teardown.

Not run, and why:

  • The OpenTUI leg itself. bun is not installed in this environment and resolveE2eCliCommand('opentui') throws without it. Everything above ran under QWEN_E2E_RENDERER=ink. The defect and the fix are renderer-independent — the SIGHUP handler is installed for any interactive session, gated only on config.isInteractive() — but the leg that reddens is the one that could not be executed here.
  • E2E Test (Linux) - sandbox:none shards. They need the self-hosted ECS pool; see the Main CI failed: E2E Tests on d4e3e4fc8747 #10994 note above.

Honest limits of this repair

This removes one proven source of unhandled errors on that leg, measured end to end. It is not a guarantee the leg stops reddening, for three reasons worth stating plainly:

  1. The leg passed at d4e3e4fc87 before this change, so a green OpenTUI run afterwards is not evidence for the fix. The meaningful signal is whether the "exit code 1, no failing test" shape stops recurring across several merges.
  2. That leg is the only Linux lane with dangerouslyIgnoreUnhandledErrors off (the shards moved to the self-hosted pool in ci: run the Linux E2E shards on the persistent pool #10085 and macOS is exempt by platform), so any other unhandled error is fatal there alone — including the onTaskUpdate RPC 60s-stall class the integration vitest config already documents. Whether hosted Linux should keep treating unhandled errors as fatal is a maintainer call about signal, and fix(test): end interactive PTY sessions a test never closed #10971 deliberately left it alone; this PR does too.
  3. One intermittent assertion failure in context-compress-interactive.test.ts was observed in a single instrumented run and did not reproduce when that file was run alone (2 passed | 1 skipped). It is a live-model test whose per-test duration swings by tens of seconds, and the change here touches only teardown timing (+38ms measured), so there is no causal path from it to a mid-test assertion. Recorded rather than quietly dropped.

One observation, deliberately not implemented

external-context-mem0-write.test.ts carries its own copy of the interactive launcher, which spawns process.execPath with env: process.env and never applies the renderer overlay. On the OpenTUI leg that file therefore drives the CLI under node with the ink default, not bun with QWEN_TUI_RENDERER=opentui plus QWEN_TUI_RENDERER_STRICT — so it sits outside the guarantee the renderer matrix exists to enforce. It is not implicated in this failure (it kills its own session in a finally and awaits the exit, so it never leaked), and moving it onto bun would change what that file exercises and could introduce failures unrelated to this issue. Flagged for a maintainer rather than folded into a CI-repair diff.

中文说明

E2E 报告 —— issue #10990(Main CI failed: E2E Tests on b7815a7

Issue 指出的两个 job,各自的真实性质

Run 33829764813(commit b7815a7e1a)有两个 job 变红。两者都没有打印任何 FAIL 行,这正是检测器按 commit 而不是按测试来记录的原因 —— 它在剥离 ANSI 与 Actions 时间戳之后匹配 ^FAIL\s+,什么都没匹配到。

E2E Interactive - OpenTUI renderer (bun) —— 这是反复出现的失败,不是偶发。该 leg 在 run 33797332289、33806428062、33813966397、33820259657(即携带 #10969 修复的那个 commit)、33829764813 和 33830499451 中都失败了,中间夹着 33795521868、33811905769 和 33831058473 的通过。所以 #10971 收窄了这一类问题,但没有关闭它。失败步骤耗时 196 秒,而健康区间是 86–192 秒,也就是说套件跑完了,然后进程才死掉。下面这个改动修的就是这一半。

E2E Test (Linux) - sandbox:none - shard 2/3 —— 该 job 自己的 annotation 已经说明了结果:sandbox:none shard failed on ecs-qwen-hk4-28 after 2334s of the 3600s job budget — not enough left for a retry。这正是 workflow 已经记录在案、并且已经配了一次重试的"共享宿主压力"瞬时类别;重试没有执行,是因为预算闸门要求 elapsed ≤2100s,而这棵树仍然在 leg 上自行构建(仅 step 11 Build project 就花了 12 分 40 秒,setup 一共吃掉了 60 分钟 job 预算里的 16 分 29 秒)。main 随后合入了 #10894,改为在托管 runner 上构建一次、各 leg 解包使用:在 d4e3e4fc87 上同一个 job 的 setup 降到 2 分 36 秒,第一次尝试在 1444 秒失败,重试确实触发了(retrying once (transient shared-host pressure class))。但两次尝试仍然都失败,那一次 run 由 #10994 单独跟踪,且该 issue 已带 autofix/in-progress。本 PR 没有任何代码改动涉及这一半,并且刻意没有碰 .github/

交互式 leg 失败的根因

#10971TestRig.cleanup() 去 kill runInteractive() 生成的每一个会话。但它只发信号,不等待 —— 而发信号并不等于结束。

CLI 对任何交互式会话都会注册 process.on('SIGHUP', …),与渲染器无关,而 node-pty 的 kill() 默认发送 SIGHUP。该 handler 会执行一条退出清理链 —— chat-recording flush、config.shutdown()(它会停掉 MCP 子进程)、telemetry shutdown、session-usage 持久化 —— 由一个 5 秒的墙钟上界约束,之后才调用 process.exit(129)。因此 cleanup() 返回时子进程仍然活着,并且在本 leg 设置的 VERBOSE/KEEP_OUTPUT 下继续把 PTY 字节转发进 worker 的 process.stdout。随后 vitest 在一个活着的子进程外面拆掉了 worker,而这恰恰就是 #10969 所描述的"向已销毁的 stdout 管道写入导致 EPIPE"路径。

针对真实 bundle 实测(而非推断):

PROBE alive immediately after kill(): true
PROBE exited after 83ms exitCode=129 signal=0

exitCode=129 正是 CLI 自己的 getSignalExitCode('SIGHUP'),证明走的是被捕获的优雅退出路径,而不是默认动作。

在父提交上跑一次完整的 interactive leg,同时监视进程表,直接抓到了后果 —— 一个 CLI 子进程,它的 vitest worker 在其身下退出,于是它被 reparent 给 init:

[t=83]    3678    2130      82 node   node …/dist/cli.js --no-chat-recording --yolo
[t=84]    3678       1      84 node   node …/dist/cli.js --no-chat-recording --yolo

它的存活时长(84 秒)与它的同胞进程(76 秒)正好对应该文件在同一次运行中的两个测试耗时(84.5 秒和 77.8 秒),所以这些正是 context-compress-interactive.test.ts 启动却从不关闭的会话。孤儿进程出现在整个 run 的最后一次采样 —— 也正是 worker 拆除的那一刻,此时 EPIPE 是致命的,而不是无害的。

#10971 的 witness 为什么没抓到:它的替身是 setInterval(() => {}, 1000),没有 SIGHUP handler,因此会以默认动作立刻死掉;而且它用 expect.poll(…, { timeout: 10_000 }) 断言 —— 这个 poll 容忍子进程在 cleanup() 之后继续存活最多十秒。真正要紧的性质"到 cleanup() 返回时已经死掉"从未被固定下来。

改动内容

cleanup() 现在会等待它发过信号的每个会话真正退出,上界设在高于 CLI 自身 5 秒 drain 天花板的位置,因此病态子进程不会把 teardown 挂死。等待以 node-pty 的 exit 事件为解除条件;上界用的 timer 会被 clear 且 unref,所以竞争获胜后不会留下任何把 worker 事件循环撑住的 handle —— 这里若残留一个 timer,就会重新造出我们正在修的那个"worker 无法退出"状态。

witness 是就地加强的,而不是另加一个,因此它对这道守卫的两半都会失败:替身现在会像真实 CLI 一样捕获 SIGHUP 并在 750 毫秒后退出,并且先报告自己已启动。第二点不是装饰而是必需 —— 在 node 装好 handler 之前发信号,会在约 3 毫秒内以默认动作结束子进程,什么都测不到,也正是这一点让本测试的第一个版本在一个正确的修复面前失败了。

验证

下面每条命令都在本 checkout(161c784514)中真实执行过,环境为 Qwen sandbox 容器内的 Linux / Node 22.23.2。

必需检查:

  • npm run build —— 通过(BUILD_EXIT=0
  • npm run typecheck —— 通过(exit 0,含 typecheck:integration
  • npm run lint —— 通过(exit 0;eslint . --ext .ts,.tsx && eslint integration-tests
  • npx prettier --check integration-tests/test-helper.ts integration-tests/test-helper.test.ts —— 通过("All matched files use Prettier code style!")
  • 定向 vitest,vitest run --root ./integration-tests test-helper.test --retry=0 —— 7 passed (7),exit 0
  • 以 CI 命令形态跑完整 interactive leg(QWEN_E2E_RENDERER=ink QWEN_SANDBOX=false KEEP_OUTPUT=true VERBOSE=true vitest run --root ./integration-tests interactive --exclude '**/interactive/cron-interactive.test.ts' --exclude '**/channel-plugin.test.ts',并 unset RUNNER_ENVIRONMENT,使 dangerouslyIgnoreUnhandledErrors 为 false,与 OpenTUI leg 完全一致)—— 共跑三次,全部 exit 0 且结果集完全相同:9 passed | 1 skipped (10) 个文件、18 passed | 2 skipped (20) 个测试;分别在父提交(基线)、修复后、以及修复后针对重新构建的 bundle 各跑一次
  • vitest run --root ./integration-tests context-compress-interactive --retry=0 —— 2 passed | 1 skipped (3),exit 0

变异探针(每道守卫都有自己的 witness;每次之后都用字节副本还原文件,并重跑到绿色):

  • cleanup() 中移除 await settleWithin(exited, INTERACTIVE_EXIT_GRACE_MS) → witness 失败cleanup() returned before the interactive CLI child exited: expected 0 to be greater than or equal to 750。还原后 → 7 passed。
  • cleanup() 中移除 ptyProcess.kill() 代码块 → witness 失败:20211 毫秒后 Matcher did not succeed in time(10 秒 grace 加 10 秒存活 poll)。还原后 → 7 passed。

孤儿与残留进程测量,整 leg 运行并以 2 秒间隔采样进程表:

  • 父提交:1 个 CLI 子进程在 worker 拆除时被 reparent 给 init(pid 3678,ppid 2130 → 1)。
  • 修复后:两次独立的整 leg 运行中均为 0 个孤儿、run 结束 15 秒后 0 个残留进程。

等待的代价,通过给 cleanup() 加插桩测得(插桩随后已移除,未提交):

PROBE cleanup waited 37ms for pid 7596
PROBE cleanup waited 42ms for pid 7598
PROBE cleanup waited 35ms for pid 7735

并由各文件耗时独立佐证:hooks-command.test.ts 从 2509ms 变为 2547ms(+38ms,与实测等待一致)。整 leg 时长在三次运行中为 166s → 190s → 308s,而这个摆动完全来自 context-compress-interactive.test.ts(162.3s → 187.1s → 303.9s),其余每个文件都在 ±250ms 之内。该文件驱动真实的 /compress 模型调用;其单个测试在各次运行中测得 71s、90s、107s 和 106s,所以这个方差是模型延迟,不是 teardown。

未运行,及原因:

  • OpenTUI leg 本身。 本环境没有安装 bun,而 resolveE2eCliCommand('opentui') 在缺少它时会抛错。以上全部在 QWEN_E2E_RENDERER=ink 下运行。缺陷与修复都与渲染器无关 —— SIGHUP handler 对任何交互式会话都会安装,只以 config.isInteractive() 为条件 —— 但变红的恰恰是这里无法执行的那个 leg。
  • E2E Test (Linux) - sandbox:none 各 shard。 它们需要 self-hosted ECS 池;见上文 Main CI failed: E2E Tests on d4e3e4fc8747 #10994 的说明。

对本次修复的诚实边界

这移除了该 leg 上一个被端到端实测证明的 unhandled error 来源。它并不保证该 leg 不再变红,有三点需要明白写出:

  1. 该 leg 在 d4e3e4fc87 上、在本改动之前就通过了,所以之后一次绿色的 OpenTUI run 并不能作为本修复的证据。有意义的信号是:"exit code 1、无失败测试"这个形态是否在若干次合并之后不再复现。
  2. 该 leg 是唯一关闭 dangerouslyIgnoreUnhandledErrors 的 Linux 通道(各 shard 在 ci: run the Linux E2E shards on the persistent pool #10085 中迁到了 self-hosted 池,macOS 按平台豁免),因此任何其他 unhandled error 都只在它这里是致命的 —— 包括 integration vitest 配置里已经记录在案的 onTaskUpdate RPC 60 秒停滞类别。托管 Linux 是否应继续把 unhandled error 判为致命,是维护者关于信号取舍的决定,fix(test): end interactive PTY sessions a test never closed #10971 刻意没有碰它;本 PR 同样没有碰。
  3. 在一次带插桩的运行中观察到 context-compress-interactive.test.ts 出现过一次间歇性断言失败,单独运行该文件时未复现(2 passed | 1 skipped)。它是一个真实模型测试,单个测试耗时会有数十秒的摆动,而本改动只影响 teardown 时序(实测 +38ms),因此从它到测试中途断言之间不存在因果路径。此处如实记录,而不是悄悄略过。

一条观察,刻意未实现

external-context-mem0-write.test.ts 自带一份交互式启动器的副本,它用 env: process.env 启动 process.execPath,并且从不套用渲染器 overlay。因此在 OpenTUI leg 上,该文件实际是用 node 加 ink 默认渲染器驱动 CLI,而不是 bun 加 QWEN_TUI_RENDERER=opentuiQWEN_TUI_RENDERER_STRICT —— 也就是说它落在渲染器矩阵本要保证的范围之外。它与本次失败无关(它在 finally 里 kill 自己的会话并等待退出,因此从未泄漏),而把它搬到 bun 上会改变该文件实际验证的内容,并可能引入与本 issue 无关的失败。因此提请维护者注意,而不是塞进一个 CI 修复的 diff 里。


🧠 Handled by Qwen Code · model/模型 qwen3.8-max-2026-09-02

@github-actions github-actions Bot added the review/self-reported The linked issue was opened by the PR author (self-reported) label Sep 4, 2026
@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

⚠️ Deferred approval withheld — 1 PR CI workflow run(s) on 161c784 did not finish green; see the updated table in the Stage 2 comment. Re-run @qwen-code /triage after fixes. finalize run

⚠️ 延迟审批已搁置 —— 161c784 有 1 个 PR CI workflow 未以绿色完成,详见 Stage 2 评论中已更新的表格。修复后可重新运行 @qwen-code /triage查看 finalize 运行

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓

Problem: observed, not theoretical. #10990 is a machine-filed main-CI failure — E2E Interactive - OpenTUI renderer (bun) exits non-zero without naming a single failing test — and the description carries real measurements behind the diagnosis rather than a plausible story. Two of them matter: the child is still alive the instant kill() returns and exits 83ms later with code 129 (the CLI's own handled-hangup code), and a whole-leg run on the parent commit caught a CLI child being reparented to init (ppid 2130 → 1) at worker teardown. That second observation is the load-bearing one, because it pins the exact instant an EPIPE stops being harmless. I confirmed the mechanism in the harness itself — runInteractive forwards every PTY byte to process.stdout whenever KEEP_OUTPUT or VERBOSE is set, which is what CI runs with, so a child that outlives its worker really is writing into a pipe with no reader.

Direction: aligned. This is test-infrastructure health, not a product surface — no production behaviour and no public contract change. The CHANGELOG signal doesn't apply to an internal harness fix. I want to call out the honesty in the scope statement: it claims to remove one proven source of the red leg and explicitly declines to claim the leg stops reddening, and it leaves "should github-hosted Linux keep treating unhandled errors as fatal" to a maintainer instead of quietly flipping that policy while it was in the neighbourhood.

Size: not applicable — no core paths are touched. Two files under integration-tests/, 58 additions / 8 deletions, and 29 of those added lines are in the test file itself.

Approach: the scope feels right, and strengthening the existing regression test in place rather than adding a parallel one is the correct call. The description also explains why the earlier witness missed this — the stand-in had no signal handler so it died on the default action, and the assertion polled with a ten-second timeout that was perfectly happy to let the child outlive teardown by up to ten seconds. Recording that is what stops the same fix shipping green a second time, and it's the part of this PR I'd most want future contributors to read.

Risk: no elevated risk signals — neither file matches the revert-correlated path list.

One thing I'm carrying into code review: the grace bound is 10s and cleanup() is called from afterEach across the interactive suite, so I want to check which budget actually governs that hook and whether the bound fits inside it.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓

问题: 是已观测到的,不是理论性的。#10990 是自动创建的 main CI 失败 issue —— E2E Interactive - OpenTUI renderer (bun) 以非零码退出,却没有指出任何一个失败的测试 —— 而 PR 描述给出的是实测数据,而不是一个"听起来合理"的故事。其中两点是关键:子进程在 kill() 返回的那一刻仍然活着,并在 83 毫秒后以退出码 129 结束(这是 CLI 自己处理 hangup 时用的码);在父提交上的一次整 leg 运行中,抓到一个 CLI 子进程在 worker 拆除时被 reparent 给 init(ppid 2130 → 1)。第二个观测是承重的,因为它精确定位了 EPIPE 从"无害"变成"致命"的那一刻。我在 harness 代码里确认了这个机制 —— runInteractive 在设置了 KEEP_OUTPUTVERBOSE 时会把每一个 PTY 字节转发到 process.stdout,而 CI 正是这样跑的,所以一个比 worker 活得更久的子进程,确实是在往一个没有读取端的管道里写。

方向: 对齐。这是测试基础设施的健康度,不是产品面 —— 没有生产行为改动,也没有公共契约变化。CHANGELOG 信号对内部 harness 修复不适用。这里要特别指出范围陈述的诚实:它只声称移除了一个被证明的红 leg 来源,明确不声称该 leg 从此不再变红;并且把"github-hosted Linux 是否应继续把 unhandled error 判为致命"留给维护者决定,而没有顺手在自己路过时改掉这个策略。

规模: 不适用 —— 没有触及核心路径。integration-tests/ 下两个文件,58 增 8 删,其中 29 行新增在测试文件里。

方案: 范围合理,而且就地加强已有回归测试、而不是另写一个平行测试,是正确的选择。描述还解释了为什么早先那个 witness 没抓到 —— 替身没有信号 handler,所以以默认动作立刻死掉;而断言用的是一个十秒超时的 poll,这个 poll 完全乐意接受子进程比 teardown 多活最多十秒。把这一点记录下来,正是防止同一个修复第二次"绿着"上线的关键,也是这个 PR 里我最希望后来的贡献者去读的部分。

风险: 无升级风险信号 —— 两个文件都不匹配与 revert 相关的路径列表。

有一点我带进代码审查:grace 上界是 10 秒,而 cleanup() 在整个 interactive 套件里都是从 afterEach 调用的,所以我要确认到底是哪个预算在管这个 hook,以及这个上界是否装得进去。

进入代码审查 🔍

Qwen Code · qwen3.8-max-2026-09-02

Reviewed at 161c784514d843a9060895da2e25693807f7d7f6 · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Code review

I wrote down what I'd do before opening the diff: track an exit promise per PTY session captured at spawn time, await it in cleanup() after signalling, bound the wait above the CLI's own 5s shutdown ceiling, make sure the bound's timer can't hold the worker's event loop open, and memoise the promise so a session a test already closed resolves immediately. The PR does all five. It also independently landed the subtlety I'd have had to be talked into — the stand-in in the test now announces STAND_IN_READY before the harness is allowed to signal it, because a signal delivered before the handler exists kills the child on the default action and measures nothing. That's precisely the hole that let #10971 ship green, and closing it in the witness is what makes this fix hard to silently undo.

So: no correctness blockers, no security concerns, no regressions I can find. The mechanism is sound and I checked the parts that could quietly not work.

What I verified

  • Two onExit listeners on one pty. The diff registers a second onExit alongside the existing one that resolves promise, so if node-pty only honoured one, either the new wait or the existing exit-code reporting would silently break. It's fine — packages/core/src/services/shellExecutionService.ts registers two concurrently (lines 1860 and 2150), and the disposable-per-registration signature is the array-listener pattern. CI corroborates it empirically, which is better than my reading: had the second registration clobbered the first, exited would never resolve, settleWithin would burn the full 10s grace, and test-helper.test.ts could not have finished in 965ms. It did, while asserting cleanupTookMs >= 750. The wait is resolving on the real exit event, not on the timeout.
  • No missed-exit race. onExit is registered synchronously in the same tick as pty.spawn, so a child cannot exit-and-be-reaped before the listener exists.
  • Already-closed sessions cost nothing. The promise is captured at spawn and stored in the tracked entry, so a session a test closed itself is already resolved by the time cleanup() splices the list. This is what makes the PR's claim about the Ctrl+C and mid-turn quit cases hold, and it's the reason the change can't slow down the tests that were already well-behaved.
  • settleWithin cannot newly fail a teardown. Both race outcomes resolve rather than reject, and clearTimeout runs on either path. Correct choice for a cleanup path — a bound that threw would turn a leaked child into a failed test.
  • Nothing left holding the loop. timer.unref() plus clearTimeout on both outcomes. The comment slightly oversells why unref is needed (node-pty's own handles keep the loop alive while the child lives, so the timer fires regardless), but it's harmless and the right instinct for code that runs during worker teardown.

One suggestion — the grace bound collides exactly with the hook budget

INTERACTIVE_EXIT_GRACE_MS is 10_000. cleanup() is called from afterEach across the interactive suite (hooks-command, context-compress-interactive, mid-turn-submit-interactive, external-context-*, …), and integration-tests/vitest.config.ts sets testTimeout to 5 minutes but never sets hookTimeout — it's the one vitest config in the repo that doesn't. Every other package pins it deliberately (packages/cli, packages/core, packages/web-shell, packages/acp-bridge, packages/node-repl, packages/qwen-live, packages/sdk-typescript, which sets it to 10000). So the hook that governs afterEach runs on Vitest's documented 10s default — the same number as the grace bound, with zero headroom.

Two consequences worth a look. A single child that ignores SIGHUP consumes the entire hook budget, so the afterEach times out at the same instant the grace expires and the rest of cleanup() — test-dir removal and the telemetry wait — never runs. And the wait loop is sequential, so the bound is per-session while the hook budget is per-hook: leaked sessions accumulate against one 10s ceiling rather than each getting its own.

The description's reasoning here addresses the CLI's ceiling ("deliberately above the CLI's own five-second shutdown ceiling") and says "hook time counts against the test timeout" — but in Vitest hooks are governed by hookTimeout, not testTimeout, and this config leaves it at the default. None of this is blocking: the measured reality is 35–42ms per session, the pathological case isn't one this suite produces, and a named hook timeout is a strictly better failure than today's silent whole-run EPIPE. But the exact collision looks unintentional, and any of these would close it — set hookTimeout in integration-tests/vitest.config.ts above the grace bound, drop the bound below the hook budget, or bound the aggregate wait instead of each session.

What I could not check

Whether node-pty can still deliver already-buffered onData after onExit fires. If it could, a single trailing process.stdout.write would remain theoretically possible after the wait resolves. node_modules is not installed in this review checkout so I could not read the package source, and I'm not going to guess at its read-loop ordering. Flagging it only to bound the claim, not as a defect — the sustained-writer case that was actually measured is closed either way.

Test evidence — this PR's own CI

I did not build or run any PR code; per the gate rules the review is static and the evidence below is this PR's own CI, read through the API at the reviewed commit.

The useful signal is that the strengthened regression test really ran and really passed: the Integration Tests (no-AK, No Sandbox) job invokes ./test-helper.test.ts explicitly and reports ✓ test-helper.test.ts (7 tests) 965ms, inside a job that finished Test Files 21 passed (21) / Tests 174 passed (174). That 965ms is doing real work as evidence — see the multi-listener point above.

The one red check is not this PR's. Dependency CVE audit failed on npm warn audit 503 Service Unavailable - POST https://registry.npmjs.org/-/npm/v1/security/audits/quicknpm error audit endpoint returned an error → exit 1. The npm registry's audit endpoint was down; this PR changes no dependency, no lockfile, and no manifest. I'm classifying that from the check's identity and the transport error in its own log, not from any claim in the PR. It does mean the Security Checks workflow is red on this commit, which matters for the deferred approval below.

The gap that CI cannot close: the job list on this commit contains no E2E Interactive - OpenTUI renderer (bun) leg, and tmux-testing and verify are both skipped. The file list that did run excludes interactive/** entirely. So the harness change is exercised in PR CI only by its own witness test — the suite that actually spawns and leaks PTY sessions, and the leg that actually reddens, are not run on pull requests at all. That is consistent with #10990 being filed per-commit against main, and the description is upfront that the OpenTUI leg "could not be run where this change was prepared" because bun was unavailable. Not verified: the OpenTUI/bun leg, and the sandbox:none shards — both absent from this commit's checks, the former also absent from the author's environment.

Final CI results for 161c784 (auto-updated by the triage finalize job after CI completed):

Check Conclusion
Dependency CVE audit ❌ failure
Classify PR ✅ success
Desktop Shell (ubuntu-22.04) ✅ success
Desktop Shell (windows-2022) ✅ success
Integration Tests (no-AK, No Sandbox) ✅ success
Lint & Static (ubuntu-latest, Node 22.x) ✅ success
Secret scan (TruffleHog) ✅ success
Test (ubuntu-latest, Node 22.x) ✅ success
web-shell E2E Smoke (ubuntu-latest, Node 22.x) ✅ success

One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。

Sandboxed verification would settle this: @qwen-code /verify — the load-bearing claim is that no interactive PTY child outlives cleanup(), and this PR's CI substantiates it only through a stand-in script in the harness's own witness test. The real leg that reddens (E2E Interactive - OpenTUI renderer (bun)) does not run on pull requests, so nothing here observes the actual CLI child under bun/OpenTUI. An A/B against the base build is what would show the orphan that the description measured (ppid 2130 → 1) appearing on base and absent on this branch. The author has write access, so @qwen-code /tmux is also available if a real TUI session is wanted, though /verify is the better fit for a process-lifetime property.

中文说明

代码审查

在读 diff 之前我先写下了自己的方案:在 spawn 时为每个 PTY 会话捕获一个 exit promise,在 cleanup() 里发完信号后等它,把上界设在高于 CLI 自身 5 秒关闭天花板的位置,确保这个上界用的 timer 不会撑住 worker 的事件循环,并且把 promise 记下来,好让测试自己已经关闭的会话立刻解除等待。这个 PR 五点全部做到了。它还独立地处理了我本来需要被说服才会想到的那个细节 —— 测试里的替身现在会先宣告 STAND_IN_READY,然后框架才被允许向它发信号,因为在 handler 装上之前发信号,子进程会以默认动作立刻死掉,什么都测不到。那正是让 #10971 "绿着"上线的那个洞,而把它在 witness 里堵上,才是让这个修复难以被悄悄撤销的关键。

所以:没有正确性阻塞项,没有安全问题,我找不到回归。机制是可靠的,我把那些"可能悄悄不生效"的地方都查了。

已验证的部分

  • 同一个 pty 上两个 onExit 监听器。 diff 在已有的那个(用于 resolve promise)旁边又注册了一个 onExit,所以如果 node-pty 只认一个,那么要么新的等待、要么已有的退出码上报会静默失效。这里没问题 —— packages/core/src/services/shellExecutionService.ts 就并发注册了两个(1860 行和 2150 行),而且"每次注册返回一个 disposable"的签名正是数组式监听器的模式。CI 还给出了比我的阅读更好的经验证据:如果第二个注册把第一个覆盖掉了,exited 就永远不会 resolve,settleWithin 会烧掉完整的 10 秒 grace,test-helper.test.ts 不可能在 965ms 内跑完。而它跑完了,同时断言了 cleanupTookMs >= 750。这个等待是靠真实的 exit 事件解除的,不是靠超时。
  • 不存在错过 exit 的竞态。 onExitpty.spawn 在同一个同步 tick 内注册,所以子进程不可能在监听器装上之前就退出并被 reap。
  • 已关闭的会话不产生代价。 promise 在 spawn 时捕获并存进被跟踪的条目里,所以测试自己关闭的会话,在 cleanup() splice 列表时早已 resolve。这正是 PR 关于 Ctrl+C 与 mid-turn quit 用例的说法成立的原因,也是这个改动不会拖慢本来就行为良好的测试的原因。
  • settleWithin 不会让 teardown 新增失败。 竞态的两条路径都是 resolve 而不是 reject,且两条路径都会 clearTimeout。对清理路径来说这是正确的选择 —— 一个会抛错的上界会把"泄漏了一个子进程"变成"测试失败"。
  • 没有东西撑着事件循环。 timer.unref() 加上两条路径都执行的 clearTimeout。注释把 unref 的必要性说得略重了(子进程活着时 node-pty 自己的 handle 就在撑着循环,timer 无论如何都会触发),但它无害,而且对于运行在 worker 拆除期间的代码来说这个直觉是对的。

一条建议 —— grace 上界与 hook 预算精确相撞

INTERACTIVE_EXIT_GRACE_MS10_000cleanup() 在整个 interactive 套件里都是从 afterEach 调用的(hooks-commandcontext-compress-interactivemid-turn-submit-interactiveexternal-context-* 等),而 integration-tests/vitest.config.tstestTimeout 设成 5 分钟,却从未设置 hookTimeout —— 它是仓库里唯一一个不设置的 vitest 配置。其它每个 package 都是刻意钉住的(packages/clipackages/corepackages/web-shellpackages/acp-bridgepackages/node-replpackages/qwen-livepackages/sdk-typescript 设成 10000)。所以真正管着 afterEach 的那个 hook 跑在 Vitest 文档默认的 10 秒上 —— 与 grace 上界是同一个数字,一点余量都没有。

有两个后果值得看一眼。一个完全忽略 SIGHUP 的子进程会吃光整个 hook 预算,于是 afterEach 会在 grace 到期的同一刻超时,而 cleanup() 剩下的部分 —— 测试目录清理和 telemetry 等待 —— 就再也不会执行。而且等待循环是串行的,所以上界是"每会话"的,hook 预算却是"每 hook"的:泄漏的会话会累积去撞同一个 10 秒天花板,而不是各自拥有自己的上界。

描述里针对这一点的推理讲的是 CLI 的天花板("刻意设在高于 CLI 自身五秒关闭天花板的位置"),并说"hook 时间是计入测试超时的" —— 但在 Vitest 里 hook 由 hookTimeout 管,不是 testTimeout,而这个配置把它留在了默认值上。这些都不构成阻塞:实测是每会话 35–42 毫秒,那种病态情况不是本套件会产生的,而一个有名字的 hook 超时,严格好于今天这种静默的整 run EPIPE。但这个精确相撞看起来是无意的,以下任一做法都能解决 —— 在 integration-tests/vitest.config.ts 里把 hookTimeout 设到 grace 上界之上、把上界降到 hook 预算之下,或者对累计等待设上界而不是对每个会话。

我无法确认的部分

node-pty 是否可能在 onExit 触发之后,仍然投递已经缓冲的 onData。如果可能,那么在等待解除之后,理论上仍会剩下一次尾随的 process.stdout.write。这个审查 checkout 里没有安装 node_modules,所以我读不到该包的源码,我也不打算去猜它读取循环的顺序。写出来只是为了给结论划个边界,不是当作缺陷 —— 那个被实测到的"持续写入者"场景,无论哪种情况都已经被关掉了。

测试证据 —— 本 PR 自己的 CI

我没有构建或运行任何 PR 代码;按 gate 规则,审查是静态的,下面的证据是本 PR 自己的 CI,通过 API 在被审查的 commit 上读取的。

有用的信号是:加强后的回归测试确实跑了、也确实过了。Integration Tests (no-AK, No Sandbox) 这个 job 显式调用了 ./test-helper.test.ts,报告 ✓ test-helper.test.ts (7 tests) 965ms,而整个 job 收尾于 Test Files 21 passed (21) / Tests 174 passed (174)。那个 965ms 本身就是有分量的证据 —— 见上面多监听器那一点。

唯一变红的检查不是这个 PR 造成的。Dependency CVE audit 失败于 npm warn audit 503 Service Unavailable - POST https://registry.npmjs.org/-/npm/v1/security/audits/quicknpm error audit endpoint returned an error → exit 1。npm registry 的 audit 端点挂了;本 PR 没有改动任何依赖、lockfile 或 manifest。我是根据这个检查的身份和它自己日志里的传输层错误来归类的,不是根据 PR 里的任何说法。这确实意味着 Security Checks workflow 在本 commit 上是红的,而这对下面的延迟批准有影响。

CI 关不掉的那个缺口:本 commit 的检查列表里没有 E2E Interactive - OpenTUI renderer (bun) 这个 leg,而且 tmux-testingverify 都是 skipped。真正跑了的那个文件列表完全不含 interactive/**。所以 harness 改动在 PR CI 里只被它自己的 witness 测试覆盖 —— 真正 spawn 并泄漏 PTY 会话的那个套件、以及真正变红的那个 leg,在 pull request 上根本不跑。这与 #10990 是按 commit 针对 main 创建的相一致,描述也坦白说 OpenTUI leg "在准备这一改动的环境里无法运行",因为那里没有 bun。未验证:OpenTUI/bun leg,以及 sandbox:none 各 shard —— 两者都不在本 commit 的检查里,前者在作者的环境中同样缺失。

(CI 表格见上,未重复翻译。)

沙箱验证可以定这件事:@qwen-code /verify —— 承重的主张是"没有任何交互式 PTY 子进程比 cleanup() 活得更久",而本 PR 的 CI 只通过 harness 自己 witness 测试里的一个替身脚本来支撑它。真正变红的那个 leg(E2E Interactive - OpenTUI renderer (bun))在 pull request 上不跑,所以这里没有任何东西观察到 bun/OpenTUI 下真实的 CLI 子进程。与 base build 做 A/B 才能显示出描述里实测到的那个孤儿进程(ppid 2130 → 1)在 base 上出现、在本分支上消失。作者有 write 权限,所以如果想要一个真实的 TUI 会话,@qwen-code /tmux 也可用,不过对于"进程生命周期"这类性质,/verify 更合适。

Qwen Code · qwen3.8-max-2026-09-02

Reviewed at 161c784514d843a9060895da2e25693807f7d7f6 · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 4/5 — the fix is correct, cheap, and witnessed; the only things keeping it from 5 are a bound that collides exactly with the hook budget, and a leg-level claim no pull-request CI can substantiate.

Stepping back: my independent proposal and this PR landed in the same place on all five points I'd have insisted on, and it beat me on one — the readiness handshake in the stand-in. I'd have written the wait, bounded it, and unref'd the timer; I'm not sure I'd have noticed that a signal delivered before the handler exists measures nothing, which is the exact reason the previous fix shipped green. That detail is the difference between a regression test that passes and one that actually pins the property.

The strongest objection I can mount is that this PR does not prove the OpenTUI leg stops reddening — and the author says so first, in more places than I would have. But that's an argument about the cure, not the change. What the change does claim, I could verify: the harness really does forward every PTY byte to process.stdout under KEEP_OUTPUT, a signalled child really is still alive when kill() returns, and awaiting the exit event really does close that window. The cost is ~38ms on the one file that leaks, and the worst case moves from "silent whole-run failure naming no test" to "a named hook timeout". Turning down a bounded, correct fix because it isn't a complete cure would be the wrong call.

Six months from now I'd thank whoever wrote this. The comments carry the why that isn't recoverable from the code — the CLI's 5s shutdown ceiling as the reason for the bound's magnitude, and the reason the stand-in needs a signal handler at all. The test was strengthened in place instead of duplicated, and the description records why the earlier witness failed, which is the part that stops this regressing quietly.

Two things I'd want the author or a maintainer to weigh, neither blocking:

  • The 10s grace bound equals Vitest's default hookTimeout, and integration-tests/vitest.config.ts is the only vitest config in the repo that doesn't set it. Since the wait loop is sequential, the bound is per-session but the hook budget is per-hook. Detail and three possible fixes are in the Stage 2 comment.
  • This is an autofix against a maintainer-approved issue (autofix/approved), and the description is unusually thorough. I treated that as no evidence either way and checked the load-bearing claims against the harness source and this commit's CI instead — they held, including one I verified better than by reading: test-helper.test.ts finished in 965ms while asserting a ≥750ms wait, which is only possible if the new exit listener actually fires.

On the approval: CI is still running (Qwen Code CI in progress — Lint & Static and Test (ubuntu-latest)), so I'm not approving in this run; approval is deferred until CI lands green on the commit below.

⚠️ Maintainers, this deferral probably will not resolve itself. Dependency CVE audit is already red on this commit because the npm registry's audit endpoint returned 503 — pre-existing infra noise, unrelated to a PR that touches no dependency. The finalize step only approves once every check on the commit is green, so that red check will most likely withhold the deferred approval even after the two running jobs pass. Remedy is a human one: re-run the CVE audit once the registry recovers, or approve directly. Flagging it so a green suite doesn't sit here waiting on a check that will never turn green on its own.

中文说明

Confidence: 4/5 —— 修复是正确、廉价且有 witness 的;让它到不了 5 分的只有两件事:一个与 hook 预算精确相撞的上界,以及一个任何 pull-request CI 都无法支撑的 leg 级主张。

退一步看:我自己独立想到的方案与这个 PR 在我会坚持的全部五点上都落在同一处,而它在一点上胜过我 —— 替身的就绪握手。我会写等待、给它设上界、把 timer unref 掉;但我不确定我会注意到"在 handler 装上之前发信号,什么都测不到",而那恰恰是上一次修复"绿着"上线的原因。这个细节,正是"一个能通过的回归测试"与"一个真正钉住性质的回归测试"之间的区别。

我能提出的最有力反对是:这个 PR 并没有证明 OpenTUI leg 不再变红 —— 而作者比我更主动地、在更多地方先说了这一点。但那是关于"疗效"的论证,不是关于"改动"的。改动所声称的部分,我都能验证:harness 确实在 KEEP_OUTPUT 下把每一个 PTY 字节转发到 process.stdout;被发过信号的子进程在 kill() 返回时确实仍然活着;而等待 exit 事件确实关掉了那个窗口。代价是那个唯一泄漏会话的文件增加约 38 毫秒,最坏情况从"静默的整 run 失败、不指出任何测试"变成"一个有名字的 hook 超时"。因为一个有界的、正确的修复不是彻底的疗效就把它拒掉,是错误的判断。

六个月后我会感谢写这段代码的人。注释承载了那些无法从代码里恢复的为什么 —— CLI 的 5 秒关闭天花板是上界取值的理由,以及替身为什么必须有一个信号 handler。测试是就地加强的而不是复制一份,而描述记录了早先那个 witness 为什么失效,那正是防止这件事悄悄退化的部分。

有两点我希望作者或维护者权衡,都不构成阻塞:

  • 10 秒的 grace 上界等于 Vitest 的默认 hookTimeout,而 integration-tests/vitest.config.ts 是仓库里唯一不设置它的 vitest 配置。由于等待循环是串行的,上界是"每会话"的,而 hook 预算是"每 hook"的。细节与三种可能的修法在 Stage 2 评论里。
  • 这是针对一个维护者已批准的 issue(autofix/approved)的 autofix,而且描述异常详尽。我对此不取任何立场,而是把承重的主张拿去对照 harness 源码和本 commit 的 CI —— 它们都成立,其中一条我验证得比阅读更好:test-helper.test.ts 在 965ms 内跑完,同时断言了一次 ≥750ms 的等待,而这只在新的 exit 监听器确实触发时才可能。

关于批准: CI 仍在运行(Qwen Code CI 进行中 —— Lint & StaticTest (ubuntu-latest)),所以本次运行我不批准;批准被延迟到下面这个 commit 的 CI 变绿之后。

⚠️ 维护者请注意,这个延迟很可能不会自行解除。 Dependency CVE audit 在本 commit 上已经是红的,原因是 npm registry 的 audit 端点返回了 503 —— 属于既有的基础设施噪声,与一个不碰任何依赖的 PR 无关。finalize 步骤只会在该 commit 上每一个检查都变绿之后才批准,所以即使那两个正在运行的 job 通过了,那个红色检查也很可能会让延迟批准被扣住。补救办法是人工的:等 registry 恢复后重跑 CVE audit,或者直接批准。把它标出来,是为了不让一套绿色的检查在这里干等一个永远不会自己变绿的检查。

Qwen Code · qwen3.8-max-2026-09-02

Reviewed at 161c784514d843a9060895da2e25693807f7d7f6 · re-run with @qwen-code /triage

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review/self-reported The linked issue was opened by the PR author (self-reported)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Main CI failed: E2E Tests on b7815a7e1a82

2 participants