From 624f24ca3b2cb65c602284bfba592f049f087a0d Mon Sep 17 00:00:00 2001 From: "Mingyu Chen (Rayner)" Date: Fri, 26 Jun 2026 16:50:52 +0800 Subject: [PATCH] [fix](arrow-flight) Harden split source error path to avoid BE crash on external table scan (#64797) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ### What problem does this PR solve? Issue Number: ref #62259 (partial — robustness layer only, does not close the issue) Related PR: N/A Problem Summary: When an Arrow Flight SQL query scans an external table (e.g. Iceberg) in batch split mode, the BE fetches file splits from the FE via the `fetchSplitBatch` thrift RPC *during* the scan. If that fetch fails — most notably when the split source has already been released — the error path could crash the BE (SIGSEGV in `arrow::flight::internal::TransportStatus::FromStatus`) instead of failing the query gracefully (see #62259). This PR is the **robustness layer** for that issue: it ensures any `fetchSplitBatch` failure makes the query fail gracefully rather than crashing the BE. It does **not** fix the underlying split source lifecycle problem (the source being released after `GetFlightInfo` but before `DoGet` on the Arrow Flight two-phase path), which is tracked separately in the issue. Changes: 1. **BE `split_source_connector`** — guard `result.status.error_msgs[0]` with an `empty()` check to avoid an out-of-bounds vector read when the FE returns a non-OK status without an error message. 2. **BE `to_arrow_status`** — truncate the error message handed to Arrow/gRPC to a length well below 8192. The message is carried in the gRPC trailer (an HTTP2 header) and may be percent-encoded, so an oversized one can break the response or crash the flight transport status conversion. The 8192 limit was already documented in a comment in this function but was never enforced. The full message is still logged on the BE. 3. **FE `fetchSplitBatch`** — when the split source has been released, return a structured `TStatus(NOT_FOUND)` with a message instead of throwing a bare `TException`. The BE then receives a well-formed, non-empty error through the normal result path instead of a thrift transport exception. ### Release note Fix a BE crash (SIGSEGV) that could happen on the error path of Arrow Flight SQL queries against external tables when fetching split batches from the FE fails. ### Check List (For Author) - Test - [ ] Regression test - [ ] Unit Test - [ ] Manual test (add detailed scripts or steps below) - [x] No need to test or manual test. Explain why: - [ ] This is a refactor/code format and no logic has been changed. - [ ] Previous test can cover this change. - [ ] No code files have been changed. - [x] Other reason: This is defensive hardening of an error path (an out-of-bounds guard, a message-length cap, and returning a structured error status instead of throwing). It only triggers when `fetchSplitBatch` fails; existing tests cover the success path, and the crash depends on Arrow/gRPC transport internals that are hard to reproduce deterministically in CI. - Behavior changed: - [x] Yes. On `fetchSplitBatch` failure the query now fails with a clear error message instead of (potentially) crashing the BE, and the FE no longer throws a bare `TException` for a released split source (it returns a `NOT_FOUND` status). - Does this need documentation? - [x] No. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- be/src/exec/scan/split_source_connector.cpp | 4 +++- be/src/format/arrow/arrow_utils.cpp | 12 +++++++++++- .../apache/doris/service/FrontendServiceImpl.java | 10 +++++++++- 3 files changed, 23 insertions(+), 3 deletions(-) diff --git a/be/src/exec/scan/split_source_connector.cpp b/be/src/exec/scan/split_source_connector.cpp index 81748261d98145..685a2d50f0c1a3 100644 --- a/be/src/exec/scan/split_source_connector.cpp +++ b/be/src/exec/scan/split_source_connector.cpp @@ -59,7 +59,9 @@ Status RemoteSplitSourceConnector::get_next(bool* has_next, TFileRangeDesc* rang coord->fetchSplitBatch(result, request); if (result.__isset.status && result.status.status_code != TStatusCode::OK) { return Status::IOError("Failed to get batch of split source: {}", - result.status.error_msgs[0]); + result.status.error_msgs.empty() + ? "unknown error" + : result.status.error_msgs[0]); } } catch (std::exception& e) { return Status::IOError("Failed to get batch of split source: {}", e.what()); diff --git a/be/src/format/arrow/arrow_utils.cpp b/be/src/format/arrow/arrow_utils.cpp index 724af28be39d53..d9c29eceed01ef 100644 --- a/be/src/format/arrow/arrow_utils.cpp +++ b/be/src/format/arrow/arrow_utils.cpp @@ -38,7 +38,17 @@ arrow::Status to_arrow_status(const Status& status) { // The length of exception msg returned to the ADBC Client cannot larger than 8192, // otherwise ADBC Client will receive: // `INTERNAL: http2 exception Header size exceeded max allowed size (8192)`. - return arrow::Status::Invalid(status.to_string_no_stack()); + // The message is carried in the gRPC trailer (an HTTP2 header) and may be + // percent-encoded (which can expand its size), so an oversized message can break the + // response or even crash the flight transport status conversion. Truncate it well below + // 8192 to leave headroom; the full message is already logged above. + constexpr size_t kMaxArrowStatusMsgLen = 1024; + std::string msg = status.to_string_no_stack(); + if (msg.size() > kMaxArrowStatusMsgLen) { + msg.resize(kMaxArrowStatusMsgLen); + msg += "... (truncated)"; + } + return arrow::Status::Invalid(msg); } } diff --git a/fe/fe-core/src/main/java/org/apache/doris/service/FrontendServiceImpl.java b/fe/fe-core/src/main/java/org/apache/doris/service/FrontendServiceImpl.java index 7441113ad5636f..f0d704341838e5 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/service/FrontendServiceImpl.java +++ b/fe/fe-core/src/main/java/org/apache/doris/service/FrontendServiceImpl.java @@ -1023,7 +1023,15 @@ public TFetchSplitBatchResult fetchSplitBatch(TFetchSplitBatchRequest request) t SplitSource splitSource = Env.getCurrentEnv().getSplitSourceManager().getSplitSource(request.getSplitSourceId()); if (splitSource == null) { - throw new TException("Split source " + request.getSplitSourceId() + " is released"); + // Return a structured error status instead of throwing a bare TException, so the BE + // receives a well-formed error (with a non-empty message) through the normal result + // path. Throwing here surfaces as a thrift transport exception on the BE side and, + // on the Arrow Flight path, may feed an empty/invalid error string into the gRPC + // status conversion. See https://github.com/apache/doris/issues/62259 + LOG.warn("split source {} is released", request.getSplitSourceId()); + result.status = new TStatus(TStatusCode.NOT_FOUND); + result.status.addToErrorMsgs("Split source " + request.getSplitSourceId() + " is released"); + return result; } try { List locations = splitSource.getNextBatch(request.getMaxNumSplits());