fix(scheduler): skip acknowledge and delete when the consumer batch is empty

[abhisheksharma2411]

Type of Change

• Bugfix

Description

get_batches decides "the stream is empty, there is nothing to do" from RedisError::StreamEmptyOrNotAvailable. XREADGROUP does not report an exhausted stream that way — it returns an empty reply — so that branch never runs for the consumer's read. Execution falls through to stream_acknowledge_entries with an empty entry_ids, and XACK key group with no IDs is a syntax error. Every poll of an idle stream logs an ERROR pair, one from the Redis layer and one from the scheduler wrapper above it.

The fix returns once entry_ids is known to be empty. That holds whichever way a backend reports an empty read, so it is not sensitive to how redis-rs and fred each choose to surface it.

Three things worth calling out:

1. XDEL is affected the same way, which the issue did not cover. stream_delete_entries runs on the same empty entry_ids, and XDEL rejects an empty ID list for exactly the same reason. It is invisible today only because the acknowledge fails first and returns via ?. A fix that made stream_acknowledge_entries tolerate an empty list would have moved the error one line down rather than removing it — which is part of why the guard belongs in get_batches.

2. The existing error branch is kept rather than removed — and the two backends genuinely differ. The issue asks: "Worth confirming whether the fred backend reports an empty stream the same way. If it does, the guard is dead for both backends." I ran it. It does not, so the guard is not dead:

backend	exhausted XREADGROUP returns	existing guard	affected?
redis-rs	Ok, empty reply	never fires	yes
fred	Err(StreamEmptyOrNotAvailable)	fires correctly	no

So this bug is redis-rs-only, and removing the error branch would have broken the fred path. Keeping both is not belt-and-braces — each is load-bearing for one backend. That is also why the fix guards on entry_ids.is_empty() rather than on which error came back: it is the one condition that holds either way.

I found this the hard way. My first version of the test asserted the redis-rs shape and went red on fred, which is how the divergence surfaced; the test now asserts the union both backends satisfy.

3. metrics::BATCHES_CONSUMED moves below the check. It was incremented immediately after the read, so every empty poll counted as a consumed batch — on an idle deployment that is the entire series. A side effect worth noting explicitly: a batch that fails to deserialize now returns before the increment too, where previously it was counted. That seems right for a counter named "batches consumed", but say the word if you would rather keep the read-attempt semantics and I will split it out.

Motivation and Context

Fixes #13642.

The reporter's analysis was accurate and I have not had to correct any of it — this PR is essentially their suggested fix, plus the XDEL half and the metric.

How did you test it?

Reproduced the mechanism against Redis 7 first, so the failure is observed rather than inferred:

2. first XREADGROUP — delivers the entry (scheduler has work)
   repro13642_stream / 1787101118373-0 / task / process_payment

3. second XREADGROUP — nothing undelivered left
   (empty reply, NOT an error => StreamEmptyOrNotAvailable never fires,
    so get_batches() falls through with entry_ids == [])

4. XACK repro13642_stream repro13642_group   (no IDs)
   ERR wrong number of arguments for 'xack' command

5. XDEL repro13642_stream                    (no IDs)
   ERR wrong number of arguments for 'xdel' command

Step 3 is the crux: the empty read is an empty reply, not an error. Step 5 is the masked second failure.

Added a test — test_stream_read_with_options_xreadgroup_empty in crates/redis_interface/src/test.rs. It builds the realistic case (append, create the group, drain it) rather than reading a stream that was never written to, and asserts the contract both backends actually satisfy: an exhausted read reports no work — as an empty reply or as StreamEmptyOrNotAvailable — and never reports entries that are not there.

To be straight about what it does and does not do: it passes both before and after this change, because it covers redis_interface, which the fix does not touch. It pins the divergence in the table above so that neither backend can drift into the other's shape unnoticed — the thing that would silently reintroduce this class of bug. The reproduction above is the evidence for the bug itself.

Placed there because that suite already requires a live Redis and runs against both backends; crates/scheduler has no Redis-backed test harness, and I did not want to make cargo test -p scheduler newly dependent on one. Happy to add one if you would prefer get_batches covered directly — that would be the test that goes red without the guard.

Verified against a live Redis 7:

command	result
cargo test -p redis_interface (redis-rs)	100 passed, 0 failed
cargo test -p redis_interface --no-default-features --features fred	74 passed, 1 failed
just clippy	clean
cargo +nightly fmt --all	clean

The one fred failure is test_scan_returns_matching_keys, which is pre-existing and unrelated — I confirmed it by stashing my changes and re-running it on main, where it fails identically. It uses a unique_test_id()-scoped pattern, so nothing I added can reach it. Flagging it since it looks like a real fred/redis-rs behavioural difference in scan rather than a flake, and it may be worth its own issue; happy to open one if that would be useful.

Note that cargo test does not run in CI, so the new test will not appear in the checks on this PR — it is a developer-run suite, per the module docs in test.rs.

Checklist

• I formatted the code cargo +nightly fmt --all
• I addressed lints thrown by cargo clippy
• I reviewed the submitted code
• I added unit tests for my changes where possible

[semanticdiff-com]

Review changes with  

Changed Files

File	Status
crates/redis_interface/src/test.rs	0% smaller
crates/scheduler/src/utils.rs	0% smaller