Skip to content

[DB-2219]: Optimized KPlane leader resolution - #5733

Open
sakno wants to merge 4 commits into
masterfrom
sakno/fix-kplane-address-selection
Open

sakno wants to merge 4 commits into
masterfrom
sakno/fix-kplane-address-selection

Conversation

@sakno

@sakno sakno commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Changed: Optimized KPlane leader resolution

@sakno sakno self-assigned this Sep 17, 2026
@sakno
sakno requested a review from a team as a code owner September 17, 2026 11:44
@linear-code

linear-code Bot commented Sep 17, 2026

Copy link
Copy Markdown

DB-2219

@qodo-code-review

qodo-code-review Bot commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

[DB-2219] Optimize KPlane leader resolution

🐞 Bug fix ✨ Enhancement 🕐 20-40 Minutes

Grey Divider

AI Description

• Prioritizes announced KPlane leaders in the shared endpoint list.
• Atomically switches value-equivalent endpoints without overwriting concurrent leader changes.
• Releases stale channels only when the effective endpoint changes.
Diagram

graph TD
  A["Announce stream"] --> B{"Leader announced?"}
  B -->|Yes| C["Select leader"] --> D["Leader-first list"]
  B -->|No| D
  D -->|Redirected| E["Reconnect stream"]
  D -->|Stable| F["Yield cluster"]
  D --> G["Leader RPCs"] -->|Failure or redirect| H["Atomic failover"] -->|Retry| G
Loading
High-Level Assessment

The scoped lock-free approach fits the existing client design: it fixes value-equivalent endpoint replacement while preserving concurrent updates and prioritizes the leader without introducing broader state management. Lock-based synchronization or a unified immutable routing-state object could simplify reasoning, but would add coordination overhead and require a substantially wider refactor for little benefit here.

Files changed (2) +37 / -8

Enhancement (1) +21 / -2
GrpcKontrolPlaneClient.csPrioritize the resolved leader in Kontrol Plane topology +21/-2

Prioritize the resolved leader in Kontrol Plane topology

• Processes leader redirects before rebuilding the shared node list, ensuring the resolved leader occupies the first position and is not duplicated. The updated topology is retained before reconnecting, and a file-scoped endpoint inequality helper is introduced.

src/KurrentDB.DataPlane/KontrolPlane/Transport/Grpc/GrpcKontrolPlaneClient.cs

Bug fix (1) +16 / -6
GrpcKontrolPlaneClient.Utils.csMake endpoint switching value-aware and concurrency-safe +16/-6

Make endpoint switching value-aware and concurrency-safe

• Reworks address replacement into a compare-and-swap loop that can replace value-equivalent endpoint instances while preserving competing updates. Uses explicit endpoint equality when deciding whether to release cached channels and when locating the next node.

src/KurrentDB.DataPlane/KontrolPlane/Transport/Grpc/GrpcKontrolPlaneClient.Utils.cs

@qodo-code-review

qodo-code-review Bot commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Action required

1. Failover retries the same dead node ✓ Resolved 🐞 Bug ☼ Reliability
Description
GetKontrollerNodes reads addresses[0] on every loop iteration, producing only the leader or
repeated copies of the first advertised endpoint instead of all members. When that endpoint fails,
NextAddress cycles through the malformed list and the appointment operations repeatedly retry an
unavailable node rather than reaching another controller.
Code

src/KurrentDB.DataPlane/KontrolPlane/Transport/Grpc/GrpcKontrolPlaneClient.cs[84]

+				var address = addresses[0].ToEndPoint();
Relevance

●●● Strong

Deterministic indexing bug: every iteration converts addresses[0], omitting all subsequent failover
endpoints.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The helper iterates over the full address count but always converts element zero, while
NextAddress advances only through the resulting _kontrollerNodes list. The server explicitly
populates responses from every controller node, so omitting all elements after the first discards
real failover targets.

src/KurrentDB.DataPlane/KontrolPlane/Transport/Grpc/GrpcKontrolPlaneClient.cs[79-89]
src/KurrentDB.DataPlane/KontrolPlane/Transport/Grpc/GrpcKontrolPlaneClient.Utils.cs[46-68]
src/KurrentDB.KontrolPlane/KontrolPlane/Transport/Grpc/GrpcKontrollerServer.cs[103-108]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`GetKontrollerNodes` reads index zero during every loop iteration, so peer Kontrol Plane nodes are omitted and failover can retry the same unavailable endpoint indefinitely.

## Fix Focus Areas
- src/KurrentDB.DataPlane/KontrolPlane/Transport/Grpc/GrpcKontrolPlaneClient.cs[79-89]

## Recommended Fix
Convert `addresses[i]` on each iteration instead of `addresses[0]`, retaining the existing leader-first ordering and leader de-duplication. Add coverage proving that multiple distinct advertised endpoints remain available in their expected failover order.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Failover uses a stale node list ✓ Resolved 🐞 Bug ☼ Reliability
Description
AnnounceNodeAsync now assigns _kontrollerNodes only inside the non-empty leader redirect branch,
replacing the previous unconditional assignment from every response. Normal streaming responses
carry an empty leader and a populated membership list, so topology changes remain absent from every
later failover until some redirect occurs.
Code

src/KurrentDB.DataPlane/KontrolPlane/Transport/Grpc/GrpcKontrolPlaneClient.cs[55]

-					_kontrollerNodes = [.. response.KontrollerNodes.Select(EndPointExtensions.ToEndPoint)];
Relevance

●●● Strong

Deterministic topology bug: ordinary responses discard populated membership, preventing later
failover updates.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The client updates _kontrollerNodes only when KontrollerLeader is non-empty, but the server sets
that field to empty on ordinary cluster responses and still populates KontrollerNodes. All
subsequent endpoint rotation reads _kontrollerNodes, making the ignored membership unavailable to
failover.

