Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
98 changes: 98 additions & 0 deletions .github/workflows/foreground-cancellation.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
name: Foreground cancellation qualification
on:
push:
branches: [fix/foreground-command-cancellation]
pull_request:
paths:
- 'libs/mcp/server/**'
- '.github/workflows/foreground-cancellation.yml'
permissions:
contents: read
jobs:
linux-qualification:
timeout-minutes: 30
runs-on: ubuntu-22.04
steps:
- uses: actions/checkout@v4
- uses: dtolnay/rust-toolchain@master
with:
toolchain: '1.94.1'
components: rustfmt, clippy
- uses: Swatinem/rust-cache@v2
with:
cache-on-failure: true
- name: Generate formatting receipt
run: |
cargo fmt --all
git diff > formatting.patch
- uses: actions/upload-artifact@v4
with:
name: formatting-${{ github.sha }}
path: formatting.patch
- run: cargo test --locked -p stakpak-mcp-server foreground_ -- --nocapture
- name: Stock executor must fail the cancellation regression
shell: bash
run: |
git fetch origin main --depth=1
git show FETCH_HEAD:libs/mcp/server/src/local_tools.rs > libs/mcp/server/src/local_tools.rs
python3 - <<'PYTHON'
from pathlib import Path
p = Path('libs/mcp/server/src/local_tools.rs')
s = p.read_text().replace(' fn local_container_with_profile(', ' pub(super) fn local_container_with_profile(')
s += '\n#[cfg(all(test, target_os = "linux"))]\n#[path = "foreground_tests.rs"]\nmod foreground_tests;\n'
p.write_text(s)
PYTHON
cargo test --locked -p stakpak-mcp-server --no-run
if cargo test --locked -p stakpak-mcp-server foreground_cancel_without_timeout -- --nocapture; then
echo 'Stock executor unexpectedly passed; regression does not demonstrate the defect'
exit 1
fi
git restore libs/mcp/server/src/local_tools.rs
cargo fmt --all
- run: cargo clippy --locked -p stakpak-mcp-server --all-targets -- -D warnings
- run: cargo build --locked --release -p stakpak
- run: git diff --exit-code
- name: Record prerelease artifact provenance
run: |
git rev-parse HEAD > target/release/source-commit.txt
rustc --version > target/release/rust-version.txt
sha256sum target/release/stakpak > target/release/stakpak.sha256
- uses: actions/upload-artifact@v4
with:
name: stakpak-linux-prerelease-${{ github.sha }}
path: |
target/release/stakpak
target/release/stakpak.sha256
target/release/source-commit.txt
target/release/rust-version.txt

upstream-suites:
timeout-minutes: 45
runs-on: ubuntu-latest
env:
# Keep assertions/debug behavior; omit symbols to bound linker memory.
CARGO_BUILD_JOBS: '1'
CARGO_PROFILE_DEV_DEBUG: '0'
CARGO_PROFILE_TEST_DEBUG: '0'
steps:
- uses: actions/checkout@v4
- uses: dtolnay/rust-toolchain@master
with:
toolchain: '1.94.1'
components: rustfmt, clippy
- uses: Swatinem/rust-cache@v2
with:
cache-on-failure: true
- run: cargo fmt --all -- --check
- run: cargo clippy --locked --all-targets -- -D warnings
- name: Complete workspace suite
timeout-minutes: 20
run: cargo test --locked --workspace --verbose
- name: Complete feature suites
timeout-minutes: 15
run: |
cargo test --locked -p stakpak-shared --features sqlite --verbose
cargo test --locked -p stakpak --features libsql-test --verbose
cargo test --locked -p stakpak-gateway --features libsql-test --verbose
cargo test --locked -p stakai --features network-tests --verbose
- run: cargo build --locked --verbose
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions libs/mcp/server/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,9 @@ grep-searcher = { workspace = true }
ignore = { workspace = true }
globset = { workspace = true }

[target.'cfg(unix)'.dependencies]
libc = "0.2"

[dev-dependencies]
tempfile = "3.8"

Expand Down
90 changes: 90 additions & 0 deletions libs/mcp/server/src/foreground_command.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
use std::io;
use std::process::ExitStatus;
use tokio::process::Child;

/// Owns a foreground child's group until the child is reaped. Commands that
/// deliberately leave this group are outside this cancellation boundary.
pub(crate) struct ForegroundCommand {
child: Child,
owned: bool,
}

