diff --git a/tests/tools/test_connectors_import_deadlock.py b/tests/tools/test_connectors_import_deadlock.py new file mode 100644 index 0000000000..fcc438cac5 --- /dev/null +++ b/tests/tools/test_connectors_import_deadlock.py @@ -0,0 +1,109 @@ +"""Regression tests for the ``tools.connectors`` import surface. + +The package is imported concurrently with its own submodules: the tool registry's discovery +scan imports ``tools.connectors.tool`` while the gateway's Group Chat worker imports the +package (``tui_gateway.methods_connectors`` -> ``tools.connectors``). When ``__init__`` +imported its submodules eagerly, one thread held the package's module lock while waiting for +a submodule another thread already held, and Python 3.14 aborted the loser with +``_frozen_importlib._DeadlockError: deadlock detected by _ModuleLock('tools.connectors.tool')`` +— the Group Chat worker then failed to start ("mutating Group Chat commands will fail closed"). + +Both checks run in a subprocess so the assertions see a pristine ``sys.modules`` and cannot +perturb the test session's own imports. +""" + +import os +import subprocess +import sys +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parents[2] + +_NO_EAGER_SUBMODULE = """ +import sys +sys.path.insert(0, REPO_ROOT) +import tools.connectors # noqa: F401 +loaded = sorted(k for k in sys.modules if k.startswith("tools.connectors.")) +print("\\n".join(loaded)) +""" + +_RACE = """ +import importlib, sys, threading +sys.path.insert(0, REPO_ROOT) +errors = [] +ITER = 25 + +def purge(): + for name in [k for k in list(sys.modules) if k.startswith("tools.connectors")]: + sys.modules.pop(name, None) + +def run(target): + for _ in range(ITER): + try: + importlib.import_module(target) + except Exception as exc: # noqa: BLE001 + errors.append((target, type(exc).__name__)) + purge() + +threads = [threading.Thread(target=run, args=("tools.connectors",)) for _ in range(3)] +threads += [threading.Thread(target=run, args=("tools.connectors.tool",)) for _ in range(3)] +for t in threads: + t.start() +for t in threads: + t.join(timeout=120) +print("alive", sum(1 for t in threads if t.is_alive())) +print("errors", [e for e in errors if e[1] != "KeyError"]) +""" + + +def _run_snippet(snippet: str) -> subprocess.CompletedProcess: + env = dict(os.environ) + env["PYTHONPATH"] = str(REPO_ROOT) + return subprocess.run( + [sys.executable, "-c", snippet.replace("REPO_ROOT", repr(str(REPO_ROOT)))], + capture_output=True, + text=True, + timeout=180, + env=env, + cwd=str(REPO_ROOT), + ) + + +def test_package_import_does_not_load_submodules(): + """Importing the package must not import any submodule: that eager edge is the cycle.""" + proc = _run_snippet(_NO_EAGER_SUBMODULE) + assert proc.returncode == 0, proc.stderr + assert proc.stdout.strip() == "", f"eagerly imported: {proc.stdout.strip()}" + + +def test_concurrent_package_and_submodule_import_never_deadlocks(): + """The two real-world import paths racing must not raise _DeadlockError.""" + proc = _run_snippet(_RACE) + assert proc.returncode == 0, proc.stderr + assert "alive 0" in proc.stdout, proc.stdout + assert "errors []" in proc.stdout, proc.stdout + + +def test_lazy_surface_matches_all_and_raises_for_unknown_names(): + import tools.connectors as connectors + + assert sorted(connectors.__all__) == [ + "CONNECTOR_BATCH_SENTINEL", + "MANAGE_CONNECTIONS_SCHEMA", + "connector_describe", + "connector_search_hits", + "connectors_available", + "dispatch_connector_batch", + "dispatch_connector_call", + "is_connector_name", + "manage_connections", + ] + for name in connectors.__all__: + assert getattr(connectors, name) is not None + assert set(connectors.__all__) <= set(dir(connectors)) + try: + connectors.not_a_real_connector_name + except AttributeError as exc: + assert "not_a_real_connector_name" in str(exc) + else: # pragma: no cover - the attribute must not resolve + raise AssertionError("unknown attribute resolved") diff --git a/tools/connectors/__init__.py b/tools/connectors/__init__.py index 65181f1b75..b558419fdc 100644 --- a/tools/connectors/__init__.py +++ b/tools/connectors/__init__.py @@ -4,22 +4,51 @@ Only the names below are cross-package surface; imports beyond it need a design Siblings: ``contract`` (states, actors, transition table), ``operation`` (the record), ``live`` (open operation per session), ``run`` (the lifecycle loop), ``managed`` / ``mcp`` (per-kind hooks), ``targets``, ``search``, ``dispatch``, ``gateway/`` (HTTP wire + client). + +The surface is exported lazily (PEP 562) on purpose: this package is imported concurrently +from threads that also import its submodules directly (the tool registry's discovery scan +imports ``tools.connectors.tool`` while the gateway's Group Chat worker imports this +package). Eager ``from tools.connectors. import …`` lines made this module hold its own +module lock while reaching for a submodule another thread already held, which Python 3.14 +reports as ``_frozen_importlib._DeadlockError: deadlock detected by +_ModuleLock('tools.connectors.tool')`` and which aborts the Group Chat worker at startup. A +package ``__init__`` that imports no submodule cannot hold that lock while waiting for one, +so the cycle is gone. Attribute access, ``from tools.connectors import x`` and ``__all__`` +behave exactly as before. """ -from tools.connectors.dispatch import dispatch_connector_batch, dispatch_connector_call -from tools.connectors.gateway.bridge import connector_describe, connector_search_hits -from tools.connectors.gateway.config import connectors_available -from tools.connectors.gateway.names import CONNECTOR_BATCH_SENTINEL, is_connector_name -from tools.connectors.tool import MANAGE_CONNECTIONS_SCHEMA, manage_connections +import importlib -__all__ = [ - "CONNECTOR_BATCH_SENTINEL", - "MANAGE_CONNECTIONS_SCHEMA", - "connector_describe", - "connector_search_hits", - "connectors_available", - "dispatch_connector_batch", - "dispatch_connector_call", - "is_connector_name", - "manage_connections", -] +# name -> module owning it: the single source of truth for __all__ and __getattr__. +_LAZY_EXPORTS: "dict[str, str]" = { + "CONNECTOR_BATCH_SENTINEL": "tools.connectors.gateway.names", + "MANAGE_CONNECTIONS_SCHEMA": "tools.connectors.tool", + "connector_describe": "tools.connectors.gateway.bridge", + "connector_search_hits": "tools.connectors.gateway.bridge", + "connectors_available": "tools.connectors.gateway.config", + "dispatch_connector_batch": "tools.connectors.dispatch", + "dispatch_connector_call": "tools.connectors.dispatch", + "is_connector_name": "tools.connectors.gateway.names", + "manage_connections": "tools.connectors.tool", +} + +__all__ = sorted(_LAZY_EXPORTS) + + +def __getattr__(name: str): + """Resolve the public surface on first use (PEP 562). + + Deliberately no eager import: see the module docstring for the import-deadlock this + prevents. Module-level ``__getattr__`` is only called for names not already bound, so + each submodule is imported at most once per process. + """ + module_name = _LAZY_EXPORTS.get(name) + if module_name is None: + raise AttributeError(f"module {__name__!r} has no attribute {name!r}") + value = getattr(importlib.import_module(module_name), name) + globals()[name] = value + return value + + +def __dir__() -> "list[str]": + return sorted(set(globals()) | set(__all__))