chore(wolf): log bug-1480, bug-1481, bug-1482
bug-1480/1481: config-push silent-drop reply gap and vote_ep_err_handler legacy-routing gap, found during raft-adapter-conformance requirements discovery (not yet fixed at time of logging). bug-1482: myreq_handler_for_test's doc comment wrongly claimed the caller owns/frees the io_packet_t after the call; xce_decode actually consumes and frees it unconditionally. Found + corrected during task 2.2 code review remediation.
This commit is contained in:
parent
b7828eed4a
commit
ecf2dac6b2
2 changed files with 421 additions and 346 deletions
File diff suppressed because one or more lines are too long
|
|
@ -114,6 +114,7 @@
|
|||
|
||||
## Do-Not-Repeat
|
||||
|
||||
- (2026-07-29, bug-1482) When a code-review finding says "X is never freed, add a free() call" citing a `_for_test` wrapper's own doc comment as the ownership contract, VERIFY the doc comment against the actual callee before complying literally — don't just trust it. `myreq_handler_for_test`'s comment claimed "the caller still owns/frees `pkt` itself", but `myreq_handler` unconditionally calls `xce_decode(pkt, ...)` as its first step, and `xce_decode()` (src/lib/request.c:274-275) itself unconditionally does `free_packet(pkt); FREEUP(pkt);` before returning — pkt is fully consumed/freed internally, no leak exists. Blindly adding the suggested `free_packet(pkt); free(pkt);` after the call produced an immediate ASAN heap-use-after-free/double-free (independent of `detect_leaks`, since UAF detection isn't gated by leak detection) — caught only by actually rebuilding and running the authoritative test command, not by code inspection alone. Fix was to correct the misleading doc comment instead of adding the free. **Lesson: for any wrapper that decodes/consumes a buffer (xce_decode-style), trace the actual free call sites transitively before trusting an ownership comment — a stale/wrong doc comment is itself a bug, and "add the missing free" is not always the right fix for a reported leak.**
|
||||
- (2026-07-19, bug-1452) `commit_engine_fini(&ce)` MUST run AFTER `persist_stop(p)`, never before, whenever the `persist_t` was `persist_start`'d (i.e. has a live `fsync_worker` thread). `commit_engine_fini`'s own header comment says this explicitly ("Callers are expected to have stopped the flusher (persist_stop) first") but `test_persist_truncate.c`'s three continuation tests called it backwards (`commit_engine_fini` then `teardown_persist`→`persist_stop`). `persist_set_durable_cb(p, NULL, NULL)` inside `commit_engine_fini` is an unsynchronized plain-pointer unhook — while the flusher thread is still alive it can already be mid-flight in `commit_on_durable_advance`/`commit_advance_locked` (no lock held, per persist.c:398-399's documented lock-ordering) and race the teardown: the guard `ce->p == NULL` check passes on a still-valid read, then a SEPARATE later re-read of `ce->p` inside `commit_advance_locked` (commit.c:208) observes the value `commit_engine_fini` just nulled — SEGV at exactly `offsetof(persist_t, committed_lsn)` (0x38). This was misdiagnosed for a day as caused by the g_drive_async_demotion/catchup_offload flags (bug-1450/1451) because flipping them changed thread-scheduling timing enough to trigger the always-latent race more reliably — the SAME crash signature was independently seen on 2026-07-07, before those flags existed, proving the flags were never causal. Fix: call `persist_stop(p)` immediately before `commit_engine_fini(&ce)`; the teardown helper's own internal `persist_stop` becomes a safe no-op second call. **Same backwards ordering also exists in `test_demotion_convergence.c` (line 233 `commit_engine_fini` before line 235 `persist_stop`) against a real `persist_start`'d fixture — not yet fixed, flag for a follow-up sweep of every `commit_engine_fini` call site in the controller test suite.**
|
||||
- (2026-07-03, raft-apply-decoupling task 1.5) `ninja -C build`'s INCREMENTAL rebuild in this worktree's shared `build/` dir can silently produce a stale/wrong `controller_unit_test` binary that reports a build success but gives WRONG test results for the actual HEAD source — distinct from bug-924 (which is the `meson test` driver hanging on overlayfs fdatasync). Symptom that tipped this off: `test_config_repl`'s `test_commit_advance_retry_clears_push_before_commit_fence` was independently measured as 70/70-passing by one task's reviewer and 10/10-failing by the next task's implementer, on what both believed was the same commit. Root-caused by isolating each measurement in its own throwaway `git worktree add <tmp-path> <commit>` + fresh `meson setup --buildtype=debug -Db_sanitize=address` + `ninja -C <tmp-build>` — proved the SAME commit passes cleanly with a fresh build and fails deterministically only in the main worktree's incrementally-rebuilt `build/`. Fix was `rm -rf build && meson setup build ... && ninja -C build` in the main worktree. **Lesson: whenever a test result looks surprising or contradicts a very recent measurement on the same commit, do NOT immediately conclude a source regression — first rule out a stale incremental build by either a full `rm -rf build` rebuild or an isolated `git worktree add` + fresh setup.** Never mutate the current worktree in place to "check an old commit" (e.g. `git checkout <sha> -- .`) — it silently reverts working-tree files against current HEAD; use `git worktree add` instead.
|
||||
- (2026-06-24, raft-sim-conformance task 1.2) A CONFORMANCE cluster (`raft_sim_conformance_init`) wires `on_send_vote_request`/`on_send_append_entries` to the unified `c->bag[]` (sim_conf_send_*), NOT to the legacy `c->pending[]`/`c->ae_pending[]`. So `raft_sim_deliver_pending` / `raft_sim_deliver_append_entries` (which drain pending[]/ae_pending[]) DO NOTHING on a conformance cluster — a 3-node conformance election will leave node 0 stuck in CANDIDATE (assert "Expected 2 Was 1" = RAFT_LEADER vs RAFT_CANDIDATE). Bag delivery is task 2.1. To force a leader in a task-1.2 test WITHOUT bag delivery, use a 1-node conformance cluster: `raft_start_election` short-circuits node 0 straight to RAFT_LEADER (no peers to canvass). The conformance `applied[]` decode reads the committed entry's `node->log[k-1].bytes` (the legacy `raft_sim_entry_t` byte WAL, reused) as a little-endian uint64 (memcpy), NOT the `raft_sim_log_entry_t {term,value}` struct (that type is only used by the bag message `entry_value` field).
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue