fix(lib): stop cbuf_put/get orphaning their mutex on cancellation, hanging vtaskmgr_fini

pthread_cond_wait() reacquires its mutex before honoring a cancellation,
but mylock.c installed no cleanup handler to release it again -- a
thread cancelled mid-wait terminated holding the lock forever. vtaskmgr's
pthread_cancel()-based teardown could land there from three angles
(dispatcher cancelled while polling the shared queue, or a vcpu racing
back into it after its own shutdown signal), permanently deadlocking any
other thread that touched the same queue and hanging vtaskmgr_fini()'s
pthread_join() indefinitely.

Reproduced live as xce_test hanging on Ctrl-D exit once a connection
context was entered; gdb confirmed a vcpu thread blocked forever on a
mutex owned by an already-exited thread ID.

Fix: cbuf_put()/cbuf_get()/cbuf_timedget() now unlock via a
pthread_cleanup_push/pop handler around the wait, so a cancelled waiter
always releases the mutex. vcpu_idle()/vcpu_entry() also recognize the
teardown poison pill and exit immediately instead of needlessly
re-announcing into the already-dead shared queue.

New test_vtaskmgr_teardown.c reproduces the race deterministically
(self-pinned to one CPU, since true multi-core parallelism rarely hits
the window) and bounds vtaskmgr_fini() with a timed join so a
regression fails fast instead of hanging the suite. Logged as bug-1810.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RLSNJhNoULvVcUhXEFafQC

src/lib/meson.build

@@ -226,6 +226,7 @@ libxce_test_src = ['testsuite/test.c',
                    'testsuite/test_ctrlstatus.c',
                    'testsuite/test_vtask_admission.c',
                    'testsuite/test_vtask_enq_race.c',
+                   'testsuite/test_vtaskmgr_teardown.c',
                    'testsuite/test_xcealloc.c',
                    'raft/raft_sim.c',
                   ]

src/lib/mylock.c

@@ -99,9 +99,18 @@ int cbuf_put(cbuf_t* b, void* it)
     }
 
     pthread_mutex_lock(&(b->mutex));
+    /* pthread_cond_wait() reacquires the mutex before honoring a pending
+     * cancellation (POSIX-mandated), so a caller cancelled while parked
+     * here would otherwise terminate holding b->mutex forever -- no other
+     * thread could ever lock it again. The cleanup handler releases it on
+     * that unwind; on the normal (non-cancelled) path pop(0) just removes
+     * the handler without running it, since the explicit unlock below
+     * still runs. */
+    pthread_cleanup_push((void (*)(void*))pthread_mutex_unlock, &(b->mutex));
     while (b->len == b->ncap) {
         pthread_cond_wait(&(b->condput), &(b->mutex));
     }
+    pthread_cleanup_pop(0);
 
     i = (b->head + b->len) % b->ncap;
 
@@ -148,9 +157,14 @@ int cbuf_get(cbuf_t* b, void* it)
     }
 
     pthread_mutex_lock(&(b->mutex));
+    /* See cbuf_put()'s comment on the same pattern: without this cleanup
+     * handler, a caller cancelled while parked in pthread_cond_wait()
+     * would reacquire and then abandon b->mutex locked forever. */
+    pthread_cleanup_push((void (*)(void*))pthread_mutex_unlock, &(b->mutex));
     while (b->len == 0) {
         pthread_cond_wait(&(b->condget), &(b->mutex));
     }
+    pthread_cleanup_pop(0);
 
     memcpy(it, (void*)((char*)b->data) + b->isize * b->head, b->isize);
 
