Adopt structured process and signal lifecycles - #38
Conversation
lynnswap
changed the title
667d685 to
6347b15
Compare
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Wait for owned process groups after cancellation and downstream failures, while surfacing permanent signal-permission failures instead of spinning forever. Keep installer CLI tests explicitly macOS-only so the iOS test graph does not pull in host execution code.
6347b15 to
b054a9b
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b054a9b644
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| // Subprocess completes capture and its group-leader lifecycle before returning. | ||
| // Preserve cancellation only after the catch path also proves group extinction. | ||
| try Task.checkCancellation() |
There was a problem hiding this comment.
When a launched command forks a background descendant that closes its inherited output descriptors and the group leader exits normally, this path returns the captured result after only checking cancellation. completeAbortedProcessGroupTeardown is invoked exclusively from catch paths, and the normal return in runStreaming has the same gap, so the descendant can outlive the CLI operation despite the newly owned process group. Verify the group is empty or tear down remaining members before returning a normal result.
Useful? React with 👍 / 👎.
| } | ||
| ) | ||
| let manifest = try makeReleaseManifest( | ||
| let manifest = try await makeReleaseManifest( |
There was a problem hiding this comment.
If SIGINT or SIGTERM arrives after the final per-artifact cancellation check in makeReleaseManifest but before control reaches the synchronous filesystem operations below, the task is already canceled yet still creates the output directory and writes the manifest. The signal coordinator then exits with 130/143 even though the supposedly interrupted operation published its output; check cancellation again immediately before mutating the destination.
Useful? React with 👍 / 👎.
| env: nil, | ||
| cwd: repoRoot | ||
| ) | ||
| try Task.checkCancellation() |
There was a problem hiding this comment.
When a source checkout contains many or very large untracked files, SIGINT or SIGTERM received after this check is not observed while untrackedPaths is sorted and every file is synchronously loaded and hashed. Because the new signal coordinator traps the signal and waits for the operation task, the installer can remain running and hold its install lock until the entire fingerprint completes instead of stopping promptly; check cancellation during traversal and use cancellation-aware chunked hashing for large files.
Useful? React with 👍 / 👎.
| errorCode: errorCode | ||
| ) | ||
| } | ||
| await Task.yield() |
There was a problem hiding this comment.
When kill reports EPERM and processGroupHasLiveMember confirms that the group contains only zombies, this branch yields and retries forever rather than completing teardown. A zombie can persist indefinitely when its parent has moved to another process group and does not reap it, so cancellation never returns and callers such as the installer keep their lock held; treat a zombie-only group as fully terminated or impose a bounded wait.
Useful? React with 👍 / 👎.
1 participant
Purpose
Give executable roots ownership of cancellation and signals while ensuring every subprocess group is fully torn down before an operation returns.
Changes
CommandRunningimplementation based on swift-subprocess 1.0.simctlas an unavailable optional simulator capability while propagating cancellation and real simulator-discovery failures.Testing
swift test(310 tests in 32 suites).PrivateHeaderKitCoreandPrivateHeaderKitCoreTests.