Add migration fence drain guards#1087
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 (eab9622):
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 introduces mechanisms to reject write operations and prefix deletions on write-fenced routes during split migrations, ensuring data consistency. It also adds a utility to scan and drain pending transaction locks within a route. The review feedback suggests two key improvements: optimizing the route intersection search in distribution/engine.go by breaking early from the sorted routes loop, and enhancing robustness in kv/migrator_lock_drain.go by checking for context cancellation within the lock scanning loop.
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.
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 (07a428e):
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. |
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
![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: 07a428ee88
ℹ️ 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 (7c17803):
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 Updated in 7c17803:
Validation:
|
|
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". |
…port' into design/hotspot-split-m2-fence-drain
|
@codex review Merged the latest design/hotspot-split-m2-store-export base into this branch at 585e5ac. No conflicts remained after the merge. Validation:
Caller audit for the fail-closed write-fence path:
|
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 (585e5ac):
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
Here are some automated review suggestions for this pull request.
Reviewed commit: 585e5acb5f
ℹ️ 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 elem == nil || len(elem.Key) == 0 { | ||
| continue | ||
| } | ||
| route, ok := c.engine.GetRoute(routeKey(elem.Key)) |
There was a problem hiding this comment.
For bucket-level S3 mutations such as ACL/bucket-meta updates, routeKey returns the raw !s3|bucket|meta|... key, while migration export/drain treats that row as belonging to the bucket's !s3route|... object route via s3BucketAuxiliaryRouteInRange. If that object route is WriteFenced, this pre-routing check misses it and allows the metadata-only write to commit during the fence; the FSM check uses the same routeKey-only predicate, so bypass/direct proposals are not caught either.
Useful? React with 👍 / 👎.
| if err := validateSplitKey(parent, splitKey); err != nil { | ||
| return nil, err | ||
| } | ||
| if err := s.rejectLiveSplitJobOverlap(ctx, snapshot, parent); err != nil { |
There was a problem hiding this comment.
This checks only the split jobs visible at snapshot.ReadTS, but the subsequent saveSplitResultViaCoordinator transaction writes route/version keys and does not include the scanned !dist|job|... rows in its read set. If another controller creates or advances an overlapping live split job after this snapshot but before the split catalog write commits, both operations can commit because they touch disjoint keys, defeating the new overlap guard.
Useful? React with 👍 / 👎.
| return Route{ | ||
| RouteID: r.RouteID, | ||
| Start: CloneBytes(r.Start), | ||
| End: CloneBytes(r.End), | ||
| GroupID: r.GroupID, |
There was a problem hiding this comment.
The new RouteHistorySnapshot.RouteOf and IntersectingRoutes APIs return cloneRoute(r), but this clone omits migration fields such as StagedVisibilityActive, MigrationJobID, and MinWriteTSExclusive. Any caller consulting a current or historical snapshot for those route properties during migration will see zero values even though the engine snapshot retained them, unlike GetRoute, Stats, and GetIntersectingRoutes.
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 (5acb33f):
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 Addressed the current-head P1 S3 bucket auxiliary write-fence finding in 5acb33f. Changes:
Validation:
Caller audit:
|
|
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". |
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 (f1aeeeb):
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. |
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 (04e8053):
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
Here are some automated review suggestions for this pull request.
Reviewed commit: 04e8053bcb
ℹ️ 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 !errors.Is(err, ErrComposed1VersionGCd) || attempt == composed1RetryAttempts || c.engine == nil { | ||
| return resp, err | ||
| } | ||
| reqs.ObservedRouteVersion = c.engine.Version() |
There was a problem hiding this comment.
When a raw OperationGroup spans multiple groups (including DEL_PREFIX broadcasts), ErrComposed1VersionGCd can be returned after some groups have already committed: ShardRouter.processGrouped continues past per-group errors, and the broadcast path can also partially succeed. This retry reissues the entire batch with a fresh route version and fresh raw timestamps, so a group that already applied a Del/DelPrefix can apply it again later and tombstone writes that arrived after the first attempt. Please retry only groups known not to have committed, or limit this automatic retry to single-group/no-side-effect failures.
Useful? React with 👍 / 👎.
| return []byte(""), nil | ||
| } | ||
| start := routeKey(prefix) | ||
| return start, prefixScanEnd(start) |
There was a problem hiding this comment.
For DynamoDB table-delete cleanup prefixes, routeKey(prefix) returns the exact table route key !ddb|route|table|<encoded-table>, but this turns it into a prefix interval. With tables such as foo and foobar (Zm9v/Zm9vYmFy), a WriteFenced route for the latter intersects the cleanup DEL_PREFIX for the former even though the physical cleanup prefix only targets foo; because this cleanup runs after table metadata is gone, an unrelated migration can make the async sweep give up and leave old item/GSI rows behind. Special-case exact per-table route keys (or use a prefix-free route-key range) instead of prefixScanEnd(start) here.
Useful? React with 👍 / 👎.
| if prepared == nil || prepared.preparedServer == nil || prepared.preparedServer.runner == nil { | ||
| return errors.New("distribution runtime server is not prepared") | ||
| } | ||
| if err := prepared.preparedServer.runner.startRaftTransport(); err != nil { |
There was a problem hiding this comment.
Starting the shared raft gRPC server here also registers DistributionServer, but the startup gate only blocks selected mutators such as SplitRange; GetRoute, ListRoutes, and GetIntersectingRoutes remain callable until waitForRaftStartupAfterTransport and setupDistributionAndRegistration have applied the durable catalog back into the engine. During replay or catalog bootstrap those methods can return the stale/default route view while the data plane is still held back, so route-aware clients or admins can cache incorrect ownership. Either delay these distribution reads or include them in the startup gate when binding transport before setup.
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 (d818602):
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
Here are some automated review suggestions for this pull request.
Reviewed commit: d818602593
ℹ️ 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".
| // Prewrite cleanup runs on the snapshot receive hot path before gRPC starts | ||
| // draining payload chunks. Only do a footer/token check here; full-payload | ||
| // CRC remains in the actual restore/open paths. | ||
| return fsmSnapshotFooterMatchesToken(fsmSnapPath(fsmSnapDir, index), tok.CRC32C) == nil |
There was a problem hiding this comment.
When prewrite cleanup runs before receiving/writing a newer snapshot, a prior .snap/.fsm pair is now considered restorable just because the .fsm footer still contains the token CRC. That does not recompute the payload CRC, so a body-corrupted .fsm with its original footer can be selected as the sole fallback and purgeUnretainedPrewriteSnapshots may delete an older valid pair before the new write finishes; if the incoming snapshot then fails, restart has only a corrupt retained snapshot. Please keep full verification for the pair that justifies pruning older fallbacks, or keep additional fallbacks when using the cheap footer check.
Useful? React with 👍 / 👎.
| if len(ch) >= cap(ch) { | ||
| if msg.GetType() == raftpb.MsgHeartbeatResp && coalesceHeartbeatResp(ch, msg) { | ||
| return nil | ||
| } | ||
| e.recordDroppedDispatch(msg) |
There was a problem hiding this comment.
Fresh evidence in this revision is that the new coalescer only protects already queued context-bearing responses: when the heartbeat-response lane is full of ReadIndex MsgHeartbeatResps (all with Context), an incoming response with its own Context falls through this branch and recordDroppedDispatch drops it. handleRead waits for that encoded context via handleReadStates, so under bursts of linearizable reads the dropped response can make a completed ReadIndex wait until its deadline; context-bearing incoming responses need a non-dropping path or reserved space.
Useful? React with 👍 / 👎.
| if prepared == nil || prepared.preparedServer == nil || prepared.preparedServer.runner == nil { | ||
| return errors.New("distribution runtime server is not prepared") | ||
| } | ||
| if err := prepared.preparedServer.runner.startRaftTransport(); err != nil { |
There was a problem hiding this comment.
Binding the raft transport here also starts the per-group gRPC server that registers RawKV/TransactionalKV; write methods go through the startup-gated coordinator, but read methods such as RawGet, Get, RawScanAt, and Scan read the store directly. Because this now happens before waitForRaftStartupAfterTransport has drained committed replay and before catalog/registration setup, any internal client using the raft gRPC address during that window can observe stale MVCC state; either delay registering those read services or gate them until WaitStarted completes.
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 (db9843a):
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 (a6d7d76):
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 Updated in a6d7d76:
Validation:
Operational note:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6d7d7609c
ℹ️ 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".
| rKey := routeKey(mut.Key) | ||
| if snap.WriteFencedForKey(rKey) { |
There was a problem hiding this comment.
For resolver-owned SQS partition keys, the coordinator now deliberately skips the byte-route write-fence precheck, but the real FSM still runs this unconditional routeKey fence and normalizes those keys to the byte-route namespace (for example !sqs|route|global). In a partitioned queue whose byte route is WriteFenced, the request can be routed to the resolver's group and then fail at apply time with ErrRouteWriteFenced, so the partition-resolver path that the coordinator allows still cannot commit in production. Carry an explicit resolver-bypass marker to the FSM or avoid bypassing the coordinator precheck until the apply-time fence can make the same distinction.
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 (c8f865e):
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 Updated in c8f865e:
Validation:
Caller audit:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c8f865ea84
ℹ️ 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".
| } | ||
|
|
||
| func isRetryableTransactWriteError(err error) bool { | ||
| return errors.Is(err, store.ErrWriteConflict) || errors.Is(err, kv.ErrTxnLocked) || errors.Is(err, kv.ErrRouteWriteFenced) |
There was a problem hiding this comment.
When ReceiveMessage tries to rotate a visible SQS candidate while that route is write-fenced, the coordinator returns ErrRouteWriteFenced before applying anything. Adding that sentinel to this broad retryable helper also changes existing skip-on-race call sites such as tryDeliverCandidate, where isRetryableTransactWriteError(err) returns skip=true, nil; in that migration window consumers can get an empty/smaller 200 response even though the message is still visible, instead of retrying or surfacing backpressure. Use the narrower non-fence race predicate at swallow sites, or keep the fence retry classification out of this shared helper.
Useful? React with 👍 / 👎.
|
@codex review Addressed the current SQS receive route-fence finding. Changes:
Validation:
Head: b553090 |
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 (b553090):
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". |
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 (d2b22ac):
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 Updated in d2b22ac:
Validation:
Runtime verification:
|
|
Codex Review: Didn't find any major issues. Bravo. 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
Summary
Author: bootjp
Implements the M2-PR5 migration guards:
WriteFencedroutes in the coordinator and FSMDEL_PREFIXby route-footprint intersection, including full-range deletesSplitRangerequests that overlap a live split job while allowing disjoint same-group splitsValidation
GOCACHE=$(pwd)/.cache GOTMPDIR=$(pwd)/.cache/tmp go test ./kv ./distribution ./adapter -run 'TestFSMRejects|TestShardedCoordinatorRejects|TestPendingTxnLocksInRoute|TestDistributionServerSplitRange_(RejectsLiveSplitJobOverlap|AllowsDisjointRouteWhileSplitJobLive|Success|UsesCoordinatorForCatalogWrites)|TestPlanMigrationBrackets|TestMigrationKnownInternalPrefixes|TestValidateMigrationRouteRange|TestSplitJobPlanner|TestVerifyComposed1|TestKvFSM' -count=1 -timeout=240sGOCACHE=$(pwd)/.cache GOTMPDIR=$(pwd)/.cache/tmp go test ./kv ./distribution -count=1 -timeout=240sGOCACHE=$(pwd)/.cache GOLANGCI_LINT_CACHE=$(pwd)/.golangci-cache golangci-lint run ./kv ./distribution ./adapter --timeout=5m