From 99ac53db6b4c477787864997914bf2cae31b4111 Mon Sep 17 00:00:00 2001 From: Tin Dang Date: Wed, 19 Aug 2026 10:50:05 +0700 Subject: [PATCH] fix(storage): blocking pops stop creating the key they miss on MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BLPOP, BRPOP, BLMOVE (source), BRPOPLPUSH (source), BZPOPMIN, BZPOPMAX and BZMPOP reached their value through the blocking-hook helpers `Database::list_pop_front` / `list_pop_back` / `zset_pop_min` / `zset_pop_max`, each of which opened with `get_or_create_list` / `get_or_create_sorted_set`. On a MISSING key that inserted an empty container first and only then discovered there was nothing to pop, early-returning through `?` and leaving the fabricated entry behind forever. The consequences were all keyspace-visible: - EXISTS / TYPE / DBSIZE / KEYS / SCAN reported a key the client never created (#523: `BLPOP ghost 0.05` -> `EXISTS ghost` = 1, `TYPE` = list). - The ordinary redis idiom of blocking on a key a producer is about to create broke: the producer's RPUSH answered WRONGTYPE because the consumer's miss had already claimed the key as a zset (#539). - Unbounded growth on the most ordinary blocking-queue workload there is — a worker looping `BLPOP job: 1` over rotating ids leaked one empty list per timed-out poll and nothing ever removed them. - Later replies for the same key silently changed shape: `LPOP k 2` on a truly absent key is a null array, but after a `BLPOP k` miss it answered an empty array. - An empty list/zset is not a representable redis value, so the phantoms were also leaking into RDB, AOF rewrite and replication. Fix: add `Database::get_mut_if_present::` — `get_or_create::` with the fabrication step removed — and take it from the four pop helpers. Everything else about the access shape is preserved exactly: expired keys are still dropped through `remove_hot`, a cold-spilled value is still promoted back to hot RAM before it is handed out, the kind's compact encoding is still upgraded in place, a type mismatch is still Err(WRONGTYPE) (which the pops still swallow into None — blocking-on-wrongtype semantics deliberately unchanged), `credit_memory` on the found-key path is untouched, and a collection that empties still removes its key. A missing key now leaves both the keyspace and `used_memory` byte-identical. BLMPOP was already correct — it length-checks through the read-only `get_list` — and is untouched. `zset_restore` (the wakeup undo path) keeps `get_or_create_sorted_set`: re-inserting a member is a genuine create. At `--shards >= 2` the phantom only appeared for client-local keys (~1/N of them, since the fast path runs against the local shard slice), which is why smoke tests read it as a flake. Tests (red against the pre-fix helpers, green after — 7 of 8 failed before): `test_list_pop_front_missing_key_creates_no_phantom`, `test_list_pop_back_missing_key_creates_no_phantom`, `test_zset_pop_min_missing_key_creates_no_phantom`, `test_zset_pop_max_missing_key_creates_no_phantom`, `test_push_after_missed_pop_is_not_wrongtype`, `test_list_pop_removes_key_only_when_it_empties`, `test_zset_pop_removes_key_only_when_it_empties`, plus `test_pop_on_wrong_type_leaves_value_intact` and `test_list_pop_front_promotes_a_cold_spilled_list` as no-change guards. End-to-end at `--shards 1` and `--shards 4`: all eight blocking commands miss with TYPE=none / EXISTS=0 / DBSIZE=0, a blocked BZPOPMIN no longer blocks a producer's RPUSH, and `LPOP k 2` after a miss is the null array again. Fixes #523 Fixes #539 author: Tin Dang --- CHANGELOG.md | 15 ++++ src/storage/db/accessors.rs | 59 +++++++++++++- src/storage/db/mod.rs | 154 ++++++++++++++++++++++++++++++++++++ 3 files changed, 224 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ee926542..3a89745c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -169,6 +169,21 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 label-gated; both unchanged. ### Fixed +- **Blocking pops no longer create the key they miss on** (#523, #539). `BLPOP`, `BRPOP`, + `BLMOVE` (source), `BRPOPLPUSH` (source), `BZPOPMIN`, `BZPOPMAX` and `BZMPOP` reached their + value through `get_or_create_list` / `get_or_create_sorted_set`, so every miss materialised an + empty list/zset before discovering there was nothing to pop — a phantom key that `EXISTS`, + `TYPE`, `DBSIZE`, `KEYS` and `SCAN` all reported, that the ordinary "block on a key another + client is about to create" idiom then hit with `WRONGTYPE` on the producer's `RPUSH`, and that + nothing ever removed (a worker polling rotating ids leaked one empty collection per timed-out + poll). An empty list/zset is not a representable Redis value, so those phantoms were also + leaking into RDB, AOF rewrite and replication. The four pop helpers now take a new non-creating + accessor (`Database::get_mut_if_present::`) — `get_or_create` minus the fabrication step, so + expiry drop, cold-tier promotion, compact-encoding upgrade, `WRONGTYPE` classification and the + become-empty ⇒ remove-the-key behaviour are all unchanged, but a missing key leaves the + keyspace and `used_memory` byte-identical. `BLMPOP` was already correct (it length-checks + through the read-only `get_list`) and is untouched. At `--shards >= 2` the phantom appeared only + for client-local keys (~1/N of them), which is why it read as a flake. - **`EXPIRE`/`PEXPIRE`/`EXPIREAT`/`PEXPIREAT` now accept the Redis 7.0 `NX | XX | GT | LT` conditions** (#544) — previously any option token was rejected with a wrong-arity error, which breaks typed clients that call them directly (redis-py `expire(k, t, nx=True)`). Semantics match diff --git a/src/storage/db/accessors.rs b/src/storage/db/accessors.rs index c92d26cd..e8d77154 100644 --- a/src/storage/db/accessors.rs +++ b/src/storage/db/accessors.rs @@ -114,6 +114,41 @@ impl Database { } } + /// Mutable typed access to an **existing** key's full (non-compact) + /// encoding — `get_or_create` minus the fabrication step. + /// + /// A missing key answers `Ok(None)` and leaves the keyspace (and + /// `used_memory`) byte-identical: no entry inserted, no birth version + /// consumed. Everything else matches `get_or_create` exactly — expired + /// keys are dropped, a cold-spilled value is promoted back to hot RAM + /// before it is handed out, the kind's compact encoding is upgraded in + /// place, and a type mismatch is `Err(WRONGTYPE)`. + /// + /// This is what the blocking-pop helpers below take (moon#523/#539): a + /// pop that finds nothing is a *read* as far as the keyspace is + /// concerned, and reads never create. + pub fn get_mut_if_present( + &mut self, + key: &[u8], + ) -> Result>, Frame> { + let now_ms = self.cached_now_ms; + self.drop_if_expired(key, now_ms); + if !self.data.contains_key(key) { + self.promote_cold_if_present(key, now_ms); + } + let Some(entry) = self.data.get_mut(key) else { + return Ok(None); + }; + K::upgrade(entry); + match entry.value.as_redis_value_mut() { + Some(v) => match K::project_mut(v) { + Ok(m) => Ok(Some(m)), + Err(db_kind::WrongType) => Err(Self::wrongtype_error()), + }, + None => Err(Self::wrongtype_error()), + } + } + /// Read typed access to the full (non-compact) encoding, promoting a /// cold-spilled value back to hot RAM on miss (this accessor takes /// `&mut self` — unlike the enum-based `get_ref_if_alive` it can @@ -576,8 +611,14 @@ impl Database { /// Pop the front element from a list. Returns None if key missing/empty/wrong type. /// Removes the key if the list becomes empty. Handles compact listpack upgrade. + /// + /// moon#523/#539: the lookup is deliberately NON-creating. This helper + /// backs the blocking fast path (`try_immediate_pop` → BLPOP/BLMOVE/…), + /// so a `get_or_create_list` here materialised an empty list on every + /// miss — a phantom key that EXISTS/TYPE/DBSIZE reported, that a later + /// RPUSH rejected with WRONGTYPE, and that no path ever removed. pub fn list_pop_front(&mut self, key: &[u8]) -> Option { - let list = self.get_or_create_list(key).ok()?; + let list = self.get_mut_if_present::(key).ok()??; let val = list.pop_front()?; let empty = list.is_empty(); // `list`'s borrow of `self` ends above. @@ -594,8 +635,10 @@ impl Database { /// Pop the back element from a list. Returns None if key missing/empty/wrong type. /// Removes the key if the list becomes empty. Handles compact listpack upgrade. + /// + /// Non-creating on a missing key — see [`Self::list_pop_front`]. pub fn list_pop_back(&mut self, key: &[u8]) -> Option { - let list = self.get_or_create_list(key).ok()?; + let list = self.get_mut_if_present::(key).ok()??; let val = list.pop_back()?; let empty = list.is_empty(); if empty { @@ -627,8 +670,12 @@ impl Database { /// Pop the minimum element from a sorted set. Returns (member, score) or None. /// Removes the key if the sorted set becomes empty. + /// + /// Non-creating on a missing key — see [`Self::list_pop_front`]. pub fn zset_pop_min(&mut self, key: &[u8]) -> Option<(Bytes, f64)> { - let (members, tree) = self.get_or_create_sorted_set(key).ok()?; + let (members, tree) = self + .get_mut_if_present::(key) + .ok()??; let first = tree.iter().next().map(|(s, m)| (s, m.clone()))?; let (score, member) = first; tree.remove(score, &member); @@ -642,8 +689,12 @@ impl Database { /// Pop the maximum element from a sorted set. Returns (member, score) or None. /// Removes the key if the sorted set becomes empty. + /// + /// Non-creating on a missing key — see [`Self::list_pop_front`]. pub fn zset_pop_max(&mut self, key: &[u8]) -> Option<(Bytes, f64)> { - let (members, tree) = self.get_or_create_sorted_set(key).ok()?; + let (members, tree) = self + .get_mut_if_present::(key) + .ok()??; let last = tree.iter_rev().next().map(|(s, m)| (s, m.clone()))?; let (score, member) = last; tree.remove(score, &member); diff --git a/src/storage/db/mod.rs b/src/storage/db/mod.rs index 141f4a47..19b3b8bf 100644 --- a/src/storage/db/mod.rs +++ b/src/storage/db/mod.rs @@ -1913,4 +1913,158 @@ mod tests { assert!(!db.spill_inflight_alive(b"k", 1_001), "past its TTL"); assert!(db.spill_inflight_entry(b"k", 1_001).is_none()); } + + // ── moon#523 / moon#539: blocking pops must never create their key ── + // + // `list_pop_front`/`list_pop_back`/`zset_pop_min`/`zset_pop_max` back the + // blocking fast path (`try_immediate_pop`) for BLPOP/BRPOP/BLMOVE/ + // BRPOPLPUSH/BZPOPMIN/BZPOPMAX/BZMPOP. A miss on an absent key used to + // reach the value through `get_or_create_*`, materialising an empty + // list/zset that EXISTS/TYPE/DBSIZE then reported and that later RPUSHes + // tripped over with WRONGTYPE. A miss must leave the keyspace — and the + // memory estimate — byte-identical. + + /// Assert that `db` holds no trace of `key` on any plane. + fn assert_keyspace_untouched(db: &Database, key: &[u8], used_before: usize) { + assert_eq!(db.data().len(), 0, "hot plane must stay empty"); + assert_eq!(db.logical_len(), 0, "DBSIZE must stay 0"); + assert!(!db.is_hot(key), "no phantom entry for the polled key"); + assert!( + !db.exists_if_alive(key, current_time_ms()), + "EXISTS must still answer 0" + ); + assert_eq!( + db.resident_bytes(), + used_before, + "a miss must not charge memory" + ); + } + + #[test] + fn test_list_pop_front_missing_key_creates_no_phantom() { + let mut db = Database::new(); + let used_before = db.resident_bytes(); + assert!(db.list_pop_front(b"ghost").is_none()); + assert_keyspace_untouched(&db, b"ghost", used_before); + } + + #[test] + fn test_list_pop_back_missing_key_creates_no_phantom() { + let mut db = Database::new(); + let used_before = db.resident_bytes(); + assert!(db.list_pop_back(b"ghost").is_none()); + assert_keyspace_untouched(&db, b"ghost", used_before); + } + + #[test] + fn test_zset_pop_min_missing_key_creates_no_phantom() { + let mut db = Database::new(); + let used_before = db.resident_bytes(); + assert!(db.zset_pop_min(b"ghost").is_none()); + assert_keyspace_untouched(&db, b"ghost", used_before); + } + + #[test] + fn test_zset_pop_max_missing_key_creates_no_phantom() { + let mut db = Database::new(); + let used_before = db.resident_bytes(); + assert!(db.zset_pop_max(b"ghost").is_none()); + assert_keyspace_untouched(&db, b"ghost", used_before); + } + + /// The #539 headline symptom: after a blocking pop misses, a producer + /// must still be able to create the key with its own type. + #[test] + fn test_push_after_missed_pop_is_not_wrongtype() { + let mut db = Database::new(); + assert!(db.zset_pop_min(b"q").is_none()); + // A list push on the same key must succeed — pre-fix the miss left a + // zset behind and `get_or_create_list` answered WRONGTYPE. + db.list_push_back(b"q", Bytes::from_static(b"job")); + assert_eq!( + db.get_list(b"q").unwrap().map(|l| l.len()), + Some(1), + "producer must own the key's type after a consumer's miss" + ); + } + + /// Wrong-type behaviour is unchanged: the pop reports "nothing" and + /// leaves the existing value alone (no clobber, no removal). + #[test] + fn test_pop_on_wrong_type_leaves_value_intact() { + let mut db = Database::new(); + db.set( + Bytes::from_static(b"s"), + Entry::new_string(Bytes::from_static(b"v")), + ); + assert!(db.list_pop_front(b"s").is_none()); + assert!(db.zset_pop_min(b"s").is_none()); + assert_eq!(db.logical_len(), 1); + match db.get(b"s").map(|e| e.value.as_redis_value()) { + Some(RedisValueRef::String(v)) => assert_eq!(v, b"v"), + _ => panic!("string must survive a wrong-type pop"), + } + } + + /// Regression guard for the found-key path: the last pop still removes + /// the key, and a non-final pop still leaves it in place. + #[test] + fn test_list_pop_removes_key_only_when_it_empties() { + let mut db = Database::new(); + db.list_push_back(b"l", Bytes::from_static(b"a")); + db.list_push_back(b"l", Bytes::from_static(b"b")); + + assert_eq!(db.list_pop_front(b"l"), Some(Bytes::from_static(b"a"))); + assert_eq!(db.logical_len(), 1, "one element left, key stays"); + + assert_eq!(db.list_pop_back(b"l"), Some(Bytes::from_static(b"b"))); + assert_eq!(db.logical_len(), 0, "emptied list must be removed"); + assert!(db.list_pop_front(b"l").is_none()); + assert_eq!(db.logical_len(), 0, "and the re-poll must not resurrect it"); + } + + /// The non-creating lookup must still reach the COLD tier: a list that + /// eviction spilled to disk is a real key, and popping it has to promote + /// it back rather than answer "missing". (Guards the one behaviour the + /// #523/#539 fix could have silently dropped along with the fabrication.) + #[test] + fn test_list_pop_front_promotes_a_cold_spilled_list() { + let tmp = tempfile::tempdir().unwrap(); + let mut list = VecDeque::new(); + list.push_back(Bytes::from_static(b"first")); + list.push_back(Bytes::from_static(b"second")); + let mut db = db_with_spilled_value(tmp.path(), b"coldlist", TestRedisValue::List(list)); + + assert_eq!( + db.list_pop_front(b"coldlist"), + Some(Bytes::from_static(b"first")), + "a cold-spilled list must be promoted and popped, not treated as missing" + ); + assert_eq!( + db.get_list(b"coldlist").unwrap().map(|l| l.len()), + Some(1), + "the promoted remainder stays in hot RAM" + ); + } + + /// Same guard on the sorted-set side. + #[test] + fn test_zset_pop_removes_key_only_when_it_empties() { + let mut db = Database::new(); + { + let (members, tree) = db.get_or_create_sorted_set(b"z").unwrap(); + members.insert(Bytes::from_static(b"a"), 1.0); + tree.insert(OrderedFloat(1.0), Bytes::from_static(b"a")); + members.insert(Bytes::from_static(b"b"), 2.0); + tree.insert(OrderedFloat(2.0), Bytes::from_static(b"b")); + } + + assert_eq!(db.zset_pop_min(b"z"), Some((Bytes::from_static(b"a"), 1.0))); + assert_eq!(db.logical_len(), 1, "one member left, key stays"); + + assert_eq!(db.zset_pop_max(b"z"), Some((Bytes::from_static(b"b"), 2.0))); + assert_eq!(db.logical_len(), 0, "emptied zset must be removed"); + assert!(db.zset_pop_max(b"z").is_none()); + assert_eq!(db.logical_len(), 0, "and the re-poll must not resurrect it"); + } }