@@ -189,18 +203,33 @@ int cbuf_tryget(cbuf_t* b, void* it)
 
 int cbuf_timedget(cbuf_t* b, void* it, const struct timespec* deadline)
 {
+    int timed_out = 0;
+
     if ((b == NULL) || (it == NULL) || (deadline == NULL)) {
         return -EINVAL;
     }
 
     pthread_mutex_lock(&(b->mutex));
+    /* See cbuf_put()'s comment on the same pattern: without this cleanup
+     * handler, a caller cancelled while parked in pthread_cond_timedwait()
+     * would reacquire and then abandon b->mutex locked forever.
+     * pthread_cleanup_push()/pop() must be a matched pair at the same
+     * lexical nesting level -- break out of the loop on timeout instead
+     * of returning from inside it, so pop() always runs right after. */
+    pthread_cleanup_push((void (*)(void*))pthread_mutex_unlock, &(b->mutex));
     while (b->len == 0) {
         int rc = pthread_cond_timedwait(&(b->condget), &(b->mutex), deadline);
         if (rc == ETIMEDOUT) {
-            pthread_mutex_unlock(&(b->mutex));
-            return -ETIMEDOUT;
+            timed_out = 1;
+            break;
         }
     }
+    pthread_cleanup_pop(0);
+
+    if (timed_out) {
+        pthread_mutex_unlock(&(b->mutex));
+        return -ETIMEDOUT;
+    }
 
     memcpy(it, (void*)((char*)b->data) + b->isize * b->head, b->isize);
 

src/lib/testsuite/test.c

@@ -55,6 +55,7 @@ extern void test_wal_wire(void);
 extern void test_ctrlstatus(void);
 extern void test_vtask_admission(void);
 extern void test_vtask_enq_race(void);
+extern void test_vtaskmgr_teardown(void);
 extern void test_xcealloc(void);
 
 void setUp(void) { return; }
@@ -180,6 +181,7 @@ int main(int argc, char* argv[])
     TEST(test_ctrlstatus);
     TEST(test_vtask_admission);
     TEST(test_vtask_enq_race);
+    TEST(test_vtaskmgr_teardown);
     TEST(test_xcealloc);
 
     return UNITY_END();

src/lib/testsuite/test_vtaskmgr_teardown.c

@@ -0,0 +1,158 @@
+/*
+ * test_vtaskmgr_teardown.c — vtaskmgr_fini() must never hang tearing down
+ * idle vcpus.
+ *
+ * Root cause under test: vcpu_fini() (vtaskmgr.c) wakes a parked vcpu via
+ * bind_vtask(NULL, vcpu) (a "poison pill" through the vcpu's own per-vcpu
+ * cbuf) and then pthread_cancel()s + pthread_join()s it. vcpu_entry()'s loop
+ * cannot distinguish that poison pill from "no task yet" -- it loops right
+ * back into vcpu_idle(), which unconditionally re-announces the vcpu into
+ * the SHARED vtaskmgr->wbuf cbuf (step A) before waiting again (step B).
+ * By the time any vcpu is torn down, vtaskmgr_fini() has already
+ * cancelled+joined the dispatcher thread -- the only consumer of
+ * vtaskmgr->wbuf -- so that shared queue is never drained again. If
+ * pthread_cancel() lands while the vcpu is blocked inside step A's
+ * cbuf_put()'s pthread_cond_wait() on that now-permanently-unconsumed
+ * queue, the thread is torn down without releasing vtaskmgr->wbuf's mutex
+ * (mylock.c's cbuf_put/cbuf_get install no pthread_cleanup_push() handler),
+ * orphaning it locked forever. Any OTHER still-alive vcpu that later loops
+ * back into vcpu_idle() then deadlocks uninterruptibly on
+ * pthread_mutex_lock() (not a cancellation point in glibc's NPTL), and
+ * vtaskmgr_fini()'s pthread_join() for it never returns.
+ *
+ * Reproduced live: xce_test (harness REPL) hung on Ctrl-D (EOF) exit, its
+ * main thread parked forever in leave_ctx() -> xcecli_ctx_fini() ->
+ * xce_ctx_fini() -> vtaskmgr_fini() -> vcpu_fini()'s pthread_join(). A gdb
+ * attach confirmed a sibling vcpu thread deadlocked in cbuf_put()'s
+ * pthread_mutex_lock() against vtaskmgr->wbuf.mutex, owned by a thread ID
+ * that had already exited.
+ *
+ * The race is timing-dependent: on a machine with more CPUs than
+ * NVCPUS+2, the dispatcher/vcpu threads run in true parallel and a vcpu
+ * almost always reaches its own pthread_testcancel() before
+ * vtaskmgr_fini()'s pthread_cancel() is even issued, so the hazardous
+ * second vcpu_idle() call is rarely reached -- confirmed empirically (0
+ * hangs across hundreds of multi-core iterations). Pinning this test's
+ * thread (and therefore, by Linux's affinity-inheritance-at-clone()
+ * rule, every vtaskmgr thread it spawns) to a SINGLE CPU forces
+ * preemptive time-slicing instead of true parallelism, which reproduces
+ * the hang deterministically -- confirmed on iteration 0 of a
+ * single-core standalone repro. The loop below still runs several
+ * iterations, with a bounded-timeout join, as a safety margin -- it
+ * FAILS (rather than hanging the whole suite) the first time any
+ * iteration doesn't tear down within the deadline.
+ */
+#define _GNU_SOURCE
+#include <pthread.h>
+#include <sched.h>
+#include <stdlib.h>
+#include <time.h>
+#include <unistd.h>
+
+#include <unity/unity.h>
+
+#include "vtaskmgr.h"
+
+typedef struct {
+    vtaskmgr_t* mgr;
+} fini_arg_t;
+
+static void* fini_thread(void* p)
+{
+    fini_arg_t* a = (fini_arg_t*)p;
+
+    vtaskmgr_fini(a->mgr);
+    return NULL;
+}
+
+static int noop_entry(void* p)
+{
+    (void)p;
+    return 0;
+}
+
+static vtask_t* make_noop_task(void)
+{
+    vtask_data_t* d = calloc(1, sizeof(*d));
+    vtask_t*      t = vtask_alloc();
+
+    d->vtype     = VTASK_GENERIC;
+    d->u.generic = NULL;
+    vtask_init(t, noop_entry, d);
+    return t;
+}
+
+static void test_vtaskmgr_fini_idle_multi_vcpu_does_not_hang(void)
+{
+    const int ITERATIONS = 10;
+    const int NVCPUS     = 16;
+    const int BURST      = NVCPUS * 40;
+
+    cpu_set_t saved_affinity;
+    cpu_set_t single_cpu;
+    int       have_saved_affinity
+        = (sched_getaffinity(0, sizeof(saved_affinity), &saved_affinity) == 0);
+
+    CPU_ZERO(&single_cpu);
+    CPU_SET(0, &single_cpu);
+    /* Threads spawned after this point (vtaskmgr's dispatcher/vcpus, and
+     * this test's own fini_thread) inherit the calling thread's affinity
+     * mask at clone() time -- pinning here forces the preemptive
+     * time-slicing that makes the race reproducible (see file header). */
+    sched_setaffinity(0, sizeof(single_cpu), &single_cpu);
+
+    for (int iter = 0; iter < ITERATIONS; iter++) {
+        vtaskmgr_t mgr;
+
+        TEST_ASSERT_EQUAL_INT(0, vtaskmgr_init(&mgr, NVCPUS));
+        TEST_ASSERT_EQUAL_INT(0, vtaskmgr_start(&mgr));
+
+        /* Flood a burst of trivial real tasks so the dispatcher is
+         * ACTIVELY cycling through pick_idle_vcpu()/bind_vtask() -- not
+         * settled in steady idle -- when fini() below tears it down. This
+         * mirrors the live scenario (an xce_test client connection with
+         * genuine in-flight I/O) rather than a purely idle REPL: the
+         * dispatcher revisiting pick_idle_vcpu()'s blocking cbuf_get() on
+         * vtaskmgr->wbuf is exactly the window where a cancel can orphan
+         * that shared mutex before any vcpu teardown even begins. No
+         * settle wait -- fini() is called immediately, mid-burst. */
+        for (int k = 0; k < BURST; k++) {
+            vtask_t* t = make_noop_task();
+            vtask_enq_classified(&mgr, t, VTASK_LIVENESS);
+        }
+
+        fini_arg_t farg = { .mgr = &mgr };
+        pthread_t  t;
+        TEST_ASSERT_EQUAL_INT(0,
+            pthread_create(&t, NULL, fini_thread, &farg));
+
+        struct timespec deadline;
+        clock_gettime(CLOCK_REALTIME, &deadline);
+        deadline.tv_sec += 2;
+
+        int jrc = pthread_timedjoin_np(t, NULL, &deadline);
+        TEST_ASSERT_EQUAL_INT_MESSAGE(0, jrc,
+            "vtaskmgr_fini() must not hang tearing down idle vcpus -- an "
+            "orphaned vtaskmgr->wbuf mutex from a mid-cancellation "
+            "cbuf_put() leaves a later vcpu deadlocked forever "
+            "(reproduces the live xce_test Ctrl-D hang)");
+
+        if (jrc != 0) {
+            /* Don't let a hung teardown thread corrupt later iterations --
+             * detach it (it will never finish) and stop looping; the mgr
+             * itself is intentionally leaked, since its vcpus may still be
+             * deadlocked mid-teardown. */
+            pthread_detach(t);
+            break;
+        }
+    }
+
+    if (have_saved_affinity) {
+        sched_setaffinity(0, sizeof(saved_affinity), &saved_affinity);
+    }
+}
+
+void test_vtaskmgr_teardown(void)
+{
+    RUN_TEST(test_vtaskmgr_fini_idle_multi_vcpu_does_not_hang);
+}

src/lib/vtaskmgr.c

@@ -48,6 +48,24 @@ void vcpu_idle(vcpu_t* vcpu)
     vtaskmgr_t* vtaskmgr = (vtaskmgr_t*)vcpu->owner;
 
     vcpu->task = NULL;
+
+    /* vcpu_fini()'s teardown protocol (bind_vtask(NULL, vcpu)) can queue a
+     * shutdown "poison pill" into this vcpu's OWN wbuf while it is busy
+     * running a real task -- the pill then sits there until the next
+     * vcpu_idle() call. Consume it directly (non-blocking) instead of
+     * first re-announcing this vcpu into the SHARED vtaskmgr->wbuf: by
+     * the time any vcpu_fini() runs, vtaskmgr_fini() has already
+     * cancelled+joined the dispatcher thread, the sole consumer of
+     * vtaskmgr->wbuf, so re-announcing here is pure wasted work at best
+     * (bug-1810) -- mylock.c's cbuf_put()/cbuf_get() now install a
+     * cancellation cleanup handler so a cancel landing there can no
+     * longer orphan the mutex, but skipping the needless re-announce
+     * entirely is still the correct, minimal behavior once a shutdown has
+     * already been signaled. */
+    if (cbuf_tryget(&(vcpu->wbuf), (void*)&(vcpu->task)) == 0) {
+        return;
+    }
+
     cbuf_put(&(vtaskmgr->wbuf), (void*)&(vcpu->cpuid));
     cbuf_get(&(vcpu->wbuf), (void*)&(vcpu->task));
     return;
@@ -112,6 +130,20 @@ static void* vcpu_entry(void* arg)
             pthread_setcancelstate(PTHREAD_CANCEL_ENABLE, NULL);
         } else {
             vcpu_idle(my);
+
+            /* vcpu_fini()'s bind_vtask(NULL, vcpu) is the ONLY caller
+             * that ever delivers a NULL task through this vcpu's own
+             * wbuf -- the dispatcher (vdispatcher_entry()) only calls
+             * bind_vtask() when pick_eligible_task() returned non-NULL.
+             * A NULL task observed here is therefore unambiguously the
+             * shutdown signal: exit immediately rather than looping back
+             * into vcpu_idle(), which would re-announce this vcpu into
+             * the shared vtaskmgr->wbuf after vtaskmgr_fini() has
+             * already torn down its sole consumer (the dispatcher
+             * thread) -- see vcpu_idle()'s own comment (bug-1810). */
+            if (my->task == NULL) {
+                break;
+            }
         }
         pthread_testcancel();
     }