feat(autopipeline): ordered full-duplex dispatch - #3964
Conversation
The dedicated pipeline connection pool was created only when
PipelineReadBufferSize or PipelineWriteBufferSize was set. Setting just
PipelinePoolSize therefore produced no pipeline pool at all, with no error and
no log line, and every pipelined operation silently kept using the main
connection pool — competing with ordinary commands for the same connections,
which is the opposite of what the option asks for.
The gating was inverted. The pool size is the primary intent ("give pipelines
their own N connections"); the buffer sizes are tuning. And the buffer sizes
already supplied a default for the size (10), so the dependency only ran one
way.
Any of the three options now creates the pool. Enabling it by size inherits the
regular ReadBufferSize/WriteBufferSize, so a caller who only wants to bound
pipeline connections no longer has to know that a buffer size is secretly
required.
Found while measuring client-side caching over a high-latency link: the CSC
background batched reads go through withPipelineConn, which falls back to the
regular pool when none exists, so they were contending with the very reader
misses they exist to prevent.
Tests cover all four option combinations and were verified to fail with the
gate reverted.
…by default Autopipelining is pipeline-heavy by definition, yet the dedicated pipeline pool still had to be enabled separately; without it every batch competes with regular commands for main-pool connections. Two paths now default it in: Config time: Options.init defaults PipelinePoolSize to DefaultPipelinePoolSize (new exported const, 10) when AutoPipelineOptions is set. Cluster node clients do not inherit AutoPipelineOptions (the cluster autopipeliner lives at cluster level and dispatches into the node clients' pipeline hooks), so clientOptions applies the same default there. Sentinel and failover node clients inherit AutoPipelineOptions and are covered by Options.init; Ring does not support autopipelining. A negative PipelinePoolSize opts out explicitly. Runtime: the common pattern configures the autopipeliner after NewClient (AutoPipelineWithOptions on a plain client), where pool creation has already happened. ensurePipelinePool now builds the pool lazily when an autopipeliner is created. baseClient.pipelinePool becomes a pointer to an atomic slot (pool + pool-name ref) so the lazy publication is race-free against concurrent Pipelined callers on the hot path, and WithTimeout clones share the slot — two sharers cannot install two different pools (CAS; the loser closes its candidate). Streaming-credentials and maintnotifications hooks are wired before publication; otel registration happens for the winner. Conn, Tx and SentinelClient never allocate the slot and keep using the main pool. The extraction into buildPipelinePool also surfaced that NewFailoverClient kept a duplicated creation block with the pre-fix gate (buffer sizes only), so PipelinePoolSize alone silently created no pipeline pool on failover clients; the shared helper fixes that, with a regression test. Stale RingOptions/FailoverOptions doc lines still describing the old gate are corrected.
…e pool The options the dedicated pipeline pool is built with are now resolved by pipelinePoolOptions, a pure function with pinned tests: Buffers default to DefaultPipelineBufferSize (new exported const, 64 KiB) rather than inheriting the regular 32 KiB. Pipeline connections move whole batches per round trip and earn bigger buffers than per-command traffic: measured on the autopipeline engine, throughput plateaus around 64 KiB, gains nothing past ~128 KiB, and very large buffers can regress it. An explicitly larger regular buffer is kept rather than shrunk, and explicit pipeline buffer sizes always win. The RESP3 minimum clamp applies as on the main pool. MinIdleConns is forced to 0 on the pipeline pool. It previously inherited the main pool's value through the options clone, so a client with MinIdleConns set pre-dialed that many pipeline connections at pool creation — a silent doubling of the idle footprint. The pipeline pool is burst capacity: its connections dial on demand, and the pool size (PipelinePoolSize or DefaultPipelinePoolSize) is only a cap, so an unused pipeline pool holds zero connections.
The autopipeline-subjects job builds its suite subject by calling AutoPipelineWithOptions on the shared rawClient, which now (correctly) creates the dedicated pipeline pool lazily - so client.Pipelined and client.Pipeline run on pipeline-pool connections, not the main pool's. Two specs assumed the old sharing: should Auth asserted main-pool stats (Hits=2/Misses=1/TotalConns=1) after two Pipelined calls. It now branches on PoolStats.PipelineStats: when the dedicated pool exists, the two pipelines account there (first dials, second reuses) and the main pool holds only the BeforeEach FlushDB dial; in the default subject mode PipelineStats is nil and the original assertions run unchanged. should ClientSetInfo issued CLIENT SETINFO through a pipeline and then read CLIENT INFO through rawClient - with a pipeline pool those are different connections, and SETINFO is per-connection state. It now reads CLIENT INFO through the same pipeline that issued the SETINFOs, which is equivalent on a single-pool client and correct on a split-pool one. Verified against redis:8 in all three GOREDIS_TEST_SUBJECT modes (default, ap-blocking, ap-async): Ran 2 of 1543 Specs, 2 Passed.
…ions Two autopipeline unit tests assumed batches execute on the main pool's already-initialized connection; with the dedicated pipeline pool now created lazily at autopipeliner creation, a batch's first dispatch dials and initializes a fresh pipeline-pool connection instead. Both tests now warm that connection with a real two-command batch first (a lone command takes the solo fast path through Process on the main pool and would not touch the pipeline pool): TestAutoPipelineRetriesOnNetworkError counted the pipeline pool's first dial as if it were a retry redial (3 dials observed, 2 expected). The warm-up runs before the write failure is armed, so the count again measures exactly the one redial the retry performs. TestAutoPipelineHookPostNextErrorPartialBatch installed a hook that unconditionally injects a post-next error before the pipeline-pool connection existed. Connection init runs its handshake pipeline through the client's hook chain (newConn shares hooksMixin), so the hook failed the pipeline conn's init itself — retries exhausted, the injected error was stamped on every command, and the test measured init poisoning rather than the post-next outcome rule it pins. The warm-up initializes the connection before the hook is installed.
…ateful CLIENT TRACKING and CLIENT MAINT_NOTIFICATIONS mutate per-connection state, exactly like CLIENT SETNAME/SETINFO, but live on cmdable: on a pooled client they land on an arbitrary pool connection that is immediately returned to the pool, so the flag applies to a connection the caller cannot address again. They belong on statefulCmdable, next to ClientSetName — but the move removes the methods from Client/UniversalClient/Cmdable (and defining them on both cmdable and statefulCmdable makes every Pipeline selector ambiguous), so it is a breaking change deferred to v10. Documented now; the move happens in a follow-up PR. No behavior change.
The ClientTracking*/ClientMaintNotifications move to statefulCmdable ships as its own PR against master (feature/stateful-conn-commands) rather than waiting for v10, so the TODO notes here are obsolete. Docs only.
The dedicated pipeline pool is now created unconditionally at NewClient/NewFailoverClient — like the pubsub pool — instead of behind a matrix of triggers (pipeline options set, AutoPipelineOptions declared, or lazily when an autopipeliner was built). It is pure burst capacity: no pre-dialing (MinIdleConns forced to 0), a small cap (DefaultPipelinePoolSize) and 64 KiB buffers, so an unused pipeline pool holds zero connections and costs nothing, while pipelines stop competing with regular commands for main-pool connections. PipelinePoolSize < 0 opts out and restores the old pipelines-on-the-main-pool behavior. To keep this from capping heavy concurrent Pipelined callers — who previously shared the much larger main pool — withPipelineConn spills to the main pool when the pipeline pool is exhausted (ErrPoolTimeout): bursts wider than the cap keep their old capacity, spilled pipelines simply run with regular buffers, and total connections stay bounded by PoolSize + PipelinePoolSize. Pipeline-pool connections are excluded from CLIENT TRACKING during init. Pipelined commands never consult or populate the built-in client-side cache (only the single-command cached path on main-pool connections does), so tracking pipeline reads only grew the server's tracking table and produced invalidation pushes for keys the cache does not hold. With creation unconditional, the lazy machinery is deleted: the atomic slot and CAS publication, ensurePipelinePool, the autopipeliner-factory hooks, the Options.init AutoPipelineOptions defaulting and its cluster clientOptions mirror. pipelinePool is a plain field set before the client is visible to any goroutine; WithTimeout clones share it. Net diff of this commit is negative. Tests: the creation-rule table becomes always-on plus opt-out; new e2e coverage for the spill (a pipeline pool of one, held busy by a pipelined BLPOP, must not queue a second pipeline behind PoolTimeout) and for the tracking exclusion (CLIENT LIST shows the main-pool connection tracked and the pipeline-pool connection untracked).
…efault The HImport mock tests choreograph exact per-connection sequences — a pool of one, booms armed to fire on the Nth replayed PREPARE, session counts pinned per connection. The always-on pipeline pool adds a second connection whose init replays fieldsets (correct production behavior: HIMPORT pipelines work on pipeline-pool connections precisely because of that replay), which fired the armed booms during the pipeline conn's init instead of the reissue under test and shifted the session counts. Opt every client in the file out with PipelinePoolSize: -1: the tests pin HIMPORT reissue/session semantics, not pool routing. Also states the PipelinePoolSize default plainly in the option docs: DefaultPipelinePoolSize (10) connections (review feedback).
The shard-level pipeline hooks in the cluster hook-order specs asserted Expect(cmds).To(HaveLen(...)) unconditionally. With the always-on pipeline pool, a node's first pipeline dials a pipeline-pool connection whose init runs its handshake pipeline through the node client's hook chain, so the assertion fired on the handshake and the resulting Ginkgo panic aborted the whole suite. Guard both hooks with the idiom ring_test.go already uses for the same situation: defer GinkgoRecover() and pass connection-initialization pipelines (hello/client) through. The cluster-level hooks are untouched — node connection init never runs through the cluster client's chain.
TestNoPipelinePoolStats pinned the stats shape of a client without a pipeline pool by creating a client without pipeline options — a premise the always-on pool inverts. Having no pool now requires the explicit opt-out, so the test sets PipelinePoolSize: -1, which is exactly the configuration whose stats shape it verifies. TestPipelineRetriesOnNetworkError is the plain-pipeline twin of the AP dial-count test adapted earlier: pipelines now run on the dedicated pipeline pool, so the first Exec dialed a fresh pipeline-pool connection and the test counted it as a phantom retry redial. Warm the pipeline connection with one Pipelined round trip before arming the write failure, so the count again measures exactly the one redial the retry performs.
…WAN-optimized)
One held pipeline-pool connection with a writer + reader goroutine pair streams
the ordered command stream instead of the half-duplex one-batch-per-round-trip
flusher. On a latency-bound link under many concurrent goroutines this gives
~1 RTT latency at one connection: WAN 50ms w=4096 inflight=5 ~389k ops/s @ 52ms
p50 vs ~207k @ 116ms for the half-duplex ordered path (errors=0).
- Gated by AutoPipelineOptions.FullDuplex; honored only on the async, ordered
(Unordered:false, MaxConcurrentBatches<=1), single-shard face of a standalone
*Client with a pipeline pool. Cluster stays half-duplex. Validate rejects the
contradictory combos; tuning via FullDuplex{Window,IdleTimeout,MaxHold}.
- Connection-failure retries re-issue the unacked ordered tail on a fresh conn,
honoring shouldRetry/MaxRetries/backoff and the NoRetry flag (parity with the
half-duplex pipeline's cmdsContainNoRetry).
- Lease/return (idle + max-hold) returns the held conn through the pool so its
per-conn hooks (streaming-creds re-auth, maintnotifications) run.
- RESP3 push frames demuxed inline (invalidation etc. never misread as a reply).
- Per-command process hooks (redisotel spans/metrics, custom ProcessHooks) fire
on the FD path; blocking/conn-hostile commands divert to a separate conn.
- Bounded in-flight window applies backpressure (Submit blocks when full).
18 FD tests, all -race. Depends on #3959 (getPipelinePool + always-on pipeline
pool). See AP_ORDERED_FULLDUPLEX_DESIGN.md.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![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: 1910c6ef83
ℹ️ 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".
…wn races Fixes four concurrency defects in the ordered full-duplex dispatch engine: - failReqs called cmd.Err(), which awaits batch.done -- the very channel failReqs then closes -- deadlocking the engine goroutine on retry exhaustion / Close. Use rawErr() (the same trap hostHook already documents). - On a connection-error session end the deque was taken (closeRecover) concurrently with the reader, so a command the reader had completed but not yet advanced could be handed to the retry loop and re-executed (and, on the hooked path, double-close its hookDone channel -- a panic). Split into hardClose + takeRemaining and take the tail only after the reader has exited. - A desynced connection was returned to the pool via Put() unless the error was classified bad-conn, poisoning the pool for the next caller (e.g. errFDReaderGone, a write timeout). Remove on any fdConnErr session end. - submit()'s send could win the race against the shutdown drain and enqueue a request that was never completed, hanging the caller forever. Gate the send with an RWMutex that drainQueue takes before draining. Also closes the readerDone/clean-result race: a reader protocol error during an idle/recycle/graceful return now removes the conn instead of Put-ing it back poisoned, and fails any stranded in-flight tail so no caller hangs. Adds pure unit tests for the failReqs deadlock and the deque ownership invariant. Existing full-duplex suite (conn-kill, close-while-backpressured, ctx-cancel, goroutine-leak) passes under -race.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf785396cb
ℹ️ 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".
Clears the three pre-existing staticcheck failures in the ordered full-duplex file (present before the teardown/recovery fix; that commit added none): - ST1008: reorder attempt() and session() return values so the error is the last argument -- (unacked, result, aerr) instead of (unacked, aerr, result). Pure reordering; the two call sites are updated to match. error and fdResult are distinct types, so any misorder fails to compile. - unused: //nolint:unused on fdInflight.peakLen, which is exercised only by the full-duplex backpressure tests (the repo lints with run.tests=false, so test-only usage is invisible to the unused check).
💡 Codex Reviewgo-redis/autopipeline_fullduplex.go Line 377 in 28dd5d3  Drain FD queue after shared pool close
When an FD autopipeliner is created on one client wrapper and another go-redis/autopipeline_fullduplex.go Line 428 in 28dd5d3 Let FD lease failures drain or spill queued work
When Line 1245 in 28dd5d3 Spill before waiting out the pipeline pool
With the pipeline pool now created by default, workloads that have more than ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
There was a problem hiding this comment.
Pull request overview
This PR introduces an opt-in ordered full-duplex dispatch path for the async ordered AutoPipeline, streaming commands over a single held pipeline-pool connection using a writer/reader goroutine pair to overlap writes with reply reads (WAN latency optimization). It also updates pipeline pooling behavior to create a dedicated pipeline pool by default (with opt-out), and expands tests to cover the new dispatch and pooling lifecycle behaviors.
Changes:
- Add a new full-duplex ordered AutoPipeline engine (
fdEngine) with backpressure windowing, retry-on-conn-failure tail replay, RESP3 push demux, and hook hosting for per-commandProcessHook. - Create and use a dedicated pipeline connection pool by default (opt-out via
PipelinePoolSize < 0), including spill-to-main-pool behavior on pipeline-pool timeouts and excluding pipeline conns fromCLIENT TRACKING. - Add/adjust tests to pin pool creation/option resolution and to validate FD correctness (ordering, recovery, backpressure, hooks, close behavior).
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
autopipeline.go |
Adds FullDuplex options, validation, and wires FD engine into ordered async submit path. |
autopipeline_fullduplex.go |
New full-duplex dispatch engine implementation (writer/reader, window backpressure, retries, push demux). |
autopipeline_fullduplex_test.go |
New test suite covering FD correctness, recovery, hooks, backpressure, and close behavior. |
redis.go |
Introduces pipelinePoolRef, shared pipeline-pool construction helpers, and updates pool routing/init/tracking/metrics integration. |
options.go |
Expands pipeline-pool documentation and introduces DefaultPipelinePoolSize / DefaultPipelineBufferSize. |
sentinel.go |
Aligns failover client pipeline pool creation/metrics and filters stale pipeline conns after failover. |
ring.go |
Updates RingOptions pipeline-pool creation docs to match new behavior. |
pipeline_pool_gate_test.go |
New tests pinning pipeline-pool creation rules, option resolution, spill behavior, and tracking exclusion. |
pipeline_exec_test.go |
Warms pipeline-pool conn to keep dial-count assertions stable under new routing. |
pipeline_buffer_test.go |
Updates “no pipeline pool” stats test to opt out explicitly. |
osscluster_test.go |
Adjusts hook-based expectations to ignore node pipeline-conn initialization handshake pipelines. |
himport_mock_test.go |
Opts out of pipeline pool where tests require deterministic main-pool per-connection choreography. |
commands_test.go |
Makes pool-stats and CLIENT SETINFO/INFO assertions robust to the dedicated pipeline pool. |
autopipeline_test.go |
Warms pipeline-pool conns where retry/hook tests are sensitive to initial handshake/dial counts. |
Suppressed comments (2)
options.go:262
- Docs say bursts wider than PipelinePoolSize “spill back to the main pool instead of queueing”, but the current spill happens only after the pipeline pool’s PoolTimeout elapses (it will still queue up to that timeout). Consider clarifying that behavior so expectations match reality.
// footprint: an unused pipeline pool holds zero connections. A burst of
// concurrent pipelines wider than the cap spills back to the main pool
// instead of queueing. Its connections use DefaultPipelineBufferSize
redis.go:1206
- The comment refers to a concurrent “ensurePipelinePool” publication, but there is no ensurePipelinePool symbol in the codebase (and pipelinePool is documented as set before publication). This stale reference makes the concurrency story around pipelinePool unclear.
// Load the ref once: a lazy ensurePipelinePool may publish concurrently,
// and every use below must see the same pool.
ref := c.loadPipelinePool()
💡 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: 28dd5d39f0
ℹ️ 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".
… bytes) Full-duplex review follow-ups (all in autopipeline_fullduplex.go): - retry read/write timeouts: shouldRetry(aerr, true), matching the cluster pipeline retry paths, so a single deadline miss no longer fails the whole unacked tail (cursor High); fdReqsNoRetry still excludes non-idempotent cmds. - recover a panicking user ProcessHook in hostHook (fail cmd + close batch), so it can neither crash the process nor leave the caller blocked. - close the conn on the connection-error path so a reader blocked in WithReader unblocks at once instead of stalling up to the read deadline (cursor High). - honor a disabled ReadTimeout (<0) as no-deadline instead of clamping to 30s. - log (not propagate) a push-drain error, matching the workers path, so a custom push processor error no longer fails an unrelated in-flight command. - return a clean conn via releaseConnToPool so pending push notifications are drained before Put (no delayed handoff). - acquire under ap.ctx so Close aborts a blocked pool.Get (init and I/O stay on Background so accepted commands still complete); backpressured submit honors the caller ctx. - apply a soft MaxBatchBytes cap in the writer drain loop.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a0e979a66
ℹ️ 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".
… bytes) Full-duplex review follow-ups (all in autopipeline_fullduplex.go): - retry read/write timeouts: shouldRetry(aerr, true), matching the cluster pipeline retry paths, so a single deadline miss no longer fails the whole unacked tail (cursor High); fdReqsNoRetry still excludes non-idempotent cmds. - recover a panicking user ProcessHook in hostHook (fail cmd + close batch), so it can neither crash the process nor leave the caller blocked. - close the conn on the connection-error path so a reader blocked in WithReader unblocks at once instead of stalling up to the read deadline (cursor High). - honor a disabled ReadTimeout (<0) as no-deadline instead of clamping to 30s. - log (not propagate) a push-drain error, matching the workers path, so a custom push processor error no longer fails an unrelated in-flight command. - return a clean conn via releaseConnToPool so pending push notifications are drained before Put (no delayed handoff). - acquire under ap.ctx so Close aborts a blocked pool.Get (init and I/O stay on Background so accepted commands still complete); backpressured submit honors the caller ctx. - apply a soft MaxBatchBytes cap in the writer drain loop. - honor opt.Limiter per session: Allow() before the session pool.Get, ReportResult() before release (report-before-release), 1:1 per session (the Limiter gates conn acquisition; FD acquires one conn per session). - on a retryable Redis error (LOADING/READONLY/CLUSTERDOWN/…) or a redirect (MOVED/ASK), re-run the command on the client normal path (pipeliner.process) off the reader goroutine (tracked by retryWg so Close waits) and settle the caller with that result; ordering across the divert is not promised (same as the blocking-command divert), NoRetry commands keep their error. - record the native per-command OTel metric (RecordOperationDuration, write->reply) from the reader, mirroring process; skipped for retry-diverted commands (they emit via process). fdReq carries its submit ctx + write timestamp. - divert managed HIMPORT (PREPARE/SET/DISCARD/DISCARDALL) off the FD pipe to the normal Process path: the FD writer streams raw commands and never replays the registered PREPARE (connection-session state), so an HIMPORT SET on the pipe could fail "no such fieldset". Process injects the PREPARE from the registry and keeps it current on PREPARE; the half-duplex sharded path injects inline (himportInjectedCmds) and is left on the pipeline. FD-scoped: gated on ap.fd. - wait for per-command hook-host goroutines during Close: a ProcessHook doing work after next() returns runs on a host goroutine that is the only code closing that command's batch. Track it in a hostWg that run() waits (so Close, which waits ap.wg, waits it too), restoring the drain-before-return contract. The hostWg.Add is gated behind submitMu+closed exactly like the ch send, so run()'s hostWg.Wait never races a live Add (every run() return is preceded by drainQueue). - stamp the FD hook host as the batch executor (dispGid) so a ProcessHook that reads its own command result after next() (cmd.Err()/cmd.String()) gets the just-executed view instead of self-deadlocking on the batch only that goroutine closes (cursor P1). Mirrors runOutsidePipeline's async dispatch guard.
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.
Suppressed comments (11)
autopipeline.go:118
- This violates the existing
AddHookcontract, which states that omittingnextprevents command execution (redis.go:150-182). Sending anyway turns policy, cache, and mock hooks into unintended server side effects; documenting the incompatibility does not preserve the API contract. The wire write must remain gated bynext, or FullDuplex must not activate when hooks are installed.
// Caveat: because the write is already queued on the shared stream when the hook
// host starts, a hook that SHORT-CIRCUITS (returns without calling next) does
// NOT cancel execution — the command still runs on the wire; only the hook's
// returned error is reflected to the caller. This differs from the half-duplex
// path, where next() gates the write. A hook that relies on short-circuiting to
// BLOCK a command (a policy/ACL/kill-switch hook, or a mock/cache that must not
// touch the server) therefore does NOT prevent the server write under FullDuplex
// — run such hooks on a plain client or the half-duplex autopipeline. Hooks that
autopipeline_fullduplex.go:356
- The request is visible to the writer before this goroutine starts. On a fast server the writer/reader can complete and close
hookDonebefore the scheduler runshostHook, producing a near-zero tracing span that does not bracket write→reply as promised. Add a startup handshake so the observing hook has entered before the command can be written.
if hookDone != nil {
fd.hostWg.Add(1)
go fd.hostHook(ctx, cmd, b, hookDone)
options.go:250
- These statements contradict both the unconditional creation below and the actual buffer resolution: a positive size overrides the default rather than creating the pool, and unset pipeline buffers use the larger of the regular size and
DefaultPipelineBufferSize, not simply the regular sizes. Update this public option documentation to matchpipelinePoolOptions.
// Setting this alone is enough to create the dedicated pipeline pool; the
// pipeline buffer sizes then default to ReadBufferSize/WriteBufferSize.
redis.go:1253
- The spill path still has active dedicated-pool limiter accounting, but
c.withConnperforms anotherAllow/ReportResultpair throughgetConn/releaseConn. A limiter can therefore reject an already-admitted spill and receives duplicate results. Acquire, initialize, and releasec.connPooldirectly under the existing limiter lease instead of nestingwithConn.
return c.withConn(ctx, fn)
redis.go:652
opt.clone()retains the main pool'sMaxActiveConns. For example,MaxActiveConns: 1makes this nominally 10-connection pipeline pool returnErrPoolExhaustedafter one connection, andwithPipelineConndoes not spill because it only handlesErrPoolTimeout. Reset this field soPipelinePoolSizeremains the dedicated pool's cap.
pipelineOpt.MinIdleConns = 0
redis.go:630
- Cloning also carries the main pool's default
PoolTimeout(normallyReadTimeout + 1s, i.e. 6s). Since spill happens only after that timeout, the 11th concurrent pipeline can queue for six seconds before using the main pool, contrary to the documented “spills … instead of queueing” behavior. Give the burst pool a short dedicated timeout and cover the default, not only the test's manually shortened timeout.
func pipelinePoolOptions(opt *Options) *Options {
pipelineOpt := opt.clone()
autopipeline_fullduplex.go:303
- The default window eagerly allocates a 65,536-element channel backing store (roughly 4.5 MiB for
fdReq) for every FullDuplex autopipeliner, even when no command is submitted. This contradicts the API claim that a generous window costs no memory until in-flight work grows. Decouple queue capacity from the in-flight window or use a lazily growing queue.
ch: make(chan fdReq, w),
autopipeline_fullduplex.go:863
- The window is checked only before draining, after which this loop can append an entire
MaxBatchSize. WithFullDuplexWindow: 1and the default batch size, in-flight can jump to 200, so the advertised hard bound is not enforced. Cap each drained batch by the remaining window room.
for len(batch) < fd.maxBatch {
autopipeline_fullduplex.go:745
- The PR description claims a RESP3 push-demux test, but
TestFullDuplexStaysAlignedUnderConcurrentMutationexplicitly says it never sends a push to the FD connection and that a mock injection test is still a follow-up (autopipeline_fullduplex_test.go:113-122). This FIFO-critical branch therefore remains unverified; add a test that injects a push frame immediately before a reply.
// Drain RESP3 push frames buffered ahead of this reply so a
// push is never misread as the command's reply (FIFO misalign).
// A push-drain error is logged and NOT propagated (matching the
// workers path in flushBatch): a custom push processor returning
// an error must not fail this unrelated in-flight command or kill
// the connection. A genuine transport error re-surfaces in the
// readReply below and is handled as the connection error it is.
if perr := fd.client.processPendingPushNotificationWithReader(bg, cn, rd); perr != nil {
sentinel.go:132
- This public documentation still describes the pool as optional and tied to these fields/AutoPipelineOptions, but
NewFailoverClientnow creates it for every client unlessPipelinePoolSize < 0. Document the default creation and explicit opt-out so callers can predict connection capacity.
// PipelineReadBufferSize, PipelineWriteBufferSize and PipelinePoolSize
// configure an optional separate connection pool used for pipelining, with
// its own (typically larger) buffers. See the same-named fields on Options
// for details. Setting any of the three creates the pool; it also defaults
// in when AutoPipelineOptions is set.
ring.go:154
- Each shard client now creates the dedicated pipeline pool by default even when all three fields are zero, so “Setting any … creates” is misleading. State the default creation and
PipelinePoolSize < 0opt-out, consistent withOptions.
// PipelineReadBufferSize, PipelineWriteBufferSize and PipelinePoolSize
// configure an optional separate connection pool used for pipelining on
// each shard, with its own (typically larger) buffers. See the same-named
// fields on Options for details. Setting any of the three creates the pool
// on each shard client.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7d9b731b0
ℹ️ 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".
Under a sustained retryable stream (LOADING/READONLY on every reply) each diverted reply spawned an independent normal-path retry goroutine while the reader kept advancing the window and admitting more writes — unbounded goroutines parked in backoff/pool acquisition. retryOnNormalConn now acquires a window-sized semaphore slot before spawning; a full semaphore blocks the READER, which stops advancing the deque, fills the window, and blocks the writer and submitters — end-to-end backpressure, worst case window on-pipe + window off-pipe. No cycle: retries drain on the main pool, independent of the reader.
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.
Suppressed comments (8)
sentinel.go:132
- This describes conditional/lazy creation, but
NewFailoverClientnow creates the pool wheneverPipelinePoolSize >= 0, even when all three fields andAutoPipelineOptionsare unset. Document the default-on behavior and the negative opt-out so callers can predict connection capacity.
// for details. Setting any of the three creates the pool; it also defaults
// in when AutoPipelineOptions is set.
options.go:250
- This says unset pipeline buffers inherit the regular buffer sizes, but
pipelinePoolOptionsactually uses the larger of those sizes andDefaultPipelineBufferSize. It also implies this field triggers creation although the pool is now default-on. Align this opening summary with the detailed behavior below.
// Setting this alone is enough to create the dedicated pipeline pool; the
// pipeline buffer sizes then default to ReadBufferSize/WriteBufferSize.
redis.go:1253
- Spilling through
withConnperforms a secondLimiter.Allow/ReportResultpair after this function already acquired and deferred reporting for the limiter. Under saturation this can consume two breaker permits for one pipeline, or let the secondAllowreject an operation that was already admitted. Acquire from the main pool without re-entering limiter accounting, while preserving report-before-release ordering.
return c.withConn(ctx, fn)
redis.go:652
- The cloned main-pool
MaxActiveConnsis still applied to the pipeline pool. For example,MaxActiveConns: 1with the default pipeline size makes a second pipeline fail immediately withErrPoolExhausted; the spill path only handlesErrPoolTimeout. Reset this field soPipelinePoolSizeindependently controls the dedicated pool as intended.
pipelineOpt.MinIdleConns = 0
redis.go:650
pipelineOptalso retains the main pool'sPoolTimeout(six seconds by default), so the first pipeline beyond this small cap waits that full interval before spilling. This contradicts the dependency's short pipeline-pool timeout and turns bursts into multi-second stalls; apply a dedicated short timeout here.
pipelineOpt.PoolSize = DefaultPipelinePoolSize
autopipeline_fullduplex.go:859
- This check does not enforce
FullDuplexWindowas the documented hard in-flight limit. Once below the window, the drain below can append an entireMaxBatchSizebatch, soFullDuplexWindow: 1can still put roughly 200 commands in flight; replaying carry also bypasses the limit. Cap each write by the remaining window capacity, including replay chunks.
for inflight.len() >= fd.window {
autopipeline_fullduplex.go:605
attemptsremains incremented after the replay succeeds and the new session resumes serving commands; it resets only on idle/max-hold return. Under continuous load, a later unrelated connection loss therefore inherits the earlier retry count and can exhaustMaxRetriesimmediately. Reset the retry budget once the recovered tail is acknowledged or otherwise establish successful-session progress before applying it to new commands.
attempts++
fd.sleepBackoff(attempts)
carry = unacked[:n]
ring.go:154
- The shard clients now create their dedicated pipeline pools by default, not only when one of these fields is set. This public option comment should state the default-on behavior and
PipelinePoolSize < 0opt-out.
// fields on Options for details. Setting any of the three creates the pool
// on each shard client.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d32b20b25e
ℹ️ 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".
…rity - run() keeps two budgets, each counting only consecutive failures of its own kind: leaseAttempts (fdLeaseErr/fdDenied) and retryAttempts (fdConnErr tail replays). leaseAttempts resets whenever a session actually runs and both reset on a clean end, so transient lease failures can no longer consume the reconnect budget and leave a genuine drop with zero replay attempts. - The reader's inline completion emits the native error metric (classifyCommandError + pool.GetMetricErrorCallback) for non-retryable Redis errors, matching the normal command path. - The writer runs a non-blocking readerDone priority check before its main select, so randomized select can no longer write another batch to a connection the engine already knows has no reader.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6625b02f3
ℹ️ 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".
Drop the !blocking term from the fdOn gate: FullDuplex is now honored on the blocking AutoPipeline face too. No new machinery was needed — submit()'s fd branch already skips setReady on the blocking face (its contract) and processBlocking waits on the returned batch, the same shape as a half-duplex enqueue; the dispGid self-deadlock guard and everything downstream are face-agnostic. A single blocking caller gains nothing (one command in flight — nothing to overlap; only held-conn overhead); the benefit appears with many concurrent blocking callers, whose commands overlap on the shared pipe like the async face (~1 RTT each instead of batch phase-locking). The FullDuplex GoDoc states this and notes the blocking face's per-goroutine ordering is unaffected by the retry divert (each caller waits per command by construction). Adds TestFullDuplexBlockingFace.
- retryOnNormalConn recovers a panic from process (hooks/encoders are user code): the command gets the panic as its error and always completes, so the caller and a hooked command's host cannot hang — matching the writer, reader and hostHook recovery discipline. - The writer's drain is capped by the remaining window room (min(MaxBatchSize, window - inflight)), so FullDuplexWindow smaller than MaxBatchSize can no longer be blown through by a single batch. - writeCarryChunked observes readerDone between chunks (carry replay and the Close flush both pass it): if the reader exits mid-replay the un-written remainder is pushed to the deque for recovery instead of being written to a reader-less connection.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1eecfcdc37
ℹ️ 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".
…ands Commands terminated through failReqs (lease failure, retry exhaustion, a NoRetry tail, Close) never reach the reader's inline completion, so they were invisible to the native error callback. failReqs now classifies the shared failure once and emits the callback per command. No duration metric: these commands have no meaningful write-to-reply span (many were never written).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c824b4d46c
ℹ️ 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".
…lush - failQueue emits the native error callback per drained request, mirroring failReqs: on fdLeaseErr/fdDenied both halves of the failure (carry and the accepted channel backlog) are now visible to extra/redisotel-native. - Validate rejects FullDuplex with a user-set NumShards > 1 (one held FIFO connection is a single stream) instead of silently falling back to half-duplex; the cluster wiring's internal contentSharded flag is exempt, preserving the documented cluster fallback. - shutdownFlush re-issues the between-sessions Close backlog in the same MaxBatchSize/MaxBatchBytes chunks as normal FD writes (fdBatchEnd) instead of one unchunked pipeline that ignored MaxBatchBytes.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38065ef00e
ℹ️ 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".
…lush - failQueue emits the native error callback per drained request, mirroring failReqs: on fdLeaseErr/fdDenied both halves of the failure (carry and the accepted channel backlog) are visible to extra/redisotel-native. - Validate rejects FullDuplex with a user-set NumShards > 1 (one held FIFO connection is a single stream) instead of silently falling back to half-duplex; the cluster wiring's internal contentSharded flag is exempt, preserving the documented cluster fallback. - shutdownFlush re-issues the between-sessions Close backlog in the same MaxBatchSize/MaxBatchBytes chunks as normal FD writes instead of one unchunked pipeline that ignored MaxBatchBytes; it stops (failing the remainder) after a transport-class chunk failure instead of re-running the retry cycle per chunk against a dead endpoint, and recovers an encoder panic. - The reconnect retry budget resets when a session completed work (including a successful carry replay): a drop after real progress is a fresh failure, not a consecutive one. - The submit queue is capped at min(window, 4096): a buffered channel allocates its capacity eagerly (~5 MiB per engine at the default window); backpressure is enforced by the in-flight deque, so the window keeps its meaning as the in-flight bound. Also compacts the review-round comments in the touched files (invariants and contracts preserved; narration removed).
Recovered panics (reply decode, batch encode) surfaced as plain errors, so shouldRetry rejected them and the connection-error path permanently failed the whole unacked tail — mostly commands the panic never touched. Wrap them with an errFDPanicRecovered sentinel and treat it (and errFDReaderGone) as replayable: the conn is desynced exactly like a transport error, so the tail replays on a fresh connection with the NoRetry split and MaxRetries budget unchanged.
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 b051877. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b051877c8a
ℹ️ 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 reader acquired a retry slot with a bare channel send, so with the semaphore full it parked where it could no longer observe hardClose or the client context, and session teardown waited on readerDone for as long as a slot took to free. The acquire now selects on the semaphore and the autopipeliner context: on Close the request fails with ErrClosed instead of parking the reader past teardown. Outside of Close the wait stays bounded — every slot holder is a retry running through process(), whose timeouts guarantee the slot frees.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53af243df3
ℹ️ 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".
| case <-fd.ap.ctx.Done(): | ||
| req.cmd.SetErr(ErrClosed) | ||
| req.complete() | ||
| return |
There was a problem hiding this comment.
When a sustained stream of retryable replies fills retrySem and Close is called while the reader is waiting for another slot, this branch replaces the already-accepted command's Redis result with ErrClosed instead of completing its configured normal-path retry. Because cancellation remains ready, the reader can similarly fail the rest of the in-flight retryable replies during teardown, contradicting Close's accepted-work drain contract; keep these retries tracked through shutdown or otherwise settle them without treating them as post-Close submissions.
Useful? React with 👍 / 👎.
| backlog = append(backlog, r) | ||
| default: | ||
| return fd.writeCarryChunked(bg, cn, inflight, backlog, readerDone) |
There was a problem hiding this comment.
When Close wins while replies are slow and the in-flight deque is already at FullDuplexWindow, this writes the entire buffered backlog without waiting for reader progress. Since the channel can contain up to min(window, 4096) additional requests, a small configured window can be exceeded by roughly 2× during shutdown, violating the option's documented hard bound on written-but-unacknowledged commands; apply the same remaining-window backpressure between close-flush chunks.
Useful? React with 👍 / 👎.
| // It records the create-time metric, unwraps the init error, and Removes the | ||
| // conn on any failure (so the defer, seeing cn=nil, does not double-release); it | ||
| // does NOT Put the conn, which suits the held-conn model. | ||
| if e := fd.client.initPooledConn(bg, fd.pool, cn); e != nil { |
There was a problem hiding this comment.
When CredentialsProviderContext derives credentials from values on the submitted command's context, every newly dialed full-duplex connection is initialized with context.Background() here, so resolveCredentials cannot see those values. Such providers can reject all FD sessions or return fallback credentials for the wrong identity even though the command supplied the correct context; initialize the session using the carry request's context, or reject FullDuplex when context-dependent authentication cannot be preserved.
AGENTS.md reference: AGENTS.md:L154-L160
Useful? React with 👍 / 👎.
| if perr := fd.client.processPendingPushNotificationWithReader(bg, cn, rd); perr != nil { | ||
| internal.Logger.Printf(bg, "autopipeline: full-duplex push drain: %v", perr) | ||
| } |
There was a problem hiding this comment.
When this drain handles a maintenance MOVING push during continuous full-duplex traffic, the handler only marks cn for handoff and the writer never checks that state. The connection is therefore not returned through Put—where the pool hook actually queues the handoff—until idle or the default five-second max-hold expires, so commands continue being written to a node known to be moving and the handoff can miss its deadline; signal the writer to drain and recycle the session as soon as cn.ShouldHandoff() becomes true.
AGENTS.md reference: AGENTS.md:L134-L146
Useful? React with 👍 / 👎.
Ordered full-duplex AutoPipeline dispatch (single-connection, WAN-optimized)
Adds an opt-in full-duplex dispatch path to the ordered AutoPipeline (async and blocking faces):
one held pipeline-pool connection with a writer + reader goroutine pair
streams the ordered command stream, instead of the half-duplex
one-batch-per-round-trip flusher. On a latency-bound link under many concurrent
goroutines this decouples writes from reads → ~1 RTT latency at one
connection, pipe-saturated throughput.
Why
Ordered mode is already one in-flight batch (
MaxConcurrentBatches=1), buthalf-duplex: write a batch → wait a full RTT → read → write the next. That caps
WAN throughput at
batch/RTTand makes late arrivals wait ~2 RTT. Full-duplex onthe same one connection removes the wait.
Measured (50ms WAN proxy, w=4096, inflight=5, errors=0)
~1.9× throughput and half the latency at the same single connection. On loopback
(no RTT to overlap) FD ≈ HD — the win is WAN.
Scope / gating
Honored on the ordered (
Unordered:false,MaxConcurrentBatches<=1),single-shard faces — both async (
AsyncAutoPipeline) and blocking(
AutoPipeline) — of a standalone*Clientthat has a pipeline pool.On the blocking face a single caller gains nothing (one command in flight —
nothing to overlap); the benefit appears with many concurrent blocking callers,
whose commands overlap on the shared pipe like the async face.
Validaterejects the contradictory combos (
Unordered,MaxConcurrentBatches>1,user-set
NumShards>1). Cluster stays on the half-duplex path (one orderedconnection can't slot-route). Enabled by
AutoPipelineOptions.FullDuplex;tuned via
FullDuplexWindow,FullDuplexIdleTimeout,FullDuplexMaxHold(zero = sane defaults).
Correctness / operational
fresh connection, honoring
shouldRetry/MaxRetries/backoff and the per-commandNoRetryflag (parity with the half-duplex pipeline'scmdsContainNoRetry).per-conn hooks (streaming-creds re-auth, maintnotifications) run.
ProcessHooks) fire on the FD path with real write→reply timing; DialHook andpool stats unchanged.
SUBSCRIBE, MULTI, …) divert to a separate pooled connection — never stall the pipe.
Submitblocks when full.
Tests
25+ full-duplex tests, all
-race: ordering under 64 goroutines, RESP3 pushdemux, conn-kill recovery, lease/return pool-hook cycling, idle/max-hold recycle,
config defaults + Validate, per-command ProcessHook + short-circuit, ctx-cancel
alignment, goroutine-leak/close-mid-flight, NoRetry guard, blocking-cmd divert,
mid-stream Redis error (FIFO stays aligned), Close-while-backpressured,
backpressure bound, blocking-face activation/ordering/concurrency, chunked-carry
suffix recovery, between-sessions Close flush.
Review hardening
The review rounds hardened failure/lifecycle semantics: accepted work is flushed
(never dropped) on every Close path, including between sessions and when Close
races a lease; a Limiter denial or exhausted lease retries fail-fast the
carry and accepted backlog (separate lease/reconnect retry budgets, each counting
only consecutive failures of its own kind); off-pipe retries are bounded by the
window (end-to-end backpressure); writer/reader/retry/encode panics all recover;
the drain is capped by the remaining window; and native error metrics fire on
every terminal path.
Depends on #3959 (getPipelinePool + always-on pipeline pool). Design notes:
AP_ORDERED_FULLDUPLEX_DESIGN.md.Note
High Risk
Large new concurrent I/O engine with connection recovery, at-least-once tail replay, and hook/ordering caveats; plus default pipeline-pool behavior change affecting all pipelined traffic.
Overview
Introduces ordered full-duplex dispatch for standalone
*ClientAutoPipeline whenFullDuplexis set on an ordered, single-shard config with a pipeline pool: a writer/reader pair on one leased connection streams commands and matches FIFO replies (~1 RTT under WAN concurrency) instead of half-duplex batch flushes. New tuning options (FullDuplexWindow, idle/max-hold timeouts), validation rules, diversions for blocking/HIMPORT/retryable errors, process-hook hosting, andLen()accounting for the FD queue.Pipeline pool behavior is aligned with that path: clients get a dedicated pipeline pool by default (
PipelinePoolSizenegative opts out), burst spill to the main pool, larger default pipeline buffers, no inheritedMinIdleConns, and pipeline connections skip client-side tracking when CSC is on.Supporting changes fix tests and docs that assumed pipelines on the main pool (warm pipeline conn before inject failures, HIMPORT mocks with
PipelinePoolSize: -1, pool-stats expectations). govulncheck uses Gostableinstead of a pinned minor version.Reviewed by Cursor Bugbot for commit 53af243. Bugbot is set up for automated code reviews on this repo. Configure here.