fix: update() not atomic, create() set+sadd not transactional, list() N+1 Redis calls #4

New Issue

2026-03-06T18:09:20Z

jason.woltje commented

2026-03-06 18:09:20 +00:00

Code review findings — must fix before production use:

1. update() bypasses WATCH (not atomic)
Plain read-then-write. Concurrent updates silently overwrite each other.
Fix: route through mutateTaskAtomically or add WATCH/MULTI/EXEC.
Also bypasses state machine validation — dangerous as a public method.

2. create() set+sadd not in same transaction
If process crashes between SET NX and SADD, task exists in Redis but is invisible to list().
Fix: wrap both in MULTI/EXEC pipeline.

3. list() is O(n) round-trips
Promise.all(taskIds.map(id => this.get(id))) — 100 tasks = 100 GET commands.
Fix: use client.mget() or a pipeline to fetch all task keys in one round-trip.

Code review findings — must fix before production use: **1. update() bypasses WATCH (not atomic)** Plain read-then-write. Concurrent updates silently overwrite each other. Fix: route through mutateTaskAtomically or add WATCH/MULTI/EXEC. Also bypasses state machine validation — dangerous as a public method. **2. create() set+sadd not in same transaction** If process crashes between SET NX and SADD, task exists in Redis but is invisible to list(). Fix: wrap both in MULTI/EXEC pipeline. **3. list() is O(n) round-trips** Promise.all(taskIds.map(id => this.get(id))) — 100 tasks = 100 GET commands. Fix: use client.mget() or a pipeline to fetch all task keys in one round-trip.

jason.woltje referenced a pull request that will close this issue

2026-03-06 18:18:25 +00:00

fix: atomic create/update and mget list #5

jason.woltje closed this issue

2026-03-06 18:18:35 +00:00

jason.woltje referenced a pull request that will close this issue

2026-03-06 19:05:13 +00:00

fix: atomic create/update and mget list #6

Sign in to join this conversation.