Skip to content

Add readiness check to tests to avoid race condition flake - #457

Draft
asmacdo wants to merge 21 commits into
con:mainfrom
asmacdo:sampling-readiness
Draft

asmacdo wants to merge 21 commits into
con:mainfrom
asmacdo:sampling-readiness

Conversation

@asmacdo

@asmacdo asmacdo commented Oct 1, 2026 •

Copy link
Copy Markdown
Member

Three tests assumed duct takes at least one sample of a short command (sleep 0.1 / sleep 0.3). On a slow runner (one ps sample takes up to 0.72 s on macos-15-intel) that is not guaranteed, so they failed with zero samples. They now keep the command alive until its pid appears in duct's usage file, so no test depends on timing.

Together with #456 this fixes the nightly Intel Mac failures; any nightly failure after both merge gets a new issue.

Closes #451
Closes #439

Stacked on #456: this branch is based on it, so the diff shows #456's commits too until #456 merges. Only the last 5 commits are this PR.

Changes

  • New test/data/until_sampled.sh USAGE_FILE: loops until its own pid shows up in the usage file (gives up with exit 1 after 60 s).
  • test_session_modes: runs until_sampled.sh instead of sleep 0.3; exit code 0 also proves the command was sampled.
  • test_execution_summary: same, with a 0.1 s report interval; rerun backoff dropped.
  • test_spawn_children: each child records its pid in a test-provided dir and stays alive until sampled; the parent script exits only once all were sampled (replacing the hold-the-door sleep, so the expected count is N, not N+1). The test compares recorded pids, not command-line text (which ps -w truncates), and requires every child to have recorded itself.
  • test_spawn_children, setsid case: the process that calls setsid now forks the recording child, so that child was never in duct's session and "its pid must not appear" is a fair check. The script waits for every child's record before counting two more reports. The case is skipped where there is no setsid command.
  • The flaky rerun markers on test_spawn_children, test_signal_int, test_signal_kill and test_execution_summary are removed; no test reruns on failure any more.

Verification

Local, Linux, py3.13, pytest-rerunfailures disabled, whole process tree capped with systemd-run --user --scope -p CPUQuota=10% (measured before the setsid change):

  • spawn_children + session_modes: 84/84 over 3 passes (before: 25 of 28 failed).
  • test_execution_summary: old 8/8 failed, new 5/5 passed.

The setsid change: all 24 spawn_children cases pass over 5 passes, and also with setsid delayed by 30, 50 and 200 ms through a PATH shim. Those delays made the previous version fail 7 of 20 and 20 of 20 times, or pass with no children recorded.

CI is the real check, especially the Intel 3.10 job, which reran these tests 7 times on #456. On this branch every job passes with zero reruns. The macOS runners have no setsid command, so the setsid cases are skipped there (they used to pass without checking anything) and run on Linux.

Why the setsid case needed fixing

An earlier push of this branch failed test_spawn_children[duct-1-setsid] on 2 of 36 Linux cases: a recorded child pid showed up in duct's samples. setsid cmd & forks inside duct's session, and that same pid only then execs setsid, calls setsid(2) and becomes the child. For those few milliseconds the pid really is in the session duct watches, so a sample landing there recorded it, and the test compares pids over the whole run. The old test did not notice because it matched on sleep in the command line.

Why one sample is slow on Intel Mac

One duct sample is ps -ax plus a session lookup per process (~500 processes on runners). Measured on CI with a diagnostics branch (single runs, no spread):

runner one sample (median / max) usage lines for sleep 0.3
Linux 0.004-0.009 s / ≤0.08 s 7
ARM Mac 0.03-0.06 s / ≤0.11 s 2-5
Intel Mac 0.05-0.24 s / up to 0.72 s 1-4 (3.10: 1,1,2)

test_session_modes[current-session] never failed: that session includes pytest and the shell, so a sample is never empty.

🤖 Generated with Claude Code

asmacdo and others added 9 commits September 30, 2026 11:17
pytest-cov 7 dropped its own subprocess support; coverage's
`patch = subprocess` replaces it. e2e tests that run duct via the CLI
now count toward coverage (TOTAL 91.28% -> 92.28% on py313).

Co-Authored-By: Claude Code 2.1.285 / Claude Opus 5.5 <noreply@anthropic.com>
`duct` execs into `con-duct run`; exec skips exit handlers, so without
coverage's `execv` patch the shim's data is lost.

Co-Authored-By: Claude Code 2.1.285 / Claude Opus 5.5 <noreply@anthropic.com>
With parallel coverage each process writes .coverage.<host>.<pid>...;
pytest-cov combines them, but an aborted run leaves them behind.

Co-Authored-By: Claude Code 2.1.285 / Claude Opus 5.5 <noreply@anthropic.com>
Starts duct as a subprocess and returns once duct has logged that it is
executing, which happens right after its SIGINT handler is installed.
Lets signal tests synchronize on duct's readiness instead of sleeping.

