Add live backup pin FSM substrate#1056
Conversation
📝 WalkthroughWalkthroughActiveTimestampTracker、バックアップFSM、固定長ワイヤ、AdminバックアップRPC、ルートスナップショット走査、タイムスタンプフロア、リーダー転送を追加し、起動配線と関連インターフェースを更新しました。 Changesライブ論理バックアップ
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant BackupClient
participant AdminServer
participant ShardedCoordinator
participant kvFSM
participant ActiveTimestampTracker
participant BackupScanner
BackupClient->>AdminServer: BeginBackup
AdminServer->>ShardedCoordinator: LeaseReadAllGroupsTimestamp
AdminServer->>kvFSM: reserve/pin proposals
kvFSM->>ActiveTimestampTracker: apply backup pin
AdminServer->>BackupScanner: capture snapshot and scan at read timestamp
BackupScanner-->>AdminServer: scoped backup records
AdminServer-->>BackupClient: StreamBackup responses
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
⚔️ Resolve merge conflicts
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. |
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 - add BeginBackup, RenewBackup, EndBackup, ListAdaptersAndScopes, and StreamBackup admin RPCs - replicate bounded backup pin reservations and per-group retention fences through Raft - gate backup start on live member capabilities and snapshot headroom - scan and classify user-visible keys at one pinned read timestamp using the existing logical encoders - rotate HMAC-protected renewal tokens with a hard deadline and reapply complete pins so partial delivery cannot leave a replica unprotected ## Safety - enforce a cluster-wide active backup cap with deterministic reservation and compensating release - reject expired tokens for renew, list, and stream while allowing EndBackup cleanup - retry transient per-group proposals and fail closed when renewal cannot finish before the prior deadline - scope compactor retention fences to their Raft group while preserving process-wide ordinary read pins ## Validation - go test ./adapter -run Test\(BeginBackup\|RenewBackup\|BackupToken\|StreamBackup\|BackupProtocol\|GetRaftGroups\|GetNodeVersion\|Admin\) -count=1 -timeout=240s - go test ./kv ./internal/backup . -count=1 -timeout=240s - go test -race ./adapter ./kv ./internal/backup -run Test\(BeginBackup\|RenewBackup\|BackupToken\|StreamBackup\|BackupProtocol\|GetRaftGroupsLeaderVersion\|GetRaftGroupsSnapshotsEachGroupOnce\|LeaderVersionProbeAttemptTimeout\|ActiveTimestampTracker\|ApplyBackup\|FSMCompactorScopesBackupPinsByGroup\|LiveDecoder\|ScopeForKey\) -count=1 -timeout=300s - golangci-lint run . ./adapter/... ./kv/... ./internal/backup/... --timeout=5m --allow-parallel-runners - buf generate - buf breaking --against the stacked admin API base Author: bootjp <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **新機能** * Admin 経由でバックアップの開始・更新・終了、対象スコープ一覧、キー・バリューのストリーミング取得に対応しました。 * スコープ単位の絞り込みと、バックアッププロトコルの対応可否をバージョンで判定します。 * **改善** * 固定した読み取り時点とルートスナップショットで安定した取得を実現しました。 * 予約/解除の管理を強化し、キーのみの走査・再利用を最適化しました。 * **信頼性** * TTL/ヘッドルーム/容量/互換性の事前検証、部分失敗時の補償、エラー時の後処理を強化しました。 * バックアップピンの同時上限を引き下げました。 <!-- end of auto-generated comment: release notes by coderabbit.ai -->
There was a problem hiding this comment.
🧹 Nitpick comments (4)
internal/backup/live.go (1)
20-25: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
redis分岐は冗長なデッドコードです。
s.Adapter == "redis"のとき"redis/" + s.Nameは default 分岐のs.Adapter + "/" + s.Nameと完全に同一の文字列になるため、この特別扱いは効果がありません。あわせてString()（Line 245-247）も同じadapter/name形式を返しており重複しています。分岐を削除し、必要ならID()をString()に委譲することを検討してください。♻️ 冗長分岐の削除案
func (s Scope) ID() string { - if s.Adapter == "redis" { - return "redis/" + s.Name - } return s.Adapter + "/" + s.Name }🤖 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 `@internal/backup/live.go` around lines 20 - 25, Remove the redundant s.Adapter == "redis" branch from Scope.ID and keep the single generic adapter/name construction. Reuse the existing String method if appropriate so ID and String share the same formatting without duplicating logic.kv/leader_admin_proposer.go (1)
77-128: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winリトライ/バックオフの骨格が複数箇所で重複しています。
forwardAdminWithRetry/runAdminForwardCycleはkv/leader_proxy.goのforwardWithRetry/runForwardCycleおよび新規のforwardLeaseReadと本質的に同じ「デッドライン計算→ループ→lastErr nilガード→バックオフ→再デッドラインチェック」の骨格です。詳細はファイル末尾の consolidated comment を参照してください。🤖 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/leader_admin_proposer.go` around lines 77 - 128, 重複しているリトライ／バックオフ処理を共通化し、leader_proxy.go の forwardWithRetry・runForwardCycle・forwardLeaseRead と leader_admin_proposer.go の forwardAdminWithRetry・runAdminForwardCycle が同じデッドライン管理、lastErr nil ガード、バックオフ、再チェックの骨格を共有するよう更新してください。各処理固有の forwardAdmin などの実行部分とエラー判定は既存の挙動を保ったまま、共通ヘルパーを再利用して重複実装を除去してください。kv/leader_proxy.go (2)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winリーダー転送のリトライ/デッドライン/バックオフ骨格が3箇所で重複しています。
kv/leader_proxy.goの既存forwardWithRetry/runForwardCycle、新規forwardLeaseRead/forwardLeaseReadOnce、そしてkv/leader_admin_proposer.goのforwardAdminWithRetry/runAdminForwardCycleは、いずれも「デッドライン計算→ループでフォワード実行→lastErr==nil時にErrLeaderNotFoundへフォールバック→デッドライン超過チェック→バックオフ→再チェック」という同一の制御フローを持ちます。戻り値型が異なるだけなので、Go genericsを使った共通ヘルパーへの抽出でリトライ挙動のバグ修正・変更を1箇所に集約できます。
kv/leader_proxy.go#L228-270:forwardLeaseRead/forwardLeaseReadOnceを共通ヘルパーを呼ぶ形に置き換える。kv/leader_admin_proposer.go#L77-128:forwardAdminWithRetry/runAdminForwardCycleも同じ共通ヘルパーを利用する形に置き換える。🤖 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/leader_proxy.go` at line 1, リーダー転送のリトライ制御がforwardWithRetry/runForwardCycle、forwardLeaseRead/forwardLeaseReadOnce、forwardAdminWithRetry/runAdminForwardCycleで重複しているため、デッドライン計算・実行ループ・ErrLeaderNotFoundフォールバック・バックオフを扱うGoジェネリック共通ヘルパーを追加する。forwardLeaseReadとforwardAdminWithRetryの各処理をそのヘルパー呼び出しへ置き換え、既存の戻り値型とリトライ挙動を維持し、個別の制御フロー重複を削除する。
228-270: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
forwardLeaseRead/forwardLeaseReadOnceは既存のforwardWithRetry/runForwardCycleとほぼ同一のリトライ骨格です。デッドライン計算、
lastErr==nilガード、バックオフ後の再デッドラインチェックが同ファイル内で二重実装されています。詳細はファイル末尾の consolidated comment を参照してください。🤖 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/leader_proxy.go` around lines 228 - 270, Refactor forwardLeaseRead to reuse the existing forwardWithRetry and runForwardCycle retry helpers instead of duplicating deadline calculation, lastErr handling, backoff, and deadline checks. Adapt the lease-read operation through these helpers while preserving transient-error retries, immediate propagation of non-transient errors, and the existing ErrLeaderNotFound fallback.
🤖 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.
Nitpick comments:
In `@internal/backup/live.go`:
- Around line 20-25: Remove the redundant s.Adapter == "redis" branch from
Scope.ID and keep the single generic adapter/name construction. Reuse the
existing String method if appropriate so ID and String share the same formatting
without duplicating logic.
In `@kv/leader_admin_proposer.go`:
- Around line 77-128: 重複しているリトライ／バックオフ処理を共通化し、leader_proxy.go の
forwardWithRetry・runForwardCycle・forwardLeaseRead と leader_admin_proposer.go の
forwardAdminWithRetry・runAdminForwardCycle が同じデッドライン管理、lastErr nil
ガード、バックオフ、再チェックの骨格を共有するよう更新してください。各処理固有の forwardAdmin
などの実行部分とエラー判定は既存の挙動を保ったまま、共通ヘルパーを再利用して重複実装を除去してください。
In `@kv/leader_proxy.go`:
- Line 1:
リーダー転送のリトライ制御がforwardWithRetry/runForwardCycle、forwardLeaseRead/forwardLeaseReadOnce、forwardAdminWithRetry/runAdminForwardCycleで重複しているため、デッドライン計算・実行ループ・ErrLeaderNotFoundフォールバック・バックオフを扱うGoジェネリック共通ヘルパーを追加する。forwardLeaseReadとforwardAdminWithRetryの各処理をそのヘルパー呼び出しへ置き換え、既存の戻り値型とリトライ挙動を維持し、個別の制御フロー重複を削除する。
- Around line 228-270: Refactor forwardLeaseRead to reuse the existing
forwardWithRetry and runForwardCycle retry helpers instead of duplicating
deadline calculation, lastErr handling, backoff, and deadline checks. Adapt the
lease-read operation through these helpers while preserving transient-error
retries, immediate propagation of non-transient errors, and the existing
ErrLeaderNotFound fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d36b16f2-fda5-408f-b7a0-4be2ee194b98
⛔ Files ignored due to path filters (5)
proto/admin.pb.gois excluded by!**/*.pb.goproto/admin_grpc.pb.gois excluded by!**/*.pb.goproto/internal.pb.gois excluded by!**/*.pb.goproto/internal_grpc.pb.gois excluded by!**/*.pb.goproto/service.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (51)
adapter/admin_backup.goadapter/admin_backup_test.goadapter/admin_grpc.goadapter/admin_grpc_test.goadapter/internal.goadapter/internal_admin_proposal_test.godistribution/engine.godocs/design/2026_04_29_proposed_logical_backup.mdinternal/backup/live.gointernal/backup/live_test.gointernal/backup/s3.gointernal/backup/sqs.gointernal/raftadmin/server_test.gointernal/raftengine/engine.gointernal/raftengine/etcd/engine.gointernal/raftengine/etcd/wal_purge_test.gokv/active_timestamp_tracker.gokv/active_timestamp_tracker_test.gokv/backup_codec.gokv/backup_codec_test.gokv/backup_scan.gokv/compactor.gokv/compactor_test.gokv/coordinator.gokv/coordinator_retry_test.gokv/fsm.gokv/fsm_backup.gokv/fsm_backup_test.gokv/keyviz_label.gokv/leader_admin_proposer.gokv/leader_admin_proposer_test.gokv/leader_proxy.gokv/leader_proxy_test.gokv/lease_read_test.gokv/shard_store.gokv/shard_store_test.gokv/sharded_coordinator.gokv/sharded_coordinator_leader_test.gokv/sharded_coordinator_txn_test.gokv/tso_test.gokv/txn_keys.gomain.gomain_admin.gomain_admin_test.gomain_bootstrap_e2e_test.gomain_encryption_write_wiring.gomain_sqs_leadership_refusal_test.gomultiraft_runtime_test.goproto/admin.protoproto/internal.protoproto/service.proto
🚧 Files skipped from review as they are similar to previous changes (4)
- multiraft_runtime_test.go
- main_encryption_write_wiring.go
- kv/active_timestamp_tracker_test.go
- kv/active_timestamp_tracker.go
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1eddbb1aa2
ℹ️ 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 nil, err | ||
| } | ||
| defer s.forgetBackupSession(tok.pinID) |
There was a problem hiding this comment.
When a client's background RenewBackup overlaps with EndBackup, this defer leaves the session live while the release entries are being proposed. If the group log orders EndBackup's Release before the in-flight renewal's complete Pin, finishRenewBackup can still extend the session before this defer runs, then EndBackup returns and forgets the session without issuing another release; the renewed pin remains active until its deadline and continues blocking compaction/capacity after the backup was ended. Mark the session as closing (or otherwise reject/serialize renewals for the pin) before proposing releases so no renewal can commit after the final release.
Useful? React with 👍 / 👎.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if err := s.requireUnexpiredBackupToken(tok); err != nil { |
There was a problem hiding this comment.
If a RenewBackup RPC commits successfully but the response carrying the rotated token is lost, the server has already extended the in-memory session in finishRenewBackup, but the client can only retry with the old token. Once the old embedded deadline passes, this pre-session check rejects the retry even though the pin is still live until the committed later deadline, leaving the backup unable to obtain the current token and eventually losing its retention fence. Check the live session for the same pin/readTS before rejecting, or return the session's current token idempotently.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| func (s *ShardStore) txnCommitTSAt(ctx context.Context, primaryKey []byte, startTS uint64, ts uint64) (uint64, bool, error) { | ||
| b, err := s.GetAt(ctx, txnCommitKey(primaryKey, startTS), ts) |
There was a problem hiding this comment.
When a backup races a route split or move, ValidateBackupSnapshotAt scans locks from the captured read-ts route set, but this lookup resolves the primary commit record through the live ShardStore routing table. If the primary key has moved after read_ts, GetAt checks the new owner instead of the historical group that contains the commit/rollback record, so an already-resolved transaction is treated as pending and BeginBackup fails despite a clean snapshot. Read the txn status via the captured route/group for the primary key rather than live routing.
Useful? React with 👍 / 👎.
Summary
Tests
Author: bootjp
Summary by CodeRabbit