dragonflydb / dragonflydb/dragonfly
P1 — `NotifyPending()` is reentrant through a suspending expiry checker
- Dominant language
- C++
- Stars
- 31.5k
- Forks
- 1.3k
- Avg merge
- 1d 10h
- Merged PRs (30d)
- 137
Description
`NotifyPending()` iterates the live `awakened_indices_` / `awakened_keys` sets with no
reentrancy guard and no snapshot (`blocking_controller.cc:150-182`). The stream readiness
checker calls `FindReadOnly()` (`stream_family.cc:3032`), which can enter
`ExpireIfNeeded()` (`db_slice.cc:1451-1493`); that path can suspend **before** deleting
the entry — in `RecordExpiryBlocking()` when a replication journal is active
(`db_slice.cc:1471-1473`), or in keyspace-notification delivery under publish-buffer
backpressure (`db_slice.cc:1476-1484`, `channel_store.cc:120`). While the checker fiber is
suspended, any concluding transaction on the shard calls `NotifyPending()` again
(`transaction.cc:668`). The inner call clears and erases the containers the outer frames
still reference — including the `DbWatchTable` and `WatchQueue` held by reference — so the
outer call resumes on invalidated iterators and a stale `PrimeIterator`.
Consequences: lost wake, CHECK/DCHECK failure, use-after-free, duplicate erase.
Reachability: requires an active replication journal with a stalled streamer, or
`notify-keyspace-events Ex` with a backpressured subscriber. A default standalone
configuration has no suspension point on this path. The synchronous same-fiber lazy-expiry
case is benign and already covered by `XReadGroupBlockLazyExpireDuringWakeDoesNotCrash`;
the uncovered defect is the cross-fiber suspension window.
Scenario (race-sensitive):
```text
CONFIG SET notify-keyspace-events Ex
PSUBSCRIBE __keyevent@0__:expired # subscriber stops consuming replies
# fill the subscriber's output budget so an expired event must wait
XGROUP CREATE s g 0 MKSTREAM
XREADGROUP GROUP g c BLOCK 0 STREAMS s > # client A
PEXPIRE s 10 # client B
# after the TTL elapses, run another write on the same shard
SET unrelated-key value
```
A deterministic test needs a latch inside `RecordExpiryBlocking()` or the notification
send; a timing-only test would be flaky.
Contributor guide
Research direction
Read blocking_controller.cc:150-182 and trace the calls through stream_family.cc:3032, db_slice.cc:1451-1493, RecordExpiryBlocking(), channel_store.cc:120, and transaction.cc:668. Use the existing XReadGroupBlockLazyExpireDuringWakeDoesNotCrash test as a starting point, then add a deterministic latch at the suspension point and verify that reentrant NotifyPending() does not invalidate the outer iteration or lose wakeups.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- databases, distributed-systems
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 38/100