fix(daemon): let the daemon own its pidfile, so stop can stop it (#91)

* fix(daemon): let the daemon own its pidfile, so stop can stop it

`bdrive stop` could fail with "no such process" and leave sync running.

Liveness became the flock in #88, and Run already announces the child's own
pid only after it holds that lock. But Start still wrote daemon.pid from the
parent, right after fork, with the pid of a child that had not earned
anything yet. Two starts inside that window — `bdrive init` followed by the
login agent's `bdrive resume`, or two resumes close together — race: Running
still reads false, a second child spawns, it loses hold(), and it exits
without ever being the daemon. Its pid is already in the file.

Everything downstream trusts that file. Stop signals the loser and gets
ESRCH, so it reports failure while the winner keeps syncing — the one command
whose job is "stop sending my files" silently does not. status prints the
phantom pid, or "pid 0" when the loser's cleanup removed the file the winner
wrote.

So the parent no longer writes it: the pidfile belongs to whoever holds the
lock. Start now waits for the lock to be taken instead of assuming the spawn
worked, which also means a caller that gets a pid back can trust a daemon
owns it — `bdrive resume` used to print "started (pid N)" for a child that
had already died.

The regression test needs the real binary (Start execs os.Executable), so it
lives with the CLI e2e rather than in internal/daemon, whose tests synthesize
locks. It is deterministic on Linux and roughly one run in five on macOS,
where the window is tighter; `sandbox/run.sh daemon-linux` is the reliable
reproducer.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DgF8JsoeNPVShGYWdooE72

* docs(architecture): the daemon owns its pidfile, Start only waits for the lock

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DgF8JsoeNPVShGYWdooE72

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

architecture/cli-sync.md

@@ -184,7 +184,7 @@ classDiagram
         volumes/id/daemon.lock
         volumes/id/daemon.pid
     }
-    note for DaemonLock "internal/daemon — liveness is the flock, held for the daemon's lifetime; the kernel drops it at death/reboot, so a leftover pid can never read as running (pid is display + signal only)"
+    note for DaemonLock "internal/daemon — liveness is the flock, held for the daemon's lifetime; the kernel drops it at death/reboot, so a leftover pid can never read as running (pid is display + signal only). The daemon writes daemon.pid ITSELF, after taking the lock, and removes it on exit: only the lock holder may name itself there, or Stop signals a pid that lost the start race. Start writes no pid — it waits for the lock, so a returned pid always has a daemon behind it"
 
     Commands --> Autostart : autostart install/uninstall (init runs install automatically)
     Autostart ..> Commands : login runs `bdrive resume`

internal/daemon/daemon.go

