flaky test: daemon liveness-lock test races its own 200ms child (fails on unrelated PRs)

[macanderson]

Problem

crates/stella-cli/src/daemon/tests.rs:154 —
the_liveness_lock_is_held_while_the_run_lives_and_free_once_it_ends — races
its own fixture and fails intermittently on CI.

Observed failing on a PR whose diff touches only crates/stella-observatory
and cannot reach stella-cli at all
(https://github.com/macanderson/stella/actions/runs/31146979055/job/92768340312):

thread 'daemon::tests::the_liveness_lock_is_held_while_the_run_lives_and_free_once_it_ends'
panicked at crates/stella-cli/src/daemon/tests.rs:160:5:
assertion `left == right` failed: a running child holds its lock
test result: FAILED. 1462 passed; 1 failed

Cause

The test spawns a child that lives for 200 ms and then asserts, with no
synchronization, that the child is still holding its lock:

let mut run = spawn_sh(&registry, "lock", "sleep 0.2");
let sidecar = registry.sidecar_dir(&run.id);

assert_eq!(
    lock_is_held(&sidecar),
    Some(true),
    "a running child holds its lock"
);

On a loaded runner, 200 ms can easily elapse between spawn_sh returning and
lock_is_held being called — process spawn, /bin/sh exec and registry I/O all
sit in that window. The child exits, the kernel releases the lock exactly as
designed, lock_is_held correctly answers Some(false), and the assertion
fails. The production behaviour is right; the test's liveness window is the
bug.

The asymmetry inside this one test is the tell. Its second half is properly
synchronized against a 10-second budget:

assert!(
    eventually(Duration::from_secs(10), || lock_is_held(&sidecar) == Some(false)),
    "the kernel must release the lock when the process exits"
);

eventually (tests.rs:52) already exists and is exactly the tool the first
assertion needs — it just is not used there, because "is it held right now"
cannot be retried: retrying would pass trivially once the child dies.

sleep 0.2 is also the outlier in the suite. Every other test that needs its
child alive across an assertion gives itself a wide margin:

Line	Script
105	sleep 30
136	sleep 30
157	sleep 0.2 ← the flake
225	sleep 120 & wait
402	trap '' TERM; sleep 120
480	... sleep 120

Build

Give the child a lifetime the test controls instead of one it races. The
sleep 0.2 exists only so the child dies on its own for the release half —
killing it explicitly is both faster and deterministic:

let mut run = spawn_sh(&registry, "lock", "sleep 120");
let sidecar = registry.sidecar_dir(&run.id);

// The child cannot have exited: nothing has signalled it.
assert_eq!(lock_is_held(&sidecar), Some(true), "a running child holds its lock");

let _ = run.child.kill();
let _ = run.child.wait();

assert!(
    eventually(Duration::from_secs(10), || lock_is_held(&sidecar) == Some(false)),
    "the kernel must release the lock when the process exits"
);

This keeps both halves of what the test is for — held while alive, released
once dead — and removes the timing assumption from the first one. It also
matches the neighbourhood (sleep 120 + explicit teardown), which is why the
other six tests in this file do not flake.

Keep the rest of the test as-is, including the stop/remove_dir_all
teardown at the end.

Verify

The failure is probabilistic, so a single green run proves nothing. Load the
machine and loop it:

# Should fail some fraction of the time on the current code, and never after.
for i in $(seq 1 50); do
  cargo test -p stella-cli --bin stella \
    daemon::tests::the_liveness_lock_is_held_while_the_run_lives_and_free_once_it_ends \
    -- --exact --nocapture || echo "FAILED on iteration $i"
done

Running it under contention (e.g. a parallel cargo test --workspace in
another shell) reproduces it far more reliably than an idle machine, which is
why it shows up on CI and not locally.

Since this is a test-only fix, the PR template's witness box should say so —
the "witness" here is the loop above going from intermittently-red to green.

Constraints

• Do not raise the eventually budget in the second half as a workaround; that
  half is not the one failing, and 10 s is already generous.
• Do not #[ignore] the test. Liveness-is-the-lock is the one liveness answer a
  recycled pid cannot forge (see the module docs), so this coverage matters.
• The test writes under a temp registry and calls remove_dir_all — keep that
  teardown, and keep the stop() call so no sleep 120 is left orphaned if an
  assertion panics before the kill.

Definition of done

the_liveness_lock_is_held_while_the_run_lives_and_free_once_it_ends no longer
depends on wall-clock timing for its first assertion, still asserts both the
held and released states, and survives the 50-iteration loop above under load.

Noticed while working on #1999 / #2026, whose PRs (#2027, #2047) touch only
crates/stella-observatory and inherited this failure.