Skip to content

fix(cargo-gamma): handle macOS process group cleanup and CLI builds - #240

Open
chinayangxiaowei wants to merge 1 commit into
microsoft:mainfrom
chinayangxiaowei:fix/cargo-gamma-macos-compatibility
Open

chinayangxiaowei wants to merge 1 commit into
microsoft:mainfrom
chinayangxiaowei:fix/cargo-gamma-macos-compatibility

Conversation

@chinayangxiaowei

@chinayangxiaowei chinayangxiaowei commented Oct 9, 2026 •

Copy link
Copy Markdown

Problem

On macOS 15.7.2 (24G325, arm64), a compiled cargo-gamma could fail to complete even a one-mutant cargo gamma run. The observed error was:

cargo ... could not be terminated after it stopped being observable: the subtree leader was already reaped

The old process-tree cleanup could report failure after a short-lived leader exited. On macOS, killpg can return EPERM when the unreaped zombie leader is the group's only member. Direct termination did not observe that exit before sweeping, and a second terminate call errored even after cleanup had succeeded. Accepting every EPERM would hide a group that still contains an inaccessible live member.

There was also a separate build blocker: the CLI selected rallocator on macOS, where that dependency reports that it supports only Linux and Windows.

Summary

  • Observe the leader's exit without reaping it before sweeping its group. On macOS, accept EPERM only when the group query lists exactly that exited leader; preserve other permission failures and errors after an external reap.
  • Make repeated terminate calls succeed on macOS only after cleanup was proven successful.
  • Detect a child already reaped by another waiter before cleanup starts. Concurrent external reaping by numeric PID during observation and sweeping is unsupported because it can free and reuse that PID between the two operations.
  • Restrict the rallocator dependency and global allocator to Linux and Windows so the CLI builds on macOS.
  • Update the affected design docs and add deterministic syscall tests and macOS process-group regression coverage.

Before and after on macOS

Tested on macOS 15.7.2 (24G325, arm64) with Rust 1.97.1. The test input was a temporary crate with no dependencies and this source:

pub fn below_limit(value: usize, limit: usize) -> bool {
    value < limit
}

#[cfg(test)]
mod tests {
    use super::below_limit;

    #[test]
    fn checks_boundary() {
        assert!(below_limit(0, 1));
        assert!(!below_limit(1, 1));
    }
}

The unmodified parent commit 0e9a995 fails cargo build --locked --release -p cargo-gamma because rallocator rejects macOS. To isolate the runtime fix, the before build used that parent commit with only the two CLI allocator-gating edits from this PR; cargo-gamma-process and cargo-gamma-unsafe remained at the parent revision. Both binaries ran the same command against the same temporary crate, with fresh temporary cache and report directories for each run:

cargo gamma run --mutators relational.lt_to_le --memory off --jobs 1
Binary Result (two runs each)
Parent + allocator gating only Both exited 3 with the subtree leader was already reaped.
This PR Both exited 0; the selected mutant was killed.

Other validation

  • cargo fmt --all -- --check (macOS)
  • cargo test --locked -p cargo-gamma-unsafe -p cargo-gamma-process (macOS)
  • cargo test --locked -p cargo-gamma (macOS)
  • cargo build --locked --release -p cargo-gamma (macOS)

Copilot AI balanced review requested due to automatic review settings October 9, 2026 16:27

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The direct terminate path still mishandles a leader that exited before termination on macOS.

4 open findings
What changed in this PR

Adds macOS-compatible allocator configuration and process-group cleanup behavior.

Changes:

  • Restricts rallocator to Linux and Windows.
  • Handles macOS zombie-only process groups and idempotent termination.
  • Adds regression tests and updates design documentation.
File Description
crates/​cargo-gamma/​src/​main.rs Gates allocator initialization by platform.
crates/​cargo-gamma/​Cargo.toml Makes rallocator target-specific.
crates/​cargo-gamma/​docs/​DESIGN.md Documents allocator behavior.
crates/​cargo-gamma-unsafe/​src/​group.rs Adds post-exit group cleanup logic.
crates/​cargo-gamma-unsafe/​tests/​group.rs Adds process-group regressions.
crates/​cargo-gamma-unsafe/​docs/​DESIGN.md Documents macOS EPERM handling.
crates/​cargo-gamma-process/​src/​process_tree.rs Integrates exit-aware sweeping and idempotent termination.
crates/​cargo-gamma-process/​docs/​DESIGN.md Updates lifecycle guarantees.

