fix(io): sweep leaked temps for long destination filenames - #524
Conversation
`tmp_sibling` truncates the destination's basename when `base + suffix` would exceed NAME_MAX, but `sweep_stale_tmps` matched candidates with `name.strip_prefix(base)` using the untruncated basename. A truncated temp never starts with the full base, so past about 234 bytes the #299 reclaim was a permanent no-op: a writer killed between temp creation and rename left a full-size sibling that nothing would ever remove, and a crash-looping writer filled the volume — the exact failure the sweep exists to prevent. The sweep now splits each candidate at its last `.tmp.` and accepts the truncated form as well as the exact one. Truncated temps are identified precisely rather than by loose prefix matching: the stem must prefix the destination's basename *and* the whole name must land on NAME_MAX, which is the only length `tmp_sibling`'s cut produces. That keeps it from reaching a different destination's temps, and the existing `is_our_tmp_suffix` and mtime>1h guards are unchanged. The regression test plants the temp `tmp_sibling` would produce for a dead pid, backdates it, and does the first save to that destination — first, because the sweep runs once per destination per process and an earlier version of this test saved before planting, claimed the memo, and passed for the wrong reason. It covers both sides of the 234-byte flip, with the short name as a positive control that the harness sweeps at all, plus a negative control that an unrelated destination's temp survives. Verified failing without the fix on exactly the reported symptom. Closes #488 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
/review |
| #[repr(C)] | ||
| struct Tv { | ||
| sec: i64, | ||
| usec: i32, |
There was a problem hiding this comment.
Both new tests fail on Linux — utimes returns -1, so this PR's regression coverage never runs.
struct timeval's tv_usec is suseconds_t, which is long (i64) on 64-bit glibc, not i32. With usec: i32 the #[repr(C)] struct is still 16 bytes, but bytes 12..16 are uninitialised padding — and the kernel reads them as the high half of tv_usec. When they're non-zero the value falls outside [0, 999999] and utimes fails with EINVAL, tripping the assert_eq! in set_mtime.
Reproduced on this branch with cargo test -p turbovec --test io_hardening:
---- a_leaked_temp_is_swept_for_a_long_destination_name stdout ----
thread ... panicked at turbovec/tests/io_hardening.rs:1067:5:
assertion `left == right` failed
left: -1
right: 0
...
test result: FAILED. 36 passed; 2 failed
A standalone repro confirms the cause is the field width — errno is 22 (EINVAL) with usec: i32, and the same call succeeds with usec: i64. With only this one-word change, all 38 tests in the file pass, and reverting the io.rs hunk then makes a_leaked_temp_is_swept_for_a_long_destination_name fail on exactly the #488 assertion — so the fix itself is sound and this is the only thing standing between it and a green A/B.
i64 is also correct on macOS: tv_usec is 32-bit there, but since the value is 0 the extra high bytes just land on the struct's padding.
| usec: i32, | |
| usec: i64, |
| /// has to be planted BEFORE the first save to that path — saving first | ||
| /// claims the memo and the later save sweeps nothing, which is what made | ||
| /// an earlier version of this test pass for the wrong reason. | ||
| #[test] |
There was a problem hiding this comment.
The Windows CI leg will fail to compile. set_mtime is #[cfg(unix)], but neither new test is gated, and both call it — here and here. On windows-latest that's E0425: cannot find function set_mtime in this scope, which fails the whole test binary rather than just these two tests.
ci.yml runs cargo test -p turbovec --release --locked across [ubuntu-latest, macos-14, windows-latest], so this leg is a required gate.
Adding #[cfg(unix)] above both #[test] attributes matches the precedent already in this file (lines 408, 530, 537). If you'd rather keep Windows coverage, set_mtime needs a SetFileTime counterpart — but note the long-name case plants a 255-byte filename inside std::env::temp_dir(), which is close enough to Windows' MAX_PATH that it deserves its own check before enabling it there.
No suggestion block since the fix spans two locations.
|
🚫 Failed review — 2 validated finding(s) posted inline. Merging is blocked until the current head passes review: address the findings, then re-run it by commenting |
| // this from matching an unrelated destination that merely shares | ||
| // a prefix. | ||
| let ours = stem == base | ||
| || (base.starts_with(stem) && name.len() == TMP_NAME_MAX); |
There was a problem hiding this comment.
name.len() == TMP_NAME_MAX misses every truncated temp whose cut landed mid-character, so #488 survives for non-ASCII destination names.
tmp_sibling does not always emit a 255-byte name when it truncates. It takes the budget and then walks backwards to a char boundary:
Lines 1033 to 1040 in 6d9a28a
So whenever TMP_NAME_MAX - suffix.len() falls inside a multi-byte character, the emitted name is 1–3 bytes short of TMP_NAME_MAX, and this equality check rejects it. The comment above ("the cut was chosen to make the whole name land on NAME_MAX") only holds for ASCII.
Worked example — basename "a"*233 + "é" + "b"*17 + ".tv" (255 bytes), pid 99999, seq 7, suffix .tmp.99999.7.deadbeef (21 bytes):
cut = 255 - 21 = 234, but byte 234 is a UTF-8 continuation byte → walk back to233- leaked temp is
233 + 21 = 254bytes - sweep:
stem != base,base.starts_with(stem)✅,name.len() == 255❌ →ours = false, never reclaimed
I confirmed this end-to-end against the unmodified crate (a scratch binary calling turbovec::io::write, planting the aged temp before the first save so the SWEPT memo isn't pre-claimed): the multibyte leak survives, the ASCII control is swept. That is the same permanent no-op this PR is fixing, still present for any long destination whose basename has a multi-byte char straddling the cut. Not a narrow corner either — suffix.len() is 15 + pid_digits + seq_digits, so the cut position shifts with pid width and with the process-wide seq counter rolling over 10/100/…; for an all-3-byte-char basename it misses for 2 of every 3 pid/seq digit widths.
Suggested fix: instead of demanding an exact 255, reconstruct the cut tmp_sibling would have made and require the stem to equal it — compute budget = TMP_NAME_MAX - (suffix.len() + ".tmp.".len()), walk min(budget, base.len()) back to a base.is_char_boundary, and require stem.len() to be that value. That is exactly as tight as the current check for ASCII (it still pins a unique cut, so it can't reach an unrelated destination) while covering the walked-back case.
Both regression tests here are ASCII-only, and plant_then_save recomputes the stem as name[..NAME_MAX - suffix.len()], which hard-codes the boundary-aligned assumption — so the suite stays green through this. Worth a multibyte case alongside the 240-byte one.
|
🚫 Failed review — 1 validated finding(s) posted inline. Merging is blocked until the current head passes review: address the findings, then re-run it by commenting |
Three review findings, all correct. The sweep's `name.len() == TMP_NAME_MAX` test missed every truncation whose cut landed inside a multi-byte character: `tmp_sibling` walks *backwards* to a char boundary, so the emitted name is 1-3 bytes short of NAME_MAX and #488 survived for non-ASCII destinations. The suffix is known at match time, so the budget and therefore the cut are computable — the sweep now reproduces the same backwards walk instead of inferring it from a length. `set_mtime` declared `struct timeval`'s `tv_usec` as i32. That is right on macOS and wrong on 64-bit glibc, where `suseconds_t` is `long`: the four bytes of padding are read as the high half, the value leaves [0, 999999], and `utimes` fails with EINVAL — so both new tests would have failed on the Linux legs. Switched to `utimensat` with `struct timespec`, whose members are `long` everywhere, removing the platform split rather than encoding it. Neither test was `#[cfg(unix)]` while the helper was, so the Windows leg would not have compiled. Both are gated now. Added a non-ASCII case with the base offset by one byte so the 234-byte budget lands mid-character and the cut walks back to 232. My first attempt used 84 three-byte chars, where 234 is a boundary and the case never arose — the vacuity assert caught it. A/B'd: it fails against the old length check. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
/review |
| // 3-byte chars offset by one ASCII byte, so the 234-byte budget | ||
| // lands *inside* a character (boundaries sit at 1 + 3k) and the cut | ||
| // walks back to 232 — a 253-byte name, three short of NAME_MAX. | ||
| let name = format!("a{}.tv", "€".repeat(84)); |
There was a problem hiding this comment.
This test always fails: the destination filename is 256 bytes, one over NAME_MAX, so the save dies before the sweep is ever checked.
€ is 3 bytes, so a + 84×3 + .tv = 256 bytes — one past the 255-byte NAME_MAX the test itself declares four lines later. The leaked temp is fine (truncated to 253, and the < NAME_MAX assert passes), but the destination can never be created, so the final rename(tmp, dest) inside write_good_tv returns ENAMETOOLONG and unwraps.
Executed on this head — cargo test -p turbovec --test io_hardening:
---- a_leaked_temp_is_swept_for_a_long_non_ascii_destination_name stdout ----
thread '...' panicked at turbovec/tests/io_hardening.rs:70:69:
called `Result::unwrap()` on an `Err` value:
Os { code: 36, kind: InvalidFilename, message: "File name too long" }
test result: FAILED. 38 passed; 1 failed
It panics before reaching its own assert!(!leaked.exists()), so the mid-character-cut behaviour this test is named for — the src/io.rs change's whole reason for reproducing the cut instead of testing len == NAME_MAX — is currently never exercised. The other three new tests pass, and I confirmed the long-ASCII one is a genuine regression test.
83 gives a 253-byte destination and keeps every property the comment above claims: boundaries still sit at 1 + 3k, the 234-byte budget still lands mid-character, the cut still walks back to 232, and the temp is still 253 bytes — three short of NAME_MAX.
| let name = format!("a{}.tv", "€".repeat(84)); | |
| let name = format!("a{}.tv", "€".repeat(83)); |
|
🚫 Failed review — 1 validated finding(s) posted inline. Merging is blocked until the current head passes review: address the findings, then re-run it by commenting |
The base was 'a' + 84 x '€' + '.tv' = 256 bytes, one past NAME_MAX, so the destination could never be created and write_good_tv's rename unwrapped on ENAMETOOLONG before the sweep was reached. macOS counts characters and let it pass locally; Linux counts bytes and did not. 83 characters gives a 253-byte destination, still past the 234-byte truncation budget, and the cut still lands mid-character and walks back to 232 — which is the case this test exists for. Added an explicit assert on the destination length so the next adjustment cannot reintroduce it silently. Re-verified the test still discriminates: it fails against the old length-equality match. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
/review |
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
|
✅ Passed review — No defects found; full turbovec test suite passes including the 3 new tests. (run) |
…ames # Conflicts: # CHANGELOG.md
|
/review |
|
✅ Passed review — No defects found; truncation logic verified against tmp_sibling, tests pass. (run) |
The mutation gate found both halves of the boundary walk untestable, and it was right about both. `while cut > 0 && !base.is_char_boundary(cut)` — the `cut > 0` guard never decides anything, because `is_char_boundary(0)` is always true and ends the loop on its own. Mutating `>` to `>=` therefore produces an identical function, and no test can separate them. Inverting the body's `cut -= 1` to `+= 1` walks past `len`, where `is_char_boundary` keeps returning false, so that mutant spins until the harness kills it. Both are properties of the loop rather than gaps in the tests, so this replaces the loop with the search it was really performing: `(0..=want).rev().find(|&c| base.is_char_boundary(c))`. Same cut, no decrement to invert, no redundant guard. Verified the two lines now generate no mutants at all, and re-ran the function: the sixteen in-diff mutants are still caught. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
/review |
…ames # Conflicts: # CHANGELOG.md
|
🚫 Failed review — the review run did not complete, so this PR is unreviewed. Merging is blocked until the current head passes review: address the findings, then re-run it by commenting |
|
/review |
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
|
✅ Passed review — No defects found; fix verified against tmp_sibling and tests pass. (run) |
Adversarial review found this PR deleting a file it does not own, which
is worse than the leak it set out to reclaim.
The truncation branch matched any entry whose stem equalled the
destination's basename cut to the NAME_MAX budget. That cut prefix is a
legal filename in its own right, so the name is byte-identical to the
temp that a *shorter* destination -- one whose whole basename is that
prefix -- creates without any truncation at all. Both land on NAME_MAX
by construction, so neither the length nor any other part of the name
separates them.
Demonstrated: destination `x*237.tv` (240 bytes), a separate index
`x*234`, and that index's own 255-byte temp. One save to the long
destination removed the other index's temp. The sweep's own doc comment
notes Python's atomic_save stages real indexes under `<dest>.tmp....`
names, so the file taken can be a live staged index rather than a leak.
The ambiguity is inherent, so this resolves it by ownership rather than
by name: if a destination named exactly that stem exists, the file is
more plausibly its write than our leak, and we leave it. Its own writer
sweeps it on that destination's first save, and failing to reclaim a
leak is survivable where deleting a live stage is not. Also requires
`cut < base.len()`, since an untruncated match is branch one's job.
The regression test carries a control: with no rival destination present
the same planted name must still be swept. Without it the case would
rest on the test's own NAME_MAX copy and could pass vacuously. Verified
it fails without the guard ("the sweep deleted a shorter destination's
temp") and that both #488 sweep tests, ASCII and non-ASCII, still pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
/review |
| ); | ||
| } | ||
|
|
||
| #[test] |
There was a problem hiding this comment.
a_leaked_temp_is_swept_for_a_long_non_ascii_destination_name is the only one of the four new sweep tests without #[cfg(unix)], but it calls set_mtime, which is gated:
turbovec/turbovec/tests/io_hardening.rs
Lines 1147 to 1150 in 16495b8
On Windows set_mtime is not compiled, so the call at line 1121 fails to resolve (E0425: cannot find function set_mtime in this scope) and the io_hardening test binary does not build. The Rust CI matrix includes windows-latest running cargo test -p turbovec --release --locked, so this breaks that leg.
| #[test] | |
| #[cfg(unix)] | |
| #[test] |
|
🚫 Failed review — 1 validated finding(s) posted inline. Merging is blocked until the current head passes review: address the findings, then re-run it by commenting |
Two review/CI failures, both mine. Windows: inserting the new test at a computed anchor put it directly after `a_leaked_temp_is_swept_for_a_long_non_ascii_destination_name`'s attributes, so my test adopted that test's `#[cfg(unix)]` and the rewrite that followed sliced away both the attribute and the test's doc comment. The non-ASCII test calls `set_mtime`, which is itself `#[cfg(unix)]`, so on Windows the call no longer resolved and the whole `io_hardening` binary failed to build -- taking the windows-latest leg down with it. Attribute and doc comment restored, and I audited all four `set_mtime` call sites: every one is now inside a unix-gated test. Mutation gate: `cut < base.len()` -> `<=` survived, correctly. At equality `stem` is the entire basename, which the first branch (`stem == base`) has already matched and short-circuited on, so the comparison cannot change the result and no test can distinguish the two. It was redundant rather than undertested, so it is gone instead of being chased with a test. Re-verified the ownership guard is still what does the work: dropping the `.exists()` check fails the new test, and the full suite is green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
/review |
| // A truncated temp is recognisable exactly: its stem is a prefix | ||
| // of the destination's basename, and the cut was chosen to make | ||
| // the whole name land on NAME_MAX. Requiring that length keeps | ||
| // this from matching an unrelated destination that merely shares | ||
| // a prefix. |
There was a problem hiding this comment.
This paragraph describes a check the PR does not implement, and the paragraph directly below it says the opposite.
ours never tests the candidate's length. It recomputes tmp_sibling's cut and compares stem == &base[..cut] — precisely because a cut landing mid-character yields a name 1–3 bytes short of NAME_MAX. The new non-ASCII test asserts exactly that, and it passes:
turbovec/turbovec/tests/io_hardening.rs
Lines 1120 to 1124 in 114e2c7
So "the cut was chosen to make the whole name land on NAME_MAX. Requiring that length keeps this from matching an unrelated destination that merely shares a prefix" is not what the shipped code does — nothing here requires that length, and the next paragraph explicitly rejects an equality-on-length test as the thing that would leave #488 alive for non-ASCII names. What actually keeps the match off an unrelated destination is the exact-cut reproduction plus the !….exists() guard below.
Suggest dropping the paragraph; the one that follows already carries the rationale in its corrected form.
The same superseded claim is published in the CHANGELOG entry — "identified precisely (a stem that prefixes the destination's basename, on a name that lands exactly on NAME_MAX)":
Lines 24 to 28 in 114e2c7
As written, the release notes say a truncated temp is recognised only when it lands exactly on NAME_MAX, which would mean long non-ASCII destinations still leak — the opposite of what this PR fixes.
| /// A truncation whose cut lands inside a multi-byte character emits a | ||
| /// name 1-3 bytes short of NAME_MAX, because `tmp_sibling` walks back to | ||
| /// a char boundary. Matching on `len == NAME_MAX` missed exactly those, | ||
| /// so #488 survived for non-ASCII destination names. |
There was a problem hiding this comment.
This doc comment is a verbatim copy of the one on a_leaked_temp_is_swept_for_a_long_non_ascii_destination_name below, and it describes that test rather than this one:
turbovec/turbovec/tests/io_hardening.rs
Lines 1089 to 1097 in 114e2c7
This test plants no multi-byte name and never exercises a mid-character cut — it covers the rival-destination case, which the in-body comment already describes correctly.
| /// A truncation whose cut lands inside a multi-byte character emits a | |
| /// name 1-3 bytes short of NAME_MAX, because `tmp_sibling` walks back to | |
| /// a char boundary. Matching on `len == NAME_MAX` missed exactly those, | |
| /// so #488 survived for non-ASCII destination names. | |
| /// A long destination's *truncated* temp is byte-identical to the temp a | |
| /// shorter destination — one whose whole basename is that prefix — | |
| /// creates untruncated, so the name alone cannot separate them. When | |
| /// that shorter destination exists, the file is its live staged index | |
| /// rather than our leak, and the sweep must leave it alone. |
|
🚫 Failed review — 2 validated finding(s) posted inline. Merging is blocked until the current head passes review: address the findings, then re-run it by commenting |
The ubuntu leg failed on a_leaked_temp_is_swept_for_a_long_non_ascii_ destination_name while macOS and Windows passed, which looks like a platform bug and is not one. The sweep is best-effort by design: claim_first_sweep takes SWEPT with try_lock, not lock, so that a fork landing mid-hold cannot leave the child with a locked mutex and hang its first write. Declining just skips an opportunistic sweep. That makes any concurrent save able to turn a sweep assertion into a failure for a reason unrelated to the sweep's logic. io_hardening runs ~40 tests in parallel and many of them save, so this is a live race, not a Linux quirk: running that suite 40 times here reproduced a spurious failure, and re-running it surfaced a *different* sweep test (the_sweep_spares_a_shorter_destinations_temp_that_looks_truncated), which is the signature of contention rather than of any one test. SWEPT is a process-global static and each integration test file is its own process, so move the five sweep tests into their own binary. The only saves in that process are these tests, and a file-local mutex keeps even those from overlapping. The product code is untouched — the tests stop competing for the lock rather than the lock changing. Verified: 60 consecutive runs of both binaries, zero failures, against roughly 1 in 40 before. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
/review |
|
✅ Passed review — No defects found; sweep matcher matches tmp_sibling truncation, tests pass. (run) |
The previous commit fixed the race but left the invariant as a rule each test had to remember: take SERIAL, then save. One future test that saves without it brings the flakiness back, and nothing would catch that until CI went red on an unrelated PR. Move the guard into the two save helpers instead — write_good_tv and a new save_index for the one test that writes a real TurboQuantIndex — and take it out of the tests. Saving is now the only thing that locks, and both ways to save from this file lock. `serial` stays private and documents why it must not be taken twice on one thread (Mutex is not reentrant, which is exactly what taking it in both a test and a helper would do). Serializing the save alone is sufficient: contention only matters for the instant claim_first_sweep tries the lock, which is inside the save. 40 runs of both binaries, zero failures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
/review |
|
✅ Passed review — Sweep matcher reproduces tmp_sibling truncation exactly; full suite green. (run) |
#524 and #532 landed. CHANGELOG was the only textual conflict — both sides append bullets to the same Unreleased "Fixed" list, so both stay. #475 (from #532) was a semantic conflict the merge could not see: an add now drops the packed rows at its commit point, so one_row_added_to_a_tight_codes_buffer_does_not_double_it panicked on `packed_codes.get().expect("packed after add")` — there is no packed buffer after an add any more. That is a real change to what #501 can assert, not just a broken test. The scales buffer and the blocked cache still grow on the same reserve and are still retained, so those stay as capacity assertions (the scales test is renamed for what it now covers). The packed rows are still reserved during the add, but the cost is peak heap for the duration rather than capacity held for the index's lifetime — so that half moves to a peak-heap test on the tracking allocator, which is the shape #501 described in the first place. A/B-verified: with the reserve reverted to plain `v.reserve`, the new test reports "a one-row add after a load peaked at 1363296 bytes against a 1360146-byte index" — a whole extra copy — and passes with the fix. CHANGELOG's #501 entry updated for the same reason: the 4.8 GB figure assumed the packed copy was retained, which #475 makes false. Full Rust and Python suites green on the merge. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#524 fixed the sweep for truncated temp names and covered it through write(). io_v7::write_full stages through the same create_tmp and calls the same sweep_stale_tmps, so the fix should cover sync() as well — but that is a claim about the code sharing a function, not a test of it, and sync() is the path a repeatedly-saved index actually takes. Only a full-rewrite sync stages through a temp at all (a first sync, one after calibrate, a new path, or a change set too large for one header); an incremental sync writes in place and never sweeps. This drives the first one. A/B-verified against the pre-#488 matcher (stem == base): the new test fails with "sync() left a leaked temp for a long destination name" alongside the three write()-path tests, and passes on main. Co-authored-by: t <t@t> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
tmp_siblingtruncates the destination's basename whenbase + suffixwould exceed NAME_MAX, butsweep_stale_tmpsmatched candidates withname.strip_prefix(base)— the untruncated basename. A truncated temp never starts with it, so past about 234 bytes the #299 reclaim was a permanent no-op: a writer killed between temp creation and rename left a full-size sibling nothing would ever remove, and a crash-looping writer filled the volume. That is the exact failure the sweep exists to prevent.I reproduced this as a live leak during an earlier durability sweep, so it is not theoretical.
Fix
Split each candidate at its last
.tmp.and accept the truncated form as well as the exact one. Truncated temps are identified precisely rather than by loose prefix matching: the stem must prefix the destination's basename and the whole name must land on NAME_MAX, which is the only lengthtmp_sibling's cut produces. So it cannot reach a different destination's temps even when two long names share a prefix.is_our_tmp_suffixand the mtime>1h guard are unchanged.Splitting at the last
.tmp.also handles a destination whose own name contains one.Tests
The regression test plants the temp
tmp_siblingwould produce for a dead pid, backdates it pastSTALE_AGE, and does the first save to that destination — first, because the sweep runs once per destination per process. An earlier version of this test saved before planting, which claimed the memo so the later save swept nothing; it passed under a filtered run and failed in the full suite, green for the wrong reason. Worth knowing if you touch this area.Covers both sides of the 234-byte flip, with the short name as a positive control that the harness sweeps at all, and a negative control that an unrelated destination's temp survives. A/B'd: fails without the fix on exactly the reported symptom.
Closes #488
🤖 Generated with Claude Code