docs: address multi-perspective review findings for persistence-and-ha-design
Fixes 15 P0/P1 issues identified by coherence, feasibility, scope, and security reviewers: - P0: traversalLimitInWords fixed to constant bound (not file_size / 8) - P0: Job ID allocator reset added to recovery sequence - P0: acked_lsn declared _Atomic uint64_t in struct definition - P0: ha.role declared _Atomic ctrl_role_t with acquire/release semantics - P0: hooks_registered idempotency guard added to ha_state_t - P0: MAX_HA_CANDIDATES reduced from 16 to 4 (speculative generality) - P0: checkpoint_mutex (pthread_mutex_t) added to persist_t - P1: Split-brain detection condition explicitly specified - P1: Heartbeat interval minimum standardized to 2000ms - P1: Degraded-mode restricted state removed from scope - P1: Config version hash in heartbeat deferred to P3 - P1: WALEntry Cap'n Proto schema clarified as replication-only - P1: Snapshot backward compatibility path documented - P1: Snapshot total_size validated BEFORE malloc - P1: WAL_QUEUE_CTRL/WAL_WORKER_CTRL replay made idempotent - P1: mTLS gate enforced before replication code (P1-1) - P1: PENDING reason ambiguity logged at DEBUG during replay Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
parent
607f9519e1
commit
8bab418c61
1 changed files with 82 additions and 16 deletions
|
|
@ -168,7 +168,7 @@ Footnotes:
|
|||
|
||||
2. **`HOOK_DETAIL_JOB_RESUME`**: Does not exist in the current `cluster_def.h` enum. It must be added as a prerequisite for P0-6. The existing `on_job_resume()` function in `job.c` must be updated to call hooks with this new detail.
|
||||
|
||||
3. **Pending reason deduplication**: Both `HOOK_DETAIL_SETPREASON` and `HOOK_DETAIL_CLRPREASON` map to `WAL_JOB_PREASON` without payload differentiation. This is acceptable because pending reasons are transient — after a controller restart or standby promotion, the scheduler re-evaluates pending jobs and publishes fresh pending reasons when scheduling resumes. On recovery, jobs in `JOB_STAT_PEND` state are assigned a default pending reason ("Job is waiting to be scheduled") until the scheduler updates them. The critical invariant is that the job's pend/run/exit state is preserved, not the specific pending reason text.
|
||||
3. **Pending reason deduplication**: Both `HOOK_DETAIL_SETPREASON` and `HOOK_DETAIL_CLRPREASON` map to `WAL_JOB_PREASON` without payload differentiation. This is acceptable because pending reasons are transient — after a controller restart or standby promotion, the scheduler re-evaluates pending jobs and publishes fresh pending reasons when scheduling resumes. On recovery, jobs in `JOB_STAT_PEND` state are assigned a default pending reason ("Job is waiting to be scheduled") until the scheduler updates them. The critical invariant is that the job's pend/run/exit state is preserved, not the specific pending reason text. **Ambiguity logging**: When `apply_wal_entry()` processes a `WAL_JOB_PREASON` entry during WAL replay, it logs at `DEBUG` level: `"WAL_JOB_PREASON replay for jobid %lu — pending reason text is not preserved across recovery"` to make the ambiguity visible in logs rather than silent.
|
||||
|
||||
**WAL payload serialization format**: Each WAL entry's payload is serialized as Cap'n Proto using the `controller.capnp` schemas. The mapping from entry type to schema is:
|
||||
|
||||
|
|
@ -216,7 +216,7 @@ wal_path/
|
|||
|
||||
**Data retention and scrubbing**: Sensitive data (environment variables, job command lines) persists in WAL files until those segments are trimmed (after checkpoint), and in snapshot files until the next checkpoint replaces them. The retention window is bounded by `finished_retain_s` (default 1h) for job payloads: once a job is GC'd at checkpoint, its data no longer appears in new snapshots, and its WAL segment is trimmed after the checkpoint that removed it. Old WAL segments and snapshot files are deleted with `unlink()` — no explicit overwrite/scrubbing of file data is performed. If the storage device requires explicit data destruction (e.g., SSDs with wear leveling where unlink does not guarantee physical erasure), this must be handled at the storage layer (encrypted volumes with key rotation, or secure erase tooling). XCE does not implement application-level data scrubbing of WAL or snapshot files.
|
||||
|
||||
**WAL payload logging policy**: The persistence and replication modules must **never log WAL payload content**. Only entry metadata (type, LSN, term, payload_len, CRC) may appear in log messages. This is a security hardening invariant: `WAL_JOB_SUBMIT` payloads contain job environment variables which frequently hold API keys, database passwords, and other credentials. All logging call sites in `persist.c` and `repl.c` that reference WAL entries must log only `hdr->type`, `hdr->lsn`, `hdr->term`, `hdr->payload_len`, and CRC values — never the `payload` pointer or its contents. Code review must verify this as a gating check before merge. The CRC mismatch error path already follows this pattern: `ERROR("replication CRC mismatch: expected %u, got %u, lsn %lu", hdr.crc, computed_crc, hdr.lsn)`.
|
||||
**WAL payload logging policy**: The persistence and replication modules must **never log WAL payload content**. Only entry metadata (type, LSN, term, payload_len, CRC) may appear in log messages. This is a security hardening invariant: `WAL_JOB_SUBMIT` payloads contain job environment variables which frequently hold API keys, database passwords, and other credentials. All logging call sites in `persist.c` and `repl.c` that reference WAL entries must log only `hdr->type`, `hdr->lsn`, `hdr->term`, `hdr->payload_len`, and CRC values — never the `payload` pointer or its contents. Code review must verify this as a gating check before merge. The CRC mismatch error path already follows this pattern: `ERROR("replication CRC mismatch: expected %u, got %u, lsn %lu", hdr.crc, computed_crc, hdr.lsn)`. **Structural enforcement (recommended)**: Define an opaque `wal_payload_t` wrapper type that cannot be format-printed without an explicit unwrap function. Alternatively, define a `persist_log_entry_metadata()` helper that only accepts the header struct — preventing callers from passing the payload pointer. Relying solely on code review is a human control that can fail; a type-level or helper-level enforcement makes credential logging a compile-time or link-time error rather than a review-time catch.
|
||||
|
||||
**Filesystem type requirement**: `wal_path` and `snap_path` must be on a local filesystem (ext4, xfs, etc.), not on NFS or other network filesystems. The `fsync()` durability guarantee depends on the OS writing to stable storage — on NFS, `fsync()` may only flush to the server's cache, not to persistent storage, undermining the WAL's crash-recovery guarantee. Run a startup check using `statfs()` and reject `f_type` values matching `NFS_SUPER_MAGIC` (0x6969) or `SMB_SUPER_MAGIC` (0x517B) — log `ERROR` and abort if detected.
|
||||
|
||||
|
|
@ -227,16 +227,16 @@ On startup, `cluster_init()` calls `persist_recover()`:
|
|||
1. **Find latest snapshot**: Scan `snap_path/` for the snapshot with the highest LSN
|
||||
2. **Load snapshot**: Read the snapshot file into memory
|
||||
2a. **Verify snapshot checksums**: (i) Recompute CRC32 over the header fields (lsn, timestamp, term, version, nJobs, nQueues, nWorkers) and compare against `SnapshotHeader.headerChecksum`. If mismatch, the header has been tampered or corrupted — log an error and try the next-oldest snapshot. (ii) Recompute CRC32 over the Cap'n Proto payload and compare against `SnapshotHeader.checksum`. If mismatch, log an error and try the next-oldest snapshot. If **all** snapshots fail verification and the WAL contains entries that predate any valid snapshot (i.e., WAL segments have already been trimmed), the controller **must abort with a fatal error** — silently booting with empty state would silently discard live job/worker state and is data loss. The fatal error message must include: the number of corrupt snapshots found, the LSN range of surviving WAL segments, and the instruction `"Contact support or restore from backup before restarting"`. Do not fall back to empty state in this scenario. The empty-state fallback is only valid on first boot (see below). **Snapshot version check**: After verifying both checksums, check `SnapshotHeader.version`. If the version is higher than the current controller's `WAL_FORMAT_VERSION` (defined in `persist.h` as `1`), log a fatal error and abort — a snapshot written by a newer controller cannot be safely loaded by an older one. This catches version mismatches earlier than encountering unknown WAL entry types during replay.
|
||||
2b. **Deserialize**: Deserialize Cap'n Proto data, rebuild kvmgr instances. Set `capnp::ReaderOptions.traversalLimitInWords` proportional to the snapshot file size (`file_size / 8` words) to prevent O(n²) traversal amplification from a crafted snapshot — this is the same hardening practice used for WAL payload deserialization (Part 6.4). If the snapshot file is larger than `MAX_SNAPSHOT_BYTES` (default: 256 MB — 4× the expected 64 MB for 10K jobs × 1 KB + overhead), log an `ERROR` and reject the snapshot.
|
||||
2b. **Deserialize**: Deserialize Cap'n Proto data, rebuild kvmgr instances. Set `capnp::ReaderOptions.traversalLimitInWords` to a **fixed upper bound** (`MAX_SNAPSHOT_BYTES / 8` words) to prevent O(n²) traversal amplification from a crafted snapshot — this is the same hardening practice used for WAL payload deserialization (Part 6.4). Do NOT set it proportional to the actual file size, as a small file with deeply nested structure can still trigger excessive traversal. The limit should be `64 * 1024 * 1024 / 8 = 8M words` (the 64 MB is the expected max snapshot size). If the snapshot file is larger than `MAX_SNAPSHOT_BYTES` (default: 256 MB — 4× the expected 64 MB for 10K jobs × 1 KB + overhead), log an `ERROR` and reject the snapshot.
|
||||
- Jobs → `makeJob(c->jobmgr, ...)` (reuse existing `create_job()`)
|
||||
- Workers → `restore_worker(c, host)` (already exists in `worker.c:367`)
|
||||
- Queues → `create_queue(c, qconf)` (already exists)
|
||||
- Resources → `makeResource(c->resmgr, ...)` (already exists)
|
||||
- Configs → `add_config(c, key, val)` (already exists)
|
||||
3. **Replay WAL**: Starting from the snapshot's LSN, read and apply WAL entries forward via `apply_wal_entry()`. Hooks are **not yet registered** during recovery — persistence hooks are registered only after `persist_recover()` completes in `cluster_init()`, so no bypass flag is needed. This avoids re-persisting (WAL write during recovery) and prevents replication sends during replay. For `WAL_JOB_SUBMIT` entries, replay calls `restore_job(c, jobid, payload)` — a dedicated function that inserts the job at its original jobid without allocating a new one. Calling `job_submit()` here would assign a new jobid, breaking replay by creating duplicate or wrong-ID jobs.
|
||||
3. **Replay WAL**: Starting from the snapshot's LSN, read and apply WAL entries forward via `apply_wal_entry()`. Hooks are **not yet registered** during recovery — persistence hooks are registered only after `persist_recover()` completes in `cluster_init()`, so no bypass flag is needed. This avoids re-persisting (WAL write during recovery) and prevents replication sends during replay. For `WAL_JOB_SUBMIT` entries, replay calls `restore_job(c, jobid, payload)` — a dedicated function that inserts the job at its original jobid without allocating a new one. Calling `job_submit()` here would assign a new jobid, breaking replay by creating duplicate or wrong-ID jobs. **CRITICAL: Job ID allocator reset** — after WAL replay completes, the job ID allocator (idgen or equivalent) must be positioned past the highest jobid seen during recovery (from both snapshot and WAL replay). Without this step, the next `job_submit()` allocates a duplicate ID, causing silent data corruption. Track `max_replayed_jobid` during snapshot deserialization and WAL replay, then call `idgen_set_next(max_replayed_jobid + 1)` after step 3.
|
||||
4. **Trim WAL**: Delete WAL segments fully covered by the loaded snapshot
|
||||
5. **Retention pass**: After replay, run the retention/GC check on all entities (equivalent to a thaw pass) to clean up any entities whose TTL has expired during the controller's downtime. This handles the case where a controller was down for longer than `finished_retain_s` or `worker_retain_s` — expired entities are cleaned immediately on recovery.
|
||||
6. **Restore election term**: Set `ha.term = max(snapshot_header.term, max_term_from_replayed_wal)`. This is a security-critical invariant: a controller must never start with a term lower than any term it has previously used, as this would undermine split-brain fencing. On first boot (empty state), `ha.term` is initialized to 0.
|
||||
6. **Restore election term**: Set `ha.term = max(snapshot_header.term, max_term_from_replayed_wal)`. This is a security-critical invariant: a controller must never start with a term lower than any term it has previously used, as this would undermine split-brain fencing. On first boot (empty state), `ha.term` is initialized to 0. **Snapshot version backward compatibility**: If `SnapshotHeader.version` is lower than the current `WAL_FORMAT_VERSION`, this indicates a snapshot written by an older controller. New schema fields must be optional with defaults to ensure backward compatibility. If the schema has breaking changes between versions, a migration function (e.g., `migrate_snapshot_v1_to_v2()`) must be written before bumping `WAL_FORMAT_VERSION`. Do not accept older versions without verifying field compatibility — the default behavior of loading an older snapshot with missing fields is safe only if all new fields are optional with sensible defaults.
|
||||
|
||||
> **Forward compatibility warning — unknown WAL entry types during recovery**: `apply_wal_entry()` logs a `WARN` and returns 0 (skips) unrecognized `wal_entry_type` values. This allows a controller running an older version to recover from WAL segments written by a newer version — the old controller boots, skips entries it cannot interpret, and continues operating. However, the state changes intended by those skipped entries are **silently lost**. This is a downgrade risk: if a newer controller writes entry types the old controller does not understand, and the old controller is started on that WAL, the cluster state will be incomplete (missing features, incorrect job/worker state for data written by the newer controller). Deployments should never roll back to an older controller version after a newer version has written persistent state. If rollback is required, the only safe procedure is: drain all jobs, stop all controllers, delete WAL and snapshot directories on all candidates, then start the old controller in empty-state-first-boot mode.
|
||||
>
|
||||
|
|
@ -1129,6 +1129,13 @@ ha:
|
|||
2. Increment `ctrl_repl_degraded_count` metric (alongside `ctrl_split_brain_detected`)
|
||||
3. The degraded state persists until the standby reconnects — subsequent submits in degraded mode each log at `ERROR` and increment the counter
|
||||
|
||||
**Split-brain detection condition**: `ctrl_split_brain_detected` is incremented when a controller receives a message (heartbeat, replication, election probe, or client request) from a peer that believes a DIFFERENT controller is primary. Specifically:
|
||||
- A standby receives `OP_heartbeat` from a controller whose term is LOWER than the standby's current term (the sender is a stale primary that hasn't learned of the promotion)
|
||||
- A primary receives a message (election probe, `OP_replJoin`) from a controller whose term is HIGHER than its own (a higher-priority candidate has promoted itself)
|
||||
- A primary receives `OP_heartbeat` from another controller that claims to be primary with a term EQUAL to or HIGHER than the receiver's term
|
||||
|
||||
This detection is passive — it requires network communication, not active probing. During an actual network partition where no communication is possible, split-brain cannot be detected (this is the fundamental limitation of LSF-style election without quorum). The metric alerts the operator once communication is restored and the controller can observe the divergence.
|
||||
|
||||
**Trade-off**: Non-preemptive — if a higher-priority candidate recovers, it does NOT preempt the current primary. This avoids disruptive failback. Admin can trigger manual failover.
|
||||
|
||||
### 5.2 Candidate Configuration
|
||||
|
|
@ -1251,7 +1258,7 @@ Controller starts
|
|||
>
|
||||
> **Peer certificate validation in election probe**: The synchronous TLS dial in `am_i_primary()` must verify the peer's certificate CN and OU against the expected values from `cluster.yaml`. Specifically: the peer's cert must have `CN=ctrl-<candidate_name>` and `OU=ha` (where `candidate_name` matches the entry in the candidates list). A probe that connects to an unexpected host (wrong CN or OU) must be rejected as if no response was received — this prevents an attacker from impersonating a lower-priority candidate to force the probing controller into standby mode.
|
||||
>
|
||||
> **`ha_init()` startup enforcement**: At startup, `ha_init()` validates: `heartbeat_interval_ms >= 1000` (values below 1000ms truncate to 0 when passed to `delay_task()`, which takes integer seconds — this disables the heartbeat watchdog entirely); `heartbeat_timeout_ms >= heartbeat_interval_ms`; `election_timeout_ms >= 500`; `ncandidates >= 1`. It also verifies `ha_key_path` permissions (must be mode 0600) and logs a `WARN` if the HA certificate expires within `ha_cert_warn_days` (default 30). **Duplicate candidate validation**: `ha_init()` scans `candidates[]` and rejects any config where two entries share the same `host` (case-insensitive `strcasecmp`) or the same `host:port` pair. Duplicate candidate names cause ambiguous election probes and mTLS CN matching — an entry appearing twice would receive two heartbeat monitors, trigger double replication connections, and confuse the priority ordering. The validation must log a `FATAL` error naming both duplicate entries and return `-EINVAL`. See `ha_config_t` in Part 5.3 for field documentation.
|
||||
> **`ha_init()` startup enforcement**: At startup, `ha_init()` validates: `heartbeat_interval_ms >= 2000` (values below 2000ms truncate to 0 or 1 second when passed to `delay_task()`, which takes integer seconds — this makes the heartbeat interval too short or disables it entirely); `heartbeat_timeout_ms >= heartbeat_interval_ms`; `election_timeout_ms >= 500`; `ncandidates >= 1`. It also verifies `ha_key_path` permissions (must be mode 0600) and logs a `WARN` if the HA certificate expires within `ha_cert_warn_days` (default 30). **Duplicate candidate validation**: `ha_init()` scans `candidates[]` and rejects any config where two entries share the same `host` (case-insensitive `strcasecmp`) or the same `host:port` pair. Duplicate candidate names cause ambiguous election probes and mTLS CN matching — an entry appearing twice would receive two heartbeat monitors, trigger double replication connections, and confuse the priority ordering. The validation must log a `FATAL` error naming both duplicate entries and return `-EINVAL`. See `ha_config_t` in Part 5.3 for field documentation.
|
||||
|
||||
### 5.5 Monitor Thread (Standby)
|
||||
|
||||
|
|
@ -1697,7 +1704,7 @@ void handle_msg(cluster_t* c, endpoint_t* ep, msg_hdr_t* hdr, void* payload) {
|
|||
|
||||
**Catch-up receive ordering**: (1) `OP_replSnapshot` chunks arrive first and must be fully received and CRC-verified before any new WAL entries are applied to the kvmgrs. (2) The primary must NOT send live `OP_replEntry` messages to the catching-up standby until the snapshot transfer is complete — the standby sends `OP_replCatchupDone` (or equivalent signal) after successfully loading the snapshot. (3) After snapshot application, the primary streams WAL entries from `snapshot_lsn + 1` forward, and the standby applies them in strict LSN order. Only after reaching the primary's `committed_lsn` does the standby begin sending `OP_replAck` and transition to normal replication. This ensures the standby's state is self-consistent at each phase boundary: snapshot-loaded → historical-WAL-replayed → caught-up.
|
||||
|
||||
**Snapshot catch-up flow control**: The `OP_replSnapshot` transfer sends the full snapshot in chunks of `MAX_WAL_PAYLOAD_BYTES` through the SSL endpoint pipeline. The primary sends chunks as fast as the endpoint pipeline allows, and the standby must receive, buffer, and reassemble all chunks before applying the snapshot. The existing iomgr backpressure mechanism (endpoint write FSM, outq buffering, SSL-level flow control via `SSL_write` partial writes) provides implicit flow control: if the standby's receive path is slow, the primary's endpoint outq fills, and subsequent `SSL_write` calls block or return partial writes, naturally throttling the sender. **No additional application-level flow control is needed** beyond the iomgr/SSL pipeline's built-in backpressure. However, operators should be aware that a snapshot transfer (up to `MAX_SNAPSHOT_BYTES` = 256 MB) temporarily increases the standby's memory usage for reassembly. The standby allocates a receive buffer of `total_size` bytes (validated against `MAX_SNAPSHOT_BYTES` at first chunk, per Part 6.6) before receiving subsequent chunks — this memory is freed after snapshot application.
|
||||
**Snapshot catch-up flow control**: The `OP_replSnapshot` transfer sends the full snapshot in chunks of `MAX_WAL_PAYLOAD_BYTES` through the SSL endpoint pipeline. The primary sends chunks as fast as the endpoint pipeline allows, and the standby must receive, buffer, and reassemble all chunks before applying the snapshot. The existing iomgr backpressure mechanism (endpoint write FSM, outq buffering, SSL-level flow control via `SSL_write` partial writes) provides implicit flow control: if the standby's receive path is slow, the primary's endpoint outq fills, and subsequent `SSL_write` calls block or return partial writes, naturally throttling the sender. **No additional application-level flow control is needed** beyond the iomgr/SSL pipeline's built-in backpressure. However, operators should be aware that a snapshot transfer (up to `MAX_SNAPSHOT_BYTES` = 256 MB) temporarily increases the standby's memory usage for reassembly. The standby allocates a receive buffer of `total_size` bytes (validated against `MAX_SNAPSHOT_BYTES` at first chunk, per Part 6.6) before receiving subsequent chunks — this memory is freed after snapshot application. **CRITICAL: validate `total_size <= MAX_SNAPSHOT_BYTES` BEFORE calling `malloc(total_size)`. Do NOT allocate the buffer and then check — the check is the first operation on first-chunk receipt.**
|
||||
|
||||
**Catch-up trigger**: When a standby reconnects (or initially joins), it includes its `applied_lsn` in the heartbeat to the primary. The primary compares this against its available WAL segments and streams missing entries from `applied_lsn + 1` forward. If the standby's `applied_lsn` is older than the primary's oldest surviving WAL segment, the primary initiates snapshot transfer (see `OP_replSnapshot` framing above) before streaming WAL entries.
|
||||
|
||||
|
|
@ -2581,7 +2588,7 @@ P0 is broken into seven sub-phases with clear deliverables and dependency orderi
|
|||
|
||||
| Step | File | Description |
|
||||
|------|------|-------------|
|
||||
| P1-1 | `src/controller/netio.c` + `src/include/xce/netio.h` | Extend `create_ssl_context()` to accept `ca_cert_path` parameter for mTLS peer verification; update all existing callers to pass NULL for the new parameter (backward compat not needed — no released version). See Part 5.9 |
|
||||
| P1-1 | `src/controller/netio.c` + `src/include/xce/netio.h` | Extend `create_ssl_context()` to accept `ca_cert_path` parameter for mTLS peer verification; update all existing callers to pass NULL for the new parameter (backward compat not needed — no released version). See Part 5.9. **GATE: No replication channel code (P2) may be written or deployed before P1-1 is merged.** The HA replication channel must use mTLS for peer authentication — without this, any process that can complete a TLS handshake could inject arbitrary WAL entries via `OP_replEntry`. |
|
||||
| P1-2 | `src/controller/ha.c` + `include/ha.h` | Role management, `am_i_primary()`, promotion/demotion |
|
||||
| P1-3 | `src/controller/ha.c` | Heartbeat watchdog: `OP_heartbeat` exchange via iomgr/vtask pipeline; `ha_check_heartbeat()` on a **dedicated `pthread` with `nanosleep()` or `timerfd_create(CLOCK_MONOTONIC)`** (NOT `delaymgr` — see Part 5.5 heartbeat detection latency note) detects primary failure (replaces `tcp_probe`) |
|
||||
| P1-3a | `src/controller/ha.c` | Promotion sequence: stop replay, increment term, recover, start primary; SIGUSR1 handler for manual failover (see Part 5.8) |
|
||||
|
|
@ -2657,6 +2664,31 @@ The following are explicitly **not** covered by this design document and are def
|
|||
- **Runtime configuration changes**: Adding/removing standby candidates at runtime, changing `persist_config_t` values without restart, and online checkpoint interval adjustment are not designed here
|
||||
- **Partial WAL re-sync**: After partition healing, the standby performs full re-sync from the primary. Incremental re-sync (sending only divergent entries) is out of scope
|
||||
- **Snapshot compression**: Snapshots are stored as uncompressed Cap'n Proto. Compression (zstd, lz4) is a potential optimization deferred to future work
|
||||
- **Config version hash in heartbeat messages** (Part 10.8): Deferred to P3 (operational tooling). Not needed for core discovery, failover, or connection behavior. The `configVersion` field in the `Request` Cap'n Proto schema and the CRC freshness signal are removed from P1 scope.
|
||||
- **Degraded-mode restricted state** (Part 7.4 `repl_degraded_timeout_s`): Removed from scope. The "restricted mode" where the primary refuses `WAL_JOB_SUBMIT` after 300s without a standby ACK is a product policy decision, not a persistence or HA requirement. The existing sync+timeout+local-WAL fallback already meets the stated durability goal. Monitoring and operator escalation handle sustained degradation. `repl_degraded_timeout_s` is removed from `persist_config_t` — if HA policy needs a degradation tolerance field, it belongs in `ha_config_t`.
|
||||
- **`MAX_HA_CANDIDATES = 16`**: Reduced to `4` in `include/config.h`. The design is explicitly validated for ≤3 controllers. `MAX_STANDBYS` reduced from 15 to 3. Setting structural limits far beyond operational needs is speculative generality.
|
||||
|
||||
### Review Resolutions
|
||||
|
||||
The following issues were identified during multi-perspective review and resolved in this version:
|
||||
|
||||
| ID | Finding | Resolution |
|
||||
|----|---------|------------|
|
||||
| R-1 | `traversalLimitInWords` set to `file_size / 8` provides no security (P0) | Changed to fixed upper bound `MAX_SNAPSHOT_BYTES / 8` words |
|
||||
| R-2 | Job ID allocator not reset after WAL replay (P0) | Added `max_replayed_jobid` tracking and `idgen_set_next()` step in recovery sequence |
|
||||
| R-3 | `acked_lsn` not declared `_Atomic` in struct (P0) | Changed `standby_conn_t.acked_lsn` to `_Atomic uint64_t` |
|
||||
| R-4 | `ha.role` not atomic — role-change window incomplete (P1) | Changed `ha_state_t.role` to `_Atomic ctrl_role_t`, documented acquire/release requirements |
|
||||
| R-5 | Missing hook idempotency guard (P0) | Added `hooks_registered` flag to `ha_state_t`, required in `ha_promote_to_primary()` |
|
||||
| R-6 | `checkpoint_mutex` missing from `persist_t` (P1) | Added `pthread_mutex_t checkpoint_mutex` field |
|
||||
| R-7 | `WALEntry` Cap'n Proto schema confused with on-disk format (P1) | Clarified that `WALEntry` is replication-only; on-disk uses raw C `wal_entry_hdr_t` |
|
||||
| R-8 | Snapshot backward compatibility not specified (P1) | Added backward-compatibility note to recovery step 6 |
|
||||
| R-9 | Split-brain detection condition unspecified (P1) | Added detection specification in Part 7.4 |
|
||||
| R-10 | Heartbeat interval minimum inconsistent (P1) | Standardized to `heartbeat_interval_ms >= 2000` across all sections |
|
||||
| R-11 | `repl_degraded_timeout_s` in wrong config struct (P1) | Removed from scope (see Out of Scope above) |
|
||||
| R-12 | `WAL_CHECKPOINT` entries skipped creates CRC chain gaps (P2) | Documented in Part 6.4 that skipped checkpoint entries produce expected single-entry CRC gaps |
|
||||
| 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 |
|
||||
|
||||
---
|
||||
|
||||
|
|
@ -2685,6 +2717,10 @@ typedef struct {
|
|||
atomic_int checkpoint_requested; /* Set by inline WAL size trigger (Part 3.11);
|
||||
cleared by checkpoint timer after completing */
|
||||
pthread_cond_t checkpoint_done; /* Signaled when checkpoint completes or is canceled */
|
||||
pthread_mutex_t checkpoint_mutex; /* Dedicated mutex paired with checkpoint_done.
|
||||
MUST NOT be persist.lock (which is mylock_t) —
|
||||
pthread_cond_timedwait requires pthread_mutex_t*.
|
||||
See Part 3.8 for rationale. */
|
||||
} persist_t;
|
||||
|
||||
int persist_init(persist_t* p, persist_config_t* config, cluster_t* c);
|
||||
|
|
@ -2753,8 +2789,16 @@ int persist_worker_hook_fun(int type, int situation, int detail, void* data, va
|
|||
* WAL_JOB_CKPT — updates job ckpt_time in-place
|
||||
* WAL_JOB_RUSAGE — updates job rusage fields in-place
|
||||
* WAL_JOB_PREASON — updates job preason field in-place
|
||||
* WAL_QUEUE_CTRL — creates or updates queue from QueueConf payload
|
||||
* WAL_WORKER_CTRL — creates or updates worker from Worker payload
|
||||
* WAL_QUEUE_CTRL — creates or updates queue from QueueConf payload.
|
||||
* Idempotent: if the queue already exists (matched by name
|
||||
* or key), update its config in-place; otherwise create it.
|
||||
* During WAL replay or standby replication, duplicate
|
||||
* WAL_QUEUE_CTRL entries for the same queue must not create
|
||||
* duplicates — the create path checks for existing queues first.
|
||||
* WAL_WORKER_CTRL — creates or updates worker from Worker payload.
|
||||
* Idempotent: if the worker already exists (matched by hostname
|
||||
* or endpoint), update its state in-place; otherwise create it.
|
||||
* Same duplicate-safety guarantee as WAL_QUEUE_CTRL.
|
||||
* WAL_CHECKPOINT — no-op (marker only, no payload)
|
||||
*
|
||||
* Returns 0 on success, -errno on deserialization or mutation failure.
|
||||
|
|
@ -2823,7 +2867,12 @@ extern int vthaw(var, retain_fn_t should_retain, void* arg);
|
|||
|
||||
typedef struct {
|
||||
endpoint_t* ep; /* Full endpoint (SSL, FSM, outq) — uses iomgr/vtask pipeline */
|
||||
uint64_t acked_lsn;
|
||||
_Atomic uint64_t acked_lsn; /* C11 atomic — accessed locklessly by WAL trim (min across standbys)
|
||||
* and written by repl_on_ack() under ack_lock. Use
|
||||
* atomic_store_explicit(acquire)/atomic_load_explicit(release).
|
||||
* On LP64 platforms a plain uint64_t is atomically readable,
|
||||
* but using _Atomic documents the intent and ensures correctness
|
||||
* if compiled for ILP32. See Part 6.2 for full analysis. */
|
||||
int lagging;
|
||||
char host[XCE_MAXNAME_LEN];
|
||||
uint32_t port;
|
||||
|
|
@ -2875,7 +2924,11 @@ int repl_health_check(replicator_t* r);
|
|||
typedef enum { CTRL_PRIMARY = 0, CTRL_STANDBY = 1 } ctrl_role_t;
|
||||
|
||||
typedef struct {
|
||||
ctrl_role_t role;
|
||||
_Atomic ctrl_role_t role; /* Atomic — ALL reads must use atomic_load_explicit(acquire).
|
||||
* vcpu threads check role without holding locks, so writes
|
||||
* (during promote/demote) use atomic_store_explicit(release).
|
||||
* If any code path reads ha.role without the acquire barrier,
|
||||
* the role-change window mitigation is incomplete. */
|
||||
int my_priority; /* My index in candidates[] */
|
||||
replicator_t repl; /* Replicate to standbys (primary) */
|
||||
uint64_t applied_lsn; /* Last applied WAL entry (standby) */
|
||||
|
|
@ -2885,6 +2938,10 @@ typedef struct {
|
|||
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 */
|
||||
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
|
||||
* calling xce_add_hook(). Reset to 0 on demote. */
|
||||
ha_config_t config; /* HA configuration (timeouts, intervals) */
|
||||
} ha_state_t;
|
||||
|
||||
|
|
@ -2916,7 +2973,11 @@ struct PersistConfig $C.mapname("persist_config_t") {
|
|||
syncTimeoutMs@7 :UInt32 $C.mapname("sync_timeout_ms") = 50;
|
||||
}
|
||||
|
||||
# WAL entry — wire format for replication and on-disk
|
||||
# WAL entry — used in Cap'n Proto schema for replication messages (OP_replEntry).
|
||||
# On-disk WAL format uses a raw C struct (wal_entry_hdr_t, see Part 2.2), NOT Cap'n Proto.
|
||||
# The WALEntry schema duplicates the C header fields for network transport only.
|
||||
# Fields: term, lsn, timestamp, type, crc, prevCrc mirror wal_entry_hdr_t.
|
||||
# Payload is the Cap'n Proto-serialized entity data (Job, QueueConf, Worker, etc.).
|
||||
struct WALEntry $C.mapname("wal_entry_t") {
|
||||
term @0 :UInt64;
|
||||
lsn @1 :UInt64;
|
||||
|
|
@ -3144,7 +3205,8 @@ All system-level tunable constants are consolidated into `include/config.h` so t
|
|||
* they live in their respective config structs and are set from cluster.yaml. */
|
||||
|
||||
/* --- HA limits --- */
|
||||
#define MAX_HA_CANDIDATES 16 /* Structural max candidates in cluster.yaml */
|
||||
#define MAX_HA_CANDIDATES 4 /* Structural max candidates in cluster.yaml.
|
||||
* Reduced from 16: design validated for ≤3. */
|
||||
#define DEFAULT_HB_INTERVAL_MS 3000 /* Primary heartbeat interval (ms) */
|
||||
#define DEFAULT_HB_TIMEOUT_MS 10000 /* Standby heartbeat timeout (ms) */
|
||||
#define DEFAULT_ELECTION_TIMEOUT_MS 2000 /* Election probe deadline (ms) */
|
||||
|
|
@ -3153,7 +3215,11 @@ All system-level tunable constants are consolidated into `include/config.h` so t
|
|||
#define DEFAULT_HA_CERT_WARN_DAYS 30 /* Warn if cert expires within N days */
|
||||
|
||||
/* --- Persistence limits --- */
|
||||
#define WAL_FORMAT_VERSION 1 /* Bumped on incompatible schema changes */
|
||||
#define WAL_FORMAT_VERSION 1 /* Bumped on incompatible schema changes.
|
||||
* When bumping, write a migration function
|
||||
* (e.g., migrate_snapshot_v1_to_v2) before
|
||||
* changing this value. Older snapshots
|
||||
* load only if all new fields are optional. */
|
||||
#define WAL_SEG_SIZE (64 * 1024 * 1024) /* 64 MB pre-allocated WAL segment */
|
||||
#define WAL_HDR_CRC_SIZE 32 /* sizeof(wal_entry_hdr_t) - 2*sizeof(uint32_t) */
|
||||
#define MAX_WAL_PAYLOAD_BYTES (1u << 20) /* 1 MB per WAL entry */
|
||||
|
|
@ -3168,7 +3234,7 @@ All system-level tunable constants are consolidated into `include/config.h` so t
|
|||
#define DEFAULT_WORKER_RETAIN_S 86400 /* retain unavailable workers (s) */
|
||||
|
||||
/* --- Derived --- */
|
||||
#define MAX_STANDBYS (MAX_HA_CANDIDATES - 1) /* 15 — structural; ≤3 deployment ceiling */
|
||||
#define MAX_STANDBYS (MAX_HA_CANDIDATES - 1) /* 3 — matches ≤3 deployment ceiling */
|
||||
```
|
||||
|
||||
**Design rule**: If a constant affects more than one module or is an operator-visible limit, it belongs in `config.h`. If it is a purely internal implementation detail of a single module (e.g., an internal struct size or a local retry count), it may stay in the module header. The test: if an operator would ask "what's the maximum?" or a developer would search for the constant when debugging, it goes in `config.h`.
|
||||
Loading…
Add table
Add a link
Reference in a new issue