From a7349b18778b1f62a5a6dd63d25dc3bea905f68e Mon Sep 17 00:00:00 2001 From: ManManavadaria Date: Fri, 24 Apr 2026 19:07:21 +0000 Subject: [PATCH 1/5] fix: handle recursive watcher ignore paths correctly Signed-off-by: ManManavadaria --- pkg/compose/watch.go | 12 +++++++++--- pkg/watch/notify.go | 4 ++-- pkg/watch/notify_test.go | 2 +- pkg/watch/watcher_darwin.go | 2 +- pkg/watch/watcher_naive.go | 34 ++++++++++++++++++++++++++++++++-- 5 files changed, 45 insertions(+), 9 deletions(-) diff --git a/pkg/compose/watch.go b/pkg/compose/watch.go index f63c51683ab..1e22c9479a6 100644 --- a/pkg/compose/watch.go +++ b/pkg/compose/watch.go @@ -198,8 +198,9 @@ func (s *composeService) watch(ctx context.Context, project *types.Project, opti eg, ctx := errgroup.WithContext(ctx) var ( - rules []watchRule - paths []string + rules []watchRule + paths []string + ignoreMatchers []watch.PathMatcher ) for serviceName, service := range project.Services { config, err := loadDevelopmentConfig(service, project) @@ -254,6 +255,11 @@ func (s *composeService) watch(ctx context.Context, project *types.Project, opti } } } + ignore, err := watch.NewDockerPatternMatcher(trigger.Path, trigger.Ignore) + if err != nil { + return nil, err + } + ignoreMatchers = append(ignoreMatchers, ignore) paths = append(paths, trigger.Path) } @@ -268,7 +274,7 @@ func (s *composeService) watch(ctx context.Context, project *types.Project, opti return nil, fmt.Errorf("none of the selected services is configured for watch, consider setting a 'develop' section") } - watcher, err := watch.NewWatcher(paths) + watcher, err := watch.NewWatcher(paths, watch.NewCompositeMatcher(ignoreMatchers...)) if err != nil { return nil, err } diff --git a/pkg/watch/notify.go b/pkg/watch/notify.go index d63f5caf28b..71dcaf1156f 100644 --- a/pkg/watch/notify.go +++ b/pkg/watch/notify.go @@ -80,8 +80,8 @@ func (EmptyMatcher) MatchesEntireDir(f string) (bool, error) { return false, nil var _ PathMatcher = EmptyMatcher{} -func NewWatcher(paths []string) (Notify, error) { - return newWatcher(paths) +func NewWatcher(paths []string, ignore PathMatcher) (Notify, error) { + return newWatcher(paths, ignore) } const WindowsBufferSizeEnvVar = "COMPOSE_WATCH_WINDOWS_BUFFER_SIZE" diff --git a/pkg/watch/notify_test.go b/pkg/watch/notify_test.go index b1ec9b9644d..0e9a7bc63bf 100644 --- a/pkg/watch/notify_test.go +++ b/pkg/watch/notify_test.go @@ -526,7 +526,7 @@ func (f *notifyFixture) rebuildWatcher() { } // create a new watcher - notify, err := NewWatcher(f.paths) + notify, err := NewWatcher(f.paths, EmptyMatcher{}) if err != nil { f.T().Fatal(err) } diff --git a/pkg/watch/watcher_darwin.go b/pkg/watch/watcher_darwin.go index a63612aedff..9486b9af375 100644 --- a/pkg/watch/watcher_darwin.go +++ b/pkg/watch/watcher_darwin.go @@ -115,7 +115,7 @@ func (d *fseventNotify) Errors() chan error { return d.errors } -func newWatcher(paths []string) (Notify, error) { +func newWatcher(paths []string, _ PathMatcher) (Notify, error) { dw := &fseventNotify{ stream: &fsevents.EventStream{ Latency: 50 * time.Millisecond, diff --git a/pkg/watch/watcher_naive.go b/pkg/watch/watcher_naive.go index 7798025e8bc..135e058ce35 100644 --- a/pkg/watch/watcher_naive.go +++ b/pkg/watch/watcher_naive.go @@ -45,6 +45,7 @@ type naiveNotify struct { // the notify list. It might be better to store this in a tree // structure, so we can filter the list quickly. notifyList map[string]bool + ignore PathMatcher isWatcherRecursive bool watcher *fsnotify.Watcher @@ -113,6 +114,10 @@ func (d *naiveNotify) watchRecursively(dir string) error { return filepath.WalkDir(dir, func(path string, info fs.DirEntry, err error) error { if err != nil { + if os.IsPermission(err) && d.shouldIgnore(path) { + logrus.Debugf("Ignoring path: %s", path) + return nil + } return err } @@ -240,6 +245,11 @@ func (d *naiveNotify) shouldSkipDir(path string) bool { return false } + // If path is present in the ignore list then we should ignore it + if d.shouldIgnore(path) { + return true + } + // Suppose we're watching // /src/.tiltignore // but the .tiltignore file doesn't exist. @@ -260,6 +270,26 @@ func (d *naiveNotify) shouldSkipDir(path string) bool { return true } +func (d *naiveNotify) shouldIgnore(path string) bool { + if d.ignore == nil { + return false + } + matches, err := d.ignore.MatchesEntireDir(path) + if err != nil { + logrus.Debugf("error checking ignored directory %q: %v", path, err) + return false + } + if matches { + return true + } + matches, err = d.ignore.Matches(path) + if err != nil { + logrus.Debugf("error checking ignored path %q: %v", path, err) + return false + } + return matches +} + func (d *naiveNotify) add(path string) error { err := d.watcher.Add(path) if err != nil { @@ -270,7 +300,7 @@ func (d *naiveNotify) add(path string) error { return nil } -func newWatcher(paths []string) (Notify, error) { +func newWatcher(paths []string, ignore PathMatcher) (Notify, error) { fsw, err := fsnotify.NewWatcher() if err != nil { if strings.Contains(err.Error(), "too many open files") && runtime.GOOS == "linux" { @@ -297,9 +327,9 @@ func newWatcher(paths []string) (Notify, error) { } notifyList[path] = true } - wmw := &naiveNotify{ notifyList: notifyList, + ignore: ignore, watcher: fsw, events: fsw.Events, wrappedEvents: wrappedEvents, From b62dbfdc4b63dc3ce25eb88c2ccd3f0037ba151f Mon Sep 17 00:00:00 2001 From: ManManavadaria Date: Wed, 6 May 2026 19:09:44 +0000 Subject: [PATCH 2/5] fix: protect nested watch-root traversal from cross-trigger ignores and apply review fixes Signed-off-by: ManManavadaria --- pkg/watch/watcher_darwin.go | 2 +- pkg/watch/watcher_naive.go | 39 ++++++++++++++++++------ pkg/watch/watcher_naive_test.go | 54 +++++++++++++++++++++++++++++++++ 3 files changed, 84 insertions(+), 11 deletions(-) diff --git a/pkg/watch/watcher_darwin.go b/pkg/watch/watcher_darwin.go index 9486b9af375..e4b6552eba5 100644 --- a/pkg/watch/watcher_darwin.go +++ b/pkg/watch/watcher_darwin.go @@ -115,7 +115,7 @@ func (d *fseventNotify) Errors() chan error { return d.errors } -func newWatcher(paths []string, _ PathMatcher) (Notify, error) { +func newWatcher(paths []string, _ ...PathMatcher) (Notify, error) { dw := &fseventNotify{ stream: &fsevents.EventStream{ Latency: 50 * time.Millisecond, diff --git a/pkg/watch/watcher_naive.go b/pkg/watch/watcher_naive.go index 135e058ce35..0ccd1f33cec 100644 --- a/pkg/watch/watcher_naive.go +++ b/pkg/watch/watcher_naive.go @@ -116,7 +116,7 @@ func (d *naiveNotify) watchRecursively(dir string) error { if err != nil { if os.IsPermission(err) && d.shouldIgnore(path) { logrus.Debugf("Ignoring path: %s", path) - return nil + return filepath.SkipDir } return err } @@ -185,6 +185,10 @@ func (d *naiveNotify) loop() { //nolint:gocyclo // TODO(dbentley): if there's a delete should we call d.watcher.Remove to prevent leaking? err := filepath.WalkDir(e.Name, func(path string, info fs.DirEntry, err error) error { if err != nil { + if os.IsPermission(err) && d.shouldIgnore(path) { + logrus.Debugf("Ignoring path: %s", path) + return filepath.SkipDir + } return err } @@ -245,11 +249,6 @@ func (d *naiveNotify) shouldSkipDir(path string) bool { return false } - // If path is present in the ignore list then we should ignore it - if d.shouldIgnore(path) { - return true - } - // Suppose we're watching // /src/.tiltignore // but the .tiltignore file doesn't exist. @@ -262,15 +261,28 @@ func (d *naiveNotify) shouldSkipDir(path string) bool { // - A child of a directory that's in our notify list, or // - A parent of a directory that's in our notify list // (i.e., to cover the "path doesn't exist" case). + // + // We prioritize "parent of watched path" checks before ignore checks so + // one trigger's ignore rules can't hide another trigger's nested watch root. + isChildOfWatchedDir := false for root := range d.notifyList { - if pathutil.IsChild(root, path) || pathutil.IsChild(path, root) { + if pathutil.IsChild(path, root) { return false } + if pathutil.IsChild(root, path) { + isChildOfWatchedDir = true + } } - return true + + // skip the dir if ignore rules match this full subtree. + if d.shouldIgnoreEntireDir(path) { + return true + } + + return !isChildOfWatchedDir } -func (d *naiveNotify) shouldIgnore(path string) bool { +func (d *naiveNotify) shouldIgnoreEntireDir(path string) bool { if d.ignore == nil { return false } @@ -282,7 +294,14 @@ func (d *naiveNotify) shouldIgnore(path string) bool { if matches { return true } - matches, err = d.ignore.Matches(path) + return false +} + +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 diff --git a/pkg/watch/watcher_naive_test.go b/pkg/watch/watcher_naive_test.go index b188de93903..ede4c0d7ab9 100644 --- a/pkg/watch/watcher_naive_test.go +++ b/pkg/watch/watcher_naive_test.go @@ -157,3 +157,57 @@ func TestDontRecurseWhenWatchingParentsOfNonExistentFiles(t *testing.T) { t.Fatalf("watching more than 5 files: %d", n) } } + +func TestShouldSkipDirWithNegatedChildException(t *testing.T) { + repoRoot := t.TempDir() + ignore, err := DockerIgnoreTesterFromContents(repoRoot, "bazel-bin/\n!bazel-bin/app-binary\n") + assert.NilError(t, err) + + d := &naiveNotify{ + ignore: ignore, + notifyList: map[string]bool{repoRoot: true}, + } + + bazelBin := filepath.Join(repoRoot, "bazel-bin") + assert.Assert(t, !d.shouldSkipDir(bazelBin), "expected bazel-bin to remain traversable for negated child patterns") +} + +func TestShouldIgnorePathStillMatchesDirectoryPattern(t *testing.T) { + repoRoot := t.TempDir() + ignore, err := DockerIgnoreTesterFromContents(repoRoot, "bazel-bin/\n!bazel-bin/app-binary\n") + assert.NilError(t, err) + + d := &naiveNotify{ignore: ignore} + + bazelBin := filepath.Join(repoRoot, "bazel-bin") + assert.Assert(t, d.shouldIgnore(bazelBin), "expected bazel-bin path to match ignore pattern") +} + +func TestShouldSkipDirForIgnoredSubtreeWithoutException(t *testing.T) { + repoRoot := t.TempDir() + ignore, err := DockerIgnoreTesterFromContents(repoRoot, "bazel-bin/\n") + assert.NilError(t, err) + + d := &naiveNotify{ + ignore: ignore, + notifyList: map[string]bool{repoRoot: true}, + } + + bazelBin := filepath.Join(repoRoot, "bazel-bin") + assert.Assert(t, d.shouldSkipDir(bazelBin), "expected fully ignored directory subtree to be skipped") +} + +func TestShouldSkipDirDoesNotSkipAncestorOfWatchedPath(t *testing.T) { + repoRoot := t.TempDir() + ignore, err := DockerIgnoreTesterFromContents(repoRoot, "parent/\n") + assert.NilError(t, err) + + watchedPath := filepath.Join(repoRoot, "parent", "child", "non-existent") + d := &naiveNotify{ + ignore: ignore, + notifyList: map[string]bool{watchedPath: true}, + } + + parent := filepath.Join(repoRoot, "parent") + assert.Assert(t, !d.shouldSkipDir(parent), "expected parent directory to remain traversable when it contains a watched path") +} From 288a42eed84887749838f0b5dfa5b7b22aa89063 Mon Sep 17 00:00:00 2001 From: ManManavadaria Date: Mon, 11 May 2026 11:51:17 +0000 Subject: [PATCH 3/5] Refactor watcher: per-path triggers and intersectPathMatcher - Add multiNotify and watcher methods for grouped per-path watches - Centralize per-path multi-watcher behavior so the naive watcher flow stays the same - Add intersectPathMatcher implementing PathMatcher for shared roots - Remove unnecessary conditions in the naive watcher as per feedback Signed-off-by: ManManavadaria --- pkg/compose/watch.go | 33 ++++++--- pkg/watch/notify.go | 124 ++++++++++++++++++++++++++++++++ pkg/watch/notify_test.go | 40 +++++++++++ pkg/watch/watcher_naive.go | 33 +++------ pkg/watch/watcher_naive_test.go | 27 +++++++ 5 files changed, 224 insertions(+), 33 deletions(-) diff --git a/pkg/compose/watch.go b/pkg/compose/watch.go index 1e22c9479a6..c5dbaaae579 100644 --- a/pkg/compose/watch.go +++ b/pkg/compose/watch.go @@ -198,10 +198,10 @@ func (s *composeService) watch(ctx context.Context, project *types.Project, opti eg, ctx := errgroup.WithContext(ctx) var ( - rules []watchRule - paths []string - ignoreMatchers []watch.PathMatcher + rules []watchRule + watchRootIgnores map[string][]watch.PathMatcher // abs watch path -> per-trigger ignore matchers ) + watchRootIgnores = make(map[string][]watch.PathMatcher) for serviceName, service := range project.Services { config, err := loadDevelopmentConfig(service, project) if err != nil { @@ -259,8 +259,13 @@ func (s *composeService) watch(ctx context.Context, project *types.Project, opti if err != nil { return nil, err } - ignoreMatchers = append(ignoreMatchers, ignore) - paths = append(paths, trigger.Path) + + absPath, err := filepath.Abs(trigger.Path) + if err != nil { + return nil, err + } + absPath = filepath.Clean(absPath) + watchRootIgnores[absPath] = append(watchRootIgnores[absPath], ignore) } serviceWatchRules, err := getWatchRules(config, service) @@ -270,15 +275,25 @@ func (s *composeService) watch(ctx context.Context, project *types.Project, opti rules = append(rules, serviceWatchRules...) } - if len(paths) == 0 { + if len(watchRootIgnores) == 0 { return nil, fmt.Errorf("none of the selected services is configured for watch, consider setting a 'develop' section") } - watcher, err := watch.NewWatcher(paths, watch.NewCompositeMatcher(ignoreMatchers...)) - if err != nil { - return nil, err + watchers := make([]watch.Notify, 0, len(watchRootIgnores)) + for path, matchers := range watchRootIgnores { + merged := watch.NewIntersectMatcher(matchers...) + triggerWatcher, err := watch.NewWatcher([]string{path}, merged) + if err != nil { + for _, w := range watchers { + _ = w.Close() + } + return nil, err + } + watchers = append(watchers, triggerWatcher) } + watcher := watch.NewMultiWatcher(watchers...) + err = watcher.Start() if err != nil { return nil, err diff --git a/pkg/watch/notify.go b/pkg/watch/notify.go index 71dcaf1156f..7e74b58c1c6 100644 --- a/pkg/watch/notify.go +++ b/pkg/watch/notify.go @@ -17,11 +17,13 @@ package watch import ( + "errors" "expvar" "fmt" "os" "path/filepath" "strconv" + "sync" ) var numberOfWatches = expvar.NewInt("watch.naive.numberOfWatches") @@ -84,6 +86,83 @@ func NewWatcher(paths []string, ignore PathMatcher) (Notify, error) { return newWatcher(paths, ignore) } +type multiNotify struct { + children []Notify + events chan FileEvent + errors chan error +} + +func NewMultiWatcher(children ...Notify) Notify { + return &multiNotify{ + children: children, + events: make(chan FileEvent), + errors: make(chan error), + } +} + +func (m *multiNotify) Start() error { + for i := range m.children { + if err := m.children[i].Start(); err != nil { + for j := 0; j < i; j++ { + _ = m.children[j].Close() + } + return err + } + } + + var eventsWG sync.WaitGroup + eventsWG.Add(len(m.children)) + for i := range m.children { + child := m.children[i] + go func() { + defer eventsWG.Done() + for e := range child.Events() { + m.events <- e + } + }() + } + go func() { + eventsWG.Wait() + close(m.events) + }() + + var errorsWG sync.WaitGroup + errorsWG.Add(len(m.children)) + for i := range m.children { + child := m.children[i] + go func() { + defer errorsWG.Done() + for err := range child.Errors() { + m.errors <- err + } + }() + } + go func() { + errorsWG.Wait() + close(m.errors) + }() + + return nil +} + +func (m *multiNotify) Close() error { + var errs []error + for _, child := range m.children { + if err := child.Close(); err != nil { + errs = append(errs, err) + } + } + return errors.Join(errs...) +} + +func (m *multiNotify) Events() chan FileEvent { + return m.events +} + +func (m *multiNotify) Errors() chan error { + return m.errors +} + const WindowsBufferSizeEnvVar = "COMPOSE_WATCH_WINDOWS_BUFFER_SIZE" const defaultBufferSize int = 65536 @@ -134,3 +213,48 @@ func (c CompositePathMatcher) MatchesEntireDir(f string) (bool, error) { } var _ PathMatcher = CompositePathMatcher{} + +// intersectPathMatcher matches iff every matcher matches. With several develop.watch +// triggers on one watch root, skip/filter a path only when every trigger's ignores agree. +type intersectPathMatcher struct { + Matchers []PathMatcher +} + +// NewIntersectMatcher returns a PathMatcher that matches iff every matcher matches. +func NewIntersectMatcher(matchers ...PathMatcher) PathMatcher { + if len(matchers) == 0 { + return EmptyMatcher{} + } + if len(matchers) == 1 { + return matchers[0] + } + return intersectPathMatcher{Matchers: matchers} +} + +func (i intersectPathMatcher) Matches(f string) (bool, error) { + for _, t := range i.Matchers { + ret, err := t.Matches(f) + if err != nil { + return false, err + } + if !ret { + return false, nil + } + } + return true, nil +} + +func (i intersectPathMatcher) MatchesEntireDir(f string) (bool, error) { + for _, t := range i.Matchers { + ret, err := t.MatchesEntireDir(f) + if err != nil { + return false, err + } + if !ret { + return false, nil + } + } + return true, nil +} + +var _ PathMatcher = intersectPathMatcher{} diff --git a/pkg/watch/notify_test.go b/pkg/watch/notify_test.go index 0e9a7bc63bf..dc4dd31f115 100644 --- a/pkg/watch/notify_test.go +++ b/pkg/watch/notify_test.go @@ -50,6 +50,46 @@ func TestWindowsBufferSize(t *testing.T) { }) } +func TestNewIntersectMatcher(t *testing.T) { + root := t.TempDir() + + vendorOnly, err := DockerIgnoreTesterFromContents(root, "vendor/\n") + assert.NilError(t, err) + tmpOnly, err := DockerIgnoreTesterFromContents(root, "tmp/\n") + assert.NilError(t, err) + + inter := NewIntersectMatcher(vendorOnly, tmpOnly) + + vendorFile := filepath.Join(root, "vendor", "a.go") + matches, err := inter.Matches(vendorFile) + assert.NilError(t, err) + assert.Assert(t, !matches, "only one trigger ignores vendor; intersection must not treat path as ignored") + + bothIgnoreBuild1, err := DockerIgnoreTesterFromContents(root, "build/\n") + assert.NilError(t, err) + bothIgnoreBuild2, err := DockerIgnoreTesterFromContents(root, "build/\n") + assert.NilError(t, err) + interBuild := NewIntersectMatcher(bothIgnoreBuild1, bothIgnoreBuild2) + buildFile := filepath.Join(root, "build", "out") + matches, err = interBuild.Matches(buildFile) + assert.NilError(t, err) + assert.Assert(t, matches) + + dirEntire1, err := DockerIgnoreTesterFromContents(root, "cache/\n") + assert.NilError(t, err) + dirEntire2, err := DockerIgnoreTesterFromContents(root, "cache/\n") + assert.NilError(t, err) + interDir := NewIntersectMatcher(dirEntire1, dirEntire2) + entire, err := interDir.MatchesEntireDir(filepath.Join(root, "cache")) + assert.NilError(t, err) + assert.Assert(t, entire) + + partialEntire := NewIntersectMatcher(vendorOnly, tmpOnly) + entire, err = partialEntire.MatchesEntireDir(filepath.Join(root, "vendor")) + assert.NilError(t, err) + assert.Assert(t, !entire, "must not skip whole dir unless every matcher agrees it is entirely ignorable") +} + func TestNoEvents(t *testing.T) { f := newNotifyFixture(t) f.assertEvents() diff --git a/pkg/watch/watcher_naive.go b/pkg/watch/watcher_naive.go index 0ccd1f33cec..f3d5cbbd244 100644 --- a/pkg/watch/watcher_naive.go +++ b/pkg/watch/watcher_naive.go @@ -114,10 +114,6 @@ func (d *naiveNotify) watchRecursively(dir string) error { return filepath.WalkDir(dir, func(path string, info fs.DirEntry, err error) error { if err != nil { - if os.IsPermission(err) && d.shouldIgnore(path) { - logrus.Debugf("Ignoring path: %s", path) - return filepath.SkipDir - } return err } @@ -185,10 +181,6 @@ func (d *naiveNotify) loop() { //nolint:gocyclo // TODO(dbentley): if there's a delete should we call d.watcher.Remove to prevent leaking? err := filepath.WalkDir(e.Name, func(path string, info fs.DirEntry, err error) error { if err != nil { - if os.IsPermission(err) && d.shouldIgnore(path) { - logrus.Debugf("Ignoring path: %s", path) - return filepath.SkipDir - } return err } @@ -228,6 +220,10 @@ func (d *naiveNotify) loop() { //nolint:gocyclo } func (d *naiveNotify) shouldNotify(path string) bool { + if d.shouldIgnore(path) { + return false + } + if _, ok := d.notifyList[path]; ok { // We generally don't care when directories change at the root of an ADD stat, err := os.Lstat(path) @@ -249,21 +245,10 @@ func (d *naiveNotify) shouldSkipDir(path string) bool { return false } - // Suppose we're watching - // /src/.tiltignore - // but the .tiltignore file doesn't exist. - // - // Our watcher will create an inotify watch on /src/. - // - // But then we want to make sure we don't recurse from /src/ down to /src/node_modules. - // - // To handle this case, we only want to traverse dirs that are: - // - A child of a directory that's in our notify list, or - // - A parent of a directory that's in our notify list - // (i.e., to cover the "path doesn't exist" case). - // - // We prioritize "parent of watched path" checks before ignore checks so - // one trigger's ignore rules can't hide another trigger's nested watch root. + // Only walk directories that are under a notifyList path, or that are under an ancestor of one + // (Start() may watch a parent when the target is missing or is a file). + // Check ancestor/descendant vs notifyList before ignores + // so ignore patterns cannot block reaching the watched root. isChildOfWatchedDir := false for root := range d.notifyList { if pathutil.IsChild(path, root) { @@ -294,7 +279,7 @@ func (d *naiveNotify) shouldIgnoreEntireDir(path string) bool { if matches { return true } - return false + return matches } func (d *naiveNotify) shouldIgnore(path string) bool { diff --git a/pkg/watch/watcher_naive_test.go b/pkg/watch/watcher_naive_test.go index ede4c0d7ab9..ed07a4cff64 100644 --- a/pkg/watch/watcher_naive_test.go +++ b/pkg/watch/watcher_naive_test.go @@ -211,3 +211,30 @@ func TestShouldSkipDirDoesNotSkipAncestorOfWatchedPath(t *testing.T) { parent := filepath.Join(repoRoot, "parent") assert.Assert(t, !d.shouldSkipDir(parent), "expected parent directory to remain traversable when it contains a watched path") } + +func TestShouldSkipDirIntersectRequiresAllTriggersToAgree(t *testing.T) { + repoRoot := t.TempDir() + ignoreVendor, err := DockerIgnoreTesterFromContents(repoRoot, "vendor/\n") + assert.NilError(t, err) + ignoreTmp, err := DockerIgnoreTesterFromContents(repoRoot, "tmp/\n") + assert.NilError(t, err) + + d := &naiveNotify{ + ignore: NewIntersectMatcher(ignoreVendor, ignoreTmp), + notifyList: map[string]bool{repoRoot: true}, + } + + vendorDir := filepath.Join(repoRoot, "vendor") + assert.Assert(t, !d.shouldSkipDir(vendorDir), "vendor must remain watched when another trigger does not ignore it") + + ignoreBuild1, err := DockerIgnoreTesterFromContents(repoRoot, "build/\n") + assert.NilError(t, err) + ignoreBuild2, err := DockerIgnoreTesterFromContents(repoRoot, "build/\n") + assert.NilError(t, err) + d2 := &naiveNotify{ + ignore: NewIntersectMatcher(ignoreBuild1, ignoreBuild2), + notifyList: map[string]bool{repoRoot: true}, + } + buildDir := filepath.Join(repoRoot, "build") + assert.Assert(t, d2.shouldSkipDir(buildDir), "when every trigger ignores the same subtree, watcher may skip it") +} From a768660d47092750ec231ca4b558a2b5e06b7719 Mon Sep 17 00:00:00 2001 From: ManManavadaria Date: Mon, 11 May 2026 13:17:57 +0000 Subject: [PATCH 4/5] Implement PathMatcher ignores in Darwin FSEvents watcher - Store PathMatcher on fseventNotify and filter events with shouldIgnore and shouldNotify, matching the naive watcher Signed-off-by: ManManavadaria --- pkg/watch/watcher_darwin.go | 40 ++++++++++- pkg/watch/watcher_darwin_test.go | 110 ++++++++++++++++++++++++++++++- 2 files changed, 148 insertions(+), 2 deletions(-) diff --git a/pkg/watch/watcher_darwin.go b/pkg/watch/watcher_darwin.go index e4b6552eba5..ad33e82cebd 100644 --- a/pkg/watch/watcher_darwin.go +++ b/pkg/watch/watcher_darwin.go @@ -26,6 +26,7 @@ import ( "time" "github.com/fsnotify/fsevents" + "github.com/sirupsen/logrus" pathutil "github.com/docker/compose/v5/internal/paths" ) @@ -39,6 +40,7 @@ type fseventNotify struct { stop chan struct{} pathsWereWatching map[string]any + ignore PathMatcher closeOnce sync.Once } @@ -62,6 +64,10 @@ func (d *fseventNotify) loop() { continue } + if !d.shouldNotify(e.Path) { + continue + } + d.events <- NewFileEvent(e.Path) } } @@ -115,7 +121,38 @@ func (d *fseventNotify) Errors() chan error { return d.errors } -func newWatcher(paths []string, _ ...PathMatcher) (Notify, error) { +func (d *fseventNotify) shouldNotify(path string) bool { + if d.shouldIgnore(path) { + return false + } + + if _, ok := d.pathsWereWatching[path]; ok { + stat, err := os.Lstat(path) + isDir := err == nil && stat.IsDir() + return !isDir + } + + for root := range d.pathsWereWatching { + if pathutil.IsChild(root, path) { + return true + } + } + return false +} + +func (d *fseventNotify) 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 +} + +func newWatcher(paths []string, ignore PathMatcher) (Notify, error) { dw := &fseventNotify{ stream: &fsevents.EventStream{ Latency: 50 * time.Millisecond, @@ -127,6 +164,7 @@ func newWatcher(paths []string, _ ...PathMatcher) (Notify, error) { events: make(chan FileEvent), errors: make(chan error), stop: make(chan struct{}), + ignore: ignore, } paths = pathutil.EncompassingPaths(paths) diff --git a/pkg/watch/watcher_darwin_test.go b/pkg/watch/watcher_darwin_test.go index 50eb442b06b..e53d57a05f7 100644 --- a/pkg/watch/watcher_darwin_test.go +++ b/pkg/watch/watcher_darwin_test.go @@ -19,15 +19,24 @@ package watch import ( + "os" + "path/filepath" "testing" "gotest.tools/v3/assert" ) +func newFseventNotifyFixture(repo string, ignore PathMatcher) *fseventNotify { + return &fseventNotify{ + pathsWereWatching: map[string]any{repo: struct{}{}}, + ignore: ignore, + } +} + func TestFseventNotifyCloseIdempotent(t *testing.T) { // Create a watcher with a temporary directory tmpDir := t.TempDir() - watcher, err := newWatcher([]string{tmpDir}) + watcher, err := newWatcher([]string{tmpDir}, nil) assert.NilError(t, err) // Start the watcher @@ -46,3 +55,102 @@ func TestFseventNotifyCloseIdempotent(t *testing.T) { err = watcher.Close() assert.NilError(t, err) } + +func TestFseventNotifyShouldNotifyNilIgnore(t *testing.T) { + repo := t.TempDir() + child := filepath.Join(repo, "child.txt") + assert.NilError(t, os.WriteFile(child, []byte("x"), 0o644)) + + d := newFseventNotifyFixture(repo, nil) + assert.Assert(t, d.shouldNotify(child), "expected file under watched root to notify") + assert.Assert(t, !d.shouldNotify(repo), "expected directory event at watched root to be skipped") +} + +func TestFseventNotifyShouldNotifyWatchedFileRoot(t *testing.T) { + repo := t.TempDir() + fileRoot := filepath.Join(repo, "watched.go") + assert.NilError(t, os.WriteFile(fileRoot, []byte("package main\n"), 0o644)) + + d := newFseventNotifyFixture(fileRoot, nil) + assert.Assert(t, d.shouldNotify(fileRoot), "expected file that is the watch root to notify") +} + +func TestFseventNotifyShouldNotifyOutsideWatchedTree(t *testing.T) { + repo := t.TempDir() + other := t.TempDir() + + d := newFseventNotifyFixture(repo, nil) + outPath := filepath.Join(other, "outside.txt") + assert.NilError(t, os.WriteFile(outPath, []byte("x"), 0o644)) + assert.Assert(t, !d.shouldNotify(outPath), "expected path outside watch roots not to notify") +} + +func TestFseventNotifyShouldNotifyRespectsDockerignore(t *testing.T) { + repo := t.TempDir() + ignore, err := DockerIgnoreTesterFromContents(repo, "vendor/\n") + assert.NilError(t, err) + + d := newFseventNotifyFixture(repo, ignore) + kept := filepath.Join(repo, "src", "main.go") + assert.NilError(t, os.MkdirAll(filepath.Dir(kept), 0o755)) + assert.NilError(t, os.WriteFile(kept, []byte("x"), 0o644)) + assert.Assert(t, d.shouldNotify(kept), "expected non-ignored path to notify") + + ignored := filepath.Join(repo, "vendor", "mod", "x.go") + assert.NilError(t, os.MkdirAll(filepath.Dir(ignored), 0o755)) + assert.NilError(t, os.WriteFile(ignored, []byte("x"), 0o644)) + assert.Assert(t, !d.shouldNotify(ignored), "expected dockerignored path not to notify") +} + +func TestFseventNotifyShouldNotifyDockerignoreNegation(t *testing.T) { + repo := t.TempDir() + ignore, err := DockerIgnoreTesterFromContents(repo, "bazel-bin/\n!bazel-bin/app-binary\n") + assert.NilError(t, err) + + d := newFseventNotifyFixture(repo, ignore) + + ignoredChild := filepath.Join(repo, "bazel-bin", "cache", "out") + assert.NilError(t, os.MkdirAll(filepath.Dir(ignoredChild), 0o755)) + assert.NilError(t, os.WriteFile(ignoredChild, []byte("x"), 0o644)) + assert.Assert(t, !d.shouldNotify(ignoredChild), "expected ignored subtree under bazel-bin not to notify") + + excepted := filepath.Join(repo, "bazel-bin", "app-binary", "binary") + assert.NilError(t, os.MkdirAll(filepath.Dir(excepted), 0o755)) + assert.NilError(t, os.WriteFile(excepted, []byte("x"), 0o644)) + assert.Assert(t, d.shouldNotify(excepted), "expected negated dockerignore path to notify") +} + +func TestFseventNotifyShouldNotifyIntersectMatcher(t *testing.T) { + repo := t.TempDir() + ignoreVendor, err := DockerIgnoreTesterFromContents(repo, "vendor/\n") + assert.NilError(t, err) + ignoreTmp, err := DockerIgnoreTesterFromContents(repo, "tmp/\n") + assert.NilError(t, err) + + d := newFseventNotifyFixture(repo, NewIntersectMatcher(ignoreVendor, ignoreTmp)) + vendorFile := filepath.Join(repo, "vendor", "x", "go.mod") + assert.NilError(t, os.MkdirAll(filepath.Dir(vendorFile), 0o755)) + assert.NilError(t, os.WriteFile(vendorFile, []byte("module x\n"), 0o644)) + assert.Assert(t, d.shouldNotify(vendorFile), "vendor must notify when only one intersect matcher ignores it") + + ignoreBuild1, err := DockerIgnoreTesterFromContents(repo, "build/\n") + assert.NilError(t, err) + ignoreBuild2, err := DockerIgnoreTesterFromContents(repo, "build/\n") + assert.NilError(t, err) + d2 := newFseventNotifyFixture(repo, NewIntersectMatcher(ignoreBuild1, ignoreBuild2)) + buildFile := filepath.Join(repo, "build", "out", "a") + assert.NilError(t, os.MkdirAll(filepath.Dir(buildFile), 0o755)) + assert.NilError(t, os.WriteFile(buildFile, []byte("x"), 0o644)) + assert.Assert(t, !d2.shouldNotify(buildFile), "expected path ignored by every intersect matcher not to notify") +} + +func TestFseventNotifyShouldIgnoreDockerignoreDirectory(t *testing.T) { + repo := t.TempDir() + ignore, err := DockerIgnoreTesterFromContents(repo, "bazel-bin/\n!bazel-bin/app-binary\n") + assert.NilError(t, err) + + d := newFseventNotifyFixture(repo, ignore) + bazelBin := filepath.Join(repo, "bazel-bin") + assert.NilError(t, os.MkdirAll(bazelBin, 0o755)) + assert.Assert(t, d.shouldIgnore(bazelBin), "expected directory path to match dockerignore") +} From eee815e3db2f1fba033a6f8e867f743dd316cea9 Mon Sep 17 00:00:00 2001 From: ManManavadaria Date: Thu, 14 May 2026 18:17:32 +0000 Subject: [PATCH 5/5] fix(watch): scope ignore rules to each watch root Use one watcher with per-root ignore maps so overlapping triggers do not apply another root's patterns when pruning or filtering events. Intersect matchers when multiple triggers share the same path. - Start a single watcher from compose watch and drop NewMultiWatcher/multiNotify - Pass map[path]PathMatcher into NewWatcher and normalize ignores as roots change - Merge matchers when paths are promoted to existing ancestors or nested roots - Apply subtree pruning and path filtering with the matcher for the matching root - Align Darwin watcher startup and ignore handling with the naive watcher - Add tests for per-root isolation, intersected ignores Signed-off-by: ManManavadaria --- pkg/compose/watch.go | 39 +++----- pkg/watch/notify.go | 81 +--------------- pkg/watch/notify_test.go | 87 +++++++++++++++++- pkg/watch/paths.go | 47 ++++++++++ pkg/watch/paths_test.go | 152 +++++++++++++++++++++++++++++++ pkg/watch/watcher_darwin.go | 79 ++++++++++------ pkg/watch/watcher_darwin_test.go | 26 ++++-- pkg/watch/watcher_naive.go | 108 +++++++++++----------- pkg/watch/watcher_naive_test.go | 41 +++++++-- 9 files changed, 456 insertions(+), 204 deletions(-) diff --git a/pkg/compose/watch.go b/pkg/compose/watch.go index c5dbaaae579..cc9ca37aceb 100644 --- a/pkg/compose/watch.go +++ b/pkg/compose/watch.go @@ -198,10 +198,12 @@ func (s *composeService) watch(ctx context.Context, project *types.Project, opti eg, ctx := errgroup.WithContext(ctx) var ( - rules []watchRule - watchRootIgnores map[string][]watch.PathMatcher // abs watch path -> per-trigger ignore matchers + rules []watchRule + paths []string + ignoresByWatchPath map[string]watch.PathMatcher ) - watchRootIgnores = make(map[string][]watch.PathMatcher) + ignoresByWatchPath = make(map[string]watch.PathMatcher) + for serviceName, service := range project.Services { config, err := loadDevelopmentConfig(service, project) if err != nil { @@ -255,19 +257,18 @@ func (s *composeService) watch(ctx context.Context, project *types.Project, opti } } } - ignore, err := watch.NewDockerPatternMatcher(trigger.Path, trigger.Ignore) + var ignore watch.PathMatcher + ignore, err = watch.NewDockerPatternMatcher(trigger.Path, trigger.Ignore) if err != nil { return nil, err } - absPath, err := filepath.Abs(trigger.Path) - if err != nil { - return nil, err + if existingMatcher, exists := ignoresByWatchPath[trigger.Path]; exists { + ignore = watch.NewIntersectMatcher(existingMatcher, ignore) } - absPath = filepath.Clean(absPath) - watchRootIgnores[absPath] = append(watchRootIgnores[absPath], ignore) + ignoresByWatchPath[trigger.Path] = ignore + paths = append(paths, trigger.Path) } - serviceWatchRules, err := getWatchRules(config, service) if err != nil { return nil, err @@ -275,25 +276,15 @@ func (s *composeService) watch(ctx context.Context, project *types.Project, opti rules = append(rules, serviceWatchRules...) } - if len(watchRootIgnores) == 0 { + if len(paths) == 0 { return nil, fmt.Errorf("none of the selected services is configured for watch, consider setting a 'develop' section") } - watchers := make([]watch.Notify, 0, len(watchRootIgnores)) - for path, matchers := range watchRootIgnores { - merged := watch.NewIntersectMatcher(matchers...) - triggerWatcher, err := watch.NewWatcher([]string{path}, merged) - if err != nil { - for _, w := range watchers { - _ = w.Close() - } - return nil, err - } - watchers = append(watchers, triggerWatcher) + watcher, err := watch.NewWatcher(paths, ignoresByWatchPath) + if err != nil { + return nil, err } - watcher := watch.NewMultiWatcher(watchers...) - err = watcher.Start() if err != nil { return nil, err diff --git a/pkg/watch/notify.go b/pkg/watch/notify.go index 7e74b58c1c6..de91721a3be 100644 --- a/pkg/watch/notify.go +++ b/pkg/watch/notify.go @@ -17,13 +17,11 @@ package watch import ( - "errors" "expvar" "fmt" "os" "path/filepath" "strconv" - "sync" ) var numberOfWatches = expvar.NewInt("watch.naive.numberOfWatches") @@ -82,87 +80,10 @@ func (EmptyMatcher) MatchesEntireDir(f string) (bool, error) { return false, nil var _ PathMatcher = EmptyMatcher{} -func NewWatcher(paths []string, ignore PathMatcher) (Notify, error) { +func NewWatcher(paths []string, ignore map[string]PathMatcher) (Notify, error) { return newWatcher(paths, ignore) } -type multiNotify struct { - children []Notify - events chan FileEvent - errors chan error -} - -func NewMultiWatcher(children ...Notify) Notify { - return &multiNotify{ - children: children, - events: make(chan FileEvent), - errors: make(chan error), - } -} - -func (m *multiNotify) Start() error { - for i := range m.children { - if err := m.children[i].Start(); err != nil { - for j := 0; j < i; j++ { - _ = m.children[j].Close() - } - return err - } - } - - var eventsWG sync.WaitGroup - eventsWG.Add(len(m.children)) - for i := range m.children { - child := m.children[i] - go func() { - defer eventsWG.Done() - for e := range child.Events() { - m.events <- e - } - }() - } - go func() { - eventsWG.Wait() - close(m.events) - }() - - var errorsWG sync.WaitGroup - errorsWG.Add(len(m.children)) - for i := range m.children { - child := m.children[i] - go func() { - defer errorsWG.Done() - for err := range child.Errors() { - m.errors <- err - } - }() - } - go func() { - errorsWG.Wait() - close(m.errors) - }() - - return nil -} - -func (m *multiNotify) Close() error { - var errs []error - for _, child := range m.children { - if err := child.Close(); err != nil { - errs = append(errs, err) - } - } - return errors.Join(errs...) -} - -func (m *multiNotify) Events() chan FileEvent { - return m.events -} - -func (m *multiNotify) Errors() chan error { - return m.errors -} - const WindowsBufferSizeEnvVar = "COMPOSE_WATCH_WINDOWS_BUFFER_SIZE" const defaultBufferSize int = 65536 diff --git a/pkg/watch/notify_test.go b/pkg/watch/notify_test.go index dc4dd31f115..1ecf22e2d09 100644 --- a/pkg/watch/notify_test.go +++ b/pkg/watch/notify_test.go @@ -90,6 +90,84 @@ func TestNewIntersectMatcher(t *testing.T) { assert.Assert(t, !entire, "must not skip whole dir unless every matcher agrees it is entirely ignorable") } +func TestWatchRespectsDockerignore(t *testing.T) { + f := newNotifyFixture(t) + + root := f.TempDir("root") + ignore, err := DockerIgnoreTesterFromContents(root, "vendor/\n") + assert.NilError(t, err) + + f.ignores = map[string]PathMatcher{root: ignore} + f.watch(root) + f.fsync() + f.events = nil + + kept := filepath.Join(root, "src", "main.go") + f.WriteFile(kept, "package main\n") + f.assertEvents(filepath.Join(root, "src"), kept) + f.events = nil + + ignored := filepath.Join(root, "vendor", "mod", "x.go") + f.WriteFile(ignored, "module x\n") + f.assertEvents() +} + +func TestWatchPerRootIgnoresDoNotLeak(t *testing.T) { + f := newNotifyFixture(t) + + rootA := f.TempDir("root-a") + rootB := f.TempDir("root-b") + ignoreA, err := DockerIgnoreTesterFromContents(rootA, "vendor/\n") + assert.NilError(t, err) + + f.ignores = map[string]PathMatcher{rootA: ignoreA} + f.watch(rootA) + f.watch(rootB) + f.fsync() + f.events = nil + + ignoredUnderA := filepath.Join(rootA, "vendor", "x.go") + f.WriteFile(ignoredUnderA, "ignored\n") + f.assertEvents() + + keptUnderB := filepath.Join(rootB, "vendor", "x.go") + f.WriteFile(keptUnderB, "kept\n") + f.assertEvents(filepath.Join(rootB, "vendor"), keptUnderB) +} + +func TestWatchIntersectMatcherRequiresAllTriggers(t *testing.T) { + f := newNotifyFixture(t) + + root := f.TempDir("root") + ignoreVendor, err := DockerIgnoreTesterFromContents(root, "vendor/\n") + assert.NilError(t, err) + ignoreTmp, err := DockerIgnoreTesterFromContents(root, "tmp/\n") + assert.NilError(t, err) + + f.ignores = map[string]PathMatcher{root: NewIntersectMatcher(ignoreVendor, ignoreTmp)} + f.watch(root) + f.fsync() + f.events = nil + + vendorFile := filepath.Join(root, "vendor", "x", "go.mod") + f.WriteFile(vendorFile, "module x\n") + f.assertEvents(filepath.Join(root, "vendor"), filepath.Join(root, "vendor", "x"), vendorFile) + + ignoreBuild1, err := DockerIgnoreTesterFromContents(root, "build/\n") + assert.NilError(t, err) + ignoreBuild2, err := DockerIgnoreTesterFromContents(root, "build/\n") + assert.NilError(t, err) + + f.ignores = map[string]PathMatcher{root: NewIntersectMatcher(ignoreBuild1, ignoreBuild2)} + f.rebuildWatcher() + f.fsync() + f.events = nil + + buildFile := filepath.Join(root, "build", "out", "a") + f.WriteFile(buildFile, "artifact\n") + f.assertEvents() +} + func TestNoEvents(t *testing.T) { f := newNotifyFixture(t) f.assertEvents() @@ -533,9 +611,10 @@ type notifyFixture struct { cancel func() out *bytes.Buffer *TempDirFixture - notify Notify - paths []string - events []FileEvent + notify Notify + paths []string + ignores map[string]PathMatcher + events []FileEvent } func newNotifyFixture(t *testing.T) *notifyFixture { @@ -566,7 +645,7 @@ func (f *notifyFixture) rebuildWatcher() { } // create a new watcher - notify, err := NewWatcher(f.paths, EmptyMatcher{}) + notify, err := NewWatcher(f.paths, f.ignores) if err != nil { f.T().Fatal(err) } diff --git a/pkg/watch/paths.go b/pkg/watch/paths.go index c0c893cd995..c4213a24b16 100644 --- a/pkg/watch/paths.go +++ b/pkg/watch/paths.go @@ -20,6 +20,8 @@ import ( "fmt" "os" "path/filepath" + + pathutil "github.com/docker/compose/v5/internal/paths" ) func greatestExistingAncestor(path string) (string, error) { @@ -39,3 +41,48 @@ func greatestExistingAncestor(path string) (string, error) { return path, nil } + +func greatestExistingAncestors(paths []string, ignoreList map[string]PathMatcher) ([]string, error) { + result := []string{} + for _, path := range paths { + newP, err := greatestExistingAncestor(path) + if err != nil { + return nil, fmt.Errorf("finding ancestor of %s: %w", path, err) + } + result = append(result, newP) + if path != newP { + ignore := ignoreList[path] + if oldMatcher, exists := ignoreList[newP]; exists { + ignore = NewIntersectMatcher(oldMatcher, ignore) + } + ignoreList[newP] = ignore + delete(ignoreList, path) + } + } + return result, nil +} + +func normalizeWatchRoots(paths []string, ignore map[string]PathMatcher) (map[string]bool, map[string]PathMatcher, error) { + notifyList := make(map[string]bool, len(paths)) + normalizedIgnores := make(map[string]PathMatcher, len(paths)) + + for _, root := range paths { + root, err := filepath.Abs(root) + if err != nil { + return nil, nil, err + } + notifyList[root] = true + + matchers := make([]PathMatcher, 0, len(ignore)) + for triggerPath, matcher := range ignore { + if matcher == nil { + continue + } + if root == triggerPath || pathutil.IsChild(root, triggerPath) || pathutil.IsChild(triggerPath, root) { + matchers = append(matchers, matcher) + } + } + normalizedIgnores[root] = NewIntersectMatcher(matchers...) + } + return notifyList, normalizedIgnores, nil +} diff --git a/pkg/watch/paths_test.go b/pkg/watch/paths_test.go index 60b4a15e7b5..9bc35dd6edc 100644 --- a/pkg/watch/paths_test.go +++ b/pkg/watch/paths_test.go @@ -17,6 +17,7 @@ package watch import ( + "path/filepath" "runtime" "testing" @@ -41,3 +42,154 @@ func TestGreatestExistingAncestor(t *testing.T) { _, err = greatestExistingAncestor(missingTopLevel) assert.ErrorContains(t, err, "cannot watch root directory") } + +func TestGreatestExistingAncestorsMovesIgnoreToAncestor(t *testing.T) { + f := NewTempDirFixture(t) + + missing := f.JoinPath("missing", "child", "file.txt") + ignore, err := DockerIgnoreTesterFromContents(f.Path(), "vendor/\n") + assert.NilError(t, err) + + ignoreList := map[string]PathMatcher{missing: ignore} + paths, err := greatestExistingAncestors([]string{missing}, ignoreList) + assert.NilError(t, err) + assert.Equal(t, 1, len(paths)) + assert.Equal(t, f.Path(), paths[0]) + assert.Assert(t, ignoreList[f.Path()] != nil) + _, exists := ignoreList[missing] + assert.Assert(t, !exists) +} + +func TestGreatestExistingAncestorsIntersectsIgnoreOnAncestor(t *testing.T) { + f := NewTempDirFixture(t) + + missing := f.JoinPath("missing", "child", "file.txt") + vendorIgnore, err := DockerIgnoreTesterFromContents(f.Path(), "vendor/\n") + assert.NilError(t, err) + tmpIgnore, err := DockerIgnoreTesterFromContents(f.Path(), "tmp/\n") + assert.NilError(t, err) + + ignoreList := map[string]PathMatcher{ + f.Path(): vendorIgnore, + missing: tmpIgnore, + } + paths, err := greatestExistingAncestors([]string{missing}, ignoreList) + assert.NilError(t, err) + assert.Equal(t, 1, len(paths)) + assert.Equal(t, f.Path(), paths[0]) + + inter, ok := ignoreList[f.Path()].(intersectPathMatcher) + assert.Assert(t, ok) + assert.Equal(t, 2, len(inter.Matchers)) +} + +func TestNormalizeWatchRootsAbsolutizesPaths(t *testing.T) { + rel := "." + abs, err := filepath.Abs(rel) + assert.NilError(t, err) + + notifyList, _, err := normalizeWatchRoots([]string{rel}, nil) + assert.NilError(t, err) + assert.Assert(t, notifyList[abs]) +} + +func TestNormalizeWatchRootsAssignsRelatedIgnores(t *testing.T) { + f := NewTempDirFixture(t) + + root := f.Path() + child := f.JoinPath("child") + vendorIgnore, err := DockerIgnoreTesterFromContents(root, "vendor/\n") + assert.NilError(t, err) + unrelatedIgnore, err := DockerIgnoreTesterFromContents(root, "build/\n") + assert.NilError(t, err) + + ignores := map[string]PathMatcher{ + root: vendorIgnore, + child: vendorIgnore, + "/other": unrelatedIgnore, + } + notifyList, normalizedIgnores, err := normalizeWatchRoots([]string{root, child}, ignores) + assert.NilError(t, err) + assert.Assert(t, notifyList[root]) + assert.Assert(t, notifyList[child]) + + vendorFile := filepath.Join(root, "vendor", "mod.go") + matches, err := normalizedIgnores[root].Matches(vendorFile) + assert.NilError(t, err) + assert.Assert(t, matches) + + matches, err = normalizedIgnores[child].Matches(vendorFile) + assert.NilError(t, err) + assert.Assert(t, matches) + + buildFile := filepath.Join(root, "build", "out") + matches, err = normalizedIgnores[root].Matches(buildFile) + assert.NilError(t, err) + assert.Assert(t, !matches) +} + +func TestNormalizeWatchRootsSkipsNilMatchers(t *testing.T) { + f := NewTempDirFixture(t) + + root := f.Path() + notifyList, normalizedIgnores, err := normalizeWatchRoots([]string{root}, map[string]PathMatcher{root: nil}) + assert.NilError(t, err) + assert.Assert(t, notifyList[root]) + _, ok := normalizedIgnores[root].(EmptyMatcher) + assert.Assert(t, ok) +} + +func TestNormalizeWatchRootsUsesEmptyMatcherWithoutIgnores(t *testing.T) { + f := NewTempDirFixture(t) + + root := f.Path() + _, normalizedIgnores, err := normalizeWatchRoots([]string{root}, nil) + assert.NilError(t, err) + _, ok := normalizedIgnores[root].(EmptyMatcher) + assert.Assert(t, ok) +} + +func TestNormalizeWatchRootsInheritsParentIgnoreForChild(t *testing.T) { + f := NewTempDirFixture(t) + + root := f.Path() + child := f.JoinPath("pkg") + vendorIgnore, err := DockerIgnoreTesterFromContents(root, "pkg/vendor/\n") + assert.NilError(t, err) + + _, normalizedIgnores, err := normalizeWatchRoots([]string{child}, map[string]PathMatcher{root: vendorIgnore}) + assert.NilError(t, err) + + vendorFile := filepath.Join(child, "vendor", "x.go") + matches, err := normalizedIgnores[child].Matches(vendorFile) + assert.NilError(t, err) + assert.Assert(t, matches) +} + +func TestNormalizeWatchRootsIntersectsNestedIgnores(t *testing.T) { + f := NewTempDirFixture(t) + + root := f.Path() + child := f.JoinPath("pkg") + vendorIgnore, err := DockerIgnoreTesterFromContents(root, "vendor/\n") + assert.NilError(t, err) + tmpIgnore, err := DockerIgnoreTesterFromContents(root, "pkg/tmp/\n") + assert.NilError(t, err) + + ignores := map[string]PathMatcher{ + root: vendorIgnore, + child: tmpIgnore, + } + _, normalizedIgnores, err := normalizeWatchRoots([]string{root, child}, ignores) + assert.NilError(t, err) + + vendorFile := filepath.Join(root, "vendor", "x.go") + matches, err := normalizedIgnores[root].Matches(vendorFile) + assert.NilError(t, err) + assert.Assert(t, !matches, "nested ignores must all match for parent root") + + tmpUnderChild := filepath.Join(child, "tmp", "a") + matches, err = normalizedIgnores[child].Matches(tmpUnderChild) + assert.NilError(t, err) + assert.Assert(t, !matches, "nested ignores must all match for child root") +} diff --git a/pkg/watch/watcher_darwin.go b/pkg/watch/watcher_darwin.go index ad33e82cebd..d7fd71ac3e4 100644 --- a/pkg/watch/watcher_darwin.go +++ b/pkg/watch/watcher_darwin.go @@ -40,7 +40,8 @@ type fseventNotify struct { stop chan struct{} pathsWereWatching map[string]any - ignore PathMatcher + // ignore maps each pathsWereWatching root to the merged PathMatcher for paths under it. + ignore map[string]PathMatcher closeOnce sync.Once } @@ -74,26 +75,41 @@ func (d *fseventNotify) loop() { } } -// Add a path to be watched. Should only be called during initialization. -func (d *fseventNotify) initAdd(name string) { +// addStreamPath registers an existing ancestor path with the FSEvents stream. +func (d *fseventNotify) addStreamPath(name string) { d.stream.Paths = append(d.stream.Paths, name) - - if d.pathsWereWatching == nil { - d.pathsWereWatching = make(map[string]any) - } - d.pathsWereWatching[name] = struct{}{} } func (d *fseventNotify) Start() error { - if len(d.stream.Paths) == 0 { + notifyRoots := make([]string, 0, len(d.pathsWereWatching)) + for path := range d.pathsWereWatching { + notifyRoots = append(notifyRoots, path) + } + if len(notifyRoots) == 0 { return nil } + pathsToWatch, err := greatestExistingAncestors(notifyRoots, d.ignore) + if err != nil { + return err + } + pathsToWatch = pathutil.EncompassingPaths(pathsToWatch) + + _, normalizedIgnores, err := normalizeWatchRoots(notifyRoots, d.ignore) + if err != nil { + return err + } + d.ignore = normalizedIgnores + d.stream.Paths = nil + for _, path := range pathsToWatch { + d.addStreamPath(path) + } + d.closeOnce = sync.Once{} numberOfWatches.Add(int64(len(d.stream.Paths))) - err := d.stream.Start() + err = d.stream.Start() if err != nil { return err } @@ -122,9 +138,6 @@ func (d *fseventNotify) Errors() chan error { } func (d *fseventNotify) shouldNotify(path string) bool { - if d.shouldIgnore(path) { - return false - } if _, ok := d.pathsWereWatching[path]; ok { stat, err := os.Lstat(path) @@ -134,25 +147,32 @@ func (d *fseventNotify) shouldNotify(path string) bool { for root := range d.pathsWereWatching { if pathutil.IsChild(root, path) { + if d.shouldIgnore(root, path) { + return false + } return true } } return false } -func (d *fseventNotify) shouldIgnore(path string) bool { - if d.ignore == nil { +func (d *fseventNotify) shouldIgnore(dir, path string) bool { + if len(d.ignore) == 0 { return false } - matches, err := d.ignore.Matches(path) - if err != nil { - logrus.Debugf("error checking ignored path %q: %v", path, err) - return false + + if ignore, exists := d.ignore[dir]; exists && ignore != nil { + matches, err := ignore.Matches(path) + if err != nil { + logrus.Debugf("error checking ignored path %q: %v", path, err) + return false + } + return matches } - return matches + return false } -func newWatcher(paths []string, ignore PathMatcher) (Notify, error) { +func newWatcher(paths []string, ignore map[string]PathMatcher) (Notify, error) { dw := &fseventNotify{ stream: &fsevents.EventStream{ Latency: 50 * time.Millisecond, @@ -164,16 +184,17 @@ func newWatcher(paths []string, ignore PathMatcher) (Notify, error) { events: make(chan FileEvent), errors: make(chan error), stop: make(chan struct{}), - ignore: ignore, } - paths = pathutil.EncompassingPaths(paths) - for _, path := range paths { - path, err := filepath.Abs(path) - if err != nil { - return nil, fmt.Errorf("newWatcher: %w", err) - } - dw.initAdd(path) + watchRoots := pathutil.EncompassingPaths(paths) + notifyList, normalizedIgnores, err := normalizeWatchRoots(watchRoots, ignore) + if err != nil { + return nil, fmt.Errorf("newWatcher: %w", err) + } + dw.ignore = normalizedIgnores + dw.pathsWereWatching = make(map[string]any, len(notifyList)) + for path := range notifyList { + dw.pathsWereWatching[path] = struct{}{} } return dw, nil diff --git a/pkg/watch/watcher_darwin_test.go b/pkg/watch/watcher_darwin_test.go index e53d57a05f7..281e2b3c184 100644 --- a/pkg/watch/watcher_darwin_test.go +++ b/pkg/watch/watcher_darwin_test.go @@ -26,7 +26,7 @@ import ( "gotest.tools/v3/assert" ) -func newFseventNotifyFixture(repo string, ignore PathMatcher) *fseventNotify { +func newFseventNotifyFixture(repo string, ignore map[string]PathMatcher) *fseventNotify { return &fseventNotify{ pathsWereWatching: map[string]any{repo: struct{}{}}, ignore: ignore, @@ -90,7 +90,7 @@ func TestFseventNotifyShouldNotifyRespectsDockerignore(t *testing.T) { ignore, err := DockerIgnoreTesterFromContents(repo, "vendor/\n") assert.NilError(t, err) - d := newFseventNotifyFixture(repo, ignore) + d := newFseventNotifyFixture(repo, map[string]PathMatcher{repo: ignore}) kept := filepath.Join(repo, "src", "main.go") assert.NilError(t, os.MkdirAll(filepath.Dir(kept), 0o755)) assert.NilError(t, os.WriteFile(kept, []byte("x"), 0o644)) @@ -107,7 +107,7 @@ func TestFseventNotifyShouldNotifyDockerignoreNegation(t *testing.T) { ignore, err := DockerIgnoreTesterFromContents(repo, "bazel-bin/\n!bazel-bin/app-binary\n") assert.NilError(t, err) - d := newFseventNotifyFixture(repo, ignore) + d := newFseventNotifyFixture(repo, map[string]PathMatcher{repo: ignore}) ignoredChild := filepath.Join(repo, "bazel-bin", "cache", "out") assert.NilError(t, os.MkdirAll(filepath.Dir(ignoredChild), 0o755)) @@ -127,7 +127,7 @@ func TestFseventNotifyShouldNotifyIntersectMatcher(t *testing.T) { ignoreTmp, err := DockerIgnoreTesterFromContents(repo, "tmp/\n") assert.NilError(t, err) - d := newFseventNotifyFixture(repo, NewIntersectMatcher(ignoreVendor, ignoreTmp)) + d := newFseventNotifyFixture(repo, map[string]PathMatcher{repo: NewIntersectMatcher(ignoreVendor, ignoreTmp)}) vendorFile := filepath.Join(repo, "vendor", "x", "go.mod") assert.NilError(t, os.MkdirAll(filepath.Dir(vendorFile), 0o755)) assert.NilError(t, os.WriteFile(vendorFile, []byte("module x\n"), 0o644)) @@ -137,7 +137,7 @@ func TestFseventNotifyShouldNotifyIntersectMatcher(t *testing.T) { assert.NilError(t, err) ignoreBuild2, err := DockerIgnoreTesterFromContents(repo, "build/\n") assert.NilError(t, err) - d2 := newFseventNotifyFixture(repo, NewIntersectMatcher(ignoreBuild1, ignoreBuild2)) + d2 := newFseventNotifyFixture(repo, map[string]PathMatcher{repo: NewIntersectMatcher(ignoreBuild1, ignoreBuild2)}) buildFile := filepath.Join(repo, "build", "out", "a") assert.NilError(t, os.MkdirAll(filepath.Dir(buildFile), 0o755)) assert.NilError(t, os.WriteFile(buildFile, []byte("x"), 0o644)) @@ -149,8 +149,20 @@ func TestFseventNotifyShouldIgnoreDockerignoreDirectory(t *testing.T) { ignore, err := DockerIgnoreTesterFromContents(repo, "bazel-bin/\n!bazel-bin/app-binary\n") assert.NilError(t, err) - d := newFseventNotifyFixture(repo, ignore) + d := newFseventNotifyFixture(repo, map[string]PathMatcher{repo: ignore}) bazelBin := filepath.Join(repo, "bazel-bin") assert.NilError(t, os.MkdirAll(bazelBin, 0o755)) - assert.Assert(t, d.shouldIgnore(bazelBin), "expected directory path to match dockerignore") + assert.Assert(t, d.shouldIgnore(repo, bazelBin), "expected directory path to match dockerignore") +} + +func TestFseventNotifyShouldIgnoreLooksUpMatcherByWatchRoot(t *testing.T) { + repo := t.TempDir() + other := t.TempDir() + ignore, err := DockerIgnoreTesterFromContents(repo, "vendor/\n") + assert.NilError(t, err) + + d := newFseventNotifyFixture(repo, map[string]PathMatcher{repo: ignore}) + vendorFile := filepath.Join(repo, "vendor", "x.go") + assert.Assert(t, d.shouldIgnore(repo, vendorFile), "expected matcher keyed to watched root to apply") + assert.Assert(t, !d.shouldIgnore(other, vendorFile), "expected unrelated watch root not to apply matcher") } diff --git a/pkg/watch/watcher_naive.go b/pkg/watch/watcher_naive.go index f3d5cbbd244..cb121bbadad 100644 --- a/pkg/watch/watcher_naive.go +++ b/pkg/watch/watcher_naive.go @@ -45,7 +45,8 @@ type naiveNotify struct { // the notify list. It might be better to store this in a tree // structure, so we can filter the list quickly. notifyList map[string]bool - ignore PathMatcher + // ignore maps each notifyList root to the merged PathMatcher for paths under it. + ignore map[string]PathMatcher isWatcherRecursive bool watcher *fsnotify.Watcher @@ -60,12 +61,13 @@ func (d *naiveNotify) Start() error { return nil } - pathsToWatch := []string{} + notifyRoots := make([]string, 0, len(d.notifyList)) + for path := range d.notifyList { - pathsToWatch = append(pathsToWatch, path) + notifyRoots = append(notifyRoots, path) } - pathsToWatch, err := greatestExistingAncestors(pathsToWatch) + pathsToWatch, err := greatestExistingAncestors(notifyRoots, d.ignore) if err != nil { return err } @@ -73,6 +75,11 @@ func (d *naiveNotify) Start() error { pathsToWatch = pathutil.EncompassingPaths(pathsToWatch) } + _, d.ignore, err = normalizeWatchRoots(notifyRoots, d.ignore) + if err != nil { + return err + } + for _, name := range pathsToWatch { fi, err := os.Stat(name) if err != nil && !os.IsNotExist(err) { @@ -220,10 +227,6 @@ func (d *naiveNotify) loop() { //nolint:gocyclo } func (d *naiveNotify) shouldNotify(path string) bool { - if d.shouldIgnore(path) { - return false - } - if _, ok := d.notifyList[path]; ok { // We generally don't care when directories change at the root of an ADD stat, err := os.Lstat(path) @@ -233,7 +236,7 @@ func (d *naiveNotify) shouldNotify(path string) bool { for root := range d.notifyList { if pathutil.IsChild(root, path) { - return true + return !d.shouldIgnore(root, path) } } return false @@ -245,53 +248,64 @@ func (d *naiveNotify) shouldSkipDir(path string) bool { return false } - // Only walk directories that are under a notifyList path, or that are under an ancestor of one + // Only walk directories under a notifyList path or under an ancestor of one // (Start() may watch a parent when the target is missing or is a file). - // Check ancestor/descendant vs notifyList before ignores - // so ignore patterns cannot block reaching the watched root. + // Decide ancestor/descendant versus notifyList before applying ignores so one + // root's patterns cannot block reaching another root. + // When walking beneath a watched ancestor, prune subtrees only with that root's + // matcher from d.ignore. 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 } } - // skip the dir if ignore rules match this full subtree. - if d.shouldIgnoreEntireDir(path) { + if isChildOfWatchedDir && d.shouldIgnoreEntireDir(dir, path) { return true } return !isChildOfWatchedDir } -func (d *naiveNotify) shouldIgnoreEntireDir(path string) bool { - if d.ignore == nil { +func (d *naiveNotify) shouldIgnoreEntireDir(dir, path string) bool { + if len(d.ignore) == 0 { return false } - matches, err := d.ignore.MatchesEntireDir(path) - if err != nil { - logrus.Debugf("error checking ignored directory %q: %v", path, err) - return false - } - if matches { - return true + + if ignore, exists := d.ignore[dir]; exists && ignore != nil { + matches, err := ignore.MatchesEntireDir(path) + if err != nil { + logrus.Debugf("error checking ignored directory %q: %v", path, err) + return false + } + if matches { + return true + } + return matches } - return matches + return false } -func (d *naiveNotify) shouldIgnore(path string) bool { - if d.ignore == nil { +func (d *naiveNotify) shouldIgnore(dir, path string) bool { + if len(d.ignore) == 0 { return false } - matches, err := d.ignore.Matches(path) - if err != nil { - logrus.Debugf("error checking ignored path %q: %v", path, err) - return false + + if ignore, exists := d.ignore[dir]; exists && ignore != nil { + matches, err := ignore.Matches(path) + if err != nil { + logrus.Debugf("error checking ignored path %q: %v", path, err) + return false + } + return matches } - return matches + return false } func (d *naiveNotify) add(path string) error { @@ -304,7 +318,7 @@ func (d *naiveNotify) add(path string) error { return nil } -func newWatcher(paths []string, ignore PathMatcher) (Notify, error) { +func newWatcher(paths []string, ignore map[string]PathMatcher) (Notify, error) { fsw, err := fsnotify.NewWatcher() if err != nil { if strings.Contains(err.Error(), "too many open files") && runtime.GOOS == "linux" { @@ -320,20 +334,20 @@ func newWatcher(paths []string, ignore PathMatcher) (Notify, error) { isWatcherRecursive := err == nil wrappedEvents := make(chan FileEvent) - notifyList := make(map[string]bool, len(paths)) + + watchRoots := paths if isWatcherRecursive { - paths = pathutil.EncompassingPaths(paths) + watchRoots = pathutil.EncompassingPaths(paths) } - for _, path := range paths { - path, err := filepath.Abs(path) - if err != nil { - return nil, fmt.Errorf("newWatcher: %w", err) - } - notifyList[path] = true + + notifyList, normalizedIgnores, err := normalizeWatchRoots(watchRoots, ignore) + if err != nil { + return nil, fmt.Errorf("newWatcher: %w", err) } + wmw := &naiveNotify{ notifyList: notifyList, - ignore: ignore, + ignore: normalizedIgnores, watcher: fsw, events: fsw.Events, wrappedEvents: wrappedEvents, @@ -345,15 +359,3 @@ func newWatcher(paths []string, ignore PathMatcher) (Notify, error) { } var _ Notify = &naiveNotify{} - -func greatestExistingAncestors(paths []string) ([]string, error) { - result := []string{} - for _, p := range paths { - newP, err := greatestExistingAncestor(p) - if err != nil { - return nil, fmt.Errorf("finding ancestor of %s: %w", p, err) - } - result = append(result, newP) - } - return result, nil -} diff --git a/pkg/watch/watcher_naive_test.go b/pkg/watch/watcher_naive_test.go index ed07a4cff64..ee73ea30332 100644 --- a/pkg/watch/watcher_naive_test.go +++ b/pkg/watch/watcher_naive_test.go @@ -164,7 +164,7 @@ func TestShouldSkipDirWithNegatedChildException(t *testing.T) { assert.NilError(t, err) d := &naiveNotify{ - ignore: ignore, + ignore: map[string]PathMatcher{repoRoot: ignore}, notifyList: map[string]bool{repoRoot: true}, } @@ -177,10 +177,37 @@ func TestShouldIgnorePathStillMatchesDirectoryPattern(t *testing.T) { ignore, err := DockerIgnoreTesterFromContents(repoRoot, "bazel-bin/\n!bazel-bin/app-binary\n") assert.NilError(t, err) - d := &naiveNotify{ignore: ignore} + d := &naiveNotify{ignore: map[string]PathMatcher{repoRoot: ignore}} bazelBin := filepath.Join(repoRoot, "bazel-bin") - assert.Assert(t, d.shouldIgnore(bazelBin), "expected bazel-bin path to match ignore pattern") + assert.Assert(t, d.shouldIgnore(repoRoot, bazelBin), "expected bazel-bin path to match ignore pattern") +} + +func TestShouldIgnoreLooksUpMatcherByWatchRoot(t *testing.T) { + repoRoot := t.TempDir() + otherRoot := t.TempDir() + ignore, err := DockerIgnoreTesterFromContents(repoRoot, "vendor/\n") + assert.NilError(t, err) + + d := &naiveNotify{ignore: map[string]PathMatcher{repoRoot: ignore}} + + vendorFile := filepath.Join(repoRoot, "vendor", "x.go") + assert.Assert(t, d.shouldIgnore(repoRoot, vendorFile), "expected matcher keyed to watched root to apply") + assert.Assert(t, !d.shouldIgnore(otherRoot, vendorFile), "expected unrelated watch root not to apply matcher") +} + +func TestShouldIgnoreEntireDirLooksUpMatcherByWatchRoot(t *testing.T) { + repoRoot := t.TempDir() + ignore, err := DockerIgnoreTesterFromContents(repoRoot, "vendor/\n") + assert.NilError(t, err) + + d := &naiveNotify{ + ignore: map[string]PathMatcher{repoRoot: ignore}, + notifyList: map[string]bool{repoRoot: true}, + } + + vendorDir := filepath.Join(repoRoot, "vendor") + assert.Assert(t, d.shouldIgnoreEntireDir(repoRoot, vendorDir), "expected directory matcher keyed to watched root to apply") } func TestShouldSkipDirForIgnoredSubtreeWithoutException(t *testing.T) { @@ -189,7 +216,7 @@ func TestShouldSkipDirForIgnoredSubtreeWithoutException(t *testing.T) { assert.NilError(t, err) d := &naiveNotify{ - ignore: ignore, + ignore: map[string]PathMatcher{repoRoot: ignore}, notifyList: map[string]bool{repoRoot: true}, } @@ -204,7 +231,7 @@ func TestShouldSkipDirDoesNotSkipAncestorOfWatchedPath(t *testing.T) { watchedPath := filepath.Join(repoRoot, "parent", "child", "non-existent") d := &naiveNotify{ - ignore: ignore, + ignore: map[string]PathMatcher{watchedPath: ignore}, notifyList: map[string]bool{watchedPath: true}, } @@ -220,7 +247,7 @@ func TestShouldSkipDirIntersectRequiresAllTriggersToAgree(t *testing.T) { assert.NilError(t, err) d := &naiveNotify{ - ignore: NewIntersectMatcher(ignoreVendor, ignoreTmp), + ignore: map[string]PathMatcher{repoRoot: NewIntersectMatcher(ignoreVendor, ignoreTmp)}, notifyList: map[string]bool{repoRoot: true}, } @@ -232,7 +259,7 @@ func TestShouldSkipDirIntersectRequiresAllTriggersToAgree(t *testing.T) { ignoreBuild2, err := DockerIgnoreTesterFromContents(repoRoot, "build/\n") assert.NilError(t, err) d2 := &naiveNotify{ - ignore: NewIntersectMatcher(ignoreBuild1, ignoreBuild2), + ignore: map[string]PathMatcher{repoRoot: NewIntersectMatcher(ignoreBuild1, ignoreBuild2)}, notifyList: map[string]bool{repoRoot: true}, } buildDir := filepath.Join(repoRoot, "build")