Unify execution interfaces to new one, deprecate old one#179
Open
Unify execution interfaces to new one, deprecate old one#179
Conversation
This marks the previous lib::App one as deprecated, moves everything over to the new interface from Vim as the default, renames that to be more similar to what we had before + a couple simplifications, and adds an alias to make sure that we don't break the tower-runner
sammuti
reviewed
Jan 22, 2026
| #[async_trait] | ||
| pub trait ExecutionHandle: Send + Sync { | ||
| /// Get a unique identifier for this execution | ||
| pub trait App: Send + Sync { |
Contributor
There was a problem hiding this comment.
Note this would require a change in the runner side for the k8s impl. I propose just keeping the new name rather than renaming since I believe it's better name than "App"
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
2 participants
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.
Will write proper description later, but basically standardizing on the one interface, with a couple of smaller name changes that I think makes it easier to follow. Couple simplifications in related code.
Since LocalApp does everything SubprocessHandle needs, we can get simplify to just using LocalApp (but maybe we should rename it to SubprocessApp? But obviously that's less clear for running locally 😐.
Additionally, since we're touching this interface, make it return a new Completion type that wraps the error code, allowing us to return out of band whether or not it's actually cancelled, compared to errored (previously we returned a -1 which I guess works since unix programs return a uint8, but doesn't differentiate with the IO error case and feels a bit icky). This should hopefully also fix the need for our control plane which special cases for cancelling.