Skip to content

Handle control-channel write failures without corrupting Query state - #1229

Open
venkat-uk wants to merge 1 commit into
anthropics:mainfrom
venkat-uk:fix/control-channel-write-failures
Open

Handle control-channel write failures without corrupting Query state#1229
venkat-uk wants to merge 1 commit into
anthropics:mainfrom
venkat-uk:fix/control-channel-write-failures

Conversation

@venkat-uk

Copy link
Copy Markdown

What changed

Query writes to the CLI on both directions of the control channel. Neither
write was guarded, so a write that fails leaves the object in a bad state.

Two changes in src/claude_agent_sdk/_internal/query.py:

  1. _handle_control_request now sends both its success and its error response
    through a new _send_control_response(), which logs a failed write at debug
    and returns instead of letting it escape.
  2. _send_control_request removes the request's entry from
    pending_control_responses when the outgoing write fails, so no waiter is
    left behind for a request the CLI never received.

Why

The detached task. _handle_control_request is spawned detached
(_spawn_control_request_handler) and answers every hook, permission and
SDK-MCP request. If the CLI has exited, or stdin has been closed, while one of
those was in flight, transport.write() raises CLIConnectionError and the
exception leaves the task. There is no caller to receive it, so it surfaces as
an unhandled task error: on trio, _task_compat._mark_done logs
Unhandled exception in detached trio task with a full traceback; on asyncio it
becomes Task exception was never retrieved at collection. Nothing is actually
broken, but the logs read as an SDK crash during an ordinary shutdown race, and
_task_compat treats an exception escaping a detached task as a bug signal by
design.

The error write is worse because it sits inside the except clause, so a
failure there also replaces the original error with the write error.

Routing both writes through one helper has a second effect. Before, a failed
success write fell into the except Exception clause and the SDK then tried
to send the CLI an error response for a request the handler had in fact
completed. Now a success write that fails is just a lost reply.

The pending entry. _send_control_request registers the waiter, then
writes:

event = anyio.Event()
self.pending_control_responses[request_id] = event
...
await self.transport.write(json.dumps(control_request) + "\n")
try:
    with anyio.fail_after(timeout):
        await event.wait()
    ...
except TimeoutError as e:
    self.pending_control_responses.pop(request_id, None)
    self.pending_control_results.pop(request_id, None)
    ...

The write is outside the try, so only the timeout path cleans up. When the
write itself fails the caller gets the error but the entry stays for the life of
the Query. ClaudeSDKClient is long lived, so an interrupt() or
set_permission_mode() that keeps failing against a dead subprocess grows both
dicts, and the read loop's failure path later writes a result into
pending_control_results for every one of those ghost ids.

How tested

Four tests added in tests/test_query.py
(TestControlChannelWriteFailures), using an AsyncMock transport whose
write() raises the same CLIConnectionError a terminated subprocess produces.
Three of the four fail on bc0c9af without the source change:

python -m pytest tests/test_query.py -q -k ControlChannelWriteFailures
# on bc0c9af, source unchanged:
#   3 failed, 1 passed, 59 deselected
# with the change:
#   4 passed, 59 deselected

The fourth (test_cancelled_request_still_skips_the_response) passes either way
and is a regression guard: except anyio.get_cancelled_exc_class(): raise must
keep coming before the new except Exception, so a cancelled request still
writes nothing.

Full suite and the repo's own gates, on Python 3.12 with mcp 2.x:

python -m pytest tests/          # 1489 passed, 5 skipped  (1485 + 5 before)
python -m ruff check src/ tests/ scripts/       # All checks passed
python -m ruff format --check src/ tests/ scripts/   # 68 files already formatted
python -m mypy src/ scripts/     # no issues found in 33 source files

Also checked by hand on both backends that the unhandled-task log is gone: a
Query with a failing transport, given one hook request that succeeds and one
with an unknown callback id, produces the trio warning and the asyncio traceback
on bc0c9af and neither after the change.

Tradeoffs

  • A control response that cannot be delivered is now only visible at DEBUG.
    That is a real loss of signal, but the transport failure itself still reaches
    the consumer through the read loop, which is where it is actionable. Raising
    from a detached task was never a way to tell anyone.
  • Dropping the pending entry on a failed write means the read loop's bulk-fail
    loop no longer resolves that id. Nothing waits on it, so there is nothing to
    resolve.
  • _send_control_request catches BaseException around the write so a
    cancellation cleans up too, then re-raises. If a caller is cancelled exactly
    between the write landing and the cleanup, the entry goes and a response that
    does arrive is dropped by the read loop's if request_id in ... guard. That is
    the same outcome as the existing timeout path.
  • _send_control_response catches Exception, not BaseException, so a
    cancellation during the write still propagates and is handled by the existing
    cancelled-request branch.

Query writes to the CLI on both directions of the control channel, and
neither write is guarded.

_handle_control_request runs in a detached task and answers every hook,
permission and SDK-MCP request. If the CLI has exited, or stdin has been
closed, while one of those was in flight, the response write raises and
the exception leaves the task: trio logs "Unhandled exception in detached
trio task" with a traceback, asyncio logs "Task exception was never
retrieved". Nothing is actually broken, but the logs read as an SDK crash
during an ordinary shutdown race. Routing both writes through a helper
also stops a failed success write from falling into the except clause,
where it was answered with an error response for a request the handler
had in fact completed.

_send_control_request registers the waiter before writing, and the write
sits outside the try that cleans up on timeout. A write that fails leaves
the entry in pending_control_responses for the life of the Query, so a
long-lived client whose interrupt() keeps failing grows both dicts.
@venkat-uk
venkat-uk force-pushed the fix/control-channel-write-failures branch from 7d40428 to f39bcb3 Compare August 22, 2026 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant