test: timezone information was dropped when parsing the cancel NotifyEnd#261
Merged
Merged
Conversation
lucaseck
previously approved these changes
Aug 21, 2025
Signed-off-by: Morgan Epp <60796713+epmog@users.noreply.github.com>
epmog
force-pushed
the
fix_timezone_in_notify
branch
from
August 21, 2025 17:07
2092846 to
ecddafb
Compare
|
epmog
enabled auto-merge (squash)
August 21, 2025 17:10
marofke
approved these changes
Aug 21, 2025
lucaseck
approved these changes
Aug 21, 2025
mwiebe
approved these changes
Aug 21, 2025
leongdl
added a commit
to leongdl/openjd-sessions-for-python
that referenced
this pull request
Jul 16, 2026
Change WrappedAction.Cancelation.Mode from an empty-string sentinel to None (string?) when the wrapped action defines no <Cancelation>, matching the EXPR semantics for optional data and the int? typing of Cancelation.NotifyPeriodInSeconds. None renders as the empty string in format-string interpolation, and the nullness is observable via EXPR null-coalescing. Tests assert `is None` for the undeclared case and add an execution test that the null mode interpolates as MODE=<>, mirroring the updated wrap-cancelation-mode-null-when-no-cancelation conformance fixture in openjd-specifications and the Rust change in openjd-rs. Addresses review feedback on openjd-rs PR OpenJobDescription#261 (discussion r3597572812) and openjd-specifications PR OpenJobDescription#148 (r3597560515). Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
leongdl
added a commit
to leongdl/openjd-sessions-for-python
that referenced
this pull request
Jul 16, 2026
Change WrappedAction.Cancelation.Mode from an empty-string sentinel to
None (string?) when the wrapped action defines no <Cancelation>,
matching the EXPR semantics for optional data and the int? typing of
Cancelation.NotifyPeriodInSeconds. None renders as the empty string in
format-string interpolation, and the nullness is observable via EXPR
null-coalescing.
Also address review findings on the wrap-action implementation:
- Fail gracefully when WrappedAction.* injection cannot resolve the
wrapped action's format strings (e.g. a wrapped onRun referencing
{{Task.File.*}} or onEnter referencing {{Env.File.*}} — embedded
files are not materialized on the wrap path yet, a known limitation
shared with the Rust runtime). The action now FAILs through the
normal callback path via a new _fail_action_before_start helper and
the session transitions to READY_ENDING, instead of a raw
FormatStringError escaping the public API — which, for
enter_environment, previously left the session stuck in RUNNING
with no runner. Also covers non-integer FEATURE_BUNDLE_1
timeout/notifyPeriod resolutions.
- WrappedAction.Environment now carries only openjd_env-defined
variables per RFC 0008, excluding the environment's declarative
variables: map seed (parity with the Rust runtime, which was
already correct).
- Log a warning when a wrap environment is active but run_task() was
not given step_name, since {{WrappedStep.Name}} renders empty.
- Hoist the Template Schemas 5.3.2 notify-period defaults (120/30)
into shared constants used by the seeding and both cancel paths.
- Guard _run_wrap_hook against unknown hook names so a typo cannot
become a silent SUCCESS no-op.
Tests assert `is None` for the undeclared cancelation case, the
injection-failure and READY_ENDING behavior, the openjd_env-only
Environment contents, and the hook-name guard. All 53 WRAP_ACTIONS
conformance tests pass against the Python CLI.
Addresses review feedback on openjd-rs PR OpenJobDescription#261 (discussion
r3597572812) and openjd-specifications PR OpenJobDescription#148 (r3597560515).
Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
leongdl
added a commit
to leongdl/openjd-sessions-for-python
that referenced
this pull request
Jul 17, 2026
Change WrappedAction.Cancelation.Mode from an empty-string sentinel to
None (string?) when the wrapped action defines no <Cancelation>,
matching the EXPR semantics for optional data and the int? typing of
Cancelation.NotifyPeriodInSeconds. None renders as the empty string in
format-string interpolation, and the nullness is observable via EXPR
null-coalescing.
Also address review findings on the wrap-action implementation:
- Fail gracefully when WrappedAction.* injection cannot resolve the
wrapped action's format strings (e.g. a wrapped onRun referencing
{{Task.File.*}} or onEnter referencing {{Env.File.*}} — embedded
files are not materialized on the wrap path yet, a known limitation
shared with the Rust runtime). The action now FAILs through the
normal callback path via a new _fail_action_before_start helper and
the session transitions to READY_ENDING, instead of a raw
FormatStringError escaping the public API — which, for
enter_environment, previously left the session stuck in RUNNING
with no runner. Also covers non-integer FEATURE_BUNDLE_1
timeout/notifyPeriod resolutions.
- WrappedAction.Environment now carries only openjd_env-defined
variables per RFC 0008, excluding the environment's declarative
variables: map seed (parity with the Rust runtime, which was
already correct).
- WrappedAction.Environment is flattened cumulatively in
environment-entry order, so a later openjd_env set yields one
effective entry (not a duplicate) and a later unset removes the
name — matching the real subprocess environment and Rust's single
cumulative env_vars map.
- On the onWrapEnvExit path, the openjd_env list is captured before
the exiting environment is removed from tracking, so the wrapped
environment's own openjd_env variables appear in
WrappedAction.Environment just as they do in the real subprocess
environment.
- Log a warning when a wrap environment is active but run_task() was
not given step_name, since {{WrappedStep.Name}} renders empty.
- Hoist the Template Schemas 5.3.2 notify-period defaults (120/30)
into shared constants used by the seeding and both cancel paths.
- Guard _run_wrap_hook against unknown hook names so a typo cannot
become a silent SUCCESS no-op.
Tests assert `is None` for the undeclared cancelation case, the
injection-failure and READY_ENDING behavior, the openjd_env-only
Environment contents (including override/unset flattening and the
exit-path capture), and the hook-name guard. All 53 WRAP_ACTIONS
conformance tests pass against the Python CLI.
Addresses review feedback on openjd-rs PR OpenJobDescription#261 (discussion
r3597572812) and openjd-specifications PR OpenJobDescription#148 (r3597560515).
Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
leongdl
added a commit
to leongdl/openjd-sessions-for-python
that referenced
this pull request
Jul 17, 2026
Runtime half of RFC 0008 cancelation/timeout round-trip forwarding (openjd-rs PR OpenJobDescription#261 review discussion r3597453516; model change in openjd-model-for-python's CancelationMethodDeferred): timeout: "{{WrappedAction.Timeout}}" cancelation: mode: "{{WrappedAction.Cancelation.Mode}}" notifyPeriodInSeconds: "{{WrappedAction.Cancelation.NotifyPeriodInSeconds}}" The problem: format strings are normally delay-processed, but a cancelation's `mode` is the schema selector — the parser needs it at parse time, while a forwarded value only exists at run time. The model now carries such a mode as CancelationMethodDeferred, and this change is where the deferred decision lands: a new shared resolve_effective_cancelation helper (see its docstring for the full story) resolves the mode against the live symbol table right when it is needed. Null (the wrapped action declared no <Cancelation>) drops the whole cancelation object; a resolved TERMINATE rejects a non-null notify period; anything else fails. Notify periods resolve with int? whole-field semantics (null = field omitted, positional default). - The seed path (_inject_wrapped_cancelation_symbols) and both cancel enforcement paths (StepScriptRunner.cancel, EnvironmentScriptRunner.cancel) resolve through the same helper, so the values wrap scripts see always match what the runtime enforces. Cancels fall back to Terminate with a warning if resolution fails, since a cancel must always proceed. - WrappedAction.Timeout is now int? — None (null) when the wrapped action specifies no timeout — replacing the 0 sentinel, per the spec change; whole-field forwarding drops the field so the hook position's default applies. - _run_action now resolves a FormatString timeout against the symtab right before launch (previously it passed the FormatString object straight into timedelta), treating a null resolution as "no timeout field" and failing the action on non-positive-integer resolutions. Tests updated for the int?/None timeout; all wrap/cancel suites pass, and all 58 WRAP_ACTIONS conformance fixtures (including the new wrap-cancelation-roundtrip-* and timeout-null cases) pass against the Python CLI. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
leongdl
added a commit
to leongdl/openjd-sessions-for-python
that referenced
this pull request
Jul 17, 2026
Runtime half of RFC 0008 cancelation/timeout round-trip forwarding (openjd-rs PR OpenJobDescription#261 review discussion r3597453516; model change in openjd-model-for-python's CancelationMethodDeferred): timeout: "{{WrappedAction.Timeout}}" cancelation: mode: "{{WrappedAction.Cancelation.Mode}}" notifyPeriodInSeconds: "{{WrappedAction.Cancelation.NotifyPeriodInSeconds}}" The problem: format strings are normally delay-processed, but a cancelation's `mode` is the schema selector — the parser needs it at parse time, while a forwarded value only exists at run time. The model now carries such a mode as CancelationMethodDeferred, and this change is where the deferred decision lands: a new shared resolve_effective_cancelation helper (see its docstring for the full story) resolves the mode against the live symbol table right when it is needed. Null (the wrapped action declared no <Cancelation>) drops the whole cancelation object; a resolved TERMINATE rejects a non-null notify period; anything else fails. Notify periods resolve with int? whole-field semantics (null = field omitted, positional default). - The seed path (_inject_wrapped_cancelation_symbols) and both cancel enforcement paths (StepScriptRunner.cancel, EnvironmentScriptRunner.cancel) resolve through the same helper, so the values wrap scripts see always match what the runtime enforces. Cancels fall back to Terminate with a warning if resolution fails, since a cancel must always proceed. - WrappedAction.Timeout is now int? — None (null) when the wrapped action specifies no timeout — replacing the 0 sentinel, per the spec change; whole-field forwarding drops the field so the hook position's default applies. - _run_action now resolves a FormatString timeout against the symtab right before launch (previously it passed the FormatString object straight into timedelta), treating a null resolution as "no timeout field" and failing the action on non-positive-integer resolutions. Tests updated for the int?/None timeout; all wrap/cancel suites pass, and all 58 WRAP_ACTIONS conformance fixtures (including the new wrap-cancelation-roundtrip-* and timeout-null cases) pass against the Python CLI. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
leongdl
added a commit
to leongdl/openjd-sessions-for-python
that referenced
this pull request
Jul 17, 2026
Runtime half of RFC 0008 cancelation/timeout round-trip forwarding (openjd-rs PR OpenJobDescription#261 review discussion r3597453516; model change in openjd-model-for-python's CancelationMethodDeferred): timeout: "{{WrappedAction.Timeout}}" cancelation: mode: "{{WrappedAction.Cancelation.Mode}}" notifyPeriodInSeconds: "{{WrappedAction.Cancelation.NotifyPeriodInSeconds}}" The problem: format strings are normally delay-processed, but a cancelation's `mode` is the schema selector — the parser needs it at parse time, while a forwarded value only exists at run time. The model now carries such a mode as CancelationMethodDeferred, and this change is where the deferred decision lands: a new shared resolve_effective_cancelation helper (see its docstring for the full story) resolves the mode against the live symbol table right when it is needed. Null (the wrapped action declared no <Cancelation>) drops the whole cancelation object; a resolved TERMINATE rejects a non-null notify period; anything else fails. Notify periods resolve with int? whole-field semantics (null = field omitted, positional default). - The seed path (_inject_wrapped_cancelation_symbols) and both cancel enforcement paths (StepScriptRunner.cancel, EnvironmentScriptRunner.cancel) resolve through the same helper, so the values wrap scripts see always match what the runtime enforces. Cancels fall back to Terminate with a warning if resolution fails, since a cancel must always proceed. - WrappedAction.Timeout is now int? — None (null) when the wrapped action specifies no timeout — replacing the 0 sentinel, per the spec change; whole-field forwarding drops the field so the hook position's default applies. - _run_action now resolves a FormatString timeout against the symtab right before launch (previously it passed the FormatString object straight into timedelta), treating a null resolution as "no timeout field" and failing the action on non-positive-integer resolutions. Tests updated for the int?/None timeout; all wrap/cancel suites pass, and all 58 WRAP_ACTIONS conformance fixtures (including the new wrap-cancelation-roundtrip-* and timeout-null cases) pass against the Python CLI. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
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
4 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.
What was the problem/requirement? (What/Why)
1 test was failing locally for me on my macOS machine -
test_cancel_notify.hint: 18000seconds is 5 hours or GMT-5
At first i thought there was some nuance in datetimes on macOS vs. other OSes. But it turns out that this fails on Windows too if the timezone for the machine isn't UTC.
What was the solution? (How)
We were dropping the timezone information ('Z') from the cancel notify file and so python was interpreting it as the local time before then changing to UTC. Didn't discover this previously since all of our runners are on UTC.
What is the impact of this change?
All non-cross-user tests run locally on macOS (and other OSes if you're not in UTC)!
How was this change tested?
Was this change documented?
N/A
Is this a breaking change?
No
Does this change impact security?
No
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.