feat(raft-adapter-conformance): send explicit reply on every config-push outcome

handle_repl_config_push now sends a ReplConfigPushReply{accepted,term}
from every exit path instead of silently dropping on the four reject
conditions (malformed request, missing bytes/sha256, sha256 mismatch,
conf_save_config failure). Term is sourced live from c->ha.term.
-EAGAIN activation-defer still replies accepted=1, unchanged.

Adds an XCE_TESTING-only send-hook mirroring ha_wal_ack_send_hook so
tests can observe the reply payload without a live endpoint.
This commit is contained in:
Rongsong Shen 2026-07-29 09:43:35 +00:00
parent ee7ede03c2
commit b7828eed4a
4 changed files with 148 additions and 13 deletions

View file

@ -7,7 +7,7 @@
- _Requirements: 7.4_ - _Requirements: 7.4_
- [ ] 2. Core: configuration-push reply integrity - [ ] 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 - 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 - 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) - Preserve the existing `-EAGAIN`-from-`config_activate_committed` behavior unchanged: that path still sends `accepted=1` (activation-deferred is not a rejection)

View file

@ -1247,6 +1247,50 @@ int handle_config_request(cluster_t *c, endpoint_t *ep, xce_request_t *req)
return rc2; 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 ──────────────────────────────────────────────── /* ── handle_repl_config_push ────────────────────────────────────────────────
* *
* Standby handler for OPCode_replConfigPush sent by the primary. * 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 * 2. Verify SHA-256 integrity
* 3. Save to disk via conf_save_config * 3. Save to disk via conf_save_config
* 4. Reload cluster_config in memory via load_primary_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) int handle_repl_config_push(cluster_t *c, endpoint_t *ep, xce_request_t *req)
{ {
if (req->body_which != Request_body_replconfigpushrequestmsg if (req->body_which != Request_body_replconfigpushrequestmsg
|| !req->body.replconfigpushrequestmsg) { || !req->body.replconfigpushrequestmsg) {
WARN("handle_repl_config_push: malformed request"); WARN("handle_repl_config_push: malformed request");
send_config_push_reply(c, ep, req, /*accepted*/0);
return -EINVAL; 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) { if (!rcpr->raw_bytes || rcpr->raw_bytes_len == 0 || !rcpr->raw_sha256) {
WARN("handle_repl_config_push: missing bytes or sha256 (tag=%016" PRIx64 ")", WARN("handle_repl_config_push: missing bytes or sha256 (tag=%016" PRIx64 ")",
rcpr->version_tag); rcpr->version_tag);
send_config_push_reply(c, ep, req, /*accepted*/0);
return -EINVAL; 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); SHA256(rcpr->raw_bytes, rcpr->raw_bytes_len, computed);
if (memcmp(computed, rcpr->raw_sha256, 32) != 0) { if (memcmp(computed, rcpr->raw_sha256, 32) != 0) {
WARN("handle_repl_config_push: SHA-256 mismatch — rejecting push"); WARN("handle_repl_config_push: SHA-256 mismatch — rejecting push");
send_config_push_reply(c, ep, req, /*accepted*/0);
return -EBADMSG; return -EBADMSG;
} }
@ -1305,6 +1355,7 @@ int handle_repl_config_push(cluster_t *c, endpoint_t *ep, xce_request_t *req)
rcpr->raw_sha256); rcpr->raw_sha256);
if (rc != 0) { if (rc != 0) {
WARN("handle_repl_config_push: conf_save_config failed: %d", rc); WARN("handle_repl_config_push: conf_save_config failed: %d", rc);
send_config_push_reply(c, ep, req, /*accepted*/0);
return rc; 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) if (act_rc != 0 && act_rc != -EAGAIN)
WARN("handle_repl_config_push: config_activate_committed rc=%d", act_rc); WARN("handle_repl_config_push: config_activate_committed rc=%d", act_rc);
/* ACK the primary */ /* ACK the primary. -EAGAIN (activation deferred) is NOT a rejection --
repl_config_push_reply_t reply_body = { * the bytes were saved out-of-band successfully, so this still replies
.accepted = 1, * accepted=1 (raft-adapter-conformance Req 2.5, 7.5: unchanged). */
.version_tag = c->myconf->config_version_tag, send_config_push_reply(c, ep, req, /*accepted*/1);
};
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);
return 0; return 0;
} }

View file

@ -463,6 +463,19 @@ int load_primary_config_for_test(cluster_t *c);
*/ */
extern void (*load_primary_config_race_hook)(void); 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 * push_ctx_t: exposed under XCE_TESTING so unit tests can construct
* synthetic push contexts to drive wait_all_with_timeout_for_test(). * synthetic push contexts to drive wait_all_with_timeout_for_test().

View file

@ -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"); 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 */ #endif /* XCE_TESTING */
/* ── raw capnp shims (Task A) ───────────────────── */ /* ── raw capnp shims (Task A) ───────────────────── */
@ -6848,6 +6924,8 @@ void test_config_repl(void)
RUN_TEST(test_wait_all_with_timeout_fast_threads); RUN_TEST(test_wait_all_with_timeout_fast_threads);
/* bug-1381: push_config_transport_only endpoint-lifetime regression */ /* bug-1381: push_config_transport_only endpoint-lifetime regression */
RUN_TEST(test_configpush_transport_holds_endpoint_ref_across_send); 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 */ /* Task 2.1: OPCode_walCatchupEntry handler */
RUN_TEST(test_wal_catchup_entry_advances_applied_lsn); RUN_TEST(test_wal_catchup_entry_advances_applied_lsn);
RUN_TEST(test_wal_catchup_entry_error_skipped_no_crash); RUN_TEST(test_wal_catchup_entry_error_skipped_no_crash);