Implement centralized TSO phase D#1114
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 |
|
@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 (3a7b865):
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. |
![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 Phase D (M7) of the Centralized Timestamp Oracle (TSO) design, which closes the legacy compatibility window and retires legacy per-shard HLC issuance. Key changes include adding a ValidateTimestamp RPC to the Distribution service, updating GetTimestamp to support Phase-D activation, and modifying various adapter read-modify-write paths (DynamoDB, Redis, S3, SQS) to allocate and validate read timestamps via BeginReadTimestampThrough. There are no review comments to address, 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
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a7b865474
ℹ️ 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".
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 (c7955d3):
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
Local verification:
The broad @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7955d3947
ℹ️ 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".
93b8633 to
295b851
Compare
c7955d3 to
1fbb3e9
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 (1fbb3e9):
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:
All three current review threads were replied to and resolved. Requesting a fresh latest-head review. @codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? 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". |
295b851 to
9182f37
Compare
1fbb3e9 to
c801865
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 (c801865):
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 rebased current HEAD
Current-head local verification:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c801865681
ℹ️ 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".
c801865 to
07460ec
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 (07460ec):
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 07460ec:\n\n- Rebased onto current dependency tip aa94f4e and rebuilt as one PR-visible commit.\n- Author and committer are bootjp contact@bootjp.me; the local GPG signature is good.\n- Applied pre-Phase-D read watermarks now carry a private timestamp-bound capability. Every reused OCC dispatch reserves exactly one one-use coordinator voucher immediately before dispatch; another timestamp is rejected and a current binding shadows any parent capability.\n- SQS live/tombstone reaping covers record, orphan-index, and dedup dispatches. Receive covers expiry, rotation, and DLQ redrive. Redis urgent/background compaction covers every collection and TTL-inline batch. EXEC retry state retains and re-vouches the exact first-attempt snapshot.\n- The design records per-dispatch voucher semantics and keeps arbitrary unvouched caller timestamps fail-closed at group 0.\n- Caller audit enumerated every production BeginReadTimestampThrough/beginTxnReadTimestamp caller and every NextTimestampThrough, NextTimestampAfterThrough, and TimestampAllocatorThrough caller. Other fanout paths either use one StartTS-bearing dispatch per snapshot or nontransactional batches; no additional shared applied-snapshot dispatch was found.\n- All four current review roots were replied to and resolved.\n- The centralized TSO design remains Partial in this dependency PR; implemented promotion stays in the runtime-operations PR only after the dependency stack is clear.\n\nValidation:\n- focused kv Phase-D/voucher tests: pass\n- focused adapter SQS/Redis compactor/EXEC tests: pass\n- focused kv and adapter race tests: pass\n- go test ./... -run ^$ -count=1: pass\n- golangci-lint run ./... --timeout=5m --allow-parallel-runners: 0 issues\n- make gen: pass, no generated diff\n- make tla-check: all safe models pass and every gap model fails only at its expected invariant\n- git diff --check: pass\n- Full local package reruns were additionally attempted; concurrent worktree tests exhausted the host filesystem and prolonged the existing Test_grpc_transaction HLC fixture. Changed-path tests remain green and current-head CI is the authoritative broad run.\n\n@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 07460ec650
ℹ️ 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 readTS == ^uint64(0) { | ||
| if alloc, ok := kv.TimestampAllocatorThrough(s.coordinator); ok { | ||
| if phaseD, phaseDOK := alloc.(kv.TSOPhaseDState); phaseDOK && (phaseD.PhaseDRequired() || phaseD.PhaseDActive()) { | ||
| readTimestamp, err := kv.BeginReadTimestampThrough(ctx, s.coordinator, readTS, label) | ||
| return readTimestamp, errors.WithStack(err) |
There was a problem hiding this comment.
When the S3 MVCC store has no commits, s.readTS() returns ^uint64(0) via snapshotTS, and S3 mutations such as createBucket now pass that value into this helper. In Phase-D mode this branch sends the sentinel to BeginReadTimestampThrough, which rejects ^uint64(0) with ErrTSOTimestampInvalid, so a fresh/empty S3 namespace cannot create its first bucket after Phase D is enabled. Normalize the empty-store sentinel to a concrete safe read/start timestamp (the other adapters use 1) before validation instead of failing the mutation.
Useful? React with 👍 / 👎.
| if !callerSupplied { | ||
| return nil | ||
| } | ||
| if c.consumeAppliedReadTimestampVoucher(startTS) || singleShard { |
There was a problem hiding this comment.
Under Phase D, this accepts a pre-floor StartTS by consuming a process-wide counter keyed only by timestamp. When two requests read the same applied watermark (common while LastCommitTS is pinned at the Phase-D floor), an unbound direct Dispatch with that StartTS can consume the voucher registered by another ReadTimestamp and bypass group-0 validation, or make the legitimate dispatch fail/retry. The voucher should be tied to the ReadTimestamp/ctx capability, not just the numeric timestamp.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| func (r *RedisServer) xaddReadTimestamp(ctx context.Context, key []byte) (uint64, error) { | ||
| readTS, err := r.beginTxnStartTS(ctx, "redis xadd: begin read timestamp") |
There was a problem hiding this comment.
In Phase D, beginTxnStartTS can reserve the only voucher that admits an applied pre-floor watermark, but this path collapses it to a uint64. The first XADD dispatch consumes that voucher; if it returns a retryable/ambiguous error, reusableXAdd keeps only startTS and dispatchXAddReuse calls plain Dispatch, so stream writes whose meta/entry keys span shards fail on the retry with ErrTSOTimestampPrePhaseD instead of probing PrevCommitTS. Keep the ReadTimestamp and bind/re-vouch the reuse dispatch.
Useful? React with 👍 / 👎.
| if err != nil { | ||
| return nil, nil, errors.WithStack(err) | ||
| } | ||
| readTS := readTimestamp.Timestamp() |
There was a problem hiding this comment.
In Phase D, this ReadTimestamp may contain the only voucher for a pre-floor applied watermark, but the dedup path immediately reduces it to readTS and stores only that number in reusableItemWrite. After the first cross-shard item/GSI dispatch consumes the voucher, itemWriteReuseAttempt reuses the same StartTS with plain Dispatch and fails group-0 validation instead of probing PrevCommitTS, so retryable item writes can stop at ErrTSOTimestampPrePhaseD. Retain the ReadTimestamp and re-bind it for reuse dispatches.
Useful? React with 👍 / 👎.
1 participant
Author: bootjp
Summary
Safety
Tests
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.