Co-Authored-By: Claude Code 2.1.285 / Claude Opus 5.5 <noreply@anthropic.com>
A SIGINT that arrives while duct's main thread is writing to stderr runs
the handler mid-write; its own log call then raises a reentrant-write
RuntimeError, which escaped before os.kill ran, so the Ctrl-C was lost.
Act first, and drop the log message if it cannot be written.

Co-Authored-By: Claude Code 2.1.285 / Claude Opus 5.5 <noreply@anthropic.com>
Replaces sleep-then-signal in a multiprocessing child (racy under spawn,
the macOS default) with start_duct, and asserts duct's own exit code.
Reruns disabled: the test must pass without them.

Co-Authored-By: Claude Code 2.1.285 / Claude Opus 5.5 <noreply@anthropic.com>
A Ctrl-C between Popen and installing the handler raised KeyboardInterrupt
in duct and left the command running untracked. The handler is now
installed first and only counts SIGINTs until attached to the command: one
before Popen means the command is not started (exit 130), one during Popen
is forwarded as soon as the pid is known.

Co-Authored-By: Claude Code 2.1.285 / Claude Opus 5.5 <noreply@anthropic.com>
signal_ignorer.py now reports "ready" and each SIGINT it receives to a
file, so the test sends the next SIGINT only once the previous one was
forwarded (pending SIGINTs merge) instead of sleeping between them.
Reruns disabled: the test must pass without them.

Co-Authored-By: Claude Code 2.1.285 / Claude Opus 5.5 <noreply@anthropic.com>
The background marker lived 10 s, but on a slow runner (macos-15-intel
pypy, with coverage now measuring duct) the two duct runs before it is
checked took ~15 s, so it had exited. The test stops it in `finally`.

Co-Authored-By: Claude Code 2.1.285 / Claude Opus 5.5 <noreply@anthropic.com>
@asmacdo asmacdo added the semver-tests Add or improve existing tests label Oct 1, 2026
@codecov

codecov Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.77778% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 94.33%. Comparing base (7a25f42) to head (cc881e0).
⚠️ Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
src/con_duct/_signals.py 96.77% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #457      +/-   ##
==========================================
+ Coverage   92.03%   94.33%   +2.29%     
==========================================
  Files          15       15              
  Lines        1344     1376      +32     
  Branches      182      185       +3     
==========================================
+ Hits         1237     1298      +61     
+ Misses         71       48      -23     
+ Partials       36       30       -6     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

asmacdo and others added 12 commits October 1, 2026 14:53
Its lifetime is the cap on how long test_signal_kill may take from the
command starting to the third SIGINT. 10 s was the tightest cap among
the reworked tests; the others give up after 60 s. The test kills the
script long before either, so a passing run is no slower.

Co-Authored-By: Claude Code 2.1.286 / Claude Fable 5.1 <noreply@anthropic.com>
start_duct waits for duct's "is executing" line, which is logged at
INFO. With DUCT_LOG_LEVEL above INFO in the environment or a .env file
the line never comes and start_duct waits until the command exits.
Pass the level explicitly so the helper does not depend on the
developer's configuration.

Co-Authored-By: Claude Code 2.1.286 / Claude Fable 5.1 <noreply@anthropic.com>
start_duct pipes duct's stderr and nothing printed it, so "duct exited
before ..." gave no hint why. Include what duct wrote in start_duct's
RuntimeError and in _wait_for_lines' assertion message.

With it, a failing test_signal_kill shows the cause directly: a
ProcessLookupError traceback from SigIntHandler, raised inside
os.waitpid after the command had already exited.

Co-Authored-By: Claude Code 2.1.286 / Claude Fable 5.1 <noreply@anthropic.com>
pyproject.toml sets [tool.coverage.run] patch = ["subprocess", "execv"],
which coverage only understands from 7.10. Older versions warn
"Unrecognized option" and silently do not measure subprocesses (checked
with 7.9.2 and 7.10.0). Declare the floor next to pytest-cov so an old
environment fails at install time with a clear message.

Co-Authored-By: Claude Code 2.1.286 / Claude Fable 5.1 <noreply@anthropic.com>
Python runs a signal handler in the main thread at its next bytecode.
For duct that can be only after its wait for the command has returned:
when the signal's delivery did not wake the main thread, or when the
Ctrl-C lands while duct is finishing up. The handler then signalled a
pid that had already been reaped, and the ProcessLookupError escaped:
duct printed a traceback and exited 1 instead of finishing normally
with the command's exit code.

CI showed it as test_signal_kill taking exactly the command's lifetime
and duct exiting 1.

Catch it and warn: the user pressed Ctrl-C and it could not be acted
on, which they should see.

Co-Authored-By: Claude Code 2.1.286 / Claude Fable 5.1 <noreply@anthropic.com>
A signal sent to a process is delivered to any one of its threads that
does not block it, but Python only runs the handler in the main thread.
When the kernel handed a SIGINT to one of duct's other threads, the
main thread stayed asleep in its wait for the command, and the Ctrl-C
was not forwarded until the command exited by itself.

