Add live backup pin FSM substrate#1056
Conversation
|
Warning Review limit reached
Next review available in: 56 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (29)
📝 WalkthroughWalkthroughActiveTimestampTrackerに期限付きバックアップピン追跡を追加し、固定長ワイヤ形式、FSM適用処理、共有トラッカーの起動配線、関連テストを実装しました。 Changesバックアップピン処理
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant RaftApply
participant kvFSM
participant applyBackup
participant decodeBackupEntry
participant ActiveTimestampTracker
RaftApply->>kvFSM: バックアップペイロードを適用
kvFSM->>applyBackup: raftEncodeBackupを処理
applyBackup->>decodeBackupEntry: エントリをデコード
decodeBackupEntry-->>applyBackup: Pin/Extend/Releaseを返却
applyBackup->>ActiveTimestampTracker: バックアップピン操作を適用
🚥 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 |
|
@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. |
|
To use Codex here, create a Codex account and connect to github. |
|
Addressed the current-head P1 and CI failure in signed commit
Validation:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f77c32f76
ℹ️ 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 !pin.deadline.After(time.Now()) { | ||
| delete(t.backupPins, key) | ||
| t.mu.Unlock() |
There was a problem hiding this comment.
In the FSM apply path (ApplyExtendForGroup(..., false) from kv/fsm_backup.go), this wall-clock expiry check makes a committed renewal disappear on any replica that applies or cold-start replays the BackupExtend after the previous deadline has passed locally, even when the extend entry carries a later future deadline. After that replica catches up, its compactor sees no backup pin and can compact away versions still needed by the live backup, while faster replicas keep the renewed fence. The apply/replay path should make the deadline monotonic from the log entry instead of deleting the pin based on local apply time.
Useful? React with 👍 / 👎.
| for _, pin := range t.backupPins { | ||
| if !pin.deadline.After(now) { | ||
| continue | ||
| } | ||
| if oldest == 0 || pin.readTS < oldest { | ||
| oldest = pin.readTS |
There was a problem hiding this comment.
In multi-group deployments where a backup covers only a subset of groups, this global scan feeds every group compactor: main.go passes one readTracker to startFSMCompactorIfEnabled, and FSMCompactor.safeMinTS calls tracker.Oldest() for each runtime. Because Oldest() returns the minimum backup pin across all groupIDs, a pin applied for group 1 also lowers the compaction watermark for unrelated groups, so a long or leaked group-local backup can freeze compaction and grow disk outside the backup scope. The backup-pin portion of the tracker should be queried by runtime.GroupID (while keeping ordinary active read pins global if needed).
Useful? React with 👍 / 👎.
| t.logExpiredBackupPins(expired) | ||
| return errors.WithStack(ErrTooManyActiveBackups) | ||
| } | ||
| t.backupPins[key] = backupDeadlinePin{readTS: readTS, deadline: deadline} |
There was a problem hiding this comment.
When a BeginBackup pin proposal is retried after the original commit response is lost, the stale BackupPin entry can be committed after a later BackupExtend; this assignment then overwrites the already-extended record with the old deadline (and can also move readTS forward if the ID is reused), shortening the retention fence while the backup is still scanning. Duplicate applies for an existing (pinID, groupID) should preserve the safest state, e.g. min(existing.readTS, requested.readTS) and max(existing.deadline, requested.deadline), or reject non-identical duplicate pins.
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 (0584f06):
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. |
|
Addressed all current-head findings in signed commit
Caller audit:
Validation:
|
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
## Summary - add the live-backup admin RPC surface for BeginBackup, RenewBackup, EndBackup, ListAdaptersAndScopes, and GetNodeVersion - expose the node build version through the gRPC Admin service and surface leader node versions in GetRaftGroups via an async cache - expose the Raft snapshot threshold through the engine interface so backup headroom checks can use the configured value Base: #1056 ## Validation - go test ./adapter -run 'Test(GetNodeVersion|GetRaftGroups|GroupOrdering|AdminTokenAuth|GetClusterOverview)' -count=1 - go test . -count=1 - go test ./internal/raftengine/etcd -run 'Test(SnapshotEvery|EngineSnapshotEvery)' -count=1 - go test ./kv -run 'Test(CoordinateDispatch|LeaderProxy|LeaseRead|ShardedCoordinator|ValidateReadOnlyShards|LeaseWarmup)' -count=1 - go test ./internal/raftadmin -count=1 - golangci-lint run --timeout=5m - go test ./... -run '^$' -count=1 - git diff --check - git verify-commit HEAD Author: bootjp
|
@copilot resolve the merge conflicts in this pull request |
Merge conflicts resolved in commit Changes made:
Build and tests ( |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 471c6afe3f
ℹ️ 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".
| type BackupExtendEntry struct { | ||
| PinID BackupPinID | ||
| Deadline time.Time |
There was a problem hiding this comment.
When a backup is renewed long enough for Raft to snapshot/compact the original BackupPin, a restarted replica or one catching up via snapshot restores only MVCC data, not the in-memory tracker. The remaining BackupExtend entries carry only PinID and Deadline, and ApplyExtendForGroup no-ops when the pin is missing, so that replica cannot recreate the read_ts retention fence and its compactor can remove versions still being scanned. This affects long-running backups that cross a snapshot/restore boundary; include the read timestamp in renewals or persist the active pins in snapshots.
Useful? React with 👍 / 👎.
| // RaftAppliedIndex from the engine's appliedIndex. | ||
| func (f *kvFSM) IsVolatileOnlyPayload(payload []byte) bool { | ||
| return len(payload) > 0 && payload[0] == raftEncodeHLCLease | ||
| return len(payload) > 0 && (payload[0] == raftEncodeHLCLease || payload[0] == raftEncodeBackup) |
There was a problem hiding this comment.
This classifies every payload starting with the backup opcode as safe to replay during the cold-start skip path, but that path calls StateMachine.Apply and discards the returned value. If the skipped WAL tail contains an unknown future backup subtype, malformed backup payload, or a backup entry on an FSM without the tracker, normal apply would halt via applyBackup, while cold start silently seeds Raft past the entry without applying the intended fence/release effect. Decode known backup subtypes here, or make volatile replay honor HaltApply.
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 (748c75b):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
|
Updated the signed HEAD to Verification:
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: 748c75bb5d
ℹ️ 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 (13d22a8):
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
Caller audit:
Verification:
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! 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". |
13d22a8 to
26d45ba
Compare
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 (26d45ba):
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. |
|
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 |
|
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
新機能
改善