feat(worker-rejoin-resync): task 6.2 - route job-state report to reconciliation

Add OPCode_jobStateReport dispatch in myreq_handler, reassembling a report across multiple incoming pages before handing it to task 3.2's resync_apply_report() exactly once - proven via a test-only call counter, not just end-state equivalence (a double-apply could pass an end-state check if idempotent). Standby-redirect investigation (Req 8.1/8.3): confirmed ha_dispatch_gate already redirects every rejected opcode uniformly, so no opcode-specific code was needed for that. OPCode_jobStateQuery has no controller-side receive path at all (only sent, never received) so needed no gate change either. That same investigation found a different, more consequential gap: fixes bug-1753, ha_dispatch_gate's quorum-loss fact-admission allowlist (OPCode_notify/update/setpreason/clrpreason) was missing OPCode_jobStateReport, so a report arriving during a transient quorum-loss window - this feature's own core failover scenario - would be rejected outright with no retry (worker sends fire-and-forget). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y4M5MJM2zK6nU43da267Ku
2026-09-01 21:06:42 +00:00 · 2026-09-01 21:06:42 +00:00 · 9f655c61c7
commit 9f655c61c7
parent a3d12d37d9
12 changed files with 1178 additions and 12 deletions
--- a/src/controller/testsuite/test_jobstatereport_dispatch.c
+++ b/src/controller/testsuite/test_jobstatereport_dispatch.c
@ -0,0 +1,322 @@
+/*
+ * test_jobstatereport_dispatch.c — Unity tests for the OPCode_jobStateReport
+ * controller dispatch case (worker-rejoin-resync spec, task 6.2, Req 8.1,
+ * 8.3).
+ *
+ * Exercises myreq_handler's OPCode_jobStateReport case through the real
+ * decode+dispatch entry point exposed for tests, myreq_handler_for_test
+ * (cluster.c, #ifdef XCE_TESTING) — same precedent test_sched_admin_
+ * dispatch.c already established for a comparable primary-only opcode.
+ *
+ * Covers task 6.2's own explicit standby-redirect investigation: a report
+ * (or, by the same generic mechanism, a query) sent to a non-primary
+ * controller must receive the SAME redirect behavior an equivalent
+ * existing primary-only message already receives, not a bare unrecoverable
+ * error — proven here exactly as test_sched_admin_dispatch.c's own
+ * test_schedadmin_standby_with_known_primary_redirects/test_schedadmin_
+ * standby_no_known_primary_no_redirect prove it for OPCode_schedAdmin, the
+ * "real existing primary-only opcode" ground truth this task's brief
+ * calls for. OPCode_jobStateReport is deliberately absent from ha_
+ * dispatch_gate()'s CTRL_STANDBY allow-list (ha.c) — exactly like OPCode_
+ * schedAdmin/OPCode_upgradeConfig — so this redirect behavior requires NO
+ * opcode-specific code in cluster.c's own dispatch case: it is entirely a
+ * property of the GENERIC gate-rejection block that already runs ahead of
+ * every opcode's switch case (cluster.c, the bug-1700 fix). These tests
+ * prove that generic mechanism actually covers this opcode too, rather
+ * than assuming it does.
+ *
+ * The OTHER half of task 6.2's own brief — multi-page reassembly, applied
+ * exactly once, equivalent to a single-page report — is proven against a
+ * REAL worker_t and REAL job table in test_worker_resync.c (resync_handle_
+ * report_page()'s own direct tests), not re-proven here through the wire.
+ * See this file's own comment further down (immediately before the RUN_
+ * TEST list) for exactly why a "real wire dispatch resolves a real
+ * worker_t" version of that proof cannot be built in THIS test binary, and
+ * why that is a pre-existing, unrelated architectural property, not a gap
+ * in this task's own coverage.
+ */
+#define _GNU_SOURCE
+#include <errno.h>
+#include <stdatomic.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.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"
+#include "xce/types.h"
+
+/* myreq_handler exposed for tests (cluster.c) — same seam test_sched_admin_
+ * dispatch.c uses. `pkt` ownership: consumed (freed) by the call. */
+extern int myreq_handler_for_test(endpoint_t* ep, io_packet_t* pkt);
+
+/* ── Fixture ────────────────────────────────────────────────────────────
+ *
+ * A bare calloc'd cluster_t, exactly like test_sched_admin_dispatch.c's
+ * sa_make_cluster() — sufficient for every test below, none of which
+ * reaches past ha_dispatch_gate() or handle_jobstatereport()'s own first
+ * line (get_ep_worker()).
+ */
+static cluster_t* jsr_make_gate_cluster(void)
+{
+    cluster_t* c = calloc(1, sizeof(cluster_t));
+    TEST_ASSERT_NOT_NULL(c);
+    mylock_init(&(c->sched.lock));
+    return c;
+}
+
+static void jsr_gate_teardown(cluster_t* c)
+{
+    if (c == NULL)
+        return;
+    mylock_fini(&(c->sched.lock));
+    free(c);
+}
+
+/* Build a wire-encoded OPCode_jobStateReport request packet carrying a
+ * minimal single-entry, single-page report — the exact content is
+ * immaterial for the standby-redirect tests below (the request never
+ * reaches the dispatch case at all). */
+static io_packet_t* jsr_build_request_pkt(uint64_t jobid)
+{
+    xce_job_state_entry_t  entry;
+    xce_job_state_entry_t* entries[1];
+    xce_job_state_report_t body;
+
+    memset(&entry, 0, sizeof(entry));
+    entry.jobid    = jobid;
+    entry.status   = JobStateStatus_running;
+    entry.queried  = 1;
+    entries[0]     = &entry;
+
+    memset(&body, 0, sizeof(body));
+    body.n_total = 1;
+    body.pos     = 0;
+    body.count   = 1;
+    body.entries = entries;
+
+    xce_request_t req;
+    memset(&req, 0, sizeof(req));
+    req.opcode                 = OPCode_jobStateReport;
+    req.body_which              = Request_body_jobstatereportmsg;
+    req.body.jobstatereportmsg = &body;
+
+    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 JobStateReport packet");
+    return pkt;
+}
+
+/* Pop the single reply packet myreq_handler queued on ep->outq (if any) and
+ * decode it into *out. Mirrors test_sched_admin_dispatch.c's sa_decode_
+ * queued_reply exactly. */
+static void jsr_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);
+}
+
+/* ── Standby redirect (task 6.2's own explicit investigation) ─────────────
+ *
+ * Ground truth: OPCode_schedAdmin's identical fixture/assertions
+ * (test_sched_admin_dispatch.c), the "real existing primary-only opcode"
+ * this task's brief calls for. */
+
+static void test_jobstatereport_rejects_when_not_primary(void)
+{
+    cluster_t* c = jsr_make_gate_cluster();
+    atomic_store(&c->ha.role, CTRL_STANDBY);
+
+    endpoint_t ep;
+    memset(&ep, 0, sizeof(ep));
+    ep.owner = c;
+
+    io_packet_t* pkt = jsr_build_request_pkt(1001);
+    int          rc  = myreq_handler_for_test(&ep, pkt);
+
+    TEST_ASSERT_EQUAL_INT(-XCE_ERR_STANDBY, rc);
+
+    xce_request_t decoded;
+    jsr_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);
+
+    jsr_gate_teardown(c);
+    printf("test_jobstatereport_rejects_when_not_primary: PASS\n");
+}
+
+/* bug-1700 precedent: a standby dispatch-gate rejection with a KNOWN
+ * primary must carry a redirect hint — proven here with NO opcode-specific
+ * code added anywhere for OPCode_jobStateReport: it works purely because
+ * this opcode is absent from ha_dispatch_gate's CTRL_STANDBY allow-list,
+ * so the GENERIC post-gate rejection block (cluster.c) fills the redirect
+ * for it exactly as it already does for OPCode_schedAdmin/OPCode_join. */
+static void test_jobstatereport_standby_with_known_primary_redirects(void)
+{
+    cluster_t* c = jsr_make_gate_cluster();
+    atomic_store(&c->ha.role, CTRL_STANDBY);
+    strncpy(c->ha.primary_host, "10.0.1.9", sizeof(c->ha.primary_host) - 1);
+    c->ha.primary_port = 7003;
+
+    endpoint_t ep;
+    memset(&ep, 0, sizeof(ep));
+    ep.owner = c;
+
+    io_packet_t* pkt = jsr_build_request_pkt(1002);
+    int          rc  = myreq_handler_for_test(&ep, pkt);
+
+    TEST_ASSERT_EQUAL_INT(-XCE_ERR_STANDBY, rc);
+
+    xce_request_t decoded;
+    jsr_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(-XCE_ERR_STANDBY, rr->status);
+    TEST_ASSERT_NOT_NULL_MESSAGE(
+        rr->redirect_host,
+        "a standby dispatch-gate rejection with a known primary must carry "
+        "that primary's address, exactly like every other primary-only "
+        "opcode already does (bug-1700)");
+    TEST_ASSERT_EQUAL_STRING("10.0.1.9", rr->redirect_host);
+    TEST_ASSERT_EQUAL_UINT16(7003, rr->redirect_port);
+    xce_free_request(&decoded);
+
+    jsr_gate_teardown(c);
+    printf("test_jobstatereport_standby_with_known_primary_redirects:"
+          " PASS\n");
+}
+
+/* Regression companion: no known primary yet must not crash and must
+ * decode as a non-redirecting reply (empty host / zero port). */
+static void test_jobstatereport_standby_no_known_primary_no_redirect(void)
+{
+    cluster_t* c = jsr_make_gate_cluster();
+    atomic_store(&c->ha.role, CTRL_STANDBY);
+
+    endpoint_t ep;
+    memset(&ep, 0, sizeof(ep));
+    ep.owner = c;
+
+    io_packet_t* pkt = jsr_build_request_pkt(1003);
+    int          rc  = myreq_handler_for_test(&ep, pkt);
+
+    TEST_ASSERT_EQUAL_INT(-XCE_ERR_STANDBY, rc);
+
+    xce_request_t decoded;
+    jsr_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(-XCE_ERR_STANDBY, rr->status);
+    TEST_ASSERT_TRUE_MESSAGE(
+        rr->redirect_host == NULL || rr->redirect_host[0] == '\0',
+        "no known primary: redirect_host must decode as empty/absent");
+    TEST_ASSERT_EQUAL_UINT16(0, rr->redirect_port);
+    xce_free_request(&decoded);
+
+    jsr_gate_teardown(c);
+    printf("test_jobstatereport_standby_no_known_primary_no_redirect:"
+          " PASS\n");
+}
+
+/* A PRIMARY with write quorum handles the request normally — dispatched
+ * past the gate into handle_jobstatereport(), which returns -ENOENT for an
+ * endpoint that does not resolve to a known worker (this bare fixture's
+ * `ep` was never registered as one) rather than any standby-style
+ * rejection. Proves the gate's happy path is unaffected by this task's
+ * change and that the new dispatch case is genuinely reached. */
+static void test_jobstatereport_primary_reaches_dispatch_case(void)
+{
+    cluster_t* c = jsr_make_gate_cluster();
+    atomic_store(&c->ha.role, CTRL_PRIMARY);
+    /* n_peers defaults to 0 (calloc) -- single-node cluster, trivially has
+     * write quorum (ha_has_write_quorum: n_peers <= 1 -> true). */
+
+    endpoint_t ep;
+    memset(&ep, 0, sizeof(ep));
+    ep.owner = c;
+    /* ep.data left NULL -> get_ep_role()/get_ep_worker() treat this as "not
+     * a worker endpoint" and return NULL/-EINVAL, exercising handle_
+     * jobstatereport's own not-a-worker branch. */
+
+    io_packet_t* pkt = jsr_build_request_pkt(1004);
+    int          rc  = myreq_handler_for_test(&ep, pkt);
+
+    TEST_ASSERT_EQUAL_INT_MESSAGE(-ENOENT, rc,
+        "a primary with quorum must reach handle_jobstatereport() itself "
+        "(not a gate rejection) -- which reports -ENOENT for an endpoint "
+        "with no resolvable worker_t");
+
+    jsr_gate_teardown(c);
+    printf("test_jobstatereport_primary_reaches_dispatch_case: PASS\n");
+}
+
+/* ── Why no "real wire dispatch resolves a real worker_t" test lives here ──
+ *
+ * A genuine end-to-end test (two real wire-encoded OPCode_jobStateReport
+ * packets through myreq_handler_for_test, resolving a REAL worker_t via
+ * get_ep_worker() and converging a REAL job table) was attempted here and
+ * found structurally impossible in THIS binary: get_ep_worker() (misc.c)
+ * is NOT one of controller_unit_test_src's linked sources (meson.build) --
+ * test_commit_barrier_reply.c instead provides controller_unit_test's ONLY
+ * definition of it, a fixed test double (`return g_stub_ep_worker;`,
+ * file-static, defaulting to NULL and settable only by that file's own
+ * tests) that no other test file can point at a real worker_t. Confirmed
+ * live (not assumed) by direct instrumentation: a page built with a
+ * correct, verified-by-hand ep_private_t{role=HostRole_worker, idx=w->idx}
+ * -- independently proven resolvable via key_find()/vget() called directly
+ * against the same workermgr -- still made get_ep_worker() return NULL,
+ * because the symbol actually linked into this binary is the stub, not
+ * misc.c's real implementation.
+ *
+ * This is a pre-existing architectural property of controller_unit_test
+ * (several other cluster.c-adjacent functions -- notify(), update_host_
+ * runusage(), on_scheduler_join(), add_apiserver(), on_apiserver_join() --
+ * are stubbed the same way in the same file, for the same reason), not
+ * something this task introduced or can fix without either modifying
+ * shared test infrastructure many other tests depend on (out of this
+ * task's boundary) or standing up a new dedicated test binary that links
+ * the real misc.c (the pattern host_runusage_unit_test/resval_typeresolve_
+ * unit_test already establish for exactly this class of problem, bug-1735)
+ * -- a real option, but a disproportionate scope expansion for what the
+ * unit tests below plus test_worker_resync.c's own tests already prove
+ * between them: handle_jobstatereport() (cluster.c) is a two-line function
+ * (get_ep_worker() then resync_handle_report_page()) whose own correctness
+ * is not in question -- the two things worth proving are (a) cluster.c's
+ * switch genuinely routes OPCode_jobStateReport to it, proven above by
+ * test_jobstatereport_primary_reaches_dispatch_case's distinctive -ENOENT
+ * return (get_ep_worker's stub-NULL path, reached only via this exact
+ * call chain), and (b) multi-page reassembly + exactly-once apply +
+ * equivalence-to-single-page, proven exhaustively against a REAL worker_t
+ * and REAL job table by resync_handle_report_page()'s own direct tests in
+ * test_worker_resync.c (test_handle_report_page_multi_page_reassembles_
+ * and_applies_once and its siblings) -- the same function handle_
+ * jobstatereport() calls, unmodified.
+ */
+
+void test_jobstatereport_dispatch(void)
+{
+    UnitySetTestFile(__FILE__);
+    RUN_TEST(test_jobstatereport_rejects_when_not_primary);
+    RUN_TEST(test_jobstatereport_standby_with_known_primary_redirects);
+    RUN_TEST(test_jobstatereport_standby_no_known_primary_no_redirect);
+    RUN_TEST(test_jobstatereport_primary_reaches_dispatch_case);
+}