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". |
1 participant
Summary
Base: #1056
Validation
Author: bootjp