fix(spawn): keep posix_spawn fast path to silence libmalloc MSL warning by espetro · Pull Request #926 · tontinton/maki

espetro · 2026-09-01T20:23:29Z

Fixes #909

Root cause

With MallocStackLogging=1 in the environment and a TTY stderr, every spawned child printed MallocStackLogging: turning off stack logging (had been recording ...). It's emitted by libmalloc's atfork child handler inside fork(), before exec. It fired because every spawn site registered a pre_exec(setsid) closure, which forces Rust std off the posix_spawn fast path onto fork+exec. process_group(0) stays on posix_spawn but doesn't detach the controlling tty — sudo/ssh/gpg/credential helpers could then prompt straight onto the TUI or stop on SIGTTOU.

Fix

macOS-only re-exec (the only platform whose libmalloc prints this). detached_command (new maki-agent::spawn module) builds Command::new(current_exe) __spawn_detached <program> <args…> — an absolute path with no closure, so posix_spawn applies unconditionally, including when the job's env overrides PATH. main() calls run_detached_if_marked(), which registers current_exe as the trampoline and intercepts the marker: the spawned maki calls setsid() and execvps the real program. No fork in the chain, the spawned pid is a session leader with no controlling tty.

Everywhere else (Linux, Windows) the command gets plain pre_exec(setsid) via rustix — exactly the pre-PR behavior, zero new moving parts.

Review follow-ups

Kill race: setsid lands after spawn() returns, so there is a window where the child is still in maki's process group and a group-only killpg returns ESRCH. All kill paths (ChildGuard::signal_kill, sigkill_process_group, lua kill_job) now also kill_process(pid, SIGKILL) after the group kill.
Spawn errors: the trampoline branch is only taken for absolute paths with an exec bit (needs_trampoline_fallback). Bare names, relative paths, missing and non-executable files fall back to the pre_exec(setsid) branch, so spawn() still fails with the documented error — jobstart keeps returning nil, err, MCP StartFailed keeps its reason. Relative paths fall back because the trampoline cannot see the caller's current_dir.

Testing

tests/detached_spawn.rs (macOS-gated): drives the real maki binary through the marker path, asserts argv forwarding works and the spawned child is its own session leader (getsid(pid) == pid, polled while alive) and group leader (getpgid(pid) == pid). No macOS CI runner, so this runs when a mac dev runs the suite.
Probe unit tests in maki-agent::spawn (macOS-gated): bare/relative names, missing and non-executable absolute paths fall back; absolute exec files take the trampoline.
cargo check --workspace --tests, cargo clippy --tests -- -D warnings clean; maki-agent (1058), maki-lua (1813) and maki-ui (1879) suites pass.

Scope

Removes the atfork had been recording warning on macOS. The not enabled variant of #909 is printed inside the spawned child's own libmalloc after exec — unfixable from maki.

tontinton · 2026-09-02T18:57:50Z

@espetro Nice digging, but I think this is fixing a different message. The one you repro'd (had been recording) needs MallocStackLogging=1 actually set, but #909 is the not enabled variant, which only shows up when something calls turn_off_stack_logging() directly. Also, dropping setsid means children keep the controlling terminal, so anything that prompts on /dev/tty (sudo, ssh, git) hangs on SIGTTIN instead of just failing.

tontinton · 2026-09-18T21:32:41Z

