Skip to content

Fix XPENDING filtering for empty consumer names - #4357

Merged
petyaslavova merged 2 commits into
redis:masterfrom
LindseyZ1205:fix/xpending-empty-consumer
Oct 7, 2026
Merged

petyaslavova merged 2 commits into
redis:masterfrom
LindseyZ1205:fix/xpending-empty-consumer

Conversation

@LindseyZ1205

@LindseyZ1205 LindseyZ1205 commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Description of change

Redis accepts an empty consumer name: XREADGROUP GROUP <group> "" ... succeeds and creates a consumer named "". This is usually accidental, for example a worker name read from an unset environment variable or a config value that defaults to "". Reading works, so nothing surfaces the mistake.

When that worker later inspects its own backlog with xpending_range(..., consumername=""), redis-py drops the consumer argument because of a truthiness check (if consumername:) and returns the pending entries of every consumer in the group. A worker that then claims or acknowledges that result acts on other consumers' messages. The same function already treats "" as a provided consumer name in its early-return validation (consumername is not None), so the two code paths disagree.

Rejecting empty names in xreadgroup and the other consumer commands would diverge from Redis semantics and break callers whose groups already contain such a consumer. This change instead sends the argument whenever it is not None, which keeps redis-py aligned with the server and makes the filter return what was asked for. The default (consumername=None, no filter) is unchanged, and the docstring now says that only None omits the filter. The command builder is shared by the sync and async clients.

Testing

  • New sync and async regression tests create pending entries for two consumers, then check the unfiltered result and the filtered result for "", b"", "consumer1" and b"consumer1". Against Redis 8.2.1, the 6 empty-name cases fail without the fix, and all 12 pass with it.
  • All XPENDING tests pass in RESP2/RESP3 × legacy/unified response modes, with single-connection and pool clients. invoke linters (Ruff check, Ruff format and Vulture) passes.
  • I could not run the cluster, TLS and module tests locally, since they need infrastructure I don't have.

AI assistance: OpenAI Codex assisted with the investigation, implementation and tests, and Claude Code helped revise this description based on the review.

Pull Request check-list

Please make sure to review and check all of these items:

  • Do tests and lints pass with this change? (XPENDING tests and all linters pass; see Testing for what I could not run locally.)
  • Do the CI tests pass with this change (enable it first in your forked repo and wait for the github action build to finish)?
  • Is the new or changed code fully tested?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Is there an example added to the examples folder (if applicable)? (Not applicable to this fix.)

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.


Note

Medium Risk
Touches stream consumer-group backlog inspection; the change corrects filtering behavior that could cause workers to see or act on other consumers’ pending messages when using empty consumer names.

Overview
Fixes xpending_range so consumername="" (or b"") is sent to Redis instead of being treated as “no filter.” Previously a truthiness check dropped the argument, so callers with legitimately empty consumer names saw every pending message in the group—misaligned with validation that already treated any non-None value as an explicit filter.

The command builder now appends consumername when it is not None; the docstring states that only None omits the consumer filter. Sync and async tests cover filtering for empty and non-empty consumer names (str and bytes).

Reviewed by Cursor Bugbot for commit d8ac3c9. Bugbot is set up for automated code reviews on this repo. Configure here.

Preserve explicitly supplied empty consumer names instead of omitting the filter. Cover sync and async clients with empty and nonempty str/bytes names.

Assisted-by: OpenAI Codex (GPT-6)

@petyaslavova petyaslavova left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @LindseyZ1205, thank you for your contribution!

The change looks good to me, but the description reads as if empty consumer names are something users want, and that invites the question of whether redis-py should reject them instead. Could you update it to explain how a caller ends up here and why the fix belongs in the filter? Something along these lines:

Redis accepts an empty consumer name: XREADGROUP GROUP <group> "" ... succeeds and creates a consumer named "". This is usually accidental, for example a worker name read from an unset environment variable or config value that defaults to "". Reading works, so nothing surfaces the mistake.

When that worker later inspects its own backlog with xpending_range(..., consumername=""), redis-py drops the filter because of a truthiness check and returns the pending entries of every consumer in the group. A worker that then claims or acknowledges that result acts on other consumers' messages. The same function already treats "" as a provided consumer name in its early-return validation, so the two code paths disagree.

Rejecting empty names in xreadgroup and the other consumer commands would diverge from Redis semantics and break callers whose groups already contain such a consumer. Sending the argument whenever it is not None keeps redis-py aligned with the server and makes the filter return what was asked for.

With that in place I am happy to approve CI and merge.

@petyaslavova petyaslavova added maintenance Maintenance (CI, Releases, etc) waiting-for-response labels Oct 2, 2026
@LindseyZ1205

Copy link
Copy Markdown
Contributor Author

Hey @LindseyZ1205, thank you for your contribution!

The change looks good to me, but the description reads as if empty consumer names are something users want, and that invites the question of whether redis-py should reject them instead. Could you update it to explain how a caller ends up here and why the fix belongs in the filter? Something along these lines:

Redis accepts an empty consumer name: XREADGROUP GROUP <group> "" ... succeeds and creates a consumer named "". This is usually accidental, for example a worker name read from an unset environment variable or config value that defaults to "". Reading works, so nothing surfaces the mistake.
When that worker later inspects its own backlog with xpending_range(..., consumername=""), redis-py drops the filter because of a truthiness check and returns the pending entries of every consumer in the group. A worker that then claims or acknowledges that result acts on other consumers' messages. The same function already treats "" as a provided consumer name in its early-return validation, so the two code paths disagree.
Rejecting empty names in xreadgroup and the other consumer commands would diverge from Redis semantics and break callers whose groups already contain such a consumer. Sending the argument whenever it is not None keeps redis-py aligned with the server and makes the filter return what was asked for.

With that in place I am happy to approve CI and merge.

Thanks @petyaslavova, that's a much clearer way to frame it. I've updated the description along those lines.

@petyaslavova petyaslavova left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating the description, it reads well now. Approving and merging once CI is green.

@petyaslavova
petyaslavova merged commit 1f084f8 into redis:master Oct 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

maintenance Maintenance (CI, Releases, etc)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants