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

fix(bug-1429): persist_stop() before commit_engine_fini() in 21 fixtures

Browse source 
The oldest unresolved bug in this project's tracker (first sighted
2026-06-20): controller_unit_test intermittently hung for the full
300s meson timeout with no crash/ASAN report, never previously caught
live under gdb.

Live-reproduced this session (first attempt): thread 1 stuck forever
in commit_engine_fini() -> pthread_cond_destroy() (test_commit_barrier_
rollout.c's jap_teardown), while two fsync_worker threads sat parked
in pthread_cond_timedwait on the same persist_t. Root cause: several
test teardown functions called commit_engine_fini() BEFORE
persist_stop() -- the exact opposite of commit_engine_fini()'s own
documented contract. This let the live background fsync_worker thread
call back into commit_on_durable_advance() while commit_engine_fini()
was mid-destroy of the engine's lock/condvar -- a genuine TOCTOU race
with no synchronization between the two threads.

This is the same race already root-caused once for a SEGV symptom in
bug-1452 (2026-07-19, test_persist_truncate.c) and partially re-fixed
in bug-1687 (2026-08-25, cbrp_teardown) -- but never swept across all
affected files. Bug-1452's own remediation note explicitly flagged
test_demotion_convergence.c as a known, unfixed instance; that flag
went unactioned until now.

Fix: insert persist_stop() immediately before commit_engine_fini() at
21 confirmed-vulnerable sites across 12 files (test_apply_pump.c,
test_apply_pump_integration.c, test_commit_barrier_rollout.c,
test_commit_durability.c, test_commit_engine.c,
test_convergence_verification.c, test_demotion_convergence.c,
test_egress_filter.c, test_ha_role_lifecycle.c, test_job_array_fence.c,
test_job_array_replay_fence.c, test_p2_integration.c) -- the same
pattern bug-1452/1687 already established. persist_stop() is
idempotent (guarded by an atomic "already running" check), so each
fixture's own pre-existing persist_stop() call becomes a safe no-op.
No production code changed.

Known remaining exposure, documented but not fixed this session:
test_config_repl.c (~23 sites) and test_term_safety_adapter.c
(~5 sites) have the same ordering bug in their own teardown helpers,
flagged as follow-up.

Verified: full controller suite run 4 times total (once as part of
the fix, three further independent sequential runs) -- Ok:6 Fail:0
every time, 38-42s wall clock each run, no hang, nowhere near the
300s timeout. 1220/1220 individual controller_unit_test cases pass.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019QxaSzHRB7pmYFp4cVnwkA
This commit is contained in:
Rongsong Shen 2026-08-30 16:04:43 +00:00
parent b0476a8424
commit 0e96a69f7d
No known key found for this signature in database
GPG key ID: 5BCDAAE0123E9FE2
12 changed files with 132 additions and 2 deletions
Download patch file Download diff file Expand all files Collapse all files

28
src/controller/testsuite/test_commit_barrier_rollout.c
View file

@ -483,6 +483,13 @@ static void test_fact_hook_append_is_retained_and_nonblocking(void)
/* Teardown */
repl_broadcast_send_hook = NULL;
/* bug-1429/bug-1452: persist_stop() must run BEFORE commit_engine_fini()
* -- otherwise the live fsync_worker background thread can still be
* mid-flight inside p->on_durable_advance(ce) (persist.c's fsync_worker)
* while commit_engine_fini() destroys ce->lock/ce->cond, a TOCTOU race
* that hangs pthread_cond_destroy() (live-reproduced, gdb-confirmed).
* The persist_stop() call below becomes a safe, idempotent no-op. */
persist_stop(g_fp);
commit_engine_fini(&g_fce);
persist_stop(g_fp);
persist_fini(g_fp);
@ -1285,6 +1292,10 @@ static void cb5_teardown(void)
cb5_drain_ep(&g_cb5_sb_ep[0]);
cb5_drain_ep(&g_cb5_sb_ep[1]);
cb5_drain_ep(&g_cb5_req_ep);
/* bug-1429/bug-1452: persist_stop() before commit_engine_fini() -- see
* jap_teardown's comment below for the full TOCTOU-race rationale
* (live-reproduced, gdb-confirmed hang in this exact class of fixture). */
persist_stop(&g_cb5_c.persist);
commit_engine_fini(&g_cb5_eng);
/* persist_fini calls repl_fini internally (started-persist fixture). */
persist_stop(&g_cb5_c.persist);
@ -1874,6 +1885,23 @@ static void jap_setup(uint32_t sync_timeout_ms)
static void jap_teardown(void)
{
repl_broadcast_send_hook = NULL;
/* bug-1429 (root cause of this session's live, gdb-confirmed hang --
* thread 1 stuck forever in commit_engine_fini -> pthread_cond_destroy
* at this exact call site, while two fsync_worker threads sat parked in
* pthread_cond_timedwait on this same g_jap): persist_stop() must run
* BEFORE commit_engine_fini() -- exactly the ordering bug-1452 already
* fixed for test_persist_truncate.c and bug-1687 for cbrp_teardown()
* (test_commit_barrier_reply.c). commit_engine_fini() destroys
* g_jace.lock/g_jace.cond; the live fsync_worker background thread
* (started by persist_start(), stopped only by persist_stop()'s
* pthread_join) can still be mid-flight inside p->on_durable_advance(ce)
* (persist.c's fsync_worker) -- a genuine TOCTOU data race with no
* synchronization between the two threads. Calling persist_stop() here
* first guarantees the flusher thread cannot be in-flight at all by the
* time commit_engine_fini() runs. The persist_stop() call below becomes
* a safe, idempotent no-op (fsync_thread_running and wal_fd are already
* cleared by the first call). */
persist_stop(&g_jap);
commit_engine_fini(&g_jace);
persist_stop(&g_jap);
persist_fini(&g_jap);