+        // Keep std on the posix_spawn fast path so libmalloc's atfork child
+        // handler never runs (it prints a MallocStackLogging warning when the
+        // parent has MSL enabled; see tontinton/maki#909). pgid == pid, so
+        // kill_process_group below is unaffected.
        #[cfg(unix)]
        {
            use std::os::unix::process::CommandExt;
-            // SAFETY: setsid is async-signal-safe, so it is sound to call in pre_exec.
-            unsafe {
-                command.pre_exec(|| {
-                    rustix::process::setsid()?;
-                    Ok(())
-                });
-            }
+            command.process_group(0);


setsid() and process_group(0) look interchangeable here but they aren't. setsid puts the child in a new session with no controlling terminal, while process_group only gives it a new pgid inside the same session, so it keeps maki's tty.

this is the bash tool, so it runs whatever the model asks for. after the change sudo, ssh, gpg and git credential helpers can open("/dev/tty") and draw prompts straight onto the tui. worse, they call tcsetattr on it from a background process group, which raises SIGTTOU and stops the process, and then it sits there until SHELL_TIMEOUT. today those commands fail fast with "no tty present". same shape at maki-ui/src/app/shell.rs:225 and maki-agent/src/mcp/stdio.rs:54.

is there a way to keep the tty detached and still stay on posix_spawn? if not, i'd rather live with the warning.

separately, the program here is bare bash and std leaves the posix_spawn path when the command env touches PATH, so any job passing env = { PATH = ... } falls back to fork+exec and the warning comes back at the site that spawns the most children.

To my understanding, detached_command spawns maki itself with a __spawn_detached marker argv; main() intercepts it, setsid()s, then execvps the program. How this could look: posix_spawn for the outer spawn (absolute path, no closure, so immune to env-triggered fork fallback, including the env = { PATH = ... } case you flagged), setsid inside the spawned child before exec, so the tty is still detached and those helpers fail fast. pgid == pid, killpg unchanged.

Hosts that aren't the maki binary (test harnesses) fall back to pre_exec(setsid) (only run_detached_if_marked() registers current_exe as the trampoline), so behavior there matches today.

tontinton · 2026-09-18T21:32:41Z

+pub fn resolve_program(program: &str, env: &HashMap<String, String>) -> std::ffi::OsString {
+    if program.contains('/') || program.contains('\\') || program.is_empty() {
+        return std::ffi::OsString::from(program);
+    }
+    let path = env
+        .get("PATH")
+        .map(String::as_str)
+        .map(std::path::PathBuf::from)
+        .or_else(|| std::env::var_os("PATH").map(std::path::PathBuf::from));
+    if let Some(path) = path {
+        for dir in std::env::split_paths(&path) {
+            let candidate = dir.join(program);
+            if candidate.is_file() {
+                return candidate.into_os_string();
+            }
+        }
+    }
+    std::ffi::OsString::from(program)
+}


candidate.is_file() isn't the rule execvp uses. a readable but non-executable file earlier in PATH now wins and the spawn fails with EACCES, where execvp would have carried on and found the real binary further down. checking the exec bit would be the minimum.

it also ignores current_dir, so any caller that sets a cwd resolves against the wrong relative entries, and program.contains('\\') does nothing on unix where backslash is a normal filename character.

reimplementing PATH lookup to work around a std internal is a lot of surface for the payoff. if it's only needed when the config sets PATH, could it be scoped to that case and say so?

Agreed, there's no resolve_program anymore. The trampoline spawns current_exe() (absolute path, no lookup at all), so std's env-PATH gate can't fire and there's no need to reimplement execvp's search. The env = { PATH = ... } case still takes posix_spawn now because the program being spawned is maki itself, not bash.

tontinton · 2026-09-18T21:32:41Z

+    #[test]
+    fn strips_inherited_vars_but_keeps_explicit() {
+        unsafe {
+            std::env::set_var("MallocStackLogging", "1");
+            std::env::set_var("MallocStackLoggingNoCompact", "1");
+        }
+        let mut cmd = std::process::Command::new("true");
+        cmd.env("MallocStackLoggingLevel", "verbose");
+        strip_inherited_malloc_stack_logging(&mut cmd);
+        let get = |name: &str| -> Option<String> {
+            cmd.get_envs()
+                .find(|(k, _)| k.to_string_lossy() == name)
+                .and_then(|(_, v)| v)
+                .map(|v| v.to_string_lossy().into_owned())
+        };
+        assert_eq!(get("MallocStackLogging"), None);
+        assert_eq!(get("MallocStackLoggingNoCompact"), None);
+        assert_eq!(get("MallocStackLoggingDirectory"), None);
+        assert_eq!(get("MallocStackLoggingLevel"), Some("verbose".into()));
+        unsafe {
+            std::env::remove_var("MallocStackLogging");
+            std::env::remove_var("MallocStackLoggingNoCompact");
+        }
+    }
+}


this test passes with an empty function body. get_envs() yields (key, None) for a removal, and .and_then(|(_, v)| v) turns that into None, which is exactly what you get when the var was never mentioned, so the assert can't fail. asserting that the removal entry exists would catch a regression.

