streams: expose SetReadDeadline so idle teardown actually engages - #31
Merged
Conversation
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>
TeoSlayer
force-pushed
the
fix/stream-read-deadline
branch
from
July 29, 2026 11:41
0d341ad to
bb16ae9
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
TeoSlayer
added a commit
to pilot-protocol/pilotprotocol
that referenced
this pull request
Jul 29, 2026
…imers (#435) * daemon: make connAdapter read deadlines real, stop leaking registry timers connAdapter's deadline setters were stubs: func (a *connAdapter) SetReadDeadline(t time.Time) error { return nil } They stored nothing and reported success. Anything that set a read deadline got no deadline and no error — a silent lie, which is worse than not implementing the method, because callers cannot detect the failure. Read consequently blocked on <-a.conn.RecvBuf forever. A peer that opened a stream and then went silent pinned its handler goroutine plus the whole Connection: a 512-slot RecvBuf, a 256-slot SendBuf and three goroutine stacks, roughly 43 KiB each. On 2026-07-28 that filled the daemon's own documented bound — DefaultMaxTotalConnections (65536) x ~43 KiB = ~2.8 GB — across 431 service agents on a 256 GB host, causing system-wide kernel OOM. The agents were reaped, restart-looped, stopped heartbeating, and the registry expired every registration, so the whole fleet went unresolvable. The daemon was not leaking past a bound; it was filling its bound and sitting there, which is why growth was ~100x and then plateaued. Idle daemons elsewhere were fine at 17 MB after 24 weeks, identifying this as per-request retention. Changes: - SetReadDeadline now stores an absolute deadline (atomic; it may be set from a goroutine other than the one parked in Read), and Read selects on RecvBuf against a timer, returning a net.Error with Timeout() == true. - SetDeadline delegates to SetReadDeadline instead of silently doing nothing. SetWriteDeadline stays a no-op but is now documented as such — Write already bounds itself via connAdapterWriteDeadline. - withRegistryDeadline no longer leaks a timer per call. time.After pins its timer for the full 8s duration, and this path runs repeatedly when the registry conn is half-open. Now an explicitly stopped timer. The result channel was already buffered, so the worker goroutine can always deposit and exit rather than parking on the send. Regression coverage in zz_connadapter_deadline_test.go: the deadline fires and reports Timeout(); a cleared deadline restores block-forever semantics; an armed deadline does not break normal reads; an already-expired deadline returns immediately. The first of these hangs for the full timeout and then fails against the old stub. NOTE: this is half the chain. Frame readers reach connAdapter through runtime's streamAdapter, which did not expose SetReadDeadline at all, so the type assertion that gates the idle teardown failed regardless of what the daemon implemented. Fixed in pilot-protocol/runtime#31. web4 pins runtime v0.3.1, so that must be released and bumped here before the timeout engages in production. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * deps: bump runtime to v0.3.2 — closes the idle-timeout chain Without this the connAdapter deadline work in this PR is a no-op in production. Frame readers reach connAdapter through runtime's streamAdapter, and streamAdapter only gained SetReadDeadline in runtime v0.3.2 (#31). Pinned at v0.3.1 the type assertion that gates dataexchange's idle teardown still fails, so the timeout never arms no matter what the daemon implements. Both halves are now in place: - runtime v0.3.2: streamAdapter exposes SetReadDeadline (+ a compile-time assertion so the capability cannot be dropped silently again) - this repo: connAdapter.SetReadDeadline stores a real deadline instead of being a stub that returned nil Verified with GOWORK=off so the resolution matches CI rather than the local go.work overlay, which shadows the module with the checkout and would have hidden a missing go.sum entry: build clean, vet clean, pkg/daemon suite green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Teodor Calin <teodor@vulturelabs.io> 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