Menubar never blank, Cursor string numbers (#151), Codex reset credits (#152)

cmd/onwatch/daemon_sandbox_helpers_test.go

@@ -0,0 +1,80 @@
+package main
+
+import (
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+	"time"
+)
+
+// daemonSandboxRemoveTimeout bounds how long the daemon-subprocess tests keep
+// retrying to empty a sandbox home before t.TempDir's cleanup runs.
+const daemonSandboxRemoveTimeout = 5 * time.Second
+
+// removeAllWithRetry removes path, retrying until the deadline. Removing a
+// directory while a process is still writing into it fails with "directory is
+// not empty" (ENOTEMPTY on Windows); a short retry window lets the last write
+// land and the removal converge. remove is injectable for tests.
+func removeAllWithRetry(path string, timeout time.Duration, remove func(string) error) error {
+	deadline := time.Now().Add(timeout)
+	delay := 10 * time.Millisecond
+	for {
+		err := remove(path)
+		if err == nil || os.IsNotExist(err) {
+			return nil
+		}
+		if !time.Now().Before(deadline) {
+			return err
+		}
+		time.Sleep(delay)
+		if delay < 200*time.Millisecond {
+			delay *= 2
+		}
+	}
+}
+
+// sandboxHomeUnderTemp returns the child home named by env (USERPROFILE on
+// Windows, HOME elsewhere) when it lies inside the OS temp directory, so the
+// caller may remove it before t.TempDir cleanup. The last duplicate key wins,
+// matching how exec.Cmd resolves duplicate environment entries (callers append
+// the sandbox to os.Environ). A real home is never returned.
+func sandboxHomeUnderTemp(env []string) (string, bool) {
+	var home string
+	for _, kv := range env {
+		if value, ok := strings.CutPrefix(kv, "USERPROFILE="); ok {
+			home = value
+		}
+	}
+	if home == "" {
+		for _, kv := range env {
+			if value, ok := strings.CutPrefix(kv, "HOME="); ok {
+				home = value
+			}
+		}
+	}
+	if home == "" {
+		return "", false
+	}
+	home = filepath.Clean(home)
+	temp := filepath.Clean(os.TempDir())
+	rel, err := filepath.Rel(temp, home)
+	if err != nil || rel == "." || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) {
+		return "", false
+	}
+	return home, true
+}
+
+// cleanupDaemonSandbox empties the child's sandbox home after a daemon
+// subprocess was stopped, so t.TempDir's cleanup does not race a last write
+// from the dying child. On Windows testing's TempDir cleanup fails with
+// "unlinkat ...: The directory is not empty" when an entry appears between its
+// scan and rmdir; Go only retries sharing violations there, not ENOTEMPTY.
+func cleanupDaemonSandbox(t *testing.T, env []string) {
+	t.Helper()
+	home, ok := sandboxHomeUnderTemp(env)
+	if !ok {
+		return
+	}
+	_ = removeAllWithRetry(home, daemonSandboxRemoveTimeout, os.RemoveAll)
+}

cmd/onwatch/daemon_sandbox_test.go

