fix(generic): stop RM from freeing a container under a yielding iteration

[vyavdoshenko]

RM deletes keys outside the transaction framework, so nothing serializes it against a command that is suspended mid-iteration. Container iteration yields every --container_iteration_yield_interval_usec (500 usec by default) while holding raw pointers into the value, so RM can free the container under the running iterator: an lpAssertValidEntry abort in debug builds, a silent use-after-free in release.

Reproducible example:

Plain main, one proactor thread, no special flags:

conn A:  RPUSH mylist <20000 elements>
conn B:  SORT mylist ALPHA            # long enough to yield; do not read the reply
conn C:  RM 0 MATCH mylist COUNT 10   # while B is still iterating

SIGABRT: __assert_fail <- lpAssertValidEntry <- lpNext <- QList::Iterator::Next
         <- QList::Iterate <- container_utils::IterateList <- OpFetchSortEntries <- SortGeneric

LRANGE and SORT_RO work as the reader too, so the victim side needs no write permission.

Found here:
https://github.com/dragonflydb/dragonfly/actions/runs/33478201692/job/99761833826
and
https://github.com/dragonflydb/dragonfly/actions/runs/33416302935/job/99567646361

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

PR Summary by Qodo

Prevent RM deletion during yielding container iteration

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

---

AI Description

• Track yielding container iterations with a thread-local nested lifetime guard.
• Defer RM deletions while iterators may retain raw container pointers.
• Add a concurrent SORT_RO and RM regression test for the use-after-free.

Diagram

sequenceDiagram
  actor Reader as Reader Command
  participant Iterator as Container Iterator
  participant Guard as Iteration Guard
  actor RM as RM Command
  participant DB as DB Slice
  Reader->>Iterator: Iterate container
  Iterator->>Guard: Enter guarded scope
  Iterator-->>Iterator: Yield fiber
  RM->>Guard: Check in-flight
  alt Iteration active
    Guard-->>RM: Active
    RM-->>RM: Defer deletion
  else No iteration
    Guard-->>RM: Inactive
    RM->>DB: Delete key
  end
  Iterator->>Guard: Exit guarded scope
  Iterator-->>Reader: Return entries

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Serialize RM through transactions

• ➕ Provides key-level coordination with readers and writers.
• ➕ Avoids conservatively delaying unrelated deletions on the same thread.
• ➖ Requires a broader redesign of RM's incremental cross-shard scan flow.
• ➖ May add transaction and locking overhead to bulk deletion.

2. Pin individual container values

• ➕ Protects only the value currently being iterated.
• ➕ Could permit unrelated RM deletions during yielded iterations.
• ➖ Requires invasive ownership or reference-counting changes to PrimeValue storage.
• ➖ Expands lifetime-management complexity across every yielding iterator.

Recommendation: Use the thread-local iteration guard as the focused, low-overhead fix: it matches the thread-affine fiber execution model and covers all shared yielding iterators. Transactional RM or per-value pinning could provide finer-grained concurrency, but their complexity is disproportionate to this targeted lifetime hazard.

Files changed (4) +70 / -0

Bug fix (3) +31 / -0

container_utils.ccTrack active container iterations with an RAII guard +19/-0

Track active container iterations with an RAII guard

• Adds a thread-local nesting counter and RAII guard around list, set, sorted-set, and map iteration. Exposes whether any guarded iteration is active, including while its fiber is yielded.

src/server/container_utils.cc

container_utils.hExpose active container iteration state +5/-0

Expose active container iteration state

• Declares IsIterationInFlight and documents why non-transactional value deletion must consult it before freeing container storage.

src/server/container_utils.h

generic_family.ccDefer RM deletion during active iteration +7/-0

Defer RM deletion during active iteration

• Checks the thread-local iteration state before deleting scanned keys. RM stops the current deletion batch when an iterator may still hold raw pointers into a container.

src/server/generic_family.cc

Tests (1) +39 / -0

generic_family_test.ccReproduce concurrent SORT_RO and RM lifetime race +39/-0

Reproduce concurrent SORT_RO and RM lifetime race

• Adds a multi-fiber regression test that repeatedly races a yielding SORT_RO over a large list against RM. The test verifies the server remains responsive instead of aborting or accessing freed storage.

src/server/generic_family_test.cc

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

