fix(test): stop background-shell tests sharing a fixed /tmp sidecar path - #8813
Conversation
`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. #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.
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/takeover
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。移除 |
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. |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review round — no action neededNo actionable feedback was found for this round, so no code changes were made and nothing was committed. What was triaged:
Result: working tree unchanged; the PR stays on commit 中文说明Autofix 评审轮次 —— 无需处理本轮未发现可处理的反馈，因此未做任何代码改动，也没有提交。 分类处理的内容：
结果： 工作区未变动；PR 停留在提交 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
|
@qwen-code /review |
|
Qwen Code review request accepted. Review is queued in workflow run. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Test Plan (not a blocker): 57 tests pass — this review observed 19547, 1124, 18670, 1481, 481, 2941, 454 passed.
中文说明
已审查。 建议见行内评论。 Test Plan（非阻断）：57 tests pass — this review observed 19547, 1124, 18670, 1481, 481, 2941, 454 passed。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| outputPath: | ||
| overrides.outputPath ?? join(makeTempDir(), `shell-${shellId}.output`), |
There was a problem hiding this comment.
[Suggestion] The pre-existing makeDirEntry helper (~lines 782-795) now duplicates this construction: it independently builds the same makeTempDir() + shell-<shellId>.output shape with the same ?? 's1' default, plus hand-derives statusPath: join(dir, \shell-${shellId}.status`)— which the already-importedstatusFilePathForproduces directly. — Concrete cost: a future change to the canonicalshell-.output` naming scheme must be made in two places in this file, and the sidecar suite would silently keep exercising the old shape (its explicit override wins over the updated default) while every other test uses the new one.
function makeDirEntry(
overrides: Partial<ShellTaskRegistration> = {},
): ShellTaskRegistration & { statusPath: string } {
const entry = makeEntry(overrides);
return { ...entry, statusPath: statusFilePathFor(entry.outputPath) };
}中文说明
已有的 makeDirEntry 辅助函数(约 L782-795)现在与这段构造重复:它独立构造了同样的 makeTempDir() + shell-<shellId>.output 路径形状和同样的 ?? 's1' 默认值,还手工推导 statusPath: join(dir, \shell-${shellId}.status`)——而这正是已导入的 statusFilePathFor能直接给出的。— 具体代价:将来若修改shell-.output` 的规范命名,需要在本文件两处同步修改;而且 sidecar 测试组会因其显式覆盖优先于新默认值而悄悄继续验证旧命名,其余测试却已用上新命名。
— qwen3.8-max via Qwen Code /review (v0.21.8)
There was a problem hiding this comment.
Applied in cc588b2. makeDirEntry is now just makeEntry(overrides) plus statusFilePathFor(entry.outputPath), so the naming scheme lives in one place and the sidecar suite exercises the same shape as everything else.
Worth noting the side effect: because that group now inherits the default outputPath instead of overriding it, reverting makeEntry to the old constant fails four tests rather than one — the uniqueness guard plus three sidecar cases. The dedupe bought a stronger regression net than it looks.
| outputPath: | ||
| overrides.outputPath ?? join(makeTempDir(), `shell-${shellId}.output`), |
There was a problem hiding this comment.
[Suggestion] Two sibling tests in this file still override outputPath with fixed /tmp paths — '/tmp/out&err.log' (~line 337, register + fail) and '/tmp/out\x03.log' (~line 409, register + complete) — keeping exactly the shared-sidecar pattern this change removes from the default; each performs two sidecar writes to a fixed /tmp/<name>.status path. — Failure scenario: probe-measured on this runner: with a root-owned squatter file planted at the path in sticky /tmp, these two tests went from 72 ms combined to 705 ms / 702 ms (2 writes × the 50+100+200 ms EPERM backoff the new comment documents), and two stray .status files leak into /tmp per run because afterEach only removes the mkdtempSync dirs.
outputPath: join(makeTempDir(), 'out&err.log'), // and likewise for the \x03 sibling(relaxing the two <output-file> XML assertions to match the escaped suffix, e.g. toContain('out&err.log')).
中文说明
本文件里另有两个兄弟测试仍然用固定的 /tmp 路径覆盖 outputPath——'/tmp/out&err.log'(约 L337,register + fail)和 '/tmp/out\x03.log'(约 L409,register + complete)——保留了本次改动刚从默认值中消除的同一种共享 sidecar 模式;它们各自向固定的 /tmp/<name>.status 路径写两次 sidecar。— 故障场景:在本 runner 上实测:在带 sticky 位的 /tmp 中放置一个 root 所有的占位文件后,这两个测试从合计 72 ms 变为 705 ms / 702 ms(2 次写 × 新注释所记载的 50+100+200 ms EPERM 退避),并且每次运行都会向 /tmp 泄漏两个残留 .status 文件,因为 afterEach 只清理 mkdtempSync 目录。
— qwen3.8-max via Qwen Code /review (v0.21.8)
There was a problem hiding this comment.
Applied in cc588b2 — both siblings now use join(makeTempDir(), ...). Confirmed the leak you described is gone: rm -f /tmp/*.status, run the suite, and no .status file appears in /tmp afterwards.
One follow-up in 9158922: moving to random temp dirs had relaxed the two <output-file> assertions to a suffix match. Rebuilt the full element from the path under test instead (<output-file>${outputPath.replaceAll("&", "&")}</output-file>, and join(dir, "out.log") for the control-byte sibling) — the prefix is random but the escaping and stripping these cases pin are exact, so they should stay anchored.
| outputPath: | ||
| overrides.outputPath ?? join(makeTempDir(), `shell-${shellId}.output`), |
There was a problem hiding this comment.
[Suggestion] The default-outputPath uniqueness this PR exists to guarantee is not pinned by any assertion. — Failure scenario: probe-measured: reverting makeEntry to the pre-PR constant '/tmp/s1.output' passed all 57 existing tests — only the guard proposed below failed. On a single-uid machine or fresh CI container that regression is invisible; on a shared multi-tenant runner the collision flake this PR fixes would return, with no in-repo guard pointing at the cause.
it('gives each entry a unique default outputPath', () => {
expect(makeEntry().outputPath).not.toBe(makeEntry().outputPath);
});中文说明
本 PR 要保证的默认 outputPath 唯一性,目前没有任何断言锁定。— 故障场景:实测将 makeEntry 还原为 PR 前的常量 '/tmp/s1.output' 后,现有 57 个测试全部通过——只有下面建议的守卫测试会失败。在单 uid 机器或全新 CI 容器上,这种回归完全无感;而在多租户共享 runner 上,本 PR 修复的冲突 flake 会再次出现,且仓库里没有任何守卫能指向成因。
— qwen3.8-max via Qwen Code /review (v0.21.8)
There was a problem hiding this comment.
Fair hit — this was the gap in my verification. I A/B tested the cost (128.9s → 3.2s with the path made immutable) but never checked that reverting the change fails anything, and you are right that it did not.
Guard added in cc588b2. Mutation-checked: restoring outputPath: "/tmp/s1.output" now fails four tests (this guard plus three in the sidecar group, which picks up the default after the makeDirEntry dedupe), where before it failed none.
…iqueness Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/100 轮）。改动内容与我反驳保留之处如下： Review feedback addressed (round 2)All three inline suggestions from the automated reviewer were implemented in one commit ( Feedback points
Nothing was declined, deferred, or escalated this round. VerificationCommands actually run, in order:
Not run, with reasons:
中文说明已处理的评审反馈(第 2 轮)自动化评审的三条行内建议已在一次提交中全部实现( 反馈要点
本轮没有拒绝、推迟或上报给维护者的事项。 验证实际执行的命令(按顺序):
未执行的命令及原因:
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
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.
|
@qwen-code /triage |
|
Sandboxed verification: The verification run did not complete, so the phases below may be partial or missing entirely. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 47 passed · 0 failed · 47 total 中文 — 判定：
|
|
🤖 AutoFix updated a stale base — the fix did not pass verification, but this PR was behind What I found before stopping: See the Qwen Autofix agent step logs for model/tool output. Run log: https://github.com/QwenLM/qwen-code/actions/runs/31323352713 🧠 Handled by Qwen Code · model/模型 |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed the current head e3268f9d01 across the complete one-file diff, existing review threads, sidecar lifecycle and cleanup, and CI state. The isolation fix itself works: the focused file passes 58/58 and a pre-existing /tmp/s1.status is untouched. I found one reproducible test-portability regression in the new dynamic XML expectation; details are inline. I am leaving this as COMMENT rather than APPROVE until it is addressed.
| // Whole element: pins that only the control byte is stripped and the | ||
| // rest of the path survives intact. | ||
| expect(modelText).toContain( | ||
| `<output-file>${join(dir, 'out.log')}</output-file>`, |
There was a problem hiding this comment.
[Suggestion] Escape the complete dynamic path in these XML expectations. dir comes from tmpdir(), but this assertion interpolates it verbatim. On the current head, setting a legal TMPDIR=/.../tmp&xml makes strips control characters from cwd and output-file XML fields fail because the actual XML correctly contains &; & is also valid in Windows temp paths. The sibling expectation above only replaces & and fails for a legal POSIX temp path containing <. Before this PR, both focused cases used fixed /tmp/... paths, so they were not environment-dependent. Please apply complete XML escaping to the dynamic expected path in both assertions, or use a small expected-value helper that handles all five XML metacharacters.
There was a problem hiding this comment.
Fixed in 5549db2 — you are right, and this was self-inflicted: those anchored assertions are mine from 9158922, and hand-rolling the escaping is exactly what made them environment-dependent once they moved off the fixed /tmp path.
Rather than escape the five metacharacters by hand, the expected value now goes through the same pipeline the registry uses to produce the element:
function expectedOutputFileElement(path: string): string {
return `<output-file>${escapeXml(stripDisplayControlChars(path))}</output-file>`;
}Both assertions use it, so they cannot drift from backgroundShellRegistry.ts:548.
A/B with TMPDIR=/tmp/qwen-xml-probe/a&b<c — covering both metacharacters you named:
| version | result |
|---|---|
| helper (this commit) | 3 focused cases pass |
hand-rolled replaceAll("&", "&") |
2 fail |
Full file 58/58, typecheck and lint clean.
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.
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.
|
🤖 Addressed the latest review feedback (round 3/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 3/100 轮）。改动内容与我反驳保留之处如下： Autofix round summary — PR #8813Head entering this round: Feedback dispositions1.
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Not explored to full depth (tool budget reached): PR #8813 (QwenLM/qwen-code, test-only change): stops ever...: I couldn't run the test file to measure actual wall-clock (no node_modules in worktree; monorepo-wide npm install was too heavy for the review budget). Per brie…; PR #8813 (QwenLM/qwen-code, test-only change): stops ever...: empirical test run of backgroundShellRegistry.test.ts to measure wall-clock under the default 15s timeout — the worktree has no node_modules and a monorepo …. Not reviewed: reverse audit — stopped before round 4 by the review time budget. Test Plan (not a blocker): 57 tests pass — this review observed 1172, 18743, 1494, 481 passed.
中文说明
已审查。 未探索到全部深度（达到工具调用预算）：PR #8813 (QwenLM/qwen-code, test-only change): stops ever...：I couldn't run the test file to measure actual wall-clock (no node_modules in worktree; monorepo-wide npm install was too heavy for the review budget). Per brie…；PR #8813 (QwenLM/qwen-code, test-only change): stops ever...：empirical test run of backgroundShellRegistry.test.ts to measure wall-clock under the default 15s timeout — the worktree has no node_modules and a monorepo …。 未审查：反向审计——评审时间预算不足，未能开始第 4 轮。 Test Plan（非阻断）：57 tests pass — this review observed 1172, 18743, 1494, 481 passed。
— qwen3.8-max via Qwen Code /review (v0.21.8)
|
🤖 AutoFix updated a stale base — the fix did not pass verification, but this PR was behind What I found before stopping: See the Qwen Autofix agent step logs for model/tool output. Run log: https://github.com/QwenLM/qwen-code/actions/runs/31336927367 🧠 Handled by Qwen Code · model/模型 |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix round: no action requiredThis round's review feedback for PR #8813 contains no actionable findings:
To close the one evidence gap the reviewer flagged, this round ran the focused test file locally:
No code changes were made; no commit was created. 中文说明Autofix 轮次：无需处理本轮针对 PR #8813 的评审反馈中没有可执行的发现：
为补齐评审器标注的唯一证据缺口，本轮在本地运行了目标测试文件：
未修改任何代码；未创建任何提交。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
…assertions (#8813) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
🤖 Addressed the latest review feedback (round 5/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 5/100 轮）。改动内容与我反驳保留之处如下： Review feedback addressed — round summaryFeedback points and dispositions
Changes
Conflict notes
Probe evidence
Verification
中文说明已处理的评审反馈 —— 本轮总结反馈点及处理结果
变更内容
冲突说明
探针证据
验证
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM. The root-cause analysis holds (shared /tmp/s1.status + sticky-bit EPERM + 350ms blocking backoff per swallowed write), and the fix is structurally sound: mkdtempSync per entry keeps worker isolation and makes the collision impossible, overrides.outputPath short-circuits before allocating, and the two special tests keep their actual subjects (& and \x03 in the basename) while asserting the whole element through the production escapeXml ∘ stripDisplayControlChars pipeline — strictly stronger than the old hard-coded contains, and TMPDIR-independent. I verified the -103 lines weaken nothing: retention-cap assertions are byte-identical, the removals are only the #8797 120s band-aid plus one-line reflow, test count goes 54→55 (the new uniqueness pin, zero dropped), and no hard-coded /tmp outputPath survives. All nine review comments are resolved with cited commits. CI green on this head.
One coordination flag for the maintainer: this clashes with the still-open #8795, which fixes the same flake with an incompatible fixture strategy — the same hunks conflict in either merge order. #8813 is the better fixture fix (it also removes the merged timeout band-aid and adds cleanup/uniqueness pins); #8795's unique value is the production bidi hardening and the xml.test.ts double-& guard. Suggested sequencing: merge this first, then rebase #8795 down to its bidi-only delta.
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed the exact head 66c20d806a across the complete one-file diff, existing review threads/comments, the sidecar lifecycle and cleanup paths, sanitizer semantics, test portability, and current CI. The per-entry temp-directory isolation and assertion-independent cleanup are sound. I found one reproducible portability mismatch in the new expected-value helper; details are inline. The required Ubuntu and desktop checks are green; the automated review check is still running.
| * escaping here would make these cases depend on the host's TMPDIR. | ||
| */ | ||
| function expectedOutputFileElement(path: string): string { | ||
| return `<output-file>${escapeXml(stripDisplayControlChars(path))}</output-file>`; |
There was a problem hiding this comment.
[Suggestion] Keep this expected sanitizer aligned with the registry implementation. The imported terminalSafe.ts helper is not the helper used by backgroundShellRegistry.ts: this one also strips U+202A–U+202E and U+2066–U+2069, while the registry's private same-named function only strips C0/C1 characters. A legal POSIX TMPDIR containing one of those Unicode characters therefore makes the expected path lose a character while the emitted XML retains it. I reproduced this at the current head with TMPDIR=.../tmp-\u202e-marker; both emits one task-notification when a shell completes and the control-character notification test fail. Since this helper was added specifically to avoid host-dependent expectations, please either calculate the expected value with the registry's actual semantics or make the registry deliberately reuse this shared sanitizer (and cover that behavior).
#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.
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - 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: 38 passed · 0 failed · 38 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：38 通过 · 0 失败 · 38 总计 Verification report<!-- qwen-triage:verify --> Sandboxed verification: ✅ passed — merge-ready (agent verdict) - follow-up round at head 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: 38 passed · 0 failed · 38 total 中文 — 判定：✅ 通过 · 可合入（agent 判定，follow-up 轮）
Verification reportPR 8813 verification (follow-up round) — fix(test): stop background-shell tests sharing a fixed /tmp sidecar pathVerdict: Previous-finding status
Central claim + A/BCentral claim: giving each test entry its own temp directory removes the shared Fault model: this container runs as uid 1000 with no root/sudo, so a foreign-owned
Witness: Path isolation: base clean run creates Symmetric control: faulting all CI-leg failure shape reproduced: under the fault, base runs all 57 tests green yet vitest exits 1 with Band-aid removal is load-bearing: with the 120s Delta since the previous round (commit
|
| variant | tests | residue |
|---|---|---|
| head, green run | 58/58 | 0 |
| head, forced assertion failure in the dir-outputPath test | 57/58 (1 forced) | 0 — cleanup survives failing assertions |
delete the tmpFiles.push(statusFilePathFor(dir)) line |
58/58 green | 1 leaked .status |
| move the push behind the failing assertion | 57/58 (1 forced) | 1 leaked .status |
Both axes of the commit (presence of the registration, and its placement before the assertions) are load-bearing. Note the asymmetry in the third row: the suite stays fully green while leaking — see Finding 4.
Findings (non-blocking)
- Carried (stands): sibling fixed-
/tmpfixtures remain — see status table row 1. Repro unchanged:cd packages/core && rm -rf /tmp/bg-out && npx vitest run src/tools/task-stop.test.ts --coverage.enabled=false && ls /tmp/bg-out(absent). Worth a follow-up sweep, as the PR itself notes. - Carried (correction to the description): test count is 58, not 57. The new head still has 58 tests; the description's "57 tests pass" is off by the uniqueness test the PR itself adds.
- Carried (stands): the 15s timeout error lands after the ~24s synchronous backoff loop, because vitest's timer cannot fire mid-sync. The removal still converts silent cost into a guaranteed red; "fails loudly" holds, "fails at 15s" should read "fails with the 15s-timeout error after the sync loop completes".
- New (completeness, non-blocking): the afterEach sidecar cleanup is not pinned by any assertion in the file. With the cleanup registration deleted, the suite runs 58/58 green while leaking a
.statusfile per run (third delta row); only an external residue scan detects it. Classification: coverage gap, not defect — the behavior is correct at head, and cleanup hygiene is exactly what the commit's comment explains, but a future regression of the cleanup would be silent in CI. A maintainer may want a residue assertion someday; not a merge condition.
Mutation matrix (scratch copies of the HEAD tree; re-runnable via harness/apply-mutation.mjs)
| guard added by PR | mutation | result | verdict |
|---|---|---|---|
per-entry unique outputPath |
restore fixed /tmp/s1.output |
1/1 FAILED (expected '/tmp/s1.output' not to be '/tmp/s1.output') |
pinned |
expectedOutputFileElement (×3) |
remove escapeXml from registry <output-file> rendering + hostile TMPDIR=a&b<c |
3 FAILED — exactly the three assertions using the helper | pinned |
| shipped helper | none + hostile TMPDIR=a&b<c |
58/58 passed | OK (TMPDIR-proof) |
| afterEach sidecar cleanup (new commit) | delete registration line | 58/58 green, residue 1 vs 0 | pinned (external scan) |
| cleanup before assertions (new commit) | push behind failing assertion | residue 1 vs 0 | pinned (external scan) |
SIDECAR_IO_TIMEOUT removal |
EPERM on all *.status renames |
4/4 FAILED (Test timed out in 15000ms.) |
loud |
Positive control: unmutated head, no fault = 58/58 passed. No mutant regressed from killed to survived relative to the previous round's matrix. Witness: evidence/03-mutation-matrix.png.
Not covered
- Per-commit attribution: the depth-2 checkout reaches 1 of the 8 snapshot commits (
git rev-list HEAD^1..HEAD^2= 1; previous head6fa044adunreachable). Verified the aggregateHEAD^1..HEADdiff, which touches onlybackgroundShellRegistry.test.ts; the delta since the previous round was identified from the commit headline plus the aggregate diff and behavior probes, not from a direct diff. - True foreign-uid sticky-bit repro: container is uid 1000 without sudo/chattr, so the reviewer-plan's
sudo -u nobody touch/chflags uchgwere not runnable. The LD_PRELOAD fault reproduces the EPERM→retry→backoff path in real production code (the shape), not the kernel's sticky-bit check (the cause). - Windows/macOS: not run here; the mechanism is platform-independent JS, and the PR tested macOS locally.
- Other suites: only the changed file (plus the informational
task-stopsibling probe) was run; the PR touches no production code.tsc --noEmitand ESLint onpackages/core/the changed file pass (ESLint liveness proven by a planted unused variable). - Runner
/tmpsweep: declared out of scope by the PR (Finding 1).
Methodology
Environment: node:22-bookworm container (uid 1000, sticky /tmp, no root). Base control = git worktree at HEAD^1; mutations in a scratch worktree at HEAD. Root node_modules reused (the PR leaves package.json/lockfile untouched); a strict grep shows the test's import closure contains no @qwen-code/* workspace imports, so no head code can leak into the base control. The fault injector (harness/eperm-inject.c, LD_PRELOAD) fails rename* with EPERM for the targeted path only; interception was proven against the real dist/ module before use. Suite cells ran via vitest's JSON reporter with coverage disabled on both arms; raw JSON in cells/, logs in logs/, captures via scripts/verify-capture.mjs in evidence/. All 38 assertions are recomputed from those artifacts (plus live residue scans of the probe TMPDIRs) by harness/tally-assertions.mjs; expected-failure cells (base broken, mutants killed, leaks present) are encoded as passing assertions, so fail: 0 means zero unexpected outcomes.
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
|
🤖 Addressed the latest review feedback (round 6/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 6/100 轮）。改动内容与我反驳保留之处如下： Review feedback round — PR #8813Feedback point
Decision: addressed in code (root cause)Verified against the exact head first: with Of the two resolutions the reviewer offered, this takes "make the registry deliberately reuse this shared sanitizer":
Changes
ConflictNone ( Verification
Commit: 中文说明评审反馈轮次 — PR #8813反馈点
决定：已在代码中处理（根因修复）先在当时的 head 上验证：将 在评审者给出的两种解决方案中，本次采用"让 registry 有意地复用这个共享 sanitizer"：
改动
冲突无（ 验证
提交： Deferred non-Critical feedbackCritical-only mode is active after 5 change-producing rounds. The workflow excluded the non-Critical feedback below from this round's actionable sections; the items remain open for human follow-up. Maintainer feedback is deferred only after its author has used 2 regular feedback batches in this window's Critical-only tail; authors at that budget, if any, are named below. ( 中文说明完成 5 个产生改动的轮次后进入仅处理 Critical 的模式。本轮可执行区域已排除下方非 Critical 反馈；这些条目保持开放，留待人工跟进。维护者反馈仅在其本人于本窗口 Critical-only 阶段已使用 2 批常规反馈预算后才会延后；达到预算的作者（如有）在下方点名。（评论 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
|
@qwen-code /takeover stop |
|
@qwen-code /triage |
qwen-code-dev-bot
removed
the
autofix/takeover
|
👋 Takeover released: the autofix loop will no longer engage this PR (an in-flight round, if any, completes its bounded work). Re-apply 中文说明👋 已释放：autofix 循环不再介入此 PR（在飞的一轮如有，将完成其有界工作）。重新打上 |
|
Thanks for the PR! Template looks good ✓ Problem: observed, and well evidenced. 18 of the last 100 Direction: aligned — this replaces the #8797 timeout band-aid with the root cause, and removing a CI flake that fails legs with zero test failures is squarely in scope. Size: 22 production lines (a helper de-duplication in Approach: the scope is right — per-entry unique temp dir as the default Risk: no elevated risk signals (no high-risk paths matched). Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：已观测到，且证据充分。最近 100 次 方向：对齐——用根因修复取代 #8797 的超时绷带；消除这种"0 个测试失败却判整条腿失败"的 CI flake 完全在范围内。 规模：22 行生产代码（ 方案：范围合理——每个条目默认使用各自独立的临时目录作为 风险：无升级风险信号（未命中高风险路径）。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewBefore reading the diff, my independent proposal for "tests share a fixed What I verified against the code:
No blockers, no convention issues. Test evidenceUnattended run — I did not build or execute PR code; the evidence below is the PR's own CI plus static verification of the claimed mechanism. Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 The in-progress leg is the one this PR targets — the only leg that has been failing. Note what a green result proves here: because this PR removes the 120s Nothing user-visible to drive in a terminal, so no live-product capture applies. 中文说明代码审查在读 diff 之前，我对"测试共享固定 对照代码核实的内容：
无阻塞项，无规范问题。 测试证据无人值守运行——未构建或执行 PR 代码；以下证据为 PR 自身 CI 加上对所声称机制的静态核实。CI 表格见上方区域：目标腿 进行中的这条腿正是本 PR 针对的腿——也是唯一一直在失败的腿。注意绿色结果在这里证明了什么：由于本 PR 移除了 120s 的 没有用户可见的行为需要在终端中驱动，因此不适用实机截取。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — clean, minimal root-cause fix with an airtight reproduction; the only nit is the body calling it "test-only" while the head carries a 22-line production helper de-duplication. Stepping back: the diagnosis is the strong part of this PR — the 351ms quantization arithmetic identifies the retry backoff before any code is read, and I verified that arithmetic against The verdict is approve, but the ubuntu Node 22 leg — the one this PR is about — is still running on the reviewed commit, so approval is deferred until CI lands green on 中文说明置信度：4/5 —— 干净、最小化的根因修复，复现无懈可击；唯一的瑕疵是正文称其为 "test-only"，而 head 上实际包含 22 行生产代码的助手函数去重。 退一步看：这个 PR 最强的部分是诊断——351ms 量化算术在读任何代码之前就指认了重试退避，而我已在 结论是通过，但 ubuntu Node 22 腿——正是本 PR 针对的那条——仍在所审提交上运行，因此在 CI 于该提交变绿之前暂缓批准。 — Qwen Code · qwen3.8-max Reviewed at |
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Re-approving at head cb794cd after my approval was dismissed by the new push. The added commit folds in exactly the shared-sanitizer delta: the registry's private stripDisplayControlChars copy is removed in favor of the shared terminalSafe helper, which extends stripping to U+202A–202E and U+2066–2069 on the notification display paths, and the control-strip test now plants an RLO in the output path and asserts it is removed through the real escapeXml ∘ stripDisplayControlChars pipeline. Everything verified in the previous round still holds (per-entry temp dirs removing the sticky-bit sidecar collision, zero weakened assertions, band-aid removal). With this delta absorbed, this PR now delivers both the fixture root-cause fix and the bidi hardening. CI green on this head. Nothing blocks merge.
|
Sandboxed verification: Skipped because the PR is not open for verification (state=MERGED, draft=false). 中文 — 判定：
|
…ar deflake #8813 landed the same shared /tmp/s1.status deflake this branch started, plus the shared display sanitizer reuse. Keep main's per-entry fixture mechanism and expectedOutputFileElement helper; preserve this branch's unique coverage on top: bidi stripping in the output tail (all nine codepoints), bidi pins at the command/cwd/output-file render sites, and the <result> render-site pin in the failure test.
|
Released in v0.21.9. |
5 participants
What this PR does
backgroundShellRegistry.test.tsbuilt every entry with a hard-codedoutputPath: '/tmp/s1.output'. The registry mirrors each entry into a<outputPath>.statussidecar, so every test in the file — across tests, across vitest workers, across CI jobs sharing a host — wrote to the single path/tmp/s1.status. This PR gives each entry its own temp directory.Why it's needed
/tmpcarries the sticky bit. Once/tmp/s1.statusbelongs to another uid, the atomic rename insideatomicWriteFileSyncfailsEPERM, andrenameWithRetrySyncburns its full backoff before giving up:50 + 100 + 200 = 350msper write, after whichwriteStatusFileswallows the error and logs a debug warning. The tests never notice — they just get slower, and the sidecar never lands.The retention-cap cases do 68 register/complete calls each, so they paid ~24s apiece. This was the single largest source of CI failures: of the last 100
Qwen Code CIruns, 18 failed and all 18 were theTest (ubuntu-latest, Node 22.x)leg; 8 of those were this file's assertions timing out, and 2 more were[vitest-worker]: Timeout calling "onTaskUpdate"— the worker's task-update RPC starving while a single file sat in blocking sleeps, which fails the leg with 0 test failures reported.The sticky-bit collision is also plain shared state: unrelated tests in this file all write to the same path, so a stale
/tmp/s1.statusfrom any earlier job on the host keeps every later job slow until the machine's/tmpis cleared.Reviewer Test Plan
How to verify
The condition needs a
/tmp/s1.statusthat cannot be replaced. On macOS the immutable flag reproduces it without another uid:On Linux,
sudo -u nobody touch /tmp/s1.statushas the same effect via the sticky bit.Run that against
mainand against this branch. Also confirm the file no longer writes there at all:rm -f /tmp/s1.status, run the suite, then check the path does not exist.Evidence (Before & After)
Measured locally with
/tmp/s1.statusmade immutable, same machine, same command:mainThis reproduces the CI shape exactly — CI measured 23850 / 23488 / 23141 / 23135 ms for the same four cases.
The arithmetic identifies the cause on its own. Every case duration in that CI run is an exact multiple of 351ms with no variance:
Disk latency does not quantize like that; a fixed retry backoff does. Confirmed directly by injecting a rename that always throws
EPERMthrough the_testFsseam — one failingatomicWriteFileSynccosts 380ms, a succeeding one is sub-millisecond.After the change,
/tmp/s1.statusis never created: 57 tests pass and the path does not exist afterwards.Tested on
Environment (optional)
macOS, local Node.js workspace.
backgroundShellRegistry.test.ts(57 tests) passes, plus lint and typecheck onpackages/core.Risk & Scope
makeEntrynow creates a temp directory per entry, so the retention-cap loops create ~34 directories each. They are registered with the existingafterEachcleanup and the whole file still runs in ~3s. Callers that pass an explicitoutputPath(thestatus sidecar filegroup) are unaffected — the override still wins.SIDECAR_IO_TIMEOUT = 120_000override fix(test): deflake three CI-load-sensitive tests #8797 added to these four cases. That was a band-aid for the cost; with the cost gone, keeping it would let a future regression silently take two minutes instead of failing./tmp/s1.statusis unwritable in the first place. Something on the self-hosted runners left that file behind under a different uid, and it will keep affecting anything else writing fixed/tmppaths until it is cleared. Worth a sweep for other hard-coded/tmpfixtures, and worth clearing on the runners.Linked Issues
Follows #8797, which raised the timeout on these same four cases.
中文说明
本 PR 的改动
backgroundShellRegistry.test.ts的makeEntry把outputPath写死成'/tmp/s1.output'。registry 会把每个条目镜像到<outputPath>.statussidecar,因此该文件里所有测试——跨测试、跨 vitest worker、跨同一台主机上的 CI job——都写向同一个路径/tmp/s1.status。本 PR 改为每个条目使用各自的临时目录。为什么需要
/tmp带 sticky bit。一旦/tmp/s1.status属于另一个 uid,atomicWriteFileSync里的原子 rename 就会EPERM,而renameWithRetrySync会把退避跑满才放弃:每次写
50 + 100 + 200 = 350ms,之后writeStatusFile吞掉异常并打一条 debug 警告。测试完全无感——只是变慢,而且 sidecar 从未写成功。retention-cap 用例每个要做 68 次 register/complete,因此各自耗时约 24 秒。这是 CI 失败的最大单一来源:最近 100 次
Qwen Code CI有 18 次失败,全部 18 次都挂在Test (ubuntu-latest, Node 22.x)这条腿;其中 8 次是这个文件的断言超时,另有 2 次是[vitest-worker]: Timeout calling "onTaskUpdate"——单个文件长时间处于阻塞睡眠,worker 的任务上报 RPC 被饿死,于是在 0 个测试失败的情况下判整条腿失败。sticky bit 冲突本身也是明确的共享状态:该文件里互不相关的测试都写同一个路径,主机上任何早先 job 残留的
/tmp/s1.status都会让之后每个 job 一直慢下去,直到该机器的/tmp被清理。审查者测试计划
验证方法
复现条件需要一个无法被替换的
/tmp/s1.status。在 macOS 上用 immutable 标志即可,无需另一个 uid:在 Linux 上,
sudo -u nobody touch /tmp/s1.status通过 sticky bit 达到同样效果。分别在
main和本分支上运行。另外确认该文件不再写入那个路径:rm -f /tmp/s1.status,运行测试套件,然后检查该路径不存在。证据(修复前与修复后)
在本地把
/tmp/s1.status设为 immutable 后测量,同一台机器、同一条命令:main这与 CI 的形状完全一致——CI 上同样这四个用例测得 23850 / 23488 / 23141 / 23135 ms。
仅凭算术就能定位成因。那次 CI 中每个用例的耗时都是 351ms 的精确整数倍,零方差:
磁盘延迟不会这样量化,固定的重试退避才会。通过
_testFs接缝注入一个总是抛EPERM的 rename 直接确认:一次失败的atomicWriteFileSync耗时 380ms,成功的则在亚毫秒级。改动之后,
/tmp/s1.status不再被创建:57 个测试通过,且该路径在运行后不存在。测试平台
环境(可选)
macOS、本地 Node.js workspace。
backgroundShellRegistry.test.ts(57 个测试)通过,packages/core的 lint 与 typecheck 亦通过。风险与范围
makeEntry现在为每个条目创建一个临时目录,因此 retention-cap 循环每个会创建约 34 个目录。它们都登记在已有的afterEach清理中,整个文件仍然只跑约 3 秒。显式传入outputPath的调用方(status sidecar file那一组)不受影响——覆盖值仍然优先。SIDECAR_IO_TIMEOUT = 120_000。那是针对该开销的止痛药;开销消除后继续保留,只会让将来的回归悄悄跑上两分钟而不是直接失败。/tmp/s1.status一开始为什么不可写。 self-hosted runner 上有什么东西以另一个 uid 留下了那个文件,在它被清理之前,会持续影响其他写固定/tmp路径的代码。值得排查是否还有别的硬编码/tmpfixture,也值得在 runner 上清掉该文件。关联 Issue
承接 #8797,该 PR 为同样这四个用例提高了超时。