fix(release): stop one flaky test from failing a stable release - #10842
Conversation
Release run 33633468180 failed with all three workspace test shards red, each for an unrelated reason, and all three are timing assumptions that only hold on an idle machine. The message list pagination test polled a fixed budget of 32 animation frames before asserting a load had fired. A frame count does not stretch when the host is loaded and the queued effect lands late, so poll against a wall-clock deadline instead. The scroll-restore test in the same file asserted the restored offset exactly one frame after the load resolved, and now waits for the restore to land. The hook process test read the descendant's pid file the moment the hook returned, but the hook is killed by its own 300ms timeout, which can fire before the descendant has started far enough to write that file. Poll for it the way every other pid read in that file already does. The session PR refresh teardown removed its temp tree with a plain recursive delete. dispose() stops the next tick but does not await the sweep already in flight, so a sweep can still be writing under the tree while teardown walks it, and the delete fails with ENOTEMPTY when a file lands between the readdir and the rmdir. Retry so cleanup waits the writer out. Claude-Session: https://claude.ai/code/session_01AWWgJEqafyAT1Mc75T8N7h
|
Re-run on the new head — Template looks good ✓ Problem: observed, and corroborated again from the Actions history rather than taken on faith — the last successful release run is 2026-08-31 14:17 UTC, and every one of the nine release runs since (scheduled and manual, seven of them today) has failed. The shape the PR describes — each failing shard dying on exactly one test, a different one each time — is what the run history shows. Direction: aligned — unblocking stable releases is release-infrastructure work the project needs right now, and the CHANGELOG/CI history confirms releases have been red for two days. One note carried into Stage 3: this PR sets the stable-release retry policy, which is a maintainer's call, so this run escalates instead of approving. Size: 35 production lines ( Approach: the scope is right. Retry parity with nightly/preview, plus hardening the specific tests that have actually been blocking runs, is exactly the two moves this situation asks for — and the release-workflow contract test pins the new default so a silent drop back to a no-retry stable lane fails CI. One observation, not a blocker: the vulnerable-deps bump ( Risk: Moving on to code review. 🔍 中文说明基于新 head 的重跑——上次审查之后落了 模板完整 ✓ 问题: 已观测到,并且再次用 Actions 历史独立佐证——最后一次成功的 release run 是 2026-08-31 14:17 UTC,此后九次 release(定时 + 手动,其中七次在今天)全部失败。PR 描述的形态——每个失败分片恰好挂一个测试、且每次各不相同——与 run 历史一致。 方向: 对齐——解锁稳定版发布是当前急需的 release 基础设施工作。注意:本 PR 设定 稳定版的重试策略,属于维护者决策范畴,因此本次运行升级给维护者而不是直接批准。 规模: 35 行生产代码( 方案: 范围合理。与 nightly/preview 对齐的重试 + 加固真正拦路过 release 的测试,正是这个局面需要的两步;且 release-workflow 契约测试 pin 住了新默认值,稳定版若被悄悄改回不重试会在 CI 失败。一处观察(非阻断):依赖安全升级( 风险: 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewMy independent baseline for "one flaky test out of ~30k keeps reddening stable releases" was three moves: give the stable lane the retry nightly/preview already have, convert the known fixed-timing waits into bounded state polls, and chase the shard that went red with zero failed tests — that shape is an unhandled rejection. The PR matches it move for move. What I verified on this head, past the two new commits:
Status of the seven /review threads — re-checked against this head, not taken from the replies: R1-9 (both locations, the two Criticals), R1-3, R1-5, R1-7, R1-11 are fixed and I confirmed each fix in the diff. R1-2 is deliberately left open: the No new findings on the two new commits — they are the fixes described above plus two comment corrections, and the comments now match the code. Test evidence — this PR's own CI at
|
| Check | Conclusion |
|---|---|
Post Coverage Comment |
🚫 cancelled |
Test (ubuntu-latest, Node 22.x) |
🚫 cancelled |
web-shell E2E Smoke (ubuntu-latest, Node 22.x) |
🚫 cancelled |
Capture web-shell visuals (ubuntu-latest, Node 22.x) |
✅ success |
Classify PR |
✅ success |
Dependency CVE audit |
✅ success |
Desktop Shell (ubuntu-22.04) |
✅ success |
Desktop Shell (windows-2022) |
✅ success |
Integration Tests (no-AK, No Sandbox) |
✅ success |
macos-latest / Java 21 |
✅ success |
OpenTUI no-flicker gate |
✅ success |
Real daemon E2E / Java 11 |
✅ success |
Secret scan (TruffleHog) |
✅ success |
Serve A/B (ubuntu-latest, Node 22.x) |
✅ success |
TUI parity snapshots (ink vs opentui) |
✅ success |
ubuntu-latest / Java 11 |
✅ success |
ubuntu-latest / Java 17 |
✅ success |
ubuntu-latest / Java 21 |
✅ success |
windows-latest / Java 21 |
✅ success |
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。
Reading the skips and cancellations: the macOS/Windows unit legs and Integration Tests (CLI, No Sandbox) are gated to non-PR events in ci.yml itself — skipped on every PR by design, not a gap this PR caused; the PR's equivalent integration lane (no-AK) ran and passed. The two cancelled route checks are superseded command-routing jobs from the re-triggered bot workflows, not PR CI. The unit suite and Serve A/B are still in flight as of this writing — not verified yet: their results; this section states what is there now.
Sandboxed verification would settle what a green suite cannot: @qwen-code /verify — the unhandled-rejection fix in Session.ts is the one behavioural claim here that no test in this diff pins (R1-2 is exactly that gap), and this PR's suite passes with the guard removed. A verify run is already in flight on this head; the two prior runs on earlier heads did not pass (one fell on the flakiness gate itself, one reported findings), so the verdict on this head is the one to read.
Real-scenario testing: N/A — no user-visible behavior changes.
中文说明
代码审查: 我独立的解题思路是三步——让稳定版获得 nightly/preview 已有的重试、把已知固定时序等待改成有界状态轮询、追查"红了却没有失败测试"的分片(那形态就是 unhandled rejection)。PR 与之一一对应。本 head 上逐项核实:
release.yml:vars.QWEN_RELEASE_VITEST_RETRY || '2'让所有发布计划默认重试 2 次;'off'哨兵值是"省略参数"而非传--retry=0(后者会压过 sdk-typescript 自带的配置级重试,做法正确)。契约测试同时 pin 住新表达式与'off'分支。Session.ts:读源码确认了机制——#recoverLateDrain只吞掉 drain promise 自身的 rejection,race 之后的 debugLogger/parse/push 仍可能抛出;裸void下会逃逸为 unhandled rejection,Node 进程直接终止,Linux 上的 vitest run 也会因此变红且报不出失败测试。新增的.catch(() => {})与同方法内既有写法一致;选择静默吞掉而不是打日志也是对的——日志器本身就是可能的抛出源。- 测试加固:所有放宽的等待仍然有界,且每个分组 90s 的 per-test 预算已包住 30s/15s 的内部等待(上次审查的两个 Critical 预算缺口已修复)。存活钩子组的超时从 300ms 放宽到 5s,保证 descendant 有机会写出 pid 再被杀;被测机制(监督者计时并杀组)未变。延迟测试在共享 ECS 上断言最快样本 < 10 倍预算,在其他环境保留原有的中位数 < 50ms 严格度。
七个 /review 线程: 逐一对照本 head 复核——R1-9(两处 Critical)、R1-3、R1-5、R1-7、R1-11 均已修复并在 diff 中确认;R1-2 刻意保留开放(Session.ts 的守卫仍无回归测试,套件在缺陷存在时照样通过)——属 Suggestion 非阻断,但建议按承诺补一个后续 PR。
测试证据:来自 PR 自身 CI(API 拉取),表格区域会在 CI 落定后原位更新。macOS/Windows 单测与 Integration Tests (CLI, No Sandbox) 在 ci.yml 中本就只对非 PR 事件运行,每次 PR 均跳过,非本 PR 造成;对应的 no-AK 集成通道已运行并通过。两个被取消的 route 检查是机器人重复触发后被取代的调度任务,与 PR CI 无关。撰写时主单测与 Serve A/B 仍在运行——其结果尚未验证。
沙箱验证:@qwen-code /verify 能补上绿色套件补不了的一环——Session.ts 的 unhandled rejection 修复是本 PR 唯一没有测试 pin 住的行为性论断。当前已有针对该 head 的 verify 在跑;此前两次针对旧 head 的 verify 未通过,应以本次结果为准。
真实场景测试:N/A——无用户可见行为变化。
— Qwen Code · qwen3.8-max
Reviewed at d6ebe036725182f6fe9f57eeb6cc81c2cc4d4c13 · re-run with @qwen-code /triage
|
Confidence: 3/5 — clean review with no blockers on the new commits; the cap is policy and pending evidence, not doubt. This PR sets the stable-release retry policy and touches the high-risk Stepping back: this one holds together, and the two new commits are exactly the fixes they claim to be. I re-checked all seven /review threads against this head rather than trusting the replies — five are fixed in the diff, and the sixth (R1-2, no regression test pinning the The reservations I'm leaving on the record, none of them blockers:
Why this run defers instead of approving: a bot approval here would flip the effective review state on a release-policy change that hasn't had a human eye on this head, while the unit suite, Serve A/B, and a fresh ⏸️ Deferring to @wenshao — no review blockers remain on 中文说明置信度:3/5 —— 新提交上的审查干净、无阻断项;压在 3 分的是策略与未落定的证据,而不是疑虑。本 PR 设定稳定版重试策略、触及高风险的 回头看:这个 PR 是立得住的,两个新提交正是它们所声称的修复。七个 /review 线程我逐一对照本 head 复核而非只看回复——五个已在 diff 中确认修复;第六个(R1-2, 留在记录上的顾虑(均非阻断):重试是有意的取舍(可能把真实原因的间歇失败变成慢测试而非红盘,但旋钮可随时调整或关闭);R1-2 仍开放(守卫正确性只能靠阅读确认,回归时 CI 不会失败,建议按承诺补后续 PR);现有的 本次选择暂缓而非批准的原因:机器人若此刻批准,会在一个尚未有人类复核此 head 的发布策略变更上翻转有效评审状态,而该 head 上的单测、Serve A/B 与新一轮 ⏸️ 转交 @wenshao —— — Qwen Code · qwen3.8-max Reviewed at |
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / after✅ No screenshot changes against the PR base. Full-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
…ease A stable release ran its workspace tests with no retry at all, while nightly and preview retried twice. With ~30k tests behind that gate, any single flaky one reddened the release outright: six consecutive stable runs failed on one test each while build, typecheck, lint, scripts and both integration suites stayed green, and the same test passed on the next attempt. Retry every release schedule, behind a repository variable so the count can be retuned or switched off without a PR. A real break still fails all three attempts; what this stops is a contended runner deciding whether a release ships. Widen the timing bounds the process-tree hook tests impose on themselves. Every test in that file spawns real `node --import=tsx/esm` processes and then waits on a hard wall-clock deadline — 5s for startup, 3-4s for reaping. Process startup is not something a smarter wait can speed up, so on a shared runner those deadlines report host load rather than hook behaviour. Size them for the busiest host; a genuine hang still fails, only later, and the passing path is unchanged at 24s for the file. Assert the recall scan latency against the fastest of its samples rather than the median. Three release shards land on one machine, so a wall-clock median moves with the neighbours' load: it measured 131ms against a 50ms ceiling on a run whose every other gate passed. The fastest sample is the one least contaminated by contention and the closest available estimate of the intrinsic cost, and the shared lane gets a ceiling only a real regression can cross. The printed table now carries best, median and worst so the detail survives. Claude-Session: https://claude.ai/code/session_01AWWgJEqafyAT1Mc75T8N7h
The mid-turn drain recovery is fired and forgotten with a bare `void`. Its own comment says it "bounds the wait and swallows a late rejection", but it only swallows a rejection of the drain promise: anything that throws after that race escapes as an unhandled rejection, and Node ends the process on one. The debug logger is among the things that can throw there, so a logging failure could take a session down — from a path whose entire purpose is best-effort recovery. This is also what made a release shard red while reporting no failing test. The throw lands on a later tick, so on a loaded host it arrives after the test that installed it has finished: Vitest attributes it to no test, counts it as an error, and exits non-zero with every one of the 8147 tests passed. A test retry cannot help, because nothing failed. Swallow silently rather than log, since the logger is itself one of the things that can throw here. Claude-Session: https://claude.ai/code/session_01AWWgJEqafyAT1Mc75T8N7h
|
@qwen-code /triage Re-running on the new head: the earlier pass reviewed |
|
Sandboxed verification: ❌ not passed — non-deterministic tests (flakiness gate) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 363 passed · 0 failed · 363 total Flakiness gate: ❌ 1 of 5 changed test file(s) returned different results across identical re-runs (4 full round(s)) The deterministic flakiness gate re-ran the test files this PR changes and got different outcomes from identical runs (agent verdict: 中文 — 判定:❌ 不通过 · 测试结果不确定(抖动门)沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码(与 base 构建 A/B 对照、无 mock harness 断言、定向门禁)。仅作为评审证据,不构成评审、批准或 CI 检查。 脚本断言:363 通过 · 0 失败 · 363 总计 抖动门:❌ 1 of 5 changed test file(s) returned different results across identical re-runs (4 full round(s)) 确定性抖动门将本 PR 改动的测试文件原样重跑了多轮,得到了不一致的结果(agent 判定: Verification reportPR #10842 Deep Verification —
|
| cell | build | oracle | result |
|---|---|---|---|
| head, counted | 8f63b437 (.catch(() => {})) |
escaped unhandled rejections (in-test listener) | 0, fault fired, prompt #2 served, exit 0 — 01-ab-head-swallowed.png |
| base, counted | 4f212873 (bare void) |
escaped unhandled rejections (in-test listener) | 1 (simulated late-tick logger failure), fault fired, exit 0 — 02-ab-base-one-escape-counted.png |
| head, no listener | 8f63b437 |
run-level shape | clean run, exit 0 |
| base, no listener | 4f212873 |
run-level shape | Tests 1 passed + Errors 1 error + exit 1 — the release-shard symptom: red run, zero failed tests — 03-base-release-symptom-red-no-failed-test.png |
The base run's own summary — Test Files 1 passed (1) / Tests 1 passed (1) / Errors 1 error with a non-zero exit — is a byte-level reproduction of the shape the PR says blocked releases ("the shard that failed with nothing failing"), attributed to no test.
Mechanism scoping (honest bound of the claim). A bare-node diagnostic (archived late-drain-harness.ts) showed that in a production daemon turn #executePrompt wraps the turn in sessionIdContext.run(sessionId, …), so the fired recovery task resolves its debug-log session from the async context ({ getSessionId: () => sessionId } — a closure that cannot throw), bypassing any process-wide setDebugLogSession fault. The realistic post-race throw actors are therefore narrower than "the debug logger" in a daemon turn: a vitest-context logger mock/fault (exactly the shape that reddened the release shard, and what this A/B drives), or a future edit adding a throwing call after the race inside #recoverLateDrain. The fix makes the path fail-safe by construction against the whole class, which is what the A/B proves: base 1 escape vs head 0 escapes, identical scenario.
Secondary claim 1 — retry wiring
run:block extracted verbatim via YAML parser: byte-identical between base and head (delta is the env expression only).bash -nclean on both.- Replayed under GitHub's Linux default shell contract (
bash --noprofile --norc -eo pipefail) with a stubbednpmrecording argv (replay-harness.mjs, 9/9): empty/unset→ no--retryarg;'2'→--retry=2after--passWithNoTests;'5'→--retry=5;'0'→--retry=0.--retry <times>confirmed present in the repo's vitest 3.2.7;packages/sdk-typescript/vitest.config.tsdoes carryretry: 2. - Contract gate:
release-workflowsuites 52/52 at head, including the new pinexpect(testStep.env.VITEST_RETRY).toBe("${{ vars.QWEN_RELEASE_VITEST_RETRY || '2' }}")— a silent revert to the no-retry stable expression fails there. - Single
VITEST_RETRYoccurrence in the file (the sharedworkspace_testsjob), consistent with "every release schedule".
Secondary claim 2 — gates + vacuity probes
| gate (at head) | result |
|---|---|
packages/core hook-runner.process.test.ts |
20/20, 25.8 s of tests on this loaded shared runner (author claimed 24 s on idle) |
packages/core recall-scan-latency.test.ts |
1/1, under the stricter non-ECS ceiling (50 ms — RUNNER_NAME unset here) |
packages/cli session-pr-refresh.test.ts |
63/63 (author: "could not run locally, rely on CI" — ran fine here) |
packages/web-shell MessageList.dom.test.tsx |
163/163 (same) |
scripts/tests release-workflow contract tests |
52/52 |
| repo's own recovery test at head (mechanism sanity) | 1/1 |
Vacuity probes (mutations in a scratch copy, deleted after): waitForLoadCount made unreachable (>= count + 99) and waitForFrames(() => list.scrollTop === 600) → waitForFrames(() => false). Both tests fail loud and bounded (~5 s — on this non-ECS box the 5 s per-test timeout binds before the 10 s FLUSH_DEADLINE_MS; on the ECS lane's 60 s budget the deadline fires first and the following assertion goes red). Neither wait can pass vacuously — 05-vacuity-probes-fail-bounded.png. (A first combined run showed 6 additional failures at 2–7 ms; those were cascade pollution from the timed-out probe skipping its finally prototype-restores, confirmed by re-running each probe in isolation.)
Corrections
None.
Findings
S1 (Suggestion, non-blocking) — the QWEN_RELEASE_VITEST_RETRY knob cannot express "omit the flag"; '0' triggers the exact override the retained comment warns about
VITEST_RETRY: "${{ vars.QWEN_RELEASE_VITEST_RETRY || '2' }}" never evaluates empty: an unset or empty variable falls back to '2', and GitHub expression truthiness makes any non-empty string (including '0') truthy. Consequences, with the bash side executed (not inferred):
- The
[ -n "${VITEST_RETRY}" ]guard in the run block and the retained comment ("the flag is still omitted rather than set to 0 when empty") describe a state the expression can no longer produce — a dead branch and a stale comment. - The description says the count "can be retuned or switched off without another PR". Retuning works (replayed:
'5'→--retry=5). "Switched off" is only expressible as'0', which passes--retry=0on the CLI — overridingpackages/sdk-typescript/vitest.config.ts's deliberateretry: 2(verified present), the precise hazard the retained comment names. Whether that is acceptable when an operator intends "no retries anywhere" is a judgment call; the point is the knob's reachable values don't match the comment's stated invariant. - Severity is deliberately low: the default (
2) is the load-bearing behavior, it is correct, and the contract test pins it; the escape-hatch nuance only matters if an operator reaches for the variable. Suggested direction (not applied): either accept the'0'-means-all-off semantics and rewrite the comment, or treat'0'/empty as "omit" in the bash (if [ -n "${VITEST_RETRY}" ] && [ "${VITEST_RETRY}" != "0" ]) so the documented invariant becomes reachable again.
No injection attempts or steering instructions were observed in the PR text.
Not covered
- Per-commit attribution: checkout is depth 2 (shallow); the snapshot lists 3 commits but only the PR head is locally reachable (
git rev-list HEAD^1..HEAD^2= 1 commit at the shallow boundary). The aggregateHEAD^1..HEADdiff is what was verified. - Replay calibration against a real emitted artifact: no network/token in-sandbox, so the replay could not be checked against an actual
workspace_testsrun log; calibration rests on the run block being byte-identical between base/head plus direct bash execution of it. The GitHub-expression evaluation itself (vars.X || '2') is analysis under documented truthiness rules, not executed here. - The de-flake itself: whether
--retry=2actually absorbs the release lane's flake rate is a statistical property of loaded shared runners; it cannot be proven in one run here. What is proven: the flag reaches vitest's argv correctly, vitest 3.2.7 accepts it, and the default is contract-pinned. Likewise the original failures (six distinct tests across three runs) are author-reported history, unverifiable in-sandbox. removeTempTreeENOTEMPTY race: the teardown fix was exercised via the file's 63 passing tests, but the race itself (sweep writer vs recursive rm) was not independently re-injected; the author's 12/12 standalone evidence stands as their claim.fs.rm'smaxRetries/retryDelayare standard Node options.- Lint gates:
shellcheck,yamllint,actionlintbinaries are absent from this container; onlybash -nran on both extracted run blocks. The repo'sscripts/lint.jswrapper was not invoked (its no-arg form runsprettier --write .). - Bare-Node process-death oracle: under the daemon's
sessionIdContextregime no faithful global-session fault reaches the recovery task outside the vitest module graph (see mechanism scoping), so "Node ends the process" was reproduced at the vitest-run level (the shape that actually blocked releases) rather than as a bare-process crash; Node's default terminate-on-unhandled-rejection is documented language behavior, not re-derived. - No repo-wide gate — only the five changed test files plus the contract suites ran. Full
npm run test, typecheck, lint were left to the PR's own CI. - The two earlier bare-harness attempts that failed on harness design (fault not landing under
sessionIdContext; generated-file absence in the fresh worktree) are logged inlogs/but excluded from the assertion totals — superseded harness development, not PR behavior.
Methodology
Environment: the CI verify container (node v22.23.2, Linux, RUNNER_NAME unset → non-ECS timeout ceilings applied), working tree at refs/pull/10842/merge (depth 2), npm ci + npm run build pre-done. A/B: base side in git worktree add tmp/base-tree HEAD^1; the PR touches no production code outside packages/cli/src/acp-integration/session/Session.ts, so the base cells ran the base source of that file under tsx/vitest with everything else shared — control purity asserted by realpath (node_modules/@qwen-code/qwen-code-core → repo packages/core, whose production sources are byte-identical between arms; cli's vitest config aliases core to source, also identical between arms), and workspace dist outputs for untouched packages were symlinked from the head build into the base tree to satisfy the vitest global-setup guard. Two generated files (git-commit.ts, pure version constants) were stubbed in the base tree. Harnesses drove the code at the highest-fidelity level available: real Session + real drain/recovery code with only constructor-dep interfaces satisfied by fakes transcribed from the suite's own scaffold, and the suite's own createDebugLogger mock pattern for fault injection. Raw per-cell logs live in logs/, harness sources in this directory (pr10842-late-drain-verify.test.ts, late-drain-harness.ts, replay-harness.mjs); evidence images in evidence/. The base worktree was removed after the cells were captured.
Flakiness gate log
rounds=5 files=5 skipped=0
file packages/cli/src/serve/server/session-pr-refresh.test.ts: (cd packages/cli) npx --no-install vitest run ./src/serve/server/session-pr-refresh.test.ts
file packages/core/src/hooks/hook-runner.process.test.ts: (cd packages/core) npx --no-install vitest run ./src/hooks/hook-runner.process.test.ts
file packages/core/src/memory/recall-scan-latency.test.ts: (cd packages/core) npx --no-install vitest run ./src/memory/recall-scan-latency.test.ts
file packages/web-shell/client/components/MessageList.dom.test.tsx: (cd packages/web-shell) npx --no-install vitest run ./client/components/MessageList.dom.test.tsx
file scripts/tests/release-workflow.test.js: (cd .) npx --no-install vitest run --config ./scripts/tests/vitest.config.ts ./scripts/tests/release-workflow.test.js
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/cli/src/serve/server/session-pr-refresh.test.ts: PPPPP
packages/core/src/hooks/hook-runner.process.test.ts: PFFPP
packages/core/src/memory/recall-scan-latency.test.ts: PPPPP
packages/web-shell/client/components/MessageList.dom.test.tsx: PPPP
scripts/tests/release-workflow.test.js: PPPP
verdict: flaky
summary: 1 of 5 changed test file(s) returned different results across identical re-runs (4 full round(s))
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/cli/src/serve/server/session-pr-refresh.test.ts: P (exit 0)
round 1 · packages/core/src/hooks/hook-runner.process.test.ts: P (exit 0)
round 1 · packages/core/src/memory/recall-scan-latency.test.ts: P (exit 0)
round 1 · packages/web-shell/client/components/MessageList.dom.test.tsx: P (exit 0)
round 1 · scripts/tests/release-workflow.test.js: P (exit 0)
round 2 · packages/cli/src/serve/server/session-pr-refresh.test.ts: P (exit 0)
round 2 · packages/core/src/hooks/hook-runner.process.test.ts: F (exit 1)
--- output tail · round 2 · packages/core/src/hooks/hook-runner.process.test.ts ---
�[1m�[46m RUN �[49m�[22m �[36mv3.2.7 �[39m�[90m/__w/qwen-code/qwen-code/packages/core�[39m
�[2mCoverage enabled with �[22m�[33mv8�[39m
�[31m❯�[39m src/hooks/hook-runner.process.test.ts �[2m(�[22m�[2m20 tests�[22m�[2m | �[22m�[31m1 failed�[39m�[2m)�[22m�[33m 55977�[2mms�[22m�[39m
�[33m�[2m✓�[22m�[39m HookRunner process tree cancellation�[2m > �[22mreaps a descendant that ignores SIGTERM before returning �[33m 2411�[2mms�[22m�[39m
�[33m�[2m✓�[22m�[39m HookRunner process tree cancellation�[2m > �[22mreaps an active synchronous hook tree on parent process-exit �[33m 1400�[2mms�[22m�[39m
�[33m�[2m✓�[22m�[39m HookRunner process tree cancellation�[2m > �[22mreaps an active synchronous hook tree on parent signal-exit �[33m 640�[2mms�[22m�[39m
�[33m�[2m✓�[22m�[39m HookRunner process tree cancellation�[2m > �[22mreaps an active synchronous hook tree on parent handled-signal-exit �[33m 793�[2mms�[22m�[39m
�[33m�[2m✓�[22m�[39m HookRunner process tree cancellation�[2m > �[22mreaps an active async hook tree on parent process-exit �[33m 947�[2mms�[22m�[39m
�[33m�[2m✓�[22m�[39m HookRunner process tree cancellation�[2m > �[22mlets a MessageDisplay hook after explicit parent exit write output and finish �[33m 1081�[2mms�[22m�[39m
�[33m�[2m✓�[22m�[39m HookRunner process tree cancellation�[2m > �[22mlets a StopFailure hook after explicit parent exit write output and finish �[33m 1264�[2mms�[22m�[39m
�[33m�[2m✓�[22m�[39m HookRunner process tree cancellation�[2m > �[22mlets a SessionDelete hook after explicit parent exit write output and finish �[33m 1337�[2mms�[22m�[39m
�[33m�[2m✓�[22m�[39m HookRunner process tree cancellation�[2m > �[22mlets an async MessageDisplay hook after explicit parent exit write output and finish �[33m 710�[2mms�[22m�[39m
�[33m�[2m✓�[22m�[39m HookRunner process tree cancellation�[2m > �[22mlets a MessageDisplay hook after natural parent exit write output and finish �[33m 1084�[2mms�[22m�[39m
�[33m�[2m✓�[22m�[39m HookRunner process tree cancellation�[2m > �[22mlets a StopFailure hook after natural parent exit write output and finish �[33m 1082�[2mms�[22m�[39m
�[33m�[2m✓�[22m�[39m HookRunner process tree cancellation�[2m > �[22mlets a SessionDelete hook after natural parent exit write output and finish �[33m 1280�[2mms�[22m�[39m
�[33m�[2m✓�[22m�[39m HookRunner process tree cancellation�[2m > �[22menforces a surviving hook timeout after the parent exits �[33m 3426�[2mms�[22m�[39m
�[32m✓�[39m HookRunner process tree cancellation�[2m > �[22mpreserves a surviving hook exit code 124 before its deadline�[32m 146�[2mms�[22m�[39m
�[33m�[2m✓�[22m�[39m HookRunner process tree cancellation�[2m > �[22mpreserves a prompt exit 124 when the parent event loop is delayed past the deadline �[33m 1435�[2mms�[22m�[39m
�[32m✓�[39m HookRunner process tree cancellation�[2m > �[22misolates the supervisor from hook NODE_OPTIONS�[32m 229�[2mms�[22m�[39m
�[33m�[2m✓�[22m�[39m HookRunner process tree cancellation�[2m > �[22mforwards abort through a surviving hook supervisor �[33m 2363�[2mms�[22m�[39m
�[33m�[2m✓�[22m�[39m HookRunner process tree cancellation�[2m > �[22mreaps a surviving hook when its supervisor is stopped before abort �[33m 2301�[2mms�[22m�[39m
�[31m �[31m�[31m HookRunner process tree cancellation�[2m > �[22mkeeps supervising a surviving hook group after its root exits�[39m�[33m 30539�[2mms�[22m�[39m
�[31m → Condition not met within 30000ms�[39m
�[33m�[2m✓�[22m�[39m HookRunner process tree cancellation�[2m > �[22mdelivers complete large input after the parent exits �[33m 1499�[2mms�[22m�[39m
�[31m⎯⎯⎯⎯⎯⎯⎯�[39m�[1m�[41m Failed Tests 1 �[49m�[22m�[31m⎯⎯⎯⎯⎯⎯⎯�[39m
�[41m�[1m FAIL �[22m�[49m src/hooks/hook-runner.process.test.ts�[2m > �[22mHookRunner process tree cancellation�[2m > �[22mkeeps supervising a surviving hook group after its root exits
�[31m�[1mError�[22m: Condition not met within 30000ms�[39m
�[36m �[2m❯�[22m waitFor src/hooks/hook-runner.process.test.ts:�[2m34:13�[22m�[39m
�[90m 32| �[39m �[35mwhile�[39m (�[33m!�[39m(�[35mawait�[39m �[34mpredicate�[39m())) {
�[90m 33| �[39m �[35mif�[39m (�[33mDate�[39m�[33m.�[39m�[34mnow�[39m() �[33m>=�[39m deadline) {
�[90m 34| �[39m �[35mthrow�[39m �[35mnew�[39m �[33mError�[39m(�[32m`Condition not met within �[39m�[36m${�[39mtimeoutMs�[36m}�[39m�[32mms`�[39m)�[33m;�[39m
�[90m | �[39m �[31m^�[39m
�[90m 35| �[39m }
�[90m 36| �[39m �[35mawait�[39m �[35mnew�[39m �[33mPromise�[39m((resolve) �[33m=>�[39m �[34msetTimeout�[39m(resolve�[33m,�[39m �[34m25�[39m))�[33m;�[39m
�[90m �[2m❯�[22m src/hooks/hook-runner.process.test.ts:�[2m924:9�[22m�[39m
�[31m�[2m⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[1/1]⎯�[22m�[39m
�[2m Test Files �[22m �[1m�[31m1 failed�[39m�[22m�[90m (1)�[39m
�[2m Tests �[22m �[1m�[31m1 failed�[39m�[22m�[2m | �[22m�[1m�[32m19 passed�[39m�[22m�[90m (20)�[39m
�[2m Start at �[22m 16:39:27
�[2m Duration �[22m 77.92s�[2m (transform 2.24s, setup 381ms, collect 2.37s, tests 55.98s, environment 0ms, prepare 2.60s)�[22m
JUNIT report written to /__w/qwen-code/qwen-code/packages/core/junit.xml
round 2 · packages/core/src/memory/recall-scan-latency.test.ts: P (exit 0)
round 2 · packages/web-shell/client/components/MessageList.dom.test.tsx: P (exit 0)
round 2 · scripts/tests/release-workflow.test.js: P (exit 0)
round 3 · packages/cli/src/serve/server/session-pr-refresh.test.ts: P (exit 0)
round 3 · packages/core/src/hooks/hook-runner.process.test.ts: F (exit 1)
--- output tail · round 3 · packages/core/src/hooks/hook-runner.process.test.ts ---
�[1m�[46m RUN �[49m�[22m �[36mv3.2.7 �[39m�[90m/__w/qwen-code/qwen-code/packages/core�[39m
�[2mCoverage enabled with �[22m�[33mv8�[39m
�[31m❯�[39m src/hooks/hook-runner.process.test.ts �[2m(�[22m�[2m20 tests�[22m�[2m | �[22m�[31m1 failed�[39m�[2m)�[22m�[33m 59229�[2mms�[22m�[39m
�[33m�[2m✓�[22m�[39m HookRunner process tree cancellation�[2m > �[22mreaps a descendant that ignores SIGTERM before returning �[33m 3198�[2mms�[22m�[39m
�[33m�[2m✓�[22m�[39m HookRunner process tree cancellation�[2m > �[22mreaps an active synchronous hook tree on parent process-exit �[33m 2161�[2mms�[22m�[39m
�[33m�[2m✓�[22m�[39m HookRunner process tree cancellation�[2m > �[22mreaps an active synchronous hook tree on parent signal-exit �[33m 1246�[2mms�[22m�[39m
�[33m�[2m✓�[22m�[39m HookRunner process tree cancellation�[2m > �[22mreaps an active synchronous hook tree on parent handled-signal-exit �[33m 1328�[2mms�[22m�[39m
�[33m�[2m✓�[22m�[39m HookRunner process tree cancellation�[2m > �[22mreaps an active async hook tree on parent process-exit �[33m 917�[2mms�[22m�[39m
�[33m�[2m✓�[22m�[39m HookRunner process tree cancellation�[2m > �[22mlets a MessageDisplay hook after explicit parent exit write output and finish �[33m 710�[2mms�[22m�[39m
�[33m�[2m✓�[22m�[39m HookRunner process tree cancellation�[2m > �[22mlets a StopFailure hook after explicit parent exit write output and finish �[33m 1087�[2mms�[22m�[39m
�[33m�[2m✓�[22m�[39m HookRunner process tree cancellation�[2m > �[22mlets a SessionDelete hook after explicit parent exit write output and finish �[33m 1297�[2mms�[22m�[39m
�[33m�[2m✓�[22m�[39m HookRunner process tree cancellation�[2m > �[22mlets an async MessageDisplay hook after explicit parent exit write output and finish �[33m 1247�[2mms�[22m�[39m
�[33m�[2m✓�[22m�[39m HookRunner process tree cancellation�[2m > �[22mlets a MessageDisplay hook after natural parent exit write output and finish �[33m 1227�[2mms�[22m�[39m
�[33m�[2m✓�[22m�[39m HookRunner process tree cancellation�[2m > �[22mlets a StopFailure hook after natural parent exit write output and finish �[33m 1319�[2mms�[22m�[39m
�[33m�[2m✓�[22m�[
...truncated -- full content in the run artifacts.
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
…adline The previous commit polled for the descendant's pid file after the hook returned. That could not help the case it targeted: the hook's 300ms timeout has the supervisor SIGTERM then SIGKILL the whole process group, and the descendant is in that group, so by the time executeHook resolves the file either already exists or never will. On a host where node takes longer than 300ms to start, the poll only moved the same failure 30s later. Raise the hook deadline instead, so the descendant is alive to write its pid before the group is reaped; the poll stays as a harmless guard against filesystem visibility. Give the release retry an honest off switch. `vars.X || '2'` could not disable the retry safely: an empty variable falls back to the default, and '0' passes --retry=0, which outranks a workspace's own config-level retry — the very thing the comment said to avoid. 'off' now omits the flag, and the contract test pins it alongside the stale schedule wording it replaces. Say what the shared-lane latency bound actually checks. A 3x ceiling still read as a budget check while letting a 2.5x regression through. Make it an order-of-magnitude guard and label it as such; the budget itself is asserted off shared runners. Claude-Session: https://claude.ai/code/session_01AWWgJEqafyAT1Mc75T8N7h
|
A second review pass turned up four findings on this PR; three are fixed in the commit just pushed, one is deliberately deferred. Fixed — the surviving-hook test. The earlier commit polled for the descendant pid file after the hook returned, and that could not help the flake it targeted: the hook's 300ms timeout has the supervisor SIGTERM/SIGKILL the whole process group, the descendant is in that group, so by the time Fixed — the retry off switch. Fixed — the shared-lane latency bound. A 3× ceiling still read as a budget check while letting a 2.5× regression through. It is now a 10× order-of-magnitude guard and labelled as one; the budget itself is asserted off shared runners. Deferred — |
|
⏸️ Deferring to @wenshao — updated on the new head 中文说明⏸️ 转交 @wenshao —— 基于新 head |
chiga0
left a comment
There was a problem hiding this comment.
No blocking findings.
Approval blockers: none.
What I checked:
Session.ts:.catch(() => {})on#recoverLateDraincorrectly prevents unhandled rejections from best-effort recovery from escaping and terminating the process. The comment accurately describes the mechanism (logger throws on a later tick → escapes barevoid→ Node unhandledRejection). Fix is the right one; swallowing silently here is intentional.release.yml:VITEST_RETRYnow defaults to"2"for all release types (previously nightly/preview only). Theoffsentinel correctly omits the flag (avoids--retry=0overriding workspace-level config). Repository variable allows tuning without a PR. The contract test inrelease-workflow.test.jsis updated to pin the new expression.session-pr-refresh.test.ts:removeTempTreewithmaxRetries: 10, retryDelay: 50mscorrectly handles the ENOTEMPTY race wheredispose()does not await an in-flight sweep before teardown deletes the tree.hook-runner.process.test.ts: All timeouts widened to match loaded-host reality (PROCESS_STARTUP_TIMEOUT_MS=30s,PROCESS_REAP_TIMEOUT_MS=15s, outer90s).HOOK_GROUP_TIMEOUT_MS=5000gives the descendant time to start before the hook deadline fires. All assertions updated consistently.recall-scan-latency.test.ts: Assertion changed frommedian < budget/2tobest < 10×budgeton shared CI,best < budget/2elsewhere.bestis the least-contaminated-by-load sample — appropriate for a throughput property on a shared runner.MessageList.dom.test.tsx: Fixed frame-count polls replaced withwaitForFrames()polling against a 10s deadline.predicateis a positive assertion, so a broken test still fails (predicate never true → exit after 10s → explicit assertion below fails).
No prior reviews to cross-check.
Tier: Standard. Execution not run locally (working tree unavailable); user confirmed Fable 5 verification.
Reviewed with AI assistance.
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
qqqys
left a comment
There was a problem hiding this comment.
Reviewed at head 6ef630da. No historical blocking issues exist on this PR (the only review on record is a dismissed empty submission; the triage pass at this exact head found no blockers), and my independent Critical-only scan finds none.
Production changes (the only two non-test hunks):
Session.ts— the fire-and-forget late-drain recovery now carries.catch(() => {}). Verified against the base:#recoverLateDrainswallows only the drain promise's own rejection; anything thrown after that race (the debug logger among it) escaped the barevoidas an unhandled rejection, which terminates the process and is exactly the "red shard with no failing test" shape that has failed eight consecutive release runs. The catch closes that hole without changing the recovery's best-effort semantics, and swallowing silently is correct because the logger is itself one of the things that can throw.release.yml— the stable lane now retries viavars.QWEN_RELEASE_VITEST_RETRY || '2', and the guard omits the flag entirely for the'off'sentinel, so disabling never passes--retry=0(which would outrank a workspace's config-level retry, e.g.packages/sdk-typescript). The contract test pins both the default and the'off' → flag omittedbehavior.
Test hardening: every deflake stays assertion-preserving and bounded — fsp.rm built-in maxRetries/retryDelay for the ENOTEMPTY teardown race; the surviving-hook group deadline moved from 300 ms to 5 s because a loaded host can kill the descendant before it writes its pid file (the original race no wait could recover), with the error-message assertion now tracking the constant; the latency test asserts the best sample against a strict budget/2 bound off shared runners and an order-of-magnitude ceiling on them; waitForFrames polls frames against a 10 s wall clock while both call sites keep their real assertions (onLoadOlderHistory count, scrollTop === 600), inside the config's 60 s shared-runner timeout, so a state that never lands still fails.
Known residual (non-blocking): startSessionPrRefreshTimer's dispose() does not await an in-flight sweep, so a teardown can still race a writer — the retrying rm absorbs it; the residual is a leaked temp dir on a CI runner, not a wrong result, and the clean fix is a product-API change out of scope here.
CI at this head: the only failing check is Dependency CVE audit, which is not introduced by this PR — the diff touches no dependency manifest and the failure comes from advisories already in the base lockfile. Test (ubuntu-latest, Node 22.x) and a few other lanes are still pending, which does not gate this review per policy; everything else is green or skipped by design.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
The lockfile bump moved fast-uri and the companion's notices carry a version on each dependency's header line, so the generated file went stale and its up-to-date check failed the build — the same shape QwenLM#10842 hit when its own lockfile bump landed without this. Derived rather than regenerated: the bump touched only fast-uri, its old version appears once in the file, and the same derivation on QwenLM#10842 came out byte-identical to the regenerated file committed there. Claude-Session: https://claude.ai/code/session_01AWWgJEqafyAT1Mc75T8N7h
'enforces a surviving hook timeout after the parent exits' configured its fixture deadline at 300ms, but the supervisor's clock starts at hook spawn, so on loaded hosts the group was killed before the hook wrote its ready/pid files; the driver's busy-wait then never broke and the test burned its 90s vitest budget instead of exercising the reap assertions. Use HOOK_GROUP_TIMEOUT_MS, the way the byte-similar group test was converted. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Concurrent self-hosted runner instances share HOME, so deleting or updating ~/.m2/toolchains.xml races with Maven startup in neighboring jobs. Point setup-java at the per-runner temp directory and make every Maven invocation consume those job-local settings and toolchains files. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
… connection
The release gate went red on this test for the first time on run
33701581941: `session/list scan alive when one connection is destroyed`
failed with `-32603` instead of the empty session list, twice in a row.
`PersistedSessionListCache` aborts an in-flight scan the moment its
waiter count reaches zero, and a request only becomes a waiter once it
reaches `lookup()`. The test posted both requests, waited for the loader,
and then deleted the first connection — which proves the *first* request
attached but says nothing about the second. On a loaded runner the second
can still be resolving params when the DELETE lands, so the count drops
to zero, the shared scan is aborted, its promise rejects, and the second
request answers out of that rejection as an internal error.
Watch `lookup()` for the single-flight join so the DELETE can never land
before the second waiter is attached. The waiter count is bumped
synchronously inside `lookup()`, so observing the `single_flight` status
is exactly the "second waiter attached" edge the test needs — and it
holds whichever of the two requests wins the race to start the scan.
The cache's own unit test already covers the invariant deterministically
("cancels one waiter without aborting the shared load"), so this is a
test-side race only; no production behavior changes.
Claude-Session: https://claude.ai/code/session_012797rgiteWJxLT9TLkKq8G
|
门禁首次拿到真实测试失败的根因已定位,不是本 PR 改动引起的。
根因: 我把这套语义单独抽出来在本地跑了一遍验证(脱离 vitest,直接驱动
修法( |
The first pass matched only `expect(elapsed).toBeLessThan(N)`, which is under a third of the family. Twenty-three more write the measurement inline — `expect(Date.now() - started).toBeLessThan(N)` — including two in acp-http/transport.test.ts, the file that flaked on #10842's CI run. Two sites stay as they are: NativeLspService's bound runs under fake timers, and hook-runner.process.test.ts belongs to #10842. Claude-Session: https://claude.ai/code/session_01MCE9CXnMVrX4fUxHpQoUr8
…ling on it `isOwnInterfaceAddress` re-reads `networkInterfaces()` on every call, while the suite snapshots the host's addresses once at collection. A shared CI host adds and drops veth interfaces continuously, so an address captured at collection can be gone minutes later when a case asserts on it — the function then correctly answers false and the case fails on a host that changed rather than on the lost normalisation it exists to guard. That is what reddened Workspace Tests (2/3) of release run 33676423730: `strips an RFC 6874 zone identifier before matching` and `matches an own address case-insensitively` — the two cases that run last here — failed `expected false to be true`, while the bare and bracketed cases that run before them passed on the same addresses. Intersect the collection snapshot with a fresh read per case. The fresh read comes from `node:os`, not from the function under test, so no case can confirm itself, and the vacuity guard stays: the loopback never churns, so the intersection never runs empty. Claude-Session: https://claude.ai/code/session_012797rgiteWJxLT9TLkKq8G
|
又加了一条(
修法是每个用例用 顺带汇报一下这个 PR 的实际覆盖面,比标题说的「one flaky test」大:最近两次 release run 的失败根因里,Session.ts 的 unhandled rejection、 还有一条本 PR 覆盖不到、需要单独处理的: |
qwen-code-dev-bot
left a comment
There was a problem hiding this comment.
Reviewed at head db1069cf.
- The release-policy change is well-reasoned: a single flake out of ~30k tests reddened six consecutive stable runs, so the stable lane now takes the same bounded retry as nightly/preview — a real break still fails every attempt, and operators can retune or disable via the repository variable, with the
offsentinel (not an empty string) correctly preserving per-workspace config retries. - Every finding from the earlier round, including both Critical budget gaps in the process-suite waits, was resolved: the widened inner waits are now aligned under the per-test timeouts by the follow-up commit, and all 7 threads are closed. The non-shared recall ceiling actually tightened (median→best-sample with a stricter bound off ECS), so flake removal did not buy slack at the cost of signal.
- The Session late-drain catch is the right hardening for a best-effort recovery path (post-race throws escaping
voidas an unhandled rejection would end the process), and the Maven per-job config isolation removes a shared-$HOMEteardown race rather than papering over it. - The standing CHANGES_REQUESTED predates the fixes at this head and its own body lists no remaining must-fix items; CI has no failures (seven lanes still running); per the channel convention the call is on the review itself.
qqqys
left a comment
There was a problem hiding this comment.
Re-reviewed at head db1069cf after the head moved past my earlier (now-dismissed) approval. Both historical Criticals from the CHANGES_REQUESTED round are verified fixed at this head, and my independent Critical-only scan over the full expanded diff (15 files) finds no blocking defects.
Historical blockers — verified fixed at this head:
- R1-9 / R1-1 (the two Criticals): every per-test budget enclosing the widened
PROCESS_STARTUP_TIMEOUT_MS(30 s) /PROCESS_REAP_TIMEOUT_MS(15 s) waits inhook-runner.process.test.tsis now90_000— bothit.eachprocess-tree groups and all sibling tests — so the inner waits can actually complete instead of dying to a bare vitest timeout with no diagnostic. - The round's non-blocking items are also addressed and present at this head: the Session.ts drain-guard regression test (a real
Session.prompt()driven through a mid-turn drain timeout with the module-graph logger faulted exactly on the recovery's debug line, asserting zero escaped rejections), the recall-latency strict branch asserting the median again (smallest[SHARED_CI ? 1 : 2]), the lane-awareFLUSH_DEADLINE_MS(10 s on ecs-qwen- runners inside the 60 s budget, 4 s elsewhere inside vitest's 5 s default), and the two corrected comments.
Critical-only pass on the scope added since that round:
Session.ts— the fire-and-forget late-drain recovery keeps its.catch(() => {}); unchanged from the earlier verified fix.- Dependency bump (
package-lock.json) — qs 6.15.2 → 6.16.0, side-channel 1.1.0 → 1.1.1, side-channel-list 1.0.0 → 1.0.1, with exactly those three NOTICES.txt headers regenerated in lockstep; the Dependency CVE audit is green at this head. sdk-java.yml— the shared-$HOME toolchains-tear fix is replaced by per-job Maven isolation (settings-pathunderrunner.tempplus explicitMAVEN_ARGSon every mvn step in both jobs), and the new contract test pins the wiring and the removal of the oldrm -fhack; the four completed Java lanes at this head pass with it.- The serve test fixes (single-flight
lookup()join wait before the DELETE teardown;stillOwn()intersection with a vacuity guard for churning veth interfaces) and the autofix suite's file-level 90 s headroom are bounded and assertion-preserving.
CI at this head: no failing or cancelled checks at review time; Test (ubuntu-latest, Node 22.x), Serve A/B, the no-AK integration lane, two Java lanes and a few orchestration checks are still pending, which does not gate this review per policy.
chiga0
left a comment
There was a problem hiding this comment.
Scope: 15 changed files — product source (1), test hardening (9), workflows (3), NOTICES (1), lock (1). Working tree unavailable; execution rungs not run. Cross-file context fetched for Session.ts and the two changed workflow files.
Round: This is round 2 (round 1 was at head 6ef630da, dismissed; current head is db1069cf).
Cross-check of prior reviews
The CI bot filed two Critical findings and five Suggestions. Both criticals are resolved at the current head:
- R1-9 resolved:
hook-runner.process.test.tsouter budgets for the twoit.eachgroups ('reaps an active %s hook tree'and'lets %s write output and finish') are now90_000at lines 358 and 531, matching the widened inner waits. The CI bot reviewed an intermediate head where those two stayed at15_000/10_000. - R1-2 resolved: The regression test the CI bot requested (
keeps a logger failure inside late drain recovery from escaping) was committed before the current head. It faults the debug-logger spy, resolves the late drain promise, flushes microtasks, and assertsunhandledRejectionnever fired. - R1-3 resolved: The tuple index assertion is
smallest[SHARED_CI ? 1 : 2]. Index 2 is median on the new 4-column tuple, so non-CI runs still assert the median — not the best sample as the CI bot's intermediate-state finding claimed. - R1-5 resolved: The stale comment is no longer present; the text at HEAD correctly states the new invariant.
Two suggestions remain open at HEAD:
- R1-7 (open, suggestion):
FLUSH_DEADLINE_MS = 4_000on non-ECS, vitest defaulttestTimeout = 5_000. A frame flush taking > 4s on ubuntu-latest could exhaust the deadline before the predicate is satisfied. This is strictly better than the original one-frame wait and is not a regression; the fix targets ECS lanes. - R1-11 (open, suggestion): The new header comment says “Every test here spawns real
node --import=tsx/esmprocesses” but only 4 of 11 blocks do. A maintainer resizing a plain-node budget using the tsx startup model would over-widen. Not a correctness issue.
Product fix — Session.ts
Mechanism confirmed (static proof chain).
#recoverLateDrain is an async function. After the await Promise.race settles, the debugLogger calls execute on a continuation microtask outside the try/catch that swallows the late rejection. If either throws, the async function's returned Promise<void> rejects. With a bare void, that rejected promise has no handler → unhandledRejection → process exit.
The fix — void this.#recoverLateDrain(drainPromise).catch(() => {}) — attaches a handler to the returned promise. Swallowing silently is appropriate here: the logger itself is listed as a possible throw site.
The regression test drives a real session.prompt() through a mid-turn drain timeout, faults debugLogger.debug on the recovery message, resolves the late promise, flushes microtasks, and asserts zero unhandledRejection events. The test is non-vacuous: without .catch(), the handler fires.
Test hardening
hook-runner.process.test.ts:PROCESS_STARTUP_TIMEOUT_MS = 30_000,PROCESS_REAP_TIMEOUT_MS = 15_000, outer budgets90_000.HOOK_GROUP_TIMEOUT_MS = 5_000(raised from300) gives the descendant time to write its PID before the supervisor fires. Template-literal interpolation of the constant into fixture code is correct; the expected error string matches.recall-scan-latency.test.ts: Best/median/worst tuple. ECS assertsbest < 10×budget; non-ECS assertsmedian < budget/2.local-bind-addresses.test.ts:stillOwn()intersects snapshot with a fresh read per case; loopback guarantees non-empty.session-pr-refresh.test.ts:maxRetries: 10onfsp.rmhandles ENOTEMPTY from the in-flight sweep.transport.test.ts:lookupSpyensures the second request joins the single-flight before DELETE issues. Spy restored infinally.MessageList.dom.test.tsx: Polling-with-deadline replaces fixed frame count. Non-true predicate →waitForFramesreturns → downstream assertion fails. Not vacuous.
Workflows
release.yml: VITEST_RETRY defaults to "2" for every release type. The 'off' sentinel skips retry_arg construction without passing --retry=0 (which would outrank workspace configs). Contract test covers all three sentinel cases.
sdk-java.yml: settings-path: '${{ runner.temp }}/setup-java-m2' + MAVEN_ARGS replaces the rm -f toolchains.xml workaround. MAVEN_ARGS is read by Maven 3.9+ and the wrapper; ECS runners are controlled infrastructure verified on Linux. The workflow test checks all four test-job steps and the one E2E step.
Verdict
No blocking findings. No approval blockers.
Unreviewable dimensions: working tree unavailable — no execution rungs run. The two open CI-bot suggestions (R1-7, R1-11) are minor boundary and documentation concerns; neither names a reachable correctness defect.
Reviewed with AI assistance.
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed at head db1069cf. Working tree unavailable — static read of the diff plus the enclosing bodies (the Session recovery path, the hook process suite, both workflow files) fetched at this commit; runtime evidence is this PR's own CI, green on every completed lane.
Approving. The substance is one correct production fix plus well-targeted test hardening, and the two historical Criticals are genuinely resolved at this head.
Production fix (Session.ts) — correct and necessary
#recoverLateDrain's try/catch wraps only the Promise.race. Everything after it — debugLogger.warn / debugLogger.debug, parseMidTurnDrainResponse, the midTurnRecoveredMessages.push — runs outside that catch, so a throw there rejects the async function's returned promise, and the bare void leaves it unhandled: the process ends on a path whose whole purpose is best-effort recovery. .catch(() => {}) attaches the missing handler, and swallowing silently (rather than logging) is the right call because the logger is itself one of the throw sites.
The regression test is non-vacuous: it drives a real session.prompt() through a mid-turn drain timeout, faults the module-graph debug spy exactly on the timed-out drain line (Session.ts:8390), asserts that line was reached, and asserts zero escaped unhandledRejections. Deleting the guard turns it red.
R1-9 / R1-1 (the two Criticals) — resolved
All eleven per-test budgets in hook-runner.process.test.ts are now 90_000, above the widened inner waits (startup 30 s / reap 15 s / hook-group 5 s; worst-case sum ~50 s). Because waitFor throws Condition not met within Nms on timeout, the inner wait now surfaces a real diagnostic before vitest's per-test timeout can kill the test silently — exactly what the finding required. The HOOK_GROUP_TIMEOUT_MS raise (300 → 5000) lets the descendant write its pid before the supervisor reaps the group, and the expected Hook timed out after Nms string is interpolated from the same constant.
Workflows
VITEST_RETRY: "${{ vars.QWEN_RELEASE_VITEST_RETRY || '2' }}" retries every schedule including stable; the off sentinel omits the flag rather than passing --retry=0 (which would outrank sdk-typescript's own config retry), and an empty variable falls back to 2 — all three cases pinned by the contract test. In sdk-java.yml, settings-path + MAVEN_ARGS (Maven 3.9.11 honors MAVEN_ARGS) move setup-java's non-atomic writes off the shared $HOME, replacing the rm -f ~/.m2/toolchains.xml mitigation instead of layering on it. All six Java lanes are green at this head, which confirms the relocated settings.xml / toolchains.xml exist and every mvn step consumes them.
Test hardening — assertion-preserving throughout
recall-scan-latency.test.ts: keeps the strictmedian < budget/2off shared runners and only relaxes tobest-of-5 < 10×where the median is contention-dominated — signal preserved where it is measurable.MessageList.dom.test.tsx: replaces fixed frame counts with a lane-aware poll deadline (10 s on ECS inside the 60 s budget, 4 s elsewhere inside vitest's 5 s default), so a flush that never lands fails as an assertion rather than a hang.- serve
transport.test.ts: gates the DELETE teardown on the second waiter joining the single-flight via alookup()spy (restored infinally), closing the race where the second request answered from an aborted scan. local-bind-addresses.test.ts: intersects the collection snapshot with a freshnode:osread per case and keeps a loopback vacuity guard, so a churning veth no longer fails a case and no case confirms itself.session-pr-refresh.test.ts:maxRetrieson the teardownrmabsorbs the ENOTEMPTY from an un-awaited in-flight sweep.
Deps
qs 6.15.2 → 6.16.0 and the side-channel / side-channel-list bumps regenerate exactly those NOTICES headers in lockstep; Dependency CVE audit green.
One non-blocking follow-up (optional, pre-existing)
hook-runner.process.test.ts:504 still asserts Date.now() - readyAt < 1000 for the natural exit-mode cases — a wall-clock upper bound in the same #10490 family. #10870 defers this file to this PR, and this PR widened the startup/reap waits but not that assertion. It was not in the observed failure set, so it is not a reason to hold anything; if the release lane reddens here later, routing it through the same treatment (or #10870's helper once it lands) is the natural next step.
CI note: at review time Test (ubuntu-latest) — the lane that runs every hardened suite here — was still in flight. Serve A/B shows cancelled, but that is its 60-min job timeout (serve-ab.yml:78) cutting off the base-side build+drive after the PR-head side had already completed successfully (~38 min on a contended runner); this PR touches neither serve-ab.yml nor its drive scripts, and the three serve files it does change are unit tests that run in the main Test lane, not the A/B drive. Every lane that actually executed this PR's code is green — all six Java lanes including daemon-e2e, Integration Tests, web-shell visuals, Dependency CVE audit. So this approval rests on the code as read plus that green evidence, with the main Test lane expected to land green; the cancellation is the shared-pool contention these two PRs exist to stop absorbing, not a defect in this diff.
…timing out `[vitest-worker]: Timeout calling "onTaskUpdate"` is Vitest's own worker-to- main RPC giving up on a starved host. It carries no product signal, and `--retry` cannot cover it: retries re-run failing TESTS while an unhandled error fails the run outright. Release run 33713579913 hit it on both attempts of v0.23.0, each time with every test green. The wrapper already parses those blocks, so it now tells the two apart: a run whose every `Error:` line is that signature is reported as a warning and handed back as a pass; one non-transport error anywhere — #10842's Session.ts rejection is the case that matters — keeps failing the run. Claude-Session: https://claude.ai/code/session_01MCE9CXnMVrX4fUxHpQoUr8
…ling (QwenLM#10805) * fix(release): report a workspace test run that fails with nothing failing Release run 33576013293 ended with "Test Files 211 passed / Tests 9480 passed" and exited 1. packages/core/vitest.config.ts sets dangerouslyIgnoreUnhandledErrors to false on Linux, so one error thrown outside any test fails a run whose tests all passed — and the log then carries no FAIL line for anyone to search for. The release notification reported it as an ordinary `quality` failure, indistinguishable from a real test failure, and the only way to learn otherwise was to read the whole shard log. Run the shards through a wrapper that streams the suite's output untouched and, only when the run exits non-zero with no failing test, adds a GitHub annotation naming the shape of the failure: the unhandled-error block when vitest reported one, the run's tail when it did not. Runs that pass, and runs that fail with failing tests, are left exactly as they were. The wrapper is where this belongs rather than the workflow: the classification needs the whole stream, and it is worth unit tests. classifyRunOutput and describeSilentFailure are pure and covered against captured samples of all three shapes — green-with-unhandled-error, ordinary failure, clean run. release-workflow.test.js and package-scripts.test.js pinned the step's exact command; both now pin the wrapper and still assert the sharding, the retry wiring and that the release lane does not fall back to test:ci. * fix(release): classify the workspace test stream instead of buffering it The wrapper collected every chunk of the suite's output and concatenated the lot to scan it once at the end. That output runs to tens of thousands of lines per shard, and the wrapper sits immediately outside the process already running under a 3GB heap cap — the one place a full copy of the log is least affordable. Nothing in the report needs the whole text. Split the scan into an incremental classifier that consumes lines as they stream past and keeps only what it reports: a bounded tail, the unhandled blocks, and the last few summary lines, which are now trimmed as they arrive rather than at the end. classifyRunOutput stays as a thin wrapper for callers that already hold the text. Each stream keeps its own partial-line remainder. A chunk can end mid-line, and splicing half a line of stdout onto the next stderr chunk would invent a line neither stream printed — and a spliced 'FAIL' is exactly the token that decides whether the run gets annotated. Claude-Session: https://claude.ai/code/session_01AWWgJEqafyAT1Mc75T8N7h * fix(release): make the wrapper's main-module guard hold on real paths The guard compared `import.meta.url` against a hand-built `file://` plus argv[1]. `import.meta.url` is a percent-encoded WHATWG URL, so the two disagree for any checkout path holding a space, `#`, `%` or a non-ASCII byte, and on Windows always. A false guard skips the whole CLI block: the wrapper exits 0 having spawned nothing, which is precisely the silent green it exists to prevent. Use `isMainModule`, which already lives in release-script-utils and compares resolved paths — one implementation of the check rather than a second spelling of it. Spawn a `.cmd` shim through a shell on Windows. Node >= 22 refuses it otherwise, and the repo pins Node 22, so the branch the file declares could not have run. Anchor the FAIL header match to the start of the line. Whitespace-bounded anywhere, a passing test NAMED for failure ("renders FAIL banner for an expired token") sets hasFailingTests, and a set flag suppresses the annotation — the script's whole purpose, undone by a test name. Move escapeWorkflowCommand into release-script-utils, where the other release scripts already import from, and alias the wrapper's escapeAnnotation to it. The escape contract had two byte-identical implementations under two names; a correction to either would have left the other silently wrong with both suites green. Cover what none of this had covered: the CLI entry spawned as a child the way the workflow invokes it (argv, the single `--`, exit-code forwarding), the same from a path containing a space, that importing the module spawns nothing, a signal-killed suite reporting failure rather than success, and a coloured failed-count line that only matches once the ANSI codes are stripped. Each was checked to go red against the exact mutant it names and green again when reverted. Claude-Session: https://claude.ai/code/session_01AWWgJEqafyAT1Mc75T8N7h * fix(release): strip the rule run from the unhandled-error header The header strip was a no-op. RULE_RE matches a line made entirely of rule characters, which a line carrying the words "Unhandled Errors" never is, so the header went into the block verbatim with its flanking rule runs. Strip only the edges, and pin the result so the no-op cannot return. Drop the escapeAnnotation alias. The previous commit moved the escaper to release-script-utils on the grounds that two names for one contract drift, then kept a second name for it anyway. The wrapper now calls escapeWorkflowCommand directly, and its test asserts the shared function. Claude-Session: https://claude.ai/code/session_01AWWgJEqafyAT1Mc75T8N7h * fix(scripts): use a shell only for the .cmd shim on Windows cmd.exe re-parses the joined command line, so routing every spawn through it mangled quoted -e payloads (and any program path with a space) handed to real executables, and the bash-driven retry test resolved the real npm.cmd via PATHEXT past its extensionless stub. Restrict shell:true to .cmd/.bat commands — the npm.cmd shim that needs it, matching scripts/dev.js — and gate the retry test off win32. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmtkfeo54dl * fix(scripts): decode piped output with a per-stream StringDecoder chunk.toString('utf8') decodes each raw pipe chunk on its own, so a multi-byte character straddling a chunk boundary became U+FFFD and the rule lines bounding an unhandled-error block stopped matching (bot witness: 102 mismatches across a 700-position byte-boundary sweep). createStreamConsumer decodes per stream ahead of the line splitter and flushes the decoders at finish; sinks still receive raw chunks byte-identical. Covered by a test cutting a Buffer inside the first rule character. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmtkfeo54dl * test(scripts): cover the stderr leg and the end-of-stream block flush Two classifier paths had no test: a FAIL line that only reaches stderr (drop the stderr leg and the wrapper annotates a silent failure over a log whose failing tests are right there), and an unhandled block whose closing rule line never arrives because the stream ended (drop finish()'s flush and the block is silently dropped). fakeRun gains a stream parameter so a fake suite can write to either pipe. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmtkfeo54dl * test(scripts): keep CI step summary clean and finish the escapeWorkflowCommand consolidation - runEntry now redirects GITHUB_STEP_SUMMARY to a scratch file so the spawned wrapper's real appendFileSync no longer appends fake report sections to the running job's own step summary; the redirect is asserted as coverage (mutation-checked: removing it turns the new test red). - Drop the escapeWorkflowCommand re-export from generate-release-notes and point its last importer (its own unit test) at release-script-utils.js, the single public home. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmtkhjtuvdq * fix(scripts): realpath both sides of the main-module guard and pin the stderr tail flush isMainModule compared path.resolve(argv[1]) — which keeps symlinks — against fileURLToPath(import.meta.url), which Node realpath-resolves for the ESM entry module. Invoked through a symlinked path (stock macOS, where os.tmpdir() links into /private/var, is one) the guard was false and the wrapper exited 0 having run nothing. Realpath both sides in the shared helper; a symlink-invocation regression test pins it. Also pin the createStreamConsumer stderr partial-line flush: a suite killed mid-line can leave its only FAIL text in an unterminated stderr fragment, and dropping it misclassifies the run as "no failing test". Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmtklu5f5dz * fix(deps): bump fast-uri to 3.1.7 in the lockfile fast-uri 3.1.5 sits in the newly published HIGH advisory range (3.0.0 - 3.1.5: GHSA-5jgf-p345-68v8, GHSA-f65p-4m7j-42xc, GHSA-fph4-wmhf-6fwf, GHSA-jqff-g426-hqxp), which fails the Dependency CVE audit's --audit-level=high gate. 3.1.7 is the newest patch within the existing ^3.0.1 range, so only the lock entry moves. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * chore(vscode): refresh NOTICES for the fast-uri bump The lockfile bump moved fast-uri and the companion's notices carry a version on each dependency's header line, so the generated file went stale and its up-to-date check failed the build — the same shape QwenLM#10842 hit when its own lockfile bump landed without this. Derived rather than regenerated: the bump touched only fast-uri, its old version appears once in the file, and the same derivation on QwenLM#10842 came out byte-identical to the regenerated file committed there. Claude-Session: https://claude.ai/code/session_01AWWgJEqafyAT1Mc75T8N7h * fix(scripts): keep isMainModule boolean when the entry path does not resolve The realpath-based guard changed the contract of a never-throwing string comparison: an unresolvable process.argv[1] (removed after startup, permissions, a symlink loop) made realpathSync throw ENOENT/EACCES/ELOOP out of a guard six release scripts run at import time. Swallow resolution failures to false — an entry that cannot be resolved is not the main module — and pin the contract with three tests (missing entry, different entry, symlinked invocation), mutation-checked against the catch removal. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(release): pass a run whose only unhandled error is the transport timing out `[vitest-worker]: Timeout calling "onTaskUpdate"` is Vitest's own worker-to- main RPC giving up on a starved host. It carries no product signal, and `--retry` cannot cover it: retries re-run failing TESTS while an unhandled error fails the run outright. Release run 33713579913 hit it on both attempts of v0.23.0, each time with every test green. The wrapper already parses those blocks, so it now tells the two apart: a run whose every `Error:` line is that signature is reported as a warning and handed back as a pass; one non-transport error anywhere — QwenLM#10842's Session.ts rejection is the case that matters — keeps failing the run. Claude-Session: https://claude.ai/code/session_01MCE9CXnMVrX4fUxHpQoUr8 * fix(release): preserve workspace test failures Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * refactor(release): simplify workspace failure reporting Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(release): let an operator retune the workspace shard timeout A shard's runtime is set by how busy the reserved host is, not by the suite: the same third measured 6.7 minutes on a quiet host and 36 on a contended one. 45 was chosen against the quiet end and has since killed shards at the boundary with every executed suite green — run 33713579913 lost 2/3 that way on both attempts, at 45.4 minutes. release.yml is code-owned, so a literal costs a review every time the fleet moves. The variable is the runtime knob QWEN_CI_VITEST_MAX_WORKERS and QWEN_RELEASE_VITEST_RETRY already are, and the default stays 45. Claude-Session: https://claude.ai/code/session_01MCE9CXnMVrX4fUxHpQoUr8 * fix(release): name which failure a red shard was A shard that died on Vitest's own worker RPC timing out reads identically to a real break: no FAIL line, exit 1. This release lost two attempts to that before anyone could tell them apart, and `--retry` cannot cover it — retries re-run failing TESTS while an unhandled error fails the run outright. Piping through `tee` lets the annotation say which of the three it is: a failing test names itself and gets none, a transport timeout gets a warning telling the operator to rerun, and anything else gets an error pointing at where to look. The child's status is re-raised untouched in every branch, so no reading of the log can turn a failure green. Claude-Session: https://claude.ai/code/session_01MCE9CXnMVrX4fUxHpQoUr8 * fix(release): trim the new comments back under the workflow size gate release.yml grew 4286 bytes against a 4096 allowance — 190 over. The gate's own advice is to cut the prose, which also serves keeping this PR small. Both blocks keep their evidence and their reason; only the wording goes. Claude-Session: https://claude.ai/code/session_01MCE9CXnMVrX4fUxHpQoUr8 * fix(release): pass a transport timeout that proves the run finished `[vitest-worker]: Timeout calling` is Vitest's own worker RPC giving up. It says nothing about the product, --retry cannot cover it (retries re-run failing TESTS while an unhandled error fails the run outright), and it has now cost v0.23.0 three of its four attempts — the last one on a host with nothing else on it, every test green, zero FAIL lines. An earlier revision downgraded on the signature alone, which was wrong: a child killed by a signal resolves without its code and would have shipped an unvalidated release. Four guards instead — a normal exit (a signal is 128+N), a passing tally, no failing tally, and no unhandled error that is not the transport. Any one of them missing and the failure stands. The baseline moves because the growth is real: the guards are the feature. Claude-Session: https://claude.ai/code/session_01MCE9CXnMVrX4fUxHpQoUr8 --------- Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Co-authored-by: yiliang114 <jinjing.zzj@gmail.com>
…#10870) * test: stop millisecond budgets from measuring the shared pool A budget written on a developer machine measures the code. On the shared ECS pool it measures the neighbours: the same third of the same suite runs in 6.7 or 36 minutes there depending only on which host it lands on, and at that spread every one of these bounds is a coin flip. Route the eleven wall-clock assertions through a helper that skips them when the workflow marks the run as pool-hosted. They still run locally and on the GitHub-hosted lanes, which is where the number means something. Claude-Session: https://claude.ai/code/session_01MCE9CXnMVrX4fUxHpQoUr8 * test: cover the inline wall-clock budgets too The first pass matched only `expect(elapsed).toBeLessThan(N)`, which is under a third of the family. Twenty-three more write the measurement inline — `expect(Date.now() - started).toBeLessThan(N)` — including two in acp-http/transport.test.ts, the file that flaked on QwenLM#10842's CI run. Two sites stay as they are: NativeLspService's bound runs under fake timers, and hook-runner.process.test.ts belongs to QwenLM#10842. Claude-Session: https://claude.ai/code/session_01MCE9CXnMVrX4fUxHpQoUr8 * ci: move the latency gate to the end of the env block Three CI PRs were extending the same anchor after NO_COLOR, so whichever landed second had to resolve a conflict that carried no disagreement. Claude-Session: https://claude.ai/code/session_01MCE9CXnMVrX4fUxHpQoUr8 * test: keep a relaxed bound where the duration is the property Five of the quarantined cases have no other expect(): a complexity bound or a no-zombie guarantee is the whole test, so skipping on the pool would leave them running and checking nothing — greener than before and worth less. They now keep a bound 20x wide there: far beyond the ~5x contention this fleet shows, far under what a quadratic regression costs. Claude-Session: https://claude.ai/code/session_01MCE9CXnMVrX4fUxHpQoUr8 * test(review): stamp the ledger entries when the test runs, not when it loads The three entries were stamped from a `Date.now()` evaluated in the describe body — at collection — while `beforeEach` writes the plan file at execution. Entries are fenced against that file's mtime with 2s of slack, so the stamps go stale by however long collection took and the fence drops them as a previous run's. Reproduced against the real module: at a 0ms and 1.5s gap the count reads 2, at 3s it reads 1, past a minute it reads 0 — the same 1/0/0 the three attempts of release run 33713579913 read after collecting for 2260s. Claude-Session: https://claude.ai/code/session_01MCE9CXnMVrX4fUxHpQoUr8 * ci: restore the latency gate dropped by the previous commit The previous commit swept in a stale copy of ci.yml left in the worktree and deleted this branch's own env key. Nothing about the gate changed. Claude-Session: https://claude.ai/code/session_01MCE9CXnMVrX4fUxHpQoUr8 * test: move the ledger stamp fix to its own PR That change is a defect — a stamp taken at collection, fenced against a file written at execution — while this PR is a tradeoff about where wall-clock budgets are worth asserting. They review differently, so they ship separately. See QwenLM#10878. Claude-Session: https://claude.ai/code/session_01MCE9CXnMVrX4fUxHpQoUr8 * test: make the pool-lane bounds able to fail again The relaxed poolMultiplier bounds could never fire: the transport zombie tests' own 3s safety abort capped elapsed far below 1500x20, and the serve close bound (5500x20) sat above both the 5s force-close timer and the pool's 60s testTimeout, so vitest decided the outcome first. Assert what the lanes can actually observe: the zombie tests record whether the safety abort fired (a zombie ends only via that abort) and drop the multiplier; the close bound uses a 10x multiple that stays under testTimeout while a real stall still fails it. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmtl54lfvf2 * test: parse QWEN_SKIP_LATENCY_BUDGETS as a boolean '0' and 'false' are truthy strings, so the raw truthiness gate skipped every budget for them — the opposite of what exporting =0 to re-enable budgets intends. Parse '1'/'true'/'yes' as skip; anything else enforces. Both byte-identical copies updated in lockstep; helper suites pin the gate both ways, including that poolMultiplier must not relax the bound when the switch is unset. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmtl54lfvf2 * test: pin that poolMultiplier never relaxes the bound off the pool Add a case to both latency-budget test copies proving that with QWEN_SKIP_LATENCY_BUDGETS unset, poolMultiplier must not move the bound: the probe (1500 vs a 100 ms budget, x20) throws off the pool while the same call keeps the relaxed bound on it. The unconditional-multiplier mutant now fails this case in both packages. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmtl79r9if7 * test: pin the QWEN_SKIP_LATENCY_BUDGETS wiring in ci.yml Extend the ECS-predicate pin case so the new skip line is nailed verbatim beside its four VITEST_* siblings. Without the pin, an edit that alters the predicate or renders '1' on every lane silently disables every millisecond budget while the helper tests stay green; the pin goes red on that mutant. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmtl79r9if7 * test: keep the overall-cap bound asserting on the pool lane Give the wall-clock cap case poolMultiplier 5 so the pool lane keeps an upper bound at 4000ms: still under the ~5000ms drain a broken cap drains sequentially, but above a ~100ms firing under the fleet's ~5x contention. Without it the pool lane kept only the trivially-true lower bound, so the cap regression this test exists to catch shipped green there; the default x20 multiple would clear the drain too. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmtl79r9if7 * test(ci): raise the scripts suite timeout where the suite actually reads it 30s was the quiet-host figure. Release run 33725742855 lost its Quality Checks (Scripts) job to two files at once — qwen-autofix-workflow, whose heaviest case measures ~14s idle, and acp-serve-boundary-guard, which nothing here touches — neither of them slow, both past 30s on a contended host. An earlier attempt put `vi.setConfig({ testTimeout: 90_000 })` at the top of one of those files. It is still on main and it does not work: these cases register their timeout at collection, before a runtime call can move it, and the same file timed out at 30000ms with that line in place. Remove it and set the value where the suite reads it, behind an env knob so the pool can be retuned without a PR. Claude-Session: https://claude.ai/code/session_01MCE9CXnMVrX4fUxHpQoUr8 * test(agent-view): give the ready wait the product's own budget Four cases capped the worker-ready wait at 1s while the product allows 15s (DEFAULT_WORKER_READY_TIMEOUT_MS). None of them asserts anything about that deadline — it is scaffolding for the state they do assert. On the shared pool a worker takes longer than a second to report ready, so they failed three times each, with --retry=2 already on, in release run 33713579913. The two cases that DO assert the timeout keep their 1ms. Claude-Session: https://claude.ai/code/session_01MCE9CXnMVrX4fUxHpQoUr8 * test(serve): give the archive waits the room the shared host needs `vi.waitFor` defaults to a 1000ms deadline — a developer-machine figure, hardcoded in Vitest with no global knob. On the shared pool the archive install and its settle take longer than that, and release run 33713579913 lost both of these cases with --retry=2 already on. Only the five waits inside those two cases move. The deadline is scaffolding for the state they assert, never the thing under test. Claude-Session: https://claude.ai/code/session_01MCE9CXnMVrX4fUxHpQoUr8 * test: keep the complexity bounds asserting on the pool lane Fourteen sites are ReDoS and complexity guards — 'stays linear on pathological inputs', 'collapses an exponentially branching $ref type tree', 'does not catastrophically backtrack', 'runs in bounded time'. Skipping them outright leaves assertions that a quadratic regression satisfies, on the one lane every PR runs: ci-bot's witness is a 1.5s busy-spin in budgetGapDisclosures, green on the pool and red under the strict bound. The earlier pass only caught the sites with no other expect() at all. That judgement was too narrow — what matters is whether the remaining assertions survive the regression the bound exists to catch. Multipliers land the relaxed bound near 20s, a third of the pool's 60s testTimeout, so Vitest never decides first. review-footer's refusing-run case carries its own 20s timeout, so it takes x5. Claude-Session: https://claude.ai/code/session_01MCE9CXnMVrX4fUxHpQoUr8 * ci: quarantine latency budgets in autofix gates Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * test(ci): let autofix upsert use the suite timeout Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> --------- Co-authored-by: yiliang114 <jinjing.zzj@gmail.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Released in v0.23.0. |





What this PR does
Stable releases now retry a workspace test that fails, the way nightly and preview releases already did, and six tests that have actually blocked a release are hardened so they stop needing the retry.
The retry is the substance. The release quality gate runs about thirty thousand tests, and a stable release ran them with no retry at all — one flaky test anywhere in that set reddened the release outright. The count sits behind a repository variable, so it can be retuned or switched off without another PR.
One product fix comes with them. The mid-turn drain recovery is fired and forgotten with a bare
void, and its own comment claims it swallows a late rejection — but it only swallows a rejection of the drain promise. Anything that throws after that race, the debug logger included, escapes as an unhandled rejection, and Node ends the process on one. A best-effort recovery path must not be able to take a session down, so the call now swallows its own failures. This is also what reddened a shard while it reported no failing test: the throw lands on a later tick, so on a loaded host it arrives after the test that installed it has finished, and Vitest counts it as an error with all 8147 tests passed. No retry can help that, because nothing failed.The rest removes the specific tests that have been doing the blocking. Two of them waited a fixed number of animation frames, or exactly one frame, before asserting that a paginated load had fired and that a scroll position had been restored; both now wait for the state they are about to assert on. A teardown deleted its temp tree while a sweep it had not awaited was still writing into it, which fails with ENOTEMPTY when a file lands between the directory read and the final remove; the delete now retries. Every test in the process-tree hook file spawns real Node processes and then waits on a hard wall-clock deadline — five seconds for startup, three or four for reaping — and those deadlines now match the slowest host rather than an idle one. The recall scan latency test asserted a wall-clock median against a fixed ceiling, and now asserts the fastest of its samples instead.
Why it's needed
No stable release has published since August 31. Every attempt since has died the same way: build, typecheck, lint, scripts and both integration suites all pass, then one unit test somewhere in one shard fails and takes the release with it.
Three runs today, on the same commit, make the shape plain. The first failed on four tests, a rerun of it failed on two entirely different ones, and the next run failed on two more — one of which the first run had already hit. Six distinct tests so far, every run surfacing at least one the previous run had not, and each failing shard failing on exactly one test. That is a long tail of timing assumptions meeting a loaded host, not a code defect: the same tests pass on the next attempt.
Hardening the six is worth doing and it is not sufficient, because the seventh is not in this diff. The retry is what covers the tests nobody has seen fail yet. A real break still fails all three attempts; what the retry stops is a contended runner deciding whether a release ships.
Replayed against today's three runs, this diff covers all of them: the two that failed on one flaky test per shard would have been absorbed by the retry, and the third — the shard that failed with nothing failing — was the unhandled rejection fixed here.
The contention itself is the background to all of this. Three test shards land on one shared host, and the per-process worker cap was lowered to four when that sharding landed, which is why the wall-clock assertions started coming apart. That cap is a repository variable and has been raised separately; this PR does not touch it.
Reviewer Test Plan
How to verify
The three core tests can be run directly:
cd packages/core && npx vitest run src/hooks/hook-runner.process.test.ts— 20 passed in 24s.cd packages/core && npx vitest run src/memory/recall-scan-latency.test.ts— 1 passed. The printed table now carries best, median and worst per topic count.npx vitest run --config ./scripts/tests/vitest.config.ts release-workflow— 51 passed. This pins the newVITEST_RETRYdefault, so a silent drop back to a no-retry stable lane fails here.The remaining two files are covered by CI:
cd packages/cli && npx vitest run src/serve/server/session-pr-refresh.test.ts -t "prunes the sweep offset of a workspace removed from the registry"cd packages/web-shell && npx vitest run client/components/MessageList.dom.test.tsx -t "turn collapse"What was verified locally, stated plainly: the three commands above were run here and pass. The cli and web-shell tests could not run in this environment for reasons unrelated to the change, and rely on CI.
For the teardown fix the mechanism was verified directly, with a standalone script that recreates the race — a writer dropping files into a subdirectory while the tree is removed. A plain recursive remove failed with ENOTEMPTY on all twelve trials, the same error the shard reported; the same remove with retries succeeded on all twelve.
For the unhandled rejection the mechanism was likewise verified standalone: a fire-and-forget task whose logger throws on a later tick raises one
unhandledRejectionbehind a barevoid, and none once the call swallows its own failure.To confirm the widened waits have not gone vacuous: each is bounded, so a genuine hang still fails, only later. The passing path is unchanged — the process-tree file still completes in 24 seconds.
Evidence (Before & After)
N/A — no user-visible behavior changes.
Tested on
Environment (optional)
Unit tests and the workflow contract tests.
Risk & Scope
QWEN_RELEASE_VITEST_RETRYis optional and defaults to 2.Linked Issues
None.
中文说明
这个 PR 做了什么
稳定版 release 现在会对失败的 workspace 测试进行重试——nightly 和 preview 本来就是这么做的——同时加固六个确实拦住过 release 的测试,让它们不再需要依赖重试。
重试是本 PR 的主体。release 质量门禁要跑大约三万个测试,而稳定版此前跑它们时完全没有重试:这一堆里任何一个测试抖一下,都会直接把 release 判红。重试次数放在一个仓库变量后面,因此以后调整或关闭它不需要再发 PR。
随之还有一处产品代码修复。mid-turn drain 的恢复逻辑是用裸
void发射后不管的,它自己的注释声称会吞掉延迟到达的 rejection——但它只吞掉 drain promise 的 rejection。在那次 race 之后抛出的任何东西(包括 debug logger)都会逃逸成 unhandled rejection,而 Node 遇到它会终止进程。一条尽力而为的恢复路径不该有能力把整个会话带走,因此该调用现在会吞掉自身的失败。这也正是某个分片"红了却报不出任何失败测试"的原因:抛出发生在后续 tick 上,机器负载高时它会在安装它的那个测试结束之后才到达,于是 Vitest 把它记为一个 error,而 8147 个测试全部通过。这种情况任何重试都救不了,因为压根没有测试失败。其余部分是把一直在拦路的那几个测试去掉。其中两个原先等待固定帧数、或者恰好一帧,就去断言分页加载已经触发、滚动位置已经恢复;现在它们都改为等待自己即将断言的那个状态。一处 teardown 在自己没有 await 的 sweep 仍在往目录树里写入时就去删除它,当某个文件恰好落在读目录和最终删除之间时会以 ENOTEMPTY 失败;现在删除会重试。进程树 hook 那个文件里的每个测试都会启动真实的 Node 进程,然后等待一个硬编码的墙钟截止时间——启动 5 秒、回收 3 到 4 秒——这些截止时间现在按最慢的机器来定,而不是按空闲的机器。recall scan 延迟测试原先用墙钟中位数去比一个固定上限,现在改为断言最快的那个样本。
为什么需要
自 8 月 31 日起没有任何稳定版发布成功。此后每一次尝试的死法都一样:build、typecheck、lint、scripts 以及两套集成测试全部通过,然后某个分片里的某一个单测失败,把整个 release 一起带走。
今天在同一个 commit 上跑的三次 run 把形态摊得很清楚。第一次挂了四个测试,对它的重跑挂了另外两个完全不同的,再下一次 run 又挂了两个——其中一个是第一次已经撞过的。目前累计六个不同的测试,每一轮都会甩出至少一个上一轮没有出现过的,而且每个失败的分片都恰好只挂一个测试。这是一条长尾的时序假设撞上高负载机器,不是代码缺陷:同样的测试在下一次尝试中就能通过。
加固这六个值得做,但并不充分,因为第七个不在这个 diff 里。重试才是覆盖那些还没人见过它失败的测试的东西。真正的问题仍然会在三次尝试中全部失败;重试挡掉的,是让一台被压满的机器来决定一个版本能不能发。
拿今天三次 run 回放,这个 diff 全部覆盖:其中两次是每个分片挂一个 flaky 测试,会被重试吃掉;第三次那个"失败却没有失败测试"的分片,正是这里修掉的 unhandled rejection。
争抢本身是这一切的背景。三个测试分片落在同一台共享机器上,而每进程的 worker 上限在分片落地时被下调到 4,这正是那些墙钟断言开始崩掉的原因。那个上限是一个仓库变量,已经单独调高;本 PR 不涉及它。
复核测试计划
如何验证
三个 core 测试可以直接运行:
cd packages/core && npx vitest run src/hooks/hook-runner.process.test.ts—— 20 个通过,24 秒。cd packages/core && npx vitest run src/memory/recall-scan-latency.test.ts—— 1 个通过。打印的表格现在按 topic 数同时给出 best、median、worst。npx vitest run --config ./scripts/tests/vitest.config.ts release-workflow—— 51 个通过。它 pin 住了新的VITEST_RETRY默认值,因此若有人悄悄把稳定版改回不重试,这里会失败。另外两个文件由 CI 覆盖:
cd packages/cli && npx vitest run src/serve/server/session-pr-refresh.test.ts -t "prunes the sweep offset of a workspace removed from the registry"cd packages/web-shell && npx vitest run client/components/MessageList.dom.test.tsx -t "turn collapse"如实说明本地验证到什么程度:上面三条命令在本地跑过并通过。cli 和 web-shell 两个测试因为与本改动无关的环境原因无法在此运行,依赖 CI 验证。
对于 teardown 那处修复,改为直接验证其机制:用一个独立脚本复现该竞态——在删除目录树的同时,另一方持续往子目录里写文件。普通的递归删除在 12 次试验中全部以 ENOTEMPTY 失败,与分片报出的错误一致;改为带重试的删除后 12 次全部成功。
unhandled rejection 那处同样用独立脚本验证了机制:一个 fire-and-forget 任务的 logger 在后续 tick 抛出时,裸
void会产生 1 次unhandledRejection,而让该调用吞掉自身失败后为 0 次。关于放宽后的等待是否变成了空转:每个等待都有上限,真正的挂起仍然会失败,只是失败得晚一些。通过路径的耗时没有变化——进程树那个文件仍然是 24 秒跑完。
证据(前后对比)
N/A —— 无用户可见行为变化。
测试平台
环境(可选)
仅单元测试和 workflow 契约测试。
风险与范围
QWEN_RELEASE_VITEST_RETRY是可选的,默认值为 2。关联 Issue
无。
https://claude.ai/code/session_01AWWgJEqafyAT1Mc75T8N7h