distribution: add migration wire contracts - #1084
Conversation
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesルート移行メタデータ
読み取りフェンス付きルーティング
Redisキー正規化
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant GRPCServer
participant LeaderRoutedStore
participant ShardStore
Client->>GRPCServer: RawScanAt(read route version, route bounds)
GRPCServer->>LeaderRoutedStore: ScanAtWithReadFence
LeaderRoutedStore->>ShardStore: 境界付きローカルスキャンまたはRaw RPC
ShardStore-->>LeaderRoutedStore: フェンス済みKV結果
LeaderRoutedStore-->>GRPCServer: ScanAt結果
GRPCServer-->>Client: RawScanAt応答
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 (72fcc6b):
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 |
To use Codex here, create a Codex account and connect to github. |
There was a problem hiding this comment.
Code Review
This pull request introduces a V2 codec version for route descriptors in the distribution catalog to support new migration-related fields, including staged visibility, migration job IDs, and exclusive write timestamps. It also updates protobuf definitions and generated gRPC code to add several new RPC methods (such as split migration and range version export/import) and includes read route versions for migration read fences. The review feedback suggests defining an explicit catalogRouteCodecVersionV2 constant instead of using the dynamic catalogRouteCodecVersion directly in encoding and decoding paths, which prevents future codec version bumps from breaking V2-specific logic.
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.
bootjp
commented
Jul 13, 2026
@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 (7dd2e6d):
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:7dd2e6d48a
ℹ️ 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.
bootjp
commented
Jul 13, 2026
@codex review Addressed current-head blockers in ef2ba76:
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 (ef2ba76):
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:ef2ba76416
ℹ️ 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.
| var err error | ||
| if groupID := req.GetGroupId(); groupID != 0 { | ||
| if fenceGetter, ok := r.store.(rawReadFenceGetter); ok { | ||
| v, err = fenceGetter.GetAtWithReadFence(ctx, req.Key, readTS, req.GetGroupId(), req.GetReadRouteVersion()) |
There was a problem hiding this comment.
External RawKV callers leave read_route_version unset, but this branch passes that zero straight into the fenced store path instead of stamping the catalog version observed while routing the request. When this server is a follower and proxies the read during a split migration, the internal RawGet still carries version 0, so the source-side cutover fence either treats a fresh routed read as a broken legacy hop once zero-version rejection is active or cannot distinguish it from a stale coordinator request; stamp the current route/catalog version before invoking the fence helper.
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 (2388b3e):
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 Addressed the current-head blockers in 2388b3e:
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:2388b3e665
ℹ️ 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.
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 (781dc36):
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 Addressed the current-head fenced raw scan findings in 781dc36:
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:781dc362ec
ℹ️ 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.
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 (1afde8f):
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 14, 2026
Fixed the two RawScanAt fence findings:
Caller audit:
Validation:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:1afde8f1ba
ℹ️ 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 reverse { | ||
| kvs, err := s.local.ReverseScanAt(ctx, start, end, limit, max(ts, fenceTS)) | ||
| return kvs, errors.WithStack(err) | ||
| } | ||
| kvs, err := s.local.ScanAt(ctx, start, end, limit, max(ts, fenceTS)) |
There was a problem hiding this comment.
When this store is the verified local leader and a RawScanAt/ScanAtWithReadFence call supplies routeStart/routeEnd, these branches read the local MVCC store directly and never apply the route-key filter/pagination that the ShardStore path uses. In a leader-routed deployment receiving a supplied-bound scan over an internal raw prefix, rows from same-group sibling route intervals can be returned or consume the page limit, while the non-leader proxy path preserves the bounds.
Useful? React with 👍 / 👎.
| scanEnd := end | ||
| for len(out) < limit { | ||
| batchLimit := routeFilteredScanBatchLimit(limit - len(out)) | ||
| kvs, err := s.scanRouteAtDirectionWithReadFenceOnce(ctx, route, scanStart, scanEnd, batchLimit, ts, reverse, explicitGroup, readRouteVersion, routeStart, routeEnd) |
There was a problem hiding this comment.
For supplied-bound scans on a local ShardStore leader, this call resolves MVCC/txn locks for the raw batch before appendRouteFilteredKVs drops rows whose routeKey is outside routeStart/routeEnd. If a same-group sibling route has a pending lock under the scanned raw prefix, the bounded scan can return ErrTxnLocked or hit the lock budget even though that sibling row should be skipped by the route filter.
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 (33c793f):
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 14, 2026
Fixed the two current-head review findings. Summary:
Caller audit:
Validation:
Commit: 33c793f @codex review |
bootjp
commented
Jul 14, 2026
Addressed the latest current-head route-bounded local scan findings in 33c793f. Changes:
Validation:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:33c793f03d
ℹ️ 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".
| var groupID uint64 | ||
| if explicitGroup { | ||
| groupID = route.GroupID | ||
| } | ||
| kvs, err := s.proxyRawScanAt(ctx, g, start, end, limit, ts, reverse, groupID, readRouteVersion, routeStart, routeEnd) |
There was a problem hiding this comment.
When a supplied route-bound scan falls into this follower branch with explicitGroup == false, groupID remains zero, so the RawScanAt sent to this route's leader is re-routed there as a multi-route scan using the supplied route bounds. On nodes that host or know multiple shard groups, that leader can return sibling-group rows; the caller then scans those sibling routes again in scanRoutesAtWithReadFence, producing duplicates and applying the page limit to the wrong combined set. The proxied route-bounded request needs to stay scoped to route.GroupID instead of relying on the receiver to rediscover routes.
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 (a456f48):
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. |
Claude finished @bootjp's task in 2m 28s —— View job Review: |
routeHasLatestVersionVisibleAt could only check version presence locally, on a linearizable leader. Every other replica fell back to comparing the newest commit timestamp, which cannot tell "a tombstone at or before ts" apart from "a newer version above ts". A key with both therefore reported latest > ts, the point read fell through to the legacy wide-column route, and a snapshot read between the two timestamps resurrected the value the tombstone should have hidden. Add version_visible_at_ts to RawLatestCommitTS so the route's leader can answer the exact question, and try it before the heuristic. The reply carries version_visible_supported separately: a peer that predates the probe leaves both unset, and the caller keeps the old heuristic instead of reading silence as "no version exists". 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
Codex P2 根本原因 修正
混在バージョン対応（fail-safe）: 応答は Caller audit
Test evidence
Self-review
参考: 前回レビューで挙げていただいた「 なお本 PR は引き続き main とコンフリクト中です（ @claude review |
Claude finished @bootjp's task in 2m 43s —— View job Review: |
Four files conflicted. This branch is the heavy rewriter here (1833 lines in kv/shard_store.go against main's 48), so main's additions were ported onto this branch's structure rather than the reverse. store/list_helpers.go: both sides independently hardened the same user-key-length overflow. main's form is kept -- bytes.CutPrefix rejects a key lacking the prefix, where TrimPrefix silently passed it through to be parsed as garbage -- along with its strict ...ScanPrefix accessors. This branch's permissive ...ScanKey accessors are kept too and rebuilt on main's helper with suffixLen 0, because a resumed scan starts from a cursor inside the prefix and must tolerate a trailing tail. listScanUserKey now tries listRouteKey first, so well-formed full keys and exact prefixes get main's strict length checks, and only the cursor form falls through to the permissive path. kv/shard_store.go: main's ReadFenceGroupKeysForRange, ReadFenceRouteVersion and scanReadFenceRouteKey are added; adapter/redis_txn.go already depends on them. main's switch to listRouteKey inside routesForScan is subsumed -- this branch already routes those keys through routesForEncodedScanWithVersion. The one behavioural divergence is called out in routesForReadFence. This branch narrows a wide-column scan to the route owning the family's logical user key, which broke main's read-fence test: the fence returned one group where the test set a catalog boundary inside the family's raw keyspace and expected two. Reading may narrow, but fencing may not -- an omitted group is read unfenced, i.e. a stale read, while an extra fence only costs a fence. So the fence keeps the raw intersection for wide-column ranges and the scan path is unchanged. List keys keep the logical expansion, whose own test asserts the single owning group. Both sides' new tests are kept in kv/shard_key_test.go and store/list_helpers_test.go; no names collided. Claude-Session: https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (a89bee8):
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
Aug 22, 2026
新規指摘・CI 失敗が無かったため、マージを塞いでいた 競合の範囲と方針競合は 4 ファイル。本ブランチが 1. |
Claude finished @bootjp's task in 9m 5s —— View job Review of the |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:a89bee8555
ℹ️ 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 _, _, _, ok := redisWideColumnScanRouteRange(start, end); ok { | ||
| return s.engine.GetIntersectingRoutes(start, end), len(start) > 0 |
There was a problem hiding this comment.
In the Redis MULTI/EXEC read-fence path, when a compatibility scan can visit a legacy raw-prefix group distinct from the logical user-key group, this returns physical wide-column keys as group representatives; however, LeaseReadGroupKeys and readFenceLeader route those representatives through ShardRouter.ResolveGroup, which normalizes them back to the logical user key. Consequently, the legacy group that ScanAt may actually read is never lease-fenced or included in the split-leader check, allowing an old node's concurrent legacy-group write during a rolling upgrade to escape the transaction's read fence. This path needs an explicit group-aware representative/API rather than a key that is normalized during routing.
Useful? React with 👍 / 👎.
| if !iter.SeekGE(seekKey) { | ||
| return false, nil |
There was a problem hiding this comment.
When Pebble encounters an I/O or corruption error during SeekGE, it returns false and retains the error in iter.Error(), but this branch reports an authoritative (false, nil). This probe decides whether a logical-route miss may fall back to a legacy wide-column copy, so a storage read failure can be misclassified as absence and cause stale legacy data to be returned instead of failing closed. Check and return iter.Error() before treating a failed seek as no version.
Useful? React with 👍 / 👎.
The merge added routesForReadFence on the reasoning that a fence may over-cover but must not under-cover, so wide-column ranges fell back to a raw range intersection. That reasoning was wrong: the intersection is not a superset, it is a different set. SplitRange normalizes every boundary through kv.RouteKey before storing it, so route boundaries governing a wide-column family live in decoded user-key space, while the scan start is a raw !hs|fld|-prefixed key. Comparing the two selects the wrong route outright -- for user key "zebra" under a boundary at "m" the intersection yields the group owning "" rather than "zebra"'s owner. That wrong Route only failed to surface as a bad fence because scanReadFenceRouteKey echoes the original bytes back whenever the chosen route's Start sorts below the raw prefix, and groupForKey re-decodes them downstream. ReadFenceGroupKeysForRange already dedupes on Route.GroupID, so the wrong group id was live in the seen set. Use the scan's own expansion, which is what the fence wanted in the first place. The superset it needs is already there and is principled: redisWideColumnLegacyScanRouteRange issues the raw range as a second query, so rows still routed under the legacy raw prefix are fenced alongside the logical owner. The replaced test split at prefix+'m', a raw-prefixed boundary SplitRange cannot produce, so it asserted behaviour for an unreachable catalog state. It now pins the reachable contract: the owner resolves first, and the legacy group joins it only when the two differ. Claude-Session: https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (89798a4):
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
Aug 22, 2026
レビューありがとうございます。指摘は完全に正しく、私の解決が誤っていました。 私の誤り「fence は過剰に張ってもよいが不足してはいけない」という理屈で raw intersection にフォールバックさせましたが、raw intersection は superset ではなく「別の集合」でした。ご指摘のとおり 指摘された miscount を再現テストで確認しました。到達可能なカタログ状態（境界
修正: fence はスキャンと同じ展開を使う
そして、fence が必要としていた superset は既に存在し、しかも原理的なものでした: 実際、 テストの置き換えご指摘のとおり
旧実装（raw intersection）に戻すとこのテストは FAIL します（確認済み）。 Caller audit（戻り値の意味変更のため）
検証
@claude review |
Claude finished @bootjp's task in 5m 15s —— View job Review: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:89798a4462
ℹ️ 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 !resp.GetVersionVisibleSupported() { | ||
| return false, false, nil |
There was a problem hiding this comment.
During a rolling upgrade, a pre-probe primary-route leader ignores version_visible_at_ts and leaves version_visible_supported false, so this branch reports the result as unsupported and routeHasLatestVersionVisibleAt falls back to latest <= ts. If that logical route has a tombstone at or before the snapshot plus a newer version after it, the heuristic returns false and the point read continues to the legacy route, resurrecting the deleted value. Fresh evidence in this revision is that the new compatibility branch explicitly retains the unsafe heuristic for old peers; fail closed or disable legacy fallback when exact version presence cannot be established.
Useful? React with 👍 / 👎.
ReadFenceGroupKeysForRange named each fenced group by a representative key, leaving every consumer to re-derive the group from those bytes. That derivation is lossy for Redis wide-column ranges. The owner target and the legacy raw-prefix target normalize through routeKey to the same user key, so LeaseReadGroupKeys collapsed them into one group: the legacy group got no lease read and no leadership check, and a concurrent write against rows physically stored there escaped a MULTI/EXEC read fence. The previous commit fixed which routes the fence selects. This fixes what survives the trip to the consumer, which is the same raw-prefix vs decoded-user-key confusion one layer down. ReadFenceTarget pairs the group id with the key. Zero means "resolve from Key" and is what plain per-command keys use, so their behaviour is unchanged. LeaseReadGroupTargets dedupes on the carried id, and LeaseReadForGroup / IsLeaderForGroup / RaftLeaderForGroup let the lease read and the leader check act on a group that is already known instead of re-resolving it. The key-only wrappers on the range path had no callers left once the consumers moved over and are removed; LeaseReadGroupKeys stays for dynamodb_lease, which fences point keys where key resolution is correct. TestReadFenceTargetsSurviveGroupDedup pins both halves: the fence names both groups, and re-deriving from bytes still yields the owner twice -- that second assertion is why the id is carried rather than recomputed. TestLeaseReadGroupTargetsKeepsLegacyGroup shows the key-only path collapsing to one group where the target path keeps two. Claude-Session: https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (4cd8928):
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
Aug 22, 2026
再度のレビューありがとうございます。指摘どおり、私の前回の修正は「ルート選択」しか直しておらず、消費側は壊れたままでした。 まず再現を確認しましたご指摘の経路を実際に走らせたところ、そのとおりでした:
前コミットのメッセージで「 修正: group id を代表キーと一緒に運ぶご提案の方向に沿って
Caller audit（戻り値の意味変更のため）target 化で range 経路のキー専用ラッパは呼び出し元がゼロになったため削除しました（
Test evidence
これで「Fence legacy wide-column groups explicitly」の指摘も閉じられると考えていますが、消費側の対応漏れがまだあればご指摘ください。 @claude review |
Claude finished @bootjp's task in 3m 57s —— View job Review of commit |
There was a problem hiding this comment.
💡 Codex Review
Lines 123 to 127 in 4cd8928
Preserve legacy list auxiliary placement on upgradeWhen upgrading a sharded cluster with existing list deltas or claims, those rows were placed using the old routing behavior: delta keys resolved to a d|... pseudo-key and claim keys fell back to their raw !lst|claim|... bytes. These new decoders route both subsequent writes and per-user scans to the logical user-key group, while routesForEncodedScanWithVersion supplies no legacy route candidate. If a catalog boundary separates the old auxiliary route from the user key, resolveListMeta, TTL, and repair scans miss the pre-upgrade rows, yielding stale list metadata or stranded claims. Fresh evidence after the earlier scan-normalization fix is that placement itself now changes here without a compatibility read or migration path.
ℹ️ 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 := i.ensureRawWriteAllowed(r); err != nil { | ||
| return err |
There was a problem hiding this comment.
When ForwardRequest.is_txn is true, stampTimestamps takes the stampTxnTimestamps branch and never enters this newly gated raw loop. A transaction whose commit timestamp was chosen against a stale catalog floor—or whose floor advances before it reaches the leader—therefore goes directly to the bare transactionManager.Commit and can install versions at commitTS <= MinWriteTSExclusive, hiding acknowledged writes behind migrated versions. Fresh evidence in this revision is that the leader-side fix invokes MutationWriteGate only here for raw requests; validate transactional mutations using the resolved commit timestamp before committing them as well.
Useful? React with 👍 / 👎.
The previous commit added the group-keyed read-fence path but nothing in production reached it. The adapters do not hold a ShardedCoordinator: Redis wraps it in WithKeyVizLabel, and main wraps that in startupGatedCoordinator. Neither forwarded IsLeaderForGroup / RaftLeaderForGroup / LeaseReadForGroup, so the fence's interface assertion failed and every target fell back to key resolution -- exactly the legacy-group collapse the change set out to fix. Tests using an unwrapped coordinator could not see it, so both wrappers now have a test asserting they satisfy the interfaces. Fix a bug introduced with the target dedup: it stamped the id returned by EngineGroupIDForKey onto the emitted target. For a single-group Coordinate that id is synthetic -- it exists only so all keys collapse to one lease -- and feeding it to a group-routed leader check looked it up in a group map that never contained it, failing closed with ErrLeaderNotFound. This surfaced as NOTLEADER across the Redis MULTI collection-TTL tests. Dedup now uses the resolved id only as a token and emits the target unchanged, so a key-resolved target stays key-resolved. startupGatedCoordinator.LeaseReadForGroup falls back to the plain lease read rather than a key-resolved one when the inner coordinator has no group-keyed variant: there is no key to resolve at that point, and the single group is the one being fenced anyway. Claude-Session: https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (bd5c2c8):
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
Aug 22, 2026
前コミット（ 1. 本番の Coordinator はラップされていて、assertion が通っていなかった
どちらも ラップされていない Coordinator を使うユニットテストではこれは見えません。そこで両ラッパについて「インタフェースを満たすこと」を assert するテストを追加しました（ 2. 前コミットで自分が入れたバグ: 合成 group id をルーティングに使っていた
その合成 id を group ルーティングに渡すと、どのグループマップにも存在しないため fail-closed で dedup は解決した id をトークンとしてのみ使い、target はそのまま emit するよう修正しました。 Caller audit
検証
@claude review |
Claude finished @bootjp's task in 3m 34s —— View job Review: |
Summary
Tests
Note: full adapter-package testing exceeded the local timeout in existing raft/gRPC integration coverage; the targeted distribution adapter tests above passed.
Author: bootjp
Summary by CodeRabbit