diff --git a/.kiro/specs/config-upgrade-snapshot/tasks.md b/.kiro/specs/config-upgrade-snapshot/tasks.md index 02c455a3..dd280368 100644 --- a/.kiro/specs/config-upgrade-snapshot/tasks.md +++ b/.kiro/specs/config-upgrade-snapshot/tasks.md @@ -70,7 +70,7 @@ - _Requirements: 3.1, 3.2, 3.3, 3.4, 4.3, 5.5_ - _Depends: 6.1_ -- [ ] 7. Remove the per-node restart/replay fallback and replace it with the marker-triggered materialization +- [x] 7. Remove the per-node restart/replay fallback and replace it with the marker-triggered materialization - Delete the configuration-upgrade handler's restart-request step now that materialization is driven entirely by the marker-apply trigger, decoupled from that request-handling code path - Add a deterministic test seam that can force a leadership change to land exactly between a configuration-change marker's commit and every member completing its snapshot install, for later regression use - Observable: pushing a configuration upgrade no longer restarts any controller process, and every member reaches the new configuration through the marker-apply trigger alone diff --git a/src/controller/config_repl.c b/src/controller/config_repl.c index 3b04d620..43b2f0b4 100644 --- a/src/controller/config_repl.c +++ b/src/controller/config_repl.c @@ -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; } diff --git a/src/controller/testsuite/test_commit_barrier_rollout.c b/src/controller/testsuite/test_commit_barrier_rollout.c index c9765309..09a3a8a2 100644 --- a/src/controller/testsuite/test_commit_barrier_rollout.c +++ b/src/controller/testsuite/test_commit_barrier_rollout.c @@ -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);