stream: consumer-group counters can exceed INT64_MAX and break clients decoding RESP integers as signed
- Dominant language
- C++
- Stars
- 4.4k
- Forks
- 658
- Avg merge
- 1d 20h
- Merged PRs (30d)
- 10
Description
### Summary
Several Redis-stream consumer-group counters are stored as `uint64_t` and emitted directly via `redis::Integer` (`std::to_string`, `src/server/redis_reply.h`). If any of them holds a value above `INT64_MAX`, the reply is a decimal like `:18446744073709551615\r\n`, which clients that decode RESP `:` integers as signed 64-bit (e.g. the Rust `fred` client) fail to parse — the connection is dropped/poisoned rather than returning group info.
PR #3578 fixes the two concrete instances we hit in production. This issue tracks the remaining, related hardening so it can be discussed before a broader patch (per the contributing guide's "discuss first / prefer focused patches").
### Already addressed by #3578
- `XAUTOCLAIM` deleting a dangling PEL entry without decrementing `pending_number` (counter drifts up, later wraps).
- `XINFO GROUPS` `lag` wrapping when `entries_read > entries_added` (unsigned subtraction).
### Remaining, same failure class
1. **Non-saturating `pending_number` decrements.** After #3578, `AutoClaim` is the only saturating decrement; these still subtract unconditionally, so any invariant break wraps a client-visible `uint64` counter (all `src/types/redis_stream.cc`):
- `DeleteConsumer` (XGROUP DELCONSUMER): `group.pending_number -= deleted_pel` — cross-level (subtracts a *consumer's* count from the *group's* count), highest risk.
- `DeletePelEntries` (XACK): `group.pending_number -= *acknowledged` and `consumer.pending_number -= ack_count`.
- `ClaimPelEntries` (XCLAIM): `original_consumer.pending_number -= 1`.
2. **`lag` can exceed `INT64_MAX` even without a wrap.** `entries_added` is settable to any `uint64` via `XSETID ... ENTRIESADDED n`, so a correctly-computed `lag = entries_added - entries_read` can be a legitimate value `> INT64_MAX` that `CheckLagValid` passes through to the reply. #3578's guard only covers the `entries_read > entries_added` wrap.
3. **No emit-side clamp for already-corrupt data.** #3578 stops *future* drift but doesn't repair or clamp existing values, and other unsigned fields are emitted raw: group/consumer `pending_number` (XINFO GROUPS/CONSUMERS), `entries-added` (XINFO STREAM), and the `idle`/`inactive` timestamp deltas (`now - stored_ts`, which wrap under clock skew / a future ts injected via XCLAIM). (The XPENDING *summary* count appears to be recomputed live from the PEL scan — worth confirming whether it reads the cached counter on any path.)
### Suggested direction
- Make all consumer-group counter decrements saturating (`x >= n ? x - n : 0`), matching #3578's AutoClaim decrement.
- Either treat `lag > INT64_MAX` as unknown (nil, via the existing `UINT64_MAX` sentinel) in `CheckLagValid`, or add an emit-side clamp helper (`min(v, INT64_MAX)`) applied to the counter/timestamp serializations so the wire value is always within i64 for strict clients. Redis already treats these as signed, so clamping is behavior-compatible.
Happy to send a PR once there's agreement on the preferred shape (per-site saturation vs. a shared emit-side clamp).
---
AI assistance: the analysis behind this issue was done with AI help; I've reviewed and verified the findings against the source.
Contributor guide
No contributing guide indexed for this repository
Research direction
Start in src/types/redis_stream.cc by tracing the listed consumer-group decrement paths, then inspect src/server/redis_reply.h and the XINFO stream/group/consumer serializers. Confirm how XPENDING obtains its summary count and compare the possible saturation and emit-side handling approaches. Done means the remaining unsigned counters and timestamp deltas cannot produce RESP integers outside signed 64-bit range, with behavior agreed before implementation.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp, redis
- Domain
- backend-api-design, databases
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 45/100