fix(replication): unified poison-record policy — decode failure kicks the link, never silent-skips (task #48)#315
Conversation
…ral apply planes Task #48: each replica-apply plane (graph WAL replay, MQ effect records, workspace create/drop, temporal invalidate, RESP framing) handled a malformed/undecodable record ad hoc — mostly warn+silently-skip-and-continue, and WS.CREATE.APPLY/WS.DROP.APPLY even swallowed the failure entirely (try_with_shard(...).is_some() returned true regardless of the inner decode outcome). A silent skip lets every subsequent record in the stream keep applying against state that has already diverged from the master, invisible until an operator notices missing data. Define one policy and apply it everywhere: a decode/parse failure on the live stream is protocol-level desync evidence, never silently skipped and never panicked on. Log loud (rate-limited to 1/sec), count it in a new INFO-visible counter, and signal the caller to drop the connection so the existing reconnect/resync loop renegotiates PSYNC — the only real recovery path. PSYNC snapshot install already failed the whole install on a malformed aux blob (fail-closed); it now also feeds the same counter. Semantic apply errors on well-formed records (WRONGTYPE, missing entity, etc.) keep the existing warn-and-continue posture, matching the generic KV dispatch path's long-standing behavior — that is data-level divergence from a legitimately-executed command, not stream corruption. - src/replication/apply.rs: new ApplyOutcome enum (Applied/Poisoned/ NoShardSlice) returned by apply_local; poison() helper (rate-limited warn + REPL_POISON_RECORDS_TOTAL counter); apply_graph/apply_mq/apply_ws_create/ apply_ws_drop/apply_temporal_invalidate now return bool instead of logging-and-swallowing; drain_replicated_commands' fatal RESP-parse path and load_snapshot's fail-closed aux-blob errors also feed the counter. - src/replication/replica.rs: both tokio and monoio stream-apply loops match on ApplyOutcome and drop the connection on Poisoned, same as the existing NoShardSlice / DrainResult::fatal paths. - src/command/connection.rs: INFO replication section gains replication_poison_records_total. - Tests (red/green): one per plane (graph/MQ/WS-create/WS-drop/temporal) feeding a corrupt record through apply_local, asserting Poisoned outcome + counter increment (+ untouched workspace registry for the WS case), plus a well-formed-record control asserting the counter does NOT advance. 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: 59 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 (4)
✨ 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 |
…roof test lock Under runtime-tokio,jemalloc (CI feature set, no graph) apply_graph and the TEMPORAL.INVALIDATE-AT arm are compiled out, so those records fall to generic dispatch (warn-and-continue) — the Poisoned contract only exists when the graph plane does. Also acquire POISON_TEST_LOCK via unwrap_or_else(into_inner) so one failing test no longer cascades poisoned-mutex panics into the other five. author: Tin Dang
…266e818802e # Conflicts: # CHANGELOG.md
…266e818802e # Conflicts: # CHANGELOG.md
2 participants
Task #48: one policy for malformed replicated records across graph/MQ/WS/temporal apply paths: decode failure = desync evidence → rate-limited loud log + new
replication_poison_records_totalINFO counter + connection drop so the reconnect/resync loop renegotiates PSYNC. Semantic apply errors on well-formed records keep warn-and-continue (data divergence ≠ stream corruption; line documented in module docs). Worst pre-fix case: WS create/drop swallowed poison entirely while reporting success.6 new red→green poison tests (per plane, asserting Poisoned outcome + counter delta + no state change; found and fixed a shared-atomic test race with a dedicated lock). Full lib 4268 green on VM, fmt+clippy both matrices clean. Refs task #48, PR #292-#294 planes.