fix(llminternal): classify a dropped live connection by type, not error text - #1603
Open
harshitwandhare wants to merge 1 commit into
Open
harshitwandhare wants to merge 1 commit into
harshitwandhare wants to merge 1 commit into
Conversation
harshitwandhare
force-pushed
the
fix/llminternal-resumable-socket-error
branch
5 times, most recently
from
September 24, 2026 22:34
6705d1f to
2db67d2
Compare
harshitwandhare
force-pushed
the
fix/llminternal-resumable-socket-error
branch
2 times, most recently
from
September 29, 2026 02:01
e64e697 to
7aa1887
Compare
Contributor
Author
|
Gentle ping on this one when someone has time. All checks pass. Happy to change the approach if you'd rather classify the dropped connection differently. |
harshitwandhare
force-pushed
the
fix/llminternal-resumable-socket-error
branch
2 times, most recently
from
October 2, 2026 07:20
afd481e to
837f184
Compare
…or text
RunLive's reader and sender goroutines both report into the same errChan
and the flow acts on whichever arrives first, so the two must agree on
whether a dropped connection is resumable. They did not agree on Windows.
isResumable matched substrings. Its list covers the reader's websocket
close text ("close 1006 ... unexpected EOF") and the POSIX sender text
("write: broken pipe"), but not the Windows sender text ("wsasend: An
established connection was aborted by the software in your host
machine."). When the sender won the race on Windows, the same connection
loss that would have resumed was pushed to the caller as fatal and the
session stopped after one connection.
errors.Is against the POSIX constants does not close the gap, because Go
does not map WSA error numbers onto them. The chain is *net.OpError ->
*os.SyscallError -> syscall.Errno(10053).
Match the transport failure by type instead. Any *net.OpError reaching
this path is a failed read or write on an already-established live
socket, since the dial is handled separately above, so it is resumable
on every platform. The substring checks stay for the websocket-level
cases (1006, 1008, GoAway), which are not *net.OpError.
isResumable moves from a closure inside RunLive to a package-level
function so it can be tested directly. Behaviour is otherwise unchanged.
Fixes google#1602
harshitwandhare
force-pushed
the
fix/llminternal-resumable-socket-error
branch
from
October 3, 2026 01:40
837f184 to
21a70bf
Compare
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
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 #1602.
What was broken
RunLiveruns a reader goroutine and a sender goroutine against one live connection. Both report failures into the same unbufferederrChanand the flow's select consumes exactly one, so whichever arrives first decides whether the session resumes or dies.isResumablemade that call by matching substrings of the error text, and the two goroutines do not produce the same text for the same event.Measured with a probe that kills the peer and then calls each path directly:
isResumablewrite tcp ...: write: broken pipetruewrite tcp ...: wsasend: An established connection was aborted by the software in your host machine.falseThe reader's error is the websocket layer's close (
close 1006 (abnormal closure): unexpected EOF) and is the same on both, so it matchedEOFand resumed. On Windows the sender's error matched nothing in the list, so an identical connection loss was pushed to the caller as fatal and the session stopped after one connection. Which goroutine noticed first decided whether a Windows live session survived a transient drop.errors.Isagainst the POSIX constants does not close the gap. Go does not map WSA error numbers ontosyscall.ECONNABORTED/ECONNRESET, verified directly:What the fix does
Classifies the transport failure by type instead of by text:
Any
*net.OpErrorreaching this path is a failed read or write on an already-established live socket, because the dial has its own error path higher up that returns before this loop. The substring checks stay for the websocket-level cases (1006, 1008, GoAway), which are not*net.OpError, so no existing behaviour changes.isResumablemoves from a closure insideRunLiveto a package-level function, which is what makes it testable. Nothing else about it changed: theio.EOFcheck and the substring list are byte-identical to before.How it surfaced
TestRunLiveNoGoroutineLeak/realtime_sender_error_after_connection_loss_does_not_leakfails intermittently on Windows:connection count = 1, want 2is the tell: the flow never opened the second connection. Despite the test's name this is not a leak.Being straight about that repro: I saw it once in roughly 550 runs of the test binary, and only under heavy CPU load, which is what makes the sender win the race often enough to observe. I did not try to turn that into a regression test, because a 1-in-550 race would be a flake in CI rather than a guard. The new unit test pins the mechanism instead, which is deterministic.
Testing Plan
Failing first, and on the real defect rather than on a missing symbol. Since
isResumabledid not exist as a callable symbol before, running the new test against unmodifiedmainonly producesundefined: isResumable, which proves nothing. So the extraction was applied on its own, without the*net.OpErrorcheck, and the new tests were run against that:With the
*net.OpErrorcheck restored, all 16 subtests pass.The test synthesizes the error shapes (
*net.OpErrorwrapping*os.SyscallErrorwrapping an explicit errno) rather than relying on the host's own spelling, so a Linux CI runner exercises the Windows cases. Confirmed: the same 16 subtests pass on linux/amd64 (WSL2) as well as windows/amd64.Gates run on this branch, windows/amd64, Go 1.26.6:
go test ./internal/llminternal/ -count=1okgo test -race ./internal/llminternal/ -count=1ok, and 20 further runs as separate processes, 0 failuresgo vet ./internal/llminternal/cleango test -race -mod=readonly -count=1 -shuffle=on work: 5 packages fail, and the same 5 fail on pristinemainat f7e16e0. Failure sets extracted and diffed rather than eyeballed, and they are identical:cmd/adkgo/internal/deploy/agentengine,cmd/adkgo/internal/deploy/cloudrun,internal/configurable/conformance/replayplugin,internal/telemetry/functionaltest,runner. All are Windows-only and none isinternal/llminternalgolangci-lint run: 0 issues, exit 0. It flagged one real gofumpt violation in the new test file first, which is fixed.Worth recording for anyone else reproducing locally: with a stale
GOLANGCI_LINT_CACHEthis run instead reports 6 spuriousSA5011findings in unrelated test files, and the files it names change between runs on an unmodified tree. That is exactly the failure mode theskip-cache: truecomment in.github/workflows/go.ymldescribes, and pointingGOLANGCI_LINT_CACHEat a fresh directory gives0 issues.I had it backwards at first and am noting it in case it saves someone the same detour.One thing worth flagging separately, since it cost me time and is not caused by this change:
-count=NwithN > 1is not usable on this package.TestLoggingSpanIDPropagationfails on every repeat after the first because it depends on process-global logger state, 19 failures out of 20 on pristinemainwith no changes applied. Repeated runs here have to be separate processes.Environment
Windows 11 / windows-amd64, Go 1.26.6, cross-checked on linux/amd64 under WSL2.