fix(persistence): MULTI/EXEC graph leg — route GRAPH.* through the txn executor + replicate (kernel M3 stage 3, task #52)#300
Merged
Conversation
…EXEC (kernel M3 stage 3, task #52) A committed cross-store transaction (SET + GRAPH.ADDNODE in one MULTI/EXEC) survived kill-9 on the KV leg (PR #247's persist_txn_aof) but silently lost the graph leg. Root cause: execute_transaction_sharded (src/server/conn/shared.rs), the executor shared by the sharded and monoio connection handlers for EXEC, had no GRAPH.* branch at all -- a queued GRAPH.ADDNODE fell through to the generic KV dispatch() table (which only knows keyspace commands) and errored "unknown command", an error MULTI/EXEC's per-command tolerance swallowed silently. The mutation never even applied in memory, let alone reached durable storage. Fixed by giving the txn executor a GRAPH.* branch that mirrors the single-command path (try_handle_graph_command): dispatches writes/reads against ShardSlice::graph_store, and flushes every command's drained wal-v3 records via wal_append after the whole transaction body runs (same per-shard append-order contract as the live path). Both the KV AOF leg and the graph wal-v3 leg are independently proven durable by the crash-matrix scenario's two separate sync-waits; ordering between the two log families is otherwise unconstrained by design. Verified 3x consecutive GREEN at shards=1 and shards=4. The sibling atomicity claim (a transaction queued but killed before EXEC applies nothing) was already GREEN and is unaffected -- nothing in this path executes before EXEC runs. Crash-matrix cells cross_plane_prod_s1_txn_isolated_committed and cross_plane_prod_s4_txn_isolated_committed (tests/crash_matrix_cross_plane/) flip from red_guard-gated RED to default-GREEN regression tripwires. Full 42-cell suite green-only run passes (33->35 GREEN by default, 9->7 RED); fmt/clippy clean on both default and runtime-tokio+jemalloc feature sets; relevant lib test suites (persistence::, graph::, command::graph, txn, shared::) all green. Files: - src/server/conn/shared.rs: GRAPH.* branch in execute_transaction_sharded + post-loop wal_append flush - tests/crash_matrix_cross_plane/tests_prod.rs: remove red_guard from the two committed-txn cells, document the fix - tests/crash_matrix_cross_plane.rs: update RED/GREEN cell inventory doc - CHANGELOG.md: Unreleased entry Refs: task #52, kernel M3 stage 3 author: Tin Dang
… leg (task #52 review round 2, P1) The durability fix in dde3240 gave execute_transaction_sharded a GRAPH.* branch that wal_append'd the drained graph records itself, from inside the function. That function has no ConnectionContext/replication access, so the graph leg of a committed cross-store transaction applied and persisted on the master but never reached a replica -- a NEW master/replica divergence the durability fix introduced (pre-fix the in-txn GRAPH.* command applied nowhere at all, so there was no divergence to have). execute_transaction_sharded now RETURNS the collected graph records (db, bytes) instead of appending them internally, bound to the db each command executed in (same per-entry-db rule PR #282 established for the KV AOF entries -- a queued SELECT redirects only the commands after it). Callers now take over replication + durability, matching the live single-command graph path's contract exactly: - handler_monoio/write.rs (EXEC block): replicates each graph record via record_local_write_db, gated `ctx.num_shards == 1 && replication_fanout_active(ctx)` -- the same scope the live try_handle_graph_command path uses (graph replication is single-shard only; multi-shard graph replication rides the R2 broadcast redesign) -- in the same synchronous stretch as the mutation, THEN wal_append, same replicate-then-append order as the live path. - handler_sharded/write.rs (tokio caller): wal_append only. Replication is monoio-only by design; this caller has no replication plane to fan out to. - shard/spsc_handler.rs (TxnExecute cross-shard hop arm): wal_append only. This arm only runs when the accepting connection's shard differs from the txn's owner shard, which by construction requires num_shards > 1 -- exactly the regime where graph replication is already out of scope, so no replication call is needed here either. New regression test tests/replication_graph.rs::replica_syncs_multi_exec_graph_leg: shards=1 master+replica, MULTI/SET+GRAPH.ADDNODE/EXEC on the master, asserts both the KV and graph legs land on the replica over one live stream (stream-health check: zero reconnects, same convention as this file's other tests). Confirmed the test is not vacuous by disabling just the new record_local_write_db call and re-running: RED (graph leg missing on replica, KV leg present) -- then restored the fix and confirmed GREEN 3x consecutive. Re-ran full gate set after this change: - crash_matrix_cross_plane's 4 txn cells (committed x2 shard counts, atomicity x2 shard counts): GREEN 3x consecutive, unaffected. - Full 42-cell crash-matrix default (green-only) run: 41/42, one unrelated pre-existing flake (cross_plane_prod_s4_mq_isolated, MQ DLQ kill-9 timing) confirmed 3/3 green in isolation -- not touched by this change. - fmt --check: clean. - clippy --release -- -D warnings: clean. - clippy --no-default-features --features runtime-tokio,jemalloc -- -D warnings: clean. - cargo build --release --no-default-features --features runtime-tokio,jemalloc: clean. - lib tests (persistence::, graph::, command::graph, txn, shared::): all green. Files: - src/server/conn/shared.rs: execute_transaction_sharded returns graph records instead of appending them; per-entry db binding - src/server/conn/handler_monoio/write.rs: EXEC block replicates + wal_appends the graph records - src/server/conn/handler_sharded/write.rs: EXEC block wal_appends the graph records (no replication leg) - src/shard/spsc_handler.rs: TxnExecute arm wal_appends the graph records - tests/replication_graph.rs: new replica_syncs_multi_exec_graph_leg test + send_txn helper - CHANGELOG.md: Unreleased entry Refs: task #52, kernel M3 stage 3, follow-up to dde3240 author: Tin Dang
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Warning Review limit reached
Next review available in: 37 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughChangesGraph-leg correctness
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Client
participant EXEC_Handler
participant Transaction_Executor
participant GraphStore
participant LocalWriteReplication
participant ShardWAL
Client->>EXEC_Handler: MULTI/EXEC with KV and GRAPH.* commands
EXEC_Handler->>Transaction_Executor: execute transaction
Transaction_Executor->>GraphStore: dispatch GRAPH.* command
GraphStore-->>Transaction_Executor: result and graph WAL record
Transaction_Executor-->>EXEC_Handler: result and graph_records
EXEC_Handler->>LocalWriteReplication: replicate eligible graph record
EXEC_Handler->>ShardWAL: append graph record
EXEC_Handler-->>Client: transaction response
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/conn/shared.rs (1)
123-198: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
GRAPH.*needs the same MULTI/EXEC handling inhandler_single
QueuedGRAPH.*commands still go throughexecute_transaction()’s genericdispatch()path here, so transactional graph commands will fall through toERR unknown commandunless this path mirrors the sharded graph branch or rejects them before queueing.🤖 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 `@src/server/conn/shared.rs` around lines 123 - 198, Update execute_transaction to handle queued GRAPH.* commands consistently with the sharded transaction path: detect graph commands before the generic dispatch call and route them through the existing graph transaction handling, or reject them during queueing so they cannot reach dispatch. Preserve atomic execution and response/AOF behavior for all other commands.Source: Learnings
🧹 Nitpick comments (1)
src/server/conn/handler_monoio/write.rs (1)
889-906: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid double-cloning each graph WAL record.
record.clone()at lines 897 and 903 copies the underlyingVec<u8>twice per record — once for replication, once for the WAL append — even thoughgraph_recordsis owned by this point in the function. Converting toBytesonce and cloning the (cheap, ref-counted)Byteshandle avoids the extra heap copy.♻️ Proposed fix
- let graph_repl_active = ctx.num_shards == 1 && repl_active; - for (entry_db, record) in &graph_records { - if graph_repl_active { - super::ft::record_local_write_db( - ctx, - *entry_db, - bytes::Bytes::from(record.clone()), - ); - } - ctx.shard_databases.wal_append( - ctx.shard_id, - crate::persistence::wal_v3::record::WalRecordType::Command, - bytes::Bytes::from(record.clone()), - ); - } + let graph_repl_active = ctx.num_shards == 1 && repl_active; + for (entry_db, record) in graph_records { + let record_bytes = bytes::Bytes::from(record); + if graph_repl_active { + super::ft::record_local_write_db(ctx, entry_db, record_bytes.clone()); + } + ctx.shard_databases.wal_append( + ctx.shard_id, + crate::persistence::wal_v3::record::WalRecordType::Command, + record_bytes, + ); + }🤖 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 `@src/server/conn/handler_monoio/write.rs` around lines 889 - 906, Update the graph record loop in the graph-enabled write path to convert each owned record into a bytes::Bytes value once, then clone that ref-counted handle only when both replication and WAL append need it. Replace the separate record.clone() conversions in the graph_repl_active branch and wal_append call while preserving their existing destinations and behavior.
🤖 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 `@src/server/conn/shared.rs`:
- Around line 274-318: Update transaction locality analysis and execution
routing for GRAPH.* commands so graph operations run on the shard returned by
graph_to_shard(name, ctx.num_shards), including graph-only and mixed MULTI/EXEC
transactions. Use the graph command’s graph name to determine ownership, and
preserve existing KV locality handling for non-graph commands; ensure
execute_transaction_sharded dispatches the graph portion on the owner shard
rather than the current shard.
---
Outside diff comments:
In `@src/server/conn/shared.rs`:
- Around line 123-198: Update execute_transaction to handle queued GRAPH.*
commands consistently with the sharded transaction path: detect graph commands
before the generic dispatch call and route them through the existing graph
transaction handling, or reject them during queueing so they cannot reach
dispatch. Preserve atomic execution and response/AOF behavior for all other
commands.
---
Nitpick comments:
In `@src/server/conn/handler_monoio/write.rs`:
- Around line 889-906: Update the graph record loop in the graph-enabled write
path to convert each owned record into a bytes::Bytes value once, then clone
that ref-counted handle only when both replication and WAL append need it.
Replace the separate record.clone() conversions in the graph_repl_active branch
and wal_append call while preserving their existing destinations and behavior.
🪄 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: 21d1cf26-f668-463a-bccf-6580ae1e62ed
📒 Files selected for processing (8)
CHANGELOG.mdsrc/server/conn/handler_monoio/write.rssrc/server/conn/handler_sharded/write.rssrc/server/conn/shared.rssrc/shard/spsc_handler.rstests/crash_matrix_cross_plane.rstests/crash_matrix_cross_plane/tests_prod.rstests/replication_graph.rs
…fication (task #52 review round 3, CodeRabbit P1) CodeRabbit review on PR #300: analyze_txn_locality (the pre-EXEC classifier deciding whether a queued body runs locally, hops to a remote owner shard, or is rejected CROSSSLOT) only scans KV keys via command_keys. GRAPH.* commands declare no keys in command metadata (first_key==last_key==0), so a graph-bearing MULTI body was misclassified -- Keyless for a graph-only body, or by its KV keys alone for a mixed body -- ignoring the graph name's REAL owner shard entirely. The standalone (non-MULTI) GRAPH.* path routes by graph_to_shard(name, num_shards), which is IDENTICAL hashing to key_to_shard (same hash-tag-aware xxh64). At num_shards > 1 a transaction whose true graph owner differed from the classified owner would durably apply the GRAPH.* write to the WRONG shard's graph_store -- invisible to subsequent normally-routed single-command reads. Verified at source before coding: the s4 crash cells (dde3240/9f44816e) never caught this because scenarios::txn_isolated_committed's KV key and graph name deliberately share one {txniso} hash tag (co-location by construction, see tests/crash_matrix_cross_plane/planes.rs's graph_name helper), so they always agree on owner regardless of the bug -- not scatter reads, not luck, just a test fixture that happened to sidestep the exact case the finding describes. Fix: fold the graph name into analyze_txn_locality's existing `visit` closure -- the SAME one KV keys and the SORT/GEORADIUS STORE-dest already use. A body whose KV keys and graph name disagree on owner becomes CrossShard (rejected CROSSSLOT, same posture as any other genuine cross-shard body -- the REJECT posture, chosen over a hop per review guidance). A graph-only body becomes SingleShard(graph_owner), which routes through the SAME whole-body-atomic owner hop (execute_txn_on_owner / ShardMessage::TxnExecute) a KV-only remote-owner body already uses -- no NEW hop mechanism is introduced, so the "body runs on ONE local shard slice" invariant holds and atomicity is preserved. GRAPH.LIST is excluded (no name argument; scatter-gather read, not owned by one shard). New tests in tests/sharded_multi_exec_locality.rs: - graph_leg_cross_shard_rejected: a KV key and graph name deterministically computed (not guessed, not random) to hash to different shards at shards=4 -- asserts CROSSSLOT and that neither leg applied. - graph_only_txn_routes_to_true_owner: a graph-only MULTI body commits and is visible via a FRESH, normally-routed connection across 12 distinct graph names -- proves owner-shard placement, not "whatever shard the writing connection happened to land on" (SO_REUSEPORT placement is not client-controllable, so this doesn't need to force which shard the writer lands on to prove correctness). Confirmed RED without the analyze_txn_locality fix (git stash the fix, rebuild, rerun): graph_leg_cross_shard_rejected instead applied the KV leg and errored "ERR graph not found" on the misrouted GRAPH.ADDNODE (proving it silently landed on the wrong shard's graph_store); graph_only_txn_routes _to_true_owner read back 0 rows on 1 of 12 graph names. Restored the fix, confirmed GREEN. Re-ran full gate set: - New locality tests: GREEN. Pre-existing KV-only locality tests (no_silent_divergence_across_shards, multi_shard_span_rejected, single_shard_unaffected) and sharded_multi_exec_routing.rs: unaffected, GREEN. - crash_matrix_cross_plane's 4 txn cells (committed x2 shard counts, atomicity x2 shard counts): GREEN 3x consecutive. - Full 42-cell crash-matrix default (green-only) run: 42/42 GREEN. - replication_graph.rs::replica_syncs_multi_exec_graph_leg: GREEN 3x consecutive (unaffected -- same-hashtag scenario, no locality change in its path). - lib tests (server::conn::shared::, incl. pre-existing txn_locality_tests module): 15/15 GREEN. - fmt --check: clean. - clippy --release -- -D warnings: clean. - clippy --no-default-features --features runtime-tokio,jemalloc -- -D warnings: clean. - cargo build --release --no-default-features --features runtime-tokio,jemalloc: clean. Files: - src/server/conn/shared.rs: analyze_txn_locality folds GRAPH.* name into the existing key-locality visit closure - tests/sharded_multi_exec_locality.rs: graph_leg_cross_shard_rejected + graph_only_txn_routes_to_true_owner + graph_row_count/ pick_kv_and_graph_on_different_shards helpers - CHANGELOG.md: Unreleased entry Refs: task #52, kernel M3 stage 3, PR #300 review round 3 (CodeRabbit) author: Tin Dang
Contributor
Author
|
CodeRabbit's shared.rs:318 finding was real — confirmed at source: Fixed in 91b28d6 by folding the graph name into the locality analyzer's existing |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/sharded_multi_exec_locality.rs (1)
435-479: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen shard-diversity guarantee for the owner-routing regression test.
The 12 candidate graph names aren't checked to actually span multiple shard owners via
graph_to_shard— the test's confidence that it exercises cross-shard owner routing (vs. the connection's own shard) relies entirely on probabilisticSO_REUSEPORTplacement. The sibling helper (pick_kv_and_graph_on_different_shards) explicitly favors deterministic computation over this kind of chance-based coverage for the same reason. Adding a cheap upfront assertion would remove that reliance for this critical regression guard.♻️ Proposed determinism check
fn graph_only_txn_routes_to_true_owner() { let Some(m) = spawn_moon("4") else { return }; + // Ensure these candidates actually span more than one shard owner — + // otherwise the test could pass without ever exercising cross-shard + // owner routing (only the connection's own shard). + let owners: std::collections::HashSet<usize> = (0..12) + .map(|i| graph_to_shard(format!("txnglocowner:{i}").as_bytes(), 4)) + .collect(); + assert!( + owners.len() > 1, + "candidate graph names must span multiple shard owners" + ); + for i in 0..12 {🤖 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 `@tests/sharded_multi_exec_locality.rs` around lines 435 - 479, Update graph_only_txn_routes_to_true_owner to compute each candidate graph’s owner shard with graph_to_shard and assert upfront that the 12 generated names span multiple shard owners, before running the transaction loop. Keep the existing fresh-connection readback and transaction assertions unchanged.
🤖 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.
Nitpick comments:
In `@tests/sharded_multi_exec_locality.rs`:
- Around line 435-479: Update graph_only_txn_routes_to_true_owner to compute
each candidate graph’s owner shard with graph_to_shard and assert upfront that
the 12 generated names span multiple shard owners, before running the
transaction loop. Keep the existing fresh-connection readback and transaction
assertions unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4c0928c9-9bc0-4243-9004-702c244101b5
📒 Files selected for processing (3)
CHANGELOG.mdsrc/server/conn/shared.rstests/sharded_multi_exec_locality.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/server/conn/shared.rs
tests/sharded_multi_exec_locality.rs imports graph_to_shard, which is cfg(feature = "graph") — the tokio CI matrix builds with --no-default-features --features runtime-tokio,jemalloc (no graph) and failed with E0432. Gate the import, the two graph helpers, and the two graph tests on the same feature. author: Tin Dang
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Kernel M3 stage 3, task #52 (P1, found by the G1 crash matrix): the graph leg of a committed cross-store MULTI/EXEC transaction (
SET+GRAPH.ADDNODEin one body) was lost on kill-9 while the KV leg survived.Root cause (worse than a durability gap):
execute_transaction_sharded— the EXEC executor shared by the sharded and monoio handlers — had noGRAPH.*branch at all. A queuedGRAPH.ADDNODEfell through to the generic KVdispatch()table, erroredERR unknown command, and MULTI/EXEC's per-command error tolerance swallowed it. The mutation never applied even in memory.Fix (2 commits)
GRAPH.*branch in the txn executor mirroring the live single-command path (try_handle_graph_command): dispatch againstShardSlice::graph_store, drain wal-v3 records, errored writes never reach the WAL.aof_entriesis the replication fanout source and graph records weren't in it). The executor now returns the graph records(db, bytes)— db bound per-entry at execution time (PR feat(replication): R1 real WAIT/ACK + consistency/durability fixes (silent replica drop, SELECT leak, AOF db attribution) #282 rule: a queued SELECT redirects later commands) — and each caller takes over:record_local_write_dbper record (gatednum_shards == 1 && replication_fanout_active, same scope + replicate-then-append order as the live path), thenwal_append.TxnExecutecross-shard hop:wal_appendonly (replication is monoio-only; the hop arm exists only atnum_shards > 1, outside graph-replication scope).Tests
cross_plane_prod_s{1,4}_txn_isolated_committedflip fromred_guard-gated RED to default-GREEN tripwires (42-cell suite: 33→35 GREEN by default).tests/replication_graph.rs::replica_syncs_multi_exec_graph_leg— shards=1 master+replica, MULTI/EXEC on master, both legs asserted on the replica; proven non-vacuous (disabling only the newrecord_local_write_dbcall makes it RED with the KV leg still landing).Gates
prod_s4_mq_isolatedtiming flake, 3/3 green in isolation).GRAPH.-prefix check inside the MULTI loop); the inline single-command KV path is untouched.Refs: task #52, kernel M3 stage 3, G1 crash matrix (#298), K2 (#299).
Summary by CodeRabbit
Bug Fixes
MULTI/EXECgraph command handling soGRAPH.*legs are dispatched correctly and no longer fall through to generic processing.MULTI/EXECgraph locality to route to the proper shard or reject withCROSSSLOTwhen ownership disagrees.Tests
MULTI/EXECcontainingGRAPH.*.