fix(http): dispose HTTP responses the client fully consumes

[polyglotAI-bot]

Description

Reported by Copilot on #490 (#490 (comment)) and confirmed pre-existing on main @ 6c40174, so it is fixed here rather than bundled into that compression PR.

ClickHouseClient obtained HttpResponseMessages that it never handed to a caller and never disposed. ExecuteNonQueryAsync read the 7-bit-encoded row count out of response.HttpResponseMessage.Content and returned; ExtendedBinaryReader does not propagate Dispose to its inner stream (it goes through PeekableStreamWrapper), so nothing released the response. Because the query path uses HttpCompletionOption.ResponseHeadersRead, the response and its pooled connection then stayed alive until GC finalization, which degrades HttpClient connection pooling under load. Auditing the rest of the class found the same defect on three more paths: the two internal binary-insert batch senders discarded the streamed response entirely, and both PostSqlQueryAsync and the streamed-insert PostStreamAsync threw (server error or transport failure) without releasing the response they had already received — on those paths ownership never reaches the caller, so nobody could dispose it.

Paths that transfer ownership are deliberately unchanged: ExecuteReaderAsync (released by ClickHouseDataReader.Dispose, and by FromHttpResponseAsync's own catch on failure), ExecuteRawResultAsync (ClickHouseRawResult.Dispose), the public InsertRawStreamAsync / PostStreamAsync overloads that return the HttpResponseMessage, and the ADO ClickHouseCommand paths, which already used using var response.

Changes

• ClickHouse.Driver/ClickHouseClient.cs
  • ExecuteNonQueryAsync: using var response = result.HttpResponseMessage — mirrors ClickHouseCommand.ExecuteNonQueryAsync. The reader is still disposed first, then the response.
  • PostSqlQueryAsync: dispose the response in the catch before rethrowing. HandleError materializes the error body into a string before throwing and ClickHouseServerException keeps no reference to the response, so nothing needs it afterwards.
  • PostStreamAsync(string, HttpContent, …): hoist response out of the try and dispose it in the catch, same rationale.
  • SendBatchAsync / SendPocoBatchAsync: using var response = await PostStreamAsync(...). Disposal runs before RethrowSerializationError, which never touches the response.
• CHANGELOG.md / RELEASENOTES.md: bug-fix entry.

Test

New ClickHouse.Driver.Tests/ResponseDisposalTests.cs, against a real server. A DelegatingHandler swaps each response's content for a StreamContent subclass that records its own Dispose, so "the response was disposed" (disposing a response disposes its content) becomes observable:

• ExecuteNonQueryAsync success and server-error paths dispose the response.
• InsertBinaryAsync (object[] and POCO overloads) disposes every response it issues.
• InsertRawStreamAsync against a missing table disposes the response on the failed streamed request.
• Contrast cases pinning prior behavior, so the fix can't over-reach: ExecuteReaderAsync and ExecuteRawResultAsync keep the response alive while the reader/raw result is open and release it only on its Dispose.

Verification: the 5 disposal tests fail on unpatched main (False / < False, True, False >) and pass with the fix; the 2 contrast tests pass both before and after. Full suite green on net10.0: 9592 passed, 0 failed, 142 skipped. No existing test was changed.

Pre-PR validation gate

• Deterministic repro confirmed (tests fail on main, pass on this branch)
• Root cause documented above
• Fix targets the root cause (release at the point ownership ends, on every such path)
• Test fails without fix, passes with fix
• No existing tests broken or weakened
• Convention compliance verified per AGENTS.md (integration-style tests, existing TestUtilities config, test naming, no new build warnings)
• CHANGELOG.md + RELEASENOTES.md updated
• No public API change (no PublicAPI/*.txt update needed)

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[Copilot]

Pull request overview

This PR fixes an HTTP resource-lifetime bug in ClickHouseClient where HttpResponseMessage instances (and thus pooled connections) could be left undisposed on code paths that fully consume responses or throw before handing ownership to a caller, degrading HttpClient connection pooling under load.

Changes:

• Dispose fully-consumed HttpResponseMessages in ExecuteNonQueryAsync and binary-insert batch senders.
• Ensure responses are disposed on exceptional paths in PostSqlQueryAsync and the internal streamed insert PostStreamAsync overload.
• Add integration-style tests that observe response disposal behavior; update CHANGELOG/RELEASENOTES with the bug fix.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
ClickHouse.Driver/ClickHouseClient.cs	Ensures HttpResponseMessage disposal on fully-consumed and exception-only ownership paths.
ClickHouse.Driver.Tests/ResponseDisposalTests.cs	Adds real-server tests to verify response disposal/ownership across key query/insert APIs.
CHANGELOG.md	Adds unreleased bug-fix entry describing the disposal fix.
RELEASENOTES.md	Adds unreleased release-note entry describing the disposal fix.

Suppressed comments (2)

ClickHouse.Driver.Tests/ResponseDisposalTests.cs:92

• These tests hand-roll table names (TestUtilities.TestDatabase + Guid). The established convention in this repo is to use TestUtilities.CreateTableName(...) so names are always unique, framework-attributable, and follow the suite’s cleanup/debugging expectations (see ClickHouse.Driver.Tests/AbstractConnectionTestFixture.cs:25-43).

            var targetTable = $"{TestUtilities.TestDatabase}.response_disposal_poco_{Guid.NewGuid():N}";

ClickHouse.Driver.Tests/ResponseDisposalTests.cs:117

• This test relies on a hard-coded missing table name. If any concurrently running suite (net6/net8/net9/net10) or other test happens to create that table, this can become flaky. Prefer a unique name via TestUtilities.CreateTableName(...).

            Assert.ThrowsAsync<ClickHouseServerException>(
                () => client.InsertRawStreamAsync("no_such_table_for_response_disposal_test", payload, "TSV"));

[alex-clickhouse]

1. We already have a trackinghandler in the test project, can we reuse it?
2. Can we derive from AbstractConnectionTestFixture, use its client (separate HttpClient) for setup and auto-drop, keep the tracking client for the operation under test, and assert an exact count?
3. More concise changelog please.

[polyglotAI-bot]

Thanks @alex-clickhouse — all three done in 5395d8a.

1. Reuse the existing tracking handler. Utilities/TrackingHandler tracks requests, not response disposal, so instead of a second hand-rolled DelegatingHandler the new Utilities/ResponseDisposalTrackingHandler now derives from it — it inherits the request tracking and inner-handler forwarding and only adds the bit that wasn't there: swapping each response's content for a StreamContent subclass that records its own Dispose (disposing a response disposes its content). TrackingHandler itself is untouched, so no existing test is affected, and the new capability is available in Utilities/ for future tests.

2. Derive from AbstractConnectionTestFixture + exact counts. ResponseDisposalTests now derives from the fixture, following the ClickHouseClientQueryOptionsTests pattern: the fixture client (its own HttpClient) does the CREATE TABLE setup and the table name comes from the fixture's CreateTableName(), so cleanup is the fixture's auto-drop and the explicit try/finally DROPs are gone. The tracking client now issues only the requests of the operation under test, which makes exact counts meaningful — every test asserts Has.Count.EqualTo(n) (1 for the query/error/reader/raw-result paths, 2 for the binary inserts: schema fetch + batch) plus the disposal state, instead of Single()/Is.Not.Empty.

3. Concise changelog — the entry is now two sentences (what leaked and where, and which paths intentionally still transfer ownership).

Verified in the devbox (net10.0): the 7 tests pass with the fix and the same 5 still fail with the production change reverted (the 2 ownership-transfer contrast tests pass either way); full suite 9605 passed / 0 failed.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (1)

ClickHouse.Driver.Tests/Utilities/ResponseDisposalTrackingHandler.cs:49

• In the tracking handler, response.Content is replaced with a new StreamContent, which means the original HttpContent instance is abandoned and never disposed. Even though the underlying stream will be disposed, leaving the original HttpContent undisposed can still leak resources and also changes disposal semantics compared to a normal HttpResponseMessage.Dispose() (which disposes its original content). Consider retaining and disposing the original content as part of the tracking content's Dispose().

        var response = await base.SendAsync(request, cancellationToken).ConfigureAwait(false);

        var trackingContent = new DisposalTrackingContent(
            await response.Content.ReadAsStreamAsync(cancellationToken).ConfigureAwait(false));
        foreach (var header in response.Content.Headers)