Repository navigation
fix: report a gate-lease wait only when the run actually queued - #2375
Merged
Merged
Conversation
acquireLease now returns how long it spent looping behind a holder, and 0 when its first attempt succeeded however long that attempt took. The "acquired after" line and the "(waited … first)" suffix key off that, so an uncontended run whose lock call is slow on a loaded machine no longer claims to have waited, and the "two concurrent runs serialize" test's count is deterministic under load. runUnderLease gains an injectable base `fs`, which the new regression test uses to make an uncontended lock call outlast several polls. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012wBGpf72yP3dJvcz8Y2PbL Signed-off-by: cliffhall <cliff@futurescale.com>
There was a problem hiding this comment.
🟢 Approval recommended
The focused fix matches #2369 and includes appropriate deterministic regression coverage.
Pull request overview
Fixes gate-lease logging so only genuinely queued runs report waiting.
Changes:
- Tracks waiting only after a failed lease attempt.
- Gates acquisition/release wait messages on actual contention.
- Adds deterministic regression coverage using delayed filesystem access.
File summaries
| File | Description |
|---|---|
scripts/gate-lease.mjs |
Corrects wait tracking and adds filesystem injection. |
scripts/gate-lease.test.mjs |
Tests slow, uncontended acquisition. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Member
Author
|
Copilot review round 1: 0 comments, no suppressed findings, approval recommended — nothing to respond to, so no further round requested. Merging |
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.
Closes #2369
Problem
runUnderLeasemeasuredwaitedas the wall time of the wholeacquireLeasecall and loggedgate-lease: acquired after …(plus the(waited … first)suffix onreleased after) whenever that was>= pollMs. An uncontended run whose singleproperLockfile.lock()call was slow — easy on a machine running other gates — therefore reported a wait it never had. That turnedtwo concurrent runs serializered under load (twoacquired afterlines, expected one), and made a real gate misreport that it had queued.Fix
Measure the right thing instead of widening
pollMs(#1596):acquireLeasenow resolves{ release, waited }.waitedis the time spent looping behind a holder, and0whenever the first attempt succeeded, however long that attempt took. The existingannouncedflag is set on the first failed attempt, so it is exactly "this call looped".waited > 0instead ofwaited >= pollMs.runUnderLeasegains an injectable basefs(defaultnode:fs), which theproper-lockfileguard shim delegates to. It exists for the regression test below.Tests
New test: an uncontended run whose lock call is slow does not report a wait (#2369). Its
mkdiris delayed by 4×pollMs, so the load that caused the flake is reproduced deterministically: noacquired afterline, and areleased afterline with nowaitedsuffix.Mutation check: with the old measurement restored (
waited= whole call, gated on>= pollMs), the new test fails. With the fix, all 21 tests inscripts/gate-lease.test.mjspass.npm run local:gatepasses.🤖 Generated with Claude Code
https://claude.ai/code/session_012wBGpf72yP3dJvcz8Y2PbL