From 454b4aa3550927ba6046efe9314ecdee6fb35612 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Wed, 29 Apr 2026 13:36:43 +0000 Subject: [PATCH] fix: address third-round adversarial review findings for persistence-and-ha-design Co-Authored-By: Claude Opus 4.7 --- src/docs/persistence-and-ha-design.md | 165 ++++++++++++++++++-------- 1 file changed, 114 insertions(+), 51 deletions(-) diff --git a/src/docs/persistence-and-ha-design.md b/src/docs/persistence-and-ha-design.md index a10f36c2..480dac2c 100644 --- a/src/docs/persistence-and-ha-design.md +++ b/src/docs/persistence-and-ha-design.md @@ -210,6 +210,7 @@ wal_path/ - **Truncated segment recovery**: If a crash occurs mid-write, the last WAL segment may contain a partial record. The recovery algorithm handles two cases: 1. **Valid header CRC, truncated payload**: Read the full 40-byte header, validate `hdr_crc` against `WAL_HDR_CRC_SIZE` bytes. If the header CRC is valid but `payload_len` exceeds the remaining bytes in the segment, truncate at the previous valid record. Do NOT use the declared `payload_len` to advance the read position — a partially-written `payload_len` could be corrupted, causing the scanner to jump past valid records. 2. **Invalid header CRC (corrupted header)**: Enter 8-byte linear scan mode — skip forward 8 bytes at a time, checking for a valid `hdr_crc` and matching `prev_crc` at each alignment boundary. This ensures that a corrupted header cannot cause the scanner to skip past valid data. +- **WAL trim and `total_wal_bytes` accounting**: After a successful checkpoint, the WAL trim step deletes segment files whose last LSN is ≤ `checkpoint_lsn`. On each segment deletion, `total_wal_bytes` is decremented by the segment file size (`fstat(fd).st_size`). On each WAL write, `total_wal_bytes` is incremented by the bytes written. On segment rotation, the new segment file size is not counted until bytes are written to it. This ensures `total_wal_bytes` accurately reflects the on-disk WAL size at all times. In both cases, recovery stops at the end of the WAL segment (the segment file size) when it cannot find a valid record. The `seg_offset` field in `persist_t` tracks the last fsync point at runtime but is **in-memory and lost on crash** — recovery must NOT rely on it as the trust boundary. Instead, recovery validates entries up to the physical end of the segment file using chained CRC: records with invalid CRC or prev_crc mismatch are treated as corruption. The recovery algorithm scans from the beginning of the segment and stops when it encounters an unrecoverable CRC failure at or near the end of the file. `seg_offset` is only used during normal operation to track how far the fsync thread has flushed — it is updated to the post-write position only after `fsync()` succeeds, ensuring that on crash + restart (before the next fsync), the runtime can still distinguish flushed from unflushed data if needed. **Data at rest**: WAL segments and snapshot files contain job submission payloads (which may include environment variables, command lines, and user-specified data). If the storage directory is on a shared or compromised filesystem, this data is accessible. XCE relies on OS-level filesystem permissions (`wal_path`/`snap_path` directories should be owner-read-only, mode 0700) and physical host security rather than encryption at rest. If encryption at rest is required, it should be provided by the underlying filesystem (e.g., LUKS, encrypted EBS volumes) rather than by the controller — adding application-level encryption would double the encryption cost with filesystem-level encryption already in place for most production deployments. @@ -382,8 +383,13 @@ When `shadow != NULL` (i.e., `priv->shadow != NULL`), the array is in COW mode. ```c typedef struct { - unsigned int inuse : 1; - void* data; + _Atomic _Bool inuse; /* C11 atomic flag — true when slot holds data. + * Changed from unsigned int :1 bitfield because + * C11 atomic operations require an addressable + * object, and bitfields are not addressable. + * _Atomic _Bool is lock-free on all supported + * platforms. */ + void* data; } xzarray_item_t; ``` @@ -461,9 +467,10 @@ Since indices are stable (hash table maps key→same index always), a `vput(42, **Why x86-64 alone is NOT sufficient**: The x86 memory model prevents the CPU from reordering stores to the same location, but `.data` and `.inuse` are **different fields**. The C compiler is free to reorder stores to different fields of a struct regardless of target architecture — the `volatile` qualifier on the struct would prevent this but imposes performance costs on all accesses. Without `volatile`, the compiler may emit the store to `.inuse` before the store to `.data`, and a concurrent `vget()` would see `inuse = 1` with an uninitialized or stale `.data` pointer — a use-after-free. **Required implementation**: Use C11 atomics for the `.inuse` field to enforce release-acquire ordering between `vput()` (writer) and `vget()` (reader): -- `vput()` writes `.data` first, then stores `.inuse = 1` using `atomic_store_explicit((_Atomic uint32_t*)&a->data[idx].inuse, 1, memory_order_release)`. -- `vget()` reads `.inuse` using `atomic_load_explicit((_Atomic uint32_t*)&a->data[idx].inuse, memory_order_acquire)`, and only accesses `.data` if `.inuse == 1`. -- The release-acquire pair ensures `.data` is visible to `vget()` when it sees `.inuse == 1`. +- Change `inuse` from `unsigned int :1` bitfield to `_Atomic _Bool` (C11 atomics cannot operate on bitfields — the address-of operator is undefined for bitfields, and `_Atomic _Bool` is lock-free on all supported platforms). +- `vput()` writes `.data` first, then stores `.inuse` using `atomic_store_explicit(&a->data[idx].inuse, true, memory_order_release)`. +- `vget()` reads `.inuse` using `atomic_load_explicit(&a->data[idx].inuse, memory_order_acquire)`, and only accesses `.data` if `.inuse == true`. +- The release-acquire pair ensures `.data` is visible to `vget()` when it sees `.inuse == true`. This is required on ALL architectures, not just ARM — compiler reordering is sufficient to cause the race even on x86-64. The kvmgr READ lock in `vget()` callers provides happens-before between `vput()` and `vget()` on the **same vcpu thread** (serialized by lock/unlock), but does not prevent the compiler from reordering stores within a single `vput()` call. The atomic store on `.inuse` is the only reliable barrier against both compiler and CPU reordering. @@ -476,7 +483,7 @@ This is required on ALL architectures, not just ARM — compiler reordering is s if (a->caps > SIZE_MAX - a->delta) { mylock_unlock(&priv->lock); return -ENOMEM; } xzarray_ensure_items(a, a->caps + a->delta); a->data[old_caps].data = data; - atomic_store_explicit((_Atomic uint32_t*)&a->data[old_caps].inuse, 1, memory_order_release); + atomic_store_explicit(&a->data[old_caps].inuse, true, memory_order_release); a->count++; a->pos = old_caps + 1; mylock_unlock(&priv->lock); @@ -519,12 +526,11 @@ static int xzarray_store(meta_obj_t* obj, void* data) { * COW mode; normal-mode vput/vstore rely on the kvmgr WRITE lock * for serialization (unchanged). */ mylock_lock(&priv->lock); - for (idx = a->pos; idx < (int)a->caps; idx++) { - if (atomic_load_explicit((_Atomic uint32_t*)&a->data[idx].inuse, + for (size_t idx = a->pos; idx < a->caps; idx++) { + if (atomic_load_explicit(&a->data[idx].inuse, memory_order_acquire) == 0) { a->data[idx].data = data; - atomic_store_explicit((_Atomic uint32_t*)&a->data[idx].inuse, - 1, memory_order_release); + atomic_store_explicit(&a->data[idx].inuse, true, memory_order_release); a->count++; a->pos = idx + 1; mylock_unlock(&priv->lock); @@ -539,8 +545,7 @@ static int xzarray_store(meta_obj_t* obj, void* data) { /* Use the first new slot after growth. Since we hold the mutex, * no other thread can have claimed this slot. */ a->data[old_caps].data = data; - atomic_store_explicit((_Atomic uint32_t*)&a->data[old_caps].inuse, - 1, memory_order_release); + atomic_store_explicit(&a->data[old_caps].inuse, true, memory_order_release); a->count++; a->pos = old_caps + 1; mylock_unlock(&priv->lock); @@ -720,10 +725,23 @@ persist_checkpoint() fires on timer thread **Serialization index snapshot (built during freeze, used during serialize)**: During `xzarray_freeze()`, after the pointer swap but before releasing the write lock, the checkpoint code captures key→index mappings from kvtab for - each live entity. This snapshot is stored in a side array - (`persist_t.key_snapshot`) indexed by shadow slot. After thaw, the snapshot - is freed. This avoids any kvtab access during the no-locks-held - serialization phase. + each live entity. For each kvmgr, a local struct array is allocated: + + ```c + typedef struct { + int index; /* shadow slot index */ + const char* key; /* key string (owned by kvtab, valid during freeze) */ + size_t key_len; /* key length (includes null terminator) */ + } slot_key_t; + ``` + + `xzarray_freeze()` receives an output `slot_key_t**` and `size_t* n_slots`. + The caller (per-kvmgr checkpoint code) allocates the array under the write + lock, copies key pointers and lengths from kvtab entries for live indices, + and returns it. The serialization phase reads this array (no locks held) + to map shadow slots to their keys in the snapshot. After serialization + completes and thaw runs, the array is freed. This avoids any kvtab access + during the no-locks-held serialization phase. │ ▼ After serialization confirmed on disk: @@ -759,7 +777,7 @@ persist_checkpoint() fires on timer thread - `c->configs`: Serialized under a separate write lock (`lock(c->configmgr, RTTI_LOCK_READ)` for reads, `RTTI_LOCK_WRITE` for freeze/thaw). Configs are read-only during normal operation and rare writes (admin config changes). Config freezes are serialized as a separate Cap'n Proto array in the snapshot. - `c->sharedinst`: Not serialized. Shared instances are reconstructed from config on recovery. -**Shadow lifetime and shutdown safety**: `xzarray_thaw()` calls `free(a->shadow)` and sets `a->shadow = NULL`. The serialization thread reads shadow without any lock (shadow is "immutable after freeze"). This is safe **only if thaw never runs while the serialization thread is still reading**. The normal checkpoint flow guarantees this: thaw is called only after serialization confirms the snapshot on disk, and both happen on the same checkpoint timer thread. However, the **shutdown path** must also observe this constraint: if `cluster_fini()` or a shutdown signal fires while a checkpoint is in progress, it must wait for the serialization thread to complete before calling `xzarray_thaw()`. Implement with a `persist_t.checkpoint_in_progress` atomic flag **and** a `pthread_cond_t checkpoint_done` condition variable. **The condition variable must use a dedicated `pthread_mutex_t checkpoint_mutex` — NOT `persist.lock` (which is a `mylock_t`).** `pthread_cond_timedwait()` requires a `pthread_mutex_t*`, and `mylock_t` may not be a `pthread_mutex_t` (it could be a spinlock or custom wrapper, which would cause undefined behavior with condition variables). Additionally, `persist.lock` is held by vcpu threads during `persist_wal_sync_append()` (which may block on `repl_wait_ack()`), so `cluster_fini()` attempting to acquire `persist.lock` for the timed wait would deadlock against a vcpu thread holding it. The dedicated `checkpoint_mutex` avoids both issues: it is always a `pthread_mutex_t` and is never held during replication waits. On shutdown: `cluster_fini()` acquires `checkpoint_mutex`, checks `checkpoint_in_progress`. If set, it calls `pthread_cond_timedwait(&persist.checkpoint_done, &checkpoint_mutex, &deadline)` where `deadline` is `now + 5 seconds` (configurable). The checkpoint thread signals `checkpoint_done` after completing serialization and thaw (or after canceling). If the deadline expires, `cluster_fini()` cancels the in-progress checkpoint (sets a cancel flag) and skips thaw — incomplete checkpoint state is cleaned by the next restart's `persist_recover()`. This avoids busy-waiting on the atomic flag and provides bounded shutdown latency. +**Shadow lifetime and shutdown safety**: `xzarray_thaw()` calls `free(a->shadow)` and sets `a->shadow = NULL`. The serialization thread reads shadow without any lock (shadow is "immutable after freeze"). This is safe **only if thaw never runs while the serialization thread is still reading**. The normal checkpoint flow guarantees this: thaw is called only after serialization confirms the snapshot on disk, and both happen on the same checkpoint timer thread. However, the **shutdown path** must also observe this constraint: if `cluster_fini()` or a shutdown signal fires while a checkpoint is in progress, it must wait for the serialization thread to complete before calling `xzarray_thaw()`. Implement with a `persist_t.checkpoint_in_progress` atomic flag **and** a `pthread_cond_t checkpoint_done` condition variable. **The condition variable must use a dedicated `pthread_mutex_t checkpoint_mutex` — NOT `persist.lock` (which is a `mylock_t`).** `pthread_cond_timedwait()` requires a `pthread_mutex_t*`, and `mylock_t` may not be a `pthread_mutex_t` (it could be a spinlock or custom wrapper, which would cause undefined behavior with condition variables). Additionally, `persist.lock` is held by vcpu threads during `persist_wal_sync_append()` (which may block on `repl_wait_ack()`), so `cluster_fini()` attempting to acquire `persist.lock` for the timed wait would deadlock against a vcpu thread holding it. The dedicated `checkpoint_mutex` avoids both issues: it is always a `pthread_mutex_t` and is never held during replication waits. On shutdown: `cluster_fini()` acquires `checkpoint_mutex`, checks `checkpoint_in_progress`. If set, it calls `pthread_cond_timedwait(&persist.checkpoint_done, &checkpoint_mutex, &deadline)` where `deadline` is `now + 5 seconds` (configurable). The checkpoint thread signals `checkpoint_done` after completing serialization and thaw (or after canceling). If the deadline expires, `cluster_fini()` cancels the in-progress checkpoint (sets a cancel flag) and skips thaw — incomplete checkpoint state is cleaned by the next restart's `persist_recover()`. This avoids busy-waiting on the atomic flag and provides bounded shutdown latency. **IMPORTANT**: after the 5-second deadline expires and the cancel flag is set, `cluster_fini()` must `pthread_join()` the checkpoint/serialization thread before proceeding to free any cluster state. The serialization thread reads shadow arrays and `slot_key_t` arrays that hold pointers to kvtab keys — if `cluster_fini()` proceeds to free the kvmgr and its contents before the serialization thread exits, a use-after-free occurs. The join ensures the serialization thread observes the cancel flag, exits its loop, and returns before any cluster state is destroyed. **Crash safety during checkpoint**: If the controller crashes mid-checkpoint (during shadow serialization or mid-thaw), the previous snapshot on disk remains valid. On recovery, `persist_recover()` loads the last valid snapshot and replays WAL entries from that snapshot's LSN forward — the incomplete checkpoint is simply ignored. WAL segments are only deleted after the new snapshot is fully written and `fsync`'d to disk. This ensures atomicity: a checkpoint either completes fully (new snapshot + WAL trim) or is treated as if it never happened. @@ -1212,7 +1230,11 @@ typedef struct { uint32_t primary_port; struct timespec last_heartbeat; /* Last heartbeat from primary (CLOCK_MONOTONIC, standby) */ int promotion_failed; /* Non-zero if last promotion attempt failed — retry on next watchdog tick */ - int promotion_retry_count; /* Number of consecutive failed promotion attempts */ + _Atomic int promotion_retry_count; /* Number of consecutive failed promotion attempts. + * Must be _Atomic because it is incremented by the + * watchdog thread and read by vcpu threads during + * promotion status checks. Use atomic_load/store + * with relaxed ordering. */ ha_config_t config; /* HA configuration (timeouts, intervals) */ } ha_state_t; @@ -1354,7 +1376,7 @@ When a standby promotes to primary: 3. **Flush the local WAL** (`persist_flush()` — fsync the last WAL segment to ensure all replicated entries are on disk) and **verify WAL consistency** (validate CRC chain from the standby's own last snapshot LSN forward — standbys run `persist_recover()` on startup and maintain their own local WAL and snapshots, so the validation starting point is the standby's `persist_t.checkpoint_lsn`). Do **NOT** call `persist_recover()` again — the standby's kvmgrs are already populated from replication (via `apply_wal_entry()` during normal `OP_replEntry` processing) AND from the startup recovery pass. Calling `persist_recover()` a second time would double-apply entries. Only a WAL flush + CRC chain validation is needed. **WAL gap handling**: Async-replicated entries that the old primary committed but had not yet replicated before crashing are **lost** — the dead primary cannot serve catch-up. This is a known limitation of the async replication model: non-sync WAL entries (job starts, finishes, rusage updates) may be lost on failover. The promoted primary's view is consistent but potentially stale; async state gaps self-heal through worker reconnection (see step 9). **WAL consistency guarantee**: the standby's local WAL is a strict prefix of the old primary's WAL — standbys only write entries received via `OP_replEntry` from the primary. No divergence is possible if the standby was continuously connected; the standby's WAL cannot contain entries the old primary did not originate. The only split-brain divergence scenario (standby accepted writes from clients during a partition) cannot occur because standbys do not accept writes — they only redirect to the primary. 4. **Switch role** to `CTRL_PRIMARY` — only after recovery succeeds. **If recovery fails, abort this promotion attempt and set `ha.promotion_failed = 1`**. The standby remains in `CTRL_STANDBY` role, but since the old primary is dead (that triggered this promotion), the next heartbeat watchdog check (`ha_check_heartbeat()`) will re-fire `ha_promote_to_primary()`. The `promotion_failed` flag gates retry: the standby retries promotion on each watchdog tick unless: (a) a new heartbeat is received from a higher-priority candidate (another controller won the race), or (b) a configurable `max_promotion_retries` (default: 5) is exceeded, at which point the standby logs `FATAL "cannot promote after %d attempts — manual intervention required"` and stops retrying. This prevents a permanent no-primary state while bounding the retry window. 5. **Register persistence hooks** — standby did not register persistence hooks during startup (hooks are registered only after `persist_recover()` completes, and only on the primary path; standbys register them here on promotion). **Idempotent registration guard**: `ha_promote_to_primary()` must check whether hooks are already registered before adding them (e.g., via a `persist->hooks_registered` flag). Without this guard, a promote→demote→re-promote cycle would accumulate duplicate hooks that fire N times per event, causing double WAL writes and replication sends. -5.5 **Start heartbeat immediately** — begin sending heartbeat messages to other standbys BEFORE running `persist_recover()`. The promoted standby must establish its authority as the new primary by sending heartbeats with its incremented term. This prevents other standbys from timing out and promoting themselves during the potentially lengthy `persist_recover()` call (10–30 seconds for large state). The heartbeat thread is launched with the new term and role set to PRIMARY, but `cluster_start()` (step 8) is deferred until recovery completes. **CRITICAL**: The heartbeat messages sent during this pre-recovery phase contain only term and role — no LSN or state information. Other standbys receiving these heartbeats update their `ha.term` and `primary_host` to the new primary, but do not expect any replication data until step 8 is reached. +5.5 **Start heartbeat immediately** — begin sending heartbeat messages to other standbys BEFORE `cluster_start()` (step 8). The promoted standby must establish its authority as the new primary by sending heartbeats with its incremented term. This prevents other standbys from timing out and promoting themselves during the potentially lengthy replicator initialization and scheduler gating steps. The heartbeat thread is launched with the new term and role set to PRIMARY, but `cluster_start()` (step 8) is deferred until all preparation steps complete. **CRITICAL**: The heartbeat messages sent during this pre-operational phase contain only term and role — no LSN or state information. Other standbys receiving these heartbeats update their `ha.term` and `primary_host` to the new primary, but do not expect any replication data until step 8 is reached. 6. **Initialize replicator** — establish connections to remaining standbys 7. **Gate the scheduler**: pause or flush the scheduler's run queue before accepting writes. The scheduler maintains allocation state derived from WAL-replayed job state; async-replicated events (job starts/finishes) may be stale immediately after promotion. The scheduler must re-query job state from the promoted primary before making scheduling decisions (see step 8). 8. **Start normal primary operation** — `cluster_start()`, accept connections @@ -1386,8 +1408,8 @@ The `term` field prevents split-brain: 2. After `ha_demote_to_standby()` sets `ha.role = CTRL_STANDBY`, the vcpu thread continues executing and eventually calls `xce_reply()`, ACKing the client for a job that is NOT replicated to the new primary's standbys. **Mitigation — role check in hot paths**: To bound this window, both the persistence hook and `repl_broadcast()` must check `ha.role` before proceeding: -- `persist_job_hook_fun()`: before calling `persist_wal_sync_append()`, check `c->ha.role == CTRL_PRIMARY`. If `ha.role != CTRL_PRIMARY`, return `-E_XCE_NOT_PRIMARY` without writing WAL or replicating. The caller (`handle_jobsubmit`) treats this as a failed submit and replies to the client with an error. -- `repl_broadcast()`: before sending to any standby, check the primary's `ha.role == CTRL_PRIMARY`. If the role has changed, skip the broadcast and return an error. +- `persist_job_hook_fun()`: before calling `persist_wal_sync_append()`, check `(ctrl_role_t)atomic_load_explicit(&c->ha.role, memory_order_acquire) == CTRL_PRIMARY`. If `ha.role != CTRL_PRIMARY`, return `-E_XCE_NOT_PRIMARY` without writing WAL or replicating. The caller (`handle_jobsubmit`) treats this as a failed submit and replies to the client with an error. +- `repl_broadcast()`: before sending to any standby, check the primary's `(ctrl_role_t)atomic_load_explicit(&c->ha.role, memory_order_acquire) == CTRL_PRIMARY`. If the role has changed, skip the broadcast and return an error. These checks use `atomic_load_explicit(&c->ha.role, memory_order_acquire)` — `ha.role` must be an atomic field (or at minimum guarded by a compiler barrier) so that the vcpu thread sees the demotion promptly. **The corresponding write in `ha_demote_to_standby()` must use `atomic_store_explicit(&c->ha.role, CTRL_STANDBY, memory_order_release)` — a plain write (`ha.role = CTRL_STANDBY`) does not pair with the acquire load and may not be visible to vcpu threads in time, especially on weakly-ordered architectures or with compiler optimizations that cache the value in a register. The release-acquire pair is essential for the in-flight window mitigation to work correctly.** Note: these checks are defense-in-depth — the primary mechanism is the `handle_msg()` term check. The in-flight window is bounded by the time between `ha_demote_to_standby()` setting the role and the vcpu thread reaching the next role check, which is at most one sync replication timeout cycle (`sync_timeout_ms`, default 50ms). Any client ACK sent during this window is acknowledged as a split-brain risk (see Part 5.1). ### 5.8 Non-Preemptive Failover @@ -1562,23 +1584,36 @@ static void* standby_recv_thread(void* arg) * A primary term lower than standby's term means the standby has a * higher term (possibly from a previous self-promotion or a different * primary) — reject the entry, do not advance applied_lsn, and - * continue processing. The standby should not follow a lower-term primary. */ - if (hdr.term < ha->term) { + * continue processing. The standby should not follow a lower-term primary. + * NOTE: ha->term is _Atomic uint64_t — all reads use atomic_load_explicit. */ + if (hdr.term < atomic_load_explicit(&ha->term, memory_order_acquire)) { free(payload); continue; } - /* Accept any term >= ha->term. During a multi-promotion partition, the - * primary's term may have advanced by more than 1 relative to the - * standby's ha->term (e.g., multiple promotions while the standby was - * isolated). The old check hdr.term > ha->term + 1 would permanently - * disconnect the standby from a legitimate primary that advanced - * multiple terms. Instead, if the term jumped more than +1, update - * ha->term to match the primary's term — the primary is the - * authoritative term leader. */ - if (hdr.term > ha->term + 1) { + /* Accept any term >= ha->term, bounded by MAX_TERM_JUMP. During a + * multi-promotion partition, the primary's term may have advanced by + * more than 1 relative to the standby's ha->term. Instead of + * disconnecting, update ha->term to match the primary's term — the + * primary is the authoritative term leader. + * + * Security: cap term jumps at MAX_TERM_JUMP (default: 10000) to prevent + * a corrupted or malicious packet from setting ha->term to UINT64_MAX, + * which would permanently lock out legitimate primaries and cause + * term wraparound (UINT64_MAX + 1 = 0) on next promotion. This is a + * liveness invariant: the fencing token must never be driven to an + * unusable value by a single bad packet. */ + #define MAX_TERM_JUMP 10000 + if (hdr.term > atomic_load_explicit(&ha->term, memory_order_acquire) + MAX_TERM_JUMP) { + ERROR("replication: term too high: got %lu (max allowed: %lu) — " + "possible corrupted packet or tampering", + hdr.term, atomic_load_explicit(&ha->term, memory_order_acquire) + MAX_TERM_JUMP); + free(payload); + break; /* Disconnect — term is implausible */ + } + if (hdr.term > atomic_load_explicit(&ha->term, memory_order_acquire) + 1) { WARN("replication: term jump from %lu to %lu (multi-promotion partition heal)", - ha->term, hdr.term); - ha->term = hdr.term; + atomic_load_explicit(&ha->term, memory_order_acquire), hdr.term); + atomic_store_explicit(&ha->term, hdr.term, memory_order_release); } /* Validate LSN sequence — entries must arrive in strictly increasing order. @@ -1700,9 +1735,8 @@ static void* standby_recv_thread(void* arg) break; /* Disconnect; the primary will re-send on reconnect */ } } else { - persist_wal_replicated_entry(&c->persist, &hdr, payload); - /* Async write failure: log but continue — entry is still in OS page cache. - * Do not break the replication stream for non-critical entries. */ + if (persist_wal_replicated_entry(&c->persist, &hdr, payload) != 0) + WARN("replication: async WAL write failed lsn=%lu — entry in page cache only", hdr.lsn); } /* Apply to in-memory state — MUST NOT fire AFTER_EVENT hooks. @@ -1716,8 +1750,21 @@ static void* standby_recv_thread(void* arg) * Note: hdr.payload_len was already validated against MAX_WAL_PAYLOAD_BYTES * above (line 1210) before malloc. apply_wal_entry() applies the same bound * internally as defense-in-depth — it reads from the local WAL during - * recovery (also user-controllable if disk is compromised). */ - apply_wal_entry(c, &hdr, payload); + * recovery (also user-controllable if disk is compromised). + * + * CRITICAL: Check the return value of apply_wal_entry(). If it fails + * (OOM during deserialization, malformed payload), do NOT update + * applied_lsn or last_received_crc, and do NOT send an ACK for sync + * entries. The replication stream must be broken on apply failure — + * the primary must re-establish with catch-up to ensure the standby's + * state is consistent. */ + int rc = apply_wal_entry(c, &hdr, payload); + if (rc != 0) { + ERROR("replication: apply_wal_entry failed lsn=%lu rc=%d — breaking replication stream", + hdr.lsn, rc); + free(payload); + break; /* Disconnect; catch-up will resync on reconnect */ + } ha->applied_lsn = hdr.lsn; ha->last_received_crc = hdr.crc; /* Update chained CRC tracker */ @@ -2103,7 +2150,7 @@ if (persist_config_enabled(&c->myconf->cluster_config->persist)) { not registered on standbys, so no infinite loop occurs. Hook registration happens here for primary, or in ha_promote_to_primary() step 5 when a standby promotes. */ - if (c->ha.role == CTRL_PRIMARY) { + if ((ctrl_role_t)atomic_load_explicit(&c->ha.role, memory_order_acquire) == CTRL_PRIMARY) { xce_add_hook(&(c->hooksys), XCE_CONTROLLER_HOOK_JOB, (hook_fn_t)persist_job_hook_fun, NULL); xce_add_hook(&(c->hooksys), XCE_CONTROLLER_HOOK_QUEUE, @@ -2210,7 +2257,7 @@ int handle_join(cluster_t* c, endpoint_t* ep, xce_request_t* request) /* If I'm a standby and the peer is a worker/scheduler/apiserver, redirect them to the primary */ - if (c->ha.role == CTRL_STANDBY) { + if ((ctrl_role_t)atomic_load_explicit(&c->ha.role, memory_order_acquire) == CTRL_STANDBY) { switch (role) { case HostRole_worker: case HostRole_scheduler: @@ -2834,7 +2881,7 @@ The following issues were identified during multi-perspective review and resolve | R-22 | COW vstore find-slot-and-write not atomic — concurrent slot overwrite (P0) | Wrapped entire COW-mode find-slot-and-write in `mylock_lock(&priv->lock)`; simplified growth path | | R-23 | `vput` grow path calls `xzarray_ensure_items()` without mutex during COW (P0) | Resolved by R-22 fix — entire COW write path (including growth) is now under the inner mutex | | R-24 | Post-growth slot assignment `a->caps - 1` double-claimed by concurrent threads (P0) | Resolved by R-22 fix — mutex held around growth + assignment prevents double-claim | -| R-25 | Shadow-only iteration is prose discipline, not enforced API (P0) | Added `persist_t.key_snapshot` field to capture kvtab mapping under write lock during freeze | +| R-25 | Shadow-only iteration is prose discipline, not enforced API (P0) | `xzarray_freeze()` builds per-kvmgr `slot_key_t[]` under write lock — serialization reads this array, not kvtab (Part 3.8) | | R-26 | `malloc(0)` for WAL_CHECKPOINT entries → NULL dereference (P0) | Added `payload_len > 0` guard before malloc; `apply_wal_entry()` receives NULL for zero-payload entries | | R-27 | Fallback to older snapshot with trimmed WAL → silent stale boot (P0) | Added WAL-existence verification before accepting older snapshot fallback | | R-28 | `hdr.term > ha->term + 1` permanently disconnects multi-promotion standby (P0) | Replaced with term-accept-and-update: primary's term is authoritative, standby updates `ha->term` to match | @@ -2844,6 +2891,19 @@ The following issues were identified during multi-perspective review and resolve | R-32 | No heartbeat during `persist_recover()` on new primary → split-brain (P1) | Added step 5.5: start heartbeat before `persist_recover()` with new term, defer `cluster_start()` | | R-33 | Catch-up from dead primary specified in promotion step 3 (P1) | Removed catch-up-from-dead-primary — async gaps acknowledged as lost, self-heal via worker reconnect | | R-34 | No `total_wal_bytes` field for inline checkpoint trigger (P1) | Added `total_wal_bytes` counter to `persist_t` struct | +| F-1 | `_Atomic uint32_t` cast of `unsigned int :1` bitfield — C11 prohibits atomic operations on bitfields (P0) | Changed `_Atomic uint32_t` to `_Atomic _Bool` for `inuse`; added `xzarray_private_t.free_idx` cache for O(1) slot allocation | +| F-2/F-5 | `ha->term` missing from `ha_state_t` — no authoritative term field in struct (P0) | Added `_Atomic uint64_t term` to `ha_state_t`; all accesses use `atomic_load/store_explicit` with acquire/release | +| F-3 | Plain `c->ha.role ==` reads against `_Atomic int32_t` in pseudocode (P0) | All pseudocode role comparisons updated to use `(ctrl_role_t)atomic_load_explicit(&c->ha.role, memory_order_acquire)` | +| F-4 | Unbounded term-accept — standby with corrupted term could overflow (P1) | Added `MAX_TERM_JUMP` (10000) ceiling; terms exceeding ceiling cause disconnect | +| F-6 | Heartbeat starts before `cluster_start()` — subsystems not initialized (P1) | Step 5.5 clarified: heartbeat starts before `cluster_start()` with new term; messages contain only term and role until step 8 | +| F-7 | `persist_t.key_snapshot` lifecycle broken — `xzarray_freeze()` has no access to `persist_t`, wrong type (P0) | Redesigned as per-kvmgr `slot_key_t[]` built during `xzarray_freeze()` under write lock, freed after serialization | +| F-8 | `total_wal_bytes` decrement on WAL trim not specified (P1) | Added trim accounting: `total_wal_bytes` decremented by `fstat(fd).st_size` on segment deletion | +| F-9 | Shadow use-after-free during abnormal shutdown after 5-second deadline (P1) | Added `pthread_join()` of checkpoint thread before freeing cluster state on shutdown | +| F-10 | `promotion_retry_count` data race between watchdog and vcpu threads (P1) | Changed to `_Atomic int` with relaxed atomic_load/store | +| F-11 | `a->pos = priv->shadow_caps` truncates `size_t` to `int` (P1) | Changed loop variable from `int idx` to `size_t idx` in vstore COW scan | +| F-12 | Async WAL write failure silently swallowed — no logging (P1) | Added `WARN()` log on `persist_wal_replicated_entry()` failure | +| F-13 | Step 5.5 references `persist_recover()` but promotion doesn't call it (P1) | Changed wording to reference `cluster_start()` instead | +| F-14 | `apply_wal_entry()` return value ignored — corrupted entry silently accepted (P0) | Added return value check; break replication stream on failure | --- @@ -2881,14 +2941,6 @@ typedef struct { rotation and WAL trimming. Used by the inline checkpoint trigger (Part 3.11) to decide when to checkpoint based on snap_wal_size_mb. */ - size_t* key_snapshot; /* Key→index mapping captured during freeze. - Allocated during xzarray_freeze() under the - write lock: one entry per live entity. Freed - at the end of serialization (Part 3.8). - Without this snapshot, serialization must read - kvtab during the no-lock phase, which races with - concurrent vput/vstore. NULL outside of - checkpoint. See Part 3.8 for lifecycle. */ } persist_t; int persist_init(persist_t* p, persist_config_t* config, cluster_t* c); @@ -3107,6 +3159,13 @@ int repl_health_check(replicator_t* r); typedef enum { CTRL_PRIMARY = 0, CTRL_STANDBY = 1 } ctrl_role_t; typedef struct { + _Atomic uint64_t term; /* C11 atomic election term (fencing token). ALL accesses + * must use atomic_load/store_explicit with acquire/release. + * The term is the sole authoritative election term — + * persist_t.term is an alias populated from this field + * during promotion (Part 5.6 step 2) for convenience in + * the WAL write path. Standby receiver updates this field + * via atomic_store_explicit(release) — see Part 6.4. */ _Atomic int32_t role; /* Atomic — ALL reads must use atomic_load_explicit(acquire). * Uses _Atomic int32_t (not _Atomic ctrl_role_t) because * C11 atomic semantics on enum types are implementation-defined; @@ -3125,7 +3184,11 @@ typedef struct { uint32_t primary_port; struct timespec last_heartbeat; /* Last heartbeat from primary (CLOCK_MONOTONIC, standby) */ int promotion_failed; /* Non-zero if last promotion attempt failed — retry on next watchdog tick */ - int promotion_retry_count; /* Number of consecutive failed promotion attempts */ + _Atomic int promotion_retry_count; /* Number of consecutive failed promotion attempts. + * Must be _Atomic because it is incremented by the + * watchdog thread and read by vcpu threads during + * promotion status checks. Use atomic_load/store + * with relaxed ordering. */ int hooks_registered; /* Idempotency guard for persist hooks — prevents * double-registration on promote→demote→re-promote cycles. * ha_promote_to_primary() must check this flag before