refactor(provider): single event-stream loop with one reconnect policy and leak-free sends#1022
Merged
Merged
Conversation
The events reconnect loop existed as two verbatim copies (docker and dockerswarm) that had already diverged: docker retried forever and never wrote its Err channel, while swarm gave up after ten attempts and emitted a terminal error - permanently killing event delivery after roughly one minute of daemon downtime, even though the daemon coming back is the common case. A maintainer patching one copy had no way to know the other existed; the attempt budget was added to swarm only. Both copies also sent events with a bare channel send. Once the consumer stops reading (the core watchers nil out the channel on error or return on cancellation), the producer goroutine blocks on that send forever, leaking the goroutine and pinning its events HTTP connection. Only the proxmox provider guarded its sends. The loop now lives once in pkg/provider/internal/mobyevents with a single documented policy: reconnect indefinitely with linear backoff capped at 30s, close both channels only on context cancellation, never emit terminal errors, and guard every send with the context. Docker, Docker Swarm and (through the docker provider) Podman share it. The sablier.InstanceEventStream contract is updated to describe the real semantics. The old docker unit test moves to the shared package, joined by a leak regression test (the old loop fails it: the goroutine is still blocked two seconds after cancellation) and a policy test pinning that the stream outlives the old ten-attempt budget without a terminal error.
|
Test Results✅ All tests passed! | 767 tests in 137.821s |
|
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.
Finding
The events reconnect loop existed as two verbatim copies that had already diverged, and the four providers implemented four different terminal-error contracts.
docker/events.goanddockerswarm/events.gocarried line-for-line duplicatestreamEvents/consumeEvents/linearBackoffimplementations - except swarm had grownmaxReconnectAttempts = 10and pushed a terminal error, while docker retried forever and never wrote itsErrchannel. The divergence is the proof of the problem: someone added the attempt budget to one copy without knowing the other existed.sablier.InstanceEventStream("Err carries a terminal error when the stream cannot be recovered; after an error is sent both channels are closed") was honored by swarm and proxmox, violated by docker (never errors) and kubernetes (never sends or closes anything).Goroutine leak in both copies (finding 4): events were delivered with a bare
instance <- event. Once the consumer stops reading - core watchers nil out the channel on error, or return on cancellation - the producer blocks on that send forever: thectx.Done()case of the select cannot fire once execution has entered the send. That leaks the goroutine and pins its events HTTP connection. Only the proxmox provider guarded its sends correctly.Discovery
Found by cross-comparing the provider event implementations during the provider-layer design review. The leak reproduced against the old loop with a test that parks the goroutine in a send and cancels:
Fix
One implementation, one policy, in
pkg/provider/internal/mobyevents(internal to the provider subtree):Errnever receives a value from these providers.Docker, Docker Swarm, and Podman (which reuses the docker provider) all share it. The
InstanceEventStreamcontract comment now describes the real semantics instead of an aspiration two of four providers ignored.Not in this PR (deliberately)
The Kubernetes watchers have the same bare-send issue inside their informer handlers, and never close their channels. Those handler bodies are being rewritten by #1018 (tombstone panics); guarding them here would conflict. Follow-up planned once #1018 lands.
Tests
TestStream_Reconnect- moved from the docker package (redial on connection drop).TestStream_ExitsWhenConsumerGone- the leak regression; fails against the old loop (output above).TestStream_ReconnectsIndefinitely- pins the policy: 15 dead connections, no terminal error, still delivering (the old swarm copy failed at attempt 10).Provider + core short suites and
golangci-lintpass; the docker/swarm testcontainers suites run in CI.Docs
No user-facing docs impact (internal refactor; behavior change is strictly "streams no longer permanently die", which needs no operator action).