Read upgraded streams through the buffered reader

[fruch]

Read upgraded streams through the buffered reader

Fixes #3332
Fixes #2042

The bug

attach and exec start are HTTP upgrades: the daemon answers with
101 UPGRADED plus headers, and then writes the stream frames on the same
connection. http.client parses those headers through
sock.makefile("rb") — an io.BufferedReader — and readline() fills that
buffer up to io.DEFAULT_BUFFER_SIZE at a time. So whenever the headers and
the first frames arrive close enough together to be returned by one recv(),
the frames end up in the reader's buffer.

APIClient._read_from_socket() then reads from the raw socket, which has
nothing left on it. The frames are simply gone. From the reporter's strace
in #3332:

# broken - the header read swallowed the frame
recvfrom(9, "HTTP/1.1 101 UPGRADED\r\n...Ostype: linux\r\n", 8192, ...) = 177
recvfrom(9, "Server: Docker/24.0.5 (linux)\r\n\r\n\1\0\0\0\0\0\0\6hello\n", 8192, ...) = 47

# working - the frame arrived after the headers were parsed
recvfrom(10, "HTTP/1.1 101 UPGRADED\r\n...Server: Docker/24.0.5 (linux)\r\n\r\n", 8192, ...) = 210
recvfrom(10, "\1\0\0\0\0\0\0\6", 8, ...) = 8
recvfrom(10, "hello\n", 6, ...) = 6

The symptom is exec_run() returning b'' with exit code 0, or output
that starts at the second frame. It is timing dependent, so it shows up on
whatever widens the window — an SSH-forwarded socket, a loaded host, a remote
daemon — and disappears if you add a sleep to the command. #2042 is the
same bug reported in 2018.

The behaviour dates to 76ed9c3 (2016), when the API moved to HTTP upgrade
and the client started reading the raw socket. It has been wrong for every
transport since.

Reproducing it deterministically only needs the headers and the first frame in
one write; the tests below do that.

The fix

_read_from_socket() hands frames_iter() the buffered reader instead of the
socket when there is one. That is the whole of the change in
docker/api/client.py, and it covers the unix, tcp, https and npipe
transports plus ssh with use_ssh_client=True.

Three things follow from that, and all of them are part of the fix rather
than tidying:

1. Do not poll a buffered reader. read() waits on the descriptor before
every read. Bytes sitting in the reader's buffer are invisible to that poll,
so the wait would block until more data arrived — turning the lost output
into a hang. Verified:

>>> b.sendall(b"HTTP/1.1 101 UPGRADED\r\n\r\n\x01\x00\x00\x00\x00\x00\x00\x06hello\n")
>>> r.readline(); r.readline()                  # http.client parses the headers
b'HTTP/1.1 101 UPGRADED\r\n'
b'\r\n'
>>> select.select([a], [], [], 0)[0]            # nothing to poll for
[]
>>> r.read1(4096)                               # ...but the frame is right here
b'\x01\x00\x00\x00\x00\x00\x00\x06hello\n'

2. Use read1(n), not read(n). BufferedReader.read(n) blocks until it
has n bytes or hits EOF; read1(n) returns what is buffered and otherwise
performs a single read — the contract read() already has with recv(n).
With read(n), a stream that stays open delivers nothing until 4096 bytes
pile up. Against a server that writes one frame and keeps the connection
open:

read(4096):  first chunk within 3s: NOTHING - stream stalled
read1(4096): first chunk within 3s: [b'hello\n']

That is #3333's remaining problem, and it is why this is a separate patch
rather than a review comment. It affects attach(stream=True) and
exec_run(stream=True, tty=True). The existing integration tests do not catch
it, because cancelling the stream shuts the socket down and the buffered bytes
are flushed at EOF; test_stream_tty below does catch it.

3. Disable the socket timeout. With the poll gone, the blocking read is
the socket's own, so timeout now bounds it — a quiet exec longer than
timeout seconds would raise TimeoutError where it used to wait
indefinitely in poll(). _read_from_socket() is the only streaming helper
that did not call _disable_socket_timeout(); it does now, matching
_stream_raw_result() and _multiplexed_response_stream_helper(). Verified
with a 2 s client timeout against a stream that stays quiet for 4 s:
TimeoutError: timed out without the call, b'hello\n' with it.
test_stream_quiet covers this.

_is_pipe_ended() keeps the npipe PIPE_ENDED-means-EOF handling working now
that the reader, not the NpipeSocket, is what read() sees.

Tests

TCPSocketStreamUpgradeTest in tests/unit/api_test.py. The existing
TCPSocketStreamTest sleeps 0.2 s between the headers and the payload, which
is exactly the case that works — the new class writes them with a single
wfile.write() instead, so http.client reads them into one buffer.

All six pass with the patch. Five fail on main, and the sixth guards the
regression that skipping the poll would otherwise introduce:

test	on main	with #3333
test_no_stream_tty	assert b'' == b'hello\noh no\n'	pass
test_no_stream_no_tty	assert b'' == b'hello\noh no\n'	pass
test_no_stream_no_tty_demux	assert (None, None) == (b'hello\n', b'oh no\n')	pass
test_stream_no_tty	assert b'oh no\n' == b'hello\n'	pass
test_stream_tty	assert b'oh no\n' == b'hello\n'	assert b'hello\noh no\n' == b'hello\n'
test_stream_quiet	pass	TimeoutError: timed out

Each test runs on a thread with a deadline, because on unpatched code the read
blocks in poll() forever rather than failing.

Not covered

Two cases share the root cause and are deliberately left alone to keep this
reviewable:

• ssh with use_ssh_client=False (the default DockerClient path).
  paramiko's ChannelFile buffers the same way, but it is not an
  io.BufferedReader and exposes no supported way to read its buffer without
  blocking, or a read1(). Its read(size) blocks until size bytes, like
  BufferedReader.read.
• attach_socket() and exec_start(socket=True), which hand
  _get_raw_response_socket() to the caller. Callers write to that socket
  (interactive exec), so a read-only buffered reader cannot be substituted; it
  would need a small duplex wrapper.

Relationship to #3333

@antontornqvist found the root cause and wrote #3333, open and unreviewed
since May 2025. The diagnosis there is right, and this patch keeps its shape.
It adds the three things above: read1() instead of read(), the socket
timeout, npipe PIPE_ENDED, and tests that reproduce the issue. Happy for
this to land as a review on #3333 instead if that is easier.

Verification

• tests/unit: 617 passed. Three failures on this machine
  (test_set_auth_headers_with_dict_and_no_auth_configs reads the local
  ~/.docker/config.json, two datetime tests are timezone dependent) fail
  identically on main.
• tests/integration/api_exec_test.py: 21 passed against Docker 29.8.0.
• tests/integration/api_container_test.py,
  tests/integration/models_containers_test.py: 119 passed, including
  test_attach_stream_and_cancel and test_logs_streaming_and_follow_and_cancel.
  Three failures (legacy container links and MacAddress, both removed in
  Docker 29; a missing websocket-client) fail identically on main.
• ruff==0.1.8 docker tests: clean.