test(desktop): force the dist suite runner to exit once its tests finish - #4941
Conversation
e4defbe to
1cb8560
Compare
afff594 to
3086f56
Compare
The desktop dist suite hung its CI job on run 34061414876 with every reported test passing: the vite-workspace-packages test file completed both subtests, but its node:test child process stayed alive for 811s until the workspace runner hit the 900s ceiling and killed the run. The lingering handle is a chokidar fs.watch on the temporary repository root, orphaned by a TOCTOU race between chokidar close() and the in-flight scan of the dev-server restart that the first test triggers (apache#4940 has the full fs-level attribution). The second test dodged this class in apache#4748 by dropping its watcher; the first test needs its watcher to assert restart-on-manifest-change, so it cannot. --test-force-exit makes the runner exit once every test has finished, so completed tests still report their real results while post-test lingering handles can no longer hold the workspace run hostage. Fixes apache#4940 Generated-by: GLM-5.3-Flash (pi)
3086f56 to
1dbc4f2
Compare
hqhq1025
left a comment
There was a problem hiding this comment.
复核 1dbc4f237f04d46ec5bf1701f65fbc3d5e4ddb64,未发现有充分证据支持的 P0–P3 问题。此提交在 apps/desktop/package.json:46 给整个 Desktop dist 套件加入 --test-force-exit,没有修补 watcher 本身。
已独立读取旧 CI run 34061414876 的原始日志:两个 Vite 子测试通过,但文件进程滞留到 Desktop 的 900000ms 上限。apps/desktop/scripts/vite-workspace-packages.test.mjs:35 确实等待 server.close();apps/desktop/vite-workspace-packages.ts:33 注册 manifest 依赖。安装版本 Vite 的 bundled chokidar 在 _addToNodeFs 等待 _handleDir 返回后先检查 closed、再登记 closer。通过真实 Vite watcher 的可控暂停复现了这一窗口:close 后注册表为空,仍有一个 FSEventWrap;手动调用被丢弃的 closer 后降为零。
Node 24.18.1 内部实现会在已知测试及 hooks 完成后执行强制退出。独立子进程探针验证:遗留 interval 的通过用例从滞留变为退出 0,断言失败、异步 after hook 失败和有截止时间的未完成测试仍退出 1。干净安装、build:test、完整 Desktop test:dist 2230/2230 通过,包含真实 Vite manifest 重载测试。所用参数在仓库最低 Node 22.19 的官方文档中已存在。
限制是整个套件结束后的遗留句柄不再暴露为挂起,不能把测试通过解释为资源都已清理;该参数也不解决尚未完成的测试或 hook。没有重现作者给出的高负载随机发生率,验证的是确定性 closer 丢失窗口和 runner 行为。当前 head 的 hosted test、audit、Linux/Windows package 均成功;与 main b06eb02e6 合并树无冲突,未在合并树上测试。
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for the update. Additional independent review of 1dbc4f237f04d46ec5bf1701f65fbc3d5e4ddb64: the force-exit workaround addresses the documented watcher hang. One minor mismatch remains between the PR description and the committed removal marker.
AI disclosure: Codex agents performed this review; the coordinating agent checked the cited evidence. This is not an independent human review.
中文说明
force-exit 缓解了已记录的 watcher 挂起;正文关于移除标记的声明与当前提交存在一处轻微不一致。
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for the change. Approving exact head 1dbc4f237f04d46ec5bf1701f65fbc3d5e4ddb64 following the completed review and Astro-Han’s explicit acceptance of this merge batch. Current checks pass and no review threads remain unresolved.
A documented Vite/chokidar watcher leak could leave completed Desktop tests hanging until the workspace timeout. Use node:test force-exit after tests finish, preserving reported failures. This bounds runner shutdown; the underlying watcher defect remains separately tracked.
AI assistance: Codex performed the review and final-state verification; Astro-Han authorized approval and merge.
中文
感谢改动。基于已完成的审查和 Astro-Han 对本批次的明确认可,批准当前精确 head;检查通过,讨论已结清。此前说明的验证边界与后续事项保持不变。本次由 Codex 执行审查和状态核对,Astro-Han 授权批准与合并。
Summary
The
testjob failed on #4878's run 34061414876 with no failing test: the desktop workspace reported 2357 pass / 0 fail / 1 cancelled, yet the run never finished —scripts/vite-workspace-packages.test.mjsstayed pending for 811s until the workspace runner hit its 900s ceiling and killed everything:Both subtests had completed and reported. The child process is what does not end. This is the same leak class #4748 already removed for the second test — but #4748 could dodge it there with
watch: nullbecause that test ignores file changes. The first test needs its watcher (it asserts restart-on-manifest-change), so it stays exposed, and the hang resurfaced on CI.Root cause, reproduced on Linux (node 24, vite 8.2.2) in ~7% of iterations under a 3-way CPU/IO load that matches
run-workspace-tests-parallel's concurrency, with fs-level attribution via afs.watch/setTimeoutshim plusprocess.getActiveResourcesInfo():workspacePackagesPluginpushes the repository and workspace manifests intoconfigFileDependencies; chokidar therefore watches each manifest file and its parent directory.server.close()neither waits for nor cancels that scan. The drop lives in the vite-bundled, pnpm-patched chokidar 3.6.0 (the top-level chokidar 5.0.0 is not in this call chain):_handleDir()creates the directory watcher and returns its closer, but the caller_addToNodeFs()registers it with_addPathCloser()only after re-checkingthis.fsw.closed. Whenclose()lands inside that await gap it sweeps a still-empty_closerslist, the resumed_addToNodeFsdiscards the closer at the closed check, and nothing ever closes the handle. An independent forced-interleave reproduction confirms the exact drop point:closed=true, closers list empty, the nativefs.watchstill open, and the lost closer closing it when invoked by hand.fs.watchon the temporary repository root (FSEventWrap, plus the scan's pendingFSReqCallbacks) keeps the child alive indefinitely;node --testwaits for the child; the workspace burns its whole timeout budget.--test-force-exitis a standing hygiene choice for this suite, not a temporary workaround pending an upstream fix: once every test has reported, no post-test lingering handle can hold the run hostage, whatever its origin, so no removal condition is tracked. The chokidar/vite closer-drop bug stays documented in #4940 for upstreaming.The fix is the node:test runner knob built for exactly this:
--test-force-exit(node ≥22; CI runs node 24). Once every test has finished, the runner exits the process tree, so completed tests still report their real results — failures included — while post-test lingering handles can no longer hold the workspace run hostage.Fixes #4940
Verification
Before — CI (run 34061414876): hang dump quoted above; the
testjob failed andDesktop e2ewas skipped.Before — local repro (docker
node:24-bookworm-slim, the repo copy,node --test scripts/vite-workspace-packages.test.mjslooped 80–100× under three CPU/IO burners): 7/100 and 6/80 iterations never exited (killed by the probe at 60–90s; on CI the same linger ran 811s). Attribution from the surviving process, once per hang, always identical:Watcher #10's creation stack is chokidar's
_handleDir → _watchWithNodeFs → fs.watchon the temp repository root — the parent-directory watch for the manifest config dep, created by the restart-triggered scan and orphaned by theclose()race. A deliberate fixture (tests pass, then a leakedsetInterval) reproduces the byte-identical interrupted-run signature, confirming the mechanism is "tests done, process stuck", not a slow test.After — same loop with
--test-force-exit: 80/80 iterations exited normally under the identical load; zero iterations exceeded 8s. The same flag on the fixture turns the indefinite hang into a normal green completion in ~3.1s with all test results intact (4 pass, exit 0).Ran:
biome format apps/desktop/package.json— clean. Not run: the full desktop dist suite and typecheck locally (no TS surface touched); this PR's CI runs both.Root cause
vite's dev-server
close()does not drain the watcher's initial scan, and the bundled chokidar 3.6.0 drops the closer at_addToNodeFs's post-awaitclosedcheck, orphaning thefs.watchhandle; current chokidar 5.0.0 shares the same structure, so this is a chokidar bug, vendored by vite. Reported as #4940 with the full attribution; upstreaming to chokidar/vite can follow separately.--test-force-exitis deliberately scoped to the desktop runner here; other workspaces can adopt it if they ever show the same signature.AI use
Select exactly one:
Tool(s) and scope: GLM-5.3-Flash (served via Ollama Cloud) running in the pi coding agent reproduced the hang in a Linux container under synthetic CI load, attributed the lingering handle with fs-level instrumentation, designed the fix, and wrote the change and this PR.
Checklist
Does this PR entail a change in behavior?