Repository navigation
test: the start lock promises mutual exclusion, not one winner ever - #284
Merged
Merged
Conversation
The Windows job on the v3.6.0 tag failed with "2 callers won the start race, want exactly 1", and the lock was right — the test was wrong. Every goroutine released after holding for 5ms, inside the goroutine. A caller scheduled after that release then acquires the lock legitimately, so "two callers won" was two callers winning in SEQUENCE. Windows found it because its scheduling spreads ten goroutines over more than five milliseconds; macOS and Linux happened to fit inside the window, which is why this passed on two PRs and failed on the tag. What the lock promises is that one caller holds at a time, not that only one ever succeeds across time. Every winner now holds until all ten have tried, and the count is taken then — which is the promise, and which still fails if O_EXCL is dropped. The lock is also asserted reusable after release, since holding to the end would otherwise hide a release that does not work. No production code changed. docs: none — test-only change
There was a problem hiding this comment.
🟢 Approval recommended
The change is isolated to tests and correctly aligns the assertion with the lock’s mutual-exclusion semantics while retaining a strong regression trigger (O_EXCL removal).
Pull request overview
This PR fixes a flaky Windows-only failure by correcting TestStartLockUnderRace to assert the lock’s actual contract: mutual exclusion (only one holder at a time), rather than “exactly one winner ever” across time/scheduling.
Changes:
- Updates the race test to keep the winning lock held until all goroutines have attempted acquisition, preventing legitimate sequential acquisitions from being counted as failures.
- Adds a post-release assertion that the lock is reusable after being released.
- Expands the test’s comment and “proved by” note to document the prior failure mode and the intended property.
File summaries
| File | Description |
|---|---|
| internal/api/start_test.go | Reworks TestStartLockUnderRace to correctly validate start-lock mutual exclusion and reusability under concurrent attempts. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
The Windows job on the
v3.6.0tag failed with2 callers won the start race, want exactly 1. The lock is right; the test was wrong.Every goroutine released after holding 5ms, inside the goroutine. A caller scheduled after that release then acquires the lock legitimately — so "two callers won" was two callers winning in sequence, which is correct behaviour counted as a failure.
Windows found it because its scheduling spreads ten goroutines over more than the five milliseconds the winner held. macOS and Linux happened to fit inside that window, which is why this passed on #272 and #282 and only failed on the tag.
The fix
Every winner holds until all ten have tried; the count is taken then. That is the property the lock actually promises — one holder at a time — and it still fails if
O_EXCLis dropped. The lock is also asserted reusable after release, so holding to the end cannot hide a broken release.No production code changed. 30 consecutive runs green, plus
-race.