lance-format / lance-format/lance

bug: the scratch dir leak tests can pass without checking anything

Open
#9,283 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug
Dominant language
Rust
Stars
7.1k
Forks
852
Avg merge
3d 18h
Merged PRs (30d)
272

Description

Problem

Two tests exist to catch a leaked scratch directory: test_hnsw_pq_scratch_dir_is_not_leaked for the legacy IVF_HNSW partition staging dir, and test_shuffle_scratch_dir_is_not_leaked (added in #8993) for the shuffle scratch dir. Both work the same way: re-run the build in a child process with the child's temp dir pointed at an isolated directory the parent owns, then scan that directory for survivors.

Both are unreliable in ways that produce a green result rather than a failure.

They check nothing on Windows. The redirect sets only TMPDIR. std::env::temp_dir() reads TMPDIR on POSIX, but on Windows it reads TMP, then TEMP, then USERPROFILE. So on a Windows runner the child stages into the real temp directory, the parent scans a directory that holds nothing but the child's dataset, and the assertion passes without having looked at anything. A reintroduced leak stays green there, and the failure mode is invisible: a leak and a fix produce the same result.

The legacy test can go green permanently after a rename. Its child is selected by a hardcoded test-path string, and the parent's only gate is the child's exit code. libtest exits 0 when --exact matches nothing, so renaming the child test, moving it, or renaming LANCE_HNSW_LEAK_TEST_ROOT on one side makes the filter match nothing, the child exit 0 without doing anything, and the parent scan an untouched directory. test_shuffle_scratch_dir_is_not_leaked closed this by deriving the filter from std::thread::current().name() and asserting the child's dataset directory exists; the legacy test has neither guard.

What should change

Redirect all three variables, and have the child prove the redirect took effect rather than trusting it, so a host where the environment cannot be redirected fails loudly instead of reporting a clean scan. Give the legacy parent the same proof-of-work check the newer test has.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with the Rust tests named test_hnsw_pq_scratch_dir_is_not_leaked and test_shuffle_scratch_dir_is_not_leaked, then run both leak tests on POSIX and Windows where possible. Check how the child process receives its temporary-directory environment and how each parent verifies child work. Done means all three variables are redirected, the child proves the redirect took effect, and the legacy test rejects a non-matching filter.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
testing-qa
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
75/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.