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 <[email protected]>
2026-10-12 02:13:08 +00:00 · 2026-08-11 14:10:40 +02:00
parent fb543842d8
commit 832673c8e4
3 changed files with 213 additions and 69 deletions
@@ -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)
+}

-		if !info.IsDir() {
-			return nil
-		}
-
-		if d.shouldSkipDir(path) {
-			logrus.Debugf("Ignoring directory and its contents (recursively): %s", path)
+// 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
+	}

-		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)
-		}
+	if !info.IsDir() {
 		return nil
-	})
+	}
+
+	if d.shouldSkipDir(path) {
+		logrus.Debugf("Ignoring directory and its contents (recursively): %s", path)
+		return filepath.SkipDir
+	}
+
+	err = d.addWatch(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
 }

 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,50 +195,56 @@ 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
-			}
-
-			if d.shouldNotify(path) {
-				d.wrappedEvents <- FileEvent(path)
-			}
-
-			// 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
-				}
-			}
-			if shouldWatch {
-				err := d.add(path)
-				if err != nil && !os.IsNotExist(err) {
-					logrus.Infof("Error watching path %s: %s", e.Name, err)
-				}
-			}
-			return nil
-		})
+		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)
 		}
 	}
 }

+// 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
+		}
+
+		if d.shouldNotify(path) {
+			d.wrappedEvents <- FileEvent(path)
+		}
+
+		// 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
+			}
+		}
+		if shouldWatch {
+			err := d.addWatch(path)
+			if err != nil && !os.IsNotExist(err) {
+				logrus.Infof("Error watching path %s: %s", name, err)
+			}
+		}
+		return nil
+	}
+}
+
 func (d *naiveNotify) shouldNotify(path string) bool {
 	if _, ok := d.notifyList[path]; ok {
 		// We generally don't care when directories change at the root of an ADD
@@ -320,6 +333,7 @@ func newWatcher(paths []string) (Notify, error) {
 		errors:             fsw.Errors,
 		isWatcherRecursive: isWatcherRecursive,
 	}
+	wmw.addWatch = wmw.add

 	return wmw, nil
 }
@@ -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")
@@ -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))
+}