Repository navigation
Fix two races behind the flaky pipeline job-control test - #1794
Open
tleonhardt wants to merge 2 commits into
Open
tleonhardt wants to merge 2 commits into
tleonhardt wants to merge 2 commits into
Conversation
test_pipeline_stops_with_cmd2_and_returns_terminal sent Ctrl-Z as soon as the pipeline owned the terminal. On macOS the pipeline can still be stopped then: when cmd2 takes the terminal back between lends, a pager blocked reading it gets SIGTTIN for its whole group, and the pipeline's watcher continues the group only once it next runs after the following lend. A Ctrl-Z in that gap is discarded by the processes already stopped, and the watcher's SIGCONT then undoes it for a shell producer, so cmd2 never suspends and the test times out waiting for the outer shell to get the terminal back. Wait until the consumer and pager are running before each Ctrl-Z. Refs #1791
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1794 +/- ##
=======================================
Coverage 99.75% 99.75%
=======================================
Files 23 23
Lines 6574 6574
=======================================
Hits 6558 6558
Misses 16 16
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
The orphaned-job-control variant intermittently timed out waiting for the first PAGER_INTERRUPT. The simulated pager handles SIGINT in Python while blocking in os.read(). A signal that arrives after Python last checked for one, but before the read() system call starts, is recorded without interrupting that read, so the handler only runs once the next key arrives. The test sends Ctrl-C as soon as it sees PAGER_ALIVE, which the pager writes just before it reads again. Wait for keys in select() on the terminal and a signal.set_wakeup_fd() pipe instead. Every handled signal makes the pipe readable, so select() returns even when the signal lands just before it. Fixes #1791
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.
Fixes #1791.
The two failures in the issue had different causes. Both were races in the test, and both reproduce
locally only under CI-like load. Each commit fixes one.
1.
[wrapper-stop-and-interrupt-busy-shell-producer]: Ctrl-Z sent to a stopped pipelineThe test sent Ctrl-Z as soon as the pipeline's group owned the terminal. On macOS the pipeline can
still be stopped at that point. A trace of a failing run showed this sequence:
PAGER_READYduring the start-gate lend and blocks reading the terminal.finds it in the background, and sends SIGTTIN to the whole pipeline group, so the bash wrapper
stops along with the pager.
do_shell()lends the terminal again. The group is now the foreground but still stopped, and thewatcher thread only continues it once it next runs. Under load that was over 200ms after the lend.
busyprintedBUSY_READYand the test sent Ctrl-Z. The bash wrapper and pager werealready stopped and discarded the SIGTSTP. Only
busystopped.busy. The main thread's next pollof
busysaw onlycontinued, so nothing relayed a suspension. That SIGCONT is thePAGER_RESUMEDin the CI transcripts.Fix: before each Ctrl-Z, wait until the consumer and pager are running (no
Tstate inps), notjust until they own the terminal. A new
running()helper shares astates()helper withstopped().2.
[orphaned-job-control]: Ctrl-C handled only on the next keyThe test timed out waiting for the first
PAGER_INTERRUPT. The terminal had ISIG on, the foregroundwas the pager's group, and the pager had nothing pending or blocked. The simulated pager handles
SIGINT in Python while blocking in
os.read(). A signal that arrives after Python last checked forone, but before the
read()system call starts, is recorded without interrupting that read, so thehandler only runs once the next key arrives. The test sends Ctrl-C as soon as it sees
PAGER_ALIVE,which the pager writes just before it reads again.
To confirm this, I made each timeout send the stuck pager a plain
x(no signal). In all 7 failuresthat made its SIGINT handler run, so the Ctrl-C had been delivered and was waiting on that read.
Fix: the pager waits for keys in
select()on the terminal and asignal.set_wakeup_fd()pipe.Every handled signal makes the pipe readable, so
select()returns even when the signal lands justbefore it. The pager still reads the terminal itself, so it still stops with SIGTTIN when it reads
from the background.
Testing
Each race was stress-tested with coverage on (which also traces the cmd2 subprocess) and 32 to 64
copies of the test running at once. Each copy had its own
COVERAGE_FILEand--basetemp. Alltemporary instrumentation used during the investigation has been removed;
cmd2/is unchanged.wrapper-stop-and-interrupt-busy-shell-producerorphaned-job-controlThe "after" count for the first variant combines 960 runs before and 960 after the second fix. For
the first race, temporary sleeps that delayed the watcher's SIGCONT made the old test fail 16/16 with
the CI signature; the fixed test passed 16/16.
make check,make test(2838 passed, 3 skipped), andmake docs-testpass locally. Coverage isunchanged.
Notes
pipeline stopped for terminal access is lost. For someone typing, that gap is normally well under a
millisecond; it only grows to hundreds of milliseconds on a heavily loaded machine. Closing it in
the library would mean continuing the pipeline as part of the lend, the way a shell's
fgdoes,which needs a restructure of the watcher's loop. That's left for a possible follow-up.
PermissionErrorfromos.killpg(pipeline_group, SIGKILL), because the pager was just killed and is an unreaped zombie(macOS answers EPERM). The timeout is still shown first, but the remaining cleanup is skipped. I
left this alone because it only affects failure reports.