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 Plus 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 !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 👍 / 👎.
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 (360418a):
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 360418a:
Validation:
Full go test ./kv ./adapter was attempted; ./kv passed, while ./adapter hit its 10-minute integration-test timeout, consistent with prior broad adapter runs. Focused changed-path tests and compile/lint are green; current-head CI is running. @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 (f641453):
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 f641453:
Validation:
reviewThreads currently has no unresolved non-outdated findings. Current-head CI is running after the push. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f6414537c0
ℹ️ 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".
| elems := []*kv.Elem[kv.OP]{} | ||
| nextRemoved := 0 | ||
| readTS := r.readTS() | ||
| readTS, err := r.beginTxnStartTS(ctx, "redis del: begin read timestamp") |
There was a problem hiding this comment.
When Phase D is enabled and BeginReadTimestampThrough has to preserve a pre-Phase-D applied watermark, the usable authority is carried by the returned ReadTimestamp voucher, but this path keeps only the bare uint64 and later dispatches the accumulated deletes without DispatchWithReadTimestamp. For a single DEL containing two keys whose routes are both led by this process but belong to different shard groups, ShardedCoordinator takes the multi-shard path and rejects the bare pre-D StartTS as ErrTSOTimestampPrePhaseD instead of consuming a voucher; keep the ReadTimestamp bound through dispatch or restrict this helper to provably single-shard mutations.
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 (c8e3ada):
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 (580c5b5):
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 580c5b5: Addressed the latest Phase-D review findings:
Caller audit:
Validation:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 580c5b58f0
ℹ️ 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 (874a14f):
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 874a14f: Addressed the latest voucher-preservation findings:
Caller audit:
Validation:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 874a14f389
ℹ️ 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 (5587fd5):
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 Addressed the latest Phase-D read-voucher findings:
Caller audit:
Validation:
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. 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
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.