Add live logical backup control plane#1119
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 Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughライブバックアップ用のFSMワイヤ、スナップショット走査、Admin RPC、トークン管理、起動時設定、アダプタ別デコード機能、管理提案転送、統合テストを追加しました。 Changesライブバックアップ基盤
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant AdminClient
participant AdminServer
participant LeaderAdminProposer
participant InternalLeader
participant RaftGroups
participant ShardStore
AdminClient->>AdminServer: BeginBackup
AdminServer->>LeaderAdminProposer: reserve and pin
LeaderAdminProposer->>InternalLeader: ForwardAdminProposal when follower
InternalLeader->>RaftGroups: apply admin proposal
RaftGroups-->>AdminServer: commit indexes
AdminServer->>ShardStore: capture route snapshot and scan baseline
AdminServer-->>AdminClient: pin token and expected keys
AdminClient->>AdminServer: StreamBackup
AdminServer->>ShardStore: scan at pinned readTS
ShardStore-->>AdminServer: BackupKV records
AdminServer-->>AdminClient: stream BackupKV
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
|
Current signed HEAD 1703991 is ready for review. Safety evidence:
Validation evidence is listed in the PR body; post-merge focused unit, targeted race, lint, proto generation, and proto breaking checks all pass. @codex review |
|
✅ Action performedReview finished.
|
![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 implements a live backup control plane, adding new gRPC endpoints (BeginBackup, RenewBackup, EndBackup, ListAdaptersAndScopes, and StreamBackup) to the admin service, along with key classification, backup scanners, and Raft FSM integration. The review feedback focuses on improving error handling: specifically, ensuring that errors from scanner.Close() are not silently ignored, and that context cancellation or deadline exceeded errors during backup streaming are correctly propagated as gRPC status codes instead of being masked as FailedPrecondition errors.
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.
| if scanner == nil { | ||
| return status.Errorf(codes.Unavailable, "%s", "backup scanner is nil") | ||
| } | ||
| defer func() { _ = scanner.Close() }() |
There was a problem hiding this comment.
The error returned by scanner.Close() is silently ignored using _. Discarding close errors on database scanners can mask resource leaks or cleanup failures. It is recommended to log this error to ensure operational visibility.
defer func() {
if err := scanner.Close(); err != nil {
// Log the error to ensure resource leaks are visible
}
}()References
- Do not silently ignore errors from Close() methods on resources like network connections. Log them to ensure resource leaks or other cleanup problems are visible.
There was a problem hiding this comment.
Fixed in 2b2c215. Baseline scanner close errors now flow through finishBackupScan and fail the List/Begin path; the new TestListBackupScopesReportsScannerCloseError covers the RPC-visible failure.
| if scanner == nil { | ||
| return nil, 0, errors.New("backup key scanner is nil") | ||
| } | ||
| defer func() { _ = scanner.Close() }() |
There was a problem hiding this comment.
The error returned by scanner.Close() is silently ignored using _. Discarding close errors on database scanners can mask resource leaks or cleanup failures. It is recommended to log this error to ensure operational visibility.
defer func() {
if err := scanner.Close(); err != nil {
// Log the error to ensure resource leaks are visible
}
}()References
- Do not silently ignore errors from Close() methods on resources like network connections. Log them to ensure resource leaks or other cleanup problems are visible.
There was a problem hiding this comment.
Fixed in 2b2c215. Stream scanner close errors return Internal after a successful scan; if the scan already failed, the original gRPC status is preserved and the cleanup failure is logged. The new stream test covers the close-error path.
|
Current signed HEAD 2b2c215 addresses all current review findings. Evidence:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| pair, ok, err := scanner.Next(stream.Context()) | ||
| if err != nil { | ||
| if errors.Is(err, context.Canceled) { | ||
| return status.Error(codes.Canceled, err.Error()) |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
error returned from external package is unwrapped: sig: func google.golang.org/grpc/status.Error(c google.golang.org/grpc/codes.Code, msg string) error (wrapcheck)
There was a problem hiding this comment.
Fixed in c9baa53. Cancellation and deadline errors now use the shared status.Errorf-based normalizer; repository golangci-lint reports 0 issues.
| return status.Error(codes.Canceled, err.Error()) | ||
| } | ||
| if errors.Is(err, context.DeadlineExceeded) { | ||
| return status.Error(codes.DeadlineExceeded, err.Error()) |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
error returned from external package is unwrapped: sig: func google.golang.org/grpc/status.Error(c google.golang.org/grpc/codes.Code, msg string) error (wrapcheck)
There was a problem hiding this comment.
Fixed in c9baa53. Cancellation and deadline errors now use the shared status.Errorf-based normalizer; repository golangci-lint reports 0 issues.
![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: 2b2c2152df
ℹ️ 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 { | ||
| return err | ||
| } | ||
| scanner := s.backupStore.NewBackupScanner(nil, nil, tok.readTS, s.backupConfig.scanPageSize) |
There was a problem hiding this comment.
When a range split/move applies after BeginBackup builds expected_keys but before the producer calls StreamBackup, this creates a fresh scanner from the current route table for the old read_ts. Keys that were still owned only by the old group at the pinned timestamp can be omitted (or a later owner can be scanned instead), so the streamed backup no longer matches the baseline returned by BeginBackup and can silently miss data if the producer does not treat the count mismatch as fatal. Capture the route snapshot/version used for the baseline in the token, or otherwise block/handle route changes while the pin is active.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c9baa53. BeginBackup captures one immutable BackupRouteSnapshot before the read fence, uses it for the expected-key baseline, and stores it under the HMAC-authenticated pin ID for List/Stream. StreamBackup constructs its scanner from that exact snapshot; a missing endpoint-local snapshot fails closed. TestBackupScannerReusesCapturedRouteSnapshot and TestStreamBackupFailsClosedWithoutPinnedRouteSnapshot cover both paths.
| } | ||
| clock := coordinate.Clock() | ||
| clock.Observe(shardStore.LastCommitTS()) | ||
| readTS, err := clock.NextFenced() |
There was a problem hiding this comment.
Because readTS is allocated after the all-groups lease read above, a concurrent write can allocate a commit timestamp before this NextFenced() call but only commit/apply after the lease read has completed. That committed version is then <= read_ts and belongs in the point-in-time backup, but the prior fence did not wait for it, so BeginBackup can immediately scan and miss it. Allocate the backup timestamp first and then issue the cluster-wide read/applied barrier for that timestamp (or an equivalent commit-index fence).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c9baa53. adminBackupReadFence now observes the local commit watermark and allocates readTS before LeaseReadAllGroupsThrough. TestAdminBackupReadFenceAllocatesTimestampBeforeBarrier records the HLC order and fails if the barrier moves ahead of timestamp issuance.
| } | ||
|
|
||
| func (s *AdminServer) gateBackupPeerVersions(ctx context.Context) error { | ||
| members := s.snapshotMembers(ctx) |
There was a problem hiding this comment.
In a multi-node cluster where any Raft Configuration call times out or errors, snapshotMembers intentionally returns a partial member list instead of failing; using that tolerant view for BeginBackup means any node known only to the failed group (and not in the static seed) is never probed for backup_protocol_version. A rolling-upgrade backup can therefore proceed while an old replica is still in the Raft configuration, defeating the capability gate before new backup FSM entries are proposed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c9baa53. The backup capability gate now uses snapshotBackupMembers, which requires every registered Raft group's Configuration call to return successfully before any peer probes or FSM proposals. The tolerant snapshotMembers path remains unchanged for GetClusterOverview. TestBeginBackupFailsClosedOnIncompleteMembership covers the failed Configuration case.
| if err != nil { | ||
| return err | ||
| } | ||
| s.page = append(s.page[:0], keys...) |
There was a problem hiding this comment.
When old-owner keys remain in a shard after a split/move, the value scanner drops them via materializeRouteForKey, but the count-only scanner accepts every key returned by scanKeyRoutesWithSourceAt. In that scenario BeginBackup/ListAdaptersAndScopes can count scopes from stale off-route data that StreamBackup will not emit, causing otherwise valid backups to fail baseline-count verification or advertise scopes that cannot actually be streamed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c9baa53. backupKeyScanner filters each raw page through the same captured-route ownership resolver as the value scanner while advancing its cursor from the raw page, including all-filtered pages. TestBackupKeyScannerFiltersStaleOffRouteKeys covers stale data followed by an owned key across page boundaries.
| if err := stream.Send(&pb.BackupKV{Key: pair.Key, Value: pair.Value}); err != nil { | ||
| return errors.WithStack(err) |
There was a problem hiding this comment.
If the client cancels or the stream deadline expires after at least one record is ready to send, the error comes from stream.Send, not scanner.Next; wrapping that raw context.Canceled/DeadlineExceeded with errors.WithStack makes status.Code see Unknown, while only scanner errors are normalized above. Handle context errors from Send the same way before wrapping so canceled backup streams retain the expected gRPC status.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c9baa53. Errors from both scanner.Next and stream.Send pass through backupContextStreamError, preserving Canceled and DeadlineExceeded. TestStreamBackupPreservesContextStatusAndReportsCloseErrors now covers a DeadlineExceeded send failure; normal and race backup suites pass.
|
Current signed HEAD c9baa53 addresses every finding from the previous-head review.\n\nSafety evidence:\n- BeginBackup captures one immutable route view and baseline/List/Stream reuse it; missing endpoint-local state fails closed.\n- read_ts is issued before the all-groups applied barrier.\n- backup membership capability gating requires every Raft Configuration call to succeed; the overview RPC remains tolerant.\n- key-only pages apply the same captured-route ownership filter as value scans.\n- scanner and send cancellation preserve gRPC Canceled/DeadlineExceeded.\n- SQS FIFO sequence side rows use a dedicated bare-key parser and remain restorable.\n\nCaller audit:\n- adminBackupReadFence has one production caller in Admin option wiring.\n- snapshotBackupMembers is used only by the backup capability gate; snapshotMembers remains on GetClusterOverview.\n- captured-snapshot scanner APIs are confined to live-backup control and scanner tests.\n\nValidation:\n- go test ./adapter -run Backup -count=1\n- go test -race ./adapter -run Backup -count=1\n- targeted root/kv/internal-backup/adapter tests\n- targeted root/kv/internal-backup/adapter race tests\n- golangci-lint run ./... --timeout=5m (0 issues)\n- go test ./... passed every package except the pre-existing adapter multi-node/election suite, which hit its 10-minute package timeout; all backup adapter tests pass independently.\n\nCI is running for this HEAD.\n\n@codex review\n@coderabbitai review |
|
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 `@adapter/admin_backup.go`:
- Around line 350-355: After renewBackupGroups succeeds but
requireUnexpiredBackupToken fails, release the renewed pin’s group fences and
capacity reservations before returning the token error. Update the error path in
the surrounding RPC method to perform compensating cleanup for the renewed pin,
while preserving the existing renewal error handling. Add a test covering token
expiry after renewal and verify all renewed resources are released.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ab9aa49b-0f99-4fe1-b4f5-b5e9a869e938
⛔ Files ignored due to path filters (2)
proto/admin.pb.gois excluded by!**/*.pb.goproto/admin_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (18)
adapter/admin_backup.goadapter/admin_backup_test.goadapter/admin_grpc.gointernal/backup/live.gointernal/backup/live_test.gointernal/backup/s3.gointernal/backup/sqs.gokv/active_timestamp_tracker.gokv/backup_codec.gokv/backup_codec_test.gokv/backup_scan.gokv/compactor_test.gokv/fsm_backup.gokv/fsm_backup_test.gokv/shard_store_test.gomain.gomain_admin_test.goproto/admin.proto
|
Current signed HEAD 693592b addresses the current-head renewal cleanup finding. Evidence:
CI is running for this HEAD. @codex review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Current commit =="
git rev-parse HEAD
git status --short
echo "== Backup implementation outline =="
ast-grep outline adapter/admin_backup.go --items all --type function,method,struct,interface --match 'Renew|expire|cleanup|Backup|session|propose|release|unreserve' || true
echo "== Relevant implementation symbols =="
rg -n -C 5 -i 'func \(s \*AdminServer\) RenewBackup|expire|cleanup.*backup|backupSessions|unreserve|release.*backup|proposeBackupAll|proposeBackupWithRetry' adapter/admin_backup.go
echo "== Backup test outline =="
ast-grep outline adapter/admin_backup_test.go --items all --type function --match 'Renew|Deadline|expire|cleanup|Backup' || true
echo "== Relevant regression tests =="
rg -n -C 8 -i 'clock|expiry|expire|renew.*(cleanup|deadline)|unreserve|session removal|backupSessions' adapter/admin_backup_test.goLength of output: 21431 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Renew / End paths =="
sed -n '328,395p' adapter/admin_backup.go
echo "== Fanout, compensation, and renewal retry paths =="
sed -n '714,872p' adapter/admin_backup.go
echo "== Session and token/group lookup paths =="
sed -n '910,960p' adapter/admin_backup.go
sed -n '1113,1165p' adapter/admin_backup.go
echo "== Test proposer behavior =="
sed -n '32,155p' adapter/admin_backup_test.go
echo "== Renewal-expiry regression test =="
sed -n '514,586p' adapter/admin_backup_test.go
echo "== Relevant backup configuration initialization =="
sed -n '89,153p' adapter/admin_backup.go
sed -n '227,256p' adapter/admin_backup_test.goLength of output: 20750 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Begin and baseline lifecycle =="
sed -n '184,328p' adapter/admin_backup.go
echo "== All renewal-helper call sites =="
rg -n -C 6 '\brenewBackupGroups\s*\(' --glob '*.go'
echo "== Tests for renewal pin-fanout failure / cleanup =="
rg -n -i -C 8 'renew.*(fail|error|retry|compensat|release|unreserve)|group pins|capacity reservation' adapter/admin_backup_test.go
echo "== Backup tracker semantics for duplicate reserve/pin/release =="
ast-grep outline kv/active_timestamp_tracker.go --items all --type function,method,struct --match 'Backup|Pin|Reserve|Release|Unreserve' || true
rg -n -C 5 'Reserve|Unreserve|PinWithDeadline|ReleaseBackupPin|backup' kv/active_timestamp_tracker.goLength of output: 6435 🏁 Script executed: #!/bin/bash
set -u
echo "== Exact renewal helper references =="
rg -n -F -C 5 'renewBackupGroups(' --glob '*.go' || true
echo "== Active timestamp tracker outline =="
ast-grep outline kv/active_timestamp_tracker.go --items all --type function,method,struct --match 'Backup|Pin|Reserve|Release|Unreserve' || true
echo "== Active timestamp tracker backup methods =="
rg -n -i -C 6 'backup|reserve|unreserve|pinwithdeadline|releasebackuppin' kv/active_timestamp_tracker.go || true
echo "== Existing renewal failure test contexts =="
rg -n -i -C 12 'TestRenew.*(Retry|Fail|Release|Cleanup)|renew backup pin|group pins|capacity reservation' adapter/admin_backup_test.go || trueLength of output: 853
That leaves the control-group reservation at its new deadline, plus any groups whose pin proposal did commit during the partially failed fanout. Since the client receives an error—and its existing token may expire—the capacity slot and retention fences can persist until expiry rather than being fail-closed immediately. Please compensate this error path as well (release all token groups, then unreserve the control group, and remove the endpoint-local session), and add a regression test that injects a pin-fanout failure after ✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Lines 372 to 374 in 693592b
Prefer owned duplicate keys before trimming
When a page contains the same raw key from the current owner and from a later-scanned stale/off-route group, such as after a range split where the old group still has a copy, this dedupe overwrites the owned item with the later duplicate before routeForRoutedKey filters ownership. The stale item is then rejected and the live key is omitted from both expected counts and StreamBackup. Filter or rank by captured route ownership before collapsing duplicates.
ℹ️ 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".
| clock := coordinate.Clock() | ||
| clock.Observe(shardStore.LastCommitTS()) | ||
| readTS, err := clock.NextFenced() |
There was a problem hiding this comment.
When BeginBackup runs on a node whose local shard store has not yet applied a write that already committed on another raft-group leader, this mints readTS before the LeaseReadAllGroupsThrough call below. That lease read can then pull in the completed write with a commit timestamp greater than readTS, so the subsequent pinned scan omits data that was already committed before the backup began. The fence should first bring all groups up to date, then observe the resulting max LastCommitTS/allocate a timestamp above it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bd8d262. The all-groups lease/read barrier now completes before LastCommitTS is observed and read_ts is minted. TestAdminBackupReadFenceAllocatesTimestampAfterBarrier applies a commit during the barrier and proves the returned timestamp is greater.
| routes := s.backupStore.CaptureBackupRouteSnapshot(nil, nil) | ||
|
|
||
| readTS, err := s.backupReadFence(ctx) |
There was a problem hiding this comment.
When a route split or rebalance commits between this route snapshot and backupReadFence, the returned readTS can include writes routed with the new catalog while later scans are forced to the older routes captured here. A key committed on the new group before readTS is then never scanned even though it belongs in the backup. Capture the route view after the fence, or bind it to a catalog version that is also fenced by readTS.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bd8d262. prepareBackup now captures the immutable route snapshot only after the read fence succeeds and validates its timestamp. TestBeginBackupCapturesRouteSnapshotAfterReadFence pins the ordering.
| for _, rt := range runtimes { | ||
| srv.RegisterGroup(rt.spec.id, rt.engine) | ||
| if group := shardGroups[rt.spec.id]; group != nil { | ||
| srv.RegisterBackupProposer(rt.spec.id, group.Proposer()) |
There was a problem hiding this comment.
In multi-group deployments where leadership is spread across nodes, registering group.Proposer() wires backup pin proposals to the local raft engine for each group. The etcd engine's ProposeAdmin is still a local proposal path and returns not-leader on followers, so BeginBackup fails unless the chosen admin endpoint is leader for every raft group. Use a leader-proxying proposer for these backup FSM entries instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bd8d262. Backup ProposeAdmin calls now use a leader-routed proposer. Followers forward the raw admin payload over Internal.ForwardAdminProposal; the verified leader applies it through its local wrap-aware ShardGroup.Proposer. Local and follower forwarding tests cover both paths.
|
Current signed HEAD bd8d262 addresses every current-head finding. Evidence:
Caller audit:
Validation:
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
adapter/internal_admin_proposal_test.go (1)
63-82: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueテーブル駆動テストへのリファクタリング
As per coding guidelines, unit tests should be co-located with packages in
*_test.gofiles; prefer table-driven test cases. 類似したテストケースをテーブル駆動形式にまとめることを推奨します。♻️ 提案するリファクタリング
func TestInternalForwardAdminProposalFailsClosed(t *testing.T) { t.Parallel() - t.Run("follower", func(t *testing.T) { - internal := NewInternalWithEngine(nil, internalAdminLeaderView{state: raftengine.StateFollower}, nil, nil) - _, err := internal.ForwardAdminProposal(context.Background(), &pb.ForwardAdminProposalRequest{}) - require.ErrorIs(t, err, ErrNotLeader) - }) - t.Run("apply response", func(t *testing.T) { - proposer := &internalAdminProposer{response: stderrors.New("apply failed")} - internal := NewInternalWithEngine( - nil, - internalAdminLeaderView{state: raftengine.StateLeader}, - nil, - nil, - WithInternalAdminProposer(proposer), - ) - _, err := internal.ForwardAdminProposal(context.Background(), &pb.ForwardAdminProposalRequest{}) - require.ErrorContains(t, err, "apply failed") - }) + tests := []struct { + name string + leaderView internalAdminLeaderView + proposer raftengine.Proposer + wantErr string + errTarget error + }{ + { + name: "follower", + leaderView: internalAdminLeaderView{state: raftengine.StateFollower}, + proposer: nil, + errTarget: ErrNotLeader, + }, + { + name: "apply response", + leaderView: internalAdminLeaderView{state: raftengine.StateLeader}, + proposer: &internalAdminProposer{response: stderrors.New("apply failed")}, + wantErr: "apply failed", + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + internal := NewInternalWithEngine( + nil, + tt.leaderView, + nil, + nil, + WithInternalAdminProposer(tt.proposer), + ) + _, err := internal.ForwardAdminProposal(context.Background(), &pb.ForwardAdminProposalRequest{}) + if tt.errTarget != nil { + require.ErrorIs(t, err, tt.errTarget) + } + if tt.wantErr != "" { + require.ErrorContains(t, err, tt.wantErr) + } + }) + } }🤖 Prompt for 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. In `@adapter/internal_admin_proposal_test.go` around lines 63 - 82, Refactor TestInternalForwardAdminProposalFailsClosed into a table-driven test covering the follower and apply-response failure scenarios. Define each case with its setup and expected error assertion, then iterate with t.Run while preserving the existing ErrNotLeader and “apply failed” checks.Source: Coding guidelines
🤖 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 `@kv/backup_scan.go`:
- Around line 393-401: Update preferredRoutedScanKey so that when current and
candidate have the same ownership state, it returns current to preserve the
stable ordering from the preceding sort.SliceStable. Keep returning current when
only currentOwned is true, and return candidate only when candidate is owned
while current is not.
---
Nitpick comments:
In `@adapter/internal_admin_proposal_test.go`:
- Around line 63-82: Refactor TestInternalForwardAdminProposalFailsClosed into a
table-driven test covering the follower and apply-response failure scenarios.
Define each case with its setup and expected error assertion, then iterate with
t.Run while preserving the existing ErrNotLeader and “apply failed” checks.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c7c9afca-f492-4391-b91d-516cb75aa5c6
⛔ Files ignored due to path filters (2)
proto/internal.pb.gois excluded by!**/*.pb.goproto/internal_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (11)
adapter/admin_backup.goadapter/admin_backup_test.goadapter/internal.goadapter/internal_admin_proposal_test.gokv/backup_scan.gokv/leader_admin_proposer.gokv/leader_admin_proposer_test.gokv/shard_store_test.gomain.gomain_admin_test.goproto/internal.proto
🚧 Files skipped from review as they are similar to previous changes (2)
- main.go
- adapter/admin_backup.go
|
Current signed HEAD f5d9e2f addresses the latest review.
Validation:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f5d9e2f42e
ℹ️ 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".
| return 0, errors.Wrap(err, "backup: fence raft groups") | ||
| } | ||
| clock.Observe(shardStore.LastCommitTS()) | ||
| readTS, err := clock.NextFenced() |
There was a problem hiding this comment.
When --tsoEnabled is in use, writes consume timestamps from the coordinator's BatchAllocator, but the backup fence bypasses that allocator and calls the underlying HLC directly. If the batch allocator already has unused timestamps reserved below the newly returned readTS, later writes can still commit below the backup timestamp after the pin is installed, breaking the snapshot guarantee; the fence needs to coordinate with or invalidate the same allocator that production writes use.
Useful? React with 👍 / 👎.
| } | ||
| s.page = s.page[:0] | ||
| for _, item := range keys { | ||
| if _, ok := routeForRoutedKey(item, s.routes); ok { |
There was a problem hiding this comment.
When backing up an SQS HT-FIFO queue whose partition resolver routes partitioned message keys to a non-default group, this filter drops those records: routeForRoutedKey only checks distribution routes using routeKey, while production ShardRouter.ResolveGroup consults the partition resolver before routeKey. The scanner does visit every group, but records from partition-only groups fail this ownership test and are neither counted nor streamed, so live backups of partitioned queues are incomplete.
Useful? React with 👍 / 👎.
| } | ||
| return func(ctx context.Context) (uint64, error) { | ||
| clock := coordinate.Clock() | ||
| if err := kv.LeaseReadAllGroupsThrough(coordinate, ctx); err != nil { |
There was a problem hiding this comment.
In a multi-group deployment where this admin endpoint is follower for any Raft group, BeginBackup fails here before the leader-forwarding proposers are used: LeaseReadAllGroupsThrough on ShardedCoordinator calls each local engine's LinearizableRead, and the etcd engine rejects reads when State != Leader. After leader balancing, no node may lead every group, making the live backup RPC unusable unless callers happen to choose an all-leader endpoint.
Useful? React with 👍 / 👎.
|
Updated the signed HEAD to Conflict-resolution evidence:
Please review the latest head. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 830d645475
ℹ️ 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".
| coordinate.Clock(), | ||
| relay, | ||
| internalTimestampOptions(coordinate)..., | ||
| append(internalTimestampOptions(coordinate), adapter.WithInternalAdminProposer(proposerForGroup(rt, shardGroups)))..., |
There was a problem hiding this comment.
When the node gRPC listener is reachable by normal clients, this wires ForwardAdminProposal on /Internal unconditionally, even when the Admin service is disabled or protected by a bearer token; the existing admin interceptor only checks /Admin methods. That lets any caller submit arbitrary ProposeAdmin payloads (for example long-lived backup pins with low read_ts, or malformed control entries) to the Raft leader. Please gate this option on the same admin authentication/enablement path or add equivalent auth/validation to the internal RPC.
Useful? React with 👍 / 👎.
| if readTS == 0 || readTS == ^uint64(0) { | ||
| return preparedBackup{}, status.Errorf(codes.FailedPrecondition, "%s", "backup read fence returned an invalid timestamp") | ||
| } | ||
| routes := s.backupStore.CaptureBackupRouteSnapshot(nil, nil) |
There was a problem hiding this comment.
When a route split or move applies after backupReadFence returns read_ts but before this call, the token stores the post-change route set while scans still read MVCC data at the older timestamp. Keys that were owned by the old group at read_ts can then be dispatched/materialized from the new captured owner, or have the old copy filtered out, so the live backup can omit data. Capture a route view that is valid at read_ts (or freeze route changes through the pin) instead of reading the live catalog here.
Useful? React with 👍 / 👎.
| ) (map[uint64]uint64, error) { | ||
| reserveEntry := kv.EncodeBackupReserveEntry(kv.BackupReserveEntry{PinID: pinID, ReadTS: readTS, Deadline: deadline}) | ||
| if _, _, err := proposeBackupAll(ctx, []backupGroup{controlGroup}, reserveEntry); err != nil { | ||
| if errors.Is(err, kv.ErrTooManyActiveBackups) { |
There was a problem hiding this comment.
When BeginBackup is handled by a follower for the control group, the reserve proposal goes through ForwardAdminProposal; the leader returns ErrTooManyActiveBackups as a gRPC error, so this local errors.Is check no longer matches and the client gets Unavailable instead of ResourceExhausted. That makes capacity exhaustion look retryable and bypasses the API contract tested for local proposals; map the internal RPC error/status back to the sentinel or ResourceExhausted before this check.
Useful? React with 👍 / 👎.
| if scanner == nil { | ||
| return status.Errorf(codes.Unavailable, "%s", "backup scanner is nil") | ||
| } | ||
| scanErr := streamBackupRecords(stream, scanner, selected) |
There was a problem hiding this comment.
For streams that last past tok.deadline when the client fails to renew, this starts the paging loop after only the upfront expiry check and never revalidates the session deadline. Once the tracker sweeper reaps the pin, FSM compaction can advance below tok.readTS while the RPC keeps reading pages, so a slow dump can silently become incomplete instead of failing when the pin is no longer valid. Recheck the deadline around each page/send or bind the stream lifetime to a live renewed pin.
Useful? React with 👍 / 👎.
13d22a8 to
26d45ba
Compare
830d645 to
6a85349
Compare
|
History-only author-compliance repair completed at
No runtime or source-tree semantics changed. CI is running on the reconstructed head. @codex review |
|
Current reconstructed head |
|
Latest signed head |
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (85e98d7):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
|
Current signed HEAD Safety evidence:
Caller/semantic audit:
Validation:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 85e98d7364
ℹ️ 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 := kv.LeaseReadAllGroupsThrough(coordinate, ctx); err != nil { | ||
| return 0, errors.Wrap(err, "backup: fence raft groups") | ||
| } | ||
| lastCommitTS := shardStore.LastCommitTS() | ||
| clock.Observe(lastCommitTS) | ||
| readTS, err := allocateBackupReadTimestamp(ctx, coordinate, lastCommitTS) |
There was a problem hiding this comment.
When BeginBackup is served by a node whose local replica is behind another shard leader, this only observes the local stores' LastCommitTS() after LeaseReadAllGroups. The lease read fences leader applied indexes, but it does not fetch the leaders' highest commit timestamps, and non-TSO forwarded writes are stamped on those leaders; the backup can therefore choose read_ts below a write that completed before the fence and then omit that committed version from the scan.
Useful? React with 👍 / 👎.
| case hasAnyBackupPrefix(key, DDBTableMetaPrefix, DDBTableGenPrefix, DDBItemPrefix, DDBGSIPrefix): | ||
| return scopeForDDBKey(key) | ||
| case hasAnyBackupPrefix(key, | ||
| S3BucketMetaPrefix, S3BucketGenPrefix, S3ObjectManifestPrefix, | ||
| S3UploadMetaPrefix, S3UploadPartPrefix, S3BlobPrefix, S3GCUploadPrefix, S3RoutePrefix, |
There was a problem hiding this comment.
When a DynamoDB table or S3 bucket has been deleted before read_ts, its generation counter can intentionally remain even though the resource is no longer user-visible. Including the generation prefixes in scope discovery makes ListAdaptersAndScopes and expected_keys report phantom resources, and StreamBackup can emit only operational generation records that the decoder later drops.
Useful? React with 👍 / 👎.
|
Addressed the latest live-backup P1 findings in 2f927a6. Changes:
Verification:
@codex review |
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (2f927a6):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
|
Current signed HEAD dd9f499 addresses the remaining live-backup safety windows. Changes:
Evidence:
|
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (dd9f499):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd9f499f93
ℹ️ 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".
| scanErr := streamBackupRecords(stream, scanner, selected, func() error { | ||
| return s.requireUnexpiredBackupToken(tok) |
There was a problem hiding this comment.
For a long-running backup where the producer renews every ttl/3, this closure keeps checking the immutable token decoded when StreamBackup started. RenewBackup extends backupSessions and returns a replacement token, but an already-open server stream cannot swap its request token, so the stream will fail at the original deadline even though the replicated pin is still live. Recheck the stored session/active pin deadline for this pin instead of the stale token deadline so renewed multi-hour dumps can continue safely.
Useful? React with 👍 / 👎.
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (6608a1c):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
|
Current signed HEAD 6608a1c addresses the latest live-backup renewal and forwarding findings. Changes:
Validation:
|
There was a problem hiding this comment.
💡 Codex Review
Line 419 in 6608a1c
Include partition-only groups in backup scans
When an SQS FIFO partition map routes a queue partition to a configured Raft group that does not own any byte-range catalog route, this loop never visits that group's store because the captured snapshot only contributes distribution routes; the partition resolver is only consulted after a key has already been found. validateShardRanges only checks that referenced ranges point at known groups, so such partition-only groups are allowed, and live backups will under-count and omit their partitioned SQS records even though BeginBackup pinned all groups. Please carry the pinned group list into the backup scan source, or otherwise add partition-routed groups to the snapshot before scanning.
ℹ️ 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 { | ||
| return nil, status.Errorf(codes.Internal, "encode renewed backup token: %v", err) | ||
| } | ||
| s.extendBackupSession(tok) |
There was a problem hiding this comment.
When EndBackup runs concurrently after this RPC's snapshot check, it can delete the session while renewBackupGroups is still waiting on Raft; extendBackupSession then silently no-ops for the missing session, but RenewBackup still returns a fresh token after re-pinning. In that interleaving the caller either gets a token for pins that EndBackup subsequently released, or the renew resurrects raft pins with no endpoint session so later stream/list calls fail and the pins linger until TTL. Please make the session extension conditional/failing with compensation, or serialize RenewBackup and EndBackup for the same pin.
Useful? React with 👍 / 👎.
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (b3b8ab9):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
|
Updated signed head Changes:
Validation:
|
There was a problem hiding this comment.
💡 Codex Review
Line 466 in b3b8ab9
Include partition-only groups in backup scans
When --sqsFifoPartitionMap routes FIFO partitions to a Raft group that has no byte-range route in the distribution catalog, this loop never scans that group at all; routeForRoutedKey only gets a chance to accept partitioned SQS keys after their group has been enumerated here. Since BeginBackup pins every registered group but the captured route snapshot can omit partition-only groups, expected counts and streams silently miss those SQS records.
ℹ️ 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".
|
Pushed d91744b. Fixed current review findings:
Caller audit:
Validation:
@codex review |
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (d91744b):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
|
Codex Review: Didn't find any major issues. Swish! 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
Safety
Validation
Author: bootjp
Summary by CodeRabbit