Repository: fsnotify/fsnotify (go)

Issue:
Directory partially watched if a file fails to open during addWatch()
**Describe the bug**
I ran into this issue while using https://github.com/spf13/viper which uses this library as a dependency and I understand that this issue may not be related to fsnotify but rather how viper handles the error. 
Suppose there's a directory that looks like this:
```
dir/
|- a.txt <- watched
|- b/ <- unix.Open returns EPERM 1 Operation not permitted
|- c.yaml <- file I'm interested in, not watched
```
And these 2 things happen:
1. c.yaml is never reached
2. The only event that's ever going to be fired is "b/ is created"

So in viper a watch for a directory is added here https://github.com/spf13/viper/blob/v1.10.1/viper.go#L450. During `Add()`, the code loops through all the files under dir/ here https://github.com/fsnotify/fsnotify/blob/v1.5.1/kqueue.go#L398, which include a.txt, b/, and c.yaml, in order. 
```
	for _, fileInfo := range files {
		filePath := filepath.Join(dirPath, fileInfo.Name())
		filePath, err = w.internalWatch(filePath, fileInfo)
		if err != nil {
			return err
		}

		w.mu.Lock()
		w.fileExists[filePath] = true
		w.mu.Unlock()
	}
```
When `w.internalWatch()` is called for b/, an err is returned, which causes Add() to return with err. However, the watcher is already updated with `w.fileExists["dir/a.txt"] = true`. On the other end of the watcher, the `w.readEvents()` goroutine loops over all the files under dir/ again to discover new files. Relevant code here: https://github.com/fsnotify/fsnotify/blob/v1.5.1/kqueue.go#L444
```
func (w *Watcher) sendFileCreatedEventIfNew(filePath string, fileInfo os.FileInfo) (err error) {
	w.mu.Lock()
	_, doesExist := w.fileExists[filePath]
	w.mu.Unlock()
	if !doesExist {
		// Send create event
		select {
		case w.Events <- newCreateEvent(filePath):
		case <-w.done:
			return
		}
	}

	// like watchDirectoryFiles (but without doing another ReadDir)
	filePath, err = w.internalWatch(filePath, fileInfo)
	if err != nil {
		return err
	}
```
It first sees a.txt, which `doesExist`. It then sees b/. b/'s `doesExist` is false so a new event is fired. When the code runs to `filePath, err = w.internalWatch(filePath, fileInfo)`, it errors again and returns early without going through the rest of the directory.


**To Reproduce**


**Expected behavior**
I think ideally for my use case, either make `watchDirectoryFiles()` and `sendDirectoryChangeEvents()` continue if a file fails to open, or have another map to track files that fail to open so it doesn't keep firing off events about them.

**Which operating system and version are you using?**

macOS: sw_vers
```
ProductName:	macOS
ProductVersion:	11.6.4
BuildVersion:	20G417
```

**Additional context**
If applicable, add screenshots or a code sample to help explain your problem.


Pull request description:
kqueue: don't set up watchers on unreadable files
Watching an entire directory would fail if it contains a file we can't
read due to permission errors. That's obviously not ideal, so just skip
the file.

Also add a test for trying to watch a directory where the permissions
are denied, and test the error is what's expected; this prevents
regressions like in #393.

Fixes #439

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index dfd991f..f64f270 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -8,7 +8,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ## [Unreleased]
 
 This version of fsnotify needs Go 1.16 (this was already the case since 1.5.1,
-but not documented).
+but not documented). It also increases the minimum Linux version to 2.6.32.
 
 ### Additions
 
