feat(xce-test-cli-ux): build interruptible-call heap-ownership mechanism

xcetest_run_interruptible() runs a blocking call on a detached worker
thread while the caller polls xcetest_interrupted() every 50ms. A single
atomic CAS (RUNNING -> DONE_UNCLAIMED | ABANDONED) decides which side
frees the heap-allocated call block, so an abandoned call's worker
thread never double-frees or leaks once it eventually finishes. Proven
under active ASAN with a real detached thread and a real SIGINT-forced
abandonment. 1282/1282 controller suite passing (+2).

Task 1.4 of .kiro/specs/xce-test-cli-ux/tasks.md.
Requirements: 1.1, 1.2, 1.3, 1.4

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019jaMf9QEwpugH2W9KAuc7f
This commit is contained in:
Rongsong Shen 2026-09-09 08:29:14 +00:00
parent b4f5a460fc
commit 9a06047da2
No known key found for this signature in database
GPG key ID: 5BCDAAE0123E9FE2
6 changed files with 467 additions and 2 deletions

View file

@ -23,7 +23,7 @@
- Observable completion: a small standalone test that sends an interrupt signal to the process during a blocking read confirms the read returns immediately with an interrupted result rather than continuing to block
- _Requirements: 9.9_
- [ ] 1.4 Build the interruptible-call adapter's heap-block-ownership mechanism
- [x] 1.4 Build the interruptible-call adapter's heap-block-ownership mechanism
- Implement a wrapper that runs a given blocking call on a detached worker thread while the caller polls the interruption accessor from 1.3 on a short interval
- Implement the atomic ownership handoff so that whichever side (the polling caller or the worker) is left holding the call's heap-allocated state is the one that frees it, with no path that frees it from both sides
- Observable completion: the unit test suite shows a normal (uninterrupted) call returning its real result and freeing cleanly, and a forced-timeout call returning immediately with no double-free and no leak (verified under the project's leak/ASAN tooling)
@ -178,4 +178,5 @@
## Implementation Notes
- `src/tools/xce-build.sh controller asan xce.build --incremental` writes its build dir at `src/xce.build/` (relative to the script's own location), not a repo-root `xce.build/` — meson logs are at `src/xce.build/meson-logs/testlog.txt`. (task 1.1)
- New Unity test files need two wiring points, not just meson.build's source list: also register an `extern void test_<module>(void);` + `TEST(test_<module>);` in `src/controller/testsuite/controller_test.c`, mirroring the existing `test_rcfile` registration, or the suite silently never runs. (task 1.1)
- `interruptcall.c`'s forced-abandonment test proves `-EINTR` via return-value disambiguation against a slow `fn`, not a direct wall-clock elapsed-time assertion — sound today (poll interval is 50ms, matching design.md) but a future regression widening the poll interval wouldn't be caught unless the test also asserts an elapsed-time bound. Non-blocking; noted for task 4.1's end-to-end Ctrl-C validation to consider. (task 1.4)
- After task 1.3's SIGINT/EINTR fix alone (before task 3.2 wires the REPL loop to check `xcetest_interrupted()`), a real Ctrl-C at `xce_test`'s top-level interactive prompt now causes the process to EXIT (readline's EINTR-driven NULL return is currently treated the same as EOF) rather than gracefully redisplaying the prompt. This is expected/deferred, not a regression — no current test exercises that path — but task 3.2's implementer must fix it as part of wiring the REPL loop, not just the RPC call sites. (task 1.3)

View file

@ -55,7 +55,7 @@ controller = executable('xce_controller',
link_args: wrap_link_args,
install: true)
xce_test_src = [ 'testsuite/test.c', 'testsuite/waldump.c', 'testsuite/walcompare.c', 'testsuite/snapverify.c', 'testsuite/snapdiff.c', 'testsuite/configcompare.c', 'testsuite/resval_typeresolve.c', 'testsuite/rcfile.c', 'testsuite/flagspec.c']
xce_test_src = [ 'testsuite/test.c', 'testsuite/waldump.c', 'testsuite/walcompare.c', 'testsuite/snapverify.c', 'testsuite/snapdiff.c', 'testsuite/configcompare.c', 'testsuite/resval_typeresolve.c', 'testsuite/rcfile.c', 'testsuite/flagspec.c', 'testsuite/interruptcall.c']
# BSD-licensed readline replacement (scheduler-core spec, task 12.1 review
# finding): GNU readline is GPL; xce_test's REPL only needs readline()/
@ -76,6 +76,7 @@ xce_test_deps = [
zlib_dep, # waldump: crc32 WAL validation
c_capnproto_dep, # waldump: decode JOB_SUBMIT jobid
ssl_dep, # configcompare: SHA256() active-config hashing
thread_dep, # interruptcall.c: xcetest_run_interruptible() detached worker (task 1.4)
]
xce_test = executable('xce_test',
xce_test_src,
@ -171,6 +172,8 @@ controller_unit_test_src = [
'testsuite/test_flagspec.c',
'testsuite/flagspec.c',
'testsuite/test_xcetest_interrupt.c',
'testsuite/test_interruptcall.c',
'testsuite/interruptcall.c',
'testsuite/test_persist_snapshot_entities.c',
'testsuite/test_apply_seam.c',
'testsuite/test_apply_committed_entry.c',

View file

@ -73,6 +73,7 @@ extern void test_configcompare(void);
extern void test_rcfile(void);
extern void test_flagspec(void);
extern void test_xcetest_interrupt(void);
extern void test_interruptcall(void);
extern void test_drive_async_config(void);
extern void test_slowlane(void);
extern void test_commit_admission_pool(void);
@ -255,6 +256,7 @@ int main(int argc, char* argv[])
TEST(test_rcfile);
TEST(test_flagspec);
TEST(test_xcetest_interrupt);
TEST(test_interruptcall);
TEST(test_drive_async_config);
TEST(test_slowlane);
TEST(test_commit_admission_pool);

View file

@ -0,0 +1,150 @@
#include "interruptcall.h"
#include <errno.h>
#include <pthread.h>
#include <stdlib.h>
#include <time.h>
#include "xcetest_interrupt.h"
/* design.md's Interruptible Call Adapter component: poll interval is a
* fixed 50ms, per the component's own Implementation Notes ("poll
* break_out every fifty milliseconds"). Fast enough that an operator's
* Ctrl-C feels immediate; slow enough not to spin the calling thread. */
#define XCETEST_POLL_INTERVAL_NS (50L * 1000L * 1000L) /* 50ms */
/* The heap-allocated call block backing one xcetest_run_interruptible()
* call. `state` is the single atomic word whose CAS transitions decide
* who frees this block -- see interruptcall.h's xcetest_call_state_t
* comment and xcetest_run_interruptible()'s own header comment for the
* full protocol. */
typedef struct {
xcetest_blocking_fn fn;
void* ctx;
int rc;
_Atomic xcetest_call_state_t state;
} xcetest_call_block_t;
#ifdef XCE_TESTING
_Atomic int xcetest_test_abandoned_free_count = 0;
#endif
/* Runs on the detached worker thread. Invokes fn(ctx), stores its result,
* then resolves ownership of `block` via the RUNNING -> DONE_UNCLAIMED
* CAS: on success, the caller (still polling, or about to read the result
* of its own successful poll) owns the block from here and this function
* must not touch it again. On failure, the caller has already forced
* RUNNING -> ABANDONED and returned -EINTR without touching the block
* again -- this thread now owns it and frees it. */
static void* xcetest_worker_main(void* arg)
{
xcetest_call_block_t* block = (xcetest_call_block_t*)arg;
xcetest_call_state_t expected;
block->rc = block->fn(block->ctx);
expected = XCT_RUNNING;
if (atomic_compare_exchange_strong(&block->state, &expected,
XCT_DONE_UNCLAIMED)) {
/* Won the CAS: the caller still owns the block. Do not touch it
* again from this thread. */
return NULL;
}
/* CAS failed. The caller is the only other party that ever writes
* block->state, and the only value it ever writes is XCT_ABANDONED --
* so `expected` (and the block's actual state) can only be
* XCT_ABANDONED here. The caller returned -EINTR without freeing;
* this thread now owns the block and must free it. */
free(block);
#ifdef XCE_TESTING
atomic_fetch_add(&xcetest_test_abandoned_free_count, 1);
#endif
return NULL;
}
/* Sleeps one poll interval. A short nanosleep() legitimately interrupted
* early by a delivered signal (e.g. SIGINT, once xcetest_interrupt.h's
* SA_RESTART-cleared installation is in effect) just means the next
* poll-loop iteration happens slightly sooner than 50ms -- harmless, so
* the return value is deliberately ignored. */
static void xcetest_sleep_poll_interval(void)
{
struct timespec ts;
ts.tv_sec = 0;
ts.tv_nsec = XCETEST_POLL_INTERVAL_NS;
nanosleep(&ts, NULL);
}
int xcetest_run_interruptible(xcetest_blocking_fn fn, void* ctx)
{
xcetest_call_block_t* block;
pthread_attr_t attr;
pthread_t worker;
int rc;
if (!fn)
return -EINVAL;
block = malloc(sizeof(*block));
if (!block)
return -ENOMEM;
block->fn = fn;
block->ctx = ctx;
block->rc = 0;
atomic_init(&block->state, XCT_RUNNING);
rc = pthread_attr_init(&attr);
if (rc != 0) {
free(block);
return -rc;
}
/* Detached: no one ever joins this thread. That is precisely why
* ownership of `block` must be resolved by the CAS protocol below
* rather than by a join -- an abandoned caller returns without
* waiting for this thread at all. */
pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED);
rc = pthread_create(&worker, &attr, xcetest_worker_main, block);
pthread_attr_destroy(&attr);
if (rc != 0) {
/* The worker never started, so fn() was never invoked and this
* thread unambiguously still owns the block by itself. */
free(block);
return -rc;
}
for (;;) {
xcetest_call_state_t state = atomic_load(&block->state);
if (state == XCT_DONE_UNCLAIMED) {
int result = block->rc;
free(block);
return result;
}
if (xcetest_interrupted()) {
xcetest_call_state_t expected = XCT_RUNNING;
if (atomic_compare_exchange_strong(&block->state, &expected,
XCT_ABANDONED)) {
/* Won the CAS: the worker (still running fn()) will see
* ABANDONED via its own failed CAS once fn() returns and
* free the block itself. Must not touch `block` again
* from here. */
return -EINTR;
}
/* CAS failed: the worker is the only other party that ever
* writes block->state, and the only value it ever writes is
* XCT_DONE_UNCLAIMED -- so the block's actual state can only
* be XCT_DONE_UNCLAIMED here. The worker won the race right
* as the interrupt was noticed; fall through (via the loop's
* next iteration) to the normal completion path above
* instead of returning -EINTR, since the call genuinely
* finished. */
continue;
}
xcetest_sleep_poll_interval();
}
}

View file

@ -0,0 +1,143 @@
#pragma once
/*
* interruptcall.h -- xcetest_run_interruptible(): a detached-worker-thread
* wrapper that lets a command make a blocking xce_* RPC call cancellably
* from the operator's Ctrl-C, without ever touching libxce's own blocking
* primitive (xce-test-cli-ux spec, task 1.4; Requirements 1.1, 1.2, 1.3,
* 1.4).
*
* ROOT CAUSE this module addresses: every blocking xce_* call
* (src/lib/xceapi.c) waits on pthread_cond_timedwait() with a fixed
* 10-second deadline (cbuf_deadline_10s/cbuf_timedget, src/lib/mylock.c) --
* a wait a signal-setting flag alone cannot interrupt (glibc's condvar
* wait retries internally). Modifying that shared libxce primitive is out
* of this spec's boundary (research.md's Architecture Pattern Evaluation:
* "Modify libxce's cbuf_timedget to poll a cancellation token" -- Rejected,
* out of boundary). The fix is entirely client-side: run the existing
* blocking call, UNMODIFIED, on a detached worker thread; have the calling
* thread poll xcetest_interrupted() (xcetest_interrupt.h, task 1.3) on a
* short interval instead of blocking on the call itself; on interrupt,
* abandon the wait and return immediately, leaving the worker thread to
* finish (or hit its own 10s deadline) on its own -- satisfying "leave the
* connection intact" (Requirement 1.5) and the design's "cancelling a
* client-side wait does not need to cancel the server-side request" note.
*
* THE HARD PART this module exists to get right: the worker thread and the
* calling thread both have a legitimate reason to touch the heap-allocated
* call state backing one call (the worker to run fn()/store its result;
* the caller to read that result) -- but at most ONE of them may free it,
* and it must be resolved without a lock the caller would have to block
* on (that would defeat the whole point of "abandon immediately"). See
* design.md's "Heap-Block Ownership Handoff" state diagram and
* research.md's "Decision: Interruptible RPC via detached worker thread
* with CAS-based ownership handoff" for the full rationale, including why
* pthread_cancel() was rejected (cancellation-safety of
* pthread_cond_timedwait()'s internal state is not established).
*
* NOTE ON SCOPE (task 1.4 boundary): this module has NO notion of
* "connection identity" at all -- it does not know or care what `ctx`
* represents, and does not serialize two calls against each other in any
* way. The per-connection lock registry that serializes overlapping calls
* sharing a connection (including a call issued right after a prior one
* on that connection was abandoned) is task 1.5's addition, layered on
* top of this exact xcetest_run_interruptible() signature -- deliberately
* unchanged by task 1.5 so it can add that behavior without this module
* needing to guess its shape.
*/
#include <stdatomic.h>
/* A blocking call this module runs on a detached worker thread. `ctx` is
* whatever heap-owned state `fn` needs -- see xcetest_run_interruptible()'s
* preconditions below for the one hard rule about what `fn` may touch. */
typedef int (*xcetest_blocking_fn)(void* ctx);
/* The heap-allocated call block's lifecycle, resolved by exactly one
* atomic compare-and-swap regardless of which side "wins":
* RUNNING -- fn() has not yet returned, and no one has decided
* to abandon the call yet. Initial state.
* DONE_UNCLAIMED -- fn() returned and the worker's CAS from RUNNING to
* this state succeeded (i.e. the caller had not yet
* abandoned it). The CALLER is left owning the
* block: it reads rc and frees it.
* ABANDONED -- the caller's CAS from RUNNING to this state
* succeeded (i.e. it observed an interrupt before
* fn() finished). The WORKER is left owning the
* block: once fn() eventually returns, its own CAS
* attempt (RUNNING -> DONE_UNCLAIMED) fails against
* this state, and it frees the block itself instead.
* See design.md's "Heap-Block Ownership Handoff" stateDiagram-v2 -- this
* enum's three values are exactly its three states. */
typedef enum {
XCT_RUNNING,
XCT_DONE_UNCLAIMED,
XCT_ABANDONED
} xcetest_call_state_t;
/*
* Runs `fn(ctx)` on a detached worker thread while THIS (the calling)
* thread polls xcetest_interrupted() every ~50ms (design.md's Interruptible
* Call Adapter component), so a Ctrl-C observed during the wait abandons
* it immediately instead of blocking for fn()'s own, possibly much
* longer, duration.
*
* Preconditions:
* - `fn` is non-NULL.
* - `fn` touches only what `ctx` itself heap-owns -- NEVER memory on
* this function's caller's own stack frame. This is the precondition
* that makes the -EINTR postcondition below safe: once ownership of
* `ctx` has passed to the still-running worker, the caller's own
* stack frame may have long since unwound.
*
* Postconditions:
* - Returns fn()'s own return value when fn() finishes before this
* thread's abandon-CAS wins the race (whether or not
* xcetest_interrupted() ever became true during the wait) -- see the
* header comment's "near-simultaneous" case below.
* - Returns -EINTR when this thread observes an interrupt and its own
* CAS (RUNNING -> ABANDONED) wins the race. On this return, the
* caller MUST NOT touch `ctx` again -- ownership has passed to the
* still-running worker thread, which will finish fn() and free the
* call block on its own, asynchronously, with no further signal back
* to this caller.
* - Returns -EINVAL if `fn` is NULL (fn is never invoked in this case).
* - Returns a negative errno from pthread_attr_init()/pthread_create()
* if the worker thread could not be set up or spawned at all; the
* call block is freed before returning and fn() is never invoked in
* this case either.
*
* The near-simultaneous race (this thread notices the interrupt in the
* same window the worker finishes fn()) is resolved by the SAME single
* CAS deciding both sides at once: whichever of {this thread's
* RUNNING->ABANDONED attempt, the worker's RUNNING->DONE_UNCLAIMED
* attempt} executes first wins, and the loser observes that the state is
* no longer RUNNING and falls back to the OTHER side's outcome -- a
* caller who loses the abandon-CAS falls through to the normal
* completion path (reads rc, frees the block, returns rc) instead of
* returning -EINTR; a worker who loses the done-CAS frees the block
* itself instead of leaving it for the caller. Exactly one free() of the
* block is therefore reachable on every possible interleaving -- never
* zero, never two.
*
* Does not acquire any lock and has no notion of connection identity --
* see the file header's "NOTE ON SCOPE" above.
*/
int xcetest_run_interruptible(xcetest_blocking_fn fn, void* ctx);
#ifdef XCE_TESTING
/*
* Test-only instrumentation, NOT part of the production API and never
* even declared unless XCE_TESTING is defined (i.e. never visible to the
* real xce_test binary): incremented (via atomic_fetch_add) by the worker
* thread immediately after it frees an ABANDONED call block.
*
* Exists solely so test_interruptcall.c's forced-abandonment test can
* confirm the abandoned worker's asynchronous cleanup genuinely completed
* before the test process exits, rather than merely hoping a fixed sleep
* was long enough -- a still-running detached worker racing a leak
* detector's exit-time check could otherwise either mask a real bug or
* (more likely, if the implementation above is correct) flag a false
* leak on an allocation that was always going to be freed, just not yet.
*/
extern _Atomic int xcetest_test_abandoned_free_count;
#endif

View file

@ -0,0 +1,166 @@
/*
* test_interruptcall.c -- proves xcetest_run_interruptible()'s (task 1.4,
* Requirements 1.1, 1.2, 1.3, 1.4) heap-block CAS ownership handoff: a
* normal/fast call returns its real result and frees cleanly, and a
* forced-abandonment call returns -EINTR immediately with no double-free
* and no leak -- the abandoned worker thread finishes in the background
* and frees the block itself, proven via a side channel rather than
* assumed from a fixed sleep.
*
* xcetest_interrupted() (declared in xcetest_interrupt.h) is normally
* defined in test.c, over test.c's own private break_out flag -- but
* test.c itself is never linked into controller_unit_test (see
* xcetest_interrupt.h's own header comment: test.c pulls in libedit/
* cJSON/libxce/Cap'n Proto/OpenSSL and defines its own main(), both of
* which make it impractical to link into a small Unity binary). So this
* file provides its OWN local fake definition of xcetest_interrupted(),
* over its own local flag -- the same "local fake standing in for a
* real dependency this binary never links" pattern already used
* elsewhere in this directory (e.g. test_commit_barrier_reply.c's/
* test_worker_join.c's own fakes for on_scheduler_join()/notify()/
* collect_confvars(), per src/controller/meson.build's comment on
* sched_push_unit_test_src). interruptcall.c only ever calls
* xcetest_interrupted() through its documented pure-read contract, so a
* fake reading a test-local flag exercises interruptcall.c's real CAS
* state machine exactly as production would.
*
* The forced-abandonment test triggers a REAL, OS-delivered SIGINT (via
* kill(getpid(), SIGINT), sent from a small helper thread once the
* worker has had time to start) rather than flipping the fake flag
* directly by hand, and installs its handler via xcetest_install_sigint()
* (task 1.3) -- the exact installer test.c's own main() uses -- so this
* test also doubles as a real, signal-level exercise of the same
* interrupt path a live xce_test process would take, not a purely
* hand-simulated one. This is choice (ii) from the task brief (a real
* SIGINT), preferred over choice (i) (a direct break_out setter) because
* no such setter exists on the REAL break_out anyway (xcetest_interrupt.h
* deliberately exposes none) -- and a real signal exercises the actual
* delivery path end-to-end, the same thing test_xcetest_interrupt.c
* already does for xcetest_install_sigint() itself.
*/
#include <errno.h>
#include <pthread.h>
#include <signal.h>
#include <stdatomic.h>
#include <time.h>
#include <unistd.h>
#include <unity/unity.h>
#include "interruptcall.h"
#include "xcetest_interrupt.h"
/* --- local fake for xcetest_interrupted() (see file header) --- */
static volatile sig_atomic_t g_test_break_out = 0;
int xcetest_interrupted(void) { return g_test_break_out; }
static void test_sigint_handler(int signo)
{
(void)signo;
g_test_break_out = 1;
}
/* ---- Scenario (a): normal, uninterrupted completion ---- */
#define NORMAL_FN_RC 7
static int fast_fn(void* ctx)
{
(void)ctx;
return NORMAL_FN_RC;
}
static void test_normal_completion_returns_real_result(void)
{
g_test_break_out = 0;
int rc = xcetest_run_interruptible(fast_fn, NULL);
TEST_ASSERT_EQUAL_INT(NORMAL_FN_RC, rc);
/* No side channel to poll here: on this path the CALLING thread's own
* poll loop reads block->rc and frees the block synchronously, before
* xcetest_run_interruptible() ever returns -- so by the time the
* TEST_ASSERT_EQUAL_INT above runs, cleanup has unconditionally
* already happened. The remaining proof is this whole suite running
* clean under ASAN (no leak, no double-free) in the validating
* build+test run. */
}
/* ---- Scenario (b): forced abandonment via a real SIGINT ---- */
#define SLOW_FN_RC 99
#define SLOW_FN_SLEEP_MS 900
#define SIGNAL_DELAY_MS 150
#define WAIT_FOR_FREE_MS 3000
#define POLL_STEP_MS 20
static int slow_fn(void* ctx)
{
(void)ctx;
struct timespec ts;
ts.tv_sec = 0;
ts.tv_nsec = (long)SLOW_FN_SLEEP_MS * 1000L * 1000L;
nanosleep(&ts, NULL);
return SLOW_FN_RC;
}
/* Runs on its own helper thread so it can deliver SIGINT to the process
* WHILE the main test thread is blocked inside
* xcetest_run_interruptible()'s poll loop below. */
static void* send_sigint_after_delay(void* arg)
{
(void)arg;
struct timespec ts;
ts.tv_sec = 0;
ts.tv_nsec = (long)SIGNAL_DELAY_MS * 1000L * 1000L;
nanosleep(&ts, NULL);
kill(getpid(), SIGINT);
return NULL;
}
static void test_forced_abandonment_returns_eintr_and_worker_cleans_up(void)
{
g_test_break_out = 0;
atomic_store(&xcetest_test_abandoned_free_count, 0);
xcetest_install_sigint(test_sigint_handler);
pthread_t signaler;
TEST_ASSERT_EQUAL_INT(
0, pthread_create(&signaler, NULL, send_sigint_after_delay, NULL));
/* SLOW_FN_SLEEP_MS (900ms) comfortably exceeds SIGNAL_DELAY_MS (150ms)
* plus several 50ms poll intervals, so the interrupt is reliably
* noticed well before slow_fn() itself would have returned -- this
* call is expected to return -EINTR, not SLOW_FN_RC. */
int rc = xcetest_run_interruptible(slow_fn, NULL);
TEST_ASSERT_EQUAL_INT(-EINTR, rc);
TEST_ASSERT_EQUAL_INT(0, pthread_join(signaler, NULL));
/* The abandoned worker is still running slow_fn() in the background
* at this point. Poll the XCE_TESTING-only side channel (NOT part of
* the production API) instead of a single fixed sleep, so this test
* proves the abandoned worker's asynchronous cleanup actually
* happened -- confirming no leak -- rather than merely hoping enough
* wall-clock time passed before the process exits. */
int waited_ms = 0;
while (atomic_load(&xcetest_test_abandoned_free_count) == 0
&& waited_ms < WAIT_FOR_FREE_MS) {
struct timespec poll;
poll.tv_sec = 0;
poll.tv_nsec = (long)POLL_STEP_MS * 1000L * 1000L;
nanosleep(&poll, NULL);
waited_ms += POLL_STEP_MS;
}
TEST_ASSERT_EQUAL_INT(1, atomic_load(&xcetest_test_abandoned_free_count));
}
void test_interruptcall(void)
{
UnitySetTestFile(__FILE__);
RUN_TEST(test_normal_completion_returns_real_result);
RUN_TEST(test_forced_abandonment_returns_eintr_and_worker_cleans_up);
}