impl ForegroundCommand {
pub(crate) fn new(child: Child) -> Self {
Self { child, owned: true }
}

pub(crate) fn child_mut(&mut self) -> &mut Child {
&mut self.child
}

#[cfg(unix)]
fn kill_group(&mut self) -> io::Result<()> {
if !self.owned {
return Ok(());
}
if let Some(pid) = self.child.id() {
let pid = libc::pid_t::try_from(pid)
.map_err(|_| io::Error::other("invalid foreground process ID"))?;
if pid <= 1 {
return Err(io::Error::other("invalid foreground process group"));
}
// SAFETY: process_group(0) made this owned child the group leader.
// Child::id() becomes None after wait/try_wait successfully reaps it;
// while unreaped (even a zombie), its PID cannot be reused.
if unsafe { libc::kill(-pid, libc::SIGKILL) } == -1 {
let error = io::Error::last_os_error();
if error.raw_os_error() != Some(libc::ESRCH) {
return Err(error);
}
}
}
Ok(())
}

pub(crate) async fn wait(&mut self) -> io::Result<ExitStatus> {
let result = self.child.wait().await;
match &result {
Ok(_) => self.owned = false,
// Child::id remains Some on a wait error. ECHILD means the
// exclusive ownership assumption was broken by another reaper;
// never signal that numeric PID/group again, including in Drop.
#[cfg(unix)]
Err(error) if error.raw_os_error() == Some(libc::ECHILD) => self.owned = false,
Err(_) => {}
}
result
}

pub(crate) async fn terminate(&mut self) -> io::Result<ExitStatus> {
#[cfg(unix)]
self.kill_group()?;
#[cfg(not(unix))]
self.child.start_kill()?;
tokio::time::timeout(std::time::Duration::from_secs(5), self.wait())
.await
.map_err(|_| io::Error::new(io::ErrorKind::TimedOut, "foreground cleanup timed out"))?
}
}

impl Drop for ForegroundCommand {
fn drop(&mut self) {
if !self.owned {
return;
}
#[cfg(unix)]
if let Err(error) = self.kill_group() {
tracing::error!("Failed to kill foreground command group: {error}");
}
// Direct-child fallback is also restricted to a still-owned child.
// Command's default kill_on_drop(false) avoids a deferred stale-PID
// kill after ECHILD; Tokio still handles reaping when Child is dropped.
if self.child.id().is_some() {
let _ = self.child.start_kill();
}
}
}

#[cfg(all(test, unix))]
#[path = "foreground_command_tests.rs"]
mod tests;
60 changes: 60 additions & 0 deletions libs/mcp/server/src/foreground_command_tests.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
// Fixture helpers intentionally fail fast on setup/transport errors.
#![allow(clippy::unwrap_used, clippy::expect_used)]

use super::*;
use tokio::process::Command;

fn successful_child() -> ForegroundCommand {
ForegroundCommand::new(
Command::new("sh")
.arg("-c")
.arg("exit 7")
.process_group(0)
.spawn()
.unwrap(),
)
}

#[tokio::test]
async fn foreground_wait_disarms_group_after_reap() {
let mut owned = successful_child();
assert_eq!(owned.wait().await.unwrap().code(), Some(7));
assert!(owned.child_mut().id().is_none());
// No numeric authority remains even if Drop is delayed.
owned.kill_group().unwrap();
drop(owned);
}

#[tokio::test]
async fn foreground_try_wait_disarms_group_after_reap() {
let mut owned = successful_child();
tokio::time::timeout(std::time::Duration::from_secs(5), async {
loop {
if let Some(status) = owned.child_mut().try_wait().unwrap() {
assert_eq!(status.code(), Some(7));
break;
}
tokio::time::sleep(std::time::Duration::from_millis(10)).await;
}
})
.await
.unwrap();
assert!(owned.child_mut().id().is_none());
owned.kill_group().unwrap();
drop(owned);
}

#[tokio::test]
async fn foreground_external_reap_disarms_signal_authority() {
let mut owned = successful_child();
let pid = owned.child_mut().id().unwrap() as libc::pid_t;
let mut status = 0;
// SAFETY: reap only this test's exact owned child, simulating an external
// reaper. It immediately exits; no global waitpid(-1) or PID reuse tricks.
assert_eq!(unsafe { libc::waitpid(pid, &mut status, 0) }, pid);
let error = owned.wait().await.unwrap_err();
assert_eq!(error.raw_os_error(), Some(libc::ECHILD));
assert!(!owned.owned);
owned.kill_group().unwrap();
drop(owned);
}
Loading