Merged
Conversation
Contributor
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Contributor
Author
|
@bgentry Again, kind of hoping that we can rely on Claude/Codex to cross reference their work and root out any tricky concurrency problems here. |
bgentry
approved these changes
Apr 13, 2026
Contributor
bgentry
left a comment
bgentry
left a comment
There was a problem hiding this comment.
Good fix. Not something we can reasonably add a test for, right?
Aims to fix another intermittent test failure: https://github.com/riverqueue/river/actions/runs/24312716933/job/70985012980?pr=1205 --- FAIL: TestElector_WithNotifier (0.00s) --- FAIL: TestElector_WithNotifier/RequestResignStress (20.41s) elector_test.go:350: Generated postgres schema "leadership_2026_04_12t17_52_43_schema_04" with migrations [1 2 3 4 5 6] on line "main" in 229.626141ms [4 generated] [0 reused] elector_test.go:352: Starting test_client_id elector_test.go:375: Requesting leadership resign elector_test.go:375: Requesting leadership resign elector_test.go:375: Requesting leadership resign elector_test.go:375: Requesting leadership resign elector_test.go:375: Requesting leadership resign test_signal.go:95: timed out waiting on test signal after 10s test_signal.go:95: timed out waiting on test signal after 10s riverdbtest.go:293: Checked in postgres schema "leadership_2026_04_12t17_52_43_schema_04"; 1 idle schema(s) [5 generated] [3 reused] FAIL FAIL github.com/riverqueue/river/internal/leadership 20.948s The problem is that when `requestResignChan` is unbuffered, if `keepLeadershipLoop` hasn't yet entered its `select`, then the `default` statement on the `select` below will cause all senders (we have 5 competing senders in the `RequestResignStress` test case) to fall through without sending anything: select { case <-ctx.Done(): case e.requestResignChan <- struct{}{}: default: // if context is not done and requestResignChan has an item in it // already, do nothing } By making the channel buffered, we guarantee at least one sender gets a message through, and we don't end up hanging the test.
brandur
force-pushed
the
brandur-buffered-request-resign-chan
branch
from
cc43053 to
bedab2f
Compare
Contributor
Author
Yeah, actually turns out we can write a test for this! Added. |
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
2 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.
Aims to fix another intermittent test failure:
https://github.com/riverqueue/river/actions/runs/24312716933/job/70985012980?pr=1205
The problem is that when
requestResignChanis unbuffered, ifkeepLeadershipLoophasn't yet entered itsselect, then thedefaultstatement on the
selectbelow will cause all senders (we have 5competing senders in the
RequestResignStresstest case) to fallthrough without sending anything:
By making the channel buffered, we guarantee at least one sender gets a
message through, and we don't end up hanging the test.