src/KurrentDB.DataPlane/KontrolPlane/Transport/Grpc/GrpcKontrolPlaneClient.cs[53-66]
src/KurrentDB.KontrolPlane/KontrolPlane/Transport/Grpc/GrpcKontrollerServer.cs[58-65]
src/KurrentDB.DataPlane/KontrolPlane/Transport/Grpc/GrpcKontrolPlaneClient.Utils.cs[46-68]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Normal announcement responses contain current Kontrol Plane membership without a redirect, but the client now ignores those lists and continues failing over through stale seed data.

## Fix Focus Areas
- src/KurrentDB.DataPlane/KontrolPlane/Transport/Grpc/GrpcKontrolPlaneClient.cs[53-66]

## Recommended Fix
Update `_kontrollerNodes` for every announcement response. Select the advertised leader when present and otherwise use the current successful streaming endpoint as the leader passed to `GetKontrollerNodes`, preserving leader-first ordering without discarding normal membership updates.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context sources
✅ Compliance rules (platform): 17 rules
✅ Cross-repo context — repo relationships
Review mode: ⚖️ Balanced: The push changes concurrent endpoint replacement and control-plane leader address selection, so it carries behavioral and concurrency risk despite being localized.

Grey Divider

Tip of the day
💡 Did you know, you can enable the Remediation agent and Qodo fixes findings in a dedicated fix PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Previous reviews

Review updated until commit c6e5727 ⚖️ Balanced

Results up to commit e3444c9 ⚖️ Balanced


🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)


Action required
1. Failover retries the same dead node ✓ Resolved 🐞 Bug ☼ Reliability
Description
GetKontrollerNodes reads addresses[0] on every loop iteration, producing only the leader or
repeated copies of the first advertised endpoint instead of all members. When that endpoint fails,
NextAddress cycles through the malformed list and the appointment operations repeatedly retry an
unavailable node rather than reaching another controller.
Code

src/KurrentDB.DataPlane/KontrolPlane/Transport/Grpc/GrpcKontrolPlaneClient.cs[84]

+				var address = addresses[0].ToEndPoint();
Relevance

●●● Strong

Deterministic indexing bug: every iteration converts addresses[0], omitting all subsequent failover
endpoints.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The helper iterates over the full address count but always converts element zero, while
NextAddress advances only through the resulting _kontrollerNodes list. The server explicitly
populates responses from every controller node, so omitting all elements after the first discards
real failover targets.

src/KurrentDB.DataPlane/KontrolPlane/Transport/Grpc/GrpcKontrolPlaneClient.cs[79-89]
src/KurrentDB.DataPlane/KontrolPlane/Transport/Grpc/GrpcKontrolPlaneClient.Utils.cs[46-68]
src/KurrentDB.KontrolPlane/KontrolPlane/Transport/Grpc/GrpcKontrollerServer.cs[103-108]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`GetKontrollerNodes` reads index zero during every loop iteration, so peer Kontrol Plane nodes are omitted and failover can retry the same unavailable endpoint indefinitely.

## Fix Focus Areas
- src/KurrentDB.DataPlane/KontrolPlane/Transport/Grpc/GrpcKontrolPlaneClient.cs[79-89]

## Recommended Fix
Convert `addresses[i]` on each iteration instead of `addresses[0]`, retaining the existing leader-first ordering and leader de-duplication. Add coverage proving that multiple distinct advertised endpoints remain available in their expected failover order.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Failover uses a stale node list ✓ Resolved 🐞 Bug ☼ Reliability
Description
AnnounceNodeAsync now assigns _kontrollerNodes only inside the non-empty leader redirect branch,
replacing the previous unconditional assignment from every response. Normal streaming responses
carry an empty leader and a populated membership list, so topology changes remain absent from every
later failover until some redirect occurs.
Code

src/KurrentDB.DataPlane/KontrolPlane/Transport/Grpc/GrpcKontrolPlaneClient.cs[55]

-					_kontrollerNodes = [.. response.KontrollerNodes.Select(EndPointExtensions.ToEndPoint)];
Relevance

●●● Strong

Deterministic topology bug: ordinary responses discard populated membership, preventing later
failover updates.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The client updates _kontrollerNodes only when KontrollerLeader is non-empty, but the server sets
that field to empty on ordinary cluster responses and still populates KontrollerNodes. All
subsequent endpoint rotation reads _kontrollerNodes, making the ignored membership unavailable to
failover.

src/KurrentDB.DataPlane/KontrolPlane/Transport/Grpc/GrpcKontrolPlaneClient.cs[53-66]
src/KurrentDB.KontrolPlane/KontrolPlane/Transport/Grpc/GrpcKontrollerServer.cs[58-65]
src/KurrentDB.DataPlane/KontrolPlane/Transport/Grpc/GrpcKontrolPlaneClient.Utils.cs[46-68]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Normal announcement responses contain current Kontrol Plane membership without a redirect, but the client now ignores those lists and continues failing over through stale seed data.

## Fix Focus Areas
- src/KurrentDB.DataPlane/KontrolPlane/Transport/Grpc/GrpcKontrolPlaneClient.cs[53-66]

## Recommended Fix
Update `_kontrollerNodes` for every announcement response. Select the advertised leader when present and otherwise use the current successful streaming endpoint as the leader passed to `GetKontrollerNodes`, preserving leader-first ordering without discarding normal membership updates.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Comment thread src/KurrentDB.DataPlane/KontrolPlane/Transport/Grpc/GrpcKontrolPlaneClient.cs Outdated
@sakno
sakno marked this pull request as draft September 17, 2026 12:04
@sakno
sakno marked this pull request as ready for review September 17, 2026 13:59
@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit c6e5727

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