-
Notifications
You must be signed in to change notification settings - Fork 6k
mcp(win): full Windows Job Object cleanup wiring (patch draft) #9835
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
3087622
mcp(windows): add Windows Job Object cleanup scaffolding; patch to at…
michaelneale 1e38819
mcp(windows): fix broken subprocess.rs import header (remove literal …
michaelneale 89e9f22
mcp(windows): fix broken imports in subprocess.rs; gate Windows code …
michaelneale e85430a
mcp-windows: remove stale crates/goose-mcp/windows_job.rs duplicate; …
michaelneale a308159
mcp-windows: replace duplicates with stub to satisfy compile while Go…
michaelneale 90c946d
windows: extend winapi features for Windows target (jobapi2, winbase,…
michaelneale a6b8dd4
mcp: remove Windows job imports from subprocess.rs; MCP path no longe…
michaelneale cff4229
windows: bail if SetInformationJobObject fails, dont publish partial …
michaelneale 71d4660
ci: trigger re-run after Windows fix
michaelneale 1aa1e6e
ci: trigger re-run after Windows fix
michaelneale 03f0909
windows: request PROCESS_SET_QUOTA and check job assignment result
02151fe
Merge remote-tracking branch 'origin/main' into micn/mcp-win-cleanup
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| // Stub Windows Job Object helpers for MCP path (no-ops on Windows). | ||
| // This file provides the minimal surface required by crates/goose-mcp/src/subprocess.rs | ||
| // when built on Windows. The real Windows Job Object integration lives in the goose crate. | ||
|
|
||
| #[cfg(windows)] | ||
| pub type HANDLE = *mut std::ffi::c_void; | ||
|
|
||
| #[cfg(windows)] | ||
| pub fn ensure_job_object() -> Option<HANDLE> { | ||
| None | ||
| } | ||
|
|
||
| #[cfg(windows)] | ||
| pub fn attach_pid_to_job(_pid: u32) {} | ||
|
|
||
| #[cfg(windows)] | ||
| pub fn init_windows_cleanup() {} | ||
|
|
||
| #[cfg(windows)] | ||
| pub fn windows_cleanup_enabled() -> bool { | ||
| false | ||
| } |
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,102 @@ | ||
| // Windows Job Object cleanup for child processes (Goose Windows support). | ||
| // | ||
| // Attaches spawned MCP subprocesses to a Job Object configured with | ||
| // JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE. When the Goose process exits (and the | ||
| // job handle is closed by the OS), Windows terminates every process in the | ||
| // job, preventing orphaned child processes. This is the Windows analog of the | ||
| // Linux PR_SET_PDEATHSIG behavior in subprocess.rs. | ||
|
|
||
| #![allow(dead_code)] | ||
|
|
||
| #[cfg(windows)] | ||
| mod windows_impl { | ||
| use std::mem::{size_of, zeroed}; | ||
| use std::ptr::null_mut; | ||
| use std::sync::atomic::{AtomicUsize, Ordering}; | ||
|
|
||
| use winapi::shared::minwindef::FALSE; | ||
| use winapi::um::handleapi::CloseHandle; | ||
| use winapi::um::jobapi2::{AssignProcessToJobObject, CreateJobObjectW}; | ||
| use winapi::um::processthreadsapi::OpenProcess; | ||
| use winapi::um::winbase::SetInformationJobObject; | ||
| use winapi::um::winnt::{ | ||
| JobObjectExtendedLimitInformation, HANDLE, JOBOBJECT_EXTENDED_LIMIT_INFORMATION, | ||
| JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE, PROCESS_SET_QUOTA, PROCESS_TERMINATE, | ||
| }; | ||
|
|
||
| // HANDLE (*mut c_void) is not Send/Sync, so we store the handle as a usize | ||
| // and cast back to HANDLE at use sites. 0 means "not yet created". | ||
| static JOB_HANDLE: AtomicUsize = AtomicUsize::new(0); | ||
|
|
||
| pub fn ensure_job_object() -> Option<HANDLE> { | ||
| let existing = JOB_HANDLE.load(Ordering::Acquire); | ||
| if existing != 0 { | ||
| return Some(existing as HANDLE); | ||
| } | ||
|
|
||
| unsafe { | ||
| let job = CreateJobObjectW(null_mut(), null_mut()); | ||
| if job.is_null() { | ||
| return None; | ||
| } | ||
|
|
||
| let mut info: JOBOBJECT_EXTENDED_LIMIT_INFORMATION = zeroed(); | ||
| info.BasicLimitInformation.LimitFlags = JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE; | ||
| let set_res = SetInformationJobObject( | ||
| job, | ||
| JobObjectExtendedLimitInformation, | ||
| &mut info as *mut _ as *mut _, | ||
| size_of::<JOBOBJECT_EXTENDED_LIMIT_INFORMATION>() as u32, | ||
| ); | ||
| if set_res == FALSE { | ||
| // If we fail to configure the job object to terminate on close, do not publish | ||
| // this handle. Cleaning up here avoids mutating global state with a partially | ||
| // configured Job Object. | ||
| CloseHandle(job); | ||
| return None; | ||
| } | ||
|
|
||
| // Publish the handle, but if another thread won the race, close ours. | ||
| match JOB_HANDLE.compare_exchange(0, job as usize, Ordering::AcqRel, Ordering::Acquire) | ||
| { | ||
| Ok(_) => Some(job), | ||
| Err(winner) => { | ||
| CloseHandle(job); | ||
| Some(winner as HANDLE) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| pub fn attach_pid_to_job(pid: u32) { | ||
| let job = match ensure_job_object() { | ||
| Some(job) => job, | ||
| None => return, | ||
| }; | ||
| unsafe { | ||
| // AssignProcessToJobObject requires PROCESS_SET_QUOTA in addition to | ||
| // PROCESS_TERMINATE; without it the assignment fails and the child is | ||
| // never tied to the job, leaving it orphaned on exit. | ||
| let proc = OpenProcess(PROCESS_TERMINATE | PROCESS_SET_QUOTA, FALSE, pid); | ||
| if !proc.is_null() { | ||
| if AssignProcessToJobObject(job, proc) == FALSE { | ||
| tracing::warn!(pid, "failed to assign child process to Windows job object"); | ||
| } | ||
| CloseHandle(proc); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| pub fn init_windows_cleanup() { | ||
| let _ = ensure_job_object(); | ||
| } | ||
|
|
||
| pub fn windows_cleanup_enabled() -> bool { | ||
| JOB_HANDLE.load(Ordering::Acquire) != 0 | ||
| } | ||
| } | ||
|
|
||
| #[cfg(windows)] | ||
| pub use windows_impl::{ | ||
| attach_pid_to_job, ensure_job_object, init_windows_cleanup, windows_cleanup_enabled, | ||
| }; |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On Windows, this assigns the PID only after
.spawn()has already started the MCP command, so launcher-style extensions such asuvx/npxcan create the real server or helper processes before the parent is added to the job. Job membership is inherited only by children created after their parent is in the job, so those early descendants remain outside the job and can survive when Goose exits; create the process in the job or start it suspended and assign it before resuming.Useful? React with 👍 / 👎.