Skip to content

test(desktop): force the dist suite runner to exit once its tests finish - #4941

Merged
Astro-Han merged 1 commit into
apache:mainfrom
orangeCatDeveloper:test/desktop-dist-force-exit
Sep 7, 2026
Merged

test(desktop): force the dist suite runner to exit once its tests finish#4941
Astro-Han merged 1 commit into
apache:mainfrom
orangeCatDeveloper:test/desktop-dist-force-exit

Conversation

@orangeCatDeveloper

@orangeCatDeveloper orangeCatDeveloper commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Summary

The test job 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.mjs stayed pending for 811s until the workspace runner hit its 900s ceiling and killed everything:

⚠ scripts/vite-workspace-packages.test.mjs
✔ renderer loads a newly exported workspace module after its manifest changes (198ms)
✔ renderer-facing Runtime Host protocol does not load Node crypto (44ms)
✖ scripts/vite-workspace-packages.test.mjs (811706ms)
  'Promise resolution is still pending but the event loop has already resolved'
[desktop] timed out after 900000ms

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: null because 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 a fs.watch/setTimeout shim plus process.getActiveResourcesInfo():

  1. workspacePackagesPlugin pushes the repository and workspace manifests into configFileDependencies; chokidar therefore watches each manifest file and its parent directory.
  2. The manifest edit mid-test triggers a dev-server restart whose fresh watcher keeps scanning in the background.
  3. 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-checking this.fsw.closed. When close() lands inside that await gap it sweeps a still-empty _closers list, the resumed _addToNodeFs discards 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 native fs.watch still open, and the lost closer closing it when invoked by hand.
  4. The orphaned fs.watch on the temporary repository root (FSEventWrap, plus the scan's pending FSReqCallbacks) keeps the child alive indefinitely; node --test waits for the child; the workspace burns its whole timeout budget.

--test-force-exit is 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 test job failed and Desktop e2e was skipped.

Before — local repro (docker node:24-bookworm-slim, the repo copy, node --test scripts/vite-workspace-packages.test.mjs looped 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:

[t1-after] res=["PipeWrap","PipeWrap","PipeWrap","FSEventWrap"]
[t2-after] res=["FSReqPromise","PipeWrap","PipeWrap","PipeWrap","FSEventWrap","Timeout",…]
[probe] 8s res={"PipeWrap":3,"FSEventWrap":1}
  fsWatchers=["/tmp/maka-workspace-exports-… (#10)"]

Watcher #10's creation stack is chokidar's _handleDir → _watchWithNodeFs → fs.watch on the temp repository root — the parent-directory watch for the manifest config dep, created by the restart-triggered scan and orphaned by the close() race. A deliberate fixture (tests pass, then a leaked setInterval) 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-await closed check, orphaning the fs.watch handle; 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-exit is deliberately scoped to the desktop runner here; other workspaces can adopt it if they ever show the same signature.

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

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

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@github-actions github-actions Bot added the effort/XS Under 10 readable lines label Sep 6, 2026
@orangeCatDeveloper
orangeCatDeveloper force-pushed the test/desktop-dist-force-exit branch 2 times, most recently from e4defbe to 1cb8560 Compare September 6, 2026 23:55
@orangeCatDeveloper
orangeCatDeveloper marked this pull request as ready for review September 6, 2026 23:55
@orangeCatDeveloper
orangeCatDeveloper force-pushed the test/desktop-dist-force-exit branch 2 times, most recently from afff594 to 3086f56 Compare September 7, 2026 00:29
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)

@hqhq1025 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

复核 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 Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 挂起;正文关于移除标记的声明与当前提交存在一处轻微不一致。

Comment thread apps/desktop/package.json

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 授权批准与合并。

@Astro-Han
Astro-Han merged commit 8d5ff30 into apache:main Sep 7, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/XS Under 10 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test(desktop): vite-workspace-packages child can outlive its tests and holds the dist suite until the 900s ceiling

3 participants