🧠 Review effort: Balanced


💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread crates/cargo-gamma-process/src/process_tree.rs Outdated
Comment thread crates/cargo-gamma-unsafe/src/group.rs Outdated
Comment thread crates/cargo-gamma-unsafe/tests/group.rs Outdated
Comment thread crates/cargo-gamma-unsafe/tests/group.rs Outdated
@chinayangxiaowei

Copy link
Copy Markdown
Author

@microsoft-github-policy-service agree

Copilot AI balanced review requested due to automatic review settings October 9, 2026 16:49
@chinayangxiaowei
chinayangxiaowei force-pushed the fix/cargo-gamma-macos-compatibility branch from 9cd147a to 5925c10 Compare October 9, 2026 16:49

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread crates/cargo-gamma-unsafe/tests/group.rs
@chinayangxiaowei
chinayangxiaowei force-pushed the fix/cargo-gamma-macos-compatibility branch from 5925c10 to 6fa57f0 Compare October 9, 2026 16:59
Copilot AI balanced review requested due to automatic review settings October 9, 2026 16:59

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The platform gating, cleanup behavior, documentation, and regression coverage are consistent and complete.

2 open findings
1 resolved since last review

🧠 Review effort: Balanced

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The race-sensitive macOS process-group and raw syscall behavior warrants final human review despite targeted coverage.

0 open findings

2 resolved since last review

🧠 Review effort: Balanced

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Low-level macOS process lifecycle and syscall race handling warrants final human review.

0 open findings

🧠 Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 10, 2026 20:26
@chinayangxiaowei
chinayangxiaowei force-pushed the fix/cargo-gamma-macos-compatibility branch from 609bf7e to 1b04562 Compare October 10, 2026 20:26

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

A post-reap test probes a reusable process ID, and the generic observation-error fallback lacks regression coverage.

2 open findings

🧠 Review effort: Balanced

Comment thread crates/cargo-gamma-process/src/process_tree.rs Outdated
Comment thread crates/cargo-gamma-unsafe/tests/group.rs Outdated
Copilot AI balanced review requested due to automatic review settings October 10, 2026 20:31
@chinayangxiaowei
chinayangxiaowei force-pushed the fix/cargo-gamma-macos-compatibility branch from 1b04562 to 101e277 Compare October 10, 2026 20:31

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

A concurrent external reap can still free and reuse the process-group identifier between observation and sweeping.

1 open finding
2 resolved since last review

🧠 Review effort: Balanced

Comment thread crates/cargo-gamma-process/src/process_tree.rs
Copilot AI balanced review requested due to automatic review settings October 10, 2026 20:39

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The new regression test uses a real subprocess and an imprecise error assertion contrary to the repository’s deterministic testing requirements.

2 open findings

🧠 Review effort: Balanced

Comment thread crates/cargo-gamma-process/src/process_tree.rs Outdated
Comment thread crates/cargo-gamma-process/src/process_tree.rs Outdated
Copilot AI balanced review requested due to automatic review settings October 10, 2026 22:28
@chinayangxiaowei
chinayangxiaowei force-pushed the fix/cargo-gamma-macos-compatibility branch from bfcda7c to 7f592ca Compare October 10, 2026 22:28

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The failed-cleanup lifecycle transition lacks integrated regression coverage, and one updated test accepts an arbitrary error.

2 open findings
2 resolved since last review

🧠 Review effort: Balanced

Comment thread crates/cargo-gamma-process/src/process_tree.rs Outdated
Comment thread crates/cargo-gamma-process/src/process_tree.rs Outdated
Copilot AI balanced review requested due to automatic review settings October 10, 2026 22:33
@chinayangxiaowei
chinayangxiaowei force-pushed the fix/cargo-gamma-macos-compatibility branch from 7f592ca to d2a3694 Compare October 10, 2026 22:33

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Darwin-specific process lifecycle and syscall semantics require final human validation despite the focused tests.

