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 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 (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. |
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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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. |
bootjp
commented
Jul 13, 2026
@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
fbd7f56CompareTLA+ 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. |
bootjp
commented
Jul 13, 2026
@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. |
bootjp
commented
Jul 13, 2026
@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. |
bootjp
commented
Jul 13, 2026
@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. |
bootjp
commented
Jul 13, 2026
@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 👍 / 👎.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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. |
bootjp
commented
Jul 13, 2026
@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".
Uh oh!
There was an error while loading. Please reload this page.
| 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 👍 / 👎.
Uh oh!
There was an error while loading. Please reload this page.
| 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 👍 / 👎.
Uh oh!
There was an error while loading. Please reload this page.
bootjp
commented
Jul 13, 2026
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 👍 / 👎.
Uh oh!
There was an error while loading. Please reload this page.
bootjp
commented
Aug 22, 2026
未解決スレッドのうち P1 / high バッジ 12 件を現 HEAD これらのスレッドは GitHub 上では すでに解消済み（9 件）
誤検出（1 件）
意図的な設計（1 件・要確認）
現 HEAD でも有効（1 件）
次の tick でこの 1 件に着手します。 @claude review |
Claude finished @bootjp's task in 3m 50s —— View job Verification of bootjp's P1/high self-review audit (HEAD |
TestShardStoreS3BucketAuxiliaryScanHonorsStagedTombstone only exercised the forward direction. The review that raised this named SCAN and reverse-scan together, so lock the reverse path down as well: a staged tombstone must hide the stale live row that the old raw route still holds, in both directions. Both already pass. filterS3AuxiliaryKVsOwnedByStagedRoutes drops the non-staged route's auxiliary rows before the dedupe merge runs, so the merge never has a stale row to replace. Claude-Session: https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
bootjp
commented
Aug 22, 2026
訂正: 前のコメントで「現 HEAD でも有効」とした 誤った理由: 実際の挙動: 非 staged ルートの行は、そもそもマージに到達する前に落とされています。 検証: 既存テスト とはいえ 1 点だけ穴がありました。ご指摘は「
更新後の P1 / high 集計（HEAD |
| 分類 | 件数 |
|---|---|
| すでに解消済み | 10 |
誤検出（store/migration_promote.go:217 lock-order） | 1 |
意図的な設計・要確認（kv/fsm.go:560 fence bypass） | 1 |
| 現 HEAD で有効 | 0 |
つまり本 PR に残っている P1 / high の作業は、kv/fsm.go:560 についてスレッドでお尋ねした 2 点の回答待ちのみです。回答次第で「実所有者ルートの floor を引く」形の絞った修正を実装します。
なお本 PR は main とコンフリクト中です。
@claude review
Claude finished @bootjp's task in 5m 1s —— View job Claude finished reviewingTodo list
Note on toolingI could not run Verifying the two open claims1.
All five are consistent. Your rebuttal is correct — the review's proposed swap would have been the one to introduce a real lock-order inversion, not fix one. 2. 3. Fresh review of the previously-unreviewed commit |
bootjp
commented
Aug 22, 2026
残る P2 バッジ 18 件の現 HEAD 今 tick で個別に確認できた分を先に共有します（未確認分は次 tick で続けます）。 すでに解消済み（8 件・コードを確認済み）
判断をお願いしたい 1 件
現 HEAD の ただし提案どおり auxiliary を優先させると、fence された raw ルートを S3 aux キーが素通りできることになり、現状より fail-open 方向になります。これは PR #1084 で 現状の「raw → auxiliary の順に両方チェックする」は fail-closed で、意図的な設計に見えます。両方適用のままで良いか、それとも S3 aux キーでは raw fence を無視すべきか、ご意見をいただけますか。 未確認（次 tick で継続）
このうち なお本 PR は引き続き main とコンフリクトしています。 @claude review |
Claude finished @bootjp's task in 3m 25s —— View job P2 verification pass — results against current HEAD ( |
| Finding | Status |
|---|---|
adapter/internal.go:517 — legacy list-delta route filter | Fixed.migrationExportFilter now special-cases MigrationFamilyLegacyListMetaDelta to use bracket.ContainsRawKey only, and wires a separate migrationExportVersionFilter → AcceptVersion: bracket.ContainsRoutedVersion(rawKey, value, ...) (adapter/internal.go:449-471). Value-aware routing for legacy deltas is in place. |
kv/fsm.go:1368 — dedup probe ignores staged S3 aux keys | Fixed.currentStagedVisibilityRouteForKey (kv/fsm.go:1354-1373) now calls currentStagedVisibilityRouteForS3BucketAuxiliaryKey first, before falling back to raw routeKey. |
kv/sharded_coordinator.go:2250 — S3 aux txn routing vs Composed-1 gate | Fixed.composed1OwnerKey (kv/fsm.go:1139-1144) now decodes S3 bucket auxiliary keys via s3BucketAuxiliaryRouteRange and uses the decoded bucket-route start as the owner key, matching the coordinator's resolveGroupAndRouteForKey/stagedVisibilityRouteForS3BucketAuxiliaryKey resolution instead of the raw routeKey. |
kv/shard_store.go:2704/:2720 — route-filtered leader scan + S3 decode ordering | Fixed, both halves.scanRouteAtLeaderRouteFilter now branches on routeHasStagedVisibility(route) into scanRouteWithStagedVisibility, matching the non-filtered leader path. And routeKeyInScanBounds (kv/shard_store.go:2014-2026) checks s3BucketAuxiliaryRouteInRange (decoded bucket range) before falling back to raw-key comparison, so staged S3 aux rows survive route filtering. |
kv/shard_store.go:4299 — staged S3 bucket metadata in scans | Fixed.routesForScanWithVersion/routesForFencedScanWithVersion explicitly call routesForS3BucketAuxiliaryScan for raw scans over BucketMetaPrefix/BucketGenerationPrefix, appending staged-visibility routes that intersect the decoded bucket route range. |
Still present / partially fixed (4 items)
kv/shard_store.go:999 (now ~989-999) — route group marking on exact legacy-delta scan — still present.
funcisBroadLegacyListDeltaScan(start []byte) bool {
...returnlogicalUserKey==nil||!bytes.Equal(start, store.LegacyListMetaDeltaScanPrefix(logicalUserKey))
}
funcshouldMarkRouteGroupOnScan(start []byte, explicitGroupbool, routeStart []byte, routeEnd []byte) bool {
return!explicitGroup&&!routeScanBoundsPresent(routeStart, routeEnd) &&isBroadLegacyListDeltaScan(start)
}When start is exactly LegacyListMetaDeltaScanPrefix(userKey), isBroadLegacyListDeltaScan returns false, so RouteGroupID marking is skipped for that scan, unchanged from the original finding.
kv/shard_store.go:2289 — staged route metadata on proxied scans — partially fixed.
Forward value scans are fixed: ScanGroupAt → scanExplicitGroupAtWithReadFence → routesForExplicitGroupScanWithRouteBounds resolves the real route (including staged visibility) via engine.GetIntersectingRoutes, falling back to a bare distribution.Route{GroupID} only when no route matches, and failing closed (ErrExplicitGroupStagedVisibilityUnresolved) on a staged mismatch.
But ReverseScanGroupAt (kv/shard_store.go:617-625) and ScanGroupKeysAt (kv/shard_store.go:629-634) still construct a bare distribution.Route{GroupID: groupID} directly and skip route resolution entirely:
func (s*ShardStore) ReverseScanGroupAt(...) (...) {
returns.scanRouteAtDirectionWithReadFence(ctx, distribution.Route{GroupID: groupID}, ...)
}
func (s*ShardStore) ScanGroupKeysAt(...) (...) {
returns.scanKeyRouteAt(ctx, distribution.Route{GroupID: groupID}, ...)
}These are real, reachable entry points — adapter/grpc.go:293/303/390 calls them directly for explicit-group RawScanAt requests with reverse=true or keys_only=true. So a follower-proxied reverse or keys-only scan on a staged-visibility route can still silently miss staged rows, while the forward value-scan path no longer has this gap. Recommend applying the same routesForExplicitGroupScanWithRouteBounds resolution to these two entry points.
kv/fsm.go:1207 — PREPARE floor check validates against startTS, not the eventual commit timestamp — present, likely intentional, worth a decision.handlePrepareRequest still calls f.uniqueMutationsAboveFloor(muts, ..., startTS) (kv/fsm.go:1206-1207), i.e. PREPARE's write-floor check runs against r.Ts (the txn start timestamp), while handleOnePhaseTxnRequest (kv/fsm.go:1277) correctly checks against commitTS. TestFSMRejectsPrepareAtMigrationTimestampFloorDuringApply only covers the case where startTS itself is below the floor — it doesn't cover a txn that starts before a floor is installed but commits after.
This may well be intentional: 2PC PREWRITE actually persists a lock/provisional MVCC version at startTS, so if that version must never land below the floor (to avoid provisional state polluting a migrated range), rejecting at PREPARE time is the safer, fail-closed choice — the client simply retries with a fresh startTS. But if that's not the intended contract, a cross-shard transaction that merely straddles a cutover would abort at PREPARE even though its COMMIT would have been valid. @bootjp — is prewrite-below-floor supposed to be disallowed outright (current behavior, keep as-is), or should PREPARE defer the floor check to the resolved commit timestamp the way COMMIT/one-phase do? I'd lean toward leaving it as a fail-closed abort-and-retry unless you have evidence it's actually breaking real cross-shard migration traffic.
kv/sharded_coordinator.go:1310 (now ~1298-1313) — resolver-owned keys not skipped in the floor precheck — confirmed still present.
func (c*ShardedCoordinator) rejectWriteFencedPointKey(key []byte) error {
ifc.partitionResolverRecognisesPointKey(key) {
returnnil// <-- fence check skips resolver-owned keys
}
rkey:=routeKey(key)
ifroute, ok:=c.engine.GetRoute(rkey); ok&&route.State== distribution.RouteStateWriteFenced {
...
}
...
}
func (c*ShardedCoordinator) rejectWriteTimestampFloorPointKey(key []byte, commitTSuint64) error {
start, end, ok:=s3BucketAuxiliaryRouteRange(key)
ifok { ... }
rkey:=routeKey(key) // <-- no partitionResolverRecognisesPointKey guardifroute, ok :=c.engine.GetRoute(rkey); ok&& route.MinWriteTSExclusive!=0&&commitTS<= route.MinWriteTSExclusive {
return errors.Wrapf(ErrRouteWriteTimestampTooLow, ...)
}
returnnil
}The write-fence precheck (rejectWriteFencedPointKey) explicitly bails out for resolver-recognized keys (e.g. SQS HT-FIFO partition keys), but the parallel timestamp-floor precheck does not. rejectWriteTimestampFloorPointKey is reached both from dispatchTxn (line ~1405, via rejectWriteTimestampFloorPointElems — called for all elems regardless of GroupID) and from the forwarded-mutation path (rejectWriteTimestampFloorMutations, line ~2524-2541). In both cases, a resolver-owned partition key collapses via routeKey() to the shared byte-range route (e.g. !sqs|route|global); if that route has MinWriteTSExclusive set from an unrelated migration, a legitimate write to a resolver-owned partition on an unaffected group gets rejected with ErrRouteWriteTimestampTooLow. This matches the codex finding exactly and should be fixed by adding the same partitionResolverRecognisesPointKey guard to rejectWriteTimestampFloorPointKey.
Summary
Of the 11 P2 items covered this pass (10 explicitly listed + the fsm.go:607 design question): 5 fixed, 1 design decision to confirm with you (PREPARE floor uses startTS), 1 partially fixed (forward scans fixed, reverse/keys-only explicit-group scans still bypass route resolution), and 2 confirmed still present (exact legacy-delta scan route marking; resolver-owned-key skip missing from the floor precheck).
The two "still present" items (shard_store.go legacy-delta marking, sharded_coordinator.go resolver-owned floor skip) and the reverse/keys-only scan gap look like real, actionable gaps rather than judgment calls — happy to push fixes+regression tests for those if you'd like, per the repo's "test-first for review-identified defects" convention.
PR still conflicts with main.
rejectWriteFencedPointKey skips keys the partition resolver recognises, because in a partition-resolved keyspace such as HT-FIFO SQS routeKey collapses a concrete partition key onto the global SQS route, so that route's state does not describe the key. rejectWriteTimestampFloorPointKey had no such exemption, so the same collapsed route's MinWriteTSExclusive was applied to those keys and rejected writes the fence precheck deliberately lets through. Apply the same exemption, using the existing partitionResolverRecognisesPointKey helper so both prechecks agree on what the resolver owns. Claude-Session: https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
bootjp
commented
Aug 22, 2026
P2 検証の続きです。1 件を修正し（ 修正: |
Claude encountered an error after 9s —— View job I'll analyze this and get back to you. |
Route-group marking was gated on isBroadLegacyListDeltaScan, which is
false when the scan start is exactly LegacyListMetaDeltaScanPrefix(userKey).
Redis cleanup and compaction build their deletes as {Del, pair.Key,
GroupID: pair.RouteGroupID}, so a zero GroupID there routes the delete by
the raw "!lst|meta|d|..." key rather than the logical list key -- after a
split the delete goes to the wrong shard and the stale delta survives.
Gate marking on isLegacyListDeltaScan, which covers both the broad and
the per-user-key shape, at all three marking sites. Route *selection*
still distinguishes them: routesForInternalScanWithVersion deliberately
narrows an exact scan to the logical key's routes, so that call keeps
isBroadLegacyListDeltaScan.
Claude-Session: https://claude.ai/code/session_013rNHooj7NF3giihWVba8QEbootjp
commented
Aug 22, 2026
P2 検証を完了しました。1 件を追加修正し（ 修正: |
| 指摘 | HEAD での状態 |
|---|---|
adapter/internal.go:517Preserve raw-route matches for S3 bucket exports | decodedS3BucketRouteFilter に allowRawRouteMatch := !s3BucketRouteBounds(...) があり、route bound が raw `!s3 |
kv/fsm.go:1368Probe staged S3 auxiliary keys during dedup | currentStagedVisibilityRouteForS3BucketAuxiliaryKey(snap, key, f.shardGroupID) を snap.RouteOf(routeKey(key)) の前に試すようになっています |
kv/shard_store.go:2581O(N) Database Queries (gemini high) | 既報のとおり ExportVersions レンジスキャン 1 回に集約済み |
最終集計（P2 バッジ 18 件 / HEAD 86c48e62）
| 分類 | 件数 |
|---|---|
| すでに解消済み | 11 |
| 今回修正 | 2（:1310 resolver 除外、:999 route group マーキング） |
| 設計判断待ち | 3（kv/fsm.go:607、kv/fsm.go:1207、kv/sharded_coordinator.go:2250） |
| 未確認 | 2（kv/shard_store.go:2289、:2704 / :2720 / :4299 のうち staged scan 系） |
未確認として残っているのは staged visibility のスキャン経路（:2289Preserve staged route metadata on proxied scans、:2704Use staged visibility for route-filtered leader scans、:2720Decode S3 auxiliary keys before route filtering、:4299Include staged S3 bucket metadata in scans）で、いずれも相互に絡むため次 tick でまとめて確認します。
設計判断待ちの 3 件はいずれも「バイト範囲ルートが所有者でないキーの所有者をどう定義するか」という同じ論点に帰着します。kv/fsm.go:560 のスレッドでお尋ねしている 2 点にご回答いただければ、3 件まとめて対応できます。
@claude review
Claude finished @bootjp's task in 1m 40s —— View job Review of P2 status update (HEAD |
bootjp
commented
Aug 22, 2026
残していた staged visibility スキャン 4 件の確認が終わり、P2 バッジ 18 件の検証が完了しました。 追加で解消済みを確認（3 件）
現 HEAD でも有効（1 件）
ご指摘のとおり現存します。 点読み側には既に 実装方針としては 2 案あります:
1 は受信側で完結しますが「範囲が複数ルートに跨る場合にどのルートを採るか」を決める必要があり、2 はワイヤ変更を伴います。どちらが設計意図に沿うかご意見をいただけますか。点読み側の fail-closed と挙動を揃えたいので、独断では進めません。 最終集計（P2 バッジ 18 件 / HEAD |
| 分類 | 件数 |
|---|---|
| すでに解消済み | 14 |
| 今回の tick で修正 | 2（:1310 resolver 除外、:999 route group マーキング） |
| 設計判断待ち | 3（kv/fsm.go:607、kv/fsm.go:1207、kv/sharded_coordinator.go:2250） |
| 現存・方針確認待ち | 1（kv/shard_store.go:2289） |
P1/high 12 件とあわせると、本 PR の指摘 30 件のうち 24 件が解消済み、2 件が誤検出/意図的設計、4 件が方針確認待ちという状況です。
@claude review
Claude finished @bootjp's task in 4m 7s —— View job Review complete
Verified against HEAD Correction to the "still open" scope
The real remaining gap is narrower than "all three":
Both are gRPC-reachable ( Recommendation: option 1 (resolve on receiver), not option 2 (wire fields).
Concrete shape:
The 3 "design judgment pending" items
Spot-check of "already resolved" claimsChecked the two commits from this tick plus one from the "already resolved" batch — all hold up:
SummaryOf your tracked items: 1 correction (scope of the proxied-scan gap is |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:86c48e6283
ℹ️ 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 err != nil { | ||
| return errors.WithStack(err) |
There was a problem hiding this comment.
When ImportVersionsRaft fails on only one target voter—for example because that replica encounters a Pebble I/O or encryption error—returning an ordinary error here lets internal/raftengine/etcd.applyNormalCommitted advance that replica's applied index because only responses implementing HaltApply stop setApplied. The leader can apply successfully and acknowledge the RPC while the failed voter permanently skips the imported versions, causing replica divergence and missing data after failover or promotion. Fatal import-store errors should use the same halt-apply path as migration promotion, while only explicitly classified request/idempotency errors remain ordinary.
Useful? React with 👍 / 👎.
applyMigrationImport returned every failure as an ordinary error. The engine only stops advancing setApplied for responses implementing HaltApply, so a store-side failure on a single target voter -- a Pebble I/O error, the encryption gate rejecting the write -- let that replica skip the imported versions while the leader applied and acked the RPC. The divergence surfaces later as missing data after failover or promotion. Classify the way applyMigrationPromote already does. Errors that are a verdict on the request bytes stay ordinary, because every replica applying the same entry reaches them identically and the group advances in step: batch gap, malformed version, oversized value. Everything else halts, including a failed HLC-floor read. Halting on the request-shaped errors instead would turn one malformed batch into a cluster-wide outage. Replay after a halt is safe: the import batch either did not commit, or committed and is recognised as a duplicate by validateNextImportBatch. validateImportVersion's anonymous errors become ErrInvalidImportVersion so the classifier can name them. Promotion also calls that validator but classifies only ErrInvalidExportCursor as ordinary, so its behaviour is unchanged. Claude-Session: https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE
bootjp
commented
Aug 22, 2026
codex の新しい P1 ( 検証: 指摘のメカニズムを確認
つまり 1 台の target voter だけが Pebble I/O / 暗号化ゲート起因で 修正: |
Claude finished @bootjp's task in 1m 44s —— View job Review complete ✅
Verdict: fix is correctThe claim in the trigger comment holds up. I traced the mechanism independently:
The fix (
I confirmed:
New tests ( No further issues found in this change. This is a solid, narrowly-scoped fix for a genuine correctness bug (data-loss category per the repo's self-review conventions), following the established |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:da3b967170
ℹ️ 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 err := f.verifyRouteWriteTimestampFloorForPrefix(prefix, commitTS); err != nil { | ||
| return err | ||
| } | ||
| if err := f.store.DeletePrefixAtRaftAt(ctx, prefix, txnCommonPrefix, commitTS, f.pendingApplyIdx); err != nil { |
There was a problem hiding this comment.
When a target route has staged visibility and a key exists only under !dist|migstage|<job>|..., a post-cutover DEL_PREFIX reaches this production FSM with the per-group Pebble store and tombstones only the raw prefix. The staged-only row therefore remains the winning visible version, so operations such as prefix cleanup or flush report success while the deleted key is still readable. The staged-prefix propagation implemented on ShardStore.DeletePrefixAtRaftAt does not help here because production constructs each FSM directly with st in main.go; this apply path must tombstone the corresponding staged logical range as part of the same Raft apply.
Useful? React with 👍 / 👎.
| MaxCommitTSInclusive: req.GetMaxCommitTs(), | ||
| Cursor: req.GetCursor(), | ||
| MaxVersions: defaultMigrationExportMaxVersions, | ||
| MaxBytes: chunkBytes, |
There was a problem hiding this comment.
When the caller supplies a large chunk_bytes, or a single exported version exceeds the requested chunk size, this passes the value through without enforcing the repository's 64 MiB gRPC/Raft command ceiling. ExportVersions appends a version before testing MaxBytes, while the store permits values up to 256 MiB, so an export can produce a response that gRPC cannot send or a batch that MarshalMigrationImportCommand can never propose. Such a row makes the migration permanently unable to advance unless export sizing rejects it or leaves enough envelope headroom for both wire formats.
Useful? React with 👍 / 👎.
| } | ||
| uniq, err := uniqueMutations(muts) | ||
| uniq, err := f.uniqueMutationsAboveFloor(muts, r.GetWriteFenceBypassKeys(), commitTS) |
There was a problem hiding this comment.
When a one-phase transaction carries a caller-supplied startTS or read set, it can legitimately have ObservedRouteVersion == 0; after cutover it is routed to the target and allowed when its newly allocated commitTS is above the migration floor. This path then checks conflicts only through the target's raw MVCC keys, so if the logical key exists only under the staged prefix with a commit timestamp newer than startTS, the transaction misses that conflict and can commit a decision based on a stale pre-cutover read. The migration-aware transaction path must include the matching staged key in write/read-set OCC validation while staged visibility is active.
Useful? React with 👍 / 👎.
| if exportRangeVersionsRequestFullyUnbounded(req) { | ||
| return errors.WithStack(status.Error(codes.InvalidArgument, "migration export requires a raw or route bound")) |
There was a problem hiding this comment.
When an export supplies raw family bounds but leaves route_start and route_end empty, this validator accepts it because the request is not fully unbounded. Internal-family brackets deliberately scan a family-wide raw prefix, and the resulting empty RouteKeyFilter accepts every routed row in that prefix, so a migration can acknowledge and promote data belonging to unrelated routes that happen to share the source group. Reject exports without the moving route bounds instead of treating raw bounds alone as sufficient.
Useful? React with 👍 / 👎.
| if landed { | ||
| return true, nil | ||
| } | ||
| route, ok := f.currentStagedVisibilityRouteForKey(meta.PrimaryKey) |
There was a problem hiding this comment.
This adds a staged probe only for one-phase deduplication, while commitApplyStartTS, appendRollbackRecord, and the 2PC lock/intent helpers still inspect live keys only. During staged visibility, a response-lost COMMIT whose imported commit record exists only under the job prefix is treated as uncommitted, and an imported rollback marker is also missed; a later COMMIT can therefore create a live commit record for an already-aborted transaction, while an ABORT can create a rollback marker for an already-committed one. Reuse the staged route lookup for the 2PC commit/rollback identity checks before producing a new outcome.
Useful? React with 👍 / 👎.
While a route has staged visibility a logical key can exist only under !dist|migstage|<job>|..., and handleDelPrefix tombstoned the raw prefix alone. The staged row stayed the winning visible version, so prefix cleanup and flush reported success with the deleted key still readable. ShardStore.DeletePrefixAtRaftAt already propagates to staged prefixes, but that is not the production path: main.go builds each FSM directly over the per-group store via NewKvFSMWithHLC(st, ...), never over ShardStore, so the apply path has to propagate itself. Ordering is load-bearing. The raw delete bundles pendingApplyIdx into its Pebble batch, so it has to be the last write and the staged deletes pass appliedIndex 0. A crash between them then replays the whole entry, and the staged tombstones are idempotent at the same commitTS. Doing the raw delete first would advance the applied index and let cold-start replay skip the staged half for good -- the same class of bug this is fixing. Staged deletes are scoped to this FSM's own group and its route's migration job, so another job's staged rows under the same logical prefix are untouched; a test pins that. Claude-Session: https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE
bootjp
commented
Aug 23, 2026
HEAD ( P1-a |
Claude finished @bootjp's task in 5m 7s —— View job Review of the 4 remaining findings (HEAD |
| # | Finding | Verdict |
|---|---|---|
| 1 | One-phase OCC misses staged keys | Already covered via ReadKeys aliasing + checkReadConflicts; add an end-to-end test for confidence but not a live bug |
| 2 | 2PC commit/rollback identity check misses staged keys | Real, unaddressed |
| 3 | Export chunk size can exceed transport/Raft limits | Real, unaddressed |
| 4 | Migration export should require route bounds | Real, unaddressed |
I did not implement fixes since this was a review request — @bootjp said they'll continue with the P1 items next. Happy to implement any of these on request.
(branch design/hotspot-split-m2-cross-group)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:8668bdcec2
ℹ️ 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 f == nil || f.routes == nil { | ||
| return nil | ||
| } | ||
| snap, ok := f.routes.Current() |
There was a problem hiding this comment.
When a target follower applies this prefix-delete entry before its asynchronous catalog watcher has installed the CUTOVER descriptor, Current() can lack staged visibility even though the leader's snapshot has it. That follower therefore skips the staged tombstone but still persists the raw delete and applied index; after its watcher catches up or it becomes leader, staged-only rows remain readable on that replica. Encode the migration job/ranges in the proposed command or otherwise use durable, apply-consistent routing metadata rather than replica-local catalog state.
Useful? React with 👍 / 👎.
| if _, _, ok := distribution.MigrationStagedDataKeyParts(key); ok { | ||
| return distribution.Route{}, false | ||
| } | ||
| snap, ok := f.routes.Current() |
There was a problem hiding this comment.
During a response-lost one-phase retry, a leader whose watcher has applied staged visibility can find PrevCommitTS under the staged prefix and no-op, while a follower whose watcher still has the pre-CUTOVER snapshot falls through and applies the retry as a new live version. With a fresh startTS above the prior commit, the staged read alias does not conflict, so the replicas permanently diverge despite applying the same Raft entry. The staged job identity used by this decision must come from deterministic command or durable FSM state, not each replica's Current() catalog snapshot.
Useful? React with 👍 / 👎.
| if start, end, auxiliary := s3BucketAuxiliaryRouteRange(key); auxiliary { | ||
| routes, version := s.engine.GetIntersectingRoutesWithVersion(nil, nil) | ||
| for _, route := range routes { | ||
| if routeHasStagedVisibility(route) && migrationRouteRangesIntersect(route.Start, route.End, start, end) { |
There was a problem hiding this comment.
Once promotion clears StagedVisibilityActive, this branch stops associating bucket metadata and generation keys with the migrated !s3route|... owner and falls back to routing their raw !s3|bucket|... keys. The promoted versions remain physically on the target group, so subsequent S3 reads, scans, and writes can be sent back to the unrelated raw-key owner and observe stale or missing bucket state; the coordinator and ShardRouter repeat the same staged-only predicate. Preserve a permanent decoded-bucket ownership rule after the staged overlay is cleared.
Useful? React with 👍 / 👎.
| StartKey: req.GetRangeStart(), | ||
| EndKey: req.GetRangeEnd(), |
There was a problem hiding this comment.
For a route in the filesystem chunk domain, the logical catalog interval is !fs|route|chk|..., while the actual payload rows are stored under !fs|chk|... and only normalize to that route through fskeys.ExtractRouteKey. The migration plan has no filesystem chunk family bracket, so the user bracket supplies the virtual route interval here as the raw StartKey/EndKey; the export reaches none of the chunk payloads, yet the bracket can complete and be promoted. A cross-group split between file boundaries therefore loses every chunk in the moved files unless the plan scans the raw chunk prefix and applies the logical route filter.
Useful? React with 👍 / 👎.
This reverts 8668bdc. The fix decided whether to write a staged tombstone from f.routes.Current(), which is WrapDistributionEngine over the route engine that CatalogWatcher.Run refreshes on a polling timer -- replica-local state that is not synchronized with Raft apply. A follower applying the prefix-delete entry before its watcher installs the CUTOVER descriptor therefore skips the staged tombstone while still persisting the raw delete and the applied index. Its replay can never revisit the entry, so staged-only rows stay readable on that replica alone. That is replica divergence, which is worse than the uniform stale-row bug it was meant to fix: before, every replica agreed. The underlying bug is real and stays open. Fixing it needs routing that is durable and apply-consistent -- the migration job and ranges carried in the proposed command, rather than read from replica-local catalog state at apply time. That changes the Raft command format, so per CLAUDE.md it wants a design doc before implementation. Reverting the test too: TestDelPrefix_LeavesOtherJobsStagedRowsIntact asserted that another job's staged copy of a deleted logical key survives, which was my scoping assumption rather than a verified requirement, and a promotion could resurrect deleted data under it. Claude-Session: https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE
bootjp
commented
Aug 23, 2026
検証指摘のとおりです。裏を取りました:
つまり 結果として起きること（ご指摘のとおり）: watcher が CUTOVER descriptor を入れる前にこの entry を適用した follower は、staged tombstone をスキップしたまま raw delete と applied index を永続化します。applied index が進むので replay で戻ることもできず、その replica だけ staged-only 行が読めたままになります。 これは元のバグより悪い状態です。 修正前は「全 replica が一様に stale な行を見せる」でしたが、修正後は「replica 間で状態が食い違う」になります。したがって部分修正やガード追加ではなく、revert が正しい対応と判断しました。 元のバグは未解決のまま残ります
テストも revert しました（重要）
現 HEAD（ |
Claude encountered an error after 9s —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:b7aa882673
ℹ️ 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 f.routes == nil || commitTS == 0 { | ||
| return nil | ||
| } | ||
| snap, ok := f.routes.Current() |
There was a problem hiding this comment.
When a write is proposed before a route floor is published but applied concurrently with the catalog update, this Current() lookup can see the new floor on some voters and the old snapshot on others because each watcher refreshes its local cache independently. Voters that see the floor return ErrRouteWriteTimestampTooLow and advance their applied index without writing, while lagging voters persist the mutation, producing permanent replica divergence. Carry the relevant floor/version in the Raft command or consult durable Raft-applied target state instead of the local route cache. CLAUDE.mdL67-L67
Useful? React with 👍 / 👎.
| return nil, err | ||
| } | ||
| stagedKey := distribution.MigrationStagedDataKey(route.MigrationJobID, key) | ||
| staged, stagedOK, err := latestMVCCVersionAt(ctx, g.Store, stagedKey, ts) |
There was a problem hiding this comment.
When a promotion Raft apply commits between the live lookup above and this staged lookup, the promotion batch can atomically copy the staged version to the live key and delete its alias after the first lookup has already observed the pre-promotion state. The second lookup then observes no staged version, so GetAt returns not-found—or an older live value—even though the winning version existed both before and after promotion. Serialize these lookups with promotion or read both aliases from one Pebble snapshot; the scan and latest-version merge paths need the same single-view guarantee. CLAUDE.mdL91-L91
Useful? React with 👍 / 👎.
| return s.scanRouteAtDirectionWithReadFence( | ||
| ctx, distribution.Route{GroupID: groupID}, start, end, limit, ts, true, true, | ||
| 0, nil, nil, |
There was a problem hiding this comment.
When RawScanAt supplies a group_id with reverse=true during staged visibility, this passes a synthetic route containing only the group ID, so routeHasStagedVisibility is false and the leader performs a live-only reverse scan. Staged-only keys are omitted and older raw values can be returned, whereas the forward ScanGroupAt path resolves the group's real staged routes first. Route reverse explicit-group scans through the same staged-aware route selection.
Useful? React with 👍 / 👎.
Summary
Tests