refactor leader election around DB-issued "terms"#1213
Open
refactor leader election around DB-issued "terms"#1213
Conversation
c9df93e to
88d9ea3
Compare
88d9ea3 to
28a0c9f
Compare
Contributor
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! ℹ️ 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". |
23fa8fd to
fce9b47
Compare
Contributor
Author
|
@brandur took a whole bunch of turns and iteration to get this to where I want it but I think it's pretty solid now! Pared back a lot of the added complexity from earlier just by going all in on "DB is source of truth including for time". |
fce9b47 to
bddcf95
Compare
bddcf95 to
e35cdb6
Compare
The old elector treated leadership as a lease held by one client ID and renewed over time. That was simple, but it left too much implicit. One leadership term was not clearly separated from the next, so reelection and resignation were not scoped to a specific term. That made same- client reacquisition harder to reason about, made it easy for stale work or cleanup to target the wrong lease, and left the elector carrying more responsibility for edge cases than it should have. This change makes the database issue explicit leadership terms using the columns we already have. `leader_id` remains the stable client ID, while `(leader_id, elected_at)` identifies one specific term. Elect, reelect, and resign now all operate on that exact term and return the leader row from the database. The elector keeps a bounded local trust window for its last successful confirmation, but that window is anchored to the attempt that produced it, not to when the response happened to arrive. That keeps slow successful reelections from stretching leadership past its real lease budget while still avoiding direct app-vs-database clock comparisons in the state machine. The notification and test story is also clearer after the rewrite. Slow subscribers now receive each leadership transition in order without blocking the elector, resignation wakeups are coalesced safely, and the poll-only coverage uses isolated fixtures so it can exercise real handoff behavior without shared-schema flakiness. The shared driver suite now covers term-scoped elect, reelect, and resign behavior across PostgreSQL and both SQLite backends, including same-client term replacement and stale-term rejection. The elector tests focus on the observable behaviors that matter: gaining leadership, handing it off, responding to resign requests, and stepping down cleanly when its trust window expires. This also rolls up the branch's earlier flake investigation and keeps the original CI reference for the shared-schema failures that led to the redesign: https://github.com/riverqueue/river/actions/runs/24406465152
e35cdb6 to
94f94fa
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
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 old elector treated leadership as a lease held by one client ID and renewed over time. That kept the happy path simple, but it left too much implicit. One leadership term was not clearly separated from the next, so reelection and resignation were not scoped to a specific term. That made same-client reacquisition and stale cleanup harder to reason about than they needed to be.
This rewrite makes the database issue explicit leadership terms using the columns we already have.
leader_idstill identifies the client, while(leader_id, elected_at)identifies one specific term. Elect, reelect, and resign now all operate on that exact term and return the leader row directly from the database.The elector still keeps a bounded local trust window for its last successful confirmation, but that window is now anchored to the attempt that produced it rather than to when a response happened to arrive. That keeps slow successful reelections from stretching leadership past the real lease budget, while still avoiding direct app-vs-database clock comparisons in the state machine.
This branch also adds an explicit
Time.NowAPI at the base-service layer. The old shared API only exposedNowUTC, even for code doing local deadline and duration math. For the elector that was the wrong abstraction, because these comparisons need to preserve Go's monotonictime.Timereading rather than normalize through UTC. Making that API explicit lets the elector use the right local clock for trust-window calculations while keepingNowUTCOrNilfor the existing test path that passes stubbed wall-clock timestamps through to database code.The tests were rewritten around the final model instead of the intermediate mechanics. The shared driver suite now covers term-scoped elect, reelect, and resign behavior across PostgreSQL and both SQLite backends, including same-client term replacement and stale-term rejection. The elector tests focus on observable behavior such as leadership handoff, resign requests, ordered subscriber notifications, and stepping down cleanly when the trust window expires.
This also rolls up the branch's earlier flake investigation and keeps the original CI reference for the shared-schema failures that led to the redesign:
https://github.com/riverqueue/river/actions/runs/24406465152