0 open findings

2 resolved since last review

🧠 Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 10, 2026 23:47
@chinayangxiaowei
chinayangxiaowei force-pushed the fix/cargo-gamma-macos-compatibility branch from d2a3694 to 0199be7 Compare October 10, 2026 23:47

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The correctness depends on subtle macOS-specific process lifecycle and syscall semantics that warrant final human review.

0 open findings

🧠 Review effort: Balanced

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cleanup decision logic now distinguishes a proven zombie-only group from a live or inconclusive group, preserves non-ECHILD observation errors after best-effort cleanup, and records failed cleanup before repeated termination. The earlier external-reap concern is explicitly bounded by the documented sole-waiter requirement.

One new testing comment identifies the lack of a CI execution path for the macOS-gated regressions. The scripted fixtures establish decision behavior, not Darwin's actual kernel visibility or successful native CLI compilation.

All five passes covered the process lifecycle, four changed unsafe sites, callers, allocator gate, tests, and contracts. No builds, tests, native processes, or benchmarks were executed.

Comment thread crates/cargo-gamma-process/src/process_tree.rs
@codecov-commenter

Codecov Comments Bot (codecov-commenter) commented Oct 11, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.9%. Comparing base (0e9a995) to head (0199be7).

Additional details and impacted files
@@          Coverage Diff          @@
##            main    #240   +/-   ##
=====================================
  Coverage   98.9%   98.9%           
=====================================
  Files        304     304           
  Lines      44469   44486   +17     
=====================================
+ Hits       44008   44025   +17     
  Misses       461     461           
Flag Coverage Δ
linux 98.9% <100.0%> (-0.1%) ⬇️
linux-arm 98.9% <100.0%> (+<0.1%) ⬆️
scheduled ?
windows 98.8% <100.0%> (+<0.1%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI balanced review requested due to automatic review settings October 13, 2026 00:38
@chinayangxiaowei
chinayangxiaowei force-pushed the fix/cargo-gamma-macos-compatibility branch from 0199be7 to 8fa098f Compare October 13, 2026 00:38
chinayangxiaowei added a commit to chinayangxiaowei/ox-tools that referenced this pull request Oct 13, 2026
macOS has no unprivileged process-tree memory boundary, so `--memory
enforce` silently degraded to `off` there and a runaway mutant was bounded
only by the wall-clock timeout. A mutant that breaks a loop's termination
while the loop allocates can grow by gigabytes per second and exhaust a
workstation long before any timeout fires.

On macOS, a measuring launch now samples the physical footprint of every
process in its process group every 10 ms and sweeps the group once the sum
passes the ceiling. The baseline calibration, ceiling arithmetic, and
`outofmem` outcome are reused unchanged. The bound is soft and stated once
at run start: allocation can overshoot between samples, and descendants
that leave the group or belong to another user are not counted.

Linux and Windows behavior is unchanged.

Depends on microsoft#240.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The platform-specific signaling, reaping, and PID-reuse behavior warrants final human review on macOS.

0 open findings

🧠 Review effort: Balanced

@chinayangxiaowei

Copy link
Copy Markdown
Author

Rebased onto current main to resolve the conflict with #248. The only conflict was in ProcessTree::terminate, where #248 added reaped_after_cleanup (reap error first, then cleanup error), the same ordering this PR's finish_termination already had. I kept this PR's version and made finish_termination reuse #248's helper, now generic over the reaped value, so #248's test keeps covering it unchanged. There is no other change. Unsafe and process tests, clippy, and fmt pass on macOS.

@sandersaares

Copy link
Copy Markdown
Member

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Copilot speaking]

Published 10 findings. No finding follows up on an existing discussion thread.

See diagnostics
Diagnostic Value
Cache Miss

Comment on lines +798 to +799
/// concurrently with observation or termination: that could free the group ID between observation
/// and cleanup. A completed external reap is detected by the next observation.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Copilot speaking]

Keep the exclusive-waiter requirement separate from its implementation rationale

