Skip to content

raft: avoid stale leadership transfer targets - #972

Merged
bootjp merged 3 commits into
mainfrom
codex/avoid-stale-raft-transfer
Jun 17, 2026
Merged

raft: avoid stale leadership transfer targets#972
bootjp merged 3 commits into
mainfrom
codex/avoid-stale-raft-transfer

Conversation

@bootjp

@bootjp bootjp commented Jun 17, 2026

Copy link
Copy Markdown
Owner

Summary

  • add raftadmin status so rollout can inspect raft log indexes
  • skip stale, unreachable, non-follower, or over-lagged leadership transfer targets and try eligible followers by lag
  • report local raft dispatch queue drops back through snapshot/unreachable report handling

Tests

  • go test ./cmd/raftadmin ./internal/raftengine/etcd
  • bash -n scripts/rolling-update.sh && git diff --check
  • go test ./adapter -run '^TestRedis_MisskeyConnectionCompatibility$' -count=1 -timeout=2m\n- go test ./... fails: adapter package timed out after 600s\n

Summary by CodeRabbit

  • New Features

    • Added status command to raftadmin CLI tool providing cluster state information
  • Improvements

    • Enhanced rolling-update script with configurable leadership transfer maximum log lag threshold
    • Improved peer failure detection by reporting dropped messages to the raft engine
  • Tests

    • Added test coverage for the new status command output
    • Added tests for message drop handling in dispatch lanes

@coderabbitai

coderabbitai Bot commented Jun 17, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@bootjp, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 5 minutes and 28 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits.

🚦 How do rate limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: f52ef576-0873-4a8e-a82f-5415bfb640c1

📥 Commits

Reviewing files that changed from the base of the PR and between 720dfdb and ee868db.

📒 Files selected for processing (1)
  • scripts/rolling-update.env.example
📝 Walkthrough

Walkthrough

Adds a status read-only command to the raftadmin CLI that calls the gRPC Status RPC and prints raft state fields. Extends the engine's dropped-dispatch path to post dispatchReport notifications to the event loop for dropped messages. Rewrites the rolling-update script's leadership-transfer candidate selection to use lag-based filtering and ranked iteration, removing the generic-transfer fallback.

Changes

raftadmin status command

Layer / File(s) Summary
status command implementation and tests
cmd/raftadmin/main.go, cmd/raftadmin/main_test.go
Registers "status" in the read-only command dispatch table, implements printStatus calling the gRPC Status RPC and printing response fields, extends CLI usage strings, and adds test assertions for status output fields and usage error messages.

Dispatch drop reporting to raft event loop

Layer / File(s) Summary
recordDroppedDispatchreportDroppedDispatch implementation
internal/raftengine/etcd/engine.go
Clarifies the postDispatchReport comment to document both producer sources, then adds reportDroppedDispatch which conditionally posts a dispatchReport (with MsgSnap-specific routing) to dispatchReportCh from within recordDroppedDispatch.
Drop-reporting tests for full lanes
internal/raftengine/etcd/dispatch_report_test.go
Two new tests pre-fill a snapshot or replication lane channel, call enqueueDispatchMessage, and assert the drop counter increment and correct dispatchReport delivery on dispatchReportCh.

Rolling-update leadership-transfer lag-based candidate selection

Layer / File(s) Summary
Config variable and parsing helpers
scripts/rolling-update.sh
Documents and defaults LEADERSHIP_TRANSFER_MAX_LOG_LAG to 10000, forwards it into the remote shell, adds extract_proto_uint for regex-based proto field parsing, and adds raft_status as a wrapper over raftadmin_text ... status.
Candidate selection and transfer execution
scripts/rolling-update.sh
Rewrites choose_transfer_candidate to read leader indexes, filter healthy followers not behind the leader snapshot, compute per-candidate lag, filter by LEADERSHIP_TRANSFER_MAX_LOG_LAG, and sort by lag/ordinal; updates ensure_not_leader_before_restart to iterate the ranked candidates with retries and abort on exhaustion without falling back to the generic leadership-transfer RPC.

Sequence Diagram(s)

