fix(replication): reject writing EVAL/EVALSHA on read-only replicas at the redis.call bridge (task #38)#317
Conversation
…task #38) EVAL/EVALSHA carry no WRITE command-metadata flag (by design — a script's writes replicate as per-call effect records, not as the literal EVAL), so the connection-level try_enforce_readonly gate never saw them: a replica executed a client-issued script's redis.call('SET', ...) unconditionally, silently diverging from the master. Fix the actual write boundary instead of the command dispatch: make_redis_ call_fn (the redis.call/redis.pcall bridge in src/scripting/bridge.rs) now rejects a WRITE-flagged inner command with -READONLY the moment the shard's ReplicationState reports the replica role, matching upstream Redis (a non-writing script still runs to completion on a replica; a writing one aborts at the first offending call). LuaEvictionCtx gained an is_replica_mirror snapshot (same lock-free AtomicBool pattern as ConnectionContext::is_replica_mirror) so a tight script write-loop never takes the ReplicationState RwLock. Reuses the WS/MQ/GRAPH.QUERY read-only- subcommand carve-outs the connection-level gate already has. Verified master-side Lua effect replication (task #34 Wave A part 2) is unaffected: replayed effects apply via replication::apply::apply_local, which dispatches straight into storage and never runs a Lua VM, so it can never reach — or be blocked by — this new gate (eval_effects_parity_shards1/_shards4, eval_incr_no_double_apply, eval_writes_survive_restart all green). Multi-db: redis.call('SELECT', ...) was already rejected inside scripts (pre-existing fail-loud guard, CURRENT_DB is pinned to one Database per script execution) — no per-script db-switching path exists that could interact with this change. Gates: cargo fmt --check, cargo clippy -D warnings (default + tokio/ jemalloc matrices), cargo test --lib scripting:: (59 tests), new tests/replication_readonly_eval.rs against a real release binary (monoio), tests/replication_readonly_ws_mq.rs + tests/replication_planes.rs eval scenarios re-verified green. 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: 58 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 (3)
✨ 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 |
…245fdfa8724 # Conflicts: # CHANGELOG.md
…245fdfa8724 # Conflicts: # CHANGELOG.md
2 participants
Task #38: replicas ran client scripts'
redis.call('SET',…)unconditionally (EVAL carries no WRITE flag by design — script writes replicate as effect records, so the connection-level READONLY gate never saw them). Fix at the real write boundary: the sharedredis.call/redis.pcallbridge checks a newLuaEvictionCtx::is_replica()(atomic mirror snapshotted from ReplicationState — one Acquire load per call, no lock, tracks live REPLICAOF role flips) and returns-READONLYbefore the write executes. Redis-parity: read-only scripts still run; first write attempt aborts. Same WS/MQ/GRAPH.QUERY carve-outs as the connection gate.Master Lua-effects replication is untouched (replay dispatches storage-direct, never a Lua VM) — verified by re-running all 4 eval replication-plane tests. New black-box test proves READONLY rejection + no partial write + read-only EVAL success. Multi-db: SELECT-in-script already rejected pre-existing; no gap. fmt+clippy both matrices, scripting lib 59 green. Refs task #38, PR #285/#292.