fix(test): deflake three CI-load-sensitive tests - #8797
Merged
Conversation
waitForMockCall polled ten zero-delay event-loop turns and gave up. The mock it waits for fires after real async work (index rebuilds, cursor I/O), so on a loaded CI runner the poll spun through its ten turns without waiting any wall-clock time and the two rebuild-isolation tests failed with 'Expected mock to be called' (seen on PR #8773's Test (ubuntu-latest) job). Wait against a 2s deadline instead; the fast path still returns on the first check.
wenshao
requested review from
LaZzyMan,
doudouOUC,
tanzhenxin and
yiliang114
as code owners
August 9, 2026 07:24
Collaborator
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
The afterAll deletes several 16k-entry fixture trees — tens of thousands of unlinks — and blew past vitest's default 10s hook timeout on a loaded CI runner (PR #8773's second Test (ubuntu-latest) run), failing a suite whose 59 tests had all passed. Same CI-load flake class as the extract test fix in this branch.
wenshao
changed the title
Collaborator
|
Thanks for the PR! (Reviewed at the new head, which adds a second deflake commit on top of the original.)
Moving on to code review. 🔍 中文说明感谢贡献！（按新 head 审查，其在原 commit 之上追加了第二个 deflake commit。）
进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
Code review
Both edits carry comments explaining the non-obvious why, assertions are preserved, and the diff touches nothing else. No blockers. Test evidence (PR's own CI, read via API — no PR code executed)On the reviewed commit: the unit suite Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Honest caveats: (1) the extract flake is load-dependent, so a single green run can't statistically prove it's gone — but the fix removes the exact mechanism seen in the failing log (ten zero-delay turns elapsing in ~30 ms without any wall-clock wait). (2) Not verified: an actual hook-timeout failure for the manifest-context teardown — no linked occurrence found; that change rests on the documented mechanism (tens of thousands of unlinks vs a 10 s default hook budget) and the observed 8 s run time. Both changes are test-only with nothing user-visible, so real-scenario testing is N/A. 中文说明代码审查
两处改动都附有解释"为什么"的注释，断言保留，diff 未触碰其他内容。无阻塞项。 测试证据（PR 自身 CI，经 API 读取——未执行任何 PR 代码）在被审 commit 上：单元测试 诚实的说明：(1) extract 的 flake 与负载相关，单次绿色运行无法在统计上证明它已消失——但修复移除了失败日志中观测到的确切机制（十次零延迟轮询在约 30 ms 内耗尽而未等待任何真实时间）。(2) 未验证：manifest-context teardown 钩子超时的实际失败——未找到链接的发生记录；该改动依据的是成文机制（上万次 unlink 对比 10 秒默认钩子预算）与观测到的 8 秒运行时长。两处改动均为纯测试、无用户可见行为，真实场景测试不适用（N/A）。 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Confidence: 4/5 — two clean, minimal deflakes that remove observed failure mechanisms while preserving every assertion; the nits are a title/body that no longer covers the whole diff, and a Stepping back: the first problem was verified in the cited job log, not taken on faith — two tests dying in 19–30 ms at a zero-delay poll is exactly the shape the fix addresses, and the deadline-based replacement is the smallest thing that repairs it. The second change (teardown hook budget 10 s → 120 s) has no linked failure, but the premise checks out against the config (no CI on the reviewed commit is still running (the unit suite is in flight), so approval is deferred until CI lands green on 中文说明置信度：4/5 —— 两个干净、最小的 deflake，移除了观测到的失败机制且保留全部断言；小瑕疵是标题/正文未覆盖完整 diff，以及一个复用 退一步看：第一个问题是在被引作业日志中核实过的，不是听信描述——两个测试在 19–30 ms 内死于零延迟轮询，正是此修复针对的形态，而基于截止时间的替代方案是修复它的最小改动。第二个改动（teardown 钩子预算 10 秒 → 120 秒）没有链接的失败记录，但前提经核实成立：配置中没有 被审 commit 上的 CI 仍在运行（单元测试进行中），因此批准推迟到 CI 在该 commit 上全绿之后。 — Qwen Code · qwen3.8-max Reviewed at |
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
qwen-code-ci-bot
previously approved these changes
Aug 9, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
…ar I/O Each register/complete in the retention-cap loop tests also writes the status sidecar via atomicWriteFileSync, and loaded CI runners have been measured at ~700ms per sidecar write — the ~70 writes of the longest loop take ~50s, past vitest's 15s default, failing four tests whose assertions are pure eviction semantics (seen twice on PR #8773, on two different runners). Give them an explicit 120s timeout; no assertion changes.
wenshao
changed the title
yiliang114
approved these changes
Aug 9, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
LGTM. Verified the diff is test-only with zero assertion changes: waitForMockCall keeps its fast path and swaps the ten zero-delay turns for a 2s wall-clock deadline (the right fix — tick counts measure nothing under load), the manifest teardown keeps the cleanup it documents as mandatory and just gets 120s, and the four retention-cap tests only gain the SIDECAR_IO_TIMEOUT argument around unchanged bodies. The retention-cap diagnosis matches an independent observation: on #8687's branch these exact four tests timed out at 15s (~23s elapsed) on two different heads while every neighbouring single-write test ran ~700ms, consistent with the ~700ms-per-sidecar-write-under-load math. One non-blocking thought for later: since these tests exercise eviction logic rather than sidecar IO, stubbing atomicWriteFileSync would remove the load sensitivity structurally instead of budgeting for it — but the explicit timeout is the minimal-risk deflake and fine as-is. CI green on this head. Nothing blocks merge.
Collaborator
Review 总结结论：LGTM。 纯测试改动（3 个测试文件，产品代码零改动），防 flake 手段正确且有界。已逐项核对： 逐文件核对1.
2.
3.
其他检查
一个非阻塞的小建议（可选）： |
ytahdn
approved these changes
Aug 9, 2026
ytahdn
left a comment
ytahdn
left a comment
Collaborator
There was a problem hiding this comment.
LGTM — test-only deflake, verified statically: assertions unchanged (whitespace-insensitive diff), no fake-timer interference with the new wall-clock deadline, vitest timeout signatures valid, prettier clean, ubuntu CI green.
ZevGit
pushed a commit
to ZevGit/qwen-code
that referenced
this pull request
Aug 10, 2026
…ath (QwenLM#8813) * fix(test): stop background-shell tests sharing a fixed /tmp sidecar path `makeEntry` defaulted to `outputPath: '/tmp/s1.output'`, so every entry in this file — across tests, across workers, across CI jobs on the same host — mirrored its status sidecar to the single path `/tmp/s1.status`. `/tmp` carries the sticky bit. Once that file belongs to another uid, the atomic rename in `atomicWriteFileSync` fails EPERM, and `renameWithRetrySync` burns its full 50+100+200ms backoff before the registry swallows the error. Every register/complete then costs ~350ms and the sidecar never lands. That is what the loop tests were paying: the retention-cap cases do 68 register/complete calls, and CI measured 23.8s each. The durations across the whole file were exact multiples of 351ms — 352 / 703 / 1405 / 2113 / 3520 — with no variance, which is the backoff sum, not disk latency. Give each entry its own temp directory instead. The shared path is gone, the rename succeeds, and the file drops from 128.9s to 3.2s locally with `/tmp/s1.status` made immutable to reproduce the CI condition. QwenLM#8797 raised these four cases to a 120s timeout to survive the cost. With the cost removed the band-aid goes too, so a future regression fails loudly instead of silently taking two minutes. * fix(test): unify sidecar test helpers and pin per-entry outputPath uniqueness Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * test: assert the whole output-file element, not just its tail Unifying the sidecar helpers moved these two entries onto random temp directories, and the assertions were relaxed to a suffix match to cope. Rebuild the expected element from the path under test instead: the temp prefix is random, but the escaping and control-byte stripping these two cases exist to pin are exact. * test: escape expected XML paths the way the registry does The anchored `<output-file>` assertions built their expected value by hand — one replaced `&` only, the other nothing at all. `tmpdir()` may legally contain XML metacharacters (`&` on Windows, `<` on POSIX), so those cases became environment-dependent the moment they moved off the fixed `/tmp` path. Run the expected path through the same `escapeXml(stripDisplayControlChars())` the registry uses. Verified with `TMPDIR=/tmp/qwen-xml-probe/a&b<c`: the helper passes all three focused cases, while the hand-rolled version fails two. * test: escape last dynamic output-file expectation, stop sidecar leak The escape conversion in 5549db2 covered two of the three dynamic <output-file> expectations; `emits one task-notification when a shell completes` still interpolated the temp path raw, so a TMPDIR containing XML metacharacters (legal on POSIX and Windows) failed it. Route it through the same expectedOutputFileElement helper. Also remove the `<dir>.status` sidecar the directory-outputPath test leaks next to its temp dir on every run; afterEach only tracks the dir. * test(core): clean the dir-outputPath sidecar in afterEach, not after assertions (QwenLM#8813) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(core): reuse shared display sanitizer in background shell registry (QwenLM#8813) The registry kept a private stripDisplayControlChars that only removed C0/C1 controls, while the shared terminalSafe helper also strips Unicode bidi overrides and isolates. With a TMPDIR containing one of those characters the test expectations (built with the shared helper) lost a character the notification XML retained. Reuse the shared helper so both background notification surfaces apply the same Trojan-Source defense and the expectations cannot drift from the implementation again; extend the control-character test to pin the bidi stripping. --------- Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>
Collaborator
|
Released in v0.21.9. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Deflakes two test-infrastructure failure modes that only appear under CI load, both observed on PR #8773's
Test (ubuntu-latest, Node 22.x)runs. Product code is untouched.packages/core/src/memory/extract.test.ts):waitForMockCallnow waits against a 2 s wall-clock deadline instead of ten zero-delay event-loop turns. The fast path is unchanged — the helper still returns on the first check when the mock has already fired.packages/cli/src/commands/review/lib/manifest-repository-context.test.ts): theafterAllthat deletes several 16k-entry fixture trees gets an explicit 120 s hook timeout instead of vitest's 10 s default.packages/core/src/services/backgroundShellRegistry.test.ts): the four eviction-loop tests get an explicit 120 s test timeout — each loop iteration also writes the status sidecar throughatomicWriteFileSync, and loaded runners were measured at ~700 ms per sidecar write, putting the longest loop (~70 writes) near 50 s against the 15 s default.Why it's needed
Three independent green-suite-killed-by-infrastructure flakes:
waitForMockCallpolledsetTimeout(0)ten times and gave up. The mocks it waits for fire only after real async work (index rebuilds, cursor I/O), so on a loaded runner the poll spun through its ten turns without waiting any actual time, and two rebuild-isolation tests failed withExpected mock to be called(run). The same file passes locally in ~3 s repeatedly./tmpwithin a handful of runs — it just needs the time it takes./tmpis itself worth a look at the runner infrastructure, but the tests should not be the canary.)Reviewer Test Plan
How to verify
cd packages/core && npx vitest run src/memory/extract.test.ts— 18 passed.cd packages/cli && npx vitest run src/commands/review/lib/manifest-repository-context.test.ts— 59 passed, 1 skipped.cd packages/core && npx vitest run src/services/backgroundShellRegistry.test.ts— 57 passed.Expected mock to be called(after 2 s); a genuinely wedged teardown still times out (after 120 s).Evidence (Before & After)
N/A — test-only change.
Tested on
Risk & Scope
Linked Issues
Both flakes first observed on #8773's CI runs.
中文说明
这个 PR 做了什么
修复两个仅在 CI 高负载下出现的测试基建性失败,均见于 PR #8773 的
Test (ubuntu-latest, Node 22.x)运行。产品代码零改动。packages/core/src/memory/extract.test.ts):waitForMockCall改为按 2 秒真实时钟截止等待,替代十次零延迟事件循环轮询。快速路径不变——mock 已触发时仍在第一次检查即返回。packages/cli/src/commands/review/lib/manifest-repository-context.test.ts):删除多个 1.6 万条目 fixture 树的afterAll获得显式 120 秒钩子超时,替代 vitest 默认的 10 秒。为什么需要
三个相互独立的「套件全绿、被基建杀死」的 flake:
waitForMockCall以setTimeout(0)轮询十次即放弃。它等待的 mock 在真实异步工作(索引重建、游标 I/O)完成后才触发,高负载 runner 上轮询在未等待任何实际时间的情况下耗尽十轮,两个 rebuild-isolation 测试以Expected mock to be called失败。同一文件本地反复运行约 3 秒全部通过。/tmp——它只是需要它实际所需的时间。/tmp的原子写要 700ms 这件事本身值得排查 runner 基建,但测试不应当这只金丝雀。)风险与范围