dhruvarya-db opened a new pull request, #2873: URL: https://github.com/apache/iceberg-rust/pull/2873
## Which issue does this PR close? <!-- No issue; a latent concurrency bug found while reviewing the delete-file loading paths. --> 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. The loader cannot call `notify_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 observing `Loading` and registering its `Notified`. 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 that over keeping the production change seam-free. -- 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]
