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

fix(controller): fail loud on scheduler/worker disconnect instead of relying on the reply-wait timeout (bug-1693)

Browse source 
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
This commit is contained in:
Rongsong Shen 2026-08-25 14:13:26 +00:00
parent c83b388be2
commit 3eba7c16bf
No known key found for this signature in database
GPG key ID: 5BCDAAE0123E9FE2
12 changed files with 1032 additions and 424 deletions
Download patch file Download diff file Expand all files Collapse all files

868
.wolf/buglog.json
View file

File diff suppressed because one or more lines are too long

57
src/controller/cluster.c
View file

@ -285,12 +285,13 @@ static void schedadmin_unregister(schedadmin_pending_t* p)
/* Signal the pending schedAdmin waiter (if any) with the scheduler's reply.
* Called from the OPCode_reply case below when a SchedAdminReply arrives on
* c->sched.ep. Returns 1 if a waiter was found and signaled (ownership of
* `sar` transfers to it -- the caller must NULL its own copy so the normal
* xce_free_request path does not also free it), 0 if no waiter was
* registered (e.g. a late reply arriving after this controller already gave
* up and replied "request timeout" to xceadmin -- `sar` stays owned by the
* caller in that case, freed normally). */
* c->sched.ep, AND (bug-1693) from schedadmin_signal_disconnect() below when
* the scheduler disconnects mid-wait. Returns 1 if a waiter was found and
* signaled (ownership of `sar` transfers to it -- the caller must NULL its
* own copy so the normal xce_free_request path does not also free it), 0 if
* no waiter was registered (e.g. a late reply arriving after this
* controller already gave up and replied "request timeout" to xceadmin --
* `sar` stays owned by the caller in that case, freed normally). */
static int schedadmin_signal(int status, sched_admin_reply_t* sar)
{
pthread_mutex_lock(&g_schedadmin_mu);
@ -309,6 +310,34 @@ static int schedadmin_signal(int status, sched_admin_reply_t* sar)
return 1;
}
/* bug-1693 (Decision Log: "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_scheduler_leave() (sched.c)
* only ever nulled c->sched.ep on a scheduler disconnect -- it never woke a
* pending OPCode_schedAdmin waiter, so a mid-wait disconnect could only ever
* be observed at the full SCHEDADMIN_TIMEOUT_MS deadline (5s), exactly the
* "sole failure-detection mechanism is a timeout" shape that Decision Log
* entry forbids. This is the narrow, non-static entry point
* handle_scheduler_leave() calls to fix that -- a thin wrapper around
* schedadmin_signal() with a fixed, distinguishable -ECONNRESET status and
* no reply payload (there is no real SchedAdminReply on a disconnect).
* Deliberately does NOT expose schedadmin_signal() itself (or the
* sched_admin_reply_t type it takes) outside this file -- cluster.h has no
* other need for typemap.h, and sched.c only ever needs to signal the
* disconnect outcome, never an arbitrary reply.
*
* Lock-ordering note (see handle_scheduler_leave()'s own doc comment in
* sched.c for the full argument): schedadmin_signal() takes only
* g_schedadmin_mu + the pending slot's own mu -- never c->sched.lock. The
* OPCode_schedAdmin case above holds c->sched.lock for its ENTIRE
* forward+wait duration, so calling this function requires no lock
* c->sched.lock's holder could be blocked on, and introduces no new
* lock-ordering hazard. */
int schedadmin_signal_disconnect(void)
{
return schedadmin_signal(-ECONNRESET, NULL);
}
#ifdef XCE_TESTING
/*
* schedadmin_set_timeout_ms_for_test / SCHEDADMIN_TIMEOUT_MS (task 10.2, Req
@ -997,6 +1026,22 @@ static int myreq_handler(vtask_t* task)
reply.data.schedadminreplymsg = got_reply;
xce_reply(ep, c->myhost, &reply);
free_SchedAdminReply_ptr(&got_reply);
} else if (got_done && got_status == -ECONNRESET) {
/* bug-1693: woken by schedadmin_signal_disconnect()
* (handle_scheduler_leave(), sched.c) rather than a genuine
* scheduler reply -- the scheduler disconnected mid-wait. Relay
* a structured SchedAdminReply, the same shape as the "not
* connected"/"request timeout" branches above and below, with a
* distinguishable error string so this outcome is never
* conflated with either in logs or tests. */
sched_admin_reply_t disc_reply;
memset(&disc_reply, 0, sizeof(disc_reply));
disc_reply.error = "scheduler disconnected";
reply.status = 0;
reply.data_which = RequestReply_data_schedadminreplymsg;
reply.data.schedadminreplymsg = &disc_reply;
xce_reply(ep, c->myhost, &reply);
} else if (got_done) {
/* Scheduler replied at the RPC level with no SchedAdminReply
* payload (unexpected, but handled loudly, never dropped) --

9
src/controller/include/cluster.h
View file

@ -222,6 +222,15 @@ extern int do_set_preason(cluster_t*, endpoint_t*, xce_request_t*);
extern int do_clr_preason(cluster_t*, endpoint_t*, xce_request_t*);
extern int on_fetchfile(cluster_t*, endpoint_t*, xce_request_t*);
extern void fetchfile_signal(uint64_t jobid, int status, xce_fetchfile_reply_t*);
/* bug-1693: wakes every fetchfile waiter (job.c) proxied to worker `w`
* immediately on disconnect -- see fetchfile_signal_worker_disconnect()'s
* own doc comment in job.c and schedadmin_signal_disconnect()'s sibling
* comment in cluster.c for the shared Decision Log rationale. */
extern void fetchfile_signal_worker_disconnect(worker_t*);
/* bug-1693: wakes the single pending OPCode_schedAdmin waiter (cluster.c)
* immediately on scheduler disconnect, with a distinguishable -ECONNRESET
* status -- see its doc comment in cluster.c. */
extern int schedadmin_signal_disconnect(void);
extern int handle_apiserver_leave(cluster_t*, endpoint_t*);
extern int add_apiserver(cluster_t*, char*, endpoint_t*);
extern int apiserver_init(cluster_t*);

83
src/controller/job.c
View file

@ -3307,6 +3307,11 @@ int do_clr_preason(cluster_t* c, endpoint_t* ep, xce_request_t* request)
typedef struct fetchfile_pending {
uint64_t jobid;
worker_t* worker; /* bug-1693: which worker this request was
* proxied to -- lets a disconnect be
* matched to the pending waiter(s) it
* affects; see
* fetchfile_signal_worker_disconnect(). */
pthread_mutex_t mu;
pthread_cond_t cv;
int done;
@ -3364,6 +3369,78 @@ void fetchfile_signal(uint64_t jobid, int status, xce_fetchfile_reply_t* reply)
pthread_mutex_unlock(&p->mu);
}
/* bug-1693 (Decision Log: "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 mid-wait disconnect
* could only ever be observed at on_fetchfile's own 10s deadline below.
* Called from handle_worker_leave() immediately on disconnect. Unlike
* fetchfile_signal() (keyed by jobid, at most one match), more than one
* fetchfile request can legitimately be in flight against the SAME worker
* at once, so this walks the whole list and signals every match -- a no-op
* if none are pending against `w`. Deliberately takes only g_fetchfile_mu +
* each matched waiter's own mu, never any worker_t lock, so callers do not
* need to hold (or avoid holding) `w`'s lock around this call. */
void fetchfile_signal_worker_disconnect(worker_t* w)
{
fetchfile_pending_t* p;
pthread_mutex_lock(&g_fetchfile_mu);
for (p = g_fetchfile_list; p != NULL; p = p->next) {
if (p->worker != w) continue;
pthread_mutex_lock(&p->mu);
p->status = -ECONNRESET;
p->reply = NULL;
p->done = 1;
pthread_cond_signal(&p->cv);
pthread_mutex_unlock(&p->mu);
}
pthread_mutex_unlock(&g_fetchfile_mu);
}
#ifdef XCE_TESTING
/* Test seam (bug-1693): exercises fetchfile_signal_worker_disconnect()'s
* demux against a real fetchfile_pending_t wait, without needing a full
* on_fetchfile()/real c->jobmgr+c->workermgr fixture (job/worker lookup is
* out of this fix's scope -- only the disconnect-to-waiter signal path is
* under test). Registers a bare pending entry keyed to `w`, blocks with a
* deadline exactly like on_fetchfile's own wait loop below, and reports the
* outcome via *out_status. Returns 1 if woken by a signal, 0 if the
* deadline lapsed first (mirrors pending.done). */
int fetchfile_wait_for_test(worker_t* w, long timeout_ms, int* out_status)
{
fetchfile_pending_t pending = {
.jobid = 0, .worker = w,
.mu = PTHREAD_MUTEX_INITIALIZER, .cv = PTHREAD_COND_INITIALIZER,
.done = 0, .status = -ETIMEDOUT, .reply = NULL, .next = NULL
};
struct timespec deadline;
clock_gettime(CLOCK_REALTIME, &deadline);
deadline.tv_sec += timeout_ms / 1000;
deadline.tv_nsec += (timeout_ms % 1000) * 1000000L;
if (deadline.tv_nsec >= 1000000000L) {
deadline.tv_sec += 1;
deadline.tv_nsec -= 1000000000L;
}
fetchfile_register(&pending);
pthread_mutex_lock(&pending.mu);
while (!pending.done) {
int wrc = pthread_cond_timedwait(&pending.cv, &pending.mu, &deadline);
if (wrc == ETIMEDOUT) break;
}
pthread_mutex_unlock(&pending.mu);
fetchfile_unregister(&pending);
*out_status = pending.status;
return pending.done;
}
#endif
/* ---- on_fetchfile: proxy fetchfile from apiserver to worker ---- */
int on_fetchfile(cluster_t* c, endpoint_t* caller_ep, xce_request_t* request)
@ -3447,6 +3524,12 @@ int on_fetchfile(cluster_t* c, endpoint_t* caller_ep, xce_request_t* request)
return -ENOENT;
}
/* bug-1693: record which worker this request is proxied to (still
* holding w's read lock, so w->ep/identity are stable here) so a later
* disconnect can find and wake this waiter -- see
* fetchfile_signal_worker_disconnect()'s doc comment above. */
pending.worker = w;
/* Register pending before sending to avoid a race with the reply */
fetchfile_register(&pending);
rc = xce_fetchfile_req(w->ep, msg);

