fix(app): surface collector failures and keep one collector

internal/cli/app.go

@@ -3,12 +3,16 @@ package cli
 import (
 	"context"
 	"encoding/json"
+	"fmt"
 	"io"
+	"log/slog"
 	"os"
 
 	"github.com/labstack/driga/internal/cleanup"
 	"github.com/labstack/driga/internal/config"
 	"github.com/labstack/driga/internal/desktop"
+	"github.com/labstack/driga/internal/logfile"
+	"github.com/labstack/driga/internal/service"
 	"github.com/labstack/driga/internal/view"
 	"github.com/labstack/driga/internal/web"
 )
@@ -64,9 +68,14 @@ func (AppCmd) Run(g *Globals) error {
 	if handed, err := handOffApp(systemLaunch); handed || err != nil {
 		return err
 	}
+	restore := appLogging()
+	defer restore()
+	// Opened from Finder or at login, an error here would reach no one: the
+	// app starts with the default settings and says what is wrong instead.
 	cfg, err := config.Load(config.Path())
+	var configErr error
 	if err != nil {
-		return err
+		cfg, configErr = config.Default(), fmt.Errorf("read %s: %w", config.Path(), err)
 	}
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
@@ -93,10 +102,21 @@ func (AppCmd) Run(g *Globals) error {
 		}},
 		Supervisor: &desktop.Supervisor{Executable: collectorExecutable},
 		Config:     cfg,
+		ConfigErr:  configErr,
 		Version:    g.Version,
 	})
 }
 
