Skip to content

fix(io): create the spill temp directory on first use, not in Session::new - #8989

Open
LuciferYang wants to merge 5 commits into
lance-format:mainfrom
LuciferYang:fix/lazy-spill-store
Open

fix(io): create the spill temp directory on first use, not in Session::new#8989
LuciferYang wants to merge 5 commits into
lance-format:mainfrom
LuciferYang:fix/lazy-spill-store

Conversation

@LuciferYang

@LuciferYang LuciferYang commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

What this changes

LocalSpillStore creates its scratch directory on the first new_spill call 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. new and with_cap keep their Result signatures and now do no I/O, and Default no longer panics. Closes #8948.

Why

Session::new builds LocalSpillStore::default() (rust/lance/src/session.rs:130 and :150), and that Default impl was Self::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 a Default impl, where no caller can handle it. On Windows CI runners it surfaces as Access is denied in 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 last Arc to it: MemorySpill::drop deletes 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: /tmp that is briefly full, or an EMFILE, does not pin the process to memory. The warning fires once per store, with a debug! on each later failure.

Reclamation on that arm cannot stat a file, so the reservation is a Reservation holding the quota and a counter of the bytes the inner writer accepted, released by its own Drop. The writer, the Spill and every reader it hands out each hold an Arc of it, so the bytes leave the budget with the last owner of the backing rather than with the handle: a reader retained past its Spill keeps them charged, which is what makes with_cap an 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 every Reader method and exists only to own that Arc. Three of those methods hand out something that owns the backing and outlives the reader, so each carries its own share: get_range returns a 'static future, and get_stream and get_range_stream return 'static streams.

What this costs, since the fallback is not free

An uncapped store trades a hard failure for memory pressure: all three Session constructors 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. With with_cap the budget does bound it, but it then bounds memory rather than disk while the error is still Error::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 NamedTempFile and 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_factory is a field, so the failing path is drivable without a host whose temp directory is actually unusable.

  • test_construction_resolves_no_backing: new, default and with_cap all leave the OnceLock empty
  • test_first_spill_resolves_the_disk_backing
  • test_memory_backing_round_trips
  • test_memory_backing_rejects_a_reader_before_shutdown, asserting the InvalidInput variant
  • test_memory_backing_releases_the_quota_on_drop, with a second spill alive across the drop so an over-release shows up
  • test_a_reader_outlives_the_memory_spill_handle
  • test_a_retained_reader_stays_charged_to_the_cap, which is a reproducer from the review on this pull request
  • test_a_detached_reader_product_stays_charged_to_the_cap, three cases, one per API that hands out a detached product, also from the review
  • test_a_transient_temp_dir_failure_is_retried

Eight mutations, each run: restoring eager resolution in new() fails test_construction_resolves_no_backing; deleting the quota release fails the quota test; releasing u64::MAX instead of the counter also fails it, at left: 0 right: 10; reintroducing the spawned delete in Drop fails test_a_reader_outlives_the_memory_spill_handle; caching the failure again fails test_a_transient_temp_dir_failure_is_retried; putting the release back on MemorySpill::drop fails test_a_retained_reader_stays_charged_to_the_cap at left: 0 right: 40; and dropping the share from get_range's future or from get_stream's stream fails only that API's case. Counting buf.len() instead of the accepted bytes fails test_spill_writer_releases_unaccepted_bytes, which is why that test now passes a reservation in.

cargo test --profile ci -p lance-io --lib is 315 passed, and cargo test --profile ci -p lance --lib -- session is 22 passed. cargo fmt --all -- --check and cargo clippy --profile ci -p lance-io --all-targets -- -D warnings are 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 OnceLock race is reasoned about rather than stress-tested.

@github-actions github-actions Bot added A-encoding Encoding, IO, file reader/writer bug Something isn't working labels Sep 4, 2026
lance-gatekeeper[bot]

This comment was marked as outdated.

@lance-gatekeeper lance-gatekeeper Bot added the K-changes Latest Gatekeeper recommendation requests changes. label Sep 4, 2026
@LuciferYang

Copy link
Copy Markdown
Contributor Author

The red mac-build (stable) is not this diff. It fails in dataset::mem_wal::scanner::vector_search::tests::test_vector_search_prefilter_blocks_base_when_active_newest_fails with CorruptFile ... shuffle_offsets.lance: final offset 2 does not match shuffle data row count 6, raised by validate_shuffle_offsets in rust/lance-index/src/vector/v3/shuffler.rs. That path never reaches lance_io::spill: the only users of the SpillStore trait are Session, which holds and exposes it, and its two unit tests, so no index build touches the code this pull request changes. The shuffler has its own temp directory, and its offsets file is a fixed name inside it (shuffler.rs:636), which is where I would look first. The other 35 checks are green, including every other Rust job.

@lance-gatekeeper lance-gatekeeper Bot removed the K-changes Latest Gatekeeper recommendation requests changes. label Sep 4, 2026
@LuciferYang

Copy link
Copy Markdown
Contributor Author

The red Build and Test with Java 25 is not this diff. It fails to compile lance-jni with error[E0119]: conflicting implementations of trait IntoJava for type &lance_table::format::BasePath, and both impls are on main: java/lance-jni/src/blocking_dataset.rs:89 and java/lance-jni/src/transaction.rs:125, the second added by #8925. main itself is red on that workflow at e728b5384 (run 33884542905) and was green on the four commits before it. This pull request changes one file, rust/lance-io/src/spill.rs, and touches no trait and nothing under java/.

lance-gatekeeper[bot]

This comment was marked as outdated.

@lance-gatekeeper lance-gatekeeper Bot added the K-changes Latest Gatekeeper recommendation requests changes. label Sep 4, 2026
@LuciferYang

Copy link
Copy Markdown
Contributor Author

Same cause for the red Rust Clippy and Fmt Check: its Rust Clippy step stops at the identical E0119 while checking lance-jni, not at anything in this diff.

@lance-gatekeeper lance-gatekeeper Bot added K-changes Latest Gatekeeper recommendation requests changes. and removed K-changes Latest Gatekeeper recommendation requests changes. labels Sep 4, 2026
@lance-gatekeeper lance-gatekeeper Bot removed the K-changes Latest Gatekeeper recommendation requests changes. label Sep 4, 2026
lance-gatekeeper[bot]

This comment was marked as outdated.

@lance-gatekeeper lance-gatekeeper Bot added the K-changes Latest Gatekeeper recommendation requests changes. label Sep 4, 2026
@lance-gatekeeper lance-gatekeeper Bot removed the K-changes Latest Gatekeeper recommendation requests changes. label Sep 4, 2026
lance-gatekeeper[bot]

This comment was marked as outdated.

@lance-gatekeeper lance-gatekeeper Bot added the K-changes Latest Gatekeeper recommendation requests changes. label Sep 4, 2026
@lance-gatekeeper lance-gatekeeper Bot removed the K-changes Latest Gatekeeper recommendation requests changes. label Sep 5, 2026

@lance-gatekeeper lance-gatekeeper Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Gate recommendation: approve with a non-blocking risk.

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.

@lance-gatekeeper lance-gatekeeper Bot added K-approved Latest Gatekeeper recommendation permits acceptance. K-risk Latest Gatekeeper recommendation includes a non-blocking risk. labels Sep 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-encoding Encoding, IO, file reader/writer bug Something isn't working K-approved Latest Gatekeeper recommendation permits acceptance. K-risk Latest Gatekeeper recommendation includes a non-blocking risk.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: Session::new eagerly creates a LocalSpillStore temp dir and panics when it cannot

1 participant