feat: add configurable action timeout for external operations #2342
Conversation
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
Add --action-timeout CLI argument to control timeouts for operations that rely on external resources (network requests, script executions, tool installations). Default timeout is 5 minutes, configurable with formats like "10m", "300s", etc. - Add --action-timeout CLI flag to init, install, check, and fmt commands - Create duration parsing utility supporting various time formats - Add action_timeout field to Settings struct with 5-minute default - Apply timeout to HTTP downloads, tool installations, script hooks, and invocations - Respect --skip-errored-plugins flag for timeout error handling This prevents hangs when external resources are slow or unresponsive, giving users control over timeout duration based on their needs.
Contributor
|
No issue mentions found. Please mention an issue in the pull request description. Use GitHub automation to close the issue when a PR is merged |
- Add timeout field to ToolBuilder and pass via with_timeout() - Add timeout fields to DownloadTool and GitHubReleaseTool - Update Download::install to accept timeout parameter - Pass timeout to all download operations instead of using thread-local - Update all runtime tools to pass None for timeout (they don't have configurable timeout yet)
Contributor
4 new issues
|
- Changed Download::install to require a timeout parameter (no longer optional) - Removed redundant timeout fallback logic from each install method - DownloadTool and GitHubReleaseTool now provide default 10-minute timeout if not configured - Runtime tools explicitly pass 10-minute timeout for downloads - Cleaner API ensures there's always a timeout to prevent infinite hangs
- Updated default action_timeout in Settings from 300s (5 min) to 600s (10 min) - Aligns with the default timeout used for downloads and prepare scripts - Provides more headroom for slower network connections and larger downloads
- Add timeout field to all runtime tool structs (Go, Java, Node, Python, Ruby, Rust, PHP) - Pass configured timeout from ToolBuilder to runtime tools instead of hardcoding 600 seconds - Update Ruby platform-specific implementations to accept timeout parameter - Use minimum of action timeout and driver timeout to preserve test behavior - Fix all test code to provide required timeout parameters This ensures runtime tools respect the CLI-configured action timeout rather than using hardcoded values. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Contributor
|
Diff Coverage for macos-15: The code coverage on the diff in this pull request is 63.2%. Total Coverage for macos-15: This PR will decrease coverage by 0.06%. File Coverage Changes
🛟 Help
|
Contributor
|
Diff Coverage for ubuntu-latest: The code coverage on the diff in this pull request is 63.2%. Total Coverage for ubuntu-latest: This PR will decrease coverage by 0.07%. File Coverage Changes
🛟 Help
|
![qltysh[bot]](https://avatars.githubusercontent.com/in/890766?s=60&v=4){kind=small}
- Make timeout required (not Option) in GitHubReleaseTool and DownloadTool - Remove hardcoded 600-second fallbacks throughout codebase - Use Settings::default().action_timeout as single source of truth - Update ToolBuilder to always provide timeout value - Fix all test code to use Settings default instead of hardcoded values - Ensure timeout flows: CLI → Settings → ToolBuilder → Tools → Methods This eliminates scattered hardcoded timeout values and ensures the CLI-configured timeout is properly threaded through the entire system. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Change action_timeout from Option<Duration> to Duration in Settings struct - Remove Some() wrappers when assigning timeout values in CLI commands - Eliminate 16+ unwrap() calls throughout the codebase - Simplify conditional logic in executor/driver.rs - Update all test code to use non-optional timeout This ensures Settings always has a valid timeout value (default 10 minutes), eliminating potential runtime panics and making the code cleaner and more type-safe. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Extract DEFAULT_ACTION_TIMEOUT_SECS constant to eliminate magic number - Make timeout required in ToolBuilder struct (Duration instead of Option<Duration>) - Update all ToolBuilder methods to use required timeout field directly - Remove Option wrapping in install.rs and planner/plugin.rs - Make Executor::install_tools accept Duration instead of Option<Duration> - Simplify Install::install method signature to use Duration directly This makes the code cleaner and more type-safe by eliminating unnecessary Option handling since timeout is always present in Settings. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Comment on lines
+221
to
234
| pub fn new_tool(version: &str, timeout: std::time::Duration) -> Box<dyn Tool> { | ||
| let platform_tool = sys::platform::Ruby::default(); | ||
| if Self::binary_install_enabled(&platform_tool) { | ||
| Box::new(Self { | ||
| version: version.to_string(), | ||
| timeout, | ||
| platform_tool, | ||
| }) | ||
| } else { | ||
| Box::new(RubySource { | ||
| version: version.to_string(), | ||
| timeout, | ||
| }) | ||
| } |
Contributor
Comment on lines
+238
to
251
| pub fn new_runtime(version: &str, timeout: std::time::Duration) -> Box<dyn RuntimeTool> { | ||
| let platform_tool = sys::platform::Ruby::default(); | ||
| if Self::binary_install_enabled(&platform_tool) { | ||
| Box::new(Self { | ||
| version: version.to_string(), | ||
| timeout, | ||
| platform_tool, | ||
| }) | ||
| } else { | ||
| Box::new(RubySource { | ||
| version: version.to_string(), | ||
| timeout, | ||
| }) | ||
| } |
Contributor
Comment on lines
+164
to
192
| fn runtime_tool(&self, runtime: Runtime, version: &str) -> Box<dyn RuntimeTool> { | ||
| let timeout = self.timeout; | ||
| match runtime { | ||
| Runtime::Node => Box::new(node::NodeJS { | ||
| version: version.to_string(), | ||
| timeout, | ||
| }), | ||
| Runtime::Python => Box::new(python::Python { | ||
| version: version.to_string(), | ||
| timeout, | ||
| }), | ||
| Runtime::Ruby => ruby::Ruby::new_runtime(version), | ||
| Runtime::Ruby => ruby::Ruby::new_runtime(version, timeout), | ||
| Runtime::Go => Box::new(go::Go { | ||
| version: version.to_string(), | ||
| timeout, | ||
| }), | ||
| Runtime::Rust => Box::new(rust::Rust { | ||
| version: version.to_string(), | ||
| timeout, | ||
| }), | ||
| Runtime::Java => Box::new(java::Java { | ||
| version: version.to_string(), | ||
| timeout, | ||
| }), | ||
| Runtime::Php => Box::new(php::Php { | ||
| version: version.to_string(), | ||
| timeout, | ||
| }), | ||
| } |
Contributor
Comment on lines
+196
to
224
| fn release_runtime_tool(&self, runtime: Runtime, version: &str) -> Box<dyn Tool> { | ||
| let timeout = self.timeout; | ||
| match runtime { | ||
| Runtime::Node => Box::new(node::NodeJS { | ||
| version: version.to_string(), | ||
| timeout, | ||
| }), | ||
| Runtime::Python => Box::new(python::Python { | ||
| version: version.to_string(), | ||
| timeout, | ||
| }), | ||
| Runtime::Ruby => ruby::Ruby::new_tool(version), | ||
| Runtime::Ruby => ruby::Ruby::new_tool(version, timeout), | ||
| Runtime::Go => Box::new(go::Go { | ||
| version: version.to_string(), | ||
| timeout, | ||
| }), | ||
| Runtime::Rust => Box::new(rust::Rust { | ||
| version: version.to_string(), | ||
| timeout, | ||
| }), | ||
| Runtime::Java => Box::new(java::Java { | ||
| version: version.to_string(), | ||
| timeout, | ||
| }), | ||
| Runtime::Php => Box::new(php::Php { | ||
| version: version.to_string(), | ||
| timeout, | ||
| }), | ||
| } |
Contributor
brynary
commented
Oct 14, 2025
| self.plan.tools(), | ||
| self.plan.jobs, | ||
| self.progress.clone(), | ||
| self.plan.settings.action_timeout, |
Member
Author
There was a problem hiding this comment.
This is a message chain smell.
In keeping with should add the action timeout to the Plan so that the Executor does not need to reach through the Plan to the Settings.
| name: String, | ||
| tool: Box<dyn Tool>, | ||
| progress: Progress, | ||
| _timeout: std::time::Duration, |
Member
Author
There was a problem hiding this comment.
Can we remove this _timeout arg?
| let pids = handle.pids(); | ||
| let timeout = plan.driver.timeout; | ||
| // Use the minimum of action_timeout and driver timeout | ||
| let timeout = std::cmp::min(plan.settings.action_timeout.as_secs(), plan.driver.timeout); |
Member
Author
There was a problem hiding this comment.
The calculation of the effective timeout should be moved from the executor to the planner
| let timeout = plan.settings.action_timeout; | ||
|
|
||
| // Start the command | ||
| let handle = cmd.start().with_context(|| { |
Member
Author
There was a problem hiding this comment.
Can we move the timeout handling down into the cmd code? It looks like we already have a CmdWrapper of some sort.
| use std::{fmt::Formatter, path::PathBuf}; | ||
| use std::{fmt::Formatter, path::PathBuf, time::Duration}; | ||
|
|
||
| const DEFAULT_ACTION_TIMEOUT_SECS: u64 = 600; // 10 minutes |
Member
Author
There was a problem hiding this comment.
Are there any plugins which should get a higher timeout than 10 mins by default?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
--action-timeoutCLI argument to control timeouts for operations that rely on external resourcesChanges
CLI Arguments
--action-timeoutflag toinit,install,check, andfmtcommandsCore Implementation
qlty-cli/src/duration.rsthat handles various time formatsaction_timeoutfield to Settings struct with 5-minute defaultqlty-config/src/http.rsqlty-check/src/tool/download.rsqlty-check/src/executor.rsandqlty-check/src/executor/driver.rsError Handling
--skip-errored-pluginsflagBenefits
This feature prevents the CLI from hanging indefinitely when:
Users can adjust the timeout based on their specific needs:
Test Plan
--action-timeout=30s,--action-timeout=10m)--skip-errored-pluginsproperly handles timeout errorsNote
There is one test failure in
tests/cmd/check/timeout.tomlthat needs investigation. The test expects a timeout to occur but with the new implementation it's not triggering as expected. This may need adjustment to align with the new timeout behavior.