Code Review by Qodo

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

---

🔴 High

1. RM permanently skips keys 🐞 Bug ≡ Correctness

Description

OpScanAndDelete advances the scan cursor before the new iteration check, then discards the scanned
key batch when it breaks without deleting. RM can therefore return cursor 0 while matching keys
remain, rather than leaving them for a later call as the comment claims.

Code

src/server/generic_family.cc[R793-794]

+    if (container_utils::IsIterationInFlight())
+      break;

Evidence

OpScan traverses matches and stores its resulting cursor before returning. OpScanAndDelete then
checks the thread-local guard and breaks, discarding its local keys vector while retaining that
advanced cursor; RmGeneric subsequently returns or continues from it, with no mechanism to replay
the skipped batch. The new test only joins both fibers and checks PING, so it does not verify that
RM deleted the key.

src/server/generic_family.cc[688-720]
src/server/generic_family.cc[778-803]
src/server/generic_family.cc[824-855]
src/server/generic_family_test.cc[2519-2532]

Agent prompt

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

## Issue description
When an iteration is in flight, `OpScanAndDelete` breaks only after `OpScan` has advanced the cursor and collected matching keys. The undeleted batch is discarded, so later RM calls resume beyond those keys and may report completion while they still exist.

## Issue Context
Ensure RM waits or retries safely without losing scan position or signaling cursor completion. Extend the concurrent regression test to verify the target key is eventually deleted and the response cursor/deletion count remain consistent.

## Fix Focus Areas
- src/server/generic_family.cc[778-855]
- src/server/generic_family_test.cc[2496-2533]

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

---

---

Context sources

✅ Cross-repo context — repo relationships

  Explored: repo: romange/helio (sha: 7ea50945)

---

Tip of the day

💡 Did you know, you can describe a rule in plain language on the Rules page and Qodo drafts it for you

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

---

Powered by Qodo

[augmentcode]

🤖 Augment PR Summary

Summary: This PR prevents RM from freeing containers while a yielding reader is iterating them.

Changes:

• Adds a thread-local in-flight depth guard for list, set, sorted-set, and map iteration helpers.
• Checks that guard in RM's scan-and-delete path and defers deletion when an iteration is active.
• Adds a regression test that races SORT_RO against RM on a large list.

Technical notes: The guard is scoped to a proactor thread so it remains visible while a fiber is suspended at an iteration yield point.

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

[augmentcode]

Review completed. 3 suggestions posted.

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

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

---

🔴 High

1. Expiry bypasses iteration guard 🐞 Bug ☼ Reliability

Description

FindMutable() may expire and delete the key before IsIterationInFlight() is checked, including
after blocking for expiry journaling and allowing another iterator to suspend. RM can therefore
still free an expired container beneath an active iterator, preserving the use-after-free this PR
intends to fix.

Code

src/server/generic_family.cc[R793-794]

+    if (container_utils::IsIterationInFlight())
+      break;

Relevance

●●● Strong

Accepted history flags lazy expiry and deletion during iteration as iterator-invalidating
reliability bugs.

PR-#7162
PR-#7163

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

RM invokes FindMutable before the newly added guard. FindInternal routes expired entries through
ExpireIfNeeded, and that function performs Del before returning an invalid iterator, so
execution never reaches the guard before the container is freed.

src/server/generic_family.cc[785-796]
src/server/db_slice.cc[620-638]
src/server/db_slice.cc[655-688]
src/server/db_slice.cc[1465-1506]

Agent prompt

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

## Issue description
RM checks for an active container iteration only after `FindMutable()`, but that lookup can expire and delete the key internally. Ensure no RM lookup path can free a value while an iterator is suspended, including when expiry journaling blocks and permits another fiber to run.

## Issue Context
`FindMutable()` reaches `ExpireIfNeeded()`, which records expiry and calls `DbSlice::Del()` before control returns to `OpScanAndDelete`. A simple post-lookup check therefore cannot protect this deletion; preserve atomicity or use a non-expiring lookup and perform deletion only after the guard.

## Fix Focus Areas
- src/server/generic_family.cc[785-796]
- src/server/db_slice.cc[655-688]
- src/server/db_slice.cc[1465-1502]

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

---

2. Blocked batch keys are skipped 🐞 Bug ≡ Correctness

