fix(chaos): bound property2 kill points by construction, not by a race

[devarismeroxa]

Fixes #2836. Tier 1 (data-path chaos tests).

What was wrong

tests/chaos/property2_test.go carried the same unsound pattern #2835 fixed in recovery_test.go: all six subtests (3 kill windows x 2 upstream classes) watched a free-running child's stdout and then SIGKILLed it. The child kept producing while the parent was descheduled, so "I last saw N READs" said nothing about where the child was when the kill landed.

Failure-mode analysis — reproduced on demand, pre-fix

2.5s stall injected between the kill gate and the SIGKILL, against the pre-fix test (the issue's injection, same mechanism):

subtest	pre-fix with stall
mid-snapshot (both classes)	HANG 42.6s — "timed out waiting for child to exit" (the starvation its own comment describes)
mid-handoff (both classes)	FAIL "got 164" — resumeEmpty asserted, a flush landed, non-zero position persisted
mid-position-write (both classes)	FAIL "got 192" — stale guard (resumePos < killAfterReads) blown through by the free-runner

A stall is the failure mode here: the parent being descheduled is exactly what makes a lagging observation unsound, so "make it bigger and it passes" is the bug, not the fix.

The fix — every kill point now has one observed LOWER bound and one STRUCTURAL upper bound

• mid-snapshot / mid-handoff (resumeEmpty): kill still gated on READ/HANDOFF progress (lower bound: the child has genuinely produced), but the "nothing durably persisted at kill time" precondition is now structural twice: persistDelayMS=600s (the Flaky test suite: multiple order/timing-dependent failures make CI green unreliable #2534 pattern — no flush can ever land before the kill, however long the parent stalls) and holdAt=80 (the fix(chaos): bound the recovery kill point by construction, not by a race #2835 pattern — the child can never reach total, so it can never finish and exit before the kill; its read loop blocks forever, so the kill can never miss an exited process).
• mid-position-write (resumeValidNonZero): kill now gated on the child's durable upstream commit watermark reaching 70 (waitForUpstreamCommittedAtLeast) — a flush has provably landed, so the resume position is provably non-zero. The watermark only moves when a flush lands, so the kill lands on a flushed state where produced == acked == committed: the checkpoint equals the watermark, which is exactly the no-gap equality the shared assertions check. The in-flight window is empty by construction — losing it is the deliberate cost of bounding the kill on the watermark; crash-with-records-in-flight remains the job of DBZ-1's mid-stream case (sigkill_test.go). Upper bound: holdAt=100 — nothing past the ceiling is ever produced, so nothing past it can ever be acked or committed, and the resume position can never catch up past it however long the parent stalls. The old resumePos < killAfterReads guard is restated structurally as resumePos <= holdAt.
• New regression test TestChild_HoldAt_CapsProductionBelowTotal pins the ceiling itself (fix(chaos): bound the recovery kill point by construction, not by a race #2835's pattern): wait for the HELD marker, stall 400ms adversarially, assert nothing past the ceiling was produced.
• parseChildEnv rejects invalid combos at child startup: holdAt >= total exits exitBadArgs (a cap at/above total bounds nothing — the child would stop at total on its own).

Every value in the bracketed interval yields the identical verdict. 70/100 is a bracket, not a tuning: the gate cannot fire before the watermark reaches 70, and the ceiling means the watermark can never exceed 100 — the kill always lands with the watermark in [70, 100]. On an unloaded machine the gate fires at the second flush (a 15ms-paced flush covers ~66 records, so 66 < 70 <= ~132), but under the very descheduling this PR targets, production slows and the gate can fire at a later flush with coverage as low as just over 70 — so the case-level resumePos <= holdAt assertion is only a soft cap-removal detector; the deterministic detector is the regression test's HELD-marker wait (an uncapped child never prints it, on any machine).

Verified

• Stall-injected (2.5s — the same injection that fails all six pre-fix subtests): all six subtests + the regression test PASS.
• Perturbation proof (cap disabled in produceLoop): the regression test FAILS deterministically (HELD marker never appears — timeout); on this machine mid-position-write also FAILS (resume 129 > ceiling 100, the expected ~132 uncapped flush-2 coverage). The bound is load-bearing, not decorative.
• go test -race -v -count=3 -shuffle=on ./tests/chaos/... (the chaos.yml invocation): PASS, 368s; re-run after review-comment fixes: PASS (see below).
• golangci-lint (v2.12.2, the version tools/go.mod pins — local v2.1.6 predates the config): clean. go vet: clean.

Review round 2 (independent review, no blockers)

• Core design sound: watermark ∈ [70, 100] always (gate waits ≥ 70; watermark ≤ produced ≤ cap; watermark moves only at flush); every value identical verdict; persistDelayMS=600_000 not vacuous (persister's only mid-run triggers are the 1s timer and a 10k-bundle threshold — capped production can't hit 10k, SIGKILL bypasses the stopped-flush, resumed child runs the real debounce; same profile Flaky test suite: multiple order/timing-dependent failures make CI green unreliable #2534 shipped); regression-test mechanism verified (cap check is after send, before pace sleep; HELD never prints without the cap).
• Should-fix 1 (done): the case comment claimed a "stale (behind the kill point)" checkpoint with real in-flight records — false post-fix; the kill lands on a flushed state with the in-flight window empty by construction. Comment rewritten to state the flush-boundary semantics and the deliberate cost, with sigkill mid-stream named as the intended home of the in-flight scenario.
• Should-fix 2 (done): the "cap removal fails immediately" claim was not load-robust (under descheduling, a later flush can cover as few as just over 70 records, so the uncapped run can pass the case-level assert). Comment rewritten to the airtight bracket argument (watermark ∈ [70, 100] always) with the regression test's HELD-marker wait cited as the deterministic cap-removal detector.
• Nit (done): dropped the vacuous committed <= holdAt assertion in the regression test (at ~480ms elapsed the default 1s debounce has not fired, so committed is 0 by construction), documenting why the committed side is pinned elsewhere.
• No code semantics changed in round 2 — comments only plus one vacuous assertion removed. Re-run after the edits: go test -race -v -count=3 -shuffle=on ./tests/chaos/... green.

Failure-mode analysis of the change itself

What could this break: nothing in the data path — this touches only the chaos harness and test-scenario knobs; pkg/connector and pkg/pipeline are untouched. The env seam (CONDUIT_CHAOS_HOLD_AT) is consumed only by the child under test; every scenario that does not set it gets 0 (inert, identical behavior). A regression would surface as a chaos-suite failure — the suite runs on every PR touching tests/chaos/ and is a required CI context. Rollback: revert the commit.

Honest limits

With a watermark-gated kill, the SIGKILL lands at a flush boundary: the mid-position-write checkpoint equals the watermark (the no-gap equality asserted), and the in-flight window is empty by construction — bounded by the ack plumbing at the flush instant, not by how long the parent was descheduled. "Mid-production with deep in-flight" is arithmetically impossible at 15ms pace and is covered by DBZ-1's mid-stream case instead. The verdict — valid non-zero resume in [70, 100], no gap, at-least-once through total — is unchanged and structural. The pre-fix "got 164"/"got 192"/hang reproductions were run with the pre-fix test; the pre-fix and post-fix numbers are not directly comparable because the test's expectations changed by design.

🤖 Generated with Claude Code