fix(storage): blocking pops stop creating the key they miss on

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::<K>`) — `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

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<K: OwnedKind>(
+        &mut self,
+        key: &[u8],
+    ) -> Result<Option<K::Mut<'_>>, 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<Bytes> {
-        let list = self.get_or_create_list(key).ok()?;
+        let list = self.get_mut_if_present::<db_kind::ListKind>(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<Bytes> {
-        let list = self.get_or_create_list(key).ok()?;
+        let list = self.get_mut_if_present::<db_kind::ListKind>(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::<db_kind::SortedSetKind>(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::<db_kind::SortedSetKind>(key)
+            .ok()??;
         let last = tree.iter_rev().next().map(|(s, m)| (s, m.clone()))?;
         let (score, member) = last;
         tree.remove(score, &member);

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");
+    }
 }