fix: cap helper cancel notify period at the action's declared terminate_delay by seant-aws · Pull Request #258 · OpenJobDescription/openjd-rs

seant-aws · 2026-07-15T18:35:36Z

Note: This PR was rewritten after #260 merged. The original version delivered mid-action cancels to cross-user helper subprocesses via a pre-armed token and an in-runner select arm; #260's SessionCancelHandle landed that capability (adopting this PR's helper-path cancel registration and Canceled-classification pieces — thanks @mwiebe). What remains here is the one piece #260 deliberately left open ("worth converging on in whichever PR lands second"): honoring the action's declared cancelation grace on the helper path.

What was the problem/requirement? (What/Why)

The same-user cancel path computes the notify grace as min(time_limit, terminate_delay) (subprocess.rs), but the helper-pipe path (send_helper_cancel_command) used the caller's time_limit as-is and never consulted the action's declared notifyThenTerminate period:

A cross-user job whose action declared terminate_delay: 3s could be granted a 60s notify period by a cancel with time_limit=60s — the same job canceled same-user is terminated after 3s.
With no time_limit, the declared delay was ignored in favor of the 5s default, while the same-user path honors the declared delay.

Same action, different grace, depending only on which OS user runs it.

What was the solution? (How)

Record the effective action's declared NotifyThenTerminate grace period in the shared per-action cancel state (CancelShared.terminate_delay), once any RFC 0008 wrap-hook substitution has resolved which action actually runs. The derivation goes through cancel_method_for_action, so the recorded value is exactly what the runner enforces on the same-user path.
Cap the notify period in send_helper_cancel_command: the effective bound is min(time_limit, terminate_delay); absent a time_limit, the declared delay applies on its own; absent both, the legacy 5s default is unchanged.
Both Session::cancel_action and SessionCancelHandle::cancel deliver through this chokepoint, so both are covered.
Actions that do not declare notifyThenTerminate keep the existing behavior byte-for-byte (no recorded delay → time_limit passthrough).

What is the impact of this change?

Cross-user cancels now honor the action's declared cancelation config the same way same-user cancels do. No API changes.

How was this change tested?

4 new integration tests covering: capping when time_limit exceeds the declared delay, smaller time_limit winning, declared delay applying when no time_limit is given, and passthrough for actions with no declared cancelation.
Full openjd-sessions suite: 158 unit + 261 integration passing; clippy and fmt clean.

Was this change documented?

Doc comments on the new field, the recording sites, and the capping logic.

Is this a breaking change?

No.

Does this change impact security?