@@ -0,0 +1,100 @@
+package main
+
+import (
+	"errors"
+	"os"
+	"path/filepath"
+	"testing"
+	"time"
+)
+
+// daemon subprocess tests sandbox the child's home inside t.TempDir. On
+// Windows the child is terminated abruptly (os.Interrupt cannot be delivered),
+// and testing's own TempDir cleanup reports "directory is not empty" when a
+// file lands between its directory scan and rmdir. The helper removes that
+// home itself, with retries, before the TempDir cleanup runs.
+
+func TestRemoveAllWithRetryRetriesTransientFailure(t *testing.T) {
+	calls := 0
+	errNotSettled := errors.New("unlinkat: The directory is not empty")
+	err := removeAllWithRetry(t.TempDir(), 2*time.Second, func(string) error {
+		calls++
+		if calls < 3 {
+			return errNotSettled
+		}
+		return nil
+	})
+	if err != nil {
+		t.Fatalf("retry did not converge: %v", err)
+	}
+	if calls != 3 {
+		t.Fatalf("remove called %d times, want 3", calls)
+	}
+}
+
+func TestRemoveAllWithRetryStopsAtDeadline(t *testing.T) {
+	calls := 0
+	err := removeAllWithRetry(t.TempDir(), 60*time.Millisecond, func(string) error {
+		calls++
+		return errors.New("unlinkat: The directory is not empty")
+	})
+	if err == nil {
+		t.Fatal("expected the last removal error")
+	}
+	if calls < 2 {
+		t.Fatalf("remove called %d times, want at least 2 before giving up", calls)
+	}
+}
+
+func TestSandboxHomeUnderTempOnlyTrustsTempPaths(t *testing.T) {
+	tmp := t.TempDir()
+
+	home, ok := sandboxHomeUnderTemp([]string{"HOME=/home/someone", "USERPROFILE=" + tmp})
+	if !ok || home != filepath.Clean(tmp) {
+		t.Fatalf("sandboxHomeUnderTemp = %q, %v; want %q, true", home, ok, filepath.Clean(tmp))
+	}
+
+	// The last duplicate key wins: callers append the sandbox after os.Environ.
+	home, ok = sandboxHomeUnderTemp([]string{"USERPROFILE=" + filepath.Join(tmp, "a"), "USERPROFILE=" + filepath.Join(tmp, "b")})
+	if !ok || home != filepath.Join(filepath.Clean(tmp), "b") {
+		t.Fatalf("duplicate key: sandboxHomeUnderTemp = %q, %v", home, ok)
+	}
+
+	// A real home must never be offered for removal.
+	if _, ok := sandboxHomeUnderTemp([]string{"HOME=/Users/someone", "USERPROFILE=/Users/someone"}); ok {
+		t.Fatal("a home outside the temp directory must not be removed")
+	}
+	if _, ok := sandboxHomeUnderTemp(nil); ok {
+		t.Fatal("no home must yield no cleanup path")
+	}
+}
+
+func TestCleanupDaemonSandboxRemovesTempHome(t *testing.T) {
+	tmp := t.TempDir()
+	home := filepath.Join(tmp, "home")
+	if err := os.MkdirAll(filepath.Join(home, "onwatch"), 0o755); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.WriteFile(filepath.Join(home, "onwatch.db"), []byte("x"), 0o600); err != nil {
+		t.Fatal(err)
+	}
+
+	cleanupDaemonSandbox(t, []string{"HOME=/not/temp", "USERPROFILE=" + home})
+	if _, err := os.Stat(home); !os.IsNotExist(err) {
+		t.Fatalf("sandbox home still exists: %v", err)
+	}
+	// t.TempDir's own cleanup now finds an empty parent and succeeds.
+}
+
+func TestCleanupDaemonSandboxLeavesRealHome(t *testing.T) {
+	dir := t.TempDir()
+	outside := filepath.Join(dir, "not-removed")
+	if err := os.MkdirAll(outside, 0o755); err != nil {
+		t.Fatal(err)
+	}
+	// A path outside os.TempDir must be ignored.
+	cleanupDaemonSandbox(t, []string{"HOME=" + filepath.Join(string(filepath.Separator), "home", "someone")})
+	if _, err := os.Stat(outside); err != nil {
+		t.Fatalf("unrelated dir gone: %v", err)
+	}
+}

cmd/onwatch/main.go

@@ -265,15 +265,43 @@ func findOnwatchOnPort(port int) []int {
 	return pids
 }
 