Description

When the guard breaks this loop, OpScan() has already advanced the cursor past the entire returned
batch. The next RM call starts from that advanced cursor, so the current and remaining undeleted
keys are skipped instead of being left for a later call as intended.

Code

src/server/generic_family.cc[R793-794]

+    if (container_utils::IsIterationInFlight())
+      break;

Relevance

●●● Strong

Accepted history supports fixing traversal-state misalignment and skipped work after loop control
changes.

PR-#6492
PR-#6970

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

OpScanAndDelete scans and mutates *cursor before iterating keys. OpScan writes the
traversal's resulting token into that cursor, and RmGeneric subsequently encodes and returns it
without retaining the unprocessed keys or restoring the batch's incoming position.

src/server/generic_family.cc[704-721]
src/server/generic_family.cc[778-802]
src/server/generic_family.cc[824-855]

Agent prompt

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

## Issue description
Do not return an advanced scan cursor when iteration protection prevents processing the remainder of a scanned batch. Ensure all undeleted keys remain reachable by the cursor returned to the caller.

## Issue Context
`OpScan` updates `*cursor` before the deletion loop starts. If the loop breaks, RM currently retains that cursor and permanently advances beyond the unprocessed batch; restoring the batch's incoming cursor is one possible approach, with already-deleted keys safely ignored when revisited.

## Fix Focus Areas
- src/server/generic_family.cc[778-802]
- src/server/generic_family.cc[824-855]

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

---

---

Context sources

✅ Cross-repo context — repo relationships

Review mode: ⚖️ Balanced

---

Tip of the day

💡 Did you know, you can describe a rule in plain language on the Rules page and Qodo drafts it for you

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

---

Powered by Qodo

[Copilot]

Pull request overview

This PR addresses a concurrency use-after-free where RM can delete a container key while another command is suspended mid-container-iteration (which holds raw pointers into the value across yields), leading to debug aborts or release UAF.

Changes:

• Add a per-thread “container iteration in flight” guard in container_utils to detect when a fiber is suspended inside Iterate* helpers.
• Gate RM deletions on that guard to avoid freeing container memory while an iterator may resume.
• Add a regression test that races SORT_RO (yielding iteration) with RM to reproduce the prior crash.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
src/server/generic_family.cc	Adds RM-path check intended to avoid deleting keys while container iteration is in-flight.
src/server/generic_family_test.cc	Adds regression test for RM vs yielding container iteration.
src/server/container_utils.h	Declares IsIterationInFlight() API for iteration state detection.
src/server/container_utils.cc	Implements iteration-depth tracking via an RAII guard in all Iterate* helpers.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[kostasrim]

You can just write an one liner:

 if (!db_slice.CheckLock(IntentLock::EXCLUSIVE, op_args.db_cntx.db_index, key)) 
    continue;

which we already do this pattern in heartbeat if I remember correctly. Adjust the check accordingly (see my other comment)

[kostasrim]

I suspect you don't need any of these and the fix is an one liner:

 if (!db_slice.CheckLock(IntentLock::EXCLUSIVE, op_args.db_cntx.db_index, key)) 
    continue;

(adjust it to cover both lock types) that way we skip those keys that are already locked by the transaction layer.

[kostasrim]

and that way we don't skip the command...

[vyavdoshenko]

Tried it verbatim, it still aborts. RmDuringContainerYield gives SIGABRT 3/3 with the same stack as the fuzzer (lpAssertValidEntry -> lpFirst -> QList::Iterate).

The lock table is empty for exactly the commands that crash: a single-shard, single-hop command runs optimistically and skips registration (transaction.cc:1293), and the lazy registration is done by the next transaction scheduled on the shard. RM is never one: it has no keys (generic_family.cc:2979) and runs through ess->Await() with a null transaction. Visible without any patch: DEBUG OBJECT on a list while SORT_RO walks it reports no lock.

The heartbeat precedent (db_slice.cc:1624) holds because a blocked client does hold a registered lock. SORT/LRANGE do not. And Check(EXCLUSIVE) already covers shared locks (intent_lock.h:29), so the mode is not the gap.

continue also would not be enough: OpScan has already advanced the cursor, so skipping loses the key, and RM could answer cursor 0 with it still there.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.