ClickHouse / ClickHouse/clickhouse-cs

ExecuteNonQueryAsync silently treats HTTP 200 with mid-stream exception as success

Open
#333 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
C#
Stars
94
Forks
22
Avg merge
11h 26m
Merged PRs (30d)
22

Description

## Description

When the ClickHouse server returns `HTTP 200 OK` while reporting an error (mid-stream exception, distributed-DDL error blocks, etc.), `ClickHouseClient.ExecuteNonQueryAsync` silently treats the response as success. Two factors combine to produce this:

1. `ClickHouseClient.HandleError` (`ClickHouse.Driver/ClickHouseClient.cs:889-901`) decides success/failure purely from `response.IsSuccessStatusCode`. It never inspects `X-ClickHouse-Exception-Code` (or `X-ClickHouse-Exception-Tag`) on the response. Any 2xx response — even one carrying an error indicator in the headers — is treated as a success and returned to the caller.

2. `ExecuteNonQueryAsync` (`ClickHouse.Driver/ClickHouseClient.cs:208-218`) does not run the body through the existing mid-stream exception detection. It opens the raw response stream and reads a single 7-bit-encoded int as the affected-rows count:

```csharp
var response = await PostSqlQueryAsync(sql, parameters, options, cancellationToken).ConfigureAwait(false);
using var reader = new ExtendedBinaryReader(await response.HttpResponseMessage.Content.ReadAsStreamAsync(cancellationToken).ConfigureAwait(false));

return reader.PeekChar() != -1 ? reader.Read7BitEncodedInt() : 0;
```

The `ExceptionTagAwareStream` wrapper that detects `__exception__` markers when `X-ClickHouse-Exception-Tag` is present (added in #168) is only wired up inside `ClickHouseDataReader.FromHttpResponseAsync`. `ExecuteNonQueryAsync` bypasses it entirely.

Net effect: a DDL/INSERT that the server reports as failed via `HTTP 200` + exception headers + body-encoded error is observable by the application as success. This risks silent schema-change failure or silent data loss.

The `ClickHouseDataReader` path is partially protected because it wraps the stream in `ExceptionTagAwareStream` when `X-ClickHouse-Exception-Tag` is present, but it still does not consult `X-ClickHouse-Exception-Code` and will not detect cases where the server has encoded the error inside a valid Native/RowBinary block (no `__exception__` marker), e.g. the distributed-DDL scenario from clickhouse-go#1398.

## ClickHouse server version

`26.4.2.10` (local server). Verified against this server that `HTTP 200` responses with `X-ClickHouse-Exception-Tag` (and an error body) are produced when streaming has begun before the error is detected — for example:

```
curl -sv 'http://localhost:8123/?query=SELECT+throwIf(number=100000,+%27boom%27)+FROM+system.numbers+LIMIT+100000000+FORMAT+RowBinary&http_response_buffer_size=0&max_block_size=10000'
< HTTP/1.1 200 OK
< Transfer-Encoding: chunked
< X-ClickHouse-Exception-Tag: pdqzvcyuuqsybmvs
```

Note that for this mid-stream case the server emits `X-ClickHouse-Exception-Tag` but not `X-ClickHouse-Exception-Code`; for fully buffered failures it emits both alongside `HTTP 500`. The cross-shard distributed-DDL case (clickhouse-go#1398) produces an `HTTP 200` response whose body encodes the error inside a Native block, with no `__exception__` marker — this is not reproducible against a single-node server.

## Reproduction

The HTTP-status-only check is straightforward to demonstrate without a server using a stub that mirrors what the server emits during a mid-stream exception. Add the following to `ClickHouse.Driver.Tests` (alongside `MidStreamExceptionTests`):

```csharp
using System.Net;
using System.Net.Http;
using System.Threading.Tasks;
using ClickHouse.Driver;
using NUnit.Framework;

namespace ClickHouse.Driver.Tests.ADO;

public class ExecuteNonQuerySilentExceptionTests
{
private sealed class StubHandler : HttpMessageHandler
{
protected override Task SendAsync(HttpRequestMessage request, System.Threading.CancellationToken ct)
{
// Mimic the server's mid-stream exception: HTTP 200 + exception headers + body.
var resp = new HttpResponseMessage(HttpStatusCode.OK)
{
Content = new ByteArrayContent(System.Text.Encoding.UTF8.GetBytes(
"Code: 44. DB::Exception: Sorting key contains nullable columns ... (ILLEGAL_COLUMN)"))
};
resp.Headers.TryAddWithoutValidation("X-ClickHouse-Exception-Code", "44");
resp.Headers.TryAddWithoutValidation("X-ClickHouse-Exception-Tag", "abc");
return Task.FromResult(resp);
}
}

[Test]
public async Task ExecuteNonQueryAsync_WhenHttp200ButExceptionHeaderSet_ShouldThrow()
{
var settings = new ClickHouseClientSettings("Host=localhost")
{
HttpClient = new HttpClient(new StubHandler()),
};
using var client = new ClickHouseClient(settings);

Assert.ThrowsAsync(
async () => await client.ExecuteNonQueryAsync("CREATE TABLE t ON CLUSTER c (...) ENGINE = ReplicatedMergeTree PRIMARY KEY (a)"));
}
}
```

Expected: `ExecuteNonQueryAsync` throws `ClickHouseServerException` (the server reported an exception via header).
Actual: `ExecuteNonQueryAsync` returns `0` and no exception is raised.

The same shape applies to the streaming reader path for the distributed-DDL case where the body is a valid Native block whose rows encode the error text — `ClickHouseDataReader` decodes the rows without seeing an `__exception__` marker, and `X-ClickHouse-Exception-Code` is not consulted.

## Suggested fix

1. In `ClickHouseClient.HandleError`, after the `IsSuccessStatusCode` short-circuit, also inspect the response for `X-ClickHouse-Exception-Code` (or `X-ClickHouse-Exception-Tag` from #168). If either is present with a non-empty value, treat the response as an error regardless of status code: read the body and throw a `ClickHouseServerException` with the parsed code/message. This single change covers `ExecuteNonQueryAsync`, `ExecuteScalarAsync`, `ExecuteReaderAsync`, `ExecuteRawResultAsync`, `PostStreamAsync`, `PingAsync`, and the insert/bulk paths because they all route through `HandleError`.
2. Optionally, default `wait_end_of_query=1` for the `Exec`/`Ping` paths (where streaming the response is not needed) so the server emits a proper non-2xx status when an exception is detected before the body is flushed.
3. Add the unit test above to `ClickHouse.Driver.Tests/ADO/MidStreamExceptionTests.cs` (it does not require a running server).

## Link

Related upstream issue: https://github.com/ClickHouse/clickhouse-rs/issues/255
Related sibling-client manifestation: https://github.com/ClickHouse/clickhouse-go/issues/1398

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.