fix(dev-mcp): sweep orphaned temp dirs left by killed processes - #6029
cristiansotogarciaxatech wants to merge 8 commits into
Conversation
themiguelamador
left a comment
There was a problem hiding this comment.
Requesting changes for three correctness issues:
-
P1 — Windows can delete a live process's directory after an unrelated probe failure. The current
OpenProcessbranch maps every failure other thanERROR_ACCESS_DENIEDtoDead. Errors such as resource exhaustion or transient system failures do not prove that the PID is absent, so this contradicts the module's fail-safe contract and can remove a live session's binaries. OnlyERROR_INVALID_PARAMETERshould establish a nonexistent PID; all other failures must remainUnknown. -
P1 — the new Unix test does not compile. It calls
nix::unistd::Uid::effective(), but this crate's nix dependency does not enable theuserfeature.cargo test -p buzz-dev-mcp sweep::tests -- --test-threads=1fails at compile time withcannot find Uid in unistd. -
P2 — that permission test would not exercise the claimed removal-error path anyway. Chmodding the target directory to 000 makes its marker unreadable first, so the sweep increments
skipped_no_markerand never reachesremove_dir_all. Injecting the remover gives deterministic coverage of the actual error branch and works across platforms/root CI.
I prepared local signed commit a78022beb that makes only the invalid-PID error deletable, covers other errors as unknown, and replaces the permission-dependent test with deterministic removal failure injection.
Validation on that commit:
cargo test -p buzz-dev-mcp -- --test-threads=1: 104 passedcargo clippy -p buzz-dev-mcp --all-targets --all-features -- -D warnings: passedcargo fmt --all -- --checkandgit diff --check: passed- Windows cross-check was attempted, but this macOS host lacks the MSVC SDK headers and
lib.exe; the extracted error classifier is host-tested.
5c43d10 to
a6b15a1
Compare
|
All three are real. Fixed in a6b15a1. 1. Windows error classification. You are right and this one is my fault for writing an I pulled the classification out into Worth noting the existing 2. The test did not compile. Confirmed, and I reproduced it rather than take it on trust. My local toolchain is 3. The test would not have covered the branch anyway. Also correct, and it is worse than not covering it. Chmod 000 on the target directory makes the marker unreadable first, so Replaced with an injected remover via VerificationCaveat, stated plainly because I would rather flag it than have you find it. My mingw toolchain broke today in a way unrelated to this change, so I could not run the full Signed-off-by: Cristian Soto Garcia cristian@xa-tech.com |
|
🤖 Request changes at exact head This cleanup is useful and needed: hard-killed Blocker 1: a dead MCP-server PID does not prove the directory is unusedThe marker records only the
The resulting sequence is possible: The deletion path is I reproduced this against the exact PR binary: after killing and reaping the parent, the shell child was still alive; the replacement startup removed the marked directory; the child's delayed shim invocation then failed with Proposed solution: make deletion depend on proof that every directory user has exited, not only the original server PID. A durable lease/lock held by the server and every spawned command is the clearest option; descendant/process-group ownership can also work if it remains valid across crash and restart. On Windows, check and handle failures from Required regression: start the real MCP server, launch a delayed command that later invokes a shim tool, hard-kill the server, restart it, and prove the live command retains the directory and succeeds. After that command exits, a later startup should reclaim the directory. Blocker 2: an unexpected marker type can block every MCP startupThis PR newly adds a synchronous scan before the server begins serving ( std::fs::read_to_string(dir.join(MARKER_FILE_NAME))at That call follows symlinks, accepts non-regular files, has no byte limit, and waits for EOF. The scanner validates that the outer entry is a directory, but it does not prove that the marker inside is a small regular file created by Buzz. I reproduced two failures with the exact PR binary in an isolated temp root:
In both cases the pre-startup scan blocked and the MCP server never became ready. A symlink to an unbounded device can similarly cause unbounded reads. This is a local denial-of-service/reliability issue, not remote code execution; on a shared Proposed solution:
The validation should be performed on the opened handle to avoid a check-then-replace race. Required tests: FIFO marker, symlink marker, device marker, and oversized regular marker. Each case must be skipped without blocking, and the real server must still become ready. Blocker 3: the committed tests do not protect the destructive policy or production wiringThe tests in The full package suite passed 103/103 even after each of these mutations:
That means the test suite can remain green if the fail-closed invariant is weakened or the feature is disconnected entirely. Proposed solution:
Verification
Bottom line: retain the cleanup feature, but do not merge this head. Strengthen ownership so a dead parent cannot invalidate a live command, harden marker reads as untrusted filesystem input, and add production-boundary tests that fail when either safety property or the real wiring is removed. |
|
All three are real. Fixed in 621c2d1. 1. A dead owner pid is not proof the directory is idleYou are right about the mechanism, and one detail makes it slightly worse than the sequence you wrote. So the marker is no longer the only thing that authorizes a delete. Every directory this crate creates now carries a lease as well, and removal needs both facts to be positive:
The lease is a file inside the directory. On Unix the server opens it, takes The order inside One residual I would rather flag than have you find. The inherited descriptor is a low number, so a surviving command that explicitly reuses that exact descriptor after the server has already died, something like Windows is deliberately not symmetric, and here is the argumentThe lease handle on Windows is opened denying First, the orphan case does not arise there. Second, inheriting the handle would make things worse rather than better. It would hold a file open inside the directory during normal shutdown, where a just-terminated child that has not finished exiting blocks That leaves the hole you pointed at, because the job object is only a guarantee if it was actually established. 2. The marker is untrusted input, so it is read like untrusted inputConfirmed and fixed, and the same treatment now applies to the lease file, which has identical exposure and would otherwise have reintroduced the blocking read you found.
Every one of those failures lands where a missing marker lands, which is leave the directory alone. Windows takes the same path minus I did not move the temp directories under a per-user owner-only parent. That is a real hardening idea and worth doing, but it relocates on-disk state for every existing deployment, it is orthogonal to this sweep once markers are parsed as hostile, and it deserves its own PR where it can be argued on its own merits rather than ride in behind a cleanup fix. 3. Tests that fail when the feature is disconnectedThis was the most useful part of your review. The old tests called What is there now:
Your mutation list, re-run against the new tests on
On the one you asked for that I did not build. Driving a real shell command through the MCP protocol to prove the production spawn path inherits the lease means speaking JSON-RPC over stdio and then racing the shell tool's own timeout, which kills the child the test needs alive. What I did instead exercises the property with a real second process and a real inherited descriptor. The gap between the two is VerificationRan, and these are the numbers I saw: Plus the Unix branch of Now the part I am not going to paper over. None of the Unix code in this commit has been executed anywhere yet. I develop on Windows, a full One thing I found on the way, which is why the Justfile is in the diff
That is how my last round shipped a Signed-off-by: Cristian Soto Garcia cristian@xa-tech.com |
|
🤖 Re-review at exact head This revision materially improves the original proposal: marker reads are hardened against FIFO/symlink hangs, Windows job errors are handled, deletion policy coverage is fail-closed, and real startup wiring tests were added. One production safety blocker remains on Unix. The cleanup decision assumes a surviving shell command retains the inherited lease descriptor (
This is not a theoretical coverage concern: the current Unix lease can classify live work as unused and delete its shim/session directories. Recommended fix before merge:
The cleanup feature is useful and needed, but the current head replaces a recoverable disk leak with a reproduced live-command failure. Please fix the lease lifetime and rerun the full package suite on Unix before merging. |
621c2d1 to
bb9a7d7
Compare
|
Both findings were real. Fixed at They have two different root causes though, and the difference matters, because only one of them was a defect in the code. 1. The failing test was broken, not the mechanism under it
I reproduced your run before changing a line, on a 2. The inherited descriptor was the real defect and it is goneYour probe is correct and I am not going to defend the design. An inherited descriptor lands in the command's own descriptor table and belongs to the command from that instant. What replaced itCommand lifetime now lives somewhere the command cannot reach. Each claimed directory holds a Nothing the command does to its descriptors, its environment or its signals can retract an entry. Buzz writes it and Buzz removes it. A server killed before it can remove one leaves the entry behind, which keeps the directory rather than losing it. Four things follow from that, and each is deliberate. Removal now needs three proofs, not two. The owner pid is confirmed dead, the owner's lease is confirmed unheld, and every registered command is confirmed gone. All twenty seven combinations are enumerated in The lease is demoted to what it can actually prove. It answers "is the owning server itself still alive", which is the pid question without the pid-reuse hole. The unit of liveness on Unix is the process group, not a pid. A command is a shell that forks, and the shell can exit while what it started keeps running with the shim dir on The registry is cross-platform even though Windows does not strictly need it. The job object still guarantees no orphan survives there, so this is belt and braces, but it means the registry code is executed on every platform instead of on one. I did not take the report-only option you offered for Unix. The proof is now executed rather than argued, below, so shipping a no-op on macOS and Linux would be giving up the platform where the leak actually hurts. One more thing came out of writing this. The kill guard and the registry entry were two separate bindings, and locals drop in reverse declaration order, so the entry naming a process group was being removed just before the guard killed that group. They are one tuple now, since tuple fields drop left to right, and there is a comment saying why ( The regression you asked for
The part I want to point at is the control. A canary orphan is planted immediately before the replacement starts, and the test waits for the canary to disappear before asserting anything. Without it "the directories are still there" is indistinguishable from "the sweep has not got to them yet", and the test would pass with the sweep deleted entirely. It is VerificationMy development machine is Windows and has no Unix on it. No WSL distro, no Linux container engine, and a cross build of the crate dies in Run 32479639441,
Green is cheap, so here is the part that is not. Run 32480277709 breaks the sweep three ways on purpose and asserts the suite notices.
The second one is your production probe, reproduced as a test and then caught by it. Two things I did not run. Fork pull request workflows on this repo still need a maintainer to approve them, so the checks on the PR itself are not something I can produce, which is why the runs above are fork-side. And Known boundsWorth stating rather than having you find them.
Signed-off-by: Cristian Soto Garcia cristian@xa-tech.com |
buzz-dev-mcp creates two tempfile::TempDir per server start (the shim dir with buzz/rg/tree/git-helper copies, and the session dir), whose cleanup runs in Drop. When the process is killed rather than exited gracefully, Drop never runs and the directories are orphaned - measured at 104 folders / 5.05 GB over 33 hours on one machine (block#6025). Each directory now gets a small ownership marker recording the creating process's pid and creation time. At startup, before creating its own directories, the server sweeps the system temp dir for buzz-dev-mcp-prefixed entries and removes one only when its marker's pid is positively confirmed dead (nix::sys::signal::kill on Unix, OpenProcess/GetExitCodeProcess on Windows). Every uncertain case - no marker, corrupt marker, pid alive, or liveness undeterminable - is left alone, since a session can legitimately run for days and an age-based sweep would risk deleting a live session's binaries out from under it. Directories that predate this change have no marker and are likewise left alone rather than guessed at with an age cutoff; the sweep is best-effort end to end and never fails startup. No new dependencies: liveness checks use nix (already a dependency, signal feature) on Unix and windows-sys (already a dependency, Win32_System_Threading feature) on Windows. Signed-off-by: Cristian Soto Garcia <cristian@xa-tech.com>
Review follow-up on three defects in the startup sweep. The Windows liveness probe mapped every OpenProcess failure except ERROR_ACCESS_DENIED to Dead. Resource exhaustion and transient kernel failures are not evidence that a pid is absent, so that could delete a live session's shim directory out from under it, contradicting the module's own fail-safe contract. Only ERROR_INVALID_PARAMETER, what Windows returns when no process object exists for the pid, now yields Dead. Everything else is Unknown. The classification moves into classify_open_process_error so it can be asserted on directly instead of requiring a real system error to reproduce. The Unix-only permission test did not compile. It called nix::unistd::Uid::effective(), but this crate pins nix with default-features = false and features = ["signal", "process"], and Uid is gated behind nix's "user" feature. No crate in the workspace enables it. Confirmed by compiling the same call against the same feature set: "could not find Uid in unistd ... the item is gated behind the user feature". That test would not have covered the branch it claimed even with the feature enabled. Chmodding the target directory to 000 makes its ownership marker unreadable first, so the sweep counts skipped_no_marker and returns before reaching removal, leaving its assert_eq!(stats.errors, 1) failing rather than merely vacuous. It is replaced with an injected remover through sweep_stale_dirs_with, which exercises the real removal-failure branch with no dependence on platform, uid, or filesystem behaviour. Two directories are failed rather than one, so the assertion proves the sweep continued past the first failure whatever order read_dir returns entries in. Signed-off-by: Cristian Soto Garcia <cristian@xa-tech.com>
…e owner pid A dead server pid does not prove the directory is idle. On Unix the shell tool spawns each command in its own process group, so a SIGKILL of the server leaves the command running with the shim dir still first on PATH, free to invoke buzz/rg/tree/the git helpers out of it later. The Drop guard that would have killed the group is exactly what a SIGKILL skips. Every directory this crate creates now carries a lease as well, a file the creating process holds flock(LOCK_SH) on with FD_CLOEXEC cleared, so every command it spawns inherits the descriptor. An flock belongs to the open file description rather than to a process, so the lease stays held while the server or any command descended from it is alive, and the kernel releases it when the last of them exits. Removal now needs both facts to be positive, owner pid confirmed dead and lease confirmed unheld. The lease is written before the marker, so a marked directory always has a lease behind it and a directory that could not be leased is never swept at all. Windows keeps the lease handle uninherited on purpose. Commands there live in a Job Object with KILL_ON_JOB_CLOSE, so a hard kill of the server takes the whole tree with it, and an inherited handle would instead block TempDir's own cleanup on the normal shutdown path and turn every clean exit into a leak. The job object return values were being dropped on the floor, so CreateJobObjectW, SetInformationJobObject and AssignProcessToJobObject are all checked now, and a failure surrenders the ownership claim rather than leaving behind a directory a later sweep could delete under a live command. The marker is also read as untrusted input now. It lives in a world-writable directory on a typical Unix box, and read_to_string on it follows symlinks, accepts any file type and waits for EOF, so a FIFO marker blocked startup before the server ever served. The read is now O_NOFOLLOW plus O_NONBLOCK, validated as a regular file on the opened handle rather than the path, capped at 256 bytes and bounded on the read itself. Every failure lands where a missing marker lands, which is leave the directory alone. Tests cover the policy across all nine owner/lease states, an orphaned command holding an inherited lease through the owner's death, FIFO and symlinked and oversized markers on a timeout so a regression fails instead of hanging, and a production-boundary test that drives the real binary through startup, a hard kill and a restart. buzz-dev-mcp joins test-unit because the Linux CI job never ran a single test from this crate, which is how a cfg(unix) test that did not compile shipped last round. Signed-off-by: Cristian Soto Garcia <cristian@xa-tech.com>
…escriptor the command owns The inherited-descriptor lease does not survive contact with a real command. It lands in the command's own descriptor table, where bash hands out 3 upward for its redirections and plenty of programs close everything above 2 on startup. Either drops the lease while the command runs on, and the sweep then reads a free lease and deletes a live command's shim directory. That is worse than the leak this PR set out to fix. Command lifetime now lives in a registry the command cannot reach: a .buzz-dev-mcp-cmds directory inside each claimed dir, one entry per running command, named by the command's process group on Unix and its pid on Windows. shell::run reserves the entry before the spawn, names it after, and drops it when the command is reaped. Nothing a command does to its descriptors, environment or signals can retract an entry, and a server killed before it can deregister leaves the entry behind, which keeps the directory instead of losing it. Removal now needs three independent proofs rather than two: the owner pid is dead, the owner's lease is unheld, and every registered command is gone. The lease is demoted to what it can actually prove, which is that the owning server itself is still alive, so FD_CLOEXEC is left alone and the descriptor no longer reaches any child. The group, not the pid, is the unit of liveness on Unix. A command is a shell that forks, and the shell can exit while what it started keeps running with the shim dir on PATH. killpg answers for the whole group, which is the same unit KillGroup already manages. a_command_that_outlives_its_owner_keeps_the_directory was not just failing on macOS, it was never testing what it claimed. It modelled the owner's death by dropping the lease guard, and nix's Flock::drop issues an explicit LOCK_UN (nix-0.31.3/src/fcntl.rs:1042) which releases the lock on the open file description the child shares, so the child's copy died with it. It now drives the registry the way shell::run does and leaks the entry with mem::forget, which is what a SIGKILL actually leaves behind. Signed-off-by: Cristian Soto Garcia <cristian@xa-tech.com>
The unit tests prove the policy and the wiring tests prove the policy is connected to something. Neither runs a command, and the failure this PR was sent back for was a command losing its shim directory, so neither would have caught it. This one is the whole scenario with nothing modelled by hand: a real server, a real shell tool call over MCP, a SIGKILL of that server while the command is still running, and a real replacement doing its startup sweep with the orphan alive. A canary orphan planted just before the replacement starts is the control, so "the directories are still there" cannot be confused with "the sweep has not run yet". The command reports back whether the shim binary it resolved before its server died is still executable, and a third server proves the directories are reclaimed once the command finally exits. Unix only by design. On Windows the command lives in a job object with KILL_ON_JOB_CLOSE, so killing the server kills the command and there is no orphan to protect. Signed-off-by: Cristian Soto Garcia <cristian@xa-tech.com>
A registry handle is a process group id on Unix, and a test process is not a group leader under a test runner, so killpg(getpid()) is ESRCH and two of the new tests asserted Busy against a handle that reads as dead. Both now use the process's own group. Signed-off-by: Cristian Soto Garcia <cristian@xa-tech.com>
Two separate bindings drop in reverse declaration order, so the registry entry was being removed while the process group it names was still up. One tuple fixes the order and says so, since the alternative is a comment nobody reads next to code that looks fine. Also gates the integration test's MCP helpers to Unix, where its only caller lives, and updates the Justfile note to the mechanism that replaced the inherited lock. Signed-off-by: Cristian Soto Garcia <cristian@xa-tech.com>
bb9a7d7 to
0f51819
Compare
🔐 Codex Security Review
|
Running `cargo fmt` outside hermit picked up rustfmt 1.9.0-stable (8bab26f4f6 2026-07-14) from the default stable toolchain, while the repo pins 1.95.0, whose rustfmt is 1.9.0-stable (59807616e1 2026-04-14). Same version string, different build, and they disagree on closure and method-chain wrapping. Six sites were red under the pinned build, one in sweep.rs and five in startup_sweep.rs. sweep.rs is whitespace only. startup_sweep.rs is whitespace plus one edit rustfmt makes on its own, dropping the braces around a single-expression closure and adding the trailing comma that goes with it. Stripped of all whitespace the two files differ from the reviewed head by exactly one opening brace, one closing brace and one comma. No behaviour change. Signed-off-by: Cristian Soto Garcia <cristian@xa-tech.com>
0f51819 to
abfca77
Compare
|
Rebased onto The correctionI told you on 17 and 19 August that fmt was clean on this crate. That was wrong, and the fault is my instrument rather than the toolchain. Bare Here is the red run, which I am linking rather than deleting: https://github.com/cristiansotogarciaxatech/buzz/actions/runs/33503349813 Fixed in the The conflictBoth sides append to the Nothing else moved. These four are byte-identical to the head you reviewed at
Green, on the pinned toolchainhttps://github.com/cristiansotogarciaxatech/buzz/actions/runs/33503601132 That run sits at
The pre-rebase head is parked at |
|
Scope note, because this PR is going to get triaged as a duplicate of #4732 and #5944 and it is not one. Those two both add The reason is Drop. Verified at
So here is what actually changes when one of those PRs lands. Today That turns an intermittent leak into a deterministic one, once per shutdown, on a path the harness takes on every respawn. Cause and residue, not two swings at the same defect. Kill the tree with #4732 or #5944, reclaim what the kill leaves behind with the startup sweep here. Landing the tree-kill on its own is the worst of the available outcomes and it would look like a fix while the disk keeps filling. |
Fixes #6025.
buzz-dev-mcpcreates twotempfile::TempDirper server start, the shim dir holding copies of buzz, rg, tree and the git helpers, and the session dir. Cleanup lives inDrop. These processes get killed rather than exiting gracefully, soDropnever runs and both directories are orphaned. On Windows a hard kill is not catchable at all, so there is no cleanup hook to hang this on in the first place. I measured 104 folders and 5.05 GB over 33 hours on one machine, at 97 MB per shim dir.The approach, and why it is not an age sweep
The obvious fix is "delete anything older than 24 hours" and it is wrong. Sessions legitimately run for days, and an age cutoff would rip the binaries and key file out from under a live one. Mine run far longer than a day.
So each directory now gets a small ownership marker,
.buzz-dev-mcp-owner, recording the creating process's pid and creation time. At startup, before creating its own directories, the server sweeps the system temp dir forbuzz-dev-mcp-prefixed entries and removes one only when the marker's pid is positively confirmed dead.Every uncertain case is left alone. No marker, corrupt marker, pid alive, or liveness undeterminable, all skipped. That failure direction is the safe one, worst case the leak persists for that directory. The sweep is best-effort end to end and never fails startup, including on a permission error against someone else's temp entry.
Liveness is
nix::sys::signal::kill(pid, None)on Unix andOpenProcess/GetExitCodeProcesson Windows. No new dependencies. Both crates were already dependencies here for the existingKillGrouptimeout-kill path. The Unix branch stays fully safe code, the Windows FFI sits behind#[allow(unsafe_code)]matching the existing pattern inshell.rsandshim.rs.Known limitation, stated up front
Directories that predate this change have no marker, so they are left alone rather than guessed at. This PR stops the leak going forward, it does not reclaim the existing backlog. Reclaiming that safely needs a separate opt-in tool, and I did not want to smuggle a destructive one-off into a bug fix. Happy to follow up with one if you want it.
Tests
Seven new tests in
sweep.rs, all pointed at atempfile::tempdir()and never at the real system temp dir.The dead pid is obtained deterministically by spawning and reaping a trivial child rather than guessing an unused number.
Verification
One caveat I would rather flag than hide. My local toolchain is
x86_64-pc-windows-gnu, not the-msvctarget CI uses, because MSVC Build Tools were not available on this machine. Thewindows-syscalls are target agnostic, but I could not verify against MSVC directly. I also could not cross-compile-check the Unix branch, so I reviewed it by hand, it only calls the safenixwrapper.Separately, two or three pre-existing
shell::tests::*cases flake under parallel execution in my sandbox. I reproduced the identical flake on unmodifiedmainwith the same command, so it is not from this change. Running with--test-threads=1is green.