fix: handle recursive watcher ignore paths correctly

[ManManavadaria]

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

[Copilot]

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]

Codecov Report

❌ Patch coverage is 58.06452% with 13 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
pkg/watch/watcher_naive.go	47.61%	8 Missing and 3 partials ⚠️
pkg/compose/watch.go	75.00%	1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[ManManavadaria]

Hi @glours @ndeloof ,
I’ve addressed all the review feedbacks in the latest commit. please review it, appreciate your time and feedback.

[glours]

/review

[docker-agent]

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:

1. macOS (Darwin) — the ignore parameter is silently discarded, so the fix does not apply on macOS.
2. 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

[ManManavadaria]

Hi @glours, Thanks for the valuable feedback. Based on the review comments, I’ve refactored the per-path trigger watchers and intersectPathMatcher and implemented multiNotify methods for grouped per-path watches and implemented PathMatcher ignores in Darwin watcher.

Please review the changes and share your feedback.

Thanks!

[glours]

Hi @ManManavadaria

Coming back with a fuller pass and I need to flag something I missed in earlier rounds.

What I missed

The previous watch.NewWatcher(paths) ran pathutil.EncompassingPaths internally, so overlapping trigger roots collapsed into a single inotify watch on the common ancestor. With the new per-path loop, each path gets its own watcher and
EncompassingPaths becomes a no-op. The result: overlapping subtrees fire every event twice, get forwarded twice through multiNotify, and are then masked by the QuietPeriod debouncer. Functionally tolerated, but we double our consumption of fs.inotify.max_user_watches, a bounded kernel resource users already hit on large repos.

I should have spotted this the first time, sorry for the late catch.

Proposed direction

Keep one shared watcher, push the ignore handling into it via a single matcher. Specifically:

• Keep: the ignore PathMatcher parameter on NewWatcher, the shouldIgnore / shouldIgnoreEntireDir / reshaped shouldSkipDir logic in the naive watcher, the Darwin shouldNotify parity, and intersectPathMatcher. These are all good.
• Drop: multiNotify / NewMultiWatcher and the per-path watcher loop in pkg/compose/watch.go.
• Add: a root-aware ignore matcher implementing PathMatcher, backed by map[absRoot]PathMatcher. On Matches / MatchesEntireDir for a file, dispatch to the roots that contain it and intersect their results, i.e. a file is ignored only if every trigger that could care about it agrees to ignore it. This preserves per-trigger isolation without losing EncompassingPaths consolidation.

Tests-wise: keep the intersect / shouldSkipDir / Darwin coverage you already have, and replace the (currently absent) multiNotify tests with a handful for the new root-aware matcher.

One ask on the PR body

When you push the rework, please describe the architectural change accurately, "applied ignore rules at the watcher layer" undersells what's actually being touched, which is partly how I under-scoped my first read.

Happy to discuss alternatives (e.g. keeping per-path watchers but running EncompassingPaths over the keys of watchRootIgnores first) if the direction above doesn't feel right. The underlying fix is correct, just want to land the smallest version of it.

[ManManavadaria]

and I need to flag something I missed in earlier rounds

Thanks for the detailed review. You're right, this isn't an optimal solution and unnecessarily doubles the fs events consumption, I should have explored alternative approaches and discussed them first instead of directly following the multiWatcher suggestion.

Your suggested direction makes sense, I'll rework this with a better solution shortly. Also, thanks for pointing out about the PR body discription, I'll keep that in mind for future PRs.

[ManManavadaria]

Hi @glours @ndeloof
I’ve implemented the changes in the latest commit. Please take a look when you have a moment, appreciate your time and feedback.

[glours]

/review

[docker-agent]

Assessment: 🟡 NEEDS ATTENTION

This PR correctly moves ignore-rule enforcement to the watcher layer and adds the intersectPathMatcher to prevent one service's ignore rules from suppressing events needed by another service. The overall approach is sound. Two medium-severity logic bugs were found in the new code that could cause the ignore rules to behave incorrectly in multi-service setups with overlapping watch paths.

[docker-agent]

[MEDIUM] shouldSkipDir uses last-matched root for ignore lookup — non-deterministic behavior with multiple overlapping watch roots

