fix: Subprocess ownership on every exit path, plus five RFC 0008 cross-check fixes - #341
Open
leongdl wants to merge 12 commits into
Open
fix: Subprocess ownership on every exit path, plus five RFC 0008 cross-check fixes#341leongdl wants to merge 12 commits into
leongdl wants to merge 12 commits into
Conversation
An unanchored `^(openjd_env|openjd_redacted_env|openjd_unset_env)` treats any line merely starting with one of those tokens as a malformed macro, and the filter then fails the action with cancel_action_mark_failed=True. So a script logging `openjd_environment: ready` or `openjd_env_setup: done` kills its own task, with no adversary involved. openjd-rs does not have this bug: is_malformed_env_command (action_filter.rs) anchors each of the same three tokens on ':', a space, or end-of-string, and its comment names exactly this false positive. In Rust the line falls through parse_directive as an unrecognised kind and passes through as ordinary log output. Adds the equivalent `(?::|\s|\Z)` tail. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> (cherry picked from commit ede6cad)
terminate() guarded on `proc.poll() is None`, so once the wrapper leader had exited it did nothing -- even though _sudo_child_process_group_id was recorded and children could still be alive. killpg on a process group still reaches surviving members after the leader is reaped, so the guard made the call a no-op exactly when the recorded group was the only remaining handle on the survivors, orphaning them past terminal publication. A wrap environment is precisely the shape that produces an exited leader with a live workload, so this PR increases exposure to it. openjd-rs never gates: every cancel/timeout/reap-failure path calls send_terminate(pid) -> killpg unconditionally, and its is_process_alive() probe is #[allow(dead_code)] for this reason. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> (cherry picked from commit c22621b)
TTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTuard, and all four call sites run before their runner exists -- so an OSError escaped run_task / enter_environment / exit_environment with no callback and no ActionStatus. Now translated to RuntimeError and routed to _fail_action_before_start at all four sites, which publishes FAILED/READY_ENDING and notifies. enter_environment still returns its identifier on this path, matching its existing resolve-variables failure contract, because the environment is already on the entered stack and the caller must be able to exit it. openjd-rs supplies the contract: std::fs::write maps to SessionError::WorkingDirectory and every call site maps that to fail_action_setup(), which sets Failed + notifies rather than propagating. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> (cherry picked from commit c5d2927)
mkstemp + write_file_for_user had no guard, and all four call sites run before their runner exists -- so an OSError escaped run_task / enter_environment / exit_environment with no callback and no ActionStatus. Now translated to RuntimeError and routed to _fail_action_before_start at all four sites, which publishes FAILED/READY_ENDING and notifies. enter_environment still returns its identifier on this path, matching its existing resolve-variables failure contract, because the environment is already on the entered stack and the caller must be able to exit it. openjd-rs supplies the contract: std::fs::write maps to SessionError::WorkingDirectory and every call site maps that to fail_action_setup(), which sets Failed + notifies rather than propagating. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> (cherry picked from commit d4748ab)
rfcs/0008-environment-wrap-actions.md:404-412 requires that runtimes "MUST
include in `WrappedAction.Environment` every `openjd_env`-defined variable
emitted by any earlier action in the same session -- regardless of whether that
action ran normally or via a wrap hook -- and every entry of each entered
environment's declarative `variables:` map."
_collect_session_env_list() iterated `_environments_entered`, which
exit_environment pops, so an earlier environment's export vanished from the
symbol the moment that environment exited. A wrap script forwarding
WrappedAction.Environment into a container therefore silently dropped variables
the spec guarantees it. Reproduced first: the new test fails against the old
code and passes against the new.
Fixed by mirroring openjd-rs, which holds two maps deliberately:
_session_env_vars session-lifetime, feeds WrappedAction.Environment. Written
by declarative `variables:` and by openjd_env; the only
remover is an explicit openjd_unset_env, not an
environment exit. (Rust: `env_vars`.)
_created_env_vars per environment, read through _environments_entered to
build the child *process* environment -- this view must
shrink on exit, and still does. (Rust: `created_env_vars`.)
Following Rust also settles the MUST's ambiguous second clause: Rust writes
declarative `variables:` into its session-lifetime map at the same point it
writes them per-environment, so both halves are session-scoped here too.
Tests, and two pre-existing tests corrected:
Five behaviours are now pinned, one per fix on this branch, and all six mutants
are caught (revert the fix, a named test fails):
- an exited environment's export is still listed, with an explicit unset as
the negative control so retention does not become "nothing is removed"
- a longer token like `openjd_environment: ready` is ordinary output, with a
genuine near-miss still failing the action as the control
- terminate() signals after the leader exits, with the released-handle case as
the control
- a cancel_info write failure is contained for both OSError and RuntimeError
- a rules-file write failure publishes FAILED instead of raising
test_wrap_actions.py::test_later_set_overrides_earlier_value_without_duplicate
and test_wrap_task_run.py::test_injects_wrapped_environment_as_key_value_list
both fabricated `_environments_entered` entries and assigned `_created_env_vars`
directly, bypassing the production writers entirely -- which is precisely why
they kept passing while the MUST was being violated. Both now drive the real
path. A test that never exercises the writers cannot notice the writers being
wrong.
878 passed / 39 skipped / 16 xfailed (861 before, +17 new). ruff, black and mypy
clean on native and --platform win32.
Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
(cherry picked from commit b7f0986)
`LoggingSubprocess.run()` logged "Command started as pid" and "Output:", and did the POSIX process-group lookup, *before* entering the `try` whose `finally` reaps the child. Anything raising in that window escaped `run()` having never waited on the child: a live process, no exit code recorded, and the runner publishing a terminal state for it anyway. The reachable trigger is a `logging.Filter` that raises. Filters propagate out of `Logger.handle`, unlike handlers, whose exceptions are routed to `handleError` -- and installing a filter is a supported use of this library; `Session` installs its own. Reproduced before the fix: the child stayed alive with `exit_code is None`, while a filter raising from inside the pump (already covered by the `finally`) reaped correctly. The ownership `try` now begins immediately after successful process creation, so every exit path from that point goes through `_reap`. The body binds `proc = self._process` once instead of re-reading the attribute, matching the bind-once convention used by `notify()`/`terminate()`. `_reap`'s own logging is now best-effort. It runs inside `run()`'s `finally`, so anything escaping it replaces the exception already propagating and hides the real cause. That containment was in place for the terminate and the wait but not for the three `logger.error` calls between them -- and a raising logging stack is exactly what routes execution into `_reap`, which makes those calls the likeliest thing to fail there, not the least. Reported by mwiebe on OpenJobDescription#333: OpenJobDescription#333 (comment) Two honest notes on the scope of the fix: - Reordering `_reap` to terminate before logging was tried and dropped. With best-effort logging in place the swallow already lets the terminate run, so the ordering changed nothing an `Exception` could observe; it only guarded a `BaseException` from the logging stack, which cannot reach the pool worker thread `run()` executes on. Mutation testing proved it inert, so it is not shipped rather than shipped behind a contrived test. - When the process-group lookup is *itself* what fails, the boundary buys the reap attempt and the handle release but not the kill: with no group recorded, `_posix_signal_subprocess` declines to signal rather than target a possibly recycled pid (the R5-9 convention). That is a pre-existing limitation of the signalling path, not something this change introduces, and the test for that path asserts only what actually happens. Recorded as a follow-up. Tests: 6 added. Every one mutation-checked -- 5 mutants, 0 survivors: the startup logging moved back outside the boundary, the process-group lookup moved back outside it, and each of the three reap log sites bypassing best-effort. The pre-existing 13 tests in the file pass unmodified. Full suite 884 passed; ruff, black, and mypy clean on native and --platform win32. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> (cherry picked from commit acee655)
`_start_subprocess()` catches every `Exception` from the launch and reports it by logging "Process failed to start" -- but that log can itself raise, and then the exception propagates out of `run()` before `self._has_started.set()` is reached. `ScriptRunnerBase._run` waits on `wait_until_started()` with **no timeout** (`_runner_base.py:869`), on the thread that called the public `Session` API. So a launch that had already definitively failed hung its caller forever. Reproduced: with a `logging.Filter` raising on the pre-launch "Running command" line, `run()` raised, `_has_started.is_set()` was False, and a waiter on `wait_until_started()` never returned. The handler's own log raises for a mundane reason -- it interpolates the original error, so any filter matching the first message matches the second too. `_has_started.set()` now happens in a `finally`, so it is reached however `_start_subprocess` leaves. Deliberately unchanged: `failed_to_start` stays False on the raising path, because the runner learns about it from the future's exception (`_on_process_exit` logs "Error running subprocess" and publishes a terminal state). Widening that flag would change what `state` reports for a launch that raised rather than returned None, which is a separate question from the hang. Found by an independent audit of the preceding commit, which made this window reachable to inspection; the defect itself is pre-existing. Tests: 3 added, in the same file as the ownership fix they sit beside -- the event is set, the public `wait_until_started()` really does return, and a negative control that an ordinary failure-to-start (where `_start_subprocess` returns None rather than raising) still reports `failed_to_start` with no exception. Both mutants caught: setting the event only on the non-raising path, and removing the set entirely. The bounded wait on a daemon thread is deliberate, so a regression fails on an assertion instead of hanging the suite. Full suite 887 passed; ruff, black, and mypy clean on native and --platform win32. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> (cherry picked from commit b81ef16)
`_posix_signal_subprocess` logged 'INTERRUPT: Sending signal "kill" to process group N' and only then called `os.killpg`, with both inside a `try` that catches `OSError`. A `logging.Filter` that raises is not an `OSError`, so the exception left the method having never signalled anything: the kill was gated behind a log call. This matters because of the commit two before this one. `run()`'s reap now reliably runs when a raising filter aborts an action, and its whole purpose is to not abandon a live child -- but the reap's terminate went through this helper, so the child survived anyway. Reproduced: filter raising on the startup line and on the announcement gave a reaped-and-released `LoggingSubprocess` with `exit_code=None` and the child still alive. The ownership fix was only as good as the signal path underneath it. Swapping the two statements is the whole change. A raising filter still propagates -- this is not an attempt to make the helper exception-proof -- but the child is dead before it does, which is the property that matters. The announcement is now emitted after a successful `killpg`; on the `OSError` path the existing handler still logs, so no failure goes unreported. Found by an independent audit of the ownership fix. Tests: 2 added. The child dies even when the announcement raises, plus a negative control that a healthy cancel still emits the announcement and still reaps with -SIGKILL -- deleting the log line satisfies the first test alone. Both mutants caught: the pre-fix ordering, and deleting the announcement. The filter helper now records which markers fired, so the first test can prove the announcement really was the thing that raised rather than passing on a tree where the signal path was never reached. Full suite 889 passed; ruff, black, and mypy clean on native and --platform win32. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> (cherry picked from commit 243e4e6)
Three behaviours around `run()`'s abandoned path were observable but asserted nowhere, which is how a later change flips one without anyone noticing. `is_running` is `_has_started and _process is not None`. Before the ownership fix an abort in the startup window left `_process` set forever, so the property reported True for a subprocess nobody would ever wait on. It now reports False, which is accurate -- pinned along with both of its inputs so a change to either is visible in one place, and with `failed_to_start` asserted False because the child did start before it was abandoned. The subprocess callback is *not* invoked on the abandoned path, even though `_reap` now records a real exit code. That is deliberate: `ScriptRunnerBase` registers `_on_process_exit` as the future's done-callback, so the Session is notified via the future's exception either way, and invoking this one too would notify twice for a single action. Pinned so it stays deliberate, with a negative control that a healthy run still invokes it -- otherwise "assert not called" would be satisfied by a callback that is never invoked at all. Also completes the bind-once convention in `_log_returncode`, which still read `self._process` while running on the pool worker, where `run()`'s `finally` is the thing that clears it. Behaviour-neutral: the pre-existing tests pass unmodified, and a mutant that clears `_process` before the log and reads the attribute again is caught by them. `_log_error_best_effort` now documents why it catches `Exception` and not `BaseException`: a KeyboardInterrupt or SystemExit must still propagate, and neither is reachable on a ThreadPoolExecutor worker anyway. This records the reasoning behind an ordering change that was tried, proved inert by mutation testing, and dropped. Not done, and the review prescription was wrong about it: completing the same bind-once refactor in `_log_subproc_stdout` would delete a `_process is None` check that is *not* dead code -- it is the R5-6 `python -O` guard, and `test_optimized_mode_invariants.py` calls that method directly to pin its error message. Removing it would require changing an existing test, which makes it a behaviour change rather than a refactor. Left alone. Tests: 3 added. 4 mutants, 0 survivors -- `_process` left set, the callback invoked on the abandoned path, the callback never invoked, and the returncode log reading a cleared attribute. Full suite 892 passed; ruff, black, and mypy clean on native and --platform win32. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> (cherry picked from commit b2a8768)
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| # openjd-rs never gates: every cancel/timeout/reap-failure path calls | ||
| # send_terminate(pid) -> killpg unconditionally (subprocess.rs), and its | ||
| # is_process_alive() probe is #[allow(dead_code)] for this reason. | ||
| if proc is not None: |
There was a problem hiding this comment.
terminate() is no longer gated on proc.poll() is None, so it now calls os.killpg(self._sudo_child_process_group_id, SIGKILL) even after the child (the session group leader, created with start_new_session=True) has already exited and been reaped.
That re-opens the PGID-recycling hazard the R5-9 fix a few lines up in run() was written to avoid: once the leader is reaped its pid — and hence the recorded PGID (os.getpgid(proc.pid)) — can be recycled by the OS, and on a busy host this killpg would then SIGKILL an unrelated process group.
The "wrapper execs and returns while its workload lives on" case that motivates ungating is legitimate, but the leader-dead-with-no-survivors case is indistinguishable here from the leader-dead-and-PGID-recycled case, so the survivor win comes with a real (if low-probability) risk of signalling a stranger's group — the exact thing R5-9 declined to do. Worth confirming this trade-off is intended, or gating on something narrower than leader liveness.
Contributor
Author
There was a problem hiding this comment.
Not fixing in this PR — flagging it as accepted-and-tracked rather than
dismissed, because the hazard you describe is real.
To be explicit about the trade-off as it stands: after the leader has been
reaped, a dead leader and a recycled PGID are genuinely indistinguishable
from terminate()'s vantage point, so this does accept a low-probability
chance of signalling an unrelated group in exchange for not orphaning the
survivors of a wrapper that execs and returns. That is the case
_sudo_child_process_group_id is the last remaining handle on, and gating
on leader liveness made the kill a no-op exactly there. The reasoning is in
the comment block above the call, along with the openjd-rs parity argument
(send_terminate -> killpg unconditionally; its is_process_alive probe
is #[allow(dead_code)] for this reason).
Why it is not being changed here: the two commits this PR most recently
gained touch _runner_base.py and tests only. Narrowing the gate is a
process-ownership design change, not a bug fix — a correct version needs
something stronger than poll(), e.g. verifying group identity at signal
time (pidfd on Linux, or a start-time//proc cross-check) so that a dead
leader can be told apart from a recycled PGID. I have not established
that such a gate exists on every platform this supports, and I am not going
to guess at one inside a PR about EXPR extension loading.
So: acknowledged, trade-off confirmed intentional, and being written up for
separate work rather than patched here.
openjd.expr is a thin facade over the native openjd._openjd_rs extension. _runner_base imported ExprValue and TypeCode from it at module level, and openjd.sessions.__init__ reaches _runner_base via _session, so `import openjd.sessions` loaded the extension unconditionally -- including for a consumer that only ever runs non-EXPR templates. This was new in 0.10.11; 0.10.10 has no such import. Verified against both released versions: after `import openjd.sessions`, openjd._openjd_rs is absent from sys.modules on 0.10.10 and present on 0.10.11. Reach the two EXPR types lazily instead. The guard is on sys.modules rather than on the value's type: FormatString.resolve_value is not limited to str and ExprValue -- a legacy whole-field interpolation returns the symbol's own value, so an int reaches _is_expr_null on a purely non-EXPR path and a "not a str" test would still load the extension for an integer-valued timeout. An ExprValue cannot exist unless the extension that defines it has been loaded, so the sys.modules check is exact rather than heuristic. openjd.model already keeps the Rust surface off its import path deliberately (openjd.model._format_strings._parser duplicates a constant rather than import it); this restores the same discipline here. The remaining module-level _openjd_rs imports are all under openjd.sessions._v1, the opt-in Rust-backed API surface, which openjd.sessions does not reach. Behaviour is unchanged: the existing EXPR null/list tests pass unmodified. Adds test_import_purity.py, pinning purity at import time and while resolving a non-EXPR template. Mutation-checked: restoring the module-level import is caught by 7 tests; dropping the sys.modules guard, or weakening it to isinstance(value, str), is caught by test_resolving_a_non_expr_field_does_not_load_native_extension. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
resolve_action_arg_values read `.is_null` off whatever resolve_value
returned, on the assumption that a non-str result is always an ExprValue.
It is not. A legacy (non-EXPR) whole-field interpolation returns the
symbol's own value, which is whatever the caller put in the symbol table.
openjd.model.ParameterValue types `value` as Any. It documents INT/FLOAT/
PATH values as strings but does not validate that, and it carries BOOL
values natively by design (RFC 0007). So both an int and a bool genuinely
reach here, and the result was AttributeError ('int' object has no
attribute 'is_null') raised out of the public Session.run_task rather than
a failed action.
Reproduced end-to-end through the public API before fixing: an INT job
parameter carrying an int, and a BOOL job parameter carrying True, each
crash run_task on a non-EXPR step script.
Dispatch on _as_expr_value() so typed null/list semantics apply only to an
actual ExprValue, and everything else resolves to its string form -- which
is what the function's docstring already promised for legacy expressions,
and what a multi-segment arg over the same symbol has always produced.
Verified that agreement directly: whole-field and "pre-{{ V }}" render
identically for int, bool, float and None.
_is_expr_null delegates to the same helper, so there is one Rust-boundary
predicate rather than two.
Which values reach the branch is decided by openjd-model, not by the
symbol table (SymbolTable validates nothing): FullNameNode.allows_nonscalar
is False, so a legacy whole-field result that is not a numbers.Real or a
str raises FormatStringError and takes the pre-existing plain-resolution
fallback. A list therefore never reaches the branch, before or after this
change.
Mutation-checked. Restoring the original ladder is caught by
test_int_job_parameter_value_reaches_the_subprocess_as_a_string and the
parametrized test_legacy_whole_field_value_resolves_to_its_string_form;
dropping the argument is caught by the same set; and the str-vs-repr
coercion is caught by the [str-not-repr] case. That last case exists
because an earlier mutant appeared to cover the coercion but did not: it
replaced the whole line including the isinstance(value, str) fast path, so
it failed via broken string handling instead. str(v) == repr(v) for every
plain int, bool and float, so pinning the coercion needs a value where
they differ. It uses an int subclass overriding __str__ rather than an
IntEnum, because str(SomeIntEnum.MEMBER) is 'E.MEMBER' on Python 3.9-3.10
and '3' from 3.11 on; the targeted tests were run on 3.9 to confirm.
The import-purity tests from the previous commit were re-checked against
the moved sys.modules guard.
Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
Addresses the CodeQL py/empty-except finding on PR OpenJobDescription#341: the `except ChildProcessError: pass` in this test's cleanup had no comment saying why swallowing it is correct. It is correct because the child being already reaped between the liveness check and the waitpid is the outcome the cleanup wants -- there is simply nothing left to wait for. Comment only; no behaviour change. 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
2 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
Nine fixes on top of
0.10.11, in two groups.Four subprocess-ownership fixes. The first is the follow-up @mwiebe asked for
when approving #333; the other three were found by auditing it and are
pre-existing.
Five RFC 0008 cross-check fixes against the
openjd-rsreferenceimplementation, which were written after #333's final review and so were never
part of that PR.
Every fix is one commit, and every commit's tests were mutation-checked: the
production behaviour was reverted, the tests were required to fail by name, and
the tree was restored and checksum-verified.
Why, per commit
run()must own its child from creation, not after loggingCloses #333 (comment)
LoggingSubprocess.run()logged "Command started as pid" and "Output:", and didthe POSIX process-group lookup, before entering the
trywhosefinallyreapsthe child. Anything raising in that window escaped
run()having never waited onthe child: a live process, no exit code recorded, and the runner publishing a
terminal state for it anyway.
The trigger is a
logging.Filterthat raises. Filters propagate out ofLogger.handle, unlike handlers, whose exceptions go tohandleError— andinstalling one is a supported use of this library;
Sessioninstalls its own.Reproduced before the fix, with a control proving a filter raising from inside
the pump was already handled.
The ownership
trynow begins immediately after successful process creation._reap's own logging is now best-effort, because_reapruns inside thatfinallyand anything escaping it replaces the exception already propagating —and a raising logging stack is precisely what routes execution there.
A failed launch must release
wait_until_started()_start_subprocess()catches everyExceptionfrom the launch and reports it bylogging "Process failed to start" — but that log can itself raise, and then the
exception propagates out of
run()before_has_started.set()is reached.ScriptRunnerBase._runwaits onwait_until_started()with no timeout, onthe thread that called the public
SessionAPI. So a launch that had alreadydefinitively failed hung its caller forever.
The handler's log raises for a mundane reason: it interpolates the original error,
so any filter matching the first message matches the second too.
_has_started.set()now happens in a
finally.Send the kill signal before announcing it
_posix_signal_subprocessloggedINTERRUPT: Sending signal "kill" to process group Nand only then calledos.killpg, both inside atrycatching onlyOSError. A raising filter is not anOSError, so the kill was gated behind alog call.
This mattered because of the first commit: the reap it now guarantees went through
this helper, so the child survived anyway. The ownership fix was only as good as
the signal path underneath it. Swapping the two statements is the whole change.
Pin the observable state the ownership fix changed
is_runningand the subprocess callback both behave differently on the abandonedpath now, and neither was asserted either way. Also completes the bind-once
convention in
_log_returncode, and documents why_log_error_best_effortcatches
Exceptionand notBaseException.The five RFC 0008 cross-check fixes
WrappedAction.Environmentmust be session-lifetime. RFC 0008 requires everyopenjd_envvariable from any earlier action in the session, but the symbol wasbuilt by walking
_environments_entered, whichexit_environmentpops — so anexport vanished the moment its environment exited. Now read from a
session-lifetime map, mirroring openjd-rs's split between
env_varsandcreated_env_vars. The only remover is an explicitopenjd_unset_env._materialize_path_mappinghad no failure path. AnOSErrorwriting therules file escaped
run_task/enter_environment/exit_environmentwith nocallback and no
ActionStatus, because all three call it before their runnerexists. openjd-rs maps the same failure to
fail_action_setup().cancel_info.jsonhandler caught the wrong exception type.terminate()no longer gates on leader liveness. The signal target is aprocess group, and
killpgstill reaches survivors after the leader has beenreaped — which is exactly the shape a wrap environment produces. openjd-rs never
gates.
openjd_envnear-miss regex is anchored with\Zrather than$, whichalso matches before a trailing newline.
Testing
test_subprocess_reaping.pywent 13 → 33 tests.
ruff,black, andmypyclean on native and--platform win32.moved back outside the boundary, the process-group lookup moved back outside it,
and each of the three reap log sites bypassing best-effort. Plus 2 mutants each
for the launch-release and kill-ordering fixes, and 4 for the pinning commit.
gap analysis, craft/conventions). They produced the second and third commits
here, and caught a test of mine whose liveness assertion was silently a
tautology because
_wait_gone(None)returnsTrue.Two things I got wrong, corrected here
logging" in
_reap. Mutation testing showed it inert: with best-effortlogging the swallow already lets the terminate run, so it only guarded a
BaseExceptionfrom the logging stack, which cannot reach the pool workerthread. Dropped rather than shipped unfalsifiable.
_terminate_processto kill directly, so itasserted a kill the shipped code does not perform on that path. It now
asserts only that the reap is reached, and the real gap is written up as a
follow-up (see below).
Known limitations, not addressed here
buys the reap attempt and the handle release but not the kill: with no group
recorded,
_posix_signal_subprocessdeclines to signal rather than target apossibly-recycled pid. Pre-existing, no production trigger identified, and a fix
is a signalling-policy change that overlaps the deferred POSIX process-group
work. Deliberately not folded in here.
_terminate_process(
INTERRUPT: Start killing the process treeprecedeskill_windows_process_tree). Source-only — CI is the first execution.OPENJD_TEST_SUDO_TARGET_USER/OPENJD_TEST_SUDO_SHARED_GROUP, and that is theconfiguration RFC 0008 primarily exists for.
onWrapTaskRunhook may define session environment variables. Python discards the macro;
openjd-rs records it into the map feeding
WrappedAction.Environmentwhile neverapplying it to any subprocess environment. RFC 0008's "any earlier action ... via
a wrap hook" and How-Jobs-Are-Run's "only the Action for entering an Environment"
point opposite ways, and no conformance fixture covers it. Not guessed at here.
Note on the branch
#333 was squash-merged, so this work — which was based on that PR's head — has a
merge-base with
mainlinethat predates the squash. The nine commits werecherry-picked onto
mainlineto keep the diff clean rather than force-pushing overthe original branch.
By submitting this pull request, I confirm that you can use, modify, copy, and
redistribute this contribution, under the terms of your choice.