The added ProcessTree rustdoc correctly requires callers not to reap the child concurrently with observation or termination. The clause after the colon then describes a consequence of violating that requirement: freeing the group ID between observation and cleanup.

Keep the ownership requirement in the interface documentation, and leave the ID-reuse explanation in the existing internal design or implementation comments.

Reproducible reasoning:

  1. ProcessTree is exported, although cargo-gamma-process is a documentation-hidden implementation crate rather than an interface intended for direct end-user dependencies. Its item documentation still states the contract between the lifecycle owner and its callers.
  2. The new sentence beginning Callers must not independently reap states the caller's exclusive-waiter obligation. The clause after the colon describes this algorithm's failure mechanism under unsupported usage, not an additional requirement or a result promised for valid usage.
  3. The complete ownership discussion explicitly chooses exclusive ownership instead of supporting concurrent external reaping. The design and kill_with_observation comments already explain the observation-to-sweep race.

This recommendation preserves that settled ownership decision; it does not request synchronization or support for concurrent waiters.

Consequence:

Mixing the caller's obligation with the current algorithm's failure mechanism makes readers separate the contract from its implementation rationale. The rationale also needs updating if the algorithm changes, even when the caller's ownership obligation remains unchanged.

Recommended action:

Retain the requirement not to independently reap the numeric PID concurrently with observation or termination. Remove only the clause explaining that this can free the group ID between observation and cleanup; retain the statement about detecting a completed external reap.

The internal design and helper comments already explain the race, so no new document or duplicate explanation is needed. This wording-only adjustment preserves the ownership policy and does not require repeating native validation.

References:

Comment on lines +95 to +96
/// Kills a group after its leader has been observed exiting without being reaped.
///

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Copilot speaking]

Describe the cleanup contract without embedding Darwin's signal-delivery mechanism

The new kill_after_exit interface documentation explains that Darwin's killpg path excludes zombies and reports the absence of live members as a permission error.

Its caller needs the observed-and-unreaped leader precondition and the qualified EPERM result. The kernel's internal signal-delivery explanation belongs with the implementation rationale already captured in the crate's design.

Reproducible reasoning:

  1. kill_after_exit is a public safe boundary used by the process crate. Although the unsafe crate is documentation-hidden and intended only as an implementation dependency, this function's rustdoc describes the obligation and result at that boundary.
  2. Its input condition is a leader observed exiting without being reaped. On macOS the wrapper first attempts kill, then treats EPERM as success only if the membership query finds exactly that leader; failed or nonmatching membership evidence preserves the error.
  3. Those conditions and outcomes describe the interface contract. The separate explanation that the kernel excludes zombies describes how Darwin produces an input to the wrapper, not a promise made by the wrapper itself.
  4. The internal design already explains the zombie-only kernel behavior and the membership predicate's limits, so removing that mechanism from the interface documentation would not discard the maintainer rationale.

Consequence:

Readers must distinguish the wrapper's guarantees from a description of the kernel implementation. Keeping that mechanism in interface documentation also creates another place to maintain the platform explanation even if the wrapper's input requirements and observable result remain unchanged.

Recommended action:

Remove the clause describing zombie filtering inside the kernel's killpg path. Preserve the observed, unreaped-leader precondition, the conservative sole-leader EPERM qualification, and the error behavior for live or uninspectable members.

Keep the kernel explanation in the existing internal design rather than linking that design from the interface rustdoc or adding another copy. No code change or repeated platform experiment is necessary.

References:

  • crates/cargo-gamma-unsafe/docs/DESIGN.md, Boundaries: zombie-only behavior and conservative membership qualification
  • crates/cargo-gamma-unsafe/src/lib.rs: implementation-dependency role

Comment on lines +4036 to +4038
#[cfg(target_os = "macos")]
#[test]
fn termination_marks_a_failed_sweep_as_unproven_after_reaping() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Copilot speaking]

Explain why the failed-sweep regression uses an already-childless subtree

termination_marks_a_failed_sweep_as_unproven_after_reaping is a nontrivial scripted lifecycle regression. It starts with a synthetic ProcessTree whose child is already absent, supplies cleanup and reap outcomes, applies finish_termination, and then calls terminate again.

