Skip to content

Commit 6ef3a3b

Browse files
authored
fix: surface BufferExec input panics instead of silently truncating output (apache#23243)
## Which issue does this close? Closes apache#23242. ## Rationale for this change When the input to a `BufferExec` panics, the panic unwinds the background producer task (`MemoryBufferedStream`), tokio catches it at the task boundary, and the dropped sender looks like a clean end-of-stream to the consumer. A panic isn't a `Result::Err`, so it also skips the `batch_tx.send(Err(..))` path — the partition gets silently truncated and the query returns partial results instead of failing. More detail in apache#23242. ## What changes are included in this PR? Catch the panic at the input poll inside `MemoryBufferedStream` and forward it as a `DataFusionError` over the channel the consumer already drains, so it propagates instead of being swallowed. I went with surfacing it as an error rather than re-raising via `join_unwind` (the way `RecordBatchReceiverStream` does), since the consumer already handles `Some(Err(..))` and this fails only the query. Happy to switch to a re-raise if that's preferred for consistency. ## Are these changes tested? Yes — added `panic_in_input_is_propagated`, which feeds a stream that panics partway through and asserts the buffered stream yields an error instead of finishing cleanly. ## Are there any user-facing changes? A query whose input panics under a `BufferExec` now fails with an error instead of silently returning truncated results. No API changes.
1 parent 01bf68c commit 6ef3a3b

1 file changed

Lines changed: 43 additions & 6 deletions

File tree

datafusion/physical-plan/src/buffer.rs

Lines changed: 43 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -41,9 +41,10 @@ use datafusion_physical_expr_common::metrics::{
4141
};
4242
use datafusion_physical_expr_common::physical_expr::PhysicalExpr;
4343
use datafusion_physical_expr_common::sort_expr::PhysicalSortExpr;
44-
use futures::{Stream, StreamExt, TryStreamExt};
44+
use futures::{FutureExt, Stream, StreamExt, TryStreamExt};
4545
use pin_project_lite::pin_project;
4646
use std::fmt;
47+
use std::panic::AssertUnwindSafe;
4748
use std::pin::Pin;
4849
use std::sync::Arc;
4950
use std::sync::atomic::{AtomicUsize, Ordering};
@@ -337,11 +338,24 @@ impl<T: Send + SizedMessage + 'static> MemoryBufferedStream<T> {
337338
let item_or_err = tokio::select! {
338339
biased;
339340
_ = batch_tx.closed() => break,
340-
item_or_err = input.next() => {
341-
let Some(item_or_err) = item_or_err else {
342-
break; // stream finished
343-
};
344-
item_or_err
341+
// Catch a panic in the input poll so it surfaces as a stream error
342+
// instead of dropping `batch_tx` and looking like a clean EOF.
343+
polled = AssertUnwindSafe(input.next()).catch_unwind() => {
344+
match polled {
345+
Ok(Some(item_or_err)) => item_or_err,
346+
Ok(None) => break, // stream finished
347+
Err(panic) => {
348+
let msg = panic
349+
.downcast_ref::<&str>()
350+
.map(|s| s.to_string())
351+
.or_else(|| panic.downcast_ref::<String>().cloned())
352+
.unwrap_or_else(|| "unknown panic".to_string());
353+
let _ = batch_tx.send(internal_err!(
354+
"BufferExec input stream panicked: {msg}"
355+
));
356+
break;
357+
}
358+
}
345359
}
346360
};
347361

@@ -554,6 +568,29 @@ mod tests {
554568
Ok(())
555569
}
556570

571+
#[tokio::test]
572+
async fn panic_in_input_is_propagated() -> Result<(), Box<dyn Error>> {
573+
// A panic while polling the input must surface as a stream error, not a
574+
// silent end-of-stream that drops the rest of the partition's output.
575+
let input = futures::stream::iter([1, 2, 3, 4]).map(|v| {
576+
if v == 3 {
577+
panic!("boom on 3");
578+
}
579+
Ok(v)
580+
});
581+
let (_, res) = memory_pool_and_reservation();
582+
583+
let mut buffered = MemoryBufferedStream::new(input, 10, res);
584+
wait_for_buffering().await;
585+
586+
pull_ok_msg(&mut buffered).await?;
587+
pull_ok_msg(&mut buffered).await?;
588+
let err = pull_err_msg(&mut buffered).await?;
589+
assert_contains!(err.to_string(), "panicked");
590+
591+
Ok(())
592+
}
593+
557594
#[tokio::test]
558595
async fn memory_gets_released_if_stream_drops() -> Result<(), Box<dyn Error>> {
559596
let input = futures::stream::iter([1, 2, 3, 4]).map(Ok);

0 commit comments

Comments
 (0)