A test that opens an index must close it before removing the file, or the Windows shard dies. That rule is currently a convention every test author has to remember, and a reviewed diff just failed to.
The invariant
Windows cannot delete a file that still has an open mapping. std::filesystem::remove's no-error_code overload throws on failure, and the unit suite is built -fno-exceptions — so the throw terminates the whole shard, not just the test. [index] tests are not excluded on Windows CI (only ~[e2e]~[shell] are).
The failure is invisible on Linux and macOS. The only signal is a Windows shard that stops partway through.
Why file it now
During #360 (PR #366) a new test_index.cpp test removed two index files while their mappings were still open. It passed make test, make tidy, make iwyu, a pair-partner review, and a coordinator review before a design review caught it by comparing against the sibling cohort.
The existing sibling sites all do it correctly — test_index.cpp:559-560, :638-639, :700, :1228-1229 are each x.file.close(); followed by std::filesystem::remove(...). Correctness here is carried entirely by whether the author noticed the neighbours.
That is the definition of a conventional invariant: enforced at N sites by cooperation, with a failure mode that is silent on the platform where the work is done.
Proposal
A scoped fixture in the test support header that owns the open/close/remove lifecycle, so the illegal ordering is unrepresentable rather than merely discouraged:
// sketch, not a design
auto idx = TempIndex { "pup_oversized_command_test" }; // owns the path
REQUIRE(write_index(idx.path(), index).has_value());
auto opened = idx.open(); // closes on scope exit, then removes
Open questions for the gate:
- Does the fixture own the path (creating a unique temp name), the mapping, or both? The current tests hand-roll
temp_directory_path() / "name", which also risks collisions between concurrently running shards.
- Several tests open the same index more than once, or open a second index inside a
SECTION. The type has to handle that without becoming a container.
- Is there a real invariant to enforce, or would this be a wrapper that renames
close()? Under this project's Helpers & Wrappers bar it earns its place only if it makes the wrong order impossible — if it merely provides a tidier spelling that authors can still bypass, the convention survives and the fixture is added surface. That question is the substance of the gate, and "no, keep the convention and rely on review" is an acceptable outcome.
- Migration: ~10 existing call sites. All at once, or new tests only?
Not urgent
No known live defect — the #360 instance was fixed before merge. This is about removing the class, and the cost of getting it wrong is one more thing for authors to remember.
A test that opens an index must close it before removing the file, or the Windows shard dies. That rule is currently a convention every test author has to remember, and a reviewed diff just failed to.
The invariant
Windows cannot delete a file that still has an open mapping.
std::filesystem::remove's no-error_codeoverload throws on failure, and the unit suite is built-fno-exceptions— so the throw terminates the whole shard, not just the test.[index]tests are not excluded on Windows CI (only~[e2e]~[shell]are).The failure is invisible on Linux and macOS. The only signal is a Windows shard that stops partway through.
Why file it now
During #360 (PR #366) a new
test_index.cpptest removed two index files while their mappings were still open. It passedmake test,make tidy,make iwyu, a pair-partner review, and a coordinator review before a design review caught it by comparing against the sibling cohort.The existing sibling sites all do it correctly —
test_index.cpp:559-560,:638-639,:700,:1228-1229are eachx.file.close();followed bystd::filesystem::remove(...). Correctness here is carried entirely by whether the author noticed the neighbours.That is the definition of a conventional invariant: enforced at N sites by cooperation, with a failure mode that is silent on the platform where the work is done.
Proposal
A scoped fixture in the test support header that owns the open/close/remove lifecycle, so the illegal ordering is unrepresentable rather than merely discouraged:
Open questions for the gate:
temp_directory_path() / "name", which also risks collisions between concurrently running shards.SECTION. The type has to handle that without becoming a container.close()? Under this project's Helpers & Wrappers bar it earns its place only if it makes the wrong order impossible — if it merely provides a tidier spelling that authors can still bypass, the convention survives and the fixture is added surface. That question is the substance of the gate, and "no, keep the convention and rely on review" is an acceptable outcome.Not urgent
No known live defect — the #360 instance was fixed before merge. This is about removing the class, and the cost of getting it wrong is one more thing for authors to remember.