fix(io): create the spill temp directory on first use, not in Session::new - #8989
fix(io): create the spill temp directory on first use, not in Session::new#8989LuciferYang wants to merge 5 commits into
Conversation
|
The red |
|
The red |
|
Same cause for the red |
There was a problem hiding this comment.
The detached future and stream quota-lifetime gap is fixed: the reservation now follows all three detached products through completion, cancellation, or drop.
Default sessions still use an uncapped memory fallback, so a temp-directory failure can shift a large spill into process memory; capped stores bound logical live spill bytes rather than peak RSS. No further change is requested.
What this changes
LocalSpillStorecreates its scratch directory on the firstnew_spillcall rather than at construction, and a call that cannot create it keeps that one spill in memory instead, retrying the directory on the next call.newandwith_capkeep theirResultsignatures and now do no I/O, andDefaultno longer panics. Closes #8948.Why
Session::newbuildsLocalSpillStore::default()(rust/lance/src/session.rs:130and:150), and thatDefaultimpl wasSelf::new().expect("failed to create temp directory for LocalSpillStore"). Opening a dataset therefore created a temp directory whether or not anything would ever spill, and on a host where that fails the process aborted from inside aDefaultimpl, where no caller can handle it. On Windows CI runners it surfaces asAccess is deniedin tests that have nothing to do with spilling.This follows the direction wjones127 gave on the issue: detect the failure on first use, warn, and fall back to an in-memory mode.
The fallback, and why it is per spill rather than per store
Each fallback spill gets its own
ObjectStore::memory(). That backend holds exactly one object, so the bytes go with the lastArcto it:MemorySpill::dropdeletes nothing and spawns nothing, and a reader that still holds its own reference keeps the bytes readable after the handle is gone, which is the same shape as unlinking a file someone still has open. Residency is bounded by the last live handle rather than by the store's lifetime.Only success is cached in the
OnceLock, so a failure decides where one spill goes, not where every later spill goes:/tmpthat is briefly full, or anEMFILE, does not pin the process to memory. The warning fires once per store, with adebug!on each later failure.Reclamation on that arm cannot stat a file, so the reservation is a
Reservationholding the quota and a counter of the bytes the inner writer accepted, released by its ownDrop. The writer, theSpilland every reader it hands out each hold anArcof it, so the bytes leave the budget with the last owner of the backing rather than with the handle: a reader retained past itsSpillkeeps them charged, which is what makeswith_capan actual ceiling rather than a counter that a retained reader can walk away from. Handing a reader its share needs a wrapper,ChargedReader, which delegates everyReadermethod and exists only to own thatArc. Three of those methods hand out something that owns the backing and outlives the reader, so each carries its own share:get_rangereturns a'staticfuture, andget_streamandget_range_streamreturn'staticstreams.What this costs, since the fallback is not free
An uncapped store trades a hard failure for memory pressure: all three
Sessionconstructors build the uncapped store, so on a host with no usable temp directory a large spill goes to RAM with one warning as the signal. Withwith_capthe budget does bound it, but it then bounds memory rather than disk while the error is stillError::DiskCapExceeded, and at commit time the writer's part buffer and the assembled object coexist, so the real peak runs to roughly twice the figure the counter releases.Also worth naming: on the disk backing the release is best effort. A handle dropped before its writer has been shut down returns only what the stat can see at that moment, which is nothing, because the file is staged in a
NamedTempFileand only renamed into place at shutdown. That predates this change; the module doc now says so. The in-memory arm does not have that hole, since its release waits for the last owner.Test plan
Seven new tests in
rust/lance-io/src/spill.rs.temp_dir_factoryis a field, so the failing path is drivable without a host whose temp directory is actually unusable.test_construction_resolves_no_backing:new,defaultandwith_capall leave theOnceLockemptytest_first_spill_resolves_the_disk_backingtest_memory_backing_round_tripstest_memory_backing_rejects_a_reader_before_shutdown, asserting theInvalidInputvarianttest_memory_backing_releases_the_quota_on_drop, with a second spill alive across the drop so an over-release shows uptest_a_reader_outlives_the_memory_spill_handletest_a_retained_reader_stays_charged_to_the_cap, which is a reproducer from the review on this pull requesttest_a_detached_reader_product_stays_charged_to_the_cap, three cases, one per API that hands out a detached product, also from the reviewtest_a_transient_temp_dir_failure_is_retriedEight mutations, each run: restoring eager resolution in
new()failstest_construction_resolves_no_backing; deleting the quota release fails the quota test; releasingu64::MAXinstead of the counter also fails it, atleft: 0 right: 10; reintroducing the spawned delete inDropfailstest_a_reader_outlives_the_memory_spill_handle; caching the failure again failstest_a_transient_temp_dir_failure_is_retried; putting the release back onMemorySpill::dropfailstest_a_retained_reader_stays_charged_to_the_capatleft: 0 right: 40; and dropping the share fromget_range's future or fromget_stream's stream fails only that API's case. Countingbuf.len()instead of the accepted bytes failstest_spill_writer_releases_unaccepted_bytes, which is why that test now passes a reservation in.cargo test --profile ci -p lance-io --libis 315 passed, andcargo test --profile ci -p lance --lib -- sessionis 22 passed.cargo fmt --all -- --checkandcargo clippy --profile ci -p lance-io --all-targets -- -D warningsare clean.Not covered
Nothing here reproduces the Windows temp-directory failure itself; the fallback is reached by handing the factory an error. The capped in-memory arm is not exercised with a payload large enough to cross several internal part buffers, and the
OnceLockrace is reasoned about rather than stress-tested.