Skip importlib for already-loaded modules in the workflow sandbox - #1833
Draft
DABH wants to merge 4 commits into
Draft
Skip importlib for already-loaded modules in the workflow sandbox#1833DABH wants to merge 4 commits into
DABH wants to merge 4 commits into
Conversation
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(<thread id>), 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.
There was a problem hiding this comment.
🟡 Changes recommended
The new fromlist probe can invoke module-level __getattr__ twice and alter import behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Optimizes sandbox imports to bypass module locks for fully loaded modules, addressing intermittent Python 3.10 workflow validation failures.
Changes:
- Adds a fast path for loaded modules.
- Adds regression coverage and a changelog entry.
File summaries
| File | Description |
|---|---|
temporalio/worker/workflow_sandbox/_importer.py |
Implements the loaded-module fast path. |
tests/worker/workflow_sandbox/test_importer.py |
Tests repeated imports bypassing importlib. |
CHANGELOG.md |
Documents the fix. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What was changed
The sandbox importer returns modules that are already fully imported straight from the sandbox
sys.modules, mirroringimportlib.__import__forfromlist, top-level and relative imports, and falls back toimportlibfor anything that still needs loading. Regression test and CHANGELOG entry.Why
CPython bug 91351: before 3.12 the per-module import lock is not re-entrant, and on 3.10 the pure-Python import path takes it even for loaded modules. Routing every import through
importlib.__import__meant a GC pass inside the lock that finalizes a never-awaited coroutine (which importswarnings) re-entered the lock on the same thread and failed withKeyError: <thread id>, surfacing asRuntimeError: Failed validating workflow. Plain Python never hits this for loaded modules because the C import path skips the lock.Testing
Targeted reproduction on 3.10: 10/10 failures before, 0/10 after; 3.11 and 3.14 unaffected either way. Worker, replayer, sandbox and contrib suites pass on 3.10 and 3.14. Lint clean.