Files
beardrive/internal/webapp/cli_daemon_race_test.go
T
b927e56fab 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>
2026-07-30 14:31:44 +09:00

105 lines
3.6 KiB
Go

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
}