fix(cli): clean orphaned managed npm update artifacts - #7539
Conversation
|
Thanks for the PR! Template looks good ✓ Problem: this is a known, observed gap — not theoretical. The managed npm update system acknowledges it in code (the "ponytail" comment at the bottom of Direction: aligned. This is Phase 1 of a maintainer-filed issue, implementing the exact low-risk cleanup the triage recommended — orphan staging dirs + stale Size: not applicable — Approach: the scope feels right. Conservative by design — only removes artifacts matching strict managed-name patterns, only for PIDs that explicitly return 中文说明感谢贡献! 模板完整 ✓ 问题:这是一个已知的、已观测到的缺口,而非理论性问题。托管 npm 更新系统在代码中已明确承认此问题( 方向:对齐。这是维护者提交的 issue 的 Phase 1 实现,与分诊推荐的低风险清理方案完全一致——清理已确定死亡 PID 的孤儿 staging 目录和残留 规模:不适用—— 方案:范围合理。设计上保守——仅删除严格匹配托管工件命名模式的条目,仅针对明确返回 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code ReviewIndependent proposal: I would add a synchronous cleanup function called at the start of Comparison: The PR matches this almost exactly. Three small helper functions ( No critical blockers found. Specific observations:
Real-Scenario TestingNon-user-visible change (internal filesystem cleanup during managed npm updates). Drove the cleanup path directly via a focused filesystem scenario script — creates orphaned artifacts for a dead PID (999999999), live artifacts for the current PID, a version dir, an unknown dir, and a staging-shaped symlink, then calls Before (main branch — no cleanup)After (PR #7539 — with cleanup)Unit tests中文说明代码审查独立方案: 在 对比: PR 与独立方案几乎完全一致。三个小辅助函数加一个调用点——63 行生产代码。没有遗漏更简路径。 未发现关键阻塞问题。具体观察:
真实场景测试非用户可见变更(托管 npm 更新期间的内部文件系统清理)。通过聚焦文件系统场景脚本直接驱动清理路径——为死亡 PID 创建孤儿工件,为当前 PID 创建存活工件,以及版本目录、未知目录和 staging 形状的符号链接,然后调用 修改前(main 分支):过期工件 persist,2 项检查失败。 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 5/5 — clean across every stage; implements exactly what the maintainer recommended in #7524, conservative by design, and the before/after proves it works. This is a textbook Phase 1 implementation. The issue triage recommended "orphan staging + temp cleanup on startup, ~30 lines, zero risk to running sessions" — the PR delivers exactly that in 63 production lines with comprehensive tests. The cleanup is deliberately conservative: only ESRCH triggers removal, symlinks and uncertain liveness are preserved, failures never propagate. My independent proposal was essentially the same approach, and I didn't find a simpler path. The before/after scenario confirms the fix: stale staging dirs and temp active files for dead PIDs are removed, while live artifacts, version dirs, unknown entries, and symlinks are all preserved. 13/13 unit tests pass, typecheck and lint are clean. If I had to maintain this in six months, I'd thank the author — the code is clear, the regex patterns are self-documenting through the test cases, and the conservative error handling means this cleanup can never break the update path. 中文说明置信度:5/5——每个阶段都干净;精确实现了维护者在 #7524 中推荐的方案,设计上保守,before/after 证明有效。 这是一个教科书式的 Phase 1 实现。Issue 分诊推荐"启动时清理孤儿 staging + 临时文件,约 30 行,对运行中会话零风险"——PR 以 63 行生产代码和全面测试精确交付。清理刻意保守:仅 ESRCH 触发删除,符号链接和不确定存活状态被保留,失败不会传播。我的独立方案本质上是相同的方法,没有找到更简路径。 before/after 场景确认了修复:死亡 PID 的过期 staging 目录和临时活动文件被删除,而存活工件、版本目录、未知条目和符号链接全部保留。13/13 单元测试通过,类型检查和 lint 干净。 — Qwen Code · qwen3.8-max-preview Reviewed at |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
— qwen3.7-max via Qwen Code /review
Review & Local Verification Report代码审查设计评价:安全的孤儿进程清理。 本 PR 在 managed npm update 的 staging 阶段清理崩溃进程遗留的临时文件。 核心逻辑:
安全设计亮点:
测试: 2 个测试——清理孤儿 + 不确定时保留。mock 结论LGTM。 保守的清理策略(不确定时保留),正确的进程存活检测,完善的边界处理。 |
🧪 Maintainer Local Verification Report (macOS)Tested at commit 1. Shipped test suite — 13/13 ✅Includes the two new tests: 2. Real-flow E2E — no mocks, real macOS
|
| Target PID | Syscall | Liveness | Verdict |
|---|---|---|---|
| self / live worker | no throw | alive | keep |
999999999 (nonexistent) |
ESRCH |
dead | remove |
| just-exited child | ESRCH |
dead | remove |
1 = launchd (root-owned) |
EPERM |
uncertain | keep |
3. A/B load-bearing proof ✅
Overlaid the base managed-npm-update.ts (git show <merge-base>:…, cleanup absent) while keeping all tests, then restored head:
BASE source → Tests 2 failed | 14 passed (16) ← the two removal tests fail
HEAD source → Tests 16 passed (16) ← all green with the fix
The two failures are exactly the discriminating removal assertions (removes only orphaned… and my real-flow removes dead-PID…); every "keep" test passes on base too (base retains everything). This proves the cleanup is load-bearing, not tautological.
4. Typecheck & Lint ✅
prettier --check(both changed files): All matched files use Prettier code style!eslint(both changed files): clean (exit 0)tsc --noEmit: 0 errors in the changed file. (52 residual errors wereTS6305— siblingdistnot built in the isolated worktree; symlinkingpackages/*/distclears 51, the last being an unrelateduseGeminiStream.tstype-skew. None touch this PR.)
5. ⚠️ The red Test (ubuntu) check is unrelated to this PR
The failing job fails on:
FAIL packages/core/src/tools/agent/agent.test.ts:3157
AgentTool > Fork dispatch (subagent_type: "fork") > runs a non-interactive fork through the background registry
AssertionError: expected "spy" to be called with arguments: [ Any<String>, …(2) ]
Test Files 1 failed | 521 passed | 1 skipped (523)
This is in packages/core, a fork-dispatch test. This PR changes only packages/cli/src/utils/managed-npm-update.{ts,test.ts} — zero files under packages/core — so that file is byte-identical to main. Evidence it is a pre-existing main-side failure, not caused by #7539:
- Reproduces deterministically (2/2) on the PR head locally.
- The same test is also red on an unrelated PR (feat(cli): add version upgrade notices #7542).
- It lives in the area touched by the recent main commit
8511de61d fix(core): make fork subagents discoverable (#7460).
Recommendation: a rebase onto current main and/or CI re-run should clear this; it does not reflect a defect in this PR.
6. Code review notes
- Conservative fail-safe design. Deletes only entries matching strict managed-artifact names (
.<semver>-<pid>-XXXXXXstaging dirs,active.json.<pid>temps) and only whenprocess.kill(pid,0)returnsESRCH. Symlinks/junctions (isSymbolicLink()guard, target left intact), uncertain liveness (EPERM), non-semver names, unknown entries, the canonicalactive.json, and immutable version payloads are all retained. AnyrmSyncerror is swallowed withcontinue. - No user-visible / TUI behavior change — the pass runs synchronously inside
prepareManagedNpmUpdatebefore staging. - Implements Phase 1 of Track disk cleanup for managed npm update artifacts #7524, consistent with the existing
ponytailnote in the source (immutable payloads left for a later lease-based GC).
Verdict
✅ The change itself is merge-ready — minimal, conservative, and now verified on macOS with real (un-mocked) process-liveness semantics, an A/B load-bearing proof, and clean lint/typecheck/format. The only outstanding item is the unrelated red core agent.test.ts check (§5), which should be resolved via rebase/re-run independently of this PR.
🇨🇳 中文报告
🧪 维护者本地验证报告(macOS)
测试提交 1cc73b8a0cd7,环境 macOS(darwin arm64,Node v22.23.1,vitest 3.2.4),在隔离 worktree 中本地构建并运行。本次更新:新增无 mock 的真实流程 E2E、A/B 载荷证明、截图,并诊断了失败的 CI 检查(与本 PR 无关,见第 5 节)。
1. 既有测试套件 — 13/13 ✅ 包含本 PR 新增的 2 个测试(孤立工件清理 + 存活状态不确定时保留)。
2. 真实流程 E2E — 无 mock,真实 macOS process.kill — 3/3 ✅
既有测试会 mock process.kill,因此从未真正调用跨平台差异所在的存活探测系统调用(本 PR 仅在 Windows 上测试过)。我新增了一个零 mock、用真实 PID 驱动真实 prepareManagedNpmUpdate() 的用例:
- self / 存活 worker → 不抛异常 → alive → 保留
999999999(不存在)/ 刚退出的子进程 →ESRCH→ dead → 删除1= launchd(root 所有)→EPERM→ 不确定 → 保留
真实观测到的行为与 PR 的假设完全一致。
3. A/B 载荷证明 ✅ 覆盖 base 版源码(git show <merge-base>:…,无清理逻辑)并保留全部测试:base 下 2 failed | 14 passed(恰好是两个"删除"断言失败,"保留"类断言在 base 下全部通过),恢复 head 后 16 passed (16)。证明清理逻辑是真正起作用的,而非恒真。
4. 类型检查 & Lint ✅ prettier、eslint 均干净;tsc --noEmit 在改动文件中 0 错误(其余 52 个是隔离 worktree 未构建 sibling dist 的 TS6305,与本 PR 无关)。
5. Test (ubuntu) 检查与本 PR 无关
失败位于 packages/core/src/tools/agent/agent.test.ts:3157(Fork dispatch > runs a non-interactive fork through the background registry)。本 PR 只改动 packages/cli/src/utils/managed-npm-update.{ts,test.ts},未触碰 packages/core 任何文件——该文件与 main 逐字节相同。证据:本地在 PR head 上稳定复现(2/2);无关 PR #7542 也在同一测试上失败;该测试位于近期 main 提交 8511de61d fix(core): make fork subagents discoverable (#7460) 涉及的区域。建议:rebase 到最新 main 并/或重跑 CI 即可清除,这并非本 PR 的缺陷。
6. 代码审查要点 保守的失败安全设计:仅当名称严格匹配托管工件格式且 process.kill 返回 ESRCH(确认已死)时才删除;符号链接、EPERM、非 semver 名称、未知条目、active.json、不可变版本 payload 全部保留;rmSync 出错时 continue。无用户可见 / TUI 行为变更。实现 #7524 第一阶段,与源码中已有的 ponytail 注释一致。
结论 ✅ 变更本身可合并——最小、保守,且已在 macOS 上用真实(未 mock)的进程存活语义、A/B 载荷证明及干净的 lint/typecheck/format 验证。唯一未决项是与本 PR 无关的 red 核心 agent.test.ts 检查(第 5 节),应通过 rebase/重跑独立解决。
wenshao
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
— qwen3.8-max-preview via Qwen Code /review
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
— qwen3.7-max via Qwen Code /review
|
The PR is approved and updated with main, but five workflows are awaiting maintainer approval. Could you approve the pending workflows and merge once checks pass? |
1412485
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
— qwen3.7-max via Qwen Code /review
🍏 macOS Local Test Report — PR #7539Maintainer-verified on macOS (darwin-arm64, Node v22.22.2) to complement the author's Windows testing. 1. Unit Tests — 13/13 ✅All 13 tests in
2. Real E2E Test — 9/9 checks ✅A real end-to-end test (no mocks) was run against the built
The test used a real dead PID (spawned a child process, captured its PID, waited for exit, confirmed 3. TypeCheck & Lint ✅
4. Code Review NotesThe implementation is conservative and well-scoped:
Test Environment
EvidenceTest evidence (screenshots, raw output, E2E script) is archived at: 🇨🇳 中文测试报告🍏 macOS 本地测试报告 — PR #7539由维护者在 macOS (darwin-arm64, Node v22.22.2) 上验证,补充作者的 Windows 测试。 1. 单元测试 — 13/13 全部通过 ✅
2. 真实 E2E 测试 — 9/9 检查项全部通过 ✅针对构建后的
测试使用了真实死 PID(启动子进程、捕获 PID、等待退出、确认 3. 类型检查 & Lint ✅
4. 代码审查要点实现保守且范围明确:
测试环境
证据存档测试证据(截图、原始输出、E2E 脚本)存档于: |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
— qwen3.7-max via Qwen Code /review




What this PR does
Adds a conservative cleanup pass before a managed npm update begins. It removes only stale update staging directories and stale temporary active-pointer files associated with a definitively dead process.
Why it's needed
An update worker terminated by force, out-of-memory termination, or shutdown skips its normal cleanup and can leave scoped temporary artifacts on disk indefinitely. This implements Phase 1 of the linked issue without reclaiming immutable version payloads.
Reviewer Test Plan
How to verify
Create a managed-update launcher root that contains a staging directory and temporary active-pointer file for a PID that no longer exists, then begin another managed update. Confirm those two artifacts are removed. Confirm artifacts owned by a live PID, an immutable version payload, unrelated entries, and a staging-shaped symlink or junction remain.
Evidence (Before & After)
Before: the focused regression test failed because a stale managed staging directory remained. After: the focused regression test passes 13/13; Prettier and targeted ESLint also pass.
node_modules\.bin\vitest run --config .qwen/test-support/managed-npm-update.vitest.config.ts packages/cli/src/utils/managed-npm-update.test.ts --coverage.enabled=falseTested on
Environment (optional)
Windows 11 focused Vitest harness; no user-visible or TUI behavior is changed.
Risk & Scope
ESRCH; symlinks, uncertain liveness, filesystem errors, and unknown entries are retained.Linked Issues
Fixes #7524
Duplicate check: searched open pull requests for
7524in the title and body before submission; no matching open PR was found.中文说明
此 PR 的内容
在托管 npm 更新开始前增加一个保守的清理步骤。它只删除与已明确不存在的进程关联的过期更新暂存目录和过期临时活动指针文件。
为什么需要它
更新工作进程被强制终止、因内存不足被终止或在关机时终止,会跳过正常清理,并可能无限期地在磁盘上留下受限范围内的临时工件。本 PR 实现关联 Issue 的第一阶段,不回收不可变的版本 payload。
审阅者测试计划
验证方式
创建一个托管更新 launcher 根目录,其中包含属于一个已不存在 PID 的暂存目录和临时活动指针文件,然后启动另一项托管更新。确认这两个工件被删除。确认属于存活 PID 的工件、不可变版本 payload、无关条目以及具有暂存目录形状的符号链接或 junction 均被保留。
证据(修改前与修改后)
修改前:聚焦回归测试失败,因为过期的托管暂存目录仍然存在。修改后:聚焦回归测试 13/13 通过;Prettier 和目标 ESLint 检查也通过。
node_modules\.bin\vitest run --config .qwen/test-support/managed-npm-update.vitest.config.ts packages/cli/src/utils/managed-npm-update.test.ts --coverage.enabled=false测试平台
环境(可选)
Windows 11 聚焦 Vitest 测试工具;没有改变用户可见或 TUI 行为。
风险与范围
ESRCH的 PID;符号链接、存活状态不确定的条目、文件系统错误和未知条目都会保留。关联 Issue
修复 #7524
重复检查:提交前已在开放 Pull Request 的标题和正文中检索
7524;未发现匹配的开放 PR。