From f9f6fa32ea5faaeba69874b710045ef30adfcfe6 Mon Sep 17 00:00:00 2001 From: dvcdsys Date: Mon, 10 Aug 2026 18:40:09 +0100 Subject: [PATCH] fix(mac): notice a deleted database, retire the bootstrap password, refuse concurrent restarts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three defects found by walking the install path by hand. **A deleted database went unnoticed.** needsFirstRun looked only for ~/.cix/server.env. Delete ~/.cix/data and the app carried on as a configured install: the server recreated the directory itself, found it empty, and minted a fresh admin from the bootstrap credentials still sitting in server.env — using the original generated password rather than whichever one the user had set since. Observed live at 18:31, "bootstrap admin created from CIX_BOOTSTRAP_ADMIN_EMAIL + CIX_BOOTSTRAP_ADMIN_PASSWORD" in the server log, with nothing on screen to say a database had gone. Setup is now needed when either the config file or the database it names is missing. **The bootstrap password never left.** It seeds the first admin and has no use afterwards, but it stayed on disk and stayed authoritative — a credential outliving the account it created, and quietly overriding a password rotated in the dashboard the next time the database was recreated. It is now removed once a running server proves the account exists (bootstrap runs before the listener opens, so an answering /health is that proof). The email stays: it is not a credential, and the reset-password dialog offers it as a default. Because the wizard can now run a second time, it merges onto the existing server.env instead of rebuilding it from defaults. Reverting someone's port or, worse, their network-access choice would be a bigger surprise than the one this removes. **Two menu items could restart the server at once.** Start at Login and Allow Network Access each restart it, each in its own goroutine, and each drives the same launchd label: one boots the job out while the other waits for its pid to disappear and then bootstraps it itself. Two bootstraps of one label leave either a failure or two processes racing for the port — from the outside, a server that went away and did not come back. Reported from use: click one, then the other during the restart, and the server hangs. A busy flag now gates every action that restarts the server, and render() respects it — without that the poller would re-enable the controls five seconds into the operation. The same guard covers a server that is still starting: a cold start loads an embedding model and takes minutes, and a restart landing in the middle of that is exactly the wedge. Serialising instead of refusing would only queue a second restart nobody asked for. Reset Password stays available throughout: it opens the database directly and neither needs a running server nor restarts one. Co-Authored-By: Claude Opus 5 --- cli/launcher/env_darwin.go | 8 ++ cli/launcher/firstrun_darwin.go | 128 +++++++++++++++++---- cli/launcher/firstrun_darwin_test.go | 160 +++++++++++++++++++++++++++ cli/launcher/menu_darwin.go | 98 +++++++++++++++- 4 files changed, 366 insertions(+), 28 deletions(-) create mode 100644 cli/launcher/firstrun_darwin_test.go diff --git a/cli/launcher/env_darwin.go b/cli/launcher/env_darwin.go index 84cf63e..cabab3b 100644 --- a/cli/launcher/env_darwin.go +++ b/cli/launcher/env_darwin.go @@ -185,3 +185,11 @@ func dashboardURL(vars map[string]string) string { func localBaseURL(vars map[string]string) string { return fmt.Sprintf("http://localhost:%d", serverPort(vars)) } + +// setDefault fills a key only when it has no value yet, so re-running setup +// keeps whatever the user has chosen since the first time. +func setDefault(vars map[string]string, key, value string) { + if strings.TrimSpace(vars[key]) == "" { + vars[key] = value + } +} diff --git a/cli/launcher/firstrun_darwin.go b/cli/launcher/firstrun_darwin.go index 11f2f88..c9f403a 100644 --- a/cli/launcher/firstrun_darwin.go +++ b/cli/launcher/firstrun_darwin.go @@ -27,14 +27,80 @@ import ( const bootstrapServerName = "local" -// needsFirstRun reports whether ~/.cix/server.env is missing. +// needsFirstRun reports whether this installation still has to be set up. +// +// Two conditions, and the second one matters more than it looks: no +// ~/.cix/server.env, or no database at the path that file names. +// +// Keying on the config file alone was wrong. Delete ~/.cix/data and the app +// carried on as a configured install — the server then recreated the directory +// itself and silently minted a fresh admin from the bootstrap credentials still +// sitting in server.env, with the original generated password rather than +// whichever one the user had since set. A missing database is not a +// configuration this app should quietly repair; it is an installation that +// needs setting up again, and saying so is the whole point. +// +// Not covered: a database file that exists but holds no users — a truncated or +// hand-emptied one. Answering that needs to open SQLite, which would drag the +// driver into the launcher for a case far rarer than "I deleted my data +// directory". The server refuses to start in that state and says why in +// ~/.cix/logs/cix-server.err. func needsFirstRun() bool { path, err := serverEnvPath() if err != nil { return false } - _, err = os.Stat(path) - return errors.Is(err, os.ErrNotExist) + if _, err := os.Stat(path); errors.Is(err, os.ErrNotExist) { + return true + } + + vars, err := readServerEnv() + if err != nil { + // Present but unreadable. Running the wizard would overwrite a file we + // cannot even parse, so leave it alone and let the failure surface. + logf("could not read %s: %v", path, err) + return false + } + db := strings.TrimSpace(vars["CIX_SQLITE_PATH"]) + if db == "" { + // Hand-edited beyond what we wrote. Not ours to second-guess. + return false + } + if _, err := os.Stat(db); errors.Is(err, os.ErrNotExist) { + logf("configured database %s is missing — treating this as an unconfigured install", db) + return true + } + return false +} + +// retireBootstrapPassword drops CIX_BOOTSTRAP_ADMIN_PASSWORD from server.env +// once there is a running server, and therefore an account, that no longer +// needs it. +// +// The password seeds the very first admin and has no purpose afterwards, but it +// used to stay on disk forever — and stay authoritative. Wipe the database and +// the account came back with THAT password, not the one the user had set in the +// dashboard; a credential outliving the account it created, silently. +// +// The email stays: it is harmless, and the password-reset dialog offers it as +// the address to reset. +// +// Called only when the server is confirmed up. Bootstrap runs before the HTTP +// listener opens, so an answering /health is proof the account exists. +func retireBootstrapPassword() { + vars, err := readServerEnv() + if err != nil { + return + } + if _, ok := vars["CIX_BOOTSTRAP_ADMIN_PASSWORD"]; !ok { + return + } + delete(vars, "CIX_BOOTSTRAP_ADMIN_PASSWORD") + if err := writeServerEnv(vars); err != nil { + logf("could not remove the bootstrap password from server.env: %v", err) + return + } + logf("removed CIX_BOOTSTRAP_ADMIN_PASSWORD from server.env — the admin account exists") } // runFirstRun walks the user through creating the admin account, writes @@ -94,26 +160,40 @@ func runFirstRun(u *updater) error { return err } - vars := map[string]string{ - "CIX_BOOTSTRAP_ADMIN_EMAIL": email, - "CIX_BOOTSTRAP_ADMIN_PASSWORD": password, - // Imported as an "env-bootstrap" legacy key when the admin is created - // on a fresh DB (bootstrap.go), so the CLI works from the first boot - // without anyone visiting the dashboard to mint one. - "CIX_API_KEY": apiKey, - "CIX_PORT": strconv.Itoa(defaultServerPort), - "CIX_DATA_DIR": dataDir, - "CIX_SQLITE_PATH": filepath.Join(dataDir, "cix.db"), - // Loopback by default, unlike the server's own all-interfaces default. - // A container has to be reachable from outside itself; a desktop app - // does not, and exposing a code index to the local network is a choice - // someone should make on purpose. The menu has a toggle for it. - "CIX_BIND_ADDR": bindLocalOnly, - // The .app owns updating itself. Leaving the server's own check on - // would mean two different components offering the user two different - // "update available" prompts for two different tag streams. - "CIX_VERSION_CHECK_ENABLED": "false", + // Start from whatever is already configured. The wizard runs a second time + // when the database has gone missing, and on that path server.env exists and + // holds decisions the user made since — the port, and whether the server is + // reachable from the network. Rebuilding the file from defaults would revert + // them without saying so, which is a worse surprise than the one this whole + // change exists to remove. + vars, err := readServerEnv() + if err != nil { + vars = map[string]string{} } + + // Always fresh: the account and its key are being created now. On a repeat + // run the previous key died with the database it was imported into. + vars["CIX_BOOTSTRAP_ADMIN_EMAIL"] = email + vars["CIX_BOOTSTRAP_ADMIN_PASSWORD"] = password + // Imported as an "env-bootstrap" legacy key when the admin is created on a + // fresh DB (bootstrap.go), so the CLI works from the first boot without + // anyone visiting the dashboard to mint one. + vars["CIX_API_KEY"] = apiKey + + // Defaults only where nothing has been chosen. + setDefault(vars, "CIX_PORT", strconv.Itoa(defaultServerPort)) + setDefault(vars, "CIX_DATA_DIR", dataDir) + setDefault(vars, "CIX_SQLITE_PATH", filepath.Join(dataDir, "cix.db")) + // Loopback by default, unlike the server's own all-interfaces default. A + // container has to be reachable from outside itself; a desktop app does not, + // and exposing a code index to the local network is a choice someone should + // make on purpose. The menu has a toggle for it. + setDefault(vars, "CIX_BIND_ADDR", bindLocalOnly) + // The .app owns updating itself. Leaving the server's own check on would + // mean two different components offering the user two different "update + // available" prompts for two different tag streams. + setDefault(vars, "CIX_VERSION_CHECK_ENABLED", "false") + if err := writeServerEnv(vars); err != nil { return fmt.Errorf("write server.env: %w", err) } @@ -140,6 +220,10 @@ func runFirstRun(u *updater) error { // minutes, in silence. Saying so beats a spinner that gives up. waitNote = "\n\nThe server is still starting. First boot downloads and loads the embedding " + "model, which can take a few minutes; the menu bar will show it as running when it is ready." + } else { + // The account exists — bootstrap runs before the listener opens — so the + // password that seeded it has done its job and stops being kept. + retireBootstrapPassword() } return alertWithCopy("cix is set up", fmt.Sprintf( diff --git a/cli/launcher/firstrun_darwin_test.go b/cli/launcher/firstrun_darwin_test.go new file mode 100644 index 0000000..f78753d --- /dev/null +++ b/cli/launcher/firstrun_darwin_test.go @@ -0,0 +1,160 @@ +package main + +import ( + "os" + "path/filepath" + "testing" +) + +// writeTestEnv lays down a server.env like the wizard's, pointing at dbPath. +func writeTestEnv(t *testing.T, home, dbPath string, extra map[string]string) { + t.Helper() + if err := os.MkdirAll(filepath.Join(home, ".cix"), 0o700); err != nil { + t.Fatal(err) + } + vars := map[string]string{ + "CIX_BOOTSTRAP_ADMIN_EMAIL": "someone@example.com", + "CIX_BOOTSTRAP_ADMIN_PASSWORD": "generated-once", + "CIX_API_KEY": "cix_testkey", + "CIX_PORT": "21847", + "CIX_SQLITE_PATH": dbPath, + "CIX_BIND_ADDR": bindLocalOnly, + } + for k, v := range extra { + if v == "" { + delete(vars, k) + continue + } + vars[k] = v + } + if err := writeServerEnv(vars); err != nil { + t.Fatal(err) + } +} + +func touch(t *testing.T, path string) { + t.Helper() + if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, []byte("not really sqlite"), 0o600); err != nil { + t.Fatal(err) + } +} + +// Setup is needed when the database is gone, not only when the config is. +// +// Keying on server.env alone meant that deleting ~/.cix/data left the app +// believing it was configured — and the server then minted a fresh admin from +// the bootstrap credentials still in that file, using the original generated +// password rather than whatever the user had set since. +func TestNeedsFirstRun(t *testing.T) { + t.Run("no server.env", func(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + if !needsFirstRun() { + t.Error("needsFirstRun() = false with no server.env") + } + }) + + t.Run("server.env but no database", func(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + writeTestEnv(t, home, filepath.Join(home, ".cix", "data", "cix.db"), nil) + if !needsFirstRun() { + t.Error("needsFirstRun() = false with a configured database that does not exist") + } + }) + + t.Run("both present", func(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + db := filepath.Join(home, ".cix", "data", "cix.db") + writeTestEnv(t, home, db, nil) + touch(t, db) + if needsFirstRun() { + t.Error("needsFirstRun() = true on a complete installation") + } + }) + + t.Run("hand-edited env with no database path", func(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + writeTestEnv(t, home, "", map[string]string{"CIX_SQLITE_PATH": ""}) + // Nothing to check against, so nothing to conclude. Running the wizard + // here would overwrite a file somebody edited on purpose. + if needsFirstRun() { + t.Error("needsFirstRun() = true on an env file with no CIX_SQLITE_PATH") + } + }) +} + +func TestRetireBootstrapPassword(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + db := filepath.Join(home, ".cix", "data", "cix.db") + writeTestEnv(t, home, db, map[string]string{"CIX_BIND_ADDR": bindAllInterfaces}) + touch(t, db) + + retireBootstrapPassword() + + vars, err := readServerEnv() + if err != nil { + t.Fatal(err) + } + if _, ok := vars["CIX_BOOTSTRAP_ADMIN_PASSWORD"]; ok { + t.Error("the bootstrap password survived") + } + // The email is what the reset-password dialog offers as a default, and it + // is not a credential. + if vars["CIX_BOOTSTRAP_ADMIN_EMAIL"] != "someone@example.com" { + t.Errorf("email = %q, want it kept", vars["CIX_BOOTSTRAP_ADMIN_EMAIL"]) + } + // Nothing else may be lost on the way through: the API key is what the CLI + // authenticates with, and the bind address is a choice the user made. + if vars["CIX_API_KEY"] != "cix_testkey" { + t.Errorf("api key = %q, want it kept", vars["CIX_API_KEY"]) + } + if vars["CIX_BIND_ADDR"] != bindAllInterfaces { + t.Errorf("bind addr = %q, want it kept", vars["CIX_BIND_ADDR"]) + } + + // The file still holds an API key, so the mode still matters. + path, _ := serverEnvPath() + info, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + if perm := info.Mode().Perm(); perm != envFileMode { + t.Errorf("server.env mode = %v, want %v", perm, os.FileMode(envFileMode)) + } + + // Idempotent: the menu calls this whenever it first sees a running server. + retireBootstrapPassword() + if vars, err := readServerEnv(); err != nil || vars["CIX_API_KEY"] != "cix_testkey" { + t.Errorf("second call damaged the file: %v %v", vars, err) + } +} + +// The wizard runs again after a database is deleted, and on that path it must +// not quietly revert settings chosen since the first run — the network toggle +// most of all, because reverting it changes what the machine exposes. +func TestSetDefaultKeepsExistingChoices(t *testing.T) { + vars := map[string]string{ + "CIX_BIND_ADDR": bindAllInterfaces, + "CIX_PORT": " ", // whitespace is not a choice + } + setDefault(vars, "CIX_BIND_ADDR", bindLocalOnly) + setDefault(vars, "CIX_PORT", "21847") + setDefault(vars, "CIX_VERSION_CHECK_ENABLED", "false") + + if vars["CIX_BIND_ADDR"] != bindAllInterfaces { + t.Errorf("bind addr = %q, want the existing choice kept", vars["CIX_BIND_ADDR"]) + } + if vars["CIX_PORT"] != "21847" { + t.Errorf("port = %q, want the blank value replaced", vars["CIX_PORT"]) + } + if vars["CIX_VERSION_CHECK_ENABLED"] != "false" { + t.Errorf("version check = %q, want the default filled in", vars["CIX_VERSION_CHECK_ENABLED"]) + } +} diff --git a/cli/launcher/menu_darwin.go b/cli/launcher/menu_darwin.go index 4ab0df1..76e92ee 100644 --- a/cli/launcher/menu_darwin.go +++ b/cli/launcher/menu_darwin.go @@ -6,6 +6,8 @@ import ( "os/exec" "path/filepath" "strings" + "sync" + "sync/atomic" "time" "fyne.io/systray" @@ -40,12 +42,47 @@ type menu struct { updater *updater + // busy is held while something is restarting the server, and it exists + // because two of these menu items are not independent. + // + // Start at Login and Allow Network Access both restart the server, each in + // its own goroutine, and each drives the same launchd label: one boots the + // job out while the other is waiting for its pid to disappear and then + // bootstraps it itself. Two bootstraps of one label leave either a failure + // or two processes racing for the port — from the outside, a server that + // went away and did not come back. + // + // Serialising them would only queue the second restart behind the first, + // which is not what anyone clicking meant. Refusing the click, and greying + // the control that would produce it, says what is actually happening. + busy atomic.Bool + + // retireOnce drops the bootstrap password from server.env the first time + // this process sees a running server. See retireBootstrapPassword. + retireOnce sync.Once + // detail holds the submenu rows under the server row. They are created // once and retitled on every render: systray has no way to remove an item, // so rebuilding the submenu per update would grow it without bound. detail [detailRows]*systray.MenuItem } +// beginBusy claims the right to restart the server. False means something else +// already has it, and the caller must do nothing. +func (m *menu) beginBusy() bool { + if !m.busy.CompareAndSwap(false, true) { + logf("ignored a menu action: another server operation is already running") + return false + } + m.render(m.poll.snapshotNow()) + return true +} + +func (m *menu) endBusy() { + m.busy.Store(false) + m.poll.refresh() +} + // detailRows is the fixed number of submenu slots. Rows with nothing to say are // hidden rather than left blank. const detailRows = 7 @@ -191,6 +228,19 @@ func (m *menu) render(s snapshot) { m.modelItem.Hide() } + // Anything that would restart the server is off the table while one restart + // is already under way, and while the server is on its way up: a cold start + // loads an embedding model and takes minutes, and a second restart landing + // in the middle of it is precisely what leaves the job wedged. + settling := m.busy.Load() || s.State == stateStarting + + if s.State == stateRunning { + // Bootstrap runs before the listener opens, so a server that answers is + // a server whose admin account exists — and the password that seeded it + // has no further use. + m.retireOnce.Do(retireBootstrapPassword) + } + switch { case !s.Managed: // Another installation owns the launchd label. Showing an enabled @@ -198,6 +248,9 @@ func (m *menu) render(s snapshot) { // the wrong behaviour; observing is useful, interfering is not. m.startStopItem.SetTitle("Start Server") m.startStopItem.Disable() + case m.busy.Load(): + m.startStopItem.SetTitle("Working…") + m.startStopItem.Disable() case s.State == stateRunning: m.startStopItem.SetTitle("Stop Server") m.startStopItem.Enable() @@ -226,17 +279,27 @@ func (m *menu) render(s snapshot) { m.autostartItem.Uncheck() } - if s.Managed { + if s.Managed && !settling { m.networkItem.Enable() m.autostartItem.Enable() - m.resetPWItem.Enable() } else { - // These live in files this app does not own — except the password - // reset, which only needs the database and is therefore still useful - // against an install-server.sh deployment. + // Not managed: these live in files this app does not own. Settling: both + // of them restart the server, and that is the click this guard exists to + // refuse. m.networkItem.Disable() m.autostartItem.Disable() - m.resetPWItem.Enable() + } + + // The password reset needs neither a running server nor a restart — it opens + // the database directly — so it stays available in both cases, including + // against an install-server.sh deployment. + m.resetPWItem.Enable() + + // An update restarts the server too, and downloads before it does. + if settling { + m.updateItem.Disable() + } else { + m.updateItem.Enable() } } @@ -251,6 +314,12 @@ func (m *menu) toggleAutostart() { if !s.Managed { return } + if !m.beginBusy() { + // The checkbox already flipped visually on the click; put it back. + m.render(s) + return + } + defer m.endBusy() enable := !s.Autostart if err := setAutostart(enable); err != nil { @@ -313,6 +382,14 @@ func (m *menu) checkForUpdates(explicit bool) { // leaving it with a llama sidecar from a different version. wasRunning := m.poll.snapshotNow().PID != 0 + // Claimed only now, not around the check: the check is HTTP and touches + // nothing, while installing stops and starts the server like the toggles do. + if !m.beginBusy() { + _ = alert("cix is busy", "Another server operation is still running. Try the update again once it has finished.") + return + } + defer m.endBusy() + quit, err := m.updater.install(av, wasRunning, m.setProgress) if err != nil { logf("update failed: %v", err) @@ -364,6 +441,10 @@ func (m *menu) toggleServer() { if !s.Managed { return } + if !m.beginBusy() { + return + } + defer m.endBusy() var err error if s.State == stateRunning { @@ -397,6 +478,11 @@ func (m *menu) toggleNetworkAccess() { _ = alert("cix", "cix is not set up yet.") return } + if !m.beginBusy() { + m.render(m.poll.snapshotNow()) + return + } + defer m.endBusy() wasRunning := m.poll.snapshotNow().State == stateRunning local := isLocalOnly(vars)