myxce/src/controller/testsuite/test_fetchfile_disconnect.c
Rongsong Shen 3eba7c16bf
fix(controller): fail loud on scheduler/worker disconnect instead of relying on the reply-wait timeout (bug-1693)
Violates this project's own recorded Decision Log entry: "No client
reply-wait timeout as a fix for a hung request -- fail loud at the real
transport/source instead of masking with a deadline backstop."

OPCode_schedAdmin (cluster.c) and on_fetchfile (job.c, the worker proxy
-- cluster.c's own comment says schedAdmin "mirrors on_fetchfile's proxy
pattern", confirming one shared design mistake, not two independent
bugs) both blocked on a multi-second pthread_cond_timedwait, replying
"timeout" on expiry. handle_scheduler_leave()/handle_worker_leave() (the
actual disconnect handlers) only nulled the endpoint pointer -- neither
ever signaled the pending waiter, so a mid-wait disconnect was invisible
until the deadline lapsed. The timeout was the ONLY failure-detection
mechanism, exactly what the decision forbids.

Both sites already had a working signal function on their success path
(schedadmin_signal(), fetchfile_signal()) -- just never wired to the
disconnect handlers. Fixed by calling them from
handle_scheduler_leave()/handle_worker_leave() with a distinguishable
-ECONNRESET outcome, so the waiter wakes immediately on disconnect
instead of only on the deadline. Timeouts kept as defensive backstops
for a genuinely wedged peer that never disconnects cleanly either --
this fix is about not relying SOLELY on the timeout, not removing it.

schedadmin_signal_disconnect() must be called BEFORE handle_scheduler_leave()
acquires c->sched.lock -- the dispatch wait holds that same lock for its
entire duration, so signaling after acquiring it would deadlock.

Found by an /xce-audit-invariants controller pass (Decision Log fidelity
dimension). New test_fetchfile_disconnect.c plus extensions to
test_sched_admin_dispatch.c/test_sched_worker_change.c; RED/GREEN
independently verified against real elapsed-time assertions (pre-fix:
5001ms full-timeout wait; post-fix: <1s immediate wake). Full controller
suite: 1181 tests, 2 failures -- both the pre-existing, unrelated
bug-1561 overlayfs sandbox flake, confirmed via a clean meson-driven run.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AgYRU3zDTqNGV5Jk8aiKLg
2026-08-25 14:13:26 +00:00

187 lines
8.3 KiB
C

