fix(stream): decrement pending_number when XAUTOCLAIM removes deleted PEL entries by wengsht · Pull Request #3578 · apache/kvrocks

wengsht · 2026-08-04T21:43:53Z

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 · 2026-08-04T21:44:04Z

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.

… entries 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 counters drift upward on every such sweep and, being unsigned, eventually underflow-wrap on later decrements, after which XINFO GROUPS / XPENDING report garbage. Decrement both when removing a deleted entry, matching DeletePelEntries (XACK). A claim only moves ownership within the group, so it is net-zero for the group count; only deleted entries reduce it. Subtractions are saturating so a counter that has already drifted cannot wrap. Signed-off-by: Shitao Weng <shitao@tbean.ai>

wengsht · 2026-08-04T22:45:05Z

@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 · 2026-08-05T02:21:47Z

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 · 2026-08-05T16:18:12Z

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.

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.
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!

…test Make the pending-count precondition independent of the XREADGROUP COUNT default, per review feedback. COUNT is >= the number of entries so all three are still delivered in one call. Signed-off-by: Shitao Weng <shitao@tbean.ai>

Reflow the saturating pending_number ternary per clang-format (the CI "Check with clang-format" lane). No behavior change. Signed-off-by: Shitao Weng <shitao@tbean.ai>

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 · 2026-08-10T08:16:06Z

+      current_consumer_metadata.pending_number = current_consumer_metadata.pending_number >= it->second
+                                                     ? current_consumer_metadata.pending_number - it->second
+                                                     : 0;
+      consumer_pending_decrements.erase(it);


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.

thanks for the review again!

updated.

…f the map Per review: the current consumer never appears in claimed_consumer_entity_count (entries are only claimed from other consumers), so track its own deleted-entry decrement in a scalar and let consumer_pending_decrements hold only the other consumers. This drops the find-and-erase on the map. No behavior change.

LindaSummer

🚀 LGTM

Thanks very much for your effort!