+// appLogging writes the app's log, and its collector's output, to the
+// collector log, which driga service logs shows. A launch from Finder or at
+// login has nowhere else to write; stderr keeps a terminal run readable.
+func appLogging() func() {
+	sink := io.MultiWriter(&logfile.Writer{Path: service.LogPath()}, os.Stderr)
+	previous := slog.Default()
+	slog.SetDefault(slog.New(logfile.TextHandler(sink)).With("process", "app"))
+	return func() { slog.SetDefault(previous) }
+}
+
 // pageActivity keeps the collector's viewer heartbeat fresh while the page is used.
 func pageActivity(active bool) {
 	if active {

internal/cli/app_darwin.go

@@ -22,12 +22,23 @@ func handOffApp(system bool) (bool, error) {
 	if !handsOff(system, exe) {
 		return false, nil
 	}
-	return true, handOffCommand(appBundle(exe)).Run()
+	return true, handOffCommand(appBundle(exe), os.Environ()).Run()
 }
 
 // handOffCommand opens bundle by its path: another copy of Driga.app, such
-// as one in ~/Applications, has the same bundle identifier.
-func handOffCommand(bundle string) *exec.Cmd { return exec.Command("open", "-a", bundle) }
+// as one in ~/Applications, has the same bundle identifier. Launch Services
+// does not pass on the caller's environment, so the DRIGA_* variables naming
+// the data, config and settings go along explicitly. A running app keeps
+// its own.
+func handOffCommand(bundle string, environ []string) *exec.Cmd {
+	args := []string{}
+	for _, entry := range environ {
+		if strings.HasPrefix(entry, "DRIGA_") && !strings.HasPrefix(entry, "DRIGA_APP=") {
+			args = append(args, "--env", entry)
+		}
+	}
+	return exec.Command("open", append(args, "-a", bundle)...)
+}
 
 // handsOff reports whether to hand off: a launch not made by the system, of
 // an executable inside an app bundle. A development build runs in place.

internal/cli/app_darwin_test.go

@@ -37,10 +37,11 @@ func TestAppHandOff(t *testing.T) {
 }
 
 // The hand-off opens the bundle the command line ran from, not whichever
-// bundle Launch Services knows by the identifier.
+// bundle Launch Services knows by the identifier, with the command line's
+// DRIGA_* variables.
 func TestAppHandOffCommand(t *testing.T) {
-	cmd := handOffCommand("/Users/someone/Applications/Driga.app")
-	if want := []string{"open", "-a", "/Users/someone/Applications/Driga.app"}; !slices.Equal(cmd.Args, want) {
+	cmd := handOffCommand("/Users/someone/Applications/Driga.app", []string{"PATH=/bin", "DRIGA_DATA_DIR=/data dir", "DRIGA_APP=1", "DRIGA_CONFIG="})
+	if want := []string{"open", "--env", "DRIGA_DATA_DIR=/data dir", "--env", "DRIGA_CONFIG=", "-a", "/Users/someone/Applications/Driga.app"}; !slices.Equal(cmd.Args, want) {
 		t.Fatalf("hand-off runs %q, want %q", cmd.Args, want)
 	}
 }

internal/cli/app_test.go

@@ -4,6 +4,9 @@ import (
 	"bytes"
 	"context"
 	"os"
+	"path/filepath"
+	"reflect"
+	"strings"
 	"testing"
 
 	"github.com/labstack/driga/internal/config"
@@ -121,3 +124,29 @@ func TestAppCommandRunsDesktop(t *testing.T) {
 		t.Fatalf("page handlers: %+v", got.Web)
 	}
 }
+
+// Opened from Finder or at login, an unreadable settings file reaches no
+// terminal: the app starts with the defaults and is told why.
+func TestAppCommandStartsWithBadConfig(t *testing.T) {
+	readEnv(t)
+	path := filepath.Join(t.TempDir(), "config.toml")
+	if err := os.WriteFile(path, []byte("[app]\ntray = \"no\"\n"), 0o600); err != nil {
+		t.Fatal(err)
+	}
+	t.Setenv("DRIGA_CONFIG", path)
+	defer func(prev func(desktop.Options) error) { runDesktop = prev }(runDesktop)
+	var got desktop.Options
+	runDesktop = func(opts desktop.Options) error { got = opts; return nil }
+	defer func(prev bool) { systemLaunch = prev }(systemLaunch)
+	systemLaunch = true
+	var out, errout bytes.Buffer
+	if code := run([]string{"app"}, "test-version", nil, &out, &errout); code != 0 {
+		t.Fatalf("driga app: %d %s", code, errout.String())
+	}
+	if got.ConfigErr == nil || !strings.Contains(got.ConfigErr.Error(), path) {
+		t.Fatalf("config error = %v, want one naming %s", got.ConfigErr, path)
+	}
+	if !reflect.DeepEqual(got.Config, config.Default()) {
+		t.Fatal("the app did not start with the default settings")
+	}
+}

internal/cli/collect.go

@@ -135,7 +135,7 @@ func (c CollectCmd) Run(g *Globals) error {
 	}
 	dir := config.DataDir()
 	if !c.Once {
-		restore, err := collectorLogging()
+		restore, err := collectorLogging(c.Owner)
 		if err != nil {
 			return err
 		}
@@ -173,13 +173,15 @@ func watchParent(r io.Reader, cancel context.CancelFunc) {
 	cancel()
 }
 
-func collectorLogging() (func(), error) {
+// collectorLogging writes the collector's log. On Linux a service's journal
+// shows it too; the app logs a child's output itself, so that copy is left out.
+func collectorLogging(owner string) (func(), error) {
 	w := &logfile.Writer{Path: service.LogPath()}
 	if _, err := w.Write(nil); err != nil {
 		return nil, err
 	}
 	var sink io.Writer = w
-	if runtime.GOOS == "linux" {
+	if runtime.GOOS == "linux" && owner != "app" {
 		sink = io.MultiWriter(sink, os.Stderr)
 	}
 	previous := slog.Default()

internal/cli/doctor.go

@@ -138,26 +138,27 @@ func Doctor(ctx context.Context, d DoctorDeps) []Check {
 		probe = collector.ReadOwner
 	}
 	owner, held, err := probe(d.DataDir)
+	restart := restartHint(goos, owner, held)
 	switch {
 	case err != nil:
 		add("collector", "fail", err.Error(), "check access to "+d.DataDir+"/driga.lock")
 	case !held:
 		add("collector", "warn", "not running; snapshot mode has no history", collectorHint(goos))
 	case d.Store == nil:
-		add("collector", "warn", "lock held but database unavailable", "driga service restart")
+		add("collector", "warn", "lock held but database unavailable", restart)
 	default:
 		raw, err := d.Store.Meta("last_pass_at")
 		last, parseErr := time.Parse(time.RFC3339Nano, raw)
 		if err != nil || parseErr != nil || last.After(d.Now) || d.Now.Sub(last) > 3*intervals.Procs {
-			add("collector", "warn", "lock held but last pass is missing or stale", "driga service restart")
+			add("collector", "warn", "lock held but last pass is missing or stale", restart)
 		} else {
 			add("collector", "ok", fmt.Sprintf("running (%s, pid %d); last pass %s ago", owner.Kind, owner.PID, d.Now.Sub(last).Round(time.Second)), "")
 		}
 	}
 	if d.Store != nil {
 		sources, err := d.Store.Sources()
 		if err != nil {
-			add("sources", "fail", err.Error(), "check database access; run driga service restart")
+			add("sources", "fail", err.Error(), "check database access; "+restart)
 		}
 		for _, source := range sources {
 			if source.Name == "otlp" || source.Name == "chat" {
@@ -177,7 +178,7 @@ func Doctor(ctx context.Context, d DoctorDeps) []Check {
 				detail = "not yet receiving events"
 			}
 			if fresh.Stale || fresh.Status == "" && (source.LastOKAt.After(d.Now) || source.LastErrorAt.After(source.LastOKAt)) {
-				status, fix = "warn", "driga service restart"
+				status, fix = "warn", restart
 			}
 			if source.LastError != "" {
 				detail += "; last error: " + source.LastError
@@ -187,7 +188,7 @@ func Doctor(ctx context.Context, d DoctorDeps) []Check {
 		}
 		procs, err := d.Store.Procs()
 		if err != nil {
-			add("collector_rss", "fail", err.Error(), "check database access; run driga service restart")
+			add("collector_rss", "fail", err.Error(), "check database access; "+restart)
 		} else {
 			sigs, _ := signatures.Load(nil)
 			for _, p := range procs {
@@ -240,9 +241,9 @@ func Doctor(ctx context.Context, d DoctorDeps) []Check {
 				detail = fmt.Sprintf("%.1f MB footprint (RSS %.1f MB)", float64(footprint)/1e6, float64(rss)/1e6)
 			}
 			if !found {
-				status, detail, fix = "warn", "collector absent from process snapshot", "driga service start"
+				status, detail, fix = "warn", "collector absent from process snapshot", restart
 			} else if budget > 50_000_000 {
-				status, fix = "warn", "driga service restart; inspect driga service logs"
+				status, fix = "warn", restart+"; inspect driga service logs"
 			}
 			add("collector_rss", status, detail, fix)
 		}

internal/cli/doctor_event_sources_test.go

@@ -23,5 +23,5 @@ func TestDoctorQuietEventSources(t *testing.T) {
 	if err := st.Tx(context.Background(), func(tx *store.Tx) error { return tx.SetSourceError("sockets", "probe failed", d.Now) }); err != nil {
 		t.Fatal(err)
 	}
-	requireCheck(t, Doctor(context.Background(), d), "source:sockets", "warn", "driga service restart")
+	requireCheck(t, Doctor(context.Background(), d), "source:sockets", "warn", "stop and rerun driga collect")
 }

internal/cli/doctor_test.go

@@ -168,7 +168,7 @@ func TestDoctorStaleSource(t *testing.T) {
 	if err := st.Tx(context.Background(), func(tx *store.Tx) error { return tx.SetSourceOK("git", d.Now.Add(-16*time.Minute)) }); err != nil {
 		t.Fatal(err)
 	}
-	requireCheck(t, Doctor(context.Background(), d), "source:git", "warn", "driga service restart")
+	requireCheck(t, Doctor(context.Background(), d), "source:git", "warn", "stop and rerun driga collect")
 }
 
 func TestDoctorIntakeRejections(t *testing.T) {
@@ -226,8 +226,8 @@ func TestDoctorCadenceAndRSS(t *testing.T) {
 		t.Fatal(err)
 	}
 	checks := Doctor(context.Background(), d)
-	requireCheck(t, checks, "collector", "warn", "driga service restart")
-	requireCheck(t, checks, "source:machine", "warn", "driga service restart")
+	requireCheck(t, checks, "collector", "warn", "stop and rerun driga collect")
+	requireCheck(t, checks, "source:machine", "warn", "stop and rerun driga collect")
 	c := requireCheck(t, checks, "collector_rss", "warn", "")
 	if !strings.Contains(c.Detail, "51.0 MB") {
 		t.Fatal(c)
@@ -447,3 +447,24 @@ func TestDoctorMalformedExcludeWarning(t *testing.T) {
 		t.Fatal(cfg.Exclude)
 	}
 }
+
+// The fix restarts whichever collector holds the lock.
+func TestRestartHintFollowsOwner(t *testing.T) {
+	for _, tc := range []struct {
+		goos, kind string
+		held       bool
+		want       string
+	}{
+		{"darwin", "", false, "driga app"},
+		{"linux", "", false, "driga service install"},
+		{"darwin", "app", true, "quit and reopen Driga"},
+		{"darwin", "service", true, "driga service restart"},
+		{"linux", "manual", true, "stop and rerun driga collect"},
+		{"windows", "unknown", true, "quit and reopen Driga"},
+		{"linux", "unknown", true, "driga service restart"},
+	} {
+		if got := restartHint(tc.goos, collector.Owner{Kind: tc.kind}, tc.held); got != tc.want {
+			t.Errorf("restartHint(%s, %q, %v) = %q, want %q", tc.goos, tc.kind, tc.held, got, tc.want)
+		}
+	}
+}

internal/cli/service.go

@@ -100,6 +100,11 @@ func (c serviceControl) Run(g *Globals) error {
 	}
 	switch c.Action {
 	case "install":
+		// A service installed while the app collects would only wait for the
+		// lock, and take over collection from the app once it quits.
+		if owner, held, err := collector.ReadOwner(config.DataDir()); err == nil && held && owner.Kind == "app" {
+			return errors.New("the Driga app collects on this computer; quit Driga first to collect with the service instead")
+		}
 		if runtime.GOOS == "darwin" {
 			// launchd opens these streams before the collector starts.
 			dir := filepath.Dir(service.LogPath())
@@ -184,6 +189,27 @@ func collectorHint(goos string) string {
 	return "driga service install"
 }
 
+// restartHint says how to restart the collector that holds the lock: the
+// app, the service or a collector run by hand. With none holding it, the
+// collector is started the way this platform runs it.
+func restartHint(goos string, owner collector.Owner, held bool) string {
+	if !held {
+		return collectorHint(goos)
+	}
+	switch owner.Kind {
+	case "app":
+		return "quit and reopen Driga"
+	case "service":
+		return "driga service restart"
+	case "manual":
+		return "stop and rerun driga collect"
+	}
+	if goos == "darwin" || goos == "windows" {
+		return "quit and reopen Driga"
+	}
+	return "driga service restart"
+}
+
 func serviceBinaryPath(bin string) (string, error) {
 	if runtime.GOOS == "windows" {
 		// Keep Scoop's stable current junction. The OS can execute this path

internal/collector/daemon.go

@@ -212,6 +212,34 @@ func (s *schedule) wait(_ time.Time, _ Intervals) time.Duration {
 	return time.Second
 }
 
+// lockWait is how often a service collector asks again for a lock another
+// collector holds; tests shorten it.
+var lockWait = 5 * time.Second
+
+// lockFor takes the collector lock. A service waits while another collector,
+// such as the desktop app's, holds it: the service manager would otherwise
+// restart it every few seconds for as long as the other one runs.
+func lockFor(ctx context.Context, clock daemonClock, dataDir string, owner Owner) (*os.File, error) {
+	logged := false
+	for {
+		lock, err := Lock(dataDir, owner)
+		if owner.Kind != "service" || !errors.Is(err, ErrAlreadyRunning) {
+			return lock, err
+		}
+		if !logged {
+			slog.Info("another collector holds the lock; waiting for it to stop", "event", "lock_wait", "err", err.Error())
+			logged = true
+		}
+		timer := clock.NewTimer(lockWait)
+		select {
+		case <-ctx.Done():
+			timer.Stop()
+			return nil, ctx.Err()
+		case <-timer.Chan():
+		}
+	}
+}
+
 // Run collects until cancellation while holding the exclusive writer lock.
 // Cancellation, by a signal or the parent going away, is a clean stop at
 // whatever step it lands, startup included: whatever a step returned then
@@ -232,7 +260,7 @@ func run(ctx context.Context, d Deps, dataDir string) error {
 	if clock == nil {
 		clock = realDaemonClock{}
 	}
-	lock, err := Lock(dataDir, d.Owner)
+	lock, err := lockFor(ctx, clock, dataDir, d.Owner)
 	if err != nil {
 		return err
 	}