lei.chen/myxce
1
0
Fork 1
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

fix(controller): bug-1452 fix commit_engine_fini/persist_stop teardown race

Browse source 
test_persist_truncate.c's three continuation tests called
commit_engine_fini(&ce) before persist_stop(p), backwards from
commit_engine_fini's own documented contract (persist_stop must run
first). With the fsync_worker flusher thread still alive, it could be
mid-flight in commit_on_durable_advance -> commit_advance_locked
(no lock held) while commit_engine_fini concurrently destroyed
ce->lock/ce->cond and nulled ce->p -- a TOCTOU race where the guard
check reads ce->p as valid but a later re-read observes NULL, SEGV at
exactly offsetof(persist_t, committed_lsn) = 0x38.

This was misdiagnosed as caused by g_drive_async_demotion/catchup_offload
(bug-1450/1451): flipping those flags only perturbed thread-scheduling
timing enough to trigger the always-latent race more reliably. The same
crash signature was independently observed on 2026-07-07, before those
flags existed, proving they were never causal.

Fix: call persist_stop(p) before commit_engine_fini(&ce) in all three
tests. teardown_persist's own internal persist_stop becomes a safe
no-op on the second call. No production code changed.

Verified: full ASAN controller_unit_test suite, 976/976 tests, 6 runs
(1 via meson + 5 standalone direct binary runs), zero ASAN reports.

test_demotion_convergence.c has the same commit_engine_fini-before-
persist_stop ordering (line 233/235) against a real persist_start'd
fixture -- not fixed here, flagged for a follow-up sweep.
This commit is contained in:
Rongsong Shen 2026-07-19 09:27:50 +00:00
parent e1f4d676de
commit d67ccc2280
1 changed files with 16 additions and 0 deletions
Download patch file Download diff file Expand all files Collapse all files

16
src/controller/testsuite/test_persist_truncate.c
View file

@ -335,6 +335,16 @@ static void test_truncate_above_resolves_pending_continuations_as_failed(void)
"truncation -- only entries strictly above the new tail were "
"removed from the log");
/* bug-1452: persist_stop() MUST run before commit_engine_fini() -- the
* fsync_worker flusher thread is still live here and can be mid-flight
* calling the durable-advance callback (commit.c's fsync_worker ->
* commit_on_durable_advance -> commit_advance_locked) with NO lock held.
* commit_engine_fini() destroys ce->lock/ce->cond and nulls ce->p; racing
* that against a still-running flusher lets commit_advance_locked observe
* ce->p go NULL between its own guard check and its dereference -> SEGV.
* persist_stop() joins the flusher thread first, so by the time
* commit_engine_fini() runs, nothing can call back into ce. */
persist_stop(p);
commit_engine_fini(&ce);
teardown_persist(p);
printf("test_truncate_above_resolves_pending_continuations_as_failed: PASS\n");
@ -371,6 +381,9 @@ static void test_truncate_noop_does_not_resolve_pending_continuations(void)
"a no-op truncate (nothing above cutoff_lsn) must not resolve any "
"pending continuation");
/* bug-1452: stop the flusher thread before tearing down ce -- see the
* comment in test_truncate_above_resolves_pending_continuations_as_failed. */
persist_stop(p);
commit_engine_fini(&ce);
teardown_persist(p);
printf("test_truncate_noop_does_not_resolve_pending_continuations: PASS\n");
@ -482,6 +495,9 @@ static void test_truncate_concurrent_burst_resolves_above_cutoff_exactly_once(vo
"only the above-cutoff admission slots are released by truncation; "
"the below-cutoff ones remain pending");
/* bug-1452: stop the flusher thread before tearing down ce -- see the
* comment in test_truncate_above_resolves_pending_continuations_as_failed. */
persist_stop(p);
commit_engine_fini(&ce);
teardown_persist(p);
printf("test_truncate_concurrent_burst_resolves_above_cutoff_exactly_once: "