fix: surface BufferExec input panics instead of silently truncating output#23243
Open
Tristan1900 wants to merge 2 commits into
Open
fix: surface BufferExec input panics instead of silently truncating output#23243Tristan1900 wants to merge 2 commits into
Tristan1900 wants to merge 2 commits into
Conversation
MemoryBufferedStream runs the input on a spawned producer task that feeds a channel. A panic while polling the input was caught by the tokio task harness, which dropped the sender; the consumer then read the closed channel as a clean EOF and silently truncated the rest of the partition's output, so the query completes with partial results instead of failing. Wrap the input poll in catch_unwind and forward the panic as a DataFusionError over the existing error channel, so it propagates and fails the stream instead of being swallowed.
gabotechs
approved these changes
Jun 29, 2026
gabotechs
left a comment
gabotechs
left a comment
Contributor
There was a problem hiding this comment.
Just double checked that the test indeed reproduces the issue in main. Good catch @Tristan1900!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this close?
Closes #23242.
Rationale for this change
When the input to a
BufferExecpanics, 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 aResult::Err, so it also skips thebatch_tx.send(Err(..))path — the partition gets silently truncated and the query returns partial results instead of failing. More detail in #23242.What changes are included in this PR?
Catch the panic at the input poll inside
MemoryBufferedStreamand forward it as aDataFusionErrorover 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 wayRecordBatchReceiverStreamdoes), since the consumer already handlesSome(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
BufferExecnow fails with an error instead of silently returning truncated results. No API changes.