Skip to content

docs(cluster): say which async ClusterPipeline methods must be awaited - #4264

Open
ckarnell wants to merge 3 commits into
redis:masterfrom
ckarnell:docs/cluster-pipeline-awaitable-methods
Open

docs(cluster): say which async ClusterPipeline methods must be awaited#4264
ckarnell wants to merge 3 commits into
redis:masterfrom
ckarnell:docs/cluster-pipeline-awaitable-methods

Conversation

@ckarnell

@ckarnell ckarnell commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Follow-up you invited on #4259: making it clear which ClusterPipeline methods have to be awaited.

The confusion is visible in the class docstring itself. The usage example chains .delete("A", "B", "K").execute() without awaiting the delete, and the note underneath lists UNLINK right beside DELETE as though the two behave alike. They do not. unlink is a coroutine that returns None, so it neither chains nor stages anything unless you await it.

So the docstring now lists the ten coroutine methods, says plainly that they return None instead of the pipeline, and shows the contrast:

pipe = rc.pipeline()
pipe.delete("A")          # stages, chainable
await pipe.unlink("B")    # stages, must be awaited, returns None
await pipe.execute()

While checking the claim I found a third case worth a line: inside a multi() block, TransactionStrategy overrides execute_command and returns a response instead of the pipeline, so commands do not chain there either. My first draft said staged commands always return the pipeline, which was wrong outside the default strategy.

Also filled in two gaps next door: discard() had an empty docstring, and unlink() had none. The unlink one records that outside a transaction it takes a single key and raises RedisClusterException for more, which is currently only discoverable by reading PipelineStrategy.

Docs only, no behaviour change. ruff check and vulture --min-confidence 80 are clean. ruff format --check reports this file, but it does so on an unmodified checkout too, so I left it alone instead of reformatting code I did not touch.

The list of ten was generated from the class, not typed by hand, so it includes the himport_* methods I would otherwise have missed.


Note

Low Risk
Documentation-only changes with no runtime or API behavior modifications.

Overview
Async ClusterPipeline gets a new docstring section that lists the ten coroutine methods that must be awaited individually, which of those return None (breaking chaining), and warns that inherited command coroutines are not pipeline steps—awaiting them re-initializes the pipeline.

It also documents the unlink vs delete contrast (delete chains; async unlink must be awaited), WATCH → MULTI behavior (commands between them run immediately and must be awaited; after multi() staging chains again), and notes on non-transactional watch/unwatch/discard and himport_* registry behavior.

discard() and unlink() docstrings are filled in on both async (redis/asyncio/cluster.py) and sync (redis/cluster.py) cluster pipelines, including the single-key-only unlink rule outside transactions.

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

Follow-up invited on redis#4259. The class docstring chains .delete(...) without
awaiting, and its note lists UNLINK beside DELETE, but unlink is a coroutine
that returns None, so it neither chains nor runs unless awaited. That is the
confusion redis#4259 came from.

Lists the ten coroutine methods, contrasts staging with awaiting, notes that
a transactional block behaves differently again because execute_command
returns a response there rather than the pipeline, and fills in the empty
discard() docstring and the missing unlink() one.

Docs only, no behaviour change.

@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 @ckarnell, thank you for your contribution! This is a great follow-up, and the unlink and discard docstrings are accurate and useful. Three corrections are needed in the class docstring before we can merge it.

  1. They return None rather than the pipeline does not hold for half the list: execute returns List[Any], initialize returns the pipeline (that is what await pipe and __aenter__ use), himport_prepare returns bool, and himport_discard/himport_discard_all return int. Only reset, discard, watch, unwatch and unlink return None.
  2. The multi() paragraph is inverted. TransactionStrategy._execute_command only sends immediately when self._watching or args[0] in IMMEDIATE_EXECUTE_COMMANDS and not self._explicit_transaction; multi() sets _explicit_transaction = True, so inside a multi() block commands are queued and do return the pipeline. The immediate, must-be-awaited case is between watch() and multi() — see test_transaction_with_watched_keys, where await pipe.get("a") is awaited while watching and pipe.set(...) after multi() is not.
  3. Please scope the list of ten, since it only covers methods defined on ClusterPipeline. Inherited coroutines such as command_info, cluster_delslots, client_tracking_on/off, hotkeys_* and the *scan_iter generators are not included, and watch/unwatch/discard raise RedisClusterException outside a transactional pipeline while himport_* stage nothing.

Could you also mirror the discard() and unlink() docstrings on the sync ClusterPipeline in redis/cluster.py? The same empty/missing docstrings and the same single-key restriction are there, and we keep the two stacks aligned. Once those points are addressed I am happy to merge this.