The function has no item-level explanation that this fixture deliberately exercises the post-reap cleanup state, rather than claiming to reproduce an operating-system process lifecycle. Its name identifies the expected result but not the reason for this unusual setup.

Reproducible reasoning:

  1. On macOS, terminate returns Ok(None) for an absent child only when output_cleanup_unproven is false. Repeated termination after a failed sweep must remain an error.
  2. finish_termination records failure on the actual ProcessTree before returning the sweep error. This test drives that production method using scripted callbacks, then exercises the absent-child branch of a repeated terminate call.
  3. The initially absent child lets this state transition be tested without launching a native child; it is not evidence that the first termination was skipped in a real lifecycle.
  4. The resolved lifecycle discussion explicitly explains why the regression applies the state and checks the repeated-call result. That useful setup rationale is absent from the source declaration, leaving the distinction to be reconstructed from the fixture and helper implementations.

The state transition and assertions are present and correct. This finding is about explaining the retained regression's role, not reopening the addressed coverage request.

Consequence:

A maintainer simplifying the fixture can mistake its initially absent child for a redundant already-reaped test, or treat the repeated-call assertion as unnecessary after checking the cleanup flag. The important distinction is that the production state application must prevent failed cleanup from becoming a successful repeated termination.

Recommended action:

Add a brief item-level comment to this regression explaining that the childless fixture represents the post-reap state, that scripted callbacks apply the production failed-cleanup transition, and that the final repeated call protects against reporting unproven cleanup as success.

Keep the fixture, helper split, and assertions unchanged. A comment on this nontrivial setup is sufficient; no additional abstraction, native process test, validation gate, or repeated macOS experiment is needed.

References:

poll_until(|| exited(child.id()).expect("the child can be observed")),
"the killed child never became observable"
);
#[cfg(not(target_os = "macos"))]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Copilot speaking]

Explain the macOS exclusion beside the group-reservation assertion

The added #[cfg(not(target_os = "macos"))] excludes the group-reservation assertion without a nearby explanation. The test's item-level comment still says that it verifies the unreaped leader's identifier remains unavailable for reuse.

The unsafe crate's design already supplies the relevant reason: Darwin's getpgid cannot query a zombie leader even when the group-membership listing still contains it. The source does not connect that distinction to the excluded assertion.

Reproducible reasoning:

  1. The test kills a process-group leader, observes its exit without reaping, and then calls group_of(pid) to assert that the group is retained.
  2. The PR excludes that assertion on macOS but retains the subsequent explicit wait and signal-status assertion.
  3. The design distinguishes the limitations of Darwin's getpgid from the group's continued membership. The exclusion is therefore about the observation API, not evidence that the group identifier is released early.
  4. Neither the test-level comment nor a comment beside the new condition explains that distinction, so the rationale is not available at the point where the expected check is skipped.

Consequence:

Someone maintaining this test can mistake the exclusion for an exception to the identifier-reservation guarantee, or restore the assertion using an API that cannot observe the zombie. Both misunderstandings are avoidable by explaining the API limitation where the exclusion is applied.

Recommended action:

Add a short comment immediately above the macOS condition explaining that Darwin's getpgid cannot query the unreaped zombie even though its group membership remains present. Retain the current condition and the private membership predicate; this recommendation does not reopen the prior request to expose that predicate or add a real-process membership assertion.

The existing design explanation can remain unchanged. No runtime behavior or completed validation needs to change.

References:

Comment on lines +1723 to 1724
#[cfg(all(not(miri), any(target_os = "linux", windows)))]
rallocator::rallocator!();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Copilot speaking]

Identify the dependency-support reason at the allocator selection

The new Linux/Windows condition on rallocator::rallocator!() selects the global allocator without an adjacent explanation of why this boundary is chosen. The corresponding target-specific dependency declaration uses the same host restriction.

The design explains that this follows rallocator's supported hosts, while macOS uses the system allocator. Making that reason visible at the selection point would distinguish a dependency constraint from a cargo-gamma performance or platform preference.

