From 55653b27c22cfdcf9c68bac4a62322b7152f6754 Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Thu, 30 Jul 2026 15:45:52 -0700 Subject: [PATCH] fix: Do not load the native extension to build an empty rules list Session.__init__ calls _build_expr_host_rules() unconditionally, and that imported openjd.expr -- a facade over the native openjd._openjd_rs extension -- before checking whether there was anything to convert. With no path mapping rules it imported the engine, iterated an empty list, and returned []. So a worker that never evaluates an EXPR expression still loaded the extension on its FIRST SESSION: 49d1804 moved the load from import time to first-session time rather than eliminating it. Reported by a reviewer reading the call site; confirmed by measurement. Return [] directly when there are no rules. That is byte-identical to what the loop produced, and it must stay [] rather than None because the session is still host scope and apply_path_mapping() has to remain available with an empty rule set. Seeding [] is free: SymbolTable stores expr_host_rules untouched as Optional[list[Any]] and only crosses into Rust at evaluation time, in _expr_support.symtab_to_expr_values -- verified that assigning None, [], and running a full non-EXPR resolve all leave the extension unloaded. Audited every load path rather than grepping for imports, using a sys.meta_path finder that records the stack at the moment openjd._openjd_rs is first imported, one flow per fresh interpreter. Before: four flows leaked -- Session() with no rules, with rules=None, a non-EXPR run_task, and a non-EXPR enter_environment -- and all four traced to the same single frame, _session.py:476 __init__ -> _build_expr_host_rules. After: every non-EXPR flow is clean, and the only remaining loads are the three that should load. Import of openjd.sessions, non-EXPR argument resolution, and non-EXPR optional-int resolution were already clean and stayed clean. Known limitation, asserted rather than left implicit: a session with real path mapping rules still loads the extension, because the rules must be converted into openjd.expr.PathMappingRule engine objects to seed host context. openjd-sessions cannot avoid that alone; closing it needs openjd-model to accept unconverted rules and convert them at the Rust boundary it already crosses. test_path_mapping_rules_do_load_the_extension pins the current behaviour so that a future fix is noticed here instead of passing silently. This matters in practice: a worker with path mapping rules configured -- the common case -- still loads the extension. Correcting my own earlier audit: I classified this site as "already function-local (lazy)" and moved on. Lazy is not conditional. A function-local import still runs unconditionally the moment its function is called, and this one is called from __init__. The existing purity tests covered import time plus one runtime path, so nothing would have caught it. Mutation-checked, 4 mutants, 0 survivors: neutralising the fast path, returning None instead of [], firing it for every session, and inverting its condition are each caught by name. Verified: 913 passed / 39 skipped / 16 xfailed; ruff, black, mypy native and mypy --platform win32 all clean. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- src/openjd/sessions/_session.py | 17 ++++ test/openjd/test_import_purity.py | 126 ++++++++++++++++++++++++++++++ 2 files changed, 143 insertions(+) diff --git a/src/openjd/sessions/_session.py b/src/openjd/sessions/_session.py index e950393..468628a 100644 --- a/src/openjd/sessions/_session.py +++ b/src/openjd/sessions/_session.py @@ -1653,6 +1653,23 @@ def _build_expr_host_rules(self) -> Optional[list[Any]]: evaluation. Returns an empty list when the session has no rules (the session is still host scope), or ``None`` when the engine bindings are unavailable (pre-EXPR openjd-model).""" + if not self._path_mapping_rules: + # Nothing to convert, so no engine objects are needed and the import + # below is pure cost. Returning the empty list here is exactly what + # the loop produced anyway, and it keeps a session that never + # evaluates an EXPR expression from loading the native extension at + # all -- ``import openjd.expr`` is a facade over + # ``openjd._openjd_rs``, and ``__init__`` calls this + # unconditionally, so without this the deferral won by the + # module-level import removal was only from import time to + # first-session time. + # + # ``[]`` and not ``None``: the session is still host scope, so + # ``apply_path_mapping()`` must remain available with an empty rule + # set. ``SymbolTable.expr_host_rules`` stores this untouched + # (``Optional[list[Any]]``) and only crosses into Rust at evaluation + # time, so seeding ``[]`` costs nothing here. + return [] try: from openjd.expr import PathFormat as ExprPathFormat from openjd.expr import PathMappingRule as ExprPathMappingRule diff --git a/test/openjd/test_import_purity.py b/test/openjd/test_import_purity.py index e057c66..3e59e8b 100644 --- a/test/openjd/test_import_purity.py +++ b/test/openjd/test_import_purity.py @@ -281,3 +281,129 @@ def test_pure_path_module_does_not_load_native_extension(tmp_path: Path, module: # THEN assert loaded == "False", f"importing {module} loaded the native extension" + + +# --------------------------------------------------------------------------- +# Import-time purity is not enough on its own: `Session.__init__` calls +# `_build_expr_host_rules()` unconditionally, so before the empty-rules fast +# path a worker that never evaluates an EXPR expression still loaded the +# extension on its FIRST SESSION. The deferral won by removing the module-level +# import was only from import time to first-session time. +# --------------------------------------------------------------------------- + + +class TestSessionLifecycleStaysExtensionFree: + def test_constructing_a_session_without_path_mapping_rules_stays_pure( + self, tmp_path: Path + ) -> None: + """The regression: `Session()` used to load the extension to build an + empty engine rules list.""" + # WHEN + loaded = _run_probe( + tmp_path, + """ + import uuid + + from openjd.sessions import Session + + session = Session(session_id=uuid.uuid4().hex, job_parameter_values={}) + try: + # `[]`, not `None`: the session is still host scope, so + # apply_path_mapping() stays available with an empty rule set. + assert session._expr_host_rules == [], session._expr_host_rules + print(RS in sys.modules) + finally: + session.cleanup() + """, + ) + + # THEN + assert loaded == "False", ( + "constructing a Session with no path mapping rules loaded the native " + "extension; the empty-rules fast path in _build_expr_host_rules is " + "what prevents this." + ) + + def test_running_a_non_expr_task_stays_pure_end_to_end(self, tmp_path: Path) -> None: + """The property a non-EXPR worker actually cares about: a whole task + runs without the extension ever being loaded.""" + # WHEN + loaded = _run_probe( + tmp_path, + """ + import time + import uuid + + from openjd.model.v2023_09 import ModelParsingContext, StepScript + from openjd.sessions import ActionState, Session, SessionState + + context = ModelParsingContext(supported_extensions=["FEATURE_BUNDLE_1"]) + script = StepScript.model_validate( + {"actions": {"onRun": {"command": "echo", "args": ["hello"]}}}, + context=context, + ) + session = Session(session_id=uuid.uuid4().hex, job_parameter_values={}) + try: + session.run_task(step_script=script, task_parameter_values={}) + deadline = time.time() + 15 + while session.state == SessionState.RUNNING and time.time() < deadline: + time.sleep(0.05) + status = session.action_status + assert status is not None and status.state == ActionState.SUCCESS, status + print(RS in sys.modules) + finally: + session.cleanup() + """, + ) + + # THEN + assert loaded == "False", ( + "running a non-EXPR task loaded the native extension somewhere in the " + "session lifecycle" + ) + + def test_path_mapping_rules_do_load_the_extension(self, tmp_path: Path) -> None: + """Negative control, and a documented limitation rather than a goal. + + Real rules must be converted into ``openjd.expr.PathMappingRule`` engine + objects to seed the session's host context, so this case genuinely loads + the extension. openjd-sessions cannot avoid it alone: closing it needs + openjd-model to accept unconverted rules and convert them at the Rust + boundary, where ``symtab_to_expr_values`` already crosses. + + Asserted so that the limitation is visible and so a future change that + makes this pure is noticed here rather than passing silently. + """ + # WHEN + loaded = _run_probe( + tmp_path, + """ + import uuid + from pathlib import PurePosixPath + + from openjd.sessions import PathFormat, PathMappingRule, Session + + rule = PathMappingRule( + source_path_format=PathFormat.POSIX, + source_path=PurePosixPath("/mnt/source"), + destination_path=PurePosixPath("/mnt/dest"), + ) + session = Session( + session_id=uuid.uuid4().hex, + job_parameter_values={}, + path_mapping_rules=[rule], + ) + try: + assert len(session._expr_host_rules) == 1, session._expr_host_rules + print(RS in sys.modules) + finally: + session.cleanup() + """, + ) + + # THEN + assert loaded == "True", ( + "a session with real path mapping rules is expected to load the " + "extension; if this is now False the limitation has been fixed and " + "this control should become a purity assertion" + )