fix(stream): decrement pending_number when XAUTOCLAIM removes deleted PEL entries

[wengsht]

The group's and the owning consumer's pending_number can be serialized as a RESP integer larger than INT64_MAX, which breaks any client that decodes integer replies as signed 64-bit — the reply fails to parse and the connection is dropped. Hit in production.

XAUTOCLAIM pending_number drift

When XAUTOCLAIM meets a PEL entry whose stream entry was trimmed or XDEL'd, it deletes the PEL entry but never decrements the group's or the owning consumer's pending_number. The counts drift upward on every such sweep and, being unsigned, eventually underflow-wrap on a later decrement — after which XINFO GROUPS and XPENDING report garbage.

The fix mirrors DeletePelEntries (XACK): when a deleted entry is removed from the PEL, decrement the owning consumer and the group. A claim only moves ownership within the group (net-zero for the group count), so only deleted entries reduce it. Subtractions are saturating so a counter that has already drifted can't wrap.

Test: read entries into a consumer, XDEL them, XAUTOCLAIM, then check the group and consumer pending counts return to 0. The existing XDEL autoclaim tests only asserted the returned deleted-id list.

This PR previously also carried an XINFO GROUPS lag-underflow fix. Per review it is a separate issue (SRP + clean cherry-pick), so I've dropped it here and will open a follow-up PR.

---

AI assistance: diagnosis and drafting were done with AI help; I've reviewed the changes and tests and understand the behavior.

[github-actions]

Hi @wengsht,

Thank you for your pull request. Please review our Contributing Guide.

Please make sure you understand your changes and explain your reasoning in this pull request. Low-quality pull requests may be closed.

[wengsht]

@LindaSummer could you help approve the CI run? This fixes pending_number drifting upward when XAUTOCLAIM sweeps PEL entries whose stream body was already trimmed/XDEL'd — the counter is an unclamped uint64, so in production it eventually wraps past INT64_MAX and breaks any client that decodes XINFO GROUPS pending as a signed int. The decrement mirrors the existing logic in DeletePelEntries. Repro added as a gocase test.

[Copilot]

🟡 Not ready to approve

A newly added test relies on the server default XREADGROUP COUNT behavior, which can make the test nondeterministic unless COUNT is set explicitly.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

Fixes two stream consumer-group counters that could underflow-wrap and be serialized as RESP integers larger than INT64_MAX, which can break clients that decode integer replies as signed 64-bit. The changes are in the stream implementation (XAUTOCLAIM PEL sweeping and XINFO GROUPS lag computation) and are covered by new Go unit tests.

Changes:

• Fix XAUTOCLAIM to decrement per-consumer and per-group pending_number when sweeping dangling PEL entries (trimmed/XDEL’d entries), using saturating subtraction to avoid wrap.
• Fix XINFO GROUPS lag computation to avoid unsigned underflow when entries_read > entries_added, falling back to the existing invalid-lag sentinel (UINT64_MAX).
• Add Go tests covering both behaviors to ensure replies remain decodable and counters return to sane values.

File summaries

File	Description
tests/gocase/unit/type/stream/stream_test.go	Adds regression tests for XAUTOCLAIM pending decrement on swept PEL entries and for XINFO GROUPS lag underflow/overflow safety.
src/types/redis_stream.cc	Adjusts XAUTOCLAIM bookkeeping for pending counters (with saturation) and guards lag subtraction to prevent underflow wrap.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[LindaSummer]

Hi @wengsht ,

Thanks for your effort in this problem.
I will review this patch in recent days.

If you want to run a full CI by yourself, you could create a PR in your forked repo and enable its github action.
This will accelerate your development without pending by upstream CI gate.

[wengsht]

Hi @wengsht ,

Thanks for your effort in this problem. I will review this patch in recent days.

If you want to run a full CI by yourself, you could create a PR in your forked repo and enable its github action. This will accelerate your development without pending by upstream CI gate.

Thanks for the tip!

Ran CI:
https://github.com/wengsht/kvrocks/pull/1/checks

