Skip to content

fix(index): clean up IVF shuffler temp dir after index build#6957

Open
geserdugarov wants to merge 2 commits into
lance-format:mainfrom
geserdugarov:fix-clean-tmp
Open

fix(index): clean up IVF shuffler temp dir after index build#6957
geserdugarov wants to merge 2 commits into
lance-format:mainfrom
geserdugarov:fix-clean-tmp

Conversation

@geserdugarov
Copy link
Copy Markdown

Summary

Fixes a temporary directory leak in IVF index shuffling by ensuring auto-created shuffle directories are
cleaned up after index build work is complete.

Changes

  • Keep ownership of auto-created IVF shuffler temp directories with a TempDir guard instead of leaking the
    path.
  • Store the temp-dir guard on IvfShuffler when no caller-provided output directory is used.
  • Keep the guard alive across returned shuffle streams so files are not removed before consumers finish
    reading them.
  • Preserve existing behavior for caller-provided output directories, where cleanup remains the caller’s
    responsibility.
  • Add a regression test that verifies the auto-created temp directory is removed after the shuffler and
    streams are dropped.

Testing

  • cargo test -p lance-index vector::ivf::shuffler::test::test_shuffler_cleans_up_auto_temp_dir

Copy link
Copy Markdown

@claude claude Bot left a comment

Choose a reason for hiding this comment

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

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@github-actions github-actions Bot added the bug Something isn't working label May 27, 2026
@geserdugarov
Copy link
Copy Markdown
Author

@Xuanwo, @wjones127, hi!
If you don't mind, could you please review this PR?
You've reviewed a lot of PRs related to indexing, so I hope this one is also in your area of interest.

Copy link
Copy Markdown
Contributor

@wjones127 wjones127 left a comment

Choose a reason for hiding this comment

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

Nice work. I appreciate the test. I made an optional suggestion to improve readability. Otherwise, I'm happy to merge this once CI is green.

Comment thread rust/lance-index/src/vector/ivf/shuffler.rs Outdated
@codecov
Copy link
Copy Markdown

codecov Bot commented May 27, 2026

Codecov Report

❌ Patch coverage is 92.45283% with 4 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
rust/lance-index/src/vector/ivf/shuffler.rs 92.45% 1 Missing and 3 partials ⚠️

📢 Thoughts on this report? Let us know!

@geserdugarov
Copy link
Copy Markdown
Author

geserdugarov commented May 28, 2026

Thanks! I applied the on_drop suggestion. CI run is green.

Co-authored-by: Will Jones <willjones127@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants