fix(resp): bound store-call latency, cap wire lengths, cover fail-closed budget path

Bound respClient's worst-case latency to one respCallTimeout per
do/pipeline call instead of per read/write/handshake step. Compute a
single absolute deadline at call entry, apply it once via
conn.SetDeadline after connecting (covering the AUTH/SELECT handshake
and the call's own command I/O), and skip the reconnect-retry when the
failing attempt was a timeout (errors.As -> net.Error, Timeout()) — a
peer that accepted the connection and went silent must not be asked
the same non-question twice. Previously each read, write, and dial got
its own fresh 2s budget and every caller queued behind the client's
single mutex, so a hung (not merely refused) Redis-compatible server
could stall one checkAndCount call for 16-32s+.

Cap decodeBulk/decodeArray's wire-declared lengths (respMaxBulkLen
64MiB, respMaxArrayLen 1<<20) before allocating: an unguarded
"$9223372036854775807\r\n" overflows n+2 into a negative slice length
and panics make([]byte, n+2); a merely huge-but-valid length forces an
outsized allocation either way. decodeBulk also now verifies its two
trailing bytes are CRLF. Bound readLine to respMaxLineLen (64KiB),
reading byte-by-byte so it can bail before ever buffering an unbounded
line from a peer that never sends '\n'.

Fix TestLimiter_FailClosed_BudgetReadReturnsStoreDownViolation: the
erroringStore it used fails incrBy too, so checkAndCount's own
increment loop already returned a storeDown violation before
evaluateScope (and therefore budgetViolation) ever ran, leaving
budgetViolation's storeDown branch uncovered. Replace it with a stub
that only fails get, so the test's TokensPerDay limit actually drives
evaluateScope into budgetViolation's fail-closed path.

Document redis_store.go's incrBy as deliberately at-least-once: the
reconnect-retry can re-send INCRBY after a lost reply (possible
over-count), and fail-open can additionally count in the fallback —
both fail-safe (more restrictive), never fail-open in the unsafe
direction.

Validate Redis.DB >= 0 in newConfiguredLimiter alongside Address, so a
negative db is a constructor error rather than a SELECT -1 on the wire.

New tests: a hung (accepts, never replies) fake RESP server proving a
respClient call and a failOpen limiter call both stay bounded rather
than stacking timeouts; decodeReply subtests for an oversized bulk
length, a length just over the cap, a huge array length, a bad bulk
trailer, and an oversized line; a negative-DB constructor-rejection
test.
This commit is contained in:
2026-08-20 02:16:07 +01:00
parent 5eab51bdef
commit 7ee635cd27
7 changed files with 303 additions and 58 deletions
+13
View File
@@ -547,6 +547,19 @@ func TestNewGateway_RedisConfigured_EmptyAddress_ReturnsConstructorError(t *test
}
}
// TestNewGateway_RedisConfigured_NegativeDB_ReturnsConstructorError is
// review item 5: a negative Redis.DB must fail construction immediately,
// same as an empty address, rather than reaching SELECT -1 on the wire.
func TestNewGateway_RedisConfigured_NegativeDB_ReturnsConstructorError(t *testing.T) {
cfg := CreateConfig()
cfg.Providers = map[string]*ProviderConfig{"openai": {Type: "openai", APIKey: "k"}}
cfg.Redis = &RedisConfig{Address: "127.0.0.1:0", DB: -1}
next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})
if _, err := New(context.Background(), next, cfg, "llmgw"); err == nil {
t.Fatal("want constructor error for a negative Redis db")
}
}
// TestNewGateway_RedisPassword_ResolvedViaSecret is an end-to-end proof
// that newGateway resolves Config.Redis.Password through resolveSecret
// (env:/file:/literal) before handing it to respClient: an "env:" password