From 66d193d609a222d8e8dda81173ba4d5010c38b51 Mon Sep 17 00:00:00 2001 From: DABH Date: Thu, 10 Sep 2026 00:42:54 -0500 Subject: [PATCH 1/5] Skip importlib for already-loaded modules in the workflow sandbox The sandbox importer routed every import, including re-imports of modules already present in the sandbox's sys.modules, through the pure-Python importlib.__import__. On Python 3.10 that path always acquires the per-module lock in importlib._bootstrap._find_and_load, and _ModuleLock.acquire is not re-entrant there: it stores the current thread in the single-slot _blocking_on dict and deletes it in a finally block (fixed in 3.12 by python/cpython#91351). A cyclic GC pass can run inside that window while a workflow module is being loaded. Finalizing a never-awaited coroutine (or showing a warning with a source object) makes the C runtime call PyImport_Import("warnings"), which goes through the sandbox's builtins.__import__ and so re-entered _ModuleLock.acquire on the same thread. The nested call removed the _blocking_on entry and the outer acquire failed with KeyError(), surfacing as "RuntimeError: Failed validating workflow" when a worker started. CPython's C import never takes the lock for an initialized module, so plain Python does not hit this for already-imported modules. Mirror that: when the target module (and, for from-imports of a package, every requested attribute) is already fully imported, return it directly and only fall back to importlib.__import__ for real loads. Module identity, passthrough handling and restriction wrapping are unchanged, and imports executed inside workflow code get cheaper on every Python version. --- CHANGELOG.md | 7 ++++ .../worker/workflow_sandbox/_importer.py | 30 +++++++++++++- .../worker/workflow_sandbox/test_importer.py | 41 +++++++++++++++++++ 3 files changed, 77 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e8597d2c2..30d1cfeae 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -61,6 +61,13 @@ to include examples, links to docs, or any other relevant information. - The experimental `GetNexusOperationResultInput` now includes the Nexus endpoint, service, and operation. +### Fixed + +- Sandboxed workflow imports of already-loaded modules 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)). + ### :boom: Breaking Changes - Experimental external storage: `ExternalStorage.driver_selector` is now called with a diff --git a/temporalio/worker/workflow_sandbox/_importer.py b/temporalio/worker/workflow_sandbox/_importer.py index 1ab0a1dd6..bc19baed5 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 = _already_imported(name, 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,32 @@ def _get_thread_local_builtin(name: str) -> _ThreadLocalCallable: return ret +def _already_imported( + name: str, full_name: str, fromlist: Sequence[str], level: int +) -> types.ModuleType | None: + # Mirrors importlib.__import__ for loaded modules without taking module locks + mod = _fully_imported(full_name) + if mod is None: + return None + if fromlist: + if hasattr(mod, "__path__") and any( + not isinstance(x, str) or x == "*" or not hasattr(mod, x) for x in fromlist + ): + return None + return mod + if level != 0: + return None + top = 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..394f67403 100644 --- a/tests/worker/workflow_sandbox/test_importer.py +++ b/tests/worker/workflow_sandbox/test_importer.py @@ -1,5 +1,7 @@ import dataclasses +import importlib import sys +from typing import Any import pytest @@ -27,6 +29,45 @@ 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.passthrough_module as passthrough_again + import tests.worker.workflow_sandbox.testmodules.stateful_module as stateful_again + from tests.worker.workflow_sandbox import testmodules + from tests.worker.workflow_sandbox.testmodules import stateful_module + + assert passthrough_again is passthrough + assert stateful_again is stateful is stateful_module + assert getattr(testmodules, "stateful_module") is stateful + assert typing is sys.modules["typing"] + assert imported == [] + + def test_workflow_sandbox_importer_passthrough_module(): # Import outside of importer import tests.worker.workflow_sandbox.testmodules.passthrough_module as outside1 From 53b15194a0b474a4d5e04e9470f7432578849a59 Mon Sep 17 00:00:00 2001 From: DABH Date: Thu, 10 Sep 2026 02:47:11 -0500 Subject: [PATCH 2/5] Check fromlist names statically in the sandbox import fast path The fast path used hasattr for fromlist names, which runs a package's module-level __getattr__ before importlib runs it again on the fallback, so a missing name was probed twice. Look only at the module dict; dynamic attributes keep going through importlib exactly as before. --- .../worker/workflow_sandbox/_importer.py | 8 ++++-- .../worker/workflow_sandbox/test_importer.py | 26 +++++++++++++++++++ .../dynamic_attr_package/__init__.py | 8 ++++++ 3 files changed, 40 insertions(+), 2 deletions(-) create mode 100644 tests/worker/workflow_sandbox/testmodules/dynamic_attr_package/__init__.py diff --git a/temporalio/worker/workflow_sandbox/_importer.py b/temporalio/worker/workflow_sandbox/_importer.py index bc19baed5..9fa2b766b 100644 --- a/temporalio/worker/workflow_sandbox/_importer.py +++ b/temporalio/worker/workflow_sandbox/_importer.py @@ -549,8 +549,12 @@ def _already_imported( if mod is None: return None if fromlist: - if hasattr(mod, "__path__") and any( - not isinstance(x, str) or x == "*" or not hasattr(mod, x) for x in fromlist + # Only statically stored attributes count; module __getattr__ stays with importlib + mod_dict = getattr(mod, "__dict__", None) + if not isinstance(mod_dict, dict): + return None + if "__path__" in mod_dict and any( + not isinstance(x, str) or x == "*" or x not in mod_dict for x in fromlist ): return None return mod diff --git a/tests/worker/workflow_sandbox/test_importer.py b/tests/worker/workflow_sandbox/test_importer.py index 394f67403..b03db30dc 100644 --- a/tests/worker/workflow_sandbox/test_importer.py +++ b/tests/worker/workflow_sandbox/test_importer.py @@ -68,6 +68,32 @@ def recording_import( assert imported == [] +def test_workflow_sandbox_importer_repeat_import_leaves_module_getattr_to_importlib(): + with Importer(restrictions, RestrictionContext()).applied(): + import tests.worker.workflow_sandbox.testmodules.dynamic_attr_package as dyn_pkg + from tests.worker.workflow_sandbox.testmodules.dynamic_attr_package import ( + dynamic_value, + ) + + assert dynamic_value == 42 + before = len(dyn_pkg.getattr_calls) + # importlib's fromlist hasattr plus the IMPORT_FROM lookup, same as without the sandbox + from tests.worker.workflow_sandbox.testmodules.dynamic_attr_package import ( # noqa: F811 + dynamic_value, + ) + + assert dynamic_value == 42 + assert dyn_pkg.getattr_calls[before:] == ["dynamic_value", "dynamic_value"] + + # A missing name is probed once by importlib and once by IMPORT_FROM, not more + before = len(dyn_pkg.getattr_calls) + with pytest.raises(ImportError): + from tests.worker.workflow_sandbox.testmodules.dynamic_attr_package import ( # type: ignore[attr-defined] + missing_value, + ) + assert dyn_pkg.getattr_calls[before:] == ["missing_value", "missing_value"] + + def test_workflow_sandbox_importer_passthrough_module(): # Import outside of importer import tests.worker.workflow_sandbox.testmodules.passthrough_module as outside1 diff --git a/tests/worker/workflow_sandbox/testmodules/dynamic_attr_package/__init__.py b/tests/worker/workflow_sandbox/testmodules/dynamic_attr_package/__init__.py new file mode 100644 index 000000000..0809882fd --- /dev/null +++ b/tests/worker/workflow_sandbox/testmodules/dynamic_attr_package/__init__.py @@ -0,0 +1,8 @@ +getattr_calls: list[str] = [] + + +def __getattr__(name: str) -> int: + getattr_calls.append(name) + if name == "dynamic_value": + return 42 + raise AttributeError(name) From d20dd2cdf68ee398e754f2dfde231ad9de93c5c6 Mon Sep 17 00:00:00 2001 From: DABH Date: Thu, 10 Sep 2026 02:49:35 -0500 Subject: [PATCH 3/5] Exercise dynamic names through __import__ in the importer test --- .../worker/workflow_sandbox/test_importer.py | 26 +++++++------------ 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/tests/worker/workflow_sandbox/test_importer.py b/tests/worker/workflow_sandbox/test_importer.py index b03db30dc..5390ab93a 100644 --- a/tests/worker/workflow_sandbox/test_importer.py +++ b/tests/worker/workflow_sandbox/test_importer.py @@ -69,28 +69,22 @@ def recording_import( def test_workflow_sandbox_importer_repeat_import_leaves_module_getattr_to_importlib(): + pkg_name = "tests.worker.workflow_sandbox.testmodules.dynamic_attr_package" with Importer(restrictions, RestrictionContext()).applied(): - import tests.worker.workflow_sandbox.testmodules.dynamic_attr_package as dyn_pkg - from tests.worker.workflow_sandbox.testmodules.dynamic_attr_package import ( - dynamic_value, - ) - - assert dynamic_value == 42 + dyn_pkg = importlib.import_module(pkg_name) + assert dyn_pkg.dynamic_value == 42 before = len(dyn_pkg.getattr_calls) - # importlib's fromlist hasattr plus the IMPORT_FROM lookup, same as without the sandbox - from tests.worker.workflow_sandbox.testmodules.dynamic_attr_package import ( # noqa: F811 - dynamic_value, - ) - assert dynamic_value == 42 + # importlib's fromlist hasattr plus the attribute read, same as without the sandbox + pkg = __import__(pkg_name, fromlist=["dynamic_value"]) + assert pkg.dynamic_value == 42 assert dyn_pkg.getattr_calls[before:] == ["dynamic_value", "dynamic_value"] - # A missing name is probed once by importlib and once by IMPORT_FROM, not more + # A missing name is probed once by importlib and once by the read, not more before = len(dyn_pkg.getattr_calls) - with pytest.raises(ImportError): - from tests.worker.workflow_sandbox.testmodules.dynamic_attr_package import ( # type: ignore[attr-defined] - missing_value, - ) + pkg = __import__(pkg_name, fromlist=["missing_value"]) + with pytest.raises(AttributeError): + getattr(pkg, "missing_value") assert dyn_pkg.getattr_calls[before:] == ["missing_value", "missing_value"] From e6782d111cf5364771467255c5abdda7b90b9f6b Mon Sep 17 00:00:00 2001 From: DABH Date: Wed, 16 Sep 2026 01:33:43 -0500 Subject: [PATCH 4/5] Simplify loaded-module import fast path --- CHANGELOG.md | 13 ++--- .../worker/workflow_sandbox/_importer.py | 25 +++----- .../worker/workflow_sandbox/test_importer.py | 57 ++++++++++++------- .../dynamic_attr_package/__init__.py | 8 --- 4 files changed, 49 insertions(+), 54 deletions(-) delete mode 100644 tests/worker/workflow_sandbox/testmodules/dynamic_attr_package/__init__.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 30d1cfeae..20d42ca49 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,12 @@ 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)). + ### Security ## [1.33.0] - 2026-09-14 @@ -61,13 +67,6 @@ to include examples, links to docs, or any other relevant information. - The experimental `GetNexusOperationResultInput` now includes the Nexus endpoint, service, and operation. -### Fixed - -- Sandboxed workflow imports of already-loaded modules 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)). - ### :boom: Breaking Changes - Experimental external storage: `ExternalStorage.driver_selector` is now called with a diff --git a/temporalio/worker/workflow_sandbox/_importer.py b/temporalio/worker/workflow_sandbox/_importer.py index 9fa2b766b..058ad5a83 100644 --- a/temporalio/worker/workflow_sandbox/_importer.py +++ b/temporalio/worker/workflow_sandbox/_importer.py @@ -258,7 +258,7 @@ def _import( sys.modules[full_name] = new_mod new_spec.loader.exec_module(new_mod) - mod = _already_imported(name, full_name, 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 @@ -541,26 +541,17 @@ def _get_thread_local_builtin(name: str) -> _ThreadLocalCallable: return ret -def _already_imported( - name: str, full_name: str, fromlist: Sequence[str], level: int +def _loaded_module_for_import( + full_name: str, fromlist: Sequence[str], level: int ) -> types.ModuleType | None: - # Mirrors importlib.__import__ for loaded modules without taking module locks + # 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 - if fromlist: - # Only statically stored attributes count; module __getattr__ stays with importlib - mod_dict = getattr(mod, "__dict__", None) - if not isinstance(mod_dict, dict): - return None - if "__path__" in mod_dict and any( - not isinstance(x, str) or x == "*" or x not in mod_dict for x in fromlist - ): - return None - return mod - if level != 0: - return None - top = name.partition(".")[0] + top = full_name.partition(".")[0] return mod if top == full_name else _fully_imported(top) diff --git a/tests/worker/workflow_sandbox/test_importer.py b/tests/worker/workflow_sandbox/test_importer.py index 5390ab93a..b8350b61a 100644 --- a/tests/worker/workflow_sandbox/test_importer.py +++ b/tests/worker/workflow_sandbox/test_importer.py @@ -1,6 +1,8 @@ import dataclasses import importlib +import importlib.machinery import sys +import types from typing import Any import pytest @@ -8,6 +10,7 @@ from temporalio import workflow from temporalio.worker.workflow_sandbox._importer import ( Importer, + _loaded_module_for_import, _thread_local_sys_modules, _ThreadLocalSysModules, ) @@ -56,36 +59,46 @@ def recording_import( # 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 - from tests.worker.workflow_sandbox import testmodules - from tests.worker.workflow_sandbox.testmodules import stateful_module assert passthrough_again is passthrough - assert stateful_again is stateful is stateful_module + assert stateful_again is stateful assert getattr(testmodules, "stateful_module") is stateful assert typing is sys.modules["typing"] - assert imported == [] + 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_workflow_sandbox_importer_repeat_import_leaves_module_getattr_to_importlib(): - pkg_name = "tests.worker.workflow_sandbox.testmodules.dynamic_attr_package" - with Importer(restrictions, RestrictionContext()).applied(): - dyn_pkg = importlib.import_module(pkg_name) - assert dyn_pkg.dynamic_value == 42 - before = len(dyn_pkg.getattr_calls) - - # importlib's fromlist hasattr plus the attribute read, same as without the sandbox - pkg = __import__(pkg_name, fromlist=["dynamic_value"]) - assert pkg.dynamic_value == 42 - assert dyn_pkg.getattr_calls[before:] == ["dynamic_value", "dynamic_value"] - - # A missing name is probed once by importlib and once by the read, not more - before = len(dyn_pkg.getattr_calls) - pkg = __import__(pkg_name, fromlist=["missing_value"]) - with pytest.raises(AttributeError): - getattr(pkg, "missing_value") - assert dyn_pkg.getattr_calls[before:] == ["missing_value", "missing_value"] + +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(): diff --git a/tests/worker/workflow_sandbox/testmodules/dynamic_attr_package/__init__.py b/tests/worker/workflow_sandbox/testmodules/dynamic_attr_package/__init__.py deleted file mode 100644 index 0809882fd..000000000 --- a/tests/worker/workflow_sandbox/testmodules/dynamic_attr_package/__init__.py +++ /dev/null @@ -1,8 +0,0 @@ -getattr_calls: list[str] = [] - - -def __getattr__(name: str) -> int: - getattr_calls.append(name) - if name == "dynamic_value": - return 42 - raise AttributeError(name) From 4febdbff46d330dfe86dc2c10cdf844877b5b82e Mon Sep 17 00:00:00 2001 From: DABH Date: Wed, 16 Sep 2026 02:01:47 -0500 Subject: [PATCH 5/5] Pin the module-lock re-entry regression in a deterministic test Rebuilds importlib._bootstrap._ModuleLock.acquire from the interpreter's own _bootstrap.py with a nested builtins.__import__("warnings") placed inside the _blocking_on window while the sandbox loads a non-passthrough module: the exact re-entry a GC finalizer causes on Python 3.10/3.11. Fails on main with KeyError(); passes with the loaded-module fast path; skipped from 3.12, where the lock is re-entrant. --- .../worker/workflow_sandbox/test_importer.py | 67 +++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/tests/worker/workflow_sandbox/test_importer.py b/tests/worker/workflow_sandbox/test_importer.py index b8350b61a..94e57b612 100644 --- a/tests/worker/workflow_sandbox/test_importer.py +++ b/tests/worker/workflow_sandbox/test_importer.py @@ -17,6 +17,7 @@ from temporalio.worker.workflow_sandbox._restrictions import ( RestrictedWorkflowAccessError, RestrictionContext, + SandboxRestrictions, ) from .testmodules import restrictions @@ -230,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