kv(composed1): M1 plumbing — ObservedRouteVersion in OperationGroup → pb.Request#881
Conversation
… pb.Request
First milestone of the Composed-1 cross-group commit-time guard per
docs/design/2026_05_29_proposed_composed1_cross_group_commit_guard.md
§M1. Plumbs a new ObservedRouteVersion field end-to-end so the M3
FSM apply-time gate (a separate PR) can read it without further
plumbing work.
Behaviour-neutral:
* Every existing caller leaves ObservedRouteVersion at zero (its
zero value).
* The FSM at this milestone IGNORES pb.Request.ObservedRouteVersion;
M3 will add the apply-time check.
* No new errors, no new sentinels, no new control-flow branches on
the hot path.
Changes:
* proto/internal.proto — adds `uint64 observed_route_version = 6`
on pb.Request with an inline comment pointing at the design doc.
* proto/internal.pb.go — regen via protoc 29.6 + protoc-gen-go
v1.36.11.
* kv/transcoder.go — adds ObservedRouteVersion to OperationGroup
with a doc comment that explicitly calls out the zero default and
the M3 follow-up.
* kv/coordinator.go — onePhaseTxnRequest signature gains the
observedRouteVersion uint64 param and sets it on the returned
pb.Request. Coordinate.dispatchTxn signature gains the param
and forwards it. Coordinate.Dispatch already routes through
dispatchTxn so the OperationGroup field flows in.
Coordinate.buildRedirectRequests (the follower→leader forward
path's txn branch) reads reqs.ObservedRouteVersion and passes it
so the field survives a redirected commit too.
* kv/sharded_coordinator.go — ShardedCoordinator.dispatchTxn and
its dispatchSingleShardTxn helper gain the same param,
propagated from the calling OperationGroup at the dispatchTxn
entry point.
Tests (kv/coordinator_txn_test.go):
* TestCoordinateDispatchTxn_PassesObservedRouteVersionToRaftEntry —
the round-trip witness: dispatchTxn(..., observedRouteVersion=42)
produces pb.Request.ObservedRouteVersion == 42. This is the M1
"done when" criterion from the design doc.
* TestCoordinateDispatchTxn_UnpinnedObservedRouteVersionStaysZero —
legacy-default witness: dispatchTxn(..., observedRouteVersion=0)
produces pb.Request.ObservedRouteVersion == 0, matching every
existing caller.
Existing dispatch tests updated for the new signature (one extra
trailing `0` per call site).
Verification:
* go build ./... — clean
* go test -race -count=1 ./kv — 10.0 s, pass
* go test -race -count=1 -run ObservedRouteVersion ./kv — 1.0 s, pass
Self-review (5 lenses):
1. Data loss — no new write paths; the new field is plumbed but
not yet read by the FSM. Cannot lose a write because there is
no new branch that could refuse one.
2. Concurrency — additive; no new locks, no new goroutines.
3. Performance — one extra uint64 field on every pb.Request; one
extra uint64 param on the dispatch path. No new atomics, no
new allocations.
4. Data consistency — the field flows from OperationGroup →
dispatchTxn → onePhaseTxnRequest → pb.Request; M3 will close
the loop at FSM apply. At this milestone the consistency
surface is unchanged.
5. Test coverage — round-trip + legacy-default tests cover the M1
contract. Existing dispatch tests adapted to the new
signature; full ./kv suite passes under -race.
Next milestone (separate PR per design doc §6): M2 — distribution
catalog version retention ring + kvFSM struct extension
(routes / shardGroupID), unlocking the M3 apply-time gate.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@claude review |
📝 WalkthroughWalkthroughThis PR threads ChangesObservedRouteVersion propagation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 (3cbe1c3):
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. |
|
Claude finished @bootjp's task in 3m 2s —— View job Code Review — PR #881Todo
OverallThe M1 plumbing is clean and the single-shard path is correctly wired end-to-end. One medium-severity gap, one low-severity toolchain concern, and a couple of nits follow. [Medium] Multi-shard 2PC path silently drops
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces plumbing for ObservedRouteVersion (part of the Composed-1 cross-group commit guard design) to propagate the durable catalog version from the transaction's read set to the FSM via pb.Request. This is implemented for single-shard transactions in both the standard and sharded coordinators, with corresponding unit tests added. However, a critical correctness issue was identified where ObservedRouteVersion is not propagated in the multi-shard (2PC) transaction path in ShardedCoordinator, which would cause multi-shard transactions to bypass the safety checks.
| // so that validateReadOnlyShards can issue a linearizable read barrier, | ||
| // preserving SSI. | ||
| return c.dispatchSingleShardTxn(ctx, startTS, commitTS, primaryKey, gids[0], elems, readKeys) | ||
| return c.dispatchSingleShardTxn(ctx, startTS, commitTS, primaryKey, gids[0], elems, readKeys, observedRouteVersion) |
There was a problem hiding this comment.
Critical Correctness Issue: Missing ObservedRouteVersion Propagation in Multi-Shard (2PC) Path
While the single-shard fast path (dispatchSingleShardTxn) correctly propagates observedRouteVersion, the multi-shard 2PC path immediately below it completely omits this propagation.
Multi-shard transactions are the most critical path for cross-group consistency and are highly susceptible to route shifts (e.g., during range splits or moves). If ObservedRouteVersion is not propagated to the PREPARE and COMMIT requests, the FSM will see a default value of 0 (unpinned) and skip the Composed-1 safety checks entirely for all multi-shard transactions.
To fix this, please propagate observedRouteVersion through the 2PC path:
- Update
dispatchTxncalls (lines 601, 606, 620):
prepared, err := c.prewriteTxn(ctx, startTS, commitTS, primaryKey, grouped, gids, groupedReadKeys, observedRouteVersion)
...
primaryGid, maxIndex, err := c.commitPrimaryTxn(ctx, startTS, primaryKey, grouped, commitTS, observedRouteVersion)
...
maxIndex = c.commitSecondaryTxns(ctx, startTS, primaryGid, primaryKey, grouped, gids, commitTS, maxIndex, observedRouteVersion)- Update
prewriteTxnsignature and request construction (lines 678, 687):
func (c *ShardedCoordinator) prewriteTxn(ctx context.Context, startTS, commitTS uint64, primaryKey []byte, grouped map[uint64][]*pb.Mutation, gids []uint64, groupedReadKeys map[uint64][][]byte, observedRouteVersion uint64) ([]preparedGroup, error) {
...
req := &pb.Request{
IsTxn: true,
Phase: pb.Phase_PREPARE,
Ts: startTS,
Mutations: append([]*pb.Mutation{prepareMeta}, grouped[gid]...),
ReadKeys: groupedReadKeys[gid],
ObservedRouteVersion: observedRouteVersion,
}
...
}- Update
commitPrimaryTxnsignature and request construction (lines 726, 739):
func (c *ShardedCoordinator) commitPrimaryTxn(ctx context.Context, startTS uint64, primaryKey []byte, grouped map[uint64][]*pb.Mutation, commitTS uint64, observedRouteVersion uint64) (uint64, uint64, error) {
...
req := &pb.Request{
IsTxn: true,
Phase: pb.Phase_COMMIT,
Ts: startTS,
Mutations: append([]*pb.Mutation{meta}, keys...),
ObservedRouteVersion: observedRouteVersion,
}
...
}- Update
commitSecondaryTxnssignature and request construction (lines 756, 770):
func (c *ShardedCoordinator) commitSecondaryTxns(ctx context.Context, startTS uint64, primaryGid uint64, primaryKey []byte, grouped map[uint64][]*pb.Mutation, gids []uint64, commitTS uint64, maxIndex uint64, observedRouteVersion uint64) uint64 {
...
req := &pb.Request{
IsTxn: true,
Phase: pb.Phase_COMMIT,
Ts: startTS,
Mutations: append([]*pb.Mutation{meta}, keyMutations(grouped[gid])...),
ObservedRouteVersion: observedRouteVersion,
}
...
}- Update
txnLogsandbuildTxnLogs(lines 1269, 1352) for completeness:
func (c *ShardedCoordinator) txnLogs(reqs *OperationGroup[OP]) ([]*pb.Request, error) {
...
return buildTxnLogs(reqs.StartTS, commitTS, grouped, gids, reqs.ObservedRouteVersion)
}
func buildTxnLogs(startTS uint64, commitTS uint64, grouped map[uint64][]*pb.Mutation, gids []uint64, observedRouteVersion uint64) ([]*pb.Request, error) {
...
// Set ObservedRouteVersion: observedRouteVersion on both PREPARE and COMMIT requests
}… (gemini critical on PR #881) Closes a critical hole gemini's review surfaced on the first revision of M1. The single-shard fast path (`dispatchSingleShardTxn`) correctly forwarded OperationGroup.ObservedRouteVersion into pb.Request via `onePhaseTxnRequest`, but the multi-shard 2PC path (`prewriteTxn` → `commitPrimaryTxn` → `commitSecondaryTxns`) and the `buildTxnLogs` helper dropped it on the floor. Every PREPARE and COMMIT envelope from a cross-shard txn arrived at the FSM with ObservedRouteVersion = 0 ("unpinned"), which would silently bypass the M3 Composed-1 apply-time gate exactly for the workload most at risk of a cross-group route shift. Multi-shard txns are precisely where a `MoveRange` or cross-group `SplitRange` between BeginTxn and Commit can land — losing the plumbing here would have left M3's guard unable to even see those txns to enforce on. Sites fixed: * dispatchTxn — three call sites updated to forward observedRouteVersion into prewriteTxn / commitPrimaryTxn / commitSecondaryTxns. * prewriteTxn — signature gains observedRouteVersion; sets it on every PREPARE pb.Request. * commitPrimaryTxn — signature gains observedRouteVersion; sets it on the primary's COMMIT pb.Request. * commitSecondaryTxns — signature gains observedRouteVersion; sets it on every secondary COMMIT pb.Request. * txnLogs / buildTxnLogs — buildTxnLogs signature gains observedRouteVersion; sets it on both PREPARE and COMMIT requests it constructs; txnLogs forwards reqs.ObservedRouteVersion from the OperationGroup. Tests (added before the fix per CLAUDE.md convention): * TestShardedCoordinatorDispatchTxn_CrossShardPropagatesObservedRouteVersion — routes two PUTs across two shards, sets ObservedRouteVersion=42 on the OperationGroup, asserts every PREPARE + COMMIT envelope across both shards carries the pinned value. Verified failing on the parent commit (0x0 instead of 0x2a, with the precise error message identifying which phase leaked) and passing on this commit. Verification: * go build ./... — clean * go test -race -count=1 ./kv — 9.9 s, pass Self-review (5 lenses): 1. Data loss — pure plumbing; the new field flows through the same Raft envelope structure that already round-trips, and the FSM ignores it pre-M3. No new branch that could refuse a write. 2. Concurrency — additive; no new locks, no new goroutines. 3. Performance — one extra uint64 set per pb.Request in the 2PC path; identical to the existing single-shard cost from #881 parent commit. No new allocations. 4. Data consistency — Composed-1 M1 plumbing is now consistent across single-shard AND multi-shard paths; M3's FSM gate will see the same field shape from every txn entry point. Closes the gemini-critical regression. 5. Test coverage — round-trip test covers PREPARE + COMMIT across both shards. Existing 2PC tests (CrossShardPhasesAndCommitIndex, SingleShardIncludesReadKeysInRaftEntry) continue to pass.
|
Gemini critical addressed in the latest commit: The single-shard fast path was forwarding ObservedRouteVersion correctly via Fixed all 5 sites per gemini's enumeration:
Regression test added before the fix per CLAUDE.md convention: @claude review |
|
Claude finished @bootjp's task in 2m 45s —— View job Code Review — PR #881 (follow-up: 2PC fix verification)Todo
OverallThe 2PC propagation fix is correct and complete. All five sites identified by Gemini are addressed, the regression test is well-constructed, and the abort path is correctly left alone. No blocking issues remain. Fix site verification
Regression test
[Observation, non-blocking]
|
…erved-route-version # Conflicts: # kv/coordinator.go # kv/sharded_coordinator.go
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 (9e2ce9b):
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@proto/internal.proto`:
- Around line 47-55: The generated proto file header in proto/internal.pb.go
indicates it was built with protoc v5.29.6 despite proto/Makefile pinning
PROTOC_VERSION := libprotoc 29.3; regenerate proto/internal.pb.go using the
pinned libprotoc 29.3 (via the Makefile's proto target or the pinned toolchain)
so the file header matches the pinned version and then commit the regenerated
file (ensure the symbol observed_route_version and other messages remain
intact).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8e3d5cff-e6b9-4425-a188-24690f5645d8
⛔ Files ignored due to path filters (1)
proto/internal.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (6)
kv/coordinator.gokv/coordinator_txn_test.gokv/sharded_coordinator.gokv/sharded_coordinator_txn_test.gokv/transcoder.goproto/internal.proto
| // observed_route_version is the durable catalog version the | ||
| // transaction's read set was captured at (set on BeginTxn from | ||
| // distribution.Engine.Version()). Zero means "unpinned" (legacy | ||
| // callers + read-only paths). M3 of the Composed-1 design | ||
| // (docs/design/2026_05_29_proposed_composed1_cross_group_commit_guard.md) | ||
| // will gate the FSM apply path on it; M1 (this field's introduction) | ||
| // is plumbing only — the FSM ignores the value, so all existing | ||
| // callers see no behaviour change. | ||
| uint64 observed_route_version = 6; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the generated field exists and the regen toolchain versions match the pinned ones.
fd -t f 'internal.pb.go' --exec rg -n 'ObservedRouteVersion|protoc-gen-go v|protoc *v' {}
# Inspect proto/Makefile for the pinned toolchain versions referenced in guidelines.
fd -t f 'Makefile' proto --exec cat {}Repository: bootjp/elastickv
Length of output: 2289
Regenerate proto/internal.pb.go with the pinned libprotoc 29.3.
proto/Makefile pins PROTOC_VERSION := libprotoc 29.3, but the generated proto/internal.pb.go header shows protoc v5.29.6 (while still containing observed_route_version = 6), so it wasn’t produced with the required toolchain.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@proto/internal.proto` around lines 47 - 55, The generated proto file header
in proto/internal.pb.go indicates it was built with protoc v5.29.6 despite
proto/Makefile pinning PROTOC_VERSION := libprotoc 29.3; regenerate
proto/internal.pb.go using the pinned libprotoc 29.3 (via the Makefile's proto
target or the pinned toolchain) so the file header matches the pinned version
and then commit the regenerated file (ensure the symbol observed_route_version
and other messages remain intact).
1 participant
Summary
First milestone (M1) of the Composed-1 cross-group commit-time guard per
docs/design/2026_05_29_proposed_composed1_cross_group_commit_guard.md§M1.Plumbs a new
ObservedRouteVersionfield end-to-end so the M3 FSM apply-time gate (separate PR) can read it without further plumbing churn. Behaviour-neutral: every existing caller leaves the field at its zero value, and the FSM at this milestone ignores it.What changes
proto/internal.proto— addsuint64 observed_route_version = 6onpb.Requestwith an inline comment pointing at the design doc. Regeneratedproto/internal.pb.goviaprotoc 29.6+protoc-gen-go v1.36.11.kv/transcoder.go— addsObservedRouteVersion uint64toOperationGroupwith a doc comment that explicitly calls out the zero default and the M3 follow-up.kv/coordinator.go—onePhaseTxnRequestgains theobservedRouteVersion uint64param and sets it on the returnedpb.Request.Coordinate.dispatchTxngains the param and forwards it from the callingOperationGroup.Coordinate.Dispatchalready routes throughdispatchTxn, so the OperationGroup field flows in.Coordinate.buildRedirectRequests(follower→leader forward path) readsreqs.ObservedRouteVersionso the field survives a redirected commit too.kv/sharded_coordinator.go—ShardedCoordinator.dispatchTxnanddispatchSingleShardTxngain the same param, propagated from the callingOperationGroup.What does NOT change
kv/fsm.go) does not readpb.Request.ObservedRouteVersion. That's the M3 milestone.Dispatchwith an OperationGroup whoseObservedRouteVersionis zero by default, which the M3 gate will treat as "unpinned" (legacy) and skip.Tests
TestCoordinateDispatchTxn_PassesObservedRouteVersionToRaftEntry— the round-trip witness:dispatchTxn(..., observedRouteVersion=42)producespb.Request.ObservedRouteVersion == 42. This is the M1 "done when" criterion from §M1.TestCoordinateDispatchTxn_UnpinnedObservedRouteVersionStaysZero— legacy-default witness:dispatchTxn(..., observedRouteVersion=0)producespb.Request.ObservedRouteVersion == 0.0per call site).Verification
go build ./...— cleango test -race -count=1 ./kv— 10.0 s, passmake lint— 0 issuesSelf-review (5 lenses)
pb.Request; one extra uint64 param on the dispatch path. No new atomics, no new allocations.Test plan
go test -race ./kvmake lintgit diff proto/internal.pb.goshould be reviewable as a normal proto field additiondistribution/Engine+kvFSMstruct extension (routes,shardGroupID). M3 — FSM apply-timeverifyComposed1+ coordinator retry onErrComposed1Violation. M4/M5 — Jepsen workload.Resolves
The M1 row in the Composed-1 design doc.
Summary by CodeRabbit
Release Notes
Chores
Tests