[LindaSummer]

Hi @wengsht ,

Thanks very much for your effort for these two issues.

I have several suggestions for this patch.

1. It appears that we solved two independent issues in one patch. This is not good practice since we need to cherry-pick in the release process, and it violates SRP (Single Responsibility Principle). We'd better create two patches for these two issues.
2. The fix for the entries-read has a case not aligning with Redis's behavior.

For the XAUTOCLAIM fix, it generally looks good to me.

Have a wonderful day!

[LindaSummer]

This behavior seems not aligning with redis.

Here is the redis version 8.8.0 docker result.

redis-cli -p 6379
127.0.0.1:6379> DEL repro-lag
(integer) 1
127.0.0.1:6379> XADD repro-lag 1-0 f v
"1-0"
127.0.0.1:6379> XADD repro-lag 2-0 f v
"2-0"
127.0.0.1:6379> XADD repro-lag 3-0 f v
"3-0"
127.0.0.1:6379> XGROUP CREATE repro-lag grp 0 ENTRIESREAD 1000000
OK
127.0.0.1:6379> XINFO GROUPS repro-lag
1)  1) "name"
    2) "grp"
    3) "consumers"
    4) (integer) 0
    5) "pending"
    6) (integer) 0
    7) "last-delivered-id"
    8) "0-0"
    9) "entries-read"
   10) (integer) 3
   11) "lag"
   12) (integer) 3

Here is this patch's result.

redis-cli -p 6666
127.0.0.1:6666> DEL repro-lag
(integer) 1
127.0.0.1:6666> XADD repro-lag 1-0 f v
 repro-lag grp 0"1-0"
127.0.0.1:6666> XADD repro-lag 2-0 f v
"2-0"
127.0.0.1:6666> XADD repro-lag 3-0 f v
"3-0"
127.0.0.1:6666> XGROUP CREATE repro-lag grp 0 ENTRIESREAD 1000000
OK
127.0.0.1:6666> XINFO GROUPS repro-lag
1)  1) "name"
    2) "grp"
    3) "consumers"
    4) (integer) 0
    5) "pending"
    6) (integer) 0
    7) "last-delivered-id"
    8) "0-0"
    9) "entries-read"
   10) (integer) 1000000
   11) "lag"
   12) (integer) 3

The entries-read should be the entries_added if entries_read is greater than entries_added.

Here is redis implemetation.

https://github.com/redis/redis/blob/8f365337d48c8a264bc9f3381c7b8cc12ffb51d2/src/t_stream.c#L3663-L3665

[wengsht]

Thanks for your careful review! @LindaSummer

I have stripped the second change from this PR and also the XAUTOCLAIM fix remains. Can you help take another look?

On the entries-read misalignment — you're right, my version was wrong. Redis clamps entries_read down to entries_added at write time. I will draft second PR for that.

[wengsht]

For the PR #3581

kvrocks with PR #3581 (:6666)

127.0.0.1:6666> DEL repro-lag
(integer) 1
127.0.0.1:6666> XADD repro-lag 1-0 f v
"1-0"
127.0.0.1:6666> XADD repro-lag 2-0 f v
"2-0"
127.0.0.1:6666> XADD repro-lag 3-0 f v
"3-0"
127.0.0.1:6666> XGROUP CREATE repro-lag grp 0 ENTRIESREAD 1000000
OK
127.0.0.1:6666> XINFO GROUPS repro-lag

1. 1. "name"
   2. "grp"
   3. "consumers"
   4. (integer) 0
   5. "pending"
   6. (integer) 0
   7. "last-delivered-id"
   8. "0-0"
   9. "entries-read"
      10) (integer) 3
      11) "lag"
      12) (integer) 3

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

[LindaSummer]

Hi @wengsht ,

Thanks very much for your dig!

Left one more comment.

Generally LGTM.

[LindaSummer]

Maybe we could make current consumer data stored outside of the consumer_pending_decrements to reduce erase of a map.

Just a nitpick, non-mandatory.

[wengsht]

thanks for the review again!

updated.