@@ -124,13 +124,39 @@ func Start(folder, volDir string, scanInterval, remoteInterval time.Duration) (i
 	if err := cmd.Start(); err != nil {
 		return 0, err
 	}
-	pid := cmd.Process.Pid
-	if err := os.WriteFile(PidPath(volDir), []byte(strconv.Itoa(pid)+"\n"), 0o644); err != nil {
-		return pid, err
+	if err := cmd.Process.Release(); err != nil {
+		return 0, err
+	}
+
+	// The child announces its own pid, and only once it holds the lifetime
+	// lock (see Run). The parent must NOT write PidPath: a child that loses
+	// the lock race exits without ever being the daemon, and its pid written
+	// here would outlive it — leaving Stop signalling a corpse (ESRCH) while
+	// the daemon that won keeps syncing, and status printing a phantom pid.
+	//
+	// So wait for the lock instead of assuming the spawn worked. A caller
+	// that gets a pid back can trust that a daemon owns it. In a race the pid
+	// is the winner's rather than the child just spawned, which is the honest
+	// answer to "which pid is the daemon" — callers that need to distinguish
+	// starting from adopting check Running first (see `bdrive resume`).
+	deadline := time.Now().Add(startTimeout)
+	for {
+		if pid, ok := Running(volDir); ok && pid > 0 {
+			return pid, nil
+		}
+		if time.Now().After(deadline) {
+			return 0, fmt.Errorf("daemon did not come up within %s; see %s",
+				startTimeout, LogPath(volDir))
+		}
+		time.Sleep(20 * time.Millisecond)
 	}
-	return pid, cmd.Process.Release()
 }
 
+// startTimeout bounds how long Start waits for the child to take the lock and
+// write its pid. Generous on purpose: it covers a cold binary on a loaded
+// machine, and the only cost of waiting is a slower `bdrive init`.
+const startTimeout = 10 * time.Second
+
 // Stop terminates the daemon for a mount and waits for it to exit. Exit is
 // observed by the lock being released, not by the pid disappearing: the pid
 // could be recycled while we wait, and the lock cannot.

internal/webapp/cli_daemon_race_test.go

@@ -0,0 +1,104 @@
+package webapp
+
+// Regression: two daemon starts close together must not leave daemon.pid
+// naming a process that is not the one holding the lock.
+//
+// Liveness is the flock (see internal/daemon.LockPath), but Start() writes
+// daemon.pid from the PARENT, with the pid of a child that has not taken the
+// lock yet. The realistic collision is `bdrive init` immediately followed by
+// the login agent's `bdrive resume`: Running() still reads false, so resume
+// starts a second daemon, the loser exits, and its pid is already in the file.
+//
+// The damage is not cosmetic. `bdrive stop` signals the pid from that file, so
+// it fails with ESRCH while the surviving daemon keeps syncing — sync cannot be
+// turned off, and `stop` is consent withdrawal. `bdrive status` also prints the
+// phantom pid.
+//
+// TestCLIOnboardingE2E already asserts `resume` reports "already running 1",
+// but only after several intervening assertions have given the child time to
+// lock. This test deliberately runs resume with no delay, which is the window a
+// real login agent can land in.
+
+import (
+	"os"
+	"path/filepath"
+	"strconv"
+	"strings"
+	"syscall"
+	"testing"
+)
+
+func TestCLIDaemonPidFileNamesTheLockHolder(t *testing.T) {
+	e := newCLIEnv(t)
+	work := t.TempDir()
+
+	if out, err := e.run(work, "init", "--name", "race-e2e", "--yes"); err != nil {
+		t.Fatalf("init: %v\n%s", err, out)
+	}
+
+	// No sleep on purpose: the absence of one IS the test.
+	resumeOut, err := e.run(work, "resume")
+	if err != nil {
+		t.Fatalf("resume: %v\n%s", err, resumeOut)
+	}
+
+	pidPath := findDaemonPidFile(t, filepath.Join(e.home, ".bdrive", "volumes"))
+	pid := readDaemonPid(t, pidPath)
+	t.Cleanup(func() { syscall.Kill(pid, syscall.SIGTERM) })
+
+	// resume must recognise the daemon init just started rather than race it.
+	if !strings.Contains(resumeOut, "already running 1") {
+		t.Errorf("resume did not see the daemon init started, so it started another:\n%s", resumeOut)
+	}
+
+	// Whatever daemon.pid names must actually exist: stop and status both
+	// trust it, and after a reboot it is all that identifies the daemon.
+	if err := syscall.Kill(pid, 0); err != nil {
+		t.Errorf("daemon.pid names pid %d, which is not running (%v) — stop and status both trust this file", pid, err)
+	}
+
+	// The consequence, asserted directly: stop must work and must leave
+	// nothing syncing behind it.
+	out, err := e.run(work, "stop")
+	if err != nil {
+		t.Errorf("stop failed while a daemon was running: %v\n%s", err, out)
+	}
+	if out, err := e.run(work, "status"); err == nil && strings.Contains(out, "daemon:   running") {
+		t.Errorf("a daemon is still running after stop:\n%s", out)
+	}
+}
+
+// findDaemonPidFile returns the single daemon.pid under root, failing if there
+// is not exactly one — more than one would mean the test mounted more than it
+// meant to, and the assertions below would be about the wrong daemon.
+func findDaemonPidFile(t *testing.T, root string) string {
+	t.Helper()
+	var found []string
+	entries, err := os.ReadDir(root)
+	if err != nil {
+		t.Fatalf("no volume dir at %s: %v", root, err)
+	}
+	for _, e := range entries {
+		p := filepath.Join(root, e.Name(), "daemon.pid")
+		if _, err := os.Stat(p); err == nil {
+			found = append(found, p)
+		}
+	}
+	if len(found) != 1 {
+		t.Fatalf("want exactly one daemon.pid under %s, found %d: %v", root, len(found), found)
+	}
+	return found[0]
+}
+
+func readDaemonPid(t *testing.T, path string) int {
+	t.Helper()
+	data, err := os.ReadFile(path)
+	if err != nil {
+		t.Fatalf("read %s: %v", path, err)
+	}
+	pid, err := strconv.Atoi(strings.TrimSpace(string(data)))
+	if err != nil || pid <= 0 {
+		t.Fatalf("%s does not hold a pid: %q", path, data)
+	}
+	return pid
+}