Skip to content

Commit 75863c5

Browse files
committed
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>
1 parent e2e60d3 commit 75863c5

2 files changed

Lines changed: 143 additions & 0 deletions

File tree

src/openjd/sessions/_session.py

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1653,6 +1653,23 @@ def _build_expr_host_rules(self) -> Optional[list[Any]]:
16531653
evaluation. Returns an empty list when the session has no rules (the
16541654
session is still host scope), or ``None`` when the engine bindings
16551655
are unavailable (pre-EXPR openjd-model)."""
1656+
if not self._path_mapping_rules:
1657+
# Nothing to convert, so no engine objects are needed and the import
1658+
# below is pure cost. Returning the empty list here is exactly what
1659+
# the loop produced anyway, and it keeps a session that never
1660+
# evaluates an EXPR expression from loading the native extension at
1661+
# all -- ``import openjd.expr`` is a facade over
1662+
# ``openjd._openjd_rs``, and ``__init__`` calls this
1663+
# unconditionally, so without this the deferral won by the
1664+
# module-level import removal was only from import time to
1665+
# first-session time.
1666+
#
1667+
# ``[]`` and not ``None``: the session is still host scope, so
1668+
# ``apply_path_mapping()`` must remain available with an empty rule
1669+
# set. ``SymbolTable.expr_host_rules`` stores this untouched
1670+
# (``Optional[list[Any]]``) and only crosses into Rust at evaluation
1671+
# time, so seeding ``[]`` costs nothing here.
1672+
return []
16561673
try:
16571674
from openjd.expr import PathFormat as ExprPathFormat
16581675
from openjd.expr import PathMappingRule as ExprPathMappingRule

test/openjd/test_import_purity.py

Lines changed: 126 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -281,3 +281,129 @@ def test_pure_path_module_does_not_load_native_extension(tmp_path: Path, module:
281281

282282
# THEN
283283
assert loaded == "False", f"importing {module} loaded the native extension"
284+
285+
286+
# ---------------------------------------------------------------------------
287+
# Import-time purity is not enough on its own: `Session.__init__` calls
288+
# `_build_expr_host_rules()` unconditionally, so before the empty-rules fast
289+
# path a worker that never evaluates an EXPR expression still loaded the
290+
# extension on its FIRST SESSION. The deferral won by removing the module-level
291+
# import was only from import time to first-session time.
292+
# ---------------------------------------------------------------------------
293+
294+
295+
class TestSessionLifecycleStaysExtensionFree:
296+
def test_constructing_a_session_without_path_mapping_rules_stays_pure(
297+
self, tmp_path: Path
298+
) -> None:
299+
"""The regression: `Session()` used to load the extension to build an
300+
empty engine rules list."""
301+
# WHEN
302+
loaded = _run_probe(
303+
tmp_path,
304+
"""
305+
import uuid
306+
307+
from openjd.sessions import Session
308+
309+
session = Session(session_id=uuid.uuid4().hex, job_parameter_values={})
310+
try:
311+
# `[]`, not `None`: the session is still host scope, so
312+
# apply_path_mapping() stays available with an empty rule set.
313+
assert session._expr_host_rules == [], session._expr_host_rules
314+
print(RS in sys.modules)
315+
finally:
316+
session.cleanup()
317+
""",
318+
)
319+
320+
# THEN
321+
assert loaded == "False", (
322+
"constructing a Session with no path mapping rules loaded the native "
323+
"extension; the empty-rules fast path in _build_expr_host_rules is "
324+
"what prevents this."
325+
)
326+
327+
def test_running_a_non_expr_task_stays_pure_end_to_end(self, tmp_path: Path) -> None:
328+
"""The property a non-EXPR worker actually cares about: a whole task
329+
runs without the extension ever being loaded."""
330+
# WHEN
331+
loaded = _run_probe(
332+
tmp_path,
333+
"""
334+
import time
335+
import uuid
336+
337+
from openjd.model.v2023_09 import ModelParsingContext, StepScript
338+
from openjd.sessions import ActionState, Session, SessionState
339+
340+
context = ModelParsingContext(supported_extensions=["FEATURE_BUNDLE_1"])
341+
script = StepScript.model_validate(
342+
{"actions": {"onRun": {"command": "echo", "args": ["hello"]}}},
343+
context=context,
344+
)
345+
session = Session(session_id=uuid.uuid4().hex, job_parameter_values={})
346+
try:
347+
session.run_task(step_script=script, task_parameter_values={})
348+
deadline = time.time() + 15
349+
while session.state == SessionState.RUNNING and time.time() < deadline:
350+
time.sleep(0.05)
351+
status = session.action_status
352+
assert status is not None and status.state == ActionState.SUCCESS, status
353+
print(RS in sys.modules)
354+
finally:
355+
session.cleanup()
356+
""",
357+
)
358+
359+
# THEN
360+
assert loaded == "False", (
361+
"running a non-EXPR task loaded the native extension somewhere in the "
362+
"session lifecycle"
363+
)
364+
365+
def test_path_mapping_rules_do_load_the_extension(self, tmp_path: Path) -> None:
366+
"""Negative control, and a documented limitation rather than a goal.
367+
368+
Real rules must be converted into ``openjd.expr.PathMappingRule`` engine
369+
objects to seed the session's host context, so this case genuinely loads
370+
the extension. openjd-sessions cannot avoid it alone: closing it needs
371+
openjd-model to accept unconverted rules and convert them at the Rust
372+
boundary, where ``symtab_to_expr_values`` already crosses.
373+
374+
Asserted so that the limitation is visible and so a future change that
375+
makes this pure is noticed here rather than passing silently.
376+
"""
377+
# WHEN
378+
loaded = _run_probe(
379+
tmp_path,
380+
"""
381+
import uuid
382+
from pathlib import PurePosixPath
383+
384+
from openjd.sessions import PathFormat, PathMappingRule, Session
385+
386+
rule = PathMappingRule(
387+
source_path_format=PathFormat.POSIX,
388+
source_path=PurePosixPath("/mnt/source"),
389+
destination_path=PurePosixPath("/mnt/dest"),
390+
)
391+
session = Session(
392+
session_id=uuid.uuid4().hex,
393+
job_parameter_values={},
394+
path_mapping_rules=[rule],
395+
)
396+
try:
397+
assert len(session._expr_host_rules) == 1, session._expr_host_rules
398+
print(RS in sys.modules)
399+
finally:
400+
session.cleanup()
401+
""",
402+
)
403+
404+
# THEN
405+
assert loaded == "True", (
406+
"a session with real path mapping rules is expected to load the "
407+
"extension; if this is now False the limitation has been fixed and "
408+
"this control should become a purity assertion"
409+
)

0 commit comments

Comments
 (0)