diff --git a/architecture/cli-sync.md b/architecture/cli-sync.md index 61e792a..a4368d0 100644 --- a/architecture/cli-sync.md +++ b/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` diff --git a/internal/daemon/daemon.go b/internal/daemon/daemon.go index f17949c..5a055d8 100644 --- a/internal/daemon/daemon.go +++ b/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. diff --git a/internal/webapp/cli_daemon_race_test.go b/internal/webapp/cli_daemon_race_test.go new file mode 100644 index 0000000..bbdb81a --- /dev/null +++ b/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 +}