test(topic): fix load-flaky WS panic in cross-instance await helpers - #518
Merged
Conversation
TestTopic_V8_…CrossInstance (and its awaitWSContains/awaitWSContainsRejecting helper class) intermittently panicked "repeated read on failed websocket connection" under the full `go test ./...` run, never in isolation. Root cause is a gorilla/websocket antipattern, not container/load contention (a targeted high-churn repro did NOT trigger it). The helpers looped SetReadDeadline(150ms) + ReadMessage, doing `continue` on any error. gorilla records a read-deadline TIMEOUT in c.readErr via hideTempErr — poisoning the connection — and there is no way to clear it. Once poisoned, every ReadMessage returns the stored error immediately, so the loop fast-spins and gorilla's readErrCount>=1000 guard panics. This only bites when a frame arrives slower than the 150ms deadline, i.e. under CPU saturation during the concurrent `./...` run, which is why it looked like a load/container race. Fix: a single reader goroutine (wsFrameReader) owns ReadMessage with NO per-read deadline, so it never times out and never poisons the conn; the wait loop selects on the delivered-frame channel vs a re-trigger ticker vs one 8s overall deadline. awaitWSContains/awaitWSContainsRejecting collapse to thin wrappers over a shared awaitWSFrame. wsFrameReader also clears the stale deadline connectWS leaves set, and closes ws via t.Cleanup so the goroutine always exits. TestWSFrameReader_SurvivesSlowFrame pins the contract: it delivers a 600ms-delayed frame without panicking (verified the pre-fix helper panics on the same input, so the guard discriminates). Verified: full `go test ./...` green; topic tests pass x8 under all-core CPU saturation (0 panics) — the exact condition that exposed the flake. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ui2cwpeGkrUfRt8rh2FgGG
ReviewSolid fix — read through the diff and the full call sites in Strengths
One footgun worth a one-line guard or comment
Given the PR is scoped as a targeted flake fix, a comment addition is probably sufficient rather than reworking the API. Minor
Test coverage / correctness: no issues found. No security or production-code concerns — this PR only touches test helpers/files. |
✅ Performance Benchmark ResultsStatus: No significant regressions detected Benchmark ComparisonThresholds
See benchmarking guide for details. |
Address the review-bot note on #518: wsFrameReader dedicates a goroutine to ws.ReadMessage for the connection's lifetime, and gorilla/websocket forbids concurrent reads, so calling the await helpers twice on one connection would spawn a second concurrent reader (a data race). No current caller does this; the doc callouts on wsFrameReader / awaitWSContains / awaitWSContainsRejecting keep a future reuse from tripping it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ui2cwpeGkrUfRt8rh2FgGG
Review SummaryTest-only fix (3 files, +134/-44) for a load-flaky WS panic in cross-instance test helpers. Root-cause analysis is thorough and the evidence (deterministic repro, negative container-churn control, discriminating regression test) is convincing — this reads like a genuine What it doesReplaces the Strengths
Minor observations (non-blocking)
Risk assessmentScope is limited to test helpers/test files — no production code touched, so blast radius is minimal even if something is off. The PR body's verification section (full suite green, Nice, well-documented diagnosis-driven fix. No blocking issues found. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TL;DR — a gorilla read-deadline antipattern, not a container race
This started as an investigation into a flaky panic that looked like a testcontainer/teardown race under concurrent load. The root cause turned out to be a
gorilla/websocketantipattern in two test helpers — which is why the diff is three test files, not container-lifecycle code.Symptom
TestTopic_V8_…CrossInstance(and theawaitWSContains/awaitWSContainsRejectinghelper class) intermittently panickedrepeated read on failed websocket connectionduring the fullgo test ./...run — never in isolation (passed 4/4 alone).Root cause (proven, not inferred)
The helpers looped
SetReadDeadline(150ms)+ReadMessage, doingcontinueon any error:gorilla records a read-deadline timeout in
c.readErr(viahideTempErr, which wraps but still returns the error), and there is no way to clear it. Once poisoned, everyReadMessagereturns the stored error immediately, so the loop fast-spins and gorilla'sreadErrCount >= 1000guard panics (conn.go:1030). This only triggers when a frame arrives slower than the 150ms deadline — i.e. under CPU saturation during the concurrent./...run — which is exactly why it masqueraded as a load/container race.Evidence:
NextReader'sfor c.readErr == nilloop is skipped oncereadErris set;SetReadDeadlinedoesn't touchreadErr.SetReadDeadline(1ms)+ retry-loopReadMessageagainst a silent server panics with the exact string in <1s.Fix
A single reader goroutine (
wsFrameReader) ownsReadMessagewith no per-read deadline, so it never times out and never poisons the conn. The wait loopselects on the delivered-frame channel vs a re-trigger ticker vs one overall 8s deadline.awaitWSContains/awaitWSContainsRejectingcollapse to thin wrappers over a sharedawaitWSFrame.wsFrameReaderalso clears the stale deadlineconnectWSleaves set for the initial-render read, and closeswsviat.Cleanupso the reader goroutine always exits.Regression guard
TestWSFrameReader_SurvivesSlowFramedelivers a 600ms-delayed frame without panicking. Verified it discriminates: the pre-fix helper panics on that same input (throwaway control), the new helper passes.Verification
go test ./...— greengo test -race -run 'TestTopic|TestWSFrameReader' -count=2— 0 data races, 0 panics (matches CI's-racejob; no double-reader-on-one-conn since each test awaits one conn once)Context
Surfaced while wiring release-script tests into CI (#513 / #517); split out as its own PR since it's an unrelated, pre-existing flake on
main.🤖 Generated with Claude Code