+// executableBaseName returns this binary's normalized base name. It is a
+// variable so tests can simulate a renamed build.
+var executableBaseName = func() string {
+	exe, err := os.Executable()
+	if err != nil {
+		return ""
+	}
+	return normalizeExecutableName(filepath.Base(exe))
+}
+
+// normalizeExecutableName lowercases a binary base name and drops the .exe
+// suffix on Windows, so comparisons are case-insensitive and extension-free.
+func normalizeExecutableName(name string) string {
+	name = strings.ToLower(strings.TrimSpace(name))
+	if runtime.GOOS == "windows" {
+		name = strings.TrimSuffix(name, ".exe")
+	}
+	return name
+}
+
 // isOnwatchProcess checks if a PID belongs to an onwatch (or legacy syntrack)
 // binary. processCommandName is per-platform: ps on Unix, the process image
 // path on Windows, which has no ps.
 func isOnwatchProcess(pid int) bool {
 	if pid <= 0 {
 		return false
 	}
-	cmd := strings.ToLower(processCommandName(pid))
-	return strings.Contains(cmd, "onwatch") || strings.Contains(cmd, "syntrack")
+	name := normalizeExecutableName(processCommandName(pid))
+	if name == "" {
+		return false
+	}
+	// A renamed build (for example a user-built binary) must still be
+	// recognised, or stop and the menubar sweep never touch it.
+	if own := executableBaseName(); own != "" && name == own {
+		return true
+	}
+	return strings.Contains(name, "onwatch") || strings.Contains(name, "syntrack")
 }
 
 func ensurePIDDir() error {
@@ -1881,7 +1909,14 @@ func run() error {
 		})
 	}
 
+	// One in-memory registry of last-poll outcome classes (network, auth,
+	// rate_limit, ...) so a stale menubar card can say why. Agents publish to
+	// the process-wide instance; the handler reads the same one.
+	pollStatus := agent.NewPollStatusRegistry()
+	agent.SetPollStatusRegistry(pollStatus)
+
 	handler := web.NewHandler(db, tr, logger, nil, cfg, zaiTr)
