diff --git a/src/controller/cluster.c b/src/controller/cluster.c index 83ab39bd..bc72882c 100644 --- a/src/controller/cluster.c +++ b/src/controller/cluster.c @@ -366,6 +366,10 @@ void schedadmin_set_timeout_ms_for_test(long ms) #define SCHEDADMIN_TIMEOUT_MS 5000L #endif +/* fill_redirect_reply is defined in join.c (no header declaration) -- + * mirrors config_repl.c's identical local extern for the same helper. */ +extern void fill_redirect_reply(xce_reply_t *reply, const ha_state_t *ha); + static int myreq_handler(vtask_t* task) { vtask_data_t* data = (vtask_data_t*)task->data; @@ -408,6 +412,25 @@ static int myreq_handler(vtask_t* task) if (rc != 0) { reply.status = rc; reply.data_which = RequestReply_data_nodata; + /* bug-1700: -XCE_ERR_STANDBY (ha_dispatch_gate's CTRL_STANDBY + * default-case rejection) is the one gate rejection where this node + * may know who the primary is -- populate the redirect hint so + * xceadmin's already-generic client-side follow_redirects() can + * transparently retry there, exactly like OPCode_join's handler + * already does (join.c:277). ha_dispatch_gate's OTHER nonzero + * rejection, -XCE_ERR_NOT_READY (CTRL_INIT/CTRL_ELECTING, and a + * fenced/quorum-less CTRL_PRIMARY), has no settled primary to + * redirect to, so it is deliberately excluded. fill_redirect_reply() + * is safe even when no primary is known yet: ha->primary_host is a + * fixed buffer embedded in ha_state_t (never NULL), so an + * as-yet-unknown primary just yields an empty string / port 0, + * which both the capnp encode path (controller.capnp.c's + * encode_RequestReply, gated on redirect_host != NULL) and the + * client's discovery.py _redirect_target() already treat as + * "no redirect" -- verified via code inspection, not assumed. */ + if (rc == -XCE_ERR_STANDBY) { + fill_redirect_reply(&reply, &c->ha); + } xce_reply(ep, c->myhost, &reply); xce_free_request(&request); return rc; diff --git a/src/controller/config_repl.c b/src/controller/config_repl.c index 3ca62e5c..1eab8d6d 100644 --- a/src/controller/config_repl.c +++ b/src/controller/config_repl.c @@ -2164,6 +2164,9 @@ char *sanitise_cn(const char *src, size_t maxlen) * Steps: * 1. Validate request body * 2. Set primary_ctrl_fd = ep->fd (first ctrlhello from winner, idempotent) + * 2b. (bug-1701) Look up the sender's name against our own static + * peers_cfg candidate list and copy its host:port into + * ha->primary_host/primary_port * 3. Call ha_on_heartbeat_received (resets liveness watchdog) * 4. Arm ha_arm_heartbeat_timeout_vtask */ @@ -2183,6 +2186,57 @@ int handle_ctrlhello(cluster_t *c, endpoint_t *ep, xce_request_t *req) memory_order_acquire) == CTRL_STANDBY && c->ha.primary_ctrl_fd == -1) { c->ha.primary_ctrl_fd = ep->fd; + + /* bug-1701: ha->primary_host/primary_port (aliased into a redirect + * reply by fill_redirect_reply(), join.c:27) were never written by + * any production path, so bug-1700's redirect fix was a live no-op. + * Fix is a pure LOCAL lookup — no wire-protocol change needed: every + * node already knows every candidate's host:port from boot + * (c->myconf->peers_cfg, parsed once from --peers). This is the + * moment a standby first learns "I am now connected to my primary"; + * req->from->name carries the sender's name= identity (set for + * every incoming request at request.c:379, "req->from = ep->peer;"), + * which is the SAME name used in --peers name=host:port. Match it + * against our own candidate list (mirrors the lookup-by-name pattern + * already used for voted_for_name in ha_init_raft, ha.c:7822-7829) + * and copy that candidate's hostname/port. On no match (real config + * mismatch) or a NULL req->from/name, leave primary_host/primary_port + * at their previous value — a stale-but-previously-correct target is + * safer than wiping a working redirect on a transient miss — and log + * a WARN so a genuine mismatch is operator-visible. + * + * No explicit clear is needed on primary loss: ha_on_primary_ctrl_fd_ + * rdhup (ha.c) resets primary_ctrl_fd back to -1, so the NEXT real + * ctrlhello (from the same or a new primary) re-satisfies this same + * idempotency guard and re-runs this lookup naturally. + * + * Locking: ha.h's `lock` field doc ("Protects primary_host/port + + * voted_for_term/name") and existing writers of the other field it + * covers (voted_for_name, ha.c:2304-2332, 5559-5593) take + * c->ha.lock around the write — matched here for the same fields. */ + if (req->from && req->from->name && c->myconf) { + const xce_cluster_config_t *pcfg = &c->myconf->peers_cfg; + int matched = 0; + for (int i = 0; i < pcfg->candidates_count; i++) { + if (strcmp(pcfg->candidates[i].name, req->from->name) == 0) { + pthread_mutex_lock(&c->ha.lock); + strncpy(c->ha.primary_host, pcfg->candidates[i].hostname, + sizeof(c->ha.primary_host) - 1); + c->ha.primary_host[sizeof(c->ha.primary_host) - 1] = '\0'; + c->ha.primary_port = (uint32_t)pcfg->candidates[i].port; + pthread_mutex_unlock(&c->ha.lock); + matched = 1; + break; + } + } + if (!matched) { + WARN("handle_ctrlhello: sender '%s' does not match any " + "configured peer candidate in --peers -- " + "primary_host/primary_port left unchanged (possible " + "config mismatch)", req->from->name); + } + } + ha_on_heartbeat_received(&c->ha); ha_arm_heartbeat_timeout_vtask(&c->ha, c); INFO("HA: primary control channel established (fd=%d)", ep->fd); diff --git a/src/controller/testsuite/test_ha_connection_reuse.c b/src/controller/testsuite/test_ha_connection_reuse.c index f514d571..76cf0b59 100644 --- a/src/controller/testsuite/test_ha_connection_reuse.c +++ b/src/controller/testsuite/test_ha_connection_reuse.c @@ -427,6 +427,127 @@ static void test_handle_ctrlhello_sets_primary_ctrl_fd(void) printf("test_handle_ctrlhello_sets_primary_ctrl_fd: PASS\n"); } +/* ── Test 8b (bug-1701): handle_ctrlhello populates primary_host/port from + * the sender's name matched against our own static peers_cfg candidate list. + * This is the local-lookup fix that unblocks bug-1700's fill_redirect_reply() + * mechanism (join.c:27), which is fed by ha->primary_host/primary_port and + * was permanently empty before this fix — no wire-protocol change, just a + * boot-time-known static lookup keyed by req->from->name. ──────────────── */ + +static void test_handle_ctrlhello_populates_primary_host_from_known_peer(void) +{ + cluster_t c; + memset(&c, 0, sizeof(c)); + ha_init(&c.ha, &g_cfg, 0); + c.ha.primary_ctrl_fd = -1; + atomic_store_explicit(&c.ha.role, (int32_t)CTRL_STANDBY, memory_order_release); + + /* Static peer list (as parsed at boot from --peers name=host:port,...) + * containing the candidate that will send us this ctrlhello. */ + myconf_t conf; + memset(&conf, 0, sizeof(conf)); + conf.peers_cfg.candidates_count = 1; + strncpy(conf.peers_cfg.candidates[0].name, "primary1", + sizeof(conf.peers_cfg.candidates[0].name) - 1); + strncpy(conf.peers_cfg.candidates[0].hostname, "10.0.1.7", + sizeof(conf.peers_cfg.candidates[0].hostname) - 1); + conf.peers_cfg.candidates[0].port = 9991; + c.myconf = &conf; + + endpoint_t ep; + memset(&ep, 0, sizeof(ep)); + ep.fd = 42; + + ctrl_hello_req_t hello; + memset(&hello, 0, sizeof(hello)); + hello.name = "primary1"; + hello.applied_lsn = 0; + hello.config_tag = 0; + + /* req->from is populated in production by request.c:379 + * ("req->from = ep->peer;") for EVERY incoming request, carrying the + * sender's name= identity from --peers name=host:port. */ + xce_peerinfo_t from; + memset(&from, 0, sizeof(from)); + from.name = "primary1"; + from.key = NULL; + + xce_request_t req; + memset(&req, 0, sizeof(req)); + req.from = &from; + req.body_which = Request_body_ctrlhellomsg; + req.body.ctrlhellomsg = &hello; + + handle_ctrlhello(&c, &ep, &req); + + TEST_ASSERT_EQUAL_STRING_MESSAGE("10.0.1.7", c.ha.primary_host, + "bug-1701: handle_ctrlhello must copy the matched candidate's " + "hostname into ha->primary_host so fill_redirect_reply() has a " + "real target"); + TEST_ASSERT_EQUAL_UINT32_MESSAGE(9991, c.ha.primary_port, + "bug-1701: handle_ctrlhello must copy the matched candidate's port " + "into ha->primary_port"); + + ha_fini(&c.ha); + printf("test_handle_ctrlhello_populates_primary_host_from_known_peer: PASS\n"); +} + +/* ── Test 8c (bug-1701 edge case): unknown sender name must not crash and + * must leave primary_host/primary_port unchanged (not cleared to empty) — a + * real config mismatch is surfaced via WARN, not a crash or data wipe. ───── */ + +static void test_handle_ctrlhello_unknown_peer_leaves_primary_host_unchanged(void) +{ + cluster_t c; + memset(&c, 0, sizeof(c)); + ha_init(&c.ha, &g_cfg, 0); + c.ha.primary_ctrl_fd = -1; + atomic_store_explicit(&c.ha.role, (int32_t)CTRL_STANDBY, memory_order_release); + + myconf_t conf; + memset(&conf, 0, sizeof(conf)); + conf.peers_cfg.candidates_count = 1; + strncpy(conf.peers_cfg.candidates[0].name, "primary1", + sizeof(conf.peers_cfg.candidates[0].name) - 1); + strncpy(conf.peers_cfg.candidates[0].hostname, "10.0.1.7", + sizeof(conf.peers_cfg.candidates[0].hostname) - 1); + conf.peers_cfg.candidates[0].port = 9991; + c.myconf = &conf; + + endpoint_t ep; + memset(&ep, 0, sizeof(ep)); + ep.fd = 42; + + ctrl_hello_req_t hello; + memset(&hello, 0, sizeof(hello)); + hello.name = "stranger"; + + xce_peerinfo_t from; + memset(&from, 0, sizeof(from)); + from.name = "stranger"; /* does not match any configured candidate */ + + xce_request_t req; + memset(&req, 0, sizeof(req)); + req.from = &from; + req.body_which = Request_body_ctrlhellomsg; + req.body.ctrlhellomsg = &hello; + + handle_ctrlhello(&c, &ep, &req); /* must not crash */ + + TEST_ASSERT_EQUAL_STRING_MESSAGE("", c.ha.primary_host, + "bug-1701: an unmatched sender name must leave primary_host at its " + "prior (fresh-node empty) value, never crash"); + TEST_ASSERT_EQUAL_UINT32_MESSAGE(0, c.ha.primary_port, + "bug-1701: an unmatched sender name must leave primary_port at its " + "prior (fresh-node zero) value"); + TEST_ASSERT_EQUAL_INT_MESSAGE(42, c.ha.primary_ctrl_fd, + "bug-1701: primary_ctrl_fd bookkeeping must proceed normally even " + "when the name lookup misses"); + + ha_fini(&c.ha); + printf("test_handle_ctrlhello_unknown_peer_leaves_primary_host_unchanged: PASS\n"); +} + /* ── Test 9 (Task 7.2): follower path is passive — no outbound connect ────── * * Req 2.1, 2.4: after raft_cb_become_follower the role is CTRL_STANDBY and @@ -511,6 +632,11 @@ void test_ha_connection_reuse(void) /* Req 3.1 — handle_ctrlhello designates primary control channel */ RUN_TEST(test_handle_ctrlhello_sets_primary_ctrl_fd); + /* bug-1701 — handle_ctrlhello populates primary_host/primary_port via a + * local peers_cfg lookup keyed by req->from->name */ + RUN_TEST(test_handle_ctrlhello_populates_primary_host_from_known_peer); + RUN_TEST(test_handle_ctrlhello_unknown_peer_leaves_primary_host_unchanged); + /* Task 7.2 — Req 2.1, 2.4: follower path is fully passive */ RUN_TEST(test_follower_passive_no_outbound_connect); diff --git a/src/controller/testsuite/test_sched_admin_dispatch.c b/src/controller/testsuite/test_sched_admin_dispatch.c index 27119501..bc917e4f 100644 --- a/src/controller/testsuite/test_sched_admin_dispatch.c +++ b/src/controller/testsuite/test_sched_admin_dispatch.c @@ -235,6 +235,106 @@ static void test_schedadmin_rejects_when_not_primary(void) printf("test_schedadmin_rejects_when_not_primary: PASS\n"); } +/* ── bug-1700: a standby dispatch-gate rejection must redirect to the known + * primary ────────────────────────────────────────────────────────────────── + * + * ha_dispatch_gate() rejects OPCode_schedAdmin on a CTRL_STANDBY node with + * -XCE_ERR_STANDBY (ha.c's CTRL_STANDBY case default branch, ~line 4774) -- + * this node MAY know who the primary is (c->ha.primary_host/primary_port). + * Before the fix, myreq_handler's generic ha_dispatch_gate rejection block + * (cluster.c ~406-414) never called fill_redirect_reply() (the exact same + * one-line helper OPCode_join's handler already uses, join.c:277), so + * redirect_host/redirect_port were left at their zero-value defaults on + * EVERY dispatch-gate rejection -- xceadmin's client-side, already fully + * implemented and generic follow_redirects() had nothing to follow and + * surfaced the bare -XCE_ERR_STANDBY status as-is instead of transparently + * retrying against the primary. This is the exact scenario the user hit + * running `xceadmin scheduler record status` against a standby. */ +static void test_schedadmin_standby_with_known_primary_redirects(void) +{ + cluster_t* c = sa_make_cluster(); + atomic_store(&c->ha.role, CTRL_STANDBY); + strncpy(c->ha.primary_host, "10.0.1.7", sizeof(c->ha.primary_host) - 1); + c->ha.primary_port = 7001; + + /* Poison sentinel mirrors test_schedadmin_rejects_when_not_primary above + * -- a gate rejection must never reach the forwarding code. */ + 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 -- a gate rejection never forwards"); + + 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(-XCE_ERR_STANDBY, rr->status); + TEST_ASSERT_NOT_NULL_MESSAGE( + rr->redirect_host, + "bug-1700: a standby dispatch-gate rejection with a known primary " + "must carry that primary's address so xceadmin's follow_redirects() " + "can transparently retry there"); + TEST_ASSERT_EQUAL_STRING("10.0.1.7", rr->redirect_host); + TEST_ASSERT_EQUAL_UINT16(7001, rr->redirect_port); + xce_free_request(&decoded); + + sa_teardown(c); + printf("test_schedadmin_standby_with_known_primary_redirects: PASS\n"); +} + +/* Regression companion: a standby that does NOT yet know any live primary + * (fresh ha_state_t straight out of calloc -- primary_host empty, + * primary_port 0, e.g. very early boot / mid-election / primary just died + * with no successor elected yet) must NOT crash and must produce a reply + * the client correctly treats as non-redirecting: discovery.py's + * `_redirect_target()` requires BOTH a non-empty host AND a non-zero port + * to treat a reply as a redirect, so an empty host / zero port here is + * harmless and falls back to today's bare-rejection behavior. Proves the + * fix does not regress the genuinely-no-primary-yet case. */ +static void test_schedadmin_standby_no_known_primary_no_redirect(void) +{ + cluster_t* c = sa_make_cluster(); + atomic_store(&c->ha.role, CTRL_STANDBY); + /* c->ha.primary_host/primary_port left at their calloc'd zero value. */ + + 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); + + 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(-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, " + "never garbage -- the client's _redirect_target() treats an empty " + "host as 'no redirect, use this reply as-is'"); + TEST_ASSERT_EQUAL_UINT16(0, rr->redirect_port); + xce_free_request(&decoded); + + sa_teardown(c); + printf("test_schedadmin_standby_no_known_primary_no_redirect: PASS\n"); +} + /* ── Observable 2: primary with no scheduler connected rejects immediately ── */ static void test_schedadmin_rejects_when_not_connected(void) @@ -775,6 +875,8 @@ void test_sched_admin_dispatch(void) { UnitySetTestFile(__FILE__); RUN_TEST(test_schedadmin_rejects_when_not_primary); + RUN_TEST(test_schedadmin_standby_with_known_primary_redirects); + RUN_TEST(test_schedadmin_standby_no_known_primary_no_redirect); RUN_TEST(test_schedadmin_rejects_when_not_connected); RUN_TEST(test_schedadmin_fast_rejects_when_admission_exhausted); RUN_TEST(test_schedadmin_round_trip_success);