fix!: cancel handle, setup-failure reporting, plain filename, let scope by mwiebe · Pull Request #260 · OpenJobDescription/openjd-rs

mwiebe · 2026-07-16T00:40:22Z

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

The Python openjd.sessions._v1 bindings hand the Rust Session to a
background thread while an action runs. Exploratory testing against the
pure-Python implementation found four bugs that trace back to this repo:

Running actions couldn't be cancelled. cancel_action needs the
Session, but the action thread owns it while an action runs — exactly
when you'd want to cancel. Every attempt failed with "session is busy".
Setup failures hung the session forever, silently. An error between
"reported Running" and "subprocess dispatched" (e.g. resolving a wrapped
action's timeout expression) left the session in Running with no error
state and no log line. Runner errors that were reported carried no
failure message.
Embedded file filename was treated as a format string. The 2023-09
schema does not mark filename as @fmtstring, and the Python reference
treats it as a plain string. Resolving it silently changed the meaning of
conformant templates (literal {{ }} in a filename) and made file-path
allocation depend on expression evaluation.
Step scripts evaluated let before allocating embedded file paths, so
Task.File.* — which decode-time validation puts in let scope — was
undefined when a let binding referenced it. Such templates validated,
then failed at runtime (silently, per bug 2). Environments already used
the correct order for Env.File.*.

What was the solution? (How)

New Session::cancel_handle() returns a thread-safe SessionCancelHandle
backed by shared per-action cancel state. It matches cancel_action's
delivery: the action's cancelation method, time_limit,
mark_action_failed, and cross-user helper pipe delivery (including
helper-routed run_subprocess). The one documented difference: it cannot
set the transient Canceling session state, since the Session is owned
by the action thread.

Credit: @seant-aws independently diagnosed and fixed the helper-routed
cancel gap in fix: cap helper cancel notify period at the action's declared terminate_delay #258. This PR adopts two pieces of that work: registering
per-action cancel state in run_subprocess_via_helper (previously a
never-fired token), and wiring the cancel-request watch receiver into the
helper subprocess config so a canceled helper subprocess classifies as
Canceled rather than Failed. fix: cap helper cancel notify period at the action's declared terminate_delay #258 also routes cancels through an
in-runner token select arm with grace = min(time_limit, terminate_delay),
which honors the action's declared cancelation config more faithfully
than the pipe-write approach here — worth converging on in whichever PR
lands second.
New fail_action_setup helper marks setup failures as a Failed action
with a fail_message, logs them, and exits Running. The runner-error path
now records fail_message and logs too.
EmbeddedFile::filename changes from Option<FormatString> to
Option<String>; the runtime uses it literally.
With filenames plain strings, allocation cannot depend on let values, so
the step runner now allocates file paths → evaluates let → writes file
contents (matching the environment runner). Task.File.* works in let
bindings; data expressions can still use let-bound values.

What is the impact of this change?

External callers can cancel running actions; setup failures surface as a
Failed action with a message instead of an infinite hang; filename behaves
per the spec; and Task.File.* / Env.File.* are usable in let bindings
with validation and runtime in agreement.

How was this change tested?

Full workspace test suite passes (openjd-model 1,828; openjd-sessions 374;
expr/cli/snapshots green).
New in-repo tests: SessionCancelHandle (cancel from another task while
the action runs, idle-cancel returns false, mark_action_failed, handle
reuse across actions without poisoning later ones, and helper-pipe
delivery — NOTIFY_THEN_TERMINATE/TERMINATE commands with auth token —
via the observable-writer hook), a forced wrap-seeding failure asserting
Failed + fail_message + callback instead of a wedged Running state, and
a sessions-layer test that a literal {{ }} filename materializes
literally even when the symbol is defined. Cancelling a helper-ROUTED
subprocess end-to-end requires a second OS user, so that case is added to
the #[ignore]-gated cross-user suites (POSIX + Windows) that run where
test users are configured.
End-to-end through rebuilt Python bindings: cancel completes in ~0.1s
(TERMINATE) and honors the notify period (NOTIFY_THEN_TERMINATE) against a
signal-immune process; the previously-hanging wrapped-env FormatString
timeout case now fails in milliseconds with a clear message; Task.File.*
in a step let resolves; the sessions_v1 scenario suite passes (38/38).
An independent code review pass was run over the diff; its findings
(helper-routed subprocess not cancellable via the handle and its error
path still able to wedge in Running, a cancel-delivery race window, and
the missing tests above) were all addressed in this revision.
Scenario/test updates: the let_bindings template's filename used the
now-removed resolution behavior and was made static, with a new
Task.File.*-in-let assertion added; test_task_file_has_path_properties
now passes at runtime, not just decode.

Was this change documented?

Yes — docstrings on the new items, and ordering/scope rules are explained at
the runner, validation, and allocation sites.

Is this a breaking change?

Yes, in the model API: EmbeddedFile::filename (template and job types)
changes from Option<FormatString> to Option<String>. Behaviorally,
{{ }} in a filename is now literal text per the spec; templates that relied
on the nonconformant resolution must move the expression into a let binding
or use a static filename. The commit carries a BREAKING CHANGE footer.
SessionCancelHandle / cancel_handle are additive.

Does this change impact security?

No. The cancel handle reuses the existing cross-user helper cancel pipe and
auth token. The filename basename check (CWE-22 defense) is retained and now
operates on a value that cannot be influenced by runtime expression
substitution at all.

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

seant-aws

good catch on the cancel handle, I've encountered the same in the linked PRs. Seems like yea, our solutions are pretty aligned.

Looks like my changes can just get folded in here, there's a few things I'd need to rebase with my openjd model changes OpenJobDescription/openjd-model-for-python#312 to use the cancel_handle()

otherwise, things look good from my standpoint

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>

leongdl · 2026-07-17T00:44:40Z

Question on change 4 (Task.File.* in step-script let bindings):

The reordering itself looks correct — with filename now a plain string, path allocation provably can't depend on let values, and it mirrors the environment runner. But the 2023-09 spec's Let Binding Scope Summary (§3.6.2) explicitly lists Env.File.* as referenceable in <EnvironmentScript>.let, while the <StepScript>.let row does not list Task.File.*. So this PR makes validation + runtime both accept something the spec text doesn't currently grant.

The spec's own rationale note for Env.File.* ("the file path is determined before evaluation, so let bindings can reference the path where the file will be written") applies identically to step scripts, so this reads like a gap in the spec table rather than an error in this PR — but as written, the implementation and the spec diverge.

Two asks:

Should this land alongside (or reference) an openjd-specifications change adding Task.File.* to the <StepScript>.let row of the scope summary table?
Would it make sense to add a conformance fixture for Task.File.*-in-let to the spec repo so the Rust and Python implementations can't silently diverge on this?

leongdl

Have a question about the following:

Fix 4 (let ordering) — correct implementation, but one spec-conformance flag worth raising. The reordering (allocate paths → evaluate let →
write contents) is sound: with filename now a plain string, allocation provably cannot depend on let values, and data is written after let
so it can still use bindings. It mirrors the environment runner exactly. However, the spec's Let Binding Scope Summary (§3.6.2) explicitly
lists Env.File.* as referenceable in .let but does not list Task.File.* for .let. The PR makes Rust
validation + runtime both accept it. Given the spec's own rationale note ("the file path is determined before evaluation, so let bindings
can reference the path") applies identically to steps, this looks like a spec-table gap rather than a PR error — but it means the Rust
implementation now intentionally accepts something the spec text doesn't grant. The PR (or a companion openjd-specifications change) should
add Task.File.* to the .let row so the implementations and spec don't diverge. Same reasoning applies to conformance fixtures:
a Task.File.*-in-let fixture in the spec repo would pin this cross-implementation.

Given the ambiguity even with AI, how can we fix this in the spec and the code? Seems we have an edge case

mwiebe · 2026-07-17T17:47:36Z

Two asks:

Should this land alongside (or reference) an openjd-specifications change adding Task.File.* to the <StepScript>.let row of the scope summary table?

Would it make sense to add a conformance fixture for Task.File.*-in-let to the spec repo so the Rust and Python implementations can't silently diverge on this?

Yes, this feels like an error we made when putting that in the specification. Let's fix that up. How do you feel about merging this now, and having that be a follow-up?

mwiebe · 2026-07-17T18:25:32Z

Two asks:

Should this land alongside (or reference) an openjd-specifications change adding Task.File.* to the <StepScript>.let row of the scope summary table?

Would it make sense to add a conformance fixture for Task.File.*-in-let to the spec repo so the Rust and Python implementations can't silently diverge on this?

Yes, this feels like an error we made when putting that in the specification. Let's fix that up. How do you feel about merging this now, and having that be a follow-up?

PR is here: OpenJobDescription/openjd-specifications#152

… in environments The Let Binding Scope Summary granted Env.File.* to <EnvironmentScript>.let but omitted Task.File.* from <StepScript>.let. The rationale for the Env case — embedded file paths are determined before let evaluation — applies identically to step scripts, and implementations already validate and evaluate Task.File.* in step-script let bindings (OpenJobDescription/openjd-rs#260). - Add Task.File.* to the <StepScript>.let scope row and generalize the embedded-file note to cover both contexts (2023-09 Template Schemas 3.6.2) - Align RFC 0005's ScriptTemplate let description, which granted embedded file symbols to environment scripts via example but never stated the step-script side - Exercise Task.File.* in the 3.6 let-host-context-symbols validation fixture (its header comment already claimed this coverage) and add a 7.3 runtime test mirroring the existing Env.File one Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com>

…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>

Import-failure handling: add tests proving the session runtime factory raises NotImplementedError (chaining the original ImportError) when the Rust adapter module cannot be imported, never silently falls back to the Python adapter on a Rust import failure, and still constructs the Python runtime when the Rust import is broken. Differential integration tests: run the same scenarios through both the Python and Rust runtime adapters and assert identical observable behavior: environment enter/run/exit, cancel mid-action, cleanup after failure, attachment-sync, env-var mutation, and embedded-file path resolution. The Rust cancel scenario is marked xfail (non-strict): the published _v1 binding cannot cancel an in-flight action. The fix is merged in the Rust core (OpenJobDescription/openjd-rs#260) but not yet in a published openjd-sessions release; remove the xfail with the pin bump. Signed-off-by: Sean Tang <171081544+seant-aws@users.noreply.github.com>

…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>

Import-failure handling: add tests proving the session runtime factory raises NotImplementedError (chaining the original ImportError) when the Rust adapter module cannot be imported, never silently falls back to the Python adapter on a Rust import failure, and still constructs the Python runtime when the Rust import is broken. Differential integration tests: run the same scenarios through both the Python and Rust runtime adapters and assert identical observable behavior: environment enter/run/exit, cancel mid-action, cleanup after failure, attachment-sync, env-var mutation, and embedded-file path resolution. The Rust cancel scenario is marked xfail (non-strict): the published _v1 binding cannot cancel an in-flight action. The fix is merged in the Rust core (OpenJobDescription/openjd-rs#260) but not yet in a published openjd-sessions release; remove the xfail with the pin bump. Signed-off-by: Sean Tang <171081544+seant-aws@users.noreply.github.com>

…te_delay (#258) * fix: cap helper cancel notify period at the action's declared terminate_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 #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> * test: surface subprocess errors and fix fixed-delay race in cross-user 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> * fix: fail closed when the working-directory DACL grant cannot be applied 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> * test: root cross-user Windows test sessions outside the profile temp dir 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> * chore: skip duplicate syn versions in helper cargo-deny bans check 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> --------- Signed-off-by: Sean Tang <171081544+seant-aws@users.noreply.github.com>

Import-failure handling: add tests proving the session runtime factory raises NotImplementedError (chaining the original ImportError) when the Rust adapter module cannot be imported, never silently falls back to the Python adapter on a Rust import failure, and still constructs the Python runtime when the Rust import is broken. Differential integration tests: run the same scenarios through both the Python and Rust runtime adapters and assert identical observable behavior: environment enter/run/exit, cancel mid-action, cleanup after failure, attachment-sync, env-var mutation, and embedded-file path resolution. The Rust cancel scenario is marked xfail (non-strict): the published _v1 binding cannot cancel an in-flight action. The fix is merged in the Rust core (OpenJobDescription/openjd-rs#260) but not yet in a published openjd-sessions release; remove the xfail with the pin bump. Signed-off-by: Sean Tang <171081544+seant-aws@users.noreply.github.com>

mwiebe requested a review from a team as a code owner July 16, 2026 00:40

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

github-actions Bot reviewed Jul 16, 2026

View reviewed changes

Comment thread crates/openjd-sessions/src/session.rs Outdated

mwiebe force-pushed the fix/cancel-handle-setup-errors-filename-let-scope branch 3 times, most recently from 5486b79 to 30d4618 Compare July 16, 2026 01:01

seant-aws approved these changes Jul 16, 2026

View reviewed changes

mwiebe force-pushed the fix/cancel-handle-setup-errors-filename-let-scope branch from 30d4618 to 9da7ef1 Compare July 16, 2026 23:22

leongdl reviewed Jul 17, 2026

View reviewed changes

leongdl approved these changes Jul 17, 2026

View reviewed changes

mwiebe merged commit efe844a into OpenJobDescription:main Jul 17, 2026
51 of 54 checks passed

mwiebe deleted the fix/cancel-handle-setup-errors-filename-let-scope branch July 17, 2026 17:50

mwiebe mentioned this pull request Jul 17, 2026

fix: allow Task.File.* in <StepScript>.let, matching Env.File.* in environments OpenJobDescription/openjd-specifications#152

Merged

seant-aws mentioned this pull request Jul 17, 2026

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

Merged

seant-aws mentioned this pull request Jul 17, 2026

test: add Rust runtime import-failure and differential integration tests aws-deadline/deadline-cloud-worker-agent#1015

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

fix!: cancel handle, setup-failure reporting, plain filename, let scope#260
mwiebe merged 1 commit into
OpenJobDescription:mainfrom
mwiebe:fix/cancel-handle-setup-errors-filename-let-scope

mwiebe commented Jul 16, 2026

Uh oh!

Uh oh!

seant-aws left a comment

Uh oh!

leongdl commented Jul 17, 2026

Uh oh!

leongdl left a comment

Uh oh!

mwiebe commented Jul 17, 2026

Uh oh!

Uh oh!

mwiebe commented Jul 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

mwiebe commented Jul 16, 2026

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!

Uh oh!

seant-aws left a comment

Choose a reason for hiding this comment

Uh oh!

leongdl commented Jul 17, 2026

Uh oh!

leongdl left a comment

Choose a reason for hiding this comment

Uh oh!

mwiebe commented Jul 17, 2026

Uh oh!

Uh oh!

mwiebe commented Jul 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants