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 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.
| pair, ok, err := scanner.Next(stream.Context()) | ||
| if err != nil { | ||
| return status.Errorf(codes.FailedPrecondition, "scan backup at read_ts: %v", err) | ||
| } |
There was a problem hiding this comment.
In streamBackupRecords, when scanner.Next(stream.Context()) returns an error, it is wrapped in a FailedPrecondition gRPC status code. If the error is due to client-side context cancellation (context.Canceled) or deadline expiration (context.DeadlineExceeded), returning FailedPrecondition is misleading. It is better to check for these context errors and return the corresponding gRPC status codes (Canceled or DeadlineExceeded) to ensure correct error propagation.
| pair, ok, err := scanner.Next(stream.Context()) | |
| if err != nil { | |
| return status.Errorf(codes.FailedPrecondition, "scan backup at read_ts: %v", err) | |
| } | |
| pair, ok, err := scanner.Next(stream.Context()) | |
| if err != nil { | |
| if errors.Is(err, context.Canceled) { | |
| return status.Error(codes.Canceled, err.Error()) | |
| } | |
| if errors.Is(err, context.DeadlineExceeded) { | |
| return status.Error(codes.DeadlineExceeded, err.Error()) | |
| } | |
| return status.Errorf(codes.FailedPrecondition, "scan backup at read_ts: %v", err) | |
| } |
There was a problem hiding this comment.
Fixed in current signed HEAD 2b2c215. scanner.Next now maps context.Canceled and context.DeadlineExceeded to the matching gRPC status codes before the FailedPrecondition fallback. TestStreamBackupPreservesContextStatusAndReportsCloseErrors covers cancellation.
|
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}
| return nil | ||
| } | ||
|
|
||
| func streamBackupRecords( |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
calculated cyclomatic complexity for function streamBackupRecords is 12, max is 10 (cyclop)
There was a problem hiding this comment.
Fixed in c9baa53. Stream error normalization is split into backupScanStreamError, backupSendStreamError, and backupContextStreamError, reducing streamBackupRecords below the cyclop limit. Full repository golangci-lint reports 0 issues.
| 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.
| ): | ||
| return scopeForS3Key(key) | ||
| case hasAnyBackupPrefix(key, | ||
| SQSQueueMetaPrefix, SQSQueueGenPrefix, SQSMsgDataPrefix, SQSQueueSeqPrefix, |
There was a problem hiding this comment.
When the producer enables IncludeSQSSideRecords for a live SQS backup, classifying SQSQueueSeqPrefix as a scoped stream record causes StreamBackup to emit !sqs|queue|seq|<queue> rows, but the live decoder routes that prefix through routeSQSSide/HandleSideRecord; with side records enabled, HandleSideRecord calls parseSQSGenericKey, which requires a generation/trailer and rejects bare queue-sequence keys as malformed. A FIFO queue with a sequence row can therefore make the live decode fail; either exclude this prefix from live scope streaming or add a sequence-specific side-record parser.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c9baa53 without dropping the FIFO sequence counter. SQSEncoder.HandleSideRecord now selects parseSQSQueueSequenceKey for the bare queue-sequence shape and keeps the generic parser for generation-bearing side rows. TestLiveDecoderAcceptsSQSQueueSequenceSideRecord exercises the live decoder with IncludeSQSSideRecords enabled.
|
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.
| if server.Address == "" { | ||
| continue |
There was a problem hiding this comment.
When a live Raft configuration contains a server with an empty Address and there is no seed entry to backfill it, this branch records the ID as seen but drops it from live.order, so snapshotBackupMembers never returns it and gateBackupPeerVersions never probes it. A dynamically added or metadata-lagging old node can then receive backup FSM entries without proving BackupProtocolVersion >= 1, defeating the rolling-upgrade safety gate. Return FailedPrecondition for blank live addresses instead of continuing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bd8d262. Live member IDs without a configuration or seed address are added to the deterministic validation order, so BeginBackup returns FailedPrecondition before proposing any FSM entry. TestBeginBackupFailsClosedOnMemberWithoutProbeAddress covers this path.
| 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".
| if applyErr == nil && f.hlc != nil { | ||
| // The pin entry is also the per-group timestamp barrier. Once it | ||
| // applies, later writes on this replica must issue above read_ts. | ||
| f.hlc.Observe(readTS) |
There was a problem hiding this comment.
When a raw write has already been timestamped but is still waiting in the normal commit path (for example in TransactionManager's raw batching window), a BeginBackup pin can be proposed and applied before that write's Raft entry. This line only advances the HLC for future timestamp allocations; it does not reject or restamp an already-stamped request that applies later with Ts <= read_ts, so a live scan at read_ts can observe a non-atomic cut under concurrent writes.
Useful? React with 👍 / 👎.
| 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 👍 / 👎.
| func (i *Internal) ForwardAdminProposal( | ||
| ctx context.Context, | ||
| req *pb.ForwardAdminProposalRequest, | ||
| ) (*pb.ForwardAdminProposalResponse, error) { |
There was a problem hiding this comment.
In deployments where the raft gRPC listener is reachable by anything other than trusted peers, this new Internal/ForwardAdminProposal method bypasses the Admin bearer-token gate: startRaftServers registers Internal on the same gRPC server, while AdminTokenAuth only checks methods under /Admin/. Because Payload is arbitrary bytes, a caller can submit backup/encryption admin opcodes directly to the leader and reserve/release pins or halt apply without an Admin token.
Useful? React with 👍 / 👎.
| if err != nil { | ||
| return nil, errors.WithStack(err) | ||
| } | ||
| if err := forwardedAdminProposalResponseError(result); err != nil { |
There was a problem hiding this comment.
When the group leader is remote, any FSM/application error from ProposeAdmin is returned here as a normal gRPC error and the sentinel is lost. For example, a remote kv.ErrTooManyActiveBackups from the reservation apply path no longer satisfies errors.Is in pinBackupGroups, so BeginBackup retries/returns Unavailable instead of the deterministic ResourceExhausted response that the local leader path produces.
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 👍 / 👎.
| s.groupsMu.RUnlock() | ||
| return nil, status.Errorf(codes.Unavailable, "backup proposer unavailable for raft group %d", id) | ||
| } | ||
| groups = append(groups, backupGroup{id: id, reader: group, status: group.Status(), every: group.SnapshotEvery(), proposer: proposer}) |
There was a problem hiding this comment.
In deployments that include the reserved TSO Raft group 0, setupAdminService registers every runtime and this loop appends group 0 as a backup group. The resulting token later fails to encode because group IDs of 0 are rejected, and group 0 would also collide with the group-zero capacity-reservation tracker entry. This makes BeginBackup fail after fan-out instead of backing up the data groups, so the reserved/non-data group should be filtered before pinning/tokenization.
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 👍 / 👎.
| } | ||
| deadline := s.nowSnapshot().Add(ttl) | ||
| controlGroup := groups[0] | ||
| commits, err := s.pinBackupGroups(ctx, groups, controlGroup, pinID, readTS, deadline) |
There was a problem hiding this comment.
If a client write allocates commit_ts < readTS in the gap between the read fence and this backup-pin fan-out, but its Raft entry applies after the pin or during the later scan, the pin's HLC Observe(readTS) only affects future timestamp allocations and does not reject that already-stamped entry. Pages read at readTS can therefore miss or inconsistently include the write depending on whether it has applied before that key is scanned. BeginBackup needs to quiesce/drain already timestamped proposals, or otherwise wait until all entries with timestamps <= readTS have applied, before building the baseline or streaming.
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 👍 / 👎.
| switch entry.subtype { | ||
| case backupSubtypePin: | ||
| err = f.readTracker.ApplyPinWithDeadlineForGroup(entry.pin.PinID, f.shardGroupID, entry.pin.ReadTS, entry.pin.Deadline) | ||
| err := f.readTracker.ApplyPinWithDeadlineForGroup(entry.pin.PinID, f.shardGroupID, entry.pin.ReadTS, entry.pin.Deadline) |
There was a problem hiding this comment.
When a follower installs a Raft snapshot while a backup is active, the BackupPin entry may already be compacted out of the log, but this apply path only records the pin in the in-memory ActiveTimestampTracker; the FSM snapshot/restore path does not carry those pins. The restored replica can therefore run compaction without the read_ts fence until a later renewal reapplies it, and if it becomes the group leader in that window it can serve backup pages with versions below read_ts already removed. Persist active pins in snapshots or block snapshot installation/compaction while pins are live.
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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a85349d2d
ℹ️ 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".
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 (f30bd74):
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. |
|
Evidence for current HEAD
Validation:
Requesting a fresh latest-head review. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f30bd74b5e
ℹ️ 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".
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".
| scope, scoped, err := logicalbackup.ScopeForKey(key) | ||
| if err != nil { | ||
| return nil, 0, errors.Wrap(err, "classify backup key baseline") | ||
| } | ||
| if scoped { |
There was a problem hiding this comment.
When a transaction prepares an insert before the backup pin, ScanKeysAt only exposes the txn lock/intent records because the user key has no visible version yet; this loop classifies those internal keys as unscoped and continues. Since COMMIT deliberately bypasses the timestamp floor, that transaction can commit later with commit_ts <= read_ts, but the new user key was neither counted nor streamed, so the backup can silently miss committed data instead of failing BeginBackup on the pending primary.
Useful? React with 👍 / 👎.
| 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 👍 / 👎.
| snapshot, err := kv.CaptureBackupRouteSnapshotAt(ctx, s.catalog, ts) | ||
| return snapshot, errors.Wrap(err, "capture backup route snapshot") |
There was a problem hiding this comment.
On an admin endpoint that is a follower for the distribution-catalog group, this reads the process-local CatalogStore directly. The earlier pin/proposal fan-out returns when the leader applies, not when this local follower has caught up, so a route update committed at or before read_ts can be missing from the captured snapshot; the baseline and stream then scan the wrong historical owner for that key range.
Useful? React with 👍 / 👎.
| var ErrBackupApply = errors.New("backup fsm apply failed") | ||
| var ErrBackupTimestampFenced = errors.New("backup timestamp fence rejects stale write") | ||
|
|
||
| var backupTimestampFloorKey = []byte(TxnKeyPrefix + "backup|timestamp_floor") |
There was a problem hiding this comment.
This new durable floor lives at !txn|backup|timestamp_floor, but isTxnInternalKey only rejects the existing lock/intent/commit/rollback/meta prefixes, so RawKV callers can write this exact key. After a restart, reloadBackupTimestampFloor interprets the user value as the replicated fence: a non-8-byte value halts apply, while an arbitrary 8-byte value can raise the floor and make normal writes fail as stale.
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 👍 / 👎.
1 participant
Summary
Safety
Validation
Author: bootjp
Summary by CodeRabbit