@petyaslavova petyaslavova added maintenance Maintenance (CI, Releases, etc) waiting-for-response labels Aug 14, 2026
@ckarnell

Copy link
Copy Markdown
Contributor Author

All three were right, fixed in 2c5a4a3.

The return types are corrected. execute returns List[Any], initialize returns the pipeline, himport_prepare returns bool, and both himport_discard variants return int. Five were wrong.

The multi() paragraph was backwards, my error. multi() sets _explicit_transaction, and the immediate-send branch requires not self._explicit_transaction, so inside multi() commands queue and do chain, which is what test_transaction_with_watched_keys shows. Rewrote it to say that.

I also added the twelve inherited coroutines that sat outside the list, including the four you named and the hotkeys_ family, and mirrored the docstrings onto the sync ClusterPipeline, since sync discard had an empty docstring and unlink had none.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2c5a4a322b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread redis/asyncio/cluster.py Outdated
Comment on lines +2484 to +2485
such as ``command_info``, ``cluster_delslots``, ``client_tracking_on`` and the
``hotkeys_*`` family must be awaited too and are not listed here.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Avoid recommending inherited async commands on pipelines

On an async cluster pipeline this guidance makes commands like client_tracking_on and cluster_delslots look like supported pipeline steps, but those inherited async client methods assume execute_command is awaitable. Here ClusterPipeline.execute_command returns the pipeline, so await pipe.client_tracking_on() awaits ClusterPipeline.__await__ and reinitializes/clears the staged command, while cluster_delslots tries to create tasks from pipeline objects; users following this doc can silently drop queued work or hit TypeError instead of staging a command.

AGENTS.md reference: AGENTS.md:L176-L179

Useful? React with 👍 / 👎.

@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 @ckarnell, thanks for the quick turnaround - the return types, the WATCH → MULTI example and the sync mirror all check out now. Two things still need a fix before merge.

  1. The inherited-coroutines paragraph names the wrong examples. Of the four, only client_tracking_on/client_tracking_off actually await the pipeline and clear the queue (they await self.client_tracking(...), which returns the pipeline). command_info raises NotImplementedError (redis/commands/core.py), the cluster hotkeys_* methods raise NotImplementedError (redis/commands/cluster.py), and cluster_delslots wraps execute_command in asyncio.create_task, which raises TypeError on a pipeline object. Please use client_tracking_on/client_tracking_off for the discard-the-queue warning and mention the raising methods separately.

  2. On the sync class the unlink docstring's "unlike delete" contrast does not hold: ClusterPipeline.delete also returns None and PipelineStrategy.delete raises the same multiple-keys RedisClusterException. In sync, neither chains and both are single-key outside a transaction, so please reword it for that stack.

Two optional extras while you are in there: "commands are sent as they are issued" is really "executed when awaited" (TransactionStrategy._execute_command hands back the un-awaited _immediate_execute_command coroutine), and the note above your new section still lists UNLINK as split across nodes even though the pipeline accepts only one key. Fixing those in the same PR would be welcome. Once points 1 and 2 are in, I am happy to merge.

@Mukller Mukller left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Docs-only PR — I fact-checked the new docstring against the code on master, and every claim holds:

  1. The ten listed methods are coroutines defined on ClusterPipeline: verified via inspect.iscoroutinefunction + __dict__ membership for watch, unwatch, unlink, discard, reset, execute, initialize, himport_prepare, himport_discard, himport_discard_all — all True on both counts.
  2. delete vs unlink asymmetry is real: delete is an inherited staging command (not a coroutine, chainable), while unlink is a coroutine defined on the class that must be awaited. The example calling this out is the most valuable part of the doc — it's exactly the trap.
  3. "Outside a transactional pipeline watch/unwatch/discard raise RedisClusterException": confirmed — the non-transactional execution strategy raises RedisClusterException("method ... is not supported outside of transactional context").
  4. watch→multi immediate-send semantics: consistent with the strategy implementations referenced above.

This documents behavior that today can only be learned by reading three classes' worth of source or by getting bitten at runtime (await pipe.unlink(...) vs pipe.delete(...)). The level of detail is appropriate given how surprising the staging/await split is.

One tiny suggestion, non-blocking: since himport_* "act on the shared fieldset registry rather than staging", they're arguably a separate concern from the await-list of pipeline steps — if you ever revisit, splitting them into their own paragraph would make the pipeline-relevant list even sharper. Fine as-is though.

CI on the PR was green when I checked; no product code touched.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

maintenance Maintenance (CI, Releases, etc) waiting-for-response

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants