Complete centralized TSO runtime operations#1129
Conversation
|
Validation evidence at
@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 (d94ba5b):
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. |
|
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 |
|
To use Codex here, create a Codex account and connect to github. |
There was a problem hiding this comment.
Code Review
This pull request implements runtime mode changes, configuration reload, and production monitoring for the centralized Timestamp Oracle (TSO). It introduces a TSORuntimeController to manage safe, one-way transitions between TSO modes, along with a DynamicTimestampAllocator for atomic allocator swapping. Additionally, it adds Prometheus metrics, alert rules, and a concurrent write-fanout benchmark. Feedback on the changes suggests improving error handling in ValidateDurableTimestamp by explicitly checking for a nil allocator and returning ErrTSOAllocatorRequired instead of failing the type assertion.
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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d94ba5b63c
ℹ️ 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 (1b3fa86):
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 at current head
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b3fa86327
ℹ️ 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".
c7955d3 to
1fbb3e9
Compare
1fbb3e9 to
c801865
Compare
1b3fa86 to
ef59050
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 (ef59050):
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
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef59050f13
ℹ️ 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 c != nil && c.tsAllocator != nil { | ||
| return c.tsAllocator, true | ||
| if c != nil { | ||
| return resolveTimestampAllocator(c.tsAllocator) |
There was a problem hiding this comment.
Preserve runtime allocator for forwarded writes
When a node starts with --tsoModeFile containing legacy, this resolves the DynamicTimestampAllocator to nil; main.go's internalTimestampOptions is evaluated once at startup and therefore does not pass any allocator to adapter.Internal. After the intended runtime rollout reloads shadow, cutover, or phase-d, forwarded writes still run adapter.Internal.nextTimestamp with a nil allocator and use the HLC fallback, so data-group leaders can keep issuing non-TSO timestamps after cutover. Keep a stable runtime-aware allocator in the Internal path rather than snapshotting the currently active slot.
Useful? React with 👍 / 👎.
| if target > current+1 && target > durableFloor { | ||
| c.observeReload("rejected") | ||
| return errors.Wrapf(ErrUnsafeTSOModeTransition, "%s -> %s", current, target) |
There was a problem hiding this comment.
Allow Phase D after a durable cutover override
If a running node misses the local cutover reload but observes the durable cutover marker, then the shared mode file may already contain phase-d; with current still legacy or shadow, this check rejects that reload as a skipped phase even though durableFloor is already cutover. The node can be stuck rejecting every poll of the cluster-wide phase-d file until operators temporarily roll that file back to cutover, so use the durable floor as the effective current step before enforcing adjacency.
Useful? React with 👍 / 👎.
c801865 to
07460ec
Compare
Summary
legacy -> shadow -> cutover -> phase-dsequenceSafety
Legacy mode keeps the coordinator allocator logically absent, preserving existing HLC fallback callers. Cutover activation is published before batch invalidation, durable group-0 markers can only advance the local mode, and invalid reloads leave the active allocator unchanged.
Validation
go test ./kv ./monitoring . -count=1go test -race ./kv ./monitoring . -count=1golangci-lint run ./... --timeout=5m --allow-parallel-runnersgo test ./kv -run ^'$' -bench ^'BenchmarkTSOWriteFanout$' -benchmem -benchtime=1s -count=3\n-promtool check config monitoring/prometheus/prometheus.yml(8 rules)\n-go test ./store -count=1 -timeout=15m\n-git diff --check\n- caller audit across allocator providers, coordinator fields, runtime flags, activation, and Phase-D state\n- zero references to the former partial design path