Windows: consume_wake_signal drops wakes on transient sharing violation + de-flake race test#60
Open
danielhertz1999-bit wants to merge 2 commits into
Conversation
…lation consume_wake_signal() did an unlink-to-consume inside a bare `except OSError: return False`. On Windows, unlink() raises PermissionError (sharing violation, WinError 5/32) when a concurrent writer or reader momentarily holds the signal file open -- Python's open() there does not request FILE_SHARE_DELETE. The bare handler swallowed that as "no signal", dropping a real wake and stranding the daemon in HIBERNATION with an unserved socket. Retry the unlink briefly on a Windows PermissionError (mirrors daemon_state._atomic_replace); on POSIX a PermissionError is a genuine permission fault, so it still returns False on the first failure. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Deterministic regression tests for the retry: a simulated Windows transient PermissionError is retried and consumed; a POSIX PermissionError is not retried. Both run on every platform via monkeypatch, so the fix is gated tightly rather than through the racy concurrency test. - De-flake test_consume_atomic_no_race: writers now produce continuously and the consumer polls until it consumes (bounded by a deadline) instead of a fixed iteration count. The old fixed-count consumer could finish before any writer's first replace landed (adverse scheduling on a loaded runner), reading consumed=0 -- the macOS CI failure `assert 0 >= 1`. - The writer harness retries os.replace on a Windows PermissionError, mirroring production so the writer thread can't die early and starve the consumer. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
17 tasks
Owner
|
Thanks. v2.0 (Rust rewrite) already has consume_wake_signal, so the wake-signal path carried over, I need to check whether the transient sharing-violation retry you added is in there or still missing. If it's missing it goes into v2.0.3 with the other Windows fixes. Keeping it open till I've confirmed. Nice de-flake on the race test too. |
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.
Problem
Two issues surfaced by
tests/test_wake_handler.py::test_consume_atomic_no_racefailing on macOS CI withassert 0 >= 1.1. A real Windows bug in
consume_wake_signal(). It does an unlink-to-consume inside a bare handler:On Windows,
unlink()raisesPermissionError(sharing violation, WinError 5/32) when a concurrent writer or reader momentarily holds the signal file open — Python'sopen()there doesn't requestFILE_SHARE_DELETE. The bareexcept OSErrorswallows that transient failure as "no signal," dropping a real wake and leaving the daemon stranded in HIBERNATION with an unserved socket. On POSIX this never happens (you can unlink an open file).2. The test itself is flaky.
consumed_truthy_count >= 1had no synchronization: the consumer ran a fixed 200 unlink iterations and could finish before any writer's firstreplacelanded (adverse thread scheduling on a loaded runner), readingconsumed=0. That's theassert 0 >= 1seen on macOS — independent of the Windows bug, and unfixed by a Windows-only change.Fix
consume_wake_signal: retry the unlink briefly on a WindowsPermissionError(mirrorsdaemon_state._atomic_replace). On POSIX aPermissionErroris a genuine permission fault, so it still returnsFalseon the first failure — no behavior change off Windows.test_consume_atomic_no_race: writers produce continuously and the consumer polls until it consumes (bounded by a deadline) instead of a fixed count. Keeps the atomicity intent (errors == [], no crash under concurrent write+consume) while making the>=1liveness check deterministic on any scheduler. The writer harness also retriesos.replaceon a WindowsPermissionError, mirroring production.PermissionErroris retried and consumed; a POSIXPermissionErroris not retried.Verification
wake_handlersuite green;test_consume_atomic_no_racelooped 10× locally on Windows with zero flakes. The two new deterministic tests fail against the pre-fix source (the retry path doesn't exist), confirming they gate the fix. macOS behavior is exercised by this PR's CI.