lei.chen/myxce
1
0
Fork 1
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

feat(config-upgrade-snapshot): remove restart/replay fallback, marker-trigger materializes alone

Browse source 
do_upgrade_config_with_bytes drops step 9 (wait-for-checkpoint,
restart_requested=1, cbuf_put stop signal) -- materialization now
happens entirely via task 5's apply_committed_entry WAL_CONFIG_MARKER
case on every member's own apply, no restart required. Confirmed
do_upgrade_config_with_bytes was the sole production setter of
restart_requested; cluster.c's read of it is now permanently
false-for-this-trigger, which is the intended behavior.

Adds xce_upgrade_marker_committed_hook (#ifdef XCE_TESTING, mirrors
xce_snapshot_chunk_hook's nullable-function-pointer shape), firing
right after config_commit_change succeeds -- a deterministic seam for
later leadership-change fault injection (task 8.7) at exactly the
marker-commit-to-every-member-installed window. NULL by default, true
no-op in production.

Task 7 of 19.

Requirements: 1.2, 1.3, 1.4, 2.2, 5.4, 6.3
This commit is contained in:
Rongsong Shen 2026-07-25 05:37:27 +00:00
parent 89b85e1680
commit 41af05402d
3 changed files with 135 additions and 46 deletions
Download patch file Download diff file Expand all files Collapse all files

63
src/controller/config_repl.c
View file

@ -1570,6 +1570,21 @@ int push_config_to_standbys(cluster_t *c, const uint8_t *bytes, size_t blen,
/* ── handle_upgrade_config ──────────────────────── */
/* Test-only seam (config-upgrade-snapshot task 7): fires in
* do_upgrade_config_with_bytes right after config_commit_change() succeeds
* -- the configuration-change marker has just reached the committed
* prefix, but materialization on every member (apply_committed_entry's
* WAL_CONFIG_MARKER case, persist.c) has not necessarily completed
* anywhere yet. This is exactly the window a later regression test needs a
* deterministic way to land a forced leadership change in (Requirements
* 4.3, 5.4, 6.3). NULL in production -- no behaviour change. Mirrors
* xce_snapshot_chunk_hook's shape. Declared extern in the test file (same
* pattern as xce_snapshot_chunk_hook). */
#ifdef XCE_TESTING
int (*xce_upgrade_marker_committed_hook)(cluster_t *c,
uint64_t version_tag) = NULL;
#endif
/*
* do_upgrade_config_with_bytes: core upgrade logic called after the raw
* capnp bytes have been extracted from the wire via
@ -1586,7 +1601,12 @@ int push_config_to_standbys(cluster_t *c, const uint8_t *bytes, size_t blen,
* 7. Push to standbys and wait for the majority-ACK barrier; on failure
* reply with send_upgrade_error and return (no success ACK, no restart)
* 8. ACK admin tool (success ⇔ majority-acked config)
* 9. Wait for in-flight checkpoint, then signal restart
*
* The function's job ends at step 8. Materialization of the new
* configuration on every member -- including this one -- happens
* asynchronously and independently via each member's own apply of the
* committed WAL_CONFIG_MARKER (apply_committed_entry, persist.c,
* config-upgrade-snapshot task 5). No restart is requested or required.
*/
static int do_upgrade_config_with_bytes(cluster_t *c, endpoint_t *ep,
xce_request_t *req,
@ -1720,6 +1740,11 @@ static int do_upgrade_config_with_bytes(cluster_t *c, endpoint_t *ep,
return rc;
}
#ifdef XCE_TESTING
if (xce_upgrade_marker_committed_hook)
(void)xce_upgrade_marker_committed_hook(c, c->myconf->config_version_tag);
#endif
/* 8. ACK the admin tool (success ⇔ majority-acked config) */
upgrade_config_reply_t urep;
memset(&urep, 0, sizeof(urep));
@ -1734,41 +1759,13 @@ static int do_upgrade_config_with_bytes(cluster_t *c, endpoint_t *ep,
reply.data.upgradeconfigreplymsg = &urep;
xce_reply(ep, c->myhost, &reply);
/* NOTICE (bug-857, spec `.kiro/specs/config-upgrade-snapshot/`) — config
* upgrade MUST snapshot current state, not re-derive it per node.
* Today the upgrade commits the config-change marker, then each member
* RESTARTS and re-materializes its post-upgrade state by replaying its own
* pre-upgrade WAL under the NEW config (steps 9 + restart recovery). Replay
* is config-dependent (`lost_and_found` re-homes entities whose config
* entity vanished), so members can DIVERGE in materialized state after an
* upgrade. The fix: at the config-change marker's (term,lsn) boundary the
* controller must materialize a full ENTITY-BEARING state snapshot, embed
* the new config in it, and install that SAME snapshot on every member via
* the snapshot_transfer_s transport (install-before-activate, hash-verified)
* so post-upgrade state is identical by construction and no member
* re-derives it. HARD dependency: `entity-bearing-snapshots` (snapshots are
* a 16-byte LSN sentinel today; the entity payload does not exist yet).
* Until that lands, the per-node-replay path below is the interim behavior. */
/* 9. Wait for in-flight checkpoint (up to 30 s), then signal restart */
struct timespec ckpt_start, ckpt_now;
clock_gettime(CLOCK_MONOTONIC, &ckpt_start);
while (atomic_load(&c->persist.checkpoint_in_progress)) {
usleep(10000);
clock_gettime(CLOCK_MONOTONIC, &ckpt_now);
if (ckpt_now.tv_sec - ckpt_start.tv_sec > 30) {
ERROR("Checkpoint did not complete within 30 s"
" -- restarting anyway");
break;
}
}
/* Materialization now happens via apply_committed_entry's
* WAL_CONFIG_MARKER case (persist.c, config-upgrade-snapshot task 5) on
* every member's own apply of this marker -- no restart needed. */
INFO("Config upgraded to tag=%016" PRIx64 " gen=%" PRIu64
" (acked=%d standbys) -- requesting restart",
" (acked=%d standbys)",
c->myconf->config_version_tag, new_gen, acked);
atomic_store(&c->restart_requested, 1);
int stop_val = 1;
cbuf_put(&c->wbuf, &stop_val);
return 0;
}

116
src/controller/testsuite/test_commit_barrier_rollout.c
View file

@ -1505,13 +1505,18 @@ static void test_upgrade_config_barrier_failure_error_reply_no_restart(void)
printf("test_upgrade_config_barrier_failure_error_reply_no_restart: PASS\n");
}
/* Success path PIN: with a full majority of covering ACKs the existing
* behavior is unchanged — status-0 reply with the upgradeConfigReply
* payload, committed_config_tag at the saved tag, restart requested.
* Both standbys pre-ack UINT64_MAX (covers any tag — the new tag is only
* known after conf_save_config hashes the re-encoded blob), so the case
* is green under both the legacy counting and the per-push arithmetic. */
static void test_upgrade_config_majority_ack_success_unchanged(void)
/* Success path PIN: with a full majority of covering ACKs, status-0 reply
* with the upgradeConfigReply payload and committed_config_tag at the saved
* tag are unchanged — but as of config-upgrade-snapshot task 7 the
* restart/replay fallback is retired: a successful upgrade must NOT set
* restart_requested (Req 1.2/1.3/1.4/2.2/5.4). Materialization is now
* driven entirely by the marker-apply trigger (apply_committed_entry's
* WAL_CONFIG_MARKER case, persist.c, task 5), decoupled from this
* request-handling path. Both standbys pre-ack UINT64_MAX (covers any tag
* — the new tag is only known after conf_save_config hashes the re-encoded
* blob), so the case is green under both the legacy counting and the
* per-push arithmetic. */
static void test_upgrade_config_majority_ack_success_no_restart(void)
{
uint8_t *blob;
size_t blen = 0;
@ -1530,7 +1535,7 @@ static void test_upgrade_config_majority_ack_success_unchanged(void)
/* Act */
rc = handle_upgrade_config(&g_cb5_c, &g_cb5_req_ep, &g_cb5_req);
/* Assert: existing success reply + restart behavior unchanged */
/* Assert: existing success reply unchanged; restart is NOT requested */
TEST_ASSERT_EQUAL_INT_MESSAGE(0, rc,
"a majority-acked upgrade must succeed");
cb5_scan_replies(&g_cb5_req_ep, &obs);
@ -1547,12 +1552,98 @@ static void test_upgrade_config_majority_ack_success_unchanged(void)
atomic_load_explicit(&g_cb5_c.ha.committed_config_tag,
memory_order_acquire),
"a majority-acked config must be committed at the saved tag (Req 4.1)");
TEST_ASSERT_EQUAL_INT_MESSAGE(1, atomic_load(&g_cb5_c.restart_requested),
"a successful upgrade must request the activation restart");
TEST_ASSERT_EQUAL_INT_MESSAGE(0, atomic_load(&g_cb5_c.restart_requested),
"a successful upgrade must NOT request a restart -- materialization"
" happens via the marker-apply trigger alone (task 7, Req 1.2-1.4)");
free(blob);
cb5_teardown();
printf("test_upgrade_config_majority_ack_success_unchanged: PASS\n");
printf("test_upgrade_config_majority_ack_success_no_restart: PASS\n");
}
/* ── config-upgrade-snapshot task 7: marker-committed test hook ─────────── */
/* Captures each xce_upgrade_marker_committed_hook firing: the call count,
* the version_tag observed, and a snapshot of ha.committed_config_tag taken
* AT FIRE TIME -- proving the hook runs strictly after config_commit_change()
* has already advanced the committed tag (i.e. after the marker reached the
* committed prefix), not before. */
static int g_cb5_hook_calls;
static uint64_t g_cb5_hook_last_tag;
static uint64_t g_cb5_hook_committed_tag_at_fire;
static int cb5_marker_committed_hook(cluster_t *c, uint64_t version_tag)
{
g_cb5_hook_calls++;
g_cb5_hook_last_tag = version_tag;
g_cb5_hook_committed_tag_at_fire =
atomic_load_explicit(&c->ha.committed_config_tag, memory_order_acquire);
return 0;
}
/* The new #ifdef XCE_TESTING seam (config-upgrade-snapshot task 7,
* xce_upgrade_marker_committed_hook in config_repl.c) must fire exactly
* once per successful upgrade, right after config_commit_change() succeeds
* -- i.e. after the configuration-change marker has reached the committed
* prefix -- and must leave every other observable behavior (reply,
* committed tag, no restart) exactly as the NULL-hook case already asserts,
* proving the seam is a true no-op in production (hook defaults to NULL). */
static void test_upgrade_config_marker_committed_hook_fires_once_after_commit(void)
{
uint8_t *blob;
size_t blen = 0;
uint8_t sha[32];
int rc;
cb5_reply_obs_t obs = { 0 };
extern int (*xce_upgrade_marker_committed_hook)(cluster_t *, uint64_t);
/* Arrange: full covering pre-ack from both standbys (same fixture as
* the no-restart success-path test) */
cb5_setup_upgrade();
blob = cb5_make_config_blob(&blen);
SHA256(blob, blen, sha);
cb5_arrange_upgrade_req(blob, blen, sha);
repl_on_config_ack(&g_cb5_c.persist.repl, &g_cb5_sb_ep[0], UINT64_MAX);
repl_on_config_ack(&g_cb5_c.persist.repl, &g_cb5_sb_ep[1], UINT64_MAX);
g_cb5_hook_calls = 0;
g_cb5_hook_last_tag = 0;
g_cb5_hook_committed_tag_at_fire = 0;
xce_upgrade_marker_committed_hook = cb5_marker_committed_hook;
/* Act */
rc = handle_upgrade_config(&g_cb5_c, &g_cb5_req_ep, &g_cb5_req);
/* Assert: fired exactly once, with the just-committed tag, and only
* after ha.committed_config_tag already reflects that same tag. */
TEST_ASSERT_EQUAL_INT_MESSAGE(0, rc, "a majority-acked upgrade must succeed");
TEST_ASSERT_EQUAL_INT_MESSAGE(1, g_cb5_hook_calls,
"the marker-committed test hook must fire exactly once per"
" successful upgrade (task 7)");
TEST_ASSERT_EQUAL_UINT64_MESSAGE(g_cb5_myconf.config_version_tag,
g_cb5_hook_last_tag,
"the hook must observe the marker's own version tag");
TEST_ASSERT_EQUAL_UINT64_MESSAGE(g_cb5_myconf.config_version_tag,
g_cb5_hook_committed_tag_at_fire,
"the hook must fire AFTER config_commit_change already advanced"
" committed_config_tag, not before");
/* The rest of the observable behavior must be identical to the
* NULL-hook (production default) case -- the hook is side-effect-free
* beyond invoking the test callback itself. */
cb5_scan_replies(&g_cb5_req_ep, &obs);
TEST_ASSERT_EQUAL_INT_MESSAGE(1, obs.n_replies,
"the requester must still receive exactly one reply with the hook set");
TEST_ASSERT_EQUAL_INT_MESSAGE(0, obs.last_status,
"the success ACK must still carry status 0 with the hook set");
TEST_ASSERT_EQUAL_INT_MESSAGE(0, atomic_load(&g_cb5_c.restart_requested),
"the hook firing must not resurrect the retired restart request");
xce_upgrade_marker_committed_hook = NULL;
free(blob);
cb5_teardown();
printf("test_upgrade_config_marker_committed_hook_fires_once_after_commit:"
" PASS\n");
}
/* ════════════════════════════════════════════════════════════════════════
@ -2351,7 +2442,8 @@ void test_commit_barrier_rollout(void)
RUN_TEST(test_push_config_withheld_acks_returns_etimedout);
RUN_TEST(test_push_config_majority_acks_returns_zero_and_commits);
RUN_TEST(test_upgrade_config_barrier_failure_error_reply_no_restart);
RUN_TEST(test_upgrade_config_majority_ack_success_unchanged);
RUN_TEST(test_upgrade_config_majority_ack_success_no_restart);
RUN_TEST(test_upgrade_config_marker_committed_hook_fires_once_after_commit);
/* Task 5.2 remediation r1 — production wire-path ACK ingestion
* (completes the unlanded half of ha-config-bootstrap task 8.1) */
RUN_TEST(test_config_ack_wire_path_completes_barrier);