dhruvarya-db opened a new pull request, #3268: URL: https://github.com/apache/iceberg-rust/pull/3268
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 wakes `Notified` futures that already exist. Previously the consumer observed the `Loading` state, released the read lock, and only then created its `Notified`. The losing interleaving: 1. The entry is `EqDelState::Loading(notify)`. 2. The consumer sees `Loading`, clones the `Arc<Notify>`, and **releases the read lock**. 3. The loader (`insert_equality_delete`) transitions the entry to `Loaded` and calls `notify_waiters()`. No `Notified` exists yet, so the wakeup is dropped. 4. The consumer calls `notify.notified().await`, creating its `Notified` **after** step 3 — it waits for the next `notify_waiters()`, which never comes → hangs forever. **Fix:** create the `Notified` (via `notified_owned`) while still holding the read lock. `notified_owned()` snapshots tokio's internal `notify_waiters_calls` counter at construction and completes on first poll if that counter has since advanced; reading it under the read lock guarantees the snapshot is taken before `insert_equality_delete` can advance it via `notify_waiters()` (which needs the write lock). So the notification is never missed even though we `.await` after 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: - Reproduction only — the test **fails** (hangs → timeout), demonstrating the bug: https://github.com/dhruvarya-db/iceberg-rust/pull/17 - The same fix **with** the regression test (using the seam) — the test **passes**: https://github.com/dhruvarya-db/iceberg-rust/pull/14 I'm happy to fold the seam + regression test into this PR if maintainers prefer. ## Carried-over review from #2873 - The comment now states the *why* (the `notify_waiters_calls` counter + the lock's role), per the review suggestion on #2873. - The `try_start_eq_del_load` / `insert_equality_delete` notify_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 error-path `RecvError` panic that can leave an entry stuck `Loading` (raised on #2873) is pre-existing and only reachable if a `DeleteFilter` is shared across concurrent `load_deletes` calls; I'll follow up separately. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
