feat(process): add owned completion results and working directories - #738
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (44)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| // Keep the record until both readers reach EOF. They can finish | ||
| // after the direct child, and a descendant can withhold pipe EOF. | ||
| loop { |
There was a problem hiding this comment.
🟡 Numeric waits block on descendant pipes
When an unowned child leaves a descendant holding its pipes, wait_for_process_result waits for both readers. Legacy numeric waits previously returned at direct-child exit, so daemon-launching programs now time out and lose the exit code.
Learn more
Numeric waits and full-result waits now share one completion path. Full-result waits need pipe EOF to return complete stdout and stderr. Legacy numeric waits only need the direct child's exit status, and this PR explicitly preserves their release behavior. With kill_on_shutdown = false, a descendant can inherit the direct child's pipes and keep both reader tasks alive after that child exits.
Example: A launcher spawns a background server, then exits with status 0. wait for process launcher to complete as status previously returned 0. It now waits for the server to close inherited stdout and stderr, then reaches the execution deadline and raises a timeout.
Recommended fix: Split numeric and full-result completion after the direct child exits. Numeric waits must remove the handle and return the stored status without awaiting capture-task EOF; dropping the handle can abort those readers as before. Keep reader joining and pipe-drain timeout behavior only for full_result waits.
Was this helpful? React with 👍 or 👎 to provide feedback.
| async fn close_process(&self, process_id: &str, idempotent: bool) -> Result<(), String> { | ||
| let handle = self.process_handles.lock().await.remove(process_id); | ||
| match handle { | ||
| Some(mut handle) => terminate_foreground_child(&mut handle.child).await, |
There was a problem hiding this comment.
🟡 Failed termination loses process handle
When termination fails for a running child, close_process has already removed its handle. The child continues untracked, so callers cannot retry cleanup and its capacity slot appears released.
Learn more
The registry is the only owner that lets later WFL statements address a background process. terminate_foreground_child can return an error when start_kill fails and a follow-up status check says the child still runs. Because removal happens first, that error drops the handle; when kill_on_shutdown is false, OwnedChild::drop does not terminate it.
Example: close process child encounters a transient operating-system termination error. WFL reports the error, but a second close or kill sees an unknown process while the original child can remain alive.
Recommended fix: Keep the handle registered until termination succeeds or the child is confirmed exited. One approach is to terminate while holding a mutable registry entry, then remove it only on success; ensure concurrent wait, poll, and close operations remain serialized for that handle.
Was this helpful? React with 👍 or 👎 to provide feedback.
| ## Required remote acceptance | ||
|
|
||
| Linux process groups/parent-death signalling cannot be executed on this Windows | ||
| host. Existing Blacksmith Linux and Windows integration/Run WFL Programs jobs, | ||
| full cargo/workspace gates, and exact-commit CI review remain required before | ||
| merge. Local Windows Green does not claim Linux acceptance. |
There was a problem hiding this comment.
🔍 Linux ownership acceptance is pending
The evidence covers Windows only. Verify Linux process-group and parent-death cleanup on this commit before merging the lifecycle change.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if stdout_done && stderr_done { | ||
| let mut handle = handles.remove(process_id).expect("process checked above"); | ||
| drop(handles); |
There was a problem hiding this comment.
🔍 Process results have one consumer
Concurrent waits on one handle race for its result. One succeeds; the other receives ProcessNotFound, so confirm this ownership contract.
Was this helpful? React with 👍 or 👎 to provide feedback.
![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: b2eddd4c63
ℹ️ 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".
| if directory.is_some() | ||
| && (program.contains('/') | ||
| || program.contains('\\') | ||
| || program.as_bytes().get(1) == Some(&b':')) | ||
| { |
There was a problem hiding this comment.
When an allowlist permits a name-only program and the parent PATH contains a relative component such as ., this leaves the program unresolved until after Command::current_dir(directory) is applied. On Unix, PATH lookup can then select an executable from the caller-chosen child directory, allowing a different binary with the permitted basename to bypass allowed_shell_commands; resolve the executable using the parent's PATH before changing directories, or reject relative PATH components for this case.
Useful? React with 👍 / 👎.
| async fn close_process(&self, process_id: &str, idempotent: bool) -> Result<(), String> { | ||
| let handle = self.process_handles.lock().await.remove(process_id); | ||
| match handle { | ||
| Some(mut handle) => terminate_foreground_child(&mut handle.child).await, | ||
| None if idempotent => Ok(()), |
There was a problem hiding this comment.
If start_kill or the subsequent wait returns an OS error while the child is still alive, the handle has already been removed from the registry and is dropped on return. In the default kill_on_shutdown = false configuration that drop does not retry termination, so the process can continue running while later kill process, close process, and interpreter shutdown can no longer reach it; reinsert or otherwise retain the handle until termination succeeds.
Useful? React with 👍 / 👎.
1 participant
Summary
Add owned subprocess completion with final stdout, stderr, exit status and success, an explicit working directory, idempotent close, and program exit codes. The full-result wait joins bounded output and releases the process handle atomically. Owned descendants are cleaned up on completion, timeout, cancellation and failure.
Motivation
Scriptorium needs a WFL-only test runner and integration driver. The existing runtime cannot reliably launch an isolated fixture directory or retrieve final stdout and stderr after numeric completion. This change adds the required lifecycle contract while preserving existing numeric wait behavior. Risk class: R3 (subprocess authorization, process ownership and CLI control flow).
Changes
in directoryfor foreground/background launches andwait for process ... to complete with timeout ... and read result as ....close processandexit program with code, with cleanup through finally blocks.current_executablefor selecting the exact running interpreter without a shell lookup.Testing
Final PR head:
f54243f43769b8d6e8ebe0b8f54ed2b2a735b234, including the HTTP #737 integration. Merged asd6993e77578892e0a93d3b8c8b08d6b1b55eede8; the merge retains the main-branch 26.9.13 version bump.Exact-head CI 35505056152 passed all required Linux and Windows jobs. Inspected program logs show 174 Linux / 173 Windows programs passed, zero failures and zero timeouts, with 52 / 53 existing platform skips. All five new process suites and the HTTP redirect suite actually executed on both platforms. Integration gates passed, including docs 36/36 and web 3/3; the Windows integration gate reports 150 WFL programs passed with 24 existing skips. Workspace tests, strict Clippy, formatting, database, fuzz compilation, release scripts and both hygiene jobs passed. Docker validation and configuration lint passed for the same head.
Fresh local validation after merging HTTP main: 23/23 process WFL cases plus 8/8 HTTP redirect cases, the existing subprocess comprehensive and exit-program compatibility programs, 106/106 existing Rust compatibility tests, required strict Clippy, formatting, locked fuzz-bin compilation and hygiene all passed. Independent source review found no remaining blocking lifecycle issue.
Historical Red and Green evidence
The results below describe earlier implementation revisions; the final integration results above supersede their pending status.
Initial tested revision:
b2eddd4c630e067583931785f9a1ee437a8bc5c8, Windows x86-64, Rust/Cargo 1.98.1; WFL reports 26.9.12.93829ff9on official nightlycargo check --all-targets --all-featurescargo clippy --all-targets --all-features -- -D warningscargo fmt --all --checkDetailed evidence records the baseline and exact local scope. New scenarios, fixtures, drivers and assertions are WFL. WFL fixtures prove descendant cleanup after normal completion, timeout, runtime failure, explicit status and failed assertions. An additional workspace-wide Clippy attempt found pre-existing LSP test warnings; the required root command above passes. At that historical revision, full workspace and Linux/Windows remote CI were pending; the final exact-head results above now verify those gates, including Linux process-group behavior. Technical review does not constitute maintainer approval.
Backward Compatibility
No schema migration is required. Applications using the new clauses require this runtime revision. Process-tree ownership intentionally prevents detached descendants from outliving an owned launch.
Checklist
f54243f4.Historical selector follow-up evidence:
a32c74f1f98f64a60b31f923785b4660d8f1d6a8adds the independently reviewedcurrent_executableselector (WFL2/2, catalog8/8). Exact-head CI35502045130 passed; inspected Linux/Windows logs show lifecycle, ownership, failure cleanup, and runtime-location suites actually ran. WFL programs172/171 passed with zero failures/timeouts. Integration, docs36, web3, strict Clippy, formatting, fuzz, database, hygiene, Docker validation and config lint passed.Historical timeout diagnostic correction: Red
012e7c89and Green96aa48cb122f48c68e8e937dc4ac44cbe283f77fpreserve bounded stdout/stderr before atomically releasing an owned timed-out process. WFL timeout1, lifecycle16, ownership2, cleanup2 and the strengthened Scriptorium runner9 passed locally. Independent review accepted the fix. Exact-head CI35503440343 is fully green; inspected Linux/Windows logs explicitly show timeout-diagnostics.test.wfl PASS. WFL programs173/172 passed with zero failures/timeouts (52/53 pre-existing platform skips), and all required format, Clippy, workspace, integration, docs, web, database, fuzz and hygiene jobs passed. Docker runtime validation and configuration lint passed.Summary by CodeRabbit
New Features
current_executableto return the running interpreter’s path.Documentation
Tests