From dd6f1646b65719c6662d481ef60b9a7ccb5bedb2 Mon Sep 17 00:00:00 2001 From: Endika Iglesias Date: Fri, 31 Jul 2026 19:01:11 +0200 Subject: [PATCH 1/2] fix(watch): skip unreadable directories instead of failing the watch Signed-off-by: Endika Iglesias --- pkg/watch/watcher_naive.go | 14 +++++++++++++ pkg/watch/watcher_naive_test.go | 36 +++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+) diff --git a/pkg/watch/watcher_naive.go b/pkg/watch/watcher_naive.go index 7798025e8bc..d9f4ec111e6 100644 --- a/pkg/watch/watcher_naive.go +++ b/pkg/watch/watcher_naive.go @@ -113,6 +113,12 @@ func (d *naiveNotify) watchRecursively(dir string) error { return filepath.WalkDir(dir, func(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 } @@ -130,6 +136,10 @@ func (d *naiveNotify) watchRecursively(dir string) error { if os.IsNotExist(err) { return nil } + if os.IsPermission(err) { + logrus.Debugf("Not watching %s: %v", path, err) + return filepath.SkipDir + } return fmt.Errorf("watcher.Add(%q): %w", path, err) } return nil @@ -180,6 +190,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) { + logrus.Debugf("Not watching %s: %v", path, err) + return filepath.SkipDir + } return err } diff --git a/pkg/watch/watcher_naive_test.go b/pkg/watch/watcher_naive_test.go index b188de93903..f6b456c4341 100644 --- a/pkg/watch/watcher_naive_test.go +++ b/pkg/watch/watcher_naive_test.go @@ -157,3 +157,39 @@ 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. +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) + } +} From 31835d31fe7c1af4b7aa6d0236ef804ff41b22f3 Mon Sep 17 00:00:00 2001 From: Endika Iglesias Date: Sat, 1 Aug 2026 15:05:18 +0200 Subject: [PATCH 2/2] test(watch): cover the permission branches as root CI runs the tests as root, where the unreadable-directory test is always skipped. Extract the WalkDir callbacks and inject the watch registration so a synthetic permission error can drive those branches instead. Signed-off-by: Endika Iglesias --- pkg/watch/watcher_naive.go | 138 +++++++++++++++------------ pkg/watch/watcher_naive_test.go | 3 + pkg/watch/watcher_naive_walk_test.go | 127 ++++++++++++++++++++++++ 3 files changed, 206 insertions(+), 62 deletions(-) create mode 100644 pkg/watch/watcher_naive_walk_test.go diff --git a/pkg/watch/watcher_naive.go b/pkg/watch/watcher_naive.go index d9f4ec111e6..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,46 +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 { - // 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 + 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 - } - if os.IsPermission(err) { - logrus.Debugf("Not watching %s: %v", path, err) - return filepath.SkipDir - } - 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 { @@ -160,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 @@ -188,47 +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 { - if os.IsPermission(err) { - logrus.Debugf("Not watching %s: %v", path, err) - return filepath.SkipDir - } - 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 } } @@ -320,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 f6b456c4341..79efb78eb8a 100644 --- a/pkg/watch/watcher_naive_test.go +++ b/pkg/watch/watcher_naive_test.go @@ -160,6 +160,9 @@ func TestDontRecurseWhenWatchingParentsOfNonExistentFiles(t *testing.T) { // 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") 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)) +}