docs: fix 6 additional P0/P1 review findings for persistence-and-ha-design
P0 fixes (3): - R-16: vstore COW growth re-check used OOB a->data[old_caps].inuse - R-17: Standby WAL write return value unchecked before sending ACK - R-18: Chained CRC validation absent from replication stream P1 fixes (3): - R-19: Replicated entry LSN handling unspecified - R-20: seg_offset in-memory and lost on crash - R-21: WAL segment rotation prev_crc handling at recovery Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
parent
bb66b9766d
commit
54bbfa5865
1 changed files with 111 additions and 12 deletions
|
|
@ -206,11 +206,11 @@ wal_path/
|
|||
- **WAL trim guard**: WAL segments are only deleted up to `min(acked_lsn)` across all connected standbys (`standby_conn_t.acked_lsn` — updated on each `OP_replAck` received). This prevents trimming segments that a lagging standby still needs for catch-up. When a standby reconnects after a gap, it reports its last applied LSN, and the primary streams missing entries from that point. If the standby's LSN is older than the primary's oldest surviving WAL segment, a full snapshot transfer is required (see Part 6 standby catch-up). Each standby heartbeat includes the standby's `applied_lsn` so the primary always knows the safe trim point. **Empty-standby fallback**: When no standbys are connected (`replicator.nstandbys == 0`), use the primary's own `checkpoint_lsn` as the trim point — WAL segments before the latest confirmed checkpoint can be safely deleted. **Standby disconnect trim delay**: When a standby disconnects, the primary should delay WAL trimming by one checkpoint interval before falling back to `checkpoint_lsn`-based trimming. This gives the standby a window to reconnect and resume catch-up without requiring a full snapshot transfer. The implementation uses per-slot tracking in `replicator_t.last_disconnected_lsn[]` and `replicator_t.last_disconnect_time[]` (see Appendix A.3). When a standby disconnects, its `acked_lsn` and current time are recorded in its slot. The trim point is computed as `min(min_over_disconnected_slots(last_disconnected_lsn[i]), checkpoint_lsn)` for one checkpoint interval after each disconnect, then falls back to `checkpoint_lsn` per slot.
|
||||
- **WAL growth bound during standby disconnect**: The worst-case WAL accumulation when no standbys are connected is: `wal_growth_MB ≈ snap_wal_size_mb + (submit_rate_per_sec * avg_WAL_submit_KB * snap_interval_s * 2 / 1024)`. The `* 2` accounts for the one-interval trim delay before `checkpoint_lsn`-based trimming takes effect. At default settings (`snap_interval_s = 60`, `snap_wal_size_mb = 64`) with 10 submits/sec and ~1 KB average `WAL_JOB_SUBMIT` payload, worst-case growth is ≈65 MB — well within a single segment. At pathological rates (1000 submits/sec, 4 KB average payload), worst-case growth is ≈535 MB, requiring ~9 segments. **Mitigation**: If WAL disk usage exceeds `wal_max_size_mb` (default: 2 GB), log an `ERROR` and trigger an immediate checkpoint to advance `checkpoint_lsn` and enable trimming. The one-interval trim delay is skipped when `replicator.nstandbys == 0` and WAL usage exceeds `wal_max_size_mb / 2` — in this scenario, preserving disk space takes priority over the reconnection catch-up window.
|
||||
- Segments before the latest snapshot are deleted during recovery, subject to the trim guard above
|
||||
- Chained CRC detects corruption: if `prev_crc` doesn't match previous record's CRC, stop replay at last valid entry. The first record in a WAL segment uses `prev_crc = WAL_CRC_NONE` (defined as `0xFFFFFFFF`, not 0 — CRC32 can legitimately produce 0 for certain inputs, which would be indistinguishable from the "no predecessor" sentinel if 0 were used). Recovery validates by checking that each record's `prev_crc` matches the previous record's `crc` field, starting from `WAL_CRC_NONE` for the first record.
|
||||
- Chained CRC detects corruption: if `prev_crc` doesn't match previous record's CRC, stop replay at last valid entry. The first record in a WAL segment uses `prev_crc = WAL_CRC_NONE` (defined as `0xFFFFFFFF`, not 0 — CRC32 can legitimately produce 0 for certain inputs, which would be indistinguishable from the "no predecessor" sentinel if 0 were used). Recovery validates by checking that each record's `prev_crc` matches the previous record's `crc` field, starting from `WAL_CRC_NONE` for the first record. **Segment boundary handling**: When recovery encounters `prev_crc == WAL_CRC_NONE` at a position that is NOT the absolute first record in the file (i.e., at the start of a rotated segment), this is treated as a **valid chain break** — the CRC chain restarts from this point. The recovery algorithm must NOT treat `WAL_CRC_NONE` at any record position that is 8-byte aligned and at the beginning of a segment file as corruption. Each segment file is independently CRC-chained from its first record.
|
||||
- **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.
|
||||
In both cases, recovery stops when it reaches `seg_offset` (the last `fsync` point) without finding a valid record. Records beyond `seg_offset` may be partially written and are not trusted.
|
||||
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.
|
||||
|
||||
|
|
@ -526,17 +526,27 @@ static int xzarray_store(meta_obj_t* obj, void* data) {
|
|||
* would both hit this path. See the concurrent vstore race fix above. */
|
||||
int old_caps = a->caps;
|
||||
mylock_lock(&priv->lock);
|
||||
if (a->data[old_caps].inuse) { /* Re-check: another thread may have grown */
|
||||
/* Re-check: another thread may have grown while we waited for the lock.
|
||||
* Check a->caps > old_caps (not a->data[old_caps].inuse) — the latter is
|
||||
* out-of-bounds when the array hasn't grown yet (index old_caps is past
|
||||
* the end), and reads garbage. The correct test is whether another thread
|
||||
* already updated a->caps past our saved value. */
|
||||
if (a->caps <= old_caps) {
|
||||
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,
|
||||
/* After growth (by us or another thread), use the first free slot at a->caps-1
|
||||
* (the slot just before the new capacity after xzarray_ensure_items zeroed it).
|
||||
* Re-read old_caps since another thread may have grown — the new slot is always
|
||||
* at the position just before a->caps after growth. */
|
||||
int free_idx = (int)a->caps - 1;
|
||||
a->data[free_idx].data = data;
|
||||
atomic_store_explicit((_Atomic uint32_t*)&a->data[free_idx].inuse,
|
||||
1, memory_order_release);
|
||||
a->count++;
|
||||
a->pos = old_caps + 1;
|
||||
a->pos = free_idx + 1;
|
||||
mylock_unlock(&priv->lock);
|
||||
return old_caps;
|
||||
return free_idx;
|
||||
}
|
||||
|
||||
/* Normal path: unchanged circular scan */
|
||||
|
|
@ -1587,6 +1597,41 @@ static void* standby_recv_thread(void* arg)
|
|||
ha->applied_lsn + 1, hdr.lsn);
|
||||
}
|
||||
|
||||
/* Validate chained CRC — detect missing/reordered entries that LSN alone
|
||||
* cannot catch (since LSNs are "strictly increasing, not consecutive").
|
||||
* Each entry's prev_crc must match the CRC of the immediately preceding
|
||||
* received entry. On the first entry (applied_lsn == 0), prev_crc must
|
||||
* be WAL_CRC_NONE (established by catch-up protocol).
|
||||
* This mirrors the on-disk chained-CRC validation (Part 2.4) but operates
|
||||
* on the live replication stream. Without it, a reordered or replayed entry
|
||||
* with a valid per-entry CRC but wrong position in the chain would pass
|
||||
* the LSN check (since LSN gaps are expected) and corrupt the standby's
|
||||
* in-memory state and local WAL.
|
||||
*
|
||||
* On segment boundaries the primary sends entries with prev_crc ==
|
||||
* WAL_CRC_NONE as the first entry of the new segment. The standby treats
|
||||
* this as a valid chain break (same as on-disk recovery, Part 2.4). */
|
||||
if (ha->applied_lsn == 0) {
|
||||
/* First entry after catch-up: prev_crc must be WAL_CRC_NONE */
|
||||
if (hdr.prev_crc != WAL_CRC_NONE) {
|
||||
ERROR("replication: first entry after catch-up has non-NULL prev_crc %u",
|
||||
hdr.prev_crc);
|
||||
free(payload);
|
||||
break;
|
||||
}
|
||||
} else {
|
||||
/* Normal mode: prev_crc must match the CRC of the last received entry */
|
||||
if (hdr.prev_crc != ha->last_received_crc && hdr.prev_crc != WAL_CRC_NONE) {
|
||||
/* WAL_CRC_NONE in the stream means a segment boundary on the primary —
|
||||
* valid chain break, same as on-disk recovery. */
|
||||
ERROR("replication: chained CRC mismatch: expected %u, got %u, lsn %lu "
|
||||
"(possible reordered/missing entry)",
|
||||
ha->last_received_crc, hdr.prev_crc, hdr.lsn);
|
||||
free(payload);
|
||||
break; /* Disconnect; primary may have diverged — reconnect + catch-up will resync */
|
||||
}
|
||||
}
|
||||
|
||||
/* Validate CRC — detect corruption in replication path.
|
||||
* WAL_HDR_CRC_SIZE is defined in persist.h (Part 2.2) — covers all
|
||||
* header fields except crc and prev_crc.
|
||||
|
|
@ -1606,11 +1651,40 @@ static void* standby_recv_thread(void* arg)
|
|||
* WAL_JOB_SUBMIT (SYNC) entries must be fsynced before ACK so the
|
||||
* primary's durability guarantee holds on the standby as well — if the
|
||||
* standby crashes before fsync, the primary's ACK to the client was
|
||||
* premature. All other entry types use async (fire-and-forget) writes. */
|
||||
if (hdr.type == WAL_JOB_SUBMIT)
|
||||
persist_wal_sync_append(&c->persist, hdr.type, payload, hdr.payload_len);
|
||||
else
|
||||
persist_wal_async(&c->persist, hdr.type, payload, hdr.payload_len);
|
||||
* premature. All other entry types use async (fire-and-forget) writes.
|
||||
*
|
||||
* LSN handling: replicated entries use the PRIMARY's LSN and CRC — the
|
||||
* standby does NOT assign local LSNs. This is critical because:
|
||||
* (a) After promotion, the former standby must serve catch-up to other
|
||||
* standbys using the original LSN space (applied_lsn).
|
||||
* (b) The WAL trim guard uses min(acked_lsn) across standbys — if
|
||||
* LSNs were remapped locally, the trim guard would trim entries
|
||||
* that a lagging standby still needs.
|
||||
* (c) The chained CRC chain must match the primary's CRC values.
|
||||
*
|
||||
* The standby's persist_t.current_lsn must be updated to hdr.lsn after
|
||||
* each replicated write so the next locally-generated entry (after
|
||||
* promotion) gets hdr.lsn + 1. See persist_wal_replicated_entry() below.
|
||||
*
|
||||
* CRITICAL: Always check the return value of persist_wal_sync_append().
|
||||
* If the standby's local WAL write fails (disk full, I/O error), the
|
||||
* entry must NOT be applied, applied_lsn must NOT advance, and NO ACK
|
||||
* may be sent. A failed fsync with an ACK would violate the sync
|
||||
* replication durability guarantee — the primary would believe the
|
||||
* standby has the entry durably, but it is not on disk. */
|
||||
if (hdr.type == WAL_JOB_SUBMIT) {
|
||||
int rc = persist_wal_replicated_entry(&c->persist, &hdr, payload);
|
||||
if (rc != 0) {
|
||||
ERROR("replication: standby WAL write failed lsn=%lu rc=%d — dropping entry, not sending ACK",
|
||||
hdr.lsn, rc);
|
||||
free(payload);
|
||||
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. */
|
||||
}
|
||||
|
||||
/* Apply to in-memory state — MUST NOT fire AFTER_EVENT hooks.
|
||||
* apply_wal_entry() mutates state directly (same as persist_recover()).
|
||||
|
|
@ -1627,6 +1701,7 @@ static void* standby_recv_thread(void* arg)
|
|||
apply_wal_entry(c, &hdr, payload);
|
||||
|
||||
ha->applied_lsn = hdr.lsn;
|
||||
ha->last_received_crc = hdr.crc; /* Update chained CRC tracker */
|
||||
|
||||
/* Send ACK for sync-replicated entries — only when not in catch-up mode.
|
||||
* During catch-up the standby is replaying historical entries; sending ACKs
|
||||
|
|
@ -2714,6 +2789,12 @@ The following issues were identified during multi-perspective review and resolve
|
|||
| R-13 | Snapshot `total_size` used for direct malloc without boundary check (P1) | Added explicit requirement: validate `total_size <= MAX_SNAPSHOT_BYTES` BEFORE `malloc()` |
|
||||
| R-14 | CRC32 for WAL integrity not tamper-evident (P2) | Documented threat model: CRC32 protects against accidental corruption; HMAC-SHA256 would be required for adversarial tamper resistance |
|
||||
| R-15 | WAL payload logging relies on code review only (P1) | Added recommendation for structural enforcement via opaque `wal_payload_t` wrapper type |
|
||||
| R-16 | `vstore` COW growth re-check uses OOB `a->data[old_caps].inuse` (P0) | Fixed to check `a->caps <= old_caps` (capacity comparison) instead of OOB memory access; re-read free index after growth |
|
||||
| R-17 | Standby WAL write return value unchecked before ACK (P0) | Added return value check on `persist_wal_replicated_entry()` — disconnect on failure, no ACK on error |
|
||||
| R-18 | Chained CRC absent from replication stream (P0) | Added `hdr.prev_crc` validation against `ha->last_received_crc` on standby; `last_received_crc` field added to `ha_state_t` |
|
||||
| R-19 | Replicated entry LSN handling unspecified (P1) | Added `persist_wal_replicated_entry()` function — writes primary's LSN/CRC directly, updates `current_lsn` and `last_crc` to match |
|
||||
| R-20 | `seg_offset` in-memory and lost on crash — recovery trust boundary undefined (P1) | Clarified: recovery scans full segment file using CRC validity, not `seg_offset` — `seg_offset` is runtime-only for fsync tracking |
|
||||
| R-21 | WAL segment rotation `prev_crc == WAL_CRC_NONE` handling during recovery unspecified (P1) | Documented: `WAL_CRC_NONE` at segment-aligned position is valid chain break, not corruption |
|
||||
|
||||
---
|
||||
|
||||
|
|
@ -2781,6 +2862,21 @@ int persist_wal_async(persist_t* p, uint32_t type, void* payload, uint32_t len)
|
|||
* degraded mode self-healing). */
|
||||
int persist_wal_sync_append(persist_t* p, uint32_t type, void* payload, uint32_t len);
|
||||
|
||||
/* Write a replicated WAL entry — uses the primary's LSN and CRC.
|
||||
* Called by the standby replication receiver (Part 6.4) to write entries
|
||||
* received from the primary. Unlike persist_wal_sync_append/async, this
|
||||
* function does NOT assign a new local LSN. Instead it:
|
||||
* 1. Writes hdr directly (primary-assigned lsn, crc, prev_crc) to the WAL
|
||||
* 2. Updates p->current_lsn = hdr.lsn so the next local entry gets hdr.lsn + 1
|
||||
* 3. Updates p->last_crc = hdr.crc so the chained CRC chain remains valid
|
||||
* 4. Updates p->term = hdr.term to stay in sync with the primary's term
|
||||
* 5. fsyncs only if hdr.type == WAL_JOB_SUBMIT (sync replication)
|
||||
* Must NOT be called on the primary — use persist_wal_async or persist_wal_sync_append.
|
||||
* Returns 0 on success, -errno on I/O failure.
|
||||
* IMPORTANT: After calling this, the caller must NOT advance its own LSN
|
||||
* counter — p->current_lsn is already set to hdr.lsn. */
|
||||
int persist_wal_replicated_entry(persist_t* p, const wal_entry_hdr_t* hdr, const void* payload);
|
||||
|
||||
/* Checkpoint — dual zarray freeze + serialize + thaw + GC */
|
||||
int persist_checkpoint(persist_t* p, cluster_t* c);
|
||||
|
||||
|
|
@ -2967,6 +3063,9 @@ typedef struct {
|
|||
* double-registration on promote→demote→re-promote cycles.
|
||||
* ha_promote_to_primary() must check this flag before
|
||||
* calling xce_add_hook(). Reset to 0 on demote. */
|
||||
uint32_t last_received_crc; /* CRC of the last received WAL entry on the replication
|
||||
* stream — used for chained CRC validation (P0-C fix).
|
||||
* Mirrors persist_t.last_crc but on the standby side. */
|
||||
ha_config_t config; /* HA configuration (timeouts, intervals) */
|
||||
} ha_state_t;
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue