feat: add timeout option to CLI trigger and improve error handling#1340
Open
sergiofilhowz wants to merge 2 commits intomainfrom
Open
feat: add timeout option to CLI trigger and improve error handling#1340sergiofilhowz wants to merge 2 commits intomainfrom
sergiofilhowz wants to merge 2 commits intomainfrom
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRefactored the CLI trigger to use the Changes
Sequence Diagram(s)sequenceDiagram
participant User as CLI User
participant CLI as iii trigger\n(cli_trigger.rs)
participant SDK as iii_sdk\nWorker API
participant Engine as Engine
rect rgba(100, 150, 255, 0.5)
Note over User,Engine: Previous flow (manual WebSocket)
User->>CLI: iii trigger --address --port --payload
CLI->>Engine: WebSocket connect()
CLI->>Engine: send("invokefunction", ...)
Engine->>CLI: send("invocationresult", ...)
CLI->>User: Print result or error
end
rect rgba(150, 200, 100, 0.5)
Note over User,Engine: New flow (iii_sdk Worker API)
User->>CLI: iii trigger --address --port --payload --timeout-ms
CLI->>SDK: register_worker(address, port)
SDK->>Engine: Connect via SDK
CLI->>SDK: trigger(function_id, payload, timeout_ms)
SDK->>Engine: Execute with timeout
Engine-->>SDK: Result or IIIError
SDK-->>CLI: Return value or error
CLI->>User: Print JSON result or structured error (exit 1 on remote error)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
engine/src/main.rs (1)
129-145: Use this defaults test to cover the new--payloaddefault too.It still passes
--payload "{}", so the parser behavior that changed in this PR—omitting--payloadyields{}—isn't pinned anywhere in the clap tests.🧪 Suggested change
let cli = Cli::try_parse_from([ "iii", "trigger", "--function-id", "test::fn", - "--payload", - "{}", ]) .expect("should parse with defaults"); match cli.command { Some(Commands::Trigger(args)) => { + assert_eq!(args.payload, "{}"); assert_eq!(args.address, "localhost"); assert_eq!(args.port, DEFAULT_PORT); assert_eq!(args.timeout_ms, 30_000); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/src/main.rs` around lines 129 - 145, The test function trigger_uses_defaults_for_address_and_port currently passes "--payload" explicitly, so it doesn't verify the new default behavior; update the Cli::try_parse_from invocation in trigger_uses_defaults_for_address_and_port to omit the "--payload" and its "{}" value, then add an assertion on the parsed Trigger args (Commands::Trigger -> args) that args.payload equals "{}" in addition to the existing address, port and timeout checks so the parser's new default payload is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@engine/src/cli_trigger.rs`:
- Around line 78-80: Update the IIIError::Timeout match arm in
engine/src/cli_trigger.rs so the error message does not assume the engine is
unreachable; instead produce a balanced message that mentions both possibilities
(no response from the engine at the address/port OR the invocation handler
exceeded the configured timeout) and, if the timeout_ms value is available in
the current context, include that value and suggest increasing it or checking
handler duration; modify the IIIError::Timeout arm to log this combined
diagnostic text rather than the current "Is the engine running at the given
address and port?" message.
- Around line 112-125: The test run_trigger_unreachable_engine_times_out assumes
an unopened localhost port will produce a timeout, but it often fails fast with
connection errors; update the test in run_trigger_unreachable_engine_times_out
to accept both timeout and connection/connect-related failures by relaxing the
assertion on result.unwrap_err() (e.g., check for "Timed out" or "timeout" OR
common connection error strings such as "connection refused", "Connection
refused", "connect", "failed to connect"), or alternatively change TriggerArgs
to use a blackholed address (e.g., 10.255.255.1) to reliably trigger a timeout;
modify the assertion in the test that currently references run_trigger and
TriggerArgs to reflect either accepted error patterns or the new address.
In `@engine/tests/dlq_redrive_e2e.rs`:
- Around line 623-629: The stderr check is too loose and case-sensitive: convert
stderr (the String from output.stderr) to lowercase and assert against specific
normalized phrases (e.g., "failed to connect", "connection refused", "timed
out", "timeout") instead of the generic "connect" token so spurious matches like
"is not connected" don't pass; update the assert that references
stderr.contains(...) to use the lowercased stderr variable and tighten the
substrings to those explicit failure messages.
---
Nitpick comments:
In `@engine/src/main.rs`:
- Around line 129-145: The test function
trigger_uses_defaults_for_address_and_port currently passes "--payload"
explicitly, so it doesn't verify the new default behavior; update the
Cli::try_parse_from invocation in trigger_uses_defaults_for_address_and_port to
omit the "--payload" and its "{}" value, then add an assertion on the parsed
Trigger args (Commands::Trigger -> args) that args.payload equals "{}" in
addition to the existing address, port and timeout checks so the parser's new
default payload is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fa2740e8-3600-413b-a638-8d292fa1d527
📒 Files selected for processing (3)
engine/src/cli_trigger.rsengine/src/main.rsengine/tests/dlq_redrive_e2e.rs
Comment on lines
+78
to
+80
| IIIError::Timeout => { | ||
| anyhow::anyhow!("Timed out waiting for the engine (no response within the timeout). Is the engine running at the given address and port?") | ||
| } |
Contributor
There was a problem hiding this comment.
Don't diagnose every timeout as an unreachable engine.
timeout_ms is documented on Lines 29-31 as the max wait for the invocation result, so this branch also covers slow handlers on a healthy engine. The current message sends users straight to address/port debugging even when the real problem is a long-running function.
💡 Suggested wording
IIIError::Timeout => {
- anyhow::anyhow!("Timed out waiting for the engine (no response within the timeout). Is the engine running at the given address and port?")
+ anyhow::anyhow!(
+ "Invocation timed out before a result was received. The engine may be unreachable, or the function may have exceeded the configured timeout."
+ )
}📝 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.
Suggested change
| IIIError::Timeout => { | |
| anyhow::anyhow!("Timed out waiting for the engine (no response within the timeout). Is the engine running at the given address and port?") | |
| } | |
| IIIError::Timeout => { | |
| anyhow::anyhow!( | |
| "Invocation timed out before a result was received. The engine may be unreachable, or the function may have exceeded the configured timeout." | |
| ) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@engine/src/cli_trigger.rs` around lines 78 - 80, Update the IIIError::Timeout
match arm in engine/src/cli_trigger.rs so the error message does not assume the
engine is unreachable; instead produce a balanced message that mentions both
possibilities (no response from the engine at the address/port OR the invocation
handler exceeded the configured timeout) and, if the timeout_ms value is
available in the current context, include that value and suggest increasing it
or checking handler duration; modify the IIIError::Timeout arm to log this
combined diagnostic text rather than the current "Is the engine running at the
given address and port?" message.
Comment on lines
+112
to
+125
| async fn run_trigger_unreachable_engine_times_out() { | ||
| let args = TriggerArgs { | ||
| function_id: "test::fn".to_string(), | ||
| payload: "{}".to_string(), | ||
| address: "localhost".to_string(), | ||
| port: 19999, | ||
| timeout_ms: 800, | ||
| }; | ||
| let result = run_trigger(&args).await; | ||
| assert!(result.is_err()); | ||
| let err = result.unwrap_err().to_string(); | ||
| assert!( | ||
| err.contains("Failed to connect"), | ||
| "expected connection error, got: {}", | ||
| err.contains("Timed out") || err.contains("timeout"), | ||
| "expected timeout when engine is unreachable, got: {}", |
Contributor
There was a problem hiding this comment.
An unopened localhost port will not reliably surface as a timeout.
localhost:19999 usually fails fast with a WebSocket/connect error; a timeout is more typical for a blackholed address. The timeout-only assertion makes this test flaky across environments.
🧪 Suggested test hardening
- let err = result.unwrap_err().to_string();
+ let err = result.unwrap_err().to_string().to_lowercase();
assert!(
- err.contains("Timed out") || err.contains("timeout"),
- "expected timeout when engine is unreachable, got: {}",
+ err.contains("timed out")
+ || err.contains("timeout")
+ || err.contains("failed to connect")
+ || err.contains("connection refused")
+ || err.contains("websocket error"),
+ "expected a timeout or connect error when engine is unreachable, got: {}",
err,
);📝 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.
Suggested change
| async fn run_trigger_unreachable_engine_times_out() { | |
| let args = TriggerArgs { | |
| function_id: "test::fn".to_string(), | |
| payload: "{}".to_string(), | |
| address: "localhost".to_string(), | |
| port: 19999, | |
| timeout_ms: 800, | |
| }; | |
| let result = run_trigger(&args).await; | |
| assert!(result.is_err()); | |
| let err = result.unwrap_err().to_string(); | |
| assert!( | |
| err.contains("Failed to connect"), | |
| "expected connection error, got: {}", | |
| err.contains("Timed out") || err.contains("timeout"), | |
| "expected timeout when engine is unreachable, got: {}", | |
| async fn run_trigger_unreachable_engine_times_out() { | |
| let args = TriggerArgs { | |
| function_id: "test::fn".to_string(), | |
| payload: "{}".to_string(), | |
| address: "localhost".to_string(), | |
| port: 19999, | |
| timeout_ms: 800, | |
| }; | |
| let result = run_trigger(&args).await; | |
| assert!(result.is_err()); | |
| let err = result.unwrap_err().to_string().to_lowercase(); | |
| assert!( | |
| err.contains("timed out") | |
| || err.contains("timeout") | |
| || err.contains("failed to connect") | |
| || err.contains("connection refused") | |
| || err.contains("websocket error"), | |
| "expected a timeout or connect error when engine is unreachable, got: {}", | |
| err, | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@engine/src/cli_trigger.rs` around lines 112 - 125, The test
run_trigger_unreachable_engine_times_out assumes an unopened localhost port will
produce a timeout, but it often fails fast with connection errors; update the
test in run_trigger_unreachable_engine_times_out to accept both timeout and
connection/connect-related failures by relaxing the assertion on
result.unwrap_err() (e.g., check for "Timed out" or "timeout" OR common
connection error strings such as "connection refused", "Connection refused",
"connect", "failed to connect"), or alternatively change TriggerArgs to use a
blackholed address (e.g., 10.255.255.1) to reliably trigger a timeout; modify
the assertion in the test that currently references run_trigger and TriggerArgs
to reflect either accepted error patterns or the new address.
Comment on lines
623
to
+629
| let stderr = String::from_utf8_lossy(&output.stderr); | ||
| assert!( | ||
| stderr.contains("Failed to connect") || stderr.contains("connect"), | ||
| "Should mention connection failure, got: {}", | ||
| stderr.contains("Failed to connect") | ||
| || stderr.contains("connect") | ||
| || stderr.contains("Timed out") | ||
| || stderr.contains("timeout"), | ||
| "Should mention connection failure or timeout, got: {}", |
Contributor
There was a problem hiding this comment.
Normalize and tighten the stderr match here.
stderr.contains("connect") will also pass on iii is not connected, which hides a trigger-before-connect regression, and it still misses common Connection refused text because the match is case-sensitive.
🧪 Suggested assertion
- let stderr = String::from_utf8_lossy(&output.stderr);
+ let stderr = String::from_utf8_lossy(&output.stderr).to_lowercase();
assert!(
- stderr.contains("Failed to connect")
- || stderr.contains("connect")
- || stderr.contains("Timed out")
+ stderr.contains("failed to connect")
+ || stderr.contains("connection refused")
+ || stderr.contains("timed out")
|| stderr.contains("timeout"),
"Should mention connection failure or timeout, got: {}",
stderr
);📝 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.
Suggested change
| let stderr = String::from_utf8_lossy(&output.stderr); | |
| assert!( | |
| stderr.contains("Failed to connect") || stderr.contains("connect"), | |
| "Should mention connection failure, got: {}", | |
| stderr.contains("Failed to connect") | |
| || stderr.contains("connect") | |
| || stderr.contains("Timed out") | |
| || stderr.contains("timeout"), | |
| "Should mention connection failure or timeout, got: {}", | |
| let stderr = String::from_utf8_lossy(&output.stderr).to_lowercase(); | |
| assert!( | |
| stderr.contains("failed to connect") | |
| || stderr.contains("connection refused") | |
| || stderr.contains("timed out") | |
| || stderr.contains("timeout"), | |
| "Should mention connection failure or timeout, got: {}", | |
| stderr | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@engine/tests/dlq_redrive_e2e.rs` around lines 623 - 629, The stderr check is
too loose and case-sensitive: convert stderr (the String from output.stderr) to
lowercase and assert against specific normalized phrases (e.g., "failed to
connect", "connection refused", "timed out", "timeout") instead of the generic
"connect" token so spurious matches like "is not connected" don't pass; update
the assert that references stderr.contains(...) to use the lowercased stderr
variable and tighten the substrings to those explicit failure messages.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Tests