Fix ReadBuffer cancellation assert in row-format verbose diagnostics

[groeneai]

Changelog category (leave one):

• Bug Fix (user-visible misbehavior in an official stable release)

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Fixed a possible Logical error: 'ReadBuffer is canceled. Can't read from it.' when a row-based input format (e.g. TSV/CSV) failed to parse input and the underlying read had already been canceled (for example a malformed HTTP chunk or a truncated body). Building the verbose parse diagnostics no longer reads from a canceled buffer.

Description

When a row-format parse fails because an underlying read throws mid-parse, ReadBuffer::next() self-cancels the buffer in its catch (...) { cancel(); throw; } handler (sets canceled = true and rethrows). The failure surfaces as a parse error, so IRowInputFormat::read() calls getDiagnosticInfo() to enrich the message. RowInputFormatWithDiagnosticInfo::getDiagnosticAndRawDataImpl() then called in->eof(), and eof() calls next(), which trips chassert(!isCanceled()) (src/IO/ReadBuffer.cpp:107) and aborts in assertion-enabled builds.

The fix guards the diagnostics entry with in->isCanceled() before eof(), mirroring the caller-side guard added for TCPHandler in #100666. A canceled buffer cannot yield further diagnostics, and re-reading it is exactly what the assertion forbids.

This is a distinct caller of the same assertion family (fuzz issues #84186, #98866, #99258, #100581) not covered by the TCPHandler fix (#100666) or the S3 backup upload-retry fix (#108573).

A gtest regression (RowInputFormatDiagnostics.CanceledBufferDoesNotAbort) drives a TabSeparatedRowInputFormat over a buffer that self-cancels on a parse-classified error mid-row; it aborts without the fix and passes with it. This behavior is not observable from SQL because the abort is a chassert in assertion-enabled builds and the exact HTTP framing that reaches the format-level read is timing-sensitive.

Version info

• Merged into: 26.7.1.773 (included in 26.7 and later)

[groeneai]

Pre-PR validation gate (click to expand)

#	Question	Answer
a	Deterministic repro?	Yes. gtest RowInputFormatDiagnostics.CanceledBufferDoesNotAbort drives TabSeparatedRowInputFormat::read() over a ReadBuffer that serves one partial row then throws CANNOT_READ_ALL_DATA (parse-classified) from nextImpl(), self-canceling the buffer. Aborts on every run without the fix.
b	Root cause explained?	A read throws mid-parse → ReadBuffer::next()'s catch(...){ cancel(); throw; } sets canceled=true and rethrows → the parse error reaches IRowInputFormat::read() which calls getDiagnosticInfo() → getDiagnosticAndRawDataImpl() calls in->eof() → eof() calls next() → chassert(!isCanceled()) aborts (ReadBuffer.cpp:107).
c	Fix matches root cause?	Yes. Guards the diagnostics entry with in->isCanceled() before eof(), so a canceled buffer returns the "Buffer has gone" message instead of re-reading. Mirrors the caller-side guard in #100666, not a symptom band-aid.
d	Test intent preserved / new tests added?	New gtest added that catches exactly this regression. No existing test weakened.
e	Both directions demonstrated?	Yes. Baseline binary: SIGABRT (rc 134) with the exact reported stack (getDiagnosticAndRawDataImpl → getDiagnosticInfo:105 → IRowInputFormat::read:247). With fix: PASSED.
f	Fix is general across code paths?	getDiagnosticAndRawDataImpl is the single diagnostics entry for all RowInputFormatWithDiagnosticInfo subclasses (TSV/CSV/etc.), so the one guard covers every row format. The two sibling callers of this assertion family were already fixed (#100666 TCPHandler, #108573 S3 upload retry).
g	Fix generalizes across inputs?	The guard is input-agnostic — it triggers on any canceled buffer regardless of the underlying read that canceled it (malformed HTTP chunk, truncated body, decompressor error).
h	Backward compatible?	Yes. No setting, format, or on-disk/wire change. Only replaces an abort with the existing graceful "Buffer has gone" fallback.
i	Invariants and contracts preserved?	Yes. Upholds the ReadBuffer use-after-cancel contract (never read a canceled buffer). Does not weaken eof()/next() themselves (a central weakening would erode the guard for other callers) — the guard is caller-side, consistent with #100666.

Session id: cron:clickhouse-worker-slot-3:20260707-191500

[groeneai]

cc @al13n321 @Avogar — could you review? This guards RowInputFormatWithDiagnosticInfo::getDiagnosticAndRawDataImpl with in->isCanceled() before eof(), so building verbose parse diagnostics no longer re-reads a buffer that self-canceled when the parse-triggering read threw mid-stream (the third caller of the ReadBuffer is canceled assertion family, after #100666 and #108573).

[clickhouse-gh]

Workflow [PR], commit [ea76890]

Summary: ✅

• Performance Comparison: Performance dashboard

---

AI Review

Summary

This PR moves the canceled-buffer guard into IRowInputFormat::read() so row-based input formats no longer re-enter ReadBuffer::next() after a throwing read has already canceled the buffer, covering both the verbose-diagnostics path and the allow_errors / syncAfterError path. I did not find a remaining code-level correctness issue in the current head, but the PR metadata still overstates the user-visible impact.

PR Metadata

• ⚠️ Changelog category is not semantically correct. The PR description explicitly says the failure is only observable in assertion-enabled builds and not from SQL in official stable releases, so Bug Fix (user-visible misbehavior in an official stable release) is the wrong category. Replace it with Not for changelog (changelog entry is not required).
• ⚠️ With that category, the Changelog entry should be removed.

Final Verdict

⚠️ No remaining code findings in the current head. I would merge after correcting the changelog metadata to match the actual scope of the fix.

[groeneai]

Good catch — confirmed and fixed in fdad22b.

You're right: with allow_errors_num / allow_errors_ratio set, the parse error is caught in IRowInputFormat::read()'s row loop (before the diagnostics guard is ever reached), and syncAfterError() is called to skip the bad row. For TSV/CSV/template/JSON row formats syncAfterError() starts with skipTo*NextLineOrEOF (or ignore), whose first buf.eof() calls next() on the same self-canceled PeekableReadBuffer, so the identical chassert(!isCanceled()) fires. Verified locally: without the guard the new regression aborts with SIGABRT at IRowInputFormat.cpp:214 syncAfterError() → skipToUnescapedNextLineOrEOF → ReadBuffer::next().

Fix: rethrow the original parse error when getReadBuffer().isCanceled() before calling syncAfterError() — mirroring the diagnostics guard. This is a single caller-side guard in IRowInputFormat::read(), so it covers every format's syncAfterError() (TSV/CSV/template/custom/JSON*/TSKV/BSON/Avro), not just TSV. A canceled buffer cannot yield more rows anyway, so skipping is not possible; the correct behavior is to surface the original parse error.

Extended the gtest with CanceledBufferDoesNotAbortWithAllowErrors (sets allow_errors_num=10, allow_errors_ratio=1.0) covering exactly this path. Both directions verified: aborts without the guard (SIGABRT), passes with it.

Pre-PR validation gate (click to expand)

#	Question	Answer
a	Deterministic repro?	Yes — gtest RowInputFormatDiagnostics.CanceledBufferDoesNotAbortWithAllowErrors: a ThrowingReadBuffer serves one row then throws CANNOT_READ_ALL_DATA from nextImpl() (self-cancels), with allow_errors_num=10. Aborts on demand without the guard.
b	Root cause explained?	Parse-classified throw from nextImpl() → ReadBuffer::next() catch calls cancel() → buffer canceled. With allow_errors set, read() catches the parse error and calls syncAfterError() → skipTo*NextLineOrEOF/ignore → buf.eof() → next() → chassert(!isCanceled()) aborts.
c	Fix matches root cause?	Yes — guards the exact call site: rethrow when getReadBuffer().isCanceled() before syncAfterError(), so next() is never called on a canceled buffer. No skip is possible on a canceled buffer, so the original parse error is surfaced.
d	Test intent preserved / new tests added?	New regression test added (CanceledBufferDoesNotAbortWithAllowErrors); existing CanceledBufferDoesNotAbort (diagnostics path) unchanged and still passing.
e	Both directions demonstrated?	Yes — without guard: SIGABRT (IRowInputFormat.cpp:214 → skipToUnescapedNextLineOrEOF → ReadBuffer::next); with guard: 2/2 tests pass.
f	Fix is general across code paths?	Yes — the guard is in IRowInputFormat::read(), the single common caller, so it covers all syncAfterError() implementations (TSV/CSV/template/custom/JSON*/TSKV/BSON/Avro), which all read from the same canceled buffer.
g	Fix generalizes across inputs?	N/A — not input/datatype dependent; the guard triggers on buffer cancellation state regardless of format, columns, or values.
h	Backward compatible?	Yes — no setting/format/API change. Behavior change is strictly: abort → normal exception (the original parse error) on an already-canceled buffer.
i	Invariants and contracts preserved?	Yes — upholds ReadBuffer::next()'s precondition !isCanceled(). No skip/rollback is attempted on a buffer that cannot be read; the error path returns via the existing rethrow.

Session id: cron:clickhouse-worker-slot-2:20260708-075500

[groeneai]

CI finish ledger — fdad22b

CI fully finished (Finish Workflow pass; 0 checks queued/in_progress; >20-min ingestion buffer). No test-level FAIL/ERROR in CIDB on this head. No PR-caused failure (this PR only guards a canceled ReadBuffer in the row-format verbose-diagnostics + syncAfterError path).

Check / test	Reason	Owner / fixing PR
Performance Comparison (arm_release, master_head, 5/6)	flaky "Errors while building the report" (unstable queries; every query row success, 0 regressions)	#109397 (ours, open)
Report messages (1 warning)	praktika non-gating soft-warning rollup	not a test failure
Integration tests (amd_asan_ubsan, targeted)	skipped (no changed files match)	N/A
Sync	—	CH Inc sync (private, not actionable)

Session id: cron:our-pr-ci-monitor:20260708-123000

[Avogar]

Let's move this check together with isParseError. So we will go to the logic of skipping bad lines only when it's a parse error that wasn't thrown in the buffer itself

[groeneai]

Done in ea76890. Folded the canceled-buffer check into the isParseError gate at the top of the inner catch: `if (!isParseError(e.code()) || getReadBuffer().isCanceled()) throw;`. Now we only enter the skip-bad-lines / syncAfterError logic for a genuine parse error that was not thrown from the buffer itself. Removed the standalone check that was before syncAfterError().

[Avogar]

We should not reach this code if buffer is cancelled. Let's check from where we call getDiagnosticInfo and check if there is a check like isParseError before that.

[groeneai]

Done in ea76890. Traced the caller: getDiagnosticInfo() is invoked from exactly one place, the outer catch in IRowInputFormat::read(), which already gates on isParseError. Added the canceled-buffer check to that same gate (`if (!isParseError(e.code()) || getReadBuffer().isCanceled()) throw;`) so we never reach getDiagnosticInfo() with a canceled buffer, and reverted the check inside getDiagnosticAndRawDataImpl() back to the original `if (in->eof())`.

[groeneai]

Restructured the canceled-buffer guards per review (ea76890): the checks are now folded into the isParseError classification in IRowInputFormat::read() (skip-bad-lines path and verbose-diagnostics path), and the internal guard in RowInputFormatWithDiagnosticInfo.cpp is reverted since that code is no longer reachable with a canceled buffer.

Pre-PR validation gate (click to expand)

#	Check	Result
a	Deterministic repro?	Yes. gtest ThrowingReadBuffer serves one row then throws CANNOT_READ_ALL_DATA from nextImpl(); ReadBuffer::next() self-cancels the buffer. Two tests drive both paths deterministically.
b	Root cause explained?	A throwing read self-cancels the buffer via ReadBuffer::next()'s catch handler. read() then classifies the error as a parse error and either (1, allow_errors) calls syncAfterError() → skipTo*NextLineOrEOF/eof() → next(), or (2) calls getDiagnosticInfo() → eof() → next(). Both re-enter ReadBuffer::next() on the canceled buffer, tripping chassert(!isCanceled()).
c	Fix matches root cause?	Yes. A self-canceled buffer is not a genuine parse error to skip, so both paths now rethrow before touching the buffer: if (!isParseError(e.code()) || getReadBuffer().isCanceled()) throw; at each gate.
d	Test intent preserved / new tests?	Two gtests added: CanceledBufferDoesNotAbort (diagnostics path) and CanceledBufferDoesNotAbortWithAllowErrors (allow_errors syncAfterError path). No existing test weakened.
e	Demonstrated both directions?	Yes. Against pristine base both tests SIGABRT (exit 134); with the fix both PASS.
f	Fix general, not a narrow patch?	Yes. The guards sit in the shared IRowInputFormat::read(), so every row format (TSV/CSV/template/custom/JSON*/TSKV) is covered — getReadBuffer() returns the same in buffer each format reads and syncAfterError() operates on.
g	Generalizes across inputs?	Covered by (f): the guard is caller-side in the base class, independent of format, error subtype, or allow_errors_num/allow_errors_ratio values.
h	Backward compatible?	Yes. No setting/format/wire change. Only converts an assertion-build abort into the normal thrown parse exception; release-build behavior (exception) is unchanged.
i	Invariants preserved?	Yes. ReadBuffer::next()'s !isCanceled() precondition is now respected on both paths; skip-bad-lines still fires for genuine parse errors that did not cancel the buffer.

Session id: cron:clickhouse-worker-slot-2:20260708-131300

[clickhouse-gh]

LLVM Coverage Report

Metric	Baseline	Current	Δ
Lines	85.90%	85.90%	+0.00%
Functions	92.70%	92.70%	+0.00%
Branches	78.00%	78.10%	+0.10%

Changed lines: Changed C/C++ lines covered: 39/39 (100.00%) · Uncovered code

Full report · Diff report

[groeneai]

CI finish ledger — ea76890

All gating checks passed (Finish Workflow + Mergeable Check green, 0 failed checks). CH Inc sync still running (private, exempt).

Check / test	Reason	Owner / fixing PR
(all gating checks)	—	all green
CH Inc sync	—	CH Inc sync (private, not actionable)

Session id: cron:our-pr-ci-monitor:20260708-180000

[alexey-milovidov]

LGTM

Changes made.