fix(cluster): populate ClusterPipeline.command_stack via execution strategy (#3703) - #4130
Conversation
…rategy (redis#3703) After the execution-strategy refactor (redis#3611), queued commands moved to `ClusterPipeline._execution_strategy.command_queue`, leaving the public `command_stack` attribute permanently empty on the sync cluster pipeline (and absent entirely on the async one). APM/tracing integrations such as Datadog's `ddtrace` introspect `command_stack` to build pipeline spans, so cluster pipeline traces silently lost all command information. Expose `command_stack` as a read-only property that delegates to the active execution strategy's `command_queue` on both the sync and async `ClusterPipeline`, restoring the legacy behavior without changing execution. No runtime deprecation warning is emitted (tracing tools read this on every execute); it is documented as legacy in the docstring instead. Approach proposed by @mathewlee11 and approved by the maintainers in redis#3703. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Hi, I’m Jit, a friendly security platform designed to help developers build secure applications from day zero with an MVS (Minimal viable security) mindset. In case there are security findings, they will be communicated to you as a comment inside the PR. Hope you’ll enjoy using Jit. Questions? Comments? Want to learn more? Get in touch with us. |
|
Hey @nazarli-shabnam, thank you for your contribution! I'll have a look at it shortly. |
@petyaslavova so, good to go? |
eeshsaxena
left a comment
There was a problem hiding this comment.
Confirmed the regression, and it's a real silent break. On master, ClusterPipeline.__init__ sets self.command_stack = [] and then nothing ever writes to it again - after the execution-strategy refactor (#3611) queued commands land in self._execution_strategy._command_queue, so pipeline.command_stack stays [] forever. Anything that introspects it (ddtrace's redis integration reads instance.command_stack to record the pipelined commands) sees an empty list and records nothing. Replacing the dead attribute with a property that delegates to self._execution_strategy.command_queue is the right fix, and returning the live list (so command_stack[i].args works) matches what those consumers expect. Nice that both sync and async are covered with tests.
One compatibility angle worth deciding on: the property is read-only, whereas on the regular (non-cluster) Pipeline, command_stack is a plain writable list attribute, and Pipeline.reset() reassigns it with self.command_stack = []. So code that treats the two pipeline types uniformly and does pipe.command_stack = [] (or pipe.command_stack = something) would now raise AttributeError: can't set attribute on a cluster pipeline. In-place mutation (.clear(), .append()) still works since you hand back the live list, and ddtrace only reads, so this is likely fine in practice - but since the execution strategy already exposes a command_queue setter, mirroring it with a command_stack.setter that assigns through to self._execution_strategy.command_queue would make the cluster pipeline behave identically to the non-cluster one for very little extra code, and remove a subtle behavioural difference between them.
Minor: the docstring says "Deprecated" - if the intent is that this stays as the compatibility surface for tracing integrations, it might read better as "compatibility shim" than "deprecated", since deprecating it would push those same integrations toward len(pipeline), which doesn't give them the command args they actually need.
Deferring to the maintainers - the core fix is correct.
Description of change
Fixes #3703.
After the execution-strategy refactor in #3611, queued commands moved into
ClusterPipeline._execution_strategy.command_queue. The publiccommand_stackattribute on the syncClusterPipelinewas left initialized to[]and never populated again, and the asyncClusterPipelinehad nocommand_stackat all.APM/tracing integrations (e.g. Datadog's
ddtrace) introspectcommand_stackto build pipeline spans, so after upgrading, cluster pipeline traces silently lost all command information (commands showed up blank). The non-clusterPipelinestill populatescommand_stackcorrectly, so this was a cluster-only regression.Change: expose
command_stackas a read-only property that delegates to the active execution strategy'scommand_queue, on both the sync and asyncClusterPipeline. This restores the legacy introspection behavior without altering execution.len(pipeline)already delegates to the same queue, so the two stay consistent.Per the discussion in #3703, no runtime deprecation warning is emitted — tracing tools read this on every pipeline execute, so a per-access warning would be noise. It is documented as legacy in the docstring instead. Both
PipelineStrategyandTransactionStrategyare covered.This follows the approach proposed by @mathewlee11 and approved by the maintainers in the issue, applied to both the sync and async cluster clients for parity.
Pull Request check-list
ruff check,ruff format --check,vultureall clean; new tests pass)tests/test_cluster.pyandtests/test_asyncio/test_cluster.py)Note
Low Risk
Read-only delegation to the existing command queue; pipeline execution behavior is unchanged aside from restoring introspection for APM/tracing.
Overview
Fixes a cluster-only regression (#3703) where queued pipeline commands lived on the execution strategy’s
command_queueafter #3611, butcommand_stackstayed empty on sync pipelines and was missing on async—breaking tools like Datadogddtracethat read it for span metadata.command_stackis now a read-only property on sync and asyncClusterPipelinethat returns_execution_strategy.command_queue. Sync__init__no longer sets a uselesscommand_stack = []. The execution-strategy ABC gainscommand_queue, implemented onAbstractStrategy. Docstrings mark the API as legacy;len(pipeline)still reflects the same queue.Regression tests cover sync (including
transaction=True) and async pipelines.Reviewed by Cursor Bugbot for commit 05cf31a. Bugbot is set up for automated code reviews on this repo. Configure here.