Skip to content

Commit d5e70df

Browse files
committed
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 <endika2@gmail.com>
1 parent 8e4aa07 commit d5e70df

3 files changed

Lines changed: 206 additions & 62 deletions

File tree

pkg/watch/watcher_naive.go

Lines changed: 76 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,10 @@ type naiveNotify struct {
5252
wrappedEvents chan FileEvent
5353
errors chan error
5454
numWatches int64
55+
56+
// addWatch registers a path with the watcher. A field so tests can inject
57+
// a permission error, which a process running as root cannot produce.
58+
addWatch func(path string) error
5559
}
5660

5761
func (d *naiveNotify) Start() error {
@@ -90,7 +94,7 @@ func (d *naiveNotify) Start() error {
9094
return fmt.Errorf("notify.Add(%q): %w", name, err)
9195
}
9296
} else {
93-
err = d.add(filepath.Dir(name))
97+
err = d.addWatch(filepath.Dir(name))
9498
if err != nil {
9599
return fmt.Errorf("notify.Add(%q): %w", filepath.Dir(name), err)
96100
}
@@ -104,46 +108,49 @@ func (d *naiveNotify) Start() error {
104108

105109
func (d *naiveNotify) watchRecursively(dir string) error {
106110
if d.isWatcherRecursive {
107-
err := d.add(dir)
111+
err := d.addWatch(dir)
108112
if err == nil || os.IsNotExist(err) {
109113
return nil
110114
}
111115
return fmt.Errorf("watcher.Add(%q): %w", dir, err)
112116
}
113117

114-
return filepath.WalkDir(dir, func(path string, info fs.DirEntry, err error) error {
115-
if err != nil {
116-
// A directory we are not allowed to read is not a reason to abandon the
117-
// whole watch: we simply cannot see inside it, so skip it and carry on.
118-
if os.IsPermission(err) {
119-
logrus.Debugf("Not watching %s: %v", path, err)
120-
return filepath.SkipDir
121-
}
122-
return err
118+
return filepath.WalkDir(dir, d.walkAndAdd)
119+
}
120+
121+
// walkAndAdd puts a watch on every directory of the tree being walked.
122+
func (d *naiveNotify) walkAndAdd(path string, info fs.DirEntry, err error) error {
123+
if err != nil {
124+
// A directory we are not allowed to read is not a reason to abandon the
125+
// whole watch: we simply cannot see inside it, so skip it and carry on.
126+
if os.IsPermission(err) {
127+
logrus.Debugf("Not watching %s: %v", path, err)
128+
return filepath.SkipDir
123129
}
130+
return err
131+
}
132+
133+
if !info.IsDir() {
134+
return nil
135+
}
136+
137+
if d.shouldSkipDir(path) {
138+
logrus.Debugf("Ignoring directory and its contents (recursively): %s", path)
139+
return filepath.SkipDir
140+
}
124141

125-
if !info.IsDir() {
142+
err = d.addWatch(path)
143+
if err != nil {
144+
if os.IsNotExist(err) {
126145
return nil
127146
}
128-
129-
if d.shouldSkipDir(path) {
130-
logrus.Debugf("Ignoring directory and its contents (recursively): %s", path)
147+
if os.IsPermission(err) {
148+
logrus.Debugf("Not watching %s: %v", path, err)
131149
return filepath.SkipDir
132150
}
133-
134-
err = d.add(path)
135-
if err != nil {
136-
if os.IsNotExist(err) {
137-
return nil
138-
}
139-
if os.IsPermission(err) {
140-
logrus.Debugf("Not watching %s: %v", path, err)
141-
return filepath.SkipDir
142-
}
143-
return fmt.Errorf("watcher.Add(%q): %w", path, err)
144-
}
145-
return nil
146-
})
151+
return fmt.Errorf("watcher.Add(%q): %w", path, err)
152+
}
153+
return nil
147154
}
148155

149156
func (d *naiveNotify) Close() error {
@@ -160,7 +167,7 @@ func (d *naiveNotify) Errors() chan error {
160167
return d.errors
161168
}
162169

163-
func (d *naiveNotify) loop() { //nolint:gocyclo
170+
func (d *naiveNotify) loop() {
164171
defer close(d.wrappedEvents)
165172
for e := range d.events {
166173
// The Windows fsnotify event stream sometimes gets events with empty names
@@ -188,47 +195,53 @@ func (d *naiveNotify) loop() { //nolint:gocyclo
188195
// because it's a bit more elegant that way.
189196
//
190197
// TODO(dbentley): if there's a delete should we call d.watcher.Remove to prevent leaking?
191-
err := filepath.WalkDir(e.Name, func(path string, info fs.DirEntry, err error) error {
192-
if err != nil {
193-
if os.IsPermission(err) {
194-
logrus.Debugf("Not watching %s: %v", path, err)
195-
return filepath.SkipDir
196-
}
197-
return err
198-
}
198+
err := filepath.WalkDir(e.Name, d.walkAndNotify(e.Name))
199+
if err != nil && !os.IsNotExist(err) {
200+
logrus.Infof("Error walking directory %s: %s", e.Name, err)
201+
}
202+
}
203+
}
199204

200-
if d.shouldNotify(path) {
201-
d.wrappedEvents <- FileEvent(path)
205+
// walkAndNotify fires an event for every path under name, watching the
206+
// directories it goes through.
207+
func (d *naiveNotify) walkAndNotify(name string) fs.WalkDirFunc {
208+
return func(path string, info fs.DirEntry, err error) error {
209+
if err != nil {
210+
if os.IsPermission(err) {
211+
logrus.Debugf("Not watching %s: %v", path, err)
212+
return filepath.SkipDir
202213
}
214+
return err
215+
}
203216

204-
// TODO(dmiller): symlinks 😭
217+
if d.shouldNotify(path) {
218+
d.wrappedEvents <- FileEvent(path)
219+
}
205220

206-
shouldWatch := false
207-
if info.IsDir() {
208-
// watch directories unless we can skip them entirely
209-
if d.shouldSkipDir(path) {
210-
return filepath.SkipDir
211-
}
221+
// TODO(dmiller): symlinks 😭
222+
223+
shouldWatch := false
224+
if info.IsDir() {
225+
// watch directories unless we can skip them entirely
226+
if d.shouldSkipDir(path) {
227+
return filepath.SkipDir
228+
}
212229

230+
shouldWatch = true
231+
} else {
232+
// watch files that are explicitly named, but don't watch others
233+
_, ok := d.notifyList[path]
234+
if ok {
213235
shouldWatch = true
214-
} else {
215-
// watch files that are explicitly named, but don't watch others
216-
_, ok := d.notifyList[path]
217-
if ok {
218-
shouldWatch = true
219-
}
220236
}
221-
if shouldWatch {
222-
err := d.add(path)
223-
if err != nil && !os.IsNotExist(err) {
224-
logrus.Infof("Error watching path %s: %s", e.Name, err)
225-
}
237+
}
238+
if shouldWatch {
239+
err := d.addWatch(path)
240+
if err != nil && !os.IsNotExist(err) {
241+
logrus.Infof("Error watching path %s: %s", name, err)
226242
}
227-
return nil
228-
})
229-
if err != nil && !os.IsNotExist(err) {
230-
logrus.Infof("Error walking directory %s: %s", e.Name, err)
231243
}
244+
return nil
232245
}
233246
}
234247

@@ -320,6 +333,7 @@ func newWatcher(paths []string) (Notify, error) {
320333
errors: fsw.Errors,
321334
isWatcherRecursive: isWatcherRecursive,
322335
}
336+
wmw.addWatch = wmw.add
323337

324338
return wmw, nil
325339
}

pkg/watch/watcher_naive_test.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -160,6 +160,9 @@ func TestDontRecurseWhenWatchingParentsOfNonExistentFiles(t *testing.T) {
160160

161161
// A directory the current user cannot read costs us visibility into that
162162
// subtree, but it must not prevent the rest of the tree from being watched.
163+
//
164+
// Uses a real unreadable directory, so it is skipped as root and covers
165+
// nothing in CI. See watcher_naive_walk_test.go for the unit tests.
163166
func TestWatchRecursivelySkipsUnreadableDir(t *testing.T) {
164167
if runtime.GOOS == "windows" {
165168
t.Skip("permission semantics differ on windows")
Lines changed: 127 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,127 @@
1+
//go:build !fsnotify
2+
3+
/*
4+
Copyright 2026 Docker Compose CLI authors
5+
6+
Licensed under the Apache License, Version 2.0 (the "License");
7+
you may not use this file except in compliance with the License.
8+
You may obtain a copy of the License at
9+
10+
http://www.apache.org/licenses/LICENSE-2.0
11+
12+
Unless required by applicable law or agreed to in writing, software
13+
distributed under the License is distributed on an "AS IS" BASIS,
14+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
15+
See the License for the specific language governing permissions and
16+
limitations under the License.
17+
*/
18+
19+
package watch
20+
21+
import (
22+
"errors"
23+
"io/fs"
24+
"os"
25+
"path/filepath"
26+
"testing"
27+
28+
"gotest.tools/v3/assert"
29+
)
30+
31+
// The walk callbacks have to tolerate parts of the tree the process cannot
32+
// read. A real unreadable directory needs a permission bit root ignores, and
33+
// CI runs as root, so these inject the error instead.
34+
35+
func permissionError(path string) error {
36+
return &fs.PathError{Op: "open", Path: path, Err: fs.ErrPermission}
37+
}
38+
39+
func dirEntry(t *testing.T, path string) fs.DirEntry {
40+
t.Helper()
41+
info, err := os.Lstat(path)
42+
assert.NilError(t, err)
43+
return fs.FileInfoToDirEntry(info)
44+
}
45+
46+
func TestWalkAndAddSkipsUnreadableDir(t *testing.T) {
47+
d := &naiveNotify{}
48+
49+
err := d.walkAndAdd("/nope", nil, permissionError("/nope"))
50+
51+
assert.Equal(t, err, filepath.SkipDir)
52+
}
53+
54+
// Anything else is a real failure and has to reach the caller.
55+
func TestWalkAndAddPropagatesOtherWalkErrors(t *testing.T) {
56+
d := &naiveNotify{}
57+
boom := errors.New("boom")
58+
59+
err := d.walkAndAdd("/nope", nil, boom)
60+
61+
assert.Assert(t, errors.Is(err, boom))
62+
}
63+
64+
// A directory can be listed and still refuse the watch, which is what inotify
65+
// reports for one the process cannot read.
66+
func TestWalkAndAddSkipsDirItCannotWatch(t *testing.T) {
67+
root := t.TempDir()
68+
var attempted []string
69+
d := &naiveNotify{
70+
notifyList: map[string]bool{root: true},
71+
addWatch: func(path string) error {
72+
attempted = append(attempted, path)
73+
return permissionError(path)
74+
},
75+
}
76+
77+
err := d.walkAndAdd(root, dirEntry(t, root), nil)
78+
79+
assert.Equal(t, err, filepath.SkipDir)
80+
assert.DeepEqual(t, attempted, []string{root})
81+
}
82+
83+
// A directory that disappeared mid-walk has nothing left below it to skip.
84+
func TestWalkAndAddIgnoresDirThatDisappeared(t *testing.T) {
85+
root := t.TempDir()
86+
d := &naiveNotify{
87+
notifyList: map[string]bool{root: true},
88+
addWatch: func(string) error {
89+
return &fs.PathError{Op: "open", Path: root, Err: fs.ErrNotExist}
90+
},
91+
}
92+
93+
err := d.walkAndAdd(root, dirEntry(t, root), nil)
94+
95+
assert.NilError(t, err)
96+
}
97+
98+
func TestWalkAndAddPropagatesOtherWatchErrors(t *testing.T) {
99+
root := t.TempDir()
100+
d := &naiveNotify{
101+
notifyList: map[string]bool{root: true},
102+
addWatch: func(string) error {
103+
return errors.New("boom")
104+
},
105+
}
106+
107+
err := d.walkAndAdd(root, dirEntry(t, root), nil)
108+
109+
assert.ErrorContains(t, err, "boom")
110+
}
111+
112+
func TestWalkAndNotifySkipsUnreadableDir(t *testing.T) {
113+
d := &naiveNotify{}
114+
115+
err := d.walkAndNotify("/nope")("/nope/inner", nil, permissionError("/nope/inner"))
116+
117+
assert.Equal(t, err, filepath.SkipDir)
118+
}
119+
120+
func TestWalkAndNotifyPropagatesOtherWalkErrors(t *testing.T) {
121+
d := &naiveNotify{}
122+
boom := errors.New("boom")
123+
124+
err := d.walkAndNotify("/nope")("/nope/inner", nil, boom)
125+
126+
assert.Assert(t, errors.Is(err, boom))
127+
}

0 commit comments

Comments
 (0)