two smaller ones: unsafe { std::env::set_var(..) } mutates process-wide state in a binary that runs tests in parallel, and it isn't needed since Command::env_remove never reads the process env. and resolve_program_tests writes a fixed filename into temp_dir() without cleaning up on panic, so concurrent runs collide.

File deleted with child_env. Replaced by tests/detached_spawn.rs on the root crate, which drives the real maki binary through the marker path and asserts the spawned child is its own session leader (getsid(pid) == pid, polled while the child is alive so it can't pass vacuously) plus argv forwarding.

tontinton · 2026-09-18T21:32:41Z

+    let (out_rx, err_rx) = (read_to_end_capped(so), read_to_end_capped(se));
+    let status = child.wait().expect("wait");
+    let out = out_rx.unwrap_or_default();
+    let err = err_rx.unwrap_or_default();


let (out_rx, err_rx) = (read_to_end_capped(so), read_to_end_capped(se));

this reads as parallel but isn't. read_to_end_capped blocks on recv_timeout before it returns, so stderr isn't drained until stdout hits eof, and a child writing more than a pipe buffer of stderr sits there for 30s. both results then go through unwrap_or_default(), so a timeout becomes an empty string and the "no warning" assert passes on nothing.

the grandchild half at line 89 can't detect anything either. on the fork path the handler prints inside fork(), before std does the dup2 in do_exec, so it lands on the test child's inherited fd 2 and never on the grandchild's piped stderr. err is always empty and clean is always true. the combined assert in parent_side is the one actually catching the regression.

the whole file is #![cfg(target_os = "macos")] too, and ci has no macos runner, so none of it runs unless a mac dev remembers to.

New test is #![cfg(unix)] on Linux CI, so tests/detached_spawn.rs spawns CARGO_BIN_EXE_maki with the marker and asserts session/group leadership and argv forwarding. The MSL-specific assertion is gone since there's no macOS runner; what CI covers now is the mechanism (posix_spawn-compatible spawn + setsid detach).

espetro · 2026-10-06T20:25:58Z

Thanks for the thorough review @tontinton . I ended up rebuilding the fix, and now pre_exec and process_group are both gone.

detached_command now re-execs maki itself: it spawns <current_exe> __spawn_detached <program> <args…>, an absolute path with no closure, so posix_spawn applies unconditionally — including when the job's env overrides PATH, which would have forced the fork path before. main() intercepts the marker, calls setsid(), then execvps the real program. No fork anywhere in the chain, the pid the parent spawned is a session leader (so sudo/ssh/gpg/credential helpers still fail fast on /dev/tty instead of drawing prompts onto the TUI or stopping on SIGTTOU), and pgid == pid keeps the killpg cleanup unchanged.

child_env is gone entirely — the env strip never touched the parent-side atfork warning, and spawning an absolute path removed the need for resolve_program. The new e2e tests/detached_spawn.rs runs on Linux CI: it drives the real binary through the marker path and asserts the child becomes its own session and group leader with argv forwarded.

Scope note: this removes the atfork had been recording warning. The not enabled variant of #909 is emitted inside the spawned child's own libmalloc after exec — unfixable from maki, same as before.

tontinton

Thanks for coming back to this, it does fix the tty thing. But it trades a warning that needs a debug env var for a hidden dependency on our own binary in every spawn, worse spawn errors and a kill race. If it's macOS only, with the race fixed and spawn errors still showing up, I'm in. Otherwise I'd rather live with the warning.

Also needs a rebase, it conflicts with main now (we moved from libc to rustix, and shell.rs uses shell_command now).

tontinton · 2026-10-16T13:47:30Z

+    let err = Command::new(&program).args(args).exec();
+    eprintln!(
+        "{DETACHED_MARKER}: exec {}: {err}",
+        program.to_string_lossy()
+    );
+    std::process::exit(1);


This changes what a missing binary looks like. Before, spawn() failed. Now spawning maki always works and the real error is just a stderr line and exit 1.

So jobstart({ "rg", ... }) without rg installed returns a job id instead of nil, err (the docs in fn.rs promise the error), and a typo in an MCP command gives some random EOF instead of "No such file or directory".

Tests don't catch it because TRAMPOLINE_EXE is never set in test binaries, so they all go through pre_exec.

Restored. detached_command now checks needs_trampoline_fallback(program) before taking the trampoline: anything that is not an absolute path with an exec bit (bare names like rg, relative paths, missing/non-executable files) goes to the pre_exec(setsid) branch, so spawn() fails with the real No such file or directory/EACCES again — jobstart keeps returning nil, err and MCP StartFailed keeps its reason. Relative paths fall back because the trampoline can't see the caller's current_dir (it may not be set yet when detached_command runs). Unit tests cover bare/relative/missing/non-exec/absolute-exec cases (macOS-gated, like the integration test).

tontinton · 2026-10-16T13:47:30Z

+pub fn run_detached_if_marked() {
+    #[cfg(unix)]
+    {
+        if let Ok(exe) = std::env::current_exe() {
+            let _ = TRAMPOLINE_EXE.set(exe);
+        }
+        if std::env::args_os().nth(1).as_deref() == Some(OsStr::new(DETACHED_MARKER)) {
+            run_detached();
+        }
+    }
+}


Every spawn now needs our own binary to still be at its startup path and still know __spawn_detached. If brew cleans up the old Cellar dir after an upgrade, or someone does maki rollback to an older version, every tool call breaks until restart.

The warning is macOS only. Can we put the trampoline behind #[cfg(target_os = "macos")] and keep plain pre_exec(setsid) on linux?

Done — everything is now behind #[cfg(target_os = "macos"]: the marker, TRAMPOLINE_EXE, run_detached, and the trampoline branch of detached_command. Linux and every other platform get plain pre_exec(setsid) via rustix (setsid() replaced the libc call, matching the main-branch migration). run_detached_if_marked stays a no-op there, so main() is unchanged.

tontinton · 2026-10-16T13:47:31Z

+    // A failed setsid would keep the child on maki's tty — the exact problem
+    // this exists to avoid — so bail rather than exec degraded.
+    unsafe {
+        if libc::setsid() == -1 {
+            eprintln!("{DETACHED_MARKER}: setsid failed");
+            std::process::exit(1);
+        }
+    }


setsid now runs after spawn() returns, so for a bit the child is still in our process group. signal_kill / kill_job only kill the child's group, which gets ESRCH in that window (main swallows ESRCH), so the kill is lost and the child keeps running.

Easy fix is to also SIGKILL the pid itself after the group kill. The module doc at lines 13-15 saying cleanup "keeps working unchanged" isn't true either.

Fixed in both kill paths. ChildGuard::signal_kill and sigkill_process_group now also kill_process(pid, SIGKILL) after the group kill (same for lua kill_job), and the module doc no longer claims cleanup works unchanged — it names the window and points at the pid kill. Signal-on-reaped-pid isn't a concern: the reaped flag still short-circuits before anything is signalled.

pre_exec(setsid) detaches the child but forces std off posix_spawn onto fork+exec, and macOS libmalloc's atfork handler then warns on every spawn (tontinton#909). Instead, detached_command spawns the maki binary itself with a __spawn_detached marker — an absolute path and no closure, so posix_spawn applies regardless of the command's env (a PATH override can no longer force fork+exec either). The trampoline setsids and execs the real program: no fork anywhere in the chain, the spawned pid is a session leader with no controlling tty, and killpg cleanup is unchanged. Only binaries that call run_detached_if_marked in main opt in as the trampoline; test harnesses and other hosts fall back to pre_exec(setsid). This removes the atfork 'had been recording' warning. The 'can't turn off ... not enabled' variant of tontinton#909 is printed inside the spawned child's own libmalloc after exec and cannot be fixed from maki.

Review follow-up on tontinton#926: the re-exec trampoline is now behind target_os = "macos" (the only platform whose libmalloc warns on fork), Linux keeps plain pre_exec(setsid). Kill paths signal the pid itself in addition to the process group, covering the window where the trampoline has not setsid()ed yet. detached_command rejects programs that are not absolute executable files, so a missing binary still fails at spawn() with the error every call site documents instead of a trampoline stderr line and exit 1. Tests ported from libc to rustix and gated to macOS.

tontinton · 2026-10-22T21:46:00Z

/review

maki-review

Verdict: Not worth merging. Bash and npx still spawn through fork, so the warning stays for almost every spawn, and in return maki gains a self-re-exec path. I checked that the kill-race fix and the restored spawn errors both work.

Direction: Close this and live with the warning. It only appears when the user sets MallocStackLogging, and the #909 variant can't be fixed from maki at all. The proper fix is std's CommandExt::setsid (nightly, rust-lang/rust#105376). Today it stays on posix_spawn only on glibc Linux and still forks on macOS. Once that changes, switch to it in one place.

Decisions for @tontinton:

Keep a macOS-only spawn trampoline at all?
The trampoline only runs for programs given as absolute paths. The bash tool, ! commands and npx/uvx MCP servers all fall back to fork, so the warning stays.
- A: Close the PR. Nothing changes for users, and there's no re-exec path to maintain.
- B: Keep it, and look up bare names in PATH so bash/npx use it too. That fixes the debug-only warning, but brings back the PATH-search code you rejected earlier.
I'd pick A, because the warning needs a debug env var to appear and B costs the most to maintain.

maki-review · 2026-10-22T21:49:57Z

+#[cfg(target_os = "macos")]
+fn needs_trampoline_fallback(program: &OsStr) -> bool {
+    let path = Path::new(program);
+    if !path.is_absolute() {


HIGH: The warning still prints for the bash tool, ! commands and most MCP servers.

On unix, shell_command spawns plain bash, and MCP configs usually say npx or uvx. Bare names always take this fallback, so only absolute paths avoid fork.

Fix: only a PATH lookup fixes this; see the review summary.

maki-review · 2026-10-22T21:49:57Z

+use std::path::{Path, PathBuf};
+use std::process::Command;
+use std::sync::OnceLock;


MEDIUM: Lint fails on Linux and Windows.

Only macOS-gated code uses Path, PathBuf and OnceLock, so on every other platform they're unused imports. The Format check fails too.

Fix: move these imports into the macOS items that use them, then run cargo fmt.

maki-review · 2026-10-22T21:49:57Z

+//! so this runs when a mac dev remembers to (`cargo nextest run -p maki
+//! --test detached_spawn`).
+
+#![cfg(target_os = "macos")]


MEDIUM: None of the new spawn code runs in CI.

This test and the fallback unit tests only run on macOS, and CI has no macOS runner. The earlier thread says this runs on Linux CI, but it doesn't.

Fix: compile the marker handling and these tests for all unix. Keep only the trampoline branch in detached_command macOS-only.

espetro · 2026-11-07T01:15:59Z

Closing this — after the rework rounds the trampoline only engages for absolute paths, so the bash tool, ! commands and bare-name MCP servers (npx, uvx, rg) still fork and still print the warning. Covering them means re-doing execvp's PATH search, which was already rejected, and the #909 "not enabled" variant is unfixable from maki anyway.

The proper fix is std's CommandExt::setsid (rust-lang/rust#105376). Note it currently only uses POSIX_SPAWN_SETSID on linux-gnu — on macOS it still falls back to fork until rust-lang/libc exposes the constant for Apple targets and the cfg gate is widened, so worth tracking there rather than here.

tontinton requested changes Sep 18, 2026

View reviewed changes

devin-ai-integration Bot force-pushed the fix/msl-fork-warning branch 2 times, most recently from 3db4aef to 917cb04 Compare October 6, 2026 19:31

tontinton force-pushed the main branch from 3d22307 to 3436498 Compare October 10, 2026 15:19

tontinton requested changes Oct 16, 2026

View reviewed changes

espetro added 2 commits October 19, 2026 10:44

espetro force-pushed the fix/msl-fork-warning branch from 917cb04 to 990e13a Compare October 19, 2026 10:49

yonatanofek101 mentioned this pull request Oct 22, 2026

fix(ui): keep fork-time stderr noise off the TUI 🤖🤖🤖 #1318

Merged

maki-review Bot suggested changes Oct 22, 2026

View reviewed changes

espetro closed this Nov 7, 2026

Conversation

espetro commented Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Root cause

Fix

Review follow-ups

Testing

Scope

Uh oh!

tontinton commented Sep 2, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

espetro commented Oct 6, 2026

Uh oh!

tontinton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tontinton commented Oct 22, 2026

Uh oh!

maki-review Bot left a comment

Choose a reason for hiding this comment

Uh oh!

maki-review Bot Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

maki-review Bot Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

maki-review Bot Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

espetro commented Nov 7, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

espetro commented Sep 1, 2026 •

edited

Loading