mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-10 15:05:40 +00:00
xarmian
cc3cfeef2b
* fix(redis): honour the caller's context on TLS dials (BUG-2754) go-redis's default dialer (v9.22.0, options.go NewDialer) honours the caller's context on plaintext and NOT on TLS: the TLS branch returns tls.DialWithDialer, which takes no context at all, so a cancelled caller could not shorten the dial and it was bounded only by DialTimeout. BUG-2749 put SSE subscription establishment on the request's context so a client that disconnects stops holding its admission slots. On plaintext that covered the dial. On TLS the dial was the one segment cancellation could not reach, so the guarantee shrank from "released at once" to "released after up to DialTimeout" — and a managed Redis is a rediss:// URL, which is the ordinary production shape rather than an exotic one. Fixed at CLIENT CONSTRUCTION rather than in any consumer, because the same dial serves Publish, the Lua scripts, the presence registry and the watch bus's reads. internal/redisdial is a small package so the thing can be tested directly; cmd/pad/cmd_server.go installs it on the one client Pad builds. THREE THINGS THAT FAIL QUIETLY IF THE REPLACEMENT GETS THEM WRONG, each with a test that fails against getting it wrong: ServerName. tls.DialWithDialer infers it from the dialled address when the config leaves it empty; a hand-rolled tls.Client does not, and an empty ServerName leaves certificate verification with no name to check. That would turn a latency fix into a silent authentication regression. Replicated, on a CLONE — mutating the caller's config would leak one host's name into every later dial that shares it. Tested by dialling a certificate issued for another name and requiring an x509.HostnameError, per the lead's correction: asserting the field is set proves the code sets a field, not that the name is checked. Verified in the pinned source rather than assumed — redis.ParseURL DOES set ServerName for rediss:// (options.go:708), so Pad's path does not depend on the fallback today; it is there because it is what the replaced code did. The timeout must bound the HANDSHAKE, not just the connect. Otherwise a server that accepts and then stalls hangs for as long as the context lives — trading a bounded failure for an unbounded one, worse than the bug being fixed. It must not EXTEND an earlier deadline. context.WithTimeout takes the sooner of the two, matching go-redis's own promise about DialTimeout. PROSE SWEPT, and the sweep found two sites my first pass missed because it only grepped non-test files: five comments across internal/events said the TLS dial could not be cancelled, including one carrying an explicit "See BUG-2754 for the TLS half" forward reference. All five now say what is true, and the confirmTimeout budget comment records that it has been amended twice. Two instrument corrections: the certificate fixture put IP literals in DNSNames where x509 will never match them, and two tests detected their mutations BY HANGING — which is not a result anyone can act on, and which stranded the mutation harness with its edit still applied. Both bound the dial in a goroutine now, so a hang is a named failure. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * fix(redis): resolve DialTimeout, keep the keep-alive, share one budget (r1) Codex round 1 found three, and the P1 was introduced BY the first draft of this fix rather than inherited — the worst kind, since the diff was sold as closing a hang. DIALTIMEOUT READ AS ZERO. go-redis's Options.init() defaults it to 5s, but NewClient CLONES the options first (redis.go:1924), so a caller reading opt.DialTimeout in order to install a Dialer — the only time it can — reads zero for the ordinary URL that sets none. And PubSubPool.NewConn calls the dialer DIRECTLY with no timeout of its own (internal/pool/pubsub.go:45), so nothing downstream supplies one either. Resolved in the package, with the coupling named. The mutation matrix then refused to confirm the failure mode the finding described, which changed the test rather than the fix. An unresolved zero does not hang HERE: this dialer wraps the dial in context.WithTimeout, and a zero duration is an already-expired deadline, so every dial would fail INSTANTLY — nothing connects at all. The original draft would have hung; this one refuses. The assertion that separates them is a healthy server being reached, not a stalled one giving up, and the comment says which draft did which. KEEPALIVECONFIG DROPPED. go-redis's default dialer sets it (options.go:608) and it governs how quickly a dead peer is noticed on every Redis connection this process holds. Reverting to OS defaults would change that across the whole client as an invisible side effect of a cancellation fix — invisible because nothing fails. My first test for it compared our copy against go-redis's published numbers, which says nothing about whether the dialer USES it: deleting the field from the dialer left that test green. Replaced with a Linux-tagged test that reads SO_KEEPALIVE and TCP_KEEPIDLE off the accepted socket. Honest partial, stated at the test: the property is platform-independent, the observation is not, and the Smoke jobs on macOS and Windows skip the file. The value-comparison test is kept as well — it catches the copy drifting from what it mirrors, which the socket test cannot. TWO SEPARATE BUDGETS. DialTimeout was applied to the TCP connect and then a fresh one started for the handshake, allowing up to 2x on the pub/sub path, which has no outer deadline to mask it. tls.DialWithDialer bounds both as one interval; this must not be laxer than the code it replaces. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * fix(redis): honour an explicitly disabled dial timeout; finish the sweep (r2) Codex round 2, four findings and one correction to a claim I had already made. EXPLICIT dial_timeout=0 WAS BEING OVERRULED. ParseURL encodes an explicit zero or negative as -1, which go-redis preserves as "no timeout at all". Treating every non-positive value as unset collapsed that into the 5s default and silently overruled an operator who had deliberately disabled the bound — the same defect as the one round 1 found, in the opposite direction. `== 0` for the unresolved case now, with a negative carried through as no bound, and a test that goes through ParseURL rather than passing -1 by hand so it pins the real path. A DRIFT GUARD for the two copied constants, compared against go-redis's RESOLVED options (NewClient runs init() on its clone and Options() returns the result) rather than against a literal. A copy that silently diverges from what it mirrors is what would make this package worse than none. TWO STALE COMMENTS I HAD CLAIMED WERE FIXED. My sweep commit said "all five now say what is true"; it was three. The first patch batch aborted on a failed anchor and, because that helper writes only after every pair matches, none of its edits landed — I re-applied some by hand and did not re-verify the rest. The grep I ran afterwards searched for phrasings the surviving comments did not use. Both now corrected: establishSubscription's two-bullet plaintext/TLS split and the mutex comment that named TLS as the case cancellation could not reach. TWO TESTS RELABELLED RATHER THAN LEFT LOOKING LIKE COVERAGE. The single-budget test does not discriminate — the server accepts immediately, so the connect consumes none of the budget and the two-budget implementation finishes in the same time. Staging a slow connect against a local listener is not deterministic, so what holds that property is structural (one context, created before the connect, passed through the handshake) and the test says so. And the Linux-only keepalive test now states what its build tag does and does not cost: the behaviour is platform-independent and the full suite runs on Linux CI, so a removal is caught; the macOS and Windows Smoke jobs are build-and-start checks and were never the guard. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X