Conversation
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>
Codecov Report❌ Patch coverage is
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. 🚀 New features to boost your workflow:
|
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
force-pushed
the
sampling-readiness
branch
from
October 1, 2026 22:34
0ca78d5 to
cc881e0
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three tests assumed duct takes at least one sample of a short command (
sleep 0.1/sleep 0.3). On a slow runner (onepssample takes up to 0.72 s onmacos-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
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: runsuntil_sampled.shinstead ofsleep 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 (whichps -wtruncates), 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 nosetsidcommand.flakyrerun markers ontest_spawn_children,test_signal_int,test_signal_killandtest_execution_summaryare removed; no test reruns on failure any more.Verification
Local, Linux, py3.13,
pytest-rerunfailuresdisabled, whole process tree capped withsystemd-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_childrencases pass over 5 passes, and also withsetsiddelayed 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
setsidcommand, 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 execssetsid, 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 onsleepin the command line.Why one sample is slow on Intel Mac
One duct sample is
ps -axplus a session lookup per process (~500 processes on runners). Measured on CI with a diagnostics branch (single runs, no spread):sleep 0.3test_session_modes[current-session]never failed: that session includes pytest and the shell, so a sample is never empty.🤖 Generated with Claude Code