Add live backup pin FSM substrate - #1056
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughライブバックアップ機能を追加しました。期限付きバックアップピン、FSMのtimestamp floor、管理RPC、固定ルート走査、リーダー転送、ライブデコーダー、RedisレガシーBlob対応、スナップショット延期処理を実装しています。 Changesライブバックアップ基盤
Estimated code review effort: 5 (Critical) | ~120 minutes Merge Risk: 🟡 Moderate · up to This PR adds cluster-wide live-backup pinning and streaming; if ending a backup partially fails, the control reservation and session can be cleared while some shard pins remain, temporarily delaying compaction or snapshots until retry or expiry. The current head also retains a conditional legacy-record compatibility risk and tests that may not exercise intended routing and error paths, so merge should wait for cleanup reconciliation or explicit owner acceptance and targeted test fixes. Sequence Diagram(s)sequenceDiagram
participant AdminClient
participant AdminServer
participant ShardedCoordinator
participant kvFSM
participant BackupScanner
AdminClient->>AdminServer: BeginBackup
AdminServer->>ShardedCoordinator: LeaseReadAllGroupsTimestamp
AdminServer->>kvFSM: propose backup pin and timestamp floor
kvFSM-->>AdminServer: applied backup state
AdminServer->>BackupScanner: capture routes and validate snapshot
AdminClient->>AdminServer: StreamBackup
AdminServer->>BackupScanner: scan captured routes
BackupScanner-->>AdminClient: BackupKV stream
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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 |
|
@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 (9a7491c):
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. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a backup pinning mechanism to the ActiveTimestampTracker and kvFSM to retain MVCC versions at live-backup read timestamps during background compaction. It adds FSM commands for pinning, extending, and releasing backup pins, alongside a background sweeper to reap expired pins. The reviewer provided critical feedback to improve robustness: first, expired backup pins should be ignored in Oldest() to avoid blocking compaction before the sweeper runs; second, validation and limit errors must not halt the FSM to prevent DoS vulnerabilities; and third, a graceful shutdown mechanism (Close() and stopCh) should be added to the tracker to prevent goroutine leaks from the background sweeper.
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.
9a7491c to
b9e3e10
Compare
|
Addressed latest-head review findings:
Validation:
|
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 (b9e3e10):
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. |
![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: b9e3e104a4
ℹ️ 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 _, exists := t.backupPins[pinID]; !exists && len(t.backupPins) >= t.maxBackupPins { | ||
| return errors.WithStack(ErrTooManyActiveBackups) |
There was a problem hiding this comment.
When a replica still has maxBackupPins expired entries that Oldest() already ignores but the sweeper has not deleted yet, this raw len(t.backupPins) check returns ErrTooManyActiveBackups for a new valid backup pin. Since applyBackup treats that error as non-fatal, the Raft entry is advanced without recording the pin on that replica; if it later compacts or becomes the backup-serving leader, the backup read timestamp is not retained. Reap or exclude expired pins before applying the cap.
Useful? React with 👍 / 👎.
| pin.deadline = deadline | ||
| t.backupPins[pinID] = pin |
There was a problem hiding this comment.
If a BackupExtend arrives after the previous deadline but before the sweeper has deleted the entry, Oldest() has already stopped honoring this pin, so compaction may have advanced past the backup's read timestamp during that gap. This assignment makes the expired pin active again and reports a successful renewal, allowing a backup to continue even though its retention fence was temporarily absent. Treat expired pins as missing/invalid before extending them.
Useful? React with 👍 / 👎.
b9e3e10 to
91a7d04
Compare
|
Addressed the latest-head findings:
Validation:
|
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 (91a7d04):
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: 91a7d04265
ℹ️ 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 errors.Is(err, ErrTooManyActiveBackups) || errors.Is(err, ErrInvalidBackupPin) { | ||
| return err |
There was a problem hiding this comment.
When this branch is reached for the local-state-dependent errors (capacity, missing/expired renewals), applyBackup returns a plain error instead of haltErr, so the Raft engine treats the entry as applied and only the proposer observes the response. In a multi-replica group, one replica can hit this branch because its tracker still has an unexpired pin or its clock has already expired a renewal while the leader installed the new/extended fence; that replica then advances without retaining read_ts, and after a leader change or local compaction the backup can lose the version it was meant to protect.
Useful? React with 👍 / 👎.
| t.mu.Lock() | ||
| expired := t.reapExpiredBackupPinsLocked(time.Now()) | ||
| key := newBackupPinKey(pinID, groupID) | ||
| if _, exists := t.backupPins[key]; !exists && len(t.backupPins) >= t.maxBackupPins { |
There was a problem hiding this comment.
Now that backupPins is keyed by (pin_id, groupID) and every shard FSM shares this tracker, len(t.backupPins) charges one slot per Raft group. A single logical backup fan-out with the same pin_id across more than 64 groups will fill the default limit and the next group’s BackupPin returns ErrTooManyActiveBackups, so large sharded deployments cannot start even one backup unless the limit is raised by group count.
Useful? React with 👍 / 👎.
91a7d04 to
b04b7fb
Compare
|
Addressed latest-head review findings:
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 (b04b7fb):
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. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
main.go (1)
389-416: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
readTracker.Close()を shutdown cleanup に追加してください。ActiveTimestampTrackerはスイーパー goroutine を持つため、cleanup.Add(readTracker.Close)で終了時に止める必要があります。🤖 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 `@main.go` around lines 389 - 416, Register readTracker.Close with the shutdown cleanup after creating the ActiveTimestampTracker, using cleanup.Add(readTracker.Close), so its sweeper goroutine is stopped during termination.
🧹 Nitpick comments (1)
kv/fsm.go (1)
372-373: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
applyBackup(data)とapplyHLCLease(data[1:])でオペコードバイトの扱いが一貫していません。
applyHLCLeaseはdata[1:]（オペコード除外）を渡すのに対し、applyBackupはdata（オペコード含む）をそのまま渡しています。decodeBackupEntryがフルデータを期待しているため機能上は問題ありませんが、将来的な保守で混乱を招く可能性があります。🤖 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 `@kv/fsm.go` around lines 372 - 373, applyBackup と applyHLCLease で入力データのオペコード除外方法を統一してください。applyBackup 呼び出し側では applyHLCLease と同様にオペコードバイトを除いた data[1:] を渡し、decodeBackupEntry の期待する入力形式も確認して関連処理を一貫させてください。
🤖 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.
Outside diff comments:
In `@main.go`:
- Around line 389-416: Register readTracker.Close with the shutdown cleanup
after creating the ActiveTimestampTracker, using cleanup.Add(readTracker.Close),
so its sweeper goroutine is stopped during termination.
---
Nitpick comments:
In `@kv/fsm.go`:
- Around line 372-373: applyBackup と applyHLCLease
で入力データのオペコード除外方法を統一してください。applyBackup 呼び出し側では applyHLCLease と同様にオペコードバイトを除いた
data[1:] を渡し、decodeBackupEntry の期待する入力形式も確認して関連処理を一貫させてください。
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b885a775-9adc-47fb-833c-e1990bc6fa44
📒 Files selected for processing (11)
kv/active_timestamp_tracker.gokv/active_timestamp_tracker_test.gokv/backup_codec.gokv/backup_codec_test.gokv/fsm.gokv/fsm_backup.gokv/fsm_backup_test.gomain.gomain_bootstrap_e2e_test.gomain_encryption_write_wiring.gomultiraft_runtime_test.go
|
Addressed latest-head review findings:
Validation:
|
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 (a39218b):
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. 👍 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". |
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 (4d56518):
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: 4d56518ea2
ℹ️ 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 errors.WithStack(ErrInvalidBackupPin) | ||
| } | ||
| pin.deadline = deadline |
There was a problem hiding this comment.
When two BackupExtend proposals for the same pin are in flight, or a retry of an older renewal commits after a newer one, this unconditional assignment can move the deadline backwards. If that stale deadline expires before the next renewal, Oldest() stops honoring the pin and local compaction can advance past the backup read timestamp while the backup is still running; apply should keep max(existing deadline, requested deadline) rather than shortening it.
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 (2f77c32):
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. |
The revert-check loop reinserted the same comment block on each pass, so the ClearBackupPinsForGroup call in Restore carried three verbatim copies. Cosmetic only -- no behaviour change. Claude-Session: https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
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 (b216434):
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: b21643474f
ℹ️ 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".
| case hasAnyBackupPrefix(key, | ||
| S3BucketMetaPrefix, S3ObjectManifestPrefix, | ||
| S3UploadMetaPrefix, S3UploadPartPrefix, S3BlobPrefix, S3GCUploadPrefix, S3RoutePrefix, | ||
| ): |
There was a problem hiding this comment.
When S3 blob offload is enabled, object manifests reference records under S3ChunkRefPrefix and payloads under S3ChunkBlobPrefix, but neither prefix is recognized by this S3 branch. backupKeySelected consequently rejects both as unscoped even when no scope filter is requested (adapter/admin_backup.go:638-643), while S3Encoder requires them to reconstruct offloaded objects. Such backups stream the manifest without its backing chunks and fail finalization with missing/incomplete blob data; route chunk refs to their bucket scope and ensure their referenced content-addressed blobs are included.
Useful? React with 👍 / 👎.
An offloaded object's manifest names its payload through !s3|chunkref| rows, but ScopeForKey did not recognise that family, so backupKeySelected rejected them as unscoped and the dump carried manifests with no chunk references. The offline decoder already routes the family; only the live classifier was missing it. The reference key carries the bucket, so it scopes like every other object-level row. Claude-Session: https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE
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 (09fc754):
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. |
|
Chunk references — fixed. Verified the claim: Content blobs — not fixed, and I do not think the suggested shape works. Two things block it, both checkable:
A live backup of offloaded objects therefore needs the producer to fetch blobs by digest over
|
|
Claude finished @bootjp's task in 1m 3s —— View job Review:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 09fc7541fe
ℹ️ 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".
| // capacity, until its deadline lapsed. The durable floor below is the part | ||
| // that does survive, and it is reloaded right after. | ||
| if f.readTracker != nil { | ||
| f.readTracker.ClearBackupPinsForGroup(f.shardGroupID) |
There was a problem hiding this comment.
Fresh evidence in this fix shows that restore clears only entries keyed by the shard's actual group ID, while BackupReserve is stored under synthetic group ID 0 via PinWithDeadline. If the control-group follower applied a reservation but missed the later unreserve before installing a post-release snapshot, that reservation survives this call and continues consuming a global active-backup slot until its deadline; repeated restores can therefore cause ErrTooManyActiveBackups when no backup is active. Reconcile the reservation owned by the restoring control group as well.
Useful? React with 👍 / 👎.
|
Confirmed by reading the apply path: The repair that looks obvious — key reservations by the applying FSM's So the real options are:
I did not implement any of them because each redefines what a capacity slot is, and that is the part you own. Say which shape you want and I will land it with the round-trip test. |
|
Claude finished @bootjp's task in 8m 41s —— View job Review:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 09fc7541fe
ℹ️ 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 f.readTracker != nil { | ||
| f.readTracker.ClearBackupPinsForGroup(f.shardGroupID) | ||
| } | ||
| f.backupFloorLoadErr = f.reloadBackupTimestampFloor(context.Background()) |
There was a problem hiding this comment.
When a running replica installs a snapshot whose persisted backup floor is above a TSO batch it cached before falling behind, this reload updates only the FSM atomic; observeStoreLastCommitTS advances the HLC but does not invoke ShardedCoordinator.ObserveTimestampFloor or invalidateTimestampWindow. If that replica later handles writes, it can continue issuing cached timestamps at or below the restored floor, causing every such proposal to be rejected by verifyBackupTimestampFloor until the stale window is exhausted. Propagate the successfully restored floor through the coordinator's floor observer so cached timestamp windows are discarded.
Useful? React with 👍 / 👎.
| if len(rest) == 0 { | ||
| return nil, false |
There was a problem hiding this comment.
When an unmigrated legacy hash or set uses the empty Redis key, its storage key is exactly !redis|hash| or !redis|set|; the command paths pass cmd.Args[1] through without rejecting an empty key, and the equivalent legacy zset parser also accepts an empty remainder. This shared helper instead classifies that valid record as malformed, so HandleHashLegacyBlob or HandleSetLegacyBlob makes the logical backup decoder fail on data that the Redis API can create. Distinguish a missing prefix from a present prefix with an empty user key.
Useful? React with 👍 / 👎.
| key []byte, | ||
| ts uint64, | ||
| ) ([]byte, error) { | ||
| rkey := routeKey(key) |
There was a problem hiding this comment.
Fresh evidence after the captured-route fix is that this path still calls routeKey directly and never consults partitionResolver. For a committed or rolled-back transaction whose primary is a partitioned SQS key, the commit/rollback row is stored in the resolver-selected group, while routeKey collapses the key into the catalog-routed SQS namespace; validation therefore reads the status from the wrong group, treats the residual lock as pending, and makes BeginBackup fail on a clean snapshot. Resolve the transaction's embedded user key through the partition resolver first and perform the fenced status read against that group.
Useful? React with 👍 / 👎.
The empty key is legal in Redis, so an unmigrated hash or set stored under it has its blob at exactly !redis|hash| or !redis|set|. The shared key parser I added treated that as malformed, which fails the decoder on data the Redis API can create; parseZSetLegacyBlobKey has always accepted it. Reject only a missing prefix, and let the value carry the validity check. Claude-Session: https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE
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 (a110654):
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. |
|
Tests: a round-trip through the empty key (the emitted filename is discovered from the directory rather than guessed at), plus the malformed case moved to a key with the wrong family prefix, which is what "malformed key" actually means. Revert-checked.
The other three at-HEAD findings on this PR are not addressed:
|
|
Claude finished @bootjp's task in 5m 2s —— View job Review:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a110654892
ℹ️ 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 scopeForDDBKey(key) | ||
| case hasAnyBackupPrefix(key, | ||
| S3BucketMetaPrefix, S3ObjectManifestPrefix, | ||
| S3UploadMetaPrefix, S3UploadPartPrefix, S3BlobPrefix, S3ChunkRefPrefix, |
There was a problem hiding this comment.
Fresh evidence at current HEAD is that 09fc7541 adds only S3ChunkRefPrefix here, while S3ChunkBlobPrefix remains unrecognized and therefore unscoped. With S3 blob offload enabled, backupKeySelected rejects those chunk-blob records (adapter/admin_backup.go:638-643), but S3Encoder can resolve each streamed reference only from payloads received by HandleChunkBlob (internal/backup/s3.go:492-537); consequently an offloaded object's manifest and references are streamed without its content and decoder finalization fails. The live path must also retrieve and emit every referenced content-addressed chunk blob.
Useful? React with 👍 / 👎.
A content-addressed chunk blob carries no bucket in its key, so ScopeForKey has no scope to stream it under and returned "not scoped" -- silently dropping it while the object's manifest and chunk references still went out. S3Encoder resolves each streamed reference only from payloads delivered to HandleChunkBlob, so such a dump cannot finalize: the live path would have produced a backup that cannot be restored. Reject the key instead. Emitting these blobs is part of enabling S3 blob offload, which s3BlobOffloadDecision still forces back to the legacy path until the M3 reference-counting and orphan-scanner work sets blobOffloadGCReady -- nothing in the tree sets it outside a test today, so no cluster can produce a chunk blob yet. When that changes, a backup fails loudly here rather than quietly emitting an undecodable dump. Claude-Session: https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE
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 (afe538e):
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: afe538e497
ℹ️ 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".
| s.compensateBackupRelease(groups[0], groups, tok.pinID) | ||
| s.forgetBackupSession(tok.pinID) |
There was a problem hiding this comment.
When two RenewBackup calls for the same token overlap and one succeeds while the other encounters a partial fan-out failure, the failing call unconditionally releases every group pin and forgets the shared session. If the successful call passes its final liveness check and extends the session before this cleanup completes, it can still return a renewed token even though the other call then removes its session and retention pins, allowing compaction to invalidate the backup. Serialize renewals per pin or make cleanup conditional on the failing attempt still owning the session generation.
Useful? React with 👍 / 👎.
| if _, _, err := proposeBackupAll(ctx, []backupGroup{controlGroup}, reserveEntry); err != nil { | ||
| if backupCapacityReservationFull(err) { | ||
| return nil, status.Errorf(codes.ResourceExhausted, "%s", kv.ErrTooManyActiveBackups) | ||
| } | ||
| return nil, status.Errorf(codes.Unavailable, "reserve backup capacity: %v", err) |
There was a problem hiding this comment.
If the control-group BackupReserve commits but its response is lost or the caller context expires, proposeBackupAll returns an error here and BeginBackup exits without proposing BackupUnreserve. The unacknowledged reservation then consumes one of the small global active-backup slots until its TTL despite no backup being returned to the caller; compensate the control group on non-capacity reservation errors because unreserve is idempotent.
Useful? React with 👍 / 👎.
|
Mechanism confirmed; the severity claim does not hold at HEAD. I traced it rather than taking it as given:
What I did instead of the suggested remedy. The report asks the live path to "retrieve and emit every referenced content-addressed chunk blob". I did not do that here: a chunk blob is content-addressed with no bucket in its key, so it has no backup scope to be streamed under, and deciding how it should be scoped (per referencing bucket? duplicated across buckets that share a blob? a scope-less side channel?) is a design question that belongs with the M3 enablement work, not a patch on this PR. What this PR does is stop the silent failure: Tests ( Verification: Self-review passes. Data loss: strictly fewer silent omissions — the guard converts a dropped record into a failure, and drops nothing new. Concurrency: pure function, no state. Performance: one prefix compare per key, ahead of the existing S3 prefix set. Consistency: the decoder's chunk-blob route ( If you'd rather the live path actually carry the blobs, that wants a |
|
Claude finished @bootjp's task in 1m 32s —— View job Review:
|
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Summary
Tests
Author: bootjp
Summary by CodeRabbit
新機能
改善