fwojciec opened a new issue, #24228: URL: https://github.com/apache/datafusion/issues/24228
### Describe the bug `DiskManager::used_disk_space` is a process-wide counter. Files add to it while they write, and `RefCountedTempFile::Drop` subtracts each file's recorded `current_file_disk_usage` when the file dies. Two error paths add to the global counter but return before they record the same amount on the per-file counter. `Drop` then subtracts less than was added, and the global counter stays inflated for the rest of the process lifetime. Later spills see less headroom than they really have, and each further trip inflates the counter more — under a temp-size cap this ratchets toward a state where every spill fails. **Instance A — `main` (unreleased), `FileSpillWriter::write`** ([disk_manager.rs#L485-L510 @ a942c0b](https://github.com/apache/datafusion/blob/a942c0b/datafusion/execution/src/disk_manager.rs#L485-L510)): The quota-trip path is correct: it does `fetch_sub(len)` before it returns the error. The `write_all` failure path is not: the function returns after the global `fetch_add(len)` and before `current_file_disk_usage.fetch_add(len)`, so the reservation is never released. **Instance B — released 54.1.0 / `branch-54`, `RefCountedTempFile::update_disk_usage`** ([disk_manager.rs#L401-L434 @ 54.1.0](https://github.com/apache/datafusion/blob/54.1.0/datafusion/execution/src/disk_manager.rs#L401-L434)): The quota check returns its `resources_err!` between the global `fetch_add(new_disk_usage)` (L415-L417) and the per-file `store(new_disk_usage)` (L430-L431). Every quota trip leaks `new_disk_usage − old_disk_usage` bytes on the global counter. This is the instance we hit in production (DataFusion 54.1.0, spill-heavy workload behind `max_temp_directory_size`): after a quota trip, subsequent queries got a reduced effective cap. The refactor in #21882 removed this code path on `main`, which fixes this instance for the next release, but every released version that has `update_disk_usage` carries it. ### To Reproduce Instance A: construct a `FileSpillWriter` over a read-only file handle so `write_all` fails, write once, and read `used_disk_space()` — it stays above the baseline. The linked PR adds this as a regression test. Instance B (54.1.0): with a small `max_temp_directory_size`, write a spill file past the cap so `update_disk_usage` returns the resources error, drop the file, and read `used_disk_space()` — it does not return to the baseline. Our downstream regression test observed exactly the expected `new − old` delta leak. ### Expected behavior Any error path that has already added to `used_disk_space` either releases the reservation before returning, or records the same amount on `current_file_disk_usage` so `Drop` reconciles it. The counter returns to its baseline after the file is dropped, in every outcome. ### Additional context - Fix for instance A: mirror the existing quota-path rollback in the `write_all` failure arm (PR incoming, with a regression test). - Fix shape for instance B, if another 54.x patch release is cut from `branch-54`: move the `current_file_disk_usage.store(new_disk_usage)` above the quota check, so the global add and the per-file record can never be split by an early return. We run 54.1.0 with exactly that one-statement reorder as a vendored patch and can confirm it holds under a spill-heavy production workload. -- 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]
