kv(composed1): M2 — versioned-snapshot ring + kvFSM RouteHistory wiring#894
Conversation
Second milestone of the Composed-1 cross-group commit-time guard
per docs/design/2026_05_29_proposed_composed1_cross_group_commit_guard.md
§M2. Lands the catalog version retention ring and the kvFSM struct
extension end-to-end so M3 (the apply-time gate) has every piece it
needs without further plumbing churn.
Behaviour-neutral. The FSM stores routes + shardGroupID but does
NOT consult them at apply time yet; nodes wired without
WithRouteHistory remain "unpinned" — the M3 gate will short-circuit
exactly like the pre-feature FSM.
Changes:
* distribution/engine.go — Engine gains a FIFO ring of
`RouteHistorySnapshot` entries keyed by catalogVersion.
Populated on every successful ApplySnapshot AND on
NewEngineWithDefaultRoute (the v=0 seed, so a txn observing
v=0 can still resolve OwnerOf without falling through to the
M3 not-found path on first commit). Default depth is the new
public constant `DefaultRouteHistoryDepth = 32` per design doc
§9 Q2.
* distribution/engine.go — `RouteHistorySnapshot` struct +
`OwnerOf(key) (groupID, bool)` mirroring Engine.GetRoute's
right-half-open interval semantics but against the historical
snapshot. `Version()` accessor. `Engine.SnapshotAt(v)` reads
from the ring; returns (zero, false) for evicted / never-seen
versions (M3 will surface this as ErrComposed1VersionGCd →
coordinator retry, so retention depth is a liveness knob, not
a safety knob).
* kv/fsm.go — kvFSM gains `routes RouteHistory` +
`shardGroupID uint64` fields with doc comments pointing at
§M2 / §4.2. New `RouteHistory` + `RouteSnapshot` interfaces
defined in kv so the FSM does not import distribution
concretely. New `WithRouteHistory(routes, shardGroupID)` FSM
option.
* kv/route_history.go — `WrapDistributionEngine` adapter boxes a
`*distribution.Engine` so it satisfies `kv.RouteHistory` (Go's
structural-interface satisfaction is byte-equivalent on return
types — `Engine.SnapshotAt` returns the concrete
`distribution.RouteHistorySnapshot`, kv.RouteHistory's
SnapshotAt returns the kv.RouteSnapshot interface; moving the
interface to distribution would create an import cycle).
* main.go + main_encryption_write_wiring.go — buildShardGroups
and its caller buildShardGroupsWithEncryptionWiring gain a
`routeEngine *distribution.Engine` parameter. Each shard
FSM's NewKvFSMWithHLC call now installs
`WithRouteHistory(WrapDistributionEngine(routeEngine), g.id)`.
* main_bootstrap_e2e_test.go + multiraft_runtime_test.go —
existing test callers updated for the new buildShardGroups
signature. The e2e test passes cfg.engine; the multiraft
runtime test passes nil (FSM stays unwired, M3 will short-
circuit — matches the pre-feature posture).
Tests:
* distribution/engine_test.go — 6 new tests pinning the M2 ring
contract:
- TestEngineSnapshotAt_RecordsApplySnapshot (round-trip witness)
- TestEngineSnapshotAt_PreservesHistoryAcrossVersions (the M3-
critical "old version is still resolvable after a new
ApplySnapshot lands")
- TestEngineSnapshotAt_FIFOEviction (depth=3 explicit, pins
eviction order so a future depth change does not silently
break the M3 contract)
- TestEngineSnapshotAt_UnknownVersionReturnsNotFound
- TestEngineSnapshotAt_SeedsVersionZeroForDefaultRoute (the
v=0 seed for the common pre-bootstrap case)
- TestEngineSnapshotAt_BareEngineHasNoHistory (bare-struct
Engine for internal test seams is nil-safe; SnapshotAt
always returns false, M3 will treat as "not in ring")
* kv/fsm_route_history_test.go — 4 new tests:
- TestKvFSMWithRouteHistory_StoresFields (the M2 "Done when"
criterion: struct extension wired through the adapter +
the v=0 seed end-to-end)
- TestKvFSM_NilRouteHistoryByDefault (legacy-default: no
WithRouteHistory → routes nil, shardGroupID 0)
- TestWrapDistributionEngine_NilEngineReturnsNil (defensive)
- TestKvFSM_WithRouteHistory_NilProviderTreatedAsUnwired
(defensive: nil RouteHistory via the option is equivalent
to no option)
Verification:
* go build ./... — clean
* go vet ./... — clean
* go test -race -count=1 ./distribution ./kv — 1.0 s + 7.9 s, pass
Self-review (5 lenses):
1. Data loss — no new write paths; the ring is in-memory only
and rebuilt from ApplySnapshot on restart (catalog state is
re-derived from the default Raft group's reserved keys via
the existing watcher). Cannot lose a write because no new
branch refuses one.
2. Concurrency — ring lives under Engine.mu (the existing
write-lock used by ApplySnapshot). recordHistorySnapshot is
called with the lock held. RouteHistorySnapshot is
immutable (routes are cloned on record). SnapshotAt takes
the read lock. No new lock ordering.
3. Performance — one extra map insert + slice append per
ApplySnapshot (operator-frequency, not data-plane). Routes
clone on record is O(|routes|); negligible vs the existing
routesFromCatalog cost. No additional per-request hot-path
work at M2.
4. Data consistency — at M2 the ring is plumbing only; no
consistency surface change. M3 will gate apply on it; the
ring contract (RouteHistorySnapshot is immutable; OwnerOf
uses the same right-half-open semantics as live GetRoute)
matches what M3's verifyComposed1 needs.
5. Test coverage — 10 new unit tests across distribution + kv
cover the round-trip, eviction order, v=0 seed, nil-safe
bare engine, the FSM struct extension, the adapter, and the
legacy default. Existing engine and FSM tests pass
unchanged.
Next milestone (separate PR per design doc §6): M3 — FSM
verifyComposed1 (observed-version + current-version checks) +
ErrComposed1Violation / ErrComposed1VersionGCd sentinels +
coordinator retry path.
|
@claude review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR adds a depth-bounded, FIFO route snapshot history ring to distribution.Engine, exposes snapshot lookup via exported types and Engine accessors, introduces kv RouteHistory/RouteSnapshot interfaces and a nil-safe adapter, and wires route-history into per-group KV FSM construction and tests. ChangesRoute History Ring and FSM Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 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 (85056fa):
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 55s —— View job Code Review — M2: versioned-snapshot ring + kvFSM RouteHistory wiringTasks
SummaryM2 is well-scoped, behaviour-neutral plumbing with good test coverage. No data-loss or correctness bugs found. Three items below warrant attention before M3 ships, one of which is a latent performance issue that will become relevant when M3 puts Issues1.
|
|
To use Codex here, create a Codex account and connect to github. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements the M2 plumbing for the Composed-1 cross-group commit guard by introducing a versioned-snapshot ring to the route catalog Engine and wiring it into the KV FSM. The review feedback identifies two critical issues: a potential loss of version 0 history when the engine is initialized with static routes, and a potential runtime panic in recordHistorySnapshot if historyDepth is left as 0. Both issues have actionable suggestions to improve the robustness of the implementation.
| e.routes = routes | ||
| e.catalogVersion = snapshot.Version | ||
| e.recordHistorySnapshot() |
There was a problem hiding this comment.
When the engine is initialized with static routes (e.g., via buildEngine in main.go using --shardRanges), the initial version 0 routes are never recorded in the history ring because recordHistorySnapshot is not called during startup. If a catalog update then occurs (moving to version 1), the version 0 history is lost. Any active transaction that observed version 0 will fail to resolve its owner via SnapshotAt(0), causing spurious retries or failures.
To fix this, we can record the pre-snapshot state (the current catalogVersion and routes) inside ApplySnapshot before overwriting them. Since recordHistorySnapshot is idempotent, this is safe and ensures version 0 (or any prior unrecorded version) is preserved.
| e.routes = routes | |
| e.catalogVersion = snapshot.Version | |
| e.recordHistorySnapshot() | |
| e.recordHistorySnapshot() | |
| e.routes = routes | |
| e.catalogVersion = snapshot.Version | |
| e.recordHistorySnapshot() |
| if len(e.historyOrder) >= e.historyDepth { | ||
| evict := e.historyOrder[0] | ||
| e.historyOrder = e.historyOrder[1:] | ||
| delete(e.history, evict) | ||
| } |
There was a problem hiding this comment.
If the Engine is constructed with history initialized but historyDepth left as 0 (the default value for int in Go), len(e.historyOrder) >= e.historyDepth will evaluate to 0 >= 0 (true) on the first snapshot record. Since e.historyOrder is empty, accessing e.historyOrder[0] will cause a runtime panic (index out of range).
To prevent this, defensively handle e.historyDepth <= 0 by falling back to DefaultRouteHistoryDepth, and ensure len(e.historyOrder) > 0 before accessing index 0.
depth := e.historyDepth
if depth <= 0 {
depth = DefaultRouteHistoryDepth
}
if len(e.historyOrder) >= depth && len(e.historyOrder) > 0 {
evict := e.historyOrder[0]
e.historyOrder = e.historyOrder[1:]
delete(e.history, evict)
}![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
🧹 Nitpick comments (1)
distribution/engine.go (1)
160-170: ⚡ Quick winUse binary search for historical lookups.
OwnerOflinearly scans a route slice that's already sorted byStart. Once M3 calls this on commit-time verification, each owner lookup becomes O(n) instead of matchingGetRoute's O(log n)` behavior.♻️ Proposed refactor
func (s RouteHistorySnapshot) OwnerOf(key []byte) (uint64, bool) { - for _, r := range s.routes { - if bytes.Compare(key, r.Start) < 0 { - continue - } - if r.End != nil && bytes.Compare(key, r.End) >= 0 { - continue - } - return r.GroupID, true - } - return 0, false + i := sort.Search(len(s.routes), func(i int) bool { + return bytes.Compare(s.routes[i].Start, key) > 0 + }) + if i == 0 { + return 0, false + } + i-- + if end := s.routes[i].End; end != nil && bytes.Compare(key, end) >= 0 { + return 0, false + } + return s.routes[i].GroupID, true }🤖 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 `@distribution/engine.go` around lines 160 - 170, The OwnerOf method currently does a linear scan over RouteHistorySnapshot.routes which are already sorted by Start; replace that scan with a binary search (similar to GetRoute) so lookups are O(log n). Implement a binary search over s.routes comparing key to route.Start using bytes.Compare to find the highest route with Start <= key, then verify the found route's End (if non-nil) with bytes.Compare(key, route.End) < 0 before returning route.GroupID, true; otherwise return 0, false. Ensure you reference RouteHistorySnapshot.OwnerOf and the s.routes slice and preserve existing semantics for nil End bounds.
🤖 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 `@distribution/engine.go`:
- Around line 199-214: In recordHistorySnapshot, guard against non-positive
history depths by treating history as disabled when e.historyDepth <= 0: add an
early return if e.history == nil || e.historyDepth <= 0 to avoid indexing
e.historyOrder[0]; use the existing symbols e.history, e.historyDepth,
e.historyOrder and e.catalogVersion to perform the check and leave the rest of
the eviction/insert logic unchanged so tests/bare-engine paths don’t panic.
---
Nitpick comments:
In `@distribution/engine.go`:
- Around line 160-170: The OwnerOf method currently does a linear scan over
RouteHistorySnapshot.routes which are already sorted by Start; replace that scan
with a binary search (similar to GetRoute) so lookups are O(log n). Implement a
binary search over s.routes comparing key to route.Start using bytes.Compare to
find the highest route with Start <= key, then verify the found route's End (if
non-nil) with bytes.Compare(key, route.End) < 0 before returning route.GroupID,
true; otherwise return 0, false. Ensure you reference
RouteHistorySnapshot.OwnerOf and the s.routes slice and preserve existing
semantics for nil End bounds.
🪄 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: 0a00b8a5-c419-4508-b73a-e20baa9bd710
📒 Files selected for processing (9)
distribution/engine.godistribution/engine_test.gokv/fsm.gokv/fsm_route_history_test.gokv/route_history.gomain.gomain_bootstrap_e2e_test.gomain_encryption_write_wiring.gomultiraft_runtime_test.go
| func (e *Engine) recordHistorySnapshot() { | ||
| if e.history == nil { | ||
| // Engines constructed via the bare struct literal (e.g. | ||
| // internal test seams) — no history ring configured. Skip | ||
| // the record so the M2 plumbing stays optional for those | ||
| // paths; the M3 gate will observe SnapshotAt → (zero, | ||
| // false) and trigger the soft-fail-as-retry path. | ||
| return | ||
| } | ||
| if _, exists := e.history[e.catalogVersion]; exists { | ||
| return | ||
| } | ||
| if len(e.historyOrder) >= e.historyDepth { | ||
| evict := e.historyOrder[0] | ||
| e.historyOrder = e.historyOrder[1:] | ||
| delete(e.history, evict) |
There was a problem hiding this comment.
Handle non-positive history depths defensively.
If an internal seam leaves history initialized but sets historyDepth to 0, the first snapshot record will hit historyOrder[0] and panic. Treating historyDepth <= 0 as “history disabled” makes this helper safe for test/bare-engine paths too.
🛡️ Proposed fix
func (e *Engine) recordHistorySnapshot() {
if e.history == nil {
// Engines constructed via the bare struct literal (e.g.
// internal test seams) — no history ring configured. Skip
// the record so the M2 plumbing stays optional for those
// paths; the M3 gate will observe SnapshotAt → (zero,
// false) and trigger the soft-fail-as-retry path.
return
}
+ if e.historyDepth <= 0 {
+ return
+ }
if _, exists := e.history[e.catalogVersion]; exists {
return
}
if len(e.historyOrder) >= e.historyDepth {
evict := e.historyOrder[0]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (e *Engine) recordHistorySnapshot() { | |
| if e.history == nil { | |
| // Engines constructed via the bare struct literal (e.g. | |
| // internal test seams) — no history ring configured. Skip | |
| // the record so the M2 plumbing stays optional for those | |
| // paths; the M3 gate will observe SnapshotAt → (zero, | |
| // false) and trigger the soft-fail-as-retry path. | |
| return | |
| } | |
| if _, exists := e.history[e.catalogVersion]; exists { | |
| return | |
| } | |
| if len(e.historyOrder) >= e.historyDepth { | |
| evict := e.historyOrder[0] | |
| e.historyOrder = e.historyOrder[1:] | |
| delete(e.history, evict) | |
| func (e *Engine) recordHistorySnapshot() { | |
| if e.history == nil { | |
| // Engines constructed via the bare struct literal (e.g. | |
| // internal test seams) — no history ring configured. Skip | |
| // the record so the M2 plumbing stays optional for those | |
| // paths; the M3 gate will observe SnapshotAt → (zero, | |
| // false) and trigger the soft-fail-as-retry path. | |
| return | |
| } | |
| if e.historyDepth <= 0 { | |
| return | |
| } | |
| if _, exists := e.history[e.catalogVersion]; exists { | |
| return | |
| } | |
| if len(e.historyOrder) >= e.historyDepth { | |
| evict := e.historyOrder[0] | |
| e.historyOrder = e.historyOrder[1:] | |
| delete(e.history, evict) |
🤖 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 `@distribution/engine.go` around lines 199 - 214, In recordHistorySnapshot,
guard against non-positive history depths by treating history as disabled when
e.historyDepth <= 0: add an early return if e.history == nil || e.historyDepth
<= 0 to avoid indexing e.historyOrder[0]; use the existing symbols e.history,
e.historyDepth, e.historyOrder and e.catalogVersion to perform the check and
leave the rest of the eviction/insert logic unchanged so tests/bare-engine paths
don’t panic.
…894 Three findings from claude's review of PR #894, all valid: * OwnerOf O(N)-vs-O(early-break) on sorted route list. Routes are sorted by Start (UpdateRoute / routesFromCatalog keep them sorted; recordHistorySnapshotLocked clones from that order). Once key < r.Start, no later route can cover key either. `continue` worked but iterated the whole tail; `break` short- circuits. Matters because M3 puts OwnerOf on every txn commit's apply path. * historyOrder backing-array growth on FIFO eviction. The previous `e.historyOrder = e.historyOrder[1:]` only moved the slice header — the head of the original backing array stayed alive across evictions, growing unboundedly. Replace with an explicit `make([]uint64, len-1, historyDepth) + copy` so the array stays bounded at historyDepth. * recordHistorySnapshot lock contract fragility. The function requires the caller to hold e.mu (write lock); the doc said so but nothing in the name signalled it. Renamed to recordHistorySnapshotLocked per the Go convention so a future refactor that moves the call past an Unlock surfaces the contract by name. Test nits also addressed: * TestEngineSnapshotAt_FIFOEviction now calls t.Parallel() and the direct `e.historyDepth = 3` write is documented as test-local (Engine not yet shared with any concurrent reader). * TestKvFSM_WithRouteHistory_NilProviderTreatedAsUnwired asserts shardGroupID=7 explicitly so a future refactor that accidentally zeroes it would be caught. Verification: go test -race -count=1 ./distribution ./kv → pass (1.0 s + 8.1 s). No spec change; no contract change visible to callers outside the distribution package.
|
All three claude review items addressed in the latest commit: 1. OwnerOf O(N) → early break. Routes are sorted by Start so once key < r.Start no later route can cover key either; 2. historyOrder backing-array leak. 3. recordHistorySnapshot lock-contract fragility. Renamed to Test nits also addressed:
@claude review |
|
Claude finished @bootjp's task in 3m 38s —— View job Code Review — M2 follow-up (post-fixup commit e2dbb16)Tasks
All three items from the original review and all noted nits are cleanly addressed. Details below. Original issues — verified fixed1. 2. 3. Nits — verified fixed4. 5. Two items for M3 trackingA.
|
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 (e2dbb16):
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. |
Third milestone of the Composed-1 cross-group commit-time guard per docs/design/2026_05_29_proposed_composed1_cross_group_commit_guard.md §M3. Wires the safety property at apply time using the M2 versioned-snapshot ring and the M1 ObservedRouteVersion field. Stacked on PR #894 (M2 plumbing). What's enforced: (a) Observed-version owner — the spec-level Composed-1 check from tla/composed/Composed.tla. Every write key of a Composed-1- pinned txn must be owned by THIS Raft group at the txn's observed catalog version (the version it read its read-set at, set in M1 via OperationGroup.ObservedRouteVersion). Refusal returns ErrComposed1Violation. (b) Current-version cross-version-read fence — the §4.4 / §3 codex P1 trace. Even when (a) passes, a route shift between BeginTxn and Commit can leave the write landing on the OLD owner while readers at the new version route to the NEW owner and miss the write. The current-version fence refuses that case so M4 retry can re-route. Refusal returns ErrComposed1Violation with a different diagnostic prefix ("current-version" vs "observed-version") so the retry path can distinguish. (c) Retention-miss fail-closed — when the txn's observed version has been evicted from the M2 ring (long-running txn or high catalog churn), return ErrComposed1VersionGCd. Per design doc §4.3, the not-found semantics is a hard retryable error, NOT a soft pass, because soft-pass would let the gate be bypassed exactly in the cases where the cross-version-read hazard is most likely. Short-circuits cleanly in three legacy / not-applicable cases: * FSM constructed without WithRouteHistory (legacy / test seam): routes == nil, gate returns nil. * Request carries ObservedRouteVersion == 0 (unpinned — pre-M1 caller, or 2PC ABORT request that doesn't carry the version): gate returns nil. This is what keeps M3 from regressing every caller that has not migrated to set OperationGroup. ObservedRouteVersion. * Engine.Current returns (zero, false) — engine has no history (bare-struct test seam): the (b) fence falls through. Changes: * distribution/engine.go — adds Engine.Current() returning the snapshot at the current catalogVersion (used by the M3 fence). Adds SetHistoryDepthForTest as the cross-package test seam that lets kv-side tests trigger eviction without bypassing the package boundary (claude review on PR #894 — fragile-but- test-local lock contract documented inline). * kv/fsm.go — defines ErrComposed1Violation and ErrComposed1VersionGCd sentinels with the wrapped-error diagnostic shape M4 retry will use. Adds verifyComposed1 and verifyOwnerFromSnapshot. Wires verifyComposed1 into handleTxnRequest at the top so PREPARE / COMMIT / ABORT / NONE all pass through the gate (ABORT carries ObservedRouteVersion=0 and falls through naturally). * kv/route_history.go — RouteHistory interface gains Current(). Adapter forwards to distribution.Engine.Current(). Tests (kv/fsm_composed1_test.go): * TestVerifyComposed1_StaleObservedVersionWithMovedKeyFails — design doc M3 "done when" criterion (i): observed-version snapshot resolves the key to a different group than this FSM. * TestVerifyComposed1_ObservedVersionOlderThanRingFails — criterion (ii): observed version is outside the ring → ErrComposed1VersionGCd. Uses depth=2 via the cross-package SetHistoryDepthForTest seam. * TestVerifyComposed1_ObservedPassesButCurrentDiffersFails — criterion (iii) / the §3 codex P1 trace: observed-version check passes (routes[1][k1]=g1) but the current snapshot at v=2 has moved k1 to g2 → ErrComposed1Violation with the "current-version" diagnostic prefix. * TestVerifyComposed1_ObservedVersionZeroSkipsGate — legacy caller (ObservedRouteVersion=0) bypasses the gate. * TestVerifyComposed1_NilRouteHistorySkipsGate — unwired FSM (no WithRouteHistory option) bypasses the gate; matches the pre-feature posture. Verification: * go build ./... — clean * go vet ./... — clean * go test -race -count=1 ./kv ./distribution — 10.2 s + 1.0 s, pass Self-review (5 lenses): 1. Data loss — the gate refuses commits but cannot lose them. Every rejection emits a sentinel the M4 coordinator path will convert into a successful commit on the correct owner. 2. Concurrency — the gate runs under the FSM's apply loop (Raft applies are serial), so no new lock ordering. RouteHistory reads take the engine's read lock; SnapshotAt and Current each acquire and release in a single call. 3. Performance — two map lookups + a per-mutation linear scan of the snapshot's sorted routes per Composed-1-pinned commit. OwnerOf short-circuits via the M2 round-1 break optimisation, so the per-mutation cost is bounded by the first non-covering gap rather than the full routes slice. Legacy callers (ObservedRouteVersion=0) pay one branch per commit and exit. 4. Data consistency — closes the Composed-1 (observed) and Composed-1a (current) gaps end-to-end. Spec correspondence: tla/composed/Composed.tla's Commit precondition is now enforced at apply time; the §4.4 fence matches Composed1a_CommitToCurrentOwner from PR #878. 5. Test coverage — 5 new unit tests cover the three design doc criteria + the two legacy short-circuit paths. Existing handleTxnRequest tests pass unchanged (their requests carry ObservedRouteVersion=0 from M1's behaviour-neutral default). Next milestone (separate PR per design doc §6): M4 — coordinator retry path. When either Composed-1 sentinel returns, the coordinator re-reads the route cache, re-routes the txn against the new owning group, and re-issues it once.
Third milestone of the Composed-1 cross-group commit-time guard per docs/design/2026_05_29_proposed_composed1_cross_group_commit_guard.md §M3. Wires the safety property at apply time using the M2 versioned-snapshot ring and the M1 ObservedRouteVersion field. Stacked on PR #894 (M2 plumbing). What's enforced: (a) Observed-version owner — the spec-level Composed-1 check from tla/composed/Composed.tla. Every write key of a Composed-1- pinned txn must be owned by THIS Raft group at the txn's observed catalog version (the version it read its read-set at, set in M1 via OperationGroup.ObservedRouteVersion). Refusal returns ErrComposed1Violation. (b) Current-version cross-version-read fence — the §4.4 / §3 codex P1 trace. Even when (a) passes, a route shift between BeginTxn and Commit can leave the write landing on the OLD owner while readers at the new version route to the NEW owner and miss the write. The current-version fence refuses that case so M4 retry can re-route. Refusal returns ErrComposed1Violation with a different diagnostic prefix ("current-version" vs "observed-version") so the retry path can distinguish. (c) Retention-miss fail-closed — when the txn's observed version has been evicted from the M2 ring (long-running txn or high catalog churn), return ErrComposed1VersionGCd. Per design doc §4.3, the not-found semantics is a hard retryable error, NOT a soft pass, because soft-pass would let the gate be bypassed exactly in the cases where the cross-version-read hazard is most likely. Short-circuits cleanly in three legacy / not-applicable cases: * FSM constructed without WithRouteHistory (legacy / test seam): routes == nil, gate returns nil. * Request carries ObservedRouteVersion == 0 (unpinned — pre-M1 caller, or 2PC ABORT request that doesn't carry the version): gate returns nil. This is what keeps M3 from regressing every caller that has not migrated to set OperationGroup. ObservedRouteVersion. * Engine.Current returns (zero, false) — engine has no history (bare-struct test seam): the (b) fence falls through. Changes: * distribution/engine.go — adds Engine.Current() returning the snapshot at the current catalogVersion (used by the M3 fence). Adds SetHistoryDepthForTest as the cross-package test seam that lets kv-side tests trigger eviction without bypassing the package boundary (claude review on PR #894 — fragile-but- test-local lock contract documented inline). * kv/fsm.go — defines ErrComposed1Violation and ErrComposed1VersionGCd sentinels with the wrapped-error diagnostic shape M4 retry will use. Adds verifyComposed1 and verifyOwnerFromSnapshot. Wires verifyComposed1 into handleTxnRequest at the top so PREPARE / COMMIT / ABORT / NONE all pass through the gate (ABORT carries ObservedRouteVersion=0 and falls through naturally). * kv/route_history.go — RouteHistory interface gains Current(). Adapter forwards to distribution.Engine.Current(). Tests (kv/fsm_composed1_test.go): * TestVerifyComposed1_StaleObservedVersionWithMovedKeyFails — design doc M3 "done when" criterion (i): observed-version snapshot resolves the key to a different group than this FSM. * TestVerifyComposed1_ObservedVersionOlderThanRingFails — criterion (ii): observed version is outside the ring → ErrComposed1VersionGCd. Uses depth=2 via the cross-package SetHistoryDepthForTest seam. * TestVerifyComposed1_ObservedPassesButCurrentDiffersFails — criterion (iii) / the §3 codex P1 trace: observed-version check passes (routes[1][k1]=g1) but the current snapshot at v=2 has moved k1 to g2 → ErrComposed1Violation with the "current-version" diagnostic prefix. * TestVerifyComposed1_ObservedVersionZeroSkipsGate — legacy caller (ObservedRouteVersion=0) bypasses the gate. * TestVerifyComposed1_NilRouteHistorySkipsGate — unwired FSM (no WithRouteHistory option) bypasses the gate; matches the pre-feature posture. Verification: * go build ./... — clean * go vet ./... — clean * go test -race -count=1 ./kv ./distribution — 10.2 s + 1.0 s, pass Self-review (5 lenses): 1. Data loss — the gate refuses commits but cannot lose them. Every rejection emits a sentinel the M4 coordinator path will convert into a successful commit on the correct owner. 2. Concurrency — the gate runs under the FSM's apply loop (Raft applies are serial), so no new lock ordering. RouteHistory reads take the engine's read lock; SnapshotAt and Current each acquire and release in a single call. 3. Performance — two map lookups + a per-mutation linear scan of the snapshot's sorted routes per Composed-1-pinned commit. OwnerOf short-circuits via the M2 round-1 break optimisation, so the per-mutation cost is bounded by the first non-covering gap rather than the full routes slice. Legacy callers (ObservedRouteVersion=0) pay one branch per commit and exit. 4. Data consistency — closes the Composed-1 (observed) and Composed-1a (current) gaps end-to-end. Spec correspondence: tla/composed/Composed.tla's Commit precondition is now enforced at apply time; the §4.4 fence matches Composed1a_CommitToCurrentOwner from PR #878. 5. Test coverage — 5 new unit tests cover the three design doc criteria + the two legacy short-circuit paths. Existing handleTxnRequest tests pass unchanged (their requests carry ObservedRouteVersion=0 from M1's behaviour-neutral default). Next milestone (separate PR per design doc §6): M4 — coordinator retry path. When either Composed-1 sentinel returns, the coordinator re-reads the route cache, re-routes the txn against the new owning group, and re-issues it once.
1 participant
Summary
Second milestone (M2) of the Composed-1 cross-group commit-time guard per
docs/design/2026_05_29_proposed_composed1_cross_group_commit_guard.md§M2.Lands the catalog version retention ring and the kvFSM struct extension end-to-end so M3 (the apply-time gate) has every piece it needs without further plumbing churn. Behaviour-neutral: the FSM stores
routes+shardGroupIDbut does not consult them at apply time yet.What changes
Distribution layer
distribution.Enginegains a FIFO ring ofRouteHistorySnapshotentries keyed bycatalogVersion. Populated on every successfulApplySnapshotAND onNewEngineWithDefaultRoute(the v=0 seed, so a txn observing v=0 can resolveOwnerOfwithout falling through to the M3 not-found path on first commit).DefaultRouteHistoryDepth = 32per design doc §9 Q2.RouteHistorySnapshotstruct +OwnerOf(key) (groupID, bool)+Version()accessor.OwnerOfmirrorsEngine.GetRoute's right-half-open interval semantics against the historical snapshot.Engine.SnapshotAt(v)reads from the ring; returns(zero, false)for evicted / never-seen versions (M3 will surface this asErrComposed1VersionGCd→ coordinator retry).kv layer
kvFSMgainsroutes RouteHistory+shardGroupID uint64fields with doc comments pointing at §M2 / §4.2.RouteHistory+RouteSnapshotinterfaces defined inkvso the FSM does not import distribution concretely.WithRouteHistory(routes, shardGroupID)FSM option.kv/route_history.go::WrapDistributionEngineadapter boxes a*distribution.Engineso it satisfieskv.RouteHistory. Go's structural-interface satisfaction is byte-equivalent on return types —Engine.SnapshotAtreturns the concrete struct, kv.RouteHistory'sSnapshotAtreturns the interface; moving the interface todistributionwould create an import cycle.main.go wiring
buildShardGroupsand its callerbuildShardGroupsWithEncryptionWiringgain arouteEngine *distribution.Engineparameter.NewKvFSMWithHLCnow installsWithRouteHistory(WrapDistributionEngine(routeEngine), g.id).main_bootstrap_e2e_test.go,multiraft_runtime_test.go) updated for the new signature.Tests (10 new)
distribution/engine_test.go (6 new):
TestEngineSnapshotAt_RecordsApplySnapshot— round-trip witnessTestEngineSnapshotAt_PreservesHistoryAcrossVersions— the M3-critical "old version is still resolvable after a new ApplySnapshot lands"TestEngineSnapshotAt_FIFOEviction— depth=3 explicit, pins eviction order so a future depth change does not silently break the M3 contractTestEngineSnapshotAt_UnknownVersionReturnsNotFoundTestEngineSnapshotAt_SeedsVersionZeroForDefaultRoute— the v=0 seed for the common pre-bootstrap caseTestEngineSnapshotAt_BareEngineHasNoHistory— bare-struct Engine is nil-safe; SnapshotAt always returns false (M3 will treat as "not in ring")kv/fsm_route_history_test.go (4 new):
TestKvFSMWithRouteHistory_StoresFields— the M2 "Done when" criterion: struct extension wired through the adapter + v=0 seed end-to-endTestKvFSM_NilRouteHistoryByDefault— legacy-default: noWithRouteHistory→routesnil,shardGroupID0TestWrapDistributionEngine_NilEngineReturnsNil— defensiveTestKvFSM_WithRouteHistory_NilProviderTreatedAsUnwired— defensive: nilRouteHistoryvia the option ≡ no optionVerification
go build ./...— cleango vet ./...— cleango test -race -count=1 ./distribution ./kv— 1.0 s + 7.9 s, passmake lint— 0 issuesSelf-review (5 lenses)
ApplySnapshoton restart (catalog state is re-derived from the default Raft group's reserved keys via the existing watcher).Engine.mu(the existing write-lock used byApplySnapshot).recordHistorySnapshotis called with the lock held.RouteHistorySnapshotis immutable (routes are cloned on record).SnapshotAttakes the read lock. No new lock ordering.ApplySnapshot(operator-frequency, not data-plane). Routes clone isO(|routes|). No additional per-request hot-path work at M2.verifyComposed1needs.Test plan
go test -race ./distribution ./kvmake lintverifyComposed1(observed-version §4.2(a) + current-version §4.4) +ErrComposed1Violation/ErrComposed1VersionGCdsentinels + coordinator retry path.Resolves
The M2 row in the Composed-1 design doc.
Summary by CodeRabbit
New Features
Tests