backup: add admin version API scaffolding#1059
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
|
Implemented the admin API/version scaffolding slice on top of #1056. Validation:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new GetNodeVersion RPC to the Admin service and implements asynchronous leader version probing and caching within GetRaftGroups. It also adds a SnapshotEvery method to the AdminGroup interface and its implementations. The review feedback highlights a potential race condition in probeLeaderVersionAsync due to accessing the request context asynchronously inside a goroutine, and suggests passing a mockable clock time into leaderNodeVersion to improve testability and avoid redundant syscalls.
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.
| func (s *AdminServer) probeLeaderVersionAsync(ctx context.Context, key, address string) { | ||
| probe := s.leaderVersionProbe | ||
| timeout := s.leaderVersionProbeTimeout | ||
| go func() { | ||
| probeCtx, cancel := context.WithTimeout(context.Background(), timeout) | ||
| defer cancel() | ||
| if md, ok := metadata.FromIncomingContext(ctx); ok { | ||
| probeCtx = metadata.NewOutgoingContext(probeCtx, md.Copy()) | ||
| } | ||
| version, err := probe(probeCtx, address) | ||
| if err != nil { | ||
| version = "" | ||
| } | ||
| s.versionCache.Store(key, versionCacheEntry{version: version, fetchedAt: time.Now()}) | ||
| }() | ||
| } |
There was a problem hiding this comment.
In probeLeaderVersionAsync, the incoming gRPC request context ctx is passed directly into a background goroutine, and metadata.FromIncomingContext(ctx) is called asynchronously inside that goroutine.
Since the background goroutine runs asynchronously, the parent GetRaftGroups request may have already completed and returned. Once the request returns, the gRPC server cancels and cleans up the request context. Accessing ctx asynchronously after the handler has returned can lead to race conditions, undefined behavior, or accessing recycled metadata.
To ensure safety and avoid race conditions, extract the metadata synchronously from ctx before spawning the goroutine.
func (s *AdminServer) probeLeaderVersionAsync(ctx context.Context, key, address string) {
probe := s.leaderVersionProbe
timeout := s.leaderVersionProbeTimeout
var md metadata.MD
if incomingMd, ok := metadata.FromIncomingContext(ctx); ok {
md = incomingMd.Copy()
}
go func() {
probeCtx, cancel := context.WithTimeout(context.Background(), timeout)
defer cancel()
if md != nil {
probeCtx = metadata.NewOutgoingContext(probeCtx, md)
}
version, err := probe(probeCtx, address)
if err != nil {
version = ""
}
s.versionCache.Store(key, versionCacheEntry{version: version, fetchedAt: time.Now()})
}()
}| CommitIndex: st.CommitIndex, | ||
| AppliedIndex: st.AppliedIndex, | ||
| LastContactUnixMs: lastContactUnixMs, | ||
| LeaderNodeVersion: s.leaderNodeVersion(ctx, st.Leader), |
There was a problem hiding this comment.
Pass the mockable clock time now (computed on line 452 using s.now()) into leaderNodeVersion instead of calling time.Now() internally. This ensures consistency with the rest of the GetRaftGroups response (such as LastContactUnixMs) and allows deterministic testing of the cache TTL using the mock clock.
| LeaderNodeVersion: s.leaderNodeVersion(ctx, st.Leader), | |
| LeaderNodeVersion: s.leaderNodeVersion(ctx, st.Leader, now), |
| func (s *AdminServer) leaderNodeVersion(ctx context.Context, leader raftengine.LeaderInfo) string { | ||
| if version, ok := s.localLeaderVersion(leader); ok { | ||
| return version | ||
| } | ||
| key := leaderVersionCacheKey(leader) | ||
| now := time.Now() | ||
| if version, ok := s.cachedLeaderVersion(key, now); ok { | ||
| return version | ||
| } |
There was a problem hiding this comment.
Update the signature of leaderNodeVersion to accept now time.Time from the caller. This avoids redundant time.Now() syscalls in the loop and allows deterministic testing of the cache TTL.
func (s *AdminServer) leaderNodeVersion(ctx context.Context, leader raftengine.LeaderInfo, now time.Time) string {
if version, ok := s.localLeaderVersion(leader); ok {
return version
}
key := leaderVersionCacheKey(leader)
if version, ok := s.cachedLeaderVersion(key, now); ok {
return version
}![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c44879386
ℹ️ 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".
| token = loaded | ||
| } | ||
| srv := adapter.NewAdminServer(self, members) | ||
| srv := adapter.NewAdminServer(self, members, adapter.WithAdminNodeVersion(buildVersion())) |
There was a problem hiding this comment.
When a Raft group is led by another node, leaderNodeVersion returns an empty leader_node_version unless leaderVersionProbe is configured, but this production constructor only sets WithAdminNodeVersion and there is no non-test call wiring WithAdminLeaderVersionProbe. That means remote leaders will permanently show an unknown version instead of asynchronously fetching GetNodeVersion; please provide the probe here (or avoid advertising the field until it is wired).
Useful? React with 👍 / 👎.
| srv := NewAdminServer( | ||
| NodeIdentity{NodeID: "n1", GRPCAddress: "10.0.0.11:50051"}, | ||
| nil, | ||
| WithAdminLeaderVersionProbeTimeout(50*time.Millisecond), |
There was a problem hiding this comment.
This test depends on the goroutine not hitting its 50 ms timeout before the test closes releaseProbe; under a slow CI scheduler, the second GetRaftGroups call or a pause after probeStarted can take longer than that, causing the probe to cache an empty version for the 1 s TTL and waitForLeaderVersion to time out. Use a much larger timeout or release the probe before asserting cache coalescing so the test is not scheduler-sensitive.
Useful? React with 👍 / 👎.
|
Addressed the current-head admin version review feedback. Changes:
Verification:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f78f4b228
ℹ️ 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".
| if err != nil { | ||
| version = "" | ||
| } | ||
| s.versionCache.Store(key, versionCacheEntry{version: version, fetchedAt: time.Now()}) |
There was a problem hiding this comment.
When tests or callers install a custom clock with SetClock, the cache freshness checks compare entries against that injected clock, but async probe completions are stamped with wall time here. If the injected clock is behind real time, now.Sub(entry.fetchedAt) stays negative and cached empty/error versions do not expire on the configured TTL; if it is ahead, a just-fetched version can expire immediately. Stamp cache entries with the same clock used by GetRaftGroups so TTL behavior remains consistent.
Useful? React with 👍 / 👎.
|
Addressed the current-head leader-version cache clock feedback. Changes:
Verification:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ece87b2f6
ℹ️ 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".
| if err != nil { | ||
| version = "" | ||
| } | ||
| s.versionCache.Store(key, versionCacheEntry{version: version, fetchedAt: now()}) |
There was a problem hiding this comment.
When a leader-version probe is still in flight after its reservation has expired (for example with a probe timeout longer than the cache TTL, or when the injected server clock advances past the TTL), a later GetRaftGroups call can replace the expired marker and start a fresh probe, but the older goroutine still unconditionally stores its result here. If that older probe then times out or returns an older value after the fresh probe has populated the cache, it overwrites the newer entry and suppresses another probe for a full TTL; use a generation/CAS-style check against the reservation before storing the completion.
Useful? React with 👍 / 👎.
|
Addressed the current-head admin version inline findings. Changes:
Caller audit:
Validation:
|
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ 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". |
|
@codex review Updated HEAD
Caller audit:
Validation run on this head:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7aece2712c
ℹ️ 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".
| } | ||
| version := "" | ||
| for _, address := range candidates { | ||
| probed, err := probe(probeCtx, address) |
There was a problem hiding this comment.
When the first candidate is a stale listener that accepts no connection, the production probe path (adminLeaderVersionProbe via GRPCConnCache.ConnFor) uses a wait-for-ready gRPC call, so this call can consume the entire 500ms probeCtx before returning. The loop then breaks on the expired context and never tries the alternate listener addresses for the same leader ID, caching an empty version for the group even though a later candidate may be reachable. Please give each candidate its own bounded attempt or otherwise avoid spending the whole budget on the first address.
Useful? React with 👍 / 👎.
|
Addressed the current-head leader-version candidate probe finding. Changes:
Verification:
|
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ 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". |
…' into design/live-backup-admin-api
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 259d59ba83
ℹ️ 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".
| LeaderView | ||
| StatusReader | ||
| ConfigReader | ||
| SnapshotReader |
There was a problem hiding this comment.
Embedding SnapshotReader into raftengine.Engine means every concrete value assigned to an Engine field must now define SnapshotEvery(). The existing followerProxyEngine in kv/shard_store_test.go is still assigned to ShardGroup.Engine in the proxy-scan tests but only implements the old method set through Close(), so the kv package no longer compiles until that fake is updated like the other test engines.
Useful? React with 👍 / 👎.
|
Current stacked HEAD is
|
|
✅ Action performedReview finished.
|
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ 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". |
|
Updated stacked HEAD Validation:
No new root inline finding is present on this HEAD; CI is running.
|
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd9ea8043d
ℹ️ 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".
| func leaderVersionCacheKey(leader raftengine.LeaderInfo) string { | ||
| if leader.ID != "" && leader.Address != "" { | ||
| return leader.ID + "\x00" + leader.Address |
There was a problem hiding this comment.
In multi-group deployments the same Leader.ID can appear with a different group listener address for each Raft group, and leaderProbeAddressesLocked already treats those addresses as alternate ways to reach the same node. Including the address in the cache key means a single GetRaftGroups poll over N groups led by the same peer starts N separate async GetNodeVersion probes and stores N cache entries, so the 10s cache does not provide the intended per-leader fanout bound on clusters with many shards. Key by Leader.ID when it is present and use the collected addresses only as probe candidates.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in signed HEAD e8c0cb2. leaderVersionCacheKey now uses Leader.ID whenever present, while the request-scoped address list remains a set of alternate candidates. The regression test asserts one per-node probe tries the two candidates exactly once.
| if leader.ID == "" { | ||
| return addresses | ||
| } | ||
| for _, id := range sortedGroupIDs(s.groups) { |
There was a problem hiding this comment.
When a node hosts many Raft groups, GetRaftGroups calls leaderProbeAddressesLocked once per output row, and this loop sorts and scans the full group map again each time. That turns an admin poll from O(groups) into O(groups² log groups) Status()/sort work before the RPC can return, so large multi-shard nodes can make the overview endpoint noticeably more expensive; build the leader-ID-to-addresses map once per request instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in signed HEAD e8c0cb2. GetRaftGroups snapshots each group status once, then builds the leader-to-address map in one pass. TestGetRaftGroupsSnapshotsEachGroupOnce verifies 64 groups cause exactly 64 Status calls.
| perCandidate := total / time.Duration(candidates) | ||
| if perCandidate <= 0 { | ||
| return total | ||
| } | ||
| return perCandidate |
There was a problem hiding this comment.
In multi-group clusters where the same leader has many group listener addresses, dividing the 500ms leader probe budget by the number of candidates can shrink each GetNodeVersion attempt to only a few milliseconds. A cold but healthy TCP/gRPC connection can miss that per-address slice, causing every candidate to time out and the empty result to be cached for the polling TTL even though the peer is reachable; cap the candidate list or enforce a reasonable minimum per-attempt timeout.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in signed HEAD e8c0cb2. Per-candidate attempts now have a 100ms minimum unless the total probe budget is smaller; the outer total timeout remains the hard cap. Tests cover large candidate counts, a smaller total budget, and the two-candidate split.
|
Current signed HEAD e8c0cb2 addresses all three current-head leader-version probe findings.
Caller audit:
Validation:
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ 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". |
1 participant
Summary
Base: #1056
Validation
Author: bootjp