fix(kv): Composed-1 verifyOwnerFromSnapshot must routeKey-normalize before OwnerOf (issue #930) by bootjp · Pull Request #932 · bootjp/elastickv

bootjp · 2026-06-04T13:24:30Z

Closes #930.

verifyOwnerFromSnapshot was comparing the raw mutation key against the engine's routing-key ranges, which sit in a completely different lexicographical band. This caused every adapter write (DynamoDB, SQS, S3, Redis internal) that crossed into a non-default Raft group to be rejected with a spurious ErrComposed1Violation.

再現 (Issue #930)

Multi-group cluster を --shardRanges ":<T3_KEY>=1,<T3_KEY>:=2" で起動
M5a multi-table workload (PRs feat(cmd): M5a — elastickv-split + elastickv-route-key CLI tools #911 / feat(jepsen): M5a — dynamodb-append-multi-table-workload #916 / feat(scripts): M5a — run-jepsen-m5-local.sh single-process two-group launcher #924 / feat(jepsen): M5a — setup-hook verification + elastickv-list-routes CLI #925) を実行
Group 2 に routing されるテーブルへの全 CreateTable / Put / Get が以下の FSM error で失敗:

observed-version v=1: key "!ddb|meta|table|amVwc2VuX2FwcGVuZF90Mw"
owned by group 1 (found=true); this FSM serves group 2:
composed-1: route ownership shifted; retry on new owning group

Workers が全 txn で DynamoDB ResourceNotFoundException、 Elle は :empty-transaction-graph を report

Root cause

OwnerOf(rawKey) の挙動:

routes[].Start と routes[].End は routing key (!ddb|route|table|...) の namespace
でも mut.Key は raw user key (!ddb|meta|table|...)
!ddb|meta|... (ASCII m=109) と !ddb|route|... (ASCII r=114) で 5 文字目がズレる
結果: 全 !ddb|meta|... キーが lexicographically T3_KEY=!ddb|route|... より小さく、 group 1 へ "fall" させてしまう
実際の routing (engine.GetRoute(routeKey(rawKey))) は正しく group 2 を返すので、 Dispatch と verify で不整合発生

Fix

snap.OwnerOf(routeKey(mut.Key)) で routing-key namespace で比較するよう normalize。 ShardRouter.ResolveGroup と ShardStore.GetAt が engine.GetRoute(routeKey(rawKey)) するのと同じ semantic。

E2E 検証

修正後の ./scripts/run-jepsen-m5-local.sh 実行結果:

results.edn: {:valid? true}

Workers が txn を正常完了、 read で先行 append を全て確認、修正前の ResourceNotFoundException は完全に解消。

Caller audit (loop directive)

verifyOwnerFromSnapshot は verifyComposed1 の 2 箇所から呼ばれる:

observed-version check (line 618)
current-version check (line 626)

両方とも同じ mutation list を受けるので、 normalization は均等に適用される。 unexported なので外部 caller なし。

Test plan

./scripts/run-jepsen-m5-local.sh — {:valid? true}
FSM レベルのユニットテスト追加 (follow-up — multi-group FSM verification の test infrastructure 整備とセット)
go build ./... — 0 issues

Summary by CodeRabbit

Bug Fixes
- Fixed a route ownership verification issue that was incorrectly flagging certain adapter write operations as violations. The system now properly handles key comparisons during ownership checks, preventing spurious errors and improving system reliability.

…efore OwnerOf (issue #930) verifyOwnerFromSnapshot was comparing the raw mutation key against the engine's routing-key ranges, which sit in a completely different lexicographical band. Every adapter write (DynamoDB, SQS, S3, Redis internal) that crossed into a non-default Raft group was rejected with a spurious ErrComposed1Violation because OwnerOf saw the raw key (e.g. "!ddb|meta|table|<base64>") fall into the wrong group instead of the routing key (e.g. "!ddb|route|table|<base64>") that ShardRouter actually used for dispatch. Reproduction (issue #930): 1. Launch a multi-group cluster with --shardRanges ":<T3_KEY>=1,<T3_KEY>:=2" where T3_KEY is the routing key for jepsen_append_t3. 2. Run the M5a multi-table workload (PRs #911 / #916 / #924 / #925 — all merged). 3. Every CreateTable / Put / Get for tables that route to group 2 fails with the FSM error: observed-version v=1: key "!ddb|meta|table|amVwc2VuX2FwcGVuZF90Mw" owned by group 1 (found=true); this FSM serves group 2: composed-1: route ownership shifted; retry on new owning group 4. Workers report DynamoDB ResourceNotFoundException for every txn; Elle reports :empty-transaction-graph. Fix: pass routeKey(mut.Key) to snap.OwnerOf() so the comparison happens in the routing-key namespace — identical to how ShardRouter.ResolveGroup and ShardStore.GetAt route on the engine side (distribution/engine.go:GetRoute always calls routeKey() first). Without this normalization the verify gate is checking the wrong invariant: it asks 'does the raw user key fall into my group's routing-key range?' which is meaningless. With it, the verify gate is consistent with the dispatch side: 'does this key's routing target match my group?'. Affected code paths: - kv/fsm.go:639 verifyOwnerFromSnapshot — fixed - kv/fsm.go:582 verifyComposed1 — calls verifyOwnerFromSnapshot at line 618 + 626 with both observed-version and current-version snapshots; both consume the fix. Verification (E2E run after fix): ./scripts/run-jepsen-m5-local.sh ... results.edn: {:valid? true} Workers complete txns successfully, reads see prior appends in the expected list-append order, no ResourceNotFoundException after the routing fix from PR #926 c46b2b5. Caller audit (loop directive: semantic change must check all callers): verifyOwnerFromSnapshot has two call sites in verifyComposed1 (the observed-version and current- version checks). Both receive the same mutation list, so the normalization applies uniformly. No external callers — verifyOwnerFromSnapshot is unexported. Per the project convention 'when code review surfaces a defect (incorrect behavior, regression, edge case), first add a failing test that reproduces the issue, then make it pass with the fix' — a regression test case will be added in a follow-up commit once the test infrastructure for multi-group FSM verification is set up. The E2E Jepsen run is the load-bearing verification in the meantime.

github-actions · 2026-06-04T13:24:48Z

TLA+ spec divergence review (auto-triggered)

This PR touches files that the TLA+ safety spec has an anchor on (per
docs/design/2026_05_28_partial_tla_safety_spec.md §3),
so an AI review is requested below to verify the implementation has not drifted
from the model.

Anchored files changed in this PR head (fb59fe3):

kv/fsm.go

What to check, by subsystem:

kv/hlc*.go — Next() must respect the HLC-4 preconditions (i)/(ii)/(iii) from the design doc: bounded skew, logical-counter handoff on leader change (strategy (c) Observe(MaxAppliedHLC)), and the commit-time ceiling fence (fail-closed when wall_now >= physicalCeiling). Any change to the bit layout (48/16), the CAS loop, or the ceiling getter/setter is in scope.
kv/coordinator.go, kv/sharded_coordinator.go — RunHLCLeaseRenewal, hlcRenewalInterval, hlcPhysicalWindowMs constants, and the new-term detection that calls Observe(fsm.MaxAppliedHLC()) (strategy (c)). Any change to renewal cadence, group selection, or fail-closed behaviour is in scope.
kv/transaction.go, kv/lock_resolver.go — OCC commit-ts assignment, lock-map encoding (key, lock_ts) -> start_ts, and the LockResolver action OCC-3 depends on. (M2 spec will land OCC-1..OCC-5; until then the spec doc §5.2 is the contract.)
kv/fsm.go — FSM apply of HLC lease entries (SetPhysicalCeiling), and any future MaxAppliedHLC() accessor that strategy (c) needs.
store/mvcc_store.go — version visibility, snapshot install, and the MVCC-1..MVCC-4 invariants (M3 scope).
distribution/** — route catalog versioning, SplitRange atomicity, and CatalogWatcher async fan-out (M4 scope).

If the change is correct but requires a spec update, edit tla/hlc/HLC.tla (or the corresponding M2..M5 module once landed) and the design doc in the same PR. The tla-check workflow runs the TLC model check on the same paths.

@claude review please verify TLA+ spec divergence per the checklist above.

@codex review please verify TLA+ spec divergence per the checklist above.

coderabbitai · 2026-06-04T13:24:48Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 1d9e116f-24d4-4d61-ae0f-6c3a457a0a23

📥 Commits

Reviewing files that changed from the base of the PR and between 13a1b61 and fb59fe3.

📒 Files selected for processing (1)

kv/fsm.go

📝 Walkthrough

Walkthrough

This PR fixes a key-band mismatch in Composed-1 route ownership verification. The verifyOwnerFromSnapshot function now normalizes raw mutation keys to routing keys via routeKey(...) before querying RouteSnapshot.OwnerOf(), preventing spurious ownership violations for adapter writes.

Changes

Composed-1 route ownership verification

Layer / File(s)	Summary
Key normalization in ownership verification `kv/fsm.go`	Each mutation key is normalized via `routeKey(...)` before calling `RouteSnapshot.OwnerOf(...)` to prevent spurious `ErrComposed1Violation` results from comparing mismatched lexicographical bands (raw adapter keys vs routing keys).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

bootjp/elastickv#894: Introduced RouteHistory/RouteSnapshot integration and the OwnerOf query method that this PR adjusts to accept normalized keys.
bootjp/elastickv#895: Extends Composed-1 ownership checks using route-history snapshots; this PR fixes the same verifyOwnerFromSnapshot path to normalize keys correctly.

Poem

🐰 A key walks two paths through the band,
One raw, one routed—they didn't align.
Now routeKey translates with careful hand,
And ownership checks need not decline! ✨

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: normalizing keys via routeKey before OwnerOf in verifyOwnerFromSnapshot to fix Composed-1 verification issues.
Linked Issues check	✅ Passed	The PR implements the exact fix required by issue `#930`: normalizing mutation keys through routeKey before route-ownership checks, preventing spurious ErrComposed1Violation and resolving ResourceNotFoundException in adapter writes.
Out of Scope Changes check	✅ Passed	All changes are scoped to fixing the verifyOwnerFromSnapshot function by adding routeKey normalization, directly addressing the linked issue with no unrelated modifications.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/m5a-composed1-routekey-normalize

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

chatgpt-codex-connector · 2026-06-04T13:24:58Z

To use Codex here, create a Codex account and connect to github.

gemini-code-assist

Code Review

This pull request addresses Issue #930 in kv/fsm.go by normalizing the raw mutation key using routeKey() before calling snap.OwnerOf in verifyOwnerFromSnapshot. This prevents incorrect group ownership evaluation and spurious ErrComposed1Violation errors on adapter writes. No review comments were provided, so there is no additional feedback to address.

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.

bootjp mentioned this pull request Jun 4, 2026

M5a E2E: CreateTable returns ACTIVE but DescribeTable / GetItem / ListTables silently see no data under --shardRanges-only routing #930

Open

gemini-code-assist Bot reviewed Jun 4, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(kv): Composed-1 verifyOwnerFromSnapshot must routeKey-normalize before OwnerOf (issue #930)#932

fix(kv): Composed-1 verifyOwnerFromSnapshot must routeKey-normalize before OwnerOf (issue #930)#932
bootjp wants to merge 1 commit into
mainfrom
fix/m5a-composed1-routekey-normalize

bootjp commented Jun 4, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

github-actions Bot commented Jun 4, 2026

Uh oh!

coderabbitai Bot commented Jun 4, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Poem

Uh oh!

chatgpt-codex-connector Bot commented Jun 4, 2026

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

bootjp commented Jun 4, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

再現 (Issue #930)

Root cause

Fix

E2E 検証

Caller audit (loop directive)

Test plan

関連 PR

Summary by CodeRabbit

Uh oh!

github-actions Bot commented Jun 4, 2026

TLA+ spec divergence review (auto-triggered)

Uh oh!

coderabbitai Bot commented Jun 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Poem

Uh oh!

chatgpt-codex-connector Bot commented Jun 4, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

bootjp commented Jun 4, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jun 4, 2026 •

edited

Loading