Repository navigation
Fix Windows file-retry deadlocks under mocked timers - #801
Merged
Merged
Conversation
The group and recovery tests mock setTimeout while exercising real file I/O. A transient Windows rename error then waits on a fake retry timer, so state loading or store disposal never completes.\n\nUse the pre-imported promise timer for filesystem backoff without changing retries, delays or lock checks. Cover a synthetic Windows sharing violation under frozen application timers; release the old wait before asserting so a regression fails rather than hanging.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue
Fix the intermittent Windows unit-test hang seen in runs 41283258046, 41303237378 and 41307553093. There is no separate issue for this CI failure.
What changed / 改了什么
Before: a real Windows filesystem retry could wait forever on a frozen test clock.
setTimeout.replaceFile()also used globalsetTimeoutfor its 40ms filesystem retry afterEPERM,EACCESorEBUSY.state.json, the save awaited a fake timer that nobody advanced. Loading the fixture or disposing its store then awaited that save. This is a clock deadlock, not a leaked shell, watcher or socket.Now: import the promise-based timer before the tests freeze application timers and use it for the real filesystem backoff. Retry counts, delays, lock checks, application scheduling and disposal behavior are unchanged. Add a regression that injects one Windows
EPERMunder frozen timers, checks that replacement completes without a clock tick, and releases the old implementation on failure so the regression reports an assertion instead of hanging.Log evidence
The cancelled logs did not finish every test: they contain neither the remaining files nor the final test totals. The spec reporter presents results in file order.
follow-through.test.jspasses “a delivery that fails is not recorded as delivered…” and logs the expected delivery error from the next test. That next test, “a reply that fails to reach the asker…”, never reports a result. Line 583 is cancellation at 08:37:16.group-work.test.jsresult. Lines 2247–2254 report 1351 passed, 1 skipped and completion in 28.2 seconds.group-status.test.jstest passes. No result from the next file,group-work.test.js, appears; line 642 is cancellation at 12:30:46.Reproduction
EPERMat the actual state-file rename while the firstgroup-work.test.jstest has frozensetTimeout. Before the fix it times out atstore.save -> replaceFile; after the fix the same test passes in about 75ms.PipeWrapresources. The unresolved wait is the mocked 40ms timeout, not an OS timer handle. Inspecting active native handles alone would miss it.npm testwithout a per-test timeout (see log line 160). Local validation used the requested--test-timeout=120000. No force-exit, timeout, retry or workflow changes are included.Changelog
none — prevent Windows unit tests from deadlocking when a real file retry meets a frozen test clock.
How I checked it / 怎么验证的
node --import ./test/harness-resolve.mjs --test --test-timeout=120000 "test/*.test.js"— 1431 passed, 2 skipped, 0 failed (run twice).node scripts/build-client.mjs --checknpm run lint— 0 errors; 39 existing warnings.--checkand lint also passed. The frozen-clock regression passes at log line 555 in 45.3ms; final totals are at lines 2339–2346.Not verified: manually inducing an actual Windows file-sharing violation; the cancelled historical processes and their original busy error codes are no longer available. The busy-error path was exercised deterministically on Mac and native Windows CI. A single green native run alone cannot rule out every intermittent failure; the before/after deterministic regression is the evidence for this deadlock fix. Desktop installers were not manually checked (there are no interface or packaging changes).
Checklist / 检查
Co-authored-by.