@@ -26,19 +26,19 @@ but not documented).
 
 - all: add cmd/fsnotify (#463)
 
-  A command-line utility for testing. This is mainly useful for developers and
-  bug reports.
+  A command-line utility for testing and as an example.
 
 ### Changes and fixes
 
 - inotify: don't ignore events for files that don't exist (#260, #470)
 
-  Previously the inotify watcher would call os.Lstat() to check if a file still
-  exists before emitting events.
+  Previously the inotify watcher would call `os.Lstat()` to check if a file
+  still exists before emitting events.
 
-  This was inconsistent with other platforms, racy, resulted in inconsistent
-  event reporting, and generally a source of confusion. It was added in 2013 to
-  fix a memory leak that no longer exists.
+  This was inconsistent with other platforms and resulted in inconsistent event
+  reporting (e.g. when a file is quickly removed and re-created), and generally
+  a source of confusion. It was added in 2013 to fix a memory leak that no
+  longer exists.
 
 - all: return `ErrNonExistentWatch` when `Remove()` is called on a path that's
   not watched (#460)
@@ -49,9 +49,22 @@ but not documented).
   written in 2014, but now it is. As a result, the minimum Linux version is
   bumped from 2.6.27 to 2.6.32. This hugely simplifies the code and is faster.
 
+- kqueue: don't check for events every 100ms (#480)
+
+  The watcher would wake up every 100ms, even when there was nothing to do. Now
+  it waits until there is something to do.
+
 - macos: retry opening files on EINTR (#475)
 
-- windows: fix renaming a watched directory, if the parent is also watched (#370)
+- kqueue: skip unreadable files (#479)
+
+  kqueue requires a file descriptor for every file in a directory; this would
+  fail if a file was unreadable by the current user. Now these files are simply
+  skipped.
+
+- windows: fix renaming a watched directory if the parent is also watched (#370)
+
+- windows: increase buffer size from 4K to 64K (#485)
 
 - windows: close file handle on Remove() (#288)
 
diff --git a/internal/unix.go b/internal/unix.go
new file mode 100644
index 0000000..7c73a9d
--- /dev/null
+++ b/internal/unix.go
@@ -0,0 +1,15 @@
+//go:build !windows
+// +build !windows
+
+package internal
+
+import (
+	"syscall"
+
+	"golang.org/x/sys/unix"
+)
+
+var (
+	SyscallEACCES = syscall.EACCES
+	UnixEACCES    = unix.EACCES
+)
diff --git a/internal/windows.go b/internal/windows.go
new file mode 100644
index 0000000..6f20859
--- /dev/null
+++ b/internal/windows.go
@@ -0,0 +1,14 @@
+//go:build windows
+// +build windows
+
+package internal
+
+import (
+	"errors"
+)
+
+// Just a dummy.
+var (
+	SyscallEACCES = errors.New("dummy")
+	UnixEACCES    = errors.New("dummy")
+)
diff --git a/kqueue.go b/kqueue.go
index c63e7b6..765e85f 100644
--- a/kqueue.go
+++ b/kqueue.go
@@ -199,13 +199,8 @@ func (w *Watcher) addWatch(name string, flags uint32) (string, error) {
 			return "", err
 		}
 
-		// Don't watch sockets.
-		if fi.Mode()&os.ModeSocket == os.ModeSocket {
-			return "", nil
-		}
-
-		// Don't watch named pipes.
-		if fi.Mode()&os.ModeNamedPipe == os.ModeNamedPipe {
+		// Don't watch sockets or named pipes
+		if (fi.Mode()&os.ModeSocket == os.ModeSocket) || (fi.Mode()&os.ModeNamedPipe == os.ModeNamedPipe) {
 			return "", nil
 		}
 
@@ -427,14 +422,24 @@ func (w *Watcher) watchDirectoryFiles(dirPath string) error {
 	}
 
 	for _, fileInfo := range files {
-		filePath := filepath.Join(dirPath, fileInfo.Name())
-		filePath, err = w.internalWatch(filePath, fileInfo)
+		path := filepath.Join(dirPath, fileInfo.Name())
+
+		cleanPath, err := w.internalWatch(path, fileInfo)
 		if err != nil {
-			return fmt.Errorf("%q: %w", filepath.Join(dirPath, fileInfo.Name()), err)
+			// No permission to read the file; that's not a problem: just skip.
+			// But do add it to w.fileExists to prevent it from being picked up
+			// as a "new" file later (it still shows up in the directory
+			// listing).
+			switch {
+			case errors.Is(err, unix.EACCES) || errors.Is(err, unix.EPERM):
+				cleanPath = filepath.Clean(path)
+			default:
+				return fmt.Errorf("%q: %w", filepath.Join(dirPath, fileInfo.Name()), err)
+			}
 		}
 
 		w.mu.Lock()
-		w.fileExists[filePath] = struct{}{}
+		w.fileExists[cleanPath] = struct{}{}
 		w.mu.Unlock()
 	}
 


Test patch:
diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
index e926f07..15bae7a 100644
--- a/.github/workflows/test.yml
+++ b/.github/workflows/test.yml
@@ -42,7 +42,8 @@ jobs:
           usesh: true
           prepare: pkg install -y go
           run: |
-            go test -race ./...
+            pw user add -n action -m
+            su action -c 'go test -race ./...'
 
   testOpenBSD:
     runs-on: macos-12
@@ -54,18 +55,18 @@ jobs:
         uses: vmactions/openbsd-vm@v0.0.6
         with:
           prepare: pkg_add go
+          # No -race as the VM doesn't include the comp set.
+          #
+          # TODO: should probably add this, but on my local machine the tests
+          #       time out with -race as the waits aren't long enough (OpenBSD
+          #       is kind of slow), so should probably look into that first.
+          #       Go 1.19 is supposed to have a much faster race detector, so
+          #       maybe waiting until we have that is enough.
           run: |
             # Default of 512 leads to "too many open files".
             ulimit -n 1024
-
-            # No -race as the VM doesn't include the comp set.
-            #
-            # TODO: should probably add this, but on my local machine the tests
-            #       time out with -race as the waits aren't long enough (OpenBSD
-            #       is kind of slow), so should probably look into that first.
-            #       Go 1.19 is supposed to have a much faster race detector, so
-            #       maybe waiting until we have that is enough.
-            go test ./...
+            useradd -mG wheel action
+            su action -c 'go test ./...'
 
   testNetBSD:
     runs-on: macos-12
@@ -77,9 +78,10 @@ jobs:
         uses: vmactions/netbsd-vm@v0.0.4
         with:
           prepare: pkg_add go
+          # TODO: no -race for the same reason as OpenBSD (the timing; it does run).
           run: |
-            # TODO: no -race for the same reason as OpenBSD (the timing; it does run).
-            go117 test ./...
+            useradd -mG wheel action
+            su action -c 'go117 test ./...'
 
   testillumos:
     runs-on: macos-12
diff --git a/integration_test.go b/integration_test.go
index 3367e80..da9bb20 100644
--- a/integration_test.go
+++ b/integration_test.go
@@ -4,6 +4,7 @@
 package fsnotify
 
 import (
+	"errors"
 	"fmt"
 	"path/filepath"
 	"runtime"
@@ -11,6 +12,8 @@ import (
 	"sync/atomic"
 	"testing"
 	"time"
+
+	"github.com/fsnotify/fsnotify/internal"
 )
 
 func TestWatch(t *testing.T) {
@@ -79,6 +82,31 @@ func TestWatch(t *testing.T) {
 				remove /sub
 				remove /file
 		`},
+
+		{"file in directory is not readable", func(t *testing.T, w *Watcher, tmp string) {
+			if runtime.GOOS == "windows" {
+				t.Skip("attributes don't work on Windows")
+			}
+
+			touch(t, tmp, "file-unreadable")
+			chmod(t, 0, tmp, "file-unreadable")
+			touch(t, tmp, "file")
+			addWatch(t, w, tmp)
+
+			cat(t, "hello", tmp, "file")
+			rm(t, tmp, "file")
+			rm(t, tmp, "file-unreadable")
+		}, `
+			WRITE     "/file"
+			REMOVE    "/file"
+			REMOVE    "/file-unreadable"
+
+			# We never set up a watcher on the unreadable file, so we don't get
+			# the REMOVE.
+			kqueue:
+                WRITE    "/file"
+                REMOVE   "/file"
+		`},
 	}
 
 	for _, tt := range tests {
@@ -445,6 +473,38 @@ func TestClose(t *testing.T) {
 	})
 }
 
+func TestAdd(t *testing.T) {
+	t.Run("permission denied", func(t *testing.T) {
+		if runtime.GOOS == "windows" {
+			t.Skip("attributes don't work on Windows")
+		}
+
+		t.Parallel()
+
+		tmp := t.TempDir()
+		dir := filepath.Join(tmp, "dir-unreadable")
+		mkdir(t, dir)
+		touch(t, dir, "/file")
+		chmod(t, 0, dir)
+
+		w := newWatcher(t)
+		defer func() {
+			w.Close()
+			chmod(t, 0o755, dir) // Make TempDir() cleanup work
+		}()
+		err := w.Add(dir)
+		if err == nil {
+			t.Fatal("error is nil")
+		}
+		if !errors.Is(err, internal.UnixEACCES) {
+			t.Errorf("not unix.EACCESS: %T %#[1]v", err)
+		}
+		if !errors.Is(err, internal.SyscallEACCES) {
+			t.Errorf("not syscall.EACCESS: %T %#[1]v", err)
+		}
+	})
+}
+
 // TODO: should also check internal state is correct/cleaned up; e.g. no
 //       left-over file descriptors or whatnot.
 func TestRemove(t *testing.T) {