fix: avoid lost-wakeup hang in get_equality_delete_predicate_for_delete_file_path#2873
Conversation
…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 <aryadhruv@gmail.com>
mbutrovich
left a comment
mbutrovich
left a comment
There was a problem hiding this comment.
Thanks for working on these scheduling issues, @dhruvarya-db! The fix is correct. Building the Notified while the read lock is held closes the window where a waiter could miss the notification and park forever. This matches the shape already merged in delete_file_index.rs (#2696) and proposed for positional deletes (#2859), so the three delete-loading paths now agree.
Two things are worth resolving before this lands. Both were already raised on #2859.
| // 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 new comment explains the outcome (a Notified created before notify_waiters() still fires) but not the reason. The reason is specific: notified_owned() records tokio's internal notify_waiters_calls counter at construction, and on its first poll it completes if that counter has advanced. Holding the read lock is what guarantees the counter is read before the loader can advance it.
Stating that keeps the code safe against a later refactor that moves notified_owned() out from under the lock, which would look harmless but would reintroduce the hang. The same point was made on #2859: #2859 (comment)
Suggested wording:
// 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.
| *entry.lock().unwrap() |= delete_vector; | ||
| } | ||
|
|
||
| pub(crate) fn insert_equality_delete( |
There was a problem hiding this comment.
Also line 112 (try_start_eq_del_load) above. The loader claims the entry with try_start_eq_del_load, which inserts EqDelState::Loading(notify_A) and returns notify_A. The caller drops notify_A unused and calls insert_equality_delete, which inserts a second EqDelState::Loading(notify_B) over the same key and only ever calls notify_waiters() on notify_B. A reader that observed the entry while it held notify_A waits on a notifier that is never signalled.
This is the same class of bug this PR fixes, and it survives the fix. It was flagged on #2859: #2859 (comment)
The window is small today (the two calls run back to back with no .await between them, and readers run after the load phase), so this is latent rather than active. Since it is the same root cause and the same file, closing it here rather than in a follow-up keeps the delete-loading paths consistent. insert_equality_delete can reuse the notifier returned by try_start_eq_del_load instead of minting and inserting a new one.
| let state = self.state.clone(); | ||
| let delete_file_path = delete_file_path.to_string(); | ||
| self.runtime.cpu().spawn(async move { | ||
| let eq_del = eq_del.await.unwrap(); | ||
| { | ||
| let mut state = state.write().unwrap(); | ||
| state | ||
| .equality_deletes | ||
| .insert(delete_file_path, EqDelState::Loaded(eq_del)); | ||
| } | ||
| notify.notify_waiters(); | ||
| }); |
There was a problem hiding this comment.
Not a blocker, just raising it while we are here. On the error path, parse_equality_deletes_record_batch_stream(...).await? returns before sender.send(predicate), dropping the sender. The spawned task's eq_del.await.unwrap() then panics on RecvError, so the Loaded insert and notify_waiters() never run and the entry stays Loading.
Within one load_deletes call this looks contained: the error propagates via item? at caching_delete_file_loader.rs:214, the call returns Err, and siblings are cancelled rather than parked. It looks like a real hang only if two load_deletes calls share one DeleteFilter (concurrent scans on the same loader): the first marks the file Loading, fails, never signals, and a second reader parks forever.
Two questions: is a shared DeleteFilter across concurrent loads a supported case? If so, would recording a failure state (or removing the entry so a reader retries) be preferable to leaving it Loading and panicking? Same shape as the positional path on #2859: #2859 (comment).
3 participants
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. The loader cannot callnotify_waiters()until it takes the write lock, which is blocked while the read lock is held, so the notification is guaranteed to happen after the future is created and is delivered.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. It is the 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 — the test must pause the consumer in the window between observingLoadingand registering itsNotified. 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 that over keeping the production change seam-free.