fix(cli): stop housekeeping hanging on a deleted bind mount - #11285
Conversation
`runThrottledOnce` ensured its lock directory with
`mkdir(..., { recursive: true })`. When the global qwen dir sits inside a
bind mount whose source directory has been deleted, the mountpoint still
stats as a directory while creating entries inside it returns ENOENT.
Node's recursive mkdir reads that ENOENT as "parent is missing", creates
the parent (EEXIST), confirms it is a directory, retries the leaf, gets
ENOENT again — with no retry cap. The returned promise never settles, so
the existing `.catch` never runs and the `await` wedges the housekeeping
chain for the rest of the process lifetime.
CI hit this on the self-hosted pool: e2e sandbox containers that outlived
their host-side `docker run` client kept running after the job had
already deleted their workspace, and each one then burned a core issuing
~2.5k failing mkdir/s. Measured on one host: 177 such processes, 45 CPU
hours accumulated by the oldest.
A single-level mkdir is enough — all three callers put `lockPath`
directly in the global qwen dir, whose own parent is $HOME — and it fails
fast with ENOENT instead of spinning, so housekeeping degrades to a skip.
|
Thanks for the PR! Template looks good ✓ — every section filled in, including a real before/after and the platform table. Problem: observed, not theoretical, and about as well-evidenced as a fix gets. There is a production incident with measurements (177 wedged processes, ~2.5k failing Direction: aligned. The real defect is that Size: not applicable — Approach: the scope feels right and I'd have proposed the same thing. I weighed the two alternatives before reading the diff — wrapping the One caveat to flag now and detail in the code review: the description justifies one level with "every caller puts Risk: no elevated risk signals. Neither changed file matches the revert-correlated path list. Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ —— 每个部分都填写了,包括真实的 before/after 和平台表格。 问题: 是已观测到的问题,不是理论推演,而且证据充分得少见。有带实测数据的生产事故(177 个卡死进程,每个每秒约 2500 次失败的 方向: 对齐。真正的缺陷在于 规模: 不适用 —— 方案: 范围合理,我自己也会提出同样的做法。在看 diff 之前我权衡过另外两条路——给 有一点现在先提出、代码审查里再展开:描述用「每个调用方都把 风险: 无升级风险信号。两个改动文件都不匹配与 revert 相关的路径列表。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Code reviewI traced the change against every consumer rather than taking the description's word for it, since dropping The load-bearing claim holds.
The two hashed marker paths were the ones worth checking — both hash the key into a filename, not into extra directory levels, so neither reaches below The failure path degrades instead of wedging, on both schedulers. This is what makes the change safe rather than just smaller. If the One non-blocking note — the "parent is The comment block earns its place. Ten lines of comment on a one-line change would normally be too much, but the why here is genuinely non-obvious and load-bearing: without it the next contributor re-adds Tests are real guards, not decoration. The first new test uses a fresh directory that genuinely doesn't exist, so it pins that dropping Neither test pins the incident itself — a never-settling promise on a deleted bind mount needs root and a mount namespace, which is reasonably out of scope for a unit test. That gap is what the verification line below is for. No reuse concerns: nothing new is being abstracted, so there's no parallel utility to consolidate. Test evidenceUpdate: CI settled after this section was first written, and it settled with the unit suite dead on arrival. The original version of this comment reported the Linux unit job as in progress; the table and the analysis below are the settled state for commit Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。 Both red checks are runner-side checkout failures, not this PR. They are the same class of failure, and neither reached any of the PR's code:
A two-file change under But "infra noise" is not the same as "evidence exists", and that distinction decides the verdict here. The one job that would have executed this PR's two new tests died before running a single one, and the macOS and Windows Not verified, and stated plainly:
Sandboxed verification would settle the one claim that currently rests on the author's word: 中文说明代码审查我是顺着每一个调用方去核对这个改动的,而不是直接采信描述里的说法——因为去掉 关键论断成立。 对全部三个调用点来说, 失败路径是降级而不是卡死,两个 scheduler 都是如此。 这才是让这个改动「安全」而不只是「更小」的原因。如果 一条不阻塞的提醒——「父目录是 这段注释对得起它占的篇幅。 一行改动配十行注释通常是过头了,但这里的 why 确实不明显、而且是承重的:没有它,下一个贡献者会把 测试是真护栏,不是摆设。 第一个新测试用的是一个确实不存在的目录,所以它钉住了「去掉 两个测试都没有钉住事故本身——「在源目录被删除的 bind mount 上 promise 永不 settle」需要 root 和 mount namespace,这对单元测试来说合理地属于范围之外。这个缺口正是下面那条验证建议要覆盖的。 复用方面没有问题:这里没有新增任何抽象,所以不存在需要合并的平行工具函数。 测试证据更新:这一节首次写下之后 CI 已经落定,而且落定的结果是单元测试套件根本没跑起来。 本评论最初版本把 Linux 单元测试 job 报为进行中;下面的表格和分析是针对 commit 两个红叉都是 runner 侧的 checkout 失败,与本 PR 无关。 它们是同一类故障,而且都没有接触到 PR 的任何代码:
一个只改了 但「基础设施噪音」不等于「证据已经存在」,而这个区别决定了此处的结论。 唯一会执行本 PR 两个新测试的 job 在跑任何一个测试之前就死了,而本 PR 的 macOS 与 Windows 明确说明未验证的部分:
沙箱化验证可以搞定目前唯一还只依赖作者自述的论断: — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Confidence: 3/5 — I'm confident in the code and would merge it as written; what I can't do is approve against a commit where the only job that would have run this PR's two new tests died at Stepping back over the whole picture: this is what a well-formed small fix looks like. I wrote down my own proposal from the title and the "Why it's needed" section before reading the diff, and the PR landed on exactly it — drop What I'd have wanted to check going in was whether "one level is enough" was actually true, because that's the single assumption the fix rests on, and it's the kind of claim that reads plausible in a PR description and turns out to have a fourth call site nobody remembered. It holds. All three call sites put On the problem itself: this is about as far from theoretical hardening as a So why not approve. Two reasons, and they're different in kind:
⏸️ Deferring, not requesting changes. What this needs is a re-run of the No approval marker is attached to this comment: the PR's own CI workflow runs have all completed ( I'm also posting this without an @mention, which I'd normally avoid on a defer. The maintainer resolver found nobody accountable to hand this to: the PR carries no labels, so no area owner matched, there's no prior human reviewer to fall back to, and no maintainer handle is configured in this run. Flagging it here rather than guessing a login. Whoever picks it up: the ask is one re-run, not a review. One optional, non-blocking follow-up for the author, already detailed above: the Risk section frames "a 中文说明信心度:3/5 —— 我对代码本身有信心,按现在的样子我就会合并;我做不到的是,在一个「唯一会执行本 PR 两个新测试的 job 死在 退一步看整体:这是一个形态良好的小 fix 该有的样子。在读 diff 之前,我先根据标题和「为什么需要」那一节写下了自己的方案,而 PR 落点与之完全一致——去掉 我一开始最想核对的是「只创建一层就够了」是否真的成立,因为这是整个 fix 所依赖的唯一假设,也正是那种在 PR 描述里读起来很合理、结果却存在第四个没人记得的调用点类型的论断。它成立。三个调用点都把 关于问题本身:这大概是 那么为什么不 approve。两个理由,性质不同:
⏸️ 暂缓(defer),不是要求修改(request changes)。 这里需要的是在同一个 commit 上重跑 本评论不附带 approve 标记:本 PR 自己的 CI workflow run 已全部完成( 我这条 defer 也没有加 @提及,这通常是我会避免的。维护者解析器没有找到可以交接的负责人:该 PR 没有任何标签,所以没有匹配到领域 owner;没有在此之前的真人 reviewer 可供回落;本次运行也没有配置维护者 handle。与其猜一个登录名,不如在这里如实说明。接手的人请注意:需要做的是一次重跑,不是一次 review。 另有一条给作者的、可选且不阻塞的后续项,上文已详述:Risk 一节把「父目录不存在的 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
@qwen-code /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. This is a fork PR, so the first round comes from the next scheduled scan (usually within minutes). Remove the 中文说明🤝 已接管:autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突,直到移除标签或达到轮次上限。本 PR 来自 fork,首轮处理将由下一次定时扫描执行(通常几分钟内)。移除 |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally.
Test Plan (not a blocker): 13 passed — this review observed 28911 passed; 80 passed — this review observed 28911 passed.
中文说明
仅完成部分审查,审查缺口已披露。
未审查(原文为英文):build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally.
Test Plan(非阻断):13 passed — this review observed 28911 passed; 80 passed — this review observed 28911 passed。
— qwen3.8-max via Qwen Code /review (v0.23.0)
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
Test Plan (not a blocker): 13 passed — this review observed 28910 passed; 80 passed — this review observed 28910 passed.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/cli/src/utils/housekeeping/throttledOnce.ts:62 — [probe] comment's '$HOME' premise false under QWEN_HOMEpackages/cli/src/utils/housekeeping/throttledOnce.ts:63 — [probe] swallowed mkdir error is never logged
中文说明
仅完成部分审查,审查缺口已披露。
未审查(原文为英文):build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally.
未审查:反向审计——评审时间预算不足,未能开始第 3 轮。
Test Plan(非阻断):13 passed — this review observed 28910 passed; 80 passed — this review observed 28910 passed。
收敛姿态下延后(第 2 轮,非阻断)——已记录,本轮不要求修改:共 2 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.23.0)
Local runtime verificationI built both arms and ran this against a real deleted bind mount instead of reading the diff. Verdict: merge as it stands — every claim in the description reproduces, the two unit tests CI never got to run pass on Linux and have teeth, the Harness
1. The mechanism reproduces exactly as described
2. The real CLI in the production sequence — the decisive A/BThe incident shape is a process that started healthy and had the mount deleted underneath it, so that is what I reproduced: launch the real TUI with Both builds in one wedged mount, sampled with
Only the (pre-PR, deleted) cell burns CPU; the fix costs nothing in the healthy cell, and the TUI is identical in both arms afterwards — no error banner, no crash. I re-ran the whole 2×2 a second time and every cell reproduced to within 0.03 s. At syscall level over the same 50 s window: 105,390 failing 3. The two unit tests, run and mutation-tested
Mutation results — 3 of 3 targeted mutants caught, working tree restored clean after each:
The 4. Windows — the
|
| case | pre-PR | this PR |
|---|---|---|
| parent exists, lock dir missing | completed, dir created | completed, dir created |
| whole parent chain missing | completed, tree bootstrapped | THREW ENOENT, nothing created |
lock dir already exists (EEXIST) |
completed | completed |
win32 and linux produced identical results on both arms, marker written and no lock file leaked in every passing case. Your platform-neutrality reasoning holds up under measurement.
5. Your open question — a concrete answer
You wrote that you could not explain why the same never-settling call shows up as a hot mkdir loop in some processes and a quiet hang with no mkdir syscalls at all in others (promise pending, 0 % CPU, idle libuv workers). The CLI re-execs itself, so the pid you first reach for is not the pid running the code. One single pre-PR run in the wedged mount, observed from both vantage points at once:
launcher pid 3311163 (what ps/top/pgrep show first) 0.00s CPU / 100s 0 mkdir syscalls
re-exec'd worker 3311177 (where housekeeping runs) 46.82s CPU / 100s 26,759 mkdir in a 3s sample
I hit this myself: my first three runs reported 0.00 s CPU and zero mkdir syscalls and I was about to write up a "quiet variant" until I followed the child. That reproduces your description exactly. (A second, independent mechanism also produces it: saturate all four libuv threadpool threads and the recursive mkdir never gets a thread — verified pending at 10 s, 0 syscalls, 0 % CPU. Neither is a blocker; both disappear with this PR.)
6. On the review note about QWEN_HOME
The reachability concern raised in review does not hold in practice, and it is worth recording why rather than rewording the risk note on faith. writeOutputLanguageFile (packages/cli/src/i18n/languageUtils.ts:238) does fs.mkdirSync(dir, { recursive: true }) on the global qwen dir straight from main(), before the UI renders. Measured: QWEN_HOME=/deep/a/b/c/qwen with none of a, b, c existing is fully bootstrapped within seconds — in both interactive and -p non-interactive runs, ~60 s before the first housekeeping pass. So the dir housekeeping wants always exists by then, and your risk note is accurate as written (a statement about a hypothetical future caller). No change needed.
7. One follow-up, explicitly not this PR
If the container starts after the workspace was deleted (qwen dir never created inside the broken mount), both builds hang before rendering anything, at 100 % of one core (25.00 s CPU / 25 s wall, ~59k failing mkdir/s on the main thread). Tapping fs.mkdirSync via --require and reading the stack pins it to the same writeOutputLanguageFile call above; dist-patching installationManager.writeInstallationIdToFile and saveSettings to non-recursive did not stop the spin, because languageUtils gets there first. Both arms are identical here, so this is neither caused nor claimed by this PR — but it is the same bug class, and packages/{core,cli}/src has 226 recursive-mkdir call sites, 120 of them mkdirSync. Worth a follow-up issue (a shared "create one level, fail fast" helper), not a change here.
Not verified
macOS (the author's numbers stand unre-run); the non-interactive scheduler's degradation path (read only — the interactive one is measured end to end); the fleet-level incident figures (177 processes, 45 CPU-hours, load average 213 → 12); Windows only through Wine, not on a real Windows host.
中文说明
本地真实环境验证
我把两侧都构建出来,在一个真实的「源目录已删除的 bind mount」上跑,而不是只看 diff。结论:可以按现状合并 —— 描述里的每一条都复现了;CI 一个都没跑到的两个单元测试在 Linux 上通过且是真护栏;平台表里的
验证环境
- 两个真实的
npm run bundleCLI 构建:base9a954b4198(PR 的父提交)与 head4d54e6a336。两者打包后的runThrottledOnce只在一个调用处不同,别无其它:- await mkdir(dirname(opts.lockPath), { recursive: true, mode: 448 }).catch(() => {}); + await mkdir(dirname(opts.lockPath), { mode: 448 }).catch(() => {});
- 私有 mount namespace(
unshare -m),内含一个源目录已被删除的 bind mount,以 root 运行。 - 真实 win32
node.exev22.22.2(win-x64)跑在 Wine 上,用于补齐平台行。 - 先把口径讲清楚:全部以 uid 0 运行;有一个代码生成的 web-template 文件在两个构建里用了完全相同的 stub(与本路径无关,只为让 bundle 能链接过去);Wine 探针把
throttledOnce.ts单独打包并 shim 掉了 debug logger;Wine 不等于 Windows。
1. 机制与描述完全一致
recursive: true 在 6 秒后仍 pending,CPU 94%;单层形式 0 毫秒就以 ENOENT 拒绝,CPU 0%。strace 逐字印证了描述中推演的循环:mkdir(叶子)=ENOENT → mkdir(父)=EEXIST → statx(父)=S_IFDIR → 重试叶子,每秒约 1 万次失败的 mkdir,无上限、无退避。另外两条值得记录:fs.mkdirSync(…, { recursive: true }) 会以同样方式卡死(CPU 99%,且卡在主线程);单个卡死的调用链不会明显拖慢进程内其它 fs 操作(readFile p50 仅 ×1.3)—— 所以代价确实是 CPU,而不是 I/O 延迟。
2. 真实 CLI 走生产时序 —— 决定性的 A/B
事故形态是「进程启动时一切正常,之后挂载在它脚下被删掉」,所以我就照这个复现:把 QWEN_HOME 指到 bind mount 内启动真实 TUI,等它把 qwen 目录初始化好,在 t+14 秒 rm -rf 挂载源,然后等 housekeeping 首轮在约 t+62 秒触发。
两个构建放在同一个已损坏的挂载里,t+90 秒用 top 采样:改动前 91.2% CPU,本 PR 0.0%,同一时刻、同一挂载。完整 2×2(无 strace 干扰),统计 t+12 秒到 t+120 秒之间消耗的 CPU:
| 构建 | 挂载 | CPU / 108 秒 | marker | 进程 |
|---|---|---|---|---|
| 改动前 | 完好 | 0.27 s(0%) | 3/3 | 存活 |
| 改动前 | 源已删除 | 54.51 s(50%,空转期间 94%) | 0/3 | 存活但卡死 |
| 本 PR | 完好 | 0.25 s(0%) | 3/3 | 存活 |
| 本 PR | 源已删除 | 0.24 s(0%) | 0/3 | 存活 |
只有「改动前 + 已删除」这一格在烧 CPU;修复在健康那格没有任何开销,事后两侧 TUI 完全一致 —— 没有报错横幅、没有崩溃。整个 2×2 我跑了两轮,每一格都复现到 0.03 秒以内。
同一 50 秒窗口的系统调用层面:base 侧 105,390 次失败 mkdir、0 次加锁;head 侧 1 次 mkdir 之后紧接着 1 次对锁文件的 openat。head 侧的抛出在 0.6 毫秒内落进 runPass 的 catch,定时器链存活,正是 PR 声称的优雅跳过。base 侧第一个 runThrottledOnce 永不返回,串行链条压根到不了第 2、3 步,runPass 也再不会安排下一轮 —— 「把整条 housekeeping 链卡死到进程生命周期结束」这一点是观测到的,而不是推理出来的。
3. 两个单元测试:真跑了,并做了变异测试
throttledOnce.test.ts 在 Linux 上 13/13 通过。housekeeping 全量:head 78 通过 / 2 失败(80),base 76 通过 / 2 失败(78)—— 两侧是同样的两个失败,即 cleanup.test.ts 的 EACCES 用例,它们在 uid 0 下不可能失败。属于环境问题,与本 PR 无关。
变异测试结果 —— 3 个针对性变异全部被抓住,每次之后工作区都恢复干净:
| 变异 | 结果 |
|---|---|
加回 { recursive: true, mode: 0o700 } |
✗ never asks for a recursive mkdir |
整个删掉 mkdir(…) 调用 |
✗ 两个新测试都失败 |
把权限放宽到 { mode: 0o755 } |
✗ creates the lock directory when it does not exist yet |
额外加一个不带 options 的 mkdir(dir) |
13 通过 —— 护栏循环不会误报 |
stat.mode & 0o077 这条断言本身也有牙齿;而且护栏循环对将来出现的「不带 options 的 mkdir」是容忍而非报错。
4. Windows —— 补齐 ⚠️ 那一行
真实 win32 Node 跑在 Wine 上,驱动两侧编译产物中的 runThrottledOnce,同一个 driver 再在 Linux 原生跑一遍:
| 场景 | 改动前 | 本 PR |
|---|---|---|
| 父目录存在、锁目录缺失 | completed,目录已创建 | completed,目录已创建 |
| 整条父路径都缺失 | completed,整棵树被补齐 | THREW ENOENT,什么都没创建 |
锁目录已存在(EEXIST) |
completed | completed |
win32 与 linux 在两侧都给出一致结果,所有通过的场景都写了 marker、也没有泄漏锁文件。你关于「改动与平台无关」的推理经实测成立。
5. 你提出的疑问 —— 一个明确答案
你说无法解释为什么同一个永不 settle 的调用,在部分进程里表现为 mkdir 热循环,在另一些进程里表现为安静挂起、完全没有 mkdir 系统调用(promise 仍 pending、CPU 0%、libuv worker 全空闲)。CLI 会 re-exec 自身,所以你第一时间拿到的那个 pid 并不是真正跑代码的那个。改动前的构建在已损坏挂载里跑一次,同时从两个视角观测:
launcher pid 3311163(ps/top/pgrep 首先给你的) 100 秒内 CPU 0.00s mkdir 系统调用 0 次
re-exec 出来的 worker 3311177(housekeeping 在此) 100 秒内 CPU 46.82s 3 秒采样 26,759 次 mkdir
我自己就踩了这个坑:前三次运行都报 0.00 秒 CPU、零 mkdir 系统调用,我差点就写下「安静变体」的结论,直到顺着子进程追下去。这与你的描述完全吻合。(还有一个独立机制也能产生同样现象:把 libuv 线程池的 4 个线程全占满,递归 mkdir 连线程都拿不到 —— 已验证:10 秒后仍 pending、0 次系统调用、CPU 0%。两者都不构成阻塞;本 PR 会一并消除。)
6. 关于评审中提到的 QWEN_HOME
评审里提出的可达性担忧在实际中不成立,而且值得把原因记下来,而不是凭感觉改措辞。writeOutputLanguageFile(packages/cli/src/i18n/languageUtils.ts:238)直接在 main() 里对全局 qwen 目录执行 fs.mkdirSync(dir, { recursive: true }),发生在 UI 渲染之前。实测:QWEN_HOME=/deep/a/b/c/qwen 且 a、b、c 都不存在时,整棵树会在几秒内被补齐 —— 交互模式与 -p 非交互模式都是如此,比首轮 housekeeping 早约 60 秒。所以 housekeeping 需要的那个目录到时候一定已经存在,你 Risk 一节的措辞按原样也是准确的(它说的是一个假设中的将来调用方)。无需改动。
7. 一个后续项,明确不属于本 PR
如果容器是在工作目录被删除之后才启动(qwen 目录从未在损坏的挂载里创建过),两个构建都会在渲染任何东西之前挂死,占满一整个核(25 秒墙钟消耗 25.00 秒 CPU,主线程每秒约 5.9 万次失败 mkdir)。用 --require 挂钩 fs.mkdirSync 并读取调用栈,定位到的正是上面那个 writeOutputLanguageFile;把 installationManager.writeInstallationIdToFile 和 saveSettings 通过 dist-patch 改成非递归都止不住空转,因为 languageUtils 抢在前面。这里两侧完全一致,所以既不是本 PR 引入的、本 PR 也没有声称要修 —— 但它属于同一类缺陷,而 packages/{core,cli}/src 里共有 226 处递归 mkdir 调用点,其中 120 处是 mkdirSync。建议单独开 issue 跟进(比如提供一个统一的「只建一层、快速失败」helper),而不是在本 PR 里改。
未验证部分
macOS(作者给出的数字未重跑);非交互 scheduler 的降级路径(只读代码 —— 交互路径是端到端实测的);集群层面的事故数据(177 个进程、45 CPU 小时、load average 213 → 12);Windows 仅通过 Wine 验证,未在真实 Windows 主机上跑。
…ledOnce 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 轮)。改动内容与我反驳保留之处如下: Autofix round — PR #11285Commit: Feedback dispositionsrc:3948439991 + rc:3948855419 — [Critical] R1-1: unguarded permission-bit assertion fails on the Windows lane — RESOLVEDVerified the claim against in-repo evidence before editing (no win32 runner exists here either, so this mirrors the reviewer's settling authorities):
Fix: wrapped only the rc:3948440012 + rc:3948855430 — [Suggestion] R1-2: pin the effect of a never-creatable lock dir, not just the mkdir options — RESOLVEDAdded One deviation from the reviewer's literal snippet, forced by physics: the round-1 code block placed the lockPath ONE level below Failed check: Classify PR — not addressable from this PR
Reviewer's self-deferred probes in rv:5130934729 — left deferred, deliberately
Mutation probes (each run, then restored; suite re-confirmed green after restore)
Verification
Settings schema untouched (no settings source changed); integration tests not applicable (behavior is fully exercised by unit tests; no bundled-CLI-only surface). 中文说明Autofix 本轮处理 — PR #11285提交: 反馈处置rc:3948439991 + rc:3948855419 —— [Critical] R1-1:未加保护的权限位断言会在 Windows lane 上失败 —— 已解决修改前先依据仓库内证据核验了该论断(本环境同样没有 win32 runner,因此采用与评审者相同的定论依据):
修复:仅把 rc:3948440012 + rc:3948855430 —— [Suggestion] R1-2:钉住「锁目录永远无法创建」的效果,而不仅是 mkdir 的选项 —— 已解决在两个新增测试旁边补了 与评审者字面代码片段的一处偏差(由实际行为强制):第 1 轮代码块把 lockPath 放在 失败的检查:Classify PR —— 本 PR 无法处置
rv:5130934729 中评审者自我延后的两条探针 —— 刻意保持延后
变异探针(每条都已实际执行并随后还原;还原后套件重新确认为绿)
验证
未触碰 settings schema(未改动任何 settings 源);集成测试不适用(该行为已被单元测试完整覆盖,无仅经打包 CLI 才能触及的面)。 🦷 Gate advisory — this round resolves a Critical/Request-changes finding with test-only changes (machine-measured): the bite check cannot verify a test-side fix, so the resolution rests on the round summary alone. · 本轮以纯测试改动解决 Critical/Request-changes 反馈(门自动测量):bite 检查无法验证测试侧修复,该解决仅以轮次摘要为凭。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧵 Resolved all 4 selected review thread(s). · 已关闭全部选中的 4 条评审线程。 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/模型 |
chiga0
left a comment
There was a problem hiding this comment.
Scope: packages/cli/src/utils/housekeeping/throttledOnce.ts, throttledOnce.test.ts. scheduler.ts read for cross-file caller verification (not in diff).
Tier: Standard — targeted bug fix changing one mkdir option; filesystem behavior boundary; caller coverage required.
No blocking findings.
Checked:
-
All 3 callers of
runThrottledOnceinscheduler.ts— every call site passeslockPath = join(qwenDir, '<marker>.lock')whereqwenDir = Storage.getGlobalQwenDir(). Sodirname(lockPath) = qwenDir, and the single-level mkdir targets only that one directory. Its parent is always a pre-existing directory (e.g.$HOMEin the default case, or the parent ofQWEN_HOMEin a customized layout). The PR's claim that one level is sufficient is verified against all three callers. -
Error handling chain —
mkdir(qwenDir, { mode: 0o700 }).catch(() => {}): EEXIST is swallowed when the dir already exists (normal case); ENOENT is swallowed when the parent is a deleted bind mount. Either way the subsequent lock acquisition (open(lockPath, 'wx')) surfaces the filesystem error andrunThrottledOncepropagates it, so housekeeping degrades to a skip — the documented intended behaviour. -
Three new tests:
'creates the lock directory when it does not exist yet'— real-fs test; parenttempDirexists; single-level mkdir succeeds; permission bits asserted with correctwin32guard (if (process.platform !== 'win32')).'never asks for a recursive mkdir'—vi.spyonnode:fs/promises; inspects everymkdircall's options; guard is real (author confirms restoringrecursive: truemakes it fail).'surfaces ENOENT instead of bootstrapping a missing parent'— real-fs test;tempDir/nope/nestedwherenopedoesn't exist; non-recursive mkdir swallowed, lock open then rejects with ENOENT → correct fast-fail.
Cross-check (findings frozen above before reading existing reviews):
-
CI bot R1-1 (
CHANGES_REQUESTED, round 1 + round 2): "permission-bit assertion has nowin32guard" — Refuted at current head (51c94b68). The autofix commit addedif (process.platform !== 'win32')around theexpect(stat.mode & 0o077).toBe(0)assertion. Confirmed in the current diff. -
CI bot R1-2 (Suggestion): spy-options pin — present at head; suggestion-level; does not block.
-
CI bot deferred (round 2):
$HOMEcomment accuracy underQWEN_HOME— confirmed minor comment imprecision, not a code defect; the actual behaviour (mkdir one level, fail-fast otherwise) is unchanged and acceptable. Swallowed mkdir error not logged — confirmed observability gap; non-blocking. -
wenshaoAPPROVED with local runtime verification on a real deleted bind mount: claims reproduced, Linux + Windows unit suites pass. Strengthens the verdict.
Reviewed with AI assistance.
Already have 2 approves,3ks.
|
Released in v0.23.1. |






What this PR does
Housekeeping ensures its lock directory exists before taking the once-per-interval lock. That call asked for a recursive create; this PR makes it create a single level instead, so a create that cannot succeed fails fast rather than retrying forever.
Why it's needed
When the global qwen dir lives inside a bind mount whose source directory has since been deleted, the mountpoint still stats as a directory while creating entries inside it returns
ENOENT. Node's recursive mkdir interprets thatENOENTas "the parent is missing", creates the parent (EEXIST), stats it to confirm it is a directory, retries the leaf, getsENOENTagain — with no retry cap and no backoff. The returned promise therefore never settles, which has two consequences: the existing.catch(() => {})never runs, so nothing is logged and nothing degrades gracefully; and theawaitwedges the housekeeping chain for the remaining lifetime of the process.This is not theoretical — it was the amplifier behind a CPU overload on the self-hosted ECS runner pool. E2E sandbox containers that outlived their host-side
docker runclient kept running after their job had already deleted the workspace those containers had bind-mounted. Roughly a minute later each container's housekeeping timer fired, hit this path, and started spinning. Measured on one 64-core host: 177 such processes,~2.5kfailingmkdir/seach,~67%system time fleet-wide, and 45 CPU-hours accumulated by the oldest single process. Across five hosts the orphaned containers were consuming about 232 of 320 cores. Reaping the containers dropped one host's load average from 213 to 12.Container leakage itself is being addressed separately in #11264; this PR removes the CLI-side amplifier so that an orphaned or otherwise wedged sandbox costs approximately nothing instead of a full core.
A single level is sufficient here: all three
runThrottledOncecall sites putlockPathdirectly in the global qwen dir, whose own parent is$HOME. When the directory genuinely cannot be created the subsequent lock acquisition surfaces the error, so housekeeping degrades to a skip — the intended behaviour — instead of hanging.Reviewer Test Plan
How to verify
The unit tests cover the invariant and the preserved behaviour:
cd packages/cli && npx vitest run src/utils/housekeeping/throttledOnce.test.ts. Two tests are added — one asserts the lock directory is still created (with no group/other permission bits) when it does not exist yet, and one asserts nomkdircall requestsrecursive: true. To confirm the second test is a real guard rather than a tautology, temporarily restorerecursive: trueinthrottledOnce.ts; it fails withexpected { recursive: true, mode: 448 } to not match object { recursive: true }.To reproduce the underlying condition on Linux (needs root for
mount;unshare -mkeeps the bind mount in a private namespace so nothing leaks to the host):Expected: the recursive form is still pending after six seconds while the single-level form has already rejected with
ENOENT. Wrapping each form in/usr/bin/timeshows the recursive form burning103%CPU against0%for the single-level form.Evidence (Before & After)
Not a user-visible or TUI change. Behavioural evidence is the reproduction above, measured on one of the affected Linux hosts:
For reference, the syscall signature observed on a live wedged process before the fix —
mkdirfailing 100% of the time, at roughly 2.5k calls per second:Tested on
macOS: unit tests —
throttledOnce.test.ts13 passed, and the full housekeeping surface (src/utils/housekeepingplussrc/services/housekeeping) 80 passed. Linux: the bind-mount reproduction and before/after measurement above. Windows: not tested; the failure mode is Linux bind-mount specific, and the change itself is platform-neutral.Environment (optional)
Unit tests only, plus a root shell on an affected Linux host for the mount-level reproduction.
Risk & Scope
lockPathwhose parent directory does not already exist, the directory is no longer created for it and housekeeping skips that pass instead of silently bootstrapping the tree. All three current call sites resolve to the global qwen dir, so none are affected; a new call site with a deeper path would need to create its own parent.mkdirloop in some processes (production, and a standalone Node repro at 103% CPU) but as a quiet hang with nomkdirsyscalls at all in others (the real CLI under my reproduction — promise still pending, 0% CPU, idle libuv workers). Both variants are removed by this change, but the trigger that selects between them is unknown; a plausible but unverified factor is the extra nested bind mount present in the CI layout. Container leakage itself is out of scope and handled by fix(ci): reap leaked E2E sandbox containers #11264. No Windows verification.Linked Issues
Related to #11264, which stops the E2E sandbox containers from leaking in the first place. This PR is independent and complementary: it removes the CLI-side cost of a container that has already been orphaned, whatever the cause.
中文说明
这个 PR 做了什么
housekeeping 在获取「每周期一次」的锁之前,会先确保锁目录存在。原先这个调用用的是递归创建;本 PR 改为只创建一层,这样当创建注定无法成功时会快速失败,而不是无限重试。
为什么需要
当全局 qwen 目录位于某个 bind mount 内、而该挂载的源目录已被删除时,挂载点本身仍然能被 stat 成目录,但在其内部创建条目会返回
ENOENT。Node 的递归 mkdir 把这个ENOENT理解为「父目录缺失」,于是去创建父目录(得到EEXIST)、stat 确认它是目录、重试叶子节点、再次得到ENOENT—— 没有重试上限,也没有退避。因此返回的 promise 永远不会 settle,带来两个后果:现有的.catch(() => {})永远不会执行,既没有日志也没有优雅降级;同时这个await会把整条 housekeeping 链卡死到进程生命周期结束。这不是理论推演 —— 它正是自建 ECS runner 集群一次 CPU 过载的放大器。E2E sandbox 容器在宿主机侧的
docker run客户端被回收后仍在运行,而对应 job 早已删除了这些容器 bind mount 进去的工作目录。大约一分钟后,每个容器的 housekeeping 定时器触发、命中这条路径并开始空转。在一台 64 核宿主机上实测:177 个这样的进程,每个每秒约 2500 次失败的mkdir,全机 system time 约 67%,其中最老的单个进程累计消耗 45 CPU 小时。五台宿主机上,这些孤儿容器合计占用了约 320 核中的 232 核。回收容器后,其中一台的 load average 从 213 降到 12。容器泄漏本身由 #11264 单独处理;本 PR 移除 CLI 侧的放大器,使得一个已经变成孤儿或以其它方式卡死的 sandbox 的开销接近于零,而不是吃满一个核。
这里只创建一层就足够了:
runThrottledOnce的三个调用点都把lockPath直接放在全局 qwen 目录下,而该目录的父目录是$HOME。当目录确实无法创建时,后续的加锁步骤会把错误暴露出来,于是 housekeeping 降级为跳过 —— 这正是预期行为 —— 而不是挂死。评审验证方案
如何验证
单元测试覆盖了这个不变量以及被保留的行为:
cd packages/cli && npx vitest run src/utils/housekeeping/throttledOnce.test.ts。新增两个测试 —— 一个断言锁目录在不存在时仍会被创建(且不带 group/other 权限位),另一个断言没有任何mkdir调用请求recursive: true。为了确认第二个测试是真正的护栏而不是同义反复,可以临时把throttledOnce.ts里的recursive: true加回去;它会失败并报出expected { recursive: true, mode: 448 } to not match object { recursive: true }。在 Linux 上复现底层条件(
mount需要 root;unshare -m把 bind mount 隔离在私有命名空间内,不会泄漏到宿主机):使用上面英文部分给出的脚本。预期结果:递归形式在六秒后仍处于 pending,而单层形式已经以
ENOENT拒绝。用/usr/bin/time分别包裹两种形式可以看到,递归形式烧掉103%CPU,单层形式为0%。证据(前后对比)
不是用户可见或 TUI 变更。行为证据即上面的复现,在一台受影响的 Linux 宿主机上实测:改动前(
recursive:true)六秒后仍未 settle、CPU 103%;改动后(recursive:false)以ENOENT拒绝、CPU 0%。作为参考,修复前在一个活体卡死进程上抓到的系统调用特征是mkdir100% 失败、每秒约 2500 次,详见英文部分表格。测试平台
macOS:单元测试 ——
throttledOnce.test.ts13 项通过,housekeeping 全量(src/utils/housekeeping加src/services/housekeeping)80 项通过。Linux:上述 bind mount 复现与前后对比测量。Windows:未测试;该故障模式是 Linux bind mount 特有的,而改动本身与平台无关。环境(可选)
仅单元测试,另加一台受影响 Linux 宿主机上的 root shell 用于挂载层面的复现。
风险与范围
lockPath,该目录将不再被自动创建,housekeeping 会跳过这一轮而不是静默地把整棵目录树补齐。当前三个调用点都指向全局 qwen 目录,因此都不受影响;若新增更深路径的调用点,需要自行创建其父目录。mkdir热循环(生产环境,以及一个 103% CPU 的独立 Node 复现),而在另一些进程中表现为安静挂起、完全没有mkdir系统调用(真实 CLI 在我的复现下 —— promise 仍 pending、CPU 0%、libuv worker 全空闲)。本改动会消除这两种表现,但决定走向哪一种的触发条件尚不清楚;一个合理但未经验证的猜测是 CI 布局中多出的一层嵌套 bind mount。容器泄漏本身超出本 PR 范围,由 fix(ci): reap leaked E2E sandbox containers #11264 处理。没有 Windows 验证。关联 Issue
与 #11264 相关,后者从源头阻止 E2E sandbox 容器泄漏。本 PR 与其独立且互补:无论成因如何,它消除的是一个已经变成孤儿的容器在 CLI 侧带来的开销。