Repository navigation
QFw: install the service-plane manager's stop handlers before it starts - #137
Merged
Merged
Conversation
The foreground manager wrote manager_pid to its state file and printed its state, and only then installed its SIGTERM and SIGINT handlers. A caller that sees manager_pid takes the manager as up, as test_foreground_run_stops_on_sigterm does when it sends SIGTERM. A signal in between took the default action: the manager died with rc -15, stop() never ran, and its components were left running. That is the test's intermittent CI failure, 4 of the 15 failed runs since 2026-09-15. run() now installs the handlers first and restores them in a finally that covers start() too. The handlers only set the stop flag, so a signal during start-up is honoured once start() returns, which its own timeouts bound, and the plane is stopped rather than cut off partway. Before, a SIGINT there raised KeyboardInterrupt past start()'s cleanup, which catches Exception only. Two tests wrap a module function to send SIGTERM to the manager as it returns, once just after the ready report and once at the end of start(), so the window is hit every time. Both fail with rc -15 without the fix. Fixes #135 Signed-off-by: Doug Oucharek <dougso@me.com>
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 #135.
The foreground service-plane manager wrote
manager_pidto its state file and printed its state, and only then installed its SIGTERM and SIGINT handlers. A caller that seesmanager_pidtakes the manager as up, astest_foreground_run_stops_on_sigtermdoes when it sends SIGTERM. A signal in between took the default action:-15;stop()never ran, so its components were left running;That is the test's intermittent CI failure, 4 of the 15 failed QFw runs since 2026-09-15.
The fix
run()installs the handlers first, and restores them in afinallythat coversstart()too. The handlers only set the stop flag:start()returns, andstart()'s own timeouts bound that wait. The plane is then stopped cleanly.Before, a SIGINT during start-up raised
KeyboardInterrupt. That went paststart()'s cleanup, which catchesExceptiononly, so the components it had already started were left behind.Tests
Two new tests wrap a module function so the manager sends SIGTERM to itself as the function returns:
test_a_sigterm_just_after_ready_still_stops_the_planewraps_print_state, which is the race behind the CI failures;test_a_sigterm_during_start_stops_the_plane_once_it_is_upwrapsstart.The window is hit every time, so these are deterministic. Without the fix, both fail 3 of 3 runs with
assert -15 == 0, the CI signature. With it, both pass, and so doestest_foreground_run_stops_on_sigterm.In the container, on main
f2c8e93: mock 619 passed and 1 skipped, unit 130 passed,tests/qiskit35 passed, ci-syntax clean. That holds with and without the container's telemetry environment.