dragonflydb / dragonflydb/dragonfly

Experimental list-node tiering: memory-accounting drift, wrong data, aborts

Open
#7,963 2 comments 0 reactions 0 assignees View on GitHub
bug
Dominant language
C++
Stars
31.5k
Forks
1.3k
Avg merge
1d 10h
Merged PRs (30d)
137

Description

Reachable only with **both** `--tiered_experimental_list_support` (default `false`) and `--list_tiering_threshold != 0` (default `0`), it fails CI on unrelated PRs.

---
Disabled tests to re-enable:

Both live in `tiered_storage_test.cc`:

| test | disabled by | why |
|---|---|---|
| `ListNodeTieringTest.RPopStashedNodes` | [#7962](https://github.com/dragonflydb/dragonfly/pull/7962) | problems 1 and 3 below |
| `PureDiskTSTest.MGETParallel` | earlier | flaky concurrent disk reads on squashed GET/MGET; not one of the problems below |

`ListNodeTieringTest.MoveWhileNodesPending` is still enabled and fails ~1/20 locally (problem 3).

---
Problems:

| # | Problem | Effect |
|---|---|---|
| 1 | `DCHECK_GE(obj_memory_usage, MallocUsed())` aborts in `DbSlice::FindMutableInternal` | debug abort |
| 2 | `object_used_memory` grows by one node's size per node load from a mutable command and never returns to 0 | silent, unbounded |
| 3 | `RPOP` returns another node's value; `lpAssertValidEntry` abort inside `QList::Pop` | data loss / crash |
| 4 | per-slot `memory_bytes` strands the sum of offloaded node bytes after `DEL` | silent, permanent |
| 5 | `CleanupListNode` cancels a load but leaves the entry in `pending_reads_`, so `NotifyFetched` still uploads into a freed node | potential UAF, **unverified** |

---
1, 2 - `AutoUpdater` double-applies tiered deltas

`DbSlice::AutoUpdater` accounts by snapshot: it records the value's `MallocUsed()` on construction
and applies `current − snapshot` on `Run()`. The list path **blocks inside that window** -
`OpPop` -> `QList::Pop` -> `QList::Materialize` -> a blocking tiered read. `QList::DelNode` adds two
more suspension points, because head and tail must stay materialized.

While the fiber is suspended, io completions mutate the same `QList` **and** the same `DbTableStats`:

| completion | effect |
|---|---|
| stash finishes (`SetExternal` for a list node) | `malloc_size_ −= len` **and** `obj_memory_usage −= len` |
| load finishes (`NotifyFetched` → `Node::Upload` + `RecordDeleted`) | `malloc_size_ += len` **and** `obj_memory_usage += node->sz` |

Both are correct in isolation; `AutoUpdater::Run()` then re-applies the same bytes because its
snapshot is stale. Drift per window = `node->sz × (in-window loads − in-window stashes)`:

* in-window **stash** -> under-count -> **problem 1** (trips the DCHECK)
* in-window **load** -> over-count -> **problem 2** (invisible to the DCHECK)

`ProcessRead` holds a `FiberAtomicGuard`, so a load completion always lands before the suspended
fiber resumes - the over-count is **deterministic**. The under-count needs a stash still in flight,
hence the CI flakiness.

Whole-value tiering is immune only because the key's pending stash is cancelled before the
`AutoUpdater` is built, and callers re-baseline via `post_updater.ReduceHeapUsage()`. Neither
exists per node.

---
3 - wrong data (mechanism not identified)

The abort is at `lpLast(node->entry)` - the first read after `Materialize(node)`, **before**
`DelNode`. Two candidates, not discriminated: a stale stash completion freeing a just-uploaded
`entry` (`QList::Node::SetExternal` calls `zfree(entry)` from the io fiber and passes both its
`DCHECK`s in that ordering), or the load delivering wrong bytes.

---
4 - the slot ledger cannot see list nodes

`memory_bytes` has a single writer, `AccountObjectMemory`, which needs a key. The list-node id is
`(db_index, QList*, Node*)` - **no key** - so a node offload cannot reach the slot ledger at all.
`QList::MallocUsed` skips offloaded nodes, so deletion subtracts only the reduced size. Nothing
slot-scoped resets it (`FLUSHSLOTS`, config change, migration cleanup); only a full `DbTable` swap
does.

The read path (`LRANGE`) has no `AutoUpdater` in scope and does **not** drift - measured flat over
5 offload/upload cycles.

---
Scenarios

---
A. Problems 1 + 3 - unit test

```bash
cd build-dbg && ninja tiered_storage_test
FLAGS_cluster_mode=emulated FLAGS_lock_on_hashtags=true \
./tiered_storage_test --gtest_filter='ListNodeTieringTest.RPopStashedNodes' --gtest_repeat=200
```

Problem 3 hits ~1/200 locally, in two shapes:

```
RPOP mylist → 2048×'c' expected 2048×'C'
[../src/redis/listpack.c:1326]: assert(lpValidateNext(lp, &p, lpbytes)) failed!
lpAssertValidEntry <- lpPrev <- lpLast <- QList::Pop <- ListWrapper::Pop <- OpPop <- CmdRPop
```

Problem 1 did not reproduce on that box (0/200+; fast NVMe rarely leaves two stash writes in
flight). It is confirmed by CI:

```
72: [ RUN ] ListNodeTieringTest.RPopStashedNodes
72: F0730 06:20:43 db_slice.cc:592] Check failed:
db_arr_[cntx.db_index]->stats.obj_memory_usage >= (*res)->second.MallocUsed() (4366 vs. 6425)
72: @ dfly::DbSlice::FindMutableInternal() <- FindMutable() <- OpPop() <- CmdRPop()
```

and by a deterministic `LSET` reproduction found earlier:

```
Check failed: obj_memory_usage >= MallocUsed() (23807 vs 23809)
CmdLSet -> OpSet -> DbSlice::FindMutable -> FindMutableInternal
```

Only the `cluster mode + FLAGS_lock_on_hashtags` unit-test step tripped it; plain IoUring, Epoll and
cluster steps of the same run passed. Cluster mode is **not** causal - the fixture runs one shard
and per-slot stats are not even allocated in emulated mode; `lock_on_hashtags` only shifts
completion timing.

The CI numbers decompose exactly. With `list_max_listpack_size=1` and 2048-byte values, one node's
listpack is 6 + 2 + 2048 + 2 + 1 = **2059** bytes, and a QList costs
`nodes*40 + 48 + resident_bytes`:

* `6425` = `5*40 + 48 + 3*2059` -> 5 nodes, 3 resident
* `4366` = `5*40 + 48 + 2*2059` -> stats believes 2 resident
* difference `2059` = exactly one node

---
B. Problem 2 - live server

```bash
build-dbg/dragonfly --proactor_threads=1 --tiered_prefix=/tmp/tp \
--tiered_experimental_list_support=true --tiered_experimental_cooling=false \
--tiered_offload_threshold=1.0 --list_max_listpack_size=1 --list_tiering_threshold=2
```

8× `RPUSH mylist <2048 B>`, then 8× `RPOP`, watching `INFO memory: object_used_memory`:

```
after 8 RPUSH: oum=10663 (= 8*40+48+5*2059, correct) stashes=3
RPOP … llen=5 oum=8484 expected 6425 → +2059 fetches=1 <- first load
RPOP … llen=0 oum=6177 expected 0 → +3*2059 fetches=3
EXISTS mylist -> 0
FINAL object_used_memory = 6177 with an empty DB (must be 0)
```

25 sequential rounds: `Δresidue == Δfetches × 2059` in **every** round, residue 6177 -> 150307
monotonically. Pipelined bursts also show the under-count direction (deficit of exactly
`3 × 2059`) - the CI failure mode. Control: same loop without list tiering -> 0.

---
C. Problem 4 - same server with `--cluster_mode=yes`

2000 × 512-byte elements in one key, 127 nodes offloaded:

```
after offload : slot raw = 1 038 346 object_used_memory = 54 477 tiered_entries = 127
after DEL : slot raw = 526 796 object_used_memory = 0 key_count = 0
```

526 796 bytes stranded from one key - ~49 % of everything the slot ever accounted.

---
Notes:

* `--list_tiering_prefetch_depth` defaults to 0; enabling it makes problem 2 worse, since every
async upload landing in a mutable window adds another double-credit.
* A large enough under-count reaches the per-type underflow branch: `LOG(FATAL)` in debug, but in
release it clamps with `LOG_EVERY_T(ERROR)` and silently desyncs `memory_usage_by_type` from
`obj_memory_usage`.
* `obj_memory_usage` feeds `INFO memory: object_used_memory`, the Prometheus
`memory_by_class_bytes` gauge, and the eviction policy's average-object-size estimate.

Contributor guide

Open the contributing guide

Research direction

Start with tiered_storage_test.cc and run the RPopStashedNodes repeat command described in the issue. Read DbSlice::AutoUpdater, QList::Materialize, QList::DelNode, NotifyFetched, and AccountObjectMemory to trace the accounting and node-lifetime paths. Done means the disabled and flaky scenarios are reliable, list memory returns to zero after deletion, and RPOP no longer returns wrong data or aborts.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
databases, performance
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.