Deep Review: 20260421-172247-pr-326

// Cleanup removes the watch for the input filename if necessary.
func Cleanup(fname string) error {
	return untrack(fname)
}

// watchFlags calls fsnotify.WatchFlags for the input filename and flags, creating
// a new Watcher if the previous Watcher was closed.
func (t *InotifyTracker) addWatch(winfo *watchInfo) error {
	t.mux.Lock()
	defer t.mux.Unlock()

	if t.chans[winfo.fname] == nil {

The doc names a function (watchFlags) and a behavior ("creating a new Watcher if the previous Watcher was closed") that the current code does not implement. addWatch tracks per-file reference counts; it never recreates the watcher. The comment was carried over verbatim when the watch/ subpackage was absorbed, and subsequent commits reshaped the surrounding design further.

Fix:

-// watchFlags calls fsnotify.WatchFlags for the input filename and flags, creating
-// a new Watcher if the previous Watcher was closed.
+// addWatch registers a new per-file watcher. If the filename is not yet
+// tracked, it also subscribes the shared fsnotify.Watcher to the path
+// (the parent directory when winfo.isCreate()).
 func (t *InotifyTracker) addWatch(winfo *watchInfo) error {

	go func() {
		for err := range t.watcher.Errors {
			if err == nil {
				continue
			}
			var sysErr *os.SyscallError
			if !errors.As(err, &sysErr) || !errors.Is(sysErr.Err, syscall.EINTR) {
				logger.Printf("Error in Watcher Error channel: %s", err)
			}
		}
	}()

	// Handle Add/Remove RPCs. Synchronous calls into fsnotify may
	// block until readEvents replies; that's fine here because the

fsnotify's Linux backend forwards w.inotifyFile.Read(buf[:]) errors directly to sendError. os.File.Read returns *os.PathError, whose Err field is typically syscall.Errno directly — not wrapped in *os.SyscallError. errors.As(err, &sysErr) walks the chain looking for *os.SyscallError and returns false; the disjunct evaluates to true and an EINTR-bearing PathError ends up logged as an error rather than silently discarded. errors.Is walks through both wrappers and matches syscall.Errno's own Is method.

Commit a2d5c9f (the lint cleanup that rewrote this block) is a natural place to correct the pattern.

Fix:

-for err := range t.watcher.Errors {
-    if err == nil {
-        continue
-    }
-    var sysErr *os.SyscallError
-    if !errors.As(err, &sysErr) || !errors.Is(sysErr.Err, syscall.EINTR) {
-        logger.Printf("Error in Watcher Error channel: %s", err)
-    }
-}
+for err := range t.watcher.Errors {
+    if err == nil || errors.Is(err, syscall.EINTR) {
+        continue
+    }
+    logger.Printf("Error in Watcher Error channel: %s", err)
+}

The os and errors.As imports are then unused; drop the first and keep errors for errors.Is.

	if ch != nil && done != nil {
		// remove() may close done and removeWatch() may close ch while we
		// are between the map lookup and the select. Both cases are then
		// select-ready; the send path panics on a closed ch. Dropping the
		// event is correct because the watch is being removed.
		defer func() { _ = recover() }()
		select {
		case ch <- event:
		case <-done:
		}
	}

The comment correctly identifies one panic path — send on a closed channel — but the recover() is unconditional and swallows every panic that reaches it, including unrelated bugs a future refactor might introduce (nil dereference, map corruption, etc.). Narrowing the recover to match the specific runtime panic keeps the safety net while letting genuine bugs surface.

Fix (narrow recover to "send on closed channel"):

-    defer func() { _ = recover() }()
+    defer func() {
+        if r := recover(); r != nil {
+            if s, ok := r.(error); ok && s.Error() == "send on closed channel" {
+                return
+            }
+            if s, ok := r.(string); ok && s == "send on closed channel" {
+                return
+            }
+            panic(r)
+        }
+    }()

The Go runtime panic string is stable but not a documented API, so this is weaker than a structural fix. If that feels too brittle, the cleaner alternative is to route close(ch) through a done-driven drain so the send path never races with a close. That is a larger restructure and can wait.

// forward through Info so a stray call at least surfaces in the log.
type tailLogger struct {
	logger logr.Logger
}

func (t tailLogger) Fatal(v ...any)                 { t.logger.Info(fmt.Sprint(v...)) }
func (t tailLogger) Fatalf(format string, v ...any) { t.logger.Info(fmt.Sprintf(format, v...)) }
func (t tailLogger) Fatalln(v ...any)               { t.logger.Info(fmt.Sprintln(v...)) }
func (t tailLogger) Panic(v ...any)                 { t.logger.Info(fmt.Sprint(v...)) }
func (t tailLogger) Panicf(format string, v ...any) { t.logger.Info(fmt.Sprintf(format, v...)) }
func (t tailLogger) Panicln(v ...any)               { t.logger.Info(fmt.Sprintln(v...)) }
func (t tailLogger) Print(v ...any)                 { t.logger.Info(fmt.Sprint(v...)) }
func (t tailLogger) Printf(format string, v ...any) { t.logger.Info(fmt.Sprintf(format, v...)) }
func (t tailLogger) Println(v ...any)               { t.logger.Info(fmt.Sprintln(v...)) }

The comment above the type is correct today — pkg/util/tail only calls Printf — but the Fatal* and Panic* methods silently demote their calling contract to Info. If a future pkg/util/tail change calls Logger.Fatal (expecting os.Exit) or Logger.Panic (expecting a panic), the caller assumption breaks silently.

Fix: honor each method's semantic so a misuse fails loudly. Fatal* should call os.Exit(1) after logging; Panic* should panic(fmt.Sprint(v...)) after logging. Both behaviors match the log.Logger contract and make a library misuse an immediate, debuggable failure.

The review-2 change returns haStdoutTail.Err() / haStderrTail.Err() when Lines closes, so runWatcher can log "Event watcher failed" on a real underlying error. The five tests in events_test.go cover callback-true stop, the begin filter, unparseable lines, ctx cancel, and stderr drain, but none force a tail to die with an error and assert that Watch returns that error. A regression that silently dropped fatal tail errors (a premature return nil) would ship green.

Fix: add a case that forces the underlying tail to fail (e.g. write to a path that becomes a directory mid-stream, or expose a test hook that calls t.Kill(someErr)) and assert Watch returns the propagated error.

// which is the correct behaviour when the watch is being removed.
//
// Without the recover, each iteration panics on the send case with
// ~50% probability. 100 iterations make a missed regression vanishingly
// unlikely (1 - 0.5^100).
func TestSendEventNoPanicOnConcurrentClose(_ *testing.T) {
	tracker := &InotifyTracker{
		mux:       sync.Mutex{},
		chans:     make(map[string]chan fsnotify.Event),
		done:      make(map[string]chan bool),
		watchNums: make(map[string]int),
	}

	const fname = "test-file"
	ch := make(chan fsnotify.Event)
	done := make(chan bool)
	tracker.chans[fname] = ch
	tracker.done[fname] = done
	close(done)
	close(ch)

	for range 100 {
		tracker.sendEvent(fsnotify.Event{Name: fname})
	}
}

Two weaknesses. First, the signature (_ *testing.T) discards the parameter, so the body cannot call t.Fatalf or t.Error — the test fails only if sendEvent itself panics. That is enough to prove recover() catches the panic, but the comment claims "~50% probability; 100 iterations make a missed regression vanishingly unlikely" while both channels are pre-closed before the loop. The test never forces an actual race; every iteration sees select with both cases ready. Second, the real race the guard is meant to handle — a concurrent removeWatch closing ch between the lock-protected map lookup in sendEvent and the select — is not exercised by this test shape.

Fix: rename to TestSendEventSurvivesClosedChannel and update the comment, or — better — spin up the full tracker via goRun, run concurrent track/sendEvent/remove from separate goroutines in a tight loop, and assert no panic across 10000 iterations.

The type doc on FileChanges describes three notification channels but not their delivery semantics. The sendOnlyIfEmpty helper and the size-1 buffers implement latest-only delivery: a notification arriving while another is pending is dropped. tail.waitForChanges drains exactly one channel per call, so two events between calls (e.g., Truncated then Modified) lose one silently. Current callers tolerate coalescing — a Truncated always triggers a reopen that re-reads new content — but a future reader could reasonably expect edge-triggered delivery.

Fix:

 // FileChanges groups the channels a FileWatcher uses to signal
-// file-level modifications, truncations, and deletions.
+// file-level modifications, truncations, and deletions. Each channel
+// is buffered to size 1; a notification that arrives while a prior
+// one is still pending is dropped (coalesced). Consumers that need
+// to observe every event must not rely on FileChanges alone.
 type FileChanges struct {

stream.go:50-53 sets writeErr and calls t.Kill(err) when the destination writer returns an error mid-stream. TestStream exercises both follow=false and follow=true with io.Pipe() but only closes the writer after the stream is already finishing, so the mid-stream error path is never hit. The review-2 changes to Stream specifically added this writer-error path; a regression that broke it would ship green.

	var writeErr error
	for line := range t.Lines {
		if writeErr != nil {
			continue // drain Lines so tailFileSync can exit cleanly
		}
		if _, err := fmt.Fprintln(writer, line.Text); err != nil {
			writeErr = err
			t.Kill(err) // non-blocking; keep draining Lines so tailFileSync can finish
		}
	}
	// Block until tailFileSync's full defer chain (including watchers.Wait)
	// has run, so a caller that re-opens the same file does not race with
	// an untrack still in flight on the shared InotifyTracker. Wait also
	// surfaces any fatal error the tail hit (e.g. a mid-stream read error

Fix: add a case that wraps the writer to return io.ErrClosedPipe after N bytes and asserts Stream returns that error without hanging.