tso: add dedicated ceiling fsm - #1095
Conversation
|
@codex review |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughTSOの永続状態、専用Raft group、Phase D、ランタイムモード切替を追加しました。各アダプターのトランザクションを ChangesTSO状態とランタイム制御
ReadTimestamp配線
起動と運用
Estimated code review effort: 5 (Critical) | ~120 minutes Merge Risk: 🔵 Low · up to The PR adds dedicated TSO state handling and related runtime, monitoring, and documentation changes. Current evidence indicates bounded integration and rollout risks: duplicate write observation, missing forwarded-write sampling, inconsistent rollout status, and alerting that may miss node-local readiness; the change is mergeable with explicit owner awareness or follow-up. Sequence Diagram(s)sequenceDiagram
participant Client
participant DistributionServer
participant TSORuntimeController
participant LeaderRoutedTSOAllocator
participant TSOStateMachine
participant ShardedCoordinator
Client->>DistributionServer: GetTimestamp or transaction request
DistributionServer->>TSORuntimeController: resolve active allocator
TSORuntimeController->>LeaderRoutedTSOAllocator: reserve or validate timestamp
LeaderRoutedTSOAllocator->>TSOStateMachine: commit durable marker or allocation state
TSOStateMachine-->>LeaderRoutedTSOAllocator: committed state
LeaderRoutedTSOAllocator-->>DistributionServer: timestamp and durable state
DistributionServer->>ShardedCoordinator: DispatchWithReadTimestamp
ShardedCoordinator-->>Client: transaction result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 9.29% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 140 functions across 24 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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 👍 / 👎.
|
Claude finished @bootjp's task in 1m 47s —— View job Review of HEAD
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d3dce2eb6
ℹ️ 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 phaseD && mode < kv.TSOModePhaseD { | ||
| return errors.Wrapf(ErrTSOActivationNotPermitted, "phase-d activation requires local mode phase-d, have %s", mode) | ||
| } | ||
| if cutover && mode < kv.TSOModeCutover { | ||
| return errors.Wrapf(ErrTSOActivationNotPermitted, "cutover activation requires local mode cutover or later, have %s", mode) |
There was a problem hiding this comment.
When cutover or Phase D has already been committed by another leader, a node whose mode file still names the preceding stage immediately follows the marker through DynamicTimestampAllocator.durableTimestampAllocator, but CurrentMode() remains stale until the next successful reload. If that node becomes the group-0 leader during the rolling transition, remote allocators continue sending the already-active ActivateCutover/ActivatePhaseD flag and this branch rejects every reservation; the resulting PermissionDenied is non-transient in isTransientTSORouteError, so timestamp allocation and writes on all other nodes fail until the leader's local configuration catches up. Treat an already-durable requested marker as a no-op before enforcing the local activation-stage check.
Useful? React with 👍 / 👎.
The gate added in a062200 checked the request's activation flags against this node's runtime mode without asking whether the marker was already committed. When another leader has activated cutover or Phase D, a node whose mode file still names the preceding stage follows the marker immediately through DynamicTimestampAllocator, but CurrentMode() stays stale until its next successful reload. If that node holds group-0 leadership during the rolling transition, remote allocators keep sending the now-redundant activation flag, the gate refuses, and PermissionDenied is not transient to isTransientTSORouteError -- so allocation and writes stall on every node until the local configuration catches up. Narrow the flags to the markers that are not durable yet before consulting the gate. Following a marker somebody else committed is mandatory, not an activation request; only a marker this node would be the first to commit is one. Claude-Session: https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE
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 (b91ae6e):
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. |
|
HEAD ( 検証指摘は正しいです。 前 tick の gate は、要求された marker が既に durable かどうかを問わずに local mode と突き合わせていました。 ロールアウト中の窓:
そして 修正gate に渡す前に、まだ durable でない marker だけにフラグを絞ります。 他人が commit 済みの marker に追随するのは義務であって活性化要求ではありません。 活性化要求と呼べるのは、このノードが最初に commit することになる marker だけです。その区別を入れました。
Caller audit
Test evidence
2 件とも「修正を戻すと FAIL する」ことを確認済みです。 前 tick の反省どおり |
|
Claude encountered an error after 15s —— View job Review of HEAD
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b91ae6e0a7
ℹ️ 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".
| state, ok := s.timestampAllocator.(interface { | ||
| CutoverActive() bool | ||
| PhaseDActive() bool | ||
| }) |
There was a problem hiding this comment.
In the production wiring, main.go assigns a *kv.RaftTSOAllocator to serverAllocator, but that type implements PhaseDActive() without implementing CutoverActive(). Consequently this two-method assertion always fails in production, all requested flags are returned unchanged, and a node with an already-durable marker still invokes the stale-mode gate and returns PermissionDenied, so the reported cluster-wide allocation outage remains unfixed. The new tests miss this because their stub adds the otherwise-missing method; exercise this path with the concrete allocator or a production state accessor.
AGENTS.md reference: AGENTS.md:L22-L24
Useful? React with 👍 / 👎.
The activation gate narrows a requested cutover/phase-D activation away when the allocator already carries the durable marker, using one type assertion for both CutoverActive and PhaseDActive. *kv.RaftTSOAllocator -- the allocator main.go actually wires -- implements only PhaseDActive, so the combined assertion never matched in production and the narrowing was inert there. It passed in tests because the test double supplied both methods. Add CutoverActive to RaftTSOAllocator and probe the two markers independently, so an allocator exposing one of them still gets that half of the narrowing. The new test drives configureCoordinatorTSO's real wiring and asserts the server allocator satisfies both interfaces, which is what the previous test double hid. Claude-Session: https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE
|
The finding is right, and it caught the exact trap this loop warns about. The activation gate narrowed a requested cutover/phase-D activation away when the allocator already carries the durable marker, using one type assertion for both Fix:
Verification: the new test FAILs with |
|
Claude finished @bootjp's task in 1m 50s —— View job Review of HEAD
|
|
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". |
Resolutions worth review: - proto/service.proto: main took tags 3 and 4 on RawLatestCommitTSResponse for the version probe while this branch was open, and this branch used the same two for group_id / leader_fenced. Main's fields keep their tags and the watermark fields move to 5 and 6. On the request side both sides named the same group_id field, so it collapses onto main's tag 3. Generated code is regenerated with the pinned buf toolchain. - adapter/grpc.go: both sides added a group_id branch to RawLatestCommitTS. They are different requests sharing one field -- keyless is this branch's leader-fenced group watermark, with a key it selects the group for main's per-key read -- so the keyless case is now guarded on an empty key and extracted into rawGroupWatermark. Without the guard this branch's early return shadowed main's per-key group read entirely. - adapter/internal.go stampRawTimestamps: this branch validated an already stamped timestamp and returned; main added a route-floor check that its own comment says must also run for already-stamped requests. Composed, so a forwarded write is both validated and floor-checked. - adapter/redis_txn.go: main's target-based fencing is the substrate, this branch's ReadTimestamp and label ride on it. The key-based entry point stays as a thin wrapper. - adapter/redis_zset_cmds.go: main's bzpopmin candidate helpers are taken whole; persistBZPopMinResult keeps this branch's ReadTimestamp so the dispatch voucher still travels with the write. - kv/sharded_coordinator.go: commit-ts allocation, the elem patch check, and the write-floor check now travel together in settleTxnCommitTimestamp. Two tests changed because production behaviour legitimately moved: FilesystemGroupIDs skips store-less groups, so main's sorting test needs real stores; and ReadFenceGroupKeysForRange's enumeration order is no longer the raw range order under user-key routing, so that assertion compares the set. Option-count assertions in main_tso_routing_test.go move by one because the startup gate now also forwards the write gate. Claude-Session: https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE
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 (721ca14):
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. |
|
Merged A proto tag collision. Main took tags 3 and 4 on A shadowed branch in Other resolutions:
Three test expectations moved, each because production behaviour legitimately changed — flagging them rather than burying them:
|
|
To use Codex here, create a Codex account and connect to github. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Claude finished @bootjp's task in 4m 37s —— View job Review of merge commit
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 721ca145c0
ℹ️ 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 timestamp == 0 || timestamp > end { | ||
| return errors.Wrapf(ErrTSOTimestampInvalid, | ||
| "timestamp=%d phase_d_floor=%d allocation_floor=%d", timestamp, floor, end) | ||
| } | ||
| if timestamp <= floor { | ||
| return errors.Wrapf(stderrors.Join(ErrTSOTimestampInvalid, ErrTSOTimestampPrePhaseD), | ||
| "timestamp=%d phase_d_floor=%d allocation_floor=%d", timestamp, floor, end) | ||
| } |
There was a problem hiding this comment.
When tsoBatchSize > 1, BatchAllocator.refill commits the entire window end as AllocationFloor before its local callers claim all offsets, but this check accepts any number in that interval. An unauthenticated Internal.Forward caller can therefore persist using an as-yet-unclaimed slot; when the owning BatchAllocator later reaches that offset, it returns the same timestamp to an unrelated write, violating global timestamp uniqueness and potentially invalidating OCC ordering. Validation needs proof that the caller actually claimed the slot, rather than only checking the enclosing reserved range.
AGENTS.md reference: AGENTS.md:L35-L35
Useful? React with 👍 / 👎.
|
The mechanism is exactly as described. Two things sharpen the severity beyond the report:
That leaves three directions, and they are not equivalent:
I did not pick one. Option 1 is a one-line guard I can land immediately if you want the hole closed now and will accept the throughput cost; option 2 is the design-doc route and I can write the proposal. Tell me which and I will do it in the next pass. One correction to the report's framing, which does not change the conclusion: |
|
Claude finished @bootjp's task in 2m 44s —— View job Review of
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 721ca145c0
ℹ️ 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 0, nil, err | ||
| return kv.ReadTimestamp{}, nil, err | ||
| } | ||
| readTimestamp, err := kv.BeginReadTimestampThrough(ctx, r.coordinator, selectTS(), label) |
There was a problem hiding this comment.
When Phase D is required on a Redis store with no committed records, r.readTS() returns the ^uint64(0) empty-store sentinel, which BeginReadTimestampThrough rejects as invalid. LRANGE reaches this helper through fenceRangeListReadGroups, so reading a nonexistent list on a fresh Phase-D cluster returns an error instead of an empty array and cannot activate Phase D. Normalize the empty watermark to a valid applied timestamp such as 1, as txnStartTS already does, before invoking the Phase-D boundary.
Useful? React with 👍 / 👎.
| phaseDActive = s.phaseDActive | ||
| phaseDFloor = s.phaseDFloor | ||
| } | ||
| snapshotLen := tsoSnapshotV3Len |
There was a problem hiding this comment.
During the rolling binary rollout while every mode file is still legacy, an upgraded node constructs TSOStateMachine immediately and this unconditional V3 choice emits a 17-byte snapshot beginning with the encoded ceiling rather than a legacy kvFSM snapshot header. If that node leads group 0 and an older follower needs a snapshot after log compaction, the old kvFSM treats these bytes as a headerless store snapshot and pebbleStore.Restore rejects the unknown magic, so the follower cannot catch up and the rollout can lose quorum. Preserve an old-reader-compatible snapshot until the compatibility window closes, and cover a new-leader-to-old-follower snapshot install.
AGENTS.md reference: AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
main's TSOStateMachine.Restore reads exactly 8 bytes and rejects anything longer as trailing bytes, while this branch emitted 17 unconditionally. During a rolling upgrade a not-yet-upgraded group-0 follower therefore rejects the leader's snapshot and cannot catch up, which on a three-node group 0 risks quorum. (The reported mechanism -- kvFSM treating the bytes as a store snapshot and pebbleStore.Restore rejecting the magic -- is not this path: main already runs TSOStateMachine on group 0, and the dedicated TSO group opens no MVCC store.) The reader already accepts all four lengths, so only the writer changes. V1 carries a floor implicitly: its reader reconstructs tsoLeaseAllocationFloor(ceiling), so a floor already equal to that value round-trips exactly -- which is the state every node holds after restoring a pre-allocation-floor snapshot, and without that case a node that caught up from an old leader would immediately become unreadable to its remaining old peers. A zero floor also fits: the substitute is only ever higher, and it widens a bound that Phase D never consults, since a real floor and the phase-D marker each need their own committed envelope. Any other floor is real allocator state a substitute could raise, so it takes V2. Five existing tests pinned the old fixed length. Their intent is kept: the zero-state payload is still all zeros, the monotonic-ceiling test trades a byte-level floor assertion for a real round trip through Restore, and the TSO-owned-ceiling test now asserts the restored floor is the one derived from the TSO ceiling and below what the unrelated HLC value would give. adapter: normalize the empty-store watermark before Phase-D validation snapshotTS answers ^uint64(0) for a store with no committed record, and BeginReadTimestampThrough rejects both 0 and that sentinel once Phase D is required, so LRANGE on a nonexistent list on a fresh Phase-D cluster returned an error instead of an empty array. Normalize at the single point every fenced read passes through, to the same floor of 1 txnStartTS already uses. docs: propose TSO batch slot claims ValidateDurableTimestamp accepts any value in (PhaseDFloor, AllocationFloor], and AllocationFloor is the committed window end -- recorded before any offset is handed out -- so with the default batch size of 256 a 255-wide band of timestamps validates while belonging to nobody. Both candidate fixes carry a decision (a Raft round trip per issued timestamp, or a new durable claim record), so this lands as a proposal rather than a patch. Claude-Session: https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE
Resolutions: - adapter/internal.go, kv/sharded_coordinator.go, kv/shard_store_test.go: both sides added independent members in the same region; unioned. - main.go: main's live-backup flags, validation, and readTracker/backup-floor wiring are additive, so they merge alongside this branch's TSO wiring. The group-build body conflict is git mis-anchoring main's unchanged inline loop against this branch's shardGroupBuilder refactor -- main changed nothing there, so the refactor stands. internalOptionsForGroup folds into main's internalServerOptions, which now also takes the group id and keeps the group-0 KV-forward rejection. - main added SnapshotEvery to raftengine.Engine and a readTracker parameter to buildShardGroups; the branch's test stubs and call sites are updated to match. Claude-Session: https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE
main threads readTracker into every data-group FSM via NewKvFSMWithHLCAndTracker; the merge kept this branch's shardGroupBuilder refactor, which still called NewKvFSMWithHLC and silently dropped it. Lint caught the now-unused parameter. Claude-Session: https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE
|
Two of the three at-HEAD findings are fixed; the third has a design doc instead of a patch, deliberately.
|
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 (aabf40a):
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. |
|
Claude finished @bootjp's task in 6m 5s —— View job Review of HEAD
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aabf40abf6
ℹ️ 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".
| startErr := ValidateDurablePersistenceTimestamp(ctx, alloc, startTS, label) | ||
| if startErr != nil && errors.Is(startErr, ErrTSOTimestampPrePhaseD) { | ||
| return nil |
There was a problem hiding this comment.
When Phase D is active, a direct Internal.Forward caller can first submit a PREPARE with an arbitrary start timestamp below the Phase-D floor (which ValidateForwardedTxnStartTimestamp explicitly admits), then submit a COMMIT with another pre-D timestamp; this branch accepts the pair solely because the request is labeled as a resolution. The newly created intent makes the subsequent commit valid to the FSM, allowing a write to be inserted retroactively at an old, non-TSO-issued timestamp and breaking repeatable MVCC snapshots. Fresh evidence in the current code is that neither validation step verifies that the commit timestamp was recorded by a primary before Phase-D activation; require durable primary-record evidence rather than trusting the phase flag.
AGENTS.md reference: AGENTS.md:L33-L35
Useful? React with 👍 / 👎.
1 participant
Summary
Validation
Notes
Author: bootjp
Summary by CodeRabbit
新機能
改善
バグ修正