Reproducible reasoning:

  1. The PR restricts both the macro invocation and the dependency declaration to Linux/Windows. The condition states the selected hosts but does not explain why they are the boundary.
  2. The changed System boundaries section expressly says that the integration dependency is available only on those hosts and that macOS uses the system allocator.
  3. Neither selection site records that dependency-support rationale. A maintainer reading the source must leave the declaration to determine whether the restriction is an allocator preference, a cargo-gamma support policy, or a dependency constraint.

The intended selection and the author's completed native validation are not disputed.

Consequence:

Someone extending allocator selection can change the host predicate without first checking whether the dependency's supported-host boundary has changed. Conversely, the condition can be mistaken for a decision that cargo-gamma itself does not support other hosts.

Recommended action:

Add one concise comment beside the macro selection identifying the dependency's supported-host boundary as the reason for restricting this integration, rather than merely restating which hosts the condition selects. Preserve the existing Miri restriction and target predicates.

The same explanation covers the matching manifest restriction; there is no need to duplicate the platform paragraph at both sites or introduce a shared configuration mechanism. This is a source-rationale clarification, not a request for another native build or platform gate.

References:

Impacted locations:

  • crates/cargo-gamma/Cargo.toml:34-35

Comment on lines +58 to +76
that called `setsid`. On macOS, both observation and direct termination check
the leader's exit without reaping it before sweeping the group. If a running
leader exits during a failed sweep, termination rechecks the exit before
retrying the exited-leader path. That path accepts `EPERM` only after checking
that the observed, unreaped leader is the group's sole remaining member. The
kernel can return this error for that zombie-only group, but an inaccessible
live member or an inconclusive group query leaves cleanup unproven. A sweep
of a running leader still reports the error. If observation finds that the
leader was already reaped elsewhere, numeric group and child capabilities are
revoked before direct termination can signal them. The owner must not reap
the leader concurrently through its numeric PID: doing so can free that ID
between observation and sweep, and no process-group signal can then prove it
still names this subtree. A non-`ECHILD` observation error still attempts a
running-leader group sweep and direct child termination, then reports the
observation error if the direct kill succeeded.
`terminate` returns the leader's exit status when it
reaps the child. On macOS, repeating it after proven cleanup succeeds without
a status; an external reap or failed cleanup still reports an error because
descendants may remain. If subsequent cleanup cannot prove that descendants

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Copilot speaking]

Separate macOS lifecycle details from the output-capture overview

The Boundaries bullet introducing the contained output convenience now embeds the macOS termination decision sequence before returning to output-reader cleanup. It covers running-to-exited retries, sole-zombie EPERM qualification, external reaping, observation-error fallback, and repeated terminate outcomes in one continuous paragraph.

These details are useful, but they are not all needed to understand the general output-capture contract.

Reproducible reasoning:

  1. The bullet begins with the general promise that both output pipes are drained concurrently and descendants are swept before inherited handles reach end of file.
  2. The added passage then describes the conditional branches of direct termination on macOS, including errors and ownership restrictions.
  3. After that passage, the same bullet resumes its output-reader error precedence and detachment rules.

The insertion mixes the general explanation with platform-specific cases rather than presenting the general contract first and its relevant qualifications separately.

Consequence:

A reader looking for the output-capture guarantee has to work through a separate termination state machine before reaching the remaining reader-cleanup rules. A reader looking for macOS termination behavior must discover it inside a bullet labelled by output, making that contract harder to locate.

Recommended action:

Keep a short statement of the general cleanup guarantee in the output bullet, and move the added macOS lifecycle passage into a separate labelled bullet or subsection. Preserve the sole-leader qualification, ownership restriction, error behavior, and return-status semantics there; this is a prose-only reorganization and does not require repeating the reported runtime validation.

References:

  • crates/cargo-gamma-process/docs/DESIGN.md, Boundaries

