diff --git a/.kiro/specs/raft-adapter-conformance/tasks.md b/.kiro/specs/raft-adapter-conformance/tasks.md index 5b24605..054978c 100644 --- a/.kiro/specs/raft-adapter-conformance/tasks.md +++ b/.kiro/specs/raft-adapter-conformance/tasks.md @@ -7,7 +7,7 @@ - _Requirements: 7.4_ - [ ] 2. Core: configuration-push reply integrity -- [ ] 2.1 (P) Send an explicit accept/reject reply carrying term on every configuration push outcome +- [x] 2.1 (P) Send an explicit accept/reject reply carrying term on every configuration push outcome - In `handle_repl_config_push`, construct and send `ReplConfigPushReply{accepted=0, term=current_term}` on each of the four reject early-returns (malformed request, missing bytes/sha256, sha256 mismatch, `conf_save_config` failure) instead of returning without a reply - Populate `term` on the existing success reply path too - Preserve the existing `-EAGAIN`-from-`config_activate_committed` behavior unchanged: that path still sends `accepted=1` (activation-deferred is not a rejection) diff --git a/src/controller/config_repl.c b/src/controller/config_repl.c index 89c209b..626edba 100644 --- a/src/controller/config_repl.c +++ b/src/controller/config_repl.c @@ -1247,6 +1247,50 @@ int handle_config_request(cluster_t *c, endpoint_t *ep, xce_request_t *req) return rc2; } +#ifdef XCE_TESTING +void (*config_push_reply_send_hook)(uint64_t term, uint64_t version_tag, + int accepted) = NULL; +#endif + +/* send_config_push_reply: construct and send ReplConfigPushReply{accepted, + * version_tag, term} back to the primary over ep. Mirrors + * ha_send_wal_ack_reply_ex (ha.c, bug-1479) exactly: when + * config_push_reply_send_hook is set (XCE_TESTING), the hook is called with + * the built {term, version_tag, accepted} INSTEAD of sending over ep -- lets + * a unit test observe the reply payload without a live endpoint. + * version_tag is always the standby's OWN current config_version_tag (per + * repl_config_push_reply_s's documented semantics, "standby's current + * version tag after decision") -- not the pushed request's tag, so a reject + * correctly reports the unchanged local state. raft-adapter-conformance + * task 2.1 (Req 2.2, 2.3, 2.5, 7.4): every handle_repl_config_push outcome + * -- accept or reject -- sends exactly one explicit reply, never silence. */ +static void send_config_push_reply(cluster_t *c, endpoint_t *ep, + xce_request_t *req, int accepted) +{ + uint64_t term = (uint64_t)atomic_load_explicit(&c->ha.term, + memory_order_acquire); + uint64_t version_tag = c->myconf->config_version_tag; + +#ifdef XCE_TESTING + if (config_push_reply_send_hook) { + config_push_reply_send_hook(term, version_tag, accepted); + return; + } +#endif + + repl_config_push_reply_t reply_body = { + .accepted = accepted, + .version_tag = version_tag, + .term = term, + }; + xce_reply_t reply; + init_reply_by_req(&reply, req); + reply.status = 0; + reply.data_which = RequestReply_data_replconfigpushreplymsg; + reply.data.replconfigpushreplymsg = &reply_body; + xce_reply(ep, c->myhost, &reply); +} + /* ── handle_repl_config_push ──────────────────────────────────────────────── * * Standby handler for OPCode_replConfigPush sent by the primary. @@ -1256,13 +1300,17 @@ int handle_config_request(cluster_t *c, endpoint_t *ep, xce_request_t *req) * 2. Verify SHA-256 integrity * 3. Save to disk via conf_save_config * 4. Reload cluster_config in memory via load_primary_config - * 5. Send ReplConfigPushReply with accepted=1 + * 5. Send ReplConfigPushReply -- accepted=1 on success (including + * activation-deferred EAGAIN), accepted=0 on any reject (raft-adapter- + * conformance Req 2.2, 2.3, 2.5); every path sends exactly one reply, + * never silence. */ int handle_repl_config_push(cluster_t *c, endpoint_t *ep, xce_request_t *req) { if (req->body_which != Request_body_replconfigpushrequestmsg || !req->body.replconfigpushrequestmsg) { WARN("handle_repl_config_push: malformed request"); + send_config_push_reply(c, ep, req, /*accepted*/0); return -EINVAL; } @@ -1282,6 +1330,7 @@ int handle_repl_config_push(cluster_t *c, endpoint_t *ep, xce_request_t *req) if (!rcpr->raw_bytes || rcpr->raw_bytes_len == 0 || !rcpr->raw_sha256) { WARN("handle_repl_config_push: missing bytes or sha256 (tag=%016" PRIx64 ")", rcpr->version_tag); + send_config_push_reply(c, ep, req, /*accepted*/0); return -EINVAL; } @@ -1290,6 +1339,7 @@ int handle_repl_config_push(cluster_t *c, endpoint_t *ep, xce_request_t *req) SHA256(rcpr->raw_bytes, rcpr->raw_bytes_len, computed); if (memcmp(computed, rcpr->raw_sha256, 32) != 0) { WARN("handle_repl_config_push: SHA-256 mismatch — rejecting push"); + send_config_push_reply(c, ep, req, /*accepted*/0); return -EBADMSG; } @@ -1305,6 +1355,7 @@ int handle_repl_config_push(cluster_t *c, endpoint_t *ep, xce_request_t *req) rcpr->raw_sha256); if (rc != 0) { WARN("handle_repl_config_push: conf_save_config failed: %d", rc); + send_config_push_reply(c, ep, req, /*accepted*/0); return rc; } @@ -1322,17 +1373,10 @@ int handle_repl_config_push(cluster_t *c, endpoint_t *ep, xce_request_t *req) if (act_rc != 0 && act_rc != -EAGAIN) WARN("handle_repl_config_push: config_activate_committed rc=%d", act_rc); - /* ACK the primary */ - repl_config_push_reply_t reply_body = { - .accepted = 1, - .version_tag = c->myconf->config_version_tag, - }; - xce_reply_t reply; - init_reply_by_req(&reply, req); - reply.status = 0; - reply.data_which = RequestReply_data_replconfigpushreplymsg; - reply.data.replconfigpushreplymsg = &reply_body; - xce_reply(ep, c->myhost, &reply); + /* ACK the primary. -EAGAIN (activation deferred) is NOT a rejection -- + * the bytes were saved out-of-band successfully, so this still replies + * accepted=1 (raft-adapter-conformance Req 2.5, 7.5: unchanged). */ + send_config_push_reply(c, ep, req, /*accepted*/1); return 0; } diff --git a/src/controller/config_repl.h b/src/controller/config_repl.h index 398a319..bef4faf 100644 --- a/src/controller/config_repl.h +++ b/src/controller/config_repl.h @@ -463,6 +463,19 @@ int load_primary_config_for_test(cluster_t *c); */ extern void (*load_primary_config_race_hook)(void); +/* + * config_push_reply_send_hook: test-only injection point for the outbound + * ReplConfigPushReply built by send_config_push_reply() (config_repl.c). + * When non-NULL, called with (term, version_tag, accepted) INSTEAD of + * building/sending the real reply over ep -- lets a unit test observe a + * config-push accept OR reject reply without a live endpoint + * (raft-adapter-conformance task 2.1, mirrors ha_wal_ack_send_hook / + * bug-1479). NULL in production. Set by test infrastructure + * (test_config_repl.c) before the relevant tests run. + */ +extern void (*config_push_reply_send_hook)(uint64_t term, uint64_t version_tag, + int accepted); + /* * push_ctx_t: exposed under XCE_TESTING so unit tests can construct * synthetic push contexts to drive wait_all_with_timeout_for_test(). diff --git a/src/controller/testsuite/test_config_repl.c b/src/controller/testsuite/test_config_repl.c index 57e58df..f9248e6 100644 --- a/src/controller/testsuite/test_config_repl.c +++ b/src/controller/testsuite/test_config_repl.c @@ -3483,6 +3483,82 @@ static void test_configpush_transport_holds_endpoint_ref_across_send(void) printf("test_configpush_transport_holds_endpoint_ref_across_send: PASS\n"); } +/* raft-adapter-conformance task 2.1 (Req 2.2, 2.3, 2.5, 7.4): a config-push + * reject must now send an explicit ReplConfigPushReply{accepted=0, term} + * back to the primary instead of returning silently. This is a minimal, + * representative proof that the new config_push_reply_send_hook actually + * fires -- exercised here via ONE reject path (SHA-256 mismatch). Task 2.3 + * expands this into full coverage of all four reject paths plus the EAGAIN + * accept-path and the cluster.c term-adoption wiring; this test is kept + * deliberately minimal so it doesn't collide with that broader coverage. */ +static uint64_t g_cpr21_hook_term; +static uint64_t g_cpr21_hook_version_tag; +static int g_cpr21_hook_accepted; +static int g_cpr21_hook_called; + +static void cpr21_reply_capture(uint64_t term, uint64_t version_tag, int accepted) +{ + g_cpr21_hook_called = 1; + g_cpr21_hook_term = term; + g_cpr21_hook_version_tag = version_tag; + g_cpr21_hook_accepted = accepted; +} + +static void test_config_push_sha256_mismatch_sends_explicit_reject_reply(void) +{ + char tmp[256]; + strcpy(tmp, "/tmp/xce_cr21_shamis_XXXXXX"); + TEST_ASSERT_NOT_NULL(mkdtemp(tmp)); + + cluster_t *c = t42_make_cluster(tmp); + TEST_ASSERT_NOT_NULL(c); + t43_attach_myconf(c, tmp); + + atomic_store(&c->ha.role, CTRL_STANDBY); + atomic_store(&c->ha.term, 7); + + /* Deliberately mismatched payload: raw_sha256 does NOT hash to + * raw_bytes -- exercises the real SHA-256-mismatch reject branch. */ + uint8_t payload[16]; + memset(payload, 0xAB, sizeof(payload)); + uint8_t wrong_sha[32]; + memset(wrong_sha, 0x00, sizeof(wrong_sha)); /* SHA256(payload) != all-zero */ + + repl_config_push_req_t rcpr; + memset(&rcpr, 0, sizeof(rcpr)); + rcpr.version_tag = 0x1234; + rcpr.raw_bytes = payload; + rcpr.raw_bytes_len = sizeof(payload); + rcpr.raw_sha256 = wrong_sha; + + xce_request_t req; + memset(&req, 0, sizeof(req)); + req.body_which = Request_body_replconfigpushrequestmsg; + req.body.replconfigpushrequestmsg = &rcpr; + + g_cpr21_hook_called = 0; + config_push_reply_send_hook = cpr21_reply_capture; + + int rc = handle_repl_config_push(c, /*ep*/NULL, &req); + + config_push_reply_send_hook = NULL; /* uninstall before any assertion can fail */ + + TEST_ASSERT_EQUAL_INT(-EBADMSG, rc); + TEST_ASSERT_TRUE_MESSAGE(g_cpr21_hook_called, + "raft-adapter-conformance 2.1: a config-push SHA-256 mismatch must " + "send an explicit ReplConfigPushReply, not silence it entirely"); + TEST_ASSERT_EQUAL_INT_MESSAGE(0, g_cpr21_hook_accepted, + "the reply for a rejected config push must carry accepted=0"); + TEST_ASSERT_EQUAL_UINT64(7, g_cpr21_hook_term); + + t43_free_myconf(c); + t42_teardown(c); + char cmd[512]; + snprintf(cmd, sizeof(cmd), "rm -rf %s", tmp); + system(cmd); + printf("test_config_push_sha256_mismatch_sends_explicit_reject_reply: PASS\n"); +} + #endif /* XCE_TESTING */ /* ── raw capnp shims (Task A) ───────────────────── */ @@ -6848,6 +6924,8 @@ void test_config_repl(void) RUN_TEST(test_wait_all_with_timeout_fast_threads); /* bug-1381: push_config_transport_only endpoint-lifetime regression */ RUN_TEST(test_configpush_transport_holds_endpoint_ref_across_send); + /* raft-adapter-conformance task 2.1: config-push reject reply integrity */ + RUN_TEST(test_config_push_sha256_mismatch_sends_explicit_reject_reply); /* Task 2.1: OPCode_walCatchupEntry handler */ RUN_TEST(test_wal_catchup_entry_advances_applied_lsn); RUN_TEST(test_wal_catchup_entry_error_skipped_no_crash);