Ensure tower-runtime setup failures surface a terminal status#253
Ensure tower-runtime setup failures surface a terminal status#253
Conversation
Several setup error paths in execute_local_app (uv spawn, venv, PYTHONPATH construction, etc.) returned without notifying the status channel, so the app got stuck reporting an unusable state and runs crashed with exit 1. Wrap the sender in a drop guard that always sends a -2 sentinel on failure, distinct from -1 (cancelled).
|
This PR is targeting If this is a regular feature/fix PR, please change the base branch to Current base: |
📝 WalkthroughWalkthroughThe changes introduce a Changes
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
This PR is targeting If this is a regular feature/fix PR, please change the base branch to Current base: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/tower-runtime/src/local.rs`:
- Around line 52-55: The comment points out that -1 is overloaded between
cancellation and child.wait() I/O failures; change the contract so
SETUP_FAILURE_EXIT_CODE remains -2 and -1 is reserved exclusively for
cancellation, and introduce a new sentinel or typed status for wait failures
(for example WAIT_FAILURE_EXIT_CODE) used by wait_for_process() instead of
returning -1 on child.wait() I/O errors; update wait_for_process(), its callers,
and any documentation/comments that reference the old -1 behavior (identify uses
in wait_for_process(), child.wait() error handling, and callers of
execute_local_app()) so callers can reliably distinguish cancellation vs. wait
failures.
In `@crates/tower-runtime/tests/local_test.rs`:
- Around line 383-385: Replace the loose check that Status::Crashed { code }
asserts code < 0 with an exact equality assertion for the setup-failure sentinel
introduced by the PR (i.e., assert_eq!(code, -2) or reference the exported
constant if available), so update the assertion in the Status::Crashed match arm
to pin the specific setup-failure code rather than any negative value; if the
constant (e.g., SETUP_FAILURE_SENTINEL) is not exported to tests, duplicate the
-2 literal in the test to make the expectation explicit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 49ee58df-251b-42f8-85be-da3fe4396056
📒 Files selected for processing (2)
crates/tower-runtime/src/local.rscrates/tower-runtime/tests/local_test.rs
| // Sent when execute_local_app returns Err before producing a real exit code | ||
| // (uv not found, venv spawn failed, PYTHONPATH construction failed, etc). | ||
| // Distinct from the cancellation sentinel -1. | ||
| pub(crate) const SETUP_FAILURE_EXIT_CODE: i32 = -2; |
There was a problem hiding this comment.
-1 is still overloaded.
These comments make -1 part of the cancellation contract, but wait_for_process() in this file also returns -1 for child.wait() I/O failures at Lines 578-581. That means callers still can't reliably distinguish cancellation from a runtime wait failure. If these negative codes are now semantic, reserve -1 for cancellation only and give wait errors their own sentinel or a typed status.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/tower-runtime/src/local.rs` around lines 52 - 55, The comment points
out that -1 is overloaded between cancellation and child.wait() I/O failures;
change the contract so SETUP_FAILURE_EXIT_CODE remains -2 and -1 is reserved
exclusively for cancellation, and introduce a new sentinel or typed status for
wait failures (for example WAIT_FAILURE_EXIT_CODE) used by wait_for_process()
instead of returning -1 on child.wait() I/O errors; update wait_for_process(),
its callers, and any documentation/comments that reference the old -1 behavior
(identify uses in wait_for_process(), child.wait() error handling, and callers
of execute_local_app()) so callers can reliably distinguish cancellation vs.
wait failures.
| Status::Crashed { code } => { | ||
| assert!(code < 0, "expected negative sentinel, got {}", code); | ||
| return; |
There was a problem hiding this comment.
Assert the exact setup-failure code.
code < 0 is too loose for this regression: it would also pass if setup failures accidentally started reporting the cancellation sentinel (-1). This test should pin the specific setup-failure code introduced by the PR, even if that means exposing the constant to integration tests or duplicating -2 here.
Suggested assertion
- assert!(code < 0, "expected negative sentinel, got {}", code);
+ assert_eq!(code, -2, "expected setup failure sentinel, got {}", code);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Status::Crashed { code } => { | |
| assert!(code < 0, "expected negative sentinel, got {}", code); | |
| return; | |
| Status::Crashed { code } => { | |
| assert_eq!(code, -2, "expected setup failure sentinel, got {}", code); | |
| return; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/tower-runtime/tests/local_test.rs` around lines 383 - 385, Replace the
loose check that Status::Crashed { code } asserts code < 0 with an exact
equality assertion for the setup-failure sentinel introduced by the PR (i.e.,
assert_eq!(code, -2) or reference the exported constant if available), so update
the assertion in the Status::Crashed match arm to pin the specific setup-failure
code rather than any negative value; if the constant (e.g.,
SETUP_FAILURE_SENTINEL) is not exported to tests, duplicate the -2 literal in
the test to make the expectation explicit.
1 participant
Several setup error paths in execute_local_app (uv spawn, venv, PYTHONPATH construction, etc.) returned without notifying the status channel, so the app got stuck reporting an unusable state and runs crashed with exit 1. Wrap the sender in a drop guard that always sends a -2 sentinel on failure, distinct from -1 (cancelled).
Summary by CodeRabbit
Bug Fixes
Tests