jobid_range_exists() (c->jobmgr) and the in-flight registry (job.c,
per-process, only populated by this node's own handle_jobsubmit calls)
both miss a real cross-node case: a client's XCECLI_FAILOVER_CALL retry
resends the same client-generated jobid after a connection drop, and the
resend lands on a newly elected primary that never processed the original
request. If the original was already committed before the old primary
died, the new primary's own local WAL already holds it (Raft Leader
Completeness) but jobmgr hasn't applied it yet — both existing guards see
nothing, so the resend appends a second, genuinely committed WAL_JOB_SUBMIT
entry for the same jobid. A live chaotic soak reproduced this exactly
(waldump: two committed entries, same term, matching apiserverId/timestamp/
sequence, one second apart) — apply's own idempotent dedup keeps jobmgr
correct, but replay's raw WAL_JOB_SUBMIT count no longer matches jobmgr's
growth, firing a false APPLY_DIVERGENT fence and deadlocking promotion.
Fix: apply_committed_entry()'s WAL_JOB_SUBMIT case now claims the jobid in
job.c's in-flight registry (new job_wal_commit_claim/release, wrapping the
existing inflight_range_try_add/remove) for the duration of dispatch,
regardless of whether this node originated the submission or is applying a
replicated/replayed entry. A resend landing mid-dispatch on any node now
sees the same -EEXIST rejection self-originated in-flight submits already
get. Release only fires when the claim call added the entry, so a
self-originated submission's own claim/release lifecycle (unchanged) is
never double-released or stolen.
Also fixes 4 pre-existing tests that broke under the earlier checkpoint_lsn
fix (0eb1054) and were missed in that commit's regression sweep: cluster-
less fixtures where last_applied_lsn never advances now correctly get
checkpoint_lsn=0 unless the test simulates apply catch-up, matching the
new (correct) semantics. One assertion in
test_checkpoint_tail_with_no_snapshot_is_apply_clean compared Phase 1's
checkpoint_lsn against Phase 2's independently-reconstructed physical WAL
tip — that equality only held under the pre-fix current_lsn-tracking
behavior and is removed, not patched around.
Also hardens run-ha-soak.sh's start_node() with setsid+nohup: a forked/
background test-harness invocation whose session tears down mid-run was
silently killing every controller process via SIGHUP forwarding (observed
twice — deploy completes, ~1s of activity, then all 3 nodes die together
with no error logged).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VgT3n4vTv4ZjCuCTmzed1F
xce_test's cmd_jsub hardcoded idgen(&idctx, 0, 0, ...) (clusterId=apiserverId=0),
and run-ha-soak.sh's submit_jobs() spawns a fresh xce_test process per retry
attempt, resetting idgen's static sequence counter each time. Two unrelated
job submissions from separate process launches landing in the same
wall-clock second collided on an identical jobid, surfacing as a spurious
APPLY_DIVERGENT fence in chaotic soak that was initially misattributed to
bug-1419's controller-side dedup logic.
Confirmed not a production risk: apiserver's GenerateJobID holds one
process-lifetime idgen_ctx_t via sync.Once, and each deployed instance has
its own distinct apiserverId, so config.h's jobid bit layout already
partitions the production namespace. Fixed by folding getpid() into the
harness's apiserverId so cross-process collisions become structurally
impossible there too.
Also adds test_jobsubmit_resubmit_after_full_resolution_rejected_no_second_job
to test_commit_barrier_reply.c, proving the post-resolution dedup window
(retry arriving after commit+apply+reply+release) is airtight, complementing
the existing before-resolution test -- both known bug-1419 dedup windows are
now provably race-free, confirming those fixes were never the cause of this
soak failure.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VgT3n4vTv4ZjCuCTmzed1F
jobid_range_exists() only checks c->jobmgr, but task 3.1 deliberately
defers job construction until after commit -- so a submission that is
durably WAL-appended and awaiting its commit outcome is invisible to
that check. A client-side retry (XCECLI_FAILOVER_CALL resending the
same jobid after a transport error or failover mid-flight) sails past
the guard and appends a second, non-identical WAL_JOB_SUBMIT entry for
the same jobid -- confirmed via waldump on a live chaotic-soak node
(duplicate submits, same jobid, differing subtime/CRC) -- which can
trip persist_recover()'s post-replay apply-correctness check into a
genuine APPLY_DIVERGENT verdict.
Adds a process-lifetime in-flight-jobid-range registry in job.c that
handle_jobsubmit atomically check-and-claims immediately after the
existing jobid_range_exists check, releasing from
jobsubmit_async_ctx_release (the single choke point already shared by
the commit-continuation, apply-complete, and OOM early-out paths). A
resubmit for a still-in-flight jobid now gets -EEXIST with no second
WAL append.
Adds a matching XCE_TESTING reset seam (declared in cluster.h) so a
test that deliberately leaves a submission's commit continuation
unresolved through teardown doesn't leak its jobid range into a later
test reusing the same range -- commit_engine_fini frees pending
continuations without invoking their callback.
Regression test added to test_commit_barrier_reply.c (RED->GREEN
confirmed). Full 78-group controller_unit_test sweep: 782 tests, 0
failures.
This fixes only the WAL duplicate-submit mechanism (layer 1 of
bug-1419). The deeper PreVote-freshness/apply-divergence liveness
deadlock (layer 2 -- no recovery path when the only fresh-enough node
is apply-divergent-fenced) remains open and needs its own design
discussion; buglog.json updated accordingly.
Full 79-group unit/integration suite re-run found exactly 2 stale tests in
test_commit_barrier_reply.c asserting task 3.2's documented interim
contract ("no observable effect until task 6 completes it"), which the
already-committed ordered apply pump (tasks 6.1/6.2) closed by construction.
Updated both to assert the real, deterministic apply-after-commit settled
state instead of the no-longer-true no-effect-yet state, matching the idiom
already used elsewhere in the same file. No production code changed.
Full suite: 0 failures across 79 groups. HA soak (--sequential, 5 rounds):
zero committed-job loss, zero divergence, all 3 nodes served as primary.
Both independently re-verified by adversarial review.
Wire persist_apply_pump_run() to call persist_take_pending_apply()
immediately after each entry's apply_committed_entry() call,
invoking the registered continuation strictly after the mutation.
Adds jobsubmit_reply_success_once() (job.c) so a successful job
submission finally sends a client reply once every array element
applies -- closes the standing gap from task 3.2 where committed
submissions never got a response.
Fixes bug-1415: commit_resolve_pending() dispatches the commit
continuation and the apply-pump kick as two independent
VTASK_LIVE_BLOCK vtasks with no completion-order guarantee. On any
real multi-vcpu pool (the production default, n_vcpus=8), if the
apply pump won the race and advanced last_applied_lsn past an LSN
before its continuation registered, the registration was permanently
orphaned -- the job would be constructed and visible, but the client
would hang forever. Untested by every existing fixture in this spec
(all use pool=NULL, which trivially serializes and can't reproduce
the race).
Fixed with a self-healing check in persist_register_pending_apply():
if the LSN is already <= last_applied_lsn, invoke the callback
immediately instead of registering for later pickup. Verified by the
spec's first genuine multi-vcpu regression test (real vtaskmgr_t,
n_vcpus=4), independently RED->GREEN reproduced by two review rounds.
Build persist_apply_pump_run()/persist_apply_pump_kick() -- the
ordered walk from last_applied_lsn+1 through committed_lsn, applying
every entry via apply_committed_entry() regardless of commit mode.
This is the first live (non-replay) consumer of that dispatch
function. Kick is wired into commit_resolve_pending() (the single
choke point all 6 internal committed_lsn-advancing paths funnel
through, discovered in task 1.2), dispatched via
vtask_enq_classified(VTASK_LIVE_BLOCK) with a CAS-coalesced
fire-and-forget trigger.
Fixes the two hazards flagged since task 4.1 as required before
going live:
1. Double-append: a thread-local call-provenance guard
(t_apply_seam_notifying) prevents persist_job_hook_fun/
persist_queue_hook_fun/persist_worker_hook_fun from re-appending
entries the apply pump itself just applied, while preserving
job_ctrl's legitimate live command-path appends (a naive
detail-based filter would have broken SIGCONT/SIGSTOP). Also
closes an undocumented instance of the same hazard in the
queue/worker hook functions.
2. Merge-staleness: record_job_fail_fact/postdone's fact recording
previously baked a receipt-time-computed final job state into the
WAL entry, which goes stale under deferred apply -- a FINISH
followed shortly by an ordinary post-exec FAIL could silently
clobber a correct DONE completion with a stale RUN|EXIT|PERR, no
guard catching it. Fixed by repurposing wal_jobstate_t's unused
_pad field to carry just enough data (FAIL's stage) to recompute
against the job's actual current state at apply time via the real
apply_job_fail_transition()/new apply_job_postdone_transition()
guard cores, instead of overwriting with a stale absolute value.
Backward compatible: _pad==0 (all prior data) routes to the same
legacy overwrite behavior.
Also fixes bug-1409 (unbounded busy-loop in apply_pump_drain when
committed_lsn advances without a corresponding on-disk entry) and a
genuine test-flakiness hazard (a second, independent apply-pump
kicker from the fsync_worker thread that existing tests didn't
account for).
Two pre-existing tests in test_commit_barrier_reply.c now fail as
expected -- their own assertions state "no job may be constructed
until task 6's apply pump exists," now correctly invalidated by this
task landing; updating them is task 7.3's explicit job.
Add test_fact_state_unchanged_throughout_quorum_loss_outage to
test_commit_barrier_reply.c (30->31), no production changes. Drives
a real repl_remove_standby quorum-loss window through all 4
fact-receipt handlers, polls live state and committed_lsn at 5
time-spread points (not once), asserts a real hook-spy never sees
AFTER_EVENT fire, and confirms durable WAL presence via direct
on-disk read.
Documents an architectural asymmetry for task 6 planning: facts have
no commit-timeout concept (COMMIT_MODE_RETAIN registers no
continuation, "never honors the timeout") unlike commands'
commit_submit_async -- the apply pump must discover newly-committed
retained facts purely via committed_lsn advancement, not any
per-fact signal.
Closes out task-group 4 (fact apply-after-commit split).
Convert on_job_start/on_job_finish/on_job_fail/on_job_postdone to
validate and durably append only -- BEFORE_EVENT validation hook
unchanged, AFTER_EVENT (post-mutation) hook no longer fires at
receipt, live state mutation removed. Adds persist_submit_job_fact()
(persist.c) since the durable append previously lived inside the
very AFTER_EVENT hook this task silences; wire format is unchanged
and identical to what apply_committed_entry() already routes.
mark_job_start/mark_job_finish (single caller each) renamed in place
to record_job_start_fact/record_job_finish_fact. mark_job_fail/
mark_job_update (shared with job_ctrl's client-command path, out of
boundary) left untouched; new durable-append-only siblings
record_job_fail_fact/record_job_update_fact added instead.
Accepts a narrow, permanent regression: on_job_postdone's finish_time
anomaly backstop has no wire-format equivalent and is dropped
(affects only a rare no-prior-exit-status path).
Two critical hazards discovered by review, documented in tasks.md,
MUST be resolved before task 6 wires apply_committed_entry() into a
live path: (1) persist_job_hook_fun still fires on HOOK_DETAIL_JOB_UPDATE
and will double-append every live-applied fact; (2) record_job_fail_fact/
record_job_update_fact precompute a final state that can be silently
clobbered by a concurrent intervening fact once apply is deferred by
the commit round-trip.
Add 4 tests to test_commit_barrier_reply.c (25->29), no production
changes: explicit-FAILED via truncation (distinct from timeout),
repeated failure loops (5 cycles) for both timeout and truncation,
and an array_size>1 mixed-outcome test proving a committed sibling's
apply hand-off isn't blocked by failing siblings while producing
zero phantom jobs for the failures and exactly one batch-level
failure reply.
Closes out task-group 3 (command apply-after-commit ordering).
Replace handle_jobsubmit()'s blocking commit call (task 3.1) with a
real async registration against commit_submit_async(). On
FAILED/timeout, replies failure immediately with no job constructed.
On COMMITTED, hands off to a new pending-apply registration
(persist_register_pending_apply) for the ordered apply pump (task
6.1, not yet built) to consume -- does not apply or reply directly,
per design.
Deviates from design.md's stated integration site: appends via the
same ha_primary_log_append/persist_wal_async primitive the RETAIN-fact
path already uses and calls commit_submit_async directly on
p->repl.ce, bypassing repl_wait_majority_ack/repl_submit_majority_commit
in repl.c. Verified safe -- majority-ack detection is driven entirely
by (ce, lsn) events inside the commit engine, agnostic to which
wrapper registered the continuation -- but narrows the reusable
primitive design.md intended; documented in tasks.md.
Deletes (not preserves-dead) task 3.1's bug-1408 notification fix --
apply_committed_entry()'s WAL_JOB_SUBMIT case (task 2.2) already
fires the notification per-entry, immune to the batch-suppression
class bug-1408 fixed.
IMPORTANT: no live path applies a successfully-committed job
submission until task 6 lands (only restart replay does) -- every
successful submission commits durably but gets zero reply until
then. This commit range must not be soak-tested or deployed against
live traffic before task 6 completes.
Reorder handle_jobsubmit() so job objects are constructed via
create_job() only after the WAL commit succeeds, not before. Adds
jobid_range_exists() for a fast-fail pre-check with no WAL work, and
persist_submit_job_payloads_and_commit() (persist.c) operating on raw
xce_job_t* payloads instead of pre-existing job_t*. Commit remains
synchronous -- converting to the continuation-based commit API is
task 3.2.
Fixes bug-1408 (found during review): the post-commit array-
construction loop suppressed notification for an entire array batch
if any one sibling element failed to construct, stranding other
durably-committed and successfully-constructed jobs invisible until
the next restart's replay. Notification now fires unconditionally
for every successfully-constructed element, independent of sibling
failures; the client reply still separately reports the batch's
construction status.
Root cause (found via live 3-node E2E + a verification test): lsn was 0-based
(hdr.lsn = current_lsn++ from current_lsn=0), and 0 was overloaded as a
sentinel, causing three cold-start defects on a freshly-promoted leader:
- the first WAL entry (own-term promotion NOOP at lsn 0) was structurally
uncommittable: commit_advance_locked only stores when cand>cur and
committed_lsn starts at 0, so lsn 0 can never advance it;
- commit_set_writable_lsn(noop_lsn=0) set writable_lsn=0, which wait_done
reads as 'gate off' — the own-term safety gate was a no-op on the first leader;
- bug-543 (WAL hole): ha_stream_wal_to_standby filters hdr.lsn>standby_lsn;
a fresh empty standby reports standby_lsn=0 (ambiguous 'empty' vs 'has lsn 0'),
so lsn 0 failed 0>0 and was dropped -> non-contiguous standby WAL.
Fix: a fresh persist store initializes current_lsn=1, reserving lsn 0 as the
none/empty sentinel. The first real entry is lsn 1 -> committable (0->1),
writable_lsn=1 -> gate effective, and standby_lsn=0 unambiguously means 'empty'
so catch-up streams from the first entry with no hole. persist_recover is
data-driven (current_lsn=max(hdr.lsn+1)), so existing on-disk WALs and non-fresh
recovery are unaffected; the change only sets the base for brand-new stores.
No wire/capnp change.
Verification: test_coldstart_lsn0 drives the real commit engine + real streamer
and proves H1 (first entry commits 0->1), H3 (gate effective + blocks when
committed<writable), H4 (empty-standby catch-up is contiguous [1,2], control
standby_lsn=1 gets only [2]). Full controller suite green under ASAN. Tests that
relied on the lsn-0 base were shifted (several strengthened); commit.c /
config_repl.c / ha.c / wire format untouched.
Owning specs: ha-commit-engine / ha-wal-replication / persist. Follow-up:
sweep stale lsn-0 comments (repl.c:429-436, persist.c:1055-1075/1240-1243/1296).
3 RED carriers (duplicate FINISH on DONE appends a 2nd WAL entry +
overwrites state; START on terminal clobbers it; replay non-equivalent)
+ 2 preservation pins (forward postdone PDONE-merge, fail-after-finish
PERR-merge) that 6.2 must not break. Reviewer-confirmed: the
UPDATE-clears-terminal arm is production-unreachable (pinned, not
driven). 22 tests / 3 RED; census 508 distinct triples, only those 3
fail.
Green integration proof: 4 facts driven during total quorum loss append
RETAIN with rc 0 in <1000ms (vs 5000ms timeout — wrongly-applied WAIT
tripwire), pending with retained-depth=4 logged, present exactly once
on disk (real WAL header scan); reconnect + one covering ACK commits
the suffix with current_lsn unchanged (no redelivery). Completes the
bug-396 fix evidence (umbrella Req 2.8). Census 495/495.
4 green pins: handle_jobdispatch replies receipt-only with zero WAL
appends through a real 2-alloc dispatch cycle (notify outcomes intact,
reply-precedes-processing); set/clr_preason and do_update_op rusage
stay receipt-style with RETAIN-pending fact appends ACK-committed.
Umbrella Reply-Ordering Contract cited per pin. DISCOVERY bug-451
(reviewer-verified): RUSAGE hook va_list type confusion — 4-byte OOB
read + garbage state in WAL_JOB_RUSAGE payloads; fix deferred to 7.1.
Census 488; zero failures.