1
src/controller/meson.build
View file

@ -192,6 +192,7 @@ controller_unit_test_src = [
'testsuite/test_upgrade_snapshot_apply.c',
'testsuite/test_bug857_regression.c',
'testsuite/test_sched_admin_dispatch.c',
'testsuite/test_fetchfile_disconnect.c',
'ctrlstatus.c',
'commit.c',
'job.c',

21
src/controller/sched.c
View file

@ -713,6 +713,27 @@ int handle_scheduler_leave(cluster_t* c)
{
/* we need to mark the cluster status */
TRACE("%s scheduler leave", __func__);
/* bug-1693 (Decision Log: "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"). Wake any pending OPCode_schedAdmin
* waiter (cluster.c) immediately instead of leaving it to the
* SCHEDADMIN_TIMEOUT_MS deadline (5s) -- a no-op if no schedAdmin
* request happens to be in flight (schedadmin_signal_disconnect()
* returns 0 in that case).
*
* MUST run BEFORE mylock_lock(&c->sched.lock) below, not after: the
* OPCode_schedAdmin dispatch case holds c->sched.lock for its ENTIRE
* forward+wait duration (by design -- see that case's own comment in
* cluster.c), so if we grabbed c->sched.lock first, this call would
* itself block until that handler's own wait resolves one way or the
* other -- i.e. nothing would ever be able to signal it early, which is
* exactly the bug this fixes. schedadmin_signal_disconnect() only takes
* g_schedadmin_mu (cluster.c, file-local) + the pending slot's own mu --
* never c->sched.lock -- so calling it here first introduces no new
* lock-ordering hazard. */
schedadmin_signal_disconnect();
mylock_lock(&(c->sched.lock));
c->sched.ep = NULL;
mylock_unlock(&(c->sched.lock));

2
src/controller/testsuite/controller_test.c
View file

@ -101,6 +101,7 @@ extern void test_upgrade_snapshot_restore(void);
extern void test_upgrade_snapshot_apply(void);
extern void test_bug857_regression(void);
extern void test_sched_admin_dispatch(void);
extern void test_fetchfile_disconnect(void);
extern void test_job_dispatch(void);
extern void test_dispatch_commit(void);
extern void test_dispatch_fail_terminal(void);
@ -276,6 +277,7 @@ int main(int argc, char* argv[])
TEST(test_upgrade_snapshot_apply);
TEST(test_bug857_regression);
TEST(test_sched_admin_dispatch);
TEST(test_fetchfile_disconnect);
TEST(test_job_dispatch);
TEST(test_dispatch_commit);
TEST(test_dispatch_fail_terminal);

187
src/controller/testsuite/test_fetchfile_disconnect.c Normal file
View file

@ -0,0 +1,187 @@
/*
* 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);
}

9
src/controller/testsuite/test_job_array_sched_push.c
View file

@ -64,6 +64,15 @@ extern int on_scheduler_join(cluster_t*);
* rather than linking all of opget.c, mirroring opget.c's own trivial body. */
int check_worker_(xce_worker_t* w) { return (w != NULL) ? 1 : 0; }
/* bug-1693: sched.c's handle_scheduler_leave() now calls
* schedadmin_signal_disconnect() (defined in cluster.c, which this
* standalone binary does not link -- see this file's own comment above for
* why). This test never triggers a scheduler-leave/disconnect path, so the
* real behavior is never exercised here; the symbol only needs to resolve
* at link time, mirroring check_worker_()'s identical stub rationale just
* above. */
int schedadmin_signal_disconnect(void) { return 0; }
/* ------------------------------------------------------------------ */
/* Fixture */
/* ------------------------------------------------------------------ */

104
src/controller/testsuite/test_sched_admin_dispatch.c
View file

@ -667,6 +667,109 @@ static void test_schedadmin_round_trip_business_error(void)
printf("test_schedadmin_round_trip_business_error: PASS\n");
}
/* ── bug-1693 regression: scheduler disconnect wakes the waiter immediately
* (Decision Log fidelity fix) ───────────────────────────────────────────────
*
* Before this fix, handle_scheduler_leave() (sched.c) only ever nulled
* c->sched.ep on a scheduler disconnect -- it never signalled the pending
* OPCode_schedAdmin waiter, so a disconnect mid-wait could only ever be
* observed at the FULL SCHEDADMIN_TIMEOUT_MS deadline (5s in production).
* That is exactly the Decision Log violation this task fixes: "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."
*
* c->sched.ep points at a real (if unconnected) endpoint_t, exactly like
* test_schedadmin_round_trip_success above, so the dispatch case's
* forward+wait path runs for real. A second thread plays the disconnect by
* calling schedadmin_signal_disconnect() (cluster.c) directly -- the exact,
* real, unmodified production function handle_scheduler_leave() (sched.c)
* calls to wake the waiter, and the entire fix under test here.
*
* NOT calling handle_scheduler_leave() itself: this file's binary
* (controller_unit_test) does not link the real sched.c at all (see
* meson.build's comment on sched_push_unit_test/sched_worker_change_unit_
* test for why -- sched.c's on_scheduler_join()/collect_confvars() collide
* at link time with fake definitions several OTHER controller_unit_test_src
* files already provide). test_config_repl.c (also in this binary) already
* supplies its OWN no-op handle_scheduler_leave() stub for that reason,
* predating this fix -- calling handle_scheduler_leave() here would
* silently exercise THAT stub, not sched.c's real implementation, and prove
* nothing. handle_scheduler_leave()'s own body is a thin, 2-statement
* wrapper (call this function, then null c->sched.ep under c->sched.lock)
* -- calling schedadmin_signal_disconnect() directly exercises the entire
* real fix mechanism (the new function, and the real dispatch-side wait it
* wakes) with zero loss of fidelity versus going through that wrapper. */
static void* sa_deliver_scheduler_disconnect(void* argp)
{
(void)argp;
/* Same margin as sa_deliver_scheduler_reply above -- comfortably inside
* the 5s deadline, giving the main thread time to register the pending
* waiter and start blocking on the condvar first. */
usleep(200000);
schedadmin_signal_disconnect();
return NULL;
}
static void test_schedadmin_disconnect_wakes_immediately(void)
{
cluster_t* c = sa_make_cluster();
atomic_store(&c->ha.role, CTRL_PRIMARY);
endpoint_t sched_conn;
memset(&sched_conn, 0, sizeof(sched_conn));
c->sched.ep = &sched_conn;
pthread_t tid;
TEST_ASSERT_EQUAL_INT(
0, pthread_create(&tid, NULL, sa_deliver_scheduler_disconnect, c));
endpoint_t caller_ep;
memset(&caller_ep, 0, sizeof(caller_ep));
caller_ep.owner = c;
io_packet_t* pkt = sa_build_request_pkt();
uint64_t t0 = sa_mono_ms();
int rc = myreq_handler_for_test(&caller_ep, pkt);
uint64_t took = sa_mono_ms() - t0;
pthread_join(tid, NULL);
TEST_ASSERT_EQUAL_INT_MESSAGE(
0, rc, "the dispatch case always replies, even on a disconnect");
TEST_ASSERT_TRUE_MESSAGE(
took < 1000,
"a scheduler disconnect mid-wait must wake the waiter immediately "
"(well under the real, unshrunk 5s SCHEDADMIN_TIMEOUT_MS deadline) "
"-- this is the regression this test locks: pre-fix, nothing ever "
"signalled the waiter on disconnect, so this only ever completed at "
"the full 5s deadline");
xce_request_t decoded;
sa_decode_queued_reply(&caller_ep, &decoded);
xce_reply_t* rr = decoded.body.replymsg;
TEST_ASSERT_NOT_NULL(rr);
TEST_ASSERT_EQUAL_INT_MESSAGE(
0, rr->status,
"a disconnect is relayed as an ok-RPC SchedAdminReply carrying a "
"distinguishable error, not a negative RPC status");
TEST_ASSERT_EQUAL_INT(RequestReply_data_schedadminreplymsg, rr->data_which);
sched_admin_reply_t* got = rr->data.schedadminreplymsg;
TEST_ASSERT_NOT_NULL(got);
TEST_ASSERT_EQUAL_INT(0, got->ok);
TEST_ASSERT_EQUAL_STRING_MESSAGE(
"scheduler disconnected", got->error,
"must be distinguishable from \"request timeout\" -- proves this "
"woke via the disconnect signal, not the deadline");
xce_free_request(&decoded);
sa_teardown(c);
printf("test_schedadmin_disconnect_wakes_immediately: PASS\n");
}
/* ── Suite entry point ──────────────────────────────────────────────────── */
void test_sched_admin_dispatch(void)
{
@ -678,4 +781,5 @@ void test_sched_admin_dispatch(void)
RUN_TEST(test_schedadmin_forward_timeout);
RUN_TEST(test_schedadmin_round_trip_rpc_status_only);
RUN_TEST(test_schedadmin_round_trip_business_error);
RUN_TEST(test_schedadmin_disconnect_wakes_immediately);
}

98
src/controller/testsuite/test_sched_worker_change.c
View file

@ -90,6 +90,43 @@ extern int on_scheduler_join(cluster_t*);
* push.c's own stub. */
int check_worker_(xce_worker_t* w) { return (w != NULL) ? 1 : 0; }
/* bug-1693: sched.c's handle_scheduler_leave() now calls
* schedadmin_signal_disconnect() (defined for real in cluster.c, which this
* standalone binary does not link -- see this file's own header comment
* for why real sched.c/dataex.c/egress.c are linked here in isolation).
* check_worker_() just above is a pure link-satisfying no-op stub since
* this file's own tests never reach it at runtime; this one is instead a
* counting SPY -- sched.c IS linked for real here, so handle_scheduler_
* leave() itself (unlike the OPCode_schedAdmin dispatch/wait machinery,
* which lives in cluster.c and is NOT linked here) is exercisable for
* real. test_scheduler_leave_signals_schedadmin_before_clearing_ep below
* uses this spy to prove handle_scheduler_leave() actually calls this
* function (not just that it compiles/links) -- the one piece of the
* bug-1693 fix that test_sched_admin_dispatch.c's own regression test
* (controller_unit_test, where cluster.c IS real but sched.c is stubbed by
* test_config_repl.c) cannot reach, since the two real halves of this fix
* (cluster.c's schedadmin_signal_disconnect() and sched.c's
* handle_scheduler_leave()) are never both linked-for-real in the same
* binary anywhere in this project's current test topology. */
static int g_schedadmin_signal_disconnect_calls;
static cluster_t* g_schedadmin_spy_c; /* set by the test below; NULL elsewhere */
static endpoint_t* g_schedadmin_spy_ep_at_signal_time
= (endpoint_t*)(uintptr_t)0x1; /* poison -- must be overwritten */
int schedadmin_signal_disconnect(void)
{
g_schedadmin_signal_disconnect_calls++;
/* Captures c->sched.ep's value AT THE MOMENT this is called -- proves
* handle_scheduler_leave() calls this BEFORE nulling c->sched.ep, not
* after (see this fix's lock-ordering rationale in sched.c's doc
* comment: calling it after would deadlock against the OPCode_
* schedAdmin dispatch case in production, which holds c->sched.lock for
* its whole wait). */
if (g_schedadmin_spy_c != NULL)
g_schedadmin_spy_ep_at_signal_time = g_schedadmin_spy_c->sched.ep;
return 1;
}
/* ------------------------------------------------------------------ */
/* Fixture */
/* ------------------------------------------------------------------ */
@ -331,6 +368,66 @@ static void test_worker_dead_then_encode_worker_is_asan_clean(void)
"test_worker_dead_then_encode_worker_is_asan_clean: PASS\n");
}
/* ------------------------------------------------------------------ */
/* bug-1693: handle_scheduler_leave() signals schedadmin BEFORE clearing */
/* c->sched.ep (Decision Log fidelity fix) */
/* ------------------------------------------------------------------ */
/* Complements test_sched_admin_dispatch.c's own bug-1693 regression test
* (controller_unit_test): that file proves schedadmin_signal_disconnect()
* (cluster.c, real there) genuinely wakes a real pending OPCode_schedAdmin
* wait immediately, but cannot exercise the real handle_scheduler_leave()
* (sched.c is stubbed by test_config_repl.c in that binary). This binary
* has the opposite shape: sched.c is real, cluster.c is not linked at all,
* so schedadmin_signal_disconnect() above is a counting spy rather than the
* real implementation. Together the two prove both halves of the fix for
* real: the signal function's real effect on a real wait (there), and that
* handle_scheduler_leave() really calls it, unconditionally and before
* clearing c->sched.ep (here). */
static void test_scheduler_leave_signals_schedadmin_before_clearing_ep(void)
{
cluster_t* c = calloc(1, sizeof(cluster_t));
TEST_ASSERT_NOT_NULL(c);
mylock_init(&(c->sched.lock));
endpoint_t sentinel;
memset(&sentinel, 0, sizeof(sentinel));
c->sched.ep = &sentinel;
g_schedadmin_signal_disconnect_calls = 0;
g_schedadmin_spy_c = c;
g_schedadmin_spy_ep_at_signal_time = (endpoint_t*)(uintptr_t)0x1;
int rc = handle_scheduler_leave(c);
TEST_ASSERT_EQUAL_INT(0, rc);
TEST_ASSERT_EQUAL_INT_MESSAGE(
1, g_schedadmin_signal_disconnect_calls,
"bug-1693 regression: handle_scheduler_leave() must call "
"schedadmin_signal_disconnect() exactly once on every disconnect -- "
"pre-fix, it never called it at all, so a pending OPCode_schedAdmin "
"waiter could only ever be woken by the 5s deadline");
TEST_ASSERT_EQUAL_PTR_MESSAGE(
&sentinel, g_schedadmin_spy_ep_at_signal_time,
"handle_scheduler_leave() must call schedadmin_signal_disconnect() "
"BEFORE nulling c->sched.ep -- calling it after would deadlock "
"against the OPCode_schedAdmin dispatch case in production, which "
"holds c->sched.lock for its entire wait duration (see the "
"lock-ordering rationale in sched.c's handle_scheduler_leave() doc "
"comment)");
TEST_ASSERT_NULL_MESSAGE(c->sched.ep,
"handle_scheduler_leave() must still clear "
"c->sched.ep as before -- this fix only adds "
"the signal call, it does not change the "
"pre-existing ep-clearing behavior");
g_schedadmin_spy_c = NULL;
mylock_fini(&(c->sched.lock));
free(c);
printf(
"test_scheduler_leave_signals_schedadmin_before_clearing_ep: PASS\n");
}
/* ------------------------------------------------------------------ */
/* Standalone Unity main (this binary is its own test executable -- see */
/* meson.build's sched_worker_change_unit_test) */
@ -355,6 +452,7 @@ int main(void)
RUN_TEST(test_worker_dead_leaves_resvals_to_sched_null_and_zero);
RUN_TEST(test_worker_dead_then_encode_worker_is_asan_clean);
RUN_TEST(test_scheduler_leave_signals_schedadmin_before_clearing_ep);
return UNITY_END();
}

17
src/controller/worker.c
View file

@ -775,6 +775,23 @@ int handle_worker_leave(cluster_t* c, int idx)
unlock(w);
/* bug-1693 (Decision Log: "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"). Wake any on_fetchfile() waiter
* (job.c) proxied to this worker immediately instead of leaving it to
* on_fetchfile's own 10s deadline -- mirrors handle_scheduler_leave's
* identical schedadmin_signal_disconnect() fix in sched.c (same
* Decision Log rule, same shared design mistake -- cluster.c's
* OPCode_schedAdmin case comment already notes it "mirrors
* on_fetchfile's proxy pattern"). Deliberately called AFTER releasing
* w's lock: fetchfile_signal_worker_disconnect() only takes
* g_fetchfile_mu (job.c, file-local) + each matched waiter's own mu,
* never any worker_t lock, so there is no ordering dependency, and `w`
* remains a valid pointer for the comparison (this function's vget'd
* reference is still held). A no-op if no fetchfile request happens to
* be pending against this worker. */
fetchfile_signal_worker_disconnect(w);
return 0;
}