fix(query): drain the result socket while rendering - #302
Conversation
0.34.0 renders each batch on the thread that reads the socket, so the connection sits idle for the whole of every render. Locally that is invisible — rendering keeps up at 0.3us/row — but over a WAN link it is a slow consumer: throughput collapses against the receive window, and a connection held open for minutes is one an intermediary drops. Measured against a deployed server on a 20M-row result: 301s and `error decoding response body` after 1,966,081 of 20,000,001 rows, where the buffering client 0.34.0 replaced completes the same query in 41s. A reader on the runtime now drains the socket into a four-batch bounded queue and the renderer pulls from it, so reading never waits on rendering. Peak memory stays bounded by the queue rather than the result, which is the property the streaming change exists for. Same query after the change: 17.2s, 26MB, all 20,000,001 rows — 2.4x faster than the buffering client at 1/65th its memory.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the full diff plus surrounding context in src/client/sdk.rs and src/commands/query.rs. The decoupling is correct: rt() is a multi-thread runtime, so the spawned reader makes progress while the renderer blocks on blocking_recv, and the renderer runs on a non-runtime thread as the module invariant requires.
Moving total_row_count() before the drain is safe. fetch_capped already reads that value pre-drain at line 833 and documents it as the server-reported header, so the short-body check keeps its meaning.
Three non-blocking comments left inline.
CI note: CI / test, Integration Tests / integration and the Aikido check were still pending when the review started, so this approval does not rest on test results.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
The reader task signalled the end of the stream by dropping its sender, which is also what a panic looks like: `spawn` stores the panic in a JoinHandle this code drops, so a reader that dies inside arrow decoding is invisible except for its silence. The renderer read that silence as the end of the result, and with no X-Total-Row-Count the row-count guard never fired — partial rows on stdout and exit 0. The stream now ends with an explicit `BatchMessage::Done`. A queue that closes without one is reported as an incomplete read. Covers the mid-stream failure path with a test that serves a body cut inside a batch and no row-count header, so nothing but the stream error can reveal the truncation.
There was a problem hiding this comment.
All four prior threads are resolved in the code: the explicit BatchMessage::Done marker, the reader-stopped error in next_queued, the mid-stream decode test, and the doc wording.
Two disclosures about the scope of this approval. CI / test had not reported when this review ran, so the new assertions are unverified here. The hotdata SDK is an external crate and is not vendored in the checkout, so the arrow/ipc substring assertion at src/commands/query.rs:2390 depends on SDK error text that cannot be read from this checkout. A patch bump of hotdata 0.18.x that rewords that error would break the assertion.
1 participant
Warning
0.34.0 is released with this bug. A large result over a real network is slower than the buffering client it replaced, and can fail outright. This needs a 0.34.1.
The bug
0.34.0 renders each batch on the same thread that reads the socket, so the connection is idle for the whole of every render.
Locally that is invisible. A size sweep against a local server shows the render is not the bottleneck and scales fine:
Over a WAN link it is a slow consumer. Throughput collapses against the receive window, and a connection held open for minutes is one an intermediary is entitled to drop. Measured against a deployed server on a 20 M-row result:
error: the result above is incomplete — the download failed partway/transport error: error decoding response body, after exactly 240 batches of 8192 rows.Note what this means: on a large result 0.34.0 is worse than the client it replaced. The buffering client looks naive by comparison —
.bytes()drains greedily — but that is exactly why it gets full bandwidth.The fix
Reading and rendering are decoupled.
Api::stream_result_batchesspawns a reader on the shared runtime that drains the socket into a four-batch bounded queue; the renderer pulls from the queue withblocking_recv. Reading never waits on rendering, and peak memory stays bounded by the queue rather than the result — the property the streaming change exists for.The channel is
tokio::sync::mpsc, notstd::sync::mpsc: the reader is async and its send has to yield when the queue is full rather than block a runtime worker.Measured after
Same query, same deployed server:
2.4x faster than the buffering client at 1/65th its memory, and the rows are an identical multiset to the 0.33.0 output.
Why it shipped
The streaming work was measured at 4 M rows against a local server. The size in the original report was 20 M, which is the one size never run — and the failure only appears over a real network, so a local test could not have found it whatever the size. It was caught by deploying the server PR image to a scratch workspace and driving released binaries at it.
Tests
525 passing, clippy clean. The renderers now take the queue rather than the stream, so the existing byte-equality tests (
streamed_csv_matches_the_buffered_render,streamed_json_matches_the_buffered_envelope, the zero-row case) exercise the new path unchanged — they assert output is identical to the buffered renderer, which is what must not move.No unit test covers the slow-consumer behaviour itself: it needs a real network with a real receive window, and a mock server on loopback cannot reproduce it. That gap is why the workspace run is the evidence here.