Skip to content

fix(transports): serialize transport discovery - #83794

Open
embwl0x wants to merge 1 commit into
NousResearch:mainfrom
embwl0x:agent/transport-discovery-lock
Open

embwl0x wants to merge 1 commit into
NousResearch:mainfrom
embwl0x:agent/transport-discovery-lock

Conversation

@embwl0x

@embwl0x embwl0x commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Summary

  • serialize first-time transport registration with double-checked locking
  • publish _discovered only after the complete import sweep, so concurrent lookups cannot observe partial registry state
  • leave discovery retryable when an unexpected transport import fails
  • add deterministic contention and failed-publication regression coverage

Root cause

_discover_transports() set _discovered = True before importing the transport modules. A concurrent caller could therefore skip the initial discovery path while the registry was only partially populated, then fall into the miss retry. The retry masked the race under normal startup but did not make publication atomic.

Testing

  • uv run pytest -q tests/agent/transports (242 passed)
  • concurrency regression file repeated 25 times
  • uv run ruff check agent/transports/__init__.py tests/agent/transports/test_transport_discovery_concurrency.py
  • uv run ruff format --check agent/transports/__init__.py tests/agent/transports/test_transport_discovery_concurrency.py
  • uv lock --check
  • git diff --check

Closes #24687.

Credit to @wesleysimplicio for reporting the race and for the earlier closed reference implementation in #24692; this replacement is rebuilt and revalidated on current main.

@embwl0x

embwl0x commented Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

CI classification: the sole Python failure is the known current-main baseline test test_profile_route_and_nonmultiplexed_resolution_preserve_boundaries. It rejects the unserved research profile and returns the default mode; #83745 is the exact focused correction and has a fully green required matrix. This branch only changes transport discovery and its tests, and all other 11 Python slices, the 242-test local transport suite, lints, OS-specific lanes, scans, attribution, and lock checks passed. I am keeping the unrelated gateway fixture correction out of this PR; rebasing after #83745 lands will absorb it.

@alt-glitch alt-glitch added type/bug Something isn't working comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P3 Low — cosmetic, nice to have labels Aug 11, 2026
@embwl0x
embwl0x force-pushed the agent/transport-discovery-lock branch from e50d9aa to 35d1512 Compare August 15, 2026 10:05
@embwl0x

embwl0x commented Aug 15, 2026

Copy link
Copy Markdown
Contributor Author

Current-main refresh complete at 35d15127e.

  • Rebased the transport discovery publication fix onto upstream main 7a16840ad, absorbing the unrelated gateway fixture repair behind the old red slice.
  • Revalidated the complete transport suite plus the formerly failing gateway boundary: 277 passed.
  • Repeated the deterministic discovery-contention regression 25 times without failure.
  • ruff, formatting on the regression file, git diff --check, public-identity/privacy checks, and gitleaks passed.
  • Replacement GitHub CI is fully green, including all 12 Python slices and both Docker architectures.

The later #83806 was already identified by upstream triage as a duplicate of this earlier PR. This branch is mergeable and ready for review.

@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

fix(transports): serialize transport discovery

Good fix. Moving _discovered = True to the end also removes the previous "poisoned-on-failure" behavior (the flag was set before the imports, so a failed sweep left the registry permanently marked discovered), and the new failure test documents the retry semantics. Observations:

  • register_transport (invoked at module import during the sweep) and get_transport mutate/read _REGISTRY without the lock. Safe under the GIL for the discovery path, but a runtime plugin registering a transport concurrently with a sweep has no ordering guarantee. Consider taking _discover_lock in register_transport or documenting that registration must not run concurrently with discovery.
  • The lock is held across the whole 4-module import sweep and threading.Lock.acquire is blocking. If discovery is ever triggered lazily from an asyncio context while another thread holds the lock, the event loop stalls. At startup this is likely fine; if a mid-session lazy trigger is possible, run_in_executor or a non-blocking acquire would be safer.
  • Minor: a transport raising a non-ImportError now propagates on every get_transport call (since _discovered stays False and nothing caches the failure). Consider caching the failure or logging at most once per attempt to avoid error spam.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P3 Low — cosmetic, nice to have type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

concurrency: agent/transports _discovered flag set before imports complete (TOCTOU)

3 participants