Repository navigation
test: wait for a timed-out body before cleanup, and retry removal where Windows retries - #561
Conversation
…re Windows retries Under Node 24 on Windows, rmSync ignores maxRetries for a locked file and throws EPERM at once, so the cleanup retries in the git and browser suites never ran. Remove their temp directories with fs.promises.rm, which does retry. Vitest runs cleanup hooks while a timed-out body is still running git or a browser. These suites now track their test bodies and wait for them before closing the node or removing a directory. Git spawns and Chromium closes slow down 4 to 30 times while the full suite runs. The suites whose tests drive them get a 60 s budget, sized from the measured worst cases and documented beside it.
|
Review attestation: ready to merge at A push to this PR makes this attestation stale; the new head needs its own review. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
If your organization's extra usage balance is empty, an organization admin can add extra usage credits at claude.ai/admin-settings/usage. If its monthly spend limit was reached, an admin can raise it on the same page. If neither applies, contact Anthropic support.
Once extra usage is available, someone with write access to this repository can comment @claude review on this pull request to trigger a review.
|
Review attestation: ready to merge at A push to this PR makes this attestation stale; the new head needs its own review. |
|
Review attestation: ready to merge at A push to this PR makes this attestation stale; the new head needs its own review. |
|
Review attestation: ready to merge at A push to this PR makes this attestation stale; the new head needs its own review. |
|
Review attestation: ready to merge at A push to this PR makes this attestation stale; the new head needs its own review. |
1 participant
Refs #535
Cause (measured on Windows 11, 28 cores, Node 24.11, git 2.55, Playwright 1.63 Chromium)
Two separate defects produce the failures in #535.
1. The
EPERMcleanup never retriedEvery suite in #535 removes its temp directory with
rmSync(dir, { maxRetries: 10, ... }), the #363 pattern. Under Node 24 on Windows,rmSyncignoresmaxRetriesfor a locked file: it throwsEPERMat once. The promise formfs.promises.rmwith the same options does retry.I tested this with a directory held as the working directory of a child process that exits after 1.5 s:
rmSync(dir, { recursive, force, maxRetries: 10, retryDelay: 100 })EPERMafter 0 msfs.promises.rm(dir, { same options })That explains the issue comment that saw
EPERM"even withmaxRetries: 10". In practice the suites never retried. A loop that launched Chromium,await context.close()d it and then ranrmSyncwith retries hitEPERMon its first pass while the full suite was running. Withfs.promises.rm, the same loop needed up to 1.6 s of retries and always succeeded.package-lifecycle-route.spec.tshad no retry at all.2. The timeouts are spawn and browser-close latency under parallel load
I measured each operation alone and during a full
vitest run, with temporary timing around the driver's launch/close and every git spawn (removed again):managed-worktree.ts)context.close()Each worktree test spawns git dozens of times (repositories, worktrees, then a sweep that asks git about each one), so a 3 s test reached 21.3 s (worktree-sweep) and 18.8 s (managed-worktree). Each browser test starts and stops a Chromium, and closing it dominates. Tests under 1 s alone took up to 10 s (driver, injection), and
session-preview-real(two browsers in one test) took up to 14 s. The default budget is 20 s.3. Cleanup raced the timed-out body
After a timeout, Vitest runs the hooks while the body is still awaiting git or the browser. The hooks then close the node's database and remove the directory under it, which is the
EPERMin the reports.What changed
tools/test-cleanup.ts(new; tests already import fromtools/, seeapps/desktop/test/appearance-tokens.spec.ts):removeTestDirectory(path):fs.promises.rmwith 10 retries at a growing delay (about 5.5 s at most), so the retry actually runs on Windows.trackedTests(budgetMs): returns anitthat records each body it starts, andsettled(), which waits for all of them. Each suite'safterEach/afterAllawaitssettled()before it closes anything or removes a directory, so cleanup never runs under a still-running body. The timed-out test still fails with Vitest's timeout error.click-answer.spec.tsalready gives its browser tests.click-answer(60 s) andsubmit-once(90 s) already carried these budgets per test, so they get onlysettled()and the retrying removal.package-lifecycle-route.spec.ts: removal with retries. Its tests stay far below 20 s (worst 7.6 s), so its budget is unchanged.The global
testTimeout/hookTimeoutand every assertion are unchanged. No product code changed.Evidence
Stress, before (
origin/mainfce8dd0) vs after, alternated. Each round ran the fullvitest runplus three concurrent runs of the 8 suites, all at once. That is heavier thanpnpm verify.EPERMEPERMAfter round 2: all four processes slowed down together in one two-minute window, with other work also running on the machine. Every browser test in that window ran 5–50× slower, and many passed at 40–58 s. Seven tests ran out of the 60 s budget,
click-answerincluded, whose 60 s budget is unchanged from main. The eighth failure was an unrelated timing assertion inscoped-fs.spec.ts(expected 2000 to be less than 2000). In one process anafterAllwaiting onsettled()hit the 20 s hook timeout. That is the intended trade: a slow body is waited for, and the directory is never removed under it. None of these failures wasEPERM.Proof that cleanup waits for a timed-out body. A throwaway spec (not committed) used a 200 ms budget and a body that runs for 1.5 s. Vitest reported
Test timed out in 200ms.afterAllsawfinished=falsebeforesettled()andfinished=trueafter it, having waited 1294 ms.Normal load. Full
vitest runonmain: 5 runs, 0 failures. Under that load the target tests took up to 21.3 s, at or over the old 20 s budget. On this branch: full runs in the stress rounds above, andpnpm verifyplus two more full runs:corepack pnpm verifyexited 0 (13/13 invariants, typecheck, lint, 524 files passed and 1 skipped, 6977 tests passed). The two full runs passed 7015/7015 with 0 failures. In those runssession-preview-realagain took 19.3 s andworktree-sweep12.4 s, so the old 20 s budget had almost no headroom.corepack pnpm typecheck: passed.corepack pnpm invariants: all 13 passed.eslinton the changed files: clean.Not changed here
rmSync(..., { maxRetries }). Under Node 24 on Windows their retries are inert too. They are not in Windows: runtime suites time out under parallel verify, then fail cleanup with EPERM #535's list, and several are touched by open PRs (service-artifact-input.spec.tsin test(runtime): write the probe's report aside and rename it into place #547, for instance), so they are left for a follow-up noted on Windows: runtime suites time out under parallel verify, then fail cleanup with EPERM #535.BrowserDriver.close()returns at once when called whilelaunchPersistentContextis still pending, and the browser that launch then opens stays running. Awaiting the body makes this unreachable in these tests. It is noted on Windows: runtime suites time out under parallel verify, then fail cleanup with EPERM #535 for the product side.net::ERR_NO_BUFFER_SPACE, which is test(ci): reference theme browser journey flakes with ERR_NO_BUFFER_SPACE on Windows #544 (PR test(ci): keep Chromium's random local ports off test loopback connects on Windows #553).Docs impact
None. Test infrastructure only.