No type safety: a key can hold multiple types at once; WRONGTYPE never returned #40

New issue

Open

opened 2026-06-10 12:49:36 +00:00 by sameh-farouk · 1 comment

sameh-farouk commented

2026-06-10 12:49:36 +00:00

Member

Summary

hero_db does not enforce Redis type exclusivity. A single key can simultaneously be a string, hash, list, and set, and commands never return WRONGTYPE.

Reproduction (passes on redis 7.0.15, fails on hero_db)

SET s v   ; LPUSH s x   -> hero_db: 1 (list created alongside string)   redis: WRONGTYPE
RPUSH l a ; INCR l      -> hero_db: 1                                   redis: WRONGTYPE
HSET h f v; GET h       -> hero_db: nil                                 redis: WRONGTYPE

Root cause

Each type lives under its own key prefix (H:/L:/S:/X:, db/engine/types.rs) and read/write paths never check for a conflicting type on the same logical key:

String ops use only the bare key — crates/hero_db/src/db/engine/string.rs (get/set/incrby/append all txn.get(&key) / txn.insert(&key, ...)).
LPUSH→L:key:*, SADD→S:key:*, HSET→H:key:* write into disjoint prefixes regardless of an existing value of another type.

There is no per-key type tag, so collisions are silent.

Impact

Correctness / data-integrity hazard. Any client relying on WRONGTYPE for safety gets silent multi-type keys (e.g. INCR on a list returns a counter while the list still exists under L:).

Shares a root cause with the keyspace fragmentation issue #39. A per-key type index would address both.

Filed from a Redis-compatibility audit (hero_db v0.6.0 @ main aacaad1). Every finding was cross-validated: the same probe passes on stock redis-server 7.0.15 and fails on hero_db, using the Apache Kvrocks gocase suite (Go) and a redis-py 8.0 probe (Python). Root causes verified against the source.

## Summary hero_db does not enforce Redis type exclusivity. A single key can simultaneously be a string, hash, list, and set, and commands never return `WRONGTYPE`. ## Reproduction (passes on redis 7.0.15, fails on hero_db) ``` SET s v ; LPUSH s x -> hero_db: 1 (list created alongside string) redis: WRONGTYPE RPUSH l a ; INCR l -> hero_db: 1 redis: WRONGTYPE HSET h f v; GET h -> hero_db: nil redis: WRONGTYPE ``` ## Root cause Each type lives under its own key prefix (`H:`/`L:`/`S:`/`X:`, `db/engine/types.rs`) and read/write paths never check for a conflicting type on the same logical key: - String ops use only the bare key — `crates/hero_db/src/db/engine/string.rs` (`get`/`set`/`incrby`/`append` all `txn.get(&key)` / `txn.insert(&key, ...)`). - `LPUSH`→`L:key:*`, `SADD`→`S:key:*`, `HSET`→`H:key:*` write into disjoint prefixes regardless of an existing value of another type. There is no per-key type tag, so collisions are silent. ## Impact Correctness / data-integrity hazard. Any client relying on `WRONGTYPE` for safety gets silent multi-type keys (e.g. `INCR` on a list returns a counter while the list still exists under `L:`). Shares a root cause with the keyspace fragmentation issue #39. A per-key type index would address both. --- *Filed from a Redis-compatibility audit (hero_db v0.6.0 @ main `aacaad1`). Every finding was cross-validated: the same probe **passes on stock `redis-server 7.0.15`** and **fails on hero_db**, using the Apache Kvrocks `gocase` suite (Go) and a `redis-py 8.0` probe (Python). Root causes verified against the source.*

sameh-farouk referenced this issue

2026-06-10 12:49:38 +00:00

Keyspace fragmentation: KEYS / DBSIZE / EXISTS / SCAN ignore hash, list & set keys #39

sameh-farouk referenced this issue

2026-06-10 12:55:01 +00:00

Docs accuracy: HERO_DB_PORT is not an OS env var, and "any Redis client" compatibility is overstated #44

sameh-farouk referenced this issue

2026-06-10 15:24:52 +00:00

Keyspace fragmentation: KEYS / DBSIZE / EXISTS / SCAN ignore hash, list & set keys #39

sameh-farouk commented

2026-06-10 15:24:54 +00:00

Author

Member

