Add live point-in-time logical backup producer#1128
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:
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 is Verification:
Please review the latest head. @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 Phase 1 of the logical backup feature, enabling point-in-time consistent logical backups from a running elastickv cluster. It introduces the elastickv-backup CLI tool, adds server-side configurations for active backup pins, implements the live backup producer lifecycle (including streaming and renewal), and updates design documents and runbooks. The review feedback highlights a potential nil pointer dereference on Windows when loading system certificates, portability issues with using os.Link for manifest publication on filesystems that do not support hard links, and noisy warning logs during graceful shutdown due to unsuppressed context cancellation 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.
|
Updated the latest head
Verification:
Caller audit for the manifest error-semantics change found one production caller ( @codex review |
830d645 to
6a85349
Compare
d5a5934 to
83f3a9f
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 |
![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: 83f3a9f6aa
ℹ️ 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".
|
Current signed HEAD Evidence:
Verification:
Caller audit for the @codex review |
There was a problem hiding this comment.
💡 Codex Review
elastickv/internal/backup/live.go
Lines 66 to 67 in 56277d2
Exclude DynamoDB generation counters from scopes
When a DynamoDB table has been deleted, the live adapter leaves the !ddb|meta|gen| counter behind while the table schema is gone, and the decoder's HandleTableGen is a no-op. Classifying that counter as a scoped table makes BeginBackup/ListAdaptersAndScopes select a deleted table and lets the live manifest publish dynamodb.tables for a scope that has no _schema.json; the native restore path then emits no table even though the manifest says it was dumped. Treat this like the other generation counters that are already unscoped, or only count it when the schema record is present.
ℹ️ 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".
|
Latest signed HEAD
Verification:
Caller audit for @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7d4834f121
ℹ️ 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".
|
Evidence for current HEAD
Validation:
Requesting a fresh latest-head review. @codex review |
ed0498d to
53e6402
Compare
|
Rebase evidence for current HEAD
Post-rebase validation:
The broad unfiltered adapter suite reached its 600-second timeout in Raft integration cleanup; the focused backup/forwarding adapter suite passed and GitHub CI is now running on this head. Requesting a fresh latest-head review after the dependency rebase. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53e64025e8
ℹ️ 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".
53e6402 to
7f11f84
Compare
|
Rebase evidence for current HEAD 7f11f84:
Validation:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f11f84972
ℹ️ 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".
99b37ee to
688c1b2
Compare
|
Latest signed HEAD
Validation:
All visible commits are signed by @codex review |
688c1b2 to
7c134ba
Compare
|
Latest signed HEAD The prior CI Validation after this update:
Current-head unresolved review threads are 0. CI is running again for this head. @codex review |
|
Codex Review: Didn't find any major issues. Keep it up! 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". |
7c134ba to
8f76d8c
Compare
|
Latest signed HEAD The rebase conflict was limited to Validation after this rebase:
Current-head unresolved review threads are 0. CI is running for this head. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f76d8c373
ℹ️ 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".
d590c6a to
db7ec7d
Compare
|
Latest signed HEAD
Validation:
Caller audit covered Current-head unresolved review threads are 0. CI is running for this head. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db7ec7d0ce
ℹ️ 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 !logicalbackup.AdapterEnabled(selection.adapters, adapter) { | ||
| return false, nil | ||
| } | ||
| scope, scoped, err := logicalbackup.ScopeForKey(key) |
There was a problem hiding this comment.
When a dump is narrowed only by Scopes (the CLI default still sends all adapters), this check treats every adapter as enabled and then calls ScopeForKey on keys from adapters that have no requested scope. In a Redis-only scoped dump, for example, a malformed or future-format S3/DynamoDB key can still abort BeginBackup even though that adapter will be omitted; derive/consult the adapter set present in selection.scopes before parsing the scoped key.
Useful? React with 👍 / 👎.
| adapter, ok := AdapterForKey(key) | ||
| return ok && adapter == adapterRedis |
There was a problem hiding this comment.
For Redis scopes this returns true for every Redis key, so the baseline metadata pass materializes user payload values such as large strings, hash/list values, and stream entries. On Redis-heavy backups, BeginBackup can therefore load a page of large payloads just to compute expected counts, defeating the key-only/metadata-only preflight path; restrict value reads to Redis metadata/TTL records that are actually needed and count payload-bearing keys from their keys.
Useful? React with 👍 / 👎.
| counts, _, err := s.scanBackupScopeCounts( | ||
| ctx, routes, tok.readTS, groups, | ||
| backupBaselineSelection{adapters: logicalbackup.AllAdapters()}, |
There was a problem hiding this comment.
After BeginBackup honors an --adapter redis selection, the producer still calls ListAdaptersAndScopes, and this server path scans every adapter with AllAdapters(). That means a disabled adapter can still abort an otherwise valid filtered dump (for example a malformed DynamoDB/S3 key fails classification during this list pass), even though its values were intentionally excluded from the baseline and will not be streamed; carry the requested adapter/scope selection into the listing RPC or avoid the full-adapter scan for filtered runs.
Useful? React with 👍 / 👎.
| // Empty means all adapters. Non-empty lets the server build the | ||
| // expected-key baseline only for adapters the producer will emit. | ||
| repeated string adapters = 2; | ||
| // Empty means every scope in the selected adapters. Non-empty narrows the | ||
| // expected-key baseline to the exact adapter/scope pairs requested by the | ||
| // producer. | ||
| repeated BackupScope scopes = 3; |
There was a problem hiding this comment.
These new request fields are silently ignored by older live-backup servers because protobuf preserves forward compatibility, while backup_protocol_version still advertises v1. During a rolling upgrade or when the CLI contacts an old admin endpoint, an explicit scoped dump can receive an unfiltered baseline and then select extra baseline scopes (especially with the default all-adapter setting), producing a broader dump than requested; bump/gate the client-server backup protocol before relying on adapters and scopes.
Useful? React with 👍 / 👎.
| if _, ok := metadataKeys[string(pair.Key)]; !ok { | ||
| continue | ||
| } | ||
| if err := counter.AddValue(pair.Key, pair.Value); err != nil { |
There was a problem hiding this comment.
When a key-only baseline pass records metadata keys and one of those keys is missing from this second value pass, the loop simply finishes and finalizes a lower retained count because observed keys are never tracked as consumed. If a backup pin is lost or compaction races between the key pass and the metadata pass, this can turn a missing table/bucket/queue metadata record into an omitted baseline instead of the expected shortfall failure; remove each consumed metadata key and fail if any remain after the scan.
Useful? React with 👍 / 👎.
1 participant
Summary
Stack
Risk
The data path is opt-in through the new backup CLI and admin RPCs. The main correctness risks are publishing a completion marker after pin loss and reading from a changing route catalog; both are covered by pinned-route and renewal-quiescence tests.
Verification
go test ./internal/backup ./cmd/elastickv-backup -count=1go test -race ./internal/backup -run 'Test(RunLiveBackup|ProducerCrash|CrossAdapterConsistency|ExpectedKeysBaseline)' -count=1go test ./adapter -run 'Test(BeginBackup|RenewBackup|StreamBackup|ListBackup|BackupProtocol|InternalForwardAdminProposal)' -count=1go test ./kv -run 'Test(Backup|BeginBackup|Pin|BackupScanner|ShardStoreScanAt_RoutesFilesystem)' -count=1make -C proto genfollowed by a clean generated diffgolangci-lint run ./... --timeout=15m --allow-parallel-runners -j 2git diff --checkdocs/design/2026_04_29_proposed_logical_backup.md