Align Windows _jobs_lock timeout with POSIX polling pattern - #62763
Open
rahulrao85 wants to merge 2 commits into
Open
Align Windows _jobs_lock timeout with POSIX polling pattern#62763rahulrao85 wants to merge 2 commits into
rahulrao85 wants to merge 2 commits into
Conversation
rahulrao85
force-pushed
the
fix/windows-jobs-lock-timeout
branch
from
July 11, 2026 17:35
d307dfd to
da1ff1d
Compare
teknium1
reviewed
Jul 11, 2026
teknium1
left a comment
teknium1
left a comment
Contributor
There was a problem hiding this comment.
Thanks for carrying the #60703 bounded-lock guarantee over to Windows. Current main still has the blocking LK_LOCK call at cron/jobs.py:226; the POSIX polling implementation was added by 7ecc822e1 and the Windows branch was not included.
Problems
- There is no regression coverage for the new Windows path.
tests/cron/test_ticker_stall_60703.py:45skips the whole bounded-lock module whenfcntlis unavailable, and this PR changes onlycron/jobs.py.
Suggested changes
- Add a platform-independent mocked-
msvcrttest for both successfulLK_NBLCKacquisition and deadline degradation (ERROR log plus critical-section entry).
Automated hermes-sweeper review.
| @@ -292,7 +292,29 @@ def _jobs_lock(): | |||
| break | |||
| time.sleep(0.1) | |||
| elif msvcrt is not None: | |||
| getattr(msvcrt, "locking")(lock_fd.fileno(), getattr(msvcrt, "LK_LOCK"), 1) | |||
| _deadline = time.monotonic() + _JOBS_LOCK_TIMEOUT_SECONDS | |||
Contributor
There was a problem hiding this comment.
Please add regression coverage for this LK_NBLCK path. The existing bounded-lock tests are module-skipped without fcntl (tests/cron/test_ticker_stall_60703.py:45), so they do not cover Windows. A fake msvcrt can verify retry-to-deadline, ERROR logging, degraded critical-section entry, and an uncontended acquisition without requiring Windows CI.
alt-glitch
added
type/bug
Author
|
Tests added as requested — \ ests/cron/test_windows_jobs_lock.py\ with a fake msvcrt module covering three scenarios:
All 3 tests pass locally. |
teknium1
added
the
sweeper:blast-moderate
The POSIX (fcntl) path polls LOCK_NB for 30 seconds with explicit timeout logging before degrading to in-process-only locking (fix for issue NousResearch#60703). The Windows (msvcrt) path used a blocking LK_LOCK with ~10s internal retry and a generic warning on failure. Replace LK_LOCK with LK_NBLCK in a polling loop matching the POSIX pattern (same 30-second _JOBS_LOCK_TIMEOUT_SECONDS, same degradation behavior, same ERROR-level log message). This ensures consistent timeout handling across platforms and gives the Windows path the same bounded contention resilience that POSIX has. Signed-off-by: Rahul Rao <rahulrao85@gmail.com>
The bounded-lock tests in test_ticker_stall_60703.py are module-skipped when fcntl is unavailable, so they never cover the Windows msvcrt path. Add a new test file with a fake msvcrt module that exercises LK_NBLCK polling on any platform: - test_lock_times_out_and_degrades: always-fail mock verifies 30-second bounded timeout, ERROR logging, and degraded critical-section entry. - test_uncontended_lock_is_fast_and_silent: immediate LK_NBLCK success. - test_lock_recovers_after_transient_contention: 1 failure then success must not trigger the timeout path. Signed-off-by: Rahul Rao <rahulrao85@gmail.com>
rahulrao85
force-pushed
the
fix/windows-jobs-lock-timeout
branch
from
July 25, 2026 16:49
d166690 to
b828a04
Compare
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
3 participants
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.
Summary
The POSIX (fcntl) path in _jobs_lock()\ polls LOCK_NB with a 30-second bounded timeout before gracefully degrading to in-process-only locking (fix for issue #60703 — P1: cron silently dies on lock contention). The Windows (msvcrt) path used a blocking LK_LOCK call with ~10s internal retry and a generic WARNING log on failure.
This change replaces LK_LOCK with LK_NBLCK in a polling loop matching the POSIX pattern: same 30-second _JOBS_LOCK_TIMEOUT_SECONDS, same ERROR-level timeout log, same \lock_fd.close()\ degradation.
Changes
\cron/jobs.py:294-295\ → replaced single blocking LK_LOCK call with a polling loop using LK_NBLCK:
Related