Add an optional, user-set execution timeout to Actions - #1563
Open
cplieger wants to merge 1 commit into
Open
Conversation
An Action that never returns leaves its Update InProgress indefinitely, which also blocks later runs of a parent Procedure and raises no alert. RunAction passed .cancel(cancel) but never .timeout(...), though CommandOptions has supported it since 2.3.0. ActionConfig.execution_timeout is seconds, 0 or below meaning no timeout, and defaults to 0 so upgrades are a no-op. Follows termination_timeout for the attribute triple, the i32 type and the UI control.
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
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.
What this is for
A ceiling a user can set on how long one Action may run, so a script that is buggy, looping, or waiting on something that never arrives gets stopped and failed instead of running until somebody notices. It is a control for the person writing the Action, not a fix for anything in Komodo.
The root-cause work is separate, in #1564: that closes the channel leaks and the missing cancellation in terminal execution, and stops a killed command's output being discarded. #1564 is the fix, this is a knob. They compose in one specific way: without #1564 a timeout here produces an empty log, and with it you get everything the script printed before the kill, which is what tells you where it stopped.
Related to #1392, whose item 1 asks for an optional timeout on the shell command function. This applies the same shape at the Action layer, with the caveat that an Action is a deliberately long-running case rather than the fast calls that issue has in mind, which is part of why I have not picked a default for you.
Change
ActionConfig.execution_timeout, in seconds,0or below meaning no timeout. Passed to the existingCommandOptions::timeout, so the process group is killed and the run is failed, which firesActionFailed.Defaults to
0, so upgrading is a no-op: absent Mongo fields deserialize to 0, omitted sync TOML shows no drift, and withtimeout = Nonetheselect!is exactly the pre-change path. I would suggest 900 as a shipped default, but that is a decision about other people's workloads, so I left it disabled rather than making it for you.Follows
DeploymentConfig.termination_timeoutthroughout: the attribute triple,i32seconds, and the UI control is that one with the label and bounds changed.Known limits
finallynever runs and any terminal the script created on a server is not deleted by the script itself. fix: terminal execution channel leaks and lost command output聽#1564 stops the forwarding task and the response channel leaking on that path, but the PTY still waits for normal cleanup. I am deliberately not proposing SIGTERM escalation: unhandled SIGTERM runs no JS in Deno, so the cleanup would not happen anyway.ActionConfigis semver-major for downstream literal constructors.Testing
cargo build,cargo test,rustfmt --checkon both files,tscwith no new errors in the touched file.types.tsregenerated with typeshare 1.13.4. I added theresources.jsonfield by hand rather than regenerating, because a full regen also sweeps in unrelated drift (custom_name,tags) that the committed schema is missing; that probably wants its own regen on your side.Not tested end to end against a running Core, so the kill path is verified from
lib/command's own tests plus a local probe rather than from a live Action.