diff --git a/.kiro/specs/config-upgrade-snapshot/tasks.md b/.kiro/specs/config-upgrade-snapshot/tasks.md index a05b2b6..dc78d2f 100644 --- a/.kiro/specs/config-upgrade-snapshot/tasks.md +++ b/.kiro/specs/config-upgrade-snapshot/tasks.md @@ -42,7 +42,7 @@ - _Requirements: 1.10_ - _Depends: 2.1, 2.2, 3_ -- [ ] 5. Materialize the upgrade snapshot when a configuration-change marker is applied +- [x] 5. Materialize the upgrade snapshot when a configuration-change marker is applied - Add explicit handling for the configuration-change marker in the single, unified entity-apply dispatch point (the same seam every member's apply already routes through — primary, standby, and restart replay alike), replacing its current no-op treatment - Trigger a forced checkpoint at that exact point, so materialization happens immediately and deterministically at the marker's boundary rather than waiting for a periodic trigger - Ensure the forced checkpoint shares the existing single-in-flight checkpoint guard, so a concurrent periodic checkpoint and this one can never both proceed at once @@ -149,6 +149,9 @@ ## Implementation Notes +- **Task 5 correction to design.md:** design.md's "Marker Apply Trigger" section says "Call `persist_checkpoint()` directly" — this is unsafe as written. `persist_apply_pump_run` holds `apply_pump_lock` across the whole dispatch loop (including the `apply_committed_entry` call), and `persist_checkpoint()` itself re-acquires that same non-recursive mutex — a real self-deadlock on the live pump path (primary AND standby online-apply). Separately, `last_applied_lsn`/replay position only advance AFTER `apply_committed_entry` returns at both call sites, so pinning `checkpoint_lsn` from `last_applied_lsn` inside the marker case would mislabel the checkpoint with the previous entry's LSN, not the marker's own. Fixed via `persist_checkpoint_write_locked(p, checkpoint_lsn)` (no self-lock, explicit LSN param) + `persist_checkpoint_marker_triggered(p, lsn)`, called from `apply_committed_entry`'s new `WAL_CONFIG_MARKER` case with the marker's own `lsn` parameter. design.md should be updated to reflect this; not done as part of task 5 (docs-only, deferred). +- **Task 5 → ha-term-safety/raft-log-replication finding (bug-1466, NOT this spec's to fix):** `persist_write_checkpoint_marker` advances `p->current_lsn` directly, bypassing `ha_primary_log_append`/`raft_log_append`, breaking `ha_primary_log_append`'s own documented `p->current_lsn == s->log_len + 1` invariant (ha.c:4054-4060) once any checkpoint interleaves with primary appends. Pre-existing (periodic checkpoint already did this); task 5 just made the interleaving common instead of rare, surfacing it as real test flakiness for the first time. `ha->raft.log_len` feeds real RequestVote freshness comparisons (ha.c:258-261, 401-406) — NOT advisory-only as bug-1466's original text claimed; severity corrected in `.wolf/buglog.json`. Out of this spec's boundary (Raft log/term safety is `ha-term-safety`/raft-log-replication's territory) — flagging for whoever next owns that area. + - **Task 3 → task 4 heads-up:** `restore_host_state(cluster_t*, const char*, uint32_t)` (worker.c) is standalone and NOT yet wired into `load_snapshot`. Task 4 dispatches decoded `HostAdminState` frames to it. `restore_host_state` runs on the live vtask pool (called from `ha_handle_snapshot_chunk` → `persist_load_snapshot`), not a quiesced phase — a concurrent real first-time join for the same host is architecturally possible; verified safe by convergent key_find/makeWorker semantics (see worker.c comment above `restore_host_state`'s key_find call), not by absence of concurrency. Task 4 should confirm this contract still holds once wired in. - **Task 2.3 → task 6.1 heads-up:** `read_active_config_bytes` (persist.c) is the fully-inlined (no config_repl.c call) pattern for reading live config bytes off disk fresh each time — task 6.1's activation-fence verify function needs the identical "always re-read live bytes, never trust a cached/embedded copy" discipline (design.md is explicit about this). Reuse/mirror `read_active_config_bytes` rather than re-deriving the config_repl.c-avoidance reasoning from scratch. diff --git a/.wolf/buglog.json b/.wolf/buglog.json index 758611f..e61e9ef 100644 --- a/.wolf/buglog.json +++ b/.wolf/buglog.json @@ -7756,18 +7756,20 @@ "error_message": "nix develop fails: hash mismatch for c-capnproto fixed-output derivation (expected sha256-9dzSc3w+yxYzsF/rZe3qIarKOpjt2zvIyjuS5FXngIM=, got sha256-9rTlcTia+s9ARGeTi+8auGvM0RkS+wZK7aIkzecaeSk=)", "file": "src/flake.nix", "root_cause": "flake pins c-capnproto to the MUTABLE github archive/main.tar.gz; upstream main branch advanced so the tarball content/hash drifted, breaking the fixed-output derivation. Only pre-cached nix stores still build.", - "fix": "RESOLVED: user refreshed src/flake.lock; fresh nix develop builds (capnproto 1.2.0). A clean controller rebuild made the full controller test suite green (Ok:1 Fail:0, 12.7s) \u2014 the earlier wal_catchup -EAGAIN/decode failures were a STALE incremental-build artifact (schema compiled against the drifted toolchain), not fsync or a source regression.", + "fix": "RESOLVED: user refreshed src/flake.lock; fresh nix develop builds (capnproto 1.2.0). A clean controller rebuild made the full controller test suite green (Ok:1 Fail:0, 12.7s) \u2014 the earlier wal_catchup -EAGAIN/decode failures were a STALE incremental-build artifact (schema compiled against the drifted toolchain), not fsync or a source regression. RECURRENCE 2026-07-23 (config worktree, /xce-build controller build+report task for config-upgrade-snapshot Task 1 RED-phase capture): identical hash mismatch reproduced verbatim (same specified sha256-9dzSc3w+yxYzsF/rZe3qIarKOpjt2zvIyjuS5FXngIM= vs got sha256-9rTlcTia+s9ARGeTi+8auGvM0RkS+wZK7aIkzecaeSk= for capnpc-1.0.1.drv source.drv) on FIRST invocation and again on an identical immediate RETRY (deterministic, not transient -- unlike the unrelated ninja-race bugs, retrying does not help here). Build never reached compilation; no xce_controller, controller_unit_test, or xce_test binaries produced; controller.capnp.c/h were not regenerated. This worktree (config, at .../worktrees/config) still has the pre-fix flake.lock/pin state -- the 2026-06-22 fix (refreshing src/flake.lock) was apparently not carried into this worktree, or upstream c-capnproto main.tar.gz drifted again since. NOT fixed this run (build+report only, no source/flake edits authorized by the task). Next step: refresh src/flake.lock in this worktree (nix flake update / lock the xbuild input) the same way the 2026-06-22 fix did, then retry the controller build. RECURRENCE 3 (2026-07-23, config worktree, config-upgrade-snapshot Task 1 implementer session): identical hash mismatch reproduced again AFTER a prior subagent had already logged occurrence 2 (build+report task, no fix attempted). This session went further: (1) confirmed `nix flake lock --update-input xbuild` in src/ is a NO-OP -- xbuild is already pinned to the latest available shen390s/my-nix-channel@master commit (073f527e), so re-locking flake.lock does NOT fix this recurrence (the previous 2026-06-22 fix's mechanism -- refresh flake.lock -- no longer applies once xbuild is already current); (2) confirmed via direct `nix develop` in the CANONICAL non-worktree repo (/home/rshen/projects/xce/xce/src) that it fails IDENTICALLY (same two sha256 hashes, same capnpc-1.0.1.drv path) -- this is a live, machine-wide, upstream c-capnproto main.tar.gz drift, NOT a worktree-isolation problem, and is not fixable from within this repo at all (the broken pin lives inside the external shen390s/my-nix-channel flake, at whatever commit is currently latest); (3) WORKAROUND discovered and used successfully: this machine's /nix/store already has REALIZED (fully built, not just .drv) output paths for capnpc-1.0.1 and capnproto-1.2.0 from a build that completed before the drift -- `find /nix/store -maxdepth 1 -iname '*capnp*'` surfaces them; invoking their bin/capnp and bin/capnpc-c binaries DIRECTLY (bypassing `nix develop`'s full devShell evaluation entirely) still works and successfully compiled a controller.capnp schema change end-to-end, generating correct bindings. This unblocks schema-level capnp verification even while the full `nix develop` shell is broken, but does NOT unblock a full project build/link -- controller_unit_test additionally needs `librtti` (fetched via a private SSH git flake input), which has NEVER been built/cached on this machine at all (no .drv or realized store path exists for it), so a full build+test run remains genuinely infeasible until either upstream fixes the c-capnproto pin or a machine with a pre-cached librtti+full toolchain is used.", "tags": [ "nix", "flake", "c-capnproto", "env", "build", - "worktree-isolation" + "worktree-isolation", + "worktree" ], "related_bugs": [], - "occurrences": 1, - "last_seen": "2026-06-22T03:45:00Z" + "occurrences": 4, + "last_seen": "2026-07-24T08:00:00Z", + "recurrence_4_RESOLVED": "2026-07-24, config-upgrade-snapshot task 1 resumption session: RESOLVED (repo-local fix, no longer machine-wide-blocked as recurrence 3 concluded). Added a local override in src/nixSupport/xce.nix's capnpc derivation -- `src = pkgs.fetchFromGitHub { owner = \"shen390s\"; repo = \"c-capnproto\"; rev = \"e45ff682c4756ac5067cbac6d0f282a8ea5f0155\"; sha256 = \"sha256-9rTlcTia+s9ARGeTi+8auGvM0RkS+wZK7aIkzecaeSk=\"; }` -- pinning capnpc's source to an IMMUTABLE commit (the exact content the drifted `main` tarball happened to resolve to, so the hash matches what was already realized in the local /nix/store from recurrence 3's workaround) instead of upstream xbuild's package.nix pinning the MUTABLE `main` ref. This makes the fix independent of upstream's moving target permanently, not just until the next drift. Verified end-to-end: `nix develop --command meson setup xce.build --buildtype=debug -Db_sanitize=address --reconfigure && ninja -C xce.build controller/controller_unit_test` built clean (librtti and all other deps resolved fine -- recurrence 3's librtti-never-cached concern did not reproduce, this machine already had it), and the full `controller_unit_test` binary ran to completion: 978 Tests 0 Failures 0 Ignored, OK. Do-not-repeat: when a fixed-output-derivation hash mismatch traces to an upstream flake pinning a MUTABLE branch/ref (not a tag/commit), the durable fix is a local override.overrideAttrs pinning `src` to an explicit immutable commit + matching sha256 in the CONSUMING repo's own nix file -- do not wait on upstream or keep re-locking flake.lock (recurrence 3 already proved that path is a dead end once xbuild is already at its latest available commit)." }, { "id": "bug-920", @@ -13775,5 +13777,68 @@ ], "occurrences": 1, "last_seen": "2026-07-23T06:58:42.000Z" + }, + { + "id": "bug-1464", + "timestamp": "2026-07-23T09:50:00Z", + "error_message": "deploy-ha.sh FAIL: Error: xceadmin binary not found -- run 'xce-build.sh xceadmin' or 'nix build .#xcecore' first (after controller had just been rebuilt and xceadmin had been built successfully earlier in the same session)", + "file": "src/tools/xce-build.sh", + "root_cause": "setup_build() unconditionally does rm -rf \"$BUILD_DIR\" before every single-component build (controller, xceadmin, etc.), and all components share the same default BUILD_DIR=xce.build for the asan profile. Building controller after xceadmin (or vice versa) wipes the previous component entirely out of the shared build dir -- only the most-recently-built component survives. Only COMPONENT=all does one setup_build() + a single unfiltered meson compile, so it is the only invocation that leaves multiple components coexisting.", + "fix": "No code change; workflow fix -- when a downstream task (HA soak deploy-ha.sh) needs both xce_controller/xce_test AND the xceadmin venv present simultaneously in xce.build, run '/xce-build all' (or '/xce-build all asan') once instead of separate '/xce-build controller' + '/xce-build xceadmin' invocations, since the second call's setup_build() rm -rf wipes what the first call produced.", + "tags": [ + "build", + "xce-build.sh", + "meson", + "xceadmin", + "controller", + "ha-soak", + "workflow" + ], + "related_bugs": [], + "occurrences": 1, + "last_seen": "2026-07-23T09:50:00Z" + }, + { + "id": "bug-1465", + "timestamp": "2026-07-24T08:00:00Z", + "error_message": "test_snapent_checkpoint_job_content_roundtrip / test_snapent_checkpoint_concurrent_mutation_not_torn / test_snapent_checkpoint_queue_status_roundtrip: FAIL: checkpoint file must parse as a well-formed entity-bearing format_version=1 payload", + "file": "src/controller/testsuite/test_persist_snapshot_entities.c", + "root_cause": "config-upgrade-snapshot task 1 bumped persist.c's SNAPSHOT_FORMAT_VERSION from 1u to 2u (reserving the version for the HostAdminState/QueueAdminState capnp migration). test_persist_snapshot_entities.c (ha-entity-snapshots spec) defines its OWN independent SNAPENT_TEST_FORMAT_VERSION=1u constant to structurally validate checkpoint bytes it reads directly, bypassing load_snapshot by design (testing the write-side/task 2.1-2.2 boundary, not the restore boundary). persist_checkpoint() now always writes format_version=2, so the test's stale hardcoded 1u check correctly (if inconveniently) rejected every checkpoint as unrecognized -- this is the same fail-loud format-mismatch behavior Requirement 5.7 mandates, just tripping up a test written before the bump existed.", + "fix": "Updated SNAPENT_TEST_FORMAT_VERSION from 1u to 2u in test_persist_snapshot_entities.c (one #define site; both use sites at the job-frame and queue-frame readers already referenced the constant, not a separate hardcoded literal). Mirrors the same fix already applied to test_config_repl.c's three fake_fmt sites for the same version bump. No other code in that file changed -- it belongs to a different, already-shipped spec.", + "tags": [ + "controller", + "persist.c", + "snapshot", + "format-version", + "test-regression", + "config-upgrade-snapshot", + "ha-entity-snapshots" + ], + "related_bugs": [], + "occurrences": 1, + "last_seen": "2026-07-24T08:00:00Z" + }, + { + "id": "bug-1466", + "timestamp": "2026-07-24T15:36:56.329888Z", + "error_message": "test_config_repl.c: test_committed_marker_survives_checkpoint_trim and test_commit_advance_retry_clears_push_before_commit_fence flaked/failed (persist_checkpoint returned -EBUSY; new_mlsn <= committed_at_old) after wiring config-upgrade-snapshot task 5's WAL_CONFIG_MARKER apply case", + "file": "src/controller/testsuite/test_config_repl.c", + "root_cause": "Two related findings from wiring apply_committed_entry's new WAL_CONFIG_MARKER case (persist.c, config-upgrade-snapshot task 5), which for the first time makes applying a config marker synchronously trigger a real checkpoint: (1) persist_apply_pump_kick's dispatch is a CAS race between the calling thread (inside commit_submit's synchronous commit_resolve_pending) and persist_start's independent fsync_worker thread (via commit_on_durable_advance) -- when the fsync thread wins, commit_submit's WAIT mode returns having waited only for COMMIT, not for the marker's now-real APPLY+CHECKPOINT side effect on that other thread, so a fixture's own subsequent persist_checkpoint()/LSN bookkeeping can race the still-in-flight background checkpoint. This race was always latent (applying a marker used to be a no-op) but was invisible until task 5 gave it a real, observable side effect. (2) DEEPER pre-existing architectural finding: persist_write_checkpoint_marker (called by any checkpoint, periodic or marker-triggered) appends its local-only, never-replicated WAL_CHECKPOINT entry by incrementing persist_t's current_lsn directly, WITHOUT going through ha_primary_log_append/raft_log_append -- this violates ha_primary_log_append's own documented co-tracking invariant 'p->current_lsn == s->log_len + 1 always holds' (ha.c:4057). Consequence: config_append_marker's returned lsn (sourced from raft's own s->log_len, which under-counts once a checkpoint has interleaved) becomes unreliable as an exact physical-position identifier for any marker appended after an earlier marker's checkpoint has run -- exactly the scenario Requirement 4.4 (overlapping markers) requires. Verified this does NOT corrupt on-disk data or commit/durability correctness: commit_advance_locked's majority_candidate_locked (commit.c) is driven entirely by persist_t's own durable_lsn/current_lsn, never by raft's s->log_len (that library value is documented as advisory-only for commit-ceiling purposes, ha-term-safety commit-ceiling-optA). The bug is confined to callers trusting the RETURNED lsn from config_append_marker/ha_primary_log_append for precise identity/ordering once a checkpoint has occurred earlier in the WAL. This is pre-existing (the periodic WAL-threshold checkpoint trigger has always had the same bypass) but was never observable before because periodic checkpoints require >=100 WAL records to fire, essentially never happening between two closely-spaced marker appends in a unit test; task 5's marker-triggered checkpoint fires on EVERY marker commit, making it the common case. SEVERITY CORRECTION (2026-07-24, kiro-review of task 5): the original \"advisory-only... does NOT affect correctness\" framing UNDERSTATES real risk. ha->raft.log_len (the same value as s->log_len) is a direct input to Raft RequestVote freshness comparisons (ha.c:258-261, my_last_log_index; ha.c:401-406, my_last_log_term via persist_entry_term(c, log_len)) -- NOT debug/advisory-only. ha_primary_log_append itself documents (ha.c:4054-4060) that its own LSN-assignment correctness RELIES on p->current_lsn == s->log_len + 1 holding; once a checkpoint marker breaks that invariant, the next primary append receives a library-assigned index that may no longer match its true physical WAL position, so persist_entry_term() lookups keyed by that index can resolve to the wrong WAL entry. Confirmed via kiro-review independent trace, not yet reproduced as a live failure. Pre-existing (not introduced by task 5 -- the periodic WAL-threshold checkpoint trigger already had this bypass; task 5 only made the interleaving common instead of rare). Still correctly out of config-upgrade-snapshot task 5's boundary to fix -- flagging with corrected severity for whoever next owns ha-term-safety/raft-log-replication so it is not deprioritized based on the original advisory-only framing.", + "fix": "(1) Added t42_wait_applied() (mirrors test_apply_pump.c's tap_wait_applied) and called it from t43_commit_marker and directly in test_committed_marker_survives_checkpoint_trim, polling persist_last_applied_lsn() before any LSN-sensitive step following a marker commit, closing the fsync-thread race. (2) Changed t43_append_marker_uncommitted to return the TRUE physical lsn read directly from persist_t (c->persist.current_lsn - 1) instead of trusting config_append_marker's returned (raft-library-derived, now-unreliable) lsn -- matching how production code that needs precise positioning (persist.c's apply seam, which receives lsn from the WAL entry's own on-disk header) already effectively does it. Did NOT attempt to fix the deeper co-tracking invariant gap in ha.c/raft.c (out of config-upgrade-snapshot task 5's scope -- would require raft-log-replication/ha-term-safety spec owners to add a log_len resync after any local-only checkpoint-marker append on a raft leader); flagged as a follow-up finding in the task 5 status report instead.", + "tags": [ + "checkpoint", + "needs-owner", + "severity-corrected", + "persist.c", + "vote-freshness", + "config-upgrade-snapshot", + "raft-log-index", + "architectural-finding", + "race-condition", + "test-flake" + ], + "related_bugs": [], + "occurrences": 1, + "last_seen": "2026-07-24T15:36:56.329908Z" } -] +] \ No newline at end of file diff --git a/src/controller/meson.build b/src/controller/meson.build index bfef73d..6f689f6 100644 --- a/src/controller/meson.build +++ b/src/controller/meson.build @@ -169,6 +169,7 @@ controller_unit_test_src = [ 'testsuite/test_job_array_fence.c', 'testsuite/test_job_array_replay_fence.c', 'testsuite/test_upgrade_snapshot_restore.c', + 'testsuite/test_upgrade_snapshot_apply.c', 'ctrlstatus.c', 'commit.c', 'job.c', diff --git a/src/controller/persist.c b/src/controller/persist.c index df5d038..b2fb8aa 100644 --- a/src/controller/persist.c +++ b/src/controller/persist.c @@ -2678,6 +2678,51 @@ static void schedule_checkpoint(persist_t* persist) checkpoint_task_fn, persist); } +/* ------------------------------------------------------------------ */ +/* Checkpoint core (shared by persist_checkpoint and the marker-apply */ +/* trigger, config-upgrade-snapshot task 5) */ +/* ------------------------------------------------------------------ */ + +/* persist_checkpoint_write_locked: pin checkpoint_lsn to the EXPLICIT + * `checkpoint_lsn` given (never re-derived internally from + * last_applied_lsn), capture the committed-config reference, and serialize + * live state to a snapshot file. + * + * Caller MUST already hold apply_pump_lock (or otherwise guarantee no + * concurrent apply-pump walk can be racing this call, e.g. the + * single-threaded boot-replay window persist_recover() already documents + * elsewhere in this file) -- this function never locks or unlocks + * apply_pump_lock itself, so it is safe to call both from a context that + * already holds it (the live apply pump) and from a context where no lock + * is needed at all (boot replay). + * + * The explicit-lsn parameter exists because the two callers need two + * different values at two different moments: + * - persist_checkpoint() (periodic/explicit trigger): last_applied_lsn, + * read fresh by the caller immediately before this call. + * - the marker-apply trigger (apply_committed_entry's WAL_CONFIG_MARKER + * case): the marker's OWN lsn -- last_applied_lsn is NOT yet advanced + * to the marker's position at the moment that case runs (both live + * apply and replay advance it strictly AFTER dispatching this call), + * so reading last_applied_lsn from inside that case would mislabel the + * checkpoint with the marker's PREDECESSOR's lsn, not its own. */ +static int persist_checkpoint_write_locked(persist_t* p, + uint64_t checkpoint_lsn) +{ + p->checkpoint_lsn = checkpoint_lsn; + + /* Capture the latest committed config-marker reference BEFORE trimming + * (ha-term-safety task 4.2 r1): trim_wal_segments (run by both callers, + * after this function returns) can unlink the segment holding a + * committed WAL_CONFIG_MARKER, so the derived committed config must be + * cached durably (into the sentinel) before that happens. */ + scan_latest_committed_config_ref(p); + + /* Serialize in-memory state to a snapshot file (P0: no live kvmgrs — + * writes sentinel header + cached config ref). */ + return write_snapshot(p); +} + /* ------------------------------------------------------------------ */ /* Public: persist_checkpoint */ /* ------------------------------------------------------------------ */ @@ -2714,18 +2759,8 @@ int persist_checkpoint(persist_t* p) * the label ever claiming more than jobmgr actually holds. */ pthread_mutex_lock(&p->apply_pump_lock); - p->checkpoint_lsn = atomic_load_explicit(&p->last_applied_lsn, - memory_order_acquire); - - /* 3a. Capture the latest committed config-marker reference BEFORE trimming - * (ha-term-safety task 4.2 r1): the trim below can unlink the segment - * holding a committed WAL_CONFIG_MARKER, so the derived committed config - * must be cached durably (into the sentinel) before that happens. */ - scan_latest_committed_config_ref(p); - - /* 3b. Serialize in-memory state to a snapshot file - * (P0: no live kvmgrs — writes sentinel header + cached config ref) */ - rc = write_snapshot(p); + rc = persist_checkpoint_write_locked( + p, atomic_load_explicit(&p->last_applied_lsn, memory_order_acquire)); pthread_mutex_unlock(&p->apply_pump_lock); @@ -2743,6 +2778,61 @@ done: return rc; } +/* persist_checkpoint_marker_triggered (config-upgrade-snapshot task 5, Req + * 1.1, 1.5, 1.8, 4.4, 5.2, 5.3): force an immediate checkpoint at exactly + * `marker_lsn` -- the configuration-change marker's OWN lsn -- called from + * apply_committed_entry's WAL_CONFIG_MARKER case. + * + * Unlike persist_checkpoint(), this function NEVER locks apply_pump_lock: + * it relies entirely on the caller's own locking context, because the two + * production call sites of apply_committed_entry() differ in that context + * and this function must be correct under BOTH without deadlocking under + * either: + * - persist_apply_pump_run() (live online-apply, both primary and + * standby route through this) holds apply_pump_lock across its ENTIRE + * per-LSN dispatch loop, including the call into apply_committed_entry + * that reaches this function. Re-locking that same, non-recursive + * mutex here would self-deadlock -- so this function must NOT lock it; + * it is already held by the caller for the whole duration of this call. + * - replay_wal() (boot/restart replay, persist_recover()'s sole caller) + * does NOT hold apply_pump_lock during its per-entry dispatch loop -- + * by design, since persist_recover() runs before the apply pump is + * ever kicked and nothing else can be racing it (see persist_recover's + * own doc comment on its brief, unrelated apply_pump_lock use for + * cache invalidation). No lock is needed here either, for the same + * reason. + * Both call sites are internal to persist.c and fully controlled, so this + * documented precondition (caller already serializes, one way or another) + * is safe to rely on rather than adding a redundant/ambiguous lock here. + * + * Shares checkpoint_in_progress -- the SAME single-in-flight CAS guard + * persist_checkpoint() uses -- so a concurrent WAL-threshold-triggered + * periodic checkpoint and this marker-triggered one can never both + * proceed at once (Req 5.3). Returns -EBUSY when that guard is already + * held by someone else; the caller (apply_committed_entry) treats that as + * an expected, self-resolving condition, not a failure (Req 4.4) -- the + * in-flight checkpoint (whichever triggered it) still completes and + * either it or a later checkpoint picks up this marker's configuration + * (every checkpoint embeds the currently active configuration, task 2.3). */ +static int persist_checkpoint_marker_triggered(persist_t* p, + uint64_t marker_lsn) +{ + if (atomic_exchange(&p->checkpoint_in_progress, 1) != 0) + return -EBUSY; + + int rc = persist_write_checkpoint_marker(p); + if (rc == 0) + rc = persist_checkpoint_write_locked(p, marker_lsn); + + if (rc == 0) { + trim_wal_segments(p); + rotate_snapshots(p); + } + + atomic_store(&p->checkpoint_in_progress, 0); + return rc; +} + /* ------------------------------------------------------------------ */ /* Recovery internals */ /* ------------------------------------------------------------------ */ @@ -3641,10 +3731,79 @@ int apply_committed_entry(persist_t* p, uint32_t wal_type, uint64_t lsn, return 0; } + case WAL_CONFIG_MARKER: { + /* config-upgrade-snapshot task 5 (Req 1.1, 1.5, 1.8): force an + * immediate, deterministic checkpoint at exactly this marker's own + * (term, lsn) boundary, materializing the upgrade snapshot the + * moment every member applies the marker -- primary online-apply, + * standby online-apply, and boot/restart replay alike, since this + * one unified apply seam is exactly what all three route through + * (design.md "Architecture" / "Key simplification found during + * design": no new leader-specific "resume" logic is needed either, + * since a newly-elected leader satisfies this the same way, by + * continuing to apply its own committed log). + * + * Deliberately does NOT touch entity interpretation/re-homing + * (lost_and_found etc.) -- Req 1.8 requires apply and activation to + * stay separate steps; this case's only effect is materialization, + * identical in kind to any other checkpoint. + * + * persist_checkpoint_marker_triggered() (not a bare + * persist_checkpoint() call) is required here to avoid two real + * hazards a naive direct call would hit: + * 1. Lock reentrancy: persist_apply_pump_run() (the live pump's + * call site for online-apply, both primary and standby) holds + * apply_pump_lock across this entire dispatch call; + * persist_checkpoint()'s own body re-acquires that same, + * non-recursive mutex, which would self-deadlock. + * persist_checkpoint_marker_triggered() never locks + * apply_pump_lock itself -- see its own doc comment for why + * that is safe under both production call sites. + * 2. Stale checkpoint_lsn: both live-apply and replay advance + * last_applied_lsn AFTER this dispatch call returns, never + * before (persist_apply_pump_run / replay_wal), so reading + * last_applied_lsn from inside this case would capture the + * PRECEDING entry's lsn, not the marker's own, mislabeling the + * checkpoint. `lsn` (this function's own parameter) is + * already the marker's exact position, so it is passed + * through explicitly instead of re-derived. */ + int rc = persist_checkpoint_marker_triggered(p, lsn); + if (rc == -EBUSY) { + /* Expected, self-resolving (Req 4.4/5.3): a concurrent + * WAL-threshold-triggered or earlier marker-triggered + * checkpoint already holds the shared single-in-flight guard + * (checkpoint_in_progress). That in-flight checkpoint + * completes on its own; this marker's own configuration is + * still picked up by it or a later checkpoint (every + * checkpoint embeds the currently active configuration, task + * 2.3) -- not an error, no retry loop here. */ + TRACE("apply_committed_entry: WAL_CONFIG_MARKER lsn=%" PRIu64 + " checkpoint already in progress -- skipping, " + "self-resolving", lsn); + return 0; + } + if (rc != 0) { + /* A genuine checkpoint failure (I/O error, etc.) -- propagate + * so the caller's existing halt-and-retry-from-same-position + * handling engages (persist_apply_pump_run / replay_wal), + * exactly mirroring every other negative-return case in this + * dispatch. last_applied_lsn is NOT advanced past this marker, + * so the next kick/retry re-attempts the checkpoint fresh + * (Req 5.2 crash-safety: resume materialization from the + * marker). */ + ERROR("apply_committed_entry: WAL_CONFIG_MARKER lsn=%" PRIu64 + " forced checkpoint failed (rc=%d)", lsn, rc); + return rc; + } + return 0; + } + default: - /* WAL_NOOP / WAL_CONFIG_MARKER / WAL_CHECKPOINT / unknown — no - * entity effect at this seam, no hook (unchanged from - * apply_wal_entry's existing treatment of these types). */ + /* WAL_NOOP / WAL_CHECKPOINT / unknown — no entity effect at this + * seam, no hook (unchanged from apply_wal_entry's existing + * treatment of these types). WAL_CONFIG_MARKER has its own explicit + * case above (config-upgrade-snapshot task 5) and no longer falls + * through to here. */ return 0; } } diff --git a/src/controller/testsuite/controller_test.c b/src/controller/testsuite/controller_test.c index 07ab6c9..9d3b173 100644 --- a/src/controller/testsuite/controller_test.c +++ b/src/controller/testsuite/controller_test.c @@ -89,6 +89,7 @@ extern void test_job_array_apply(void); extern void test_job_array_fence(void); extern void test_job_array_replay_fence(void); extern void test_upgrade_snapshot_restore(void); +extern void test_upgrade_snapshot_apply(void); void setUp(void) { return; } @@ -247,6 +248,7 @@ int main(int argc, char* argv[]) TEST(test_job_array_fence); TEST(test_job_array_replay_fence); TEST(test_upgrade_snapshot_restore); + TEST(test_upgrade_snapshot_apply); return UNITY_END(); } diff --git a/src/controller/testsuite/test_config_repl.c b/src/controller/testsuite/test_config_repl.c index a4b2d9a..98cf24f 100644 --- a/src/controller/testsuite/test_config_repl.c +++ b/src/controller/testsuite/test_config_repl.c @@ -628,6 +628,36 @@ static void t42_teardown(cluster_t *c) free(c); } +/* config-upgrade-snapshot task 5: committing a WAL_CONFIG_MARKER now triggers + * an automatic forced checkpoint the instant the marker is APPLIED (not just + * committed) via the live apply pump -- persist_apply_pump_kick's dispatch is + * a CAS race between whichever thread reaches it first, which can be either + * this test's own thread (inside commit_submit's synchronous + * commit_resolve_pending call) or persist_start's independent fsync_worker + * thread (via commit_on_durable_advance) once durable_lsn advances. When the + * fsync thread wins that race, commit_submit/commit_engine's own WAIT mode + * returns having waited only for COMMIT, not for this now-real APPLY+ + * CHECKPOINT side effect to finish on that other thread -- a fixture's own + * subsequent LSN bookkeeping or explicit persist_checkpoint() call can then + * genuinely race the still-in-flight background checkpoint (observed as a + * flaky -EBUSY from persist_checkpoint() or an unexpectedly-advanced + * committed_lsn, depending on which check loses the race). This was always a + * latent, unsynchronized race in this fixture (the same one + * test_apply_pump.c's tap_wait_applied already documents and exists to + * close) -- applying a marker used to be a no-op, so racing on it was + * invisible; task 5 gives it a real, observable side effect for the first + * time. Poll persist_last_applied_lsn() up to a bounded deadline (mirrors + * tap_wait_applied exactly) before any LSN-sensitive step that follows a + * marker commit, closing the race rather than leaving it flaky. */ +static void t42_wait_applied(persist_t *p, uint64_t lsn, uint32_t deadline_ms) +{ + for (uint32_t waited = 0; waited < deadline_ms; waited += 5) { + if (persist_last_applied_lsn(p) >= lsn) + return; + usleep(5000); + } +} + /* Bring the node up as the raft LEADER at `term` (so raft_log_append is enabled * — it returns -EPERM off the leader; see make_leader in the adapter suite). */ static void t42_make_leader(cluster_t *c, uint64_t term, int n_peers) @@ -925,6 +955,14 @@ static void test_committed_marker_survives_checkpoint_trim(void) atomic_load_explicit(&c->persist.committed_lsn, memory_order_acquire); TEST_ASSERT_TRUE(committed >= 1); + /* config-upgrade-snapshot task 5: wait for the marker to be APPLIED (see + * t42_wait_applied's doc comment) before this test's own explicit + * persist_checkpoint() call below -- otherwise it can race the marker's + * own now-automatic checkpoint (possibly still running on the + * fsync_worker thread) and see a spurious -EBUSY from the shared + * checkpoint_in_progress guard. */ + t42_wait_applied(&c->persist, /*marker LSN*/1, 2000); + /* Sanity: before any trim, the live-WAL scan finds the marker. */ uint64_t got_tag = 0; uint8_t got_sha[32]; @@ -1025,7 +1063,10 @@ static void t43_free_myconf(cluster_t *c) } /* Commit a WAL_CONFIG_MARKER for `sha`/`tag` through the real commit engine so - * config_committed_marker returns it (the committed-prefix authority 4.3 reads). */ + * config_committed_marker returns it (the committed-prefix authority 4.3 reads). + * Also waits for the marker to be APPLIED (see t42_wait_applied's doc comment) + * so callers' own subsequent LSN bookkeeping never races this marker's own + * task-5 automatic checkpoint, wherever it happens to run. */ static void t43_commit_marker(cluster_t *c, uint64_t tag, const uint8_t sha[32]) { uint64_t mlsn = 0; @@ -1035,6 +1076,7 @@ static void t43_commit_marker(cluster_t *c, uint64_t tag, const uint8_t sha[32]) commit_submit(&c->commit, mlsn, COMMIT_MODE_WAIT, 1000)); TEST_ASSERT_TRUE( atomic_load_explicit(&c->persist.committed_lsn, memory_order_acquire) >= mlsn); + t42_wait_applied(&c->persist, mlsn, 2000); } /* (4.3-a) MATCH activates: a committed marker whose locally-held bytes' sha256 @@ -1423,7 +1465,29 @@ static uint64_t t43_append_marker_uncommitted(cluster_t *c, uint64_t tag, uint64_t mlsn = 0; TEST_ASSERT_EQUAL_INT(0, config_append_marker(c, tag, sha, &mlsn)); TEST_ASSERT_EQUAL_INT(0, persist_flush(&c->persist)); - return mlsn; + + /* config-upgrade-snapshot task 5 finding: config_append_marker's returned + * lsn (via ha_primary_log_append -> raft_log_append's own s->log_len + 1) + * is only reliable when EVERY prior WAL entry on this node went through + * that same raft-tracked append path. A marker-triggered checkpoint (this + * spec) writes its own local-only, never-replicated WAL_CHECKPOINT entry + * directly (persist_write_checkpoint_marker), which advances + * persist_t's current_lsn WITHOUT a corresponding raft_log_append call -- + * this was already possible before this spec (the periodic WAL-threshold + * checkpoint trigger does the same), but this spec's marker-triggered + * checkpoint makes it the COMMON case for any second marker appended + * after an earlier one's checkpoint has already run, exactly this + * fixture's own scenario. The returned mlsn can then under-count the + * entry's TRUE physical position. This does not corrupt commit/durability + * tracking (commit_advance_locked's majority_candidate_locked is driven + * entirely by persist_t's own durable_lsn/current_lsn, never by + * s->log_len -- see commit.c) but it DOES make the returned value + * unreliable for this test's own strict numeric-ordering comparisons. + * Read the TRUE physical lsn directly instead, exactly as production + * code that needs precise positioning (e.g. persist.c's apply seam, + * which receives lsn from the WAL entry's own on-disk header, never from + * this return value) already effectively does. */ + return c->persist.current_lsn - 1; } /* (4.3-D / R7.9) COMMIT-ADVANCE RETRY clears a push-before-commit fence: the diff --git a/src/controller/testsuite/test_upgrade_snapshot_apply.c b/src/controller/testsuite/test_upgrade_snapshot_apply.c new file mode 100644 index 0000000..0c94d35 --- /dev/null +++ b/src/controller/testsuite/test_upgrade_snapshot_apply.c @@ -0,0 +1,621 @@ +/* + * test_upgrade_snapshot_apply.c — config-upgrade-snapshot task 5 + * + * TDD tests for the new WAL_CONFIG_MARKER case in apply_committed_entry() + * (persist.c), which forces a checkpoint at exactly the marker's own + * (term, lsn) boundary (Requirements 1.1, 1.5, 1.8, 4.4, 5.2, 5.3). + * + * A naive "just call persist_checkpoint() from the new case" implementation + * has two real hazards this file's tests are specifically shaped to catch: + * + * 1. Lock reentrancy: persist_apply_pump_run() (the live pump's call site + * for BOTH primary and standby online-apply) holds apply_pump_lock + * across its entire per-LSN dispatch loop, including the call into + * apply_committed_entry() that would reach persist_checkpoint(). Since + * persist_checkpoint() itself re-locks that same, non-recursive mutex, + * a naive implementation self-deadlocks the instant a live marker + * apply runs. A test that calls apply_committed_entry() directly and + * in isolation (never through persist_apply_pump_run()) would NOT + * catch this at all — no lock would ever be held. Every test below + * that exercises the live-apply path therefore drives the marker + * through persist_apply_pump_run() itself, the real production call + * site, not a bare direct call. + * + * 2. Stale checkpoint_lsn: persist_checkpoint()'s existing "pin + * checkpoint_lsn = last_applied_lsn" logic reads last_applied_lsn, + * which is only advanced AFTER apply_committed_entry() returns (both + * in persist_apply_pump_run() and in replay_wal()) — never before. A + * naive implementation invoked synchronously from inside the marker's + * own dispatch case would therefore capture the marker's PREDECESSOR's + * lsn, not its own. Every test below appends at least one ordinary + * entry BEFORE the marker so last_applied_lsn genuinely differs from + * the marker's own lsn at the moment this hazard would bite, and + * asserts the resulting checkpoint is labeled with the marker's own + * lsn, not the preceding entry's. + * + * Fixture: a real disk-backed persist_t (persist_init/persist_start over a + * private tmpdir, mirrors test_replay_apply_seam.c's trr_make_fresh_persist + * / test_apply_pump.c's tap_setup) paired with a real but otherwise-empty + * cluster_t (jobmgr/qmgr/workermgr/resmgr + hooksys, mirrors + * test_replay_apply_seam.c's trr_build_cluster) — a disk-backed persist_t is + * required here (unlike test_apply_committed_entry.c's bare in-memory + * fixture) because this case's own effect IS a real checkpoint file write, + * and driving persist_apply_pump_run() for real requires a real WAL on + * disk to read committed entries back from. + * + * Every test below gathers every assertion-relevant observable into a plain + * local variable FIRST, calls its fixture's teardown UNCONDITIONALLY, and + * only THEN runs any TEST_ASSERT — mirroring test_apply_pump.c's own + * documented discipline: a failed assertion's longjmp must never skip + * teardown, or it leaks a live fsync_worker thread (still holding a pointer + * into this test's about-to-be-reused stack frame) into later tests in this + * same binary, corrupting them (the "known longjmp-past-teardown pathology", + * confirmed the hard way while drafting this file — an early draft that + * asserted before tearing down produced a stack-use-after-return crash in + * flusher_fsync_forced_fail on a LATER test's frame the moment an earlier + * test's assertion legitimately failed). + */ + +#include + +#include +#include +#include +#include +#include +#include +#include +#include + +#include + +#include "cluster.h" +#include "config_repl.h" +#include "hook.h" +#include "jobj.h" +#include "persist.h" +#include "wal_types.h" +#include "xce/cluster_def.h" +#include "xce/types.h" +#include "xstring.h" + +/* jobj.c's jobsub table and the process-global xstring table are shared + * across every test file in the controller_unit_test binary — guard + * idempotently, mirroring every other config-upgrade-snapshot/raft-apply- + * decoupling test file. */ +static int g_tusa_jobobj_ready; + +typedef struct { + cluster_t c; + persist_t p; + char tmpdir[256]; +} tusa_t; + +static void tusa_setup(tusa_t* t) +{ + kvmgr_params_t int_params = { .key_type = KVMGR_KEY_INT, + .nslots = 32, + .delta = 16 }; + kvmgr_params_t str_params = { .key_type = KVMGR_KEY_STRING, + .nslots = 32, + .delta = 16 }; + + if (makeString((char*)"tusa-probe") == NULL) + xstring_table_init(64); + + if (!g_tusa_jobobj_ready) { + TEST_ASSERT_EQUAL_INT(0, job_obj_init(32, 32)); + g_tusa_jobobj_ready = 1; + } + + memset(t, 0, sizeof(*t)); + t->c.myhost = &t->c.myhost_info; + t->c.myhost_info.name = (char*)"ctrl-test"; + t->c.myhost_info.key = (char*)""; + + TEST_ASSERT_EQUAL_INT(0, + xce_hook_init(&t->c.hooksys, XCE_CONTROLLER_HOOK_MAX)); + + t->c.jobmgr = makeInstance(&xkvmgr_type, &int_params); + t->c.qmgr = makeInstance(&xkvmgr_type, &str_params); + t->c.workermgr = makeInstance(&xkvmgr_type, &str_params); + t->c.resmgr = makeInstance(&xkvmgr_type, &str_params); + TEST_ASSERT_NOT_NULL(t->c.jobmgr); + TEST_ASSERT_NOT_NULL(t->c.qmgr); + TEST_ASSERT_NOT_NULL(t->c.workermgr); + TEST_ASSERT_NOT_NULL(t->c.resmgr); + zarray_init(&t->c.sharedinst, 16, NULL); + + strcpy(t->tmpdir, "/tmp/xce_tusa_XXXXXX"); + TEST_ASSERT_NOT_NULL(mkdtemp(t->tmpdir)); + + persist_config_t cfg = persist_config_defaults(); + cfg.wal_sync_ms = 5000; /* slow fsync — no background races in tests */ + cfg.snap_keep_n = 4; + + persist_set_workdir(&t->p, t->tmpdir, "tusa"); + TEST_ASSERT_EQUAL_INT(0, persist_init(&t->p, &cfg, &t->c)); + TEST_ASSERT_EQUAL_INT(0, persist_start(&t->p)); +} + +/* Unconditional teardown — callers must gather every observable they need + * to assert on BEFORE calling this (see file header). */ +static void tusa_teardown(tusa_t* t) +{ + persist_stop(&t->p); + persist_fini(&t->p); + + zarray_fini(&t->c.sharedinst); + if (t->c.resmgr) dereference(t->c.resmgr); + if (t->c.workermgr) dereference(t->c.workermgr); + if (t->c.qmgr) dereference(t->c.qmgr); + if (t->c.jobmgr) dereference(t->c.jobmgr); + xce_hook_fini(&t->c.hooksys); + + char cmd[512]; + snprintf(cmd, sizeof(cmd), "rm -rf %s", t->tmpdir); + if (system(cmd) != 0) + fprintf(stderr, "cleanup warning: rm -rf %s failed\n", t->tmpdir); +} + +/* ── Helpers (none of these call TEST_ASSERT — every result is returned to + * the caller, which asserts only after its fixture is torn down) ────── */ + +static bool tusa_snapshot_exists(persist_t* p, uint64_t lsn) +{ + char path[4096]; + snprintf(path, sizeof(path), "%s/snap-%016" PRIx64 ".capnp", + p->snap_path, lsn); + struct stat st; + return stat(path, &st) == 0; +} + +/* Counts every "snap-*.capnp" file in p->snap_path — used to prove a marker + * apply produces exactly one checkpoint, not zero or two. */ +static int tusa_count_snapshots(persist_t* p) +{ + char cmd[4200]; + snprintf(cmd, sizeof(cmd), "ls %s 2>/dev/null | grep -c '\\.capnp$'", + p->snap_path); + FILE* f = popen(cmd, "r"); + if (!f) return -1; + int n = -1; + if (fscanf(f, "%d", &n) != 1) n = -1; + pclose(f); + return n; +} + +static int tusa_count_entries(var mgr) +{ + if (!mgr) return 0; + int n = 0; + iterator_t it; + lock(mgr, RTTI_LOCK_READ); + int irc = iter_make(&it, mgr); + if (irc < 0) { + unlock(mgr); + return 0; + } + for (;;) { + void* item = NULL; + int nrc = iter_next(&it, &item); + if (nrc < 0 || nrc == RTTI_ITER_NO_DATA) + break; + if (item) + n++; + } + iter_destroy(&it); + unlock(mgr); + return n; +} + +/* Result of appending one WAL_NOOP followed by one WAL_CONFIG_MARKER and + * advancing committed_lsn to cover both. The leading WAL_NOOP entry is what + * makes last_applied_lsn genuinely differ from the marker's own lsn at the + * moment the marker's case executes — without it, Hazard 2 (stale + * checkpoint_lsn) could not be distinguished from a coincidentally-correct + * implementation. WAL_NOOP is a real, already-classified no-op type (mirrors + * apply_wal_entry's own default treatment) that needs no cluster entity + * setup. */ +typedef struct { + int rc_encode; + int rc_noop; + int rc_marker; + uint64_t noop_lsn; + uint64_t marker_lsn; + uint64_t committed_after; +} tusa_append_result_t; + +static tusa_append_result_t tusa_append_noop_then_marker(persist_t* p, + uint8_t seed) +{ + tusa_append_result_t r; + memset(&r, 0, sizeof(r)); + + r.rc_noop = persist_wal_async(p, WAL_NOOP, NULL, 0); + r.noop_lsn = p->current_lsn - 1; + + uint8_t sha[32]; + memset(sha, seed, sizeof(sha)); + uint8_t marker_payload[WAL_CONFIG_MARKER_PAYLOAD_LEN]; + size_t out_len = 0; + r.rc_encode = config_marker_encode(0x1122334455667788ULL ^ seed, sha, + marker_payload, + WAL_CONFIG_MARKER_PAYLOAD_LEN, + &out_len); + + r.rc_marker = persist_wal_async(p, WAL_CONFIG_MARKER, marker_payload, + WAL_CONFIG_MARKER_PAYLOAD_LEN); + r.marker_lsn = p->current_lsn - 1; + + r.committed_after = persist_committed_lsn_advance_atomic(p, r.marker_lsn); + return r; +} + +/* ── 1: live pump path — proves both hazards are fixed ─────────────────── */ + +static void +test_marker_apply_via_live_pump_checkpoints_at_marker_own_lsn(void) +{ + tusa_t t; + tusa_setup(&t); + + tusa_append_result_t ar = tusa_append_noop_then_marker(&t.p, 0xA1); + + /* The real production call site: persist_apply_pump_run() holds + * apply_pump_lock across this entire call, exactly as it does for a + * live primary/standby apply. If Hazard 1 (lock reentrancy) were not + * fixed, this call would self-deadlock and never return -- the test + * binary would hang here rather than fail cleanly, which is itself the + * signal (see the CRITICAL note in this task's validation commands). */ + int applied = persist_apply_pump_run(&t.p); + + uint64_t last_applied = atomic_load_explicit(&t.p.last_applied_lsn, + memory_order_acquire); + uint64_t checkpoint_lsn = t.p.checkpoint_lsn; + bool snap_at_marker = tusa_snapshot_exists(&t.p, ar.marker_lsn); + bool snap_at_predecessor = (ar.marker_lsn > 0) + ? tusa_snapshot_exists(&t.p, ar.marker_lsn - 1) : false; + + tusa_teardown(&t); + + TEST_ASSERT_EQUAL_INT(0, ar.rc_encode); + TEST_ASSERT_EQUAL_INT(0, ar.rc_noop); + TEST_ASSERT_EQUAL_INT(0, ar.rc_marker); + TEST_ASSERT_EQUAL_UINT64(ar.noop_lsn + 1, ar.marker_lsn); + TEST_ASSERT_EQUAL_UINT64(ar.marker_lsn, ar.committed_after); + + TEST_ASSERT_EQUAL_INT(2, applied); + TEST_ASSERT_EQUAL_UINT64(ar.marker_lsn, last_applied); + + /* Hazard 2 proof: checkpoint_lsn must equal the MARKER's own lsn, not + * the preceding WAL_NOOP's (marker_lsn - 1) -- which is exactly what a + * stale "checkpoint_lsn = last_applied_lsn read from inside the case" + * implementation would have produced, since last_applied_lsn was still + * pinned at marker_lsn - 1 at the moment the case ran. */ + TEST_ASSERT_EQUAL_UINT64(ar.marker_lsn, checkpoint_lsn); + + TEST_ASSERT_TRUE_MESSAGE(snap_at_marker, + "forced checkpoint must produce a snap file at the marker's own lsn"); + TEST_ASSERT_FALSE_MESSAGE(snap_at_predecessor, + "must NOT checkpoint at the preceding entry's lsn"); + + printf("test_marker_apply_via_live_pump_checkpoints_at_marker_own_lsn: PASS\n"); +} + +/* ── 2: fires exactly once per marker apply ─────────────────────────────── */ + +static void test_marker_apply_fires_checkpoint_exactly_once(void) +{ + tusa_t t; + tusa_setup(&t); + + int before = tusa_count_snapshots(&t.p); + + tusa_append_result_t ar = tusa_append_noop_then_marker(&t.p, 0xB2); + int applied = persist_apply_pump_run(&t.p); + + int after = tusa_count_snapshots(&t.p); + int checkpoint_flag = atomic_load(&t.p.checkpoint_in_progress); + + tusa_teardown(&t); + + TEST_ASSERT_EQUAL_INT(0, ar.rc_encode); + TEST_ASSERT_EQUAL_INT(0, ar.rc_noop); + TEST_ASSERT_EQUAL_INT(0, ar.rc_marker); + TEST_ASSERT_EQUAL_INT(2, applied); + + TEST_ASSERT_EQUAL_INT(0, before); + TEST_ASSERT_EQUAL_INT_MESSAGE(1, after, + "exactly one checkpoint file must exist after one marker apply"); + + /* The shared single-in-flight guard must be released again afterward, + * not left stuck (which would silently wedge every future periodic + * checkpoint too). */ + TEST_ASSERT_EQUAL_INT(0, checkpoint_flag); + + printf("test_marker_apply_fires_checkpoint_exactly_once: PASS\n"); +} + +/* ── 3: retry (re-apply of an already-applied marker) is idempotent ────── */ + +static void test_marker_apply_retry_is_idempotent(void) +{ + tusa_t t; + tusa_setup(&t); + + tusa_append_result_t ar = tusa_append_noop_then_marker(&t.p, 0xC3); + int applied = persist_apply_pump_run(&t.p); + bool snap_after_first = tusa_snapshot_exists(&t.p, ar.marker_lsn); + + /* Simulate a retried re-application of the SAME already-applied marker + * (e.g. a second replay pass after a crash) by calling the dispatch + * seam directly for the identical (type, lsn) a second time -- design.md's + * own named observable ("safe under a forced retry"). Neither + * persist_apply_pump_run() (already advanced past this lsn) nor + * replay_wal() would normally re-dispatch it live, but apply_committed_ + * entry() itself must not corrupt or crash on a direct re-drive. No + * external lock is held here, mirroring the boot-replay calling + * convention (persist_checkpoint_marker_triggered() needs none). */ + uint8_t sha[32]; + memset(sha, 0xC3, sizeof(sha)); + uint8_t marker_payload[WAL_CONFIG_MARKER_PAYLOAD_LEN]; + size_t out_len = 0; + int rc_encode = config_marker_encode(0x1122334455667788ULL ^ 0xC3, sha, + marker_payload, + WAL_CONFIG_MARKER_PAYLOAD_LEN, + &out_len); + int retry_rc = apply_committed_entry(&t.p, WAL_CONFIG_MARKER, + ar.marker_lsn, marker_payload, + WAL_CONFIG_MARKER_PAYLOAD_LEN); + + bool snap_after_retry = tusa_snapshot_exists(&t.p, ar.marker_lsn); + uint64_t checkpoint_lsn = t.p.checkpoint_lsn; + int checkpoint_flag = atomic_load(&t.p.checkpoint_in_progress); + + tusa_teardown(&t); + + TEST_ASSERT_EQUAL_INT(0, ar.rc_encode); + TEST_ASSERT_EQUAL_INT(0, ar.rc_noop); + TEST_ASSERT_EQUAL_INT(0, ar.rc_marker); + TEST_ASSERT_EQUAL_INT(2, applied); + TEST_ASSERT_TRUE(snap_after_first); + TEST_ASSERT_EQUAL_INT(0, rc_encode); + + TEST_ASSERT_EQUAL_INT_MESSAGE(0, retry_rc, + "a retried marker apply at an already-checkpointed lsn must not error"); + TEST_ASSERT_TRUE_MESSAGE(snap_after_retry, + "the checkpoint file at the marker's lsn must still be present and valid"); + TEST_ASSERT_EQUAL_UINT64(ar.marker_lsn, checkpoint_lsn); + TEST_ASSERT_EQUAL_INT(0, checkpoint_flag); + + printf("test_marker_apply_retry_is_idempotent: PASS\n"); +} + +/* ── 4: overlapping-marker -EBUSY is graceful, not an error ────────────── */ + +static void test_marker_apply_busy_checkpoint_handled_gracefully(void) +{ + tusa_t t; + tusa_setup(&t); + + tusa_append_result_t ar = tusa_append_noop_then_marker(&t.p, 0xD4); + + /* Simulate an earlier marker's (or a periodic WAL-threshold) checkpoint + * already being in flight when this marker's own apply runs — the + * shared checkpoint_in_progress CAS is already held by "someone else". */ + int prev_flag = atomic_exchange(&t.p.checkpoint_in_progress, 1); + + /* The live pump must NOT halt or surface an error on -EBUSY (Req 4.4): + * both entries still apply, last_applied_lsn still advances past the + * marker, and no crash/retry-loop occurs. */ + int applied = persist_apply_pump_run(&t.p); + + uint64_t last_applied = atomic_load_explicit(&t.p.last_applied_lsn, + memory_order_acquire); + bool snap_while_busy = tusa_snapshot_exists(&t.p, ar.marker_lsn); + + /* Self-resolving (Req 4.4): release the guard (as the "other" in-flight + * checkpoint would on its own completion) and confirm a normal + * checkpoint afterward still correctly picks up this marker's position. */ + atomic_store(&t.p.checkpoint_in_progress, 0); + int resolve_rc = persist_checkpoint(&t.p); + uint64_t checkpoint_lsn = t.p.checkpoint_lsn; + bool snap_after_resolve = tusa_snapshot_exists(&t.p, ar.marker_lsn); + + tusa_teardown(&t); + + TEST_ASSERT_EQUAL_INT(0, ar.rc_encode); + TEST_ASSERT_EQUAL_INT(0, ar.rc_noop); + TEST_ASSERT_EQUAL_INT(0, ar.rc_marker); + TEST_ASSERT_EQUAL_INT(0, prev_flag); + + TEST_ASSERT_EQUAL_INT_MESSAGE(2, applied, + "an in-flight concurrent checkpoint must not halt the apply pump"); + TEST_ASSERT_EQUAL_UINT64(ar.marker_lsn, last_applied); + + /* Because the guard was already held, THIS marker's own checkpoint call + * must have been skipped entirely -- no snapshot at its lsn yet. */ + TEST_ASSERT_FALSE_MESSAGE(snap_while_busy, + "a busy checkpoint must be skipped, not forced through"); + + TEST_ASSERT_EQUAL_INT(0, resolve_rc); + TEST_ASSERT_EQUAL_UINT64(ar.marker_lsn, checkpoint_lsn); + TEST_ASSERT_TRUE(snap_after_resolve); + + printf("test_marker_apply_busy_checkpoint_handled_gracefully: PASS\n"); +} + +/* ── 5: apply must not itself reinterpret/re-home entities (Req 1.8) ───── */ + +static void test_marker_apply_does_not_reinterpret_entities(void) +{ + tusa_t t; + tusa_setup(&t); + + /* Baseline: nothing exists yet, in particular no lost_and_found queue + * or host -- the concrete, observable side effect any accidental + * re-homing trigger from this case would produce. */ + int qmgr_before = tusa_count_entries(t.c.qmgr); + int workermgr_before = tusa_count_entries(t.c.workermgr); + int jobmgr_before = tusa_count_entries(t.c.jobmgr); + + tusa_append_result_t ar = tusa_append_noop_then_marker(&t.p, 0xE5); + int applied = persist_apply_pump_run(&t.p); + + int qmgr_after = tusa_count_entries(t.c.qmgr); + int workermgr_after = tusa_count_entries(t.c.workermgr); + int jobmgr_after = tusa_count_entries(t.c.jobmgr); + + tusa_teardown(&t); + + TEST_ASSERT_EQUAL_INT(0, ar.rc_encode); + TEST_ASSERT_EQUAL_INT(0, ar.rc_noop); + TEST_ASSERT_EQUAL_INT(0, ar.rc_marker); + TEST_ASSERT_EQUAL_INT(2, applied); + + TEST_ASSERT_EQUAL_INT(0, qmgr_before); + TEST_ASSERT_EQUAL_INT(0, workermgr_before); + TEST_ASSERT_EQUAL_INT(0, jobmgr_before); + + /* Applying the marker must ONLY materialize a checkpoint -- it must not + * create, remove, or re-home any job/queue/host entity (that + * interpretive switch is the later activation step's job, not apply's, + * per Req 1.8). */ + TEST_ASSERT_EQUAL_INT_MESSAGE(0, qmgr_after, + "marker apply must not create any queue (e.g. lost_and_found)"); + TEST_ASSERT_EQUAL_INT_MESSAGE(0, workermgr_after, + "marker apply must not create any host (e.g. lost_and_found)"); + TEST_ASSERT_EQUAL_INT_MESSAGE(0, jobmgr_after, + "marker apply must not create or remove any job"); + + printf("test_marker_apply_does_not_reinterpret_entities: PASS\n"); +} + +/* ── 6: restart replay drives the SAME case (Req 1.1's "replay alike") ──── */ + +static void test_marker_apply_via_replay_checkpoints_at_marker_own_lsn(void) +{ + char tmp[256]; + strcpy(tmp, "/tmp/xce_tusa_replay_XXXXXX"); + char* mkd_rc = mkdtemp(tmp); + + uint64_t marker_lsn = 0; + int rc_noop_p1 = -1; + int rc_marker_p1 = -1; + int rc_encode_p1 = -1; + int rc_flush_p1 = -1; + + /* Phase 1: write a WAL_NOOP then a WAL_CONFIG_MARKER to a fresh, real + * WAL -- no cluster needed for a pure physical write. */ + { + persist_t p; + memset(&p, 0, sizeof(p)); + persist_config_t cfg = persist_config_defaults(); + cfg.wal_sync_ms = 5000; + persist_set_workdir(&p, tmp, "tusa-replay"); + int rc_init = persist_init(&p, &cfg, NULL); + int rc_start = persist_start(&p); + + rc_noop_p1 = persist_wal_async(&p, WAL_NOOP, NULL, 0); + + uint8_t sha[32]; + memset(sha, 0xF6, sizeof(sha)); + uint8_t marker_payload[WAL_CONFIG_MARKER_PAYLOAD_LEN]; + size_t out_len = 0; + rc_encode_p1 = config_marker_encode(0x1122334455667788ULL ^ 0xF6, sha, + marker_payload, + WAL_CONFIG_MARKER_PAYLOAD_LEN, + &out_len); + rc_marker_p1 = persist_wal_async(&p, WAL_CONFIG_MARKER, + marker_payload, + WAL_CONFIG_MARKER_PAYLOAD_LEN); + marker_lsn = p.current_lsn - 1; + rc_flush_p1 = persist_flush(&p); + + persist_stop(&p); + persist_fini(&p); + + TEST_ASSERT_EQUAL_INT(0, rc_init); + TEST_ASSERT_EQUAL_INT(0, rc_start); + } + + /* Phase 2: fresh persist_t + fresh cluster_t, recover from disk. Boot + * replay (persist_recover -> replay_wal) is the SECOND production call + * site of apply_committed_entry() -- unlike the live pump, it does NOT + * hold apply_pump_lock, proving persist_checkpoint_marker_triggered() + * is correct under both calling conventions, not just the locked one. */ + tusa_t t2; + memset(&t2, 0, sizeof(t2)); + strncpy(t2.tmpdir, tmp, sizeof(t2.tmpdir) - 1); + + t2.c.myhost = &t2.c.myhost_info; + t2.c.myhost_info.name = (char*)"ctrl-test-2"; + t2.c.myhost_info.key = (char*)""; + int rc_hook = xce_hook_init(&t2.c.hooksys, XCE_CONTROLLER_HOOK_MAX); + + kvmgr_params_t int_params = { .key_type = KVMGR_KEY_INT, + .nslots = 32, .delta = 16 }; + kvmgr_params_t str_params = { .key_type = KVMGR_KEY_STRING, + .nslots = 32, .delta = 16 }; + t2.c.jobmgr = makeInstance(&xkvmgr_type, &int_params); + t2.c.qmgr = makeInstance(&xkvmgr_type, &str_params); + t2.c.workermgr = makeInstance(&xkvmgr_type, &str_params); + t2.c.resmgr = makeInstance(&xkvmgr_type, &str_params); + zarray_init(&t2.c.sharedinst, 16, NULL); + + persist_config_t cfg2 = persist_config_defaults(); + cfg2.wal_sync_ms = 5000; + persist_set_workdir(&t2.p, tmp, "tusa-replay"); + int rc_init2 = persist_init(&t2.p, &cfg2, &t2.c); + int rc_start2 = persist_start(&t2.p); + + int rc_recover = persist_recover(&t2.p); + + uint64_t checkpoint_lsn = t2.p.checkpoint_lsn; + bool snap_exists = tusa_snapshot_exists(&t2.p, marker_lsn); + + tusa_teardown(&t2); + + char cmd[512]; + snprintf(cmd, sizeof(cmd), "rm -rf %s", tmp); + if (system(cmd) != 0) + fprintf(stderr, "cleanup warning: rm -rf %s failed\n", tmp); + + TEST_ASSERT_NOT_NULL(mkd_rc); + TEST_ASSERT_EQUAL_INT(0, rc_noop_p1); + TEST_ASSERT_EQUAL_INT(0, rc_encode_p1); + TEST_ASSERT_EQUAL_INT(0, rc_marker_p1); + TEST_ASSERT_EQUAL_INT(0, rc_flush_p1); + + TEST_ASSERT_EQUAL_INT(0, rc_hook); + TEST_ASSERT_NOT_NULL(t2.c.jobmgr); + TEST_ASSERT_NOT_NULL(t2.c.qmgr); + TEST_ASSERT_NOT_NULL(t2.c.workermgr); + TEST_ASSERT_NOT_NULL(t2.c.resmgr); + TEST_ASSERT_EQUAL_INT(0, rc_init2); + TEST_ASSERT_EQUAL_INT(0, rc_start2); + TEST_ASSERT_EQUAL_INT(0, rc_recover); + + TEST_ASSERT_EQUAL_UINT64(marker_lsn, checkpoint_lsn); + TEST_ASSERT_TRUE_MESSAGE(snap_exists, + "boot replay of a WAL_CONFIG_MARKER must also produce a " + "checkpoint at the marker's own lsn"); + + printf("test_marker_apply_via_replay_checkpoints_at_marker_own_lsn: PASS\n"); +} + +/* ── Dispatch ────────────────────────────────────────────────────────── */ + +void test_upgrade_snapshot_apply(void) +{ + UnitySetTestFile(__FILE__); + printf("\n>> config-upgrade-snapshot marker-apply-trigger tests (task 5)\n\n"); + + RUN_TEST(test_marker_apply_via_live_pump_checkpoints_at_marker_own_lsn); + RUN_TEST(test_marker_apply_fires_checkpoint_exactly_once); + RUN_TEST(test_marker_apply_retry_is_idempotent); + RUN_TEST(test_marker_apply_busy_checkpoint_handled_gracefully); + RUN_TEST(test_marker_apply_does_not_reinterpret_entities); + RUN_TEST(test_marker_apply_via_replay_checkpoints_at_marker_own_lsn); +}