tso: add dedicated ceiling fsm#1095
Conversation
|
@codex review |
|
Warning Review limit reached
Next review available in: 1 minute 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 (18)
📝 WalkthroughWalkthrough専用TSO用Raftステートマシンを追加し、leaseとallocation floorの適用、スナップショット互換復元、group-0 runtime構築、EncryptionAdminのmutator制御を実装した。関連する設計文書と検証テストも更新した。 Changes専用TSO FSM
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant RaftRuntime
participant TSOStateMachine
participant HLC
participant EncryptionAdmin
RaftRuntime->>TSOStateMachine: group-0のlease/floorエントリをApply
TSOStateMachine->>HLC: ceilingまたはallocation floorを反映
TSOStateMachine-->>RaftRuntime: 更新結果または拒否結果
RaftRuntime->>EncryptionAdmin: group単位の管理配線を登録
EncryptionAdmin-->>RaftRuntime: group-0のmutator要求をFailedPreconditionで拒否
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 |
![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 implements the TSOStateMachine, a minimal state machine for the dedicated timestamp group that handles HLC lease-renewal entries, along with its snapshotting, restoration, and volatile entry classification logic. It also updates the design documentation to reflect this progress and adds comprehensive unit tests for the new state machine. There are no review comments to evaluate, and I have no additional feedback to provide.
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.
![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
Update stale M6 status row
This milestone row still says the minimal TSOStateMachine remains open, but this same change marks it implemented in §1.0 and adds kv/tso_fsm.go. Readers using this design doc to plan M6 will incorrectly think the FSM work is still pending, so please update the row to match the new runtime-wiring-only remaining status.
ℹ️ 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".
| } | ||
| ceilingMs := int64(binary.BigEndian.Uint64(data[1:])) //nolint:gosec // value is a Unix ms timestamp encoded as uint64. | ||
| if f != nil && f.hlc != nil && ceilingMs > 0 { | ||
| f.hlc.SetPhysicalCeiling(ceilingMs) |
There was a problem hiding this comment.
When this FSM is used for the dedicated TSO group, applying a lease only updates physicalCeiling while leaving HLC.last at zero on followers/restarted nodes. If the current TSO leader has handed out timestamps under a future ceilingMs and then fails before the wall clock reaches it, the new leader has applied the same lease but has not observed any of those allocations, so its first NextBatchFenced can return (ceilingMs<<16)|0 again and duplicate timestamps. The TSO path needs to persist/observe an allocation floor or force a fresh higher lease before serving after leadership changes.
Useful? React with 👍 / 👎.
|
|
||
| func (f *TSOStateMachine) Apply(data []byte) any { | ||
| if len(data) != hlcLeaseEntryLen { | ||
| return errors.Wrapf(ErrTSOStateMachineInvalidEntry, "expected %d bytes, got %d", hlcLeaseEntryLen, len(data)) |
There was a problem hiding this comment.
If a malformed payload is ever committed to the dedicated TSO group, returning a bare error here does not actually reject it at the raft apply layer: applyNormalCommitted only stops before setApplied for responses implementing HaltApply() error, while plain error responses are still marked applied and only delivered back to the proposer. In a misrouted group-0 proposal or mixed-version entry scenario, the invalid entry would be permanently advanced past instead of failing closed, violating the FSM's “accepts only HLC lease entries” contract.
Useful? React with 👍 / 👎.
|
@codex review Updated commit: 4e49dfbe7f31f084fc98bb5bec47a0e1458c4c24 Addressed current-head review findings:
Caller/semantic audit:
Validation:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e49dfbea8
ℹ️ 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 | ||
| } | ||
| hlc.SetPhysicalCeiling(ceilingMs) | ||
| hlc.Observe(tsoLeaseAllocationFloor(ceilingMs)) |
There was a problem hiding this comment.
When the dedicated TSO FSM is wired, applying/restoring a lease with this Observe only records ceilingMs|maxLogical in memory while snapshots/WAL entries still persist just ceilingMs. If a leader applies ceiling C, serves one timestamp, NextBatchFenced returns (C+1,0) because the floor is already C|maxLogical; after a restart or re-election with only the same lease entry/snapshot, applyTSOLeaseToHLC recreates the same floor and can serve (C+1,0) again. This means the new floor does not actually prevent reissuing timestamps unless the allocation floor/current HLC is made durable or allocation is blocked until a higher ceiling is committed.
Useful? React with 👍 / 👎.
| if hlc == nil || ceilingMs <= 0 { | ||
| return |
There was a problem hiding this comment.
When a full HLC-lease entry encodes 0 or a high-bit value that decodes negative, this branch returns without error, so Apply returns nil and the engine advances the applied index. Renewals only generate positive Unix-ms ceilings, so these are malformed group-0 lease entries; accepting them masks the bad proposal instead of using the new fail-closed HaltApply path. Please reject non-positive ceilings in Apply while keeping snapshot restore's empty-state handling separate if needed.
Useful? React with 👍 / 👎.
| if f != nil { | ||
| hlc = f.hlc | ||
| } | ||
| return &tsoFSMSnapshot{ceilingMs: hlcCeilingFromHLC(hlc)}, nil |
There was a problem hiding this comment.
When group 0 is wired during the documented bridge, this FSM is likely to share the process HLC that shard FSMs also advance; then Snapshot can serialize a ceiling raised by some data-group lease even if the TSO group never committed that lease. If a shard renewal succeeds while the group-0 proposal fails or lags, installing/restoring the group-0 snapshot advances dedicated TSO state beyond its own consensus log. Keep a TSO-FSM-owned ceiling field updated only from this FSM's Apply/Restore path and snapshot that value instead.
Useful? React with 👍 / 👎.
|
@codex review Updated commit: 44e6c18 Addressed current-head review findings:
Caller/semantic audit:
Validation:
Additional check:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 44e6c1852b
ℹ️ 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 tsoSnapshotV1Len: | ||
| ceilingMs = int64(binary.BigEndian.Uint64(payload[:hlcLeasePayloadLen])) //nolint:gosec // legacy snapshot value. |
There was a problem hiding this comment.
When this restores an 8-byte snapshot emitted by the previous TSO FSM format, allocationFloor remains 0. Those legacy snapshots were produced by code that also observed tsoLeaseAllocationFloor(ceilingMs) on apply/restore to prevent a restarted or newly elected TSO leader from handing out logical slots from the old ceiling millisecond. After upgrading from such a snapshot, the HLC mirror has only the ceiling, so the next NextBatchFenced can start at ceiling<<16|0 and overlap timestamps already covered by the prior floor; derive the legacy floor from the decoded ceiling before calling restoreSnapshotState.
Useful? React with 👍 / 👎.
| f.ceilingMs.Store(ceilingMs) | ||
| f.allocationFloor.Store(allocationFloor) |
There was a problem hiding this comment.
When Restore is called on an FSM that has already applied a higher ceiling/floor (the monotonic-restore test exercises this shape), these unconditional stores regress the FSM-owned state even though the HLC mirror remains high via its monotonic setters. Because Snapshot() now serializes f.ceilingMs and f.allocationFloor rather than the HLC, the next snapshot from that FSM can persist the lower values and lose the higher applied TSO floor/ceiling; restore should max these fields the same way apply does.
Useful? React with 👍 / 👎.
|
@codex review Updated commit: 5b6fd30 Addressed current-head P2 findings:
Caller/semantic audit:
Validation:
|
|
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". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 46be7a832f
ℹ️ 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".
| sg := &kv.ShardGroup{Engine: runtime.engine} | ||
| sg.Txn = kv.NewLeaderProxyForShardGroup(sg, kv.WithProposalObserver(proposalObserver)) |
There was a problem hiding this comment.
When group 0 is configured alongside a single data group, this returned ShardGroup is added to encWiring.attachRaftEnvelopeGroup in buildShardGroups. I checked adapter/encryption_admin.go's EnableRaftEnvelope path and main_raft_envelope_wiring.go's ValidateCutoverScope: the cutover precheck rejects unless exactly one group is attached, so enabling the dedicated TSO group turns a previously valid single-data-group raft-envelope rollout into FailedPrecondition (got 2) even though group 0 holds no user data. Exclude group 0 from that scope check or handle its cutover separately.
Useful? React with 👍 / 👎.
| legacy, err := hasLegacyKVFSMSnapshotHeader(br) | ||
| if err != nil || !legacy { | ||
| return legacy, err |
There was a problem hiding this comment.
When upgrading a reserved group-0 log that previously ran kvFSM from a version that emitted the supported headerless snapshot format, this probe returns legacy=false, so Restore falls through to readTSOSnapshotState and accepts only exactly 8 or 16 bytes. ReadSnapshotHeader deliberately leaves non-magic/headerless snapshots unconsumed for the inner store restore path, and those raw store payloads are not constrained to the TSO lengths, so affected nodes can fail to restore or misparse a 16-byte store payload instead of draining the legacy MVCC snapshot. Detect and drain headerless kvFSM snapshots before trying the raw TSO format.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e701cb221b
ℹ️ 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".
e701cb2 to
97a42ba
Compare
|
Evidence for current HEAD
History sanitation:
Validation:
@codex review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
kv/tso_fsm.go (1)
94-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff
//nolint:gosecの抑制は境界チェックへの置き換えを検討してください。コーディングガイドラインでは
//nolintの追加を避け、リファクタリングを優先することが求められています。タイムスタンプのuint64⇄int64変換については、変換前にmath.MaxInt64との境界チェックを行う小さなヘルパー（例:func unixMillisToInt64(v uint64) (int64, error)）を導入することで、複数箇所の//nolint:gosecを排除できます。既存コードで許容される慣例であれば据え置きで構いませんが、新規追加分については抑制の集約を推奨します。As per coding guidelines: "Avoid adding
//nolintunless absolutely required; prefer refactoring."Also applies to: 165-167, 197-197, 325-325
🤖 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/tso_fsm.go` at line 94, Replace the new `//nolint:gosec` suppressions around the `ceilingMs` conversion and the corresponding conversions at the other referenced sites with a shared checked conversion helper, such as `unixMillisToInt64`. Have the helper validate against `math.MaxInt64` before converting and return an error for overflow, then propagate or handle that error at each caller while preserving existing timestamp behavior.Source: Coding guidelines
🤖 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 `@kv/tso_fsm.go`:
- Line 94: Replace the new `//nolint:gosec` suppressions around the `ceilingMs`
conversion and the corresponding conversions at the other referenced sites with
a shared checked conversion helper, such as `unixMillisToInt64`. Have the helper
validate against `math.MaxInt64` before converting and return an error for
overflow, then propagate or handle that error at each caller while preserving
existing timestamp behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 378b480f-a7cd-42e8-a4ec-e03b8e9fa899
📒 Files selected for processing (7)
docs/design/2026_04_16_partial_centralized_tso.mdkv/tso_fsm.gokv/tso_fsm_test.gomain.gomain_encryption_admin.gomain_encryption_admin_test.gomultiraft_runtime_test.go
|
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 - route dedicated group-0 timestamp requests to the current TSO leader - commit every returned window end and fence each leader term above authoritative data-group commit floors - add synchronous fail-closed shadow migration and a durable one-way cutover marker - preserve rolling compatibility by rejecting legacy timestamp responses without durable reservation metadata - update the centralized TSO design status through M6 ## Migration safety - shadow candidates are serialized through group 0 before the legacy value is returned - overlapping legacy candidates are discarded and retried - cutover commits the marker before the first production window - group-0, shadow, and cutover failures stop timestamp issuance instead of falling back ## Validation - `go test ./kv -count=1 -timeout=10m` - `go test . -count=1 -timeout=10m` - `go test ./adapter -run 'Test(DistributionServerGetTimestamp|GRPCServer_RawLatestCommitTS_EmptyKeyReturnsGlobalWatermark)' -count=1 -timeout=5m` - `go test ./... -run '^$' -count=1 -timeout=10m` - `go test -race ./kv -run 'Test(RaftTSOAllocator|LeaderRoutedTSOAllocator|ShadowTimestampAllocator|ShardStoreGlobalCommittedTimestampFloor)' -count=1 -timeout=10m` - `golangci-lint --config=.golangci.yaml run ./kv ./adapter . --timeout=5m` - `make gen BREAKING_AGAINST='../.git#subdir=proto,branch=design/dedicated-tso-runtime'` ## Stack Base: #1103 Author: bootjp
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa94f4eedb
ℹ️ 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 min == ^uint64(0) { | ||
| return empty, errors.WithStack(ErrTxnCommitTSRequired) |
There was a problem hiding this comment.
When ReserveBatchAfter is called with n > 1 and a minimum close to MaxUint64 (for example Distribution.GetTimestamp{count:2, min_timestamp:MaxUint64-1}), this guard lets the request through; a.clock.Observe(min) then causes NextBatchFenced to wrap the reserved base to 0 and the HLC current value to 1 before the later response validation can fail, and commitAllocationFloor can persist that regressed floor. Reject any min that cannot fit the requested window before observing it.
Useful? React with 👍 / 👎.
| cfg.engine, | ||
| distCatalog, | ||
| adapter.WithDistributionCoordinator(coordinate), | ||
| adapter.WithDistributionTimestampAllocator(tsoWiring.serverAllocator), |
There was a problem hiding this comment.
When a client can reach the raft gRPC listener during startup rotation, this wiring makes Distribution.GetTimestamp call the dedicated allocator and commit group-0 allocation/cutover entries, but startupRotationGatedMethod only gates Distribution.SplitRange plus the RawKV/Transactional/Internal/Admin mutators. That lets timestamp proposals bypass the same startup mutator fence before waitRotateOnStartup completes and before public traffic is marked ready; include GetTimestamp in the gate or delay exposing the allocator until the gate opens.
Useful? React with 👍 / 👎.
| return &pb.GetTimestampResponse{Timestamp: s.engine.NextTimestamp()}, nil | ||
| } | ||
|
|
||
| reservation, err := s.allocateTimestampReservation(ctx, count, minTimestamp, activateCutover) |
There was a problem hiding this comment.
When group 0 is configured for Phase A/B but this node was not started with --tsoEnabled, any caller that can reach Distribution.GetTimestamp can set activate_cutover=true; this line forwards that untrusted request bit into the allocator, which commits the one-way cutover marker. Since main wires the server allocator even before production cutover, this bypasses the operator flag and the all-nodes-shadow rollout precondition, so keep activation tied to local/internal configuration rather than the public RPC field.
Useful? React with 👍 / 👎.
| return 0, errors.Wrapf(ErrTSOCommitFloorUnavailable, | ||
| "data group %d is not led by this node", groupID) | ||
| } | ||
| if _, err := linearizableReadEngineCtx(nonNilTSOContext(ctx), engine); err != nil { |
There was a problem hiding this comment.
When the TSO leader also leads a data group and the incoming timestamp request has no deadline, this local LinearizableRead inherits an unbounded context, unlike verifyLeaderEngineCtx and the remote floor RPC timeout. If that ReadIndex stalls, ReserveBatchAfter is still holding the allocator mutex while initializing the term floor, so one stuck local floor probe can wedge all subsequent TSO allocations; wrap this path in a bounded context.
Useful? React with 👍 / 👎.
1 participant
Summary
Validation
Notes
Author: bootjp
Summary by CodeRabbit