fix(server): skip evicting keys in buckets a full sync hasn't capture…

[Shikha-code36]

Fixes a race between heartbeat/policy-based eviction and full sync that lets a replica retain a key the master has already evicted.

Root cause: DbSlice::FreeMemWithEvictionStepAtomic deletes evicted keys via Del() without going through the CoW OnChange hook (PreUpdateBlocking/CallChangeCallbacks) — that hook can't be called here because eviction runs under FiberAtomicGuard, which disallows the hook's blocking call. Normally, OnChange is what protects an in-progress full-sync snapshot: if a bucket hasn't been serialized yet, it forces the bucket's pre-mutation state to be captured before the mutation is applied.

Without that protection, eviction can delete a key whose bucket a full-sync snapshot hasn't visited yet, or whose value is currently mid-flight across multiple chunks (large values get split across several PushToConsumerIfNeeded calls). The eviction's journal DEL is written as untagged raw bytes into the same output stream and can land between two chunks of that key's own still-in-progress baseline entry. On the replica, RDB_OPCODE_JOURNAL_BLOB entries are applied immediately in stream order (rdb_load.cc), so the DEL becomes a no-op (the key hasn't finished loading yet) — and once the remaining chunks arrive and the value is fully reassembled, the key gets inserted, resurrecting a key the master no longer has.

