Notify the mailbox reader when the last pending data offer gives up - #19329
Open
davecromberge wants to merge 1 commit into
Open
Notify the mailbox reader when the last pending data offer gives up#19329davecromberge wants to merge 1 commit into
davecromberge wants to merge 1 commit into
Conversation
An error EOS drains the queued data blocks and makes every parked sender give up. While those offers are still in flight, `poll` returns `null` and asks the reader to wait for them. But a sender that gives up returns `ALREADY_TERMINATED` without notifying, and the reader coalesces notifications into a single slot, so the EOS notification cannot stand in for the missing one. Nothing wakes the reader again: it blocks until the query deadline and the query reports `EXECUTION_TIMEOUT` instead of the error that actually caused it. Reachable whenever the queue is full and senders are parked when an error EOS or a cancellation arrives. The same gap swallows the `TimeoutException` and `InterruptedException` paths out of `offerDataToBuffer`, which unwind into an `offerEos` that is rejected as already terminated. `offerDataToBuffer` now notifies on every exit path when it is the last pending offer and the upstream has finished, which is exactly the condition `poll` waits on. On the success path this duplicates the notification from `offerData`, which is harmless because notifications coalesce. The test asserts the notification count rather than blocking on a read, so it is deterministic: the wake-up is delivered when the last pending offer completes, whether or not the reader managed to poll first. It fails on master with `expected [3] but found [2]`.
davecromberge
added a commit
to permutive-engineering/pinot
that referenced
this pull request
Aug 21, 2026
). Notify the mailbox reader when the last pending data offer gives up. An error EOS drains the queued data blocks and makes every parked sender give up; while those offers are still in flight `poll` returns null and asks the reader to wait for them, but a sender that gives up returns ALREADY_TERMINATED without notifying and the reader coalesces notifications into a single slot. Nothing wakes the reader again, so it blocks until the query deadline and the query reports EXECUTION_TIMEOUT instead of the real error. Replaces the previously carried cherry-pick of fix-contrib/mse-inmemory-mailbox-reader-race (9c6408a, upstream apache#19323), which is closed: that registerReader wake-up is provably unreachable because the reader scans every mailbox before it parks, so a notification dropped before registration is always redundant with that scan. Upstream PR: apache#19329 (open)
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #19329 +/- ##
============================================
+ Coverage 67.02% 67.03% +0.01%
- Complexity 1418 1424 +6
============================================
Files 3463 3463
Lines 221673 221674 +1
Branches 34955 34956 +1
============================================
+ Hits 148578 148610 +32
+ Misses 61260 61229 -31
Partials 11835 11835
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
yashmayya
requested review from
gortiz and
yashmayya
and
a balanced review from Copilot
August 21, 2026 16:58
yashmayya
added
bug
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
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.
An error EOS drains the queued data blocks and makes every parked sender give up. While those offers are still in flight,
pollreturnsnulland asks the reader to wait for them (ReceivingMailbox.java:657). But a sender that gives up returnsALREADY_TERMINATEDwithout notifying, and the reader coalesces notifications into a single slot, so the EOS notification cannot stand in for the missing one. Nothing wakes the reader again: it blocks until the query deadline and the query reportsEXECUTION_TIMEOUTinstead of the error that actually caused it.Reachable whenever the queue is full and senders are parked when an error EOS or a cancellation arrives. The same gap swallows the
TimeoutExceptionandInterruptedExceptionpaths out ofofferDataToBuffer, which unwind into anofferEosthat is rejected as already terminated.offerDataToBuffernow notifies on every exit path when it is the last pending offer and the upstream has finished — exactly the conditionpollwaits on. The lock is held there, sinceawaitNanosreacquires it on all three exits. On the success path this duplicates the notification fromofferData, which is harmless because notifications coalesce.The test asserts the notification count rather than blocking on a read, so it is deterministic: the wake-up is delivered when the last pending offer completes, whether or not the reader managed to poll first. It fails on master with
expected [3] but found [2].Supersedes #19323, which proposed a fix for a lost wake-up at reader registration. That one turned out to be unreachable: registration precedes the reader's first scan, and the scan makes the dropped notification redundant. Thanks to @yashmayya for pushing back on it and pointing at this path instead.