diff --git a/internal/cli/doctor.go b/internal/cli/doctor.go index ae5acc3..3b3ea90 100644 --- a/internal/cli/doctor.go +++ b/internal/cli/doctor.go @@ -425,7 +425,7 @@ func Doctor(ctx context.Context, d DoctorDeps) []Check { if err != nil { add("service_definition", "warn", err.Error(), "check the service plist; driga service install") } else if stale { - add("service_definition", "warn", "the service plist points at a stale binary", "driga service install") + add("service_definition", "warn", "the service plist runs another binary or collects into another data directory", "driga service install") } } status := d.NotificationStatus diff --git a/internal/service/bundle.go b/internal/service/bundle.go index b5a1071..b33d7e4 100644 --- a/internal/service/bundle.go +++ b/internal/service/bundle.go @@ -8,6 +8,7 @@ import ( "path/filepath" "strings" + "github.com/labstack/driga/internal/config" "github.com/labstack/driga/internal/self" ) @@ -60,8 +61,11 @@ func IsDaemon(binPath string) bool { return name == "drigad" || strings.HasPrefix(name, "drigad-") } -// PlistStale is a read-only diagnostic. Compare path strings, not inode identity: -// an old bare PATH symlink can still resolve to the new bundle executable. +// PlistStale is a read-only diagnostic: the plist runs another executable, lacks +// the bundle association, or collects into another data directory than this +// process uses, so its clients would find no service. Compare path strings, +// not inode identity: an old bare PATH symlink can still resolve to the new +// bundle executable. func PlistStale(path, binPath string) (bool, error) { data, err := os.Open(path) if errors.Is(err, os.ErrNotExist) { @@ -126,7 +130,14 @@ func PlistStale(path, binPath string) (bool, error) { if executable == "" { return false, errors.New("service plist has no ProgramArguments executable") } - return executable != DaemonPath(ExecutablePath(binPath)) || (BundlePath(binPath) != "" && !associated), nil + if executable != DaemonPath(ExecutablePath(binPath)) || (BundlePath(binPath) != "" && !associated) { + return true, nil + } + dir, err := plistEnv(path, "DRIGA_DATA_DIR") + if err != nil { + return false, err + } + return resolveDataDir(dir) != config.DataDir(), nil } // plistEnv is the value the collector plist at path sets for the environment diff --git a/internal/service/bundle_test.go b/internal/service/bundle_test.go index 225fe1f..52dd85c 100644 --- a/internal/service/bundle_test.go +++ b/internal/service/bundle_test.go @@ -123,3 +123,44 @@ func TestPlistEnv(t *testing.T) { t.Fatal("missing plist read") } } + +// A plist that collects into another data directory than this process uses is +// stale, so install rewrites it and doctor reports it: its clients would +// otherwise find no service and fall back to snapshot mode. +func TestPlistStaleDataDir(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("launchd plist") + } + dir := t.TempDir() + bin := filepath.Join(dir, "driga") + plistPath := filepath.Join(dir, "collector.plist") + write := func(dataDir string) { + t.Helper() + t.Setenv("DRIGA_DATA_DIR", dataDir) + body, err := renderLaunchd(configForOS(bin, "darwin")) + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(plistPath, body, 0o600); err != nil { + t.Fatal(err) + } + } + data := filepath.Join(dir, "Application Support", "driga") + for _, tc := range []struct { + name, recorded, current string + stale bool + }{ + {"same", data, data, false}, + {"escaped space", filepath.Join(dir, `Application\ Support`, "driga"), data, true}, + {"other", filepath.Join(dir, "other"), data, true}, + {"unset matches default", "", "", false}, + } { + t.Run(tc.name, func(t *testing.T) { + write(tc.recorded) + t.Setenv("DRIGA_DATA_DIR", tc.current) + if stale, err := PlistStale(plistPath, bin); err != nil || stale != tc.stale { + t.Fatalf("stale = %t, %v; want %t", stale, err, tc.stale) + } + }) + } +} diff --git a/internal/service/launchd_darwin.go b/internal/service/launchd_darwin.go index cc1a31e..f1e8f5e 100644 --- a/internal/service/launchd_darwin.go +++ b/internal/service/launchd_darwin.go @@ -3,6 +3,7 @@ package service import ( "errors" "fmt" + "log/slog" "os" "os/exec" "path/filepath" @@ -68,6 +69,9 @@ func (s *launchdController) Install() error { if !stale { return nil } + if dir, err := s.recordedDataDir(); err == nil && resolveDataDir(dir) != config.DataDir() { + slog.Info("the Driga service collected into another data directory; moving it to this one", "event", "service_data_dir_moved", "from", resolveDataDir(dir), "to", config.DataDir()) + } status, err := s.Status() if err != nil && !errors.Is(err, ks.ErrNotInstalled) { return err diff --git a/internal/service/service.go b/internal/service/service.go index fb83226..3ee567c 100644 --- a/internal/service/service.go +++ b/internal/service/service.go @@ -176,10 +176,19 @@ func DataDir(c Controller) (string, error) { if err != nil { return "", err } + return resolveDataDir(dir), nil +} + +// resolveDataDir is the directory a service definition's DRIGA_DATA_DIR +// names, or the default when it records none. +func resolveDataDir(dir string) string { if dir == "" { - return config.DefaultDataDir(), nil + return config.DefaultDataDir() + } + if abs, err := filepath.Abs(config.ExpandHome(dir)); err == nil { + return abs } - return filepath.Abs(config.ExpandHome(dir)) + return filepath.Clean(config.ExpandHome(dir)) } type controlOnly struct{}