From 74c2deb60bf81a5fe16ded6cb298bccb54f0c839 Mon Sep 17 00:00:00 2001 From: Vishal Rana Date: Sun, 25 Oct 2026 06:33:32 -0700 Subject: [PATCH] fix(app): surface collector failures and keep one collector A QA pass on the desktop app found failures that left users with no explanation, and advice that broke collection: - The app captures what its collector prints before the collector's own log is open, logs it, and ends the reported exit error with its last line, so the tray says why the collector cannot start (for example a corrupt database) instead of "exit status 1". - The app logs to the collector log, which `driga service logs` prints. A launch from Finder or at login previously lost every warning. - An unreadable settings file no longer makes a Finder or login launch exit silently: the app starts with the defaults and shows a dialog naming the file and the error. - `driga doctor` fixes follow who holds the collector lock: quit and reopen Driga for the app, `driga collect` for a manual collector, and `driga service restart` only for the service. - `driga service install` refuses while the app collects, and a service collector waits for a lock another collector holds instead of exiting for launchd to restart it every few seconds. - `driga app` passes the terminal's DRIGA_* variables through `open --env`, and the tray says when the login shell sets DRIGA_* values the app does not use. - The upgrade watch reads a bundle's version from its Info.plist rather than running the new binary, which can wait on Gatekeeper, and collector restarts wait while an upgrade is pending so an older app never starts a newer collector. - The login-environment tests no longer share a directory between runs, so they pass with -count above one. Claude-Session: https://claude.ai/code/session_01T2vtcW6ihn1ywy9vxUq9a4 --- internal/cli/app.go | 22 +++++- internal/cli/app_darwin.go | 17 ++++- internal/cli/app_darwin_test.go | 7 +- internal/cli/app_test.go | 29 +++++++ internal/cli/collect.go | 8 +- internal/cli/doctor.go | 15 ++-- internal/cli/doctor_event_sources_test.go | 2 +- internal/cli/doctor_test.go | 27 ++++++- internal/cli/service.go | 26 +++++++ internal/collector/daemon.go | 30 +++++++- internal/collector/daemon_test.go | 43 ++++++++++- internal/desktop/app.go | 32 +++++++- internal/desktop/child.go | 69 ++++++++++++++++- internal/desktop/child_test.go | 39 +++++++++- internal/desktop/env.go | 14 ++++ internal/desktop/env_test.go | 16 ++-- internal/desktop/notify_test.go | 50 ++++++++++++ internal/desktop/upgrade.go | 92 ++++++++++++++++++++++- site/src/content/docs/start/install.mdx | 5 ++ 19 files changed, 508 insertions(+), 35 deletions(-) diff --git a/internal/cli/app.go b/internal/cli/app.go index aff39d8d..4d9ce128 100644 --- a/internal/cli/app.go +++ b/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 { diff --git a/internal/cli/app_darwin.go b/internal/cli/app_darwin.go index e970985c..1038c51f 100644 --- a/internal/cli/app_darwin.go +++ b/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. diff --git a/internal/cli/app_darwin_test.go b/internal/cli/app_darwin_test.go index 30bb6af3..a76c25e1 100644 --- a/internal/cli/app_darwin_test.go +++ b/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) } } diff --git a/internal/cli/app_test.go b/internal/cli/app_test.go index 6116c89d..7798a6b6 100644 --- a/internal/cli/app_test.go +++ b/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") + } +} diff --git a/internal/cli/collect.go b/internal/cli/collect.go index 781e97ab..77995b9d 100644 --- a/internal/cli/collect.go +++ b/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() diff --git a/internal/cli/doctor.go b/internal/cli/doctor.go index b9e8a51e..5ad75a50 100644 --- a/internal/cli/doctor.go +++ b/internal/cli/doctor.go @@ -138,18 +138,19 @@ 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)), "") } @@ -157,7 +158,7 @@ func Doctor(ctx context.Context, d DoctorDeps) []Check { 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) } diff --git a/internal/cli/doctor_event_sources_test.go b/internal/cli/doctor_event_sources_test.go index ffb89845..59b5d69f 100644 --- a/internal/cli/doctor_event_sources_test.go +++ b/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") } diff --git a/internal/cli/doctor_test.go b/internal/cli/doctor_test.go index 4a2cf656..0d66ea63 100644 --- a/internal/cli/doctor_test.go +++ b/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) + } + } +} diff --git a/internal/cli/service.go b/internal/cli/service.go index 17b44ada..6e278831 100644 --- a/internal/cli/service.go +++ b/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 diff --git a/internal/collector/daemon.go b/internal/collector/daemon.go index 6aa9f750..03f1ee06 100644 --- a/internal/collector/daemon.go +++ b/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 } diff --git a/internal/collector/daemon_test.go b/internal/collector/daemon_test.go index 0a77e5b3..5b899637 100644 --- a/internal/collector/daemon_test.go +++ b/internal/collector/daemon_test.go @@ -39,7 +39,9 @@ func TestRunExclusiveLock(t *testing.T) { if err != nil || !held || owner.Kind != "service" || owner.Version != "test" || owner.PID != os.Getpid() { t.Fatalf("run owner: %+v, %v, %v", owner, held, err) } - err = Run(context.Background(), d, dir) + manual := d + manual.Owner = Owner{Kind: "manual"} + err = Run(context.Background(), manual, dir) if !errors.Is(err, ErrAlreadyRunning) || !strings.Contains(err.Error(), "another driga collector is running (service, pid ") { t.Fatalf("lock: %v", err) } @@ -64,6 +66,45 @@ func TestRunExclusiveLock(t *testing.T) { lock.Close() } +// A service waits for a lock another collector holds, rather than exiting +// for the service manager to restart it, and collects once it is free. +func TestRunServiceWaitsForLock(t *testing.T) { + d, _, _, a := setup(t) + lockWait = 10 * time.Millisecond + t.Cleanup(func() { lockWait = 5 * time.Second }) + d.Owner = Owner{Kind: "service"} + dir := config.DataDir() + app, err := Lock(dir, Owner{Kind: "app"}) + if err != nil { + t.Fatal(err) + } + ready := make(chan struct{}) + a.collect = func(ctx context.Context) { close(ready); <-ctx.Done() } + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + done := make(chan error, 1) + go func() { done <- Run(ctx, d, dir) }() + select { + case err := <-done: + t.Fatalf("service exited while the lock was held: %v", err) + case <-ready: + t.Fatal("service collected while the lock was held") + case <-time.After(200 * time.Millisecond): + } + if err := app.Close(); err != nil { + t.Fatal(err) + } + select { + case <-ready: + case <-time.After(5 * time.Second): + t.Fatal("service never took the freed lock") + } + cancel() + if err := <-done; err != nil { + t.Fatal(err) + } +} + func TestCadenceFastWithHeartbeat(t *testing.T) { fast := cadence(time.Second) idle := cadence(5 * time.Second) diff --git a/internal/desktop/app.go b/internal/desktop/app.go index b47fe3df..053625fb 100644 --- a/internal/desktop/app.go +++ b/internal/desktop/app.go @@ -8,6 +8,7 @@ import ( "os" "path/filepath" "runtime" + "slices" "strconv" "strings" "sync" @@ -47,7 +48,10 @@ type Options struct { // fills Start, ReadOwner, Ready and Executable when they are unset. Supervisor *Supervisor Config config.Config - Version string + // ConfigErr is why the settings file could not be read; Config then + // holds the defaults, and the app says so once it is ready. + ConfigErr error + Version string } // shell is the running app: the main window, the tray and the collector. @@ -84,6 +88,8 @@ type shell struct { status Status envErr error serviceErr error + // shadowed are the DRIGA_* variables the login shell sets otherwise. + shadowed []string // restartPending is set once an upgrade's relaunch was cancelled. restartPending bool needs []view.BoardRow @@ -159,6 +165,9 @@ func Run(opts Options) error { } }) mygo.App.WhenReady(func() { + if opts.ConfigErr != nil { + s.reportConfigError(opts.ConfigErr) + } _ = s.applyOpenAtLogin(opts.Config.App.LaunchAtLogin) s.applyTray(opts.Config.App.Tray) if !startHidden(s.goos, mygo.App.WasOpenedAtLogin(), s.trayCreated()) { @@ -211,13 +220,22 @@ func (s *shell) prepareSupervisor(dataDir string) (string, error) { sup.Start = func(ctx context.Context) (Child, error) { // Without the login environment the collector still runs, with // the app's own; the status says what is missing. + if s.upgrade != nil && s.upgrade.pending() { + return nil, errUpgradePending + } env, err := s.loginEnv.get(ctx) + shadowed := shadowedKeys(env, os.Getenv) s.mu.Lock() s.envErr = err + logShadowed := !slices.Equal(s.shadowed, shadowed) + s.shadowed = shadowed s.mu.Unlock() if err != nil { slog.Warn("login environment unavailable", "event", "login_env_failed", "err", err.Error()) } + if logShadowed && len(shadowed) > 0 { + slog.Warn("the login shell sets DRIGA_* variables the app does not use", "event", "login_env_shadowed", "keys", strings.Join(shadowed, ",")) + } return StartCollector(ctx, sup.Executable, env) } } @@ -246,6 +264,16 @@ func (s *shell) replaceService() { }() } +// reportConfigError says the settings could not be read. The collector +// reads the same file, so the tray shows its error until the file is fixed. +func (s *shell) reportConfigError(err error) { + slog.Warn("settings unreadable; using the defaults", "event", "config_invalid", "err", err.Error()) + go func() { + _, _ = mygo.Dialog.Message(mygo.MessageOptions{Parent: s.mainWindow(), Type: mygo.MessageWarning, + Message: "Driga could not read its settings and uses the defaults.", Detail: err.Error() + "\n\nFix the file, then quit and reopen Driga.", Buttons: []string{"OK"}}) + }() +} + // removeService removes the background service when the app replaces it, and // reports whether it did. A failure is kept for the tray, whose status line // then says how to remove the service by hand. @@ -372,6 +400,8 @@ func (s *shell) trayStatus() Status { st.Err = errors.New("background service not removed; run driga service uninstall") case s.envErr != nil && st.Collecting: st.Err = fmt.Errorf("without login shell PATH: %w", s.envErr) + case len(s.shadowed) > 0 && st.Collecting: + st.Err = fmt.Errorf("ignoring %s from the login shell; quit Driga and run driga app from it", strings.Join(s.shadowed, ", ")) } return st } diff --git a/internal/desktop/child.go b/internal/desktop/child.go index 6ba9be27..639e5eba 100644 --- a/internal/desktop/child.go +++ b/internal/desktop/child.go @@ -1,12 +1,15 @@ package desktop import ( + "bytes" "context" "errors" "fmt" "io" + "log/slog" "os" "os/exec" + "strings" "sync" "time" ) @@ -29,7 +32,9 @@ type execChild struct { // StartCollector runs exe as the app's collector with login merged over a // minimal environment. Notifications arrive on one pipe; the other is the -// lifeline. The collector writes its own log, so its output is discarded. +// lifeline. The collector writes its own log once it runs; what it prints +// before that, such as why it could not start, goes to the app's log and +// ends the error Wait returns. func StartCollector(_ context.Context, exe string, login map[string]string) (Child, error) { notifyR, notifyW, err := os.Pipe() if err != nil { @@ -41,6 +46,10 @@ func StartCollector(_ context.Context, exe string, login map[string]string) (Chi } cmd := exec.Command(exe) cmd.Env = childEnvironment(login) + output := &outputLog{} + cmd.Stdout, cmd.Stderr = output, output + // A program the collector started may hold its output open after it exits. + cmd.WaitDelay = time.Second notifyFD, parentFD, err := passFiles(cmd, notifyW, parentR) if err == nil { cmd.Args = append(cmd.Args, "collect", "--owner=app", fmt.Sprintf("--notify-fd=%d", notifyFD), fmt.Sprintf("--parent-fd=%d", parentFD)) @@ -61,6 +70,10 @@ func StartCollector(_ context.Context, exe string, login map[string]string) (Chi c := &execChild{cmd: cmd, notes: &closingReader{f: notifyR}, lifeline: parentW, done: make(chan struct{})} go func() { c.err = cmd.Wait() + output.flush() + if line := output.last(); c.err != nil && line != "" { + c.err = fmt.Errorf("%w: %s", c.err, line) + } _ = c.lifeline.Close() close(c.done) }() @@ -103,3 +116,57 @@ func (r *closingReader) Read(p []byte) (int, error) { } return n, err } + +// outputLog logs each line the collector prints and keeps the last one, +// which is why it exited when it could not start. +type outputLog struct { + mu sync.Mutex + partial []byte + line string +} + +// maxOutputLine bounds a line held while it has no newline yet. +const maxOutputLine = 4 << 10 + +func (o *outputLog) Write(p []byte) (int, error) { + o.mu.Lock() + defer o.mu.Unlock() + o.partial = append(o.partial, p...) + for { + i := bytes.IndexByte(o.partial, '\n') + if i < 0 { + break + } + o.emit(o.partial[:i]) + o.partial = o.partial[i+1:] + } + if len(o.partial) > maxOutputLine { + o.emit(o.partial) + o.partial = nil + } + return len(p), nil +} + +// flush logs a last line that ended without a newline. +func (o *outputLog) flush() { + o.mu.Lock() + defer o.mu.Unlock() + o.emit(o.partial) + o.partial = nil +} + +func (o *outputLog) emit(b []byte) { + line := strings.TrimSpace(string(b)) + if line == "" { + return + } + // The CLI prefixes its fatal error with its name. + o.line = strings.TrimPrefix(line, "driga: ") + slog.Warn("collector output", "event", "collector_output", "line", line) +} + +func (o *outputLog) last() string { + o.mu.Lock() + defer o.mu.Unlock() + return o.line +} diff --git a/internal/desktop/child_test.go b/internal/desktop/child_test.go index 98156748..4ab7e673 100644 --- a/internal/desktop/child_test.go +++ b/internal/desktop/child_test.go @@ -20,8 +20,14 @@ const helperEnv = "DESKTOP_TEST_COLLECTOR" // arguments and environment as a notification, then runs until its parent // closes the lifeline. func TestMain(m *testing.M) { - if os.Getenv(helperEnv) == "" { + switch os.Getenv(helperEnv) { + case "": os.Exit(m.Run()) + case "fail": + // A collector that cannot start says why on stderr, as cli.Main does. + _, _ = os.Stdout.WriteString("starting\n") + _, _ = os.Stderr.WriteString("driga: open store: file is not a database (26)") + os.Exit(1) } args := os.Args[1:] fs := flag.NewFlagSet("collect", flag.ExitOnError) @@ -93,3 +99,34 @@ func TestStartCollectorRunsUntilStopped(t *testing.T) { t.Fatal("notification stream did not end with the child") } } + +// Why a collector could not start ends the error the app reports. +func TestStartCollectorReportsWhyItExited(t *testing.T) { + child, err := StartCollector(context.Background(), os.Args[0], map[string]string{helperEnv: "fail"}) + if err != nil { + t.Fatal(err) + } + _, _ = io.Copy(io.Discard, child.Notifications()) + err = child.Wait() + if err == nil || !strings.HasSuffix(err.Error(), ": open store: file is not a database (26)") { + t.Fatalf("Wait = %v, want the collector's last line", err) + } +} + +func TestOutputLogKeepsLastLine(t *testing.T) { + var o outputLog + _, _ = o.Write([]byte("first\nsec")) + _, _ = o.Write([]byte("ond\n\n \n")) + if got := o.last(); got != "second" { + t.Fatalf("last = %q, want second", got) + } + _, _ = o.Write([]byte("driga: no newline")) + o.flush() + if got := o.last(); got != "no newline" { + t.Fatalf("last = %q, want the unterminated line without the CLI prefix", got) + } + _, _ = o.Write([]byte(strings.Repeat("x", maxOutputLine+1))) + if got := len(o.last()); got != maxOutputLine+1 { + t.Fatalf("long line held back: last has %d bytes", got) + } +} diff --git a/internal/desktop/env.go b/internal/desktop/env.go index d3410398..3670173f 100644 --- a/internal/desktop/env.go +++ b/internal/desktop/env.go @@ -192,3 +192,17 @@ func childEnvironment(login map[string]string) []string { slices.Sort(out) return out } + +// shadowedKeys returns, sorted, the DRIGA_* variables the login environment +// sets to another value than the app's own. The collector uses the app's, so +// a data directory exported in a shell profile is not the one the app shows. +func shadowedKeys(login map[string]string, getenv func(string) string) []string { + var keys []string + for key, value := range login { + if strings.HasPrefix(key, "DRIGA_") && getenv(key) != value { + keys = append(keys, key) + } + } + slices.Sort(keys) + return keys +} diff --git a/internal/desktop/env_test.go b/internal/desktop/env_test.go index 3a4b788e..1a01f0a4 100644 --- a/internal/desktop/env_test.go +++ b/internal/desktop/env_test.go @@ -13,17 +13,14 @@ import ( "time" ) -// fakeShell writes a login shell under the isolated HOME that prints a fixed +// fakeShell writes a login shell in a fresh directory that prints a fixed // environment, with noise a profile might print, and records its arguments. func fakeShell(t *testing.T, body string) string { t.Helper() if runtime.GOOS == "windows" { t.Skip("login shells are a Unix concept") } - dir := filepath.Join(os.Getenv("HOME"), t.Name()) - if err := os.MkdirAll(dir, 0o700); err != nil { - t.Fatal(err) - } + dir := t.TempDir() path := filepath.Join(dir, "shell") if err := os.WriteFile(path, []byte("#!/bin/sh\n"+body), 0o700); err != nil { t.Fatal(err) @@ -212,3 +209,12 @@ func TestChildEnvironmentDrigaFromAppOnly(t *testing.T) { } } } + +func TestShadowedKeys(t *testing.T) { + login := map[string]string{"PATH": "/bin", "DRIGA_DATA_DIR": "/shell", "DRIGA_CONFIG": "/same", "DRIGA_SPOOL_DIR": "/spool"} + own := map[string]string{"DRIGA_CONFIG": "/same", "DRIGA_SPOOL_DIR": "/other"} + got := shadowedKeys(login, func(k string) string { return own[k] }) + if want := []string{"DRIGA_DATA_DIR", "DRIGA_SPOOL_DIR"}; !slices.Equal(got, want) { + t.Fatalf("shadowed = %q, want %q", got, want) + } +} diff --git a/internal/desktop/notify_test.go b/internal/desktop/notify_test.go index 3a94fbe5..2df7797d 100644 --- a/internal/desktop/notify_test.go +++ b/internal/desktop/notify_test.go @@ -3,6 +3,8 @@ package desktop import ( "context" "errors" + "os" + "path/filepath" "slices" "strings" "testing" @@ -358,3 +360,51 @@ func TestClickTargetFromStore(t *testing.T) { t.Fatalf("ClickTarget = %d %v; want local session %d", id, ok, local) } } + +// A bundle's version is read from its Info.plist, never by running the +// executable, which can wait on Gatekeeper after an upgrade. +func TestInstalledVersionReadsBundle(t *testing.T) { + contents := filepath.Join(t.TempDir(), "Driga.app", "Contents") + if err := os.MkdirAll(filepath.Join(contents, "MacOS"), 0o755); err != nil { + t.Fatal(err) + } + plist := ` + + LSEnvironmentDrigaReleaseVersionnested + CFBundleVersion2026.10.17 + DrigaReleaseVersion2026.10.17-alpha +` + if err := os.WriteFile(filepath.Join(contents, "Info.plist"), []byte(plist), 0o644); err != nil { + t.Fatal(err) + } + // No executable exists: running it would fail. + got, err := installedVersion(context.Background(), filepath.Join(contents, "MacOS", "driga")) + if err != nil || got != "2026.10.17-alpha" { + t.Fatalf("installedVersion = %q, %v", got, err) + } + if _, err := plistString(strings.NewReader(plist), "Missing"); err == nil { + t.Fatal("a missing key was found") + } +} + +// Collector restarts wait while the executable changed and its version is +// not read yet. +func TestUpgradeWatchPending(t *testing.T) { + stamp := time.Unix(100, 0) + w := &upgradeWatch{ + exe: "/app/driga", version: "v1", + stat: func(string) (time.Time, error) { return stamp, nil }, + read: func(context.Context, string) (string, error) { return "v1", nil }, + } + w.start() + if w.pending() { + t.Fatal("unchanged executable pending") + } + stamp = time.Unix(200, 0) + if !w.pending() { + t.Fatal("replaced executable not pending") + } + if w.changed(context.Background()) || w.pending() { + t.Fatal("same version still pending after it was read") + } +} diff --git a/internal/desktop/upgrade.go b/internal/desktop/upgrade.go index 30bb8dcb..07d8e820 100644 --- a/internal/desktop/upgrade.go +++ b/internal/desktop/upgrade.go @@ -3,9 +3,14 @@ package desktop import ( "context" "encoding/json" + "encoding/xml" "errors" + "io" "os" "os/exec" + "path/filepath" + "strings" + "sync" "time" ) @@ -21,22 +26,45 @@ type upgradeWatch struct { interval time.Duration stat func(path string) (time.Time, error) read func(ctx context.Context, exe string) (string, error) - stamp time.Time + + mu sync.Mutex + stamp time.Time } func newUpgradeWatch(exe, version string) *upgradeWatch { - return &upgradeWatch{exe: exe, version: version, stat: modTime, read: exeVersion} + return &upgradeWatch{exe: exe, version: version, stat: modTime, read: installedVersion} } +// errUpgradePending holds collector restarts while the executable changed. +var errUpgradePending = errors.New("waiting for Driga to restart after an update") + // start records the executable as it is now. -func (w *upgradeWatch) start() { w.stamp, _ = w.stat(w.exe) } +func (w *upgradeWatch) start() { + stamp, _ := w.stat(w.exe) + w.mu.Lock() + w.stamp = stamp + w.mu.Unlock() +} + +// pending reports whether the executable changed since the watch last read +// its version. A collector started now could be another version than the +// app, and migrate the store the app reads. +func (w *upgradeWatch) pending() bool { + stamp, err := w.stat(w.exe) + w.mu.Lock() + defer w.mu.Unlock() + return err == nil && !w.stamp.IsZero() && !stamp.Equal(w.stamp) +} // changed reports whether the executable now holds another version. It asks // the executable only after its modification time moved, and asks again on // the next check while it cannot answer, as during an upgrade. func (w *upgradeWatch) changed(ctx context.Context) bool { stamp, err := w.stat(w.exe) - if err != nil || stamp.Equal(w.stamp) { + w.mu.Lock() + unchanged := stamp.Equal(w.stamp) + w.mu.Unlock() + if err != nil || unchanged { return false } version, err := w.read(ctx, w.exe) @@ -46,7 +74,9 @@ func (w *upgradeWatch) changed(ctx context.Context) bool { if version != w.version { return true } + w.mu.Lock() w.stamp = stamp + w.mu.Unlock() return false } @@ -81,6 +111,60 @@ func modTime(path string) (time.Time, error) { return info.ModTime(), nil } +// installedVersion reads the version of the app bundle the executable is +// in from its Info.plist, and asks any other executable. Running a freshly +// installed bundle can wait on Gatekeeper's first-launch prompt. +func installedVersion(ctx context.Context, exe string) (string, error) { + contents := filepath.Dir(filepath.Dir(exe)) + if filepath.Base(filepath.Dir(exe)) == "MacOS" && filepath.Base(contents) == "Contents" && strings.HasSuffix(filepath.Dir(contents), ".app") { + f, err := os.Open(filepath.Join(contents, "Info.plist")) + if err != nil { + return "", err + } + defer f.Close() + return plistString(f, "DrigaReleaseVersion") + } + return exeVersion(ctx, exe) +} + +// plistString returns the string under key in an XML property list's +// top-level dictionary. +func plistString(r io.Reader, key string) (string, error) { + dec := xml.NewDecoder(r) + depth, found := 0, false + for { + tok, err := dec.Token() + if err != nil { + if errors.Is(err, io.EOF) { + return "", errors.New(key + " not in Info.plist") + } + return "", err + } + switch t := tok.(type) { + case xml.StartElement: + depth++ + // plist > dict > key/string: the top-level entries sit at depth 3. + if depth != 3 { + continue + } + var text string + if err := dec.DecodeElement(&text, &t); err != nil { + return "", err + } + depth-- + if found { + if t.Name.Local != "string" || text == "" { + return "", errors.New(key + " is not a string") + } + return text, nil + } + found = t.Name.Local == "key" && text == key + case xml.EndElement: + depth-- + } + } +} + // exeVersion asks a driga executable for its version. func exeVersion(ctx context.Context, exe string) (string, error) { ctx, cancel := context.WithTimeout(ctx, 10*time.Second) diff --git a/site/src/content/docs/start/install.mdx b/site/src/content/docs/start/install.mdx index 65519e79..1423750c 100644 --- a/site/src/content/docs/start/install.mdx +++ b/site/src/content/docs/start/install.mdx @@ -27,6 +27,11 @@ new version. If an earlier release installed the collector service, the app removes it on its first launch, says so once, and collects itself. +The app reads `DRIGA_*` variables from its own environment, not your shell +profile: `driga app` passes the terminal's on to it. It logs to the collector +log, which `driga service logs` prints, and `driga service install` refuses +while the app collects. +