streams: expose SetReadDeadline so idle teardown actually engages - #31
Open
TeoSlayer wants to merge 6 commits into
Open
streams: expose SetReadDeadline so idle teardown actually engages#31TeoSlayer wants to merge 6 commits into
TeoSlayer wants to merge 6 commits into
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Frame readers gate their slowloris/idle teardown on an optional interface:
dl, canDeadline := conn.(readDeadliner) // dataexchange/service.go:227
if canDeadline && idle > 0 {
_ = dl.SetReadDeadline(time.Now().Add(idle))
}
streamAdapter did not implement SetReadDeadline, so canDeadline was false
for every in-daemon plugin connection and dataexchange's DefaultIdleTimeout
(2 minutes) was never applied. A peer that opened a stream, sent one frame
and then stalled held its handler goroutine and the entire underlying
Connection — a 512-slot RecvBuf, a 256-slot SendBuf and three goroutine
stacks, roughly 43 KiB — until the process died.
On 2026-07-28 that filled the daemon's documented connection bound
(DefaultMaxTotalConnections = 65536 x ~43 KiB = ~2.8 GB per daemon) across
431 service agents on a 256 GB host, triggering system-wide kernel OOM. The
agents were reaped, restart-looped, stopped heartbeating, and the registry
expired every registration — the whole fleet became unresolvable. Idle
daemons on other hosts were unaffected and had run 24 weeks at 17 MB, which
is what identified this as per-request retention rather than a time-based
leak.
SetReadDeadline forwards to the underlying ConnReadWriter when that
transport supports deadlines, and reports success otherwise so callers that
treat a failure as fatal are not broken by transports that legitimately
cannot do deadlines.
Added a compile-time assertion that *streamAdapter satisfies the interface,
so the capability cannot be silently dropped again. The failure mode here
was invisible precisely because a failed type assertion has no diagnostic —
it just quietly selects the no-timeout path.
Note: this is only half the chain. The daemon's connAdapter.SetReadDeadline
was itself a stub returning nil without storing anything; that is fixed in
pilotprotocol separately. Both are required for the timeout to engage.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ines
daemonEventBus.Subscribe wraps the daemon bus in a forwarder goroutine that
did a BLOCKING send onto the channel it hands back:
out <- coreapi.Event{...}
The underlying bus deliberately uses non-blocking drop semantics so a slow
subscriber can never stall its publishers. Forwarding with a blocking send
silently converted that guarantee into a goroutine leak.
If a plugin consumer stopped draining out — handler loop exited, panicked
past its recover, or merely ran slower than the publisher for cap(src) events
— this goroutine parked on the send permanently, retaining itself, the
buffered contents of src, and every payload map they referenced.
cancel() does not rescue it. cancel closes src, which does nothing for a
goroutine already blocked on a send to out. Nothing else can ever unblock it,
so the leak lasts the process lifetime.
Every other subscriber in the tree consumes the bus channel directly rather
than through an intermediate forwarder, which is why this only affects the
plugin path.
Dropping matches what the bus would have done once its buffer filled, so a
slow consumer now loses events — the documented behaviour — instead of
leaking a goroutine forever.
Co-Authored-By: Claude Opus 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.
The bug
Frame readers gate their slowloris/idle teardown on an optional interface:
streamAdapterdid not implementSetReadDeadline, socanDeadlinewas false for every in-daemon plugin connection and dataexchange'sDefaultIdleTimeout(2 minutes) was never applied.A peer that opened a stream, sent one frame and then stalled held its handler goroutine and the entire underlying
Connection— a 512-slotRecvBuf, a 256-slotSendBuf, and three goroutine stacks, roughly 43 KiB — until the process died.What it cost
On 2026-07-28 this filled the daemon's documented connection bound across the service fleet:
431 agents × that on a 256 GB host → system-wide kernel OOM. Agents were reaped, restart-looped, stopped heartbeating, and the registry expired every registration. The entire fleet became unresolvable.
The diagnostic that isolated it: idle daemons on other hosts were completely unaffected — one had run 24 weeks at 17 MB. That ruled out a time-based leak and pointed at per-request retention.
The fix
SetReadDeadlineforwards to the underlyingConnReadWriterwhen that transport supports deadlines, and reports success otherwise so callers treating a failure as fatal aren't broken by transports that legitimately can't do deadlines.Also added a compile-time assertion that
*streamAdaptersatisfies the interface. This failure mode was invisible precisely because a failed type assertion produces no diagnostic — it just quietly selects the no-timeout path. The assertion makes dropping the capability a build error.The daemon's
connAdapter.SetReadDeadlinewas itself a stub returningnilwithout storing anything — a no-op that reported success. That's fixed separately inpilot-protocol/pilotprotocol.Both are required for the timeout to engage, and web4 currently pins
runtime v0.3.1, so this needs a release + a dependency bump before the fix reaches production.🤖 Generated with Claude Code