Skip to content

Design gate: close-before-remove in index tests is a convention that a reviewed diff just missed #367

Description

@typeless

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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions