docs(wolf): correct bug-1306 note — clobber does NOT extend to QUEUE_CTRL/WORKER_CTRL

Their WAL_MODE_WAIT hook (wal_hook_append_classified) does local-append only
and carries no durability verdict through the hook return, so there is nothing
for a later hook to clobber. JOB was unique in running the full commit barrier
inside the hook. Their commit barrier is the separate ha-commit-barrier-rollout
handler barrier — verify independently, do not mechanically extend the JOB fix.
This commit is contained in:
Rongsong Shen 2026-07-01 01:20:28 +00:00
parent adda4b8b9c
commit 9944c689db

View file

@ -9,7 +9,7 @@
<!-- How the user likes things done. Code style, tools, patterns, communication. --> <!-- How the user likes things done. Code style, tools, patterns, communication. -->
## Key Learnings ## Key Learnings
- (2026-06-30, bug-1306 FIXED — hook-rc clobber) `xce_run_hooks` (src/lib/hook.c:134-157) returns ONLY the LAST-registered hook's rc — `rc` is overwritten every loop iteration and the `break` is `BEFORE_EVENT`-only. So a protocol/durability step must NEVER carry its verdict through a hook return: multiple JOB `AFTER_EVENT` subscribers are registered (persist + sched + apiserver + a promotion re-add in ha.c), and a later one returning 0 CLOBBERS an earlier `-ETIMEDOUT`. This silently defeated the job-submit majority-commit gate — `handle_jobsubmit` saw `hook_rc==0` and replied `jobsubmit ok` for an entry that never reached majority → a new-term leader legally overwrote it (committed-job-loss, "reply-success ⇒ durable" violated). FIX (raft-apply-decoupling item #1): extracted the WAL append + commit barrier into an explicit `persist_submit_wal_and_commit(p, jlist)` that `handle_jobsubmit` calls DIRECTLY and reads the rc from; the JOB_NEW hook is now notification-only (regression pin `test_job_hook_new_is_notify_only`). SAME latent clobber still affects the other two `WAL_MODE_WAIT` ops (WAL_QUEUE_CTRL, WAL_WORKER_CTRL JOIN/UPDATE) whose handlers read `hook_rc` — extend the extraction to them. Diagnosis method that nailed it: `xce_test waldump <data_dir>` (REPL cmd) proved all 3 WALs gapless/CRC-intact/full-payload with the lost job only on the old leader at a term boundary (term-9 JOB vs term-10 NOOP at the same LSN) → divergence-from-premature-ack, NOT the catch-up prefix-gap; then the controller log's `commit: WAIT deadline expired` + absence of any `rejecting job` line proved the gate fired but its verdict was dropped. - (2026-06-30, bug-1306 FIXED — hook-rc clobber) `xce_run_hooks` (src/lib/hook.c:134-157) returns ONLY the LAST-registered hook's rc — `rc` is overwritten every loop iteration and the `break` is `BEFORE_EVENT`-only. So a protocol/durability step must NEVER carry its verdict through a hook return: multiple JOB `AFTER_EVENT` subscribers are registered (persist + sched + apiserver + a promotion re-add in ha.c), and a later one returning 0 CLOBBERS an earlier `-ETIMEDOUT`. This silently defeated the job-submit majority-commit gate — `handle_jobsubmit` saw `hook_rc==0` and replied `jobsubmit ok` for an entry that never reached majority → a new-term leader legally overwrote it (committed-job-loss, "reply-success ⇒ durable" violated). FIX (raft-apply-decoupling item #1): extracted the WAL append + commit barrier into an explicit `persist_submit_wal_and_commit(p, jlist)` that `handle_jobsubmit` calls DIRECTLY and reads the rc from; the JOB_NEW hook is now notification-only (regression pin `test_job_hook_new_is_notify_only`). CORRECTION 2026-06-30: the clobber does NOT extend to the other two `WAL_MODE_WAIT` ops (WAL_QUEUE_CTRL, WAL_WORKER_CTRL). JOB's hook was UNIQUE in running the full commit barrier (`persist_wal_sync_append`) INSIDE the hook; queue/worker hooks call `wal_hook_append_classified`, which for `WAL_MODE_WAIT` returns after the LOCAL append only (persist.c:2764 `if (rc != 0 || mode != WAL_MODE_RETAIN) return rc;`) — so they carry NO durability verdict through the hook return, nothing to clobber. Their majority-commit barrier (if any) lives outside the hook (ha-commit-barrier-rollout "handler barrier", tasks 2-3) — a SEPARATE question from the bug-1306 clobber; verify it independently, do NOT mechanically extend the JOB extraction. Diagnosis method that nailed it: `xce_test waldump <data_dir>` (REPL cmd) proved all 3 WALs gapless/CRC-intact/full-payload with the lost job only on the old leader at a term boundary (term-9 JOB vs term-10 NOOP at the same LSN) → divergence-from-premature-ack, NOT the catch-up prefix-gap; then the controller log's `commit: WAIT deadline expired` + absence of any `rejecting job` line proved the gate fired but its verdict was dropped.
- (2026-06-23, bug-1028 FIXED) There MUST be exactly ONE authoritative source for a WAL entry's Raft term. The leader's library send path (`ha_read_wal_entry_at`, ha.c:1472-1476) and the standby's `logOk`/`on_get_entry_term` both source terms from `persist_entry_term` (in-memory index, legacy-0→WAL_LEGACY_TERM). The heartbeat catch-up streamer (`ha_stream_wal_to_standby`, config_repl.c) — the third consumer — was sourcing terms from raw on-disk `hdr.term` (0 for legacy entries). A standby's `prev_log_term=1` (normalised) ≠ streamer's `prev_log_term=0` (raw) → `logOk` rejects the new-term NOOP forever → `applied_lsn` stuck → primary re-streams every heartbeat cycle. Fix: all three `hdr.term` reads in the streamer replaced with `persist_entry_term(&persist, hdr.lsn)`; function gained a `const persist_t*` parameter. This is the pattern: every source of the per-LSN (term,index) tuple MUST go through `persist_entry_term`; NEVER read `hdr.term` raw from disk — it is not normalised. - (2026-06-23, bug-1028 FIXED) There MUST be exactly ONE authoritative source for a WAL entry's Raft term. The leader's library send path (`ha_read_wal_entry_at`, ha.c:1472-1476) and the standby's `logOk`/`on_get_entry_term` both source terms from `persist_entry_term` (in-memory index, legacy-0→WAL_LEGACY_TERM). The heartbeat catch-up streamer (`ha_stream_wal_to_standby`, config_repl.c) — the third consumer — was sourcing terms from raw on-disk `hdr.term` (0 for legacy entries). A standby's `prev_log_term=1` (normalised) ≠ streamer's `prev_log_term=0` (raw) → `logOk` rejects the new-term NOOP forever → `applied_lsn` stuck → primary re-streams every heartbeat cycle. Fix: all three `hdr.term` reads in the streamer replaced with `persist_entry_term(&persist, hdr.lsn)`; function gained a `const persist_t*` parameter. This is the pattern: every source of the per-LSN (term,index) tuple MUST go through `persist_entry_term`; NEVER read `hdr.term` raw from disk — it is not normalised.
- (2026-06-23, bug-854 FIXED) The `XCECLI_FAILOVER_CALL` retry loop classifies a status-0 reply from ANY reachable node as SUCCESS and returns — it only rotates on failure. But ctrlstatus is answerable in every role, so a standby always answers 0, making `xcecli_ctrlstatus` pin to the first reachable node forever. The outer `xcecli_ctrlstatus_primary` loop captures a redirect hint from the non-authoritative reply but never forces rotation to the next master — so the configured master list is never traversed. Fix: `xcecli_ctrlstatus_primary` now calls `xce_failover_rotate(ctx)` + `xcecli_ctx_recover(ctx)` under `failover.lock` on every non-authoritative reply; the floor of `max_attempts` was raised to `nmasters+1` to cover the full ring. General pattern: any outer retry loop that queries a role-agnostic status must **force endpoint rotation** on a non-authoritative reply — do not rely on the inner failover wrapper, because status-0 preempts it. - (2026-06-23, bug-854 FIXED) The `XCECLI_FAILOVER_CALL` retry loop classifies a status-0 reply from ANY reachable node as SUCCESS and returns — it only rotates on failure. But ctrlstatus is answerable in every role, so a standby always answers 0, making `xcecli_ctrlstatus` pin to the first reachable node forever. The outer `xcecli_ctrlstatus_primary` loop captures a redirect hint from the non-authoritative reply but never forces rotation to the next master — so the configured master list is never traversed. Fix: `xcecli_ctrlstatus_primary` now calls `xce_failover_rotate(ctx)` + `xcecli_ctx_recover(ctx)` under `failover.lock` on every non-authoritative reply; the floor of `max_attempts` was raised to `nmasters+1` to cover the full ring. General pattern: any outer retry loop that queries a role-agnostic status must **force endpoint rotation** on a non-authoritative reply — do not rely on the inner failover wrapper, because status-0 preempts it.
- (2026-06-22, ha-term-safety task 3.1) The single primary-append seam is `ha_primary_log_append(cluster_t*,type,payload,len,&out_lsn)` (ha.c, decl ha.h ~333): packs the `raft_entry_blob_t` envelope and calls `raft_log_append(&c->ha.raft,...)` so the LIBRARY stamps `s->term` + assigns LSN + fires `on_persist_entry`. CRITICAL: `raft_log_append` returns `-EPERM` unless `s->role==RAFT_LEADER` — and `ha_do_promote`/the non-raft `ha_promote_to_primary` road sets `ha->role=CTRL_PRIMARY` but NOT `ha->raft.role` (only the raft election road via on_become_leader sets RAFT_LEADER). So `ha_primary_log_append` MUST have a `-EPERM`→legacy `persist_wal_async` fallback (stamping `ha->term` via persist_set_term first) or init-time/non-raft/engine-less promotion breaks. Retired the hand-rolled bypass by routing persist.c `wal_hook_append_classified` (RETAIN job/queue/worker hooks) + `persist_wal_sync_append` (WAL_JOB_SUBMIT WAIT) through `ha_primary_log_append` GUARDED on `p->cluster != NULL` — standalone persist (lib-level tests, `persist_init(p,&cfg,NULL)`) keeps the direct persist_wal_async path. `p->cluster` is the back-ref (persist.h, set in persist_init); ha.h is reachable in persist.c via `xce/cluster_def.h`. Co-tracking invariant `p->current_lsn == s->log_len + 1` holds because raft_init (2.5) seeds log_len from durable floor and every primary append advances both by 1. To make a controller TEST drive raft_log_append: `ha_init_raft` then set `c->ha.raft.role=RAFT_LEADER` + `c->ha.raft.term=T` directly (the raft-sim convention). on_persist_entry calls persist_wal_async (NOT the hooks) so no recursion. `ha_arm_own_term_gate` is static → test seam `ha_arm_own_term_gate_for_test`. Validation: pre-existing `test_wal_catchup_*` (7, bug-924 overlayfs) fail IDENTICALLY on baseline (git stash + rebuild proves it); every other group 0 fail. - (2026-06-22, ha-term-safety task 3.1) The single primary-append seam is `ha_primary_log_append(cluster_t*,type,payload,len,&out_lsn)` (ha.c, decl ha.h ~333): packs the `raft_entry_blob_t` envelope and calls `raft_log_append(&c->ha.raft,...)` so the LIBRARY stamps `s->term` + assigns LSN + fires `on_persist_entry`. CRITICAL: `raft_log_append` returns `-EPERM` unless `s->role==RAFT_LEADER` — and `ha_do_promote`/the non-raft `ha_promote_to_primary` road sets `ha->role=CTRL_PRIMARY` but NOT `ha->raft.role` (only the raft election road via on_become_leader sets RAFT_LEADER). So `ha_primary_log_append` MUST have a `-EPERM`→legacy `persist_wal_async` fallback (stamping `ha->term` via persist_set_term first) or init-time/non-raft/engine-less promotion breaks. Retired the hand-rolled bypass by routing persist.c `wal_hook_append_classified` (RETAIN job/queue/worker hooks) + `persist_wal_sync_append` (WAL_JOB_SUBMIT WAIT) through `ha_primary_log_append` GUARDED on `p->cluster != NULL` — standalone persist (lib-level tests, `persist_init(p,&cfg,NULL)`) keeps the direct persist_wal_async path. `p->cluster` is the back-ref (persist.h, set in persist_init); ha.h is reachable in persist.c via `xce/cluster_def.h`. Co-tracking invariant `p->current_lsn == s->log_len + 1` holds because raft_init (2.5) seeds log_len from durable floor and every primary append advances both by 1. To make a controller TEST drive raft_log_append: `ha_init_raft` then set `c->ha.raft.role=RAFT_LEADER` + `c->ha.raft.term=T` directly (the raft-sim convention). on_persist_entry calls persist_wal_async (NOT the hooks) so no recursion. `ha_arm_own_term_gate` is static → test seam `ha_arm_own_term_gate_for_test`. Validation: pre-existing `test_wal_catchup_*` (7, bug-924 overlayfs) fail IDENTICALLY on baseline (git stash + rebuild proves it); every other group 0 fail.