apache / apache/kvrocks

stream: consumer-group counters can exceed INT64_MAX and break clients decoding RESP integers as signed

Open
#3,579 1 comment 1 reaction 0 assignees View on GitHub
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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.