chore(refactor): move per-connection CLIENT commands to statefulCmdable - #3961
chore(refactor): move per-connection CLIENT commands to statefulCmdable#3961ndyakov wants to merge 11 commits into
Conversation
CLIENT TRACKING (and its On/Off variants) and CLIENT MAINT_NOTIFICATIONS mutate per-connection state, exactly like CLIENT SETNAME/SETINFO, but lived on cmdable: on a pooled client they executed on an arbitrary pool connection that was immediately returned to the pool, so the flag applied to a connection the caller could not address again — and for CLIENT TRACKING the invalidation pushes arrived on a connection nothing was reading. They now live on statefulCmdable, next to ClientSetName, where the connection is pinned: usable on *redis.Conn (Client.Conn), Pipeline and Tx. The pooled clients — Client, ClusterClient, Ring and AutoPipeliner — keep same-named methods so the Cmdable and UniversalClient interfaces are unchanged and existing code still compiles. Those methods now return a pre-failed StatusCmd with guidance instead of silently misconfiguring one random connection: CLIENT TRACKING points at Client.Conn, Pipeline/Tx or the built-in client-side cache (Options.ClientSideCacheConfig); CLIENT MAINT_NOTIFICATIONS points at Options.MaintNotificationsConfig, which the library already applies to every connection during connection init. The returned command carries the real command name and arguments so instrumentation that inspects commands still sees them. The library's own uses were already per-connection (the connection-init handshake issues CLIENT TRACKING ON through the pinned init pipeline and CLIENT MAINT_NOTIFICATIONS through the pinned init wrapper) and are unaffected. The option-validation spec moves to a dedicated connection, and a new spec pins the pooled-client rejection on the raw client and both autopipeliner faces, plus the working Conn path.
The pooled-client ClientTracking methods are now rejected first as per-connection state — the general rule, independent of CSC — so the CSC-specific error is no longer reachable through them. It remains the backstop on the paths that actually reach a connection: the raw Do escape hatch (asserted here) and pipelines (the _Pipeline test). The test now asserts each layer with the error that layer produces.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 335ee0556b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
The pooled-client rejection spec ended by opening a dedicated connection and asserting CLIENT TRACKING ON/OFF succeed with no server-support gate, so it would fail on RE/ACL-restricted targets where TRACKING is unavailable -- even though the behavior under test (client-side rejection) already passed. Gate the dedicated-connection tail with skipIfClientTrackingUnavailable, matching the sibling specs at commands_test.go:409 and :432; the client-side rejection assertions above stay ungated (they need no server support).
|
@codex review |
There was a problem hiding this comment.
Pull request overview
This PR refactors per-connection Redis CLIENT commands—specifically CLIENT TRACKING (and On/Off) plus CLIENT MAINT_NOTIFICATIONS—so the working implementations live on statefulCmdable (usable on pinned connections like Client.Conn, Pipeline, and Tx). For pooled clients (Client, ClusterClient, Ring, AutoPipeliner), the same method names remain for interface compatibility but now return a pre-failed StatusCmd` with guidance instead of silently mutating an arbitrary pooled connection.
Changes:
- Move
ClientTracking*andClientMaintNotificationsfromcmdabletostatefulCmdableso they run only on pinned connections. - Add pooled-client wrappers that reject these calls with explanatory errors while preserving a
StatusCmdreturn shape. - Update tests to validate option-checking on a dedicated connection and assert pooled-client rejection behavior (including autopipeliner).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
commands.go |
Moves CLIENT TRACKING and CLIENT MAINT_NOTIFICATIONS to statefulCmdable and documents intended usage on pinned connections. |
redis.go |
Adds pooled-client rejection errors and wrapper methods for Client that return pre-failed StatusCmds. |
osscluster.go |
Adds pooled-client rejection wrappers for ClusterClient to keep Cmdable compatibility. |
ring.go |
Adds pooled-client rejection wrappers for Ring to keep Cmdable compatibility. |
autopipeline.go |
Adds pooled-client rejection wrappers for AutoPipeliner. |
commands_test.go |
Updates specs to validate tracking option validation via a dedicated connection and asserts pooled-client rejection paths. |
csc_test.go |
Updates CSC-related tests/comments to reflect the new “pooled-client rejection first” layering. |
Suppressed comments (4)
redis.go:2141
- This pooled-client error message references
Options.MaintNotificationsConfig, but the same error is returned by ClusterClient/Ring/AutoPipeliner which use different option structs. Rewording to a type-agnostic "set MaintNotificationsConfig in client options" keeps the guidance correct everywhere.
errClientMaintNotificationsOnPooledClient = errors.New(
"redis: CLIENT MAINT_NOTIFICATIONS is per-connection state and cannot be applied through a connection pool; " +
"set Options.MaintNotificationsConfig to enable it on every connection automatically, " +
"or use a dedicated connection (Client.Conn) for manual control")
redis.go:2181
- The pooled-client
ClientMaintNotificationswrapper currently constructs a StatusCmd with onlyclient maint_notifications, dropping the requestedon/offand endpoint-type args. If this is meant to mirror the real command args for instrumentation, it should includeenabled/endpointTypelike the stateful implementation does.
func (c *Client) ClientMaintNotifications(ctx context.Context, enabled bool, endpointType string) *StatusCmd {
return pooledConnStateCmd(ctx, errClientMaintNotificationsOnPooledClient, "client", "maint_notifications")
}
osscluster.go:2655
- The pooled
ClientMaintNotificationswrapper currently constructs a StatusCmd with onlyclient maint_notifications, dropping the requestedon/offand endpoint-type args. If this cmd is meant to mirror real arguments for instrumentation, include them the same way as the stateful implementation does.
// ClientMaintNotifications on a pooled cluster client fails with guidance;
// set ClusterOptions.MaintNotificationsConfig instead.
func (c *ClusterClient) ClientMaintNotifications(ctx context.Context, enabled bool, endpointType string) *StatusCmd {
return pooledConnStateCmd(ctx, errClientMaintNotificationsOnPooledClient, "client", "maint_notifications")
}
autopipeline.go:1097
- After moving the
Watchdoc comment out of theClientTrackingcomment group, add it back immediately aboveWatchso godoc/users still see it on the correct method.
func (ap *AutoPipeliner) ClientMaintNotifications(ctx context.Context, enabled bool, endpointType string) *StatusCmd {
return pooledConnStateCmd(ctx, errClientMaintNotificationsOnPooledClient, "client", "maint_notifications")
}
func (ap *AutoPipeliner) Watch(ctx context.Context, fn func(*Tx) error, keys ...string) error {
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05634f64e6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
- Pooled ClientTracking/ClientTrackingOn/ClientMaintNotifications (Client, Ring, ClusterClient, AutoPipeliner) now build the FULL argument list before failing with guidance (via appendClientTrackingOptions and moving-endpoint-type), mirroring the stateful commands, so the returned command's Args() reflect the real command instead of dropping the caller's options. - Error strings drop the Options. type prefix (correct for Ring/Cluster too) and no longer suggest a pooled Pipeline/Tx (a pooled pipeline releases the conn after Exec, losing tracking) -- guidance points to Client.Conn / built-in CSC. - Ring doc corrected: no RingOptions.MaintNotificationsConfig field; point at the per-shard Options via RingOptions.NewClient. - Autopipeliner ClientTracking* overrides now return ErrClosed on a closed autopipeliner, matching every other dispatch path. - Moved the stray "Watch runs a transactional function" godoc off ClientTracking onto func Watch.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fadbf2c23c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (9)
redis.go:2174
- ClientTrackingOff on a closed client should return ErrClosed (consistent with the rest of the API and ErrClosed’s doc), but the pooled-client stub always returns the guidance error. Add a closed check before returning the pre-failed command.
func (c *Client) ClientTrackingOff(ctx context.Context) *StatusCmd {
return pooledConnStateCmd(ctx, errClientTrackingOnPooledClient, "client", "tracking", "off")
redis.go:2192
- ClientMaintNotifications’ pooled-client stub should prefer ErrClosed once the client has been closed, but it currently always returns the guidance error. This can be surprising for callers that expect redis.ErrClosed after Close().
args = append(args, "on", "moving-endpoint-type", endpointType)
} else {
args = append(args, "off")
}
return pooledConnStateCmd(ctx, errClientMaintNotificationsOnPooledClient, args...)
ring.go:985
- ClientTrackingOff should return ErrClosed after the Ring has been closed (consistent with ringSharding.GetByKey/Process), but this stub always returns the guidance error instead.
func (c *Ring) ClientTrackingOff(ctx context.Context) *StatusCmd {
return pooledConnStateCmd(ctx, errClientTrackingOnPooledClient, "client", "tracking", "off")
}
ring.go:999
- ClientMaintNotifications on a closed Ring should return ErrClosed, but the pooled stub currently always returns the guidance error without considering ringSharding’s closed state.
args = append(args, "on", "moving-endpoint-type", endpointType)
} else {
args = append(args, "off")
}
return pooledConnStateCmd(ctx, errClientMaintNotificationsOnPooledClient, args...)
osscluster.go:2651
- ClientTrackingOff should return ErrClosed after ClusterClient.Close(), but this pooled-client stub always returns the guidance error instead.
func (c *ClusterClient) ClientTrackingOff(ctx context.Context) *StatusCmd {
return pooledConnStateCmd(ctx, errClientTrackingOnPooledClient, "client", "tracking", "off")
}
osscluster.go:2665
- ClientMaintNotifications’ pooled ClusterClient stub should return ErrClosed once the client has been closed, but it currently always returns the guidance error without consulting c.nodes.closed.
args = append(args, "on", "moving-endpoint-type", endpointType)
} else {
args = append(args, "off")
}
return pooledConnStateCmd(ctx, errClientMaintNotificationsOnPooledClient, args...)
redis.go:2169
- These pooled-client wrappers always return the guidance error even after the client has been closed. That violates the package contract that operations on a closed client return ErrClosed, and it’s also inconsistent with the AutoPipeliner wrappers which preserve ErrClosed when closed. Consider returning ErrClosed when the client is closing/closed (e.g., via the baseClient apClosed flag).
This issue also appears in the following locations of the same file:
- line 2173
- line 2188
args := []interface{}{"client", "tracking", "on"}
if opt != nil {
args = appendClientTrackingOptions(args, opt)
}
return pooledConnStateCmd(ctx, errClientTrackingOnPooledClient, args...)
ring.go:979
- After Ring.Close(), most operations return ErrClosed via ringSharding’s closed flag, but these pooled-client stubs always return the guidance error. This breaks the usual closed-client behavior; consider checking c.sharding.closed (under its lock) and returning ErrClosed when closed.
This issue also appears in the following locations of the same file:
- line 983
- line 995
args := []interface{}{"client", "tracking", "on"}
if opt != nil {
args = appendClientTrackingOptions(args, opt)
}
return pooledConnStateCmd(ctx, errClientTrackingOnPooledClient, args...)
osscluster.go:2645
- These pooled ClusterClient stubs always return the guidance error even after Close(), but the rest of ClusterClient operations return ErrClosed when c.nodes is closed. Consider checking c.nodes.closed (under its lock) and returning ErrClosed when the cluster client is closed.
This issue also appears in the following locations of the same file:
- line 2649
- line 2661
args := []interface{}{"client", "tracking", "on"}
if opt != nil {
args = appendClientTrackingOptions(args, opt)
}
return pooledConnStateCmd(ctx, errClientTrackingOnPooledClient, args...)
*Pipeline overrides ClientTracking/ClientTrackingOn/Off/ClientMaintNotifications: on a pooled pipeline (Client/Ring/Cluster/Tx) they fail with the pooled-client guidance error and are not queued (the borrowed conn returns to the pool after Exec); on a pipeline from a dedicated *Conn (new Pipeline.sticky) they are queued normally. Adds TestPipelineRejectsStateCommandsWhenPooled.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4198290800
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
A pooled Pipeline's ClientTracking*/ClientMaintNotifications overrides returned a pre-failed command without queuing it, so Pipelined/Exec saw an empty batch and returned nil — the rejection was silently dropped and TestClientTrackingRejectedWithCSC_Pipeline failed across the CI matrix. Queue the pre-failed command with a stateRejected marker; generalProcessPipeline returns its guidance error (or the CSC-specific error first, for CSC clients) without sending it to a borrowed connection. Also mark Tx pipelines sticky: WATCH pins one connection for the Tx lifetime, so CLIENT TRACKING belongs there rather than being rejected as pooled.
setup-go's "1.26.x" resolved to go1.26.5, which govulncheck flags for two standard-library vulnerabilities fixed in go1.26.6: GO-2026-6090 (crypto/tls) and GO-2026-5972 (encoding/asn1). Track the latest stable toolchain so future security patches are picked up automatically instead of pinning a patch.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b55e0f9655
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
ClusterClient.processPipeline/processTxPipeline bypass baseClient.generalProcessPipeline, so a per-connection state command a pooled pipeline queued for rejection (stateRejected) was still routed and written to a node connection, its reply overwriting the guidance error. Both cluster entries now check the shared stateRejectedErr helper before any node mapping. Adds TestClusterPipelineRejectsStateCommands (dial-free).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23f6ed4c53
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Ring.generalProcessPipeline shards a batch and dispatches shard groups concurrently, so a queued pooled-state rejection (stateRejected) only failed its own shard group while the other groups still executed. Check the shared stateRejectedErr helper before sharding — matching baseClient and the cluster entries — so the whole batch fails before any dispatch. Adds TestRingPipelineRejectsStateCommands (dial-free; asserts a keyed command in the same batch carries the guidance error, proving its shard group never ran).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 57d18a94b6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Suppressed comments (2)
redis.go:1620
- The CSC guard returns
errClientTrackingWithCSC, butsetCmdsErrpreserves the pooled rejection already stored on this command. Consequently, even a one-command pipeline returns the CSC error fromExecwhile that command'sErr()remainserrClientTrackingOnPooledClient, violatingPipeline.Exec's contract that it returns the first failed command's error. Set the detected command's error to the guard error before propagating it to the batch, and assert both errors in the CSC pipeline test.
if err := c.cscCommandError(cmd); err != nil {
setCmdsErr(cmds, err)
return err
}
pipeline.go:82
- This comment incorrectly classifies a pipeline from
Txas pooled, while bothTx.PipelineandTx.TxPipelinenow setsticky: true. DocumentTxalongsideConnso the explanation matches the new behavior.
// are per-connection state. On a POOLED pipeline (from a Client/Ring/ClusterClient,
// or a Tx) the connection is borrowed for Exec and returned to the pool afterwards,
// so queuing these would leave state on an arbitrary connection — reject them with
// guidance, exactly like the pooled-client wrappers. On a pipeline from a dedicated
// *Conn (sticky) they are queued normally, since the state stays on that connection.
Comment-only: the pooled-state rejection comments are reduced to their invariants (fail-before-dispatch in every pipeline entry, sticky semantics, queue-so-Exec-surfaces), with the rationale deduplicated into the sticky field doc and the stateRejectedErr helper doc.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit cdafb4e. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cdafb4e8da
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
- On a CSC client, Exec returned the CSC-specific error while a queued pooled-state rejection's command kept its pre-set pooled error (setCmdsErr skips already-errored commands). The CSC guard now overwrites the triggering command's error so Exec and Err() agree. - cloneBaseCmd copies the stateRejected flag, so a hook rebuilding the command slice via Clone cannot launder a queued rejection into a sendable command.
CLIENT TRACKING (and its On/Off variants) and CLIENT MAINT_NOTIFICATIONS mutate per-connection state, exactly like CLIENT SETNAME/SETINFO, but lived on cmdable: on a pooled client they executed on an arbitrary pool connection that was immediately returned to the pool, so the flag applied to a connection the caller could not address again — and for CLIENT TRACKING the invalidation pushes arrived on a connection nothing was reading. They now live on statefulCmdable, next to ClientSetName, where the connection is pinned: usable on *redis.Conn (Client.Conn), Pipeline and Tx.
The pooled clients — Client, ClusterClient, Ring and AutoPipeliner — keep same-named methods so the Cmdable and UniversalClient interfaces are unchanged and existing code still compiles. Those methods now return a pre-failed StatusCmd with guidance instead of silently misconfiguring one random connection: CLIENT TRACKING points at Client.Conn, Pipeline/Tx or the built-in client-side cache (Options.ClientSideCacheConfig); CLIENT MAINT_NOTIFICATIONS points at Options.MaintNotificationsConfig, which the library already applies to every connection during connection init. The returned command carries the real command name and arguments so instrumentation that inspects commands still sees them.
The library's own uses were already per-connection (the connection-init handshake issues CLIENT TRACKING ON through the pinned init pipeline and CLIENT MAINT_NOTIFICATIONS through the pinned init wrapper) and are unaffected. The option-validation spec moves to a dedicated connection, and a new spec pins the pooled-client rejection on the raw client and both autopipeliner faces, plus the working Conn path.
Review refinements
On a POOLED Pipeline/TxPipeline the rejected state command is now QUEUED
(flagged internally) so
Exec/Pipelinedsurface the guidance error even whenthe callback ignores the returned command — enforced fail-before-dispatch in
every pipeline entry point (standalone, ClusterClient, Ring; a batch mixing
keyed commands never partially executes). Pipelines from a dedicated
*ConnAND from a
*Txare sticky (WATCH pins one connection for the Tx lifetime),so state commands queue there normally, matching the direct
tx.ClientTracking*face.
Note
Medium Risk
Touches pipeline execution paths across client types and changes observable errors for pooled CLIENT TRACKING/MAINT_NOTIFICATIONS callers, but behavior is intentionally stricter with broad test coverage and interface compatibility preserved.
Overview
CLIENT TRACKING and CLIENT MAINT_NOTIFICATIONS move to
statefulCmdablefor real execution on pinned connections (Conn, sticky pipelines). PooledClient,ClusterClient,Ring, andAutoPipelinerkeep the same method names but return pre-failedStatusCmds with guidance (and fullArgs()for observability) instead of mutating a random pool connection.On pooled
Pipeline/TxPipeline, those commands are still queued but markedstateRejected;Exec/Pipelinedfail before dispatch viastateRejectedErrin standalone, cluster, and ring paths—so ignored return values and mixed batches cannot partially run or silently succeed. Sticky pipelines (Conn,Txwithsticky: true) queue and send them normally.Tests cover args preservation, pooled vs conn pipelines, cluster/ring fail-before-dispatch, and Tx stickiness. CI govulncheck uses Go
stableinstead of1.26.x.Reviewed by Cursor Bugbot for commit 3b6008a. Bugbot is set up for automated code reviews on this repo. Configure here.