Severity: Low · Area: Tests / quality · Category: async-misuse
Location: services/agent/tests/test_companion_upload.py:65
What's wrong
Each happy-path test calls register_pending_upload() on its own asyncio.run loop, then drives TestClient in an executor thread; the upload endpoint (companion.py:274) calls fut.set_result(payload) on the TestClient portal's separate loop thread. Future.set_result from a foreign thread invokes non-thread-safe loop.call_soon without waking the awaiting loop's selector. The tests only pass because loop.run_in_executor completion (which does use call_soon_threadsafe) happens to wake the outer loop afterwards and drain the ready queue — the await asyncio.wait_for(fut, timeout=5) would otherwise block until its 5s timer. This is broken async usage that works by accident and would flag under PYTHONASYNCIODEBUG; production is unaffected (single loop).
How it fails
A refactor that awaits the future before/independently of the executor task (or asyncio internals asserting thread affinity, as debug mode already does) turns each of these tests into a 5-second hang followed by TimeoutError, with no product bug present.
Suggested fix
Run register+await inside the TestClient's portal (e.g. starlette TestClient portal or anyio.from_thread), or replace the pattern with a small async test app driven by httpx.AsyncClient/ASGITransport on one loop.
Adversarial verification — both skeptics confirmed
Skeptic 1 (confirmed) — Confirmed, not refuted. The tests create the future on the asyncio.run loop (main thread) while starlette 0.39's TestClient runs the upload endpoint on an anyio blocking-portal loop in a separate thread, where companion.py:274 calls plain fut.set_result(). Empirically verified the mechanism: cross-thread set_result uses non-threadsafe ca…
Skeptic 2 (confirmed) — Confirmed empirically. The three happy-path tests bind the pending-upload future to the asyncio.run loop (companion.py:61 uses get_event_loop().create_future()), then TestClient — driven from an executor thread — resolves it via bare fut.set_result(payload) at companion.py:274 on starlette's separate blocking-portal loop thread. Reproduc…
Filed from a multi-agent audit of 855f5cc: 16 reviewers over ~42k lines produced 170 raw findings; each was handed to 2 independent agents prompted to refute it, and only findings both confirmed were kept (19 refuted, 10 split-verdict, 127 unique confirmed). Line numbers are 1-indexed against 855f5cc and will drift as the code changes.
Severity: Low · Area: Tests / quality · Category:
async-misuseLocation:
services/agent/tests/test_companion_upload.py:65What's wrong
Each happy-path test calls register_pending_upload() on its own
asyncio.runloop, then drives TestClient in an executor thread; the upload endpoint (companion.py:274) callsfut.set_result(payload)on the TestClient portal's separate loop thread. Future.set_result from a foreign thread invokes non-thread-safeloop.call_soonwithout waking the awaiting loop's selector. The tests only pass becauseloop.run_in_executorcompletion (which does use call_soon_threadsafe) happens to wake the outer loop afterwards and drain the ready queue — theawait asyncio.wait_for(fut, timeout=5)would otherwise block until its 5s timer. This is broken async usage that works by accident and would flag under PYTHONASYNCIODEBUG; production is unaffected (single loop).How it fails
A refactor that awaits the future before/independently of the executor task (or asyncio internals asserting thread affinity, as debug mode already does) turns each of these tests into a 5-second hang followed by TimeoutError, with no product bug present.
Suggested fix
Run register+await inside the TestClient's portal (e.g. starlette TestClient portal or anyio.from_thread), or replace the pattern with a small async test app driven by httpx.AsyncClient/ASGITransport on one loop.
Adversarial verification — both skeptics confirmed
Filed from a multi-agent audit of
855f5cc: 16 reviewers over ~42k lines produced 170 raw findings; each was handed to 2 independent agents prompted to refute it, and only findings both confirmed were kept (19 refuted, 10 split-verdict, 127 unique confirmed). Line numbers are 1-indexed against855f5ccand will drift as the code changes.