diff --git a/.github/workflows/govulncheck.yml b/.github/workflows/govulncheck.yml index 4f79174656..7a24060808 100644 --- a/.github/workflows/govulncheck.yml +++ b/.github/workflows/govulncheck.yml @@ -29,7 +29,7 @@ jobs: - name: Set up Go uses: actions/setup-go@v7 with: - go-version: "1.26.x" + go-version: "stable" cache: true - name: Install govulncheck diff --git a/autopipeline.go b/autopipeline.go index 42ac8d5059..31fb8ecced 100644 --- a/autopipeline.go +++ b/autopipeline.go @@ -63,6 +63,94 @@ type AutoPipelineOptions struct { // is a configuration error. Unordered bool + // FullDuplex enables the ordered full-duplex dispatch path: one held + // pipeline-pool connection with a writer+reader goroutine pair streaming the + // ordered command stream, instead of the half-duplex one-batch-per-round-trip + // flusher. Its win is a latency-bound (WAN) link under many concurrent + // goroutines: ~1 RTT latency and pipe-saturated throughput on a single + // connection. On a fast link (loopback) prefer half-duplex — with no RTT to + // overlap, full-duplex only adds coordination overhead. + // + // Honored on the ordered (Unordered:false, MaxConcurrentBatches<=1), + // single-shard face of a standalone *Client that has a pipeline pool — BOTH the + // deferred (AsyncAutoPipeline) and the blocking (AutoPipeline) face. A SINGLE + // blocking caller gains nothing: it has one command in flight, so there is + // nothing to overlap, and it still pays the held connection and goroutine + // overhead; the win needs MANY concurrent blocking callers, whose commands then + // overlap on the shared pipe exactly as on the async face (~1 RTT each instead + // of batch phase-locking). NOT supported on cluster clients: a ClusterClient + // silently falls back to half-duplex (the options type cannot see the client + // type, so Validate cannot catch it). Validate does reject the contradictory + // standalone combos (FullDuplex with Unordered or MaxConcurrentBatches>1). + // + // Ordering caveat: blocking and connection-hostile commands (BLPOP, WAIT, + // XREAD BLOCK, SUBSCRIBE, MULTI, ...) are diverted to a separate pooled + // connection so they cannot stall the shared pipe. Managed HIMPORT + // (PREPARE/SET/DISCARD/DISCARDALL) is diverted too, but only on the full-duplex + // path: a fieldset is connection-session state that the FD writer does not + // replay and the FD reader does not track, so it runs through the normal Process + // path — which injects the registered PREPARE and keeps the registry current — + // instead of failing with "no such fieldset". A reply that is a retryable Redis + // error (LOADING/READONLY/…) or a redirect (MOVED/ASK) is likewise re-run + // through Process, off the FD reader, so the reader keeps completing later + // replies. Per-caller ordering therefore does NOT hold across a diverted + // command: it may settle AFTER a command submitted later on the same goroutine. + // That reorders only a caller holding TWO causally-dependent commands in flight + // WITHOUT awaiting the first (e.g. Set(k) then Get(k) both fired on the async + // face before reading Set's result); awaiting a result before issuing a + // dependent one preserves order, and the blocking face waits per command by + // construction, so its per-goroutine ordering is unaffected. NoRetry commands + // are never diverted. Half-duplex diverts identically; blocking commands were + // never part of the ordered stream. + // + // Observability: process hooks (redisotel spans/metrics, custom AddHook + // ProcessHooks) DO fire on the full-duplex path — each command runs the hook + // chain individually (withProcessHook, not the batch ProcessPipelineHook), the + // span bracketing its real write→reply latency; with none registered the hosting + // is skipped entirely (the fast path). Presence is checked per command at submit + // time, so a hook registered via AddHook is observed only by commands submitted + // after it (one already in flight is not retroactively spanned). DialHook and + // pool stats work as usual. Caveat: the write is already queued on the shared + // stream when the hook host starts, so a hook that SHORT-CIRCUITS (returns + // without calling next) does NOT cancel execution — the command still runs on + // the wire and only the hook's returned error reaches the caller, unlike 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 always call next are unaffected. + // + // TODO(fullduplex): offer opt-in write-gating for blocking hooks — a per-client + // or per-command flag that waits for the hook to call next before enqueuing the + // command onto fd.ch, so a policy hook can veto the write, at the cost of the + // ~1-RTT concurrency for gated commands (observability-only hooks keep the fast + // path). Until then the short-circuit-does-not-block semantics above are + // intentional, not a bug. + FullDuplex bool + + // FullDuplexWindow is the maximum in-flight (written-but-unacknowledged) + // commands before the writer applies backpressure — a hard memory bound AND the + // cap on how deep the pipe can fill, so it must exceed the bandwidth-delay + // product (RTT × target rate) or it throttles throughput. The deque holds only + // ACTUAL in-flight (self-limited by throughput), so a generous window costs no + // memory until a stalled peer makes in-flight grow. Only used when FullDuplex is + // set; 0 means the default (65536, covering ~50ms links at ~1.3M ops/s) and a + // negative value is rejected by Validate. + FullDuplexWindow int + + // FullDuplexIdleTimeout is how long the held full-duplex connection may sit + // with no queued work and a drained in-flight before it is returned to the pool + // (so it is reusable and its per-conn hooks — streaming-creds re-auth, + // maintnotifications — get a chance to run). Only used when FullDuplex is set. + // 0 means the default (1s); a negative value is rejected by Validate. + FullDuplexIdleTimeout time.Duration + + // FullDuplexMaxHold forces the same clean return under continuous load, so the + // per-conn hooks run at least this often even when the connection never goes + // idle. Only used when FullDuplex is set. 0 means the default (5s); a negative + // value is rejected by Validate. + FullDuplexMaxHold time.Duration + // contentSharded is set internally by cluster wiring when commands are // routed to shards by content (slot), so same-key commands always share a // shard and per-key order holds even with several shards. It exempts that @@ -213,6 +301,29 @@ func DefaultBlockingAutoPipelineOptions() *AutoPipelineOptions { // Options.AutoPipelineOptions is validated lazily — on the first getter // call, not in NewClient. func (cfg *AutoPipelineOptions) Validate() error { + if cfg.FullDuplex { + // Full-duplex matches replies to commands by FIFO position on one connection, + // which Unordered / parallel batches break. Checked BEFORE the generic + // MaxConcurrentBatches rule so the message is FullDuplex-specific. + if cfg.Unordered { + return fmt.Errorf("redis: AutoPipelineOptions.FullDuplex requires an ordered stream " + + "(Unordered:false); full-duplex matches replies by in-flight FIFO position, which " + + "Unordered breaks") + } + if cfg.MaxConcurrentBatches > 1 { + return fmt.Errorf("redis: AutoPipelineOptions.FullDuplex requires MaxConcurrentBatches<=1 "+ + "(an ordered single stream); got %d", cfg.MaxConcurrentBatches) + } + // A USER-set NumShards>1 contradicts FullDuplex the same way (one held FIFO + // connection is one stream); reject it rather than silently falling back to + // half-duplex. contentSharded is exempt: that flag is set by the CLUSTER + // wiring (never by users), where the silent fallback IS the documented + // behavior, since the options type cannot see the client type. + if cfg.NumShards > 1 && !cfg.contentSharded { + return fmt.Errorf("redis: AutoPipelineOptions.FullDuplex requires NumShards<=1 "+ + "(one held FIFO connection is a single stream); got %d", cfg.NumShards) + } + } if cfg.MaxConcurrentBatches > 1 && !cfg.Unordered { return fmt.Errorf("redis: AutoPipelineOptions.MaxConcurrentBatches=%d requires Unordered:true "+ "(parallel batches do not preserve command ordering); set Unordered:true to allow it, "+ @@ -241,6 +352,15 @@ func (cfg *AutoPipelineOptions) Validate() error { "(adaptive delay scales MaxFlushDelay by queue fill; with no MaxFlushDelay it would " + "silently disable batch accumulation entirely)") } + if cfg.FullDuplexWindow < 0 { + return fmt.Errorf("redis: AutoPipelineOptions.FullDuplexWindow=%d must be >= 0 (0 = default)", cfg.FullDuplexWindow) + } + if cfg.FullDuplexIdleTimeout < 0 { + return fmt.Errorf("redis: AutoPipelineOptions.FullDuplexIdleTimeout=%s must be >= 0 (0 = default)", cfg.FullDuplexIdleTimeout) + } + if cfg.FullDuplexMaxHold < 0 { + return fmt.Errorf("redis: AutoPipelineOptions.FullDuplexMaxHold=%s must be >= 0 (0 = default)", cfg.FullDuplexMaxHold) + } return nil } @@ -541,11 +661,16 @@ func putQueueSlice(slice []Cmder) { // waiting for it. // // EXPERIMENTAL: this API is subject to change, use with caution. + type AutoPipeliner struct { cmdable // Embed cmdable to get all Redis command methods pipeliner cmdableClient config *AutoPipelineOptions + // fd, when non-nil, is the ordered full-duplex dispatch engine. When set, + // submit() streams on one held connection instead of the sharded batch queue + // and no shard flusher is started. See autopipeline_fullduplex.go. + fd *fdEngine // blocking selects how the typed command surface (Set, Get, ...) behaves: // when true the command call itself blocks until the command has executed // (drop-in, synchronous shape); when false the call returns immediately and @@ -828,6 +953,19 @@ func newAutoPipeliner(pipeliner cmdableClient, config *AutoPipelineOptions, bloc perShard = 1 remainder = 0 } + // Ordered full-duplex: the ordered single-shard face on a standalone *Client + // with a pipeline pool, async or blocking. When on, submit() streams on one + // held connection and no shard flusher runs. The blocking face needs nothing + // extra: submit's fd branch skips setReady (the blocking contract) and + // processBlocking Waits on the returned batch, as for a half-duplex enqueue. + var fdClient *Client + fdOn := false + if config.FullDuplex && !config.Unordered && config.MaxConcurrentBatches <= 1 && nShards == 1 { + if c, ok := pipeliner.(*Client); ok && c.getPipelinePool() != nil { + fdOn, fdClient = true, c + } + } + ap.shards = make([]*apShard, nShards) for i := range ap.shards { permits := perShard @@ -854,8 +992,16 @@ func newAutoPipeliner(pipeliner cmdableClient, config *AutoPipelineOptions, bloc s.stripes[j].curBatch = newAPBatch() } ap.shards[i] = s + if !fdOn { + ap.wg.Add(1) + go s.flusher() + } + } + + if fdOn { + ap.fd = newFDEngine(ap, fdClient) ap.wg.Add(1) - go s.flusher() + go ap.fd.run() } return ap, nil @@ -1233,6 +1379,16 @@ var blockingCommands = map[string]struct{}{ "migrate": {}, } +// isHImportCmd reports whether cmd is a managed HIMPORT command +// (PREPARE/SET/DISCARD/DISCARDALL). It uses the same predicate himportInjectedCmds +// uses to spot HIMPORT in a batch (the himportCmder marker), so it stays in sync +// with the injection path and covers every subcommand without a name switch. Used +// only on the full-duplex path to divert HIMPORT off the shared pipe (see submit). +func isHImportCmd(cmd Cmder) bool { + _, ok := cmd.(himportCmder) + return ok +} + // isBlockingCmd reports whether cmd parks the connection. XREAD/XREADGROUP are // decided by ARGUMENTS, not by name: only the BLOCK form blocks, and // blanket-diverting the (far more common) non-blocking form would drop it out @@ -1302,7 +1458,13 @@ func (ap *AutoPipeliner) submit(ctx context.Context, cmd Cmder) AutoFuture { // commands that would have worked — typed WAIT/WAITAOF on a cluster with // command policies enabled (review finding by codex on #3942). diverted := cmd.readTimeout() != nil || runsOutsidePipeline(cmd.Name()) || isBlockingCmd(cmd) || - (ap.mustDivert != nil && ap.mustDivert(ctx, cmd)) + (ap.mustDivert != nil && ap.mustDivert(ctx, cmd)) || + // Managed HIMPORT rides connection-session state (the registered PREPARE) + // that the full-duplex writer never injects, so an HIMPORT SET on the FD + // pipe can fail "no such fieldset". Divert it to the normal Process path, + // which injects the PREPARE (and updates the registry). The half-duplex + // sharded path injects inline (himportInjectedCmds) and stays on the pipeline. + (ap.fd != nil && isHImportCmd(cmd)) if !diverted && ap.preflight != nil { if err := ap.preflight(ctx, cmd); err != nil { cmd.SetErr(err) @@ -1330,6 +1492,16 @@ func (ap *AutoPipeliner) submit(ctx context.Context, cmd Cmder) AutoFuture { // No finish here: enqueue stamps ready under the stripe lock, before the // command is visible to any drain (the error paths above still go through // finish for uniform accessor behavior). + if ap.fd != nil { + // Ordered full-duplex: stream on one held connection. enqueue's async + // setReady is replicated here since we bypass it. ctx is threaded so a + // per-command process-hook host can parent its span correctly. + b := ap.fd.submit(ctx, cmd) + if !ap.blocking { + cmd.setReady(b) + } + return AutoFuture{cmd: cmd, batch: b} + } return AutoFuture{cmd: cmd, batch: ap.enqueue(cmd)} } @@ -2533,6 +2705,13 @@ func (ap *AutoPipeliner) Len() int { for _, s := range ap.shards { total += s.Len() } + // Full-duplex accepts commands onto fd.ch instead of the shard queues, so + // include its backlog — otherwise Len() reports 0 while accepted commands are + // buffered behind a backpressured/stalled FD writer, and callers using Len() + // for monitoring or local backpressure lose the signal in FullDuplex mode. + if ap.fd != nil { + total += len(ap.fd.ch) + } return total } diff --git a/autopipeline_fullduplex.go b/autopipeline_fullduplex.go new file mode 100644 index 0000000000..e8f1d9d97f --- /dev/null +++ b/autopipeline_fullduplex.go @@ -0,0 +1,1322 @@ +package redis + +import ( + "context" + "errors" + "fmt" + "runtime/debug" + "sync" + "sync/atomic" + "time" + + "github.com/redis/go-redis/v9/internal" + "github.com/redis/go-redis/v9/internal/otel" + "github.com/redis/go-redis/v9/internal/pool" + "github.com/redis/go-redis/v9/internal/proto" +) + +// Ordered full-duplex dispatch for the ordered AutoPipeline faces (async and +// blocking). +// +// Half-duplex runs one batch per round trip, so a slow link caps throughput at +// batch/RTT and late arrivals wait a full RTT behind the in-flight batch. +// Full-duplex holds ONE pipeline-pool connection with a writer+reader goroutine +// pair: the writer streams command groups back-to-back without waiting for +// reads, the reader drains replies in FIFO order and completes each command as +// its reply lands — ~1 RTT latency, pipe-saturated throughput. +// +// Ordering contract: each goroutine's commands execute in the order it submitted +// them; nothing is promised between goroutines. The submit channel is MPSC (one +// goroutine's sequential submits arrive in order) and one connection is FIFO on +// the wire, so in-flight deque position is the reply-matching key. +// +// Retries: on a connection failure the unacked tail (in order) is re-issued on a +// fresh connection ahead of newly queued work, respecting +// shouldRetry/MaxRetries/backoff AND the per-command NoRetry flag — a NoRetry +// command in the tail fails the tail instead of replaying it (half-duplex does +// the same via cmdsContainNoRetry). After exhaustion those commands are failed +// and the engine keeps serving on a fresh connection. Same at-least-once contract +// as a normal Pipeline: a command whose write landed but whose reply was lost may +// re-execute (matters for non-idempotent writes); the ambiguous set is only the +// unacked tail. +// +// Gated by AutoPipelineOptions.FullDuplex, honored on the ordered single-shard +// faces of a standalone *Client with a pipeline pool and tuned by the +// FullDuplex* options (see their GoDoc). RESP3 push frames are demuxed inline; +// cluster support and window auto-tune are follow-ups (see +// AP_ORDERED_FULLDUPLEX_DESIGN.md). + +var errFDReaderGone = errors.New("redis: autopipeline full-duplex reader exited") + +// errFDPanicRecovered marks a session failure caused by a recovered panic +// (reply decode, batch encode). Wrapped with %w so the retry decision can +// recognize it: the connection is desynced exactly like a transport error, and +// the unacked tail — mostly commands the panic never touched — must be REPLAYED +// on a fresh connection, not failed (shouldRetry alone would reject these plain +// error values and permanently fail innocent in-flight commands). +var errFDPanicRecovered = errors.New("redis: autopipeline: full-duplex panic recovered") + +// Full-duplex tuning defaults, applied by newFDEngine when the corresponding +// AutoPipelineOptions field is zero (rationale in the FullDuplex* GoDoc). The +// window must exceed the bandwidth-delay product (RTT × target rate) or it +// throttles throughput; the deque holds only ACTUAL in-flight, so a generous +// default costs no memory until a stalled peer makes in-flight grow. +const ( + fdDefaultWindow = 65536 + fdDefaultIdle = time.Second + fdDefaultMaxHold = 5 * time.Second +) + +// fdResult is why a full-duplex session ended. +type fdResult int + +const ( + fdGraceful fdResult = iota // AutoPipeliner Close: engine exits + fdConnErr // connection failure: unacked tail returned for replay + fdIdle // idle: conn returned cleanly; re-lease on next command + fdRecycle // max-hold: conn returned cleanly; re-lease immediately (work pending) + fdDenied // acquisition denied (Limiter.Allow reject): fail carry + backlog, engine stays alive + fdLeaseErr // could not lease/init a conn for a new session: retry, then fail carry + backlog after MaxRetries +) + +// fdReq pairs a command with the per-command apBatch whose done channel is +// closed once that command's reply has landed (or it is finally failed). +// +// hookDone is non-nil only when the client has process hooks: the command then +// has a host goroutine (hostHook) running the hook chain, and finalizing closes +// hookDone instead of the batch — the host closes the batch after the hook +// returns, so the hook brackets the command and can rewrite its result before the +// waiter wakes. Nil (the hook-free fast path) finalizes the batch directly. +type fdReq struct { + cmd Cmder + batch *apBatch + hookDone chan struct{} + // ctx is the caller's submit context, kept so the per-command OTel metric can + // be recorded against it (span/baggage correlation), mirroring process(). + ctx context.Context + // writtenAt is stamped when the command is flushed to the wire; the reader + // uses write→reply as the command's operation duration for the OTel metric. + writtenAt time.Time +} + +// complete finalizes a command whose result is already set on it: it wakes the +// caller directly, or (when hooks are present) hands off to the command's host +// goroutine, which runs the hook chain and then wakes the caller. +func (r fdReq) complete() { + if r.hookDone != nil { + close(r.hookDone) + return + } + r.batch.close() +} + +// fdInflight is an ordered FIFO of written-but-unacknowledged commands. The +// writer appends to the back; the reader reads the front's reply then pops it. +// On a connection failure the remaining entries (front→back) are exactly the +// unacked tail, in order, ready to replay. Two close modes: +// - graceful: no more pushes, but the reader keeps reading the remaining +// replies and exits once drained (clean Close). +// - recover: hard stop; the reader abandons the remaining, which are returned +// to the retry loop for replay. +type fdInflight struct { + mu sync.Mutex + cond *sync.Cond + q []fdReq + noMorePush bool // graceful: drain remaining then reader exits + hardClosed bool // recover: reader stops immediately, remaining replayed + room chan struct{} // cap-1 signal: the reader popped, so there is room + peak int // high-water mark of len(q); observability for the backpressure test + advanced int // total entries the reader completed this session (progress signal) +} + +func newFDInflight() *fdInflight { + f := &fdInflight{room: make(chan struct{}, 1)} + f.cond = sync.NewCond(&f.mu) + return f +} + +func (f *fdInflight) len() int { + f.mu.Lock() + n := len(f.q) + f.mu.Unlock() + return n +} + +// pushBatch appends a whole write batch under one lock (fewer lock ops than +// per-command push — matters at loopback op rates). +func (f *fdInflight) pushBatch(reqs []fdReq) { + f.mu.Lock() + f.q = append(f.q, reqs...) + if len(f.q) > f.peak { + f.peak = len(f.q) + } + f.cond.Signal() + f.mu.Unlock() +} + +// peakLen returns the high-water mark of in-flight entries seen so far (test +// observability for the backpressure bound). +// +//nolint:unused // used by the full-duplex backpressure tests; lint runs with tests:false. +func (f *fdInflight) peakLen() int { + f.mu.Lock() + n := f.peak + f.mu.Unlock() + return n +} + +// fdReadBatch caps how many replies the reader snapshots per lock acquisition: +// enough to amortize the mutex over many reads, small enough that the reader +// advances (and signals writer room) frequently even with a deep in-flight. +const fdReadBatch = 256 + +// frontBatch blocks until entries are available (or the deque is closing) and +// returns a snapshot of the front (up to fdReadBatch). ok=false means the reader +// should exit. The writer only ever appends, so this prefix stays the front +// until the reader advance()s it. +func (f *fdInflight) frontBatch(buf []fdReq) ([]fdReq, bool) { + f.mu.Lock() + for len(f.q) == 0 && !f.noMorePush && !f.hardClosed { + f.cond.Wait() + } + if f.hardClosed || len(f.q) == 0 { + f.mu.Unlock() + return buf[:0], false + } + n := len(f.q) + if n > fdReadBatch { + n = fdReadBatch + } + buf = append(buf[:0], f.q[:n]...) + f.mu.Unlock() + return buf, true +} + +// advance removes the front n entries the reader has completed and signals the +// writer that in-flight has room. +func (f *fdInflight) advance(n int) { + if n <= 0 { + return + } + f.mu.Lock() + if n > len(f.q) { + n = len(f.q) + } + // Zero the consumed prefix before reslicing: the reslice keeps the backing + // array (curInflight holds the deque while the engine idles), so otherwise a + // drained burst retains a window's worth of completed fdReq values — command + // args, caller contexts, batches — until the next session overwrites them. + for i := 0; i < n; i++ { + f.q[i] = fdReq{} + } + f.q = f.q[n:] + f.advanced += n + f.mu.Unlock() + select { + case f.room <- struct{}{}: + default: + } +} + +// advancedTotal reports how many commands the reader completed this session — +// the progress signal that resets the reconnect retry budget (a session that +// completed work makes the next connection drop a NEW failure, not a +// consecutive one). +func (f *fdInflight) advancedTotal() int { + f.mu.Lock() + n := f.advanced + f.mu.Unlock() + return n +} + +func (f *fdInflight) empty() bool { + f.mu.Lock() + n := len(f.q) + f.mu.Unlock() + return n == 0 +} + +func (f *fdInflight) closeGraceful() { + f.mu.Lock() + f.noMorePush = true + f.cond.Broadcast() + f.mu.Unlock() +} + +// hardClose signals the reader to stop immediately (used on a connection error). +// It deliberately does NOT take the queue: the caller must wait for the reader to +// exit (<-readerDone) and THEN call takeRemaining, so every entry stays owned by +// exactly one of {the reader completed it, recovery replays/fails it}. A +// concurrent grab could scoop an entry the reader had completed but not yet +// advanced, handing an already-executed command to the retry loop — a double +// execution and, on the hooked path, a double close of hookDone (panic). +func (f *fdInflight) hardClose() { + f.mu.Lock() + f.hardClosed = true + f.cond.Broadcast() + f.mu.Unlock() +} + +// takeRemaining returns the entries the reader left unacknowledged, in order, +// and clears the queue. Call ONLY after the reader has exited (<-readerDone): +// the reader advance()s every command it completes, so what remains is exactly +// the unacked tail, and with the reader gone there is no concurrent access. +func (f *fdInflight) takeRemaining() []fdReq { + f.mu.Lock() + rem := f.q + f.q = nil + f.mu.Unlock() + return rem +} + +type fdEngine struct { + ap *AutoPipeliner + client *Client + pool pool.Pooler + ch chan fdReq // MPSC ordered queue: many submitters -> the writer + maxBatch int + window int // max in-flight (written, unacked) before the writer waits + idle time.Duration // return the conn after this idle gap (0 = never) + maxHold time.Duration // force a clean return at least this often (0 = never) + + recycles atomic.Int64 // clean returns (idle + max-hold); observability/tests + curInflight atomic.Pointer[fdInflight] // current session's in-flight deque; test observability + + submitMu sync.RWMutex // guards closed; RLock across the submit send, WLock to close the gate + closed bool // set once run() is tearing down; submit then rejects new work + + retryWg sync.WaitGroup // tracks off-pipe retries diverted to the normal client path; run() waits it so Close does too + retrySem chan struct{} // caps concurrent off-pipe retries at the window (see retryOnNormalConn) + hostWg sync.WaitGroup // tracks per-command hook-host goroutines (see hostHook); run() waits it so Close does not return while a post-next ProcessHook is still running +} + +func newFDEngine(ap *AutoPipeliner, client *Client) *fdEngine { + mb := ap.config.MaxBatchSize + if mb <= 0 { + mb = 200 + } + // Resolve tuning ONCE here: a zero field means "use the default". In + // particular window must never be 0 — the writer's backpressure gate is + // `for inflight.len() >= window`, so window==0 (0 >= 0) would block the + // writer on the very first submit. Validate rejects negatives. + w := ap.config.FullDuplexWindow + if w <= 0 { + w = fdDefaultWindow + } + idle := ap.config.FullDuplexIdleTimeout + if idle <= 0 { + idle = fdDefaultIdle + } + maxHold := ap.config.FullDuplexMaxHold + if maxHold <= 0 { + maxHold = fdDefaultMaxHold + } + // The submit queue does not need window-sized storage: backpressure is + // enforced by the in-flight deque (which grows only with ACTUAL in-flight), + // while a buffered channel allocates its full capacity up front — at the + // default window that is several MiB per engine before any command is + // submitted. Cap the queue; total outstanding stays bounded by cap+window + // and submit simply blocks a little earlier under a burst. + chCap := w + if chCap > 4096 { + chCap = 4096 + } + return &fdEngine{ + ap: ap, + client: client, + pool: client.getPipelinePool(), + ch: make(chan fdReq, chCap), + maxBatch: mb, + window: w, + idle: idle, + maxHold: maxHold, + retrySem: make(chan struct{}, w), + } +} + +// submit enqueues a command onto the ordered stream and returns its batch. +// Blocks when the queue is full (backpressure) or bails if the engine is +// closing. The caller (AutoPipeliner.submit) stamps setReady on the async face. +// With process hooks installed a per-command host goroutine runs the hook chain +// (see hostHook) and ctx parents its span; the hook-free path skips that +// goroutine and channel entirely. +func (fd *fdEngine) submit(ctx context.Context, cmd Cmder) *apBatch { + if fd.ap.isClosed() { + cmd.SetErr(ErrClosed) + return completedBatch + } + b := newAPBatch() + var hookDone chan struct{} + if fd.ap.pipeliner.hookCount() > 0 { + hookDone = make(chan struct{}) + } + req := fdReq{cmd: cmd, batch: b, hookDone: hookDone, ctx: ctx} + + // Send under RLock and re-check closed so a send can never win the race with + // run()'s shutdown drain (takeQueue: WLock, set closed, drain fd.ch). Once the + // final drain has run no new req can land in fd.ch, where it would never be + // completed and would hang its caller forever. A send blocked on a full channel + // is released by the ctx.Done() branch below, so holding the RLock cannot wedge + // the WLock. + fd.submitMu.RLock() + if fd.closed { + fd.submitMu.RUnlock() + cmd.SetErr(ErrClosed) + // Submit-time rejection: return the shared completedBatch sentinel (no host + // was started) so processAsync surfaces the error from raw Process(ctx,cmd), + // matching every other submit-time-rejection path. + return completedBatch + } + select { + case fd.ch <- req: + // Accepted. Start the hook host ONLY now: a submission that is never admitted + // (the cancel paths below) must not leak a host goroutine. The Add happens under + // the gate, so it is ordered before the shutdown drain's WLock and run()'s + // hostWg.Wait never races an Add on a zero counter. + if hookDone != nil { + fd.hostWg.Add(1) + go fd.hostHook(ctx, cmd, b, hookDone) + } + fd.submitMu.RUnlock() + return b + case <-ctx.Done(): + // Caller's ctx expired while backpressured (window/channel full): honor it + // instead of blocking until room or Close (#3964). Not admitted and no host + // started, so this is a submit-time failure — return the completedBatch sentinel + // so raw Process(ctx,cmd) reports the ctx error. + fd.submitMu.RUnlock() + cmd.SetErr(ctx.Err()) + return completedBatch + case <-fd.ap.ctx.Done(): + fd.submitMu.RUnlock() + cmd.SetErr(ErrClosed) + return completedBatch + } +} + +// hostHook runs the user process-hook chain for one full-duplex command on its +// own goroutine, started only when hookCount()>0. The chain starts at ≈ submit +// time and its next() blocks until the reader (or a failure/close path) signals +// hookDone, so an observing hook spans the command's real write→reply latency and +// a hook that rewrites the result is honored before the waiter wakes. Each +// command is reported individually (withProcessHook), not as a pipeline batch. +func (fd *fdEngine) hostHook(ctx context.Context, cmd Cmder, b *apBatch, hookDone chan struct{}) { + // Declared first so it runs last: Close waits hostWg (via run()), and the host + // is done only after the recover defer below has also run. + defer fd.hostWg.Done() + // Mark this goroutine as the batch's executor so a hook that reads its own + // command's result after next() (cmd.Err(), a documented pattern) sees the + // just-executed view instead of blocking on batch.done — which only THIS + // goroutine closes, below, so without the mark such a hook self-deadlocks. + // Mirrors runOutsidePipeline's async dispatch guard. + if fd.ap.armSelfDeadlockGuard() { + b.dispGid.Store(curGoroutineID()) + } + // A user ProcessHook runs on this goroutine; an unrecovered panic here would + // crash the process (and leave the caller blocked on b). Recover, fail the + // command, and close the batch so the waiter always wakes — mirroring the + // dispatch path's recoverDispatchPanic. + nextCalled := false + defer func() { + if r := recover(); r != nil { + // The command was already streamed, so the reader still owns cmd and will + // write its reply into it. A panic BEFORE next() means hookDone was never + // awaited — await it here so the reader's writes happen-before the caller's + // reads. After next() it was already awaited. + if !nextCalled { + <-hookDone + } + if cmd.rawErr() == nil { + cmd.SetErr(fmt.Errorf("redis: autopipeline: panic in full-duplex process hook: %v", r)) + } + internal.Logger.Printf(ctx, "autopipeline: recovered full-duplex hook panic: %v\n%s", r, debug.Stack()) + b.close() + } + }() + err := fd.ap.pipeliner.withProcessHook(ctx, cmd, func(context.Context, Cmder) error { + nextCalled = true + <-hookDone // reply landed (or the command was failed) + return cmd.rawErr() // direct read: cmd.Err() would await batch.done, which + // this goroutine itself closes below → self-deadlock. + }) + // A hook that SHORT-CIRCUITS (returns without calling next) never received + // hookDone, but the command is already on the wire and the reader will still + // write into cmd: await that before releasing the caller. The command still + // executed; the hook's error is honored anyway (see the FullDuplex GoDoc). + if !nextCalled { + <-hookDone + } + cmd.SetErr(err) // honor a hook that rewrote / short-circuited the result + b.close() // now wake the waiter +} + +// retryOnNormalConn re-runs a full-duplex command that came back with a retryable +// Redis error (LOADING/READONLY/…) or a redirect (MOVED/ASK) on the client's +// NORMAL path: that path routes redirects to the proper node and applies the +// standard retry/backoff, neither of which the fixed single-conn FD socket can +// do. It runs on its own goroutine so it does not stall the FD reader, is tracked +// by retryWg so Close waits for it, and settles the FD request with the outcome. +// process() is the raw exec (no hook chain) — with hooks installed the FD +// hostHook still brackets the command and reports via req.complete(). Background +// ctx: the command was already accepted, so it completes even under a Close. +func (fd *fdEngine) retryOnNormalConn(req fdReq) { + // Bound concurrent off-pipe retries to the FD window: a sustained retryable + // stream would otherwise spawn one goroutine per reply, all parked in + // backoff/pool acquisition. Blocking here blocks the READER, which stops + // advancing the deque, which fills the window and blocks the writer and then + // submitters — end-to-end backpressure. No cycle: retries drain on the main + // pool, independent of the reader waiting here. + // Interruptible acquire: the reader must not park here past Close. A wait is + // otherwise BOUNDED — every slot holder is a retry running through process(), + // whose own timeouts/backoff guarantee it releases — but on Close nothing + // should keep the reader from observing teardown, so fail the request + // directly instead. + select { + case fd.retrySem <- struct{}{}: + case <-fd.ap.ctx.Done(): + req.cmd.SetErr(ErrClosed) + req.complete() + return + } + fd.retryWg.Add(1) + go func() { + defer func() { + <-fd.retrySem + fd.retryWg.Done() + }() + // process runs user code (hooks, arg encoders) and can panic; without + // recovery the batch never completes and the caller (and a hooked command's + // host, parked on hookDone) waits forever. + defer func() { + if r := recover(); r != nil { + req.cmd.SetErr(fmt.Errorf("redis: autopipeline: panic in full-duplex off-pipe retry: %v", r)) + internal.Logger.Printf(context.Background(), + "autopipeline: recovered full-duplex retry panic: %v\n%s", r, debug.Stack()) + req.complete() + } + }() + err := fd.ap.pipeliner.process(context.Background(), req.cmd) + req.cmd.SetErr(err) + req.complete() + }() +} + +// run owns the engine for the AutoPipeliner's lifetime: acquire a pipeline-pool +// connection, run one full-duplex attempt on it, and on connection failure +// replay the unacked tail on a fresh connection (bounded by MaxRetries/backoff) +// while continuing to serve the queue. Exits only on graceful Close. +func (fd *fdEngine) run() { + defer fd.ap.wg.Done() + // Runs before wg.Done (LIFO), so Close — which waits ap.wg — also waits for + // any off-pipe retries still running on the normal client path. + defer fd.retryWg.Wait() + // Same for per-command hook hosts: a ProcessHook doing work after next() + // closes the command's batch on its host goroutine, so Close must not return + // while one runs. Every hostWg.Add is gated behind submitMu+closed, and every + // run() return follows the shutdown drain, so this never races a live Add. + defer fd.hostWg.Wait() + bg := context.Background() + var carry []fdReq // unacked tail to re-issue at the start of the next attempt + // Two SEPARATE budgets, each counting only CONSECUTIVE failures of its own + // kind: a shared counter would let transient lease failures eat the reconnect + // budget, so the first genuine mid-session drop would fail the whole unacked + // tail with zero replay attempts. leaseAttempts resets whenever a session + // actually ran; retryAttempts resets on a clean session end (idle/recycle). + leaseAttempts := 0 // consecutive fdLeaseErr/fdDenied acquisition failures + retryAttempts := 0 // consecutive fdConnErr tail-replay failures + for { + if fd.ap.ctx.Err() != nil { + fd.shutdownFlush(bg, carry) + return + } + // Never lease a connection (or hit the Limiter / dial) without work in hand: + // block for the first command whenever the carry is empty. That covers the + // initial entry, the fdIdle return, and the fail-fast exits below (fdDenied / + // exhausted fdLeaseErr / failed fdConnErr tail), which would otherwise loop + // straight back into attempt against an empty queue — hammering the Limiter or + // dialing a down server forever. Work already queued makes this non-blocking, + // so fdRecycle re-leases immediately; an empty recycle parks here. + if len(carry) == 0 { + select { + case r := <-fd.ch: + carry = []fdReq{r} + case <-fd.ap.ctx.Done(): + fd.shutdownFlush(bg, nil) + return + } + } + unacked, result, aerr := fd.attempt(bg, carry) + switch result { + case fdGraceful: + return // Close: attempt drained written work; queue failed there. + case fdIdle: + // Conn returned cleanly to the pool (its per-conn hooks can run); the + // loop-top wait keeps an idle engine from churning Get/Put. + carry, leaseAttempts, retryAttempts = nil, 0, 0 + case fdRecycle: + // Max-hold reached; conn returned cleanly. Work is pending — re-lease + // immediately. + fd.recycles.Add(1) + carry, leaseAttempts, retryAttempts = nil, 0, 0 + case fdLeaseErr: + // Could not lease/init a connection for a new session (server down, pool + // saturated). Retry for a transient outage; once retries are exhausted, + // fail-fast the carry tail AND the fd.ch backlog rather than leaving accepted + // commands buffered indefinitely, and stay alive to serve again once the + // server/pool recovers. Replaying the carry wholesale is safe: it is already + // NoRetry-split (or nil) and was never written on a new conn. + // Close racing the lease surfaces here as a lease failure (the acquisition ctx + // is cancelled), so flush the accepted work through the normal pipeline path + // instead of failing it with a canceled error. + if fd.ap.ctx.Err() != nil { + fd.shutdownFlush(bg, carry) + return + } + if shouldRetry(aerr, true) && leaseAttempts < fd.client.opt.MaxRetries { + leaseAttempts++ + fd.sleepBackoff(leaseAttempts) + continue // carry unchanged; re-lease + } + fd.failReqs(carry, aerr) + fd.failQueue(aerr) + carry = nil + leaseAttempts++ + fd.sleepBackoff(leaseAttempts) + if leaseAttempts >= fd.client.opt.MaxRetries { + leaseAttempts = 0 + } + case fdDenied: + // Same Close-race guard as fdLeaseErr: on shutdown, flush instead of failing + // accepted work with a denial that only exists because Close interrupted the + // acquisition. + if fd.ap.ctx.Err() != nil { + fd.shutdownFlush(bg, carry) + return + } + // The Limiter denied acquisition: fail-fast the carry tail AND the whole fd.ch + // backlog with the limiter error (as the plain-client getConn path does) + // instead of leaving them buffered until the breaker closes. The engine stays + // alive and backs off, so it serves again once the limiter admits. + fd.failReqs(carry, aerr) + fd.failQueue(aerr) + carry = nil + leaseAttempts++ + fd.sleepBackoff(leaseAttempts) + if leaseAttempts >= fd.client.opt.MaxRetries { + leaseAttempts = 0 + } + default: // fdConnErr — a real connection error occurred + // A session ran, so the lease succeeded: acquisition failures are no + // longer consecutive. + leaseAttempts = 0 + // A session that COMPLETED work (advanced the deque — including a + // successful carry replay) makes this drop a new failure, not a + // consecutive one: reset the reconnect budget so long-lived sessions + // under continuous traffic do not inherit stale failure counts. + if fi := fd.curInflight.Load(); fi != nil && fi.advancedTotal() > 0 { + retryAttempts = 0 + } + // retryTimeout=true: a read/write timeout is a retryable connection + // failure here (re-issue the unacked tail on a fresh conn), matching + // the cluster pipeline retry paths — otherwise a single WAN timeout + // fails the whole tail. The engine's internal failure markers + // (recovered panics, reader-gone) desync the conn exactly like a + // transport error and the tail is mostly commands they never touched, + // so they are replayable too — shouldRetry alone would reject them and + // permanently fail innocent in-flight commands. The NoRetry guard + // below still protects non-idempotent writes. + replayable := shouldRetry(aerr, true) || + errors.Is(aerr, errFDReaderGone) || errors.Is(aerr, errFDPanicRecovered) + if len(unacked) > 0 && replayable && + retryAttempts < fd.client.opt.MaxRetries { + // Split at the first NoRetry command: replay the retryable PREFIX and fail + // that command plus everything ordered after it (a NoRetry command must + // never be re-sent). With NoRetry first (n==0) nothing is retryable ahead + // of it, so fall through and fail the whole tail. + if n := fdFirstNoRetry(unacked); n > 0 { + if n < len(unacked) { + fd.failReqs(unacked[n:], aerr) + } + retryAttempts++ + fd.sleepBackoff(retryAttempts) + carry = unacked[:n] + continue + } + } + // Not retrying the tail (none, non-retryable, or exhausted): fail it, + // then ALWAYS back off before re-leasing so a dead server cannot spin + // this loop. Keep the engine alive to serve new work when it recovers. + fd.failReqs(unacked, aerr) + carry = nil + retryAttempts++ + fd.sleepBackoff(retryAttempts) + if retryAttempts >= fd.client.opt.MaxRetries { + retryAttempts = 0 // reset so backoff restarts small once we're serving again + } + } + } +} + +// attempt acquires a connection, runs one full-duplex session (re-issuing carry +// first), and releases the connection. Returns the unacked tail + error on +// connection failure, or graceful=true on Close. +func (fd *fdEngine) attempt(bg context.Context, carry []fdReq) (unacked []fdReq, result fdResult, aerr error) { + // Per-session Limiter: FD acquires ONE conn per session, so Allow() once here — + // if it rejects, do not acquire and do not report (mirrors getConn). Otherwise + // ReportResult exactly once at release and BEFORE the conn becomes available + // again, so a breaker sees the failure before admitting the next session; that + // is why the report and the release share ONE deferred func, in that order. + limited := fd.client.opt.Limiter != nil + if limited { + if err := fd.client.opt.Limiter.Allow(); err != nil { + // Denied (e.g. an open circuit breaker): report nothing (no conn was + // acquired) and signal fdDenied so run() fail-fasts the carry and the fd.ch + // backlog instead of leaving them buffered until the breaker closes. + return carry, fdDenied, err + } + } + + var cn *pool.Conn + defer func() { + if limited { + fd.client.opt.Limiter.ReportResult(aerr) + } + if cn == nil { + return // nothing acquired, or already Removed inline below + } + // ANY connection-error end (result==fdConnErr) leaves the conn desynced — + // an unread reply tail, a partial write, or a reader protocol error — so it + // MUST be removed; Put()ing it would poison the pool. Keying on result (not + // isBadConn) is deliberate: errFDReaderGone or a plain write timeout are not + // classified bad-conn, yet the conn is still unusable. Clean ends go through + // releaseConnToPool (drains pending pushes before Put; removes if desynced). + if result == fdConnErr { + fd.pool.Remove(bg, cn, aerr) + } else { + fd.client.releaseConnToPool(bg, fd.pool, cn, nil) + } + }() + + // Acquire under ap.ctx (not bg): if Close cancels ap.ctx while this Get is + // blocked on a saturated pool it returns at once instead of waiting out + // PoolTimeout, so shutdown is not delayed. Everything after — init and the + // session I/O — stays on bg so already-accepted commands still complete during + // Close (Close waits for run() via ap.wg). + cn, aerr = fd.pool.Get(fd.ap.ctx) + if aerr != nil { + cn = nil + return carry, fdLeaseErr, aerr + } + // Init + acquire through initPooledConn rather than hand-inlining + // initConn/TryAcquire (the main and pipeline paths drift when mirrored by hand). + // 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 { + cn = nil // initPooledConn already Removed it + return carry, fdLeaseErr, e + } + + unacked, result, aerr = fd.session(bg, cn, carry) + return unacked, result, aerr +} + +// session runs the writer (this goroutine) + reader (spawned) on one connection +// until Close (graceful) or a connection error (returns the unacked tail). +func (fd *fdEngine) session(bg context.Context, cn *pool.Conn, carry []fdReq) (unacked []fdReq, result fdResult, aerr error) { + inflight := newFDInflight() + fd.curInflight.Store(inflight) // test observability (peak in-flight) + readerDone := make(chan struct{}) + + // Honor opt.ReadTimeout as-is for each per-reply read: options.go maps a + // disabled timeout (-1) to 0 and WithReader treats <= 0 as "no deadline", so + // disabled stays disabled instead of being clamped to some fixed value (a + // default client keeps its 5s, which bounds each read). A genuinely stuck read + // is still unblocked by the conn Close on the fdConnErr path. + readTimeout := fd.client.opt.ReadTimeout + var errOnce sync.Once + var sharedErr error + failOnce := func(e error) { errOnce.Do(func() { sharedErr = e }) } + + // Reader: read replies in FIFO order, completing each command as its reply + // lands. Works a bounded front-snapshot per lock (amortizes the mutex over + // many reads), then advances. On a connection/protocol error it stops and + // leaves the unread tail in the deque (it becomes the unacked recovery set). + go func() { + defer close(readerDone) + // done counts commands completed in the CURRENT frontBatch snapshot that + // have not yet been advanced out of the in-flight deque; it is 0 outside the + // inner read loop (reset at the top of each iteration, advanced at the end). + done := 0 + // A reply decoder can panic (e.g. a RawWriteToCmd whose user io.Writer panics + // while readReply streams the raw reply). Recover and mark the session failed + // (failOnce) so run() takes the connection-error path: the reader exits, the + // unacked tail is recovered and the conn is removed. advance(done) FIRST so + // commands already completed in the panicking snapshot leave the deque — + // otherwise recovery re-owns and re-completes them, overwriting good results + // and double-closing hookDone (a second panic) when hooks are installed. + defer func() { + if r := recover(); r != nil { + inflight.advance(done) + failOnce(fmt.Errorf("%w: reader: %v", errFDPanicRecovered, r)) + internal.Logger.Printf(bg, "autopipeline: recovered full-duplex reader panic: %v\n%s", r, debug.Stack()) + } + }() + var buf []fdReq + for { + done = 0 + var ok bool + buf, ok = inflight.frontBatch(buf) + if !ok { + return + } + var rerr error + // Read each reply as it lands (one WithReader per reply). Reading the + // whole snapshot inside a single WithReader was measurably slower on + // loopback: it blocks on commands the writer has pushed but not yet + // flushed, collapsing writer/reader overlap. + for i := range buf { + req := buf[i] + e := cn.WithReader(bg, readTimeout, func(rd *proto.Reader) error { + // Drain RESP3 push frames buffered ahead of this reply so a push is + // never misread as the command's reply (FIFO misalign). A drain error + // is logged, NOT propagated (as in flushBatch): a custom push + // processor's error must not fail this unrelated in-flight command or + // kill the connection. A real transport error resurfaces in readReply. + if perr := fd.client.processPendingPushNotificationWithReader(bg, cn, rd); perr != nil { + internal.Logger.Printf(bg, "autopipeline: full-duplex push drain: %v", perr) + } + return req.cmd.readReply(rd) + }) + if e != nil && !isRedisError(e) { + rerr = e // connection/protocol error: stop; unread tail stays + break + } + // A retryable Redis error or a redirect (MOVED/ASK) is NOT the caller's + // final answer: the FD conn is one fixed socket/node, so re-run the + // command on the client's NORMAL path, which routes redirects and applies + // the standard retry/backoff. Done off the reader goroutine so it does not + // stall other in-flight replies, and counted in `done` so the reader + // advances past it now. Per-caller ordering is NOT promised across this + // divert (same exception as the blocking-command divert); NoRetry commands + // keep their error. + if e != nil && !req.cmd.NoRetry() { + moved, ask, _ := isMovedError(e) + // MOVED/ASK are redirects, not retries — always follow them (the fixed + // FD socket cannot). A retryable error diverts only when retries are + // enabled: with MaxRetries normalized to 0 the divert's process() would + // still run attempt zero, re-sending a command whose retry the caller + // explicitly disabled — surface the Redis error instead. + if moved || ask || (shouldRetry(e, false) && fd.client.opt.MaxRetries > 0) { + fd.retryOnNormalConn(req) + done++ + continue + } + } + req.cmd.SetErr(e) // nil or a non-retryable Redis error (WRONGTYPE, …) + // Per-command OTel duration (write→reply): the FD reader bypasses + // process, which is what normally emits it. Inline-completed commands + // only — a diverted command emits its own through process. + if cb := otel.GetOperationDurationCallback(); cb != nil { + octx := req.ctx + if octx == nil { + octx = bg + } + cb(octx, time.Since(req.writtenAt), req.cmd, 1, e, cn, fd.client.opt.DB) + } + // Same parity for errors: an inline-completed non-retryable Redis error + // (WRONGTYPE, NOPERM, …) must reach the native error callback, which + // process() would otherwise emit via classifyCommandError. + if e != nil { + if errorCallback := pool.GetMetricErrorCallback(); errorCallback != nil { + errorType, statusCode, isInternal := classifyCommandError(e) + errorCallback(bg, errorType, cn, statusCode, isInternal, 0) + } + } + req.complete() // wake the caller, or hand off to the hook host + done++ + } + inflight.advance(done) + if rerr != nil { + failOnce(rerr) + return + } + } + }() + + // Idle / max-hold timers arm the clean-return paths. A disabled timer uses a + // nil channel (never selected). + var idleC, maxC <-chan time.Time + var idleT, maxT *time.Timer + if fd.idle > 0 { + idleT = time.NewTimer(fd.idle) + idleC = idleT.C + defer idleT.Stop() + } + if fd.maxHold > 0 { + maxT = time.NewTimer(fd.maxHold) + maxC = maxT.C + defer maxT.Stop() + } + resetIdle := func() { + if idleT == nil { + return + } + if !idleT.Stop() { + select { + case <-idleT.C: + default: + } + } + idleT.Reset(fd.idle) + } + + result = fdConnErr // default until a break sets otherwise + + // Writer: re-issue the recovered tail first, then serve the queue. The tail goes + // in the SAME MaxBatchSize/MaxBatchBytes-capped chunks as freshly drained work — + // it can hold up to fd.window commands, so one flush would ignore MaxBatchBytes + // and hit a write-timeout/burst on the new connection. + writeErr := fd.writeCarryChunked(bg, cn, inflight, carry, readerDone) + if writeErr == nil { + scratch := make([]fdReq, 0, fd.maxBatch) + byteLimit := int64(fd.ap.config.MaxBatchBytes) // 0 = disabled + serve: + for { + // Backpressure: bound the in-flight (written-but-unacked) deque. Wait + // for the reader to drain below the window BEFORE taking new work, so + // a slow/stalled peer cannot grow in-flight without bound. Done here + // (not mid-batch) so no drained work is ever held during the wait. + for inflight.len() >= fd.window { + select { + case <-inflight.room: + case <-readerDone: + break serve // reader hit a connection error + case <-fd.ap.ctx.Done(): + result = fdGraceful + break serve + case <-maxC: + result = fdRecycle + break serve + } + } + // Go's select picks randomly among ready cases, so with work queued AND + // the reader gone (decode panic, protocol error) the main select below + // could write a batch to a connection known to have no reader — needlessly + // enlarging the ambiguous at-least-once set. Check readerDone first. + select { + case <-readerDone: + break serve // result stays fdConnErr; unacked tail is recovered + default: + } + select { + case req := <-fd.ch: + batch := append(scratch[:0], req) + batchBytes := cmdApproxBytes(req.cmd) + // Cap this batch by the REMAINING window room, not just MaxBatchSize: + // the gate above only ensures in-flight < window before draining, so a + // window smaller than MaxBatchSize would let one drain blow through it + // (window=1, batch=200 → 200 in flight). The first command always goes + // (room is >= 1 after the gate). + limit := fd.maxBatch + if room := fd.window - inflight.len(); room < limit { + limit = room + } + drain: + for len(batch) < limit { + // Soft MaxBatchBytes cap (like the half-duplex path): stop + // accumulating once the payload reaches the limit, so one flush + // cannot buffer an unbounded write. The first command is always + // included, so a lone oversized command still goes. + if byteLimit > 0 && batchBytes >= byteLimit { + break drain + } + select { + case r := <-fd.ch: + batch = append(batch, r) + batchBytes += cmdApproxBytes(r.cmd) + default: + break drain + } + } + if e := fd.writeBatch(bg, cn, inflight, batch); e != nil { + writeErr = e + break serve + } + resetIdle() + case <-readerDone: + break serve // reader hit a connection error (result stays fdConnErr) + case <-fd.ap.ctx.Done(): + result = fdGraceful + break serve + case <-idleC: + // Only return the conn when genuinely idle: nothing queued AND the + // in-flight drained. Otherwise the timer fired mid-stream (e.g. a long + // flush) — re-arm and keep the hot session. + if inflight.empty() && len(fd.ch) == 0 { + result = fdIdle + break serve + } + resetIdle() + case <-maxC: + // Max-hold reached. With the pipe drained (nothing in-flight, nothing + // queued) return fdIdle so run() blocks for the next command: otherwise a + // quiet engine with FullDuplexMaxHold < FullDuplexIdleTimeout would + // Get/Put-churn (and re-charge the Limiter/session hooks) every interval. + // With work pending, recycle to keep serving. + if inflight.empty() && len(fd.ch) == 0 { + result = fdIdle + } else { + result = fdRecycle + } + break serve + } + } + } + if writeErr != nil { + failOnce(writeErr) + result = fdConnErr + } + + switch result { + case fdGraceful: + // Clean Close: flush the accepted-but-unwritten fd.ch backlog on this + // connection first, so Close honors "accepted ⇒ completes" instead of failing + // it ErrClosed, then let the reader drain every in-flight reply to a RESP + // boundary. A flush write error surfaces via the sharedErr path below. + if e := fd.flushBacklogForClose(bg, cn, inflight, readerDone); e != nil { + // The backlog write failed partway: some flushed commands have no reply + // coming and writeCarryChunked pushed the unwritten suffix into inflight, so + // closeGraceful would park the reader on replies that never arrive. Take the + // connection-error path: close the conn to wake the reader, then fail the + // whole unacked tail (accepted suffix included). + failOnce(e) + inflight.hardClose() + _ = cn.Close() + <-readerDone + fd.failReqs(inflight.takeRemaining(), e) + return nil, fdConnErr, e + } + inflight.closeGraceful() + <-readerDone + if sharedErr != nil { + // Reader failed during the final drain: fail the stranded tail (its callers + // would hang) and report the error so attempt() removes the desynced conn. + // run() exits on its next loop, so this does not retry. + fd.failReqs(inflight.takeRemaining(), sharedErr) + return nil, fdConnErr, sharedErr + } + return nil, fdGraceful, nil + case fdIdle, fdRecycle: + // Clean return: no more pushes, reader drains remaining replies, then the + // conn is at a RESP boundary and safe to Put back to the pool. + inflight.closeGraceful() + <-readerDone + if sharedErr != nil { + // Reader failed while draining for the clean return: recover the unacked + // tail for replay and report the error so the conn is removed instead of + // Put back poisoned. + return inflight.takeRemaining(), fdConnErr, sharedErr + } + return nil, result, nil + default: // fdConnErr + // Stop the reader, wait for it to exit, THEN take the unacked tail: the reader + // advances every command it completes, so taking only after <-readerDone is + // what keeps an entry from being owned by both sides. + // + // Close the connection before waiting: on a WRITE error the reader is + // typically parked in WithReader awaiting a reply that will never arrive, and + // hardClose only wakes a reader parked in frontBatch. Closing makes that read + // return at once so recovery does not stall for the read deadline; attempt() + // removes this conn right after, so the close is safe and idempotent. + inflight.hardClose() + _ = cn.Close() + <-readerDone + unacked = inflight.takeRemaining() + if sharedErr == nil { + sharedErr = errFDReaderGone + } + return unacked, fdConnErr, sharedErr + } +} + +// writeBatch pushes each req onto the in-flight FIFO (so it is tracked as +// unacked even if the flush then fails) and writes the whole batch in one +// buffered flush. A write error leaves the reqs in the deque for recovery. +func (fd *fdEngine) writeBatch(bg context.Context, cn *pool.Conn, inflight *fdInflight, reqs []fdReq) (err error) { + if len(reqs) == 0 { + return nil + } + // A command encoder can panic (e.g. a user BinaryMarshaler failing while + // writeCmd serializes the args) on the writer goroutine, where it would crash + // the process. Convert it to a connection error: the batch is already in the + // deque and the write may be partial, so the conn is desynced and every caller + // settles through the normal conn-error recovery. + defer func() { + if r := recover(); r != nil { + err = fmt.Errorf("%w: encoding batch: %v", errFDPanicRecovered, r) + internal.Logger.Printf(bg, "autopipeline: recovered full-duplex write panic: %v\n%s", r, debug.Stack()) + } + }() + // Stamp the wire-write time so the reader can record write→reply as the + // command's OTel operation duration. Done before pushBatch so the copies the + // reader reads from the deque carry it. + now := time.Now() + for i := range reqs { + reqs[i].writtenAt = now + } + inflight.pushBatch(reqs) + return cn.WithWriter(bg, fd.client.opt.WriteTimeout, func(wr *proto.Writer) error { + for i := range reqs { + if e := writeCmd(wr, reqs[i].cmd); e != nil { + return e + } + } + return nil + }) +} + +// fdBatchEnd returns the exclusive end index of the next write chunk starting at +// `start`, applying the same caps as the drain loop: at most maxBatch commands, +// and (when byteLimit > 0) stop once the accumulated approximate payload reaches +// the limit — but always include the first command, so a lone oversized command +// still goes. Pure; the boundary logic is unit-tested. +func fdBatchEnd(reqs []fdReq, start, maxBatch int, byteLimit int64) int { + end := start + 1 + bytes := cmdApproxBytes(reqs[start].cmd) + for end < len(reqs) && end-start < maxBatch { + if byteLimit > 0 && bytes >= byteLimit { + break + } + bytes += cmdApproxBytes(reqs[end].cmd) + end++ + } + return end +} + +// writeCarryChunked re-issues a recovered tail on a fresh connection in the same +// capped chunks as freshly drained work (see fdBatchEnd), so a large recovered +// window is not flushed in one oversized write. Returns the first write error. +func (fd *fdEngine) writeCarryChunked(bg context.Context, cn *pool.Conn, inflight *fdInflight, carry []fdReq, readerDone <-chan struct{}) error { + byteLimit := int64(fd.ap.config.MaxBatchBytes) // 0 = disabled + for i := 0; i < len(carry); { + // Between chunks, stop if the reader is gone (decode panic, protocol + // error mid-replay): writing further chunks to a reader-less connection + // only enlarges the ambiguous at-least-once set — same priority rule as + // the serve loop. Push the un-written remainder so takeRemaining recovers + // the whole accepted set. + if readerDone != nil { + select { + case <-readerDone: + inflight.pushBatch(carry[i:]) + return errFDReaderGone + default: + } + } + end := fdBatchEnd(carry, i, fd.maxBatch, byteLimit) + if e := fd.writeBatch(bg, cn, inflight, carry[i:end]); e != nil { + // writeBatch pushed carry[i:end] into inflight before the failed write, but + // the suffix carry[end:] was never pushed. Push it too, or it sits in neither + // fd.ch nor inflight and its callers hang: on fdConnErr takeRemaining replays + // it, on Close the caller fails it. It is only ever settled via failReqs or + // replayed — never completed inline by the reader — so its zero writtenAt + // never reaches the write→reply metric. + if end < len(carry) { + inflight.pushBatch(carry[end:]) + } + return e + } + i = end + } + return nil +} + +// fdFirstNoRetry returns the index of the first NoRetry command in reqs, or +// len(reqs) when there is none. The unacked tail is retried up to this index and +// failed from it on: retryable commands ahead of a NoRetry still get their +// network retries, while the NoRetry command and anything ordered after it is +// never re-sent. +func fdFirstNoRetry(reqs []fdReq) int { + for i := range reqs { + if reqs[i].cmd.NoRetry() { + return i + } + } + return len(reqs) +} + +// failReqs completes a set of commands with err (used on retry exhaustion / Close). +func (fd *fdEngine) failReqs(reqs []fdReq, err error) { + // Error-metric parity: commands terminated here (lease failure, retry + // exhaustion, a NoRetry tail, Close) never reach the reader's inline + // completion, so emit the native error callback per command. One classification + // for the whole set (every req fails with the same err), and no duration metric + // — many of these were never written. + errorCallback := pool.GetMetricErrorCallback() + var errorType, statusCode string + var isInternal bool + if errorCallback != nil && len(reqs) > 0 { + errorType, statusCode, isInternal = classifyCommandError(err) + } + for i := range reqs { + // rawErr(), not Err(): this runs on the engine goroutine, and Err() + // awaits batch.done — the very channel complete() closes just below — so + // awaiting here would self-deadlock (the same trap hostHook documents). + if reqs[i].cmd.rawErr() == nil { + reqs[i].cmd.SetErr(err) + } + if errorCallback != nil { + octx := reqs[i].ctx + if octx == nil { + octx = context.Background() + } + errorCallback(octx, errorType, nil, statusCode, isInternal, 0) + } + reqs[i].complete() + } +} + +// takeQueue closes the submit gate and returns everything buffered in fd.ch. The +// WLock blocks until in-flight submit sends finish (each either landed in fd.ch, +// is drained below, or took its ctx.Done() branch), so after the drain no submit +// can enqueue work that would be left un-completed. +// +// INVARIANT: every takeQueue call is a terminal shutdown drain — run() exits +// right after, past a ctx-cancel check. Never call it on a non-close path: a +// submit blocked on a full channel is unwedged only by its ctx.Done() branch, so +// without a cancelled ctx the WLock deadlocks against the RLock held across that +// send. +func (fd *fdEngine) takeQueue() []fdReq { + fd.submitMu.Lock() + fd.closed = true + fd.submitMu.Unlock() + var reqs []fdReq + for { + select { + case r := <-fd.ch: + reqs = append(reqs, r) + default: + return reqs + } + } +} + +// shutdownFlush is the between-sessions Close flush: accepted commands in carry +// (an unacked tail from a failed session, never re-leased) and in fd.ch +// (accepted while no session held a connection) are executed on the client's +// normal pipeline path, honoring the "accepted ⇒ completes" Close contract +// instead of failing them ErrClosed just because Close won the race between +// sessions (#3964). Uses a background ctx (ap.ctx is already cancelled); +// processPipeline bounds it with the client's own timeouts/retries and setCmdsErr +// puts any failure on every command, so callers always settle. +func (fd *fdEngine) shutdownFlush(bg context.Context, carry []fdReq) { + backlog := append(carry, fd.takeQueue()...) + if len(backlog) == 0 { + return + } + // A panic here (user arg encoder inside processPipeline) runs on the engine + // goroutine with no other recovery; fail and complete the remainder so no + // caller hangs. + i := 0 + defer func() { + if r := recover(); r != nil { + err := fmt.Errorf("redis: autopipeline: panic in shutdown flush: %v", r) + internal.Logger.Printf(bg, "autopipeline: recovered shutdown-flush panic: %v\n%s", r, debug.Stack()) + fd.failReqs(backlog[i:], err) + } + }() + // Same MaxBatchSize/MaxBatchBytes chunking as normal FD writes: the backlog can + // hold the carry plus the whole channel, and one unchunked pipeline would + // ignore MaxBatchBytes and burst the connection. + byteLimit := int64(fd.ap.config.MaxBatchBytes) // 0 = disabled + for i < len(backlog) { + end := fdBatchEnd(backlog, i, fd.maxBatch, byteLimit) + cmds := make([]Cmder, end-i) + for j := i; j < end; j++ { + cmds[j-i] = backlog[j].cmd + } + err := fd.client.processPipeline(bg, cmds) // per-command results/errors are set inside + for j := i; j < end; j++ { + backlog[j].complete() + } + i = end + // A transport failure that survived processPipeline's own retries means + // the server is unreachable: stop, and fail the remaining chunks with the + // same error instead of re-running the full retry cycle per chunk against + // a dead endpoint (Close would otherwise stall chunks × retries × backoff). + // Per-command Redis errors are normal results and do not stop the flush. + if err != nil && !isRedisError(err) { + fd.failReqs(backlog[i:], err) + return + } + } +} + +// failQueue fails every command currently buffered in fd.ch with err WITHOUT +// closing the engine (unlike takeQueue, the shutdown drain, which sets closed). +// Used on fdLeaseErr/fdDenied, where the carry goes through failReqs and this +// drains the accepted backlog — both halves emit the native error metric. The +// engine stays alive, so a command submitted after this returns is failed on the +// next denied attempt or served once the limiter admits. The channel receive is +// safe against a concurrent submit send, so no lock is taken here. +func (fd *fdEngine) failQueue(err error) { + errorCallback := pool.GetMetricErrorCallback() + var errorType, statusCode string + var isInternal bool + classified := false + for { + select { + case r := <-fd.ch: + r.cmd.SetErr(err) + if errorCallback != nil { + if !classified { + errorType, statusCode, isInternal = classifyCommandError(err) + classified = true + } + octx := r.ctx + if octx == nil { + octx = context.Background() + } + errorCallback(octx, errorType, nil, statusCode, isInternal, 0) + } + r.complete() + default: + return + } + } +} + +// flushBacklogForClose is the graceful-Close flush: it stops new submits (sets +// closed) and writes every command still buffered in fd.ch on the current +// connection, in the same MaxBatchSize/MaxBatchBytes chunks as normal writes, so +// ACCEPTED commands complete instead of failing ErrClosed. The caller then +// closeGraceful()s the deque so the reader drains these replies before exiting. +// Returns the first write error (the caller then degrades to the conn-error path). +func (fd *fdEngine) flushBacklogForClose(bg context.Context, cn *pool.Conn, inflight *fdInflight, readerDone <-chan struct{}) error { + fd.submitMu.Lock() + fd.closed = true + fd.submitMu.Unlock() + var backlog []fdReq + for { + select { + case r := <-fd.ch: + backlog = append(backlog, r) + default: + return fd.writeCarryChunked(bg, cn, inflight, backlog, readerDone) + } + } +} + +// sleepBackoff waits the retry backoff, interruptible by Close. +func (fd *fdEngine) sleepBackoff(attempt int) { + d := internal.RetryBackoff(attempt, fd.client.opt.MinRetryBackoff, fd.client.opt.MaxRetryBackoff) + if d <= 0 { + return + } + t := time.NewTimer(d) + defer t.Stop() + select { + case <-t.C: + case <-fd.ap.ctx.Done(): + } +} diff --git a/autopipeline_fullduplex_test.go b/autopipeline_fullduplex_test.go new file mode 100644 index 0000000000..9f598b4c9e --- /dev/null +++ b/autopipeline_fullduplex_test.go @@ -0,0 +1,2271 @@ +package redis + +import ( + "context" + "errors" + "fmt" + "io" + "net" + "runtime" + "strings" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/redis/go-redis/v9/internal/otel" + "github.com/redis/go-redis/v9/internal/pool" +) + +// fdCountHook counts Get/Put on the pipeline pool — used to prove the held +// full-duplex connection actually cycles through the pool's per-conn hooks on +// lease/return, which is the mechanism maintnotifications and streaming-creds +// re-auth rely on. +type fdCountHook struct{ gets, puts atomic.Int64 } + +func (h *fdCountHook) OnGet(_ context.Context, _ *pool.Conn, _ bool) (bool, error) { + h.gets.Add(1) + return true, nil +} + +func (h *fdCountHook) OnPut(_ context.Context, _ *pool.Conn) (bool, bool, error) { + h.puts.Add(1) + return true, false, nil +} +func (h *fdCountHook) OnRemove(_ context.Context, _ *pool.Conn, _ error) {} + +// TestFullDuplexReturnRunsPoolHooks verifies the held FD connection passes back +// through the pipeline pool's PoolHook Get/Put path on lease/return — that path +// is what gives per-conn hooks (maintnotifications, streaming-creds re-auth) a +// chance to run. +func TestFullDuplexReturnRunsPoolHooks(t *testing.T) { + ctx := context.Background() + + c := fdTestClient(":6379") + defer c.Close() + if err := c.Ping(ctx).Err(); err != nil { + t.Skipf("no redis: %v", err) + } + pp := c.getPipelinePool() + if pp == nil { + t.Fatal("no pipeline pool") + } + hook := &fdCountHook{} + pp.AddPoolHook(hook) // must be installed before the engine leases its conn + + // Fast idle-return so it fires within the test window; max-hold left at its + // default (5s) so it does not fire in this <2s test — the idle path is what + // we are proving. + ap, err := c.AsyncAutoPipelineWithOptions(&AutoPipelineOptions{ + FullDuplex: true, + FullDuplexIdleTimeout: 40 * time.Millisecond, + }) + if err != nil { + t.Fatalf("AsyncAutoPipeline: %v", err) + } + defer ap.Close() + if ap.fd == nil { + t.Fatal("full-duplex engine not active") + } + + // The engine leases lazily — on the first command, not at startup (an unused FD + // autopipeliner must stay idle rather than dial in the background, #3964). Submit + // one command to trigger the lease; after serving it the engine idle-returns the + // conn, so a Get and a Put are both observed by the pool hook. + if err := ap.Set(ctx, "fd:hook:k", "v", 0).Err(); err != nil { + t.Fatalf("set: %v", err) + } + for deadline := time.Now().Add(2 * time.Second); time.Now().Before(deadline); { + if hook.gets.Load() >= 1 && hook.puts.Load() >= 1 { + break + } + time.Sleep(10 * time.Millisecond) + } + if g, p := hook.gets.Load(), hook.puts.Load(); g < 1 || p < 1 { + t.Fatalf("FD conn did not cycle through pool hooks (gets=%d puts=%d) — maintnotif/re-auth hooks would never run", g, p) + } + + // And a command still works after the return (re-lease → another Get). + if err := ap.Set(ctx, "fd:hook:k2", "v", 0).Err(); err != nil { + t.Fatalf("post-return set: %v", err) + } + if v, err := ap.Get(ctx, "fd:hook:k2").Result(); err != nil || v != "v" { + t.Fatalf("post-return get: v=%q err=%v", v, err) + } +} + +func fdTestClient(addr string) *Client { + return NewClient(&Options{ + Addr: addr, + Protocol: 3, + PipelinePoolSize: 4, + PipelineReadBufferSize: 64 * 1024, + PipelineWriteBufferSize: 64 * 1024, + PoolSize: 4, + }) +} + +// TestFullDuplexStaysAlignedUnderConcurrentMutation verifies the ordered +// full-duplex reader keeps command↔reply FIFO alignment while a SECOND client +// mutates the key concurrently: every GET after the mutation returns the new +// value and never errors. It does NOT cover the reader's RESP3 push-drain — no +// invalidation push can reach the FD conn (pipeline-pool connections are excluded +// from CLIENT TRACKING), so push demux is covered by the maintnotifications e2e +// suite instead. +func TestFullDuplexStaysAlignedUnderConcurrentMutation(t *testing.T) { + ctx := context.Background() + addr := ":6379" + + c := fdTestClient(addr) + defer c.Close() + if err := c.Ping(ctx).Err(); err != nil { + t.Skipf("no redis at %s: %v", addr, err) + } + + ap, err := c.AsyncAutoPipelineWithOptions(&AutoPipelineOptions{FullDuplex: true}) + if err != nil { + t.Fatalf("AsyncAutoPipeline: %v", err) + } + defer ap.Close() + if ap.fd == nil { + t.Fatal("full-duplex engine not active (ap.fd is nil)") + } + + key := "fd:align:key" + if err := c.Set(ctx, key, "v0", 0).Err(); err != nil { + t.Fatalf("seed set: %v", err) + } + if v, err := ap.Get(ctx, key).Result(); err != nil || v != "v0" { + t.Fatalf("prime GET: v=%q err=%v", v, err) + } + + other := NewClient(&Options{Addr: addr}) + defer other.Close() + if err := other.Set(ctx, key, "v1", 0).Err(); err != nil { + t.Fatalf("concurrent set: %v", err) + } + + for i := 0; i < 100; i++ { + v, err := ap.Get(ctx, key).Result() + if err != nil { + t.Fatalf("GET %d after concurrent mutation: %v (FIFO misalignment?)", i, err) + } + if v != "v1" { + t.Fatalf("GET %d: got %q want %q (reply/command misaligned)", i, v, "v1") + } + } +} + +// TestFullDuplexOrderedManyGoroutines is a correctness/-race check: many +// concurrent goroutines each run a SET then GET of their own key through the +// ordered full-duplex stream and must read back exactly what they wrote +// (per-caller order + reply/command alignment hold). +func TestFullDuplexOrderedManyGoroutines(t *testing.T) { + ctx := context.Background() + c := fdTestClient(":6379") + defer c.Close() + if err := c.Ping(ctx).Err(); err != nil { + t.Skipf("no redis: %v", err) + } + ap, err := c.AsyncAutoPipelineWithOptions(&AutoPipelineOptions{FullDuplex: true}) + if err != nil { + t.Fatalf("AsyncAutoPipeline: %v", err) + } + defer ap.Close() + if ap.fd == nil { + t.Fatal("full-duplex engine not active") + } + + const workers, iters = 64, 200 + errCh := make(chan error, workers) + for w := 0; w < workers; w++ { + go func(w int) { + key := "fd:ord:" + itoa(w) + for i := 0; i < iters; i++ { + val := itoa(w) + ":" + itoa(i) + if err := ap.Set(ctx, key, val, 0).Err(); err != nil { + errCh <- err + return + } + got, err := ap.Get(ctx, key).Result() + if err != nil { + errCh <- err + return + } + if got != val { + errCh <- &fdOrderErr{w, i, val, got} + return + } + } + errCh <- nil + }(w) + } + for w := 0; w < workers; w++ { + if err := <-errCh; err != nil { + t.Fatal(err) + } + } +} + +// TestFullDuplexRecoversFromConnKill is the retry fault-injection test: many +// goroutines run continuous SET-then-GET of their own key through the ordered +// full-duplex stream while a second client repeatedly kills the connection +// (CLIENT KILL TYPE normal — SKIPME skips the killer). The engine must re-issue +// the unacked tail on a fresh connection: every worker keeps reading back +// exactly what it wrote (per-caller order + alignment survive the failure), and +// nothing hangs. +func TestFullDuplexRecoversFromConnKill(t *testing.T) { + ctx := context.Background() + c := fdTestClient(":6379") + defer c.Close() + if err := c.Ping(ctx).Err(); err != nil { + t.Skipf("no redis: %v", err) + } + ap, err := c.AsyncAutoPipelineWithOptions(&AutoPipelineOptions{FullDuplex: true}) + if err != nil { + t.Fatalf("AsyncAutoPipeline: %v", err) + } + defer ap.Close() + if ap.fd == nil { + t.Fatal("full-duplex engine not active") + } + + killer := NewClient(&Options{Addr: ":6379"}) + defer killer.Close() + if err := killer.Ping(ctx).Err(); err != nil { + t.Skipf("no redis for killer: %v", err) + } + + const workers = 16 + var stop atomic.Bool + done := make(chan error, workers) + for w := 0; w < workers; w++ { + go func(w int) { + key := "fd:kill:" + itoa(w) + for n := 0; !stop.Load(); n++ { + val := itoa(w) + ":" + itoa(n) + if err := ap.Set(ctx, key, val, 0).Err(); err != nil { + done <- fmt.Errorf("worker %d set #%d: %w", w, n, err) + return + } + got, err := ap.Get(ctx, key).Result() + if err != nil { + done <- fmt.Errorf("worker %d get #%d: %w", w, n, err) + return + } + if got != val { + done <- fmt.Errorf("worker %d get #%d: got %q want %q (misaligned across recovery)", w, n, got, val) + return + } + } + done <- nil + }(w) + } + + // Let load build, then kill the full-duplex connection a few times mid-stream. + time.Sleep(120 * time.Millisecond) + for i := 0; i < 3; i++ { + if err := killer.Do(ctx, "CLIENT", "KILL", "TYPE", "normal").Err(); err != nil { + t.Logf("CLIENT KILL #%d: %v", i, err) + } + time.Sleep(90 * time.Millisecond) + } + time.Sleep(120 * time.Millisecond) + stop.Store(true) + + deadline := time.After(15 * time.Second) + for w := 0; w < workers; w++ { + select { + case err := <-done: + if err != nil { + t.Fatal(err) + } + case <-deadline: + t.Fatal("timeout: a worker hung after a connection kill (retry deadlock / lost completion?)") + } + } +} + +// TestFullDuplexIdleReturnsConn verifies lease/return: after an idle gap the +// held connection goes back to the pipeline pool (so its per-conn hooks can +// run), and the next command re-leases it and still reads correctly. +func TestFullDuplexIdleReturnsConn(t *testing.T) { + ctx := context.Background() + + c := fdTestClient(":6379") + defer c.Close() + if err := c.Ping(ctx).Err(); err != nil { + t.Skipf("no redis: %v", err) + } + // Fast idle-return; max-hold pushed out so only the idle path fires here. + ap, err := c.AsyncAutoPipelineWithOptions(&AutoPipelineOptions{ + FullDuplex: true, + FullDuplexIdleTimeout: 60 * time.Millisecond, + FullDuplexMaxHold: 10 * time.Second, + }) + if err != nil { + t.Fatalf("AsyncAutoPipeline: %v", err) + } + defer ap.Close() + if ap.fd == nil { + t.Fatal("full-duplex engine not active") + } + pp := c.getPipelinePool() + if pp == nil { + t.Fatal("no pipeline pool") + } + + if err := ap.Set(ctx, "fd:idle:k", "v", 0).Err(); err != nil { + t.Fatalf("initial set: %v", err) + } + // Immediately after the command the engine still holds the conn (idle gap not + // yet elapsed) — it is checked out of the pool, so IdleLen is 0. This is the + // baseline that makes the return below meaningful (the assertion would not + // hold if lease/return were removed — the conn would stay held, never idle). + if got := pp.IdleLen(); got != 0 { + t.Fatalf("expected the FD conn held (IdleLen=0) right after a command, got %d", got) + } + + returned := false + for deadline := time.Now().Add(2 * time.Second); time.Now().Before(deadline); { + if pp.IdleLen() >= 1 { + returned = true + break + } + time.Sleep(10 * time.Millisecond) + } + if !returned { + t.Fatalf("pipeline conn not returned to the pool after idle (IdleLen=%d)", pp.IdleLen()) + } + + if v, err := ap.Get(ctx, "fd:idle:k").Result(); err != nil || v != "v" { + t.Fatalf("re-lease GET: v=%q err=%v", v, err) + } +} + +// TestFullDuplexMaxHoldRecycles forces periodic clean returns under continuous +// load (max-hold) and asserts correctness holds, nothing hangs, and the +// connection is recycled through the same small pool (no leak). +func TestFullDuplexMaxHoldRecycles(t *testing.T) { + ctx := context.Background() + + c := fdTestClient(":6379") + defer c.Close() + if err := c.Ping(ctx).Err(); err != nil { + t.Skipf("no redis: %v", err) + } + // Force frequent max-hold recycles; idle pushed out so only max-hold fires + // under the continuous load below. + ap, err := c.AsyncAutoPipelineWithOptions(&AutoPipelineOptions{ + FullDuplex: true, + FullDuplexMaxHold: 80 * time.Millisecond, + FullDuplexIdleTimeout: 10 * time.Second, + }) + if err != nil { + t.Fatalf("AsyncAutoPipeline: %v", err) + } + defer ap.Close() + if ap.fd == nil { + t.Fatal("full-duplex engine not active") + } + pp := c.getPipelinePool() + + const workers = 8 + var stop atomic.Bool + done := make(chan error, workers) + for w := 0; w < workers; w++ { + go func(w int) { + key := "fd:mh:" + itoa(w) + for n := 0; !stop.Load(); n++ { + val := itoa(w) + ":" + itoa(n) + if err := ap.Set(ctx, key, val, 0).Err(); err != nil { + done <- err + return + } + got, err := ap.Get(ctx, key).Result() + if err != nil { + done <- err + return + } + if got != val { + done <- fmt.Errorf("worker %d n%d: got %q want %q across recycle", w, n, got, val) + return + } + } + done <- nil + }(w) + } + time.Sleep(500 * time.Millisecond) // several 80ms max-hold recycles + if pp != nil { + if n := pp.Len(); n > workers { + t.Fatalf("pipeline pool grew under recycling (Len=%d) — conn leak?", n) + } + } + stop.Store(true) + + // Prove the max-hold path actually fired (else this test would be green over + // an inert feature). + if r := ap.fd.recycles.Load(); r == 0 { + t.Fatal("no max-hold recycles observed — the recycle path did not fire") + } + deadline := time.After(15 * time.Second) + for w := 0; w < workers; w++ { + select { + case err := <-done: + if err != nil { + t.Fatal(err) + } + case <-deadline: + t.Fatal("timeout: worker hung under max-hold recycling") + } + } +} + +func itoa(n int) string { + if n == 0 { + return "0" + } + var b [20]byte + i := len(b) + for n > 0 { + i-- + b[i] = byte('0' + n%10) + n /= 10 + } + return string(b[i:]) +} + +type fdOrderErr struct { + w, i int + want, got string +} + +func (e *fdOrderErr) Error() string { + return "per-caller order/alignment broken: worker=" + itoa(e.w) + " iter=" + itoa(e.i) + + " want=" + e.want + " got=" + e.got +} + +// TestFullDuplexConfigDefaults pins the zero-value resolution. A zero +// FullDuplexWindow MUST become the default, never 0: the writer's backpressure +// gate is `for inflight.len() >= window`, so window==0 would block the writer on +// the first submit (0 >= 0). This is a construction check — no server needed. +func TestFullDuplexConfigDefaults(t *testing.T) { + c := fdTestClient(":6379") + defer c.Close() + ap, err := c.AsyncAutoPipelineWithOptions(&AutoPipelineOptions{FullDuplex: true}) + if err != nil { + t.Fatalf("AsyncAutoPipeline: %v", err) + } + defer ap.Close() + if ap.fd == nil { + t.Fatal("full-duplex engine not active") + } + if ap.fd.window != fdDefaultWindow { + t.Fatalf("zero FullDuplexWindow resolved to %d, want default %d (window 0 deadlocks the writer)", ap.fd.window, fdDefaultWindow) + } + if ap.fd.idle != fdDefaultIdle { + t.Fatalf("zero FullDuplexIdleTimeout resolved to %s, want default %s", ap.fd.idle, fdDefaultIdle) + } + if ap.fd.maxHold != fdDefaultMaxHold { + t.Fatalf("zero FullDuplexMaxHold resolved to %s, want default %s", ap.fd.maxHold, fdDefaultMaxHold) + } + // The submit queue is capped (min(window, 4096)): a buffered channel + // allocates its full capacity eagerly, so a window-sized queue would cost + // several MiB per engine up front; backpressure comes from the in-flight + // deque, which grows only with actual in-flight. + if want := 4096; cap(ap.fd.ch) != want { + t.Fatalf("queue capacity %d, want %d (capped; window %d bounds in-flight, not the queue)", + cap(ap.fd.ch), want, fdDefaultWindow) + } + if ap.fd.window != fdDefaultWindow { + t.Fatalf("window %d, want default %d", ap.fd.window, fdDefaultWindow) + } +} + +// TestFullDuplexValidateRejects covers the config surface: Validate must reject +// the contradictory standalone combos and negative tunings. Pure — no server. +func TestFullDuplexValidateRejects(t *testing.T) { + bad := []struct { + name string + cfg AutoPipelineOptions + // wantMsg, when set, must appear in the error — proves the FD-specific + // branch fired rather than a generic check (the FD checks run first). + wantMsg string + }{ + {"unordered", AutoPipelineOptions{FullDuplex: true, Unordered: true}, "FullDuplex requires an ordered stream"}, + {"concurrent-batches", AutoPipelineOptions{FullDuplex: true, MaxConcurrentBatches: 2}, "FullDuplex requires MaxConcurrentBatches"}, + {"neg-window", AutoPipelineOptions{FullDuplex: true, FullDuplexWindow: -1}, "FullDuplexWindow"}, + {"neg-idle", AutoPipelineOptions{FullDuplex: true, FullDuplexIdleTimeout: -1}, "FullDuplexIdleTimeout"}, + {"neg-maxhold", AutoPipelineOptions{FullDuplex: true, FullDuplexMaxHold: -1}, "FullDuplexMaxHold"}, + } + for _, tc := range bad { + err := tc.cfg.Validate() + if err == nil { + t.Errorf("%s: Validate() = nil, want error", tc.name) + continue + } + if tc.wantMsg != "" && !strings.Contains(err.Error(), tc.wantMsg) { + t.Errorf("%s: Validate() = %q, want it to contain %q", tc.name, err, tc.wantMsg) + } + } + good := []struct { + name string + cfg AutoPipelineOptions + }{ + {"default", AutoPipelineOptions{FullDuplex: true}}, + {"ordered-1batch", AutoPipelineOptions{FullDuplex: true, MaxConcurrentBatches: 1}}, + {"tuned", AutoPipelineOptions{FullDuplex: true, FullDuplexWindow: 1024, FullDuplexIdleTimeout: time.Second, FullDuplexMaxHold: time.Second}}, + } + for _, tc := range good { + if err := tc.cfg.Validate(); err != nil { + t.Errorf("%s: Validate() = %v, want nil", tc.name, err) + } + } +} + +// fdProcessCounterHook counts ProcessHook / ProcessPipelineHook invocations and +// always calls next — the shape of an observability hook (redisotel). +type fdProcessCounterHook struct { + process atomic.Int64 + pipeline atomic.Int64 + sawNext atomic.Int64 +} + +func (h *fdProcessCounterHook) DialHook(next DialHook) DialHook { return next } +func (h *fdProcessCounterHook) ProcessHook(next ProcessHook) ProcessHook { + return func(ctx context.Context, cmd Cmder) error { + h.process.Add(1) + err := next(ctx, cmd) + h.sawNext.Add(1) + return err + } +} + +func (h *fdProcessCounterHook) ProcessPipelineHook(next ProcessPipelineHook) ProcessPipelineHook { + return func(ctx context.Context, cmds []Cmder) error { + h.pipeline.Add(1) + return next(ctx, cmds) + } +} + +// TestFullDuplexRunsProcessHooks proves per-command observability works on the FD +// path: with a ProcessHook registered, every FD-dispatched command runs through +// the hook chain (redisotel spans/metrics + custom hooks fire), next() is called, +// and results are still correct. Without this, the FD engine's raw conn I/O would +// bypass hooks entirely. +func TestFullDuplexRunsProcessHooks(t *testing.T) { + ctx := context.Background() + c := fdTestClient(":6379") + defer c.Close() + if err := c.Ping(ctx).Err(); err != nil { + t.Skipf("no redis: %v", err) + } + hook := &fdProcessCounterHook{} + c.AddHook(hook) + ap, err := c.AsyncAutoPipelineWithOptions(&AutoPipelineOptions{FullDuplex: true}) + if err != nil { + t.Fatalf("AsyncAutoPipeline: %v", err) + } + defer ap.Close() + if ap.fd == nil { + t.Fatal("full-duplex engine not active") + } + + base := hook.process.Load() + const n = 50 + for i := 0; i < n; i++ { + if err := ap.Set(ctx, "fdhook:"+itoa(i), itoa(i), 0).Err(); err != nil { + t.Fatalf("set %d: %v", i, err) + } + } + for i := 0; i < n; i++ { + if v, err := ap.Get(ctx, "fdhook:"+itoa(i)).Result(); err != nil || v != itoa(i) { + t.Fatalf("get %d: v=%q err=%v", i, v, err) + } + } + // 2n commands (n SET + n GET), each must have run the hook chain with next(). + if got := hook.process.Load() - base; got < 2*n { + t.Fatalf("ProcessHook fired %d times for %d FD commands — hooks not running on the FD path", got, 2*n) + } + if got := hook.sawNext.Load(); got < 2*n { + t.Fatalf("hook next() reached %d times, want >= %d — chain not completing on FD", got, 2*n) + } +} + +// fdShortCircuitHook returns err WITHOUT calling next for one command name — a +// fail-fast / cache / circuit-breaker style hook. Selective (only the target +// command) so it does not also short-circuit the connection handshake. +type fdShortCircuitHook struct { + name string + err error + calls atomic.Int64 +} + +func (h *fdShortCircuitHook) DialHook(next DialHook) DialHook { return next } +func (h *fdShortCircuitHook) ProcessHook(next ProcessHook) ProcessHook { + return func(ctx context.Context, cmd Cmder) error { + if cmd.Name() == h.name { + h.calls.Add(1) + return h.err // short-circuit: next is never called + } + return next(ctx, cmd) + } +} + +func (h *fdShortCircuitHook) ProcessPipelineHook(next ProcessPipelineHook) ProcessPipelineHook { + return next +} + +// TestFullDuplexHookShortCircuit exercises the short-circuit path, where host and +// reader both finalize the command: it must be race-free (-race), the caller must +// see the hook's error, and the command must still execute on the wire (FD cannot +// un-send an already-queued command). Guards the race where the host releases the +// caller before the reader has finished writing into the command. +func TestFullDuplexHookShortCircuit(t *testing.T) { + ctx := context.Background() + c := fdTestClient(":6379") + defer c.Close() + if err := c.Ping(ctx).Err(); err != nil { + t.Skipf("no redis: %v", err) + } + // Separate hook-free client to verify the writes actually reached the server. + verify := NewClient(&Options{Addr: ":6379"}) + defer verify.Close() + + sentinel := errors.New("short-circuited by hook") + hook := &fdShortCircuitHook{name: "set", err: sentinel} + c.AddHook(hook) + ap, err := c.AsyncAutoPipelineWithOptions(&AutoPipelineOptions{FullDuplex: true}) + if err != nil { + t.Fatalf("AsyncAutoPipeline: %v", err) + } + defer ap.Close() + if ap.fd == nil { + t.Fatal("full-duplex engine not active") + } + + const n = 30 + for i := 0; i < n; i++ { + if err := ap.Set(ctx, "fdsc:"+itoa(i), itoa(i), 0).Err(); !errors.Is(err, sentinel) { + t.Fatalf("set %d: err=%v, want the short-circuit sentinel", i, err) + } + } + if hook.calls.Load() < n { + t.Fatalf("short-circuit hook fired %d times, want >= %d", hook.calls.Load(), n) + } + // The commands still ran on the wire despite the short-circuit — verified from + // a hook-free client. This also proves the reader stayed aligned (a desync + // would have corrupted or errored these writes). + for i := 0; i < n; i++ { + if v, err := verify.Get(ctx, "fdsc:"+itoa(i)).Result(); err != nil || v != itoa(i) { + t.Fatalf("verify get %d: v=%q err=%v — command did not execute or stream desynced", i, v, err) + } + } +} + +// delayReplyProxy is a tiny TCP proxy that forwards client->server immediately +// but delays every server->client chunk by `delay`, simulating reply latency so +// the FD reader lags the writer. Used to force backpressure without touching the +// server (DEBUG SLEEP is often disabled). +func delayReplyProxy(t *testing.T, backend string, delay time.Duration) (addr string, stop func()) { + t.Helper() + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("proxy listen: %v", err) + } + var mu sync.Mutex + var conns []net.Conn // active endpoints, closed by stop() to kill live sessions + track := func(c net.Conn) { + mu.Lock() + conns = append(conns, c) + mu.Unlock() + } + go func() { + for { + client, err := ln.Accept() + if err != nil { + return + } + go func(client net.Conn) { + srv, err := net.Dial("tcp", backend) + if err != nil { + client.Close() + return + } + track(client) + track(srv) + go io.Copy(srv, client) // client -> server: immediate + buf := make([]byte, 64*1024) + for { + n, rerr := srv.Read(buf) + if n > 0 { + b := append([]byte(nil), buf[:n]...) + time.Sleep(delay) // server -> client: delayed + if _, werr := client.Write(b); werr != nil { + break + } + } + if rerr != nil { + break + } + } + srv.Close() + client.Close() + }(client) + } + }() + return ln.Addr().String(), func() { + ln.Close() + mu.Lock() + for _, c := range conns { + c.Close() // kill live sessions so in-flight replies never arrive + } + mu.Unlock() + } +} + +// TestFullDuplexBackpressure proves the bounded in-flight window actually applies +// backpressure: with reply latency injected so the reader lags, a caller +// submitting far more than the window BLOCKS once the channel + in-flight are +// full — outstanding stays bounded, memory does not grow without limit — and once +// the reads catch up everything drains with the correct values (no drops/reorders +// under backpressure). +func TestFullDuplexBackpressure(t *testing.T) { + ctx := context.Background() + if err := NewClient(&Options{Addr: ":6379"}).Ping(ctx).Err(); err != nil { + t.Skipf("no redis: %v", err) + } + const delay = 25 * time.Millisecond + paddr, stop := delayReplyProxy(t, "127.0.0.1:6379", delay) + defer stop() + + c := fdTestClient(paddr) + defer c.Close() + + const window, maxBatch = 32, 4 + ap, err := c.AsyncAutoPipelineWithOptions(&AutoPipelineOptions{ + FullDuplex: true, + FullDuplexWindow: window, + MaxBatchSize: maxBatch, + }) + if err != nil { + t.Fatalf("AsyncAutoPipeline: %v", err) + } + defer ap.Close() + if ap.fd == nil { + t.Fatal("full-duplex engine not active") + } + // Prime the session so the engine has leased a conn + a live in-flight deque. + if err := ap.Set(ctx, "bp:prime", "x", 0).Err(); err != nil { + t.Fatalf("prime: %v", err) + } + + const N = 500 + key := func(i int) string { return fmt.Sprintf("bp:%d", i) } + val := func(i int) string { return fmt.Sprintf("v%d", i) } + + // Burst N submits. Reply latency (delay) means the reader lags, so in-flight + // fills to the window, the writer blocks, the channel fills, and ap.Set blocks + // once ~channel+in-flight are outstanding. + var accepted atomic.Int64 + cmds := make([]*StatusCmd, N) + submittedAll := make(chan struct{}) + go func() { + for i := 0; i < N; i++ { + cmds[i] = ap.Set(ctx, key(i), val(i), 0) // blocks here once full = backpressure + accepted.Add(1) + } + close(submittedAll) + }() + + // The burst saturates in microseconds; replies do not start returning for + // `delay`. Sample well within that window: outstanding must be bounded, not N. + time.Sleep(delay / 3) + acc := accepted.Load() + peak := ap.fd.curInflight.Load().peakLen() + if acc >= N { + t.Fatalf("no backpressure: accepted %d of %d before any reply returned (unbounded submit)", acc, N) + } + if acc > 4*window { + t.Fatalf("outstanding %d exceeds the expected bound (~channel %d + in-flight %d+%d); backpressure too loose", acc, window, window, maxBatch) + } + if peak > window+maxBatch { + t.Fatalf("in-flight peak %d exceeded window+maxBatch=%d — deque not bounded", peak, window+maxBatch) + } + t.Logf("under reply latency: accepted=%d/%d (bounded to ~channel+in-flight), in-flight peak=%d (<= %d)", acc, N, peak, window+maxBatch) + + // Reads catch up: the submitter finishes and every command lands. + select { + case <-submittedAll: + case <-time.After(15 * time.Second): + t.Fatalf("submitter did not drain after backpressure (accepted %d/%d)", accepted.Load(), N) + } + for i := 0; i < N; i++ { + if err := cmds[i].Err(); err != nil { + t.Fatalf("cmd %d failed after backpressure: %v", i, err) + } + } + // Correctness after backpressure: sampled keys hold the values we set. + for _, i := range []int{0, 1, N / 3, N / 2, N - 2, N - 1} { + if got, err := ap.Get(ctx, key(i)).Result(); err != nil || got != val(i) { + t.Fatalf("post-backpressure GET %s: got=%q err=%v want=%q", key(i), got, err, val(i)) + } + } + // Final peak sanity over the whole run. + if peak := ap.fd.curInflight.Load().peakLen(); peak > window+maxBatch { + t.Fatalf("final in-flight peak %d exceeded window+maxBatch=%d", peak, window+maxBatch) + } +} + +// TestFDShutdownFlushCompletesBetweenSessionsBacklog pins the Close contract for +// work accepted while NO session holds a connection: a command sitting in fd.ch +// (or an unacked carry) when Close wins the between-sessions race must be +// EXECUTED via the normal pipeline path, not failed ErrClosed. Drives +// shutdownFlush directly, which is what run()'s two shutdown sites call. +func TestFDShutdownFlushCompletesBetweenSessionsBacklog(t *testing.T) { + ctx := context.Background() + c := fdTestClient(":6379") + defer c.Close() + if err := c.Ping(ctx).Err(); err != nil { + t.Skipf("no redis: %v", err) + } + ap, err := c.AsyncAutoPipelineWithOptions(&AutoPipelineOptions{FullDuplex: true}) + if err != nil { + t.Fatalf("AsyncAutoPipeline: %v", err) + } + defer ap.Close() + fd := ap.fd + if fd == nil { + t.Fatal("full-duplex engine not active") + } + + // Simulate the between-sessions Close: backlog queued in fd.ch (bypassing + // submit — run() must not consume it, so this test does not race the engine's + // own loop; the queue is drained below by takeQueue inside shutdownFlush). + const n = 5 + reqs := make([]fdReq, n) + for i := 0; i < n; i++ { + cmd := NewStatusCmd(ctx, "set", fmt.Sprintf("fdsf:%d", i), fmt.Sprintf("v%d", i)) + reqs[i] = fdReq{cmd: cmd, batch: newAPBatch()} + fd.ch <- reqs[i] + } + // carry: an unacked tail from a failed session that was never re-leased. + carryCmd := NewStatusCmd(ctx, "set", "fdsf:carry", "vc") + carry := []fdReq{{cmd: carryCmd, batch: newAPBatch()}} + + fd.shutdownFlush(context.Background(), carry) + + // Every batch settles and every command EXECUTED (not ErrClosed). + for i := 0; i < n; i++ { + select { + case <-reqs[i].batch.done: + case <-time.After(5 * time.Second): + t.Fatalf("backlog cmd %d never completed after shutdownFlush", i) + } + if err := reqs[i].cmd.rawErr(); err != nil { + t.Fatalf("backlog cmd %d err = %v, want executed (nil)", i, err) + } + } + if err := carryCmd.rawErr(); err != nil { + t.Fatalf("carry cmd err = %v, want executed (nil)", err) + } + verify := NewClient(&Options{Addr: ":6379"}) + defer verify.Close() + for i := 0; i < n; i++ { + if v, err := verify.Get(ctx, fmt.Sprintf("fdsf:%d", i)).Result(); err != nil || v != fmt.Sprintf("v%d", i) { + t.Fatalf("fdsf:%d = %q, %v — accepted command was not executed on Close", i, v, err) + } + } + if v, err := verify.Get(ctx, "fdsf:carry").Result(); err != nil || v != "vc" { + t.Fatalf("fdsf:carry = %q, %v — carry was not executed on Close", v, err) + } + for i := 0; i < n; i++ { + verify.Del(ctx, fmt.Sprintf("fdsf:%d", i)) + } + verify.Del(ctx, "fdsf:carry") +} + +// failWriteNetConn is a net.Conn whose Write always fails, so a buffered flush +// (the end of WithWriter) returns an error while the command bytes were already +// buffered — used to drive a write failure in writeCarryChunked deterministically. +type failWriteNetConn struct{ mockNetConn } + +func (c *failWriteNetConn) Write(b []byte) (int, error) { + return 0, errors.New("write boom") +} + +// TestWriteCarryChunkedRecoversSuffixOnWriteError pins deque recovery of a +// partially written carry: writeBatch pushes each chunk into the in-flight deque +// BEFORE writing it, so when a multi-chunk carry write fails the un-written +// suffix must be pushed too — otherwise those accepted commands are in neither +// fd.ch nor the deque and their callers hang. Deterministic and dial-free. +func TestWriteCarryChunkedRecoversSuffixOnWriteError(t *testing.T) { + cn := pool.NewConn(&failWriteNetConn{}) + fd := &fdEngine{ + ap: &AutoPipeliner{config: &AutoPipelineOptions{}}, // MaxBatchBytes 0 = disabled + client: &Client{baseClient: &baseClient{opt: &Options{WriteTimeout: time.Second}}}, + maxBatch: 2, // 5 commands -> chunks [0:2] [2:4] [4:5] + } + inflight := newFDInflight() + + const n = 5 + carry := make([]fdReq, n) + for i := range carry { + carry[i] = fdReq{cmd: NewStatusCmd(context.Background(), "set", fmt.Sprintf("k%d", i), "v")} + } + + if err := fd.writeCarryChunked(context.Background(), cn, inflight, carry, nil); err == nil { + t.Fatal("writeCarryChunked returned nil error despite a failing write") + } + // The failing chunk was pushed by writeBatch; the suffix must be pushed too, so + // the whole carry is recoverable by takeRemaining (without the fix only the + // first chunk is present and the rest are lost). + if got := inflight.len(); got != n { + t.Fatalf("in-flight deque holds %d of %d carry commands after a write error — the unwritten suffix was dropped (callers would hang)", got, n) + } +} + +// TestFullDuplexCloseCompletesBacklogAfterConnKill is a broad bounded-hang guard: +// killing the connection mid-flight and then closing the engine must settle every +// accepted command (via the connection-error recovery + drainQueue path) rather +// than hang a caller. End-to-end only; the chunked-carry suffix case is pinned +// deterministically by TestWriteCarryChunkedRecoversSuffixOnWriteError. +func TestFullDuplexCloseCompletesBacklogAfterConnKill(t *testing.T) { + ctx := context.Background() + if err := NewClient(&Options{Addr: ":6379"}).Ping(ctx).Err(); err != nil { + t.Skipf("no redis: %v", err) + } + const delay = 40 * time.Millisecond + paddr, stop := delayReplyProxy(t, "127.0.0.1:6379", delay) + defer stop() + + c := fdTestClient(paddr) + defer c.Close() + const window, maxBatch = 8, 4 // small so the backlog spans multiple write chunks + ap, err := c.AsyncAutoPipelineWithOptions(&AutoPipelineOptions{ + FullDuplex: true, + FullDuplexWindow: window, + MaxBatchSize: maxBatch, + }) + if err != nil { + t.Fatalf("AsyncAutoPipeline: %v", err) + } + if ap.fd == nil { + t.Fatal("full-duplex engine not active") + } + + // Burst far more than the window so a backlog builds behind the lagging reader + // (in fd.ch and the in-flight deque). Submit off-goroutine: backpressure blocks + // ap.Set once full, and Close (ap.ctx cancel) releases it. + const N = 200 + cmds := make([]*StatusCmd, N) + var wg sync.WaitGroup + wg.Add(1) + go func() { + defer wg.Done() + for i := 0; i < N; i++ { + cmds[i] = ap.Set(ctx, fmt.Sprintf("fdkill:%d", i), "v", 0) + } + }() + + // Let a backlog accumulate, then kill the connection mid-flight and close the + // engine: the Close flush hits a dead connection with a multi-chunk backlog. + time.Sleep(delay / 2) + stop() // kill live conns so the next write/read fails + + closed := make(chan struct{}) + go func() { _ = ap.Close(); close(closed) }() + select { + case <-closed: + case <-time.After(10 * time.Second): + t.Fatal("ap.Close() hung after conn kill — the Close flush blocked the reader on replies that never arrive") + } + wg.Wait() // every ap.Set returned (accepted, or released by Close) + + // Every command's future MUST settle — a dropped/unrecovered command would + // block forever here. Bound it so a regression fails loudly instead of + // deadlocking the whole test binary. Values or errors are both fine; only a + // hang is a failure. + done := make(chan struct{}) + go func() { + for i := 0; i < N; i++ { + if cmds[i] != nil { + _ = cmds[i].Err() + } + } + close(done) + }() + select { + case <-done: + case <-time.After(10 * time.Second): + t.Fatal("some accepted commands never completed after conn kill + Close — chunked backlog/carry dropped (callers would hang)") + } +} + +// TestFullDuplexBlockingFace pins full-duplex on the BLOCKING AutoPipeline face: +// the engine activates, a single caller gets normal synchronous semantics (result +// already on the returned cmd, errors surface on the call), per-goroutine +// ordering holds by construction (each call waits), many concurrent blocking +// callers all complete correctly, and Close is clean. +func TestFullDuplexBlockingFace(t *testing.T) { + ctx := context.Background() + c := fdTestClient(":6379") + defer c.Close() + if err := c.Ping(ctx).Err(); err != nil { + t.Skipf("no redis: %v", err) + } + ap, err := c.AutoPipelineWithOptions(&AutoPipelineOptions{FullDuplex: true}) + if err != nil { + t.Fatalf("AutoPipelineWithOptions: %v", err) + } + defer ap.Close() + if ap.fd == nil { + t.Fatal("full-duplex engine not active on the blocking face") + } + if !ap.blocking { + t.Fatal("expected the blocking face") + } + + // Single caller: the call itself blocks until executed — the returned cmd + // already holds its result, no accessor gating involved. + if err := ap.Set(ctx, "fdblk:k", "v1", 0).Err(); err != nil { + t.Fatalf("set: %v", err) + } + if v, err := ap.Get(ctx, "fdblk:k").Result(); err != nil || v != "v1" { + t.Fatalf("get = %q, %v; want v1 (synchronous read-your-write)", v, err) + } + + // A per-command Redis error surfaces on the call, synchronously. + if err := ap.LPush(ctx, "fdblk:k", "x").Err(); err == nil || + !strings.Contains(err.Error(), "WRONGTYPE") { + t.Fatalf("LPush on a string = %v; want WRONGTYPE surfaced on the blocking call", err) + } + // And the stream is not desynced by it. + if v, err := ap.Get(ctx, "fdblk:k").Result(); err != nil || v != "v1" { + t.Fatalf("get after WRONGTYPE = %q, %v; want v1", v, err) + } + + // Per-goroutine ordering by construction: INCR sequence observed in order. + ap.Del(ctx, "fdblk:ctr") + for i := 1; i <= 20; i++ { + n, err := ap.Incr(ctx, "fdblk:ctr").Result() + if err != nil || n != int64(i) { + t.Fatalf("incr %d = %d, %v; want %d (per-goroutine order)", i, n, err, i) + } + } + + // Many concurrent blocking callers: all complete with their own results. + const G, M = 16, 25 + var wg sync.WaitGroup + errs := make(chan error, G) + for g := 0; g < G; g++ { + wg.Add(1) + go func(g int) { + defer wg.Done() + for i := 0; i < M; i++ { + key := fmt.Sprintf("fdblk:g%d:%d", g, i) + if err := ap.Set(ctx, key, key, 0).Err(); err != nil { + errs <- fmt.Errorf("g%d set %d: %w", g, i, err) + return + } + if v, err := ap.Get(ctx, key).Result(); err != nil || v != key { + errs <- fmt.Errorf("g%d get %d = %q, %v", g, i, v, err) + return + } + } + }(g) + } + wg.Wait() + select { + case err := <-errs: + t.Fatal(err) + default: + } + // Cleanup. + for g := 0; g < G; g++ { + for i := 0; i < M; i++ { + ap.Del(ctx, fmt.Sprintf("fdblk:g%d:%d", g, i)) + } + } + ap.Del(ctx, "fdblk:k", "fdblk:ctr") +} + +// TestFullDuplexMidStreamRedisError proves a per-command Redis error (WRONGTYPE) +// is delivered to THAT command and does NOT desync the stream: valid commands +// interleaved before and after it still get their own correct replies. +func TestFullDuplexMidStreamRedisError(t *testing.T) { + ctx := context.Background() + c := fdTestClient(":6379") + defer c.Close() + if err := c.Ping(ctx).Err(); err != nil { + t.Skipf("no redis: %v", err) + } + ap, err := c.AsyncAutoPipelineWithOptions(&AutoPipelineOptions{FullDuplex: true}) + if err != nil { + t.Fatalf("AsyncAutoPipeline: %v", err) + } + defer ap.Close() + if ap.fd == nil { + t.Fatal("full-duplex engine not active") + } + + const K = 64 + key := func(i int) string { return fmt.Sprintf("mse:%d", i) } + val := func(i int) string { return fmt.Sprintf("mv%d", i) } + for i := 0; i < K; i++ { + if err := ap.Set(ctx, key(i), val(i), 0).Err(); err != nil { + t.Fatalf("prewrite %d: %v", i, err) + } + } + // A string key that LPUSH will reject with WRONGTYPE. + if err := ap.Set(ctx, "mse:str", "s", 0).Err(); err != nil { + t.Fatalf("prewrite str: %v", err) + } + + // Interleave, all in flight: GET (valid) then LPUSH on the string key (errors). + gets := make([]*StringCmd, K) + errs := make([]*IntCmd, K) + for i := 0; i < K; i++ { + gets[i] = ap.Get(ctx, key(i)) + errs[i] = ap.LPush(ctx, "mse:str", "x") // WRONGTYPE, mid-stream + } + // The erroring commands must all carry a Redis error (not a conn teardown)... + for i := 0; i < K; i++ { + e := errs[i].Err() + if e == nil { + t.Fatalf("LPush %d on a string key returned no error (expected WRONGTYPE)", i) + } + if !isRedisError(e) { + t.Fatalf("LPush %d error is not a Redis error (stream torn down?): %v", i, e) + } + } + // ...and every interleaved GET must still return its own correct value. + for i := 0; i < K; i++ { + got, e := gets[i].Result() + if e != nil || got != val(i) { + t.Fatalf("GET %s after mid-stream error: got=%q err=%v want=%q (stream desynced by the error?)", key(i), got, e, val(i)) + } + } +} + +// TestFullDuplexCloseWhileBackpressured proves Close unblocks a caller that is +// blocked on a full window (backpressure): the submitter must return promptly +// (ctx.Done bail in submit), not hang, and pending commands fail rather than +// wedge. +func TestFullDuplexCloseWhileBackpressured(t *testing.T) { + ctx := context.Background() + if err := NewClient(&Options{Addr: ":6379"}).Ping(ctx).Err(); err != nil { + t.Skipf("no redis: %v", err) + } + const delay = 30 * time.Millisecond + paddr, stop := delayReplyProxy(t, "127.0.0.1:6379", delay) + defer stop() + + c := fdTestClient(paddr) + defer c.Close() + ap, err := c.AsyncAutoPipelineWithOptions(&AutoPipelineOptions{ + FullDuplex: true, FullDuplexWindow: 32, MaxBatchSize: 4, + }) + if err != nil { + t.Fatalf("AsyncAutoPipeline: %v", err) + } + if ap.fd == nil { + t.Fatal("full-duplex engine not active") + } + if err := ap.Set(ctx, "bpc:prime", "x", 0).Err(); err != nil { + t.Fatalf("prime: %v", err) + } + + const N = 5000 + done := make(chan struct{}) + go func() { + for i := 0; i < N; i++ { + ap.Set(ctx, fmt.Sprintf("bpc:%d", i), "v", 0) // blocks once the window is full + } + close(done) + }() + time.Sleep(delay / 2) // submitter now blocked on backpressure + + select { + case <-done: + t.Fatal("submitter finished before Close — backpressure did not engage") + default: + } + + if err := ap.Close(); err != nil { + t.Fatalf("Close while backpressured: %v", err) + } + select { + case <-done: + case <-time.After(10 * time.Second): + t.Fatal("submitter did not unblock after Close — Close did not release the backpressure wait") + } +} + +// TestFullDuplexBlockingCmdDetection proves the divert predicate catches blocking +// commands — in particular a RAW XREAD whose BLOCK token is a []byte (the form a +// plain string type switch would miss, letting it ride the shared pipe). Pure. +func TestFullDuplexBlockingCmdDetection(t *testing.T) { + ctx := context.Background() + if raw := NewCmd(ctx, "xread", []byte("BLOCK"), int64(0), "streams", "s", "$"); !isBlockingCmd(raw) { + t.Fatal("raw XREAD BLOCK ([]byte token) not detected as blocking — would ride the FD pipe") + } + if raw := NewCmd(ctx, "xreadgroup", "group", "g", "c", []byte("BLOCK"), int64(0), "streams", "s", ">"); !isBlockingCmd(raw) { + t.Fatal("raw XREADGROUP BLOCK not detected as blocking") + } + if nb := NewCmd(ctx, "xread", "streams", "s", "$"); isBlockingCmd(nb) { + t.Fatal("non-blocking XREAD wrongly diverted — loses batching for nothing") + } + if !isBlockingCmd(NewCmd(ctx, "blpop", "k", 0)) { + t.Fatal("BLPOP not detected as blocking") + } +} + +// TestFullDuplexBlockingDivertsOffPipe proves a parked blocking command does NOT +// head-of-line-block the shared FD pipe: it is diverted to a separate pooled +// connection, so pipelined commands submitted while it is parked complete +// promptly. Ordering across the divert boundary is deliberately not asserted. +func TestFullDuplexBlockingDivertsOffPipe(t *testing.T) { + ctx := context.Background() + c := fdTestClient(":6379") + defer c.Close() + if err := c.Ping(ctx).Err(); err != nil { + t.Skipf("no redis: %v", err) + } + ap, err := c.AsyncAutoPipelineWithOptions(&AutoPipelineOptions{FullDuplex: true}) + if err != nil { + t.Fatalf("AsyncAutoPipeline: %v", err) + } + defer ap.Close() + if ap.fd == nil { + t.Fatal("full-duplex engine not active") + } + + c.Del(ctx, "fd:block:list") + // Park a BLPOP (empty list → blocks up to 3s) on its own goroutine. + blDone := make(chan error, 1) + go func() { _, e := ap.BLPop(ctx, 3*time.Second, "fd:block:list").Result(); blDone <- e }() + time.Sleep(100 * time.Millisecond) // let the BLPOP be issued and park + + // While BLPOP is parked, 100 SET+GET round-trips on the SAME ap must complete + // well under the 3s block — if they were queued behind BLPOP on one pipe this + // would take ~3s. + start := time.Now() + for i := 0; i < 100; i++ { + if err := ap.Set(ctx, "fd:block:k"+itoa(i), itoa(i), 0).Err(); err != nil { + t.Fatalf("set %d during parked BLPOP: %v", i, err) + } + } + for i := 0; i < 100; i++ { + if v, err := ap.Get(ctx, "fd:block:k"+itoa(i)).Result(); err != nil || v != itoa(i) { + t.Fatalf("get %d during parked BLPOP: v=%q err=%v", i, v, err) + } + } + if elapsed := time.Since(start); elapsed > 2*time.Second { + t.Fatalf("200 pipelined ops took %s while BLPOP parked — head-of-line blocked behind the blocking command", elapsed) + } + select { + case e := <-blDone: + t.Fatalf("BLPOP returned early (%v); expected it parked while the pipe served other work", e) + default: // still parked, as expected + } + + // Release the BLPOP and let its goroutine exit cleanly. + c.LPush(ctx, "fd:block:list", "x") + select { + case <-blDone: + case <-time.After(2 * time.Second): + t.Fatal("BLPOP did not return after LPush — divert path stuck") + } +} + +// TestFullDuplexContextCancelStaysAligned proves that abandoning a command's +// wait (WaitContext with a cancelled context) mid-stream does NOT desync the +// reader: the reader still drains that command's reply in FIFO order, so every +// following command matches its own reply. Half the SET waits are abandoned; the +// SETs must still have executed correctly, proven by reading them all back. +func TestFullDuplexContextCancelStaysAligned(t *testing.T) { + ctx := context.Background() + c := fdTestClient(":6379") + defer c.Close() + if err := c.Ping(ctx).Err(); err != nil { + t.Skipf("no redis: %v", err) + } + ap, err := c.AsyncAutoPipelineWithOptions(&AutoPipelineOptions{FullDuplex: true}) + if err != nil { + t.Fatalf("AsyncAutoPipeline: %v", err) + } + defer ap.Close() + if ap.fd == nil { + t.Fatal("full-duplex engine not active") + } + + const n = 200 + cancelled, cancel := context.WithCancel(ctx) + cancel() // already-done context: WaitContext returns immediately + futures := make([]AutoFuture, n) + for i := 0; i < n; i++ { + futures[i] = ap.Submit(ctx, NewStatusCmd(ctx, "set", "fdctx:"+itoa(i), itoa(i))) + } + // Abandon every other wait with a cancelled context; the reader must still + // drain those replies so the stream stays aligned. + for i := 0; i < n; i += 2 { + _ = futures[i].WaitContext(cancelled) // may return ctx.Err() or the result — we don't care + } + // Read them all back: if any abandoned reply had been skipped, the FIFO would + // be off by one and these values would be wrong or errored. + for i := 0; i < n; i++ { + if v, err := ap.Get(ctx, "fdctx:"+itoa(i)).Result(); err != nil || v != itoa(i) { + t.Fatalf("get %d after abandoned waits: v=%q err=%v (stream desynced?)", i, v, err) + } + } +} + +// TestFullDuplexNoGoroutineLeakOnClose opens and closes the FD engine repeatedly +// (with in-flight, never-waited work at Close time) and asserts the writer/reader +// goroutines are reaped each time — no accumulation across cycles. +func TestFullDuplexNoGoroutineLeakOnClose(t *testing.T) { + ctx := context.Background() + c := fdTestClient(":6379") + defer c.Close() + if err := c.Ping(ctx).Err(); err != nil { + t.Skipf("no redis: %v", err) + } + // With a hook installed, every submitted command also spawns a host goroutine + // (hostHook) — so this asserts those are reaped too, not just writer/reader. + c.AddHook(&fdProcessCounterHook{}) + // Warm one cycle so any one-time client goroutines exist before the baseline. + if ap, err := c.AsyncAutoPipelineWithOptions(&AutoPipelineOptions{FullDuplex: true}); err == nil { + ap.Set(ctx, "fdleak:warm", "1", 0).Err() + ap.Close() + } + time.Sleep(100 * time.Millisecond) + base := runtime.NumGoroutine() + + for iter := 0; iter < 5; iter++ { + ap, err := c.AsyncAutoPipelineWithOptions(&AutoPipelineOptions{FullDuplex: true}) + if err != nil { + t.Fatalf("AsyncAutoPipeline: %v", err) + } + if ap.fd == nil { + t.Fatal("full-duplex engine not active") + } + // Fire in-flight work and Close WITHOUT waiting — exercises Close-mid-flight. + for i := 0; i < 50; i++ { + _ = ap.Set(ctx, "fdleak:"+itoa(i), itoa(i), 0) + } + if err := ap.Close(); err != nil { + t.Fatalf("Close iter %d: %v", iter, err) + } + } + + var now int + for deadline := time.Now().Add(3 * time.Second); time.Now().Before(deadline); { + now = runtime.NumGoroutine() + if now <= base+2 { + break + } + time.Sleep(20 * time.Millisecond) + } + if now > base+4 { + t.Fatalf("goroutine leak after 5 FD open/close cycles: base=%d now=%d (writer/reader not reaped)", base, now) + } +} + +// TestFDFailReqsNoDeadlock covers the failReqs self-deadlock: failReqs runs on +// the engine goroutine and must finalize each command WITHOUT awaiting its batch +// (cmd.Err() blocks on the very done channel failReqs is about to close). Pure, +// no server. +func TestFDFailReqsNoDeadlock(t *testing.T) { + ctx := context.Background() + cmd := NewStatusCmd(ctx, "set", "k", "v") + b := newAPBatch() + cmd.setReady(b) // now cmd.Err()/await() would block until b closes + + fd := &fdEngine{} + done := make(chan struct{}) + go func() { + fd.failReqs([]fdReq{{cmd: cmd, batch: b}}, ErrClosed) + close(done) + }() + + select { + case <-done: + case <-time.After(2 * time.Second): + t.Fatal("failReqs deadlocked: it awaited the batch.done it is responsible for closing") + } + if err := cmd.rawErr(); !errors.Is(err, ErrClosed) { + t.Fatalf("cmd err = %v, want ErrClosed", err) + } + select { + case <-b.done: + default: + t.Fatal("failReqs did not close the batch") + } +} + +// TestFDInflightHardCloseTakesUnackedTail pins deque ownership: an entry the +// reader has completed+advanced must NEVER also be returned by the recovery path. +// hardClose stops the reader; takeRemaining (called after the reader would have +// exited) returns exactly the un-advanced tail, in order, so completed commands +// are never replayed or failed a second time. Pure, no server. +func TestFDInflightHardCloseTakesUnackedTail(t *testing.T) { + ctx := context.Background() + f := newFDInflight() + all := make([]fdReq, 10) + for i := range all { + all[i] = fdReq{cmd: NewStatusCmd(ctx, "set", itoa(i), "v"), batch: newAPBatch()} + } + f.pushBatch(all) + + // Reader snapshots the front and completes+advances the first 4. + buf, ok := f.frontBatch(nil) + if !ok || len(buf) != 10 { + t.Fatalf("frontBatch ok=%v n=%d, want ok=true n=10", ok, len(buf)) + } + f.advance(4) + + // Connection-error recovery: stop the reader, then take the tail. + f.hardClose() + if _, ok := f.frontBatch(nil); ok { + t.Fatal("frontBatch returned ok after hardClose — the reader would not exit") + } + rem := f.takeRemaining() + if len(rem) != 6 { + t.Fatalf("takeRemaining n=%d, want 6 (advanced entries must not reappear)", len(rem)) + } + for i := 0; i < 6; i++ { + if rem[i].cmd != all[4+i].cmd { + t.Fatalf("tail[%d] mismatch — recovery order/ownership broken", i) + } + } + // Taking again yields nothing (queue cleared): no entry can be recovered twice. + if again := f.takeRemaining(); len(again) != 0 { + t.Fatalf("second takeRemaining n=%d, want 0", len(again)) + } +} + +// TestFDInflightOwnershipPartition pins the deque partition invariant: with the +// correct usage (hardClose, then takeRemaining ONLY after the reader has exited) +// every entry is owned by EXACTLY ONE side — advanced by the reader OR returned +// by takeRemaining, never both and never neither. A reader drains+advances while +// a second goroutine races a hardClose, so -race also proves advance/hardClose/ +// takeRemaining are lock-clean. The session-level path (take the tail only after +// <-readerDone) is covered end-to-end by TestFullDuplexRecoversFromConnKill. +func TestFDInflightOwnershipPartition(t *testing.T) { + ctx := context.Background() + for iter := 0; iter < 200; iter++ { + f := newFDInflight() + const n = 64 + all := make([]fdReq, n) + for i := range all { + all[i] = fdReq{cmd: NewStatusCmd(ctx, "set", itoa(i), "v"), batch: newAPBatch()} + } + f.pushBatch(all) + + advanced := make(map[Cmder]struct{}, n) + readerDone := make(chan struct{}) + go func() { + defer close(readerDone) + var buf []fdReq + for { + var ok bool + buf, ok = f.frontBatch(buf) + if !ok { + return // hardClose observed → reader exits (mirrors the real reader) + } + // Complete a slice of the snapshot, then advance exactly that many. + take := len(buf)/2 + 1 + if take > len(buf) { + take = len(buf) + } + for i := 0; i < take; i++ { + advanced[buf[i].cmd] = struct{}{} + } + f.advance(take) + } + }() + + // Race the hard close against the reader's progress. + f.hardClose() + <-readerDone // MUST wait before taking — that ordering is the fix + rem := f.takeRemaining() + + // Partition check: advanced ⊎ remaining == all, disjoint, complete. + if len(advanced)+len(rem) != n { + t.Fatalf("iter %d: advanced=%d + remaining=%d != %d (entry lost or double-owned)", + iter, len(advanced), len(rem), n) + } + for _, r := range rem { + if _, dup := advanced[r.cmd]; dup { + t.Fatalf("iter %d: cmd both advanced AND in recovery tail — double-owned (would double-execute)", iter) + } + } + } +} + +// fdCountLimiter counts Allow/ReportResult to verify the full-duplex engine +// accounts the Limiter once per session (Allow on acquire, ReportResult before +// release), balanced 1:1. +type fdCountLimiter struct{ allow, report atomic.Int64 } + +func (l *fdCountLimiter) Allow() error { l.allow.Add(1); return nil } +func (l *fdCountLimiter) ReportResult(_ error) { l.report.Add(1) } + +// TestFullDuplexLimiterPerSession verifies FullDuplex honors opt.Limiter with +// per-session accounting: every session's conn acquisition is bracketed by +// exactly one Allow and one ReportResult, the report before the release. +func TestFullDuplexLimiterPerSession(t *testing.T) { + ctx := context.Background() + lim := &fdCountLimiter{} + c := NewClient(&Options{ + Addr: ":6379", + Protocol: 3, + PipelinePoolSize: 4, + PipelineReadBufferSize: 64 * 1024, + PipelineWriteBufferSize: 64 * 1024, + PoolSize: 4, + Limiter: lim, + }) + defer c.Close() + if err := c.Ping(ctx).Err(); err != nil { + t.Skipf("no redis: %v", err) + } + + ap, err := c.AsyncAutoPipelineWithOptions(&AutoPipelineOptions{FullDuplex: true}) + if err != nil { + t.Fatalf("AsyncAutoPipeline: %v", err) + } + if ap.fd == nil { + t.Fatal("full-duplex engine not active") + } + if err := ap.Set(ctx, "fd:lim:k", "v", 0).Err(); err != nil { + t.Fatalf("set: %v", err) + } + // A tiny idle window so any late session settles before we read the counters. + time.Sleep(20 * time.Millisecond) + // Close waits for the engine (ap.wg), so every session's deferred + // ReportResult has run by the time Close returns. + if err := ap.Close(); err != nil { + t.Fatalf("close: %v", err) + } + + a, r := lim.allow.Load(), lim.report.Load() + if a < 1 { + t.Fatalf("FullDuplex never called Limiter.Allow (allow=%d) — Limiter bypassed", a) + } + if a != r { + t.Fatalf("FullDuplex Limiter unbalanced: Allow=%d ReportResult=%d (must be 1:1 per session)", a, r) + } +} + +// TestFullDuplexRetryDivertsToNormalConn verifies the mechanism the FD reader +// uses for a retryable Redis error (LOADING/READONLY/…) or a redirect (MOVED/ASK): +// the command is re-run on the client's normal path and the caller is settled with +// that result — not left with the FD error. It drives retryOnNormalConn directly +// (a deterministic stand-in for the reader's divert) since inducing a real LOADING +// on a live server is not reproducible. +func TestFullDuplexRetryDivertsToNormalConn(t *testing.T) { + ctx := context.Background() + c := fdTestClient(":6379") + defer c.Close() + if err := c.Ping(ctx).Err(); err != nil { + t.Skipf("no redis: %v", err) + } + ap, err := c.AsyncAutoPipelineWithOptions(&AutoPipelineOptions{FullDuplex: true}) + if err != nil { + t.Fatalf("AsyncAutoPipeline: %v", err) + } + defer ap.Close() + if ap.fd == nil { + t.Fatal("full-duplex engine not active") + } + + if err := c.Set(ctx, "fd:retry:k", "v", 0).Err(); err != nil { + t.Fatalf("seed: %v", err) + } + + // As the reader does on a retryable error: hand an FD request to the divert. + cmd := NewStringCmd(ctx, "get", "fd:retry:k") + b := newAPBatch() + cmd.setReady(b) + ap.fd.retryOnNormalConn(fdReq{cmd: cmd, batch: b}) + + select { + case <-b.done: + case <-time.After(2 * time.Second): + t.Fatal("retryOnNormalConn did not complete the diverted command") + } + if v, err := cmd.Result(); err != nil || v != "v" { + t.Fatalf("diverted GET = %q err=%v, want \"v\" (re-run on the normal path)", v, err) + } +} + +// fdOtelRecorder counts RecordOperationDuration to prove the full-duplex reader +// emits the native per-command OTel metric itself (it bypasses process, which +// would otherwise emit it). All other Recorder methods are no-ops. +type fdOtelRecorder struct{ opDurations atomic.Int64 } + +func (r *fdOtelRecorder) RecordOperationDuration(context.Context, time.Duration, otel.Cmder, int, error, *pool.Conn, int) { + r.opDurations.Add(1) +} +func (r *fdOtelRecorder) RecordPipelineOperationDuration(context.Context, time.Duration, string, int, int, error, *pool.Conn, int) { +} +func (r *fdOtelRecorder) RecordConnectionCreateTime(context.Context, time.Duration, *pool.Conn) {} +func (r *fdOtelRecorder) RecordConnectionRelaxedTimeout(context.Context, int, *pool.Conn, string, string) { +} +func (r *fdOtelRecorder) RecordConnectionHandoff(context.Context, *pool.Conn, string) {} +func (r *fdOtelRecorder) RecordError(context.Context, string, *pool.Conn, string, bool, int) {} +func (r *fdOtelRecorder) RecordMaintenanceNotification(context.Context, *pool.Conn, string) {} +func (r *fdOtelRecorder) RecordConnectionWaitTime(context.Context, time.Duration, *pool.Conn) {} +func (r *fdOtelRecorder) RecordConnectionClosed(context.Context, *pool.Conn, string, error) {} +func (r *fdOtelRecorder) RecordPubSubMessage(context.Context, *pool.Conn, string, string, bool) { +} +func (r *fdOtelRecorder) RecordStreamLag(context.Context, time.Duration, *pool.Conn, string, string, string) { +} +func (r *fdOtelRecorder) RecordConnectionCount(context.Context, int, *pool.Conn, string, bool) {} +func (r *fdOtelRecorder) RecordPendingRequests(context.Context, int, *pool.Conn, string) {} + +// TestFullDuplexRecordsOTelOperationDuration verifies the FD reader records the +// native per-command OTel duration metric (redisotel-native) itself, which it +// must because it completes commands without going through process(). +func TestFullDuplexRecordsOTelOperationDuration(t *testing.T) { + ctx := context.Background() + c := fdTestClient(":6379") + defer c.Close() + if err := c.Ping(ctx).Err(); err != nil { + t.Skipf("no redis: %v", err) + } + ap, err := c.AsyncAutoPipelineWithOptions(&AutoPipelineOptions{FullDuplex: true}) + if err != nil { + t.Fatalf("AsyncAutoPipeline: %v", err) + } + defer ap.Close() + if ap.fd == nil { + t.Fatal("full-duplex engine not active") + } + + rec := &fdOtelRecorder{} + otel.SetGlobalRecorder(rec) + defer otel.SetGlobalRecorder(nil) + + // Runs through the FD pipe (writer -> reader), which is where the metric is + // emitted; Result() blocks until the reader completed it (emit is before + // complete()). + if err := ap.Set(ctx, "fd:otel:k", "v", 0).Err(); err != nil { + t.Fatalf("set: %v", err) + } + if n := rec.opDurations.Load(); n < 1 { + t.Fatalf("FullDuplex recorded no OTel RecordOperationDuration (n=%d) — native metric bypassed", n) + } +} + +// TestFullDuplexDivertsHImportOffPipe verifies the FD engine routes a managed +// HIMPORT command off the shared pipe to the normal Process path: the FD writer +// never injects the registered PREPARE, so an HIMPORT SET riding the pipe can +// fail "no such fieldset". The assertion is routing, not end-to-end HIMPORT +// (which needs Redis 8.10+): a diverted command runs on the MAIN pool while an +// FD-pipe command never touches it, so a main-pool Hits/Misses bump after a lone +// HImportSet proves the divert. Its own result is irrelevant — it may error on an +// old server. +func TestFullDuplexDivertsHImportOffPipe(t *testing.T) { + ctx := context.Background() + c := fdTestClient(":6379") + defer c.Close() + if err := c.Ping(ctx).Err(); err != nil { + t.Skipf("no redis: %v", err) + } + ap, err := c.AsyncAutoPipelineWithOptions(&AutoPipelineOptions{FullDuplex: true}) + if err != nil { + t.Fatalf("AsyncAutoPipeline: %v", err) + } + defer ap.Close() + if ap.fd == nil { + t.Fatal("full-duplex engine not active") + } + + // Warm the main pool so a later reuse registers as a Hit, then snapshot. The + // FD engine uses the pipeline pool, so nothing but the diverted HImportSet + // touches the main pool between the two snapshots. + if err := c.Ping(ctx).Err(); err != nil { + t.Fatalf("warm ping: %v", err) + } + before := c.PoolStats() + + // Err() on the async face blocks until the diverted command has executed, so + // the "after" snapshot reflects its pool use. Ignore the result: it may be + // "no such fieldset" (registry empty) or "unknown command" (Redis < 8.10) — + // either way it ran on the main pool, which is what we assert. + _ = ap.HImportSet(ctx, "fd:himport:k", "fd:himport:fs", "v").Err() + + after := c.PoolStats() + beforeN := before.Hits + before.Misses + afterN := after.Hits + after.Misses + if afterN <= beforeN { + t.Fatalf("HImportSet did not use the main pool (before=%d after=%d) — it rode the FD pipe instead of diverting to the normal path", beforeN, afterN) + } +} + +// fdCloseHook is a ProcessHook that does work AFTER next() returns, to prove the +// full-duplex engine tracks its hook-host goroutines so AutoPipeliner.Close waits +// for post-next hook work before returning. It signals once when it has entered +// the post-next phase, then holds briefly and records completion. +type fdCloseHook struct { + entered chan struct{} + once sync.Once + finished atomic.Bool + holdFor time.Duration + watchName string +} + +func (h *fdCloseHook) DialHook(next DialHook) DialHook { return next } + +func (h *fdCloseHook) ProcessHook(next ProcessHook) ProcessHook { + return func(ctx context.Context, cmd Cmder) error { + err := next(ctx, cmd) + if cmd.Name() != h.watchName { + return err + } + h.once.Do(func() { close(h.entered) }) + // Post-next work: if Close does not wait for this host goroutine, Close + // returns while we are still sleeping and finished is still false. + time.Sleep(h.holdFor) + h.finished.Store(true) + return err + } +} + +func (h *fdCloseHook) ProcessPipelineHook(next ProcessPipelineHook) ProcessPipelineHook { + return next +} + +// TestFullDuplexCloseWaitsForHookHosts verifies AutoPipeliner.Close does not +// return until a full-duplex command's post-next ProcessHook has finished. The FD +// hook host is the only goroutine that closes such a command's batch, so an +// untracked host would let Close return with accepted commands still blocked +// behind post-reply hooks — violating drain-before-return. +func TestFullDuplexCloseWaitsForHookHosts(t *testing.T) { + ctx := context.Background() + c := fdTestClient(":6379") + defer c.Close() + if err := c.Ping(ctx).Err(); err != nil { + t.Skipf("no redis: %v", err) + } + + hook := &fdCloseHook{entered: make(chan struct{}), holdFor: 120 * time.Millisecond, watchName: "set"} + c.AddHook(hook) + + ap, err := c.AsyncAutoPipelineWithOptions(&AutoPipelineOptions{FullDuplex: true}) + if err != nil { + t.Fatalf("AsyncAutoPipeline: %v", err) + } + if ap.fd == nil { + t.Fatal("full-duplex engine not active") + } + + // Fire the command on the async face and do NOT read its result (reading would + // itself block on the host closing the batch, hiding the bug). Wait until the + // host is in its post-next phase, then Close must wait for it to finish. + ap.Set(ctx, "fd:close:k", "v", 0) + select { + case <-hook.entered: + case <-time.After(3 * time.Second): + t.Fatal("post-next hook never ran — command did not reach the FD reader") + } + + if err := ap.Close(); err != nil { + t.Fatalf("close: %v", err) + } + if !hook.finished.Load() { + t.Fatal("Close returned before the post-next ProcessHook finished — FD hook host not waited (drain-before-return violated)") + } +} + +// fdSelfReadHook calls next and then reads the command's OWN result +// (cmd.Err()), the documented pattern also exercised by the async autopipeline +// hook tests. On the full-duplex path the host goroutine is the only code that +// closes the command's batch, so without the executor guard this read blocks on +// batch.done forever. It records the error it observed. +type fdSelfReadHook struct { + watchName string + observed chan error +} + +func (h *fdSelfReadHook) DialHook(next DialHook) DialHook { return next } + +func (h *fdSelfReadHook) ProcessHook(next ProcessHook) ProcessHook { + return func(ctx context.Context, cmd Cmder) error { + err := next(ctx, cmd) + if cmd.Name() != h.watchName { + return err + } + // Read own result after next: must return the just-executed view, not + // block on the batch this very goroutine is responsible for closing. + got := cmd.Err() + select { + case h.observed <- got: + default: + } + return err + } +} + +func (h *fdSelfReadHook) ProcessPipelineHook(next ProcessPipelineHook) ProcessPipelineHook { + return next +} + +// TestFullDuplexHookReadingOwnResultDoesNotDeadlock verifies a ProcessHook that +// reads its command's result after next() completes instead of hanging, on the +// full-duplex path: unless the FD host marks itself as the batch executor, +// cmd.Err() inside the hook blocks on batch.done — which only that same goroutine +// closes, a hard deadlock. +func TestFullDuplexHookReadingOwnResultDoesNotDeadlock(t *testing.T) { + ctx := context.Background() + c := fdTestClient(":6379") + defer c.Close() + if err := c.Ping(ctx).Err(); err != nil { + t.Skipf("no redis: %v", err) + } + + hook := &fdSelfReadHook{watchName: "set", observed: make(chan error, 1)} + c.AddHook(hook) + + ap, err := c.AsyncAutoPipelineWithOptions(&AutoPipelineOptions{FullDuplex: true}) + if err != nil { + t.Fatalf("AsyncAutoPipeline: %v", err) + } + defer ap.Close() + if ap.fd == nil { + t.Fatal("full-duplex engine not active") + } + + // Guard the whole command with a timeout: a deadlock leaves the hook (and so + // the caller awaiting the batch) blocked forever. + var callerErr atomic.Value + done := make(chan struct{}) + go func() { + if e := ap.Set(ctx, "fd:selfread:k", "v", 0).Err(); e != nil { + callerErr.Store(e) + } + close(done) + }() + select { + case <-done: + case <-time.After(3 * time.Second): + t.Fatal("deadlock: hook read its command result after next and blocked on its own FD batch") + } + if v := callerErr.Load(); v != nil { + t.Fatalf("set: %v", v) + } + select { + case got := <-hook.observed: + if got != nil { + t.Fatalf("hook observed err=%v after next, want nil (just-executed view)", got) + } + default: + t.Fatal("hook did not run on the FD path") + } +} + +// fdPrePanicHook panics BEFORE calling next, to prove the FD host waits for the +// reader to finish writing the command before releasing the caller: the command +// is already streamed, so closing the batch immediately would race the reader's +// write into cmd against the caller's read of it. +type fdPrePanicHook struct{ watchName string } + +func (h *fdPrePanicHook) DialHook(next DialHook) DialHook { return next } +func (h *fdPrePanicHook) ProcessHook(next ProcessHook) ProcessHook { + return func(ctx context.Context, cmd Cmder) error { + if cmd.Name() == h.watchName { + panic("boom: hook panic before next") + } + return next(ctx, cmd) + } +} +func (h *fdPrePanicHook) ProcessPipelineHook(next ProcessPipelineHook) ProcessPipelineHook { + return next +} + +// TestFullDuplexPreNextHookPanicSettlesWithoutRace verifies a ProcessHook that +// panics before next() surfaces an error to the caller, does not hang, and does +// not race the reader's write into the command (run with -race): a recover that +// closed the batch without first awaiting hookDone would let the reader write cmd +// after the caller had already observed the failure. +func TestFullDuplexPreNextHookPanicSettlesWithoutRace(t *testing.T) { + ctx := context.Background() + c := fdTestClient(":6379") + defer c.Close() + if err := c.Ping(ctx).Err(); err != nil { + t.Skipf("no redis: %v", err) + } + c.AddHook(&fdPrePanicHook{watchName: "set"}) + ap, err := c.AsyncAutoPipelineWithOptions(&AutoPipelineOptions{FullDuplex: true}) + if err != nil { + t.Fatalf("AsyncAutoPipeline: %v", err) + } + defer ap.Close() + if ap.fd == nil { + t.Fatal("full-duplex engine not active") + } + done := make(chan error, 1) + go func() { done <- ap.Set(ctx, "fd:prepanic:k", "v", 0).Err() }() + select { + case e := <-done: + if e == nil { + t.Fatal("expected the hook panic to surface as an error") + } + case <-time.After(3 * time.Second): + t.Fatal("caller hung after a pre-next hook panic") + } +} + +// TestFDFirstNoRetry verifies the tail-split index that lets the FD retry path +// replay the retryable PREFIX of an unacked tail while never re-sending a NoRetry +// command (or anything ordered after it). +func TestFDFirstNoRetry(t *testing.T) { + ctx := context.Background() + retry := fdReq{cmd: NewStringCmd(ctx, "get", "k")} // NoRetry() == false + nore := fdReq{cmd: NewRawWriteToCmd(ctx, nil, "x")} // NoRetry() == true + cases := []struct { + name string + in []fdReq + want int + }{ + {"empty", nil, 0}, + {"all-retryable", []fdReq{retry, retry}, 2}, + {"leading-noretry", []fdReq{nore, retry}, 0}, + {"noretry-in-middle", []fdReq{retry, retry, nore, retry}, 2}, + {"trailing-noretry", []fdReq{retry, nore}, 1}, + } + for _, tc := range cases { + if got := fdFirstNoRetry(tc.in); got != tc.want { + t.Fatalf("%s: fdFirstNoRetry = %d, want %d", tc.name, got, tc.want) + } + } +} + +// TestFDBatchEnd verifies the replay chunk boundaries: the recovered tail is +// re-issued in the same MaxBatchSize/MaxBatchBytes-capped chunks as freshly +// drained work, so a large recovered window is not flushed in one oversized write. +func TestFDBatchEnd(t *testing.T) { + ctx := context.Background() + mk := func(val string) fdReq { return fdReq{cmd: NewStatusCmd(ctx, "set", "k", val)} } + + small := make([]fdReq, 7) + for i := range small { + small[i] = mk("v") + } + // maxBatch cap, byteLimit disabled: chunks of maxBatch, last chunk the remainder. + if got := fdBatchEnd(small, 0, 3, 0); got != 3 { + t.Fatalf("maxBatch: end=%d want 3", got) + } + if got := fdBatchEnd(small, 6, 3, 0); got != 7 { + t.Fatalf("tail remainder: end=%d want 7", got) + } + if got := fdBatchEnd(small[:1], 0, 3, 0); got != 1 { + t.Fatalf("single element: end=%d want 1", got) + } + + // byteLimit cap. Each command is ~1052 bytes (cmdApproxBytes: per-arg len + 16). + big := make([]fdReq, 4) + for i := range big { + big[i] = mk(strings.Repeat("x", 1000)) + } + // Limit below one command's size: the lone oversized command still goes (chunk 1). + if got := fdBatchEnd(big, 0, 100, 500); got != 1 { + t.Fatalf("byteLimit lone oversized: end=%d want 1", got) + } + // Limit spanning ~two commands: first always in, stop once the payload reaches it. + if got := fdBatchEnd(big, 0, 100, 1500); got != 2 { + t.Fatalf("byteLimit two: end=%d want 2", got) + } +} + +// fdPanicWriter panics from Write, simulating a user io.Writer that panics while +// a RawWriteToCmd's readReply streams the raw reply on the FD reader goroutine. +type fdPanicWriter struct{} + +func (fdPanicWriter) Write(p []byte) (int, error) { panic("boom: reply decoder panic") } + +// TestFullDuplexReaderPanicRecovers verifies a panic in reply decoding on the FD +// reader goroutine is recovered (not a process crash): the command is settled +// with an error and the engine keeps serving on a fresh session. A raw +// RawWriteToCmd rides the FD pipe and its writer panics while the reader streams +// the reply. +func TestFullDuplexReaderPanicRecovers(t *testing.T) { + ctx := context.Background() + c := fdTestClient(":6379") + defer c.Close() + if err := c.Ping(ctx).Err(); err != nil { + t.Skipf("no redis: %v", err) + } + if err := c.Set(ctx, "fd:rpanic:k", "hello", 0).Err(); err != nil { + t.Fatalf("seed: %v", err) + } + ap, err := c.AsyncAutoPipelineWithOptions(&AutoPipelineOptions{FullDuplex: true}) + if err != nil { + t.Fatalf("AsyncAutoPipeline: %v", err) + } + defer ap.Close() + if ap.fd == nil { + t.Fatal("full-duplex engine not active") + } + + cmd := NewRawWriteToCmd(ctx, fdPanicWriter{}, "get", "fd:rpanic:k") + f := ap.Submit(ctx, cmd) + done := make(chan struct{}) + go func() { _ = f.Wait(); close(done) }() + select { + case <-done: + case <-time.After(3 * time.Second): + t.Fatal("command never settled after a reader-decoder panic (reader did not recover)") + } + if cmd.Err() == nil { + t.Fatal("expected an error after the reader-decoder panic, got nil") + } + // The engine must survive: a subsequent command works on a fresh session. + if err := ap.Set(ctx, "fd:rpanic:after", "v", 0).Err(); err != nil { + t.Fatalf("engine did not recover after the reader panic: %v", err) + } +} + +// fdNoopHook is a passthrough ProcessHook whose only effect is to make the FD +// engine host each command (hookCount > 0), so every completed command has a +// hookDone channel — a double-complete would then double-close it (panic). +type fdNoopHook struct{} + +func (fdNoopHook) DialHook(next DialHook) DialHook { return next } +func (fdNoopHook) ProcessHook(next ProcessHook) ProcessHook { return next } +func (fdNoopHook) ProcessPipelineHook(next ProcessPipelineHook) ProcessPipelineHook { return next } + +// TestFullDuplexReaderPanicMidBatchNoDoubleComplete guards the reader-panic +// recover path: when the panic hits a command sharing a frontBatch snapshot with +// EARLIER, already-completed commands, the recover must advance those out of the +// deque — otherwise recovery re-owns and re-completes them, double-closing their +// hookDone (a second panic that crashes the process). A no-op hook makes every +// command hooked, with completed GETs ahead of the panicking one in a batch. +func TestFullDuplexReaderPanicMidBatchNoDoubleComplete(t *testing.T) { + ctx := context.Background() + c := fdTestClient(":6379") + defer c.Close() + if err := c.Ping(ctx).Err(); err != nil { + t.Skipf("no redis: %v", err) + } + c.AddHook(fdNoopHook{}) + if err := c.Set(ctx, "fd:rpanic2:k", "hello", 0).Err(); err != nil { + t.Fatalf("seed: %v", err) + } + ap, err := c.AsyncAutoPipelineWithOptions(&AutoPipelineOptions{FullDuplex: true}) + if err != nil { + t.Fatalf("AsyncAutoPipeline: %v", err) + } + defer ap.Close() + if ap.fd == nil { + t.Fatal("full-duplex engine not active") + } + + // Fire several hooked GETs immediately followed by a decoder-panicking command, + // none awaited, so they land in one writer batch / reader snapshot: the GETs + // complete (done > 0) before the panic. + for i := 0; i < 8; i++ { + ap.Get(ctx, "fd:rpanic2:k") + } + panicCmd := NewRawWriteToCmd(ctx, fdPanicWriter{}, "get", "fd:rpanic2:k") + f := ap.Submit(ctx, panicCmd) + done := make(chan struct{}) + go func() { _ = f.Wait(); close(done) }() + select { + case <-done: + case <-time.After(3 * time.Second): + t.Fatal("panicking command never settled") + } + // If the completed GETs were re-completed, their hookDone double-close would + // have crashed the process. Reaching here on a working engine proves it did not. + if err := ap.Set(ctx, "fd:rpanic2:after", "v", 0).Err(); err != nil { + t.Fatalf("engine did not recover after the mid-batch reader panic: %v", err) + } +} + +var errFDLimiterOpen = errors.New("fd test: limiter breaker open") + +// fdRejectLimiter denies every Allow(), simulating an open circuit breaker. +type fdRejectLimiter struct{ allow atomic.Int64 } + +func (l *fdRejectLimiter) Allow() error { l.allow.Add(1); return errFDLimiterOpen } +func (l *fdRejectLimiter) ReportResult(_ error) {} + +// TestFullDuplexLimiterRejectFailsQueuedWork verifies that when the Limiter +// denies FD session acquisition (fdDenied), accepted commands fail-fast with the +// limiter error instead of hanging in fd.ch until the breaker closes. +func TestFullDuplexLimiterRejectFailsQueuedWork(t *testing.T) { + ctx := context.Background() + probe := NewClient(&Options{Addr: ":6379"}) + defer probe.Close() + if err := probe.Ping(ctx).Err(); err != nil { + t.Skipf("no redis: %v", err) + } + + c := NewClient(&Options{ + Addr: ":6379", + Protocol: 3, + PipelinePoolSize: 4, + PipelineReadBufferSize: 64 * 1024, + PipelineWriteBufferSize: 64 * 1024, + PoolSize: 4, + Limiter: &fdRejectLimiter{}, + MaxRetries: 2, + }) + defer c.Close() + ap, err := c.AsyncAutoPipelineWithOptions(&AutoPipelineOptions{FullDuplex: true}) + if err != nil { + t.Fatalf("AsyncAutoPipeline: %v", err) + } + defer ap.Close() + if ap.fd == nil { + t.Fatal("full-duplex engine not active") + } + + done := make(chan error, 1) + go func() { done <- ap.Set(ctx, "fd:limreject:k", "v", 0).Err() }() + select { + case e := <-done: + if !errors.Is(e, errFDLimiterOpen) { + t.Fatalf("got %v, want the limiter error (fail-fast, not hang)", e) + } + case <-time.After(3 * time.Second): + t.Fatal("command hung on limiter rejection instead of failing fast") + } +} + +// TestFullDuplexProcessReportsSubmitRejection verifies raw Process(ctx,cmd) +// surfaces an FD submit-time rejection instead of returning nil: every submit +// reject path (closed / caller-ctx cancel / engine ctx) returns the shared +// completedBatch sentinel, which processAsync checks to report cmd.rawErr(). The +// closed path is the one exercised here; the others use the identical return. +func TestFullDuplexProcessReportsSubmitRejection(t *testing.T) { + ctx := context.Background() + c := fdTestClient(":6379") + defer c.Close() + if err := c.Ping(ctx).Err(); err != nil { + t.Skipf("no redis: %v", err) + } + ap, err := c.AsyncAutoPipelineWithOptions(&AutoPipelineOptions{FullDuplex: true}) + if err != nil { + t.Fatalf("AsyncAutoPipeline: %v", err) + } + if ap.fd == nil { + t.Fatal("full-duplex engine not active") + } + if err := ap.Close(); err != nil { + t.Fatalf("close: %v", err) + } + // Raw Process on a closed FD autopipeliner must report ErrClosed, not nil. + if err := ap.Process(ctx, NewStatusCmd(ctx, "ping")); !errors.Is(err, ErrClosed) { + t.Fatalf("Process after Close = %v, want ErrClosed (submit rejection not surfaced)", err) + } +} + +// TestFullDuplexCloseFlushesBacklog verifies graceful Close executes the +// accepted-but-unwritten fd.ch commands instead of failing them ErrClosed +// ("accepted ⇒ completes"). A burst is submitted and Close called immediately, so +// some commands are still in the backlog when Close runs; none may be ErrClosed. +func TestFullDuplexCloseFlushesBacklog(t *testing.T) { + ctx := context.Background() + c := fdTestClient(":6379") + defer c.Close() + if err := c.Ping(ctx).Err(); err != nil { + t.Skipf("no redis: %v", err) + } + ap, err := c.AsyncAutoPipelineWithOptions(&AutoPipelineOptions{FullDuplex: true}) + if err != nil { + t.Fatalf("AsyncAutoPipeline: %v", err) + } + if ap.fd == nil { + t.Fatal("full-duplex engine not active") + } + + const n = 300 + cmds := make([]*StatusCmd, n) + for i := 0; i < n; i++ { + cmds[i] = ap.Set(ctx, "fd:closeflush:"+itoa(i), "v", 0) + } + if err := ap.Close(); err != nil { + t.Fatalf("close: %v", err) + } + for i, cmd := range cmds { + if errors.Is(cmd.Err(), ErrClosed) { + t.Fatalf("cmd %d came back ErrClosed — Close did not flush the accepted fd.ch backlog", i) + } + } +} + +// TestFullDuplexLeaseFailureFailsBacklog verifies that when the engine cannot +// lease a connection for a new session (fdLeaseErr, server down), accepted +// commands fail-fast once the lease retries are exhausted instead of hanging in +// fd.ch. Uses a dead address, so it needs no live server. +func TestFullDuplexLeaseFailureFailsBacklog(t *testing.T) { + ctx := context.Background() + c := NewClient(&Options{ + Addr: "127.0.0.1:1", // nothing listening: dial refused + Protocol: 3, + PipelinePoolSize: 2, + PipelineReadBufferSize: 64 * 1024, + PipelineWriteBufferSize: 64 * 1024, + PoolSize: 2, + MaxRetries: 2, + DialTimeout: 150 * time.Millisecond, + MinRetryBackoff: time.Millisecond, + MaxRetryBackoff: 5 * time.Millisecond, + }) + defer c.Close() + ap, err := c.AsyncAutoPipelineWithOptions(&AutoPipelineOptions{FullDuplex: true}) + if err != nil { + t.Fatalf("AsyncAutoPipeline: %v", err) + } + defer ap.Close() + if ap.fd == nil { + t.Fatal("full-duplex engine not active") + } + + done := make(chan error, 1) + go func() { done <- ap.Set(ctx, "fd:leasefail:k", "v", 0).Err() }() + select { + case e := <-done: + if e == nil { + t.Fatal("expected a connection error on a persistent lease failure, got nil") + } + case <-time.After(6 * time.Second): + t.Fatal("command hung on a persistent lease failure instead of failing after retries") + } +} + +// TestFullDuplexMaxHoldIdleDoesNotChurn verifies that with FullDuplexMaxHold set +// shorter than FullDuplexIdleTimeout a quiet engine does NOT Get/Put-recycle +// every max-hold interval: the max-hold branch returns fdIdle when the pipe is +// drained, so run() blocks for the next command instead of re-leasing. +func TestFullDuplexMaxHoldIdleDoesNotChurn(t *testing.T) { + ctx := context.Background() + c := fdTestClient(":6379") + defer c.Close() + if err := c.Ping(ctx).Err(); err != nil { + t.Skipf("no redis: %v", err) + } + ap, err := c.AsyncAutoPipelineWithOptions(&AutoPipelineOptions{ + FullDuplex: true, + FullDuplexMaxHold: 40 * time.Millisecond, + FullDuplexIdleTimeout: 3 * time.Second, + }) + if err != nil { + t.Fatalf("AsyncAutoPipeline: %v", err) + } + defer ap.Close() + if ap.fd == nil { + t.Fatal("full-duplex engine not active") + } + + // No work for several max-hold intervals. A drained engine must idle, not churn. + time.Sleep(300 * time.Millisecond) + if n := ap.fd.recycles.Load(); n > 2 { + t.Fatalf("idle engine recycled %d times in 300ms (max-hold ~40ms) — it churns Get/Put when idle", n) + } +} diff --git a/autopipeline_test.go b/autopipeline_test.go index 26a276bda1..41bbfbe8ae 100644 --- a/autopipeline_test.go +++ b/autopipeline_test.go @@ -1874,8 +1874,24 @@ func TestAutoPipelineRetriesOnNetworkError(t *testing.T) { } defer ap.Close() - // Arm after the handshake: the pooled conn is healthy, so it passes the - // pool health check, and the batch's first write dies on the wire. + // Warm the DEDICATED PIPELINE POOL connection first: creating the + // autopipeliner created the pipeline pool (lazily), and BATCHES run there, + // not on the main-pool conn the Ping dialed. Without this warm-up the + // batch's first dispatch dials a fresh pipeline conn and the dial count + // reads one high for a reason unrelated to the retry. The warm-up must be + // a real multi-command batch: a lone command takes the solo fast path + // (Process on the main pool) and would not touch the pipeline pool. + w1 := ap.Set(ctx, "apr:warm", 1, 0) + w2 := ap.Incr(ctx, "apr:warm2") + if err := w1.Err(); err != nil { + t.Fatal(err) + } + if err := w2.Err(); err != nil { + t.Fatal(err) + } + + // Arm after the handshake: the pipeline-pool conn is healthy, so it passes + // the pool health check, and the batch's first write dies on the wire. dialsBefore := dials.Load() failNextWrite.Store(true) @@ -3927,7 +3943,6 @@ func TestAutoPipelineHookPostNextErrorPartialBatch(t *testing.T) { if err := c.Set(ctx, "pnp:present", "v", 0).Err(); err != nil { t.Fatal(err) } - c.AddHook(postNextErrorHook{err: errInjected}) // Wide flush window so all three commands deterministically land in ONE // pipeline batch (the rule is per-batch: hooks fire per batch). @@ -3940,6 +3955,23 @@ func TestAutoPipelineHookPostNextErrorPartialBatch(t *testing.T) { } defer ap.Close() + // Initialize the dedicated pipeline-pool connection BEFORE installing the + // error-injecting hook: connection init runs its handshake pipeline through + // the client's hook chain (newConn shares hooksMixin), so a hook that + // unconditionally injects an error would fail the pipeline conn's init and + // this test would measure init poisoning instead of the post-next rule. + // Must be a real multi-command batch — a lone command takes the solo fast + // path (Process on the main pool) and would not init the pipeline conn. + w1 := ap.Set(ctx, "pnp:warm", 1, 0) + w2 := ap.Set(ctx, "pnp:warm2", 1, 0) + if err := w1.Err(); err != nil { + t.Fatal(err) + } + if err := w2.Err(); err != nil { + t.Fatal(err) + } + c.AddHook(postNextErrorHook{err: errInjected}) + runWithWatchdog(t, 30*time.Second, func() { // One batch: a hit, a miss (redis.Nil), and a write. hit := ap.Get(ctx, "pnp:present") diff --git a/commands_test.go b/commands_test.go index 26c248df1e..62fda94620 100644 --- a/commands_test.go +++ b/commands_test.go @@ -83,11 +83,27 @@ var _ = Describe("Commands", func() { Expect(cmds[0].Err().Error()).To(authErr) stats := rawClient.PoolStats() - Expect(stats.Hits).To(Equal(uint32(2))) - Expect(stats.Misses).To(Equal(uint32(1))) - Expect(stats.Timeouts).To(Equal(uint32(0))) - Expect(stats.TotalConns).To(Equal(uint32(1))) - Expect(stats.IdleConns).To(Equal(uint32(1))) + if stats.PipelineStats != nil { + // The autopipeline subject faces create the dedicated pipeline + // pool lazily, so the two Pipelined calls above ran there: the + // first dialed (miss), the second reused (hit). The main pool + // served only the BeforeEach FlushDB. + Expect(stats.PipelineStats.Hits).To(Equal(uint32(1))) + Expect(stats.PipelineStats.Misses).To(Equal(uint32(1))) + Expect(stats.PipelineStats.Timeouts).To(Equal(uint32(0))) + Expect(stats.PipelineStats.TotalConns).To(Equal(uint32(1))) + Expect(stats.PipelineStats.IdleConns).To(Equal(uint32(1))) + Expect(stats.Hits).To(Equal(uint32(0))) + Expect(stats.Misses).To(Equal(uint32(1))) + } else { + // No dedicated pipeline pool: FlushDB dialed the one connection + // (miss) and both Pipelined calls reused it (hits). + Expect(stats.Hits).To(Equal(uint32(2))) + Expect(stats.Misses).To(Equal(uint32(1))) + Expect(stats.Timeouts).To(Equal(uint32(0))) + Expect(stats.TotalConns).To(Equal(uint32(1))) + Expect(stats.IdleConns).To(Equal(uint32(1))) + } }) It("should hello", func() { @@ -393,8 +409,16 @@ var _ = Describe("Commands", func() { }() pipe.ClientSetInfo(ctx, libInfo) }).To(Panic()) - // Test setting the default options for libName, libName suffix and libVer - clientInfo := rawClient.ClientInfo(ctx).Val() + // Test setting the default options for libName, libName suffix and libVer. + // CLIENT SETINFO is per-connection state, so read CLIENT INFO through the + // same pipeline path that issued the SETINFOs above: when the client has a + // dedicated pipeline pool (the autopipeline subject faces create one + // lazily), those ran on a pipeline-pool connection and rawClient.ClientInfo + // would inspect a different, main-pool connection. + infoCmd := pipe.ClientInfo(ctx) + _, err = pipe.Exec(ctx) + Expect(err).NotTo(HaveOccurred()) + clientInfo := infoCmd.Val() Expect(clientInfo.LibName).To(ContainSubstring("go-redis(go-redis,")) // Test setting the libName suffix in options opt := redisOptions() diff --git a/himport_mock_test.go b/himport_mock_test.go index a6ed47bd1d..69fab49fc1 100644 --- a/himport_mock_test.go +++ b/himport_mock_test.go @@ -356,10 +356,13 @@ func TestHImportLazyReplay(t *testing.T) { ctx := context.Background() client := redis.NewClient(&redis.Options{ - Addr: srv.addr(), - Protocol: 2, - PoolSize: 1, // deterministic: every command runs on the same connection - DisableIdentity: true, + Addr: srv.addr(), + Protocol: 2, + PoolSize: 1, // deterministic: every command runs on the same connection + // This file choreographs exact per-connection sequences (armed booms, + // session counts) on the MAIN pool; keep pipelines there too. + PipelinePoolSize: -1, + DisableIdentity: true, }) defer client.Close() @@ -450,10 +453,13 @@ func TestHImportPipelineRecoversAfterSessionLoss(t *testing.T) { ctx := context.Background() client := redis.NewClient(&redis.Options{ - Addr: srv.addr(), - Protocol: 2, - PoolSize: 1, - DisableIdentity: true, + Addr: srv.addr(), + Protocol: 2, + PoolSize: 1, + // This file choreographs exact per-connection sequences (armed booms, + // session counts) on the MAIN pool; keep pipelines there too. + PipelinePoolSize: -1, + DisableIdentity: true, }) defer client.Close() @@ -500,11 +506,14 @@ func TestHImportPipelineReissueTransportErrorScoped(t *testing.T) { ctx := context.Background() client := redis.NewClient(&redis.Options{ - Addr: srv.addr(), - Protocol: 2, - PoolSize: 1, - MaxRetries: -1, - DisableIdentity: true, + Addr: srv.addr(), + Protocol: 2, + PoolSize: 1, + // This file choreographs exact per-connection sequences (armed booms, + // session counts) on the MAIN pool; keep pipelines there too. + PipelinePoolSize: -1, + MaxRetries: -1, + DisableIdentity: true, }) defer client.Close() @@ -553,10 +562,13 @@ func TestHImportTxSurfacesSessionLoss(t *testing.T) { ctx := context.Background() client := redis.NewClient(&redis.Options{ - Addr: srv.addr(), - Protocol: 2, - PoolSize: 1, - DisableIdentity: true, + Addr: srv.addr(), + Protocol: 2, + PoolSize: 1, + // This file choreographs exact per-connection sequences (armed booms, + // session counts) on the MAIN pool; keep pipelines there too. + PipelinePoolSize: -1, + DisableIdentity: true, }) defer client.Close() @@ -602,10 +614,13 @@ func TestHImportLazyDiscardPropagation(t *testing.T) { ctx := context.Background() client := redis.NewClient(&redis.Options{ - Addr: srv.addr(), - Protocol: 2, - PoolSize: 2, - DisableIdentity: true, + Addr: srv.addr(), + Protocol: 2, + PoolSize: 2, + // This file choreographs exact per-connection sequences (armed booms, + // session counts) on the MAIN pool; keep pipelines there too. + PipelinePoolSize: -1, + DisableIdentity: true, }) defer client.Close() @@ -708,8 +723,11 @@ func TestHImportRingFanOut(t *testing.T) { "shard1": srv1.addr(), "shard2": srv2.addr(), }, - PoolSize: 1, - DisableIdentity: true, + PoolSize: 1, + // This file choreographs exact per-connection sequences (armed booms, + // session counts) on the MAIN pool; keep pipelines there too. + PipelinePoolSize: -1, + DisableIdentity: true, }) defer ring.Close() @@ -790,11 +808,14 @@ func TestHImportInjectedPrepareWithPushNotification(t *testing.T) { ctx := context.Background() client := redis.NewClient(&redis.Options{ - Addr: srv.addr(), - Protocol: 3, - PoolSize: 1, - MaxRetries: -1, // fail BOOM fast; the injected PREPARE needs no retries - DisableIdentity: true, + Addr: srv.addr(), + Protocol: 3, + PoolSize: 1, + // This file choreographs exact per-connection sequences (armed booms, + // session counts) on the MAIN pool; keep pipelines there too. + PipelinePoolSize: -1, + MaxRetries: -1, // fail BOOM fast; the injected PREPARE needs no retries + DisableIdentity: true, // The mock is not a real cluster; keep maintenance-notification // machinery out of the connection lifecycle. MaintNotificationsConfig: &maintnotifications.Config{Mode: maintnotifications.ModeDisabled}, @@ -847,6 +868,9 @@ func TestHImportPipelineInjectedReplyFailureStampsBatch(t *testing.T) { Addr: srv.addr(), Protocol: 2, PoolSize: 1, + // This file choreographs exact per-connection sequences (armed booms, + // session counts) on the MAIN pool; keep pipelines there too. + PipelinePoolSize: -1, // Exhaust the budget on the first attempt: stamping must not // depend on a later attempt reaching the read path. MaxRetries: -1, diff --git a/options.go b/options.go index 0863b38628..ebd9770e0d 100644 --- a/options.go +++ b/options.go @@ -246,15 +246,27 @@ type Options struct { PipelineWriteBufferSize int // PipelinePoolSize is the pool size for the separate pipeline connection pool. - // Only used if PipelineReadBufferSize or PipelineWriteBufferSize is set. + // Setting this alone is enough to create the dedicated pipeline pool; the + // pipeline buffer sizes then default to ReadBufferSize/WriteBufferSize. // // Pipelining typically needs fewer connections than regular operations because // batching reduces connection contention. A smaller pool saves memory while // maintaining high throughput. // - // If not set (0), defaults to 10 connections. + // The dedicated pipeline pool is created unconditionally at NewClient — + // like the pubsub pool — so pipelines never compete with regular commands + // for main-pool connections. It never pre-dials (MinIdleConns is forced + // to 0 on it), so the size is a cap on burst capacity, not a standing + // 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 + // buffers unless the pipeline buffer sizes are set explicitly. // - // default: 10 + // Set to a negative value to opt out of the dedicated pool entirely: + // pipelines then run on the main pool, as they did before the pool + // existed. + // + // default: DefaultPipelinePoolSize (10) connections PipelinePoolSize int // AutoPipelineOptions is the default config for BOTH autopipeliner faces: @@ -452,6 +464,23 @@ const ( CSCStrategySharedTracking CSCStrategy = iota ) +// DefaultPipelinePoolSize is the pipeline pool size used when +// PipelinePoolSize is not set. Pipelining batches many commands per round +// trip, so it needs far fewer connections than regular traffic. The pool is +// pure burst capacity: it never pre-dials idle connections (MinIdleConns is +// forced to 0 on it), so an unused pipeline pool holds no connections at all +// and the size is only a cap — bursts wider than it spill to the main pool. +const DefaultPipelinePoolSize = 10 + +// DefaultPipelineBufferSize is the per-connection read/write buffer size for +// the dedicated pipeline pool when no explicit pipeline buffer size is set +// (the larger of this and the regular buffer size is used). Pipeline +// connections move whole batches per round trip, so they earn bigger buffers +// than regular per-command traffic: measured on the autopipeline engine, +// throughput plateaus around 64 KiB and gains nothing past ~128 KiB, while +// very large buffers (>=512 KiB) can regress it. +const DefaultPipelineBufferSize = 64 * 1024 + func (opt *Options) init() { if opt.Addr == "" { opt.Addr = "localhost:6379" @@ -497,6 +526,7 @@ func (opt *Options) init() { if opt.PoolSize == 0 { opt.PoolSize = 10 * runtime.GOMAXPROCS(0) } + if opt.MaxConcurrentDials <= 0 { opt.MaxConcurrentDials = opt.PoolSize } else if opt.MaxConcurrentDials > opt.PoolSize { diff --git a/osscluster_test.go b/osscluster_test.go index e0d6884b90..979017d87d 100644 --- a/osscluster_test.go +++ b/osscluster_test.go @@ -1564,6 +1564,13 @@ var _ = Describe("ClusterClient", func() { node.AddHook(&hook{ processPipelineHook: func(hook redis.ProcessPipelineHook) redis.ProcessPipelineHook { return func(ctx context.Context, cmds []redis.Cmder) error { + defer GinkgoRecover() + // skip the connection initialization: the node's + // pipeline-pool connection initializes lazily, and + // its handshake pipeline runs through this hook chain + if len(cmds) == 0 || cmds[0].Name() == "hello" || cmds[0].Name() == "client" { + return hook(ctx, cmds) + } Expect(cmds).To(HaveLen(1)) cmdStr := cmds[0].String() @@ -1659,6 +1666,13 @@ var _ = Describe("ClusterClient", func() { node.AddHook(&hook{ processPipelineHook: func(hook redis.ProcessPipelineHook) redis.ProcessPipelineHook { return func(ctx context.Context, cmds []redis.Cmder) error { + defer GinkgoRecover() + // skip the connection initialization: the node's + // pipeline-pool connection initializes lazily, and + // its handshake pipeline runs through this hook chain + if len(cmds) == 0 || cmds[0].Name() == "hello" || cmds[0].Name() == "client" { + return hook(ctx, cmds) + } Expect(cmds).To(HaveLen(3)) Expect(cmds[1].String()).To(Equal("ping: ")) mu.Lock() diff --git a/pipeline_buffer_test.go b/pipeline_buffer_test.go index 6bbebe2ac4..30809de7c8 100644 --- a/pipeline_buffer_test.go +++ b/pipeline_buffer_test.go @@ -176,15 +176,18 @@ func TestPipelinePoolStats(t *testing.T) { t.Log("PoolStats includes pipeline pool stats correctly") } -// TestNoPipelinePoolStats verifies that PoolStats works without pipeline pool +// TestNoPipelinePoolStats verifies that PoolStats works without pipeline pool. +// The pool is now created by default, so having none requires the explicit +// opt-out (PipelinePoolSize < 0). func TestNoPipelinePoolStats(t *testing.T) { ctx := context.Background() - // Create client WITHOUT custom pipeline buffer sizes + // Opt out of the dedicated pipeline pool: pipelines run on the main pool. client := redis.NewClient(&redis.Options{ - Addr: apTestAddr(), - ReadBufferSize: 64 * 1024, // 64 KiB for all connections - WriteBufferSize: 64 * 1024, // 64 KiB for all connections + Addr: apTestAddr(), + ReadBufferSize: 64 * 1024, // 64 KiB for all connections + WriteBufferSize: 64 * 1024, // 64 KiB for all connections + PipelinePoolSize: -1, }) defer client.Close() skipWithoutRedis(t, ctx, client) diff --git a/pipeline_exec_test.go b/pipeline_exec_test.go index 39c4842963..9c5deb2f6b 100644 --- a/pipeline_exec_test.go +++ b/pipeline_exec_test.go @@ -202,6 +202,17 @@ func TestPipelineRetriesOnNetworkError(t *testing.T) { t.Fatal(err) } + // Warm the DEDICATED PIPELINE POOL connection first: pipelines run there, + // not on the main-pool conn the Ping dialed. Without this the pipeline's + // first Exec dials a fresh pipeline conn and the dial count reads one high + // for a reason unrelated to the retry. + if _, err := client.Pipelined(ctx, func(p redis.Pipeliner) error { + p.Ping(ctx) + return nil + }); err != nil { + t.Fatal(err) + } + dialsBefore := dials.Load() failNextWrite.Store(true) diff --git a/pipeline_pool_gate_test.go b/pipeline_pool_gate_test.go new file mode 100644 index 0000000000..7abb5b2efd --- /dev/null +++ b/pipeline_pool_gate_test.go @@ -0,0 +1,294 @@ +package redis + +import ( + "context" + "os" + "strings" + "sync" + "testing" + "time" +) + +// gateTestAddr mirrors autopipeline_test.go's apTestAddr, which lives in the +// external test package (redis_test) and is unreachable from here. +func gateTestAddr() string { + if p := os.Getenv("REDIS_PORT"); p != "" { + return ":" + p + } + return ":6379" +} + +// TestPipelinePoolAlwaysCreated pins the creation rule: the dedicated pipeline +// pool is built unconditionally at NewClient — like the pubsub pool — because +// it is pure burst capacity (no pre-dialing, small cap, larger buffers) and an +// unused one holds zero connections. Pipelines therefore never compete with +// regular commands for main-pool connections by default. A negative +// PipelinePoolSize is the explicit opt-out and restores the old +// pipelines-on-the-main-pool behavior. +func TestPipelinePoolAlwaysCreated(t *testing.T) { + cases := []struct { + name string + opt *Options + want bool + }{ + { + name: "plain client gets the pool by default", + opt: &Options{Addr: "127.0.0.1:0"}, + want: true, + }, + { + name: "explicit size gets the pool", + opt: &Options{Addr: "127.0.0.1:0", PipelinePoolSize: 8}, + want: true, + }, + { + name: "buffer sizes get the pool", + opt: &Options{Addr: "127.0.0.1:0", PipelineReadBufferSize: 64 * 1024}, + want: true, + }, + { + name: "negative size opts out", + opt: &Options{Addr: "127.0.0.1:0", PipelinePoolSize: -1}, + want: false, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + c := NewClient(tc.opt) + defer c.Close() + if got := c.baseClient.loadPipelinePool() != nil; got != tc.want { + t.Fatalf("pipelinePool present = %v, want %v", got, tc.want) + } + }) + } +} + +// TestPipelinePoolSharedByClones: WithTimeout clones share the parent's pools; +// the pipeline pool must be the SAME pool, not a second one over the same +// server, or the clone pair would hold up to 2x the pipeline connections. +func TestPipelinePoolSharedByClones(t *testing.T) { + c := NewClient(&Options{Addr: "127.0.0.1:0"}) + defer c.Close() + clone := c.WithTimeout(0) + if p, q := c.baseClient.loadPipelinePool(), clone.baseClient.loadPipelinePool(); p == nil || p != q { + t.Fatalf("clone pipeline pool %p != parent %p", q, p) + } +} + +// TestPipelinePoolOptionsResolution pins the rules the dedicated pool is built +// with. The three that matter: +// +// - buffers default to DefaultPipelineBufferSize (not the regular 32 KiB) — +// pipeline connections move whole batches per round trip — but never +// SHRINK a larger explicitly-configured regular buffer; +// - MinIdleConns is forced to 0: the pool is burst capacity, and inheriting +// the main pool's MinIdleConns would pre-dial that many pipeline +// connections at creation, silently doubling the client's idle footprint; +// - the regular buffers and pool size of the MAIN pool are never touched. +func TestPipelinePoolOptionsResolution(t *testing.T) { + t.Run("buffers default to DefaultPipelineBufferSize", func(t *testing.T) { + opt := &Options{Addr: "127.0.0.1:0", PipelinePoolSize: 4} + opt.init() + po := pipelinePoolOptions(opt) + if po.ReadBufferSize != DefaultPipelineBufferSize || po.WriteBufferSize != DefaultPipelineBufferSize { + t.Fatalf("pipeline buffers = %d/%d, want %d each", + po.ReadBufferSize, po.WriteBufferSize, DefaultPipelineBufferSize) + } + if po.PoolSize != 4 { + t.Fatalf("pipeline PoolSize = %d, want 4", po.PoolSize) + } + }) + + t.Run("larger regular buffers are kept, not shrunk", func(t *testing.T) { + opt := &Options{Addr: "127.0.0.1:0", PipelinePoolSize: 4, + ReadBufferSize: 128 * 1024, WriteBufferSize: 128 * 1024} + opt.init() + po := pipelinePoolOptions(opt) + if po.ReadBufferSize != 128*1024 || po.WriteBufferSize != 128*1024 { + t.Fatalf("pipeline buffers = %d/%d, want 131072 each (never shrink)", + po.ReadBufferSize, po.WriteBufferSize) + } + }) + + t.Run("explicit pipeline buffers always win", func(t *testing.T) { + opt := &Options{Addr: "127.0.0.1:0", + PipelineReadBufferSize: 96 * 1024, PipelineWriteBufferSize: 96 * 1024} + opt.init() + po := pipelinePoolOptions(opt) + if po.ReadBufferSize != 96*1024 || po.WriteBufferSize != 96*1024 { + t.Fatalf("pipeline buffers = %d/%d, want 98304 each", + po.ReadBufferSize, po.WriteBufferSize) + } + }) + + t.Run("MinIdleConns is never inherited", func(t *testing.T) { + opt := &Options{Addr: "127.0.0.1:0", PipelinePoolSize: 4, MinIdleConns: 8} + opt.init() + if po := pipelinePoolOptions(opt); po.MinIdleConns != 0 { + t.Fatalf("pipeline MinIdleConns = %d, want 0: the pipeline pool is burst "+ + "capacity and must not pre-dial", po.MinIdleConns) + } + }) + + t.Run("main pool options are untouched", func(t *testing.T) { + opt := &Options{Addr: "127.0.0.1:0", PipelinePoolSize: 4, MinIdleConns: 8} + opt.init() + _ = pipelinePoolOptions(opt) + if opt.ReadBufferSize != 32*1024 || opt.MinIdleConns != 8 { + t.Fatalf("main options mutated: ReadBufferSize=%d MinIdleConns=%d", + opt.ReadBufferSize, opt.MinIdleConns) + } + }) +} + +// TestClusterPipelinePoolSizePropagates: node clients are built from +// clientOptions, which must hand PipelinePoolSize through verbatim — 0 means +// each node client creates the default pool (the always-on rule applies at +// NewClient), and the negative opt-out must survive the copy. +func TestClusterPipelinePoolSizePropagates(t *testing.T) { + for _, tc := range []struct{ in, want int }{{0, 0}, {8, 8}, {-1, -1}} { + co := &ClusterOptions{Addrs: []string{"127.0.0.1:0"}, PipelinePoolSize: tc.in} + co.init() + if got := co.clientOptions().PipelinePoolSize; got != tc.want { + t.Fatalf("PipelinePoolSize %d propagated as %d, want %d", tc.in, got, tc.want) + } + } +} + +// TestFailoverPipelinePoolCreated: NewFailoverClient builds its pools in its +// own constructor (it duplicated — and drifted from — NewClient's creation +// logic once before), so pin the always-on rule and the opt-out there too. +func TestFailoverPipelinePoolCreated(t *testing.T) { + c := NewFailoverClient(&FailoverOptions{ + MasterName: "mymaster", + SentinelAddrs: []string{"127.0.0.1:0"}, + }) + defer c.Close() + if c.baseClient.loadPipelinePool() == nil { + t.Fatal("failover client must create the pipeline pool by default") + } + + optOut := NewFailoverClient(&FailoverOptions{ + MasterName: "mymaster", + SentinelAddrs: []string{"127.0.0.1:0"}, + PipelinePoolSize: -1, + }) + defer optOut.Close() + if optOut.baseClient.loadPipelinePool() != nil { + t.Fatal("PipelinePoolSize < 0 must opt the failover client out") + } +} + +// TestPipelinePoolSpillsToMainPool: the pipeline pool is a small burst-capacity +// pool; a burst of concurrent pipelines wider than its cap must SPILL to the +// main pool instead of queueing behind PoolTimeout. Without the spill, capping +// the pool at PipelinePoolSize would be a silent concurrency regression for +// heavy Pipelined callers, who shared the (much larger) main pool before the +// dedicated pool existed. +func TestPipelinePoolSpillsToMainPool(t *testing.T) { + ctx := context.Background() + c := NewClient(&Options{ + Addr: gateTestAddr(), + PipelinePoolSize: 1, + PoolTimeout: 100 * time.Millisecond, // spill latency bound + }) + defer c.Close() + if err := c.Ping(ctx).Err(); err != nil { + t.Skipf("no redis: %v", err) + } + + // Occupy the single pipeline-pool connection: BLPOP inside a pipeline + // blocks server-side for its timeout, holding the connection busy. + var wg sync.WaitGroup + wg.Add(1) + go func() { + defer wg.Done() + _, _ = c.Pipelined(ctx, func(p Pipeliner) error { + p.BLPop(ctx, time.Second, "pps:never") + return nil + }) + }() + time.Sleep(100 * time.Millisecond) // let the blocker acquire the pipeline conn + + // A second pipeline must not wait out the blocker: it spills to the main + // pool after PoolTimeout (100ms) and completes well under the 1s the + // blocker holds the pipeline conn. + start := time.Now() + cmds, err := c.Pipelined(ctx, func(p Pipeliner) error { + p.Set(ctx, "pps:k", "v", 0) + p.Get(ctx, "pps:k") + return nil + }) + elapsed := time.Since(start) + if err != nil { + t.Fatalf("spilled pipeline failed: %v", err) + } + if got := cmds[1].(*StringCmd).Val(); got != "v" { + t.Fatalf("spilled pipeline GET = %q, want v", got) + } + if elapsed >= time.Second { + t.Fatalf("second pipeline took %v: it queued behind the blocker instead of spilling", elapsed) + } + wg.Wait() +} + +// TestPipelineConnsSkipClientTracking: with the built-in client-side cache +// enabled, pipeline-pool connections must NOT run CLIENT TRACKING ON during +// init. Pipelined commands never consult or populate the cache — only the +// single-command cached path on main-pool connections does — so tracking +// pipeline reads would only grow the server's tracking table and produce +// invalidation pushes for keys the cache does not hold. +func TestPipelineConnsSkipClientTracking(t *testing.T) { + ctx := context.Background() + name := "pipetrackskip" + c := NewClient(&Options{ + Addr: gateTestAddr(), + ClientName: name, + Protocol: 3, + ClientSideCacheConfig: &ClientSideCacheConfig{}, + }) + defer c.Close() + if err := c.Ping(ctx).Err(); err != nil { + t.Skipf("no redis: %v", err) + } + + // One main-pool connection (tracked: CSC is on) and one pipeline-pool + // connection (must not be tracked). + if err := c.Set(ctx, "pts:k", "v", 0).Err(); err != nil { + t.Fatal(err) + } + if _, err := c.Pipelined(ctx, func(p Pipeliner) error { + p.Get(ctx, "pts:k") + p.Get(ctx, "pts:k2") + return nil + }); err != nil && err != Nil { // pts:k2 does not exist; Nil is the expected outcome + t.Fatal(err) + } + + list, err := c.ClientList(ctx).Result() + if err != nil { + t.Fatal(err) + } + tracked, untracked := 0, 0 + for _, line := range strings.Split(list, "\n") { + if !strings.Contains(line, "name="+name) { + continue + } + for _, f := range strings.Fields(line) { + if strings.HasPrefix(f, "flags=") { + if strings.Contains(strings.TrimPrefix(f, "flags="), "t") { + tracked++ + } else { + untracked++ + } + } + } + } + if tracked < 1 { + t.Fatalf("tracked=%d: CSC main-pool connections must run CLIENT TRACKING", tracked) + } + if untracked < 1 { + t.Fatalf("untracked=%d: the pipeline-pool connection must NOT be tracked", untracked) + } +} diff --git a/redis.go b/redis.go index 8883a1adfd..021b8b5631 100644 --- a/redis.go +++ b/redis.go @@ -350,6 +350,17 @@ func (h *onCloseHooks) run() error { return firstErr } +// pipelinePoolRef bundles the dedicated pipeline pool with the pool name its +// connections carry (pool.Conn.PoolName()), so poolForConn can route a +// connection back to the pool that owns it — e.g. streaming-credentials +// re-auth must close/account a failed pipeline connection against the +// pipeline pool, not connPool. Bundling keeps the pair consistent under the +// single atomic publication in baseClient.pipelinePool. +type pipelinePoolRef struct { + pool *pool.ConnPool + name string +} + type baseClient struct { // apClosed flips when the shared pools begin closing; every wrapper and // every clone SHARING those pools refuses to build a new autopipeliner @@ -361,16 +372,18 @@ type baseClient struct { optLock sync.RWMutex connPool pool.Pooler pubSubPool *pool.PubSubPool - // pipelinePool is an optional separate connection pool for pipelining - // operations, used when PipelineReadBufferSize/PipelineWriteBufferSize is - // set so pipelines can use large buffers without bloating the main pool. - // nil means pipelines use connPool. - pipelinePool pool.Pooler - // pipelinePoolName is the pool name assigned to pipelinePool's connections - // (pool.Conn.PoolName()). It lets poolForConn route a connection back to the - // pool that owns it — e.g. so streaming-credentials re-auth closes/accounts a - // failed pipeline connection against pipelinePool, not connPool. - pipelinePoolName string + // pipelinePool is the dedicated connection pool for pipelining + // operations (Pipeline, TxPipeline and autopipeline batches), created + // unconditionally at NewClient/NewFailoverClient — like pubSubPool — with + // pipeline-appropriate options (see pipelinePoolOptions): larger buffers, + // no pre-dialing, a small connection cap. It is pure burst capacity: an + // unused pipeline pool holds zero connections. PipelinePoolSize < 0 opts + // out; nil means pipelines use connPool (opt-out, and the internal + // Conn/Tx/Sentinel wrappers which never create one). The field is set + // before the client is visible to any goroutine and never mutated after, + // so plain reads are safe; WithTimeout clones copy the pointer and share + // the pool. + pipelinePool *pipelinePoolRef hooksMixin // onClose holds named callbacks invoked when the client is closed. @@ -443,11 +456,13 @@ func (c *baseClient) clone() *baseClient { c.maintNotificationsManagerLock.RUnlock() clone := &baseClient{ - apClosed: c.apClosed, - opt: c.opt, - connPool: c.connPool, + apClosed: c.apClosed, + opt: c.opt, + connPool: c.connPool, + // Pointer copy on purpose: the clone shares the parent's pipeline-pool + // SLOT, so a lazy creation through either is visible to both and they + // cannot build two different pipeline pools over one shared pool set. pipelinePool: c.pipelinePool, - pipelinePoolName: c.pipelinePoolName, pubSubPool: c.pubSubPool, onClose: c.onClose, pushProcessor: c.pushProcessor, @@ -560,17 +575,96 @@ func (c *baseClient) initPooledConn(ctx context.Context, p pool.Pooler, cn *pool return nil } +// loadPipelinePool returns the pipeline-pool ref, or nil when pipelines use +// the main pool (PipelinePoolSize < 0, or an internal wrapper client). +func (c *baseClient) loadPipelinePool() *pipelinePoolRef { + return c.pipelinePool +} + +// getPipelinePool returns the dedicated pipeline pool as a pool.Pooler, or a +// true nil interface when there is none. Callers must use this rather than +// wrapping loadPipelinePool().pool themselves where a pool.Pooler is expected: +// a typed-nil *pool.ConnPool inside the interface would defeat `!= nil` checks. +func (c *baseClient) getPipelinePool() pool.Pooler { + if ref := c.loadPipelinePool(); ref != nil { + return ref.pool + } + return nil +} + +// isPipelinePoolConn reports whether cn was dialed by the dedicated pipeline +// pool, identified by the pool name its connections carry. +func (c *baseClient) isPipelinePoolConn(cn *pool.Conn) bool { + ref := c.loadPipelinePool() + return ref != nil && cn.PoolName() == ref.name +} + // poolForConn returns the pool that owns cn — the dedicated pipeline pool when // cn was dialed there, otherwise the main pool. Re-auth close/accounting must // target the owning pool so a failed pipeline connection is removed from the // pipeline pool's books, not the main pool's. func (c *baseClient) poolForConn(cn *pool.Conn) pool.Pooler { - if c.pipelinePool != nil && c.pipelinePoolName != "" && cn.PoolName() == c.pipelinePoolName { - return c.pipelinePool + if ref := c.loadPipelinePool(); ref != nil && cn.PoolName() == ref.name { + return ref.pool } return c.connPool } +// pipelinePoolOptions resolves the Options the dedicated pipeline pool is +// built with. Pure function of the client options, so the resolution rules are +// testable without dialing anything: +// +// - Buffers: the explicit pipeline buffer size when set; otherwise the +// LARGER of the regular buffer size and DefaultPipelineBufferSize. +// Pipelines move whole batches per round trip, so their connections earn +// bigger buffers than regular per-command traffic (measured: throughput +// plateaus around 64 KiB and very large buffers can regress it). The +// RESP3 minimum clamp applies as on the main pool. +// - PoolSize: PipelinePoolSize when set, DefaultPipelinePoolSize otherwise. +// - MinIdleConns: always 0. The pipeline pool is burst capacity — its +// connections dial on demand and there is nothing to keep warm before +// the first pipeline runs. Without this the clone would inherit the +// main pool's MinIdleConns and pre-dial that many pipeline connections +// at creation, silently doubling a client's idle footprint. +func pipelinePoolOptions(opt *Options) *Options { + pipelineOpt := opt.clone() + if opt.PipelineReadBufferSize > 0 { + pipelineOpt.ReadBufferSize = opt.PipelineReadBufferSize + } else if pipelineOpt.ReadBufferSize < DefaultPipelineBufferSize { + pipelineOpt.ReadBufferSize = DefaultPipelineBufferSize + } + // Same clamp Options.init applies to the main pool: RESP3 push parsing + // needs a minimum read buffer, and a tiny pipeline reader would break + // push-notification handling on pipeline conns. + if pipelineOpt.Protocol == 3 && pipelineOpt.ReadBufferSize < proto.MinRESP3ReadBufferSize { + pipelineOpt.ReadBufferSize = proto.MinRESP3ReadBufferSize + } + if opt.PipelineWriteBufferSize > 0 { + pipelineOpt.WriteBufferSize = opt.PipelineWriteBufferSize + } else if pipelineOpt.WriteBufferSize < DefaultPipelineBufferSize { + pipelineOpt.WriteBufferSize = DefaultPipelineBufferSize + } + if opt.PipelinePoolSize > 0 { + pipelineOpt.PoolSize = opt.PipelinePoolSize + } else { + pipelineOpt.PoolSize = DefaultPipelinePoolSize + } + pipelineOpt.MinIdleConns = 0 + return pipelineOpt +} + +// buildPipelinePool constructs the dedicated pipeline pool from the client's +// options as resolved by pipelinePoolOptions. Shared by the NewClient and +// NewFailoverClient creation paths and the lazy ensurePipelinePool path so +// they cannot drift. +func (c *baseClient) buildPipelinePool(poolName string) (*pipelinePoolRef, error) { + p, err := newConnPool(pipelinePoolOptions(c.opt), c.dialHook, poolName) + if err != nil { + return nil, err + } + return &pipelinePoolRef{pool: p, name: poolName}, nil +} + func (c *baseClient) reAuthConnection() func(poolCn *pool.Conn, credentials auth.Credentials) error { return func(poolCn *pool.Conn, credentials auth.Credentials) error { var err error @@ -828,7 +922,13 @@ func (c *baseClient) initConn(ctx context.Context, cn *pool.Conn) error { // drainer. Once CSC serving stops (owner Close, GC cleanup, or drainer // damping), new and re-inited conns skip tracking — nothing consumes the // pushes into the cache anymore. - trackingEnabled := !helloFallbackToRESP2 && !cn.IsPubSub() && c.cscTrackingRequested() + // Pipeline-pool connections are excluded from CLIENT TRACKING: pipelined + // commands never consult or populate the client-side cache (only the + // single-command cached path on main-pool connections does), so tracking + // reads made on pipeline connections would only grow the server's tracking + // table and produce invalidation pushes for keys the cache does not hold. + trackingEnabled := !helloFallbackToRESP2 && !cn.IsPubSub() && c.cscTrackingRequested() && + !c.isPipelinePoolConn(cn) if trackingEnabled && c.cscConnInitGen(cn.GetID()) == 0 { // First initialization establishes generation 1. Reinitialization // already bumped and evicted through onCscReinit before replacing the @@ -1101,9 +1201,13 @@ func (c *baseClient) withPipelineConn( ctx context.Context, fn func(context.Context, *pool.Conn) error, ) (retErr error) { // Use pipeline pool if available, otherwise fall back to regular pool. - if c.pipelinePool == nil { + // Load the ref once: a lazy ensurePipelinePool may publish concurrently, + // and every use below must see the same pool. + ref := c.loadPipelinePool() + if ref == nil { return c.withConn(ctx, fn) } + pipelinePool := ref.pool // Honor the Limiter on the dedicated pipeline-pool path too, mirroring // getConn/releaseConn: Allow() before acquiring and ReportResult() on every @@ -1131,17 +1235,27 @@ func (c *baseClient) withPipelineConn( c.opt.Limiter.ReportResult(retErr) } if cn != nil { - c.releaseConnToPool(ctx, c.pipelinePool, cn, fnErr) + c.releaseConnToPool(ctx, pipelinePool, cn, fnErr) } }() - cn, retErr = c.pipelinePool.Get(ctx) + cn, retErr = pipelinePool.Get(ctx) if retErr != nil { cn = nil // nothing acquired: no release, but still report above + if errors.Is(retErr, pool.ErrPoolTimeout) { + // The pipeline pool is a small burst-capacity pool; under a burst + // of concurrent pipelines wider than its cap, SPILL to the main + // pool instead of failing. Spilled pipelines run with the regular + // buffer sizes — a throughput detail, not a behavior change — and + // total connections stay bounded by PoolSize + PipelinePoolSize. + // This keeps the pre-pipeline-pool capacity for heavy concurrent + // Pipelined callers, who previously shared the main pool. + return c.withConn(ctx, fn) + } return retErr } - if err := c.initPooledConn(ctx, c.pipelinePool, cn); err != nil { + if err := c.initPooledConn(ctx, pipelinePool, cn); err != nil { // initPooledConn already removed the conn from the pool on failure. cn = nil retErr = err @@ -1503,8 +1617,8 @@ func (c *baseClient) enableMaintNotificationsUpgrades() error { // maintnotifications hook to it as well. Otherwise autopipelined/pipelined // commands run on pipeline-pool connections that never receive MOVING/ // MIGRATING handoff handling. - if c.pipelinePool != nil { - manager.InitPoolHookForPool(c.pipelinePool, c.dialHook) + if pp := c.getPipelinePool(); pp != nil { + manager.InitPoolHookForPool(pp, c.dialHook) } return nil } @@ -1564,15 +1678,15 @@ func (c *baseClient) closeResources() error { } // Unregister pools from OTel before closing them - otel.UnregisterPools(c.connPool, c.pubSubPool, c.pipelinePool) + otel.UnregisterPools(c.connPool, c.pubSubPool, c.getPipelinePool()) if c.connPool != nil { if err := c.connPool.Close(); err != nil && firstErr == nil { firstErr = err } } - if c.pipelinePool != nil { - if err := c.pipelinePool.Close(); err != nil && firstErr == nil { + if pp := c.getPipelinePool(); pp != nil { + if err := pp.Close(); err != nil && firstErr == nil { firstErr = err } } @@ -1958,41 +2072,25 @@ func NewClient(opt *Options) *Client { panic(fmt.Errorf("redis: failed to create pubsub pool: %w", err)) } - // Optionally create a separate connection pool for pipelining, with its own - // (typically larger) buffers, so pipelines can use big buffers without - // bloating the main pool. Enabled when either pipeline buffer size is set. - if opt.PipelineReadBufferSize > 0 || opt.PipelineWriteBufferSize > 0 { - pipelineOpt := opt.clone() - if opt.PipelineReadBufferSize > 0 { - pipelineOpt.ReadBufferSize = opt.PipelineReadBufferSize - // Same clamp Options.init applies to the main pool: RESP3 push - // parsing needs a minimum read buffer, and a tiny pipeline reader - // would break push-notification handling on pipeline conns. - if pipelineOpt.Protocol == 3 && pipelineOpt.ReadBufferSize < proto.MinRESP3ReadBufferSize { - pipelineOpt.ReadBufferSize = proto.MinRESP3ReadBufferSize - } - } - if opt.PipelineWriteBufferSize > 0 { - pipelineOpt.WriteBufferSize = opt.PipelineWriteBufferSize - } - if opt.PipelinePoolSize > 0 { - pipelineOpt.PoolSize = opt.PipelinePoolSize - } else { - pipelineOpt.PoolSize = 10 // default smaller pool for pipelining - } - pipelinePoolName := opt.Addr + "_" + uniqueID + "_pipeline" - c.pipelinePoolName = pipelinePoolName - c.pipelinePool, err = newConnPool(pipelineOpt, c.dialHook, pipelinePoolName) + // Create the dedicated pipeline pool unconditionally, like pubSubPool: it + // is pure burst capacity (no pre-dialing, small cap, larger buffers — see + // pipelinePoolOptions), so an unused pipeline pool holds zero connections + // and costs nothing. Pipelines stop competing with regular commands for + // main-pool connections; a burst wider than the pool's cap spills back to + // the main pool (see withPipelineConn). PipelinePoolSize < 0 opts out. + if opt.PipelinePoolSize >= 0 { + ref, err := c.buildPipelinePool(opt.Addr + "_" + uniqueID + "_pipeline") if err != nil { panic(fmt.Errorf("redis: failed to create pipeline connection pool: %w", err)) } + c.pipelinePool = ref } if opt.StreamingCredentialsProvider != nil { c.streamingCredentialsManager = streaming.NewManager(c.connPool, c.opt.PoolTimeout) c.connPool.AddPoolHook(c.streamingCredentialsManager.PoolHook()) - if c.pipelinePool != nil { - c.pipelinePool.AddPoolHook(c.streamingCredentialsManager.PoolHook()) + if pp := c.getPipelinePool(); pp != nil { + pp.AddPoolHook(c.streamingCredentialsManager.PoolHook()) } } @@ -2038,7 +2136,7 @@ func NewClient(opt *Options) *Client { // Register pools with OTel recorder if it supports pool registration // This allows async gauge metrics to pull stats from pools periodically - otel.RegisterPools(c.connPool, c.pubSubPool, c.pipelinePool, opt.Addr) + otel.RegisterPools(c.connPool, c.pubSubPool, c.getPipelinePool(), opt.Addr) return &c } @@ -2210,8 +2308,8 @@ type PoolStats pool.Stats func (c *Client) PoolStats() *PoolStats { stats := c.connPool.Stats() stats.PubSubStats = *c.pubSubPool.Stats() - if c.pipelinePool != nil { - stats.PipelineStats = c.pipelinePool.Stats() + if pp := c.getPipelinePool(); pp != nil { + stats.PipelineStats = pp.Stats() } return (*PoolStats)(stats) } diff --git a/ring.go b/ring.go index a22e166724..de4f32df3e 100644 --- a/ring.go +++ b/ring.go @@ -150,7 +150,8 @@ type RingOptions struct { // 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. The pool is created only when PipelineReadBufferSize or PipelineWriteBufferSize is set (PipelinePoolSize alone does not enable it). + // fields on Options for details. Setting any of the three creates the pool + // on each shard client. PipelineReadBufferSize int PipelineWriteBufferSize int PipelinePoolSize int diff --git a/sentinel.go b/sentinel.go index 5720fc476b..13a5c93a1a 100644 --- a/sentinel.go +++ b/sentinel.go @@ -19,7 +19,6 @@ import ( "github.com/redis/go-redis/v9/internal" "github.com/redis/go-redis/v9/internal/otel" "github.com/redis/go-redis/v9/internal/pool" - "github.com/redis/go-redis/v9/internal/proto" "github.com/redis/go-redis/v9/maintnotifications" "github.com/redis/go-redis/v9/push" ) @@ -129,7 +128,8 @@ type FailoverOptions struct { // 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. The pool is created only when PipelineReadBufferSize or PipelineWriteBufferSize is set (PipelinePoolSize alone does not enable it). + // for details. Setting any of the three creates the pool; it also defaults + // in when AutoPipelineOptions is set. PipelineReadBufferSize int PipelineWriteBufferSize int PipelinePoolSize int @@ -588,39 +588,21 @@ func NewFailoverClient(failoverOpt *FailoverOptions) *Client { panic(fmt.Errorf("redis: failed to create pubsub pool: %w", err)) } - // Optionally create a separate connection pool for pipelining, with its own - // (typically larger) buffers. Enabled when either pipeline buffer size is set. - if opt.PipelineReadBufferSize > 0 || opt.PipelineWriteBufferSize > 0 { - pipelineOpt := opt.clone() - if opt.PipelineReadBufferSize > 0 { - pipelineOpt.ReadBufferSize = opt.PipelineReadBufferSize - // Same clamp Options.init applies to the main pool: RESP3 push - // parsing needs a minimum read buffer, and a tiny pipeline reader - // would break push-notification handling on pipeline conns. - if pipelineOpt.Protocol == 3 && pipelineOpt.ReadBufferSize < proto.MinRESP3ReadBufferSize { - pipelineOpt.ReadBufferSize = proto.MinRESP3ReadBufferSize - } - } - if opt.PipelineWriteBufferSize > 0 { - pipelineOpt.WriteBufferSize = opt.PipelineWriteBufferSize - } - if opt.PipelinePoolSize > 0 { - pipelineOpt.PoolSize = opt.PipelinePoolSize - } else { - pipelineOpt.PoolSize = 10 // default smaller pool for pipelining - } - rdb.pipelinePoolName = mainPoolName + "_pipeline" - rdb.pipelinePool, err = newConnPool(pipelineOpt, rdb.dialHook, rdb.pipelinePoolName) + // Create the dedicated pipeline pool unconditionally, mirroring NewClient + // via the shared buildPipelinePool helper. PipelinePoolSize < 0 opts out. + if opt.PipelinePoolSize >= 0 { + ref, err := rdb.buildPipelinePool(mainPoolName + "_pipeline") if err != nil { panic(fmt.Errorf("redis: failed to create pipeline connection pool: %w", err)) } + rdb.pipelinePool = ref } // Register pools for OTel async gauge metrics, matching NewClient (the // failover client previously registered none, so pool gauges were silent // for the identical standalone setup). The pipeline pool is nil when not // configured. - otel.RegisterPools(rdb.connPool, rdb.pubSubPool, rdb.pipelinePool, opt.Addr) + otel.RegisterPools(rdb.connPool, rdb.pubSubPool, rdb.getPipelinePool(), opt.Addr) rdb.onClose.register(onCloseHookIDSentinelFailover, failover.Close) @@ -633,8 +615,10 @@ func NewFailoverClient(failoverOpt *FailoverOptions) *Client { } // Drop stale pipeline-pool connections dialed to the demoted master too; // otherwise pipelined traffic keeps using the old address after failover. - if pipelinePool, ok := rdb.pipelinePool.(*pool.ConnPool); ok { - _ = pipelinePool.Filter(func(cn *pool.Conn) bool { + // Loaded through the atomic ref: the pool may have been created lazily + // by ensurePipelinePool after this callback was registered. + if ref := rdb.loadPipelinePool(); ref != nil { + _ = ref.pool.Filter(func(cn *pool.Conn) bool { return cn.RemoteAddr().String() != addr }) }