Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions pkg/compose/watch.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
}

Expand All @@ -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...))
Comment thread
ManManavadaria marked this conversation as resolved.
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 err != nil {
return nil, err
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/watch/notify.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
2 changes: 1 addition & 1 deletion pkg/watch/notify_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/watch/watcher_darwin.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 shouldSkipDir at the watcher layer; the naive (Linux/Windows) watcher stores and uses d.ignore for both shouldIgnore and shouldSkipDir, 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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is inconsistent with the naive implementation which use _ PathMatcher

dw := &fseventNotify{
stream: &fsevents.EventStream{
Latency: 50 * time.Millisecond,
Expand Down
57 changes: 53 additions & 4 deletions pkg/watch/watcher_naive.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 filepath.SkipDir
}
Comment thread
ManManavadaria marked this conversation as resolved.
return err
}

Expand Down Expand Up @@ -180,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
}
Comment on lines +188 to +191
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same this is already catch by block lines 204-206

return err
}

Expand Down Expand Up @@ -252,12 +261,52 @@ 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
}
}

// skip the dir if ignore rules match this full subtree.
if d.shouldIgnoreEntireDir(path) {
return true
}

return !isChildOfWatchedDir
}

func (d *naiveNotify) shouldIgnoreEntireDir(path string) bool {
if d.ignore == nil {
return false
}
return true
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
}
return false
Comment on lines +294 to +297
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if matches {
return true
}
return false
return matches

}

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
Comment on lines +300 to +309
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function isn't needed, it's only called from the 2 previous unnecessary check blocks above

}

func (d *naiveNotify) add(path string) error {
Expand All @@ -270,7 +319,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" {
Expand All @@ -297,9 +346,9 @@ func newWatcher(paths []string) (Notify, error) {
}
notifyList[path] = true
}

wmw := &naiveNotify{
notifyList: notifyList,
ignore: ignore,
watcher: fsw,
events: fsw.Events,
wrappedEvents: wrappedEvents,
Expand Down
54 changes: 54 additions & 0 deletions pkg/watch/watcher_naive_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}