From d19b06173723b1f28a26985e00fe6658b14c611e Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 22 Sep 2026 11:46:51 -0400 Subject: [PATCH 1/4] fix(integrations): don't shortcut a module that is still initializing The sys.modules fast path in _import_optional_module returned any module present in sys.modules. The loader inserts a module there *before* running its body, so when setup() races a provider import on another thread the shortcut hands back a half-built module: its patch targets do not exist yet, applies() concludes they are absent, and the patcher is silently skipped. importlib.import_module would have blocked on the per-module lock instead. Guard the shortcut with the spec's _initializing flag, which is the same predicate CPython uses to decide the lock is unnecessary -- 3.11+ has this exact fast path inside _find_and_load. 3.10 locks unconditionally, which is why the shortcut is worth keeping there at all. Reported by Codex review on #783. Co-Authored-By: Claude Opus 5 (1M context) --- py/src/braintrust/integrations/base.py | 21 +++++++++++----- py/src/braintrust/integrations/test_base.py | 28 +++++++++++++++++++++ 2 files changed, 43 insertions(+), 6 deletions(-) diff --git a/py/src/braintrust/integrations/base.py b/py/src/braintrust/integrations/base.py index 127ed893..5c43e1eb 100644 --- a/py/src/braintrust/integrations/base.py +++ b/py/src/braintrust/integrations/base.py @@ -630,14 +630,23 @@ def detect_version(cls, module: Any) -> str | None: def _import_optional_module(name: str) -> Any | None: """Return the named module, or ``None`` when it cannot be imported. - ``sys.modules`` is consulted first so an already-imported module never - reacquires the import lock. Patcher resolution calls this for every - patcher on every ``setup()``, and on CPython 3.10 that lock traffic can - trip the interpreter's own re-entrancy bookkeeping, surfacing as - ``KeyError: `` from ``importlib._bootstrap``. + A module that is already imported is returned straight from + ``sys.modules`` so it never reacquires the import lock. Patcher resolution + calls this for every patcher on every ``setup()``, and CPython 3.10's + ``_find_and_load`` locks unconditionally, so that traffic can trip the + interpreter's own re-entrancy bookkeeping and surface as + ``KeyError: `` from ``importlib._bootstrap``. (3.11+ added this + same shortcut upstream.) + + The ``_initializing`` check is what makes the shortcut safe: a module whose + body is still executing is already in ``sys.modules`` but does not have its + attributes yet, so returning it would make patch targets look absent and + silently skip instrumentation. Falling through to ``import_module`` blocks + on the per-module lock until the other thread finishes. This mirrors the + predicate CPython uses for the same decision. """ module = sys.modules.get(name) - if module is not None: + if module is not None and not getattr(getattr(module, "__spec__", None), "_initializing", False): return module try: return importlib.import_module(name) diff --git a/py/src/braintrust/integrations/test_base.py b/py/src/braintrust/integrations/test_base.py index 00b8495f..93aa81ce 100644 --- a/py/src/braintrust/integrations/test_base.py +++ b/py/src/braintrust/integrations/test_base.py @@ -1,3 +1,4 @@ +import importlib.machinery import sys import types @@ -73,3 +74,30 @@ def explode(name): # pragma: no cover - must never be reached def test_import_optional_module_imports_when_absent(): assert _import_optional_module("json") is sys.modules["json"] assert _import_optional_module("braintrust_module_that_does_not_exist") is None + + +def test_import_optional_module_waits_for_initializing_module(monkeypatch): + """A half-built module must not short-circuit the import machinery. + + The loader puts a module in sys.modules *before* running its body, so + during a concurrent import the attributes a patcher looks for do not + exist yet. Returning it would make the target look absent and silently + skip instrumentation, so we must fall through and let import_module + block on the per-module lock. + """ + partial = types.ModuleType("braintrust_partial_sdk") + partial.__spec__ = importlib.machinery.ModuleSpec("braintrust_partial_sdk", loader=None) + partial.__spec__._initializing = True + monkeypatch.setitem(sys.modules, "braintrust_partial_sdk", partial) + + finished = types.ModuleType("braintrust_partial_sdk") + monkeypatch.setattr( + "braintrust.integrations.base.importlib.import_module", + lambda name: finished, + ) + + assert _import_optional_module("braintrust_partial_sdk") is finished + + # Once initialization completes the shortcut applies again. + partial.__spec__._initializing = False + assert _import_optional_module("braintrust_partial_sdk") is partial From 1b0c93081c889a759b274d5cf4224cf87cabe97f Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 22 Sep 2026 12:44:36 -0400 Subject: [PATCH 2/4] fix types --- py/src/braintrust/integrations/test_base.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/py/src/braintrust/integrations/test_base.py b/py/src/braintrust/integrations/test_base.py index 93aa81ce..cb022c4a 100644 --- a/py/src/braintrust/integrations/test_base.py +++ b/py/src/braintrust/integrations/test_base.py @@ -1,4 +1,5 @@ import importlib.machinery +import importlib.util import sys import types @@ -85,9 +86,11 @@ def test_import_optional_module_waits_for_initializing_module(monkeypatch): skip instrumentation, so we must fall through and let import_module block on the per-module lock. """ - partial = types.ModuleType("braintrust_partial_sdk") - partial.__spec__ = importlib.machinery.ModuleSpec("braintrust_partial_sdk", loader=None) - partial.__spec__._initializing = True + # module_from_spec wires up __spec__ the way the real loader does, so the + # flag can be flipped on the spec itself. + spec = importlib.machinery.ModuleSpec("braintrust_partial_sdk", loader=None) + spec._initializing = True + partial = importlib.util.module_from_spec(spec) monkeypatch.setitem(sys.modules, "braintrust_partial_sdk", partial) finished = types.ModuleType("braintrust_partial_sdk") @@ -99,5 +102,5 @@ def test_import_optional_module_waits_for_initializing_module(monkeypatch): assert _import_optional_module("braintrust_partial_sdk") is finished # Once initialization completes the shortcut applies again. - partial.__spec__._initializing = False + spec._initializing = False assert _import_optional_module("braintrust_partial_sdk") is partial From f290f75ed28096a75ec42a219ce4105b08a7cd21 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 22 Sep 2026 12:58:46 -0400 Subject: [PATCH 3/4] fix(integrations): stop retrying imports of absent patcher targets The sys.modules fast path missed the case that was actually crashing. CI's traceback names _ModuleLock('mistralai.chat') -- a module that does not exist under mistralai 2.x. An absent module is never in sys.modules, so the shortcut never applies to it and every call falls through to import_module, takes CPython 3.10's unconditional module lock, and raises. Patchers carry target modules for layouts only some provider versions ship, so misses are normal and permanent. Measured over two MistralIntegration .setup() calls: 96 failing imports, 12 of them for mistralai.chat alone, across seven absent modules (chat, embeddings, fim, agents, conversations, ocr, transcriptions). Caching the failure takes that to 7 and 1. The sys.modules lookup still runs first, so a module that genuinely appears later is found -- the cache suppresses the retry, it does not shadow a real module. This still does not reproduce locally (test_mistral(latest) is 5/5 green on 3.10 here), so it is not proven to be the cure. It does delete the exact call path in the traceback, which the previous attempt did not. Co-Authored-By: Claude Opus 5 (1M context) --- py/src/braintrust/integrations/base.py | 14 +++++++ py/src/braintrust/integrations/test_base.py | 41 ++++++++++++++++++++- 2 files changed, 54 insertions(+), 1 deletion(-) diff --git a/py/src/braintrust/integrations/base.py b/py/src/braintrust/integrations/base.py index 5c43e1eb..874a4bd1 100644 --- a/py/src/braintrust/integrations/base.py +++ b/py/src/braintrust/integrations/base.py @@ -627,6 +627,11 @@ def detect_version(cls, module: Any) -> str | None: return detect_module_version(module, cls.import_names) +# Module names that raised ImportError once. Bounded by the patchers' static +# target_module attributes. +_UNIMPORTABLE_MODULES: set[str] = set() + + def _import_optional_module(name: str) -> Any | None: """Return the named module, or ``None`` when it cannot be imported. @@ -648,9 +653,18 @@ def _import_optional_module(name: str) -> Any | None: module = sys.modules.get(name) if module is not None and not getattr(getattr(module, "__spec__", None), "_initializing", False): return module + if name in _UNIMPORTABLE_MODULES: + return None try: return importlib.import_module(name) except ImportError: + # Patchers carry target modules for layouts that only some versions of + # a provider ship, so a miss is normal and permanent -- mistralai 2.x, + # for example, leaves seven of them unimportable and two setup() calls + # retry them ~96 times between them. Each retry takes the import lock, + # which is what CPython 3.10 trips over. The sys.modules check above + # runs first, so a module that genuinely shows up later is still found. + _UNIMPORTABLE_MODULES.add(name) return None diff --git a/py/src/braintrust/integrations/test_base.py b/py/src/braintrust/integrations/test_base.py index cb022c4a..d03b79e4 100644 --- a/py/src/braintrust/integrations/test_base.py +++ b/py/src/braintrust/integrations/test_base.py @@ -72,7 +72,8 @@ def explode(name): # pragma: no cover - must never be reached assert _import_optional_module("braintrust_fake_sdk") is sentinel -def test_import_optional_module_imports_when_absent(): +def test_import_optional_module_imports_when_absent(monkeypatch): + monkeypatch.setattr("braintrust.integrations.base._UNIMPORTABLE_MODULES", set()) assert _import_optional_module("json") is sys.modules["json"] assert _import_optional_module("braintrust_module_that_does_not_exist") is None @@ -104,3 +105,41 @@ def test_import_optional_module_waits_for_initializing_module(monkeypatch): # Once initialization completes the shortcut applies again. spec._initializing = False assert _import_optional_module("braintrust_partial_sdk") is partial + + +def _raise_import_error(name): + raise ImportError(name) + + +def test_import_optional_module_caches_failures(monkeypatch): + """A permanently-absent target must not be retried on every setup(). + + Patchers carry target modules for provider layouts that only some + versions ship, so misses are normal -- and each retry takes the import + lock, which is what CPython 3.10 trips over. + """ + monkeypatch.setattr("braintrust.integrations.base._UNIMPORTABLE_MODULES", set()) + attempts = [] + + def failing(name): + attempts.append(name) + raise ImportError(name) + + monkeypatch.setattr("braintrust.integrations.base.importlib.import_module", failing) + + assert _import_optional_module("braintrust_absent_sdk") is None + assert _import_optional_module("braintrust_absent_sdk") is None + assert _import_optional_module("braintrust_absent_sdk") is None + assert attempts == ["braintrust_absent_sdk"] + + +def test_import_optional_module_still_sees_a_late_arrival(monkeypatch): + """A cached failure must not hide a module that shows up afterwards.""" + monkeypatch.setattr("braintrust.integrations.base._UNIMPORTABLE_MODULES", set()) + monkeypatch.setattr("braintrust.integrations.base.importlib.import_module", _raise_import_error) + + assert _import_optional_module("braintrust_late_sdk") is None + + late = types.ModuleType("braintrust_late_sdk") + monkeypatch.setitem(sys.modules, "braintrust_late_sdk", late) + assert _import_optional_module("braintrust_late_sdk") is late From 02306056ba9118b87b7603160c63b522f82cb8af Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 22 Sep 2026 13:15:20 -0400 Subject: [PATCH 4/4] fix(integrations): tolerate CPython's import-lock KeyError on 3.10/3.11 Third attempt at the ubuntu/3.10 mistral failure, and the first one aimed at the right thing. The previous two reduced how often we take the import lock; the latest traceback shows mistralai.chat failing on its first and only attempt, so volume was never the variable. The real cause is in the interpreter. Checked across all five versions we support: 3.10, 3.11 _blocking_on holds one slot per thread, and _ModuleLock.acquire() clears it unconditionally in finally 3.12+ list-based stack (fixed upstream) So when an import nests on one thread, the inner frame clears the slot and the outer frame's cleanup raises KeyError: . Patcher resolution cannot avoid importing optional submodules, so we have to tolerate it. Catch only that KeyError, retry once (the bookkeeping is per-call, so a fresh slot normally works), and if it trips again report the module as unavailable -- setup() runs inside the caller's application, and skipping one optional patch target beats raising an interpreter-internal KeyError at them. ImportError and everything else propagate untouched, so a genuinely absent module is still reported absent. On 3.12+ a KeyError is a real bug and re-raises. Keeps the negative cache from the previous commit: 96 failing imports down to 7 over two setup() calls is worth having on its own, it just was not sufficient. Verified: pylint + test_types on 3.10-3.14 (10/10), test_mistral(latest) 5/5 on 3.10, test_core on 3.10 and 3.14, and an eight-session integration sweep. Co-Authored-By: Claude Opus 5 (1M context) --- py/src/braintrust/integrations/base.py | 35 ++++++++++++- py/src/braintrust/integrations/test_base.py | 57 +++++++++++++++++++++ 2 files changed, 91 insertions(+), 1 deletion(-) diff --git a/py/src/braintrust/integrations/base.py b/py/src/braintrust/integrations/base.py index 874a4bd1..66e708ef 100644 --- a/py/src/braintrust/integrations/base.py +++ b/py/src/braintrust/integrations/base.py @@ -631,6 +631,39 @@ def detect_version(cls, module: Any) -> str | None: # target_module attributes. _UNIMPORTABLE_MODULES: set[str] = set() +# CPython < 3.12 records "which lock is this thread blocked on" in a single +# dict slot per thread (``importlib._bootstrap._blocking_on[tid]``) and clears +# it unconditionally in ``_ModuleLock.acquire``'s finally. When an import +# nests on one thread -- a meta-path finder importing something while +# resolving a submodule, say -- the inner frame clears the slot and the outer +# frame's cleanup raises ``KeyError: ``. It is a bug in the +# interpreter, not in the module being imported. +_IMPORT_LOCK_BOOKKEEPING_IS_BUGGY = sys.version_info < (3, 12) + + +def _import_module_tolerating_lock_bug(name: str) -> Any: + """``importlib.import_module`` that retries the interpreter's lock bug. + + Only the spurious ``KeyError`` is handled; ``ImportError`` and every other + failure propagate untouched, so a genuinely absent module is still + reported as absent rather than papered over. + """ + try: + return importlib.import_module(name) + except KeyError: + if not _IMPORT_LOCK_BOOKKEEPING_IS_BUGGY: + raise + # The bookkeeping is per-call, so a second attempt normally gets a clean + # slot. If the interpreter trips again, report the module as unavailable: + # setup() runs inside the caller's application, and skipping one optional + # patch target beats raising an interpreter-internal KeyError at them. + try: + return importlib.import_module(name) + except KeyError: + if not _IMPORT_LOCK_BOOKKEEPING_IS_BUGGY: + raise + return None + def _import_optional_module(name: str) -> Any | None: """Return the named module, or ``None`` when it cannot be imported. @@ -656,7 +689,7 @@ def _import_optional_module(name: str) -> Any | None: if name in _UNIMPORTABLE_MODULES: return None try: - return importlib.import_module(name) + return _import_module_tolerating_lock_bug(name) except ImportError: # Patchers carry target modules for layouts that only some versions of # a provider ship, so a miss is normal and permanent -- mistralai 2.x, diff --git a/py/src/braintrust/integrations/test_base.py b/py/src/braintrust/integrations/test_base.py index d03b79e4..eba6538d 100644 --- a/py/src/braintrust/integrations/test_base.py +++ b/py/src/braintrust/integrations/test_base.py @@ -3,6 +3,8 @@ import sys import types +import pytest +from braintrust.integrations import base from braintrust.integrations.base import _import_optional_module, _resolve_attr_path @@ -143,3 +145,58 @@ def test_import_optional_module_still_sees_a_late_arrival(monkeypatch): late = types.ModuleType("braintrust_late_sdk") monkeypatch.setitem(sys.modules, "braintrust_late_sdk", late) assert _import_optional_module("braintrust_late_sdk") is late + + +def test_import_optional_module_survives_the_import_lock_bug(monkeypatch): + """CPython < 3.12 can raise KeyError from its own import bookkeeping. + + _blocking_on holds one slot per thread and acquire() clears it + unconditionally, so a nested import makes the outer frame's cleanup raise + KeyError: . That is an interpreter bug, not a signal about the + module, and it must not escape into the caller's setup(). + """ + monkeypatch.setattr("braintrust.integrations.base._UNIMPORTABLE_MODULES", set()) + monkeypatch.setattr("braintrust.integrations.base._IMPORT_LOCK_BOOKKEEPING_IS_BUGGY", True) + + recovered = types.ModuleType("braintrust_locky_sdk") + calls = [] + + def flaky(name): + calls.append(name) + if len(calls) == 1: + raise KeyError(140000000000000) + return recovered + + monkeypatch.setattr("braintrust.integrations.base.importlib.import_module", flaky) + + assert _import_optional_module("braintrust_locky_sdk") is recovered + assert len(calls) == 2 + + +def test_import_optional_module_gives_up_if_the_lock_bug_persists(monkeypatch): + """A second KeyError reports the module absent rather than crashing.""" + monkeypatch.setattr("braintrust.integrations.base._UNIMPORTABLE_MODULES", set()) + monkeypatch.setattr("braintrust.integrations.base._IMPORT_LOCK_BOOKKEEPING_IS_BUGGY", True) + + def always_keyerror(name): + raise KeyError(140000000000000) + + monkeypatch.setattr("braintrust.integrations.base.importlib.import_module", always_keyerror) + + assert _import_optional_module("braintrust_locky_sdk") is None + # Not cached as unimportable: the import never actually resolved. + assert "braintrust_locky_sdk" not in base._UNIMPORTABLE_MODULES + + +def test_import_optional_module_propagates_keyerror_on_fixed_interpreters(monkeypatch): + """On 3.12+ a KeyError is a real error and must not be swallowed.""" + monkeypatch.setattr("braintrust.integrations.base._UNIMPORTABLE_MODULES", set()) + monkeypatch.setattr("braintrust.integrations.base._IMPORT_LOCK_BOOKKEEPING_IS_BUGGY", False) + + def boom(name): + raise KeyError("a real bug in the module's import") + + monkeypatch.setattr("braintrust.integrations.base.importlib.import_module", boom) + + with pytest.raises(KeyError): + _import_optional_module("braintrust_locky_sdk")