acpmux: fix the host approvals daemon-lock test flake (inherited flock in parallel runs)

Merged
TE
8d ago
+13-15

Flake fix for a red seen on base (never blocked): cli::host_approvals::tests::a_running_daemon_whose_socket_does_not_answer_fails_the_change failed once in a full parallel cargo test -p acpmux --lib run at c8f95c77 with did not answer (connection refused). It passes alone (5/5 with --exact at c8f95c77 and at e85bfbd2).

The shared resource

It is not a path or an env var. Each test already had its own directory, named per pid plus a per-test prefix. What the tests share is the process's open file table:

  1. The test takes flock on its daemon.lock, checks that a held lock fails the change, then drop(held) and expects the lock to read as free: "a lock nobody holds: no daemon".
  2. Other lib tests spawn child processes in parallel. Between its fork and its exec, a child has a copy of every open fd, including the lock's open file description. O_CLOEXEC closes that copy only at exec.
  3. If a spawn lands in that window, the dropped flock stays held for a moment. daemon_runs then sees WouldBlock and reload_on returns the "did not answer (connection refused)" error. The .unwrap() on that step panics with it.

The fix

Check the "lock file nobody holds" case before the test takes the lock, not after it drops it. The test still makes the same three checks (no lock file means no daemon, a lock file nobody holds means no daemon, a held lock fails the change), and they go through the same daemon_runs / reload_on code paths. No assertion is weaker. A comment on the test explains the ordering.

The two sibling tests (a_change_rewrites_the_file_whole_and_keeps_each_mac_once, a_change_holds_its_lock_until_the_reload_returns) move from per-pid temp_dir() folders to tempfile::tempdir(), so a panic no longer leaves a folder behind. They did not share a path, and the lock-hold test's 10 s wait already absorbs a brief inherited hold. Production code is unchanged.

Evidence

  • Reproduction (run 40870018660), with a throwaway test that is not part of this PR:
    • Deterministic: a child parked between fork and exec (pre_exec sleep). After drop(held), daemon_runs = true. After the child's exec it is false.
    • Stress, with 8 threads spawning true the way sibling tests spawn processes: the old order (lock, drop, check) read a daemon 185/20000 times. The new order (check, then lock) read one 0/20000 times.
  • At this head (run 40870087970):
    • cargo clippy -p acpmux --all-targets -- -D warnings passes with no warnings.
    • cargo test -p acpmux --lib -- --test-threads=32 ran 5 times. Every host_approvals test passed in all 5.
    • The one red in every run is cli::hosts::tests::linux_setup_installs_the_unit_and_hands_a_detached_daemon_to_it, a known base red with its own fix in a separate PR. Run 5 also had a second failure outside host_approvals. My filter did not capture its name, so I can't say which test it was; it was not in the file this PR changes.

Risk: test-only, one file.

Agent session: agent session

šŸ¤– Generated with Claude Code


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need.


Summary by cubic

Fixes a flaky host_approvals test in acpmux that failed when parallel tests spawned child processes while the test held its daemon lock.

The test previously took the lock, dropped it, then asserted the lock read as free. Between a child's fork and exec, the child inherits a copy of every open file descriptor, including the dropped lock, so the assertion could briefly read a running daemon and panic with "did not answer (connection refused)". The test now checks the unheld lock file before taking the lock; all three assertions and code paths are unchanged.

  • Moves the two sibling tests from per-pid temp dirs to tempfile::tempdir() so panics don't leave folders behind.
  • Production code is untouched.