migration: complete target promotion catalog state - #1090
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 |
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 (351477d):
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. |
![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 the target promotion completion logic for split job migrations, including the state transition function CompleteTargetPromotionState and the transactional catalog store method CompleteSplitJobTargetPromotion. Feedback suggests strengthening defensive checks by validating the route's parent ID and start key during route clearing, and removing a redundant history lookup that could impact performance.
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.
| if !route.StagedVisibilityActive || route.GroupID != job.TargetGroupID { | ||
| return nil, errors.WithStack(ErrMigrationInvalidRoute) | ||
| } |
There was a problem hiding this comment.
To enforce defensive programming and ensure catalog consistency, we should also validate that the target route's ParentRouteID and Start key match the split job's SourceRouteID and SplitKey before clearing the staged visibility fields. This prevents accidentally promoting an inconsistent or incorrect route descriptor.
| if !route.StagedVisibilityActive || route.GroupID != job.TargetGroupID { | |
| return nil, errors.WithStack(ErrMigrationInvalidRoute) | |
| } | |
| if !route.StagedVisibilityActive || route.GroupID != job.TargetGroupID || route.ParentRouteID != job.SourceRouteID || !bytes.Equal(route.Start, job.SplitKey) { | |
| return nil, errors.WithStack(ErrMigrationInvalidRoute) | |
| } |
| if err := s.expectLiveSplitJobAt(ctx, expected.JobID, expectedRaw, readTS); err != nil { | ||
| return 0, 0, nil, err | ||
| } | ||
| if _, found, err := s.historySplitJobAt(ctx, expected.JobID, readTS); err != nil { | ||
| return 0, 0, nil, err | ||
| } else if found { | ||
| return 0, 0, nil, errors.WithStack(ErrCatalogSplitJobConflict) | ||
| } |
There was a problem hiding this comment.
The check s.historySplitJobAt is redundant and inefficient here. Since s.expectLiveSplitJobAt has already succeeded, the split job is guaranteed to be in the live state. In a consistent MVCC catalog, a split job cannot exist in both the live state and the history state simultaneously. Scanning the entire history of split jobs (which can grow very large in production) is expensive and unnecessary. We can safely remove this check.
| if err := s.expectLiveSplitJobAt(ctx, expected.JobID, expectedRaw, readTS); err != nil { | |
| return 0, 0, nil, err | |
| } | |
| if _, found, err := s.historySplitJobAt(ctx, expected.JobID, readTS); err != nil { | |
| return 0, 0, nil, err | |
| } else if found { | |
| return 0, 0, nil, errors.WithStack(ErrCatalogSplitJobConflict) | |
| } | |
| if err := s.expectLiveSplitJobAt(ctx, expected.JobID, expectedRaw, readTS); err != nil { | |
| return 0, 0, nil, err | |
| } |
References
- Remove dead or unreachable code paths, such as asynchronous handlers for tasks that are always performed synchronously, to improve code clarity and maintainability.
![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: 351477d4e1
ℹ️ 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".
| out.Changed = true | ||
| out.ClearedRouteIDs = cleared | ||
| out.Job.TargetPromotionDone = true | ||
| out.Job.PromotionCompletedTS = completionTS |
There was a problem hiding this comment.
In the promotion-complete path, this stores the caller-supplied completionTS in promotion_completed_ts, but the design contract for that field is the HLC commit timestamp of the default-group CAS that clears the descriptor (see docs/design/2026_06_11_proposed_hotspot_split_milestone2_migration.md, promotion_completed_ts). In normal calls the target-local promotion timestamp/max promoted timestamp can differ from the catalog MVCC commit timestamp chosen later in applyPromotionCompleteMutations, so target cleared-descriptor ACKs can prove or wait on the wrong timestamp; compute the catalog commit timestamp before encoding the job and persist that value instead.
Useful? React with 👍 / 👎.
351477d to
c57e42a
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 (c57e42a):
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. |
c57e42a to
2edefb8
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 (2edefb8):
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 Addressed the latest-head metadata cleanup and MVCC snapshot findings in dcb7744. Changes:
Semantic audit:
Validation:
|
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 (dcb7744):
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: dcb7744f35
ℹ️ 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".
| Ready: time.Now().UnixMilli() >= req.GetReadDrainNotBeforeMs() && | ||
| (i.readTracker == nil || i.readTracker.Oldest() == 0), |
There was a problem hiding this comment.
When a split reaches source-read-drain cleanup, this probe only reports ready if the process-wide ActiveTimestampTracker is completely empty. In production the same tracker is wired into the Internal service and all frontend read paths, so any unrelated Redis/Dynamo/S3 read on this node keeps Oldest() nonzero even if it started after cutover or targets another range. On a busy node this can keep SOURCE_READ_DRAINED false indefinitely, preventing source cleanup/metadata cleanup and leaving the migration retention pin in place; the drain check needs to distinguish pre-cutover reads for the moving range (or at least reads older than the cutover/grace point) instead of requiring zero global reads.
Useful? React with 👍 / 👎.
The SOURCE_READ_DRAINED probe required a globally empty ActiveTimestampTracker, but that tracker is process-wide and is wired into every frontend read path. On a busy node any unrelated Redis/DynamoDB/S3 read keeps Oldest() non-zero, so source cleanup and the migration retention pin could stay blocked indefinitely. Carry the promotion-completed HLC as read_drain_min_ts and only count pins at or below it: reads issued after the source read fence armed carry a newer timestamp and are already rejected for the moving range. A zero floor (pre-upgrade coordinator) keeps the old empty-tracker requirement, so the relaxation is opt-in from the control plane. Claude-Session: https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE
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 (75d86c0):
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. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
To use Codex here, create a Codex account and connect to github. |
|
Codex P2 ( 変更点
Caller audit: Test evidence
Self-review
なお、レビューが併記していた「移動対象レンジ以外の read を除外する」ためのレンジ単位トラッキングは、全 read パスの @claude review |
|
Claude finished @bootjp's task in 3m 27s —— View job Review:
|
The only remaining M3 work is M3-PR4 least-loaded target_group_id selection, and its own entry said it waits on the M2 migration plane. An entry that names an unmet prerequisite does not belong under "Ready now -- no unmet prerequisites", where a contributor reading the index would take it as schedulable today. Move it to the gated list, where it names step 1 the way the other gated entries name what they wait for. Also correct the audit date. The index tells readers each status is a snapshot from that date, and 2026-08-22 cannot describe a baseline whose M3 row cites 07c48af -- that commit merged on 2026-08-25. Re-verified against origin/main and the open pull requests while dating it: the M2 row's #1084/#1085/#1088/#1090, the dedicated TSO row's #1095, and the live backup row's #1056/#1128 are all still open, and no other row's evidence moved. Claude-Session: https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE
|
Claude finished @bootjp's task in 2m 43s —— View job Review: current HEAD
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 75d86c08c9
ℹ️ 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".
| readinessStart, readinessEnd := readinessRouteRangeForScan(start, end) | ||
| if err := s.verifyExplicitGroupRoutesForRange(ctx, groupID, routes, readinessStart, readinessEnd); err != nil { |
There was a problem hiding this comment.
When a RawScanAt request uses keys_only, especially against an explicit or stale source group during cutover, this new readiness check is bypassed: ScanKeysAtWithReadFence proceeds through scanKeyRouteAtWithReadFence, whose leader and local key-scan paths never call targetReadyRouteForRange. Consequently, an armed source read fence can still return old-source keys, while an armed target guard can return live-only or empty keys during catalog-watcher lag; apply the same readiness proof before key-only scans.
Useful? React with 👍 / 👎.
| return gateErr | ||
| } | ||
| } | ||
| return s.runSplitJobPhase(ctx, job) |
There was a problem hiding this comment.
When a phase encounters a persistent error such as a corrupt export/cleanup cursor or a changed source route, this return only passes the error to the outer logging loop. No runner path assigns SplitJobPhaseFailed, RetryPhase, or LastError, so the job remains runnable, is retried every tick, keeps occupying the single live-job slot and any post-cutover guards indefinitely, and can never be acted on through RetrySplitJob; classify and durably record failures requiring operator intervention before returning.
Useful? React with 👍 / 👎.
| if pendingLocks { | ||
| return nil |
There was a problem hiding this comment.
When ELASTICKV_LOCK_RESOLVER_ENABLED=false and a client leaves an expired prepared lock on a cold key in the moving range, this branch returns successfully forever: ProbeMigrationLocks only counts locks and neither this runner nor the probe resolves or aborts them. Because no foreground read needs to touch that cold key, the job remains in FENCE with the route write-fenced indefinitely; actively run the route-faithful lock resolver here or keep migration capability closed when the background resolver is disabled.
Useful? React with 👍 / 👎.
| current.FenceTS = fenceTS | ||
| current.FenceCatalogVersion = fenceCatalogVersion | ||
| current.FenceAckCursor = distribution.CloneBytes(ackCursor) | ||
| current.SourceRetentionPinTS = 1 |
There was a problem hiding this comment.
After deltaFloor has been computed, retaining a pin of 1 keeps the group-wide FSM compactor from pruning essentially any historical MVCC versions throughout DELTA_COPY, CUTOVER, and CLEANUP, even though the remaining export window is only (deltaFloor, fenceTS]. A slow or stalled migration can therefore cause unbounded disk growth on the source group; persist and reapply deltaFloor as the source control record's retention pin once that floor is durable.
Useful? React with 👍 / 👎.
scanKeyRouteAtWithReadFence resolved the group and went straight to the local, leader, or proxy branch without calling targetReadyRouteForRange, so keys_only reads skipped the readiness proof every value scan performs. An armed source read fence still handed back keys from the old source, and an armed target guard handed back live-only or empty keys while the catalog watcher lagged. RawScanAt with keys_only against an explicit source group is exactly the shape those fences exist for. Take the ready route the same way scanRouteAtDirection does, right after the group lookup and before any branch. Claude-Session: https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE
|
Verified the whole path before changing anything. Fix: take the ready route right after the group lookup and before any branch — the same shape Test: The three P2s on this PR (
|
|
Claude finished @bootjp's task in 1m 51s —— View job Review:
|
|
Codex Review: Didn't find any major issues. 🎉 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". |
|
I attempted the base-branch merge ( What is resolvable and what I already worked out:
The blocker is
Picking a layout here decides what a V3 snapshot means on disk, and getting the write and read sides subtly out of step corrupts snapshot round-trips rather than failing loudly. The mechanical-looking answer — base's V3 as the substrate, this branch's readiness section appended as V4 — is probably right, but it is a format decision with a data-loss failure mode, so I would rather you confirm the intended version sequence and section order than have me invent it. Say which way you want it and I will do the merge and the round-trip verification in the next pass. The branch is untouched; nothing half-merged was left behind. |
|
Claude finished @bootjp's task in 3m 3s —— View job Snapshot version decision
Environment note first: this session is a shallow, network-disabled sandbox — Decision: don't stack V2 → V3 → V4. Collapse into one unified V2. The deciding fact: Given that, defining V3 as "base's format" and V4 as "base + this branch's readiness section" would permanently bake an intermediate, never-shipped dev format into the version history for no compatibility benefit — and it's the riskier path precisely because it invites exactly the kind of silent field-order mismatch you're worried about (two authors independently deciding what "V3" vs "V4" byte-for-byte contains, verified only by each branch's own tests, not by a merged round-trip test). Concrete resolution:
One risk I can't rule out from here: if some other in-flight branch or a running dev/Jepsen environment already has real V2- or V3-tagged snapshot files on disk that need to survive this merge, the "nothing has shipped" argument doesn't hold and you'd need the versioned-tiers approach instead. From what's visible in this PR's history that looks unlikely (M2 is still If you agree with the merged-V2 approach, I can do the merge and round-trip verification in the next pass — just confirm and I'll proceed (this branch is otherwise untouched, no changes pushed). |
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37b1d8f088
ℹ️ 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".
| MaxScannedBytes: maxScannedBytes, | ||
| KeyFamily: req.GetKeyFamily(), | ||
| AcceptVersion: func(key, value []byte) bool { | ||
| return bracket.ContainsRoutedVersion(key, value, req.GetRouteStart(), req.GetRouteEnd(), routeKey) |
There was a problem hiding this comment.
When the migrated interval contains !sqs|route|global and HT-FIFO partitions remain assigned to the source group, this predicate treats every partitioned SQS row as belonging to the catalog route because routeKey collapses those keys to the global SQS key. The export path instead uses RouteKeyFilterForGroup and the partition resolver, but neither the catalog cutover nor this cleanup updates that resolver; requests therefore continue routing those partitions to the source while CLEANUP deletes their rows, causing message loss. Pass the partition-aware ownership predicate through cleanup, or reject catalog migrations that intersect resolver-owned SQS data.
Useful? React with 👍 / 👎.
| if !bytes.Equal(importResp.GetAckedCursor(), nextCursor) { | ||
| return errors.New("split migration import acknowledged a different cursor") |
There was a problem hiding this comment.
When the target applies a batch but the runner dies before persistSplitJobCopyProgress, the target is one sequence ahead of the durable job. If a concurrent source write changes the replayed chunk boundary before restart, re-exporting from the old cursor produces a different nextCursor; the importer correctly treats the repeated sequence as a duplicate and returns its previously acknowledged cursor, but this comparison rejects that recovery state forever. The job then remains live with its migration guards and retention pin held, so reconcile progress from the target's durable acknowledgement instead of requiring the newly replayed chunk to have the same boundary.
Useful? React with 👍 / 👎.
| if route.ParentRouteID != job.SourceRouteID { | ||
| continue |
There was a problem hiding this comment.
After FENCE replaces the original route with left and right children, SplitRange intentionally permits another split wholly within the disjoint left child. That operation replaces the left child with grandchildren whose ParentRouteID is the left child's ID, so this filter can no longer find the route ending at job.SplitKey; once the moving right child belongs to the target, neither the original parent nor a matching direct sibling remains from which to recover the source group. CLEANUP then returns ErrMigrationSourceRouteChanged permanently after cutover, retaining guards and the live job. Persist the source group in the job, follow route ancestry, or prevent sibling rewrites while it is needed.
Useful? React with 👍 / 👎.
A runner that dies after the target durably acknowledged an import batch but before persisting its own progress replays that batch on restart. The source keeps taking writes while the copy runs, so the replayed chunk can end on a different boundary than the one the target accepted; the target then answers as a duplicate with its own recorded cursor, and the strict boundary check rejected that recovery state on every retry, leaving the job live with its migration guards and retention pin held. store.ImportVersionsResult already reported Duplicate on both backends and every caller dropped it. Carry it on ImportRangeVersionsResponse and let the runner adopt the target's durable acknowledgement: cursor and batch sequence advance, the replayed rows are not counted again, and the stream is restarted from the adopted cursor rather than continuing on a boundary the target never accepted. Claude-Session: https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE
FENCE replaces the source route with a left child that keeps the source group and a right child that moves to the target. SplitRange then permits another split wholly inside the left child, since that range is disjoint from the moving one, and it replaces the left child with grandchildren that name the left child as their parent. From then on no route ends at the split key under the original parent, and the parent itself is already gone, so the group still holding the source data cannot be recovered from the route shape at all: cleanup failed with ErrMigrationSourceRouteChanged on every attempt after cutover and kept the job live with its migration guards and retention pin held. Record the source group on the job at plan time, where it is known and where it cannot be rewritten by a later split, and consult it after the route shape and the original parent have both been tried. Jobs written before the field decode with group 0 and keep resolving exactly as they did. The three call sites that each carried their own copy of the parent fallback now share one resolver. Claude-Session: https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b216767870
ℹ️ 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".
| case req.GetMode() == pb.MigrationCleanupMode_MIGRATION_CLEANUP_MODE_VERSIONS && req.GetKeyFamily() == 0: | ||
| return errors.WithStack(status.Error(codes.InvalidArgument, "migration cleanup key_family is required")) |
There was a problem hiding this comment.
When the cleanup opcode gate is open, a VERSIONS request with a nonzero job_id and key_family but omitted range_start, range_end, route_start, and route_end passes this validation. migrationCleanupOptionsFromProto then creates a bracket with no route check, and the store treats the nil raw bounds as a full-keyspace scan; if max_commit_ts is also omitted, zero means no timestamp ceiling. Such a malformed internal RPC therefore Raft-replicates deletion of arbitrary user versions across the entire group (up to the per-call limit), so require a raw or route bound for version cleanup, analogous to export validation.
Useful? React with 👍 / 👎.
routeKey collapses every HT-FIFO partitioned SQS row to the single global SQS route key, so a migrated interval covering that key looked to cleanup like it owned every partition -- including the ones the resolver still routes to this group, whose rows the export never claimed. Cleanup deleted them while requests kept being served from here, losing messages. Apply cannot ask the resolver which partitions moved: it is process-local config (--sqsFifoPartitionMap), not replicated, so a per-node answer would make FSM apply diverge across replicas. Recognise the partitioned shape structurally instead -- a prefix test over compile-time constants, identical on every replica -- and leave that keyspace alone: catalog route cleanup deletes only catalog-routed data. Retiring a partition's rows belongs to the resolver's own migration path. kv.IsPartitionedSQSKey carries its own copy of the family prefixes, so it is pinned against keys built by the adapter's real constructors. Claude-Session: https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE
|
1.
|
|
Claude finished @bootjp's task in 3m 11s —— View job Review: HEAD
|
|
Codex Review: Didn't find any major issues. 🎉 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". |
Author: bootjp
Summary:
Tests: