# Deep Review: 20260421-172247-pr-326

| | |
|---|---|
| **Date** | 2026-04-21 17:22 |
| **Repo** | [rancher-sandbox/rancher-desktop-daemon](https://github.com/rancher-sandbox/rancher-desktop-daemon) |
| **Round** | 1 (of target) |
| **Author** | [@jandubois](https://github.com/jandubois) |
| **PR** | [#326](https://github.com/rancher-sandbox/rancher-desktop-daemon/pull/326) — Fix deadlock in vendored nxadm/tail (observed as Windows CI hostagent-watcher hang) |
| **Branch** | `fix-tail-windows-deadlock` |
| **Commits** | `a9bcf89` nxadmtail: absorb upstream watch/, util/, winfile/ subpackages<br>`35d6f05` tail: rename pkg/util/nxadmtail to pkg/util/tail, merge with CLI wrapper<br>`e074dd9` tail/watch: fix deadlock in the shared InotifyTracker<br>`92881d9` rdd: switch hostagent watcher to local tail-based events package<br>`a2d5c9f` tail: resolve golangci-lint issues in the absorbed code<br>`32015c9` tail/watch: synchronise watcher goroutine cleanup with Stop<br>`673e91e` Address review 1 findings for PR #326 + prep tail for spin-out<br>`6d977b0` Address review 2 findings for PR #326 |
| **Reviewers** | Claude Opus 4.7 (effort: xhigh), Codex GPT 5.4 (effort: high), Gemini 3.1 Pro (effort: default) |
| **Verdict** | **Merge with optional fixes** — the deadlock fix is structurally sound and the lifecycle race is correctly closed; remaining findings are latent crash risks in absorbed upstream code plus quality/polish items. |
| **Wall-clock time** | `16 min 43 s` |

---

## Executive Summary

PR #326 absorbs `github.com/nxadm/tail` (plus its `watch/`, `util/`, and `winfile/` subpackages) in-tree at `pkg/util/tail`, fixes a Windows deadlock in the shared `InotifyTracker` by splitting its single `select` into three goroutines (RPC handler + events drainer + errors drainer), closes a subsequent lifecycle race between `tail.Stop()` and the per-watcher cleanup goroutine via a `sync.WaitGroup`, and replaces rdd's usage of `lima/pkg/hostagent/events.Watch` with a local `pkg/hostagent/events` package backed by the patched tail fork.

Structure: 2 vendoring/rename commits, 1 deadlock fix, 1 call-site swap, 1 lint cleanup, 1 lifecycle-race fix, 2 review fixup commits. The root-cause analysis in the PR description is accurate (verified against `fsnotify` backend source), the fix preserves the process-wide singleton so Linux `fs.inotify.max_user_instances` budgets are unaffected, and the 200-cycle stress test reproduces both races and is gated behind an explicit `TAIL_STRESS=1` env var plus a dedicated CI job.

Agents agreed on zero critical issues. Gemini raised one important-severity concern: the absorbed `watch/` subpackage still `panic()`s on a handful of I/O and initialization errors, which now propagate from rdd's own process (see I1). Claude raised eight suggestions covering a stale upstream doc comment, an EINTR check that misses `*os.PathError` wrapping, a narrow test that doesn't exercise the concurrency it claims to, and a few untested error paths. Codex returned no findings.

---

## Critical Issues

None.

## Important Issues

I1. **Absorbed `watch/` code panics on I/O errors, now crashing the rdd daemon** — `pkg/util/tail/watch/inotify.go:132`, `pkg/util/tail/watch/inotify_tracker.go:253` [Gemini 3.1 Pro]

```go
// inotify.go:131-132 — inside ChangeEvents's goroutine
// XXX: report this error back to the user
panic(fmt.Sprintf("Failed to stat file %v: %v", fw.Filename, err))
```

```go
// inotify_tracker.go:250-254 — inside the shared run() goroutine
func (t *InotifyTracker) run() {
    watcher, err := fsnotify.NewWatcher()
    if err != nil {
        panic("failed to create fsnotify.Watcher: " + err.Error())
    }
```

Absorbing `nxadm/tail`'s `watch/` subpackage brings its `panic()` calls from an external library into the rdd daemon's own process. Two call sites are now reachable from the hostagent watcher path:

1. `inotify.go:132` fires inside the per-file `ChangeEvents` goroutine when `os.Stat` on the watched log file returns an error other than `IsNotExist` (and, on Windows, other than `IsPermission`). Transient I/O errors, EBUSY on Windows, or any future stat-error class would crash the entire rdd process.
2. `inotify_tracker.go:253` fires inside the shared tracker's `run()` goroutine if `fsnotify.NewWatcher()` fails at first use (EMFILE/ENOMEM under host inotify exhaustion). An unrecovered panic in a goroutine crashes the process.

A third panic in `polling.go:95` exists but rdd does not set `Poll=true`, so it is unreachable on the hostagent path today.

The PR does not introduce these panics — the comment `// XXX: report this error back to the user` is upstream's. The exposure is new: previously the panic originated in a third-party dependency, now it originates in code rdd owns and ships.

Fix: on `Stat` failure in the watcher goroutines, treat the error as a deletion so the tail reopens on the next cycle instead of crashing. For `fsnotify.NewWatcher()`, surface the error through the first `track()` RPC's `t.error` channel instead of panicking in a detached goroutine.

```diff
 fi, err := os.Stat(fw.Filename)
 if err != nil {
     if os.IsNotExist(err) {
         _ = untrack(fw.Filename)
         changes.NotifyDeleted()
         return
     }
-    // XXX: report this error back to the user
-    panic(fmt.Sprintf("Failed to stat file %v: %v", fw.Filename, err))
+    _ = untrack(fw.Filename)
+    changes.NotifyDeleted()
+    return
 }
```

## Suggestions

S1. **Stale `watchFlags` comment on `addWatch`** — `pkg/util/tail/watch/inotify_tracker.go:142-143` [Claude Opus 4.7]

```go
// 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 {
```

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:

```diff
-// 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 {
```

S2. **EINTR check misses `*os.PathError` wrapping** — `pkg/util/tail/watch/inotify_tracker.go:274-277` [Claude Opus 4.7]

```go
var sysErr *os.SyscallError
if !errors.As(err, &sysErr) || !errors.Is(sysErr.Err, syscall.EINTR) {
    logger.Printf("Error in Watcher Error channel: %s", err)
}
```

`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:

```diff
-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`.

S3. **Bare `recover()` in `sendEvent` swallows unrelated panics** — `pkg/util/tail/watch/inotify_tracker.go:221` [Claude Opus 4.7]

```go
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"):

```diff
-    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.

S4. **`tailLogger.Fatal`/`Panic` silently demote their contracts** — `pkg/hostagent/events/events.go:148-156` [Claude Opus 4.7]

```go
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) Panic(v ...any)                 { t.logger.Info(fmt.Sprint(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.

S5. **`events.Watch`'s tail-error propagation has no test** — `pkg/hostagent/events/events.go:107-108,131` [Claude Opus 4.7]

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.

S6. **`TestSendEventNoPanicOnConcurrentClose` tests the guard, not the race** — `pkg/util/tail/watch/inotify_tracker_test.go:25-44` [Claude Opus 4.7]

```go
func TestSendEventNoPanicOnConcurrentClose(_ *testing.T) {
    tracker := &InotifyTracker{...}
    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.

S7. **`FileChanges` doc does not mention coalescing** — `pkg/util/tail/watch/filechanges.go:8-22` [Claude Opus 4.7]

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:

```diff
 // 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 {
```

S8. **`Stream` writer-error path has no test** — `pkg/util/tail/stream_test.go:20-131` [Claude Opus 4.7]

`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.

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

---

## Design Observations

### Strengths

- **Three-goroutine split preserves the singleton** (in-scope) [Codex GPT 5.4, Claude Opus 4.7] — Splitting `InotifyTracker.run()` into an RPC handler, an events drainer, and an errors drainer fixes the deadlock without creating a second `fsnotify.Watcher`. Linux's `fs.inotify.max_user_instances` budget stays untouched, and the inline comment at `inotify_tracker.go:229-249` explains the cycle well enough that a maintainer can reconstruct the reasoning without reading fsnotify internals.

- **WaitGroup-based teardown closes the real lifecycle gap** (in-scope) [Codex GPT 5.4, Gemini 3.1 Pro] — The `Tail.watchers sync.WaitGroup`, the `ChangeEvents(*tomb.Tomb, int64, *sync.WaitGroup)` signature change, and the `defer tail.watchers.Wait()` between `close()` and `Done()` in `tailFileSync` give `tail.Stop()` an end-of-life guarantee that matches what callers assume. The polling watcher takes the same signature for interface symmetry even though it has no `track`/`untrack` to wait on, which keeps the `FileWatcher` contract uniform.

- **Local hostagent events package inherits controller-runtime logging** (in-scope) [Claude Opus 4.7] — `pkg/hostagent/events/events.go:74` pulls the caller's `logr.Logger` via `log.FromContext(ctx)` and wires it into the tail config, so tail diagnostics inherit the controller-runtime instance/component fields. Upstream `lima/pkg/hostagent/events.Watch` dumps through a global logrus logger, which loses that context.

- **Stress-test gating matches the opt-in rule** (in-scope) [Claude Opus 4.7] — The 7-minute stress test is guarded by both `testing.Short()` and `TAIL_STRESS=1`, and a dedicated CI job sets the env var. An opt-out CI never sets would be effectively off; opt-in with an explicit CI job is the right shape.

### Concerns

- **Package-level logger writes straight to `os.Stderr`** (future) [Claude Opus 4.7] — `inotify_tracker.go:65` keeps a package-level `log.Logger` writing to `os.Stderr` with `log.LstdFlags`. In the rdd daemon, the rdd hostagent subprocess's stderr is captured by the parent and parsed as JSON logrus output (see `pkg/hostagent/command.go`); a tracker error would land there as a raw `log.LstdFlags` line and trip the JSON parser. Making the logger injectable via a package-level `SetLogger` would let rdd plug in its own, and the move is a natural prerequisite for spinning `pkg/util/tail` out as a standalone module.

- **Shared per-filename event channel prevents concurrent tails of the same path** (future) [Gemini 3.1 Pro] — `InotifyTracker.chans` is keyed by filename, so two `Tail` instances opened for the same path share a single unbuffered channel. Each event goes to whichever goroutine wins the receive race; the other silently misses it. rdd currently tails mutually exclusive paths (`ha.stdout.log` vs `ha.stderr.log`) so this does not bite, but it is worth flagging before the package is spun out as a general-purpose library. Keying by a unique watch-instance ID would fix it without changing the singleton design.

## Testing Assessment

Unit coverage for `pkg/hostagent/events` is solid (callback stop, begin filter, unparseable line, ctx cancel, stderr drain). The `TestInotifyTrackerNoDeadlockOnRepeatedRotation` stress test reliably reproduces both the Windows deadlock (cycles 2-30) and the Linux lifecycle race (~cycle 130) on the pre-fix code, and passes green post-fix. The writer subprocess is necessary because only external-process writes produce enough `ReadDirectoryChanges` volume to trigger fsnotify's overflow error on Windows, and the `GOMAXPROCS=1` setting mirrors the 2-vCPU windows-latest runner.

Gaps, ranked by risk:

1. `events.Watch` fatal-tail error propagation has no test (S5).
2. `InotifyTracker.sendEvent`'s concurrency guard is tested with pre-closed channels, not a real race (S6).
3. `Stream`'s writer-error path is uncovered (S8).
4. No controller-level integration test exercises `hostagent_watcher.go` end-to-end through the new `pkg/hostagent/events.Watch` across a hostagent restart/rotation sequence [Codex GPT 5.4].
5. No command-level test covers `rdd service log[s]` or `rdd limavm log[s]` through `tail.Stream` [Codex GPT 5.4].

## Documentation Assessment

- `pkg/util/tail/watch/inotify_tracker.go:142-143` carries a stale `watchFlags` comment (S1).
- `pkg/util/tail/watch/filechanges.go` omits the coalescing semantic (S7).
- `pkg/hostagent/events/events.go:57-72` gives `Watch` a precise contract (return conditions, line filtering, stderr handling, and the dropped `propagateStderr` flag).
- `pkg/util/tail/tail.go:8-18` correctly sources the vendored-plus-patched code.
- The `NOTICE` entries track the rename to `pkg/util/tail`.

## Commit Structure

Clean. Each of the eight commits is a self-contained step — absorb upstream, rename/merge with the CLI wrapper, apply the deadlock fix, swap rdd's watcher, resolve lint, add the lifecycle-race fix, and two review-fixup commits. The two fixup commits stay explicitly scoped to review feedback, matching the project convention of preserving rebase reviewability. Commit messages describe motivation, not mechanics.

## Acknowledged Limitations

- `events.go:86-90, 96-100` — `defer` blocks explicitly note that calling `tail.Cleanup` after `Stop` would unregister the tracker entry and prevent subsequent tailing of the same file. Intentional.
- `inotify_tracker.go:229-249` — `run()` deliberately allows the RPC goroutine to block on fsnotify; the drainers prevent starvation. No stronger guarantee is claimed.
- PR description §"Why not fix it upstream first" — upstreaming to `nxadm/tail` and `fsnotify` is acknowledged as preferable and explicitly deferred.
- PR description §"Out of scope" — converting Lima's `hostagent/events.Watch` to the fixed code is explicitly deferred; rdd bypasses it via the local package.
- `hostagent_watcher.go:101` (pre-existing) — `ev.Status.Degraded` is intentionally not handled because Lima does not emit degraded events. This PR does not change that.

## Unresolved Feedback

None. The PR body reports "No review comments" and no inline PR-review comments were supplied to the agents. Findings from review rounds 1 and 2 are documented as addressed in the `673e91e` and `6d977b0` commit messages.

---

## Agent Performance Retro

### [Claude]

Claude produced the largest finding set (8 suggestions) and all of them survived verification against the code. Its strongest contribution was the EINTR-via-`*os.PathError` analysis (S2), which required tracing through fsnotify's backend source to understand where `sendError` sources its errors and how `os.File.Read` wraps syscall errors — a chain neither other agent followed. S6 correctly diagnoses a test that proves the guard works but does not prove the race handling works, which is a subtle distinction. Two of Claude's findings (S3, S6) were labeled "regression" but describe pre-existing upstream behavior imported through a refactor; classifications updated to "gap" on consolidation. No false positives.

### [Codex]

Codex returned zero findings and a clean bill of health. That matches Claude's and Gemini's read of the deadlock fix and lifecycle race as structurally sound, but a tighter review pass would have caught at least the stale `watchFlags` doc comment and the EINTR check's narrow error-unwrapping — both live in the code Codex marked "Reviewed, no issues." Codex's design-observation calls (single-fsnotify-watcher preservation, WaitGroup shape as "right shape") were accurate and mirrored in the consolidated report. Testing-assessment notes about missing controller-level and command-level integration tests are legitimate and were incorporated.

### [Gemini]

Gemini's single finding (I1: panics in absorbed watcher goroutines) was the only Important-severity result across all three agents, and it surfaces a real daemon-crash risk that the other two agents missed entirely. Its fix proposal for `os.Stat` failures (treat as deletion to force a reopen) is pragmatic and lands cleanly; its alternative for `fsnotify.NewWatcher` (propagate through the first `track()` RPC) is more involved but also correct. Gemini's "Shared channel multiplexing" design observation is accurate and properly scoped as future/pre-existing — it would bite only if the package gained a concurrent-same-path caller. Per the known behavior, Gemini did not run `git blame`; no regression-mislabeling consequences followed because nothing it raised turned on that distinction.

### Summary

| | Claude Opus 4.7 | Codex GPT 5.4 | Gemini 3.1 Pro |
|---|---|---|---|
| Duration | 9m 17s | 5m 31s | 4m 41s |
| Findings | 8S | none | 1I |
| Tool calls | 37 (Read 19, Bash 18) | 43 (shell 42, plan 1) | 7 (grep_search 7) |
| Design observations | 5 | 2 | 4 |
| False positives | 0 | 0 | 0 |
| Unique insights | 8 | 2 | 2 |
| Files reviewed | 25 | 25 | 25 |
| Coverage misses | 0 | 2 | 0 |
| **Totals** | **8S** | **none** | **1I** |
| Downgraded | 0 | 0 | 0 |
| Dropped | 0 | 0 | 0 |


Overall, Claude carried the most signal on this review by count and by depth (the EINTR analysis and test-rigor critique were unique to it), but Gemini's single I1 was the highest-impact finding — a latent daemon-crash path the other two agents accepted as "acknowledged limitation." Codex was the least valuable on this particular PR: no unique findings, though its design-observation accuracy was high.

---

## Review Process Notes

### Skill improvements

- **Promote absorbed-upstream-panics heuristic to a review dimension.** When a PR brings third-party code in-tree (vendoring, forking, or absorbing a dependency), every `panic`, `log.Fatal`, `os.Exit`, and long-lived goroutine in the imported code becomes a crash path that the daemon now owns. Review should flag each one explicitly and require either a replacement or an acknowledged-limitation entry — imported code rarely follows the host project's error-handling conventions, and deferring the audit lets landmines ship. Two of the three agents missed this pattern on this review; only one surfaced it.

### Repo context updates

None. The existing `deep-review-context.md` was sufficient for this review; no entries proved stale against the code under review.

---

## Appendix: Original Reviews

### [Claude] — Pass 1

Now I have enough context. Let me also sanity-check a concern about `errors.Is` vs `errors.As` for EINTR handling, and verify the test file structure.

I have enough context. Let me finalize the review.

---

## Executive Summary

PR #326 absorbs the upstream `nxadm/tail` subpackages into `pkg/util/tail`, fixes a deadlock in the shared `InotifyTracker` by splitting its single-select `run()` into three goroutines, synchronises watcher-goroutine teardown with `tail.Stop()` via a `sync.WaitGroup`, and adds a local `pkg/hostagent/events` package so rdd no longer goes through Lima's unpatched `events.Watch`. The analysis is thorough, the fixes preserve the singleton design, and test coverage (unit + stress) is well-scoped. Findings are minor: a stale upstream doc comment, a slightly too-narrow EINTR check, and a couple of modest documentation and test-coverage suggestions.

## Findings

### Critical Issues

None.

### Important Issues

None.

### Suggestions

S1. **Stale `watchFlags` comment on `addWatch`** — `pkg/util/tail/watch/inotify_tracker.go:142-143` (suggestion, gap)

```go
// 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 {
```

The comment at lines 142-143 names a function (`watchFlags`) and a behaviour (“creating a new Watcher if the previous Watcher was closed”) that do not match the current code at line 144. The function is `addWatch` and it simply tracks a watch count; it never recreates the watcher. The comment was carried over verbatim from upstream when `a9bcf89` absorbed the watch package, and `e074dd9` further changed the surrounding design so the comment is doubly stale. A future reader cross-referencing this with `run()` will be misled.

Fix:

```diff
-// 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 {
```

S2. **EINTR check misses `*os.PathError` wrapping** — `pkg/util/tail/watch/inotify_tracker.go:274-277` (suggestion, gap)

```go
var sysErr *os.SyscallError
if !errors.As(err, &sysErr) || !errors.Is(sysErr.Err, syscall.EINTR) {
    logger.Printf("Error in Watcher Error channel: %s", err)
}
```

`fsnotify.backend_inotify.go:357` forwards `w.inotifyFile.Read(buf[:])` errors directly to `sendError`. `os.File.Read` returns `*os.PathError`, not `*os.SyscallError`. `errors.As(err, &sysErr)` where `sysErr` is `*os.SyscallError` walks the chain looking for that concrete type and returns false; the disjunct evaluates to true and an EINTR arriving via a `PathError` ends up logged as an error rather than silently discarded. The intent (ignore EINTR, log everything else) is better expressed with `errors.Is`, which walks through both wrappers and matches the `syscall.Errno`’s own `Is` method. This was the behaviour in the absorbed upstream code, not a regression introduced by this PR, but the commit that rewrote this block (`a2d5c9f`) is an opportunity to correct it.

Fix:

```diff
-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 `var sysErr` and the `errors.As` import are then unused; drop them.

S3. **`recover()` in `sendEvent` is broader than it needs to be** — `pkg/util/tail/watch/inotify_tracker.go:221` (suggestion, regression)

```go
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 bare `_ = recover()` at line 221 will swallow *any* panic that reaches it, including an unrelated bug such as a nil deref if the maps are ever held to a changed invariant. Narrowing the recover to the exact runtime panic (`"send on closed channel"`) keeps the intended safety net while letting true bugs surface. The send-on-closed-channel runtime panic value is stable and is what `fsnotify` itself checks in `w.send` (`fsnotify.go`).

Fix:

```diff
-    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)
+        }
+    }()
```

If that is considered too brittle, an alternative is to let `removeWatch` close `ch` while the `sendEvent` path is guaranteed not to have a stale local reference — e.g. by moving `close(ch)` behind a `done`-driven drain goroutine. That is a larger restructure and can wait.

S4. **`tailLogger` methods named Fatal/Panic silently swallow their semantics** — `pkg/hostagent/events/events.go:144-156` (suggestion, gap)

```go
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) Panic(v ...any)                 { t.logger.Info(fmt.Sprint(v...)) }
```

The comment on line 141-143 is correct today — `pkg/util/tail` only calls `Printf` — but the `Fatal*` and `Panic*` methods at lines 148-153 downgrade their calling contract to `Info`. If a future `pkg/util/tail` change were to call `Logger.Fatal` (expecting `os.Exit`) or `Logger.Panic` (expecting a panic), the caller assumption would be silently violated. Either honour the contract or make the violation explicit.

Fix: in `Fatal*`/`Panic*`, log through `logr` and then actually invoke the expected semantic — e.g. `os.Exit(1)` for Fatal and `panic(fmt.Sprint(v...))` for Panic — so a library misuse fails loudly rather than silently.

S5. **`events.Watch` fatal-tail error path is not covered by the new tests** — `pkg/hostagent/events/events.go:107-108,131` (suggestion, gap)

The review-2 change returns `haStdoutTail.Err()` / `haStderrTail.Err()` when the `Lines` channel closes (events.go:108, 131), so `runWatcher` can log "Event watcher failed" for a real underlying error. The five tests in `events_test.go` exercise callback-true stop, begin filter, unparseable line, ctx cancel, and stderr drain, but none of them force a tail to die with an error and assert that `Watch` returns that error. A regression that silently drops fatal tail errors (e.g. via a premature `return nil`) would ship green.

Fix: add a test that deletes the stdout log file after a brief delay with `ReOpen=false` disabled via a custom `Config` — or simpler, kills the underlying `tail.Tail` directly through an exported test hook — and asserts `Watch` returns the propagated error. A smaller alternative: write a malformed log path (e.g. a directory rather than a file) to `haStdoutPath` so `tail.Open` succeeds but the subsequent reopen loop errors, and verify the error surfaces.

S6. **`TestSendEventNoPanicOnConcurrentClose` asserts nothing and is not a real concurrency test** — `pkg/util/tail/watch/inotify_tracker_test.go:25-44` (suggestion, regression)

```go
func TestSendEventNoPanicOnConcurrentClose(_ *testing.T) {
    tracker := &InotifyTracker{...}
    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 issues. (1) The signature `(_ *testing.T)` discards the parameter, so the body cannot call any `t.Fatalf`/`t.Error` even if it wanted to — the test fails only if `sendEvent` itself panics. That is enough to guard the `recover()`, but the comment on line 23 says "~50% probability; 100 iterations make a missed regression vanishingly unlikely", which is not a useful statement because the test never forces an actual race — both `ch` and `done` are pre-closed, so every iteration definitely sees the closed channel. It is checking that `recover()` catches the panic, not that the race handling is right. (2) The real race is between a concurrent `removeWatch` closing `ch` *between* the lock-protected map lookup in `sendEvent` and the `select`. The test does not exercise the lock/unlock window at all.

Fix: rename the test to `TestSendEventSurvivesClosedChannel` and update the comment, or — better — write a true concurrency test that spins up the full tracker via `goRun`, calls `track`/`sendEvent`/`remove` from separate goroutines in a tight loop, and asserts no panic across e.g. 10 000 iterations.

S7. **`FileChanges` notification-channel reuse contradicts the declared semantics** — `pkg/util/tail/watch/filechanges.go:10-22` (suggestion, gap)

The doc on `FileChanges` and the channels buffered at size 1 implement a "latest-only" semantic: `sendOnlyIfEmpty` drops a notification if one is already pending. But `tail.waitForChanges` drains only one channel and then returns; on the next call, if two events fired between calls (e.g. Truncated followed by Modified), only one survives and the other is silently dropped. For the current callers this is almost certainly fine — a Truncated will cause a reopen that reads the new content anyway — but the docstring on line 24-28 ("whenever the watched file is modified...") oversells what the API guarantees. A reader could reasonably expect edge-triggered delivery. Worth mentioning in the package or type doc that notifications are coalesced.

Fix: extend the `FileChanges` type doc:

```diff
-// FileChanges groups the channels a FileWatcher uses to signal
-// file-level modifications, truncations, and deletions.
+// FileChanges groups the channels a FileWatcher uses to signal
+// 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.
```

S8. **Stream test is renamed but still passes a writer-error scenario through without covering it** — `pkg/util/tail/stream_test.go:20-131` (suggestion, gap)

The new `stream.go:50-53` path that sets `writeErr` and calls `t.Kill(err)` is not exercised by `TestStream` — both test cases use an `io.Pipe()` whose `w.Close()` comes after the stream is already stopping. A dedicated case that wraps the writer to return an error mid-stream would directly cover that path, which is one of the things the review-2 changes specifically added.

Fix: add a case that uses a custom `io.Writer` which returns `io.ErrClosedPipe` after N bytes and asserts `Stream` returns that error without hanging.

## Design Observations

**Strengths**

- The three-goroutine split of `InotifyTracker.run()` is the right fix shape: it preserves the singleton (keeping the Linux `fs.inotify.max_user_instances` budget intact) while eliminating the circular wait in the clearest possible way. The commit message and the inline comment on lines 229-249 explain the cycle well enough that a future maintainer can reconstruct the reasoning without digging into fsnotify internals.
- Pairing the deadlock fix with a WaitGroup-based lifecycle fix (commit `32015c9`) in the same PR is defensible because the second race was only observable after the first fix removed the serialisation that masked it; they are genuinely the same work unit.
- `pkg/hostagent/events/events.go:74` pulls the caller's `logr.Logger` via `log.FromContext(ctx)` and wires it into the tail config, keeping the controller-runtime structured-log context attached to the tail diagnostics. This is noticeably better than the upstream `lima/pkg/hostagent/events.Watch` behaviour of dumping through global logrus.
- Gating the ~7-minute stress test behind `TAIL_STRESS=1` plus a dedicated CI job, rather than `testing.Short()` alone, is the correct interpretation of the steering-text rule that "an opt-out CI never sets is not a guard".

**Concerns (future)**

- `logger` at `inotify_tracker.go:65` is a package-level `log.Logger` writing to `os.Stderr`. In the rdd service process, the only thing reading that stream is whatever captures the `rdd hostagent` subprocess (which already carries JSON-formatted logrus output); a fsnotify error from the tracker would land there as a raw `log.LstdFlags` line and break the JSON stream parsing logic in `pkg/hostagent/command.go`. If the tail package is spun out as planned, this stays. If not, consider making the logger injectable or route through a package-level `SetLogger` so rdd can attach its own. Future-scope.

## Testing Assessment

Unit coverage for the new `pkg/hostagent/events` package is solid. The stress test reliably reproduces the reported deadlock and lifecycle race and is well isolated. Gaps, ranked by risk:

1. No assertion that `events.Watch` propagates a fatal tail error (see S5).
2. The `InotifyTracker.sendEvent` race is only guarded by a degenerate pre-closed-channel scenario, not a true concurrency test (see S6).
3. `stream.go`'s writer-error + tail-Kill path has no targeted coverage (see S8).
4. No coverage of `ReOpen=true` across rotations in `pkg/hostagent/events` — the stress test exercises it but only in the tail package.

## Documentation Assessment

- `pkg/util/tail/watch/inotify_tracker.go:142-143` carries a stale `watchFlags` comment (S1).
- `pkg/util/tail/watch/filechanges.go` does not mention the coalescing semantic (S7).
- Package and symbol docstrings in the absorbed and rdd-authored files are thorough. `events.go:57-72` gives a clear contract for `Watch`. `tail.go:8-18` properly sources the vendored-plus-patched code.
- The `NOTICE` update reflects the rename correctly.

## Commit Structure

Clean. Each of the eight commits represents a self-contained concept — absorb upstream, rename/merge with the CLI wrapper, apply the deadlock fix, switch rdd's watcher, resolve lint, add the lifecycle-synchronisation fix, and two round-of-review fixup commits. The two fixup commits are explicitly scoped to review feedback, which the project convention allows for reviewability across rebases. Messages describe motivation (not mechanics) and stay consistent with the diffs.

## Acknowledged Limitations

- **`events.go:86-90, 96-100`** — the explicit `// Do NOT call Cleanup; …` comments on both tail `Stop` defers warn that calling `tail.Cleanup` after `Stop` unregisters the tracker entry in a way that prevents subsequent tailing of the same file. Author has flagged this intentionally; the change does not make it more likely to matter.
- **`inotify_tracker.go:229-249`** — the `run()` function's doc comment acknowledges that the RPC goroutine is still allowed to block on fsnotify; this is deliberate and the drainers prevent starvation. No deeper guarantee is claimed.
- **PR description §"Why not fix it upstream first"** — author acknowledges that upstreaming to `nxadm/tail` and `fsnotify` would be preferable and defers it. Marked out of scope.
- **PR description §"Out of scope"** — converting Lima's `hostagent/events.Watch` to the fixed code is explicitly deferred; rdd bypasses it via the local `pkg/hostagent/events` package.

## Unresolved Feedback

None. The PR body reports "No review comments" and no PR-review-comment URLs were supplied. Inline review findings from rounds 1 and 2 are documented as addressed in the `673e91e` and `6d977b0` commit messages.

## Coverage Summary

| File | Status |
|------|--------|
| `.github/workflows/lint.yaml` | Reviewed, no issues |
| `Makefile` | Trivial (LTAG_EXCLUDES rename) |
| `NOTICE` | Trivial (path rename) |
| `cmd/rdd/limavm.go` | Trivial (`tail.File` → `tail.Stream`) |
| `cmd/rdd/service.go` | Trivial (`tail.File` → `tail.Stream`) |
| `go.mod` | Reviewed, no issues (fsnotify promoted to direct, nxadm/tail now indirect — matches the code) |
| `pkg/controllers/lima/limavm/controllers/hostagent_watcher.go` | Reviewed, no issues (import swap + dropped `propagateStderr=false` arg; matches new `events.Watch` signature) |
| `pkg/hostagent/command.go` | Trivial (one-character doc fix: `Watcher()` → `Watch()`) |
| `pkg/hostagent/events/events.go` | S4 |
| `pkg/hostagent/events/events_test.go` | S5 |
| `pkg/util/nxadmtail/tail.go` (deleted) | Trivial (rename target) |
| `pkg/util/tail/LICENSE` | Trivial (rename) |
| `pkg/util/tail/stream.go` | Reviewed, no issues |
| `pkg/util/tail/stream_test.go` | S8 |
| `pkg/util/tail/tail.go` | Reviewed, no issues |
| `pkg/util/tail/tail_posix.go` | Trivial (package rename) |
| `pkg/util/tail/tail_stress_test.go` | Reviewed, no issues |
| `pkg/util/tail/tail_windows.go` | Trivial (import rename) |
| `pkg/util/tail/watch/filechanges.go` | S7 |
| `pkg/util/tail/watch/inotify.go` | Reviewed, no issues |
| `pkg/util/tail/watch/inotify_tracker.go` | S1, S2, S3 |
| `pkg/util/tail/watch/inotify_tracker_test.go` | S6 |
| `pkg/util/tail/watch/polling.go` | Reviewed, no issues |
| `pkg/util/tail/watch/watch.go` | Reviewed, no issues |
| `pkg/util/tail/winfile/winfile.go` | Reviewed, no issues |
### [Codex] — Pass 1

### Executive Summary

This PR fully vendors the `nxadm/tail` runtime path, fixes the shared watcher deadlock and watcher-cleanup race in the local fork, and switches the Lima hostagent watcher over to that local implementation. No blocking regressions were identified in the changed code, and focused tests for `pkg/hostagent/events` and `pkg/util/tail/...` passed locally.

### Findings

**Critical Issues**

None.

**Important Issues**

None.

**Suggestions**

None.

### Design Observations

**Strengths**

- Keeping a single process-wide `fsnotify.Watcher` while splitting event draining, error draining, and add/remove RPC handling in [pkg/util/tail/watch/inotify_tracker.go](/private/var/folders/s2/9q03h0c11_s4fdw169wqz3qc0000gn/T/review-codex-pass-1-3612148582/pkg/util/tail/watch/inotify_tracker.go:229) fixes the deadlock without regressing Linux inotify-instance usage.
- The added `WaitGroup` synchronization between `Tail.Stop()` and watcher-goroutine teardown in [pkg/util/tail/tail.go](/private/var/folders/s2/9q03h0c11_s4fdw169wqz3qc0000gn/T/review-codex-pass-1-3612148582/pkg/util/tail/tail.go:101) and [pkg/util/tail/watch/watch.go](/private/var/folders/s2/9q03h0c11_s4fdw169wqz3qc0000gn/T/review-codex-pass-1-3612148582/pkg/util/tail/watch/watch.go:18) is the right shape for the second race: it closes the lifetime gap instead of papering over one call site.

### Testing Assessment

1. There is still no controller-level test that drives [pkg/controllers/lima/limavm/controllers/hostagent_watcher.go](/private/var/folders/s2/9q03h0c11_s4fdw169wqz3qc0000gn/T/review-codex-pass-1-3612148582/pkg/controllers/lima/limavm/controllers/hostagent_watcher.go:1) through the new local [pkg/hostagent/events/events.go](/private/var/folders/s2/9q03h0c11_s4fdw169wqz3qc0000gn/T/review-codex-pass-1-3612148582/pkg/hostagent/events/events.go:57) path during an actual hostagent restart/rotation sequence. The unit tests cover the shim and the tail fork separately, but not the combined controller integration.
2. The CLI rename from `tail.File` to `tail.Stream` is covered indirectly by [pkg/util/tail/stream_test.go](/private/var/folders/s2/9q03h0c11_s4fdw169wqz3qc0000gn/T/review-codex-pass-1-3612148582/pkg/util/tail/stream_test.go:20), but there is no command-level test for `rdd service log[s]` or `rdd limavm log[s]`.

### Documentation Assessment

`NOTICE`, `Makefile`, and the package-level docs were updated consistently with the vendoring move. No additional documentation gaps stood out in the changed code.

### Acknowledged Limitations

- **Code comment**: [pkg/controllers/lima/limavm/controllers/hostagent_watcher.go](/private/var/folders/s2/9q03h0c11_s4fdw169wqz3qc0000gn/T/review-codex-pass-1-3612148582/pkg/controllers/lima/limavm/controllers/hostagent_watcher.go:101) notes that `ev.Status.Degraded` is intentionally not handled because Lima does not currently emit degraded events. This PR does not make that limitation more likely to matter.
- **Code comment**: [pkg/util/tail/watch/inotify.go](/private/var/folders/s2/9q03h0c11_s4fdw169wqz3qc0000gn/T/review-codex-pass-1-3612148582/pkg/util/tail/watch/inotify.go:131) and [pkg/util/tail/watch/polling.go](/private/var/folders/s2/9q03h0c11_s4fdw169wqz3qc0000gn/T/review-codex-pass-1-3612148582/pkg/util/tail/watch/polling.go:94) both acknowledge that unexpected `os.Stat` failures inside watcher goroutines are not yet surfaced cleanly to callers. The fork makes those paths patchable in-tree later, but this PR does not change the limitation itself.

### Coverage Summary

- `.github/workflows/lint.yaml` — Reviewed, no issues
- `Makefile` — Trivial
- `NOTICE` — Trivial
- `cmd/rdd/limavm.go` — Reviewed, no issues
- `cmd/rdd/service.go` — Reviewed, no issues
- `go.mod` — Reviewed, no issues
- `pkg/controllers/lima/limavm/controllers/hostagent_watcher.go` — Reviewed, no issues
- `pkg/hostagent/command.go` — Trivial
- `pkg/hostagent/events/events.go` — Reviewed, no issues
- `pkg/hostagent/events/events_test.go` — Reviewed, no issues
- `pkg/util/nxadmtail/tail.go` — Reviewed, no issues
- `pkg/util/tail/LICENSE` — Trivial
- `pkg/util/tail/stream.go` — Reviewed, no issues
- `pkg/util/tail/stream_test.go` — Reviewed, no issues
- `pkg/util/tail/tail.go` — Reviewed, no issues
- `pkg/util/tail/tail_posix.go` — Trivial
- `pkg/util/tail/tail_stress_test.go` — Reviewed, no issues
- `pkg/util/tail/tail_windows.go` — Trivial
- `pkg/util/tail/watch/filechanges.go` — Reviewed, no issues
- `pkg/util/tail/watch/inotify.go` — Reviewed, no issues
- `pkg/util/tail/watch/inotify_tracker.go` — Reviewed, no issues
- `pkg/util/tail/watch/inotify_tracker_test.go` — Reviewed, no issues
- `pkg/util/tail/watch/polling.go` — Reviewed, no issues
- `pkg/util/tail/watch/watch.go` — Reviewed, no issues
- `pkg/util/tail/winfile/winfile.go` — Reviewed, no issues

### [Gemini] — Pass 1

### Executive Summary

This PR successfully fixes a reliable deadlock on Windows by absorbing `nxadm/tail` into the tree and splitting its shared `InotifyTracker` event and error processing into dedicated goroutines. It also resolves a lifecycle race condition during watcher shutdown and correctly hooks the new local package into the hostagent watcher without leaking contexts or goroutines. The only notable concern is that the absorbed upstream code contains residual `panic()` calls on I/O errors, which are dangerous now that the tail runs in-process within the main daemon.

### Findings

#### Critical Issues
None.

#### Important Issues

I1. **Uncaught panics on I/O errors in background goroutines will crash the daemon** — `pkg/util/tail/watch/inotify.go:132` (important, gap)

```go
				fi, err := os.Stat(fw.Filename)
				if err != nil {
					if os.IsNotExist(err) {
						_ = untrack(fw.Filename)
						changes.NotifyDeleted()
						return
					}
					// XXX: report this error back to the user
					panic(fmt.Sprintf("Failed to stat file %v: %v", fw.Filename, err))
				}
```

By absorbing `nxadm/tail`, the project has brought upstream `panic()` calls directly into the `rdd` control plane. This occurs in three places:
1. `pkg/util/tail/watch/inotify.go:132`
2. `pkg/util/tail/watch/polling.go:95`
3. `pkg/util/tail/watch/inotify_tracker.go:253`

Because `events.Watch` runs these watchers in background goroutines, hitting any of these panics (e.g., if a user restricts file permissions on the log file causing `os.Stat` to return access denied, or if `fsnotify.NewWatcher()` fails due to system limits) will crash the entire `rdd` process. 

**Fix**: Replace the panics with graceful degradation. For `os.Stat` failures in the watcher loops, log the error and treat it as a deletion so the tail can attempt to cleanly reopen the file or exit:
```diff
 				fi, err := os.Stat(fw.Filename)
 				if err != nil {
-					if os.IsNotExist(err) {
+					if os.IsNotExist(err) || (runtime.GOOS == "windows" && os.IsPermission(err)) {
 						_ = untrack(fw.Filename)
 						changes.NotifyDeleted()
 						return
 					}
-					// XXX: report this error back to the user
-					panic(fmt.Sprintf("Failed to stat file %v: %v", fw.Filename, err))
+					// Treat stat failures as deletions to force a reopen or clean stop rather than crashing.
+					_ = untrack(fw.Filename)
+					changes.NotifyDeleted()
+					return
 				}
```
For `fsnotify.NewWatcher()`, consider passing the initialization error back through the `t.error` channel during the first `track()` call instead of panicking on startup.

#### Suggestions
None.

### Design Observations

#### Concerns
- **Shared channel multiplexing (pre-existing):** `InotifyTracker` shares a single, unbuffered events channel per filename (`shared.chans[fname]`). If two `Tail` instances are opened for the exact same file path concurrently, they will both try to pull from the same channel, causing each event to be randomly delivered to only one of the tails while the other silently misses it. While `rdd` currently avoids this by tailing mutually exclusive files (`ha.stdout.log` vs `ha.stderr.log`), this architectural flaw limits `pkg/util/tail` as a general-purpose library. The tracker's maps should ideally be keyed by a unique watch instance ID instead of the filename. (future)

#### Strengths
- **Deadlock Resolution:** Extracting the unbuffered `Errors` channel drainer into its own goroutine (`watch/inotify_tracker.go:266`) perfectly addresses the root cause of the Windows deadlock by preventing `sendError` from blocking the `Add`/`Remove` RPC processing thread.
- **Teardown Synchronization:** The addition of `watchers.Wait()` cleanly synchronises the shutdown of the `ChangeEvents` goroutines with `tail.Stop()`, eliminating the lifecycle race condition and preventing leaked inotify watches.
- **Cancellation Model:** The `stream.go` API implementation handles context cancellation flawlessly, utilizing a highly clean Goroutine and channel close pattern that ensures `tailFileSync` and all associated file handles gracefully exit without leaking. 

### Testing Assessment
1. The new `TestInotifyTrackerNoDeadlockOnRepeatedRotation` is an excellent, deterministic stress test that accurately mirrors the concurrency pressure responsible for the Windows failures.
2. `TestSendEventNoPanicOnConcurrentClose` verifies the safety of the channel send/recover logic mathematically by executing enough iterations to reliably catch any `select` non-determinism.

### Coverage Summary

- `.github/workflows/lint.yaml` - Trivial
- `Makefile` - Trivial
- `NOTICE` - Trivial
- `cmd/rdd/limavm.go` - Reviewed, no issues
- `cmd/rdd/service.go` - Reviewed, no issues
- `go.mod` - Trivial
- `pkg/controllers/lima/limavm/controllers/hostagent_watcher.go` - Reviewed, no issues
- `pkg/hostagent/command.go` - Trivial
- `pkg/hostagent/events/events.go` - Reviewed, no issues
- `pkg/hostagent/events/events_test.go` - Reviewed, no issues
- `pkg/util/nxadmtail/tail.go` - Trivial
- `pkg/util/tail/LICENSE` - Trivial
- `pkg/util/tail/stream.go` - Reviewed, no issues
- `pkg/util/tail/stream_test.go` - Trivial
- `pkg/util/tail/tail.go` - Reviewed, no issues
- `pkg/util/tail/tail_posix.go` - Trivial
- `pkg/util/tail/tail_stress_test.go` - Reviewed, no issues
- `pkg/util/tail/tail_windows.go` - Trivial
- `pkg/util/tail/watch/filechanges.go` - Reviewed, no issues
- `pkg/util/tail/watch/inotify.go` - I1
- `pkg/util/tail/watch/inotify_tracker.go` - I1
- `pkg/util/tail/watch/inotify_tracker_test.go` - Reviewed, no issues
- `pkg/util/tail/watch/polling.go` - I1
- `pkg/util/tail/watch/watch.go` - Reviewed, no issues
- `pkg/util/tail/winfile/winfile.go` - Reviewed, no issues

## Resolution

| | |
|---|---|
| **Addressed** | 2026-04-21 |
| **Commit** | `692e4ea` Address review 3 findings for PR 326 |

| # | Finding | Action |
|---|---------|--------|
| 1 | Important #1: Absorbed `watch/` code panics on I/O errors | Fixed |
| 2 | Suggestion #1: Stale `watchFlags` comment on `addWatch` | Fixed |
| 3 | Suggestion #2: EINTR check misses `*os.PathError` wrapping | Fixed |
| 4 | Suggestion #3: Bare `recover()` in `sendEvent` | Skipped |
| 5 | Suggestion #4: `tailLogger.Fatal`/`Panic` demote contracts | Fixed |
| 6 | Suggestion #5: `events.Watch` tail-error propagation untested | Test written |
| 7 | Suggestion #6: `TestSendEventNoPanicOnConcurrentClose` weak | Test written |
| 8 | Suggestion #7: `FileChanges` doc does not mention coalescing | Fixed |
| 9 | Suggestion #8: `Stream` writer-error path untested | Test written |
| 10 | Testing Gap #4: No controller-level integration test for hostagent_watcher | Skipped |
| 11 | Testing Gap #5: No command-level test for `rdd service log[s]` / `rdd limavm log[s]` | Skipped |