Skip to content

Unify execution interfaces to new one, deprecate old one#179

Open
socksy wants to merge 3 commits into
developfrom
ben/one-interface-to-rule-them-all
Open

Unify execution interfaces to new one, deprecate old one#179
socksy wants to merge 3 commits into
developfrom
ben/one-interface-to-rule-them-all

Conversation

@socksy
Copy link
Copy Markdown
Contributor

@socksy socksy commented Jan 21, 2026

Standardising on one execution interface. Renames ExecutionBackend/ExecutionHandle back to Backend/App (the old names) and adds default method impls so backends don't stub what they don't use. LocalApp implements App directly now; SubprocessHandle sticks around as a thin wrapper because auto_cleanup's temp-dir lifecycle has to hang off something.

Cancellation used to ride in as exit code -1, which conflated it with IO errors and signal kills. execute_local_app now sends a Result<Status, Error> and Status grows a Cancelled variant. Hopefully this lets the control plane drop the "preserve Cancelled if last_known_status was Cancelled" heuristic.

Cancelled is a hard break for exhaustive matches — one-line arm needed in tower-runner when it relinks, tracked out of band. The trait rename is kept deprecated-but-defined (not aliased) so downstream impls compile with a guiding message rather than an unresolved-import error. Root-level tower_runtime::App is also deprecated in favour of execution::App.

@socksy socksy requested review from bradhe, jo-sm and sammuti January 22, 2026 12:45
#[async_trait]
pub trait ExecutionHandle: Send + Sync {
/// Get a unique identifier for this execution
pub trait App: Send + Sync {
Copy link
Copy Markdown
Contributor

@sammuti sammuti Jan 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

@socksy socksy force-pushed the ben/one-interface-to-rule-them-all branch from 290029a to 8ffdb01 Compare April 20, 2026 19:14
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 20, 2026

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 0390ee28-fd3f-47b7-8785-298a76bfbdf3

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch ben/one-interface-to-rule-them-all

Comment @coderabbitai help to get the list of available commands and usage tips.

socksy added 3 commits April 20, 2026 21:29
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
keeps the previous ExecutionBackend/ExecutionHandle names as deprecated
traits so tower-runner sees guiding deprecation warnings on its next
relink rather than unresolved-import errors.
@socksy socksy force-pushed the ben/one-interface-to-rule-them-all branch from 8ffdb01 to 5e60137 Compare April 20, 2026 19:33
@bradhe
Copy link
Copy Markdown
Contributor

bradhe commented May 13, 2026

I think the main thrust of this PR is to standardize the interfaces slightly better than they are now. I do think that's valid, it just needs to be modernized a bit. I don't have strong opinions about how that should come together however. I'd be open to keeping this around and reintegrating the latest into it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants