fix: route mid-action cancel and time_limit through the fallback cancel path#312
fix: route mid-action cancel and time_limit through the fallback cancel path#312seant-aws wants to merge 3 commits into
Conversation
Previously, cancel_action required the session mutex to be available, which it never is during a running action (the background thread takes ownership). This meant cancel was effectively a no-op during execution. Add a shared CancellationToken slot (`cancel_token`) that is set before each action spawns. When cancel_action is called while the session is taken, it cancels this token — which cascades to the action's child token that the subprocess runner selects on — killing the process. Also update the snapshot state to CANCELING so the Python poll thread can observe the transition and continue polling until the terminal READY_ENDING state arrives with the correct ActionStatus. Signed-off-by: Sean Tang <seant-aws@users.noreply.github.com> Signed-off-by: Sean Tang <171081544+seant-aws@users.noreply.github.com>
…el path When an action is running the session object is checked out to a background thread, so cancel_action falls back to firing the shared cancellation token. That path previously dropped the time_limit argument entirely. cancel_action now sends the time_limit on the per-action cancel-request channel (pre-created and injected in prepare_action_cancel) before firing the token, so subprocess runners receive both the wake-up and the grace-period payload, and completed actions classify as Canceled. Signed-off-by: Sean Tang <171081544+seant-aws@users.noreply.github.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| # openjd-expr = { path = "../../openjd-rs/crates/openjd-expr" } | ||
| # openjd-model = { path = "../../openjd-rs/crates/openjd-model" } | ||
| # openjd-sessions = { path = "../../openjd-rs/crates/openjd-sessions" } | ||
| [patch.crates-io] |
There was a problem hiding this comment.
This uncomments the local-dev [patch.crates-io] overrides, but the comment block directly above (lines 51-53) states these must stay commented out for committed work and CI: "the published wheel is built against the crates.io versions, which is what the Cargo.lock file pins."
With this active, any build outside a machine that has a sibling ~/openjd-rs checkout at ../../openjd-rs/crates/... will fail to resolve the path dependencies — this breaks CI and the published-wheel build. Consistent with that, Cargo.lock now drops the source/checksum for openjd-expr, confirming it is being resolved from the local patch rather than crates.io.
This looks like an accidental commit of a local iteration state. It should be reverted (re-comment the block, and remove the corresponding [patch.crates-io] addition in the workspace Cargo.toml) before merge.
1 participant
What was the problem/requirement? (What/Why)
When an action is running, the session object is checked out to a background thread, so
cancel_actionfalls back to firing the shared cancellation token. That path droppedtime_limitand (before the paired openjd-rs change) had no listener on the cross-user helper path, so cancellation was silently lost for running actions.Paired change: OpenJobDescription/openjd-rs#258
What was the solution? (How)
prepare_action_cancel.cancel_actionsendstime_limiton that channel before firing the token so runners receive both the wake-up and the grace period.What is the impact of this change?
Mid-action cancellation now delivers
time_limitto the Rust runner, allowing it to gracefully terminate cross-user processes within the specified grace period instead of silently ignoring the cancel request.How was this change tested?
cargo build/clippycleanWas this change documented?
Is this a breaking change?
No. The new methods (
prepare_action_cancel,set_cancel_parent_token,set_cancel_request_channel) are additive. Existing callers that do not use cancellation are unaffected.Does this change impact security?
No. The cancel channel is internal to the session and does not create or modify files.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.