diff --git a/limits.go b/limits.go index c2a7144..65f2b1a 100644 --- a/limits.go +++ b/limits.go @@ -13,6 +13,19 @@ import ( // the log with one line per request. const storeErrorLogEvery = 30 * time.Second +// storeDownLatchFor bounds how long the limiter avoids a configured store +// after any operation against it fails: for this long after the failure, +// every store operation short-circuits straight to the fail-open/ +// fail-closed policy without attempting the network call at all. Each +// respClient call is already bounded to ~respCallTimeout on its own +// (resp.go), but one request can still touch several counters (its own +// and its group's, request/token/cost, day/month) — without this latch, +// every one of those pays a fresh bounded wait against a store that just +// told the previous op it was unreachable. After the window, the next +// operation probes the real store again: a natural half-open retry, no +// separate recovery state needed. +const storeDownLatchFor = 5 * time.Second + // Window identifiers used throughout windowKey, counter metrics, and // retry-after calculation. const ( @@ -236,13 +249,14 @@ type limitViolation struct { // limiter enforces per-minute/day/month request, token, and cost limits // using fixed windows keyed by windowKey. type limiter struct { - store counterStore // configured backend; nil means always use fallback (see newLimiter) - fallback *memoryStore // in-process counter store, always available - nowFn func() time.Time // injected for tests; defaults to time.Now - logf func(format string, args ...any) // injectable store-error log; defaults to a no-op - lastLogAt time.Time // guarded by logMu; last time a store error was logged - logMu sync.Mutex - failOpen bool // store-error policy: true falls back to fallback, false refuses the request + store counterStore // configured backend; nil means always use fallback (see newLimiter) + fallback *memoryStore // in-process counter store, always available + nowFn func() time.Time // injected for tests; defaults to time.Now + logf func(format string, args ...any) // injectable store-error log; defaults to a no-op + lastLogAt time.Time // guarded by logMu; last time a store error was logged + lastStoreFailure time.Time // guarded by logMu; zero means the store-down latch is not open (see storeLatched) + logMu sync.Mutex + failOpen bool // store-error policy: true falls back to fallback, false refuses the request } // newLimiter returns a limiter. A nil store means every operation uses the @@ -283,45 +297,88 @@ func (l *limiter) logStoreError(err error) { } } +// storeLatched reports whether l is within storeDownLatchFor of its last +// recorded store failure. When true, storeIncrBy/storeGet must skip the +// network call entirely and go straight to the fail-open/fail-closed +// policy — the same outcome a fresh call would reach anyway, just without +// paying respCallTimeout again to find out. Guarded by logMu, the same +// mutex lastLogAt already uses for this kind of small timestamp state. +func (l *limiter) storeLatched() bool { + l.logMu.Lock() + defer l.logMu.Unlock() + return !l.lastStoreFailure.IsZero() && l.now().Sub(l.lastStoreFailure) < storeDownLatchFor +} + +// recordStoreFailure logs err (rate-limited, see logStoreError) and opens +// the store-down latch: every storeIncrBy/storeGet call for the next +// storeDownLatchFor skips the network call and applies the fail-open/ +// fail-closed policy directly. +func (l *limiter) recordStoreFailure(err error) { + l.logStoreError(err) + l.logMu.Lock() + l.lastStoreFailure = l.now() + l.logMu.Unlock() +} + // storeIncrBy increments key by n with the given ttl, applying the -// limiter's fail-open/fail-closed policy when a configured store errors. -// ok is false only when the store errored and failOpen is false — the -// fail-closed case a caller must refuse the request for, rather than -// treating a zero value as a real counter reading. +// limiter's fail-open/fail-closed policy when a configured store errors +// or when the store-down latch (storeLatched) is already open from a +// recent failure. ok is false only in the fail-closed case — a caller +// must refuse the request for that, rather than treating a zero value as +// a real counter reading. func (l *limiter) storeIncrBy(key string, n int64, ttl time.Duration) (v int64, ok bool) { if l.store == nil { v, _ = l.fallback.incrBy(key, n, ttl) // fallback never errors return v, true } + if l.storeLatched() { + return l.failPolicyIncrBy(key, n, ttl) + } v, err := l.store.incrBy(key, n, ttl) if err == nil { return v, true } - l.logStoreError(err) - if !l.failOpen { - return 0, false - } - v, _ = l.fallback.incrBy(key, n, ttl) - return v, true + l.recordStoreFailure(err) + return l.failPolicyIncrBy(key, n, ttl) } -// storeGet mirrors storeIncrBy for a read: it applies the same fail-open -// (transparently reads the fallback) / fail-closed (ok=false) policy on a -// configured store's error. +// storeGet mirrors storeIncrBy for a read: it applies the same fail-open/ +// fail-closed/latched handling on a configured store's error. func (l *limiter) storeGet(key string) (v int64, ok bool) { if l.store == nil { v, _ = l.fallback.get(key) return v, true } + if l.storeLatched() { + return l.failPolicyGet(key) + } v, err := l.store.get(key) if err == nil { return v, true } - l.logStoreError(err) + l.recordStoreFailure(err) + return l.failPolicyGet(key) +} + +// failPolicyIncrBy applies the limiter's fail-open/fail-closed policy for +// an incrBy the caller has decided not to attempt against the configured +// store (it just failed, or the store-down latch is open): failOpen=true +// counts key in the fallback instead; failOpen=false reports the +// operation as failed. +func (l *limiter) failPolicyIncrBy(key string, n int64, ttl time.Duration) (int64, bool) { if !l.failOpen { return 0, false } - v, _ = l.fallback.get(key) + v, _ := l.fallback.incrBy(key, n, ttl) + return v, true +} + +// failPolicyGet mirrors failPolicyIncrBy for a read. +func (l *limiter) failPolicyGet(key string) (int64, bool) { + if !l.failOpen { + return 0, false + } + v, _ := l.fallback.get(key) return v, true } diff --git a/limits_test.go b/limits_test.go index 7537f49..245d445 100644 --- a/limits_test.go +++ b/limits_test.go @@ -526,6 +526,68 @@ func TestLimiter_LogsStoreErrorOncePerRateLimit(t *testing.T) { } } +// countingErrorStore is a counterStore stub that always errors and counts +// how many times each method is actually invoked. It proves the +// store-down latch (limits.go) skips the network call entirely during +// its window, rather than merely tolerating the error each time. +type countingErrorStore struct { + err error + incrCalls int + getCalls int +} + +func (s *countingErrorStore) incrBy(string, int64, time.Duration) (int64, error) { + s.incrCalls++ + return 0, s.err +} + +func (s *countingErrorStore) get(string) (int64, error) { + s.getCalls++ + return 0, s.err +} + +// TestLimiter_StoreDownLatch_SkipsStoreCallsWithinWindow is review-round-3 +// item 2: after a store operation fails, every operation for the next +// storeDownLatchFor must short-circuit straight to the fail-open policy +// without calling the store at all — a request that touches several +// counters (min/day/month, tokens/cost, user/group) must not pay a fresh +// bounded wait against a store that just proved unreachable for each one. +// failOpen=true is used so checkAndCount completes both its req:min and +// req:day increments per call instead of short-circuiting after the +// first — this is what makes the "zero additional store calls" assertion +// meaningful, rather than trivially true because checkAndCount stops +// after one op regardless of the latch. +func TestLimiter_StoreDownLatch_SkipsStoreCallsWithinWindow(t *testing.T) { + store := &countingErrorStore{err: errors.New("boom")} + l := newLimiter(store, true) + now := time.Date(2026, 8, 20, 10, 0, 0, 0, time.UTC) + l.nowFn = func() time.Time { return now } + scopes := []limitScope{{kind: "user", id: "u", limits: nil}} + + if v := l.checkAndCount(scopes); v != nil { + t.Fatalf("want no violation with failOpen=true, got %+v", v) + } + firstCalls := store.incrCalls + if firstCalls == 0 { + t.Fatal("want the first call to actually reach the store at least once") + } + + if v := l.checkAndCount(scopes); v != nil { + t.Fatalf("want no violation with failOpen=true, got %+v", v) + } + if store.incrCalls != firstCalls { + t.Errorf("incrBy calls = %d after a second checkAndCount within the latch window, want unchanged at %d (the store must not be called)", store.incrCalls, firstCalls) + } + + now = now.Add(storeDownLatchFor) // latch expires + if v := l.checkAndCount(scopes); v != nil { + t.Fatalf("want no violation with failOpen=true, got %+v", v) + } + if store.incrCalls <= firstCalls { + t.Errorf("incrBy calls = %d after the latch window elapsed, want more than %d (the store must be probed again)", store.incrCalls, firstCalls) + } +} + // TestLimitsConfigValidate is carried-item (a): validate rejects // NaN/±Inf/negative cost limits and negative int64 count limits, accepts // zero/positive values, and treats a nil receiver (no limits configured diff --git a/redis_store_test.go b/redis_store_test.go index 3dfce1b..49c497e 100644 --- a/redis_store_test.go +++ b/redis_store_test.go @@ -138,14 +138,17 @@ func TestLimiter_FailOpenTrue_DeadRedisAddress_FallsBackAndAllows(t *testing.T) } // TestLimiter_FailOpenTrue_HungRedisServer_FallsBackQuickly is review item -// 1's limiter-level proof: a hung (accepts the connection, never replies) -// Redis-compatible server must not make a failOpen=true request wait out -// several stacked multi-second timeouts (the pre-fix behavior for a -// checkAndCount call that performs several store operations) — each of -// checkAndCount's two store ops here (req:min, req:day) is bounded by one -// respCallTimeout, not respCallTimeout times every read/write step it -// happens to perform, so the whole call stays well under the old -// multi-times-respCallTimeout stall. +// 1's limiter-level proof (round 1), tightened by the round-3 store-down +// latch: a hung (accepts the connection, never replies) Redis-compatible +// server must not make a failOpen=true request wait out several stacked +// multi-second timeouts (the pre-fix behavior for a checkAndCount call +// that performs several store operations). checkAndCount's two store ops +// here (req:min, req:day) would each independently be bounded by one +// respCallTimeout — already a large improvement over the old +// per-operation-multiplied stall — but the store-down latch (limits.go) +// goes further: the first op's failure latches the store down, so the +// second op skips the network call entirely and the whole call costs +// only ~one respCallTimeout, not two. func TestLimiter_FailOpenTrue_HungRedisServer_FallsBackQuickly(t *testing.T) { ln := newHungListener(t) store := newRedisStore(newRESPClient(ln.Addr().String(), "", 0)) @@ -159,7 +162,7 @@ func TestLimiter_FailOpenTrue_HungRedisServer_FallsBackQuickly(t *testing.T) { if v != nil { t.Fatalf("want no violation with failOpen=true and a hung store, got %+v", v) } - if elapsed >= 6*time.Second { - t.Fatalf("elapsed = %v, want well under the old worst case (two bounded store ops, not many stacked timeouts)", elapsed) + if elapsed >= 3*time.Second { + t.Fatalf("elapsed = %v, want < 3s (one respCallTimeout for the first op, the second latched and skipping the network entirely)", elapsed) } } diff --git a/resp.go b/resp.go index 14324d7..14a17fd 100644 --- a/resp.go +++ b/resp.go @@ -36,11 +36,11 @@ const ( // actually reads (integer counter values and short simple/error strings). const respMaxBulkLen = 64 << 20 -// respMaxArrayLen caps a RESP2 array's declared element count, for the -// same reason as respMaxBulkLen: this client only ever pipelines a -// handful of commands, so a reply claiming millions of elements is -// malformed or hostile, not legitimate. -const respMaxArrayLen = 1 << 20 +// respMaxArrayLen caps a RESP2 array's declared element count. None of +// this client's commands (AUTH, SELECT, INCRBY, EXPIRE, GET) ever return +// an array reply, so 1024 is already generous headroom, not a limit tuned +// to real traffic. +const respMaxArrayLen = 1024 // respMaxLineLen caps a single RESP2 header line (the "+", "-", ":", "$", // or "*" line preceding any body) read by readLine. Without a cap, a peer @@ -137,6 +137,32 @@ func isTimeout(err error) bool { return errors.As(err, &netErr) && netErr.Timeout() } +// dialTimeoutFor returns the duration a dial starting now should use to +// avoid running past deadline: the smaller of respDialTimeout and the +// time remaining until deadline. Without this clamp, a dial always +// spending its own full respDialTimeout regardless of deadline would let +// a future change to respDialTimeout alone (leaving respCallTimeout +// untouched) silently widen how much of one call's budget a stalled dial +// can consume. +func dialTimeoutFor(deadline time.Time) time.Duration { + if remaining := time.Until(deadline); remaining < respDialTimeout { + return remaining + } + return respDialTimeout +} + +// respDeadlineExceededErr signals that a call's deadline had already +// elapsed before an attempt could even dial — e.g. this call queued +// behind respClient's mutex for the whole budget while an earlier call +// was stuck. It implements net.Error so pipeline's isTimeout check treats +// it the same as any other timed-out I/O: no retry, since retrying an +// already-elapsed deadline can only fail the same way again. +type respDeadlineExceededErr struct{} + +func (respDeadlineExceededErr) Error() string { return "resp: call deadline already elapsed" } +func (respDeadlineExceededErr) Timeout() bool { return true } +func (respDeadlineExceededErr) Temporary() bool { return true } + // attemptPipelineLocked runs one full attempt of cmds over c's connection, // connecting first if needed, with every read and write bound by // deadline. Callers must hold c.mu. @@ -155,7 +181,7 @@ func (c *respClient) attemptPipelineLocked(cmds [][]string, deadline time.Time) replies := make([]any, len(cmds)) for i := range cmds { - v, err := decodeReply(c.r) + v, err := decodeReply(c.r, 0) if err != nil { return nil, fmt.Errorf("resp: read reply %d/%d: %w", i+1, len(cmds), err) } @@ -177,7 +203,11 @@ func (c *respClient) ensureConnLocked(deadline time.Time) error { return nil } - conn, err := net.DialTimeout("tcp", c.addr, respDialTimeout) + dialTimeout := dialTimeoutFor(deadline) + if dialTimeout <= 0 { + return fmt.Errorf("resp: dial %q: %w", c.addr, respDeadlineExceededErr{}) + } + conn, err := net.DialTimeout("tcp", c.addr, dialTimeout) if err != nil { return fmt.Errorf("resp: dial %q: %w", c.addr, err) } @@ -209,7 +239,7 @@ func (c *respClient) handshakeLocked(cmd, arg string) error { if _, err := io.WriteString(c.conn, encodeCommand([]string{cmd, arg})); err != nil { return fmt.Errorf("resp: %s: write: %w", cmd, err) } - v, err := decodeReply(c.r) + v, err := decodeReply(c.r, 0) if err != nil { return fmt.Errorf("resp: %s: read reply: %w", cmd, err) } @@ -241,22 +271,27 @@ func encodeCommand(args []string) string { return b.String() } -// decodeReply reads and decodes one RESP2 reply from r: +// decodeReply reads and decodes one RESP2 reply from r. depth is 0 for a +// top-level reply and depth+1 for each element decodeArray recurses into; +// an array is only accepted at depth 0 (see the '*' case) — none of this +// client's commands (AUTH, SELECT, INCRBY, EXPIRE, GET) ever return a +// nested array, so a server or attacker sending one is malformed input, +// not legitimate traffic worth the recursion: // // - "+simple\r\n" -> string // - "-error\r\n" -> respErr (implements error; not returned as err) // - ":123\r\n" -> int64 // - "$len\r\n...\r\n" -> []byte, or nil for a null bulk ("$-1\r\n") // - "*n\r\n..." -> []any of n decoded elements, or nil for a null -// array ("*-1\r\n") +// array ("*-1\r\n"); only at depth 0 // // The returned error is non-nil only for a transport or protocol failure // (short read, malformed line, unknown type byte, an out-of-range or -// mistrailed bulk/array) — a well-formed RESP error reply decodes -// successfully to a respErr value, not to a non-nil error, so a caller -// pipelining several commands can still read every reply after one of -// them fails server-side. -func decodeReply(r *bufio.Reader) (any, error) { +// mistrailed bulk/array, a nested array) — a well-formed RESP error reply +// decodes successfully to a respErr value, not to a non-nil error, so a +// caller pipelining several commands can still read every reply after one +// of them fails server-side. +func decodeReply(r *bufio.Reader, depth int) (any, error) { line, err := readLine(r) if err != nil { return nil, err @@ -280,7 +315,16 @@ func decodeReply(r *bufio.Reader) (any, error) { case '$': return decodeBulk(r, rest) case '*': - return decodeArray(r, rest) + if depth > 0 { + // A nested array reply — "*1\r\n*1\r\n..." repeated — could + // otherwise recurse to Go's stack limit (a fatal, unrecoverable + // error, not a panic recoverPanic could catch) with only a few + // megabytes of crafted wire input. No command this client sends + // ever gets one back, so reject it outright instead of + // recursing. + return nil, fmt.Errorf("resp: nested array reply not supported") + } + return decodeArray(r, rest, depth) default: return nil, fmt.Errorf("resp: unknown reply type %q", prefix) } @@ -318,8 +362,12 @@ func decodeBulk(r *bufio.Reader, lenField string) (any, error) { // decodeArray reads n elements given the already-parsed "*" length field // lenField, returning nil for a null array ("*-1"). It rejects a negative // length other than -1 and a length exceeding respMaxArrayLen before -// allocating, for the same reason as decodeBulk's cap. -func decodeArray(r *bufio.Reader, lenField string) (any, error) { +// allocating, for the same reason as decodeBulk's cap. depth is the +// depth decodeReply was called at for this array's own "*" line (always +// 0 — see decodeReply's '*' case); each element decodes at depth+1, so an +// element that is itself an array is rejected by decodeReply rather than +// recursed into. +func decodeArray(r *bufio.Reader, lenField string, depth int) (any, error) { n, err := strconv.Atoi(lenField) if err != nil { return nil, fmt.Errorf("resp: malformed array length %q: %w", lenField, err) @@ -333,7 +381,7 @@ func decodeArray(r *bufio.Reader, lenField string) (any, error) { arr := make([]any, n) for i := 0; i < n; i++ { - v, err := decodeReply(r) + v, err := decodeReply(r, depth+1) if err != nil { return nil, fmt.Errorf("resp: array element %d/%d: %w", i+1, n, err) } diff --git a/resp_test.go b/resp_test.go index 985b59d..66244f6 100644 --- a/resp_test.go +++ b/resp_test.go @@ -36,10 +36,13 @@ func TestEncodeCommand(t *testing.T) { // TestDecodeReply covers every RESP2 type respClient must decode: simple // string, error, integer, bulk string (including the null bulk "$-1"), and -// array (including the null array "*-1" and nesting). +// a single top-level array (including the null array "*-1"). Every call +// passes depth 0 — a top-level reply, matching every real call site in +// resp.go (attemptPipelineLocked, handshakeLocked, and decodeArray's own +// element loop, which passes depth+1). func TestDecodeReply(t *testing.T) { t.Run("simple string", func(t *testing.T) { - v, err := decodeReply(newReader("+OK\r\n")) + v, err := decodeReply(newReader("+OK\r\n"), 0) if err != nil { t.Fatalf("decodeReply: %v", err) } @@ -49,7 +52,7 @@ func TestDecodeReply(t *testing.T) { }) t.Run("error reply decodes to respErr, not a decode error", func(t *testing.T) { - v, err := decodeReply(newReader("-ERR wrong number of arguments\r\n")) + v, err := decodeReply(newReader("-ERR wrong number of arguments\r\n"), 0) if err != nil { t.Fatalf("decodeReply returned an error for a well-formed RESP error reply: %v", err) } @@ -63,7 +66,7 @@ func TestDecodeReply(t *testing.T) { }) t.Run("integer", func(t *testing.T) { - v, err := decodeReply(newReader(":42\r\n")) + v, err := decodeReply(newReader(":42\r\n"), 0) if err != nil { t.Fatalf("decodeReply: %v", err) } @@ -73,7 +76,7 @@ func TestDecodeReply(t *testing.T) { }) t.Run("negative integer", func(t *testing.T) { - v, err := decodeReply(newReader(":-7\r\n")) + v, err := decodeReply(newReader(":-7\r\n"), 0) if err != nil { t.Fatalf("decodeReply: %v", err) } @@ -83,7 +86,7 @@ func TestDecodeReply(t *testing.T) { }) t.Run("bulk string", func(t *testing.T) { - v, err := decodeReply(newReader("$5\r\nhello\r\n")) + v, err := decodeReply(newReader("$5\r\nhello\r\n"), 0) if err != nil { t.Fatalf("decodeReply: %v", err) } @@ -94,7 +97,7 @@ func TestDecodeReply(t *testing.T) { }) t.Run("null bulk (missing key)", func(t *testing.T) { - v, err := decodeReply(newReader("$-1\r\n")) + v, err := decodeReply(newReader("$-1\r\n"), 0) if err != nil { t.Fatalf("decodeReply: %v", err) } @@ -103,8 +106,8 @@ func TestDecodeReply(t *testing.T) { } }) - t.Run("array", func(t *testing.T) { - v, err := decodeReply(newReader("*2\r\n:5\r\n:1\r\n")) + t.Run("array (flat, not nested)", func(t *testing.T) { + v, err := decodeReply(newReader("*2\r\n:5\r\n:1\r\n"), 0) if err != nil { t.Fatalf("decodeReply: %v", err) } @@ -115,7 +118,7 @@ func TestDecodeReply(t *testing.T) { }) t.Run("null array", func(t *testing.T) { - v, err := decodeReply(newReader("*-1\r\n")) + v, err := decodeReply(newReader("*-1\r\n"), 0) if err != nil { t.Fatalf("decodeReply: %v", err) } @@ -125,12 +128,12 @@ func TestDecodeReply(t *testing.T) { }) t.Run("malformed type byte is a decode error", func(t *testing.T) { - if _, err := decodeReply(newReader("?nope\r\n")); err == nil { + if _, err := decodeReply(newReader("?nope\r\n"), 0); err == nil { t.Fatal("want error for an unknown reply type byte") } }) - // The following four subtests are review item 3: decodeBulk/ + // The following subtests are review item 3 (round 1): decodeBulk/ // decodeArray must reject an out-of-range wire length before // allocating, and decodeBulk must validate its CRLF trailer. @@ -139,25 +142,25 @@ func TestDecodeReply(t *testing.T) { // slice length; make([]byte, n+2) with that unguarded would // panic. Reaching this line without panicking already proves the // fix; the error check confirms it fails cleanly too. - if _, err := decodeReply(newReader("$9223372036854775807\r\n")); err == nil { + if _, err := decodeReply(newReader("$9223372036854775807\r\n"), 0); err == nil { t.Fatal("want an error for a bulk length exceeding respMaxBulkLen") } }) t.Run("bulk length just over the cap is a protocol error", func(t *testing.T) { - if _, err := decodeReply(newReader(fmt.Sprintf("$%d\r\n", respMaxBulkLen+1))); err == nil { + if _, err := decodeReply(newReader(fmt.Sprintf("$%d\r\n", respMaxBulkLen+1)), 0); err == nil { t.Fatal("want an error for a bulk length one over respMaxBulkLen") } }) t.Run("huge array length is a protocol error", func(t *testing.T) { - if _, err := decodeReply(newReader("*1000000000\r\n")); err == nil { + if _, err := decodeReply(newReader("*1000000000\r\n"), 0); err == nil { t.Fatal("want an error for an array length exceeding respMaxArrayLen") } }) t.Run("bulk with wrong trailer is a protocol error", func(t *testing.T) { - if _, err := decodeReply(newReader("$5\r\nhelloXX")); err == nil { + if _, err := decodeReply(newReader("$5\r\nhelloXX"), 0); err == nil { t.Fatal("want an error when a bulk reply's trailing bytes are not CRLF") } }) @@ -167,10 +170,33 @@ func TestDecodeReply(t *testing.T) { // readLine would keep buffering every byte offered until the // reader errors (e.g. EOF) rather than bailing out early. huge := strings.Repeat("x", respMaxLineLen+10) - if _, err := decodeReply(newReader("+" + huge)); err == nil { + if _, err := decodeReply(newReader("+"+huge), 0); err == nil { t.Fatal("want an error for a reply line exceeding respMaxLineLen") } }) + + // The following subtests are review item 1 (round 3): decodeReply + // must reject an array nested inside another array's elements — the + // only way a reply could otherwise recurse — and the array cap must + // be the new, much lower respMaxArrayLen (1024), since no command + // this client sends ever gets an array reply back at all. + + t.Run("nested array reply is a protocol error, not unbounded recursion", func(t *testing.T) { + // A one-element array whose element is itself a one-element array. + // Repeated, this pattern is what would otherwise recurse to Go's + // stack limit (a fatal, unrecoverable error) from only a few + // megabytes of wire input; one level already exercises the guard, + // since decodeReply rejects any '*' at depth > 0 outright. + if _, err := decodeReply(newReader("*1\r\n*1\r\n:1\r\n"), 0); err == nil { + t.Fatal("want an error for an array nested inside another array") + } + }) + + t.Run("array length just over the new (1024) cap is a protocol error", func(t *testing.T) { + if _, err := decodeReply(newReader(fmt.Sprintf("*%d\r\n", respMaxArrayLen+1)), 0); err == nil { + t.Fatal("want an error for an array length one over respMaxArrayLen") + } + }) } // newReader wraps s in a *bufio.Reader, matching what respClient reads