Repository navigation
fix: avoid lost-wakeup hang in get_equality_delete_predicate_for_delete_file_path - #3268
Conversation
mbutrovich
left a comment
mbutrovich
left a comment
There was a problem hiding this comment.
Thanks for reviving this, @dhruvarya-db! The fix is still correct on current main. It now matches both merged siblings, delete_file_index.rs (#2696) and the positional path (#2859). The one inline comment is about the code comment, which didn't pick up the change the description mentions.
The
try_start_eq_del_load/insert_equality_deletenotify_A / notify_B double-mint raised on #2873 is a separate lost-wakeup already fixed by the approved-but-unmerged #2630; it composes with this fix and should not be re-fixed here. Happy to coordinate ordering.
The equality-delete path isn't hang-free until both land. The two changes touch different functions in this file, so they shouldn't conflict. Could you and @fallintoplace agree on which merges first, and link that decision here?
The error-path
RecvErrorpanic that can leave an entry stuckLoading(raised on #2873) is pre-existing and only reachable if aDeleteFilteris shared across concurrentload_deletescalls; I'll follow up separately.
Could you file an issue for it and link it here, so the follow-up has somewhere to live?
I'm happy to fold the seam + regression test into this PR if maintainers prefer.
I'd keep the test hook out of this PR. The old code has no .await between releasing the lock and building the Notified, so the race only happens across threads. The hook in your fork holds that window open with a 500 ms sleep, switched on by a process-global AtomicBool. Since cargo test runs tests on threads in one process, any other test that calls this method while the flag is set would also pause, and the test depends on sleep timing. #2696 merged the same fix without a test.
| // Create the `Notified` while holding the read lock. The read lock ensures that | ||
| // when we go inside it, either the state is already at Loaded or it is still at | ||
| // Loading AND `notify_waiters()` has not been called yet. Any `Notified` created | ||
| // before the invocation of `notify_waiters()` will be notified by it even if | ||
| // `await` has not been called on it yet. |
There was a problem hiding this comment.
The description says this comment now explains why the fix works, as suggested on #2873, but it's the same text as before. It still describes the outcome without the reason: notified_owned() snapshots tokio's notify_waiters_calls counter when it's built, and holding the read lock is what orders that snapshot before insert_equality_delete can call notify_waiters(). Without that, a later refactor that moves notified_owned() out from under the lock looks harmless and brings the hang back. Could you push the updated wording? Something like:
| // Create the `Notified` while holding the read lock. The read lock ensures that | |
| // when we go inside it, either the state is already at Loaded or it is still at | |
| // Loading AND `notify_waiters()` has not been called yet. Any `Notified` created | |
| // before the invocation of `notify_waiters()` will be notified by it even if | |
| // `await` has not been called on it yet. | |
| // Build the `Notified` while holding the read lock. `notified_owned()` records tokio's | |
| // `notify_waiters_calls` counter at construction and completes on first poll if that | |
| // counter has since advanced. Reading the counter under the lock guarantees it is taken | |
| // before `insert_equality_delete` can advance it via `notify_waiters()`, so the | |
| // notification is never missed even though we `.await` after releasing the lock. |
There was a problem hiding this comment.
My bad. Your suggestion looks a bit better, updated the PR with it
|
@mbutrovich Thanks for the quick review!
Created the issue #3274 and also the fix PR: #3275 I have also updated the comments as per your suggestion in this PR |
…te_file_path The equality-delete loader signals completion with Notify::notify_waiters(), which stores no permit and only wakes Notified futures created before the call. get_equality_delete_predicate_for_delete_file_path observed the Loading state, released the read lock, and only then created the Notified future; a notify_waiters() fired in that window is lost and the await hangs forever. Create the Notified (via notified_owned) while still holding the read lock, so it is guaranteed to precede any notify_waiters() and be delivered. Signed-off-by: Dhruv Arya <[email protected]>
…lost wakeup State the mechanism rather than the outcome: notified_owned() snapshots tokio's notify_waiters_calls counter at construction, and holding the read lock orders that snapshot before insert_equality_delete can advance it. This keeps a later refactor from moving notified_owned() out from under the lock unnoticed. Signed-off-by: Dhruv Arya <[email protected]>
118dda4 to
c746f98
Compare
mbutrovich
left a comment
mbutrovich
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround, @dhruvarya-db! The comment now explains why the Notified has to be built under the read lock, the error-path hang has its own issue (#3274) and fix (#3275), and @fallintoplace agreed that this PR lands before #2630. I have nothing further on this one.
|
Hi @blackmwk / @laskoviymishka , can one of you please help merge this PR? Thanks! |
laskoviymishka
left a comment
laskoviymishka
left a comment
There was a problem hiding this comment.
LGTM! Thanks @dhruvarya-db and @mbutrovich
…lity-delete hang branch-1.1 pins iceberg-rust at bb1e4a4, where a native Iceberg scan can wait forever while loading an equality delete file. The reader released the state lock before creating its Notified future, so a notify_waiters() that fired in that gap was lost (apache/iceberg-rust#3268). main has had the fix since apache#6649 moved the pin to af1da4c. Take the pin move and the two API updates it needs from apache#6649, without its spark.comet.iceberg.ioTimeout config.
Revives #2873, which was auto-closed by the stale bot on 2026-09-01. Same fix, rebased onto current
main. The review from #2873 is being carried over — see the note at the bottom.Which issue does this PR close?
N/A — a latent lost-wakeup bug in the equality-delete loading path.
What changes are included in this PR?
Fixes a lost-wakeup hang in
DeleteFilter::get_equality_delete_predicate_for_delete_file_path(crates/iceberg/src/arrow/delete_filter.rs).tokio::sync::Notify::notify_waiters()stores no permit and only wakesNotifiedfutures that already exist. Previously the consumer observed theLoadingstate, released the read lock, and only then created itsNotified. The losing interleaving:EqDelState::Loading(notify).Loading, clones theArc<Notify>, and releases the read lock.insert_equality_delete) transitions the entry toLoadedand callsnotify_waiters(). NoNotifiedexists yet, so the wakeup is dropped.notify.notified().await, creating itsNotifiedafter step 3 — it waits for the nextnotify_waiters(), which never comes → hangs forever.Fix: create the
Notified(vianotified_owned) while still holding the read lock.notified_owned()snapshots tokio's internalnotify_waiters_callscounter at construction and completes on first poll if that counter has since advanced; reading it under the read lock guarantees the snapshot is taken beforeinsert_equality_deletecan advance it vianotify_waiters()(which needs the write lock). So the notification is never missed even though we.awaitafter releasing the lock.This is latent today (the loader does real work before signaling, so the window is rarely hit) but is one scheduling hiccup from a permanent hang. Same bug class as #2696 (
DeleteFileIndex) and #2859 (positional deletes).Are these changes tested?
A deterministic regression test is possible, but it requires a small
#[cfg(test)]timing seam in the consumer: unlike the positional-delete path (#2859), this method does not hand the notifier back to its caller, so the losing interleaving cannot be forced through the public API alone. To keep this production change minimal and seam-free, the seam + tests live in companion PRs on my fork:I'm happy to fold the seam + regression test into this PR if maintainers prefer.
Carried-over review from #2873
notify_waiters_callscounter + the lock's role), per the review suggestion on fix: avoid lost-wakeup hang in get_equality_delete_predicate_for_delete_file_path #2873.try_start_eq_del_load/insert_equality_deletenotify_A / notify_B double-mint raised on fix: avoid lost-wakeup hang in get_equality_delete_predicate_for_delete_file_path #2873 is a separate lost-wakeup already fixed by the approved-but-unmerged fix(reader): avoid deadlock when loading equality deletes #2630; it composes with this fix and should not be re-fixed here. Happy to coordinate ordering.RecvErrorpanic that can leave an entry stuckLoading(raised on fix: avoid lost-wakeup hang in get_equality_delete_predicate_for_delete_file_path #2873) is pre-existing and only reachable if aDeleteFilteris shared across concurrentload_deletescalls; I'll follow up separately.AI Disclosure
The code was written using Claude but was manually reviewed by me.