Fix startup deadlock and event loss in discovery - #62
Merged
Merged
Conversation
Previously, NewEventNotif emitted the initial container snapshot into a channel with a fixed buffer of 100 while the consumer could not start until the constructor returned, so with more than 100 allowed running containers the send blocked forever and no logging began. After this change the snapshot is collected first and the events channel is sized to fit it. The docker events listener channel was unbuffered, and the client drops events for any listener which is not immediately ready, so events were lost whenever the consumer was busy. The listener channel is buffered now.
Coverage Report for CI Build 32267610063Coverage decreased (-0.1%) to 79.226%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions2 previously-covered lines in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
umputun
approved these changes
Aug 20, 2026
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.
Two defects in
app/discoveryaround event delivery, both reproduced by the tests added here.Startup deadlock with more than 100 running containers. Previously,
NewEventNotifcreatedeventsChwith a fixed buffer of 100 and then emitted the initial snapshot into it from inside the constructor, while the only consumer (runEventLoop) cannot start until the constructor returns. With 101 or more containers passing the filters the 101st send blocked forever, sodo()never reached the event loop and nothing was logged at all. After this change the snapshot is collected into a slice first and the channel is created withmax(100, len(initial)), so the whole initial batch fits.emitRunningContainersbecamerunningContainerEventsreturning the events instead of publishing them; error handling and the exported API are unchanged.Events dropped when the consumer is busy. The channel passed to
AddEventListenerwas unbuffered, and go-dockerclient publishes to listeners with a non-blocking send,vendor/github.com/fsouza/go-dockerclient/event.go:340-345:Any event arriving while
activatewas busy handling the previous one was therefore discarded by the client, leaving a started container without a log stream or a stopped one with a stale streamer. The listener channel is buffered now, which removes the loss for bursts up to the buffer size.Note that this narrows the loss window rather than closing it completely: if the outgoing channel stays full long enough for the listener buffer to fill as well, the client will start dropping again, and events fired between
ListContainersandAddEventListenerare still missed. Both need a larger change (aSincetimestamp viaAddEventListenerWithOptions, which alters the exportedDockerClientinterface), so I have described them separately in an issue rather than deciding the direction here.