fix(replication): deterministic replica TTL — absolute-rewrite + role-gated expiry (#71) - #335
Conversation
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: 55 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 (12)
📝 WalkthroughWalkthroughRelative expiry commands are rewritten to absolute deadlines before AOF and replication serialization. Replica active-expiry sweeps are disabled, and integration tests verify identical deadlines and master-driven removal convergence. ChangesReplica TTL semantics
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Master
participant LogSerializer
participant Replica
Client->>Master: issue relative-expiry command
Master->>LogSerializer: serialize command for log
LogSerializer-->>Master: absolute-deadline command bytes
Master->>Replica: replicate deadline command
Replica-->>Master: retain matching expiry deadline
Master->>Replica: stream authoritative key removal
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 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 |
21e9cce to
8b87b84
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
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/handler_monoio/mod.rs (1)
1-1: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy liftMove cross-shard AOF serialization to the target shard.
serialize_command_for_logis still called on the origin thread in both deferred-dispatch paths, so relative-expiry rewrites can use the sender’scurrent_time_ms()tick instead of the shard that actually applies the command. That makes the AOF / replication bytes for foreign-shard writes diverge from the deadline stored in memory.
src/server/conn/handler_monoio/mod.rs#L2085-L2107: don’t precomputeaof_bytesbefore the SPSC send; compute it from the exact frame/tick used by the target shard or after it applies the command.src/server/conn/handler_sharded/mod.rs#L1260-L1263: same issue on the remote-shard path.🤖 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/mod.rs` at line 1, Move serialize_command_for_log out of the origin-thread setup in both deferred cross-shard dispatch paths. Generate aof_bytes on the target shard using the exact command frame and current_time_ms() tick that applies the command, either immediately after application or within the target-side callback, and apply the same change to the remote-shard path so AOF/replication bytes match stored deadlines.
🤖 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/handler_sharded/mod.rs`:
- Around line 1260-1263: Move aof::serialize_command_for_log out of the
pre-dispatch calculation in the sharded handler and defer it until the command
executes on its owning shard. Ensure both local execution and the remote_groups
dispatch path derive aof_bytes from the target shard’s current frame/clock,
while preserving the existing is_write and metadata::is_persisted_write
conditions.
In `@tests/replication_ttl_semantics.rs`:
- Around line 224-236: Increase the TTL used by the “doomed” key in this
replication test from 1500ms to a more reliable 4000–5000ms value, keeping it
below the existing wait_until convergence timeout and preserving the
absolute-deadline assertion.
---
Outside diff comments:
In `@src/server/conn/handler_monoio/mod.rs`:
- Line 1: Move serialize_command_for_log out of the origin-thread setup in both
deferred cross-shard dispatch paths. Generate aof_bytes on the target shard
using the exact command frame and current_time_ms() tick that applies the
command, either immediately after application or within the target-side
callback, and apply the same change to the remote-shard path so AOF/replication
bytes match stored deadlines.
🪄 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: 7b8e39a2-f511-434a-837d-2637ce252d85
📒 Files selected for processing (12)
CHANGELOG.mdsrc/persistence/aof/mod.rssrc/replication/expire_rewrite.rssrc/replication/mod.rssrc/server/conn/handler_monoio/mod.rssrc/server/conn/handler_monoio/txn.rssrc/server/conn/handler_sharded/mod.rssrc/server/expiration.rssrc/server/listener.rssrc/shard/event_loop.rssrc/shard/timers.rstests/replication_ttl_semantics.rs
| let is_write = if ctx.aof_pool.is_some() || conn.tracking_state.enabled { metadata::is_write(cmd) } else { false }; | ||
| // `is_persisted_write`: never AOF a literal client SELECT | ||
| // (task #35 — poisons the stream db context). | ||
| let aof_bytes = if is_write && ctx.aof_pool.is_some() && metadata::is_persisted_write(cmd) { Some(aof::serialize_command(&frame)) } else { None }; | ||
| let aof_bytes = if is_write && ctx.aof_pool.is_some() && metadata::is_persisted_write(cmd) { Some(aof::serialize_command_for_log(&frame)) } else { None }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
Same cross-shard clock-mismatch risk as the monoio handler's deferred dispatch path.
aof_bytes is computed here from frame (via serialize_command_for_log) before the is_local/cross-shard branch is even decided. When the target is a foreign shard (line ~1935, remote_groups.entry(target)...push((..., aof_bytes, ...))), this precomputed absolute deadline was derived from the origin thread's clock at enqueue time, while the command itself is actually applied later on the target shard's own thread/tick using its own clock. This can make the value logged to AOF / streamed to replicas diverge from what the target shard actually stores.
🤖 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_sharded/mod.rs` around lines 1260 - 1263, Move
aof::serialize_command_for_log out of the pre-dispatch calculation in the
sharded handler and defer it until the command executes on its owning shard.
Ensure both local execution and the remote_groups dispatch path derive aof_bytes
from the target shard’s current frame/clock, while preserving the existing
is_write and metadata::is_persisted_write conditions.
| // A key that lives long enough to reach the replica, then expires. | ||
| send_cmd(master_addr, "SET doomed v PX 1500"); | ||
| assert!( | ||
| wait_until(Duration::from_secs(5), || get(replica_addr, "doomed") | ||
| .is_some()), | ||
| "replica never received the key before expiry" | ||
| ); | ||
| // Both carry the same absolute deadline. | ||
| assert_eq!( | ||
| send_int(master_addr, "PEXPIRETIME doomed"), | ||
| send_int(replica_addr, "PEXPIRETIME doomed"), | ||
| "absolute deadline diverged between master and replica" | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Tight TTL margin risks test flakiness.
PX 1500 gives only 1.5s for the key to reach the replica before get(replica_addr, "doomed") starts returning None due to logical expiry (#71b). Under CI load/slow replication startup this can produce a false "replica never received the key before expiry" failure even when replication is healthy. Consider a larger TTL (e.g. 4000-5000ms) to give a comfortable propagation margin while still completing well within wait_until's later 8s convergence timeout.
🤖 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/replication_ttl_semantics.rs` around lines 224 - 236, Increase the TTL
used by the “doomed” key in this replication test from 1500ms to a more reliable
4000–5000ms value, keeping it below the existing wait_until convergence timeout
and preserving the absolute-deadline assertion.
…-gated expiry (#71) Closes the replica-TTL caveat disclosed in the v0.7.0 tag. Two coordinated changes make key expiry deterministic across a master/replica pair (and across a master restart from the AOF). Relative-expiry commands are rewritten to absolute deadlines before they enter the durable log AND the replication stream, via a pure transform (`replication::expire_rewrite::rewrite_expire_for_propagation`) applied through a new `aof::serialize_command_for_log` wrapper at every KV write-propagation site (monoio local + cross-shard, sharded, txn): EXPIRE/PEXPIRE -> PEXPIREAT key <abs_ms> SETEX/PSETEX -> SET key value PXAT <abs_ms> SET ... EX/PX -> SET ... PXAT <abs_ms> (other opts preserved) GETEX ... EX/PX -> PEXPIREAT key <abs_ms> The deadline uses the master's per-tick cached `current_time_ms()` — the exact value the command handler stored (same shard thread, same tick -> same TL_NOW_MS) — so the replica reproduces the master's expiry INSTANT instead of restarting the countdown at apply time. Already-absolute forms (PEXPIREAT, EXPIREAT, EXAT, PXAT, PERSIST) and past-time deletes propagate verbatim. A replica no longer runs its own active-expiry deletion sweep. Both the monoio shard tick (`timers::run_active_expiry`) and the tokio background task (`expiration::run_active_expiration`) now read the lock-free `is_replica_mirror` and skip the sweeps while attached to a master; logical expiry on reads still applies. The replica keeps a logically-expired key resident until the master streams the authoritative DEL, so both nodes remove a key at the same point in the stream instead of racing independent TTL sweeps. (The tokio expiration task is now spawned after `repl_state` exists so the mirror is live from tick one.) Tests (red/green TDD) - 16 unit tests for the pure rewrite (every form + case-insensitivity, non-positive/overflow/malformed verbatim fallback, option preservation, value-looks-like-option guard). - 2 black-box integration tests (tests/replication_ttl_semantics.rs): * relative TTL replicates as an IDENTICAL absolute deadline (master vs replica PEXPIRETIME exact-equality — the #71a discriminator); * an expiring key converges via the master's decision (#71a+#71b). - Regression: AOF lib 88/88, key 67/67, expiration 5/5, streaming replication 7/7 — all green. fmt + lib clippy clean on both feature sets. author: Tin Dang <tindang.ht97@gmail.com>
8b87b84 to
f5e1156
Compare
2 participants
Deterministic replica TTL semantics (#71)
Closes the replica-TTL caveat disclosed in the v0.7.0 tag. Two coordinated changes make key expiry deterministic across a master/replica pair and across a master restart from the AOF.
#71a — master-side absolute rewrite
Relative-expiry commands are rewritten to absolute deadlines before they enter the durable log and the replication stream:
EXPIRE/PEXPIREPEXPIREAT key <abs_ms>SETEX/PSETEXSET key value PXAT <abs_ms>SET … EX/PXSET … PXAT <abs_ms>(other options preserved)GETEX … EX/PXPEXPIREAT key <abs_ms>A pure transform (
replication::expire_rewrite::rewrite_expire_for_propagation) applied through a newaof::serialize_command_for_logwrapper at every KV write-propagation site. The absolute deadline uses the master's per-tick cachedcurrent_time_ms()— the exact value the command handler stored (same shard thread, same tick → sameTL_NOW_MS) — so a replica (or an AOF replay after restart) reproduces the master's expiry instant instead of restarting the countdown at apply/replay time. Already-absolute forms (PEXPIREAT/EXPIREAT/EXAT/PXAT/PERSIST) and past-time deletes propagate verbatim.#71b — role-gated active expiry
A replica no longer runs its own active-expiry deletion sweep — both the monoio shard tick (
timers::run_active_expiry) and the tokio background task (expiration::run_active_expiration) read the lock-freeis_replica_mirrorand skip the sweeps while attached to a master. Logical expiry on reads still applies; the replica keeps a logically-expired key resident until the master streams the authoritativeDEL, so both nodes remove a key at the same point in the stream instead of racing independent TTL sweeps.Tests (red/green TDD)
tests/replication_ttl_semantics.rs):PEXPIRETIMEexact-equality — the #71a discriminator: verbatim relative replication would drift by the apply delay);cargo fmt --check+ libclippy -D warningsclean on default andruntime-tokio,jemalloc.Docs
The v0.7.0
clustering.mdwarning already discloses this as "lands in v0.7.1" — a follow-up doc pass will flip that note to shipped once this lands.Closes #71 (#71a + #71b).
Summary by CodeRabbit