test: release the handles a temp project holds before removing it (Windows EPERM) - #1723
test: release the handles a temp project holds before removing it (Windows EPERM)#1723bompus wants to merge 2 commits into
Conversation
These tests build a CodeGraph over a temp project and never close it, so the SQLite handle is still open when the directory is removed. POSIX unlinks an open file happily; Windows refuses, and the removal — in a finally or an afterEach, after the assertions have passed — throws EPERM and fails the test. 14 tests across the three files, every one of them green on the assertions themselves. Closed in the teardown rather than at the end of the test body, so a failing assertion cannot skip it and turn one red into two. resolution.test.ts also opens a second connection of its own for a raw edge query; that one is hoisted out of the try and closed the same way.
A signalled process still holds its file handles until it is actually gone, and these suites SIGKILL the spawned MCP server and then remove the directory synchronously — on Windows that is EPERM, and the test fails after its assertions passed. Await the exit instead of only sending the signal; mcp-daemon had a fixed 50ms grace period for this, which is now the same await plus the file's own waitProcessExit for the detached daemon it reaps by pid. Awaiting the tracked children is not sufficient on its own: the server may leave a detached daemon this suite never sees, and mcp-daemon's concurrent-launcher test deliberately leaves one alive. So the removals also retry, which is what fs.rmSync's maxRetries is for — 1s of retries in the three proxy suites, 6s in mcp-daemon where the daemon is the point of the test. Full suite on Windows: 4,017 passing, 0 failing, twice in a row; before this and the previous commit, 24 failing.
|
Duplicate of #1717, which I opened this morning and did not check before filing this. #1717 is the better patch and supersedes this one: same close-in-teardown for arkts-resolution / frameworks-integration / resolution, same await-the-exit for the mcp-* suites, plus a shared The one thing worth carrying across: I reached the same 'awaiting the tracked child is necessary but not sufficient' conclusion independently here, from a run that still failed 2 of 4,240 with a different pair each time, which is the same detached-daemon case #1717 documents. |
1 participant
Fixes #1722.
24 tests across 7 files fail on Windows, every one of them after its assertions have passed, in the
fs.rmSyncthat removes the temp project. Windows will not remove a directory holding an open file; POSIX unlinks one happily, which is why CI is green. Nothing about the indexer is implicated — this is teardown only.The two causes, fixed separately
fb89ac5— the graph is never closed.arkts-resolution.test.tscallsCodeGraph.initSync7 times andclose()zero times; three of the four JVM tests inframeworks-integration.test.tsomit thecg.close()their two neighbours already have; the C++/PHP end-to-end tests inresolution.test.tsalso open a secondDatabaseConnectionfor a raw edge query and close neither.Closed in the teardown, not at the end of the test body, so a failing assertion cannot skip the close and turn one red into two. In
arkts-resolution.test.tsthat means alet cgper describe and acg?.close()in the existingafterEach; inresolution.test.tsthe test's own connection is hoisted out of thetryand closed in thefinallybeside the graph.frameworks-integration.test.tskeeps the end-of-bodycg.close()its passing neighbours use, since that is the file's own convention.476b229— a killed child still holds its handles. The fourmcp-*suites do close the graph and dochild.kill('SIGKILL'), thenrmSyncsynchronously in the same tick; the process has been signalled, not reaped. They now await the actualexit.mcp-daemon.test.tshad a fixedsetTimeout(50)standing in for this, now replaced by the same await plus the file's ownwaitProcessExitfor the detached daemon it reaps by pid.Awaiting the tracked child is necessary but not sufficient, and I have the failed run that shows it: with only the awaits, a full suite still failed 2 of 4,240 — a different pair each time — because
serve --mcpcan leave a detached daemon the suite never tracks, andmcp-daemon's concurrent-launcher test deliberately leaves one alive and asserts it is alive. So the removals also retry, which is whatfs.rmSync'smaxRetriesexists for: 1s in the three proxy suites, 6s inmcp-daemonwhere the live daemon is the point of the test.Measured
The diagnosis is an A/B, not a reading: adding
cg.close()toarkts-resolution.test.ts'sohpm main entrytest turns it green, removing that one line turns it red with EPERM again, same file and command back to back.Run twice in a row after the change, both green, because one green run does not close a race. The 7 affected files alone: 214 passing / 24 failing → 238 passing.
tsc --noEmitclean.Built tree in both arms (
tsc+ assets + theuiworkspace), Windows 11, Node 22. I did not reproduce a Linux run; on POSIX these teardowns cannot fail this way, and the change is inert there apart from the awaits.