No. The cancel channel and helper protocol are unchanged; only the numeric notify period is bounded.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Also included (second commit): hardening of the cross-user cancel-handle integration tests, which currently fail deterministically on the Windows cross-user CI job — including on main (failing main run on the #260 merge commit, passing pre-merge run for comparison). The tests delivered the cancel after a fixed 500ms sleep and discarded error results: when the subprocess exits before the delay elapses, the strict test fails with a misleading delivered None assertion while the mark_failed sibling passes vacuously (it completed in 356ms — less than its own 500ms delay — in the failing main run), and the underlying subprocess error is never printed. The hardened tests retry cancel delivery until the in-flight action is found, and fail with the actual subprocess error when it dies on its own — so if there is a real environment issue on the runners, the next CI run will show it instead of masking it.

github-actions · 2026-07-15T19:24:49Z


            let cancel_token = self.new_action_cancel_token();
-            let (cancel_tx, cancel_rx) = tokio::sync::watch::channel(None);
+            let cancel_rx = if let Some(rx) = self.cancel.request_rx_preinjected.take() {


request_rx_preinjected.take() consumes the pre-injected receiver, so it is only used by the first action after set_cancel_request_channel is called. Every subsequent action (line 1103/1225/1363/1432) falls into the else branch, creates a fresh watch channel, and overwrites self.cancel.request_tx with a new sender.

But the PyO3 binding retains a clone of the original sender (that is the stated purpose of this method). After the first action, that retained sender is orphaned — the session no longer holds a receiver for it, and the new channel it created has a sender the binding never sees. So cancel path B (time-limit delivery via the retained sender) silently stops working for the 2nd and later actions on the same session.

If the binding re-calls set_cancel_request_channel before every action this is moot — but the doc ("retain a clone of the sender") reads as set-once. Worth confirming, or re-injecting the receiver (self.cancel.request_rx_preinjected = Some(rx.clone())) rather than taking it, so it persists across actions.

mwiebe · 2026-07-16T00:18:57Z

I've ended up down a similar path as your PR starting from having an agent doing comparative testing between @leongdl 's sessions v0 PR and the v1 implementation. I'll post what I ended up with as well, and let's compare notes!

Fixes found by exploratory differential testing of the Python openjd.sessions._v1 bindings against the pure-Python v0 implementation. 1. sessions: Session::cancel_handle() — a thread-safe, reusable handle that cancels the running action while the Session value is owned by the thread driving the action (how the Python bindings operate). Per-action cancellation state moves behind a shared Arc<Mutex<..>>; the handle honors the action's cancelation method, forwards time_limit, supports mark_action_failed, and delivers the cross-user helper cancel command like Session::cancel_action (extracted into send_helper_cancel_command). Previously the only concurrent-cancel affordance was the one-shot SessionConfig::cancel_token, which poisons all later actions once cancelled — so external callers could not cancel a running action at all. run_subprocess_via_helper now registers per-action cancel state so the handle can reach helper-routed subprocesses, and wires the cancel-request watch receiver into the subprocess config so a canceled helper subprocess classifies as Canceled instead of Failed. The classification gap and the never-fired-token diagnosis for this path come from Sean Tang's independent fix of the same bug in OpenJobDescription#258; this commit adopts that wiring. 2. sessions: report action-setup failures instead of wedging in Running. The dispatch methods transition to Running before fallible setup (path-mapping materialization, RFC 0008 WrappedAction.* seeding including wrapped-action timeout resolution); an error there left the session Running forever with no terminal ActionStatus and an empty log. New fail_action_setup helper marks the action Failed with fail_message, logs, resets cancel state, leaves Running, and notifies the callback. The drive_action runner-error branch and the run_subprocess_via_helper error path now also record fail_message and log the error (was: Failed with no message, nothing logged). 3. model+sessions: embedded file `filename` is a plain string, per the 2023-09 schema (the field is not annotated @fmtstring) and matching the Python reference. It was previously a FormatString that the runtime resolved, which silently changed the meaning of conformant templates whose filename contains literal `{{ }}` text and made file-path allocation depend on expression evaluation. The model field type changes to Option<String>; the runtime uses the value literally. 4. sessions+model: step scripts now allocate embedded file paths BEFORE evaluating script-level `let` bindings (file contents are still written after, so `data` can reference let values). With filenames plain strings, allocation cannot depend on `let`, so `Task.File.*` is well-defined in `let` scope — mirroring the environment runner, which already evaluated Env.File.* this way. Decode-time validation already seeded Task.File.* into the `let` scope; the runtime now agrees, fixing templates that validated but failed at runtime with an undefined-symbol error (silently, per bug 2). BREAKING CHANGE: openjd_model::job::EmbeddedFile::filename and openjd_model::template::EmbeddedFile::filename change from Option<FormatString> to Option<String>. `{{ }}` sequences in an embedded file's `filename` are now literal text, not expressions; templates relying on the nonconformant resolution behavior must move the expression into a `let` binding or use a static filename. Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com>

Fixes found by exploratory differential testing of the Python openjd.sessions._v1 bindings against the pure-Python v0 implementation. 1. sessions: Session::cancel_handle() — a thread-safe, reusable handle that cancels the running action while the Session value is owned by the thread driving the action (how the Python bindings operate). Per-action cancellation state moves behind a shared Arc<Mutex<..>>; the handle honors the action's cancelation method, forwards time_limit, supports mark_action_failed, and delivers the cross-user helper cancel command like Session::cancel_action (extracted into send_helper_cancel_command). Previously the only concurrent-cancel affordance was the one-shot SessionConfig::cancel_token, which poisons all later actions once cancelled — so external callers could not cancel a running action at all. run_subprocess_via_helper now registers per-action cancel state so the handle can reach helper-routed subprocesses, and wires the cancel-request watch receiver into the subprocess config so a canceled helper subprocess classifies as Canceled instead of Failed. The classification gap and the never-fired-token diagnosis for this path come from Sean Tang's independent fix of the same bug in #258; this commit adopts that wiring. 2. sessions: report action-setup failures instead of wedging in Running. The dispatch methods transition to Running before fallible setup (path-mapping materialization, RFC 0008 WrappedAction.* seeding including wrapped-action timeout resolution); an error there left the session Running forever with no terminal ActionStatus and an empty log. New fail_action_setup helper marks the action Failed with fail_message, logs, resets cancel state, leaves Running, and notifies the callback. The drive_action runner-error branch and the run_subprocess_via_helper error path now also record fail_message and log the error (was: Failed with no message, nothing logged). 3. model+sessions: embedded file `filename` is a plain string, per the 2023-09 schema (the field is not annotated @fmtstring) and matching the Python reference. It was previously a FormatString that the runtime resolved, which silently changed the meaning of conformant templates whose filename contains literal `{{ }}` text and made file-path allocation depend on expression evaluation. The model field type changes to Option<String>; the runtime uses the value literally. 4. sessions+model: step scripts now allocate embedded file paths BEFORE evaluating script-level `let` bindings (file contents are still written after, so `data` can reference let values). With filenames plain strings, allocation cannot depend on `let`, so `Task.File.*` is well-defined in `let` scope — mirroring the environment runner, which already evaluated Env.File.* this way. Decode-time validation already seeded Task.File.* into the `let` scope; the runtime now agrees, fixing templates that validated but failed at runtime with an undefined-symbol error (silently, per bug 2). BREAKING CHANGE: openjd_model::job::EmbeddedFile::filename and openjd_model::template::EmbeddedFile::filename change from Option<FormatString> to Option<String>. `{{ }}` sequences in an embedded file's `filename` are now literal text, not expressions; templates relying on the nonconformant resolution behavior must move the expression into a `let` binding or use a static filename. Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com>

github-actions · 2026-07-17T22:00:43Z

+        crate::runner::CancelMethod::NotifyThenTerminate { terminate_delay } => {
+            Some(terminate_delay)
+        }
+        crate::runner::CancelMethod::Terminate => None,


For Terminate-declared actions (which is also the default when cancelation is None — see cancel_method_for_action: None | Some(Terminate) => Terminate), this arm returns None, so terminate_delay is left None. When such an action is then canceled through the handle with no time_limit, send_helper_cancel_command computes effective_limit = None and emits NOTIFY_THEN_TERMINATE with the 5s legacy default instead of TERMINATE.

That diverges from the same-user path this PR says it is mirroring: in subprocess.rs the (CancelMethod::Terminate, _) arm SIGKILLs immediately regardless of time_limit, whereas the helper here would grant a ~5s grace before killing. The None sentinel conflates "no notifyThenTerminate declared → terminate immediately" with "no cap → use legacy default". Consider carrying enough information (e.g. an explicit Terminate marker) so a Terminate action still produces a TERMINATE command on the helper pipe.

seant-aws · 2026-07-17T23:21:27Z

Cross-User Tests (Windows): GREEN (run 29619530959, 24/24) — and genuinely green: the cancel tests now take ~6s each (real cancel + grace-period kill), versus the 356ms vacuous passes of before.

The confirmed root cause (documented, not guessed): the test harness rooted sessions under the runner's %TEMP%, whose path contains the 8.3 short component RUNNER~1. PowerShell normalizes its startup CWD by enumerating every parent directory (PowerShell#7760); the session user has no List access on another user's profile dir, so powershell died at startup with UnauthorizedAccessException — while cmd/whoami/bat, which never normalize, passed. The old tests' 500ms fixed delay raced that startup death: Jul 16 the cancel won (vacuous green), Jul 17 the death won (misleading red). Production is unaffected — real session roots live under C:\ProgramData with listable parents.

What's on #258 now (4 commits): the terminate_delay capping fix (be26ab7), test hardening (6c3eb9b), fail-closed DACL setup + two probe tests (d4452c0), and the %PUBLIC% session-root fix (e92c3ff). The probes stay as permanent regression coverage — a working-dir ACL pin and the first test that requires powershell to succeed as the session user.

seant-aws · 2026-07-17T23:22:12Z

conformance tests failing now because openjd-specs were updated today iwth WrappedAction.Cancelation.*, so expected to fail

github-actions · 2026-07-18T15:38:04Z

+    cancelation: &Option<openjd_model::job::CancelationMode>,
+    default_notify_period: Duration,
+) -> Option<Duration> {
+    match crate::runner::cancel_method_for_action(cancelation, default_notify_period) {


This will not compile. cancel_method_for_action (runner/mod.rs:326) has the signature:

pub(crate) fn cancel_method_for_action( cancelation: &Option<CancelationMode>, symtab: &SymbolTable, library: Option<&FunctionLibrary>, default_notify_period: Duration, ) -> Result<CancelMethod, SessionError>

but here it is called with only two arguments (cancelation, default_notify_period), and its return value is matched directly as a bare CancelMethod. Two problems:

Arity mismatch — the required symtab and library arguments are missing.

The return is Result<CancelMethod, SessionError>, so the match arms do not type-check without first handling the Result (via ?/unwrap/Ok(..)).

Because the function resolves format strings against the symbol table, declared_terminate_delay needs the same symtab/library used at each callsite (onEnter/onExit/onRun) and must propagate the Result. As written the crate cannot build.

…te_delay The same-user cancel path computes the notify grace as min(time_limit, terminate_delay) (subprocess.rs), but the helper-pipe path used the caller's time_limit as-is: a cross-user job whose action declared notifyThenTerminate with a short terminate_delay could be granted a much longer notify period, and with no time_limit the declared delay was ignored in favor of the 5s default. Record the effective action's declared NotifyThenTerminate grace period in the shared per-action cancel state (once any wrap-hook substitution has resolved which action runs), and cap the notify period sent over the helper pipe at it. Both Session::cancel_action and SessionCancelHandle::cancel deliver through the same chokepoint, so both paths are covered. Actions that do not declare notifyThenTerminate keep the existing behavior unchanged. Distilled from the original version of this PR after the cancel-handle infrastructure landed in OpenJobDescription#260, which adopted this PR's helper-path cancel registration and Canceled classification pieces. Signed-off-by: Sean Tang <171081544+seant-aws@users.noreply.github.com>

…r cancel-handle tests The cancel-handle integration tests delivered the cancel after a fixed 500ms sleep and discarded error results. A subprocess that exits before the delay elapses (e.g. failing to start) made the strict variant fail with a misleading 'delivered None' assertion, while the mark_failed variant passed without exercising cancellation at all - and the actual subprocess error was never printed. Observed deterministically on the Windows cross-user CI job since 2026-07-17, including on main. Replace the fixed delay with delivery-with-retry (500ms head start, then retry until the in-flight action is found, 10s deadline), join the canceller before asserting, and distinguish 'canceled action surfaced as an error result' (tolerated, requires the cancel to have been delivered) from 'subprocess failed on its own' (fails the test with the underlying error and exit state). Applied to both the Windows and Linux twins. Signed-off-by: Sean Tang <171081544+seant-aws@users.noreply.github.com>

TempDir::new silently skipped the entire cross-user DACL setup when the process-user lookup failed, leaving the session user without access to the session working directory and deferring the failure to whichever subprocess first touched it. Surface the failure as an error instead. Also add two Windows probe tests: one pinning the working directory's own ACL (existing permission tests only cover files placed inside it), and one running powershell to success as the session user - its provider initialization enumerates the CWD, detecting directory-ACL problems that cmd/whoami never notice. Until now the only tests needing powershell to genuinely run as the session user were the cancel pair, which conflates subprocess health with cancellation behavior. Signed-off-by: Sean Tang <171081544+seant-aws@users.noreply.github.com>

The test harness rooted sessions under %TEMP%, which on GitHub runners is the process user's profile Temp containing an 8.3 short component (C:\Users\RUNNER~1\...). Windows PowerShell normalizes its startup working directory by enumerating every parent directory (see PowerShell/PowerShell#7760), and other local users have no List Folder access on the profile directory - so powershell running as the session user died with UnauthorizedAccessException before executing any command, while cmd.exe (no such normalization) sailed through. This is what the cancel-handle tests were intermittently racing against, and what the hardened tests now surface deterministically. Root test sessions under %PUBLIC% instead: listable by all local users, matching the parent-ACL topology of production session roots under C:\ProgramData. Production is unaffected by the underlying issue; this is strictly a test-harness fix. Signed-off-by: Sean Tang <171081544+seant-aws@users.noreply.github.com>

github-actions · 2026-07-20T17:21:36Z

+        // StepScriptRunner's default_cancel_period.
+        self.cancel.set_terminate_delay(declared_terminate_delay(
+            &effective_script.actions.on_run.cancelation,
+            &action_symtab,


declared_terminate_delay is resolved here against action_symtab, but the same-user runner resolves cancel_method_for_action against a different symbol table. In StepScriptRunner::run (and run_env_action), run_action is called with final_symtab — i.e. action_symtab plus the evaluated let bindings and the embedded-file symbols (Task.File.* / Env.File.*). Those symbols do not exist in action_symtab at the point set_terminate_delay is called (embedded files/let bindings are materialized inside the runner).

So if notifyPeriodInSeconds is an expression referencing a let binding or a {{Task.File.*}} symbol, resolution against action_symtab fails and declared_terminate_delay returns None (the Err(_) => None arm), while the same-user path resolves it to the real value. The result: the cross-user (helper-pipe) cancel uses the legacy/positional default grace period while the same-user path uses the declared one — contradicting the invariant this function documents ("exactly what the runner will enforce on the same-user path").

Consider computing the terminate delay from the same post-let / post-embedded-file symbol table the runner uses, or documenting that expression-derived notify periods depending on let/embedded symbols are not honored on the helper path.

This looks like an issue that needs addressing.

github-actions · 2026-07-20T17:22:33Z

+        Ok(crate::runner::CancelMethod::NotifyThenTerminate { terminate_delay }) => {
+            Some(terminate_delay)
+        }
+        Ok(crate::runner::CancelMethod::Terminate) => None,


Mapping CancelMethod::Terminate to None conflates two distinct cases: "action declares TERMINATE mode" and "action declares no cancelation". They diverge on the helper path.

For a TERMINATE-mode action, the same-user runner sends an immediate SIGKILL (subprocess.rs, CancelMethod::Terminate arm). But here it yields terminate_delay = None, so in send_helper_cancel_command effective_limit = time_limit.or(None); with a non-zero time_limit (or none, falling back to the 5s default) is_terminate is false and the helper is told NOTIFY_THEN_TERMINATE with a grace period. So a TERMINATE-declared action, when canceled cross-user, gets a grace period the same-user path would never grant — the opposite of this PR's stated goal of mirroring the same-user path.

To mirror faithfully, a TERMINATE-mode action should force is_terminate on the helper path (e.g. a Some(Duration::ZERO) sentinel) rather than being indistinguishable from "undeclared".

This seems right to me though - in the Terminate case there is no declared terminate delay.

serde_derive 1.0.229+ depends on syn 3.x while the windows proc-macro crates still use syn 2.x, so the helper's multiple-versions = "deny" ban now trips on every fresh resolution. Both versions are forced by upstream; syn is a build-time proc-macro dependency and never reaches the shipped helper binary. Signed-off-by: Sean Tang <171081544+seant-aws@users.noreply.github.com>

mwiebe

I think this is ready for merge, if we flag #258 (comment) to address in a follow-up PR.

seant-aws mentioned this pull request Jul 15, 2026

fix: deliver mid-action cancels through SessionCancelHandle OpenJobDescription/openjd-model-for-python#312

Merged

github-actions Bot added the waiting-on-maintainers label Jul 15, 2026

github-actions Bot reviewed Jul 15, 2026

View reviewed changes

seant-aws mentioned this pull request Jul 15, 2026

feat: add RustSessionRuntime adapter for OpenJD v1 sessions aws-deadline/deadline-cloud-worker-agent#1002

Merged

mwiebe mentioned this pull request Jul 16, 2026

fix!: cancel handle, setup-failure reporting, plain filename, let scope #260

Merged

seant-aws force-pushed the phase1-containers branch from 5d8273b to be26ab7 Compare July 17, 2026 20:09

seant-aws changed the title ~~fix: deliver mid-action cancels to cross-user helper subprocesses~~ fix: cap helper cancel notify period at the action's declared terminate_delay Jul 17, 2026

seant-aws marked this pull request as ready for review July 17, 2026 20:58

seant-aws requested a review from a team as a code owner July 17, 2026 20:58

github-actions Bot reviewed Jul 17, 2026

View reviewed changes

mwiebe force-pushed the phase1-containers branch from e92c3ff to 30e9bdf Compare July 18, 2026 15:34

github-actions Bot reviewed Jul 18, 2026

View reviewed changes

seant-aws added 4 commits July 20, 2026 17:16

seant-aws force-pushed the phase1-containers branch from 30e9bdf to e7279ed Compare July 20, 2026 17:17

github-actions Bot reviewed Jul 20, 2026

View reviewed changes

mwiebe approved these changes Jul 20, 2026

View reviewed changes

leongdl approved these changes Jul 21, 2026

View reviewed changes

seant-aws merged commit 556d172 into OpenJobDescription:main Jul 21, 2026
22 checks passed

github-actions Bot mentioned this pull request Jul 21, 2026

chore: release #259

Merged

seant-aws deleted the phase1-containers branch July 22, 2026 18:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: cap helper cancel notify period at the action's declared terminate_delay - #258

fix: cap helper cancel notify period at the action's declared terminate_delay#258
seant-aws merged 5 commits into
OpenJobDescription:mainfrom
seant-aws:phase1-containers

seant-aws commented Jul 15, 2026 •

edited

Loading

Uh oh!

github-actions Bot Jul 15, 2026

Uh oh!

mwiebe commented Jul 16, 2026

Uh oh!

github-actions Bot Jul 17, 2026

Uh oh!

seant-aws commented Jul 17, 2026

Uh oh!

seant-aws commented Jul 17, 2026

Uh oh!

github-actions Bot Jul 18, 2026

Uh oh!

github-actions Bot Jul 20, 2026

Uh oh!

mwiebe Jul 20, 2026

Uh oh!

github-actions Bot Jul 20, 2026

Uh oh!

mwiebe Jul 20, 2026

Uh oh!

mwiebe left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

seant-aws commented Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What was the problem/requirement? (What/Why)

What was the solution? (How)

What is the impact of this change?

How was this change tested?

Was this change documented?

Is this a breaking change?

Does this change impact security?

Uh oh!

github-actions Bot Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

mwiebe commented Jul 16, 2026

Uh oh!

github-actions Bot Jul 17, 2026

Choose a reason for hiding this comment

Uh oh!

seant-aws commented Jul 17, 2026

Uh oh!

seant-aws commented Jul 17, 2026

Uh oh!

github-actions Bot Jul 18, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jul 20, 2026

Choose a reason for hiding this comment

Uh oh!

mwiebe Jul 20, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jul 20, 2026

Choose a reason for hiding this comment

Uh oh!

mwiebe Jul 20, 2026

Choose a reason for hiding this comment

Uh oh!

mwiebe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

seant-aws commented Jul 15, 2026 •

edited

Loading