Skip to content

runtime: the handshake failure paths leak an outbound writer task (residual from C-01/C-02/C-04) #287

Closed

Description

@ViperJuice

Found by the adversarial seat during the pre-merge code review of #286, and reproduced by it — filed as a residual rather than fixed there because it is bounded and cannot redeliver, so it did not warrant holding that merge.

What

#286 added a bounded outbound writer task (_drain_outbound) and wired its teardown into two paths:

  • _cleanup_client — cancels outbound_writer and nulls both managed.outbound and managed.outbound_writer
  • disconnect_server — cancels the writer, then sweeps background tasks

There is a third teardown path that was not in that plan's acceptance list: handshake failure. The except-paths in _connect_stdio (around manager.py:2456) and _connect_remote_stream (around manager.py:2780) cancel only read_task and stderr_task. If a writer was started before the handshake failed, it survives that pop.

The reviewer reproduced a live _drain_outbound task after the except-path pop.

Severity — bounded, stated accurately

  • Retry-capped. MAX_CONNECTION_RETRIES = 3, and the reconnect loop makes 3 attempts, so this is at most a small number of leaked tasks per reconnect cycle rather than unbounded growth.
  • Cannot redeliver onto a new process. The leaked writer holds the discarded ManagedClient. All three constructors (_connect_stdio:2413, _connect_remote_stream:2731, adopt_process:3641) allocate a fresh object, so no production path reuses one.
  • Net effect is a slow task/queue leak against a server whose handshake repeatedly fails — not a protocol-correctness defect at the downstream boundary.

Suggested fix

Include outbound_writer in the handshake except-paths the same way #286 included it in _cleanup_client, and null outbound/outbound_writer there too so the postcondition holds on every teardown path rather than on two of three.

Prove it with a measured mutation, per the convention these runtime fixes have used: assert no live _drain_outbound task survives a failed handshake, then remove the fix and confirm the test is red for a named reason with the mutation confirmed applied via git diff --stat. Name the test so it is selected by the existing filter (it must contain one of c01_, c02_, c04_, stalled_sink, outbound_queue, outbound_writer) — a name matching no filter runs as part of nothing and still exits 0.

Two other non-blocking nits from the same review

  • disconnect_server cancels the writer but does not null managed.outbound. Harmless today because the object is discarded, but the same postcondition argument applies.
  • A frame whose "method" is present but not a string falls through to the pending-response lookup. It is not a valid JSON-RPC request, so this is a robustness nit at an untrusted boundary rather than a correctness bug.

See #232, #286.

Activity

  1. ViperJuice commented on Sep 30, 2026

    @ViperJuice
    CollaboratorAuthor

    Resolved by #321 (merged as 7208815). Both handshake except-paths and disconnect_server now tear down the outbound writer and null outbound/outbound_writer through one helper; tests test_outbound_writer_does_not_survive_failed_stdio_handshake / ..._remote_stream_handshake / test_outbound_writer_disconnect_server_resets_outbound_queue go red when the fix is reverted (mutation rows M1-M3 in the PR). Both nits are fixed too: a non-string method is dropped, and every malformed-frame shape (non-object, bad id types, settled futures, parser limits, and on SSE/streamable HTTP the ValidationError mcp puts on the stream) is dropped without ending the read loop. Remaining non-blocking item: #324.

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions