tso: add dedicated ceiling fsm#1095
Conversation
|
@codex review |
|
Warning Review limit reached
Next review available in: 20 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 ignored due to path filters (3)
📒 Files selected for processing (64)
📝 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 👍 / 👎.
Author: bootjp ## Summary - persist a one-way Phase D marker and floor after the production cutover marker - stop data-group HLC renewal and legacy/shadow timestamp issuance once Phase D is durable - validate caller-supplied cross-shard SSI timestamps at the current group-0 leader before allocating a commit timestamp or proposing writes - migrate adapter read-modify-write snapshots to dedicated TSO allocations while preserving single-shard and pre-Phase-D compatibility - retain the parent branch's same-term allocation fences and RawLatestCommitTS group_id/leader_fenced schema ## Safety - activation commits cutover, Phase D, and the first post-Phase-D allocation in order - validation accepts only timestamps in the durable post-marker allocation interval and fails closed on inactive state, stale leadership, unsupported routing, or out-of-range values - leader term is revalidated after floor/marker work and after the allocation-floor commit - proto output was regenerated from the combined schema ## Tests - make gen BREAKING_AGAINST='../.git#subdir=proto,branch=design/dedicated-tso-leader-routing' - go test ./kv ./adapter . -run 'TSO|PhaseD|RenewHLC|CrossShard|ValidateTimestamp|RawLatestCommitTS' -count=1 - go test ./kv -run 'TestRaftTSOAllocator(RejectsTermChange|DropsCommittedWindow|CommitsPhaseD)' -count=1 - go test ./adapter -run '^Test_consistency_satisfy_write_after_read_sequence$' -count=1 -timeout=10m - go test ./adapter -run '^TestRedis_StreamXReadLatencyIsConstant$' -count=1 -timeout=10m - golangci-lint run ./... --timeout=5m --allow-parallel-runners The full parallel go test ./... run completed all packages except two long adapter stress tests after their test HLC ceilings expired under concurrent local load; both tests passed when rerun independently.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca1a812729
ℹ️ 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 { |
There was a problem hiding this comment.
When Phase D is activated with no previous floor, or after a wall-clock/lease jump, the first reserved TSO Base can be far above PhaseDFloor (for example PhaseDFloor == 0 and Base == current_ms<<16). This validation accepts any timestamp in (floor, allocationFloor], so ValidateDurableTimestamp(1) can succeed even though group 0 never issued that timestamp, letting caller-supplied cross-shard StartTS values in the gap bypass the durable-TSO allocation gate. Track the first post-D base/ranges or set the marker floor to Base-1 before treating the interval as valid.
AGENTS.md reference: AGENTS.md:L35-L35
Useful? React with 👍 / 👎.
| func (r *RedisServer) beginTxnStartTS(ctx context.Context, label string) (uint64, error) { | ||
| readTimestamp, err := r.beginTxnReadTimestamp(ctx, label) | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
| return readTimestamp.Timestamp(), nil |
There was a problem hiding this comment.
When Phase D is active and the adapter read watermark predates the Phase-D floor, beginTxnReadTimestamp returns a voucher that authorizes that applied snapshot, but this helper discards it and returns only the numeric timestamp. Callers such as wide set/zset/list mutations then dispatch via dispatchElems/plain Dispatch; in a sharded catalog where the wide collection storage prefixes resolve to more than one group, the coordinator treats the StartTS as caller-supplied without the bound voucher and rejects the command as a pre-Phase-D timestamp. Return the ReadTimestamp through these paths or bind it with DispatchWithReadTimestamp at dispatch time.
Useful? React with 👍 / 👎.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| defer revoke() |
There was a problem hiding this comment.
When the same ReadTimestamp is used by overlapping DispatchWithReadTimestamp calls during Phase D, each call increments the same (timestamp, ref) use count, but a call that successfully reaches consumeAppliedReadTimestampVoucher still runs this deferred revoke afterward. If one dispatch consumes its token while another prepared token is still in the map, the defer can delete/decrement the other dispatch's authorization before it validates, causing valid applied-watermark operations to fail as pre-Phase-D timestamps. Make the revoke conditional on the prepared token remaining unused, or mint a distinct ref per prepared dispatch.
Useful? React with 👍 / 👎.
| err := s.retryS3Mutation(r.Context(), func() error { | ||
| readTS := s.readTS() | ||
| startTS, err := s.txnStartTS(r.Context(), readTS) | ||
| readTimestamp, err := s.beginTxnReadTimestamp(r.Context(), readTS, "s3 delete object: begin read timestamp") |
There was a problem hiding this comment.
When Phase D is active and the applied S3 read watermark predates the Phase-D floor, this call can return a ReadTimestamp carrying the voucher for the bucket-metadata read at readTS, but the later delete dispatch uses bare s.coordinator.Dispatch with only the object-manifest mutation. If the bucket metadata route and object route are split, the coordinator sees a single mutation shard and no read keys, so it accepts the legacy StartTS without consuming the voucher even though the operation semantically read another shard. Dispatch this transaction with readTimestamp.WithDispatchVoucher/DispatchWithReadTimestamp.
Useful? React with 👍 / 👎.
|
Addressed the latest review and merge-resolution findings:
Tests:
|
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 (fea84f9):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
|
Codex Review: Didn't find any major issues. Swish! 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". |
fea84f9 to
227aff5
Compare
|
Review feedback addressed on the latest head. Changes:
Validation:
|
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (227aff5):
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
elastickv/adapter/dynamodb_migration.go
Line 99 in 227aff5
Bind migration dispatches to the read voucher
This migration path receives a ReadTimestamp but dispatches the cross-key table-generation update without binding/registering its voucher. Under Phase D, tables created before the cutover commonly read at a pre-D applied watermark; when the metadata/generation keys are on different shards, the coordinator rejects this caller-supplied StartTS as pre-D instead of consuming the intended applied-read voucher, blocking the legacy key migration. Dispatch through kv.DispatchWithReadTimestamp(readTimestamp.WithDispatchVoucher(ctx), ...) for these migration writes.
ℹ️ 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 err | ||
| } | ||
| req.StartTS = readTS | ||
| if _, err := d.coordinator.Dispatch(ctx, req); err == nil { |
There was a problem hiding this comment.
When Phase D is required and readTS is an applied pre-D watermark, beginTxnReadTimestamp returns a voucher that is only registered by kv.DispatchWithReadTimestamp; this direct Dispatch drops that capability. If the table metadata and generation keys route to different shards, ShardedCoordinator treats StartTS=readTS as an arbitrary caller timestamp and rejects it with ErrTSOTimestampPrePhaseD, so CreateTable can fail after Phase-D activation on upgraded clusters with pre-D data. Use the read timestamp's dispatch helper here, as the item-write path does.
Useful? React with 👍 / 👎.
| readTimestamp, err := s.beginTxnReadTimestamp(ctx, "sqs send message: begin read timestamp") | ||
| if err != nil { | ||
| return nil, 0, errors.WithStack(err) | ||
| } | ||
| readTS := readTimestamp.Timestamp() |
There was a problem hiding this comment.
Here beginTxnReadTimestamp may mint a Phase-D applied-read voucher, but the helper immediately collapses it to a bare uint64; the standard SendMessage paths then dispatch StartTS=readTS via plain coordinator.Dispatch. On Phase-D clusters with pre-D queue metadata, any standard send whose queue metadata/read keys and message index writes route across shards is rejected as an arbitrary pre-D timestamp instead of consuming the voucher, so standard SQS sends can fail after cutover. Return and bind the ReadTimestamp through the send dispatch path.
Useful? React with 👍 / 👎.
| if err != nil { | ||
| return false, errors.WithStack(err) | ||
| } | ||
| readTS := readTimestamp.Timestamp() |
There was a problem hiding this comment.
This path keeps the ReadTimestamp only long enough to extract readTS; the later CreateQueue transaction, and the analogous DeleteQueue/PurgeQueue generation bumps, still dispatch with plain s.coordinator.Dispatch. Under Phase D, when the queue meta/gen/tombstone prefixes route to different shards and the applied watermark is pre-D, cross-shard validation rejects that StartTS as an arbitrary caller timestamp, so queue create/delete/purge can fail after cutover. Carry the ReadTimestamp through those dispatches and bind it with DispatchWithReadTimestamp.
Useful? React with 👍 / 👎.
1 participant
Summary
Validation
Notes
Author: bootjp
Summary by CodeRabbit