Comment on lines +312 to +313
let exited = exited_with_waitid(42, |pid| {
assert_eq!(pid, 42);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Copilot speaking]

Explain the selection rule for the scripted process identifiers

The new waitid and group-membership tests repeatedly use 42 as the leader identifier and 43 as an additional or mismatching member, without a comment explaining whether those exact numbers matter.

The tests clearly explain their scenarios, but the selection rule for these numeric fixtures is left implicit.

Reproducible reasoning:

  1. The added tests pass numeric identifiers to injected closures, rather than using identifiers allocated by the operating system.
  2. Matching versus different identifiers drives the membership cases; the specific positive numbers are otherwise unexplained.
  3. The surrounding test-module comments and the individual tests provide no shared explanation that these are arbitrary valid identifiers and that only their equality relationships matter.

Consequence:

A maintainer adjusting or extending the scripted cases has to infer which fixture values are incidental and which relationships must be preserved. A brief shared explanation avoids treating these ordinary identifiers as undocumented boundary cases.

Recommended action:

Add a short comment covering the scripted identifier fixtures: state whether they are arbitrary valid identifiers and explain the matching-leader versus distinct-member selection rule. There is no need to repeat the numbers in prose, introduce shared fixtures, change the scenarios, or repeat the completed native validation for this comment-only clarification.

Impacted locations:

  • crates/cargo-gamma-unsafe/src/group.rs:321-412

Comment on lines +146 to +148
// SAFETY: `members` is writable storage for two pid_t values, and `bytes` is its exact
// byte length. The kernel writes no more than this length and returns the number of pids
// written.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Copilot speaking]

Describe the membership-buffer invariant without repeating its capacity

The membership query's comments repeat the buffer capacity in prose: the function comment says “two-entry snapshot,” and the unsafe-call comment describes storage for “two pid_t values.” The capacity itself is defined by the array type and initializer in only_group_leader_is_listed_with.

The existing explanation of why an additional member must be detected is useful; it need not also restate the array length.

Reproducible reasoning:

  1. The helper declares its capacity with [libc::pid_t; 2] and [0; 2].
  2. The syscall byte length is correctly derived with size_of_val(members), so its safety argument depends on the supplied slice of storage and the derived byte count, not on a separately stated numeric count.
  3. The prose in the wrapper independently repeats that capacity. A capacity adjustment could leave those comments describing a different buffer even though the byte count still tracks the actual storage.

Consequence:

Duplicating the capacity in safety documentation adds a separate statement that must stay synchronized with the array declaration. If it drifts, readers have to resolve whether the unsafe-call explanation or the actual buffer describes the intended invariant.

Recommended action:

Keep the rationale, but phrase it in terms of detecting any additional member and supplying writable PID storage whose byte length is derived from members. Avoid repeating the capacity in the comment. This is a local wording change, requires no algorithm or buffer-size change, and preserves the completed behavioral validation.

Impacted locations:

  • crates/cargo-gamma-unsafe/src/group.rs:136-138

Comment on lines +310 to +311
#[test]
fn a_pending_waitid_result_is_not_an_exit() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Copilot speaking]

Separate the new scripted group regressions from the native-process Miri exclusion

The new waitid decision regressions are inside the existing #[cfg(all(test, not(miri)))] mod tests. That excludes them under Miri, Rust's interpreter for detecting invalid execution, even though their callbacks supply results without invoking the operating-system call.

The added macOS membership/error cases are also entirely scripted and share this exclusion whenever a macOS Miri target is used. This is a test-selection issue distinct from whether native macOS tests or CI have been run.

Reproducible reasoning:

  1. The enclosing module's not(miri) condition prevents all its tests from entering a Miri test binary.
  2. The added pending-result, reported-PID, and error-preservation cases call exited_with_waitid. That helper performs the identifier conversion and interprets a supplied PID or error; its production adapter, not these tests, owns the actual waitid operation.
  3. The macOS cases similarly supply fixed membership arrays and errors to only_group_leader_is_listed_with and kill_after_exit_with. They do not call the native membership query, launch a child, or signal a group.
  4. The neighboring real-process tests explain why a native-only module exists, but do not justify placing these newly introduced in-process regressions behind the same exclusion.

The exclusion predates the PR. The departure is the placement of new scripted coverage underneath it; no repair of all existing exclusions is requested.

Consequence:

Miri cannot exercise the new decision regressions even though they avoid the native operations that require separate execution. Keeping scripted cases inside a native-only module also makes this unnecessary exclusion easy to propagate to future in-process cases.

Recommended action:

