feat(sessions): Rust-backed openjd.sessions._v1 via PyO3#316
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| status_message=status.status_message, | ||
| ) | ||
| self._callback(self._session_id, running_status) | ||
| reported_running = True |
| # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
|
|
||
| import sys | ||
| import ctypes |
| # Note: We must ensure that 'handles_list' must persist until the | ||
| # attribute list is destroyed using DeleteProcThreadAttributeList. We do this by holding on | ||
| # to a reference to it until after the finally block of this try. | ||
| handles_list = inherit_handles( # noqa: F841 # ignore: assigned but not used |
Architecture summary (for reviewers)
Bird's eyeThis branch introduces 86 files changed, +4122 / −0 (purely additive — no v0 deletions). Commits
Architecturev0 → v1 API delta
Open questions / things to flag
Tests
Reviewer prerequisitesTo run the test suite locally, the |
1e92e69 to
201aeac
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| command: Path, os_env_vars: Optional[dict[str, Optional[str]]], working_dir: str | ||
| ) -> str: | ||
| path_var: Optional[str] = _get_path_var_for_shutil_which(os_env_vars, working_dir) | ||
| exe = str(shutil.which(str(command), path=path_var)) |
There was a problem hiding this comment.
shutil.which() returns None when the command is not found, so str(shutil.which(...)) produces the literal string "None". That string is truthy, so the if not exe: guard on the next line never triggers for the not-found case — instead this function returns "None" as the resolved executable path, and the failure surfaces later as a confusing "cannot find/execute 'None'" error rather than the intended RuntimeError("Could not find executable file: ...").
Suggested fix: check the result before stringifying.
exe = shutil.which(str(command), path=path_var)
if not exe:
raise RuntimeError("Could not find executable file: %s" % command)
return exe
| yield False | ||
| return | ||
|
|
||
| caps = libcap.cap_get_proc() |
There was a problem hiding this comment.
cap_get_proc() returns a heap-allocated cap_t that the caller owns and must release with cap_free() (see the cap_get_proc(3) man page). This code never calls cap_free(caps) on any of its exit paths, so every invocation of try_use_cap_kill() leaks the capability state object. Since this context manager is entered on each action cancellation, the leak accumulates over the lifetime of a long-running session host.
Consider binding libcap.cap_free and freeing caps in a finally (and note that cap_set_flag/cap_set_proc were registered without an errcheck, so failures there are silently ignored — unlike the get-path functions).
| text=True, | ||
| ) | ||
| if pgrep_result.returncode != 0: | ||
| raise FindSignalTargetError("Unable to query child processes of sudo process") |
There was a problem hiding this comment.
pgrep exits with status 1 specifically to mean "no processes matched" (only status >1 indicates an actual error). On the non-Linux/macOS path this is the normal case while sudo has not yet forked its child — exactly the situation find_sudo_child_process_group_id's retry loop is designed to poll through.
But because this raises FindSignalTargetError on any non-zero return, and the caller wraps the entire while loop in a single try/except FindSignalTargetError, the first poll iteration where the child does not yet exist raises, the exception propagates out of the loop, and the retry never happens. The result is that on macOS the signal target frequently can't be found even though a short wait would have succeeded.
Suggest distinguishing the two cases:
if pgrep_result.returncode == 1:
return None # no child yet — let the caller retry
if pgrep_result.returncode != 0:
raise FindSignalTargetError("Unable to query child processes of sudo process")
| return [] | ||
|
|
||
| def __del__(self): | ||
| self.cleanup() |
There was a problem hiding this comment.
If the _RustSession(...) constructor in __init__ raises (e.g. bad credentials, invalid session_root_directory), the Session object exists but self._rust_session was never assigned. When that half-constructed object is garbage-collected, __del__ → cleanup() → self._rust_session.cleanup() will raise AttributeError, masking the original construction error with a noisy error from __del__.
Consider guarding cleanup, e.g. getattr(self, "_rust_session", None) before calling .cleanup(), or setting self._rust_session = None at the top of __init__.
| @@ -0,0 +1,258 @@ | |||
| # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | |||
|
|
|||
| """This module contains code for interacting with Linux capabilities. The module uses the ctypes | |||
There was a problem hiding this comment.
We should rebase, and get this PR merged soon since the OpenJD side is merged.
There was a problem hiding this comment.
Pending the release of OpenJD-model once it is released.
198556a to
aa7f038
Compare
| stdin=DEVNULL, | ||
| text=True, | ||
| ) | ||
| if pgrep_result.returncode != 0: |
There was a problem hiding this comment.
pgrep -P <pid> exits with code 1 when no child processes match (not just on error). Here any non-zero return code raises FindSignalTargetError, which propagates out of the retry loop in find_sudo_child_process_group_id (caught by the outer except, logged as a warning, and the function returns None).
On non-Linux POSIX hosts (macOS) the retry loop exists precisely because sudo's child may not exist yet on the first scan. But the very first pgrep before the child is spawned returns exit 1 → raises → the loop is abandoned on the first iteration and the signal target is never found. To preserve the intended retry-until-timeout behavior, treat "no match" (returncode 1 with empty stdout) as return None and only raise on genuine errors (returncode >= 2).
| yield False | ||
| return | ||
|
|
||
| caps = libcap.cap_get_proc() |
There was a problem hiding this comment.
cap_get_proc() allocates a cap_t on the heap that must be released with cap_free() (per the libcap man page: the caller should free releasable memory with cap_free()). Here caps is never freed in any of the three branches that follow, so every call to try_use_cap_kill() leaks one capability-state object. Since this context manager runs on each action cancel/signal, the leak accumulates over the lifetime of a long-running session.
Consider declaring libcap.cap_free (argtype cap_t, restype c_int) in _get_libcap() and calling it on caps from a finally that wraps the body below.
| [tool.coverage.report] | ||
| show_missing = true | ||
| fail_under = 79 | ||
| fail_under = 50 |
There was a problem hiding this comment.
The coverage gate is being lowered from 79% to 50%. This is a substantial regression in the enforced quality bar for the package as a whole. Given that most of the new _v1 code (e.g. the Win32 ctypes layer, _capabilities.py) is thin OS-binding glue that is hard to unit test, this may be intentional — but it also means the new pure-Python logic (_session.py polling/callback state machine, _sudo.py child-discovery) can land largely uncovered. Worth confirming this drop is a deliberate, temporary concession rather than masking untested new logic.
Add a new `openjd.sessions._v1` package that wraps the Rust
`openjd_sessions` crate (in the sibling `openjd-rs` workspace),
alongside the existing pure-Python v0 implementation under
`openjd.sessions`. The v1 namespace is a thin Python shell over
the Rust runtime: format-string evaluation, action dispatch,
process control, environment lifecycle, path mapping, and logging
all happen inside Rust; the Python layer wires up callbacks,
holds the SessionUser objects, and provides the platform-specific
helpers that need to call into native APIs.
Layout
======
The package coexists with the legacy v0 implementation (which
remains functional and untouched):
src/openjd/sessions/
├── _v1/ # Rust-backed v1 (new)
│ ├── __init__.py # public surface re-exports
│ ├── _session.py # Session class — thin wrapper over Rust
│ ├── _session_user.py # PosixSessionUser / WindowsSessionUser
│ ├── _types.py # type aliases for the model._v1.job types
│ │ # used here as Action, EmbeddedFile,
│ │ # Environment, EnvironmentScript, StepScript
│ ├── _logging.py # Python ↔ Rust logging bridge
│ ├── _os_checker.py # cross-platform helpers
│ ├── _path_mapping.py # PathMappingRule re-export
│ ├── _linux/ # Linux-specific helpers (sudo, capabilities)
│ │ ├── _capabilities.py # POSIX capabilities query
│ │ └── _sudo.py # sudo subprocess support
│ ├── _win32/ # Windows-specific helpers
│ │ ├── _api.py # Win32 API bindings
│ │ ├── _helpers.py # convenience wrappers
│ │ ├── _locate_executable.py
│ │ └── _popen_as_user.py # CreateProcessAsUser / CreateProcessWithLogonW
│ └── _scripts/
│ ├── _posix/_signal_subprocess.sh
│ └── _windows/_signal_win_subprocess.py
├── (existing v0 modules — _session.py, _types.py, etc. — are unchanged)
└── ...
Public surface
==============
from openjd.sessions._v1 import (
Session, # Rust pyclass
SessionState, # enum: Ready, Running, Canceling, …
ActionState, ActionStatus, ActionResult,
ScriptRunnerState,
PosixSessionUser, WindowsSessionUser,
SessionError, BadCredentialsException,
PathMappingRule, PathFormat,
FormatString, # re-exported from openjd.expr
)
s = Session(
session_id="...",
job_parameter_values={...},
path_mapping_rules=[...],
session_user=PosixSessionUser("runner", "renderers"),
on_state_change=callback,
on_action_change=callback,
)
s.enter_environment(environment_id, environment, ...)
s.run_task(step, task_parameter_values, ...)
s.cancel_action()
s.exit_environment(environment_id)
s.cleanup()
Implementation summary
======================
1. `Session` is a thin wrapper around the Rust
`openjd._openjd_rs.Session` pyclass. The Python class
manages: callback registration, the in-flight action threading
model, post-completion cleanup, and `__enter__` /
`__exit__` / `cleanup()` semantics. State changes and action
status updates are forwarded synchronously to user callbacks
on the worker thread that observes them — including the
initial RUNNING transition (fixed during this work).
2. The session dispatches actions through
`openjd._openjd_rs.Session` methods. Format-string evaluation,
parameter binding, working-directory setup, embedded-file
materialization, script execution, signal forwarding, timeouts,
and child-process tracking all happen inside the Rust crate;
the Python layer is purely glue.
3. Logging: the v1 layer installs a Python ↔ Rust logging bridge
so messages emitted from Rust (`log::info!`, etc.) appear
under the standard Python `openjd.sessions` logger
hierarchy. Logger name is `openjd.sessions` (not
`openjd.sessions.v1`) so v0 and v1 share the same log
namespace.
4. Session users are split per platform:
- `PosixSessionUser(user, group)` is a thin wrapper around the
Rust pyclass. Replaces the v0 Python `PosixSessionUser` for
v1 callers.
- `WindowsSessionUser` is exposed for v1 callers but the
full Win32 implementation remains in Python (the Rust crate
covers the cross-user invocation primitives but some
Win32-specific surface — like locating executables on PATH
under a different user, or capturing redirected stdout — is
still handled here).
5. Linux helpers under `_v1/_linux/`: `_capabilities.py` queries
POSIX capabilities so callers can decide whether sudo is
necessary; `_sudo.py` supplies the subprocess-spawning helper
the Rust runtime hands out to the Python layer when
PosixSessionUser is configured.
6. Windows helpers under `_v1/_win32/`: `_api.py` and
`_helpers.py` expose the Win32 API surface needed for
user-impersonating subprocess creation; `_locate_executable.py`
resolves PATH lookups under a non-default user;
`_popen_as_user.py` mirrors Python's `subprocess.Popen` API
for child processes spawned via CreateProcessAsUser /
CreateProcessWithLogonW.
7. The `openjd.sessions._v1` package consumes
`openjd.model._v1` types directly. The v1 commit in
`openjd-model-for-python` split that namespace into
submodules (`template`/`job`/`types`/`errors`); the
structural types we need (`Action`, `EmbeddedFile`,
`Environment`, `EnvironmentScript`, `StepScript`) come from
`openjd.model._v1.job`. Type aliases for them are exposed
under their `_2023_09` suffix as `Action_2023_09`,
`EmbeddedFileText_2023_09`, etc., for callers that need to be
explicit about the schema revision.
Tests
=====
`test/openjd/sessions-v1/` is a new package with:
- `test_session_scenarios.py` — full session lifecycle scenarios
driven from YAML scenario+template pairs under
`scenarios/`. Covers EXPR-extension features (let bindings,
host context references), all task parameter types,
environment-file let bindings, path mapping rules, and the
step/env let-binding scope precedence rules.
- `test_pickle.py` — pickle round-trip coverage for v1 value
types (Session is not pickleable, but state enums, status
values, and supporting pyclasses are).
- `test_os_checker.py` — basic platform-check sanity.
- `scenarios/` — six end-to-end scenarios with template +
scenario YAML files exercising the full Python ↔ Rust path.
The test_session_scenarios coverage is comprehensive enough to
substitute for v0's per-feature unit tests for the surface that
v1 exposes. Pre-existing test failures (`utils.windows_acl_helper`
imports, etc.) are unrelated to this work and are documented as
pre-existing in their respective files.
Side-by-side dependency
=======================
This package consumes `openjd.model._v1` from the
`openjd-model-for-python` companion repo (which builds the
`openjd._openjd_rs` extension module). The two repos must move
together: the corresponding openjd-model-for-python PR
("feat: Rust-backed openjd.model._v1, openjd.expr,
openjd.sessions._v1 via PyO3") must land before this PR.
Signed-off-by: Sean Tang <171081544+seant-aws@users.noreply.github.com>
chore: rename test/openjd/sessions-v{0,1}/ to sessions_v{0,1}/ for pytest+mypy
Mirrors the same rename in the sibling repo openjd-model-for-python
(commit `9f7f55d chore: rename test/openjd/model-v{0,1}/ to
model_v{0,1}/ for mypy`). The pre-existing test directories
`test/openjd/sessions-v0/` and `test/openjd/sessions-v1/` carried
hyphens in their names. Each contained an `__init__.py` (so they
look like Python packages to tools that walk the filesystem). This
broke pytest's import-mode discovery — tests that referenced
sibling modules with relative imports failed with
`ImportError: attempted relative import with no known parent
package`, which cascaded into 16 collection errors and 13 ERROR
test runs. mypy fails the same way for the same reason
(`sessions-v0 is not a valid Python package name`).
This commit:
1. `git mv test/openjd/sessions-v0 test/openjd/sessions_v0` (and
the v1 variant) — 68 file renames in total, no content changes.
2. No textual references in `pyproject.toml`, `hatch.toml`,
docstrings, or source needed updating: unlike
openjd-model-for-python's parallel rename, sessions-for-python
never embedded the path in config or comments.
Verification:
- `hatch run test` collection no longer fails. Test count goes
from 101 passed / 16 ERROR / 13 FAILED on the old layout to
559 passed / 18 FAILED / 33 skipped / 16 xfailed.
- The remaining 18 failures are pre-existing issues unrelated to
this rename:
- 2 sudo-impersonation tests in `test_subprocess.py` need
`OPENJD_TEST_SUDO_*` env vars to actually run (xfail-on-
bare-environment).
- 3 OS-checker tests in `test_os_checker.py` are platform-
gated (`is_windows`/`is_posix` assertions on a Linux runner).
- 13 v1 session-scenario tests fail with
`AttributeError: 'JobParameterType' object has no
attribute 'value'` — a port-from-v0 bug introduced when
`test_session_scenarios.py` was added in
`98656ec feat(sessions): Rust-backed openjd.sessions._v1
via PyO3`. v0's `JobParameterType` is a `str`-Enum (so
`.value` returns the string); v1's pyclass exposes
`.as_str()` instead, but the test wasn't updated.
- mypy will now actually run instead of bailing on the invalid
package name. (Existing `hatch run lint` reports 20 ruff
errors and unrelated lint debt that mypy may reveal further;
separate concern.)
Signed-off-by: Sean Tang <171081544+seant-aws@users.noreply.github.com>
feat(sessions-v1): add Session context-manager protocol; port scenario tests to v1 enum API
Two follow-ons to the directory rename in `d6f7f69` that
unblock 7 previously-failing v1 session-scenario tests.
**1. Session context manager.** `openjd.sessions._v1.Session` had
`cleanup()` and `__del__` but no `__enter__` / `__exit__`
methods, so `with Session(...) as session:` raised
`TypeError: 'Session' object does not support the context
manager protocol`. The v0 reference `openjd.sessions._session.Session`
implements both as the trivial pattern (`__enter__` returns
`self`, `__exit__` calls `self.cleanup()`); v1 now matches.
**2. Scenario test port-from-v0 cleanup.**
`test/openjd/sessions_v1/test_session_scenarios.py` was added in
`98656ec feat(sessions): Rust-backed openjd.sessions._v1 via PyO3`
by copy-paste from a v0 scaffold without adjusting two
v0-isms that v1's pyclass enums don't support:
* `param_def.type.value` and `session.state.value`. v0's
`JobParameterType` and `SessionState` are `str`-Enums (where
`.value` returns the underlying string). The v1 binding
exposes them as PyO3 pyclass enums without `.value`. The
`build_parameter_values()` helper now passes the
`JobParameterType` variant directly (the upstream constructor
accepts it); the polling loops compare `session.state` against
a module-level `_QUIESCENT_STATES` set of `SessionState`
variants.
* `ParameterValueType(name_str)`. v1's pyclass enum is not
constructible from a string; the helper switched to passing
the variant straight through, dropping the string round-trip.
Test results: `hatch run test` goes from 559 passed / 18 failed
to 566 passed / 11 failed (+7 newly-passing scenario tests, no
regressions). The 11 remaining failures are all real bugs that
need their own fixes:
- 3 platform-gated OS-checker tests (Windows assertions on
Linux runner).
- 2 sudo-impersonation tests needing
`OPENJD_TEST_SUDO_*` env vars.
- 6 scenario tests with output-string assertion mismatches
(real session behaviour or scenario-yaml issues, not
mechanical fixes).
Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com>
Signed-off-by: Sean Tang <171081544+seant-aws@users.noreply.github.com>
Signed-off-by: Jericho Tolentino <68654047+jericht@users.noreply.github.com>
| from typing import Optional, Sequence | ||
|
|
||
| from .._session_user import SessionUser | ||
| from .._subprocess import LoggingSubprocess |
There was a problem hiding this comment.
This imports from .._subprocess import LoggingSubprocess, but the _v1 package has no _subprocess.py (only the legacy openjd.sessions._subprocess exists). Any attempt to import this module — e.g. from openjd.sessions._v1._win32._locate_executable import locate_windows_executable — will fail with ModuleNotFoundError: No module named 'openjd.sessions._v1._subprocess'.
Nothing in _v1 currently references locate_windows_executable, so this is a latent/dead module today, but the import is broken as written. Either point it at the real module (from openjd.sessions._subprocess import LoggingSubprocess) or drop the file if the Rust runner now handles executable location.
| command: Path, os_env_vars: Optional[dict[str, Optional[str]]], working_dir: str | ||
| ) -> str: | ||
| path_var: Optional[str] = _get_path_var_for_shutil_which(os_env_vars, working_dir) | ||
| exe = str(shutil.which(str(command), path=path_var)) |
There was a problem hiding this comment.
str(shutil.which(...)) stringifies the result before the falsy check. When shutil.which fails to find the executable it returns None, but str(None) is the string "None", which is truthy — so if not exe is never taken, and the function returns the literal string "None" as the executable path instead of raising RuntimeError. The caller then tries to launch "None" as the command.
Check the raw result before stringifying:
exe = shutil.which(str(command), path=path_var)
if not exe:
raise RuntimeError("Could not find executable file: %s" % command)
return exe
seant-aws
changed the title
5 participants
Planned Merge Date: TBD
Note: force minor bump
Fixes: N/A — this is a multi-commit feature branch, not a bugfix.
What was the problem/requirement? (What/Why)
openjd-sessions-for-pythonis being migrated to Rust (seeopenjd-rs/crates/openjd-sessions). This branch adds the Python consumer side of that migration: a newopenjd.sessions._v1package that delegates the core session machinery (subprocess management, template evaluation, path mapping) to the compiled Rust extensionopenjd._openjd_rs, while preserving the existing v0 API surface untouched.What was the solution? (How)
src/openjd/sessions/_v1/that re-exports symbols fromopenjd._openjd_rsand adds a thin Python orchestration layer (_session.py).Sessionclass wraps the Rust_RustSessionand adds:SessionCallbackTypecallback contract that the worker agent depends on.RUNNINGcallback to avoid a race with the worker-agent scheduler.__enter__/__exit__) that guaranteescleanup()is called.pyo3-logbridge so Rustlog::info!()etc. routes into Python'sloggingmodule.openjd.sessions) is untouched. v1's public API is a strict superset of v0's (same 18 symbols + 3 new:ActionResult,ScriptRunnerState,SessionRuntimeError).See the comment below for the full architecture diagram, file-by-file breakdown, and open questions.
What is the impact of this change?
openjd.sessionstoopenjd.sessions._v1.openjd.sessions._v1requires the compiledopenjd._openjd_rsextension to be installed._linux/and_win32/under_v1/are still pure-Python ctypes (capabilities + Win32 impersonation) — only the session core is Rust-backed in this PR.How was this change tested?
test/openjd/sessions/was renamed totest/openjd/sessions_v0/to coexist cleanly with the newsessions_v1test tree (no v0 test content changes).New
test/openjd/sessions_v1/:test_session_scenarios.py— single parametrized test driving 18 YAML scenarios covering let-bindings and parameter types (incl. path mapping). Platform-gated viarun_on: posix|windows|all.test_pickle.py— 11 tests verifying the Rust-backed types (ActionState,SessionState,ActionStatus,ActionResult,PosixSessionUser) round-trip throughpickle. The worker agent crosses process boundaries with these types, so this matters.test_importable.py— smoke import.Unit tests run locally — blocked on getting
openjd._openjd_rsinstalled in the reviewer's environment; will update once verified.Was this change documented?
__all__in_v1/__init__.py.DEVELOPMENT.mdyet — that should follow once the v0 → v1 migration story is settled.Is this a breaking change?
No. The change is purely additive. v0's public contract (
openjd.sessions) is unchanged. v1 is a strict superset accessed via a new import path.Does this change impact security?
Possibly — flagging for review:
_v1/_win32/retains the Win32 user-impersonation logic (LogonUser, token handling). It's pure-Python ctypes, ported from v0, and there are open TODOs around domain handling that may warrant a threat-model pass before this lands.Session(inopenjd-rs/crates/openjd-sessions) handles subprocess execution and working-directory creation. Its threat model lives upstream inopenjd-rs; this PR only consumes the compiled artifact.If reviewers want to label this with "security", happy to engage.
Cross-port to openjd-rs
openjd-rs/crates/openjd-sessionscrate as the backend foropenjd.sessions._v1. The Rust crate is the source of truth; this PR introduces the Python binding layer.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.