feat(scheduler-core): add controller-side OPCode_schedAdmin dispatch (task 10.1)

Adds the spec's one and only src/controller/ change: a new case OPCode_schedAdmin in myreq_handler() (cluster.c) that mirrors the existing OPCode_upgradeConfig case -- primary-only gate, explicit reply on every path, stateless pass-through with no WAL entry. Forwards to the scheduler via a new xce_schedadmin_req() (async send, mirrors xce_fetchfile_req) and blocks the vtask thread on a condvar with a 5s deadline while holding c->sched.lock for the round-trip, matching this spec's own research-phase design decision (parity with OPCode_upgradeConfig's no-RBAC posture, blocking wait accepted for low-frequency admin traffic). A new branch in the existing OPCode_reply case demuxes the scheduler's SchedAdminReply to a process-global single-slot pending-waiter mechanism, serialized by c->sched.lock, matching job.c's existing fetchfile-proxy pattern. Adversarial review independently traced double-free/cleanup, the timeout/signal race, and reply-opcode demux correctness against source; found and a follow-up round closed a narrow-window leak via a carefully lock-ordered unlock->unregister->relock sequence (avoiding an AB-BA inversion against schedadmin_signal()'s lock acquisition order). New test_sched_admin_dispatch.c drives the real myreq_handler via the existing myreq_handler_for_test seam: not-primary rejection (proven via a poison-pointer sched.ep that would crash if forwarding were ever attempted), not-connected rejection, and a genuine two-thread round-trip test exercising the real signal/demux path. Controller suite 3/3 (1090/1090 assertions), scheduler suite 26/26 unaffected, clang-format clean.
2026-08-06 03:38:42 +00:00 · 2026-08-06 03:38:42 +00:00 · 045be6b3d6
commit 045be6b3d6
parent 2d533e652e
7 changed files with 598 additions and 3 deletions
--- a/src/controller/testsuite/test_sched_admin_dispatch.c
+++ b/src/controller/testsuite/test_sched_admin_dispatch.c
@ -0,0 +1,310 @@
+/*
+ * test_sched_admin_dispatch.c — Unity tests for the OPCode_schedAdmin
+ * controller dispatch case (scheduler-core spec, task 10.1, Req 5.10).
+ *
+ * Exercises myreq_handler's OPCode_schedAdmin case through the real
+ * decode+dispatch entry point exposed for tests, myreq_handler_for_test
+ * (cluster.c, #ifdef XCE_TESTING) — same precedent as test_config_repl.c's
+ * config-push-reply term-adoption tests (myreq_handler_for_test's own doc
+ * comment cites peer_ctrl_msg_handler_for_test, ha.c, as the original
+ * precedent for this style of test seam).
+ *
+ * Covers this task's two literal Observables:
+ *   - a request against a non-primary controller returns the not-primary
+ *     error immediately, with NO forwarding attempt (c->sched.ep is never
+ *     touched — proven by pointing it at an intentionally-invalid sentinel
+ *     that would crash the process on any dereference, not just asserted on
+ *     the returned status);
+ *   - a request against a primary with no scheduler connected returns the
+ *     not-connected error immediately, carrying an explicit
+ *     SchedAdminReply{error="scheduler not connected"} payload.
+ *
+ * Also includes a basic successful forward/reply round trip (register ->
+ * forward -> a second, independently-dispatched OPCode_reply carrying a
+ * SchedAdminReply wakes the blocked waiter), since the test scaffolding
+ * built for the two Observables above made it straightforward. The full
+ * dispatch test matrix (malformed requests, concurrent/serialized
+ * round-trips, etc.) belongs to task 10.2.
+ */
+#define _GNU_SOURCE
+#include <errno.h>
+#include <pthread.h>
+#include <stdatomic.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+
+#include <unity/unity.h>
+
+#include "cluster.h"
+#include "controller.capnp.h"
+#include "ha.h"
+#include "netio.h"
+#include "request.h"
+#include "typemap.h"
+#include "xce/errnox.h"
+
+/* myreq_handler exposed for tests (cluster.c). Packet ownership follows
+ * myreq_handler's own contract — see its doc comment in cluster.c: `pkt` is
+ * consumed (freed) by the call, never touch/free it afterward. */
+extern int myreq_handler_for_test(endpoint_t* ep, io_packet_t* pkt);
+
+/* ── Fixture ──────────────────────────────────────────────────────────────
+ *
+ * The OPCode_schedAdmin dispatch case (and the ha_dispatch_gate primary/
+ * standby gate ahead of it) only touch c->ha.role, c->ha.config_serve_fence /
+ * c->ha.upgrade_materialization_fence (both NULL-safe no-ops when
+ * c->ha.raft_c is left NULL — config_serve_fenced/upgrade_materialization_
+ * fenced both guard on `if (!c) return 0;`), c->ha.n_peers (0 → single-node
+ * quorum trivially satisfied, ha_has_write_quorum), c->sched.{lock,ep}, and
+ * c->myhost (NULL-safe — xce_reply's encode path null-checks its `from`
+ * argument, matching every other encode_*_ptr in controller.capnp.c). None
+ * of this needs persist_init/persist_start (unlike test_config_repl.c's
+ * t42_make_cluster, which exists for config-marker/WAL-touching tests) — a
+ * bare calloc'd cluster_t is sufficient and keeps this file free of
+ * temp-directory bookkeeping. */
+static cluster_t* sa_make_cluster(void)
+{
+    cluster_t* c = calloc(1, sizeof(cluster_t));
+    TEST_ASSERT_NOT_NULL(c);
+    mylock_init(&(c->sched.lock));
+    return c;
+}
+
+static void sa_teardown(cluster_t* c)
+{
+    if (c == NULL)
+        return;
+    mylock_fini(&(c->sched.lock));
+    free(c);
+}
+
+/* Build a wire-encoded OPCode_schedAdmin request packet carrying a minimal
+ * SchedAdminRequest — task 10.1 passes every field through untouched, so the
+ * exact action/recordFile content is immaterial to these tests. */
+static io_packet_t* sa_build_request_pkt(void)
+{
+    sched_admin_req_t areq;
+    memset(&areq, 0, sizeof(areq));
+    areq.action = SchedAdminAction_recordStatus;
+
+    xce_request_t req;
+    memset(&req, 0, sizeof(req));
+    req.opcode             = OPCode_schedAdmin;
+    req.body_which         = Request_body_schedadminmsg;
+    req.body.schedadminmsg = &areq;
+
+    io_packet_t* pkt = calloc(1, sizeof(*pkt));
+    TEST_ASSERT_NOT_NULL(pkt);
+    TEST_ASSERT_TRUE_MESSAGE(xce_encode(&req, pkt) >= 0,
+                             "failed to encode SchedAdminRequest packet");
+    return pkt;
+}
+
+/* Pop the single reply packet myreq_handler queued on ep->outq and decode it
+ * into *out (caller must xce_free_request(out) when done with it). */
+static void sa_decode_queued_reply(endpoint_t* ep, xce_request_t* out)
+{
+    data_item_t* it = endpoint_deq(ep);
+    TEST_ASSERT_NOT_NULL_MESSAGE(it, "expected a reply packet on ep->outq");
+
+    io_packet_t* pkt = ITPTR_TO_PACKET(it);
+    memset(out, 0, sizeof(*out));
+    int rc = xce_decode(pkt, out);
+    TEST_ASSERT_EQUAL_INT(0, rc);
+}
+
+/* ── Observable 1: non-primary rejects immediately, no forwarding attempt ──
+ *
+ * ha_dispatch_gate (ha.c) denies OPCode_schedAdmin for every non-primary
+ * role — it is absent from the CTRL_STANDBY/CTRL_INIT/CTRL_ELECTING
+ * allow-lists, exactly like OPCode_upgradeConfig — so the request is
+ * rejected BEFORE myreq_handler's switch is ever reached; the
+ * OPCode_schedAdmin case's own ha_is_primary() check is pure defensive
+ * belt-and-suspenders for a role change racing between the gate and the
+ * switch. c->sched.ep is set to a deliberately-invalid, non-NULL sentinel:
+ * if a regression ever let this request reach the forwarding code (both
+ * mylock_lock and the `sched.ep == NULL` check would pass on a garbage
+ * non-NULL pointer), xce_schedadmin_req would dereference it and crash —
+ * making this a self-verifying proof that no forwarding attempt was made,
+ * not just an assertion on the returned status. */
+static void test_schedadmin_rejects_when_not_primary(void)
+{
+    cluster_t* c = sa_make_cluster();
+    atomic_store(&c->ha.role, CTRL_STANDBY);
+
+    endpoint_t* poison = (endpoint_t*)(uintptr_t)0x1;
+    c->sched.ep        = poison;
+
+    endpoint_t ep;
+    memset(&ep, 0, sizeof(ep));
+    ep.owner = c;
+
+    io_packet_t* pkt = sa_build_request_pkt();
+    int          rc  = myreq_handler_for_test(&ep, pkt);
+
+    TEST_ASSERT_EQUAL_INT(-XCE_ERR_STANDBY, rc);
+    TEST_ASSERT_TRUE_MESSAGE(
+        c->sched.ep == poison,
+        "c->sched.ep must be untouched when the request never reaches the "
+        "dispatch case");
+
+    xce_request_t decoded;
+    sa_decode_queued_reply(&ep, &decoded);
+    TEST_ASSERT_EQUAL_INT(Request_body_replymsg, decoded.body_which);
+    TEST_ASSERT_NOT_NULL(decoded.body.replymsg);
+    TEST_ASSERT_EQUAL_INT(-XCE_ERR_STANDBY, decoded.body.replymsg->status);
+    TEST_ASSERT_EQUAL_INT(RequestReply_data_nodata,
+                          decoded.body.replymsg->data_which);
+    xce_free_request(&decoded);
+
+    sa_teardown(c);
+    printf("test_schedadmin_rejects_when_not_primary: PASS\n");
+}
+
+/* ── Observable 2: primary with no scheduler connected rejects immediately ──
+ */
+static void test_schedadmin_rejects_when_not_connected(void)
+{
+    cluster_t* c = sa_make_cluster();
+    atomic_store(&c->ha.role, CTRL_PRIMARY);
+    c->sched.ep = NULL;
+
+    endpoint_t ep;
+    memset(&ep, 0, sizeof(ep));
+    ep.owner = c;
+
+    io_packet_t* pkt = sa_build_request_pkt();
+    int          rc  = myreq_handler_for_test(&ep, pkt);
+
+    TEST_ASSERT_EQUAL_INT(0, rc);
+
+    xce_request_t decoded;
+    sa_decode_queued_reply(&ep, &decoded);
+    TEST_ASSERT_EQUAL_INT(Request_body_replymsg, decoded.body_which);
+    xce_reply_t* rr = decoded.body.replymsg;
+    TEST_ASSERT_NOT_NULL(rr);
+    TEST_ASSERT_EQUAL_INT(0, rr->status);
+    TEST_ASSERT_EQUAL_INT(RequestReply_data_schedadminreplymsg, rr->data_which);
+    TEST_ASSERT_NOT_NULL(rr->data.schedadminreplymsg);
+    TEST_ASSERT_EQUAL_STRING("scheduler not connected",
+                             rr->data.schedadminreplymsg->error);
+    xce_free_request(&decoded);
+
+    sa_teardown(c);
+    printf("test_schedadmin_rejects_when_not_connected: PASS\n");
+}
+
+/* ── Basic round trip: forward -> scheduler reply wakes the waiter ──────────
+ *
+ * c->sched.ep points at a real (if unconnected) endpoint_t so the dispatch
+ * case's forward+wait path is exercised for real: xce_schedadmin_req just
+ * encodes+enqueues (like xce_reply — see request.c), so a zeroed,
+ * non-socket-backed endpoint_t is sufficient — the same technique the two
+ * tests above already rely on for the caller-side ep. A second thread plays
+ * the scheduler: after a short delay it independently dispatches an
+ * OPCode_reply/SchedAdminReply through myreq_handler_for_test on the SAME
+ * cluster_t, exactly how the real reply arrives in production (a second
+ * vtask handling c->sched.ep's inbound traffic) — this drives the real
+ * schedadmin_signal()/demux path in the OPCode_reply case, not a shortcut. */
+typedef struct {
+    cluster_t* c;
+} sa_reply_thread_arg_t;
+
+static void* sa_deliver_scheduler_reply(void* argp)
+{
+    sa_reply_thread_arg_t* arg = (sa_reply_thread_arg_t*)argp;
+
+    /* Give the main thread time to register the pending waiter and start
+     * blocking on the condvar — comfortably inside the dispatch case's 5s
+     * deadline. */
+    usleep(200000);
+
+    sched_admin_reply_t sreply;
+    memset(&sreply, 0, sizeof(sreply));
+    sreply.ok         = 1;
+    sreply.recording  = 1;
+    sreply.file       = "/tmp/xce_sa_roundtrip.replay";
+    sreply.eventCount = 42;
+    sreply.startedAt  = 1234;
+
+    xce_reply_t rr;
+    memset(&rr, 0, sizeof(rr));
+    rr.opcode                  = OPCode_schedAdmin;
+    rr.status                  = 0;
+    rr.data_which              = RequestReply_data_schedadminreplymsg;
+    rr.data.schedadminreplymsg = &sreply;
+
+    xce_request_t req;
+    memset(&req, 0, sizeof(req));
+    req.opcode        = OPCode_reply;
+    req.body_which    = Request_body_replymsg;
+    req.body.replymsg = &rr;
+
+    io_packet_t* pkt = calloc(1, sizeof(*pkt));
+    TEST_ASSERT_NOT_NULL(pkt);
+    TEST_ASSERT_TRUE(xce_encode(&req, pkt) >= 0);
+
+    endpoint_t sched_reply_ep;
+    memset(&sched_reply_ep, 0, sizeof(sched_reply_ep));
+    sched_reply_ep.owner = arg->c;
+
+    myreq_handler_for_test(&sched_reply_ep, pkt);
+    return NULL;
+}
+
+static void test_schedadmin_round_trip_success(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;
+
+    sa_reply_thread_arg_t arg = { .c = c };
+    pthread_t             tid;
+    TEST_ASSERT_EQUAL_INT(
+        0, pthread_create(&tid, NULL, sa_deliver_scheduler_reply, &arg));
+
+    endpoint_t caller_ep;
+    memset(&caller_ep, 0, sizeof(caller_ep));
+    caller_ep.owner = c;
+
+    io_packet_t* pkt = sa_build_request_pkt();
+    int          rc  = myreq_handler_for_test(&caller_ep, pkt);
+
+    pthread_join(tid, NULL);
+
+    TEST_ASSERT_EQUAL_INT_MESSAGE(
+        0, rc,
+        "the round trip must complete (woken by the reply), not time out");
+
+    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(0, rr->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(1, got->ok);
+    TEST_ASSERT_EQUAL_INT(1, got->recording);
+    TEST_ASSERT_EQUAL_UINT64(42, got->eventCount);
+    TEST_ASSERT_EQUAL_UINT64(1234, got->startedAt);
+    TEST_ASSERT_EQUAL_STRING("/tmp/xce_sa_roundtrip.replay", got->file);
+    xce_free_request(&decoded);
+
+    sa_teardown(c);
+    printf("test_schedadmin_round_trip_success: PASS\n");
+}
+
+/* ── Suite entry point ──────────────────────────────────────────────────── */
+void test_sched_admin_dispatch(void)
+{
+    UnitySetTestFile(__FILE__);
+    RUN_TEST(test_schedadmin_rejects_when_not_primary);
+    RUN_TEST(test_schedadmin_rejects_when_not_connected);
+    RUN_TEST(test_schedadmin_round_trip_success);
+}