Move only the newly added scripted cases into a separate #[cfg(test)] module. Preserve the macOS conditions on cases that need macOS-only helpers, and leave the real-process tests in their existing native-only module.

This requires relocating a small set of tests and its imports, not changing production behavior or adding a new CI job. It does not replace native syscall coverage or reopen the settled native-macOS CI discussion, and does not require repeating the author's before/after CLI experiment.

References:

Impacted locations:

  • crates/cargo-gamma-unsafe/src/group.rs:310-411

Comment on lines +107 to +110
pub fn kill_after_exit(group: i32) -> io::Result<()> {
#[cfg(target_os = "macos")]
{
kill_after_exit_with(group, kill, only_group_leader_is_listed)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Copilot speaking]

Associate the qualified sweep with the leader whose exit was observed

The new kill_after_exit(group: i32) operation accepts the same unrestricted group identifier as ordinary cleanup, although its macOS exception additionally requires that this exact group's leader was observed exiting without being reaped.

A non-reaping observation leaves the exited leader present as a zombie until its owner consumes the exit status. A sole-member listing establishes group membership, not that exited state. The current callers supply the required observation correctly, but the API and ProcessTree::sweep carry the exit fact separately from the identifier.

Reproducible reasoning:

  1. At adoption, the retained group identifier corresponds to the child's PID. The Unix observation path calls group::exited(child.id()), and cleanup_after_observation executes self.sweep(true) only for a reported exit.
  2. On direct macOS termination, kill_with_observation passes the observed Boolean to sweep, and retries with the exited-leader path only after a successful second exit observation. The existing production paths therefore establish the precondition.
  3. sweep(leader_exited) selects group::kill_after_exit using that separate Boolean and passes the stored i32 group identifier.
  4. kill_after_exit does not observe the leader itself. After EPERM, its membership check accepts a single listed PID equal to that identifier. The private predicate's documentation and design explicitly state that this does not establish exit.
  5. Thus an observed-exited leader and an ordinary group have the same accepted representation at the new qualified boundary. The association between the observation and the group exists only in sequencing and documentation, rather than in the value handed to that operation.

The settled exclusive-waiter requirement remains necessary. An observation value would preserve the identity/state association, not prevent a concurrent external waiter from reaping the leader.

Consequence:

A later lifecycle change or another caller can select the qualified operation with a live leader's ordinary identifier. If that group returns a permission error and contains only its still-live leader, the membership predicate has the same matching result as for the exited leader, so unsupported sequencing can turn unproven cleanup into success.

This is a preventive state-representation concern. It is not a claim that the current paths skip observation, that concurrent external reaping is supported, or that the completed native validation is invalid.

Recommended action:

Introduce a small opaque exited-leader observation value retaining the observed identifier, construct it only from a successful non-reaping exit observation, and make the qualified cleanup operation obtain its identifier from that value. Carry it through the observation/direct-termination path instead of pairing an unrelated Boolean with a stored group ID.

Keep ordinary running-group cleanup separate. Existing Boolean observations used elsewhere need not all acquire a new interface; limit the typed result and consumer changes to the qualified sweep path and its immediate scripted fixtures. No general process-state framework, new operating-system mechanism, or additional CI gate is needed.

This is more work than a comment-only adjustment because it changes the boundary representation and immediate tests, while preserving the cleanup algorithm. Retain the sole-waiter obligation and the native regressions; the observation value must not be presented as synchronization or as proof valid after an external reap.

References:

  • crates/cargo-gamma-unsafe/docs/DESIGN.md, Boundaries: separate exit observation and membership evidence
  • crates/cargo-gamma-process/docs/DESIGN.md, Boundaries: exclusive waiting and observe-before-sweep ordering
  • crates/cargo-gamma-process/src/process_tree.rs:1212–1246, observed-exit cleanup selection
  • crates/cargo-gamma-process/src/process_tree.rs:1334–1341 and 1655–1698, direct-termination observation flow
  • Ownership discussion and explicit limit on concurrent external reaping

Impacted locations:

  • crates/cargo-gamma-process/src/process_tree.rs:1418-1438
  • crates/cargo-gamma-unsafe/src/group.rs:24-50

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants