Skip to content

test(collect): stop the single-flight test failing on slow runners - #273

Closed
GeiserX wants to merge 1 commit into
mainfrom
test/single-flight-deterministic
Closed

GeiserX wants to merge 1 commit into
mainfrom
test/single-flight-deterministic

Conversation

@GeiserX

@GeiserX GeiserX commented Nov 8, 2026

Copy link
Copy Markdown
Owner

TestCollectAllSingleFlight failed on #272's macOS race run with "total collects = 6, want 3", on a PR that changed no Go code. The test started 8 goroutines calling collectAll and relied on a 2 ms sleep per collect to keep the first run open. On a slow runner one goroutine started only after that run had finished. That is a legitimate second run, because the guard rejects overlapping runs and not later ones.

The fake collector now has a gate channel that parks every collect. The test closes it only after the other 7 calls have returned. A call can only return while the gate is shut by bailing on the guard, so at that point exactly one run is open. The timing guess is gone. The unused hold sleep is removed.

Checked on a separate machine:

  • go test -race -count=50 -run 'TestCollectAllSingleFlight$' . passes.
  • Negative control: with the guard's CompareAndSwap disabled, the test fails with "single-flight violated: only 0 of 7 calls bailed while one run was in progress".
  • go test -race . and go vet . pass.

The test raced 8 goroutines against a 2 ms sleep. On a slow macOS race
runner a call could start after the first run had finished, a legitimate
second run, and the test failed with 6 collects instead of 3. The fake
collector now parks on a gate that the test closes only after the other
7 calls have returned, so exactly one run is open while they try.
@coderabbitai

coderabbitai Bot commented Nov 8, 2026

Copy link
Copy Markdown

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 23 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

Commits

Reviewing files that changed from the base of the PR and between 6115c5f and f26bcf6.

Files selected for processing: 1

  • app_test.go
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

@GeiserX

GeiserX commented Nov 8, 2026

Copy link
Copy Markdown
Owner Author

Held so it ships in the same release as the proxied-copy device ID fix: auto-tag runs on any .go change, so merging this test-only change alone would cut a release with nothing user-facing in it. Its commit is carried into that PR.

@GeiserX

GeiserX commented Nov 8, 2026

Copy link
Copy Markdown
Owner Author

This commit shipped in #275 (merged as 1dfe1e4), together with the device-registration retry, so it didn't cut a release of its own.

@GeiserX GeiserX closed this Nov 8, 2026
@GeiserX
GeiserX deleted the test/single-flight-deterministic branch November 8, 2026 02:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant