migration: add range version RPC handlers#1088
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 (64dc6f4):
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 adds support for range migrations by implementing ExportRangeVersions and ImportRangeVersions on the internal gRPC server, alongside updating the distribution Route metadata to track migration states. Feedback focuses on adding a defensive check in ExportRangeVersions to prevent infinite loops when the cursor does not progress, and removing an unreachable nil guard on i.leader in verifyInternalLeader to comply with guidelines against dead defensive code, which also requires updating the corresponding unit tests to use a mock leader.
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.
![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: 64dc6f4239
ℹ️ 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 (59a1672):
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 59a1672:
Validation:
Note: a broader go test ./adapter ./distribution ./kv ./store -count=1 -timeout=240s timed out in the adapter integration suite; the targeted touched-path tests above passed. |
59a1672 to
fbd7f56
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 (fbd7f56):
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 Rebased onto the latest design/hotspot-split-m2-fence-drain head in fbd7f56 after #1087 advanced. The migration import/export fixes remain intact and now include the parent broad DEL_PREFIX fence fix. Validation:
|
|
To use Codex here, create a Codex account and connect to github. |
|
@codex review Updated in 0fe341d:
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 (0fe341d):
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 (7d3b01c):
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 7d3b01c:
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 (e7f69ef):
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 e7f69ef:
Validation:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7f69efd9e
ℹ️ 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 latestMVCCVersionAt(ctx context.Context, st store.MVCCStore, key []byte, ts uint64) (store.MVCCVersion, bool, error) { | ||
| result, err := st.ExportVersions(ctx, store.ExportVersionsOptions{ | ||
| StartKey: key, | ||
| EndKey: nextScanCursor(key), |
There was a problem hiding this comment.
When the backing store is Pebble, ExportVersions converts EndKey to encodeKey(EndKey, MaxUint64) (store/lsm_migration.go), and nextScanCursor(key) (key + 0x00) sorts before normal encoded versions of key because the timestamp suffix begins with ^commitTS. On a staged-visible route backed by Pebble, both the live and staged point exports therefore return no candidates for ordinary keys, so GetAt can incorrectly return ErrKeyNotFound even though the key exists. Use a true single-key upper bound/exact-key filter instead of the scan cursor here.
Useful? React with 👍 / 👎.
| ts uint64, | ||
| reverse bool, | ||
| ) ([]*store.KVPair, error) { | ||
| live, err := collectLatestLogicalVersions(ctx, g.Store, start, end, start, end, ts, liveLogicalVersionKey) |
There was a problem hiding this comment.
For staged-visible routes, this call starts an export loop that runs until result.Done, and the caller only applies limit after collecting both the full live range and the full staged prefix into maps. A user scan such as ScanAt(..., limit=100) over a migrated production range can therefore read and retain every version in the route before returning one page, which is a large latency/memory regression versus the previous bounded store scan. Please stop the merge once enough ordered candidates are available or otherwise page the merge by the requested limit.
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 (ca1a050):
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 Updated in ca1a050:
Validation:
|
|
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: ca1a05070c
ℹ️ 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".
| nextWindow := nextStagedVisibilityCandidateWindow(window) | ||
| if nextWindow == window { | ||
| return out, nil |
There was a problem hiding this comment.
For staged-visibility routes with more than 8192 visible logical keys, any caller asking for a limit above stagedVisibilityMaxCandidateWindow can get a truncated result. Once window reaches 8192, a full underlying scan with len(out) < limit falls into nextWindow == window and returns immediately, even though there may be more keys after the current candidate window. This makes large ScanAt/ReverseScanAt calls silently miss keys during staged cutover.
Useful? React with 👍 / 👎.
| if routeStart != nil && bytes.Compare(routeKey, routeStart) < 0 { | ||
| return false | ||
| } | ||
| return routeEnd == nil || bytes.Compare(routeKey, routeEnd) < 0 |
There was a problem hiding this comment.
This decoded S3 predicate only treats nil as an unbounded route_end, unlike RouteKeyFilter and the route descriptor convention where nil or empty means +infinity. If an unbounded last route is passed as a non-nil empty byte slice, every decoded bucket route key compares greater than []byte{}, so bucket meta/generation exports for that route return no versions. Check len(routeEnd) == 0 here instead of only routeEnd == nil.
Useful? React with 👍 / 👎.
| if err := proto.Unmarshal(data, req); err != nil { | ||
| return errors.WithStack(err) | ||
| } | ||
| result, err := f.store.ImportVersions(ctx, store.ImportVersionsOptions{ |
There was a problem hiding this comment.
In encrypted clusters where storage-envelope cutover is active but a replica's writer registration is not yet committed, this FSM-apply path calls store.ImportVersions, whose Pebble implementation uses the direct write/encryption gate (encryptForKey(..., true)). That can return ErrWriterNotRegistered while replaying the Raft entry and fail the import on that replica; migration imports need a raft-apply variant that uses the non-gated FSM path instead of the direct ImportVersions path.
Useful? React with 👍 / 👎.
|
|
||
| if engineForGroup(g) == nil { | ||
| return s.localGetAt(ctx, g, key, ts) | ||
| return s.localGetAt(ctx, g, distribution.Route{}, key, ts) |
There was a problem hiding this comment.
When callers use the explicit-group path (for example RawGet/RawScanAt with group_id for SQS partitioned ownership), this passes a zero route into the local read path, so routeHasStagedVisibility is always false and imported staged versions are ignored after cutover. Those reads can return not found or stale live data even though normal GetAt/ScanAt for the same logical route would merge staged data; the explicit group path needs the staged route metadata or must fail closed while staged visibility is active.
Useful? React with 👍 / 👎.
|
Addressed the staged-visibility migration review items. Changes:
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 (d33bf35):
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: d33bf3589a
ℹ️ 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 route, ok := s.engine.GetRoute(routeKey(key)); ok && route.GroupID == groupID { | ||
| return route, nil | ||
| } | ||
| if s.groupHasStagedVisibility(groupID) { |
There was a problem hiding this comment.
When an explicit-group RawGet arrives with a stale group_id after cutover (for example, the caller still targets the old source group while engine.GetRoute(key) now returns the target route with StagedVisibilityActive), this checks staged visibility on the requested group instead of the resolved route. If the old group has no staged route, the function falls through to fallback and reads the old group's live MVCC without the staged merge, serving stale pre-migration data instead of failing closed or rerouting.
Useful? React with 👍 / 👎.
| if len(matched) > 0 { | ||
| return matched, true, nil | ||
| } | ||
| if s.groupHasStagedVisibility(groupID) { |
There was a problem hiding this comment.
For explicit-group RawScan, a stale proxy can still send the old source group_id after the current intersecting route has moved to the target with staged visibility. In that case matched is empty, this check only inspects the requested group, and the fallback scans the old group's local store, so range reads can miss staged target data instead of failing closed; fail when any intersecting current route is staged but not matched to the requested group.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5affdac49a
ℹ️ 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 (cb7b3de):
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 Latest HEAD cb7b3de merges the current parent and addresses the latest findings: decoded S3 auxiliary floor routing, resolver-bypass floor handling across all apply callers, and staged/live key-only scans. Validation: go test ./kv -count=1; go test ./... -run ^$ -count=1; golangci-lint run ./... --timeout=5m. |
|
/gemini review |
|
@claude review |
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
Code Review
This pull request implements staged range migration support, introducing target-group migration import batches and staged-data promotion. It adds new RPCs for exporting, importing, and promoting staged versions, integrates staged visibility merging into shard store reads and scans, and enforces write timestamp floors to reject mutations below the migration floor. The review feedback is highly accurate, pointing out critical compilation errors in adapter/internal.go and adapter/internal_test.go where the undefined prefixScanEnd function is called instead of store.PrefixScanEnd or testPrefixScanEnd.
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.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements staged range migration support, introducing features to import, promote, and merge staged data versions with live keyspaces, alongside write timestamp floor validation to reject writes below the migration cutover floor. The review feedback highlights a critical lock-order inversion deadlock in PromoteVersions and a severe performance bottleneck in latestStagedVisibilityCandidates due to O(N) point lookups. Additionally, improvements are suggested to optimize slice allocations during key aliasing and to correctly handle empty-but-non-nil slices in scan bounds.
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.
| s.dbMu.RLock() | ||
| defer s.dbMu.RUnlock() | ||
|
|
||
| s.applyMu.Lock() | ||
| defer s.applyMu.Unlock() |
There was a problem hiding this comment.
Concurrency Issue: Lock-Order Inversion
There is a lock-order inversion between s.dbMu and s.applyMu.
In other write paths (such as ApplyMutations), the serialization lock s.applyMu is acquired first, followed by the database read lock s.dbMu.RLock(). However, in PromoteVersions, s.dbMu.RLock() is acquired before s.applyMu.Lock().
If one goroutine is executing ApplyMutations (holding applyMu and waiting for dbMu) while another goroutine is executing PromoteVersions (holding dbMu and waiting for applyMu), a deadlock will occur.
To prevent this, always acquire s.applyMu before s.dbMu.
| s.dbMu.RLock() | |
| defer s.dbMu.RUnlock() | |
| s.applyMu.Lock() | |
| defer s.applyMu.Unlock() | |
| s.applyMu.Lock() | |
| defer s.applyMu.Unlock() | |
| s.dbMu.RLock() | |
| defer s.dbMu.RUnlock() |
| func (s *ShardStore) latestStagedVisibilityCandidates( | ||
| ctx context.Context, | ||
| st store.MVCCStore, | ||
| route distribution.Route, | ||
| liveKVs []*store.KVPair, | ||
| stagedKVs []*store.KVPair, | ||
| ts uint64, | ||
| ) (map[string]store.MVCCVersion, error) { | ||
| keys := stagedVisibilityCandidateKeys(liveKVs, stagedKVs) | ||
| out := make(map[string]store.MVCCVersion, len(keys)) | ||
| for _, key := range keys { | ||
| live, liveOK, err := latestMVCCVersionAt(ctx, st, key, ts) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| stagedKey := distribution.MigrationStagedDataKey(route.MigrationJobID, key) | ||
| staged, stagedOK, err := latestMVCCVersionAt(ctx, st, stagedKey, ts) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if stagedOK { | ||
| staged.Key = bytes.Clone(key) | ||
| } | ||
| if winner, ok := newerMigrationVersion(live, liveOK, staged, stagedOK); ok { | ||
| out[string(key)] = winner | ||
| } | ||
| } | ||
| return out, nil | ||
| } |
There was a problem hiding this comment.
Efficiency Issue: O(N) Database Queries during Scan
The latestStagedVisibilityCandidates function performs two individual point lookups (latestMVCCVersionAt) for every single candidate key in the scan window. Under large scan windows (where stagedVisibilityMaxCandidateWindow can be up to 8192), this can result in up to 16,384 individual database queries (st.ExportVersions) within a single page scan. This will cause severe latency spikes and CPU overhead.
Suggested Optimization
Instead of performing O(N) point lookups, perform a single range-based ExportVersions scan for both the live and staged ranges, and then merge the two sorted version streams in memory. This reduces the database query complexity from O(N) to O(1) (specifically, just 2 range queries).
| func (s *ShardStore) readKeysWithStagedVisibilityAliases(group *ShardGroup, readKeys [][]byte) [][]byte { | ||
| if len(readKeys) == 0 { | ||
| return readKeys | ||
| } | ||
| for _, key := range readKeys { | ||
| readKeys = s.appendStagedVisibilityAlias(group, readKeys, key) | ||
| } | ||
| return readKeys | ||
| } | ||
|
|
||
| func (s *ShardStore) readKeysWithStagedVisibilityMutationAliases(group *ShardGroup, readKeys [][]byte, mutations []*store.KVPairMutation) [][]byte { | ||
| for _, mut := range mutations { | ||
| if mut == nil { | ||
| continue | ||
| } | ||
| readKeys = s.appendStagedVisibilityAlias(group, readKeys, mut.Key) | ||
| } | ||
| return readKeys | ||
| } | ||
|
|
||
| func (s *ShardStore) appendStagedVisibilityAlias(group *ShardGroup, readKeys [][]byte, key []byte) [][]byte { | ||
| if s == nil || s.engine == nil || group == nil { | ||
| return readKeys | ||
| } | ||
| alias, ok := s.stagedVisibilityReadKeyAlias(group, key) | ||
| if !ok { | ||
| return readKeys | ||
| } | ||
| out := append([][]byte(nil), readKeys...) | ||
| return append(out, alias) | ||
| } |
There was a problem hiding this comment.
Efficiency & Maintainability: Redundant Slice Copying and Unused Helper
The current implementation of readKeysWithStagedVisibilityAliases and readKeysWithStagedVisibilityMutationAliases reassigns and ranges over the slice being modified, and calls appendStagedVisibilityAlias which performs a full slice copy (append([][]byte(nil), readKeys...)) on every single successful alias append. This results in O(M) slice allocations and copies (where M is the number of keys/mutations).
Additionally, by refactoring these functions to append directly to a single slice, the helper function appendStagedVisibilityAlias becomes completely unused and can be removed.
Suggested Improvement
We can lazily allocate the output slice only when the first alias is found, avoiding any allocations or copies when no aliases are present.
func (s *ShardStore) readKeysWithStagedVisibilityAliases(group *ShardGroup, readKeys [][]byte) [][]byte {
if len(readKeys) == 0 {
return readKeys
}
out := readKeys
for _, key := range readKeys {
if alias, ok := s.stagedVisibilityReadKeyAlias(group, key); ok {
if out == readKeys {
out = append([][]byte(nil), readKeys...)
}
out = append(out, alias)
}
}
return out
}
func (s *ShardStore) readKeysWithStagedVisibilityMutationAliases(group *ShardGroup, readKeys [][]byte, mutations []*store.KVPairMutation) [][]byte {
out := readKeys
for _, mut := range mutations {
if mut == nil {
continue
}
if alias, ok := s.stagedVisibilityReadKeyAlias(group, mut.Key); ok {
if out == readKeys {
out = append([][]byte(nil), readKeys...)
}
out = append(out, alias)
}
}
return out
}| func stagedVisibilityScanBounds(jobID uint64, start []byte, end []byte) ([]byte, []byte) { | ||
| prefix := distribution.MigrationStagedDataKeyPrefix(jobID) | ||
| scanStart := prefix | ||
| if start != nil { | ||
| scanStart = distribution.MigrationStagedDataKey(jobID, start) | ||
| } | ||
| if err != nil { | ||
| return nil, errors.WithStack(err) | ||
| scanEnd := prefixScanEnd(prefix) | ||
| if end != nil { | ||
| scanEnd = distribution.MigrationStagedDataKey(jobID, end) | ||
| } | ||
| lockStart, lockEnd := scanLockBoundsForKVsDirection(kvs, start, end, limit, reverse) | ||
| lockKVs, err := scanTxnLockRangeAt(ctx, g, lockStart, lockEnd, ts, limit) | ||
| if err != nil { | ||
| return nil, err | ||
| return scanStart, scanEnd | ||
| } |
There was a problem hiding this comment.
Correctness: Handle Empty-but-Non-Nil Slices in Scan Bounds
The stagedVisibilityScanBounds function checks start != nil and end != nil to decide whether to append the keys to the staged prefix. However, in Go, an empty slice (e.g., []byte("")) is not nil (end != nil is true), but its length is 0.
If a caller passes an empty-but-non-nil slice as end, scanEnd will be resolved to distribution.MigrationStagedDataKey(jobID, []byte("")), which is identical to the start prefix. This results in an empty scan range [prefix, prefix) and scans zero staged keys.
Using len(start) > 0 and len(end) > 0 is much more robust and correctly handles both nil and empty slices.
| func stagedVisibilityScanBounds(jobID uint64, start []byte, end []byte) ([]byte, []byte) { | |
| prefix := distribution.MigrationStagedDataKeyPrefix(jobID) | |
| scanStart := prefix | |
| if start != nil { | |
| scanStart = distribution.MigrationStagedDataKey(jobID, start) | |
| } | |
| if err != nil { | |
| return nil, errors.WithStack(err) | |
| scanEnd := prefixScanEnd(prefix) | |
| if end != nil { | |
| scanEnd = distribution.MigrationStagedDataKey(jobID, end) | |
| } | |
| lockStart, lockEnd := scanLockBoundsForKVsDirection(kvs, start, end, limit, reverse) | |
| lockKVs, err := scanTxnLockRangeAt(ctx, g, lockStart, lockEnd, ts, limit) | |
| if err != nil { | |
| return nil, err | |
| return scanStart, scanEnd | |
| } | |
| func stagedVisibilityScanBounds(jobID uint64, start []byte, end []byte) ([]byte, []byte) { | |
| prefix := distribution.MigrationStagedDataKeyPrefix(jobID) | |
| scanStart := prefix | |
| if len(start) > 0 { | |
| scanStart = distribution.MigrationStagedDataKey(jobID, start) | |
| } | |
| scanEnd := prefixScanEnd(prefix) | |
| if len(end) > 0 { | |
| scanEnd = distribution.MigrationStagedDataKey(jobID, end) | |
| } | |
| return scanStart, scanEnd | |
| } |
There was a problem hiding this comment.
💡 Codex Review
Lines 255 to 258 in cb7b3de
Avoid key-only proxy scans losing staged visibility
For a follower key-only scan on a staged-visibility route, proxyScanGroupID sends only GroupId to the leader; the gRPC key-only fast path then calls ScanKeysAtWithReadFence with an explicit group but no route descriptor, so the receiver constructs Route{GroupID: ...} and cannot merge MigrationJobID staged rows. This makes follower SCAN/keys-only reads omit keys that exist only in staged data during cutover, while the value scan path below would materialize through ScanAtWithReadFence and return them.
ℹ️ 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 _, bypass := writeFenceBypassKeys[string(mut.Key)]; !bypass { | ||
| if err := f.verifyRouteNotFencedForKey(mut.Key); err != nil { | ||
| return err | ||
| } | ||
| if err := f.verifyRouteWriteTimestampFloorForKey(mut.Key, commitTS); err != nil { |
There was a problem hiding this comment.
When a mutation is present in WriteFenceBypassKeys (used for resolver/explicit-group routing), this block skips both the write-fence check and the new MinWriteTSExclusive check. If a route's floor advances between timestamp allocation/dispatch and Raft apply, a bypassed write with commitTS <= MinWriteTSExclusive can still land on followers, defeating the post-cutover stale-write fence; the bypass should only suppress ownership/write-fence routing checks, not the timestamp-floor validation.
Useful? React with 👍 / 👎.
| MaxBytes: chunkBytes, | ||
| MaxScannedBytes: maxScannedBytes, | ||
| KeyFamily: req.GetKeyFamily(), | ||
| AcceptKey: i.migrationExportFilter(req), |
There was a problem hiding this comment.
Legacy list delta keys share the !lst|meta|d| raw prefix with list metadata whose user key begins d|, and distribution.MigrationBracket.ContainsRoutedVersion is the only code that disambiguates by value. This RPC only wires an AcceptKey predicate, so ExportVersions route-filters MigrationFamilyLegacyListMetaDelta rows using the key-only kv.RouteKeyFilter; actual legacy deltas can be omitted from their owning route or copied under the wrong split, while the ListMeta bracket also excludes the ambiguous metadata. A split migration containing such legacy list rows will lose or misplace list metadata/deltas unless an AcceptVersion value-aware filter is passed for that family.
Useful? React with 👍 / 👎.
| if len(out) <= limit { | ||
| return out | ||
| if dedupeByKey { | ||
| out = appendReplacingKVsByKey(out, kvs) |
There was a problem hiding this comment.
For S3 bucket-meta/generation raw scans during staged visibility, non-staged raw routes are scanned first and then staged routes are merged with dedupeByKey, but this replacement only happens when the staged route emits a visible KV. If the staged/new route has a newer tombstone for an auxiliary key while the old raw-route group still has an older live row, visibleLogicalKVs emits nothing for that key, so this merge leaves the stale live KV in SCAN/reverse-scan results even though point GetAt would hide it; the merge needs to carry delete decisions, not just visible replacements.
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 (cfff9b7):
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
Lines 3987 to 3989 in cfff9b7
Send staged route metadata for key proxies
When a key-only scan for a staged-visible route is served by a follower, this proxy request carries only group_id and read_route_version; the leader receives KeysOnly without route bounds and the gRPC path calls ShardStore.ScanKeysAtWithReadFence with a bare distribution.Route{GroupID: groupID}. In that context routeHasStagedVisibility is false, so keys that exist only under !dist|migstage|... are omitted from backups/key-only pages during staged visibility. Fresh evidence after the prior “fixed in b9ac0cd” reply is that current proxyRawScanKeysAt still does not populate RouteStart, RouteEnd, or RouteBoundsPresent, unlike the value-scan proxy.
Line 1198 in cfff9b7
Filter staged control keys from key scans
When a key-only scan or BackupScanner page covers the reserved !dist|migstage| prefix before a route has staged visibility active, the underlying store can return staged shadow rows as ordinary keys, and this refill path only removes transaction-internal keys. Value scans now use isScanInternalKey to suppress MigrationStagedDataKey rows, but key-only scans still expose those physical shadow keys to callers and backups instead of keeping them internal until they are decoded through a staged-visible route.
ℹ️ 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(key) | ||
| if snap.WriteFencedForKey(rkey) { | ||
| return errors.Wrapf(ErrRouteWriteFenced, "key %q routeKey %q", key, rkey) |
There was a problem hiding this comment.
For S3 bucket metadata/generation writes, a write-fenced raw !s3|... route still trips this raw routeKey check before the decoded !s3route|<bucket> auxiliary range is considered. In the same scenario covered by the raw-floor tests—an unrelated raw S3 route is fenced/floored while the bucket's auxiliary owner is active—bucket metadata writes are rejected at Raft apply even though ownership for these auxiliary keys is decided by the auxiliary route; the coordinator's rejectWriteFencedPointKey has the same raw-first ordering, so both admission and apply need to skip unrelated raw fences for these keys.
Useful? React with 👍 / 👎.
e870fb5 to
4061c00
Compare
cfff9b7 to
30ce8c7
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 (30ce8c7):
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. |
|
Rebased onto #1085 HEAD 87d5776 and fixed both current-head P2 findings in 30ce8c7. Staged-visible key-only follower scans now carry the selected group and route bounds, and every key-only refill path filters staged control keys through the shared scan-internal predicate. Evidence:
|
|
To use Codex here, create a Codex account and connect to github. |
30ce8c7 to
1d0a97d
Compare
|
Follow-up HEAD 1d0a97d fixes the Jepsen pre-build failure from the rebase. stampTxnTimestamps now preserves the current (commitTS, error) contract while applying the migration write-floor check, and both affected test callers were updated. The merged filesystem/staged scan preparation was also extracted to keep cyclomatic complexity within the repository limit. Evidence:
|
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 (1d0a97d):
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. |
1d0a97d to
0a1b9c8
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 (0a1b9c8):
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. |
|
Rebased the latest #1088 changes onto parent #1085 HEAD f624305. Evidence:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a1b9c890c
ℹ️ 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(key) | ||
| if route, ok := c.engine.GetRoute(rkey); ok && route.MinWriteTSExclusive != 0 && commitTS <= route.MinWriteTSExclusive { | ||
| return errors.Wrapf(ErrRouteWriteTimestampTooLow, "key %q routeKey %q commit_ts=%d floor=%d", key, rkey, commitTS, route.MinWriteTSExclusive) |
There was a problem hiding this comment.
For partition-resolved keyspaces such as HT-FIFO SQS, routeKey collapses concrete partition keys to the global SQS route; the write-fence precheck above deliberately bypasses resolver-recognized keys, and the Raft request carries WriteFenceBypassKeys so the FSM floor check skips them. This new timestamp-floor precheck does not, so when that global route has MinWriteTSExclusive during migration, legitimate writes to resolver-owned partition groups with commitTS <= floor are rejected before routing. Please mirror the partition-resolver/bypass handling here and in the forwarded stamping path.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| func shouldMarkRouteGroupOnScan(start []byte, explicitGroup bool, routeStart []byte, routeEnd []byte) bool { | ||
| return !explicitGroup && !routeScanBoundsPresent(routeStart, routeEnd) && isBroadLegacyListDeltaScan(start) |
There was a problem hiding this comment.
When the scan start is exactly store.LegacyListMetaDeltaScanPrefix(userKey), isBroadLegacyListDeltaScan returns false, so this condition stops marking scanned delta KVs with their owning RouteGroupID. Redis cleanup/compaction paths build delete elems from those scan results with GroupID: pair.RouteGroupID; leaving it zero makes legacy delta keys route as raw !lst|meta|d|... metadata keys instead of the logical list key, so after a split the deletes can be sent to the wrong shard and stale deltas remain. Please keep marking exact legacy delta scans as well.
Useful? React with 👍 / 👎.
|
|
||
| startTS := r.Ts | ||
| uniq, err := uniqueMutations(muts) | ||
| uniq, err := f.uniqueMutationsAboveFloor(muts, r.GetWriteFenceBypassKeys(), startTS) |
There was a problem hiding this comment.
On the 2PC path, dispatchTxn resolves and preflights the transaction's commitTS before sending PREPARE, but the PREPARE request still has r.Ts == startTS. Passing startTS here makes a transaction that began before a migration floor but commits after it fail during PREPARE even though the later COMMIT/one-phase paths are checked against the actual commit timestamp; this aborts otherwise valid cross-shard transactions during or after cutover. Please skip the floor check for PREPARE or carry the resolved commit timestamp into this validation.
Useful? React with 👍 / 👎.
1 participant
Summary
Tests