fix(auxiliary): never release Codex client FDs from the timeout Timer

[necoweb3]

Summary

_CodexCompletionsAdapter.create arms a daemon threading.Timer that calls client.close() when the auxiliary Responses stream exceeds its timeout. On a stalled stream — the exact failure the timeout exists for — the Timer is the only thing that fires, so close() runs on a thread that does not own the in-flight httpx connection.

That is the file-descriptor ownership violation this repo has already fixed twice on the main transport (#29507, #67142, #70773). agent/auxiliary_client.py never received the same treatment: it contains no thread-ownership machinery at all.

Problem

The rule is stated in the repo's own guard, agent/agent_runtime_helpers.py:3505 force_close_tcp_sockets:

shutdown() from any thread is FD-safe; close() is not.

and its docstring spells out the consequence: the ssl.SSLSocket's OpenSSL BIO caches the raw integer fd, so once os.close(fd) runs the kernel may immediately recycle that integer to the next open() — "e.g. the kanban dispatcher opening kanban.db" — and the owning thread's unwinding TLS flush writes the encrypted bytes into the wrong file (#29507: a 24-byte TLS application-data record clobbering SQLite header bytes 5..28).

The guarded twins enforce exactly this, and their comments name the hazard:

• run_agent.py:4393 _retire_shared_openai_client — "Only an owner may release FDs … So nobody calls close(): we shutdown() the pooled sockets … and defer the actual FD release to garbage collection."
• run_agent.py:4553 _abort_request_openai_client — "Calling client.close() from a thread that does not own the active httpx connection raced the still-live SSL BIO and corrupted unrelated file descriptors when the kernel recycled the just-freed TCP FD (Interrupted OpenAI/httpx request thread survives across turns and writes TLS record bytes to unrelated file descriptors on delayed close #29507)."
• Dispatched at agent/chat_completion_helpers.py:2594-2620 via stranger_thread = ... owner_tid != threading.get_ident().

The unguarded path. agent/auxiliary_client.py:1150, inside _close_client_on_timeout:

close = getattr(self._client, "close", None)
if callable(close):
    close()          # <- runs on the threading.Timer thread

grep -n "get_ident|force_close_tcp_sockets|SHUT_RDWR|shutdown(" agent/auxiliary_client.py returns zero hits. The guarded twins live in run_agent.py and are unreachable here — _CodexCompletionsAdapter holds no AIAgent reference.

The same callback is invoked from two threads with no dispatch between them: _check_cancelled() runs it on the owning thread (safe), and the threading.Timer armed at line 1186 runs it on a stranger thread (unsafe). Once the Timer sets timed_out, line 1166's if not timed_out.is_set() means the stranger-thread close is the only close.

Why the Timer wins. _check_cancelled is reachable only from _on_each_event, i.e. only when an SSE event actually arrives. The Timer fires at the deadline unconditionally. On a stalled body no event ever arrives, so the owner-thread path cannot run at all.

Trigger. Main or auxiliary provider on a Responses-only backend (openai-codex ChatGPT OAuth, or xai-oauth) — _resolve_auto Step 1 routes aux tasks to the user's main provider, so this needs no extra config. Then any ordinary long session where compression, flush_memories, MoA aggregation or title generation exceeds its timeout while still streaming. _effective_aux_timeout always returns a positive value (_DEFAULT_AUX_TIMEOUT = 30.0; compression floored at 300s), so the Timer is always armed.

Blast radius. The leaf client is process-global (_client_cache, keyed per provider/model) and shared across compression, memory flush, MoA, title generation, session_search and web_tools — and AsyncCodexAuxiliaryClient runs the same sync adapter on asyncio.to_thread workers, so the Timer's close() can also kill a connection owned by an unrelated worker thread.

Reproduced

Both legs of the causal chain, in-process, driving the real _CodexCompletionsAdapter:

=== leg 1: which thread calls close() ? ===
owner (calling) thread : 4344
close() ran on thread  : 14440
STRANGER-THREAD CLOSE  : True

=== leg 2: is that fd recycled into the database ? ===
fd released by Timer   : 3
fd handed to open(db)  : 3
SAME FD NUMBER         : True
state.db header now    : b'\x17\x03\x03\x00\x13^W^Q]H-\xca\xff\xb1\xc3'
still 'SQLite format 3': False

The 24-byte TLS application-data record lands on the SQLite header — the documented #29507 signature. Corruption there sends SessionDB down the quarantine path, which opens a fresh empty database: every session in the old file is gone from the running product. There is no spill file, no active=0 twin (the damaged bytes are the storage), no content-bearing log, and create_quick_snapshot is not continuous — it runs on hermes update and deliberately skips a large state.db.

Fix

Dispatch on ownership, mirroring agent/chat_completion_helpers.py:

• Capture owner_tid = threading.get_ident() at the top of create().
• In _close_client_on_timeout, the owning thread keeps closing directly; a stranger thread only calls force_close_tcp_sockets() (shutdown(SHUT_RDWR)), which unblocks the owner's pending recv without releasing any fd.
• The owning thread then performs the real close() in the finally once it has unwound — where the FD release belongs.

Cache eviction on timeout (#23432) is unchanged.

Scope

• agent/auxiliary_client.py — ownership dispatch in _close_client_on_timeout; owner-side close in the existing finally. No change to timeout values, retry behaviour, cache eviction, or the Responses streaming logic.
• Not addressed here: fix(auxiliary): isolate title-generation client lifecycle #46390 (open) touches auxiliary client lifecycle but explicitly lists "no timeout timer / ownership redesign" as a non-goal, so the two do not overlap. fix(auxiliary): evict Codex auxiliary client on timeout #25064 previously edited this same callback for cache-eviction reasons and reasoned only that "close() is safe to call twice" — it did not consider FD ownership.

Testing

New TestCodexAuxiliaryTimeoutFdOwnership in tests/agent/test_auxiliary_client.py, built on the nested _client._transport._pool._connections shape that force_close_tcp_sockets traverses, recording (action, thread) for every socket operation:

• test_stalled_stream_timeout_does_not_release_fds_from_timer_thread — on a stalled stream the stranger thread performs shutdown and never client.close / sock.close.
• test_owner_thread_releases_fds_after_a_stranger_thread_timeout — the deferred close still happens, on the owner, and strictly after the shutdown.

Both fail on main and pass here:

2 failed, 2 passed    <- on main (the 2 passing are the pre-existing timeout tests)
4 passed              <- with this change

python -m pytest tests/agent/test_auxiliary_client.py -o addopts= -q
364 passed

python -m pytest tests/run_agent/test_70773_shared_client_fd_corruption.py \
  tests/run_agent/test_tls_fd_recycle_corruption.py \
  tests/agent/test_auxiliary_compression_timeout_floor.py \
  tests/agent/test_auxiliary_transient_retry.py -o addopts= -q
27 passed

The two pre-existing tests in TestCodexAuxiliaryAdapterTimeout pass unmodified, so timeout forwarding and timing behaviour are unchanged.

[teknium1]

Thanks for tracing the Timer-thread close to the existing FD-recycling guard.

Problems

• The proposed owner-side close is still unsafe for this client lifecycle. agent/auxiliary_client.py:6681-6700 hands the same cached client to concurrent callers with no borrower tracking. After a timeout, the added client.close() in the timed-out request's finally can release pooled FDs owned by another active worker. The main shared-client path deliberately avoids that: run_agent.py:4546-4565 uses shutdown-only retirement and defers release until borrowers unwind.

Suggested changes

• Make timeout cleanup shared-client-safe: avoid closing the cached leaf from one request's finally; use request-local ownership or a lease/refcount lifecycle, and add a two-borrower regression test.

Automated hermes-sweeper review.

[teknium1]

self._client is cache-shared (agent/auxiliary_client.py:6681-6700), so this request's owner thread may not own another worker's active pooled connection. Closing here can still release that other worker's FD; use a shared-client lease/lifecycle or defer release after all borrowers unwind.

[teknium1]

Salvaged and merged in PR #99709 (merge SHA 8a766c3) with your commit authorship preserved — thank you for the thorough FD-ownership analysis, it was spot on. Your fix was reapplied onto the re-armable no-progress watchdog that landed today in #99660, plus one addition your original predates: the stranger-thread path also closes the attempt-owned event stream so an owner blocked inside the SDK stream (no reachable raw socket) still unwinds promptly.