test_heartbeat_eviction_propagation (#8090) reproduces this directly: it populates 1MB values (DEBUG POPULATE 233 size 1048576) against a 300KB serialization chunk size, guaranteeing multi-chunk transmission for the values in play.

test_policy_based_eviction_propagation (#7925) shows the same failure signature (replica retains a key master evicted) — plausibly the same underlying gap, though I haven't independently confirmed its value sizes hit the identical multi-chunk condition as #8090.

Fix

Before evicting a candidate key, skip it if any currently-registered full-sync consumer either:

• hasn't yet serialized that key's bucket (evict_it.GetVersion() < cb->snapshot_version_), or
• is currently mid-flight serializing some bucket (cb->IsAnyBucketBlocked())

Both checks reuse existing non-blocking infrastructure (DbSlice::change_cb_, ChangeConsumerInterface::IsAnyBucketBlocked()) and add no locking or yielding, so they're safe to call under the existing FiberAtomicGuard. Skipped keys remain eligible for eviction on the next heartbeat tick once the snapshot has moved past them — this doesn't reduce total eviction throughput, only defers eviction of specific keys that are momentarily in the race window.

Test plan

• test_heartbeat_eviction_propagation — 5/5 passes locally with the fix (previously failing intermittently in CI, see test_heartbeat_eviction_propagation #8090)
• test_policy_based_eviction_propagation — 5/5 passes locally with the fix (previously failing intermittently in CI, see test_policy_based_eviction_propagation #7925)
• Not stress-tested: sustained full sync + heavy concurrent memory pressure, to confirm eviction doesn't stall behind a slow-syncing replica for longer than acceptable

Notes for reviewers

This is a targeted mitigation, not the full fix implied by the snapshot.cc comment referencing a "delayed deletion queue proposal" design doc — that would presumably address the same class of gap more comprehensively (e.g. for expiry too, not just eviction). Flagging in case this should be superseded by or coordinated with that design work rather than merged as a standalone patch.

Fixes #8090
Fixes #7925

[qodo-code-review]

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Fix eviction/full-sync snapshot race by skipping unsafe keys during eviction

🐞 Bug fix 🕐 20-40 Minutes

---

AI Description

• Skip eviction for keys whose buckets aren’t yet captured by any full-sync snapshot.
• Avoid deleting keys while snapshots are mid-flight serializing large, multi-chunk values.
• Prevent replica key resurrection from journal DEL interleaving with baseline streaming.

Diagram

sequenceDiagram
  participant Evict as "Heartbeat eviction"
  participant Slice as "DbSlice"
  participant Cbs as "Snapshot consumers"
  participant Snap as "Full-sync serializer"
  participant Repl as "Replica loader"

  Evict->>Slice: "FreeMemWithEvictionStepAtomic()"
  Slice->>Cbs: "Check bucket/version + IsAnyBucketBlocked()"
  alt "Any snapshot at risk"
    Slice-->>Evict: "Skip key (defer eviction)"
  else "Safe to evict"
    Slice->>Slice: "Del()"
    Snap-->>Repl: "Baseline stream (possibly multi-chunk)"
    Slice-->>Repl: "Journal DEL (ordered after safe point)"
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Deferred deletion queue for snapshot safety

• ➕ Guarantees ordering between baseline serialization and deletes for all mutation sources (eviction/expiry/etc.)
• ➕ Avoids per-candidate eviction scanning of snapshot consumers
• ➖ Larger design/implementation effort and more moving parts
• ➖ Requires careful memory/backpressure handling for queued deletions

2. Make eviction go through a non-blocking OnChange path

• ➕ Keeps snapshot correctness centralized in CoW/change-hook mechanism
• ➕ Reduces special-casing in eviction
• ➖ Hard under FiberAtomicGuard constraints (no blocking/yielding)
• ➖ May still need buffering to prevent interleaving with chunked streaming

3. Replica-side buffering of journal ops until baseline key completion

• ➕ Addresses this class of ordering issues even if producers interleave output
• ➕ Could be generalized to other opcodes
• ➖ Increases replica loader complexity and memory usage
• ➖ Requires precise “key complete” boundaries for chunked values

Recommendation: Merge this targeted mitigation: it is minimal, non-blocking under FiberAtomicGuard, and directly addresses the observed resurrection race by deferring only unsafe candidates. Track the broader “delayed deletion queue” (or equivalent) separately as a more comprehensive correctness mechanism across all delete-like mutations (eviction, expiry, etc.).

Files changed (1) +18 / -0

Bug fix (1) +18 / -0

db_slice.ccDefer eviction of keys unsafe during full-sync snapshot streaming +18/-0

Defer eviction of keys unsafe during full-sync snapshot streaming

• Adds a pre-eviction guard that scans registered change consumers and skips candidates if any full-sync snapshot has not yet reached the key’s bucket (version check) or is mid-flight serializing any bucket (blocked check). This prevents eviction-driven DEL entries from racing ahead of still-streaming baseline data and resurrecting keys on replicas.

src/server/db_slice.cc

[augmentcode]

🤖 Augment PR Summary

Summary: Prevents cache eviction during full-sync windows where a replica snapshot has not yet covered a bucket or is streaming a bucket in multiple chunks.

Technical Notes: The eviction loop now examines registered snapshot consumers and defers affected candidates so the baseline entry remains ordered before its journaled deletion.

_{🤖 Was this summary useful? React with 👍 or 👎}

[augmentcode]

Review completed. No suggestions at this time.

Comment augment review to trigger a new review at any time.

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

---

🟠 Medium

1. Eviction blocked by callbacks 🐞 Bug ☼ Reliability

Description

FreeMemWithEvictionStepAtomic now skips eviction when any registered ChangeConsumerInterface has
snapshot_version_ ahead of the candidate bucket version, but RegisterOnChange assigns
snapshot_version_ to every consumer (including FlushSlots' one-shot CallbackConsumer). While
FlushSlots is running (it yields between bucket traversals), this can broadly suppress heartbeat
eviction and delay maxmemory enforcement until the consumer is unregistered.

Code

src/server/db_slice.cc[R1726-1729]

+        for (auto* cb : change_cb_) {
+          if (evict_it.GetVersion() < cb->snapshot_version_ || cb->IsAnyBucketBlocked()) {
+            snapshot_race_risk = true;
+            break;

Evidence

The new eviction loop consults every change_cb_ entry and skips eviction if the bucket version is
behind snapshot_version_. FlushSlots registers a CallbackConsumer via RegisterOnChange and
keeps it registered while a long-running fiber yields between bucket traversals; since
RegisterOnChange assigns snapshot_version_ = NextVersion() unconditionally and bucket versions
are typically lower unless recently mutated, most eviction candidates will be skipped during that
window even though FlushSlots is not a full-sync serializer.

src/server/db_slice.cc[1717-1734]
src/server/db_slice.cc[935-990]
src/server/db_slice.cc[1002-1052]
src/server/db_slice.cc[1542-1557]
src/server/db_slice.h[100-117]
src/server/db_slice.h[625-627]
src/server/db_slice.cc[875-879]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`FreeMemWithEvictionStepAtomic` defers eviction based on `cb->snapshot_version_` / `cb->IsAnyBucketBlocked()` for *all* entries in `change_cb_`. However, `change_cb_` includes non-snapshot, non-streaming consumers such as `FlushSlots`' callback consumer; because `RegisterOnChange` assigns a fresh `snapshot_version_`, most buckets will compare older and eviction will be skipped for much of the keyspace while that consumer is registered.

## Issue Context
- `FlushSlotsFb` runs in a fiber, yields between bucket traversals, and keeps the change-consumer registered until the end — so heartbeat eviction can run concurrently while the consumer remains registered.
- The snapshot-version based guard is only appropriate for consumers whose output stream can interleave a key’s baseline serialization with journal mutations (i.e., full-sync/snapshot serializers), not one-shot mutation callbacks.

## Fix Focus Areas
- src/server/db_slice.cc[1717-1734]
- src/server/db_slice.cc[935-990]
- src/server/db_slice.cc[1002-1052]
- src/server/db_slice.cc[1542-1557]
- src/server/db_slice.h[100-117]

## Suggested fix direction
Introduce an explicit marker on `ChangeConsumerInterface` indicating whether the consumer represents a snapshot/full-sync serializer that needs eviction deferral (e.g., `bool is_snapshot_consumer_` default false).
- Set it to true in `SerializerBase::RegisterChangeListener` (and any other snapshot/full-sync serializers).
- Leave it false for `CallbackConsumer`/FlushSlots.
- In `FreeMemWithEvictionStepAtomic`, only apply the new `snapshot_version_` / `IsAnyBucketBlocked()` checks to consumers with that marker enabled.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

---

Context

✅ Tickets: 🎫 test_policy_based_eviction_propagation 🎫 test_heartbeat_eviction_propagation

✅ Cross-repo context — repo relationships

---

Tip of the day

💡 Did you know, you can keep summaries lean with Finding overflow, which tucks the rest behind 'View more'

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

---

Powered by Qodo