…, add tests Review follow-ups on top of the rebase onto unstable (post-apache#3578): - CheckLagValid: guard entries_read <= entries_added before the unsigned subtraction. A clamp on write (CREATE/SETID) does not stop ReadGroup from incrementing entries_read past entries_added on a subsequent XREADGROUP, which would re-underflow lag to ~2^64 and break XINFO GROUPS decoding again. Falling through to the estimate path keeps the reply decodable. (Copilot review.) - ClampEntriesRead: guard entries_read >= 0 rather than != -1, so any negative value is handled before the uint64 cast. (LindaSummer review.) - Merge the size==0 (all-entries-deleted) lag=0 case into the entries_added==0 branch. (LindaSummer review.) - Tests: add the read-after-clamp regression (XREADGROUP then XINFO GROUPS stays decodable, lag 0) and an emptied-stream lag=0 case. Restore the apache#3578 XAUTOCLAIM pending_number test dropped during conflict resolution. (Copilot review.) Ran clang-format-18 and gofmt.

wengsht force-pushed the fix-xautoclaim-pending-number branch from 383cfa8 to ce83a6e Compare August 4, 2026 21:58

wengsht force-pushed the fix-xautoclaim-pending-number branch from ce83a6e to 809bc37 Compare August 4, 2026 22:02

wengsht changed the title ~~fix(stream): decrement pending_number when XAUTOCLAIM removes deleted entries~~ fix(stream): fix consumer-group counter overflows (XAUTOCLAIM pending_number, XINFO GROUPS lag) Aug 5, 2026

jihuayu requested review from LindaSummer and a lite review from Copilot August 5, 2026 01:26

Copilot started reviewing on behalf of jihuayu August 5, 2026 01:27 View session

Copilot AI reviewed Aug 5, 2026

View reviewed changes

Comment thread tests/gocase/unit/type/stream/stream_test.go

wengsht force-pushed the fix-xautoclaim-pending-number branch from de3bb5f to 8d8ad67 Compare August 5, 2026 01:33

wengsht mentioned this pull request Aug 5, 2026

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

Open

git-hulk requested a review from torwig August 5, 2026 02:04

git-hulk changed the title ~~fix(stream): fix consumer-group counter overflows (XAUTOCLAIM pending_number, XINFO GROUPS lag)~~ fix(stream): consumer-group counter overflows (XAUTOCLAIM pending_number, XINFO GROUPS lag) Aug 5, 2026

wengsht mentioned this pull request Aug 5, 2026

fix: correct XAUTOCLAIM pending number wengsht/kvrocks#1

Open

LindaSummer reviewed Aug 9, 2026

View reviewed changes

Comment thread src/types/redis_stream.cc Outdated

wengsht added 2 commits August 9, 2026 10:13

style(stream): satisfy clang-format in the AutoClaim decrement

cd37ad1

Reflow the saturating pending_number ternary per clang-format (the CI "Check with clang-format" lane). No behavior change. Signed-off-by: Shitao Weng <shitao@tbean.ai>

wengsht force-pushed the fix-xautoclaim-pending-number branch from b140b8b to cd37ad1 Compare August 9, 2026 17:13

wengsht changed the title ~~fix(stream): consumer-group counter overflows (XAUTOCLAIM pending_number, XINFO GROUPS lag)~~ fix(stream): decrement pending_number when XAUTOCLAIM removes deleted PEL entries Aug 9, 2026

wengsht mentioned this pull request Aug 9, 2026

fix(stream): align XINFO GROUPS entries-read and lag with Redis #3581

Open

wengsht requested a review from LindaSummer August 9, 2026 18:16

wengsht closed this Aug 9, 2026

wengsht reopened this Aug 9, 2026

LindaSummer requested a balanced review from Copilot August 10, 2026 08:06

Copilot started reviewing on behalf of LindaSummer August 10, 2026 08:07 View session

Copilot AI reviewed Aug 10, 2026

View reviewed changes

LindaSummer reviewed Aug 10, 2026

View reviewed changes

LindaSummer and others added 2 commits August 10, 2026 16:17

Merge branch 'unstable' into fix-xautoclaim-pending-number

6c42cd5

wengsht requested a review from LindaSummer August 10, 2026 15:53

LindaSummer approved these changes Aug 11, 2026

View reviewed changes

jihuayu approved these changes Aug 11, 2026

View reviewed changes

jihuayu merged commit 3c1ac18 into apache:unstable Aug 11, 2026
37 checks passed

waterWang mentioned this pull request Aug 12, 2026

fix(stream): saturate pending_number decrements and clamp lag to INT64_MAX [FaaFyfxR9WAQrL7FcAgEHJvztd8cVMxvjHRS55rw1nwH] #3589

Closed

Navigation Menu

Uh oh!

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

fix(stream): decrement pending_number when XAUTOCLAIM removes deleted PEL entries#3578
jihuayu merged 5 commits into
apache:unstablefrom
wengsht:fix-xautoclaim-pending-number

wengsht commented Aug 4, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Aug 4, 2026

Uh oh!

wengsht commented Aug 4, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

LindaSummer commented Aug 5, 2026

Uh oh!

wengsht commented Aug 5, 2026

Uh oh!

LindaSummer left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

LindaSummer left a comment

Uh oh!

LindaSummer Aug 10, 2026 •

edited

Loading

Uh oh!

wengsht Aug 10, 2026

Uh oh!

LindaSummer left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

wengsht commented Aug 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

XAUTOCLAIM pending_number drift

Uh oh!

github-actions Bot commented Aug 4, 2026

Uh oh!

wengsht commented Aug 4, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Not ready to approve

Uh oh!

Uh oh!

LindaSummer commented Aug 5, 2026

Uh oh!

wengsht commented Aug 5, 2026

Uh oh!

LindaSummer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

LindaSummer left a comment

Choose a reason for hiding this comment

Uh oh!

LindaSummer Aug 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wengsht Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

LindaSummer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wengsht commented Aug 4, 2026 •

edited

Loading

XAUTOCLAIM `pending_number` drift

LindaSummer Aug 10, 2026 •

edited

Loading