fix: handle recursive watcher ignore paths correctly#13756
fix: handle recursive watcher ignore paths correctly#13756ManManavadaria wants to merge 2 commits intodocker:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to fix Docker Compose watch startup failures when an ignored path is unreadable by pushing ignore handling down into the watcher layer. In the codebase, that primarily affects how pkg/compose constructs file watchers and how pkg/watch traverses directories on different platforms.
Changes:
- Added an ignore matcher parameter to the watcher API and threaded it through Compose watch setup.
- Updated the naive watcher to skip ignored directories during recursive traversal and to suppress permission errors for ignored paths during startup walks.
- Adjusted Darwin watcher and test fixture construction to use the new watcher signature.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
pkg/watch/watcher_naive.go |
Adds ignore-aware traversal and permission handling to the non-fsevents watcher implementation. |
pkg/watch/watcher_darwin.go |
Updates the Darwin watcher constructor signature to match the new watcher API. |
pkg/watch/notify_test.go |
Updates test fixture setup to call NewWatcher with an ignore matcher. |
pkg/watch/notify.go |
Extends the exported watcher constructor to accept an ignore matcher. |
pkg/compose/watch.go |
Builds ignore matchers from watch triggers and passes them into watcher creation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
ManManavadaria
force-pushed
the
13750-watch-ignore-fix
branch
from
1e22d66 to
bba54a8
Compare
Signed-off-by: ManManavadaria <manmanavadaria@gmail.com>
…nd apply review fixes Signed-off-by: ManManavadaria <manmanavadaria@gmail.com>
ManManavadaria
force-pushed
the
13750-watch-ignore-fix
branch
from
bba54a8 to
4ab735b
Compare
|
/review |
![docker-agent[bot]](https://avatars.githubusercontent.com/in/2787757?s=60&v=4){kind=small}
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
This PR correctly applies ignore rules to the watcher layer for Linux/Windows (naive watcher) and handles permission errors from ignored paths during recursive directory traversal. However, there are two issues worth addressing:
- macOS (Darwin) — the
ignoreparameter is silently discarded, so the fix does not apply on macOS. - Multi-trigger ignore OR-logic — merging all trigger ignore matchers with OR logic at the watcher layer can cause directories needed by one trigger to be skipped because another trigger ignores them.
Reviewed 6 files: pkg/compose/watch.go, pkg/watch/notify.go, pkg/watch/notify_test.go, pkg/watch/watcher_darwin.go, pkg/watch/watcher_naive.go, pkg/watch/watcher_naive_test.go
| } | ||
|
|
||
| func newWatcher(paths []string) (Notify, error) { | ||
| func newWatcher(paths []string, _ ...PathMatcher) (Notify, error) { |
There was a problem hiding this comment.
[MEDIUM] Darwin watcher silently discards the ignore PathMatcher — ignore rules not applied on macOS
newWatcher on Darwin uses _ ...PathMatcher (variadic, discarded), so the ignore matcher passed from pkg/compose/watch.go is never stored or used. On macOS:
- Permission errors on paths in the ignore list will still surface as hard errors, defeating the purpose of this PR.
- No directories will be skipped via
shouldSkipDirat the watcher layer; the naive (Linux/Windows) watcher stores and usesd.ignorefor bothshouldIgnoreandshouldSkipDir, but the Darwin path has no equivalent.
The fix should store the matcher in fseventNotify and implement analogous shouldIgnore/shouldSkipDir logic, or at minimum handle permission errors on ignored paths in the Darwin event loop.
| } | ||
|
|
||
| watcher, err := watch.NewWatcher(paths) | ||
| watcher, err := watch.NewWatcher(paths, watch.NewCompositeMatcher(ignoreMatchers...)) |
There was a problem hiding this comment.
[MEDIUM] OR-logic composite ignore matcher at watcher layer may suppress directory watching for one trigger due to another trigger's ignore rules
All trigger ignore matchers are merged into a single CompositePathMatcher with OR logic before being passed to NewWatcher. Since CompositePathMatcher.Matches returns true on the first matcher that matches, trigger A's ignore patterns (e.g., dist/) could cause a directory that trigger B legitimately needs to watch to be skipped at the watcher layer entirely.
Example: Service A ignores shared/dist/, Service B watches shared/ without ignoring dist/. At the watcher layer, shared/dist/ is skipped for both because trigger A's ignore rule matches — B will never receive events from shared/dist/.
The per-trigger watchRule.Matches() logic does apply per-trigger ignores correctly for event routing, but the upstream watcher-layer skip prevents events from ever being generated for those directories.
Consider building a per-path/per-trigger watcher or using an intersection (AND) approach — only skip a path if all trigger matchers that cover that path agree it should be ignored.
| if matches { | ||
| return true | ||
| } | ||
| return false |
There was a problem hiding this comment.
| if matches { | |
| return true | |
| } | |
| return false | |
| return matches |
| if os.IsPermission(err) && d.shouldIgnore(path) { | ||
| logrus.Debugf("Ignoring path: %s", path) | ||
| return filepath.SkipDir | ||
| } |
There was a problem hiding this comment.
Same this is already catch by block lines 204-206
| func (d *naiveNotify) shouldIgnore(path string) bool { | ||
| if d.ignore == nil { | ||
| return false | ||
| } | ||
| matches, err := d.ignore.Matches(path) | ||
| if err != nil { | ||
| logrus.Debugf("error checking ignored path %q: %v", path, err) | ||
| return false | ||
| } | ||
| return matches |
There was a problem hiding this comment.
This function isn't needed, it's only called from the 2 previous unnecessary check blocks above
| } | ||
|
|
||
| func newWatcher(paths []string) (Notify, error) { | ||
| func newWatcher(paths []string, _ ...PathMatcher) (Notify, error) { |
There was a problem hiding this comment.
This is inconsistent with the naive implementation which use _ PathMatcher
What I did
Applied ignore rules at the watcher layer and updated recursive traversal to skip ignored files/directories, ensuring permission errors from paths in the ignore list do not fail watch.
Related issue
fixes #13750
(not mandatory) A picture of a cute animal, if possible in relation to what you did