diff --git a/pkg/watch/watcher_naive.go b/pkg/watch/watcher_naive.go index 7798025e8bc..6247c2a13cd 100644 --- a/pkg/watch/watcher_naive.go +++ b/pkg/watch/watcher_naive.go @@ -52,6 +52,10 @@ type naiveNotify struct { wrappedEvents chan FileEvent errors chan error numWatches int64 + + // addWatch registers a path with the watcher. A field so tests can inject + // a permission error, which a process running as root cannot produce. + addWatch func(path string) error } func (d *naiveNotify) Start() error { @@ -90,7 +94,7 @@ func (d *naiveNotify) Start() error { return fmt.Errorf("notify.Add(%q): %w", name, err) } } else { - err = d.add(filepath.Dir(name)) + err = d.addWatch(filepath.Dir(name)) if err != nil { return fmt.Errorf("notify.Add(%q): %w", filepath.Dir(name), err) } @@ -104,36 +108,49 @@ func (d *naiveNotify) Start() error { func (d *naiveNotify) watchRecursively(dir string) error { if d.isWatcherRecursive { - err := d.add(dir) + err := d.addWatch(dir) if err == nil || os.IsNotExist(err) { return nil } return fmt.Errorf("watcher.Add(%q): %w", dir, err) } - return filepath.WalkDir(dir, func(path string, info fs.DirEntry, err error) error { - if err != nil { - return err + return filepath.WalkDir(dir, d.walkAndAdd) +} + +// walkAndAdd puts a watch on every directory of the tree being walked. +func (d *naiveNotify) walkAndAdd(path string, info fs.DirEntry, err error) error { + if err != nil { + // A directory we are not allowed to read is not a reason to abandon the + // whole watch: we simply cannot see inside it, so skip it and carry on. + if os.IsPermission(err) { + logrus.Debugf("Not watching %s: %v", path, err) + return filepath.SkipDir } + return err + } + + if !info.IsDir() { + return nil + } + + if d.shouldSkipDir(path) { + logrus.Debugf("Ignoring directory and its contents (recursively): %s", path) + return filepath.SkipDir + } - if !info.IsDir() { + err = d.addWatch(path) + if err != nil { + if os.IsNotExist(err) { return nil } - - if d.shouldSkipDir(path) { - logrus.Debugf("Ignoring directory and its contents (recursively): %s", path) + if os.IsPermission(err) { + logrus.Debugf("Not watching %s: %v", path, err) return filepath.SkipDir } - - err = d.add(path) - if err != nil { - if os.IsNotExist(err) { - return nil - } - return fmt.Errorf("watcher.Add(%q): %w", path, err) - } - return nil - }) + return fmt.Errorf("watcher.Add(%q): %w", path, err) + } + return nil } func (d *naiveNotify) Close() error { @@ -150,7 +167,7 @@ func (d *naiveNotify) Errors() chan error { return d.errors } -func (d *naiveNotify) loop() { //nolint:gocyclo +func (d *naiveNotify) loop() { defer close(d.wrappedEvents) for e := range d.events { // The Windows fsnotify event stream sometimes gets events with empty names @@ -178,43 +195,53 @@ func (d *naiveNotify) loop() { //nolint:gocyclo // because it's a bit more elegant that way. // // 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 { - return err - } + err := filepath.WalkDir(e.Name, d.walkAndNotify(e.Name)) + if err != nil && !os.IsNotExist(err) { + logrus.Infof("Error walking directory %s: %s", e.Name, err) + } + } +} - if d.shouldNotify(path) { - d.wrappedEvents <- FileEvent(path) +// walkAndNotify fires an event for every path under name, watching the +// directories it goes through. +func (d *naiveNotify) walkAndNotify(name string) fs.WalkDirFunc { + return func(path string, info fs.DirEntry, err error) error { + if err != nil { + if os.IsPermission(err) { + logrus.Debugf("Not watching %s: %v", path, err) + return filepath.SkipDir } + return err + } - // TODO(dmiller): symlinks 😭 + if d.shouldNotify(path) { + d.wrappedEvents <- FileEvent(path) + } - shouldWatch := false - if info.IsDir() { - // watch directories unless we can skip them entirely - if d.shouldSkipDir(path) { - return filepath.SkipDir - } + // TODO(dmiller): symlinks 😭 + shouldWatch := false + if info.IsDir() { + // watch directories unless we can skip them entirely + if d.shouldSkipDir(path) { + return filepath.SkipDir + } + + shouldWatch = true + } else { + // watch files that are explicitly named, but don't watch others + _, ok := d.notifyList[path] + if ok { shouldWatch = true - } else { - // watch files that are explicitly named, but don't watch others - _, ok := d.notifyList[path] - if ok { - shouldWatch = true - } } - if shouldWatch { - err := d.add(path) - if err != nil && !os.IsNotExist(err) { - logrus.Infof("Error watching path %s: %s", e.Name, err) - } + } + if shouldWatch { + err := d.addWatch(path) + if err != nil && !os.IsNotExist(err) { + logrus.Infof("Error watching path %s: %s", name, err) } - return nil - }) - if err != nil && !os.IsNotExist(err) { - logrus.Infof("Error walking directory %s: %s", e.Name, err) } + return nil } } @@ -306,6 +333,7 @@ func newWatcher(paths []string) (Notify, error) { errors: fsw.Errors, isWatcherRecursive: isWatcherRecursive, } + wmw.addWatch = wmw.add return wmw, nil } diff --git a/pkg/watch/watcher_naive_test.go b/pkg/watch/watcher_naive_test.go index b188de93903..79efb78eb8a 100644 --- a/pkg/watch/watcher_naive_test.go +++ b/pkg/watch/watcher_naive_test.go @@ -157,3 +157,42 @@ func TestDontRecurseWhenWatchingParentsOfNonExistentFiles(t *testing.T) { t.Fatalf("watching more than 5 files: %d", n) } } + +// A directory the current user cannot read costs us visibility into that +// subtree, but it must not prevent the rest of the tree from being watched. +// +// Uses a real unreadable directory, so it is skipped as root and covers +// nothing in CI. See watcher_naive_walk_test.go for the unit tests. +func TestWatchRecursivelySkipsUnreadableDir(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("permission semantics differ on windows") + } + if os.Geteuid() == 0 { + t.Skip("root bypasses the permission bit this test relies on") + } + + root := t.TempDir() + unreadable := filepath.Join(root, "unreadable") + if err := os.MkdirAll(filepath.Join(unreadable, "inner"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(root, "watched.txt"), []byte("x"), 0o600); err != nil { + t.Fatal(err) + } + if err := os.Chmod(unreadable, 0o000); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.Chmod(unreadable, 0o755) }) + + if _, err := os.ReadDir(unreadable); err == nil { + t.Skip("could not make the directory unreadable in this environment") + } + + notify, err := NewWatcher([]string{root}) + assert.NilError(t, err) + t.Cleanup(func() { _ = notify.Close() }) + + if err := notify.Start(); err != nil { + t.Fatalf("Start() must not fail because of an unreadable directory: %v", err) + } +} diff --git a/pkg/watch/watcher_naive_walk_test.go b/pkg/watch/watcher_naive_walk_test.go new file mode 100644 index 00000000000..262269198d0 --- /dev/null +++ b/pkg/watch/watcher_naive_walk_test.go @@ -0,0 +1,127 @@ +//go:build !fsnotify + +/* + Copyright 2026 Docker Compose CLI authors + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +package watch + +import ( + "errors" + "io/fs" + "os" + "path/filepath" + "testing" + + "gotest.tools/v3/assert" +) + +// The walk callbacks have to tolerate parts of the tree the process cannot +// read. A real unreadable directory needs a permission bit root ignores, and +// CI runs as root, so these inject the error instead. + +func permissionError(path string) error { + return &fs.PathError{Op: "open", Path: path, Err: fs.ErrPermission} +} + +func dirEntry(t *testing.T, path string) fs.DirEntry { + t.Helper() + info, err := os.Lstat(path) + assert.NilError(t, err) + return fs.FileInfoToDirEntry(info) +} + +func TestWalkAndAddSkipsUnreadableDir(t *testing.T) { + d := &naiveNotify{} + + err := d.walkAndAdd("/nope", nil, permissionError("/nope")) + + assert.Equal(t, err, filepath.SkipDir) +} + +// Anything else is a real failure and has to reach the caller. +func TestWalkAndAddPropagatesOtherWalkErrors(t *testing.T) { + d := &naiveNotify{} + boom := errors.New("boom") + + err := d.walkAndAdd("/nope", nil, boom) + + assert.Assert(t, errors.Is(err, boom)) +} + +// A directory can be listed and still refuse the watch, which is what inotify +// reports for one the process cannot read. +func TestWalkAndAddSkipsDirItCannotWatch(t *testing.T) { + root := t.TempDir() + var attempted []string + d := &naiveNotify{ + notifyList: map[string]bool{root: true}, + addWatch: func(path string) error { + attempted = append(attempted, path) + return permissionError(path) + }, + } + + err := d.walkAndAdd(root, dirEntry(t, root), nil) + + assert.Equal(t, err, filepath.SkipDir) + assert.DeepEqual(t, attempted, []string{root}) +} + +// A directory that disappeared mid-walk has nothing left below it to skip. +func TestWalkAndAddIgnoresDirThatDisappeared(t *testing.T) { + root := t.TempDir() + d := &naiveNotify{ + notifyList: map[string]bool{root: true}, + addWatch: func(string) error { + return &fs.PathError{Op: "open", Path: root, Err: fs.ErrNotExist} + }, + } + + err := d.walkAndAdd(root, dirEntry(t, root), nil) + + assert.NilError(t, err) +} + +func TestWalkAndAddPropagatesOtherWatchErrors(t *testing.T) { + root := t.TempDir() + d := &naiveNotify{ + notifyList: map[string]bool{root: true}, + addWatch: func(string) error { + return errors.New("boom") + }, + } + + err := d.walkAndAdd(root, dirEntry(t, root), nil) + + assert.ErrorContains(t, err, "boom") +} + +func TestWalkAndNotifySkipsUnreadableDir(t *testing.T) { + d := &naiveNotify{} + + err := d.walkAndNotify("/nope")("/nope/inner", nil, permissionError("/nope/inner")) + + assert.Equal(t, err, filepath.SkipDir) +} + +func TestWalkAndNotifyPropagatesOtherWalkErrors(t *testing.T) { + d := &naiveNotify{} + boom := errors.New("boom") + + err := d.walkAndNotify("/nope")("/nope/inner", nil, boom) + + assert.Assert(t, errors.Is(err, boom)) +}