From a8c627d3ba85e2f0d37f6bbdc1c80867014bcf4d Mon Sep 17 00:00:00 2001 From: sdk-sentinel-bot Date: Mon, 21 Sep 2026 00:43:54 +0000 Subject: [PATCH] Avoid sandbox module-lock re-entry --- CHANGELOG.md | 5 + .../worker/workflow_sandbox/_importer.py | 25 +++- .../worker/workflow_sandbox/test_importer.py | 141 ++++++++++++++++++ 3 files changed, 170 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 03a2243ed..2e5d651b9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -52,6 +52,11 @@ to include examples, links to docs, or any other relevant information. ### Fixed +- Ordinary absolute imports of already-loaded modules in sandboxed workflows no longer go through + importlib's module locks, fixing intermittent `Failed validating workflow` errors on Python 3.10 + caused by a `KeyError` in `importlib._bootstrap._ModuleLock.acquire` when a garbage-collection + finalizer imported `warnings` during a workflow load + ([#585](https://github.com/temporalio/sdk-python/issues/585)). - `GoogleAdkPlugin` now passes the optional `anthropic`, `litellm`, and `openai` SDKs through the workflow sandbox. - `contrib.deepagents`: prevent duplicate input messages after continue-as-new. diff --git a/temporalio/worker/workflow_sandbox/_importer.py b/temporalio/worker/workflow_sandbox/_importer.py index 1ab0a1dd6..058ad5a83 100644 --- a/temporalio/worker/workflow_sandbox/_importer.py +++ b/temporalio/worker/workflow_sandbox/_importer.py @@ -258,7 +258,9 @@ def _import( sys.modules[full_name] = new_mod new_spec.loader.exec_module(new_mod) - mod = importlib.__import__(name, globals, locals, fromlist, level) + mod = _loaded_module_for_import(full_name, fromlist, level) + if mod is None: + mod = importlib.__import__(name, globals, locals, fromlist, level) # Check for restrictions if necessary and apply if mod.__name__ not in self.modules_checked_for_restrictions: self.modules_checked_for_restrictions.add(mod.__name__) @@ -539,6 +541,27 @@ def _get_thread_local_builtin(name: str) -> _ThreadLocalCallable: return ret +def _loaded_module_for_import( + full_name: str, fromlist: Sequence[str], level: int +) -> types.ModuleType | None: + # The failing GC warning path uses an ordinary absolute import. Leave the + # more involved forms, which may load children or run module hooks, to importlib. + if fromlist or level: + return None + mod = _fully_imported(full_name) + if mod is None: + return None + top = full_name.partition(".")[0] + return mod if top == full_name else _fully_imported(top) + + +def _fully_imported(name: str) -> types.ModuleType | None: + mod = sys.modules.get(name) + if mod is None or getattr(getattr(mod, "__spec__", None), "_initializing", False): + return None + return mod + + def _resolve_module_name( name: str, globals: Mapping[str, object] | None, level: int ) -> str: diff --git a/tests/worker/workflow_sandbox/test_importer.py b/tests/worker/workflow_sandbox/test_importer.py index 0ed478c03..94e57b612 100644 --- a/tests/worker/workflow_sandbox/test_importer.py +++ b/tests/worker/workflow_sandbox/test_importer.py @@ -1,17 +1,23 @@ import dataclasses +import importlib +import importlib.machinery import sys +import types +from typing import Any import pytest from temporalio import workflow from temporalio.worker.workflow_sandbox._importer import ( Importer, + _loaded_module_for_import, _thread_local_sys_modules, _ThreadLocalSysModules, ) from temporalio.worker.workflow_sandbox._restrictions import ( RestrictedWorkflowAccessError, RestrictionContext, + SandboxRestrictions, ) from .testmodules import restrictions @@ -27,6 +33,75 @@ def test_workflow_sandbox_importer_invalid_module(): ) +def test_workflow_sandbox_importer_repeat_import_skips_import_machinery( + monkeypatch: pytest.MonkeyPatch, +): + imported: list[str] = [] + orig_import = importlib.__import__ + + def recording_import( + name: str, + globals: Any = None, + locals: Any = None, + fromlist: Any = (), + level: int = 0, + ) -> Any: + imported.append(name) + return orig_import(name, globals, locals, fromlist, level) + + monkeypatch.setattr(importlib, "__import__", recording_import) + with Importer(restrictions, RestrictionContext()).applied(): + import tests.worker.workflow_sandbox.testmodules.passthrough_module as passthrough + import tests.worker.workflow_sandbox.testmodules.stateful_module as stateful + + assert imported + imported.clear() + + # Loaded modules are served from sys.modules without re-entering importlib + import typing + + import tests.worker.workflow_sandbox.testmodules as testmodules + import tests.worker.workflow_sandbox.testmodules.passthrough_module as passthrough_again + import tests.worker.workflow_sandbox.testmodules.stateful_module as stateful_again + + assert passthrough_again is passthrough + assert stateful_again is stateful + assert getattr(testmodules, "stateful_module") is stateful + assert typing is sys.modules["typing"] + assert imported == [] + + # Imports with a fromlist keep using importlib's full semantics. + from tests.worker.workflow_sandbox.testmodules import stateful_module + + assert stateful_module is stateful + assert imported == ["tests.worker.workflow_sandbox.testmodules"] + + +def test_loaded_module_fast_path_requires_completed_modules( + monkeypatch: pytest.MonkeyPatch, +): + top_name = "test_loaded_module_fast_path" + child_name = f"{top_name}.child" + top = types.ModuleType(top_name) + child = types.ModuleType(child_name) + top_spec = importlib.machinery.ModuleSpec(top_name, loader=None) + child_spec = importlib.machinery.ModuleSpec(child_name, loader=None) + top.__spec__ = top_spec + child.__spec__ = child_spec + monkeypatch.setitem(sys.modules, top_name, top) + monkeypatch.setitem(sys.modules, child_name, child) + + assert _loaded_module_for_import(child_name, ("child",), 0) is None + assert _loaded_module_for_import(child_name, (), 1) is None + setattr(child_spec, "_initializing", True) + assert _loaded_module_for_import(child_name, (), 0) is None + setattr(child_spec, "_initializing", False) + setattr(top_spec, "_initializing", True) + assert _loaded_module_for_import(child_name, (), 0) is None + setattr(top_spec, "_initializing", False) + assert _loaded_module_for_import(child_name, (), 0) is top + + def test_workflow_sandbox_importer_passthrough_module(): # Import outside of importer import tests.worker.workflow_sandbox.testmodules.passthrough_module as outside1 @@ -156,3 +231,69 @@ def test_thread_local_sys_module_attrs(): norm |= {"baz": 789} thread_local |= {"baz": 789} # type: ignore assert norm.copy() == thread_local.copy() + + +@pytest.mark.skipif( + sys.version_info >= (3, 12), + reason="importlib's module lock is re-entrant from 3.12 (python/cpython#91351)", +) +def test_workflow_sandbox_importer_loaded_module_import_inside_module_lock( + monkeypatch: pytest.MonkeyPatch, tmp_path: Any +): + # Regression for the 3.10/3.11 failure behind "Failed validating workflow": + # while the sandbox loads a workflow module, importlib._bootstrap._ModuleLock + # .acquire holds _blocking_on[tid] until its finally. A GC finalizer inside + # that window imports `warnings` through builtins.__import__ (the sandbox's); + # routing that already-loaded module through importlib re-enters the lock and + # the outer acquire fails with KeyError(). Reproduced here + # deterministically by rebuilding acquire from the interpreter's own + # _bootstrap.py with the nested import placed inside the window. + import ast + import builtins + import importlib + import importlib._bootstrap as bootstrap + import pathlib + import textwrap + + text = pathlib.Path(importlib.__file__).with_name("_bootstrap.py").read_text() + acquire_src = next( + ast.get_source_segment(text, item) + for node in ast.walk(ast.parse(text)) + if isinstance(node, ast.ClassDef) and node.name == "_ModuleLock" + for item in node.body + if isinstance(item, ast.FunctionDef) and item.name == "acquire" + ) + assert acquire_src is not None + marker = "_blocking_on[tid] = self\n" + assert marker in acquire_src + + nested_done = False + + def nested_import() -> None: + nonlocal nested_done + if not nested_done: + nested_done = True + builtins.__import__("warnings", {"__builtins__": builtins}, {}, [], 0) + + namespace = dict(vars(bootstrap)) + namespace["_test_nested_import"] = nested_import + exec( + textwrap.dedent(acquire_src).replace( + marker, marker + " _test_nested_import()\n", 1 + ), + namespace, + ) + # Not in typeshed, so reach it dynamically. + module_lock = getattr(bootstrap, "_ModuleLock") + monkeypatch.setattr(module_lock, "acquire", namespace["acquire"]) + + # A module the sandbox has to load itself (not passthrough), so the outer + # import really holds importlib's lock with the sandbox importer applied. + (tmp_path / "sandbox_lock_reentry_module.py").write_text("VALUE = 1\n") + monkeypatch.syspath_prepend(str(tmp_path)) + # Production restrictions: stdlib (including warnings) is passthrough. + with Importer(SandboxRestrictions.default, RestrictionContext()).applied(): + import sandbox_lock_reentry_module # type: ignore[import-not-found] + + assert sandbox_lock_reentry_module.VALUE == 1 + assert nested_done