perf: stop reading every task_result payload to prune keys Redis already expires

[adhikjoshi]

The problem

prune_old_task_results() ran from _pruning_loop every PRUNE_CHECK_INTERVAL (60s), in every worker. It SCANned the entire keyspace and GET the JSON of every task_result:* key just to compare one timestamp.

It could never delete anything. task_result keys are written with a TTL — ex=3600 when a task finishes — while the prune only deletes when now - finished_at > TASK_RESULT_RETENTION (86400). The key self-destructs 23 hours before the prune would ever consider it. Redis expiry always wins the race.

Measured on a production shard (redis-va8u7z49) over 26 days:

command	calls	event-loop time
SCAN	196,726,349	2,631 s
GET	228,765,062	5,185 s
DEL	2	—
everything else combined	—	~290 s

SCAN + GET were ~96% of all command time on that shard: ~2.2 hours of blocked event loop and 13.5 TB of network output, to issue 2 deletes. Sampling 1,200 live task_result keys across three shards found zero without a TTL.

Redis is single-threaded, so this landed on callers as GET task_result:* entries at 27–68 ms in the slowlog and intermittent ModelQ enqueue failed: read error on connection errors when an enqueue timed out behind the scan.

The change

• Drop the call from _pruning_loop. Redis TTL is the mechanism; nothing needs to poll for it.
• Keep the method as a manual repair entry point for the one case Redis cannot handle by itself: a key whose TTL went missing (a write path that forgot ex=, a RENAME/RESTORE that dropped it), which would otherwise live forever.
• The rewritten method pipelines TTL — an 8-byte reply — and reads a value only for keys already known to be broken. A healthy keyspace is walked without transferring a single payload.
• UNLINK instead of delete, so multi-MB frees land off the main thread.
• scan_iter(count=500) instead of the default 10.

Concretely, on a shard holding 17,209 task_result keys, one pass goes from ~1,721 SCAN round-trips plus 17,209 payload reads (gigabytes) to ~35 SCAN round-trips plus 17,209 pipelined TTL replies — about 138 KB total.

Behaviour is otherwise unchanged: an old TTL-less key is still deleted along with its task: twin. A TTL-less key that is not yet old gets an expiry set, so it can't leak.

Why not MGET

Batching reads makes this worse, not better. task_result payloads reach 7 MB, so one MGET builds a single enormous client output buffer — and that is precisely what pushes RSS past the container memory limit and gets redis-server OOM-killed.

Testing

51 → 55 tests, all passing. The pre-existing test_prune_old_task_results passes unchanged, which is the signal that the contract held.

New coverage uses a wrapper that counts value reads, pipelined ones included:

• test_prune_does_not_read_keys_that_have_a_ttl — the control case: 50 healthy keys cost 0 value reads
• test_prune_reads_only_the_key_that_lost_its_ttl — 50 healthy + 1 leaked costs exactly 1 read
• test_prune_bounds_a_recent_orphan_instead_of_deleting_it — a fresh TTL-less key is kept but given an expiry
• test_pruning_loop_does_not_scan_task_results — the loop no longer calls the scan

Mutation-tested, each reverted independently and confirmed to turn the suite red:

1. reverting the TTL filter → 2 tests fail (get_calls 50 instead of 0)
2. re-adding the call to _pruning_loop → loop test fails
3. dropping the task: twin unlink → the original prune test fails

Worth noting the original test built its fixture with set() and no TTL — the one case that never occurs in production. That is why this went unnoticed.

Related

The PHP port is ModelsLab/modelq-php#2. Both need releasing before the fleet sees any of this; workers currently run 1.0.14.

---

^{Need help on this PR? Tag @codesmith-bot with what you need. Autofix is enabled.}