tsan: key the pool-lifetime edge on a token, not the pool pointer - #140
Merged
Conversation
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. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01AxN8SYwbj24nAyAAsPRdm4
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.
GLib recycles pool threads rather than joining them, so TSAN sees no edge between a worker finishing and the teardown that frees what it wrote. The annotations bridge that gap: each worker releases when it is done, the teardown acquires. Both sides named the
GThreadPoolpointer.That pointer is precisely what teardown clears:
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 mode is worse than a spurious report. If the read lands after the store, the
if (pool)guard sees NULL and skips the release entirely — the edge is never published, and the teardown's unrelated frees are reported as races elsewhere. That indirection is why this presented as a bug in the progress-slot teardown (pscan_free_threadsfreeing vspscan_finish_filewriting) rather than here.Both sides now name a token the caller owns — a one-byte static in
run_dedupe.c, a field instruct threads_pool— written once, never cleared, outliving every worker.oans_tsan_work_collect()is the acquire half, so the edge no longer rides ong_thread_pool_free()'s argument.threads.chad the same latent flaw (pool->pool = NULLinfree_pool(), read atpool_trampoline()) and is fixed alongside it. It simply had not been caught yet.Scope: ThreadSanitizer builds only. Both helpers are empty inlines otherwise (
src/tsan.h), so nothing changes for users.How it was found
Running the integration suite in parallel (#139) loads the box enough to lose this race reliably. It is invisible to a serial suite, which is why CI has been green.
Same binary at
-j 1passed 3/3 both before and after, confirming it is load-exposed rather than a regression.scripts/verify.shpasses, and the serial TSAN leg (make integration SANITIZE=thread) passes at 120 tests.One dead end worth recording: capturing
dedupe_poolinto a local at worker entry does not work — the race just moves to the entry read (30 failures instead of 2), because a worker can be at its first instruction while the main thread clears the global. The handle cannot be read from shared mutable storage at any point, which is what motivated the token.#139 should merge after this; it is what makes the TSAN leg run under this load.
Generated by Claude Code