Add TSO state machine#1150
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Warning Review limit reached
Next review available in: 44 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 Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughTSO専用Raft FSM、HLC ceilingのフェンス処理、snapshot復元、関連テストと設計文書を更新しました。Redisストリーム互換性テストでは、シードおよびMVCC可視性判定の時刻計算を実際のコミット時刻基準に変更しました。 ChangesTSO ceiling管理
Redisストリーム互換性テスト
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Raft
participant TSOStateMachine
participant HLC
Raft->>TSOStateMachine: Apply HLC lease
TSOStateMachine->>TSOStateMachine: Update committed ceiling
TSOStateMachine->>HLC: Observe and SetPhysicalCeiling
HLC-->>TSOStateMachine: Updated ceiling state
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
1 similar comment
|
@codex review |
![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: 518a3800b1
ℹ️ 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.hlc != nil && ceilingMs > 0 { | ||
| f.hlc.SetPhysicalCeiling(ceilingMs) |
There was a problem hiding this comment.
When group 0 is switched to this FSM, a replica that has only applied lease entries can later become the TSO leader with HLC.last still at zero; because this apply path only raises physicalCeiling, NextBatchFenced will floor to the same lease millisecond and start logical allocation at 0, reissuing timestamps the previous leader may already have handed out in that lease window. The existing KV FSM avoids this for data timestamps by observing applied commit timestamps, but the TSO-only FSM has no analogous full-HLC floor, so a leader transfer or restart before a fresh ceiling renewal can break timestamp uniqueness.
Useful? React with 👍 / 👎.
| func (f *TSOStateMachine) Snapshot() (raftengine.Snapshot, error) { | ||
| return &tsoSnapshot{ceilingMs: hlcCeilingFromHLC(f.hlc)}, nil |
There was a problem hiding this comment.
When TSOStateMachine is constructed over the process-wide HLC used by the sharded coordinator, any data group can advance f.hlc.PhysicalCeiling() before group 0 snapshots. Snapshotting that shared value lets a group-0 snapshot persist a ceiling that was never committed in the group-0 Raft log; after restore, the TSO leader can issue timestamps under a lease only another group agreed to. Track the TSO group's applied ceiling separately and snapshot that instead.
AGENTS.md reference: AGENTS.md:L33-L35
Useful? React with 👍 / 👎.
|
Updated signed head Changes:
Validation:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea1e04c897
ℹ️ 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".
| f.hlc.SetPhysicalCeiling(ceilingMs) | ||
| f.hlc.Observe(tsoCeilingMaxTimestamp(ceilingMs)) |
There was a problem hiding this comment.
When dedicated TSO issuance uses this FSM, applying/restoring a lease with ceiling C now only snapshots C but advances the in-memory HLC to C|maxLogical, so the first fenced batch is C+1,0. If that TSO leader dies before a newer ceiling is committed, the replacement restores/replays the same committed ceiling C, runs the same Observe(C|maxLogical), and can hand out C+1,0 again. The post-ceiling floor has to be part of the Raft-committed/restored state or allocation must wait for a fresh committed lease above the restored ceiling.
Useful? React with 👍 / 👎.
| f.hlc.SetPhysicalCeiling(ceilingMs) | ||
| f.hlc.Observe(tsoCeilingMaxTimestamp(ceilingMs)) |
There was a problem hiding this comment.
While this FSM is applying a lease and the node is also serving timestamp allocations, SetPhysicalCeiling(C) becomes visible before Observe(C|maxLogical) fences off the logical values in that millisecond. A concurrent NextBatchFenced in that gap can see the new ceiling with the old last value and allocate C,0 (or another low logical value), defeating the handoff this change is trying to enforce when that ceiling millisecond was already used by the prior lease holder. Publish the observed floor before the ceiling, or add a single HLC operation that advances both atomically.
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 (fe2dc4c):
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 signed head Changes:
Validation:
Caller audit:
Note: plain @codex review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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.
Inline comments:
In `@kv/hlc.go`:
- Around line 178-183: Update the error message associated with
ErrCeilingExpired in rejectFencedPhysicalOverflow so it does not assert wall_now
>= physicalCeiling; use wording that accurately describes rejection due to the
fenced physical/logical ceiling without depending on the specific triggering
condition. Preserve the existing rejection condition, counter increment, and
returned error symbol.
In `@kv/tso_fsm.go`:
- Around line 117-118: In the snapshot write path, update the short-write branch
checking n against tsoSnapshotLen to return io.ErrShortWrite wrapped with
cockroachdb/errors.WithStack, matching the wrapping applied to w.Write errors.
- Around line 69-77: applyTSOCeiling の ceiling 更新処理を、復元時と通常の C→D
更新で分けてください。復元時は復元した ceiling を HLC の floor とし、通常更新では D を設定する前に前回の committed
ceiling C を floor としてから D を適用します。呼び出し元の復元経路を明示的に区別し、2 回目の lease 後に
NextBatchFenced の allocation が成功するテストへ更新してください。
- Around line 38-44: Validate the decoded ceiling in applyHLCLease before
calling applyTSOCeiling, rejecting values above the maximum timestamp
representable after the HLC logical-bit shift. Apply the same pre-mutation
validation in the snapshot restore path so oversized lease state cannot alter
HLC current. Add Apply and Restore tests covering out-of-range lease and
snapshot payloads, including the expected rejection and unchanged state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ea4c1253-4741-475a-b2bb-85a8bed51429
📒 Files selected for processing (7)
adapter/redis_compat_commands_stream_test.godocs/design/2026_04_16_partial_centralized_tso.mdkv/hlc.gokv/hlc_test.gokv/tso_fsm.gokv/tso_fsm_test.gokv/tso_test.go
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe2dc4c7b8
ℹ️ 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".
| } | ||
| f.advanceCommittedCeiling(ceilingMs) | ||
| if f.hlc != nil { | ||
| f.hlc.Observe(tsoCeilingMaxTimestamp(ceilingMs)) |
There was a problem hiding this comment.
When this FSM is paired with the existing TSO allocation path, applying any fresh lease makes the window immediately unusable: Observe(tsoCeilingMaxTimestamp(ceilingMs)) sets HLC.last to ceiling|maxLogical, then SetPhysicalCeiling(ceilingMs) publishes that same ceiling, so the next NextBatchFenced has to advance to ceiling+1 and is rejected by the new overflow fence. This does fence restored/exhausted handoffs, but it also means every newly committed renewal is exhausted before the TSO leader can serve a single timestamp; the handoff floor needs to be separated from normal renewal application (or observe the prior floor rather than the newly published ceiling).
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 (4ee268c):
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 signed head Changes:
Validation:
|
|
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
Summary by CodeRabbit
新機能
ドキュメント
テスト