Run the integration suite in parallel - #139
Merged
Merged
Conversation
martinus
force-pushed
the
claude/parallel-integration-tests
branch
from
July 25, 2026 17:27
82cd878 to
b37a727
Compare
martinus
added a commit
that referenced
this pull request
Jul 25, 2026
GLib recycles pool threads instead of joining them, so TSAN sees no edge between a worker finishing and the teardown that frees what it wrote. The annotations bridge that: each worker releases when it is done, and the teardown acquires. Both sides named the GThreadPool pointer. That pointer is exactly what teardown clears. dedupe_phase_end() does `g_thread_pool_free(); dedupe_pool = NULL;` and free_pool() ends with `pool->pool = NULL`, and a worker can still be in its tail when that store lands - so reading it there to publish on is itself a data race. The failure is worse than a spurious report: if the read lands after the store, the `if (pool)` guard sees NULL and skips the release, the edge is never published, and the teardown's unrelated frees get reported as races instead. That indirection is why this read as a bug in the progress-slot teardown (pscan_free_threads vs pscan_finish_file) rather than here. Both sides now name a token the caller owns - a one-byte static in run_dedupe.c, a field in struct threads_pool - written once, never cleared, outliving every worker. oans_tsan_work_collect() is the acquire half, so the edge no longer rides on g_thread_pool_free()'s argument. threads.c had the same latent flaw and is fixed with it; it simply had not been caught yet. Only ThreadSanitizer builds are affected: both helpers are empty inlines otherwise, so no behaviour changes for users. Found by running the integration suite in parallel (#139), which loads the box enough to lose that race reliably: 3 of 3 parallel runs failed with 14+ reports before, 4 of 4 clean with zero after. verify.sh and the serial TSAN leg both pass. Claude-Session: https://claude.ai/code/session_01AxN8SYwbj24nAyAAsPRdm4 Co-authored-by: Claude <[email protected]>
The 120-test integration suite ran serially at ~26 s. That is tolerable for a
plain `make check` but not for the sanitizer builds, which re-run the whole
suite at 4-8x the cost (~103 s under TSAN, ~215 s under ASAN), nor for
`integration-valgrind` at ~7x.
Add `-j` to tests/run.py: `auto` (the default) is min(nproc, 8), `-j 1` keeps
the previous serial TextTestRunner verbatim. `make integration JOBS=…` plumbs
it through, and `integration-valgrind` picks up the same knob.
Measured on a 4-core box against an XFS scratch: 26.4 s -> 7.9 s (3.3x), with
`make check` going 35 s -> 13 s. Serial and parallel agree exactly (120 tests,
4 skipped), and five repeat runs - including two oversubscribed at -j 8 - were
stable.
Two details worth keeping:
- Workers are processes, not threads. test_long_path chdir()s, and cwd is
process-global, so threads would race it. Tests were already isolated by
setUp's per-test mkdtemp and its per-test hashfile, so no test changes
were needed.
- Work is dispatched one test at a time rather than as fixed shards, so the
slow outliers (autotune ~1.3 s, vacuum ~0.7 s) can't strand a worker at
the end of the run.
Co-Authored-By: Claude Opus 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01AxN8SYwbj24nAyAAsPRdm4
Follow-up to the parallel runner, from a review pass. Reporting: the parallel path hand-rolled ~35 lines that unittest already provides, which left two output formats to keep in step and silently dropped information - only the first traceback per test, and nothing for expectedFailure/unexpectedSuccess. Worker outcomes now replay into a real TextTestResult, so unittest does the formatting and every category survives; a test that fails in its body *and* errors in cleanup now shows both tracebacks. With one formatter the `jobs == 1` special case dissolves: a one-worker pool is already sequential, and the old branch meant TEST_JOBS=1 silently switched execution model (in-process vs forked), so the mode you debugged in was not the mode that failed. Concurrency: `auto` was min(nproc, 8), which under-subscribes badly - the suite blocks in subprocess.run(), sync() and autotune's cache drop, so total CPU is flat across job counts. Measured on 4 cores over XFS: j=4 6.9s, j=8 4.4s, j=12 3.6s, j=16 3.6s. Now 2 * nproc under the same ceiling, taking the suite 26.4s -> 4.3s (was 7.9s). The cap stays 8 rather than the ~12 plateau because a worker under valgrind or ASAN costs far more than a plain oans; the comment now says that instead of asserting a measurement I never made. Also: pin the fork start method, which was load-bearing and undeclared (3.14 defaults Linux to forkserver, under which each worker would re-import harness and re-probe reflink support); reuse harness.scratch_fstype() instead of a second `stat -f` subprocess; rename JOBS to TEST_JOBS so it stops reading as make's own -j; and cut the rationale from three copies to one. Skipped from the review: a per-test `serial = True` opt-out (nothing needs it yet) and longest-first scheduling (measured: no gain, discovery order already happens to front-load the slow tests). Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01AxN8SYwbj24nAyAAsPRdm4
CI on btrfs failed test_extent_order_independent and test_streaming_dedupe, and locally test_sparse_file_scans failed 4 runs in 12. Both are latent dependencies on writeback that a serial suite never had to confront, not breakage in the change - but they are the change's problem. Physical extent layout. Several tests build a specific on-disk layout - the fsync-forced extent boundary, or fiemap extent counts - and concurrent I/O perturbs btrfs writeback enough that the setup does not get the layout it intends. No amount of file isolation helps, so DuperemoveTest gains a `serial` flag: tests/run.py holds those classes back and runs them one at a time after the pool drains, keeping the parallel win for the other 110 tests. The four fsync-boundary files are marked. Delayed allocation. make_sparse handed a file straight to a scan asserting on its extents, without ever fsyncing it. Under xfs delalloc those extents may not be allocated yet, so oans maps none and correctly records none; running in parallel pushed writeback further behind and turned a latent race into a 1-in-3 failure. The sparse-layout helpers now fsync, which is what the harness's own sync() docstring already said this class of assertion needs. Fixed at the helper rather than by serialising the test - the assertion is about oans, not about writeback timing. 12 consecutive parallel runs clean, from 4/12 failing. The serial flag is the mechanism the review pass proposed and I skipped as speculative. CI found the case for it within the hour. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01AxN8SYwbj24nAyAAsPRdm4
martinus
force-pushed
the
claude/parallel-integration-tests
branch
from
July 25, 2026 17:53
b37a727 to
77d0545
Compare
test_hardlink_pair_does_not_empty_hashfile saw 381 of 401 extents under the TSAN build's load - the same delayed-allocation problem as the sparse tests, just reached through mkrand/write rather than make_sparse. Twenty files had not been written back when oans mapped them, so it recorded no extents for them, correctly, and the test read the shortfall as a scanner bug. Fixing it per file does work and costs far too much: fsync()ing every created file is a journal commit apiece, and took the suite from 5.4s to 18s - most of the parallel win, to make a guarantee no test needs at that granularity. What the tests actually need is that the tree is on disk before oans walks it, which is one syncfs() of the scratch at each oans invocation. That measures at 5.0s, free against the 5.4s baseline. So the per-helper fsyncs added with the sparse fix are gone again, replaced by the single settle in dm(), which covers every test rather than the ones that had already been caught flaking. 12 consecutive runs clean on the normal build, 5 on the TSAN build in parallel with zero race reports. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01AxN8SYwbj24nAyAAsPRdm4
martinus
force-pushed
the
claude/parallel-integration-tests
branch
from
July 25, 2026 18:06
77d0545 to
68fd342
Compare
This was referenced Jul 25, 2026
martinus
added a commit
that referenced
this pull request
Jul 25, 2026
test_hardlink_pair_does_not_empty_hashfile asserted exactly 401 extents for 401 inodes, which quietly assumes btrfs gives each of these files exactly one extent. It does not have to: CI saw 402 under the ASAN leg, where the load of the parallel suite let writeback split one file in two. Earlier the same assertion failed the other way, at 381, before the scratch settle went in. How many extents a file gets is btrfs's business. The bug this test guards emptied the hashfile - a batched-writer REPLACE cascade - so what matters is that every inode has extents at all. The exact invariant is the row count on the line above, which stays ==401, and the extent check becomes >=401. Fallout from running the suite in parallel (#139), not from anything in the scan path. Claude-Session: https://claude.ai/code/session_01AxN8SYwbj24nAyAAsPRdm4 Co-authored-by: Claude <[email protected]>
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
2 participants
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.
The 120-test integration suite ran serially at ~26 s. That is tolerable for a plain
make check, but the sanitizer builds re-run the whole thing at 4-8x the cost (~103 s under TSAN, ~215 s under ASAN), andintegration-valgrindat ~7x.tests/run.pygains-j;make integration TEST_JOBS=…plumbs it through, and the valgrind leg picks up the same knob.Measured on a 4-core box against an XFS scratch:
make checkNo test changes were needed:
setUpalready gave every test its ownmkdtempscratch and its own hashfile inside it.Notes
Workers are processes, not threads.
test_long_pathchdir()s and cwd is process-global, so threads would race it.autodeliberately over-subscribes.min(nproc, 8)was the obvious default and it is the wrong model — the suite blocks insubprocess.run(),sync()and autotune's cache drop, so total CPU is flat across job counts (~8.7 s either way). Measured: j=4 6.9 s, j=8 4.4 s, j=12 3.6 s, j=16 3.6 s. It is now2 * nprocunder the same ceiling. The cap stays at 8 rather than the ~12 plateau because a worker under valgrind or ASAN costs far more than a plainoans, and the scratch filesystem is shared — a judgement call, not a measured optimum, and the comment says so.One output path. Worker outcomes replay into a real
TextTestResult, so unittest does the formatting in every mode. This also means-j 1no longer switches execution model as a side effect of the job count — previously it ran in-process while every other value forked, so the mode you debugged in was not the mode that failed.Fork is pinned. It was load-bearing and undeclared: 3.14 defaults Linux to
forkserver, under which each worker would re-importharnessand re-probe reflink support (~0.15 s each).Verification
scripts/verify.shpasses end to end (build,make check, valgrind smoke). Serial and parallel agree exactly — 120 tests, 4 skipped, rc=0 — and five repeat runs, two of them oversubscribed at-j 8on 4 cores, were all green.Failure rendering was checked against a throwaway test exercising failure, error, skip,
expectedFailure, and a body-failure-plus-cleanup-error: all five render as unittest would, the exit code is 1, and the double-fault case shows both tracebacks (the first draft of this change showed only the first).