Skip to content

tests/chaos: property2 kill-point guards race a free-running child; mid-snapshot starves for 40s instead of failing #2836

Description

@devarismeroxa

Summary

tests/chaos/property2_test.go gates its SIGKILL on waitForReadCount — a lagging, parent-side
observation of a free-running child
— and then asserts kill-point preconditions that only hold if
the kill actually landed where the parent last looked. It doesn't reliably.

This is the same defect fixed for TestSIGKILL_RecoveryLoop_CrashDuringRecoveredRun in #2835, and
for sigkill_test.go's mid-snapshot case in #2534. property2_test.go never received either
treatment — it has no persistDelayMS, and its comments still carry the superseded arithmetic
("~30ms in, far short of the persister's FIRST automatic flush (~1000ms)").

Affects all three cases across both prune classes — 6 subtests.

Evidence

With a 1.5s stall injected between the read-count wait and the kill:

case result
mid-handoff expected RESUME_POSITION to be empty/fresh …, got 100
mid-position-write expected RESUME_POSITION (128) to be STALE - strictly behind the kill point (read #95)
mid-snapshot hangs — timed out waiting for child to exit, 41.6s

mid-snapshot is the worst of the three: it doesn't fail fast, it starves. That is the hang
sigkillCase's own doc comment describes — a full resume leaves produceLoop nothing to read. On a
required status check (tests/chaos (race, x3)) that is a 40s stall per affected subtest before
anything goes red.

Proposed fix

Mostly mechanical, and the machinery already exists:

Whatever lands should keep the vacuity guards intact. They are the good part: they are what turns
"this test silently stopped testing anything" into a visible failure.

Not affected

fanout_sigkill_test.go:65 and nsource_sigkill_test.go:102 also gate kills on waitForReadCount,
but their post-kill assertions don't depend on the child being mid-run — they restart and assert
gapless delivery through total. They degrade to silently weaker coverage (the crash may not be
mid-flight), not to red. fanout's own comment already labels its pre-kill check "Diagnostic only
(best-effort)". Worth tightening eventually; not a flake source.

Acceptance criteria

  • All 6 subtests survive an injected stall an order of magnitude larger than the natural margin
  • mid-snapshot fails fast rather than starving, if its precondition is ever violated
  • Vacuity guards retained, not loosened
  • Superseded arithmetic in the comments corrected
  • Deterministic reproduction of the current defect demonstrated before the fix, per fix(chaos): bound the recovery kill point by construction, not by a race #2835

Related: #2835, #2534, #2832, #2834

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions