Conversation
A gain callback that errors left the replica holding the leader advisory lock on a pooled session with is_leader still true, so no other replica could take over. Now a failed gain runs the lose callback, unlocks, and returns the connection; a ping failure closes the session instead of handing a possibly-still-locked connection back to the pool; and shutdown unlocks explicitly. The covering test is driven deterministically: the election cadence is a stream the test feeds tick by tick instead of a paused tokio clock, and the unlock is observed with a blocking pg_advisory_lock that PostgreSQL grants as part of the leader session's release. The previous paused-clock version was #[ignore]d after hanging on three consecutive CI runs. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Deploying control-layer with
|
| Latest commit: |
4e10e5a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://eb233c2d.control-layer.pages.dev |
| Branch Preview URL: | https://peter-leader-election-retry.control-layer.pages.dev |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The keepalive-failure path releases the advisory lock before running on_lose_leadership, which can permit overlapping “leader” work, and there is a redundant drop(conn) likely to trip Clippy.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes a leader-election failure mode in dwctl where an instance could retain a PostgreSQL advisory lock after failing to fully assume leadership, preventing other replicas from taking over. It also refactors the election loop to accept an externally-driven tick stream so the leader-election test can be deterministic in CI.
Changes:
- Added explicit advisory-lock cleanup (
release_leader_connection) and invoked it on failed gain, keepalive failure, and shutdown paths. - Refactored the election loop into
run_leader_election(Stream<Item = ()>)while keepingleader_election_task’s public signature (using a 30s interval stream in production). - Reworked the leader-election test to drive election ticks via an mpsc channel and validate unlock behavior with a blocking
pg_advisory_lockobservation.
File summaries
| File | Description |
|---|---|
| dwctl/src/leader_election.rs | Ensures advisory locks are released on failure/shutdown and makes election cadence injectable for deterministic testing. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
214
to
+218
| is_leader.store(false, Ordering::Relaxed); | ||
| leader_conn = None; | ||
| // The ping failing does not prove the session is dead: a | ||
| // statement timeout leaves it alive and still holding the | ||
| // advisory lock. Never return such a connection to the pool. | ||
| release_leader_connection(&mut leader_conn, lock_id).await; |
Comment on lines
+55
to
+56
| drop(conn); | ||
| } |
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.
Summary
Extracts the leader-election fix from #1481 into its own change and reworks its test so it is deterministic in CI.
Production change (
dwctl/src/leader_election.rs):on_gain_leadershipnow clearsis_leader, runson_lose_leadership, and releases the advisory lock viarelease_leader_connection. Previously the replica kept the lock on a pooled session with the flag set, so no other replica could take over.run_leader_election, which takes the election cadence as aStream<Item = ()>.leader_election_taskkeeps its signature and feeds it the same 30 s interval.Test rework (
failed_gain_cleans_up_unlocks_and_retries):tokio::time::pause()/advance. The test feeds election ticks over an mpsc channel, so the retry cannot happen until the test asks for it.pg_advisory_lockon a separate session, which PostgreSQL grants as part of the leader session's release. No polling against a real database under a fake clock.#[ignore]removed.Verification
cargo testandcargo nextest), ~0.15 s each.failed gain must release its advisory lock: Elapsedinside the wait budget, not hang.🤖 Generated with Claude Code