-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Fix: Robust shutdown and error handling for large file change batches in docker compose watch #13525
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
pkg/compose/watch.go
Outdated
| select { | ||
| case <-ctx.Done(): | ||
| options.LogTo.Log(api.WatchLogger, "Watch disabled") | ||
| options.LogTo.Log(api.WatchLogger, "Watch disabled (context canceled)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"context" doesn't make any sense for end-user
| ctx, cancel := context.WithCancel(ctx) | ||
| defer cancel() | ||
|
|
||
| // debounce and group filesystem events so that we capture IDE saving many files as one "batch" event |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why remove this comment?
pkg/compose/watch.go
Outdated
| case batch := <-batchEvents: | ||
| case batch, ok := <-batchEvents: | ||
| if !ok { | ||
| options.LogTo.Log(api.WatchLogger, "Watch event channel closed") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WatchLogger is used to provide end-user message on the TUI, we should not refer to technical / implementation details
Head branch was pushed to by a user without write access
pkg/compose/create.go
Outdated
| } | ||
|
|
||
| err = s.ensureImagesExists(ctx, project, options.Build, options.QuietPull) | ||
| prepareNetworks(project) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unrelated changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i just tried to fix an issue, i just changed reorders the create function to ensure networks and volumes are provisioned before building images, which fixes a test failure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAICT this "fix" is unrelated to the current PR, and I can't see a test failure on main branch. Please avoid such undocumented changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, i have reverted back the irrelevant changes, take a look now and lets try running workflows.
pkg/compose/create.go
Outdated
| } | ||
|
|
||
| err = s.ensureImagesExists(ctx, project, options.Build, options.QuietPull) | ||
| prepareNetworks(project) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAICT this "fix" is unrelated to the current PR, and I can't see a test failure on main branch. Please avoid such undocumented changes.
52025fd to
3f43fdc
Compare
|
You PR is now mixing changes from upstream. Please squash commit and prefer rebase over merge from main |
Ensured all watcher and sync goroutines and channels are robustly closed on context cancellation or error. Added explicit logging for large batches and context cancellation to prevent stuck processes and ensure graceful shutdown on Ctrl-C. Signed-off-by: Amol Yadav <amyssnipet@yahoo.com>
|
@ndeloof now?? |
This MR contains the following updates: | Package | Update | Change | |---|---|---| | [docker/compose](https://github.com/docker/compose) | patch | `v5.0.1` → `v5.0.2` | MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot). **Proposed changes to behavior should be submitted there as MRs.** --- ### Release Notes <details> <summary>docker/compose (docker/compose)</summary> ### [`v5.0.2`](https://github.com/docker/compose/releases/tag/v5.0.2) [Compare Source](docker/compose@v5.0.1...v5.0.2) #### What's Changed ##### 🐛 Fixes - Fixed progress UI to adapt to terminal width by [@​ndeloof](https://github.com/ndeloof) in [#​13519](docker/compose#13519) - Removed warning when no explicit build has been requested. by [@​ndeloof](https://github.com/ndeloof) in [#​13493](docker/compose#13493) - Restored `runtime_flags` support in `models` by [@​ilopezluna](https://github.com/ilopezluna) in [#​13460](docker/compose#13460) - Added service name completion to `down` command by [@​bmo-at](https://github.com/bmo-at) in [#​13470](docker/compose#13470) - Fixed tilde in `--env-file` paths expanded to user home directory by [@​tensorworkerr](https://github.com/tensorworkerr) in [#​13510](docker/compose#13510) - Handle `healthcheck.disable: true` by [@​stavros-k](https://github.com/stavros-k) in [#​13494](docker/compose#13494) - Fixed shutdown and error handling for large file change batches in `watch` by [@​amyssnippet](https://github.com/amyssnippet) in [#​13525](docker/compose#13525) ##### 🔧 Internal - Added unit test for upOptions.OnExit method by [@​htoyoda18](https://github.com/htoyoda18) in [#​13489](docker/compose#13489) - clean up temporary compose files after conversion by [@​htoyoda18](https://github.com/htoyoda18) in [#​13483](docker/compose#13483) - Fixed typo in isSwarmEnabled method name by [@​htoyoda18](https://github.com/htoyoda18) in [#​13481](docker/compose#13481) - Fixed incorrect usage of errgroup.WithContext by [@​htoyoda18](https://github.com/htoyoda18) in [#​13480](docker/compose#13480) - Fixed timeout initialization when waitTimeout is zero by [@​htoyoda18](https://github.com/htoyoda18) in [#​13471](docker/compose#13471) - Extracted API version constants to dedicated file by [@​htoyoda18](https://github.com/htoyoda18) in [#​13503](docker/compose#13503) - Replace tabbed indentation in sdk.md by [@​pkqk](https://github.com/pkqk) in [#​13505](docker/compose#13505) - Improved attach error handling and cleanup by [@​htoyoda18](https://github.com/htoyoda18) in [#​13488](docker/compose#13488) - Modernize tests by [@​dgageot](https://github.com/dgageot) in [#​13531](docker/compose#13531) - set fsnotify build tag when building for OSX by [@​ndeloof](https://github.com/ndeloof) in [#​13532](docker/compose#13532) ##### ⚙️ Dependencies - build(deps): bump github.com/klauspost/compress to v1.18.2 by [@​thaJeztah](https://github.com/thaJeztah) in [#​13499](docker/compose#13499) - go.mod: remove exclude rules by [@​thaJeztah](https://github.com/thaJeztah) in [#​13498](docker/compose#13498) - build(deps): bump github.com/containerd/containerd/v2 to v2.2.1 by [@​thaJeztah](https://github.com/thaJeztah) in [#​13497](docker/compose#13497) - build(deps): bump golang.org/x/sys from 0.39.0 to 0.40.0 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​13502](docker/compose#13502) - build(deps): bump google.golang.org/grpc from 1.77.0 to 1.78.0 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​13475](docker/compose#13475) - build(deps): bump github.com/go-viper/mapstructure/v2 from 2.4.0 to 2.5.0 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​13506](docker/compose#13506) - build(deps): bump github.com/sirupsen/logrus v1.9.4 by [@​thaJeztah](https://github.com/thaJeztah) in [#​13518](docker/compose#13518) - Bump compose to v2.10.1 by [@​ndeloof](https://github.com/ndeloof) in [#​13528](docker/compose#13528) #### New Contributors - [@​bmo-at](https://github.com/bmo-at) made their first contribution in [#​13470](docker/compose#13470) - [@​pkqk](https://github.com/pkqk) made their first contribution in [#​13505](docker/compose#13505) - [@​tensorworkerr](https://github.com/tensorworkerr) made their first contribution in [#​13510](docker/compose#13510) - [@​stavros-k](https://github.com/stavros-k) made their first contribution in [#​13494](docker/compose#13494) - [@​amyssnippet](https://github.com/amyssnippet) made their first contribution in [#​13525](docker/compose#13525) - [@​dgageot](https://github.com/dgageot) made their first contribution in [#​13531](docker/compose#13531) **Full Changelog**: <docker/compose@v5.0.1...v5.0.2> </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever MR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0Mi44Ni4xIiwidXBkYXRlZEluVmVyIjoiNDIuODYuMSIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsiUmVub3ZhdGUgQm90IiwiYXV0b21hdGlvbjpib3QtYXV0aG9yZWQiLCJkZXBlbmRlbmN5LXR5cGU6OnBhdGNoIl19-->
What I did
Related issue
This PR addresses an issue where docker compose up --watch could become unresponsive after a large number of file changes (such as after a branch switch), causing logs to stop and Ctrl-C to not exit cleanly. The issue is #13136 . The fix ensures that all watcher and sync goroutines and channels are robustly closed on context cancellation or error, and adds explicit logging for large batches and context cancellation. This prevents the process from getting stuck and ensures graceful shutdown, even after large file change events.
By fixing: