TCP Q3/Q4: an idle read deadline, and telling the server when a result is abandoned

[alex-clickhouse]

Stacked on #590 (tcp/epic-q1-exceptions). Review only the last commit.

Closes Q3 (timeout handling) and Q4 (cancellation → cancel and kill).

Q3: the deadline that nothing armed

ReadTimeout was parsed from the connection string, validated, stored on the options — and read by nothing. A connection dropped without a FIN mid-operation stalled until TCP gave up, about fifteen minutes on Linux, and the caller's own token was the only bound. IsReusable said as much in its remarks.

It is now armed immediately before each read from the transport and disarmed the moment that read returns, so it measures silence, not duration. Two properties fall out of that rather than needing to be arranged:

• a block that takes longer than the deadline to transfer never trips it, because bytes keep arriving;
• nothing is armed while the iterator sits at its yield, so a consumer that holds a block for ten minutes is not mistaken for a server that stopped answering.

ReadBuffer's two stream.ReadAsync calls are the only places a byte arrives, and readsFromTransport already distinguished them from the frame decoder's adapter buffer — so the compressed path is bounded by the transport buffer underneath it, and the decoder's own buffer correctly carries no deadline.

Failure is a TimeoutException naming ReadTimeout, following DialTimeout's precedent rather than adding a fourth leaf to Q1's deliberately closed hierarchy. The handshake stays under DialTimeout alone, so the two never stack on one exchange.

TimeSpan.Zero now disables it

Validate rejected Zero, so a caller reading a legitimately silent stream had no opt-out. Zero is the opt-out for IdleTimeout and MaxConnectionLifetime already; ReadTimeout now matches. Negatives and the ~24.8-day timer ceiling are still rejected.

Writes take the caller's token, reads take the deadline's

ReadTimeout bounds reads, so only reads observe it. Otherwise a deadline that fires as a read completes — CancelAfter(Infinite) cannot recall a timer callback already running — would surface on the next write as an OperationCanceledException for a token the caller never cancelled.

Q4: Cancel, then terminate

The packet is written and flushed from the same finally that already chose between Ready and Terminate, whenever the operation ended short of a packet boundary. One condition covers four cases: caller cancellation, a read that gave up on ReadTimeout, a broken transport, and a consumer that simply breaks out of the await foreach — which previously left the server producing a result nobody would read with nothing sent to say so.

writer.Reset() first, then a flush on its own two-second deadline: the operation's own token is usually the cancelled one, and flushing on it would send nothing. That flush runs before the pool lease goes back, which is the one thing to know about the constant — it bounds how long the next caller can wait for the slot, and only against a peer whose receive window is shut.

What the server does with it, measured rather than assumed

The first version of the integration test asserted QUERY_WAS_CANCELLED_BY_CLIENT (735) and failed with Broken pipe (210). The cause is real and worth writing down:

Result shape	How the query ends
slow, small	ExceptionWhileProcessing, code 735 — Received 'Cancel' packet from the client
saturating the socket	broken pipe (210)

The server reads client packets between the blocks it sends. A result large enough to fill the socket blocks it in a write() where it reads nothing, and the close that follows is what stops it. Both stop the query at once — so Cancel is what turns a silent abandonment into an explicit one, not the only thing that ends the query. The tests use the slow shape deliberately, and say so.

Not sent during the insert row phase

A block is part-written there, so an appended Cancel is read as more block bytes rather than as a packet. The flag is false for exactly that stretch and true either side of it (waiting for the schema block, and draining for end-of-stream).

Tests

3381 pass, 5 skipped (TLS, no local certificates), on net9.0 and net10.0 against a real 26.7.1.

The three tests that assert the idle semantics were mutation-checked, because that property is easy to assert vacuously:

• with Disarm() removed, QueryAsync_ConsumerHoldsABlockPastReadTimeout_... fails;
• with the deadline armed once per operation instead of per read, all three of ConsumerHoldsABlockPastReadTimeout, ResponseSlowerOverallThanReadTimeout and the integration QueryLongerThanReadTimeout fail.

Three earlier drafts of these passed while proving nothing, and are worth naming so they are not reintroduced: a SELECT count() FROM (SELECT sleepEachRow(...)) that the planner prunes to 4 ms; a scripted case whose 87-byte script was swallowed whole by the handshake's first 16 KiB fill, leaving the query phase reading from memory; and a third with a ~30 ms margin against its own deadline. All three now use maxChunk to force a read per packet, and the integration case selects the rows instead of counting them.

Also covered: caller cancellation while the deadline is armed reports cancellation and not a timeout; a compressed connection stalling inside a block still times out, through the decoder; the handshake is not bounded by ReadTimeout; and a second query on the same pooled connection rearms cleanly.

Coverage on the changed files: IdleReadDeadline.cs and ReadBuffer.cs 100%, ClickHouseTcpConnection.cs 95.8%, ClickHouseTcpClientOptions.cs 99.4%.

One existing test changed rather than added to

Validate_NonPositiveReadTimeout_ThrowsArgumentOutOfRangeException asserted that Zero throws, which is the behaviour this change reverses. It is split into Validate_NegativeReadTimeout_... and Validate_ZeroReadTimeout_IsAccepted.

Performance

The cost is two timer-queue updates per socket read — not per row, per scalar or per block. Measured over a 240 MB streaming read (30M rows, 9 rounds, deadline on vs off) the difference is not distinguishable from run-to-run variance, which is ±40% and larger than the effect in both directions. The arithmetic agrees: a few thousand reads at ~100 ns each is under 0.1% of an 0.9 s transfer. The ad-hoc probe is not committed.

A fast path that skips the timer when the read completes synchronously was considered and rejected for now: it moves stream.ReadAsync outside the try, which would silently drop transport-failure translation for a synchronous throw — a real fidelity loss to buy something unmeasurable.

Not in scope

• N11b (a server error during Query-packet read poisons the pooled connection). The fold-in assumed the client could detect the pre-acceptance case. It cannot: TCPHandler.cpp:993-1012 sends a clean exception, then discovers in skipData that the leftovers are a mid-packet tail, and closes well after the client has read the exception and judged the connection fine. With no client-visible signal this is a policy choice — Go-parity "terminate on any error", which kills a session on a typo, or a Ping probe on the error path — and it gets its own change.
• The abandoned-IAsyncEnumerator lease (PinnedConnectionSource.cs:104-110). Cancelling the token does not reach an enumerator parked at its yield, so nothing added here can free it; the only fix is a finalizer on the lease.
• A caller-supplied deadline, and max_execution_time derived from it — the surviving half of the dropped M12. A CancellationToken carries no deadline, so Go's "context deadline replaces ReadTimeout" rule has nothing to read; a per-operation deadline is new public surface and nothing here needs it. Today the two simply stack.

No changelog fragment, consistent with the rest of this stack.

[Copilot]

Pull request overview

Adds native TCP idle-read timeouts and best-effort server cancellation when responses are abandoned.

Changes:

• Enforces ReadTimeout per transport read, with zero disabling it.
• Sends Cancel before terminating incomplete queries/inserts.
• Adds timeout, cancellation, pooling, and idle-semantics coverage.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
ClickHouse.Driver.Tcp/Protocol/ReadBuffer.cs	Arms deadlines around transport reads.
ClickHouse.Driver.Tcp/Protocol/IdleReadDeadline.cs	Implements reusable idle deadlines.
ClickHouse.Driver.Tcp/Protocol/ClickHouseTcpConnection.cs	Integrates deadlines and cancellation packets.
ClickHouse.Driver.Tcp/Client/IConnectionFactory.cs	Passes configured read timeout.
ClickHouse.Driver.Tcp/Client/ClickHouseTcpConnectionStringBuilder.cs	Documents zero-timeout behavior.
ClickHouse.Driver.Tcp/Client/ClickHouseTcpClientOptions.cs	Documents and validates timeout semantics.
ClickHouse.Driver.Tcp.Tests/Utilities/ScriptedDuplexStream.cs	Simulates delayed transport reads.
ClickHouse.Driver.Tcp.Tests/Protocol/ClickHouseTcpConnectionQueryTests.cs	Tests query timeout and cancellation behavior.
ClickHouse.Driver.Tcp.Tests/Protocol/ClickHouseTcpConnectionInsertTests.cs	Tests insert cancellation boundaries.
ClickHouse.Driver.Tcp.Tests/Integration/ClickHouseTcpCancellationIntegrationTests.cs	Verifies server-side cancellation and pooling.
ClickHouse.Driver.Tcp.Tests/Client/ClickHouseTcpClientOptionsTests.cs	Covers updated timeout validation.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!