Repository navigation
Keep a fixture's cleanup failure from aborting its test binary while a test is already failing - #1353
Merged
Merged
Conversation
…, and pin it failing on the fixture of the report and on every copy, red until agent-grounds#1226 (§AR-ci-fixture-cleanup) A fixture Drop that runs `fs::remove_dir_all(..).unwrap()` or `.expect(..)` raises its removal failure even while the test holding it is already failing. Rust aborts the whole process on a panic during unwinding, so every test the binary has not yet reported goes unrun. On windows-latest the abort is the silent exit 0xC0000409 of agent-grounds#1226, which `rules_contract`'s `Configured` produced. §AR-ci-fixture-cleanup is its own page under the architecture index, beside the other `AR-ci-*` pages, because AR-ci.md is over its fissile budget. It says a fixture writes the removal error to stderr while unwinding and raises nothing, that the passing path stays as strict as §AR-ci.10.4 asks, and that one helper, `remove_fixture_tree`, makes the decision. Two pins, both red until the fix: - `offer_scope::cleanup::a_failing_test_whose_configured_cannot_be_removed_reports_its_own_panic` holds a real `Configured` on its own thread, refuses its removal with a read-only directory inside the root, and fails the test. Today the whole `rules_contract` binary dies of SIGABRT, "panic in a destructor during cleanup". It is ignored until the fix, and `implement` removes the marker. Its control, `a_passing_test_whose_configured_cannot_be_removed_still_fails`, passes today and must keep passing. - `tests/integration/test_fixture_cleanup.py` holds every tree-removing `impl Drop` under `crates/` to three shapes: it raises nothing, it calls the helper, or it tests `std::thread::panicking()` before it raises. It names the 24 that raise today. It lands as `expectedFailure`, which the fix takes off.
…hich writes a removal failure while unwinding and raises it otherwise, and claim Configured's root by pid and serial, so a failing test no longer aborts its binary (§AR-ci-fixture-cleanup) Closes the 24 copies of the panicking fixture Drop under crates/, the one that aborted rules_contract on windows-latest (agent-grounds#1226), and takes the red markers off both pins.
vjovanov
force-pushed
the
fix/issue-1226
branch
from
November 11, 2026 00:04
dc1c09e to
f303b1d
Compare
vjovanov
marked this pull request as ready for review
November 11, 2026 00:04
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1226
A test that fails while its fixture's tree cannot be removed is now reported as one failed test, and the rest of its binary still runs. Before this change, the fixture's
Droppanicked a second time during unwinding. That aborted the whole test binary, and on windows-latest it did so silently with0xC0000409, which is howrules_contractlost 205 tests in #1226.Before, on main, the new pin holds a real
Configured(the fixture of the report) whose tree cannot be removed, and fails the test:Triage's reproducer injects the same failure into the whole
rules_contractbinary. On main it dies of SIGABRT with 66 of its 112 tests unrun.After, the same reproducer on this branch:
What changed
The cause. About two dozen fixture
Drops undercrates/*/testscalledfs::remove_dir_all(..).unwrap()or.expect(..)with nostd::thread::panicking()guard. If a test failed while its tree could not be removed, theDroppanicked during unwinding, and Rust aborts the process on that ("panic in a destructor during cleanup"). On Windows MSVC that abort is a fast-fail, exit0xC0000409(STATUS_STACK_BUFFER_OVERRUN), and libtest never flushes the output it captured. So the CI log carried an exit code and nothing else, and every test after it in the shard went unrun.The rule. §AR-ci-fixture-cleanup is a new page under
docs/architecture/, with a row in the architecture README. It says:support/case_copy.rs, which toleratesNotFound) guards inline (§AR-ci-fixture-cleanup.3);It is a page of its own rather than §AR-ci.10.6 because that number is already taken on main, and fissile refuses further edits to
AR-ci.md(promotion limit). That is the same remedy the otherAR-ci-*pages took.AR-ci.mdis untouched.The helper.
remove_fixture_tree(path, what)in the newcrates/grund-cli/tests/support/fixture_cleanup.rs. It removes the tree and, if that fails, writes<what> <path>: <err>to stderr while unwinding and panics with the same message otherwise.Every copy. All 24 tree-removing fixture
Drops now call the helper, each keeping its old message. Triage counted 18, and 6 more arrived from main since. 23 are incrates/grund-cli/tests, and binaries that do not includesupportreach the helper with#[path = ...] mod fixture_cleanup;. One is in grund-lsp (two_names_temp_roots.rs), and it reaches the same helper across crates through#[path = "../../grund-cli/tests/support/fixture_cleanup.rs"]instead of a second copy.value_home_unsearchablestill resets itsvaluesdirectory to0755before removing the tree, but that reset no longer panics. The three copies that were already guarded (case_copy.rs,agreement_host.rs,agreement_editor_worker.rs) are left as they were.Configured's root. The fixture of the report named its root from the pid and the clock, and adopted it withcreate_dir_all, so two roots named in one clock tick could share a tree. It now names its rootgrund-1001-configured-<pid>-<serial>from a process-wide counter (§AR-ci.10.1). It claims the root with an exclusivefs::create_dirbefore writing into it (§AR-ci.10.2), so a taken name panics before anyConfiguredexists and cannot remove another test's tree.No behaviour of the shipped
grundchanges: only test fixtures, test support and one architecture page.How it was verified
offer_scope::cleanupin therules_contractbinary (#[cfg(unix)]) holds a realConfiguredon a thread of its own. It makes a directory inside the root read-only, so the removal fails without touching anything outside it.a_failing_test_whose_configured_cannot_be_removed_reports_its_own_panicpanics and asserts that the join returns the test's own panic and that the tree was left in place. On main it aborts the binary, as shown above.a_passing_test_whose_configured_cannot_be_removed_still_failsis the control: with no first panic, theDropmust still fail withPermission denied. It catches a fix that silences the passing path.skipped: …and return.tests/integration/test_fixture_cleanup.pyreads everyimpl Dropundercrates/that removes a tree. It requires each one to raise nothing, call the helper, or teststd::thread::panicking()before its first raise, and holds the helper itself to both halves. On main it fails, naming all 24 sites. TenShapeTestcases show that it tells the shapes apart.rules_contractbinary on Linux with injected failures and unremovable trees. It went from exit 1 (SIGABRT, 66 of 112 tests unrun) to exit 0 (all 114 ran).pre-commit run --all-filesexited 0 ondc1c09ef04(22m37s), with fmt, build with-Dwarnings, the cargo and Python tests,grund check --full,grund fmt,grund init --check, lychee and fissile.Not established
accbb882d7puts fourrules_contracttests in flight at the abort, and three of them build aConfigured. The clock-named, adopted root fixed above is the likeliest trigger. A handle held by another process on the runner would also do it.abort_internalis a__fastfailwithFAST_FAIL_FATAL_APP_EXIT), not on a Windows run. The mechanism was reproduced on Linux only.Found and deliberately not fixed
Dropcalls before the helper, and apanicking()call that gates nothing. No site in the tree has either shape today. An independent scan of everyimpl Dropundercrates/found no tree-removingDropthat still raises unguarded.value_home_unsearchablenow ignores a failed mode reset (let _ = fs::set_permissions(..)). This is harmless: a reset that fails leaves the tree unremovable, and the strict removal that follows still fails the test. The one case it now passes silently is a missingvaluesdirectory, where no tree is left behind anyway.Reset(crates/grund-cli/src/cli_watch_observer.rs:160) andHarness(tests/support/watch_observer.rs:286) panic inDrop, through a mutexlock().unwrap(), but remove no tree.rules_contractleaves itssupport::scratchrepositories undertarget/rules-contract-work.Configuredthat still name their roots from the clock.AI workflow: `rhei`, 14 agent invocations across 1 model; 6 tasks completed, 10 in progress.
github-issues-agent-grounds-grund-1226-implement-f358abf1.ticketsupervising (visit 1) — cld, anthropic/claude-opus-5-5 — 1.0s — 0 in / 0 outgithub-issues-agent-grounds-grund-1226-implement-f358abf1.ticketsupervising (visit 1) — cld, anthropic/claude-opus-5-5 — 2.0s — 0 in / 0 outgithub-issues-agent-grounds-grund-1226-implement-f358abf1.ticketsupervising (visit 1) — cld, anthropic/claude-opus-5-5 — 2m05s — 1.2M in / 11.8k outgithub-issues-agent-grounds-grund-1226-implement-f358abf1.ticket.triageassessing (visit 1) — cld, anthropic/claude-opus-5-5 — 30m00s — 17.2M in / 119.1k outgithub-issues-agent-grounds-grund-1226-implement-f358abf1.ticket.triageassessing (visit 1) — cld, anthropic/claude-opus-5-5 — 22m47s — 3.6M in / 24.2k outgithub-issues-agent-grounds-grund-1226-implement-f358abf1.ticket.triagereproducing (visit 1) — cld, anthropic/claude-opus-5-5 — 9m16s — 3.7M in / 50.1k outgithub-issues-agent-grounds-grund-1226-implement-f358abf1.ticketsupervising (visit 2) — cld, anthropic/claude-opus-5-5 — 3m51s — 2.7M in / 21.6k outgithub-issues-agent-grounds-grund-1226-implement-f358abf1.ticket.specifyspecify — cld, anthropic/claude-opus-5-5 — 72m31s — 13.5M in / 75.1k outgithub-issues-agent-grounds-grund-1226-implement-f358abf1.ticketsupervising (visit 3) — cld, anthropic/claude-opus-5-5 — 2m50s — 701.3k in / 4.9k outgithub-issues-agent-grounds-grund-1226-implement-f358abf1.ticket.implementimplement — cld, anthropic/claude-opus-5-5 — 22m06s — 2.7M in / 17.0k outgithub-issues-agent-grounds-grund-1226-implement-f358abf1.ticketsupervising (visit 4) — cld, anthropic/claude-opus-5-5 — 35.0s — 407.5k in / 3.6k outgithub-issues-agent-grounds-grund-1226-implement-f358abf1.ticket.review-1review — cld, anthropic/claude-opus-5-5 — 2m11s — 1.4M in / 10.2k outgithub-issues-agent-grounds-grund-1226-implement-f358abf1.ticketsupervising (visit 5) — cld, anthropic/claude-opus-5-5 — 40.0s — 518.3k in / 2.9k outgithub-issues-agent-grounds-grund-1226-implement-f358abf1.ticketsupervising (visit 6) — cld, anthropic/claude-opus-5-5 — 28.0s — 313.9k in / 2.9k out