dhruvarya-db opened a new pull request, #2696: URL: https://github.com/apache/iceberg-rust/pull/2696
## Which issue does this PR close? N/A — concurrency bug found while reviewing #2590. ## What changes are included in this PR? Fixes a lost-wakeup hang in `DeleteFileIndex::get_deletes_for_data_file`. ## The bug `DeleteFileIndex` populates asynchronously on a background task that signals completion with `tokio::sync::Notify::notify_waiters()`. Per the tokio docs, `notify_waiters()` stores no permit and only wakes `Notified` futures that have already been **created**. `get_deletes_for_data_file` observes the `Populating` state, **releases the read lock**, and only *then* creates the `Notified` future. If the populating task flips the state to `Populated` and calls `notify_waiters()` in that window, the wakeup is lost and the query awaits forever. The window is normally only a few instructions wide and is effectively never hit, because the querier usually registers before the populator is even scheduled. This is reachable from the `SnapshotValidator` commit path (apache/iceberg-rust#2590), which drops the channel sender immediately before querying — making the populator finish right as the query starts. ## The fix Create the `Notified` future (via `notified_owned`) **while still holding the read lock**. The populating task cannot call `notify_waiters()` until it acquires the write lock, which is blocked while the read lock is held — so the notification is guaranteed to happen *after* the future has been created, and tokio guarantees such notifications are delivered. No re-check needed. The same latent pattern exists in `arrow/delete_filter.rs` and `arrow/caching_delete_file_loader.rs`; this PR fixes only the index site, and I'm happy to follow up on the others. ## Are these changes tested? The change is verified by a deterministic reproduction that uses a small test-only seam to make the otherwise nanosecond-wide window reliable. To keep this production change minimal, the seam + test live in companion PRs on my fork rather than here: - Reproduction only — the test **fails** (hangs → timeout), demonstrating the bug: https://github.com/dhruvarya-db/iceberg-rust/pull/9 - The same fix **with** the regression test — the test **passes**: https://github.com/dhruvarya-db/iceberg-rust/pull/10 I'm happy to fold a regression test directly into this PR if maintainers prefer. This pull request and its description were written by Isaac. -- 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]