sequenceDiagram
  participant Shell as rolling-update.sh
  participant raftadmin as raftadmin_text / raft_status
  participant LeaderNode as Leader Node gRPC
  participant FollowerNode as Follower Node gRPC

  Shell->>raftadmin: raft_status(leader_addr)
  raftadmin->>LeaderNode: Status RPC
  LeaderNode-->>raftadmin: commit_index, last_log_index, snapshot_index, peer list
  raftadmin-->>Shell: parsed status output
  Shell->>Shell: choose_transfer_candidate: filter/rank peers by log lag
  loop each candidate in ranked order
    Shell->>FollowerNode: LeaderTransfer(candidate_id)
    alt transfer succeeds
      FollowerNode-->>Shell: ok
      Shell->>Shell: proceed with restart
    else transfer fails
      Shell->>Shell: try next candidate
    end
  end
  Shell->>Shell: all candidates failed → abort restart
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Possibly related PRs

  • bootjp/elastickv#532: Extends raftengine Status with LeadTransferee for leadership-transfer state detection, directly affecting the status payload consumed by the new raftadmin status command added in this PR.
  • bootjp/elastickv#535: Modifies the same engine.go dispatch-report flow (dispatchReport/dispatchReportCh) for dropped/unreachable MsgSnap vs other messages, which this PR extends further in recordDroppedDispatch.
  • bootjp/elastickv#577: Modifies per-lane dispatcher drop behavior (enqueueDispatchMessage/dispatchReport) in engine.go, aligning directly with the new lane-full drop-reporting tests added in dispatch_report_test.go here.

Poem

🐇 Hoppy the rabbit taps the status key,
"state: Follower, commit_index: three!"
Dropped messages now whisper, "peer's gone away,"
The rolling shell ranks candidates by lag today.
No fallback remains when all transfers fail neat—
just abort and hop back, with paws on the beat! 🎵

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'raft: avoid stale leadership transfer targets' directly and specifically describes the main objective of the pull request, which is to prevent stale or unsuitable nodes from being selected as leadership transfer targets.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/avoid-stale-raft-transfer

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

Comment @coderabbitai help to get the list of available commands and usage tips.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new status command to the raftadmin CLI to query detailed Raft status, and updates the etcd raft engine to report dropped dispatch messages to the event loop. Additionally, the rolling-update.sh script is updated to leverage this new status command to select leadership transfer candidates based on log lag, avoiding stale peers. The review feedback highlights two issues in the shell script: first, the error output is omitted when the leader status check fails, and second, a failed status check on followers is masked by || true and incorrectly parsed, which could lead to misleading logs.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread scripts/rolling-update.sh
Comment thread scripts/rolling-update.sh Outdated
@bootjp

bootjp commented Jun 17, 2026

Copy link
Copy Markdown
Owner Author

@codex review

Addressed Gemini findings on scripts/rolling-update.sh:

  • include captured leader raft_status failure output in the leader status error
  • stop masking follower raft_status failures with || true, and log the captured RPC error directly

Validation after the fix:

  • bash -n scripts/rolling-update.sh && git diff --check
  • go test ./cmd/raftadmin ./internal/raftengine/etcd

@bootjp
bootjp marked this pull request as ready for review June 17, 2026 17:30
@bootjp

bootjp commented Jun 17, 2026

Copy link
Copy Markdown
Owner Author

@codex review

PR is now ready for review. Gemini threads were fixed in 720dfdbe and resolved. Current local validation:

  • bash -n scripts/rolling-update.sh && git diff --check
  • go test ./cmd/raftadmin ./internal/raftengine/etcd

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Bravo.

Reviewed commit: 720dfdbeba

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

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".

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@scripts/rolling-update.sh`:
- Around line 979-981: Update the documentation in rolling-update.env.example
(around lines 76-81) to reflect that the generic transfer fallback behavior has
been removed. The current documentation still mentions "falling back to generic
transfer" as part of the retry behavior, but this is no longer accurate since
the code now refuses generic transfer and returns with error status when all
targeted candidates are exhausted. Modify the documentation to clearly state
that the restart will abort if all targeted leadership transfer attempts fail,
removing any reference to the fallback behavior.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 73fb569d-e6af-46e8-9d76-fb7f0026636e

📥 Commits

Reviewing files that changed from the base of the PR and between 40d661f and 720dfdb.

📒 Files selected for processing (5)
  • cmd/raftadmin/main.go
  • cmd/raftadmin/main_test.go
  • internal/raftengine/etcd/dispatch_report_test.go
  • internal/raftengine/etcd/engine.go
  • scripts/rolling-update.sh

Comment thread scripts/rolling-update.sh
@bootjp

bootjp commented Jun 17, 2026

Copy link
Copy Markdown
Owner Author

@codex review

Addressed CodeRabbit docs feedback in ee868db2: updated scripts/rolling-update.env.example so the retry docs say all eligible targeted candidates failing aborts the rollout instead of falling back to generic transfer.

Validation:

  • git diff --check

@bootjp
bootjp enabled auto-merge June 17, 2026 18:26
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. More of your lovely PRs please.

Reviewed commit: ee868db21e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

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".

@bootjp
bootjp merged commit ddf29af into main Jun 17, 2026
13 checks passed
@bootjp
bootjp deleted the codex/avoid-stale-raft-transfer branch June 17, 2026 18:35
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