In the new shouldSkipDir implementation, dir is overwritten on every iteration of the d.notifyList map range loop:

isChildOfWatchedDir := false
var dir string
for root := range d.notifyList {
    if pathutil.IsChild(path, root) {
        return false
    }
    if pathutil.IsChild(root, path) {
        isChildOfWatchedDir = true
        dir = root  // overwritten on each matching root!
    }
}
if isChildOfWatchedDir && d.shouldIgnoreEntireDir(dir, path) {
    return true
}

When a path is a child of more than one root in notifyList (which is possible for non-recursive watchers, since pathutil.EncompassingPaths is only applied for recursive watchers), dir ends up pointing to whichever root happened to be iterated last. Because Go map iteration is randomised, the root — and therefore the ignore matcher — used for the skip decision is non-deterministic.

Impact: A directory that should be skipped (because one root's matcher marks it as fully ignored) may not be skipped, or vice versa, depending on map iteration order. This can cause permission errors on non-existent/restricted paths to resurface (the original bug this PR aims to fix) in multi-service setups with overlapping watch roots.

Suggested fix: Either collect all matching roots and apply the most-specific (deepest) one, or ensure a path is only skipped when every matching root's matcher agrees:

for root := range d.notifyList {
    if pathutil.IsChild(path, root) {
        return false
    }
    if pathutil.IsChild(root, path) {
        isChildOfWatchedDir = true
        // Only skip if this root's matcher agrees
        if !d.shouldIgnoreEntireDir(root, path) {
            return false  // at least one root needs this dir
        }
    }
}
return !isChildOfWatchedDir

[docker-agent]

[MEDIUM] normalizeWatchRoots intersects child-trigger matchers into parent root — causes under-ignoring of paths

When building the normalized ignore matcher for a watch root, normalizeWatchRoots collects matchers from ALL related trigger paths — including those where the trigger is a child of the root:

if root == triggerPath || pathutil.IsChild(root, triggerPath) || pathutil.IsChild(triggerPath, root) {
    matchers = append(matchers, matcher)
}
normalizedIgnores[root] = NewIntersectMatcher(matchers...)

NewIntersectMatcher requires every matcher to agree before a path is ignored (logical AND). This means that if a child trigger path contributes its matcher to the parent root's intersection, a path must satisfy both the parent-root matcher and the unrelated child-root matcher to be ignored at the parent level.

Concrete example with a realistic multi-service compose setup:

• Service A: path: /project, ignore: [vendor/] → ignoresByWatchPath["/project"] = vendor_matcher
• Service B: path: /project/pkg, ignore: [tmp/] → ignoresByWatchPath["/project/pkg"] = tmp_matcher

For root /project, normalizeWatchRoots collects [vendor_matcher, tmp_matcher] (because /project/pkg is a child of /project).

normalizedIgnores["/project"] = intersect(vendor_matcher, tmp_matcher)

Now /project/vendor/x.go:

• vendor_matcher.Matches(...) → true ✓
• tmp_matcher.Matches(...) → false (it only matches tmp/)
• Result: not ignored ✗

Expected: /project/vendor/x.go should be ignored per Service A's config, but it is not.

Impact: Paths that should be ignored per one service's configuration are not ignored when another service with overlapping (but different) watch roots exists. This means the watcher emits spurious events for these paths and could still encounter permission errors on paths that were supposed to be skipped.

Suggested fix: Use CompositePathMatcher (union/OR) when aggregating matchers from different trigger paths into a root's ignore matcher, rather than intersectPathMatcher. A path should be ignored if any trigger for that root scope ignores it:

normalizedIgnores[root] = CompositePathMatcher(matchers)

Reserve intersectPathMatcher for its intended purpose: combining matchers for the same trigger path (where a file should only be ignored if every watch rule for that exact path agrees).

[ManManavadaria]

Initially I thought of using CompositeMatcher in normalizeWatchRoots. but in this case it would work incorrectly

Service A: path: /project, ignore: [repo/]
Service B: path: /project/repo/pkg, ignore: [tmp/]

In this example, the normalized root would be /project. If we use a CompositeMatcher, then /repo would be ignored, which means /project/repo/pkg would not be watched. That’s why I used IntersectMatcher instead.