Skip to content

fix: surface insert errors reported with HTTP 200 - #709

Open
Mr444-dev wants to merge 2 commits into
ClickHouse:mainfrom
Mr444-dev:fix/705-insertbinary-2xx-error
Open

Mr444-dev wants to merge 2 commits into
ClickHouse:mainfrom
Mr444-dev:fix/705-insertbinary-2xx-error

Conversation

@Mr444-dev

@Mr444-dev Mr444-dev commented Nov 2, 2026 •

Copy link
Copy Markdown

Summary

  • Treat X-ClickHouse-Exception-Code as a server error even when progress headers leave the HTTP status at 200.
  • Preserve the server error code and body, and report an error if the response body is empty.
  • Add regression coverage for InsertBinaryAsync and a changelog fragment.

Fixes #705

Validation

  • Targeted .NET 9 tests: 3 passed, 0 failed. The test fixture setup used a local HTTP stub; the regression tests exercise a fake HTTP response.
  • dotnet run scripts/changelog.cs -- --check passed.

Note

Medium Risk
Central HTTP error handling changed for all callers of HandleError, including inserts and queries; behavior is more strict but aligns with server semantics and prevents silent insert failures.

Overview
Fixes a case where binary inserts (and other operations using HandleError) could succeed even though ClickHouse failed after streaming progress headers—the HTTP status stays 200 while X-ClickHouse-Exception-Code carries the real failure.

HandleError now treats any response with that header as an error, not only non-2xx statuses. When the body is non-empty, the thrown ClickHouseServerException uses the header value as ErrorCode when present; empty-body paths get clearer messaging for “exception reported with HTTP 200.”

Tests add InsertBinaryAsync regression coverage and extend the empty-body + header test to HttpStatusCode.OK as well as 503. Changelog fragment for #705.

Reviewed by Cursor Bugbot for commit 40850ae. Bugbot is set up for automated code reviews on this repo. Configure here.

Copilot AI balanced review requested due to automatic review settings November 2, 2026 23:27
@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

Reviewed by Cursor Bugbot for commit 2044fba. Configure here.

Copy link
Copy Markdown
Author

Removed the generated changelog placeholder so the fragment now contains only the release note for #705. dotnet run scripts/changelog.cs -- --check passes.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

InsertBinaryAsync silently reports success when the server fails mid-insert with send_progress_in_http_headers=1

3 participants