migration: promote staged versions#1089
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 |
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 (0df13a2):
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 Validation for latest head 0df13a2:
|
![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 a new feature to promote staged range-migration data into the live keyspace, adding the PromoteStagedVersions gRPC endpoint, a new Raft FSM command, and implementing the MigrationPromoter interface for both mvccStore and pebbleStore. While the implementation is well-tested, a critical issue was identified in store/migration_promote.go where a slice stored in the B-tree is mutated in-place during version removal. This can lead to data races or corruption for concurrent readers, and should be resolved by allocating a new slice and copying the elements instead.
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 idx < 0 { | ||
| return false | ||
| } | ||
| versions = append(versions[:idx], versions[idx+1:]...) |
There was a problem hiding this comment.
Mutating the versions slice in-place using append on versions[:idx] modifies the underlying array of the slice stored in the B-tree. Since this slice may be concurrently accessed or held by active iterators/readers (even if protected by s.mtx during the mutation, readers might have already obtained a reference to the slice), in-place mutation can lead to data races or unexpected data corruption. To ensure safety, allocate a new slice and copy the elements instead of mutating the existing slice's underlying array.
newVersions := make([]VersionedValue, len(versions)-1)
copy(newVersions, versions[:idx])
copy(newVersions[idx:], versions[idx+1:])
versions = newVersionsThere was a problem hiding this comment.
Fixed in b4f2477 by allocating a fresh version slice before removing the promoted staged version, so the tree-held backing array is not mutated in place.
bootjp
force-pushed
the
design/hotspot-split-m2-promote
branch
from
0df13a2 to
8c59f29
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 (8c59f29):
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 Validation for latest head 8c59f29:
|
|
To use Codex here, create a Codex account and connect to github. |
bootjp
force-pushed
the
design/hotspot-split-m2-promote
branch
from
8c59f29 to
b4f2477
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 (b4f2477):
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. |
![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: b4f247751b
ℹ️ 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".
| return errors.WithStack(err) | ||
| } | ||
| } | ||
| if err := batch.Commit(s.directApplyWriteOpts()); err != nil { |
There was a problem hiding this comment.
When this path is reached from kvFSM.applyMigrationPromote, the batch has copied live versions and deleted staged rows but commits without writing metaAppliedIndex for f.pendingApplyIdx, unlike the other Raft data paths (ApplyMutationsRaftAt / DeletePrefixAtRaftAt). On disk-backed groups whose post-snapshot traffic is only promotion chunks, LastAppliedIndex stays behind the committed WAL tail, so every restart misses the cold-start skip gate and restores/replays the full FSM snapshot until some unrelated write or snapshot advances the meta key. Thread the apply index through promotion and include it in this batch.
Useful? React with 👍 / 👎.
|
Addressed latest promotion applied-index feedback in c1afe27:
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 (c1afe27):
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: c1afe27060
ℹ️ 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 opts.JobID != 0 && state.Done { | ||
| return PromoteVersionsResult{Done: true, TotalPromotedRows: state.PromotedRows}, nil |
There was a problem hiding this comment.
When a retry/controller proposes another PromoteStagedVersions entry after the job state is already Done, this Pebble path returns before commitPebblePromoteVersions, so opts.AppliedIndex is never written to metaAppliedIndex. The promotion is idempotent, but LastAppliedIndex remains at the previous entry, so the cold-start skip gate can replay this log entry on every restart until a later mutating entry advances the meta key; persist the applied index in this no-op branch too.
Useful? React with 👍 / 👎.
|
Addressed current-head promotion blockers in dd52db8. Changes:
Validation:
Note: attempted go test ./store ./kv ./adapter; kv passed, store hit a timeout in TestPebbleStore_RestoreWaitsForMaintenanceLock during the combined run, and that exact test passed when rerun alone. @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 (dd52db8):
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: dd52db8198
ℹ️ 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".
| for _, version := range versions { | ||
| targets = append(targets, version.target) | ||
| } | ||
| if err := s.applyImportVersionsBatch(batch, targets); err != nil { |
There was a problem hiding this comment.
When storage envelopes are active and this node has not yet marked its writer registration for the active DEK, this call routes promoted live values through applyImportVersionsBatch, which invokes encryptForKey(..., true) and returns ErrWriterNotRegistered. Unlike direct writes, promotion is applied from the Raft FSM (applyMigrationPromote), and the existing Raft apply path explicitly passes gateRegistration=false so committed entries can replay deterministically before local registration catches up; with the new HaltApply wrapping, a promotion chunk containing any non-tombstone value can halt the apply loop on such replicas during an encrypted migration.
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 (b04d30f):
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. |
|
Addressed current-head P1 promotion gate feedback in b04d30f. Changes:
Validation:
Note: attempted go test ./...; adapter package timed out after 10m while unrelated full-package network/raft adapter tests were still running. The targeted adapter migration tests above passed, and the all-package compile-only check passed. @codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. 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 (a9c9afe):
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. 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". |
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 (5254a43):
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. |
|
Resolved the merge conflict against design/hotspot-split-m2-cross-group in 5254a43. Integrated the newer exact-key migration metadata/export cursor changes with the staged promotion implementation by keeping promotion state under the reserved _migpromote metadata key and preserving the normalized export cursor path for promotion scans. Validation:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5254a4330d
ℹ️ 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 endKey != nil && bytes.Compare(pos.key, endKey) >= 0 { | ||
| return errors.WithStack(ErrInvalidExportCursor) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
When the supplied cursor uses the skipped-key tag for a key inside the staged prefix, this helper returns nil because it only checks bounds. PromoteStagedVersions then proposes the command, but the FSM path revalidates through ExportVersions/validateExportCursorRange and treats the same cursor as ErrInvalidExportCursor, which applyMigrationPromote wraps as a halting apply error. With the promote opcode gate enabled, a malformed or buggy cursor can therefore be committed to Raft and halt apply instead of being rejected as InvalidArgument before propose; mirror the skipped-cursor rule used by the store here.
Useful? React with 👍 / 👎.
| if opts.JobID == 0 { | ||
| return PromotionState{}, opts.Cursor | ||
| } | ||
| state := clonePromotionState(s.migrationPromotions[opts.JobID]) |
There was a problem hiding this comment.
For an in-memory store that snapshots before promotion, then later records migrationPromotions[jobID] and restores that snapshot on the same store, restoreStreamingSnapshot replaces the tree and clears the import/HLC maps but leaves this promotion map intact. This lookup can then reuse a cursor or Done bit from the pre-restore timeline, so a restored staged range for the same job may be reported done or resumed mid-range and never promoted; reset the promotion map during restore like the other migration metadata.
Useful? React with 👍 / 👎.
|
|
||
| func validatePromoteStagedVersionsRequest(req *pb.PromoteStagedVersionsRequest) error { | ||
| prefix := distribution.MigrationStagedDataKeyPrefix(req.GetJobId()) | ||
| if err := store.ValidateExportCursorForRange(req.GetCursor(), prefix, store.PrefixScanEnd(prefix)); err != nil { |
There was a problem hiding this comment.
This RPC accepts any decoded cursor that falls inside the staged prefix, but promotion scans never legitimately return scanned/pruned cursor tags because they have no key filter or min-timestamp pruning. If a retry or controller bug passes one of those cursors, the store interprets it as already-processed state: scanned skips that exact staged version, and pruned skips the rest of the staged key, allowing promotion to finish while leaving rows unpromoted. Validate that promotion cursors are empty or emitted cursors before proposing the Raft command.
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 (b1381f3):
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. |
|
Addressed the current-head promotion blockers in b1381f3. Changes:
Validation:
@codex review |
|
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: b1381f3afb
ℹ️ 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 !exportSkippedCursorOutsideRange(opts, pos.key) { | ||
| return errors.WithStack(ErrInvalidExportCursor) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
This accepts a skipped-key cursor whose key is below startKey, but the in-memory exporter resumes from pos.key whenever a cursor is present and only treats PrunedKey as a whole-key skip. If that below-range key has older versions than the cursor timestamp, those older versions are emitted despite the requested start bound (for example, StartKey=m with a skipped cursor for a@10 exports a@5). That can leak/import versions outside the migration bracket; either keep rejecting below-start skipped cursors or make the memory resume path skip the whole key and re-seek to the requested range.
Useful? React with 👍 / 👎.
| if len(state.Cursor) == 0 { | ||
| return state, opts.Cursor | ||
| } |
There was a problem hiding this comment.
When a job has no stored PromotionState (for example after the MVCC snapshot restore path clears migrationPromotions), this falls back to the caller-provided cursor. If the migration driver retries with the last NextCursor from before that restore while the restored snapshot still contains staged rows before that cursor, those rows are skipped and never promoted; a job-scoped promotion with absent local state needs to restart from the empty cursor/remaining staged rows rather than trusting a stale client cursor.
Useful? React with 👍 / 👎.
|
@codex review Merged latest design/hotspot-split-m2-cross-group into this branch in b4b8664 and resolved the adapter/internal.go + adapter/internal_migration_test.go conflicts. Conflict resolution:
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 (b4b8664):
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: b4b8664c84
ℹ️ 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".
| state.Cursor = bytes.Clone(exported.NextCursor) | ||
| state.Done = exported.Done | ||
| state.PromotedRows += promoted.PromotedRows |
There was a problem hiding this comment.
When a promotion chunk commits, the store persists only the cursor/done/row count, while applyMigrationPromote observes MaxPromotedTS only after PromoteVersions returns. If the process crashes after the Pebble batch commits but before that observe, the retry resumes from this persisted cursor and never reports the skipped chunk's max timestamp again; when imported source timestamps are ahead of the local clock, the leader can later issue commit timestamps below already-promoted live versions. The promotion resume state needs to carry the max promoted timestamp (or otherwise return a durable floor) just like import retries do.
Useful? React with 👍 / 👎.
| if !ok || len(state.Cursor) == 0 { | ||
| return state, opts.Cursor, nil |
There was a problem hiding this comment.
When there is no target-local promotion state yet, this trusts the request cursor as the starting point. The RPC-side check only proves that the cursor decodes to an emitted position inside the job prefix, so a first PromoteStagedVersions call with a later valid cursor can skip all earlier staged MVCC rows, then persist progress/done from that point. The target can report promotion complete while the skipped staged rows were never copied to live, which becomes data loss once staged visibility is cleared.
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 (40c2f52):
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 current-head stale promotion cursor handling in 40c2f52. Changes:
Validation:
|
|
To use Codex here, create a Codex account and connect to github. |
…oup' into design/hotspot-split-m2-promote
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 (327cb1c):
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 Synced PR #1089 with latest origin/design/hotspot-split-m2-cross-group at 19a28e9 via merge commit 327cb1c. Current head includes the stale promotion cursor fix from 40c2f52:
Validation after the base merge:
|
…oup' into design/hotspot-split-m2-promote
|
@codex review Merged latest design/hotspot-split-m2-cross-group into this branch in 4b1d950 after the #1088 base advanced. 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 (4b1d950):
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: 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
Tests