Traced with strace: in 3 of 3 such cases a worker thread took the
SIGINT, mid-run, while in 447 normal cases the main thread did. On an
idle Linux machine with the test suite's 10 ms sampling interval 11 of
1000 first SIGINTs went unforwarded; with this change 0 of 1000. At
duct's default 1 s sampling interval none did in 600 trials, so users
were less exposed than the tests.

A new thread inherits its creator's signal mask, so start_thread()
blocks SIGINT just around Thread.start(). This is the usual way to
steer signals in a threaded program, costs two system calls per thread
start and nothing while the command runs, and leaves the handler's
escalation (forward, forward, kill) as it was, since only the main
thread ever ran it. A test checks that every thread started during a
run goes through it.

Side effect: processes started by those threads inherit the block, so
the short-lived `ps` of the sampling thread cannot be interrupted by a
terminal Ctrl-C. The command is started by the main thread and its
mask is unchanged. Not tested on macOS.

What remains is a SIGINT landing in the microseconds between the main
thread's last check for signals and its blocking wait. That is a
CPython limitation of any signal.signal() plus blocking-call program;
a second Ctrl-C gets through and the pair is handled as one press.

Co-Authored-By: Claude Code 2.1.286 / Claude Fable 5.1 <noreply@anthropic.com>
Until now --fail-time was only tested through the signal tests'
parametrization, where "logs removed" silently depended on the
interrupted run finishing inside the threshold. Test the thresholds
directly, with a command that just fails: under the threshold (3, the
default, and 60) the logs are removed; a command that runs longer than
the threshold (1 s) keeps them. Only the first two assume anything
about speed, and only that `exit 1` takes less than 3 s.

Co-Authored-By: Claude Code 2.1.286 / Claude Fable 5.1 <noreply@anthropic.com>
test_signal_int and test_signal_kill ran with --fail-time unset, 0, 10,
-1 and -3.14, a list they have carried since --fail-time was added.
For the unset (3 s) and 10 cases the "logs removed" check depends on
the interrupted run finishing inside the threshold, which a slow
runner does not guarantee.

Keep the two values whose effect does not depend on how long the run
took: 0 always keeps a failed command's logs, a negative value never
does. The thresholds themselves are covered by
test_fail_time_threshold.

Co-Authored-By: Claude Code 2.1.286 / Claude Fable 5.1 <noreply@anthropic.com>
The command was `sleep 0.3`, which can exit before a slow runner takes
the first sample. until_sampled.sh instead stays alive until its own pid
appears in duct's usage file (giving up after 60 s), so the test needs no
timing assumption and exit code 0 also proves the command was sampled.

Co-Authored-By: Claude Code 2.1.285 / Claude Opus 5.5 <noreply@anthropic.com>
Its checks need at least one sample, which `sleep 0.1` did not guarantee
on a slow runner (8/8 failures under a 10% CPU cap without reruns). Use
until_sampled.sh instead and drop the rerun backoff.

Co-Authored-By: Claude Code 2.1.285 / Claude Opus 5.5 <noreply@anthropic.com>
Children slept 0.3 s and the test counted `sleep` pids, so a slow runner
could miss children entirely. Now each child records its pid in a
test-provided directory and stays alive (until_sampled.sh) until duct has
recorded it; the script itself exits only once all of them were sampled,
replacing the hold-the-door sleep. The test checks exactly those pids, not
command-line text, which `ps -w` truncates. setsid children (outside duct's
session) are killed after two more reports and must not appear.

Co-Authored-By: Claude Code 2.1.285 / Claude Opus 5.5 <noreply@anthropic.com>
…session

The setsid case failed on CI now and then: a recorded child pid showed
up in duct's samples. `setsid cmd &` forks in duct's session, and that
same pid only then execs setsid, calls setsid(2) and becomes the child.
For those few milliseconds the pid really is in the session duct
watches, so a sample landing there recorded it, and the test compares
pids over the whole run.

Have the process that calls setsid fork the recording child instead
(`sh -c '"$@"; true'`; the `; true` keeps sh from exec'ing it in
place). That child was never in duct's session, so "its pid must not
appear" is now a fair check.

The case could also pass without checking anything: the script counted
its two extra reports from the moment of the forks, so slow children
were killed before they recorded themselves, and a missing setsid
command left nothing to compare. Wait for every child's record first,
require the full count in the test, and skip the case where there is
no setsid command.

With setsid delayed 30, 50 and 200 ms the old test failed or passed
empty; this one passes.

Co-Authored-By: Claude Code 2.1.286 / Claude Fable 5.1 <noreply@anthropic.com>
@asmacdo
asmacdo force-pushed the sampling-readiness branch from 0ca78d5 to cc881e0 Compare October 1, 2026 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

semver-tests Add or improve existing tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test_spawn_children and others remains flaky on OSX Mac-15-intel is flakey and one of them often fails scheduled tests

1 participant