Repository navigation
Conversation
A Session is meant to be brittle: once an action ends FAILED, CANCELED or TIMEOUT, it only accepts exit_environment and cleanup. The session moved to READY_ENDING when an action failed, but it didn't set _ending_only, which only exit_environment(keep_session_running=False) set. So after a failed task, exit_environment(keep_session_running=True) on an environment whose onExit succeeded, or that had no onExit, put the session back in READY, and it accepted new tasks and environments. _action_callback and _fail_action_before_start now set _ending_only when an action doesn't succeed, and the between-action state is derived from it. The exit_environment and SessionState.READY_ENDING docstrings describe the rule. This is the same bug and fix as OpenJobDescription/openjd-rs#287. Tests: test/openjd/sessions_v0/test_brittle_session.py (20 tests). At 2eb39a5, 12 fail: every case that passes keep_session_running=True after a failure. Full suite: 1029 passed, 46 skipped, 16 xfailed. black, ruff and mypy pass. Signed-off-by: Mark Wiebe <399551+mwiebe@users.noreply.github.com>
A failed, canceled or timed-out action, or exit_environment without keep_session_running, makes a Session ending-only. keep_session_running=True now only means "don't make the Session ending-only on this exit". It never brings an ending-only Session back to READY. Before the previous commit it did, quietly, after a failed action. The only way from READY_ENDING back to READY is the new public method Session.clear_ending_only(). It is allowed only in READY_ENDING and raises RuntimeError in any other state, like the other state checks. It clears ending-only mode whatever set it and changes nothing else: the entered Environments, the last action's status and the environment variables stay as they were. Its docstring warns that by default a Session only allows shutdown after a failure, and that callers who keep running work after that take responsibility for the Session's state. The keep_session_running and SessionState.READY_ENDING docstrings now say this. 16 new tests in test_brittle_session.py: for each of FAILED, CANCELED and TIMEOUT, with and without a keep_session_running=True exit first, the clear returns the Session to READY, new tasks and Environments work, the remaining Environments' variables still reach new tasks, and an exited Environment stays exited; it clears ending-only set by a default exit; it keeps the constructor's os_env_vars; a later failure makes the Session ending-only again; it raises in READY, RUNNING, CANCELING and ENDED, and a refusal leaves ending-only set. This matches the same change in openjd-rs (OpenJobDescription/openjd-rs#287). BREAKING CHANGE: exit_environment(keep_session_running=True) no longer brings a Session back to READY after a FAILED, CANCELED or TIMEOUT action. A caller that relied on it to keep going after a failure must call Session.clear_ending_only() first. Signed-off-by: Mark Wiebe <399551+mwiebe@users.noreply.github.com>
An independent review of 30c1b2e found no high or medium issues. This commit takes its two low items and matches two test gaps found in the same change in openjd-rs. - test_clear_keeps_action_status: the failing task reports progress, a status message and openjd_fail first, so comparing the action status before and after clear_ending_only() can't pass by comparing two Nones. Clearing any of the three fields in clear_ending_only() is now caught. - test_clear_after_failure_before_start: ending-only set by _fail_action_before_start clears the same way. - _assert_accepts_new_actions also runs a subprocess after the clear. - The keep_session_running docstring now says an onExit that ends FAILED, CANCELED or TIMEOUT makes the Session ending-only, not only a failing one. Full suite 1047 passed, 46 skipped, 16 xfailed; black, ruff and mypy pass. Signed-off-by: Mark Wiebe <399551+mwiebe@users.noreply.github.com>
|
Claude review · advisory Reviewed Open: 0 blocking · 0 should-fix · 0 nit. ✅ Nothing blocking. Fix or reply to each thread; the next revision's review re-checks open threads and resolves those it agrees are handled. Resolving a thread yourself also closes it. Later revisions review only what changed. |
mwiebe
enabled auto-merge (squash)
November 10, 2026 20:06
Between actions, Session._ending_only and _state == READY_ENDING held the
same information, and the brittle-session bug fixed earlier on this branch
was the two drifting apart. Only the flag has to stay, because it is what
remembers ending-only mode through RUNNING. The Session now never stores
READY_ENDING in _state: the state property reports a stored READY with
_ending_only set as READY_ENDING, and every other state as stored. The
same change is made in openjd-rs.
- _action_callback and _fail_action_before_start set the flag for a
failed, canceled or timed-out action, then store READY. The flag is set
first, so a thread polling the state property never sees READY for a
failed action.
- clear_ending_only() checks the state property, as before, and clears
the flag only. The stored state is already READY.
- Every internal check and error message already used the state property,
so none of them changes.
Behavior is unchanged in every state the Session reaches through its
public API. The one difference is in exit_environment() without
keep_session_running: it sets the flag before it stores RUNNING, so while
that call is setting up the exit (or if it raises an unexpected exception
before storing RUNNING) the state reads READY_ENDING where it used to read
READY.
Tests: one existing test changes its mechanism, as in openjd-rs.
TestClearEndingOnly.test_refusal_keeps_ending_only set _state = READY on
an ending-only Session to run a task and check it settled to READY_ENDING.
A stored READY with the flag set now reads READY_ENDING, so run_task
refuses; the test now forces READY and asserts the state property is
READY_ENDING. Its refusal assertions are unchanged, and no other existing
test changed. New TestEndingOnlyStoredInFlag pins the state property's
mapping over every stored state and flag value, checks that a failed
action, a failure before start and a default exit store READY with the
flag set, that clear_ending_only() clears only the flag, and that
run_subprocess's error names the derived state ("Current state:
ready_ending").
Full suite 1058 passed, 46 skipped, 16 xfailed; black, ruff and mypy
pass.
Signed-off-by: Mark Wiebe <399551+mwiebe@users.noreply.github.com>
An independent review of 716c90d found one medium and two low items. - Medium: the state property read _state twice, once to check for READY and once to return it. If the action callback ran on another thread between the two reads, setting _ending_only and storing READY, the property returned READY for a failed action. A trace hook that paused the reader between the reads reproduced it every time (the parent a299030 returned READY_ENDING). The property now reads _state once. Because _ending_only is always set before READY is stored for a failed action, a single read never reports READY for one. New test test_poller_never_sees_ready_for_a_failed_action runs the callback inside the property's read and fails against the double read. - Low: nothing tested that the flag is set before READY is stored. New test test_flag_set_before_ready_is_stored records the writes from _action_callback and _fail_action_before_start and fails if either stores READY first. - Low: the previous commit message's description of the exit_environment difference holds when the Session starts from READY; if it was already ending-only, both versions read READY_ENDING. It also didn't say that after an exception raised from that window, run_task is now refused where it used to be accepted from a READY state with the flag set. Full suite 1060 passed, 46 skipped, 16 xfailed; black, ruff and mypy pass. Signed-off-by: Mark Wiebe <399551+mwiebe@users.noreply.github.com>
A second independent review of a299030..83a62a9 was clean (no high or medium items). This commit takes its low items. - The state property's comment now says that a reader paused between its read of _state and its read of _ending_only, while a whole failing action runs on another thread, can report READY_ENDING a moment early while _state is still RUNNING. It never reports READY for a failed action; reading the flag first would bring that back. - For the record, the full list of public calls whose result differs after exit_environment(keep_session_running=False) raises an exception before it stores RUNNING (the Session reads READY_ENDING where it read READY): run_task, enter_environment and run_subprocess are now refused where they were accepted, and clear_ending_only() is now accepted where it was refused. Comment-only change; black and ruff pass, and test_brittle_session.py passes (51). Signed-off-by: Mark Wiebe <399551+mwiebe@users.noreply.github.com>
This is the one behavior change in the otherwise behavior-neutral refactor in 716c90d, and this message replaces that commit's "behavior is unchanged" wording: a failed default exit_environment() now leaves the Session READY_ENDING instead of READY. exit_environment() without keep_session_running sets _ending_only before it stores RUNNING. If it raises in between, the Session used to report READY with the flag set, so a later run_task, enter_environment or run_subprocess was accepted and clear_ending_only() was refused. That is the same "READY while ending-only" drift the refactor removes. It now reports READY_ENDING: those three calls are refused, clear_ending_only() is accepted, and only exits and cleanup remain, which is what the exit asked for. Calls with correctly typed arguments don't raise in that window. Only wrongly typed arguments reach it through the public API, for example a resolved_symtab that isn't a SerializedSymbolTable, or os_env_vars with a non-string key. So the new test, test_default_exit_that_raises_leaves_ready_ending, injects the failure into _evaluate_current_session_env_vars, then checks that the stored state is READY, the state property reads READY_ENDING, and run_task, enter_environment and run_subprocess are refused. It fails at a299030, where the state reads READY. Full suite 1061 passed, 46 skipped, 16 xfailed; black, ruff and mypy pass. Signed-off-by: Mark Wiebe <399551+mwiebe@users.noreply.github.com>
This branch has not been deployed
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
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.
Related: OpenJobDescription/openjd-rs#287
Related PRs:
What was the problem/requirement? (What/Why)
Sessions are meant to be "brittle": once an action ends
FAILED,CANCELEDorTIMEOUT, the session should accept onlyexit_environmentandcleanup. But onlyexit_environment(keep_session_running=False)set the private_ending_onlyflag, so after a failed task a laterexit_environment(keep_session_running=True)quietly returned the session toREADY.keep_session_runningexists to run tasks from another step after exiting a step's environments, not to recover from a failure, and there was no explicit way to do that.What was the solution? (How)
_action_callbackand_fail_action_before_startset_ending_onlywhen an action doesn't succeed._ending_only._statenever holdsREADY_ENDING; thestateproperty derives it from a storedREADYplus the flag, so the two can't disagree.keep_session_running=Truenow means only "don't make the Session ending-only on this exit". It never brings an ending-only Session back toREADY.Session.clear_ending_only(), the only way fromREADY_ENDINGback toREADY. It clears ending-only mode whatever set it: a failed or canceled action, or a defaultexit_environment. It changes nothing else: entered environments, the last action status and environment variables stay as they were. In any other state it raisesRuntimeError, like the other state checks.What is the impact of this change?
After an action fails, is canceled or times out, the Session stays
READY_ENDINGuntil the caller callsclear_ending_only()orcleanup(). Sessions where every action succeeds behave as before. Storing ending-only mode only in the flag changes one more case: ifexit_environment()withoutkeep_session_runningraises before its action starts, the Session is nowREADY_ENDINGinstead ofREADY, so it refuses new work as that exit asked. The Rust-backedopenjd.sessions._v1gets the method when the bindings move to an openjd-sessions crate release that has it.How was this change tested?
black --check,ruff checkandmypy src testpass.test/openjd/sessions_v0/test_brittle_session.pyhas 52 tests. 20 cover the ending-only rule; without the fix 12 fail. 18 coverclear_ending_only(): after each failing state, with and without akeep_session_running=Trueexit first; after a default exit; refusal in every other state; and that environments, status and env vars are kept. 14 cover how thestateproperty derivesREADY_ENDINGfrom the flag, including a default exit that raises and a reader racing the action callback.Was this change documented?
clear_ending_only,exit_environmentandSessionState.READY_ENDING.Is this a breaking change?
Yes. A caller that used
keep_session_running=Trueto keep running work after a failure now gets aRuntimeErrorsaying the Session must be in theREADYstate. It must callclear_ending_only()first.Does this change impact security?
No.
Cross-port to openjd-rs
This package is being migrated to Rust in
openjd-rs/crates/openjd-sessions.Behavioral changes made here should be replicated there to keep the two
implementations in sync until the migration is complete.
openjd-rs(link the PR here): fix(sessions)!: keep a session ending-only after a failed action, and add clear_ending_only() openjd-rs#524BREAKING CHANGE:
exit_environment(keep_session_running=True)no longer brings a Session back toREADYafter aFAILED,CANCELEDorTIMEOUTaction. A caller that relied on it to keep going after a failure must callSession.clear_ending_only()first.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.