Reap fork children off the proxy loop#1816
Open
xeophon wants to merge 2 commits into
Open
Conversation
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 2cb75b6. Configure here.
ApprovabilityVerdict: Needs human review This PR changes concurrency behavior in fork child reaping - adding new locking, moving blocking operations to a ThreadPoolExecutor, and modifying cancellation semantics. Changes to process lifecycle management and async concurrency patterns warrant human review. You can customize Macroscope's approvability policy. Learn more. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

Overview
Move fork-child reaping and private-CWD deletion off the shared MCP proxy event loop while preserving the fork parent's single-threaded spawn boundary.
Why
A rollout close currently performs blocking
waitpid(..., 0)and recursivermtree(...)calls directly on the proxy loop. A metadata-heavy child directory can therefore pause heartbeats, timers, and requests to otherwise-ready rollout children for the full cleanup duration.Design
SIGKILLownership on the proxy loop.fork()can run while a cleanup thread exists.waitpidandrmtreework in one short-lived executor worker, then join that worker before releasing the fork lock.Performance and resources
A standalone PEP 723 benchmark reaped one real child and deleted an identical tree of 20,000 one-byte files across 21 directories. Three paired macOS/APFS runs produced these medians:
The optimization targets loop responsiveness rather than deletion throughput: the filesystem work remains the same, and wall time varies with metadata/cache behavior. The temporary worker is fully joined before another child may be forked. The workload opens no network connections, copies no bytes, and removes the same child process, files, directories, and payload in both modes.
Note
Medium Risk
Changes fork-child lifecycle and lock ordering around
fork()and directory deletion; mistakes could leak processes/dirs or race spawns, though scope is limited tomultiplex.pyreap.Overview
Fork-child teardown no longer blocks the MCP proxy loop. In
serve_forked'sreap, blockingwaitpidandshutil.rmtreeon the rollout's private CWD are moved to a single-workerThreadPoolExecutor, whileSIGKILL, map removal, and waiter wakeups stay on the asyncio loop.Cleanup is serialized with spawning by holding the existing
forkinglock for the full reap (including joining the executor), so a new child cannotfork()while a cleanup thread is still running and a directory is not deleted under a reused key.Cancellation still finishes cleanup: the executor future is awaited with
asyncio.shield, and onCancelledErrorthe code awaits the future before re-raising so teardown cannot orphan the private CWD.Reviewed by Cursor Bugbot for commit a0c743e. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Reap fork children off the proxy event loop in
serve_forkedreapcoroutine inmultiplex.pynow acquires the forking lock before reaping, serializing cleanup with concurrent fork operations.waitpidand directory removal are offloaded to aThreadPoolExecutorvialoop.run_in_executor, keeping the event loop unblocked.asyncio.shieldwraps the cleanup future so that cancellation of the coroutine still awaits full cleanup before re-raising.Macroscope summarized a0c743e.