Org-wide consumer blast-radius audit (#39 + #40)

Scanned all 132 non-empty repos across 6 orgs (lhumina_code, geomind_code, geomind_research, ourworld_it, ourworld_org, projectmycelium) — remote shallow-clone of each default branch, then classified hero_db references and drilled into the data consumers.

Who actually consumes hero_db (data path)

Repo	How it uses hero_db	Calls KEYS/SCAN/EXISTS/DBSIZE?	Reuses a key across types?	Persists data?
hero_lib_rhai	`HeroDbClient`; exposes generic `herodb_keys`/`herodb_exists`/`get`/`set`/… to Rhai scripts	✅ `herodb_keys()`+`herodb_exists()` (`clients_rhai/src/herodb.rs:480,491`)	only string ops exposed	via scripts
hero_wallet	`HeroDbClient`; `set/get/del` (string) + `sadd/smembers` (set), disjoint key namespaces	no	no (clean typed design, `storage.rs:100-147`)	✅ wallets/accounts (sensitive)
hero_aibroker	RESP/RPC `redis.get/set/del/hset/hgetall/sadd/smembers` on distinct keys + `database.create`	no	no	✅ model/provider config
hero_slides	`redis.hset/hget/hgetall/hdel` on one fixed hash key	no	no	✅ slide collections
hero_memory	`HeroDBServerClient` for `ontology.*`/graph (its own `scan`/`keys` are NOT hero_db's)	no (hero_db side)	typed	✅ ontology/memory
hero_bundle	integration-test only (ping via both clients)	no	no	test fixture
hero_router / hero_proc / hero_coordinator / ourworld_framework	route socket / supervise / `6378` config	n/a	n/a	n/a
hero_osis	references the old flat socket `hero_db_*.sock` (stale, #34-class)	n/a	n/a	n/a

(The ~25 raw "risky" grep hits were false positives — Path::exists() and "keys" in OpenRPC JSON schemas.)

#39 blast radius (KEYS/DBSIZE/EXISTS/SCAN see all types) — narrow

🟠 hero_lib_rhai is the only real exposure: herodb_keys() / herodb_exists() are exposed to Rhai scripts. Today they see string keys only; after #39 they'd also surface hash/list/set keys (relevant if the instance is shared with hero_slides/wallet/aibroker). A script that does enumerate-keys → GET each could get non-string keys back (nil, or WRONGTYPE once #40 lands). Regression-check the Rhai scripts.
✅ No other consumer calls KEYS/SCAN/DBSIZE/EXISTS on hero_db (hero_memory's are its own API; hero_db's CLI uses them internally only). EXISTS flipping to true for live hash/list/set keys is a correctness fix nobody currently depends on.

#40 blast radius (type-safety / WRONGTYPE + possible type index) — two risks

🟢 WRONGTYPE enforcement — low break risk. Every data consumer uses disjoint key namespaces per type (verified: hero_wallet {prefix}{id} vs index_key; hero_aibroker set/hash/string on distinct keys; hero_slides single hash key). None reuses a key as two types. Only wildcard is hero_lib_rhai's generic scripting surface — but it exposes string ops only, so it can't create conflicting types itself.
🔴 Migration / on-disk format — the real danger. If #40 introduces a per-key type index or changes key layout, every persisted hero_db instance needs migration. Affected data holders: hero_wallet (sensitive), hero_aibroker, hero_slides, hero_memory (+ any prod hero_db). A format change without a read-compat path = data loss. (hero_wallet's sensitive data also rides on hero_db's hardcoded encryption key — #46.)

Recommendations

Ship #39 standalone, storage-unchanged — implement by aggregating prefix scans in KEYS/DBSIZE/EXISTS/SCAN (the way TYPE/DEL already walk all prefixes). No on-disk change, zero migration risk. Only gate: a regression pass on hero_lib_rhai / Rhai scripts that enumerate keys.
For #40, prefer deriving type from existing prefixes (read-back-compatible) over a new on-disk index — or include a migration and a coordinated, same-branch-name release with hero_wallet / hero_aibroker / hero_slides / hero_memory. SDK signatures don't change (return types unchanged; WRONGTYPE just adds an error variant), so no SDK regen is forced on the 5 cargo_dep consumers.

Audit method: org-wide remote scan of 132 repos / 6 orgs (not just locally-cloned repos), per the cross-org blast-radius convention.

## Org-wide consumer blast-radius audit (#39 + #40) Scanned **all 132 non-empty repos across 6 orgs** (`lhumina_code`, `geomind_code`, `geomind_research`, `ourworld_it`, `ourworld_org`, `projectmycelium`) — remote shallow-clone of each default branch, then classified hero_db references and drilled into the data consumers. ### Who actually consumes hero_db (data path) | Repo | How it uses hero_db | Calls KEYS/SCAN/EXISTS/DBSIZE? | Reuses a key across types? | Persists data? | |---|---|---|---|---| | **hero_lib_rhai** | `HeroDbClient`; exposes generic `herodb_keys`/`herodb_exists`/`get`/`set`/… to **Rhai scripts** | ✅ `herodb_keys()`+`herodb_exists()` (`clients_rhai/src/herodb.rs:480,491`) | only string ops exposed | via scripts | | **hero_wallet** | `HeroDbClient`; `set/get/del` (string) + `sadd/smembers` (set), disjoint key namespaces | no | **no** (clean typed design, `storage.rs:100-147`) | ✅ wallets/accounts (sensitive) | | **hero_aibroker** | RESP/RPC `redis.get/set/del/hset/hgetall/sadd/smembers` on distinct keys + `database.create` | no | no | ✅ model/provider config | | **hero_slides** | `redis.hset/hget/hgetall/hdel` on one fixed hash key | no | no | ✅ slide collections | | **hero_memory** | `HeroDBServerClient` for `ontology.*`/graph (its own `scan`/`keys` are NOT hero_db's) | no (hero_db side) | typed | ✅ ontology/memory | | **hero_bundle** | integration-test only (ping via both clients) | no | no | test fixture | | hero_router / hero_proc / hero_coordinator / ourworld_framework | route socket / supervise / `6378` config | n/a | n/a | n/a | | **hero_osis** | references the **old flat socket** `hero_db_*.sock` (stale, #34-class) | n/a | n/a | n/a | (The ~25 raw "risky" grep hits were false positives — `Path::exists()` and `"keys"` in OpenRPC JSON schemas.) ### #39 blast radius (KEYS/DBSIZE/EXISTS/SCAN see all types) — narrow - 🟠 **hero_lib_rhai is the only real exposure**: `herodb_keys()` / `herodb_exists()` are exposed to Rhai scripts. Today they see string keys only; after #39 they'd also surface hash/list/set keys (relevant if the instance is shared with hero_slides/wallet/aibroker). A script that does *enumerate-keys → GET each* could get non-string keys back (nil, or `WRONGTYPE` once #40 lands). **Regression-check the Rhai scripts.** - ✅ No other consumer calls KEYS/SCAN/DBSIZE/EXISTS on hero_db (hero_memory's are its own API; hero_db's CLI uses them internally only). `EXISTS` flipping to `true` for live hash/list/set keys is a correctness fix nobody currently depends on. ### #40 blast radius (type-safety / WRONGTYPE + possible type index) — two risks 1. 🟢 **WRONGTYPE enforcement — low break risk.** Every data consumer uses **disjoint key namespaces per type** (verified: hero_wallet `{prefix}{id}` vs `index_key`; hero_aibroker set/hash/string on distinct keys; hero_slides single hash key). None reuses a key as two types. Only wildcard is hero_lib_rhai's generic scripting surface — but it exposes string ops only, so it can't create conflicting types itself. 2. 🔴 **Migration / on-disk format — the real danger.** If #40 introduces a per-key type index or changes key layout, **every persisted hero_db instance needs migration.** Affected data holders: **hero_wallet (sensitive), hero_aibroker, hero_slides, hero_memory** (+ any prod hero_db). A format change without a read-compat path = data loss. (hero_wallet's sensitive data also rides on hero_db's hardcoded encryption key — #46.) ### Recommendations 1. **Ship #39 standalone, storage-unchanged** — implement by aggregating prefix scans in KEYS/DBSIZE/EXISTS/SCAN (the way `TYPE`/`DEL` already walk all prefixes). No on-disk change, **zero migration risk**. Only gate: a regression pass on **hero_lib_rhai** / Rhai scripts that enumerate keys. 2. **For #40, prefer deriving type from existing prefixes (read-back-compatible) over a new on-disk index** — or include a migration and a **coordinated, same-branch-name release** with hero_wallet / hero_aibroker / hero_slides / hero_memory. SDK signatures don't change (return types unchanged; WRONGTYPE just adds an error variant), so no SDK regen is forced on the 5 `cargo_dep` consumers. *Audit method: org-wide remote scan of 132 repos / 6 orgs (not just locally-cloned repos), per the cross-org blast-radius convention.*