/*
* test_fetchfile_disconnect.c — bug-1693 regression: a worker disconnect
* mid-wait wakes on_fetchfile()'s pending waiter immediately (Decision Log
* fidelity fix).
*
* Background: cluster.c's OPCode_schedAdmin dispatch case documents itself
* as mirroring on_fetchfile's proxy pattern (job.c) -- register a waiter,
* forward async, block on a condvar with a deadline. An invariant audit
* found BOTH sites violated the same recorded Decision Log entry: "No
* client reply-wait timeout as a fix for a hung request -- fail loud at the
* real transport/source instead of masking with a deadline backstop."
* Before this fix, handle_worker_leave() (worker.c) only ever nulled w->ep
* on a worker disconnect -- it never woke a pending on_fetchfile() waiter
* proxied to that worker, so a disconnect mid-wait could only ever be
* observed at on_fetchfile's own 10s deadline.
*
* The fix: fetchfile_signal_worker_disconnect(worker_t*) (job.c) walks
* every registered fetchfile_pending_t and wakes each one whose ->worker
* matches the disconnected worker with a distinguishable -ECONNRESET
* status (not the deadline's -ETIMEDOUT). handle_worker_leave() now calls
* it immediately, right after nulling w->ep.
*
* Test strategy for THIS file: on_fetchfile() itself needs a real,
* RTTI-registered job_t/worker_t pair reachable through c->jobmgr/
* c->workermgr (key_find + lock()/reference() -- see on_fetchfile's own
* body, job.c) to run end-to-end; building that fixture is squarely a
* job/worker-lookup concern this fix's scope explicitly does not touch
* (only the disconnect-to-waiter SIGNAL path is in scope). So this test
* exercises the real fetchfile_signal_worker_disconnect() function against
* a real fetchfile_pending_t wait, using job.c's XCE_TESTING-only seam
* fetchfile_wait_for_test() (mirrors on_fetchfile's own register/wait/
* unregister sequence exactly, including a real pthread_cond_timedwait
* deadline) instead of the full on_fetchfile()/handle_worker_leave() call
* chain -- a "direct unit test of the disconnect-handler's new signal call
* reaching the right wait context", per this task's own stated minimum bar
* for whichever of the two sites doesn't get the full live two-thread
* reproduction (that's test_sched_admin_dispatch.c's
* test_schedadmin_disconnect_wakes_immediately, which does drive the real
* handle_scheduler_leave() end-to-end against a real dispatch wait).
* worker_t itself is a plain, stack-owned struct here (not routed through
* makeWorker()/wobj.c's RTTI machinery) -- unneeded, since neither
* fetchfile_signal_worker_disconnect() nor fetchfile_wait_for_test() ever
* locks/references it; only pointer identity (matching ->worker) matters,
* mirroring test_sched_worker_change.c's identical "plain struct, not
* through the real object constructor" rationale.
*/
#define _GNU_SOURCE
#include <errno.h>
#include <pthread.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <time.h>
#include <unistd.h>
#include <unity/unity.h>
#include "cluster.h"
#include "xce/cluster_def.h"
/* Test-only seam (job.c, XCE_TESTING-only) -- see this file's header
* comment above for why it's used instead of a full on_fetchfile()
* fixture. */
extern int fetchfile_wait_for_test(worker_t* w, long timeout_ms,
int* out_status);
/* Monotonic milliseconds -- same construction as test_sched_admin_
* dispatch.c's sa_mono_ms() / test_commit_resolve.c's mono_ms_r(). */
static uint64_t ff_mono_ms(void)
{
struct timespec ts;
clock_gettime(CLOCK_MONOTONIC, &ts);
return (uint64_t)ts.tv_sec * 1000u + (uint64_t)ts.tv_nsec / 1000000u;
}
typedef struct {
worker_t* w;
} ff_disconnect_arg_t;
static void* ff_deliver_disconnect(void* argp)
{
ff_disconnect_arg_t* arg = (ff_disconnect_arg_t*)argp;
/* Give the main thread time to register the pending waiter and start
* blocking on the condvar first -- comfortably inside the backstop
* deadline below, same margin convention as test_sched_admin_
* dispatch.c's sa_deliver_scheduler_reply. */
usleep(200000);
fetchfile_signal_worker_disconnect(arg->w);
return NULL;
}
/* ── Regression: disconnect wakes the waiter immediately, not at the
* deadline ──────────────────────────────────────────────────────────────── */
static void test_fetchfile_worker_disconnect_wakes_immediately(void)
{
worker_t w;
memset(&w, 0, sizeof(w));
ff_disconnect_arg_t arg = { .w = &w };
pthread_t tid;
TEST_ASSERT_EQUAL_INT(0,
pthread_create(&tid, NULL, ff_deliver_disconnect, &arg));
/* Backstop deadline: much larger than the ~200ms disconnect delay above,
* so a genuine wake is trivially distinguishable from a regression that
* falls through to the deadline (asserted on below via elapsed time) --
* but far shorter than production's real 10s on_fetchfile deadline, so
* a regression fails this test in bounded time rather than hanging for
* 10s. */
const long timeout_ms = 5000;
int status = 12345; /* poison -- must be overwritten */
uint64_t t0 = ff_mono_ms();
int woke = fetchfile_wait_for_test(&w, timeout_ms, &status);
uint64_t took = ff_mono_ms() - t0;
pthread_join(tid, NULL);
TEST_ASSERT_TRUE_MESSAGE(
woke,
"must be woken by the disconnect signal, not merely reach the "
"backstop deadline with pending.done still 0");
TEST_ASSERT_EQUAL_INT_MESSAGE(
-ECONNRESET, status,
"a worker disconnect must be relayed with a distinguishable "
"-ECONNRESET status, never the deadline's -ETIMEDOUT");
TEST_ASSERT_TRUE_MESSAGE(
took < 1000,
"a worker disconnect mid-wait must wake the waiter immediately "
"(well under the 5s backstop used by this test, and far under "
"production's real 10s on_fetchfile deadline) -- this is the "
"regression this test locks: pre-fix, handle_worker_leave() never "
"signalled fetchfile waiters at all, so this would only ever "
"complete at the deadline with status -ETIMEDOUT");
printf("test_fetchfile_worker_disconnect_wakes_immediately: PASS\n");
}
/* ── A disconnect on an UNRELATED worker must not wake this waiter ────────
*
* Proves fetchfile_signal_worker_disconnect() actually matches by worker
* identity (the ->worker field on fetchfile_pending_t) rather than waking
* every registered waiter indiscriminately -- multiple fetchfile requests
* against different workers must not cross-signal each other. */
static void test_fetchfile_worker_disconnect_does_not_wake_other_worker(void)
{
worker_t w_target, w_other;
memset(&w_target, 0, sizeof(w_target));
memset(&w_other, 0, sizeof(w_other));
ff_disconnect_arg_t arg = { .w = &w_other };
pthread_t tid;
TEST_ASSERT_EQUAL_INT(0,
pthread_create(&tid, NULL, ff_deliver_disconnect, &arg));
/* Short backstop -- this test's whole point is that NOTHING wakes it
* early, so the wait genuinely runs out; keep it small to keep the test
* fast rather than proving a large deadline elapses. */
const long timeout_ms = 400;
int status = 12345;
int woke = fetchfile_wait_for_test(&w_target, timeout_ms, &status);
pthread_join(tid, NULL);
TEST_ASSERT_FALSE_MESSAGE(
woke,
"a disconnect on a DIFFERENT worker must not wake this waiter -- "
"fetchfile_signal_worker_disconnect() must match by worker "
"identity, not signal indiscriminately");
TEST_ASSERT_EQUAL_INT_MESSAGE(-ETIMEDOUT, status,
"an unmatched waiter must fall through to "
"the ordinary deadline outcome");
printf(
"test_fetchfile_worker_disconnect_does_not_wake_other_worker: PASS\n");
}
/* ── Suite entry point ──────────────────────────────────────────────────── */
void test_fetchfile_disconnect(void)
{
UnitySetTestFile(__FILE__);
RUN_TEST(test_fetchfile_worker_disconnect_wakes_immediately);
RUN_TEST(test_fetchfile_worker_disconnect_does_not_wake_other_worker);
}