+	handler.SetPollStatusRegistry(pollStatus)
 	handler.SetVersion(version)
 	handler.SetNotifier(notifier)
 	if anthropicTr != nil {

cmd/onwatch/menubar_lock.go

@@ -0,0 +1,153 @@
+package main
+
+// Single-instance guard for the menubar companion process.
+//
+// Two companions can otherwise run at once. A daemon restart launches a new
+// companion before the old one notices its daemon is gone (the watcher grants a
+// grace period), and the pid file is then overwritten by the new process, so the
+// old one is orphaned with a second tray icon. Every companion takes an
+// exclusive OS lock on menubar.lock for its whole lifetime; a second one fails
+// the lock and exits before creating a tray UI. The kernel releases the lock
+// when the process dies, so a crash never leaves a stale lock.
+//
+// The daemon also sweeps any previously started companion before launching a
+// new one (terminateCompanionSweep). Together these make the duplicate tray a
+// bug of the past rather than something the stale indicator has to mask.
+
+import (
+	"errors"
+	"fmt"
+	"io"
+	"os"
+	"path/filepath"
+	"strconv"
+	"strings"
+	"time"
+)
+
+// menubarLockName is the lock file, kept beside the pid file.
+const menubarLockName = "menubar.lock"
+
+// menubarLockOffset is where the locked byte range starts. The pid is written
+// at offset 0 so the file stays readable for diagnostics; Windows byte-range
+// locks are mandatory, so locking that region would make it unreadable. Unix
+// flock locks the whole file regardless and ignores the offset.
+const menubarLockOffset = 1 << 30
+
+// menubarLockLength is the size of the locked region.
+const menubarLockLength = 1
+
+// errMenubarCompanionRunning is returned when another companion holds the lock.
+var errMenubarCompanionRunning = errors.New("another menubar companion is already running")
+
+// menubarLockWaitTimeout is how long a replacement companion waits for the
+// previous one to release the lock. A companion can be inside a slow refresh
+// (two 15s snapshot attempts) when its daemon asks it to quit.
+const menubarLockWaitTimeout = 10 * time.Second
+
+// menubarLockRetryInterval is the retry cadence inside that window.
+const menubarLockRetryInterval = 250 * time.Millisecond
+
+// menubarLock is a held single-instance lock. Close releases it.
+type menubarLock struct {
+	file *os.File
+}
+
+func menubarLockPath(testMode bool) string {
+	name := menubarLockName
+	if testMode {
+		name = "menubar-test.lock"
+	}
+	return filepath.Join(pidDir, name)
+}
+
+// acquireMenubarLock takes the process-wide lock once. It returns
+// errMenubarCompanionRunning when another companion already holds it.
+func acquireMenubarLock(testMode bool) (*menubarLock, error) {
+	return acquireMenubarLockWait(testMode, 0, 0)
+}
+
+// acquireMenubarLockWait retries the non-blocking lock every retryInterval
+// until it succeeds or the timeout expires. A slow-exiting predecessor is the
+// normal case after a daemon restart, which is why the companion waits instead
+// of exiting immediately.
+func acquireMenubarLockWait(testMode bool, timeout, retryInterval time.Duration) (*menubarLock, error) {
+	path := menubarLockPath(testMode)
+	if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil {
+		return nil, fmt.Errorf("create menubar lock dir: %w", err)
+	}
+	if retryInterval <= 0 {
+		retryInterval = menubarLockRetryInterval
+	}
+	deadline := time.Now().Add(timeout)
+	for {
+		file, err := os.OpenFile(path, os.O_CREATE|os.O_RDWR, 0o644)
+		if err != nil {
+			return nil, fmt.Errorf("open menubar lock: %w", err)
+		}
+		ok, err := lockFileExclusive(file)
+		if err != nil {
+			_ = file.Close()
+			return nil, err
+		}
+		if ok {
+			_ = file.Truncate(0)
+			_, _ = file.Seek(0, 0)
+			_, _ = fmt.Fprintf(file, "%d\n", os.Getpid())
+			return &menubarLock{file: file}, nil
+		}
+		_ = file.Close()
+		if timeout <= 0 || !time.Now().Before(deadline) {
+			return nil, errMenubarCompanionRunning
+		}
+		time.Sleep(retryInterval)
+	}
+}
+
+// Close releases the lock. Safe to call more than once.
+func (l *menubarLock) Close() error {
+	if l == nil || l.file == nil {
+		return nil
+	}
+	unlockFile(l.file)
+	err := l.file.Close()
+	l.file = nil
+	return err
+}
+
+// menubarLockOwnerProbe reports the pid recorded inside the lock file and
+// whether the lock is currently held. It never takes the lock: it opens the
+// lock file read-only (so the holder is unaffected) and tries a non-blocking
+// lock on that second handle. blocked means another process holds it; owner is
+// that process's pid as written at acquisition (0 when unknown).
+//
+// The pid inside the lock is written by the holder and only trusted while the
+// lock is held, so it distinguishes the companion from an unrelated process
+// that happens to reuse the pid in the runtime pid file.
+func menubarLockOwnerProbe(testMode bool) (owner int, blocked bool) {
+	path := menubarLockPath(testMode)
+	file, err := os.OpenFile(path, os.O_RDWR, 0o644)
+	if err != nil {
+		return 0, false
+	}
+	defer file.Close()
+
+	if ok, lockErr := lockFileExclusive(file); lockErr != nil {
+		return 0, false
+	} else if ok {
+		unlockFile(file)
+		return 0, false
+	}
+
+	// Held by someone: read the pid the holder wrote at offset 0. The read is
+	// done on the same handle so Windows' mandatory lock semantics cannot get
+	// in the way (the locked range starts past the text).
+	if _, err := file.Seek(0, io.SeekStart); err == nil {
+		if data, readErr := io.ReadAll(io.LimitReader(file, 32)); readErr == nil {
+			if pid, convErr := strconv.Atoi(strings.TrimSpace(string(data))); convErr == nil && pid > 0 {
+				owner = pid
+			}
+		}
+	}
+	return owner, true
+}