From d67ccc2280f001b2953e7096720f85e483ee1f8f Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Sun, 19 Jul 2026 09:27:50 +0000 Subject: [PATCH] fix(controller): bug-1452 fix commit_engine_fini/persist_stop teardown race 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. --- src/controller/testsuite/test_persist_truncate.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/controller/testsuite/test_persist_truncate.c b/src/controller/testsuite/test_persist_truncate.c index 56750a14..41257002 100644 --- a/src/controller/testsuite/test_persist_truncate.c +++ b/src/controller/testsuite/test_persist_truncate.c @@ -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: "