Repository navigation
fix(api): a busy consumer is not an idle stream (#46) - #48
Merged
Merged
Conversation
Follow's idle timer also expires while Follow itself is busy (a slow onFrame, or the goroutine descheduled on a loaded runner). The reader is then blocked handing over a line it already read, so lastRead goes stale and select could pick the expired timer over the waiting line: a working stream was reconnected as idle. Before an oversized frame, that turned the fatal refusal into a second connection, which is the calls=2 flake in TestFollowRefusesOversizedLinesAndFrames. When the timer fires, a line already waiting now wins, like a read that already ended did. TestFollowBusyConsumerIsNotIdle reproduces the race deterministically (20/20 failures before, including the exact "frame over 4194304 bytes calls=2"). The oversize tests no longer run on the 200ms test idle limit they do not exercise, and the slowly arriving line test keeps a wide margin between chunk gaps and the limit. Fixes #46 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review follow-up: a 100ms idle limit with a 200ms busy consumer keeps the race reproducible (10/10 failures on the old client) without the limit itself being tight enough to reconnect on a loaded runner. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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
2 participants
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.
Fixes #46
Root cause
A real (minor) client bug, not a racy test counter. In
followOnce, the idle timer keeps running while Follow itself is busy: a slowonFrame, or the main goroutine descheduled on a loaded runner. During that time the scanner goroutine has already read the next line and is blocked handing it over on the unbufferedlineschannel, so it stops reading andlastReadgoes stale. When the main loop returns toselect, bothtimer.Candlinesare ready; if select picked the timer, the quiet check failed and the working connection was dropped aserrIdleand reconnected.In
TestFollowRefusesOversizedLinesAndFrames(5 MiB frame, 200ms test idle limit) a stall on the ubuntu runner did exactly that before the 4 MiB frame cap was reached, so the second connection refused the frame: fatal error, butcalls=2. The oversize refusal itself was always fatal and never retried; the retry came from the spurious idle reconnect before it.Fix
runs.go: when the idle timer fires, a line already waiting wins (non-blocking receive), the same way a read that already ended already did. Only when nothing is waiting does thelastReadquiet check decide idleness. Genuinely idle connections are detected exactly as before.TestFollowBusyConsumerIsNotIdle:onFramesleeps 200ms (2x the 100ms idle limit) on each of 5 events, followed by eitherendor an oversized frame. Deterministic repro on the old client (first draft 20/20 failures, final version 10/10), including the exact CI stringframe: err=the platform sent a stream frame over 4194304 bytes calls=2.TestFollowRefusesOversizedLinesAndFramesno longer runs on the 200ms helper idle limit it does not exercise (30s), so a stalled runner cannot reconnect it.TestFollowOversizedLineArrivingSlowlyIsNotIdlekeeps its purpose (the line takes ~660ms to arrive, longer than the 250ms idle limit) with a wide margin between each 20ms chunk gap and the limit (was 60ms vs 20ms).Test evidence
go test -count=20 -run TestFollowBusyConsumerIsNotIdle ./internal/api/-> 20/20 FAIL.yeshogs):go test -race -count=2000 -cpu=1,2,4 -run TestFollowRefusesOversizedLinesAndFrames ./internal/api/-> ok (6000 runs).GOMAXPROCS=1 go test -race -count=60 -cpu=1,2,4 -run 'TestFollowBusyConsumerIsNotIdle|TestFollowOversizedLineArrivingSlowlyIsNotIdle'-> ok.GOMAXPROCS=1 go test -race -count=30 -cpu=1,2 -run 'TestFollowBusyConsumerIsNotIdle|TestFollowRefusesOversized'-> ok.go test -race ./...,go vet ./...,gofmt -lclean.Review
Blocking QA review (Claude Code, Opus): APPROVED, no blockers. Nits applied: wider margins in the new test, comment on where the frame cap trips. Known residual window (not fixed, by design): if the whole process freezes for longer than the idle limit right after the reader hands over a line and before it reads the next one (already in the scanner buffer), that is still read as idle. Unrealistic with the production 45s limit, and the oversize test now runs with 30s.
Deploy notes
Test-and-client-internal change; no VERSION bump on purpose (merging a bump auto-releases). It ships with the next release.
🤖 Generated with Claude Code