add tests for more events by JoanFM · Pull Request #14939 · redis/redis
This PR adds test coverage for keyspace notifications that affect modules like RediSearch, which use SetKeyMeta in notification callbacks.
Test Coverage Analysis
Legend
- ✅ = Already covered (before this PR)
- 🆕 = Added in this PR
- ❌ = Not covered
Hash Events (NOTIFY_HASH)
| Event | Commands That Trigger | Coverage | Test Name |
|---|---|---|---|
hset |
HSET, HMSET, HSETNX | ✅ Already covered | HSET with SetKeyMeta..., HMSET with SetKeyMeta..., HSETNX with SetKeyMeta... |
hset |
HSETEX | 🆕 This PR | HSETEX with SetKeyMeta in notification does not crash |
hdel |
HDEL | 🆕 This PR | HDEL with SetKeyMeta in notification does not crash |
hdel |
HGETDEL | 🆕 This PR | HGETDEL with SetKeyMeta in notification does not crash |
hdel |
HGETEX/HSETEX/HEXPIRE (past timestamp) | 🆕 This PR | Covered via HGETEX, HSETEX, HEXPIRE tests |
hincrby |
HINCRBY | ✅ Already covered | HINCRBY with SetKeyMeta in notification does not crash |
hincrbyfloat |
HINCRBYFLOAT | ✅ Already covered | HINCRBYFLOAT with SetKeyMeta in notification does not crash |
hexpire |
HEXPIRE, HPEXPIRE, HEXPIREAT, HPEXPIREAT | 🆕 This PR | HEXPIRE with SetKeyMeta in notification does not crash |
hexpire |
HGETEX (with EX/PX), HSETEX (with EX/PX) | 🆕 This PR | HGETEX with SetKeyMeta..., HSETEX with SetKeyMeta... |
hpersist |
HPERSIST | 🆕 This PR | HPERSIST with SetKeyMeta in notification does not crash |
hpersist |
HGETEX (with PERSIST) | 🆕 This PR | HGETEX with SetKeyMeta in notification does not crash (with setcount assertion) |
hexpired |
Lazy/Active field expiration | 🆕 This PR | Hash field expiration (hexpired) with SetKeyMeta in notification does not crash |
Generic Events (NOTIFY_GENERIC)
| Event | Commands That Trigger | Coverage | Test Name |
|---|---|---|---|
del |
DEL, UNLINK | ✅ Already covered | DEL with SetKeyMeta in notification does not crash |
del |
Hash becomes empty | 🆕 This PR | Via HDEL with SetKeyMeta..., HGETDEL with SetKeyMeta... tests |
rename_from |
RENAME, RENAMENX | ✅ Already covered | RENAME with SetKeyMeta in notification does not crash |
rename_to |
RENAME, RENAMENX | ✅ Already covered | RENAME with SetKeyMeta in notification does not crash |
restore |
RESTORE | ✅ Already covered | RESTORE with SetKeyMeta in notification does not crash |
expire |
EXPIRE, PEXPIRE, EXPIREAT, PEXPIREAT | ✅ Already covered | EXPIRE and key expiry with SetKeyMeta in notification does not crash |
persist |
PERSIST | 🆕 This PR | PERSIST with SetKeyMeta in notification does not crash |
copy_to |
COPY | 🆕 This PR | COPY with SetKeyMeta in notification does not crash |
loaded |
RDB load (DEBUG RELOAD) | ✅ Already covered | DEBUG RELOAD with SetKeyMeta in notification does not crash |
New Tests Added
| Test Name | Events Covered | Status Without Fix |
|---|---|---|
HSETEX with SetKeyMeta in notification does not crash |
hset, hexpire, hdel |
❌ CRASHES |
HGETDEL with SetKeyMeta in notification does not crash |
hdel, hexpired |
❌ CRASHES |
HGETEX with SetKeyMeta in notification does not crash |
hexpire, hpersist, hdel |
❌ CRASHES |
HDEL with SetKeyMeta in notification does not crash |
hdel |
❌ CRASHES |
HEXPIRE with SetKeyMeta in notification does not crash |
hexpire, hdel |
❌ CRASHES |
HPERSIST with SetKeyMeta in notification does not crash |
hpersist |
✅ Passes |
Hash field expiration (hexpired) with SetKeyMeta in notification does not crash |
hexpired |
✅ Passes |
PERSIST with SetKeyMeta in notification does not crash |
persist |
✅ Passes |
COPY with SetKeyMeta in notification does not crash |
copy_to |
✅ Passes |
Bug Fixes Required
Commands that need fixing for use-after-reallocation when SetKeyMeta is called:
hsetexCommand- accessesoafternotifyKeyspaceEventhgetdelCommand- accessesoafternotifyKeyspaceEventhgetexCommand- accessesoafternotifyKeyspaceEventhdelCommand- accessesoafternotifyKeyspaceEventhexpireGenericCommand- accesseshashObjafternotifyKeyspaceEvent
Still Not Covered (Future Work)
| Event | Command/Trigger | Reason |
|---|---|---|
evicted |
Memory eviction | Requires maxmemory configuration |