mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-25 11:52:08 +00:00
xarmian
3c6b412b0f
Codex round 15 raised a P1 against the reconnect handling: a bare pubsub.Receive loop does not start go-redis's health check, which only the Channel* constructors do, so a HALF-OPEN connection — no FIN, no RST, just a route that stopped working — blocks forever with no error, no retry and no coverage drop. That is the exact failure the reconnect handling exists to prevent, reintroduced one layer down by the mechanism meant to fix it. The diagnosis is right. The remedy is not, and the difference is measured rather than argued. PubSub.Ping ONLY WRITES the command — writeCmd, then return; it never reads a reply (go-redis v9.22.0, pubsub.go). So the health check's pingErr is nil for as long as the socket accepts writes, which a half-open socket does until its send buffer fills, and the channel path sets no read deadline. Probed with a TCP proxy that silently stopped forwarding, with ChannelWithSubscriptions in use and the health check running: NO RECONNECT IN 24 SECONDS. A test asserting detection was written, failed, and was deleted rather than left in a shape that cannot pass. So this switches to ChannelWithSubscriptions on the grounds that survive the measurement — it is the supported API, go-redis owns the redial and backoff, and a hand-rolled retry loop is machinery this package does not need to maintain — while the function's comment now states plainly that NEITHER form detects a half-open connection, with the mechanism and the probe result, so nobody re-derives the health check as a solution. The residual is filed on BUG-2730, which already owns "this bus is under- delivering and cannot say so", together with what would actually work (application-level idle tracking) and why it is a decision rather than a patch: it needs a threshold, and too low a threshold resyncs quiet workspaces for no reason — the load-posture inversion this family keeps having to avoid. Mutation: removing the coverage drop on resubscription fails two tests, so the detection still discriminates through the new API. Refs BUG-2731, BUG-2730