Implement stricter daemon lock file validation (bd-161)
- Add JSON format to daemon.lock with database path, version, PID, and timestamp - Validate database path on client connection (fail if mismatch) - Backward compatible with old plain-PID lock files - Add comprehensive tests for JSON format and validation - Update all lock acquisition callsites to pass database path Amp-Thread-ID: https://ampcode.com/threads/T-137e6a9c-b690-4ade-9bec-13fcd7d0e4ed Co-authored-by: Amp <amp@ampcode.com>
This commit is contained in:
@@ -829,9 +829,9 @@ func setupDaemonLogger(logPath string) (*lumberjack.Logger, daemonLogger) {
|
|||||||
return logF, logger
|
return logF, logger
|
||||||
}
|
}
|
||||||
|
|
||||||
func setupDaemonLock(pidFile string, global bool, log daemonLogger) (io.Closer, error) {
|
func setupDaemonLock(pidFile string, dbPath string, log daemonLogger) (io.Closer, error) {
|
||||||
beadsDir := filepath.Dir(pidFile)
|
beadsDir := filepath.Dir(pidFile)
|
||||||
lock, err := acquireDaemonLock(beadsDir, global)
|
lock, err := acquireDaemonLock(beadsDir, dbPath)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
if err == ErrDaemonLocked {
|
if err == ErrDaemonLocked {
|
||||||
log.log("Daemon already running (lock held), exiting")
|
log.log("Daemon already running (lock held), exiting")
|
||||||
@@ -1044,7 +1044,22 @@ func runDaemonLoop(interval time.Duration, autoCommit, autoPush bool, logPath, p
|
|||||||
logF, log := setupDaemonLogger(logPath)
|
logF, log := setupDaemonLogger(logPath)
|
||||||
defer func() { _ = logF.Close() }()
|
defer func() { _ = logF.Close() }()
|
||||||
|
|
||||||
lock, err := setupDaemonLock(pidFile, global, log)
|
// Determine database path first (needed for lock file metadata)
|
||||||
|
daemonDBPath := ""
|
||||||
|
if !global {
|
||||||
|
daemonDBPath = dbPath
|
||||||
|
if daemonDBPath == "" {
|
||||||
|
if foundDB := beads.FindDatabasePath(); foundDB != "" {
|
||||||
|
daemonDBPath = foundDB
|
||||||
|
} else {
|
||||||
|
log.log("Error: no beads database found")
|
||||||
|
log.log("Hint: run 'bd init' to create a database or set BEADS_DB environment variable")
|
||||||
|
os.Exit(1)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
lock, err := setupDaemonLock(pidFile, daemonDBPath, log)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
os.Exit(1)
|
os.Exit(1)
|
||||||
}
|
}
|
||||||
@@ -1058,17 +1073,6 @@ func runDaemonLoop(interval time.Duration, autoCommit, autoPush bool, logPath, p
|
|||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
|
||||||
daemonDBPath := dbPath
|
|
||||||
if daemonDBPath == "" {
|
|
||||||
if foundDB := beads.FindDatabasePath(); foundDB != "" {
|
|
||||||
daemonDBPath = foundDB
|
|
||||||
} else {
|
|
||||||
log.log("Error: no beads database found")
|
|
||||||
log.log("Hint: run 'bd init' to create a database or set BEADS_DB environment variable")
|
|
||||||
os.Exit(1)
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
// Check for multiple .db files (ambiguity error)
|
// Check for multiple .db files (ambiguity error)
|
||||||
beadsDir := filepath.Dir(daemonDBPath)
|
beadsDir := filepath.Dir(daemonDBPath)
|
||||||
matches, err := filepath.Glob(filepath.Join(beadsDir, "*.db"))
|
matches, err := filepath.Glob(filepath.Join(beadsDir, "*.db"))
|
||||||
|
|||||||
@@ -1,16 +1,26 @@
|
|||||||
package main
|
package main
|
||||||
|
|
||||||
import (
|
import (
|
||||||
|
"encoding/json"
|
||||||
"errors"
|
"errors"
|
||||||
"fmt"
|
"fmt"
|
||||||
"os"
|
"os"
|
||||||
"path/filepath"
|
"path/filepath"
|
||||||
"strconv"
|
"strconv"
|
||||||
"strings"
|
"strings"
|
||||||
|
"time"
|
||||||
)
|
)
|
||||||
|
|
||||||
var ErrDaemonLocked = errors.New("daemon lock already held by another process")
|
var ErrDaemonLocked = errors.New("daemon lock already held by another process")
|
||||||
|
|
||||||
|
// DaemonLockInfo represents the metadata stored in the daemon.lock file
|
||||||
|
type DaemonLockInfo struct {
|
||||||
|
PID int `json:"pid"`
|
||||||
|
Database string `json:"database"`
|
||||||
|
Version string `json:"version"`
|
||||||
|
StartedAt time.Time `json:"started_at"`
|
||||||
|
}
|
||||||
|
|
||||||
// DaemonLock represents a held lock on the daemon.lock file
|
// DaemonLock represents a held lock on the daemon.lock file
|
||||||
type DaemonLock struct {
|
type DaemonLock struct {
|
||||||
file *os.File
|
file *os.File
|
||||||
@@ -30,7 +40,8 @@ func (l *DaemonLock) Close() error {
|
|||||||
|
|
||||||
// acquireDaemonLock attempts to acquire an exclusive lock on daemon.lock
|
// acquireDaemonLock attempts to acquire an exclusive lock on daemon.lock
|
||||||
// Returns ErrDaemonLocked if another daemon is already running
|
// Returns ErrDaemonLocked if another daemon is already running
|
||||||
func acquireDaemonLock(beadsDir string, _ bool) (*DaemonLock, error) {
|
// dbPath is the full path to the database file (e.g., /path/to/.beads/beads.db)
|
||||||
|
func acquireDaemonLock(beadsDir string, dbPath string) (*DaemonLock, error) {
|
||||||
lockPath := filepath.Join(beadsDir, "daemon.lock")
|
lockPath := filepath.Join(beadsDir, "daemon.lock")
|
||||||
|
|
||||||
// Open or create the lock file
|
// Open or create the lock file
|
||||||
@@ -48,10 +59,19 @@ func acquireDaemonLock(beadsDir string, _ bool) (*DaemonLock, error) {
|
|||||||
return nil, fmt.Errorf("cannot lock file: %w", err)
|
return nil, fmt.Errorf("cannot lock file: %w", err)
|
||||||
}
|
}
|
||||||
|
|
||||||
// Write our PID to the lock file for debugging (optional)
|
// Write JSON metadata to the lock file
|
||||||
|
lockInfo := DaemonLockInfo{
|
||||||
|
PID: os.Getpid(),
|
||||||
|
Database: dbPath,
|
||||||
|
Version: Version,
|
||||||
|
StartedAt: time.Now().UTC(),
|
||||||
|
}
|
||||||
|
|
||||||
_ = f.Truncate(0)
|
_ = f.Truncate(0)
|
||||||
_, _ = f.Seek(0, 0)
|
_, _ = f.Seek(0, 0)
|
||||||
_, _ = fmt.Fprintf(f, "%d\n", os.Getpid())
|
encoder := json.NewEncoder(f)
|
||||||
|
encoder.SetIndent("", " ")
|
||||||
|
_ = encoder.Encode(lockInfo)
|
||||||
_ = f.Sync()
|
_ = f.Sync()
|
||||||
|
|
||||||
// Also write PID file for Windows compatibility (can't read locked files on Windows)
|
// Also write PID file for Windows compatibility (can't read locked files on Windows)
|
||||||
@@ -80,16 +100,23 @@ func tryDaemonLock(beadsDir string) (running bool, pid int) {
|
|||||||
if err := flockExclusive(f); err != nil {
|
if err := flockExclusive(f); err != nil {
|
||||||
if err == ErrDaemonLocked {
|
if err == ErrDaemonLocked {
|
||||||
// Lock is held - daemon is running
|
// Lock is held - daemon is running
|
||||||
// Try to read PID for display (best effort)
|
// Try to read PID from JSON format (best effort)
|
||||||
_, _ = f.Seek(0, 0) // Seek to beginning before reading
|
_, _ = f.Seek(0, 0)
|
||||||
data := make([]byte, 32)
|
var lockInfo DaemonLockInfo
|
||||||
n, _ := f.Read(data)
|
if err := json.NewDecoder(f).Decode(&lockInfo); err == nil {
|
||||||
if n > 0 {
|
pid = lockInfo.PID
|
||||||
_, _ = fmt.Sscanf(string(data[:n]), "%d", &pid)
|
} else {
|
||||||
}
|
// Fallback: try reading as plain integer (old format)
|
||||||
// Fallback to PID file if we couldn't read PID from lock file
|
_, _ = f.Seek(0, 0)
|
||||||
if pid == 0 {
|
data := make([]byte, 32)
|
||||||
_, pid = checkPIDFile(beadsDir)
|
n, _ := f.Read(data)
|
||||||
|
if n > 0 {
|
||||||
|
_, _ = fmt.Sscanf(string(data[:n]), "%d", &pid)
|
||||||
|
}
|
||||||
|
// Fallback to PID file if we couldn't read PID from lock file
|
||||||
|
if pid == 0 {
|
||||||
|
_, pid = checkPIDFile(beadsDir)
|
||||||
|
}
|
||||||
}
|
}
|
||||||
return true, pid
|
return true, pid
|
||||||
}
|
}
|
||||||
@@ -102,6 +129,55 @@ func tryDaemonLock(beadsDir string) (running bool, pid int) {
|
|||||||
return false, 0
|
return false, 0
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// readDaemonLockInfo reads and parses the daemon lock file
|
||||||
|
// Returns lock info if available, or error if file doesn't exist or can't be parsed
|
||||||
|
func readDaemonLockInfo(beadsDir string) (*DaemonLockInfo, error) {
|
||||||
|
lockPath := filepath.Join(beadsDir, "daemon.lock")
|
||||||
|
|
||||||
|
data, err := os.ReadFile(lockPath)
|
||||||
|
if err != nil {
|
||||||
|
return nil, err
|
||||||
|
}
|
||||||
|
|
||||||
|
var lockInfo DaemonLockInfo
|
||||||
|
if err := json.Unmarshal(data, &lockInfo); err != nil {
|
||||||
|
// Try parsing as old format (plain PID)
|
||||||
|
var pid int
|
||||||
|
if _, err := fmt.Sscanf(string(data), "%d", &pid); err == nil {
|
||||||
|
return &DaemonLockInfo{PID: pid}, nil
|
||||||
|
}
|
||||||
|
return nil, fmt.Errorf("cannot parse lock file: %w", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
return &lockInfo, nil
|
||||||
|
}
|
||||||
|
|
||||||
|
// validateDaemonLock validates that the running daemon matches expected parameters
|
||||||
|
// Returns error if validation fails (mismatch detected)
|
||||||
|
func validateDaemonLock(beadsDir string, expectedDB string) error {
|
||||||
|
lockInfo, err := readDaemonLockInfo(beadsDir)
|
||||||
|
if err != nil {
|
||||||
|
// No lock file or can't read - not an error for validation
|
||||||
|
return nil
|
||||||
|
}
|
||||||
|
|
||||||
|
// Validate database path if specified in lock
|
||||||
|
if lockInfo.Database != "" && expectedDB != "" {
|
||||||
|
if lockInfo.Database != expectedDB {
|
||||||
|
return fmt.Errorf("daemon database mismatch: daemon uses %s but expected %s", lockInfo.Database, expectedDB)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// Version mismatch is a warning, not a hard error (handled elsewhere)
|
||||||
|
// But we return the info for caller to decide
|
||||||
|
if lockInfo.Version != "" && lockInfo.Version != Version {
|
||||||
|
// Not a hard error - version compatibility check happens via RPC
|
||||||
|
// This is just informational
|
||||||
|
}
|
||||||
|
|
||||||
|
return nil
|
||||||
|
}
|
||||||
|
|
||||||
// checkPIDFile checks if a daemon is running by reading the PID file.
|
// checkPIDFile checks if a daemon is running by reading the PID file.
|
||||||
// This is used for backward compatibility with pre-lock daemons.
|
// This is used for backward compatibility with pre-lock daemons.
|
||||||
func checkPIDFile(beadsDir string) (running bool, pid int) {
|
func checkPIDFile(beadsDir string) (running bool, pid int) {
|
||||||
|
|||||||
@@ -17,15 +17,17 @@ func TestDaemonLockPreventsMultipleInstances(t *testing.T) {
|
|||||||
t.Fatal(err)
|
t.Fatal(err)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
dbPath := filepath.Join(beadsDir, "beads.db")
|
||||||
|
|
||||||
// Acquire lock
|
// Acquire lock
|
||||||
lock1, err := acquireDaemonLock(beadsDir, false)
|
lock1, err := acquireDaemonLock(beadsDir, dbPath)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
t.Fatalf("Failed to acquire first lock: %v", err)
|
t.Fatalf("Failed to acquire first lock: %v", err)
|
||||||
}
|
}
|
||||||
defer lock1.Close()
|
defer lock1.Close()
|
||||||
|
|
||||||
// Try to acquire lock again - should fail
|
// Try to acquire lock again - should fail
|
||||||
lock2, err := acquireDaemonLock(beadsDir, false)
|
lock2, err := acquireDaemonLock(beadsDir, dbPath)
|
||||||
if err != ErrDaemonLocked {
|
if err != ErrDaemonLocked {
|
||||||
if lock2 != nil {
|
if lock2 != nil {
|
||||||
lock2.Close()
|
lock2.Close()
|
||||||
@@ -37,7 +39,7 @@ func TestDaemonLockPreventsMultipleInstances(t *testing.T) {
|
|||||||
lock1.Close()
|
lock1.Close()
|
||||||
|
|
||||||
// Now should be able to acquire lock
|
// Now should be able to acquire lock
|
||||||
lock3, err := acquireDaemonLock(beadsDir, false)
|
lock3, err := acquireDaemonLock(beadsDir, dbPath)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
t.Fatalf("Failed to acquire lock after release: %v", err)
|
t.Fatalf("Failed to acquire lock after release: %v", err)
|
||||||
}
|
}
|
||||||
@@ -51,6 +53,8 @@ func TestTryDaemonLockDetectsRunning(t *testing.T) {
|
|||||||
t.Fatal(err)
|
t.Fatal(err)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
dbPath := filepath.Join(beadsDir, "beads.db")
|
||||||
|
|
||||||
// Initially no daemon running
|
// Initially no daemon running
|
||||||
running, _ := tryDaemonLock(beadsDir)
|
running, _ := tryDaemonLock(beadsDir)
|
||||||
if running {
|
if running {
|
||||||
@@ -58,7 +62,7 @@ func TestTryDaemonLockDetectsRunning(t *testing.T) {
|
|||||||
}
|
}
|
||||||
|
|
||||||
// Acquire lock
|
// Acquire lock
|
||||||
lock, err := acquireDaemonLock(beadsDir, false)
|
lock, err := acquireDaemonLock(beadsDir, dbPath)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
t.Fatalf("Failed to acquire lock: %v", err)
|
t.Fatalf("Failed to acquire lock: %v", err)
|
||||||
}
|
}
|
||||||
@@ -107,6 +111,78 @@ func TestBackwardCompatibilityWithOldDaemon(t *testing.T) {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func TestDaemonLockJSONFormat(t *testing.T) {
|
||||||
|
tmpDir := t.TempDir()
|
||||||
|
beadsDir := filepath.Join(tmpDir, ".beads")
|
||||||
|
if err := os.MkdirAll(beadsDir, 0700); err != nil {
|
||||||
|
t.Fatal(err)
|
||||||
|
}
|
||||||
|
|
||||||
|
dbPath := filepath.Join(beadsDir, "beads.db")
|
||||||
|
|
||||||
|
// Acquire lock
|
||||||
|
lock, err := acquireDaemonLock(beadsDir, dbPath)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("Failed to acquire lock: %v", err)
|
||||||
|
}
|
||||||
|
defer lock.Close()
|
||||||
|
|
||||||
|
// Read the lock file and verify JSON format
|
||||||
|
lockInfo, err := readDaemonLockInfo(beadsDir)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("Failed to read lock info: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
if lockInfo.PID != os.Getpid() {
|
||||||
|
t.Errorf("Expected PID %d, got %d", os.Getpid(), lockInfo.PID)
|
||||||
|
}
|
||||||
|
|
||||||
|
if lockInfo.Database != dbPath {
|
||||||
|
t.Errorf("Expected database %s, got %s", dbPath, lockInfo.Database)
|
||||||
|
}
|
||||||
|
|
||||||
|
if lockInfo.Version != Version {
|
||||||
|
t.Errorf("Expected version %s, got %s", Version, lockInfo.Version)
|
||||||
|
}
|
||||||
|
|
||||||
|
if lockInfo.StartedAt.IsZero() {
|
||||||
|
t.Error("Expected non-zero StartedAt timestamp")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestValidateDaemonLock(t *testing.T) {
|
||||||
|
tmpDir := t.TempDir()
|
||||||
|
beadsDir := filepath.Join(tmpDir, ".beads")
|
||||||
|
if err := os.MkdirAll(beadsDir, 0700); err != nil {
|
||||||
|
t.Fatal(err)
|
||||||
|
}
|
||||||
|
|
||||||
|
dbPath := filepath.Join(beadsDir, "beads.db")
|
||||||
|
|
||||||
|
// No lock file - validation should pass
|
||||||
|
if err := validateDaemonLock(beadsDir, dbPath); err != nil {
|
||||||
|
t.Errorf("Expected no error when no lock file exists, got: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
// Acquire lock with correct database
|
||||||
|
lock, err := acquireDaemonLock(beadsDir, dbPath)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("Failed to acquire lock: %v", err)
|
||||||
|
}
|
||||||
|
defer lock.Close()
|
||||||
|
|
||||||
|
// Validation should pass with matching database
|
||||||
|
if err := validateDaemonLock(beadsDir, dbPath); err != nil {
|
||||||
|
t.Errorf("Expected no error with matching database, got: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
// Validation should fail with different database
|
||||||
|
wrongDB := filepath.Join(beadsDir, "wrong.db")
|
||||||
|
if err := validateDaemonLock(beadsDir, wrongDB); err == nil {
|
||||||
|
t.Error("Expected error with mismatched database")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
func TestMultipleDaemonProcessesRace(t *testing.T) {
|
func TestMultipleDaemonProcessesRace(t *testing.T) {
|
||||||
if testing.Short() {
|
if testing.Short() {
|
||||||
t.Skip("Skipping race condition test in short mode")
|
t.Skip("Skipping race condition test in short mode")
|
||||||
|
|||||||
@@ -132,7 +132,8 @@ func TestIsDaemonRunning_CurrentProcess(t *testing.T) {
|
|||||||
|
|
||||||
// Acquire the daemon lock to simulate a running daemon
|
// Acquire the daemon lock to simulate a running daemon
|
||||||
beadsDir := filepath.Dir(pidFile)
|
beadsDir := filepath.Dir(pidFile)
|
||||||
lock, err := acquireDaemonLock(beadsDir, false)
|
dbPath := filepath.Join(beadsDir, "beads.db")
|
||||||
|
lock, err := acquireDaemonLock(beadsDir, dbPath)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
t.Fatalf("Failed to acquire daemon lock: %v", err)
|
t.Fatalf("Failed to acquire daemon lock: %v", err)
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -269,18 +269,30 @@ var rootCmd = &cobra.Command{
|
|||||||
daemonStatus.Detail = fmt.Sprintf("version mismatch (daemon: %s, client: %s) and restart failed",
|
daemonStatus.Detail = fmt.Sprintf("version mismatch (daemon: %s, client: %s) and restart failed",
|
||||||
health.Version, Version)
|
health.Version, Version)
|
||||||
} else {
|
} else {
|
||||||
// Daemon is healthy and compatible - use it
|
// Daemon is healthy and compatible - validate database path
|
||||||
daemonClient = client
|
beadsDir := filepath.Dir(dbPath)
|
||||||
daemonStatus.Mode = cmdDaemon
|
if err := validateDaemonLock(beadsDir, dbPath); err != nil {
|
||||||
daemonStatus.Connected = true
|
_ = client.Close()
|
||||||
daemonStatus.Degraded = false
|
daemonStatus.FallbackReason = FallbackHealthFailed
|
||||||
daemonStatus.Health = health.Status
|
daemonStatus.Detail = fmt.Sprintf("daemon lock validation failed: %v", err)
|
||||||
if os.Getenv("BD_DEBUG") != "" {
|
if os.Getenv("BD_DEBUG") != "" {
|
||||||
fmt.Fprintf(os.Stderr, "Debug: connected to daemon at %s (health: %s)\n", socketPath, health.Status)
|
fmt.Fprintf(os.Stderr, "Debug: daemon lock validation failed: %v\n", err)
|
||||||
|
}
|
||||||
|
// Fall through to direct mode
|
||||||
|
} else {
|
||||||
|
// Daemon is healthy, compatible, and validated - use it
|
||||||
|
daemonClient = client
|
||||||
|
daemonStatus.Mode = cmdDaemon
|
||||||
|
daemonStatus.Connected = true
|
||||||
|
daemonStatus.Degraded = false
|
||||||
|
daemonStatus.Health = health.Status
|
||||||
|
if os.Getenv("BD_DEBUG") != "" {
|
||||||
|
fmt.Fprintf(os.Stderr, "Debug: connected to daemon at %s (health: %s)\n", socketPath, health.Status)
|
||||||
|
}
|
||||||
|
// Warn if using daemon with git worktrees
|
||||||
|
warnWorktreeDaemon(dbPath)
|
||||||
|
return // Skip direct storage initialization
|
||||||
}
|
}
|
||||||
// Warn if using daemon with git worktrees
|
|
||||||
warnWorktreeDaemon(dbPath)
|
|
||||||
return // Skip direct storage initialization
|
|
||||||
}
|
}
|
||||||
} else {
|
} else {
|
||||||
// Health check failed or daemon unhealthy
|
// Health check failed or daemon unhealthy
|
||||||
|
|||||||
Reference in New Issue
Block a user