tso: route durable timestamps through group leader#1108
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
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 (3147271):
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. |
|
Latest head: Implemented and locally verified:
Local validation passed:
@codex review |
![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 a centralized Timestamp Oracle (TSO) by introducing a dedicated Raft group 0, a minimal TSOStateMachine, and allocators (RaftTSOAllocator, LeaderRoutedTSOAllocator, and ShadowTimestampAllocator) to manage durable timestamp windows and serialize legacy HLC candidates during migration. Key feedback on these changes highlights critical issues, including an uninitialized connCache in LeaderRoutedTSOAllocator that leads to a nil pointer dereference, a performance bottleneck caused by holding a mutex across blocking network and Raft operations, a race condition between Raft proposal commits and FSM application in commitCutover, sequential latency bottlenecks when querying data-group commit floors, and a bug where wrapped errors prevent status.Code from extracting the correct gRPC status codes.
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.
| func NewLeaderRoutedTSOAllocator( | ||
| local TSOAllocator, | ||
| leader raftengine.LeaderView, | ||
| opts ...LeaderRoutedTSOAllocatorOption, | ||
| ) (*LeaderRoutedTSOAllocator, error) { | ||
| if local == nil { | ||
| return nil, errors.WithStack(ErrTSOAllocatorRequired) | ||
| } | ||
| if leader == nil { | ||
| return nil, errors.WithStack(ErrTSOGroupRequired) | ||
| } | ||
| a := &LeaderRoutedTSOAllocator{ | ||
| local: local, | ||
| leader: leader, | ||
| retryBudget: defaultTSORouteRetryBudget, | ||
| retryInterval: defaultTSORouteRetryInterval, | ||
| } | ||
| for _, opt := range opts { | ||
| if opt != nil { | ||
| opt(a) | ||
| } | ||
| } | ||
| a.remoteRequest = a.requestRemoteBatch | ||
| return a, nil | ||
| } |
There was a problem hiding this comment.
The connCache field of LeaderRoutedTSOAllocator is never initialized or populated in NewLeaderRoutedTSOAllocator. Since GRPCConnCache is an interface, calling a.connCache.ConnFor(addr) in requestRemoteBatch will result in a nil pointer dereference panic in production. Consider adding a WithGRPCConnCache option or passing the connection cache to the constructor so it can be properly wired.
| a.mu.Lock() | ||
| defer a.mu.Unlock() | ||
| return a.reserveBatchAfterLocked(ctx, n, min, activateCutover) | ||
| } |
There was a problem hiding this comment.
Holding the mutex a.mu across the entire reserveBatchAfterLocked call is a severe performance bottleneck. This method performs sequential network gRPC calls (GlobalCommittedTimestampFloor via termCommitFloor) and blocking Raft proposals (commitCutover and commitAllocationFloor). Because these blocking operations are executed under the lock, all concurrent TSO allocation requests on this node will experience head-of-line blocking, limiting the TSO throughput to a fraction of its potential. Consider initializing the term floor outside the hot-path lock (e.g., using a double-checked locking pattern or a separate coordination state) and minimizing the duration the lock is held.
| func (a *RaftTSOAllocator) commitCutover(ctx context.Context, engine raftengine.Engine) error { | ||
| if _, err := a.group.Proposer().Propose(ctx, marshalTSOCutover()); err != nil { | ||
| wrapped := errors.Wrap(err, "tso commit cutover marker") | ||
| if tsoProposalLostLeadership(engine, err) { | ||
| return stderrors.Join(ErrTSONotLeader, wrapped) | ||
| } | ||
| return wrapped | ||
| } | ||
| if !a.state.CutoverActive() { | ||
| return errors.New("tso cutover proposal committed without applied marker") | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
There is a race condition in commitCutover between the Raft proposal commit and the FSM apply. Propose blocks until the entry is committed to the Raft log, but the FSM Apply runs asynchronously on the Raft apply loop. Immediately after Propose returns, a.state.CutoverActive() is highly likely to still be false in production, causing the cutover to spuriously fail with "tso cutover proposal committed without applied marker". Consider removing this strict synchronous assertion, or waiting for the FSM to apply the entry before returning.
| var maxTS uint64 | ||
| for _, groupID := range ids { | ||
| ts, err := s.authoritativeGroupLastCommitTS(ctx, groupID, s.groups[groupID]) | ||
| if err != nil { | ||
| return 0, errors.Wrapf(err, "tso commit floor: group %d", groupID) | ||
| } | ||
| if ts > maxTS { | ||
| maxTS = ts | ||
| } | ||
| } | ||
| return maxTS, nil |
There was a problem hiding this comment.
GlobalCommittedTimestampFloor queries every data group sequentially to find the maximum commit timestamp. To avoid sequential latency bottlenecks, execute these checks concurrently (e.g., using an errgroup.Group) while optimizing with a fast path for the single-group case to avoid concurrency overhead.
References
- When performing lease or freshness checks on multiple keys in a sharded or distributed system, deduplicate the keys by their owning shard or Raft group first to minimize network round-trips. If multiple groups are involved, execute the checks concurrently to avoid sequential latency bottlenecks, while optimizing with a fast path for the single-group case to avoid concurrency overhead.
There was a problem hiding this comment.
対応しました。data group が1つのときは従来どおり直列fast path、複数のときは上限16でerrgroup並列照会し、いずれかのleader fence失敗時は全体をfail-closedにします。2つのRPCが双方開始するまで解放しないbarrier testをrace付きで追加しました。HEAD: df46968
| func timestampRPCError(err error) error { | ||
| if code := status.Code(err); code != codes.Unknown { | ||
| return err | ||
| } | ||
| switch { | ||
| case errors.Is(err, context.Canceled): | ||
| return errors.WithStack(status.Error(codes.Canceled, err.Error())) | ||
| case errors.Is(err, context.DeadlineExceeded): | ||
| return errors.WithStack(status.Error(codes.DeadlineExceeded, err.Error())) | ||
| case errors.Is(err, kv.ErrInvalidTSOBatchSize), errors.Is(err, kv.ErrTxnCommitTSRequired): | ||
| return errors.WithStack(status.Error(codes.InvalidArgument, err.Error())) | ||
| case errors.Is(err, kv.ErrTSONotLeader), errors.Is(err, kv.ErrLeaderNotFound): | ||
| return errors.WithStack(status.Error(codes.FailedPrecondition, err.Error())) | ||
| default: | ||
| return errors.WithStack(status.Error(codes.Unavailable, err.Error())) | ||
| } | ||
| } |
There was a problem hiding this comment.
status.Code(err) is called directly on err, which may be wrapped using cockroachdb/errors (e.g., errors.Wrap or errors.WithStack). Wrapped errors do not expose the gRPC status code directly to status.Code, causing them to return codes.Unknown. This will make all wrapped gRPC errors fall through to the default case and return codes.Unavailable instead of preserving their original gRPC status code. Consider unwrapping the error using errors.Cause(err) or errors.UnwrapAll(err) before extracting the status code.
|
TLA CI の失敗は実装ではなく、TLA Tools v1.8.0 の配布 jar が更新され checksum が変わったことが原因でした。Makefile の固定 SHA-256 を現在の配布物に更新し、 最新 HEAD: @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 (ff83dd6):
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. |
|
Proto CI が最新 main の既存 検証:
最新 HEAD: @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 (2114663):
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. |
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 (df46968):
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. |
|
data-group commit floor の直列照会を改善しました。単一groupはfast path、複数groupは上限16で並列化し、どのgroupのauthoritative leader fenceにも到達できなければ従来どおりfail-closedです。 検証:
最新 HEAD: @codex review |
|
To use Codex here, create a Codex account and connect to github. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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: df4696893a
ℹ️ 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".
| } | ||
| requestCtx, cancel := context.WithTimeout(nonNilTSOContext(ctx), proxyForwardTimeout) | ||
| defer cancel() | ||
| resp, err := pb.NewRawKVClient(conn).RawLatestCommitTS(requestCtx, &pb.RawLatestCommitTSRequest{}) |
There was a problem hiding this comment.
When the local ReadIndex path fails or Leader() is stale, this empty-key RawLatestCommitTS call can still succeed against a deposed/non-leader endpoint: the server-side empty-key handler just returns store.LastCommitTS() and does not verify leadership for the target data group. In that leader-churn case the TSO term can accept a stale follower watermark instead of failing closed, so it may initialize below a commit that already exists on the new data-group leader.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d4444e7 and retained in latest HEAD 8927523. Remote floor requests now carry the explicit group ID; the receiving ShardStore.GroupCommittedTimestampFloor requires local leadership and completes a linearizable ReadIndex before returning that group watermark. Targeted follower-rejection and leader-success tests pass with race enabled.
| uint64 ts = 4; // optional read timestamp; if zero, server uses current HLC | ||
| bool reverse = 5; | ||
| uint64 group_id = 6; // optional explicit Raft group for non-range-owned keyspaces | ||
| bool keys_only = 7; // when true, response kv entries omit values |
There was a problem hiding this comment.
This new wire flag advertises that values are omitted when keys_only is true, but the RawKV server never reads GetKeysOnly() and adapter/grpc.go still always serializes each RawKVPair.Value via rawKvPairs. Any client relying on this option for a keys-only scan will still receive full values, which defeats the API contract and can unnecessarily expose or transfer large payloads.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in latest HEAD 8927523. RawScanAt now honors keys_only: it uses key-only store/group scan interfaces when available and otherwise strips values before building the response, including reverse and explicit-group compatibility paths. Added race-tested coverage for the explicit-group optimized path and the fallback path; both assert empty response values.
| if !*tsoEnabled && !*tsoShadowEnabled { | ||
| return wiring, nil |
There was a problem hiding this comment.
If a node restarts after another node has durably set the group-0 cutover marker but comes up with both --tsoEnabled and --tsoShadowEnabled false, this early return leaves the coordinator on the legacy HLC path and never pins timestamp issuance to group 0. That can reintroduce non-serialized legacy timestamps after cutover, undermining the one-way marker that is supposed to prevent mixed timestamp sources during restarts or partial rollouts.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in latest HEAD 8927523. Startup now reads the restored group-0 cutover marker before mode selection. A durable marker forces production group-0 routing even when both flags are false or only shadow mode is requested, so restart cannot return to legacy issuance. Regression tests pre-commit the marker, reset proposal counts, reconfigure, and prove only the allocation-floor proposal occurs.
|
自己監査で、remote data-group floor が接続先groupを明示せず、RPC到着時のleadershipを再検証していない経路を修正しました。 検証:
|
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 (d4444e7):
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: d4444e72c5
ℹ️ 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".
| uint64 ts = 4; // optional read timestamp; if zero, server uses current HLC | ||
| bool reverse = 5; | ||
| uint64 group_id = 6; // optional explicit Raft group for non-range-owned keyspaces | ||
| bool keys_only = 7; // when true, response kv entries omit values |
There was a problem hiding this comment.
When a client sets this new keys_only option, the advertised contract says values are omitted, but RawScanAt never reads req.GetKeysOnly() and always builds the response through adapter/grpc.go:200 -> rawKvPairs, which copies each Value unconditionally at adapter/grpc.go:236-239. Any caller using the new option still receives full values, so large or sensitive scans pay the full transfer cost and violate the API contract.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8927523. RawScanAt now honors keys_only for explicit-group optimized scanners and the compatibility fallback. The fallback strips values before returning, and adapter regression tests cover both paths.
| return 0, errors.Wrapf(ErrTSOCommitFloorUnavailable, | ||
| "read data group %d leader %s watermark: %v", groupID, addr, err) | ||
| } | ||
| return resp.GetTs(), nil |
There was a problem hiding this comment.
During a rolling upgrade where the data-group leader still runs a binary that predates RawLatestCommitTSRequest.group_id, protobuf drops the unknown group_id and the old handler treats this as the empty-key request, returning a node-global LastCommitTS without the new group-specific leadership/ReadIndex fence. Since this caller accepts any successful response here, TSO term initialization can be satisfied by the legacy path instead of failing closed, so the floor may be stale or for the wrong group.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ccc2916. Explicit-group watermark responses now echo group_id and set leader_fenced only after the group ReadIndex fence succeeds. The TSO floor client rejects a missing marker or mismatched group as both protocol-unsupported and floor-unavailable. Legacy-response and wrong-group regression tests cover the rolling-upgrade case.
| if err != nil { | ||
| return empty, err | ||
| } | ||
| previousFloor := max(a.state.AllocationFloor(), termFloor) |
There was a problem hiding this comment.
If this goroutine loses group-0 leadership while termCommitFloor or the cutover proposal is in flight and later regains leadership before commitAllocationFloor, the cached term/floor from this point is still used to allocate from the local HLC. A different leader may already have committed and returned a higher allocation floor; because TSO floor entries apply with storeMaxUint64, this node can commit a lower floor entry successfully and return a window overlapping that prior leader's window. Re-check the current term/floor after the blocking steps, or reject on term change, before exposing the reservation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ccc2916. The allocator revalidates the same group-0 term with a fresh leader fence after floor/cutover work, then checks the term again after the allocation floor commits and before returning the window. A term change drops the committed window; a retry starts above its durable floor. Regression tests cover changes during floor discovery and immediately after proposal apply.
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 (8927523):
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. |
|
Latest head: 8927523 Resolved the current-head review findings:
Validation passed:
An additional package-wide adapter run reached its existing 10-minute multi-node integration timeout; the changed adapter tests pass under race and GitHub CI is rerunning. @codex review |
|
✅ Action performedReview finished.
|
|
Codex Review: Didn't find any major issues. Keep it up! 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 (ccc2916):
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 Latest head ccc2916 addresses the remaining current findings:
Verification:
Caller audit covered every RawLatestCommitTS request/response path, both commit-floor entrypoints, and all RaftTSOAllocator construction/reservation call sites. |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ccc291672d
ℹ️ 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 *tsoEnabled { | ||
| routedOpts = append(routedOpts, kv.WithTSOCutoverActivation()) | ||
| } |
There was a problem hiding this comment.
When this node has restored the one-way cutover marker but is restarted without --tsoEnabled, this leaves routed.activate false even though the coordinator is put into production TSO mode below. In the follower case, requestRemoteBatch only rejects cutover_active=false responses when activate is true, so a stale or mismatched TSO leader response can be accepted after local cutover restore without confirming the leader also fenced the cutover marker. Include cutoverActive in this condition so restored-cutover nodes continue to fail closed on non-cutover responses.
Useful? React with 👍 / 👎.
| if err != nil { | ||
| return wiring, errors.Wrap(err, "configure dedicated tso allocator") | ||
| } | ||
| wiring.serverAllocator = local |
There was a problem hiding this comment.
Because serverAllocator is populated before the no-mode early return, a group-0 leader started with neither --tsoEnabled nor --tsoShadowEnabled will still accept a follower's activate_cutover RPC through Distribution.GetTimestamp, commit the durable cutover marker, and then keep using its legacy local coordinator until restart. In that partial rollout, local writes from the leader can continue issuing unshadowed legacy timestamps after cutover has become durable; either avoid exporting the allocator in this mode or reject activation unless the local process is already in shadow/TSO mode.
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 (93b8633):
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. |
e701cb2 to
97a42ba
Compare
93b8633 to
295b851
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 (295b851):
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. |
|
Evidence for current HEAD
Validation:
Current-root inline findings were zero before the base rebuild. Requesting a fresh latest-head review now. @codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. 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". |
|
Current HEAD @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 295b85152f
ℹ️ 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".
295b851 to
9182f37
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 (9182f37):
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. |
|
Evidence for current HEAD
Verification:
@codex review |
|
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
Migration safety
Validation
go test ./kv -count=1 -timeout=10mgo test . -count=1 -timeout=10mgo test ./adapter -run 'Test(DistributionServerGetTimestamp|GRPCServer_RawLatestCommitTS_EmptyKeyReturnsGlobalWatermark)' -count=1 -timeout=5mgo test ./... -run '^$' -count=1 -timeout=10mgo test -race ./kv -run 'Test(RaftTSOAllocator|LeaderRoutedTSOAllocator|ShadowTimestampAllocator|ShardStoreGlobalCommittedTimestampFloor)' -count=1 -timeout=10mgolangci-lint --config=.golangci.yaml run ./kv ./adapter . --timeout=5mmake gen BREAKING_AGAINST='../.git#subdir=proto,branch=design/dedicated-tso-runtime'Stack
Base: #1103
Author: bootjp