Add live backup pin FSM substrate#1056
Conversation
|
Warning Review limit reached
Next review available in: 55 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 selected for processing (4)
📝 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.
bootjp
force-pushed
the
design/live-backup-pin-substrate
branch
from
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 👍 / 👎.
bootjp
force-pushed
the
design/live-backup-pin-substrate
branch
from
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 👍 / 👎.
bootjp
force-pushed
the
design/live-backup-pin-substrate
branch
from
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". |
1 participant
Summary
Tests
Author: bootjp
Summary by CodeRabbit
新機能
改善