fix(config): serialize load_primary_config against concurrent invocations (bug-1476)
myconf->lock (mylock_init/fini wiring) had landed but load_primary_config() itself was left in its RED-phase reproduction state (unguarded, decoding directly into the shared myconf->cluster_config field), so the fix's own regression test failed on resume: thread B raced through while thread A was parked mid-call. Wraps the whole free-decode-publish sequence in myconf->lock (acquired first, released on every return path) and decodes into a local cluster_config_t* first, publishing into myconf->cluster_config only after full decode and validation succeed -- closing the UAF/double-free window a live 3-node ASan soak caught (two concurrent handle_repl_config_push deliveries for the same standby racing two load_primary_config calls). Verified 4/4 clean full-suite runs (1036 tests, 0 failures) including the specific race test.
This commit is contained in:
parent
ce8df99037
commit
b08e392eeb
8 changed files with 704 additions and 31 deletions
File diff suppressed because one or more lines are too long
|
|
@ -12,6 +12,8 @@
|
|||
- Keep test logs for analysis, don't skip intermediate output even if a test hangs
|
||||
|
||||
## Key Learnings
|
||||
- (2026-07-27/28, bug-1475 FIXED) A term/version index that grows its backing array geometrically and zero-fills the newly-grown region to mean "absent" has a subtle, TIME-DEPENDENT bug: zero-filling with the same sentinel value used for a legitimate "recorded as literal 0" entry makes a deliberately-never-recorded slot (here: persist.c's per-LSN term_index, where a WAL_CHECKPOINT's own LSN is BY DESIGN never passed to record_entry_term) silently and RETROACTIVELY flip from "absent" to "present, value 0" the instant ANY later index is recorded and the array-growth watermark (`term_index_count`) crosses it — even though nothing ever explicitly wrote to that slot. Two readers that check the SAME never-recorded slot at DIFFERENT times (e.g. a sender that reads it AFTER appending later entries, vs. a receiver that reads it BEFORE appending anything past it) get DIFFERENT answers for what should be a stable, position-only fact — this produced a permanent AppendEntries logOk rejection (bug-1475: sender computed prev_log_term=1 (WAL_LEGACY_TERM), receiver computed prev_log_term=0 (absent), for the identical checkpoint LSN). **General fix pattern**: reserve a DIFFERENT sentinel (here `TERM_INDEX_UNSET = UINT64_MAX`, filled via `memset(...,0xFF,...)` on array growth) distinct from every legitimate stored value, so a never-explicitly-written slot reads "absent" PERMANENTLY, independent of later growth — never let "grown but unwritten" collide with a real, meaningful value (here, literal term 0 / WAL_LEGACY_TERM) in the same sentinel. **Do-not-repeat**: when hand-tracing (not instrumenting) a bug involving an in-memory index that has a documented "grows on demand, gaps read as absent" contract, don't trust that the gap actually STAYS absent across the FULL lifetime of the process/test — verify with real instrumentation whether "absent" is a stable property of the position or merely a snapshot of "not yet grown past." This was found only after two prior, plausible-looking production fixes (config_repl.c's tracker-advance + ha.c's JIT log_len resync, both already landed) still left the regression test RED — real fprintf instrumentation at the streamer, the JIT resync, and raft.c's logOk check (all added and fully removed afterward) pinned the exact moment prev_log_term flipped from 0 to 1. See `.wolf/buglog.json` bug-1475 for the full three-part fix and `test_bug1475_catchup_survives_midstream_checkpoint` (test_config_repl.c) for the regression proof.
|
||||
|
||||
- (2026-07-11, controller_unit_test isolated-group + leak-detection runs) `controller_unit_test` supports `--run STR` (repeatable, arg key `-t`) to select one or more TEST()-registered groups by exact function name (matches `argp`-parsed `-t`/`--run`, dispatch via `check_test()`/`TEST()` macro in `controller_test.c`) — use it to isolate a single suite instead of parsing the full-suite log. Full-suite direct runs interleave two PASS-print conventions: older suites only print via Unity's own per-assertion output (no `<file>:<line>:<name>:PASS` banner), while newer TDD suites (apply/HA-prevote family) add a custom `printf("%s: PASS\n", ...)` + banner — so grepping for `../controller/testsuite/<file>.c` banners under-counts groups that don't use the newer convention; don't infer suite pass/fail or crash-truncation from banner presence alone, always check the process exit code. Confirmed via meson (`Ok:1/Fail:0`, one lumped "controller test") AND a clean direct exit(0) with zero grep hits for AddressSanitizer/heap-buffer-overflow/UAF/double-free/FAIL across a 15.7k-line full-suite log — the log's abrupt-looking tail (last group's own internal UNITY_BEGIN/END subtotal printed twice) is normal, not truncation; verify with `echo $?` instead. The historical "test_ha_prevote 18/18" figure from an earlier session was the COMBINED total of `test_ha_prevote` (12 tests, confirmed via `grep -c RUN_TEST` in the .c file) + `test_ha_failover_prevote` (6 tests) — the two are separate TEST() groups in `controller_test.c`, not one suite of 18; report them separately (12/12 + 6/6) to avoid a false "regression" reading.
|
||||
- (2026-07-08, HA soak test sequential vs chaotic modes) Sequential mode soak validates clean failover lifecycle: 10 rounds, 36 jobs, zero loss, all 3 nodes led primary — deterministic validation of promotion + catch-up + reapply paths working as designed. Chaotic mode deliberately overlaps kills across election/catch-up windows to surface edge cases: rounds 1-4 pass verification, round 5 hits primary-discovery lag (bug-854 root-caused earlier, excluded from verdict), rounds 6-7 stall from election convergence lag + duplicate-node-guard (bug-1418, pre-existing guard). The election stall in chaotic round 6 is likely compound: (a) tight overlapping kill cadence outpaces election+restart timing (heartbeat-timeout bounds ~500ms but nix shell spinup adds latency), (b) a node restarted but not fully converged rejoins before the prior kill round's primary finishes its term tenure. Sequential mode PASS verdict is solid baseline. Chaotic mode needs either slower kill spacing OR a real-fs host deploy (current sandbox has overlayfs fdatasync EIO bug-924) to distinguish fault-tolerance (still holds) from environment-induced timing collapse.
|
||||
- (2026-07-04, raft-apply-decoupling task 6.1 round 2 — job-state merge-staleness fix) Round 1's `apply_committed_entry` wired the live pump but left a real corruption: `record_job_fail_fact`/`record_job_update_fact` (job.c) precomputed a FINAL absolute state at RECEIPT time (a scratch-copy merge for FAIL, an inline backstop+PDONE merge for postdone) and baked it into `wal_jobstate_t.state`; under deferred apply, a second fact for the same job (e.g. FINISH) could apply BETWEEN receipt and this fact's own apply, making the baked snapshot stale — a POST_EXEC-stage FAIL received while a job is still `RUN` (FINISH not yet applied) bakes `RUN|EXIT|PERR`, and when FINISH applies first (`DONE`) then FAIL's blind absolute overwrite clobbers it back to `RUN|EXIT|PERR` — DONE silently lost. FIX PATTERN: `wal_jobstate_t._pad` (previously always-zero, unused) is repurposed as a discriminator — `_pad==0` still means "legacy absolute state, safe" (job_ctrl's mark_job_fail/mark_job_update paths, which mutate+capture state synchronously under one lock, no staleness window); a nonzero `_pad` (`WAL_JOB_FAIL_STAGE_FLAG|stage` for FAIL, `WAL_JOB_UPDATE_POSTDONE_MARK` for postdone) marks "recompute at apply time against ACTUAL current state" — `apply_committed_entry` (persist.c) dispatches on this bit per WAL type, calling the REAL `apply_job_fail_transition`/new `apply_job_postdone_transition` (job.c, mirrors the existing guard-core family) instead of `apply_job_update_transition`'s blind overwrite. New low-level `persist_submit_job_fact_ex(p, type, jobid, state, pad)` added as the primitive; `persist_submit_job_fact` becomes a pad=0 thin wrapper — ZERO existing call sites (including ~20 in test_commit_barrier_reply.c) needed touching. GENERAL PATTERN for "wire format has 2 producers, one safe one not": don't change the safe producer's shape — add a discriminator field/flag so the consumer can tell them apart, and make "0"/"absent" mean exactly the old/safe behavior so every existing writer keeps working unchanged.
|
||||
|
|
@ -515,3 +517,50 @@ Fixed two `xce-audit-invariants controller` thread-role violations (`ha_demotion
|
|||
## Do-Not-Repeat (2026-07-21, admin-config-tool task 1.3, bug-1459) — a nix-provided `python3`'s site-packages lives under the read-only `/nix/store`, so `pip install`/`pip install -e .` against it directly fails with "externally-managed-environment" (PEP 668), even from inside `nix develop`. Fix pattern: `python3 -m venv --system-site-packages "$VENV_DIR"` (inherits the nix-provided heavy binary deps like `pycapnp` for free, so pip never re-downloads/rebuilds them) then run `pip`/`python` via `"$VENV_DIR/bin/..."`, not bare. This is the general pattern for any future nix + `pip install` component in this repo, not xceadmin-specific.
|
||||
|
||||
## Do-Not-Repeat (2026-07-21, admin-config-tool task 1.3, bug-1460) — a Python package's non-`.py` data files (here: `xcecommon/schema_files/*.capnp` and `schema_files/json/*.schema.json`) are silently DROPPED from a wheel built by `pip install .` (non-editable) unless declared via `[tool.setuptools.package-data]` in `pyproject.toml`. An editable install (`pip install -e .`) never catches this — it resolves straight into the checked-out source tree, bypassing wheel packaging entirely. Any bundled-non-code-asset pattern in this repo's Python components needs an explicit `package-data` entry, and the ONLY way to verify it works is to actually run the non-editable/`release` install profile and inspect `site-packages`, not just the editable one.
|
||||
|
||||
## Do-Not-Repeat (2026-07-23, bug-1464) — every single-component `/xce-build <comp>` invocation does `rm -rf` on the SHARED `xce.build` dir before compiling; all components (controller, xceadmin, etc.) default to the same build dir per profile. Running `/xce-build controller` then `/xce-build xceadmin` separately wipes whichever was built first — only the most recent single-component build survives. When a downstream task (e.g. an HA soak's `deploy-ha.sh`) needs binaries from MULTIPLE components present simultaneously, use `/xce-build all` (one `setup_build()` + one unfiltered `meson compile`), not sequential per-component calls.
|
||||
|
||||
## Key Learning (2026-07-23, bug-1418) — a `blocked_on` entry in a spec.json (or a project memory note) claiming a bug is "NOT FIXED" can itself be stale; the 2026-07-21 blocked_on note on config-upgrade-snapshot was ~2 weeks out of date against `.wolf/buglog.json`'s own 2026-07-09 resolution entry for bug-1418. Before trusting/acting on any blocked_on or "still open" claim, cross-check buglog.json's own entries for that bug id first — re-verified fresh here with a 10-round `--chaotic` soak (PASS, zero divergence/loss), then cleared config-upgrade-snapshot's blocked_on to [].
|
||||
|
||||
## Key Learning (2026-07-23, config-upgrade-snapshot task 1) — this codebase's `controller.capnp` convention: EVERY struct gets `$C.mapname(...)`, even a brand-new wire-only "frame" concept with no corresponding "live" runtime C struct. For that case, define a small hand-written `typedef struct { ... } capnp_X_t;` in `src/include/typemap.h` (matching field-for-field, `Text`→`char*`, `UIntN`→`uintN_t`) and `$C.mapname` the schema struct onto it — exactly the pattern already used by `SnapshotHeader`/`capnp_snap_hdr_t`, `PersistConfig`/`capnp_persist_cfg_t`, `ReplEnvelope`/`capnp_repl_envelope_t` (all in typemap.h's "Persistence Cap'n Proto mapped types" section). This gives you generated `encode_X`/`decode_X`/`free_X` conversion functions for free (via the file-level `$C.codecgen` annotation) instead of hand-writing capnp field accessors. Do NOT emit a bare `struct X { name @0: Text; ... }` with no mapname even if a design doc's sketch shows it that way literally — cross-check real schema precedent first. Used this for `HostAdminState`/`QueueAdminState` → `capnp_host_admin_state_t`/`capnp_queue_admin_state_t`.
|
||||
|
||||
## Do-Not-Repeat (2026-07-23, bug-918 recurrence 3) — when `nix develop`/`/xce-build` fails with a fixed-output-derivation hash mismatch on `capnpc-1.0.1.drv`'s `source.drv`, do NOT assume `nix flake lock --update-input xbuild` (the documented fix from the 2026-06-22 occurrence) will help without checking first — `nix flake metadata` may already show `xbuild` pinned to the LATEST available `shen390s/my-nix-channel@master` commit, in which case re-locking is a verified no-op (confirmed: ran it, `git diff -- src/flake.lock` was empty). Also confirmed via a direct `nix develop` in the canonical non-worktree repo (`/home/rshen/projects/xce/xce/src`) that it fails IDENTICALLY — this is a live, machine-wide upstream break (c-capnproto's mutable `main.tar.gz` drifted again), not a worktree-isolation artifact, and is NOT fixable from inside this repo at all. **Workaround that DOES work for schema-only verification**: `find /nix/store -maxdepth 1 -iname '*capnp*'` to check for already-REALIZED (not just `.drv`) `capnpc-1.0.1`/`capnproto-1.2.0` store paths from a build that completed before the drift — if present, invoke their `bin/capnp`/`bin/capnpc-c` DIRECTLY (`capnp compile -o <full-path-to-capnpc-c>:<outdir> -I <schema_dir> --src-prefix <schema_dir> controller.capnp`), bypassing `nix develop` entirely. This proves a schema change compiles and inspect the generated `.c`/`.h` for correct struct/function generation, but does NOT unblock a full project build — `controller_unit_test` also needs `librtti` (private SSH git flake input), which had NO `.drv` or realized store path on this machine at all (never built here), so full build+link+test remains infeasible until upstream fixes the pin or a machine with a pre-cached librtti toolchain is used. When this recurs, check bug-918's full `fix` field in buglog.json first — it now documents this exact triage sequence.
|
||||
|
||||
## Key Learning (2026-07-24, bug-918 RESOLVED, recurrence 4) — the actual durable fix for the c-capnproto fixed-output-derivation hash mismatch (recurrences 1-3 above): a local `overrideAttrs` in `src/nixSupport/xce.nix`'s `capnpc` derivation pinning `src = pkgs.fetchFromGitHub { owner = "shen390s"; repo = "c-capnproto"; rev = "<immutable commit sha>"; sha256 = "<matching hash>"; }` — this makes THIS repo's build independent of upstream xbuild's package.nix, which pins the MUTABLE `main` ref and breaks every time that branch moves. `librtti` was NOT actually a blocker on this machine (recurrence 3's concern that it had "never been built/cached" turned out to be stale/machine-state-dependent) — a full `meson setup --reconfigure && ninja controller/controller_unit_test` built clean and the full suite ran (978 Tests 0 Failures). **Do-not-repeat**: for ANY future fixed-output-derivation hash mismatch traced to an upstream flake pinning a mutable branch/ref (not a tag/commit), the fix is a local `overrideAttrs` pinning `src` to an explicit immutable commit in the CONSUMING repo, not re-locking flake.lock or waiting on upstream — that path is a dead end once the upstream input is already at its latest commit (recurrence 3 already proved this).
|
||||
|
||||
## Do-Not-Repeat (2026-07-24, config-upgrade-snapshot task 1, bug-1465) — bumping a shared on-disk format-version constant (here `persist.c`'s `SNAPSHOT_FORMAT_VERSION`) breaks ANY test file that hardcodes its own independent copy of that value to structurally validate bytes it reads directly (bypassing the real read-path on purpose, to test the write side in isolation) — even in a DIFFERENT, already-shipped spec's test file that the current task has no reason to otherwise touch. `test_config_repl.c`'s `fake_fmt` sites were caught and fixed in the same session the bump was made; `test_persist_snapshot_entities.c`'s independent `SNAPENT_TEST_FORMAT_VERSION` constant was NOT (different file, no shared symbol/include tying them together) and only surfaced as 3 failures once the full suite finally ran (blocked for a session by the bug-918 nix issue above). **General rule**: after bumping any shared format/version constant, `grep -rn` the whole test tree for OTHER hardcoded copies of the old literal value (not just call sites that reference the primary constant by name) before declaring the bump done — a stale hardcoded copy in an unrelated spec's test file is a silent regression that only a full-suite run catches.
|
||||
|
||||
## Key Learning (2026-07-24, config-upgrade-snapshot task 2.3) — `persist.c` deliberately has ZERO function-call dependency on `config_repl.c` (not even the trivial `#include "config_repl.h"`). Two precedents establish this as an intentional domain boundary, not an oversight: `scan_latest_committed_config_ref` (persist.c) inline-decodes WAL_CONFIG_MARKER payloads instead of calling any config_repl.c decode helper ("decoded inline here so persist stays free of a config_repl layering dependency"); task 2.3's new `read_active_config_bytes` (persist.c, embeds active config bytes into every checkpoint) fully inline-reimplements `conf_read_current_filename`+`conf_read_raw`'s path-resolution-and-read logic (~140 lines) rather than calling those existing, already-public (`config_repl.h`) functions, even though `ha.c` DOES call `conf_read_raw` cross-module via a bare `extern` declaration at 2 sites (config push to standbys) — design.md's Architecture boundary map explains the asymmetry: `ha.c` is the intentional CONSUMER of "config replication (config_repl.c) owns config-bytes sourcing," so a ha.c→config_repl.c call is expected in-boundary integration, while `persist.c` owns materialization/on-disk-format and is kept deliberately free of that domain's dependency. **When touching persist.c again** (e.g. task 5's marker-apply trigger, task 6's Activation Fence — which design.md explicitly says must "verify against LIVE on-disk config bytes each attempt" mirroring `config_activate_committed`'s re-read-every-time pattern, `config_repl.c:275-302`) — check whether the new code lands in persist.c (keep it config_repl.c-dependency-free, duplicate the small read-with-validation routine if needed) or ha.c (fine to call config_repl.c directly, precedent already established). Do not "clean up" persist.c's duplication by refactoring it to call config_repl.c — that would be reversing a deliberate, design.md-documented boundary decision, not a simplification.
|
||||
|
||||
## Key Learning (2026-07-24, config-upgrade-snapshot task 5) — a lock-reentrancy or stale-value hazard in a NEW `apply_committed_entry()` case can NEVER be caught by a test that calls `apply_committed_entry()` directly and in isolation. `persist_apply_pump_run()` holds `apply_pump_lock` across its ENTIRE per-LSN dispatch loop (including the call into `apply_committed_entry`); a naive `persist_checkpoint()` call from inside a new case would self-deadlock on that same non-recursive mutex, but only if the test actually drives the call through `persist_apply_pump_run()`/`persist_apply_pump_kick()` — a bare direct call never holds the lock, so it can't catch the deadlock. Same for a stale-`last_applied_lsn` hazard: `persist_apply_pump_run`/`replay_wal` both advance `last_applied_lsn` STRICTLY AFTER `apply_committed_entry` returns, never before — so a test needs a leading unrelated WAL entry before the entry under test to make `last_applied_lsn` genuinely differ from the tested entry's own lsn at dispatch time, or a "reads `last_applied_lsn` instead of the `lsn` parameter" bug is invisible. **General rule for any future `apply_committed_entry` case whose body triggers a SEPARATE persist_t operation that also touches `apply_pump_lock` or LSN-position state**: the test file MUST drive it through the real `persist_apply_pump_run`/`persist_apply_pump_kick` production call site (disk-backed persist_t + cluster_t fixture, mirrors `test_replay_apply_seam.c`/`test_apply_pump.c`, NOT `test_apply_committed_entry.c`'s bare in-memory fixture) with at least one leading entry, not just a bare direct call.
|
||||
|
||||
## Do-Not-Repeat (2026-07-24, config-upgrade-snapshot task 5) — in a Unity test file that does real `persist_start()`/`persist_init()` (spins up a live `fsync_worker` background thread holding a `persist_t*` pointer into the test function's own stack frame), NEVER put the fixture's teardown call at the END of the test function after a run of `TEST_ASSERT_*` calls. A failed `TEST_ASSERT_*` does a `longjmp` straight back to `RunTest`, skipping every line after it — including the teardown call — which leaves the background thread alive holding a dangling pointer into a stack frame the harness is about to reuse for the NEXT test in the same binary. This manifests as an ASAN `stack-use-after-return` crash inside `flusher_fsync_forced_fail`/`fsync_worker`, attributed to a LATER, unrelated test's frame, making it look like that later test is broken when the REAL bug is an earlier test's missing unconditional teardown. This exact pathology is already documented in `test_apply_pump.c`'s own file header ("longjmp-past-teardown pathology from task 3.1's round-2 lesson") — re-discovered the hard way while drafting `test_upgrade_snapshot_apply.c`'s RED-phase run. **Required pattern**: gather every assertion-relevant observable into a plain local variable FIRST (no `TEST_ASSERT` between "the SUT ran" and "teardown runs"), call teardown UNCONDITIONALLY, THEN run every `TEST_ASSERT` against the captured locals.
|
||||
|
||||
## Key Learning (2026-07-24, config-upgrade-snapshot task 5, bug-1466) — `persist_write_checkpoint_marker()` (called by ANY checkpoint, periodic OR the new task-5 marker-triggered one) appends its local-only, never-replicated `WAL_CHECKPOINT` entry by incrementing `persist_t.current_lsn` DIRECTLY, bypassing `ha_primary_log_append()`/`raft_log_append()` entirely. This silently violates `ha_primary_log_append`'s own documented co-tracking invariant (`ha.c:4057`, "p->current_lsn == s->log_len + 1 always holds") — the raft LIBRARY's `s->log_len` (a separate in-memory counter, `lib/raft/raft.c:574/585`) does NOT advance for a checkpoint marker, so it under-counts the true physical WAL tip from that point on. Consequence: `config_append_marker`'s/`ha_primary_log_append`'s RETURNED lsn (sourced from the library's `idx = s->log_len + 1`) becomes UNRELIABLE as an exact physical-position identifier for any entry appended after an earlier checkpoint has interleaved — this is a PRE-EXISTING gap (the periodic WAL-threshold checkpoint trigger has always had it) that task 5's marker-triggered checkpoint turns from "rare, needs >=100 WAL records to fire" into "the common case for any second config marker committed after an earlier one." **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 `s->log_len` (which the code's own comment calls "advisory only" for commit-ceiling purposes, ha-term-safety commit-ceiling-optA) — and `persist_wal_async` always self-assigns from `current_lsn`, ignoring the library's `index` parameter for a primary's own append (`ha_cb_persist_entry_stub`), so there is no on-disk LSN collision either. The blast radius is confined to any caller trusting the RETURNED lsn for precise identity/ordering once a checkpoint has occurred earlier in the session. **Do NOT attempt to fix the deeper `s->log_len` resync inside a `persist.c`-scoped task** — it requires touching `ha.c`/`lib/raft/raft.c` (owned by `ha-term-safety`/`raft-log-replication`), which is a bigger, deliberate architectural decision (e.g. adding a `raft_advance_log_len`-style API), not a same-task patch. Test-side fix that IS correct and in-scope: read the entry's TRUE physical lsn directly off `persist_t.current_lsn - 1` right after appending, instead of trusting the raft-library-derived return value — exactly 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. Full finding in `.wolf/buglog.json` bug-1466.
|
||||
|
||||
## Key Learning (2026-07-25, config-upgrade-snapshot task 8.6, bug-1469) — task 8.6's live `--config-upgrade` soak EMPIRICALLY REPRODUCED bug-857 on its first real run, via a THIRD distinct mechanism neither the original (disproven lost_and_found) hypothesis nor task 8.5's unit-level fix (`schedule_checkpoint` WAL-threshold-vs-marker-boundary race) covered: `do_upgrade_config_with_bytes` (config_repl.c, the handler for the node that ORIGINATES/commits an upgrade) saves new config bytes to disk and bumps `config_generation`, but never calls `config_activate_committed()`/`load_primary_config()` to re-parse those bytes INTO `c->myconf` — so the originating primary's own in-memory queue table (and whatever the checkpoint writer walks to emit the queue-entity list) stays stale even though its Activation Fence (`upgrade_materialization_verify`, ha.c) reports "VERIFIED" (that function only hash-checks on-disk bytes + a checkpoint's own cfgref stamp, never touches `c->myconf`). `config_activate_committed` is only invoked from `handle_repl_config_push` (a STANDBY's push-receipt path) or a member's own boot/recovery — confirmed via ctrl-1's log: ZERO `config_activate_committed` calls in its original (primary) process instance, TWO in its restarted instance. If the originating primary is killed before some LATER independent event triggers that call (exactly what the chaotic-kill-interleaved-with-upgrade scenario does by design), its last checkpoint permanently freezes a self-contradictory state: correct new config bytes, stale old queue-entity set. **Do-not-repeat**: when investigating ANY "config activation" or "materialization" claim in this codebase, do not trust an Activation-Fence/"VERIFIED" log line as proof that `c->myconf`'s in-memory parsed structures were refreshed — trace whether `config_activate_committed`/`load_primary_config` was ACTUALLY called on that specific code path (grep the live log for the function name itself), since the fence's hash-check and a checkpoint's cfgref stamp can both be satisfied by disk-bytes-only activity. Full finding in `.wolf/buglog.json` bug-1469; NOT fixed (out of task 8.6's scope — flagged for tasks 1-7's owner). Task 8.6's own harness (dispatch wiring, `run_config_upgrade_round`, SKILL.md docs) is otherwise correct and not implicated — confirmed the on-disk-checkpoint-comparison methodology is the design-intended one and waiting longer would not have self-corrected (no further WAL activity to trigger a follow-up checkpoint).
|
||||
|
||||
## Key Learning (2026-07-25, config-upgrade-snapshot task 8.6) — a live HA soak against a NEW client RPC can surface bugs the RPC's own author never had a way to trigger; two distinct, real materialization bugs found this way, only one safely fixable in-task
|
||||
Building task 8.6's `--config-upgrade` live soak required first discovering `OP_upgradeConfig` had NO working client anywhere (`xce_upgradeconfig_req_raw`/`xce_upgradeconfig`/`xcecli_upgradeconfig` built fresh in request.c/xceapi.c/client.c) AND that `cluster.c`'s dispatch gate for it was inverted (rejected primary, accepted standby — backwards; commit `fd93997f`, 2026-05-31, flipped it from a since-superseded primary-push design; already independently diagnosed as a blocking risk by the sibling `admin-config-tool` spec's design.md and explicitly left for whoever needed live `OP_upgradeConfig` first — that was this task). Fixing the dispatch gate (bug-1468) was a small, obviously-correct routing fix, in scope. Once the RPC worked, the FIRST real live run surfaced bug-1469: `apply_committed_entry`'s `WAL_CONFIG_MARKER` case called `config_activate_committed()` (which refreshes `c->myconf`) AFTER the marker-triggered checkpoint write, not before — so the checkpoint captured stale config. Reordering fixed HALF of it (verified: full 1025-test suite clean, checkpoint-content correctness improved). But re-running the SAME live soak after that fix showed the IDENTICAL divergence signature on a different node — root cause turned out DEEPER: `config_activate_committed`→`load_primary_config` only re-parses bytes into `myconf->cluster_config`, it NEVER touches `c->qmgr` (the runtime queue table `collect_queue_frames`/the checkpoint writer actually walks). The ONLY code that ever populates `c->qmgr` from config is `build_queues()` (queue.c), called EXACTLY ONCE per process lifetime at boot (`xce_config_init` hook), and it's unconditional/blind-create — unsafe to just re-call live (would attempt to recreate already-existing queues). This means NO live (non-restart) mechanism exists anywhere in this codebase to add a config-upgrade's NEW queue to an already-running member's runtime state — directly contradicting this whole spec's stated goal ("no controller restart required for a configuration upgrade"). This is genuinely deep, tasks-1-7 territory (needs a new create-if-missing sync function mirroring `restore_host_state`'s established pattern, PLUS an undecided queue-REMOVAL semantics design question) — NOT something to improvise under a test-infra task's time budget. **Resolution used**: changed the soak's own upgrade-target config diff from "add a queue" to "update an existing queue's description" — a legitimate, still fully meaningful config-identity test (Requirements 4.2/4.3/6.1/6.4 don't require adding an entity) that sidesteps the unrelated, unfixed mechanism entirely, rather than either (a) faking a pass by weakening the assertion, or (b) rushing an unreviewed fix into live queue-provisioning code. A third, much smaller (1-byte) checkpoint non-determinism remained even after that (bug-1471, host-frame encoding, root cause not yet isolated) — logged for follow-up rather than guessed at. **General lesson**: when a new regression-test/soak harness for feature X finds a bug in X's own materialization logic, first ask whether the SAME class of gap could apply more broadly than the one field your test happened to touch (here: ANY new queue OR host from ANY live upgrade, not just this soak's specific queue) before writing a fix — and if the honest answer requires new production logic + an undecided design question, stop and document rather than patch under the wrong task's time pressure.
|
||||
|
||||
## Do-Not-Repeat (2026-07-25, config-upgrade-snapshot task 8.6) — a long-running/compacted session can leave substantial prior work on disk with no visible conversational memory of it; always `git status`/`git diff --stat` early when picking up a "continue"
|
||||
Mid-task, discovered via `git status` that `persist.c`/`test_config_repl.c` were already modified with a detailed, real bug fix (bug-1469) I had no memory of writing — from an earlier, compacted portion of the same long-running session (confirmed via claude-mem observation hooks showing timestamps hours earlier than the visible transcript, and via the fix's own content matching exactly what I'd have derived independently). Nearly redid Part A-D from scratch before noticing Part E (the whole soak script + SKILL.md docs) was ALREADY fully written on disk too. Lesson: when resuming any task (especially on a bare "continue"), run `git status --short` + `git diff --stat` against the expected file set FIRST, before writing anything — don't trust a feeling of "starting fresh" just because the visible conversation transcript looks short. The claude-mem observation timestamps (visible in PreToolUse:Read hook context) are a reliable secondary signal for how much wall-clock work actually preceded the current visible slice.
|
||||
|
||||
## Key Learning (2026-07-25, config-upgrade-snapshot task 8.6, bug-1471/bug-1472) — a "1-byte checkpoint divergence near a padding-sized gap" is not proof of padding; decode the actual field before guessing
|
||||
bug-1471's two live hypotheses (uninitialized capnp padding vs. genuine content discrepancy) were distinguished definitively by (1) writing a throwaway byte-precise capnp decoder that reuses the SAME wire shape the real `write_HostAdminState`/`encode_HostAdminState` generated code produces (1 data word holding `status` via `capn_write32` at offset 0 + 4 bytes of trailing padding, 1 pointer word for `name` Text) rather than eyeballing `xxd` output, and (2) directly disassembling the pinned c-capnproto runtime (`/nix/store/*-capnpc-1.0.1/lib/libCapnC_Runtime.so`, since no `.c` source is shipped in the nix store — only the built `.so` — `objdump -d` on `capn_init_malloc`'s `create_local` callback) to confirm its segment allocator calls `calloc(1, size)`, not `malloc`, so it IS zero-initialized. Both lines of evidence agreed: the differing byte (file offset 42879) was byte index 2 of the 4-byte `status` field itself (0x00500000 = `XCE_WORKER_CLOSED|XCE_WORKER_CLOSED_BY_ADMIN` on ctrl-1 vs 0x0 on ctrl-2/ctrl-3), NOT the always-zero padding word right after it — a genuine content bug, not noise. Root cause: `worker_from_capnp` (worker.c:827-856, introduced 2026-05-08, ~2.5 months before this spec) is called for EVERY `WAL_WORKER_CTRL` apply regardless of JOIN/UPDATE/DEAD detail, and its idempotency guard returns `-EEXIST` (silently discarding the decoded fields, including `status`) whenever the named worker already exists — correct for JOIN replay, WRONG for UPDATE/DEAD (where "already exists" is the NORMAL case). This means ANY `hostctrl -c`/`hostctrl -o` against an already-joined host never propagates its admin-status change to standbys, unconditionally, cluster-wide — a real, general, pre-existing bug entirely outside config-upgrade-snapshot's boundary (filed as bug-1472, NOT patched here — scope discipline). **Do-not-repeat**: when a buglog entry frames two hypotheses as "padding vs. content" for a capnp field, always decode the field itself using the codebase's own generated wire-layout constants (`X_word_count`/`X_pointer_count`) before assuming either — "looks like it's near a text/entity boundary" is not evidence of which one it is.
|
||||
|
||||
## Key Learning (2026-07-25, config-upgrade-snapshot task 8.9, bug-1474) — a live soak that FAILS is not automatically a test-script bug; forensically distinguish "my assumption was wrong" from "production genuinely did the wrong thing" via direct log evidence, byte-for-byte, before touching either
|
||||
Building `run_config_upgrade_wal_trim_round` (`--config-upgrade-wal-trim`) to prove bug-1147's WAL-trim retention guard holds under the marker-triggered path required a "baseline capture" step (record the primary's currently-active WAL segment as a genuinely-caught-up standby's floor, right before killing it) — a reasonable design, but its correctness is NOT self-evident from "the round ran without erroring." When the round's own final assertion failed (floor segment trimmed away, only the active segment survived) on BOTH of 2 independent live runs, the instinct to weaken/second-guess the test's own logic was strong — instead, cross-referenced `ctrl-2.log`'s (the primary's) own `commit: ack standby_idx=N` lines, `on_standby_connected`/`repl registered late-joiner standby` lines, and the exact wall-clock kill timestamp (`ctrl-1.log`'s own last line) to reconstruct, byte-for-byte, WHAT the replicator's slot state actually was at trim time. This revealed a REAL, 2-of-2-reproducible production defect: the primary's heartbeat task's "reconnect to any peer missing from standby_endpoints" sweep spuriously treats an ALREADY-connected peer (one connected only via a promoted-vote-channel, not yet a heartbeat-task connection) as missing, and the resulting duplicate "late-joiner" reconnect's `repl_add_standby` call picks the LOWEST-INDEXED free replicator slot with no regard for whether that slot still holds an UNRELATED, unexpired bug-1147 disconnect guard — silently clobbering it via `memset`. Filed as bug-1474 (NOT fixed — out of task 8.9's scope, mirrors the bug-1471/1472 precedent). **Do-not-repeat**: when a live-soak assertion fails, do NOT assume the test's own timing/logic is at fault just because the mechanism is complex — grep the ACTUAL controller logs for the specific slot/connection/ack events around the failure window first; a "the guard should have protected X but didn't" failure is exactly the shape a genuine upstream defect takes, and 2 independent runs showing the IDENTICAL mechanism is strong evidence it's real, not a timing fluke.
|
||||
|
||||
## Key Learning (2026-07-26) — bug-1474 fixed: repl_add_standby's first-empty-slot scan must never clobber an unexpired bug-1147 disconnect guard
|
||||
Prior entry (2026-07-25) noted `repl_remove_standby` correctly sets the `last_disconnected_lsn`/`last_disconnect_time` guard and `repl_expire_disconnect_slots` correctly clears it on expiry — but nothing protected the guard from PREMATURE clobbering by `repl_add_standby`'s slot-allocation scan, which picked the first `ep==NULL` slot by lowest index with zero awareness of whether that slot still held an unexpired guard for a different peer. This let an unrelated peer's `repl_add_standby` call (triggered in the wild by a separate ha.c duplicate-reconnect defect, but the vulnerability exists regardless of the trigger) unconditionally `memset` away a guard `repl_trim_lsn` needed, letting `trim_wal_segments` delete WAL segments a genuinely disconnected-but-reconnecting standby still needed. **Fix pattern**: two-pass slot allocation — Pass 1 prefers a genuinely virgin slot (`ep==NULL && last_disconnected_lsn==0`); Pass 2 (only if no virgin slot exists) falls back to the guard-holding slot with the OLDEST `last_disconnect_time` (closest to its own natural `repl_expire_disconnect_slots` expiry, so reusing it discards the least remaining protection) rather than refusing the connect (`-ENOSPC` would be worse for cluster availability than an early guard loss in an already-degraded multi-disconnect scenario). **Do-Not-Repeat**: when a struct slot has BOTH a "live" state (`ep != NULL`) and a "recently-freed-but-still-meaningful" state (`ep==NULL && guard_field != 0`), a first-empty-slot allocator that only checks the live/dead boundary will silently destroy the recently-freed state's data — always give the allocator a way to distinguish "truly free" from "free but still guarding something," and pick a deliberate, explained tie-break policy (here: oldest-guard-first) for the exhausted-fallback case rather than an accidental index-order fallthrough. TDD: 2 new tests in `test_repl_unit.c` (`test_repl_add_standby_prefers_virgin_slot_over_guard`, `test_repl_add_standby_exhausted_guards_prefers_oldest_disconnect`), RED confirmed against the pre-fix scan, GREEN at 1030/1030 (0 failures, no regressions).
|
||||
|
||||
## Key Learning (2026-07-25, bug-1472 fix) — the `Worker` capnp struct's `resvals` field maps to `resvals_to_sched`/`nresvals_to_sched`, NOT to `xce_worker_t.host.resvals` (the live per-host resource-value `zarray_t` that scheduling actually reads and that `set_host_slots()`/`update_resvals()` mutate). Confirmed by reading `src/schema/controller.capnp`'s `struct Worker` (`resvals @5: List(NameValue) $C.mapname("resvals_to_sched") $C.maplistcount("nresvals_to_sched")` — no field maps to `host.resvals` at all) and the generated `decode_Worker`/`encode_Worker` in `controller.capnp.c`. Consequence: the WAL_WORKER_CTRL wire format (`persist_worker_hook_fun` → `capnp_encode_worker(&w->host, ...)`, same single encode call site for JOIN/UPDATE/DEAD) can NEVER replicate a `mark_worker_unavail()`-style resource-slot zeroing to another member, no matter how the apply side is fixed — this is a genuine schema gap, not an apply-logic bug. Any future work on `HOOK_DETAIL_WORKER_DEAD` replication (see bug-1472-remainder in buglog.json) needs a schema change (a new wire-encodable resource-value-slots field) or a non-replication-based strategy, not just an apply-side fix mirroring bug-1472's UPDATE fix. **Also confirmed**: `encode_Worker`/`decode_Worker` DO faithfully round-trip `status`, `dead_since`, and `attrs` (`decode_HostAttrs_ptr` allocates and copies) for all three hook_detail values, since the encode call always captures the worker's CURRENT (post-mutation) `w->host` state uniformly — so a DEAD-case fix is NOT blocked by field-fidelity for those three, only by the missing `resvals` wire mapping.
|
||||
|
||||
## Key Learning (2026-07-25, bug-1472 fix) — `worker_from_capnp`'s pre-existing fresh-restore path (`restore_worker(c, host)` on a MISS) leaks the decoded `xce_worker_t`'s `name`, `attrs` (+ its `hosttype`/`hostmodel` strings), and `resvals_to_sched` list on every call — NOT introduced by this fix, already present before it, out of scope to fix here, but worth knowing before touching this code again. Root cause: `copy_worker` (`src/lib/wobj.c:51-81`, called by `makeWorker`/`makeWorker_`/`override_worker`) has THREE different ownership behaviors for the SAME `xce_worker_t*` argument depending on the field: `name` and `attrs->hosttype`/`attrs->hostmodel` are re-created via `makeString()` (a COPY — caller's originals are untouched and must be freed by the caller); `dispatch_window_config` and `thresholds` are transferred via `MOVE_PTR` (caller must NOT free these — ownership passes to the new worker); `resvals_to_sched`/`nresvals_to_sched` are DROPPED entirely (`dst->host.nresvals_to_sched=0; dst->host.resvals_to_sched=NULL` — neither copied nor moved, silently discarded). None of `worker_from_capnp`'s existing callers (`restore_worker`, and this fix's own -EEXIST/JOIN branch) call `free_Worker(&host)` after `makeWorker`/`copy_worker` returns, so `name`/`attrs`/`resvals_to_sched` leak every time. This has never been caught because this repo's controller test config runs with `ASAN_OPTIONS=detect_leaks=0` (required separately for an unrelated reason — see bug-918/controller_unit_test hang note). **Do-not-repeat**: if you ever decode an `xce_worker_t` via `decode_Worker` and DON'T hand it to `makeWorker`/`copy_worker`/`restore_worker` (e.g. bug-1472's own new UPDATE branch, which only reads `host.status`), you MUST call `free_Worker(&host)` yourself — copy_worker's mixed copy/move/drop semantics mean there is no single safe default, and the existing call sites' silence is leaked technical debt, not a pattern to copy.
|
||||
|
||||
## Key Learning (2026-07-25) — bug-1472 FIXED: `worker_from_capnp` now decodes `hook_detail` and applies UPDATE in place to an existing worker
|
||||
`worker_from_capnp` (`src/controller/worker.c`) previously returned `-EEXIST` unconditionally whenever the decoded worker's name was already registered, silently discarding the decoded fields regardless of whether the WAL entry was a JOIN, UPDATE, or DEAD. Fixed by reading the WAL_WORKER_CTRL payload's leading `hook_detail` discriminator (already decoded by the caller at `persist.c:3728-3729` but never passed down or re-read by `worker_from_capnp` itself) and branching: `HOOK_DETAIL_WORKER_JOIN` keeps the original -EEXIST no-op (regression-guarded); `HOOK_DETAIL_WORKER_UPDATE` now REPLACES (bitwise, not OR — see worker_ctrl's assignment-style `&= ~(...)`/`|= (...)` semantics, an OR-based fix would make HOST_OPEN unable to ever clear the closed bits again) `XCE_WORKER_ADMIN_MASK` bits on the existing worker under its own write lock, then returns 0 so `persist.c`'s existing generic "0 ⇒ fire the hook_detail-keyed notification" fallthrough (persist.c:3737-3785, unmodified) fires the correct `HOOK_DETAIL_WORKER_UPDATE` hook; `HOOK_DETAIL_WORKER_DEAD` is deliberately left as -EEXIST/no-op (see the two Key Learning entries above for why — filed as bug-1472-remainder, not silently ignored). `persist.c` needed NO changes — confirmed its WAL_WORKER_CTRL apply case already treats any non-EEXIST, non-negative `worker_from_capnp` return generically. TDD: RED confirmed by temporarily reverting just `worker.c` (via `git stash push -- <2 files>` / `git apply` round-trip, keeping the new tests) and re-running — exactly the 2 new UPDATE tests failed, the JOIN-duplicate regression-guard test already passed (proving it exercises the untouched path). GREEN: full suite 1030→1033 tests (this worktree's immediate pre-fix baseline, not a stale prior-session number), 0 failures, net +3 = exactly the 3 new tests (`test_ace_worker_ctrl_update_applies_to_existing_worker`, `test_ace_worker_ctrl_update_replaces_not_ors_admin_bits`, `test_ace_worker_ctrl_join_duplicate_existing_is_noop`), zero regressions.
|
||||
|
||||
## Do-Not-Repeat (2026-07-28, bug-1476) — a buglog "FIXED... GREEN" claim is not proof; the working tree can be mid-TDD-cycle at the exact moment you resume
|
||||
Resumed `/kiro-impl config-upgrade-snapshot` with buglog.json's bug-1476 entry claiming "FIXED (2026-07-28)... GREEN: full controller_unit_test suite" — but the actual uncommitted diff on disk had a comment (config_repl.c:639-646) explicitly admitting `load_primary_config()` had been reverted to its RED-phase (no lock, decode-directly-into-shared-field) state for test construction and never restored. Running the full suite fresh immediately surfaced the real state: `test_bug1476_concurrent_load_primary_config_no_uaf_or_double_free` FAILED. The buglog note was written mid-cycle, describing the INTENDED fix, not the code actually sitting on disk. Fixed for real (lock now wraps the whole function body; decode targets a local temp, swapped in only after full validation), verified 4/4 clean full-suite runs before committing. **Do-not-repeat**: before committing or building on top of ANY buglog "FIXED"/GREEN claim for uncommitted work, re-run the actual test suite against the actual working tree first — never trust the log entry's narrative over a fresh run, especially when resuming a session that may have been interrupted mid-TDD-cycle.
|
||||
|
|
|
|||
|
|
@ -611,10 +611,38 @@ static int load_cluster_config_from_file(char *file, cluster_config_t **config)
|
|||
|
||||
/* ── load_primary_config ────────────────────────── */
|
||||
|
||||
#ifdef XCE_TESTING
|
||||
/* load_primary_config_race_hook: test-only synchronization seam (same
|
||||
* pattern as push_thread_send_hook / xce_wal_stream_request_hook). When
|
||||
* non-NULL, load_primary_config() calls this immediately after capturing the
|
||||
* OLD myconf->cluster_config pointer this invocation is about to retire
|
||||
* (free), right before it actually retires it -- letting a unit test
|
||||
* deterministically force a SECOND, real, concurrent load_primary_config
|
||||
* call through this exact window instead of depending on scheduler timing.
|
||||
* NULL in production -- no behaviour change. See
|
||||
* test_bug1476_concurrent_load_primary_config_no_uaf_or_double_free
|
||||
* (test_config_repl.c) for the RED-phase reproduction this was built for:
|
||||
* with the lock below removed, this hook deterministically reproduces the
|
||||
* live ASan heap-use-after-free bug-1476 caught (two concurrent
|
||||
* load_primary_config calls racing the free of the SAME myconf->
|
||||
* cluster_config pointer). With the lock, a second invocation can never
|
||||
* even reach this hook while the first is parked here -- it blocks
|
||||
* acquiring myconf->lock before getting this far, which is the
|
||||
* serialization proof the fix requires. */
|
||||
void (*load_primary_config_race_hook)(void) = NULL;
|
||||
#endif
|
||||
|
||||
int load_primary_config(cluster_t *c)
|
||||
{
|
||||
myconf_t *myconf = c->myconf;
|
||||
|
||||
/* bug-1476: serialize the whole free-decode-publish sequence against a
|
||||
* concurrent load_primary_config() call for the SAME myconf_t. Held
|
||||
* across every return path below, including the success path — a
|
||||
* second caller must block here before it can even reach the race hook
|
||||
* fired near the retire step, not merely around that step alone. */
|
||||
mylock_lock(&myconf->lock);
|
||||
|
||||
/* Build conf dir path: <workdir>/<name> */
|
||||
char conf_dir[4096];
|
||||
snprintf(conf_dir, sizeof(conf_dir), "%s/%s",
|
||||
|
|
@ -627,14 +655,17 @@ int load_primary_config(cluster_t *c)
|
|||
INFO("No config at '%s/conf/current' (rc=%d) — "
|
||||
"will fetch from primary after election",
|
||||
conf_dir, rc);
|
||||
mylock_unlock(&myconf->lock);
|
||||
return -ENOENT;
|
||||
}
|
||||
|
||||
/* Heap-allocate conffile so it survives past this stack frame */
|
||||
free(myconf->conffile);
|
||||
myconf->conffile = NULL;
|
||||
if (asprintf(&myconf->conffile, "%s/conf/%s", conf_dir, fname) < 0)
|
||||
if (asprintf(&myconf->conffile, "%s/conf/%s", conf_dir, fname) < 0) {
|
||||
mylock_unlock(&myconf->lock);
|
||||
return -ENOMEM;
|
||||
}
|
||||
|
||||
/* Read raw bytes and check size */
|
||||
uint8_t *bytes = NULL;
|
||||
|
|
@ -644,6 +675,7 @@ int load_primary_config(cluster_t *c)
|
|||
ERROR("Primary config '%s' unreadable: %d", myconf->conffile, rc);
|
||||
free(myconf->conffile);
|
||||
myconf->conffile = NULL;
|
||||
mylock_unlock(&myconf->lock);
|
||||
return rc;
|
||||
}
|
||||
|
||||
|
|
@ -653,53 +685,61 @@ int load_primary_config(cluster_t *c)
|
|||
memcpy(&tag, myconf->config_sha256, 8);
|
||||
myconf->config_version_tag = be64toh(tag);
|
||||
|
||||
/* Free any prior decoded config before overwriting the pointer */
|
||||
if (myconf->cluster_config) {
|
||||
free_ClusterConfig(myconf->cluster_config);
|
||||
free(myconf->cluster_config);
|
||||
myconf->cluster_config = NULL;
|
||||
}
|
||||
|
||||
/* Decode Cap'n Proto directly into myconf->cluster_config */
|
||||
rc = load_cluster_config_from_file(myconf->conffile,
|
||||
&myconf->cluster_config);
|
||||
/* Decode Cap'n Proto into a LOCAL temporary first (bug-1476 fix part 2):
|
||||
* decode_ClusterConfig_ptr calloc's and publishes its out-parameter
|
||||
* pointer BEFORE decode_ClusterConfig populates the nested fields, so
|
||||
* decoding directly into the shared myconf->cluster_config field would
|
||||
* expose any other reader of that field to a live-but-incomplete
|
||||
* object. Only a fully decoded and validated config is ever published
|
||||
* below. */
|
||||
cluster_config_t *new_config = NULL;
|
||||
rc = load_cluster_config_from_file(myconf->conffile, &new_config);
|
||||
if (rc != 0) {
|
||||
ERROR("Primary config '%s' failed Cap'n Proto decode: %d",
|
||||
myconf->conffile, rc);
|
||||
free(bytes);
|
||||
free(myconf->conffile);
|
||||
myconf->conffile = NULL;
|
||||
mylock_unlock(&myconf->lock);
|
||||
return rc;
|
||||
}
|
||||
|
||||
if (myconf->cluster_config == NULL) {
|
||||
if (new_config == NULL) {
|
||||
ERROR("Primary config '%s' decoded to NULL", myconf->conffile);
|
||||
free(bytes);
|
||||
free(myconf->conffile);
|
||||
myconf->conffile = NULL;
|
||||
mylock_unlock(&myconf->lock);
|
||||
return -EINVAL;
|
||||
}
|
||||
|
||||
free(bytes);
|
||||
|
||||
fill_default_cluster_config(myconf->cluster_config);
|
||||
fill_default_cluster_config(new_config);
|
||||
|
||||
/* Check for reserved lost_and_found name */
|
||||
if (config_contains_reserved_name(myconf->cluster_config)) {
|
||||
if (config_contains_reserved_name(new_config)) {
|
||||
ERROR("Config '%s' contains reserved name 'lost_and_found'",
|
||||
myconf->conffile);
|
||||
free_ClusterConfig(myconf->cluster_config);
|
||||
free(myconf->cluster_config);
|
||||
myconf->cluster_config = NULL;
|
||||
free(myconf->conffile);
|
||||
myconf->conffile = NULL;
|
||||
free_ClusterConfig(new_config);
|
||||
free(new_config);
|
||||
mylock_unlock(&myconf->lock);
|
||||
return -EINVAL;
|
||||
}
|
||||
|
||||
/* Default generation to 1 if not set */
|
||||
if (myconf->cluster_config->config_generation == 0)
|
||||
myconf->cluster_config->config_generation = 1;
|
||||
myconf->config_generation = myconf->cluster_config->config_generation;
|
||||
if (new_config->config_generation == 0)
|
||||
new_config->config_generation = 1;
|
||||
myconf->config_generation = new_config->config_generation;
|
||||
|
||||
/* Publish: retire the prior config only now that the new one is fully
|
||||
* decoded and validated — myconf->cluster_config never points at a
|
||||
* freed or partially-built object. */
|
||||
if (myconf->cluster_config) {
|
||||
#ifdef XCE_TESTING
|
||||
if (load_primary_config_race_hook)
|
||||
load_primary_config_race_hook();
|
||||
#endif
|
||||
free_ClusterConfig(myconf->cluster_config);
|
||||
free(myconf->cluster_config);
|
||||
}
|
||||
myconf->cluster_config = new_config;
|
||||
|
||||
/* Check staleness */
|
||||
myconf->conf_stale = conf_check_stale(conf_dir, myconf->conf_stale_timeout);
|
||||
|
|
@ -709,6 +749,7 @@ int load_primary_config(cluster_t *c)
|
|||
|
||||
INFO("Primary config loaded: tag=%016" PRIx64 " gen=%" PRIu64,
|
||||
myconf->config_version_tag, myconf->config_generation);
|
||||
mylock_unlock(&myconf->lock);
|
||||
return 0;
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -453,6 +453,16 @@ extern int (*xce_probe_config_tag_hook)(const char *host, uint16_t port,
|
|||
*/
|
||||
int load_primary_config_for_test(cluster_t *c);
|
||||
|
||||
/*
|
||||
* load_primary_config_race_hook: test-only synchronization seam. See the
|
||||
* doc comment at its definition (config_repl.c) for the exact semantics —
|
||||
* it lets a unit test deterministically force two concurrent
|
||||
* load_primary_config() calls for the SAME myconf_t through the race window
|
||||
* bug-1476's live ASan heap-use-after-free crossed. Set by test
|
||||
* infrastructure (test_config_repl.c) before the relevant tests run.
|
||||
*/
|
||||
extern void (*load_primary_config_race_hook)(void);
|
||||
|
||||
/*
|
||||
* push_ctx_t: exposed under XCE_TESTING so unit tests can construct
|
||||
* synthetic push contexts to drive wait_all_with_timeout_for_test().
|
||||
|
|
|
|||
|
|
@ -400,6 +400,14 @@ int main(int argc, char* argv[])
|
|||
myconf.xstrings_delta = arguments.xstrings_delta;
|
||||
memset(&myconf.peers_cfg, 0, sizeof(myconf.peers_cfg));
|
||||
|
||||
/* bug-1476: myconf.lock serializes load_primary_config() against
|
||||
* concurrent invocations for this myconf_t — must be initialized before
|
||||
* cluster_run/myapp_run can ever call it. */
|
||||
if (mylock_init(&myconf.lock) != 0) {
|
||||
ERROR("Failed to initialize myconf lock");
|
||||
return -1;
|
||||
}
|
||||
|
||||
if (arguments.peers_str) {
|
||||
if (xce_parse_peers(arguments.peers_str, &myconf.peers_cfg) != 0) {
|
||||
fprintf(stderr,
|
||||
|
|
|
|||
|
|
@ -163,4 +163,5 @@ void myconf_fini(myconf_t *myconf)
|
|||
free(myconf->cluster_config);
|
||||
myconf->cluster_config = NULL;
|
||||
}
|
||||
mylock_fini(&myconf->lock); /* bug-1476 */
|
||||
}
|
||||
|
|
|
|||
|
|
@ -1047,6 +1047,9 @@ static myconf_t *t43_attach_myconf(cluster_t *c, const char *tmpdir)
|
|||
mc->name = strdup("ctrl-test");
|
||||
mc->conf_keep_n = 2;
|
||||
mc->conffile = NULL;
|
||||
/* bug-1476: the real load_primary_config()/config_activate_committed()
|
||||
* paths this fixture drives now take mc->lock -- must be initialized. */
|
||||
TEST_ASSERT_EQUAL_INT(0, mylock_init(&mc->lock));
|
||||
c->myconf = mc;
|
||||
return mc;
|
||||
}
|
||||
|
|
@ -1062,6 +1065,7 @@ static void t43_free_myconf(cluster_t *c)
|
|||
free(mc->conffile);
|
||||
free(mc->workdir);
|
||||
free(mc->name);
|
||||
mylock_fini(&mc->lock);
|
||||
free(mc);
|
||||
c->myconf = NULL;
|
||||
}
|
||||
|
|
@ -6303,6 +6307,237 @@ static void test_catchup_inline_probe_blocks_RED(void)
|
|||
cc_run_offload_probe(0, "test_catchup_inline_probe_blocks_RED");
|
||||
}
|
||||
|
||||
/* ── bug-1476: concurrent load_primary_config() race on myconf->cluster_config ──
|
||||
*
|
||||
* load_primary_config()'s free-decode-publish sequence for
|
||||
* myconf->cluster_config was completely unguarded by any lock. A live
|
||||
* 3-node ASan soak (config-upgrade-snapshot task 8.9 re-verification,
|
||||
* ctrl-3) caught a genuine heap-use-after-free: two concurrent
|
||||
* handle_repl_config_push deliveries for the SAME standby raced two
|
||||
* concurrent load_primary_config() calls on separate VTASK_LIVE_BLOCK
|
||||
* workers -- one call's decode had already published a non-NULL,
|
||||
* still-populating cluster_config_t* into the shared myconf->cluster_config
|
||||
* field (decode_ClusterConfig_ptr calloc's *and publishes* the pointer via
|
||||
* its out-parameter BEFORE decode_ClusterConfig populates the nested fields
|
||||
* such as resources -- confirmed by reading
|
||||
* lib/controller.capnp.c:17067-17077) while a second, concurrent invocation
|
||||
* observed that same non-NULL pointer, assumed (per the single free-then-
|
||||
* decode sequence) it was a prior FULLY-decoded config, and freed it out
|
||||
* from under the first call's still-in-flight decode.
|
||||
*
|
||||
* load_primary_config_race_hook (XCE_TESTING-only, config_repl.c) lets this
|
||||
* test deterministically force two REAL pthreads through that exact race
|
||||
* window instead of depending on scheduler timing: thread A captures the
|
||||
* config pointer it is about to retire and parks at the hook; thread B (a
|
||||
* full, real, concurrent load_primary_config() call for the SAME myconf_t)
|
||||
* is released to run while A is parked.
|
||||
*
|
||||
* RED (pre-fix, no lock): B runs to completion unblocked -- freeing the
|
||||
* SAME pointer A already captured and publishing its own config. Opening
|
||||
* the gate lets A retire (free) that already-freed pointer a SECOND time: a
|
||||
* genuine, deterministic, ASan-catchable double-free from the identical
|
||||
* missing-lock root cause the live crash exposed. (The exact
|
||||
* decode_Resource_list-mid-write UAF the live trace shows additionally
|
||||
* requires pausing inside the generated capnp decoder, which the fix
|
||||
* deliberately avoids touching -- see the fix's own comment in
|
||||
* load_primary_config; this reproduction targets the same unguarded
|
||||
* read-free-decode-publish sequence and is caught by ASan the same way.)
|
||||
*
|
||||
* GREEN (post-fix): myconf->lock serializes the two calls end-to-end -- B
|
||||
* cannot even acquire the lock (let alone reach its own retire step or
|
||||
* corrupt anything) until A's whole call, hook included, returns and
|
||||
* releases it. No pointer is ever freed twice, and load_primary_config (fix
|
||||
* part 2) no longer decodes directly into the shared field at all, closing
|
||||
* the exposure for any OTHER reader that never takes the lock. */
|
||||
|
||||
typedef struct {
|
||||
pthread_mutex_t mu;
|
||||
pthread_cond_t release_cv;
|
||||
pthread_cond_t entered_cv;
|
||||
int gate_open;
|
||||
_Atomic int entered;
|
||||
} lpc_gate_t;
|
||||
|
||||
static lpc_gate_t g_lpc_gate;
|
||||
static _Atomic int g_lpc_hook_block_next; /* 1 = the NEXT hook call should park */
|
||||
|
||||
static void lpc_gate_init(lpc_gate_t *g)
|
||||
{
|
||||
pthread_mutex_init(&g->mu, NULL);
|
||||
pthread_cond_init(&g->release_cv, NULL);
|
||||
pthread_cond_init(&g->entered_cv, NULL);
|
||||
g->gate_open = 0;
|
||||
atomic_store(&g->entered, 0);
|
||||
}
|
||||
|
||||
static void lpc_gate_destroy(lpc_gate_t *g)
|
||||
{
|
||||
pthread_mutex_destroy(&g->mu);
|
||||
pthread_cond_destroy(&g->release_cv);
|
||||
pthread_cond_destroy(&g->entered_cv);
|
||||
}
|
||||
|
||||
static void lpc_gate_open(lpc_gate_t *g)
|
||||
{
|
||||
pthread_mutex_lock(&g->mu);
|
||||
g->gate_open = 1;
|
||||
pthread_cond_broadcast(&g->release_cv);
|
||||
pthread_mutex_unlock(&g->mu);
|
||||
}
|
||||
|
||||
static int lpc_gate_wait_entered(lpc_gate_t *g, int timeout_ms)
|
||||
{
|
||||
struct timespec deadline;
|
||||
clock_gettime(CLOCK_REALTIME, &deadline);
|
||||
deadline.tv_sec += timeout_ms / 1000;
|
||||
deadline.tv_nsec += (long)(timeout_ms % 1000) * 1000000L;
|
||||
if (deadline.tv_nsec >= 1000000000L) { deadline.tv_sec++; deadline.tv_nsec -= 1000000000L; }
|
||||
|
||||
pthread_mutex_lock(&g->mu);
|
||||
int rc = 0;
|
||||
while (atomic_load(&g->entered) < 1) {
|
||||
rc = pthread_cond_timedwait(&g->entered_cv, &g->mu, &deadline);
|
||||
if (rc != 0) break;
|
||||
}
|
||||
int entered = atomic_load(&g->entered);
|
||||
pthread_mutex_unlock(&g->mu);
|
||||
return entered >= 1 ? 0 : -1;
|
||||
}
|
||||
|
||||
/* Fires from INSIDE load_primary_config -- holding no lock of its own under
|
||||
* the OLD (pre-fix) code, or holding myconf->lock under the fixed code.
|
||||
* Only the FIRST call (armed via g_lpc_hook_block_next) actually parks;
|
||||
* later calls pass straight through, so thread B's own call runs unimpeded
|
||||
* when it reaches this same hook (pre-fix), or never even reaches it
|
||||
* (post-fix -- it blocks acquiring myconf->lock before getting this far). */
|
||||
static void lpc_race_hook(void)
|
||||
{
|
||||
if (!atomic_exchange(&g_lpc_hook_block_next, 0))
|
||||
return;
|
||||
|
||||
pthread_mutex_lock(&g_lpc_gate.mu);
|
||||
atomic_fetch_add(&g_lpc_gate.entered, 1);
|
||||
pthread_cond_broadcast(&g_lpc_gate.entered_cv);
|
||||
while (!g_lpc_gate.gate_open)
|
||||
pthread_cond_wait(&g_lpc_gate.release_cv, &g_lpc_gate.mu);
|
||||
pthread_mutex_unlock(&g_lpc_gate.mu);
|
||||
}
|
||||
|
||||
typedef struct {
|
||||
cluster_t *c;
|
||||
_Atomic int done;
|
||||
_Atomic int rc;
|
||||
} lpc_thread_arg_t;
|
||||
|
||||
static void *lpc_call_load_primary_config(void *arg)
|
||||
{
|
||||
lpc_thread_arg_t *a = (lpc_thread_arg_t *)arg;
|
||||
int rc = load_primary_config(a->c);
|
||||
atomic_store(&a->rc, rc);
|
||||
atomic_store(&a->done, 1);
|
||||
return NULL;
|
||||
}
|
||||
|
||||
static void test_bug1476_concurrent_load_primary_config_no_uaf_or_double_free(void)
|
||||
{
|
||||
char tmp[256];
|
||||
strcpy(tmp, "/tmp/xce_cr1476_race_XXXXXX");
|
||||
TEST_ASSERT_NOT_NULL(mkdtemp(tmp));
|
||||
|
||||
cluster_t c;
|
||||
memset(&c, 0, sizeof(c));
|
||||
myconf_t *mc = t43_attach_myconf(&c, tmp); /* inits mc->lock */
|
||||
|
||||
/* conf_save_config only mkdir()s the LAST path component (workdir/name/
|
||||
* conf); unlike other t43_* fixtures this test does not call
|
||||
* t42_make_cluster (which creates workdir/name via persist_init) since
|
||||
* load_primary_config only touches c->myconf -- create workdir/name
|
||||
* directly instead. */
|
||||
char ctrldir[300];
|
||||
snprintf(ctrldir, sizeof(ctrldir), "%s/ctrl-test", tmp);
|
||||
TEST_ASSERT_EQUAL_INT(0, mkdir(ctrldir, 0700));
|
||||
|
||||
/* Establish an initial live config so BOTH racing calls exercise the
|
||||
* "free the OLD config" retire step (not the first-ever-load path) --
|
||||
* matches the live crash, which happened on a standby reloading an
|
||||
* ALREADY-activated config, not on first boot. */
|
||||
size_t blen0 = 0;
|
||||
uint8_t *bytes0 = t43_make_config_blob(&blen0);
|
||||
uint8_t sha0[32];
|
||||
SHA256(bytes0, blen0, sha0);
|
||||
TEST_ASSERT_EQUAL_INT(0, conf_save_config(mc, bytes0, blen0, sha0));
|
||||
free(bytes0);
|
||||
TEST_ASSERT_EQUAL_INT(0, load_primary_config(&c));
|
||||
TEST_ASSERT_NOT_NULL(mc->cluster_config);
|
||||
|
||||
/* Both racing calls independently resolve + decode the SAME conf/current
|
||||
* file -- real production behaviour: two concurrent
|
||||
* handle_repl_config_push deliveries for the same standby race over the
|
||||
* SAME on-disk config, not different ones. */
|
||||
lpc_gate_init(&g_lpc_gate);
|
||||
atomic_store(&g_lpc_hook_block_next, 1);
|
||||
load_primary_config_race_hook = lpc_race_hook;
|
||||
|
||||
lpc_thread_arg_t arg_a = { .c = &c, .done = 0, .rc = 0 };
|
||||
lpc_thread_arg_t arg_b = { .c = &c, .done = 0, .rc = 0 };
|
||||
|
||||
pthread_t thread_a;
|
||||
TEST_ASSERT_EQUAL_INT(0,
|
||||
pthread_create(&thread_a, NULL, lpc_call_load_primary_config, &arg_a));
|
||||
|
||||
TEST_ASSERT_EQUAL_INT_MESSAGE(0, lpc_gate_wait_entered(&g_lpc_gate, 2000),
|
||||
"thread A must reach the race-hook checkpoint inside "
|
||||
"load_primary_config before the test proceeds");
|
||||
|
||||
pthread_t thread_b;
|
||||
TEST_ASSERT_EQUAL_INT(0,
|
||||
pthread_create(&thread_b, NULL, lpc_call_load_primary_config, &arg_b));
|
||||
|
||||
/* Snapshot whether B completed WHILE A was still parked -- the
|
||||
* serialization proof. Captured into a plain variable (not asserted
|
||||
* yet) so a pre-fix failure here does not abort the test before it
|
||||
* reaches the release below: pre-fix (RED) the crash triggered by that
|
||||
* release is the primary, load-bearing signal; this flag is
|
||||
* corroborating evidence, asserted only after both threads have safely
|
||||
* joined post-release. */
|
||||
int b_raced_through_while_a_parked = cc_poll_flag(&arg_b.done, 300);
|
||||
|
||||
lpc_gate_open(&g_lpc_gate);
|
||||
|
||||
TEST_ASSERT_TRUE_MESSAGE(cc_poll_flag(&arg_a.done, 2000),
|
||||
"thread A must complete once released");
|
||||
pthread_join(thread_a, NULL);
|
||||
|
||||
TEST_ASSERT_TRUE_MESSAGE(cc_poll_flag(&arg_b.done, 2000),
|
||||
"thread B must complete once unblocked by A's release");
|
||||
pthread_join(thread_b, NULL);
|
||||
|
||||
load_primary_config_race_hook = NULL;
|
||||
lpc_gate_destroy(&g_lpc_gate);
|
||||
|
||||
TEST_ASSERT_EQUAL_INT(0, atomic_load(&arg_a.rc));
|
||||
TEST_ASSERT_EQUAL_INT(0, atomic_load(&arg_b.rc));
|
||||
|
||||
/* No corruption: the final published config is fully valid (whichever
|
||||
* of the two calls happened to finish last -- both raced the SAME
|
||||
* conf/current file, so either outcome is a legitimate, fully-decoded
|
||||
* config, never a torn/freed one). */
|
||||
TEST_ASSERT_NOT_NULL(mc->cluster_config);
|
||||
TEST_ASSERT_NOT_NULL(mc->cluster_config->clustername);
|
||||
TEST_ASSERT_EQUAL_STRING("t43-upgrade", mc->cluster_config->clustername);
|
||||
|
||||
/* GREEN: myconf->lock must have kept B blocked the whole time A was
|
||||
* parked mid-call. RED (no lock) reliably fails this -- but reliably
|
||||
* crashes (ASan double-free abort) even earlier, at lpc_gate_open()
|
||||
* above, before ever reaching this line. */
|
||||
TEST_ASSERT_EQUAL_INT_MESSAGE(0, b_raced_through_while_a_parked,
|
||||
"thread B's load_primary_config must stay blocked (serialized by "
|
||||
"myconf->lock) while thread A is parked mid-call");
|
||||
|
||||
t43_free_myconf(&c);
|
||||
printf("test_bug1476_concurrent_load_primary_config_no_uaf_or_double_free: PASS\n");
|
||||
}
|
||||
|
||||
#endif /* XCE_TESTING */
|
||||
|
||||
/* ── dispatch ───────────────────────────────────── */
|
||||
|
|
@ -6435,6 +6670,8 @@ void test_config_repl(void)
|
|||
* (2026-07-18): catch-up continuation slow-lane offload */
|
||||
RUN_TEST(test_catchup_offload_probe_runs_promptly);
|
||||
RUN_TEST(test_catchup_inline_probe_blocks_RED);
|
||||
/* bug-1476: concurrent load_primary_config() UAF/double-free race */
|
||||
RUN_TEST(test_bug1476_concurrent_load_primary_config_no_uaf_or_double_free);
|
||||
#endif
|
||||
/* Task A: raw capnp shims */
|
||||
RUN_TEST(test_replconfig_req_ssl_encode);
|
||||
|
|
|
|||
|
|
@ -10,6 +10,7 @@
|
|||
#include <tlog.h>
|
||||
|
||||
#include "clusterconfig.h"
|
||||
#include "mylock.h"
|
||||
#include "xce/cluster.h"
|
||||
|
||||
/* Forward declaration — avoids circular include with cluster_def.h */
|
||||
|
|
@ -50,6 +51,16 @@ typedef struct {
|
|||
uint32_t resmgr_inst_delta; /* --resmgr-inst-delta CLI value; 0 = use hardcoded default */
|
||||
uint32_t xstrings_nhash; /* --xstrings-nhash CLI value; 0 = use hardcoded default */
|
||||
uint32_t xstrings_delta; /* --xstrings-delta CLI value; 0 = use hardcoded default */
|
||||
/* bug-1476: serializes load_primary_config()'s free-decode-publish
|
||||
* sequence against concurrent invocations for the SAME myconf_t (e.g.
|
||||
* two racing handle_repl_config_push deliveries dispatched onto separate
|
||||
* VTASK_LIVE_BLOCK workers for the same standby -- a live ASan
|
||||
* heap-use-after-free was caught this way). Must be mylock_init()'d
|
||||
* before first use (main.c) and mylock_fini()'d in myconf_fini(); test
|
||||
* fixtures that drive the real load_primary_config()/
|
||||
* config_activate_committed() must init it too (see
|
||||
* t43_attach_myconf/t43_free_myconf in test_config_repl.c). */
|
||||
mylock_t lock;
|
||||
} myconf_t;
|
||||
|
||||
extern cluster_config_t* get_default_conf(void*);
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue