fix: fork before keychain access to prevent SIGBUS on macOS daemon start

[chanyeinthaw]

Summary

Fix daemon mode silently crashing on macOS. spacebot start (daemon mode) exits immediately with SIGBUS because bootstrap_secrets_store() initializes CoreFoundation via the macOS Keychain before fork(), and CoreFoundation state is not safe to use after fork().

Root Cause

The startup sequence was:

1. bootstrap_secrets_store() → calls security_framework::passwords::get_generic_password → initializes CoreFoundation
2. load_config() → needs secrets store for secret: reference resolution
3. daemonize() → fork() → child receives SIGBUS (KERN_PROTECTION_FAILURE)

macOS crash reports confirmed:

CoreFoundation: ["*** single-threaded process forked ***"]
libsystem_c.dylib: ["crashed on child side of fork pre-exec"]

The crash was in _os_log_preferences_refresh → CFPrefsSource, triggered by CoreFoundation detecting tainted state post-fork. Foreground mode (-f) was unaffected because it skips fork().

Key Changes

Reorder startup to fork before Keychain access

Move bootstrap_secrets_store() and load_config() to after the daemonize() call. The fork now happens before any CoreFoundation/Security framework initialization, so the child process starts clean.

To make this possible, the instance_dir (needed for the PID file path) is resolved via a new lightweight resolve_instance_dir() helper that determines the path from the config file location or the default (~/.spacebot) without loading the full config or touching the Keychain.

New startup order (daemon mode)

1. resolve_instance_dir()        — lightweight, no CoreFoundation
2. onboarding (if needed)        — interactive, needs terminal
3. daemonize()                   — fork() with clean CF state
4. bootstrap_secrets_store()     — Keychain access now safe in child
5. load_config()                 — secrets store available
6. build tokio runtime + run()

Foreground mode is unaffected — no fork occurs, so the ordering doesn't matter.

Files Changed

File	What changed
src/main.rs	Restructured cmd_start to fork before secrets/config. Added resolve_instance_dir() helper.

Testing

• cargo check --all-targets passes
• cargo fmt --all clean
• cargo clippy --all-targets passes (via just gate-pr)
• Verified spacebot start + spacebot status works on macOS (Apple Silicon, macOS 26.2)
• Verified spacebot start -f still works
• No more SIGBUS crash reports in ~/Library/Logs/DiagnosticReports/

Note

Core Fix: The startup sequence was reordered to call daemonize() (fork) before any Keychain/CoreFoundation initialization. Previously, bootstrap_secrets_store() was called before fork, causing the child process to inherit a tainted CoreFoundation state, which the kernel rejected with SIGBUS. A lightweight resolve_instance_dir() helper now provides the instance directory path needed for daemon file setup without touching the Keychain. This single-file change (46 lines added/removed in src/main.rs) ensures the daemon starts cleanly on macOS while maintaining foreground and test behavior.

_{Written by Tembo for commit 9274342. This will update automatically on new commits.}

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d520beff-82e5-4c12-ab86-02d84ecc038c

📥 Commits

Reviewing files that changed from the base of the PR and between 9274342 and abcd98d.

📒 Files selected for processing (1)

• src/main.rs

---

Walkthrough

Reworked startup/daemonization flow: added resolve_instance_dir(config_path) to derive the daemon instance directory early, changed fork ordering to pre-fork when running in background so config loading and secret access occur in the correct process, and adjusted related paths and comments.

Changes

Cohort / File(s)	Summary
Daemon startup & runtime logic src/main.rs	Added resolve_instance_dir() helper; changed start/daemonization flow to fork earlier for background starts, moved instance-dir-based initialization (DaemonPaths, PID/daemon files) to use resolved path; reordered config loading and secret store/bootstrap calls and updated comments about Keychain/CoreFoundation and Tokio safety.
Manifest Cargo.toml	Minor manifest edits (lines changed) accompanying the startup refactor.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: reordering the startup sequence to fork before Keychain access, directly addressing the SIGBUS crash on macOS daemon startup.
Description check	✅ Passed	The description is comprehensive and directly related to the changeset, explaining the root cause of the SIGBUS crash, the fix approach, and the new startup sequence.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/main.rs (1)

217-223: ⚠️ Potential issue | 🔴 Critical

Path mismatch between "already running" check and daemonization when custom config is provided.

Line 217 uses DaemonPaths::from_default() which always derives paths from Config::default_instance_dir(). However, line 248-249 uses resolve_instance_dir(&resolved_config_path) which derives from the config file's parent directory when a custom -c path is provided.

This causes a mismatch:

• spacebot start -c /custom/path/config.toml checks PID at ~/.spacebot/spacebot.pid
• But writes PID to /custom/path/spacebot.pid

The daemon may start multiple times or fail to detect an already-running instance.

