Deep Review: 20260421-131403-pr-326

cfg := tail.Config{
    Follow:    true,
    ReOpen:    true,
    MustExist: false,
    Logger:    logrus.StandardLogger(),
}
...
if line.Err != nil {
    logrus.WithError(line.Err).Error("hostagent stdout tail error")
    continue
}

runWatcher builds a contextual logger at hostagent_watcher.go:72 (log.FromContext(ctx).WithValues("instance", name, "component", "watcher")), and the embedded service routes controller logs through klog. The local events.Watch drops that context and emits through global logrus instead, so diagnostic lines on the watcher hot path lose their instance/component fields. The PR claims to own this code now (that was the whole point of committing 4), so there is no portability constraint to preserve.

Fix: accept a logr.Logger and route tail.Config.Logger through a small adapter, or set tail.Config.Logger = tail.DiscardingLogger and log errors directly through the passed-in logr.Logger.

-func Watch(ctx context.Context, haStdoutPath, haStderrPath string, begin time.Time, onEvent func(Event) bool) error {
+func Watch(ctx context.Context, logger logr.Logger, haStdoutPath, haStderrPath string, begin time.Time, onEvent func(Event) bool) error {

Gemini graded this important, Codex suggestion. The old path (Lima's events.Watch) also routed through logrus, so the PR does not introduce a regression — but it passed up the opportunity to fix it while the code was in transit.

if err := json.Unmarshal([]byte(line.Text), &ev); err != nil {
    return fmt.Errorf("failed to unmarshal %q as %T: %w", line.Text, ev, err)
}

If Lima's hostagent writes one non-JSON line to ha.stdout.log (a panic trace, a library preamble, a truncated write on a crash), Watch returns. runWatcher logs the error and exits; state.done closes. The reconciler does not restart the watcher, so the in-memory VM phase mirror stops updating until the next reconcile tears the whole instance down. The old Lima path had the same behavior, so this is not a regression introduced here, but moving the implementation in-tree makes it easy to fix now.

Fix: log and continue rather than returning.

-if err := json.Unmarshal([]byte(line.Text), &ev); err != nil {
-    return fmt.Errorf("failed to unmarshal %q as %T: %w", line.Text, ev, err)
-}
+if err := json.Unmarshal([]byte(line.Text), &ev); err != nil {
+    logger.Error(err, "Ignoring unparseable hostagent stdout line", "line", line.Text)
+    continue
+}

if !follow {
    go func() { _ = t.StopAtEOF() }()
}
go func() {
    <-ctx.Done()
    _ = t.Stop()
}()

for line := range t.Lines {
    fmt.Fprintln(writer, line.Text)
}
return nil

The pre-PR tail.File had the identical body; commit 35d6f05 renamed it and moved it to stream.go without changes. Two issues carried over:

1. fmt.Fprintln errors are discarded. In a pipe-close case (rdd svc log | head), the loop keeps consuming and discarding lines until the CLI's context cancels.
2. When follow=false the tomb has already been killed with errStopAtEOF; a subsequent Kill(nil) from the ctx goroutine is a no-op in tomb.v1. For a one-shot rdd svc log this is harmless (the CLI exits shortly after), but the ctx goroutine also leaks past the for ... range t.Lines return because nothing signals it to exit until ctx cancels.

Fix: check the write error, and give the ctx goroutine an exit signal tied to the main function returning:

+    tailDone := make(chan struct{})
+    defer close(tailDone)
     go func() {
-        <-ctx.Done()
-        _ = t.Stop()
+        select {
+        case <-ctx.Done():
+            _ = t.Stop()
+        case <-tailDone:
+        }
     }()

-    for line := range t.Lines {
-        fmt.Fprintln(writer, line.Text)
-    }
-    return nil
+    for line := range t.Lines {
+        if _, err := fmt.Fprintln(writer, line.Text); err != nil {
+            _ = t.Stop()
+            return err
+        }
+    }
+    return nil

Gemini graded this important (regression), Claude suggestion (pre-existing); verified against the base-branch tail.File — the body is byte-for-byte identical except for the nxadmtail package qualifier, so this is pre-existing.

This project is licensed under the Apache License, Version 2.0.
See LICENSE for the full text. Some included files use the MIT License,
which is compatible with the Apache-2.0 License:

- pkg/util/nxadmtail/: a vendored copy of github.com/nxadm/tail.
  See pkg/util/nxadmtail/LICENSE for details.

Commit 35d6f05 renamed the directory and moved LICENSE to pkg/util/tail/LICENSE, but NOTICE was not touched. The directory the file points at no longer exists. pkg/util/tail/ also now contains three Apache-2.0 files (stream.go, stream_test.go, tail_stress_test.go) under what NOTICE describes as an MIT vendor boundary, so a blanket pointer at the directory is wrong even after the path fix.

Fix:

-- pkg/util/nxadmtail/: a vendored copy of github.com/nxadm/tail.
-  See pkg/util/nxadmtail/LICENSE for details.
+- pkg/util/tail/: the tail.go, tail_posix.go, tail_windows.go, watch/,
+  and winfile/ files are a vendored copy of github.com/nxadm/tail.
+  See pkg/util/tail/LICENSE for details.

	return written, err
}

func initHostagentLogrus(stderr io.Writer) {
	logrus.SetOutput(stderr)
	// JSON logs are parsed in pkg/hostagent/events.Watcher()
	logrus.SetFormatter(new(logrus.JSONFormatter))
	// HostAgent logging is one level more verbose than the start command itself
	if logrus.GetLevel() == logrus.DebugLevel {
		logrus.SetLevel(logrus.TraceLevel)
	} else {

The exported symbol in the new pkg/hostagent/events package is Watch, not Watcher. The comment predates the local package but was not updated when commit 92881d9 introduced it with a different name.

Fix: pkg/hostagent/events.Watch().

// ChangeEvents reports on changes to a file, be it modification,
// deletion, renames or truncations. The returned FileChanges
// channels will be closed, thus become unusable, after a deletion
// or truncation event.

Neither inotify.go:115-141 nor polling.go:91-127 closes the Modified/Truncated/Deleted channels; they call NotifyDeleted/NotifyTruncated and return. The goroutine exits and no further notifications arrive, but the channels stay open. Before another consumer depends on the "closed" semantics, either fix the doc or fix the implementations.

Fix: update the comment to describe the actual behavior ("no further notifications arrive after a deletion or truncation event; the goroutine returns").

for {
    select {
    case <-t.Dying():
        return
    default:
    }

    time.Sleep(POLL_DURATION)

time.Sleep cannot be cancelled. tail.Stop now correctly synchronises on watchers.Wait(), so this sleep directly adds up to POLL_DURATION (250 ms today) to every service shutdown or log rotation when the polling backend is in use. Pre-existing upstream behavior, but worth a one-line fix while the code is being touched:

-    time.Sleep(POLL_DURATION)
+    select {
+    case <-t.Dying():
+        return
+    case <-time.After(POLL_DURATION):
+    }

//nolint:revive,staticcheck // Keep the upstream name for API compatibility.
var POLL_DURATION time.Duration

The nolint comment appeals to an external API that no longer exists: this is a private in-tree fork. Commit a2d5c9f already renamed other upstream identifiers (Watch→track, Events→eventsFor); this one was missed.

Fix: const pollDuration = 250 * time.Millisecond, drop the init(), update the two call sites.

defer func() {
    _ = haStdoutTail.Stop()
    // Do NOT call Cleanup; it unregisters the tracker entry in a way
    // that prevents the process from ever tailing the file again.
}()
...
defer func() {
    _ = haStderrTail.Stop()
}()

Both defers are structurally identical. The stdout one carries the upstream warning; the stderr one does not. A later editor patching the "missing Cleanup()" on the stderr half would silently reintroduce the bug the comment warns about. Upstream Lima has the note on both blocks.

Fix: mirror the comment, or hoist both into a local helper so there is one place to read the rule.

cmd := exec.CommandContext(context.Background(), selfExe, "-test.run=^$")

The comment correctly notes that stdin closure reaps the child. But if the test panics between cmd.Start() (line 108) and cmd.Wait() (line 124), the subprocess outlives the test. t.Context() lets testing's cleanup kill the subprocess automatically on a failed/panicked run, which also covers the case where a future refactor reorders cycle teardown.

Fix:

-    cmd := exec.CommandContext(context.Background(), selfExe, "-test.run=^$")
+    cmd := exec.CommandContext(t.Context(), selfExe, "-test.run=^$")

The new local events.Watch is the integration boundary between runWatcher and the fixed tail library, but it has no tests of its own — no coverage for begin-time filtering, the "stop when onEvent returns true" contract, malformed JSON handling (S2), or stdout/stderr teardown ordering. TestInotifyTrackerNoDeadlockOnRepeatedRotation exercises the underlying tail, not this adapter. Any future change to the parsing loop would go in blind.

Fix: add a small test that feeds a temp file through Watch and verifies each of the above contracts.