🐛 Proposed fix: Use resolve_instance_dir consistently

 fn cmd_start(
     config_path: Option<std::path::PathBuf>,
     debug: bool,
     foreground: bool,
 ) -> anyhow::Result<()> {
-    let paths = spacebot::daemon::DaemonPaths::from_default();
-
-    // Bail if already running
-    if let Some(pid) = spacebot::daemon::is_running(&paths) {
-        eprintln!("spacebot is already running (pid {pid})");
-        std::process::exit(1);
-    }
-
     // Run onboarding interactively before daemonizing
     let resolved_config_path = if config_path.is_some() {
         config_path.clone()
     } else if spacebot::config::Config::needs_onboarding() {
         // Returns Some(path) if CLI wizard ran, None if user chose the UI.
         spacebot::config::run_onboarding().with_context(|| "onboarding failed")?
     } else {
         None
     };

+    // Resolve instance directory early — needed for PID file path before daemonizing
+    let instance_dir = resolve_instance_dir(&resolved_config_path);
+    let paths = spacebot::daemon::DaemonPaths::new(&instance_dir);
+
+    // Bail if already running
+    if let Some(pid) = spacebot::daemon::is_running(&paths) {
+        eprintln!("spacebot is already running (pid {pid})");
+        std::process::exit(1);
+    }
+
     if !foreground {
         // Fork BEFORE touching the macOS Keychain or any CoreFoundation API.
         // ...
-        let instance_dir = resolve_instance_dir(&resolved_config_path);
-        let paths = spacebot::daemon::DaemonPaths::new(&instance_dir);
         spacebot::daemon::daemonize(&paths)?;
     }

Also applies to: 248-250

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main.rs` around lines 217 - 223, The "already running" PID check uses
DaemonPaths::from_default(), causing a mismatch when a custom config path is
provided; change the initial path derivation so DaemonPaths is built from the
resolved instance directory (use resolve_instance_dir(&resolved_config_path)
when a config path is supplied) before calling is_running(&paths), so the PID
check and PID file written during daemonization use the same instance dir;
ensure any later code that currently calls DaemonPaths::from_default() (or
constructs paths from default) is replaced to construct DaemonPaths from the
resolved instance dir consistently.

🧹 Nitpick comments (1)

src/main.rs (1)

282-293: Consider reusing resolve_instance_dir in bootstrap_secrets_store to reduce duplication.

The logic at lines 1172-1178 in bootstrap_secrets_store duplicates this function:

let instance_dir = if let Some(path) = config_path {
    path.parent()
        .map(|p| p.to_path_buf())
        .unwrap_or_else(|| std::path::PathBuf::from("."))
} else {
    spacebot::config::Config::default_instance_dir()
};

♻️ Proposed refactor in bootstrap_secrets_store

 fn bootstrap_secrets_store(
     config_path: &Option<std::path::PathBuf>,
 ) -> Option<Arc<spacebot::secrets::store::SecretsStore>> {
     // Probe kernel keyring support before any workers spawn.
     spacebot::secrets::keystore::probe_keyring_support();

-    let instance_dir = if let Some(path) = config_path {
-        path.parent()
-            .map(|p| p.to_path_buf())
-            .unwrap_or_else(|| std::path::PathBuf::from("."))
-    } else {
-        spacebot::config::Config::default_instance_dir()
-    };
+    let instance_dir = resolve_instance_dir(config_path);

     let data_dir = instance_dir.join("data");

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main.rs` around lines 282 - 293, The duplicated logic in
bootstrap_secrets_store that computes instance_dir should be replaced by calling
the existing resolve_instance_dir function to avoid duplication; locate the
computation in bootstrap_secrets_store (the if let Some(path) = config_path {
... } else { ... } block) and swap it for a single call to
resolve_instance_dir(config_path), ensuring the returned std::path::PathBuf is
used the same way and that imports/signatures remain compatible with
resolve_instance_dir.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@src/main.rs`:
- Around line 217-223: The "already running" PID check uses
DaemonPaths::from_default(), causing a mismatch when a custom config path is
provided; change the initial path derivation so DaemonPaths is built from the
resolved instance directory (use resolve_instance_dir(&resolved_config_path)
when a config path is supplied) before calling is_running(&paths), so the PID
check and PID file written during daemonization use the same instance dir;
ensure any later code that currently calls DaemonPaths::from_default() (or
constructs paths from default) is replaced to construct DaemonPaths from the
resolved instance dir consistently.

---

Nitpick comments:
In `@src/main.rs`:
- Around line 282-293: The duplicated logic in bootstrap_secrets_store that
computes instance_dir should be replaced by calling the existing
resolve_instance_dir function to avoid duplication; locate the computation in
bootstrap_secrets_store (the if let Some(path) = config_path { ... } else { ...
} block) and swap it for a single call to resolve_instance_dir(config_path),
ensuring the returned std::path::PathBuf is used the same way and that
imports/signatures remain compatible with resolve_instance_dir.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d8fbe981-f864-415f-b25a-c2f04457f2f6

📥 Commits

Reviewing files that changed from the base of the PR and between 6e5bff1 and 9274342.

📒 Files selected for processing (1)

• src/main.rs