fix: restore signal handlers on lock release - #1329
Open
davidberenstein1957 wants to merge 3 commits into
Open
Conversation
Lock installed SIGINT/SIGTERM handlers and threw away the previous ones, so the host application's handlers were destroyed and Ctrl-C stopped raising KeyboardInterrupt. Save the previous handlers, chain to them from _handle_exit, and restore them in release(). Also unregister the atexit hook so a released lock is not pinned. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1329 +/- ##
==========================================
+ Coverage 91.39% 91.51% +0.11%
==========================================
Files 49 49
Lines 5056 5067 +11
==========================================
+ Hits 4621 4637 +16
+ Misses 435 430 -5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
_handle_exit calls release(), which takes _thread_lock; a signal delivered while the same thread is inside acquire()/release() deadlocked on a plain Lock. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
davidberenstein1957
marked this pull request as ready for review
August 12, 2026 19:14
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.
Closes #1310
What changed
codecarbon/lock.py:Lock.__init__now keeps the handlers returned bysignal.signal()inself._previous_handlersinstead of discarding them._handle_exitreleases the lock and then delegates to the handler it replaced: it calls a previous Python handler, or reproduces the default disposition withos.kill(os.getpid(), signum)when it wasSIG_DFL, or does nothing forSIG_IGN. It no longer raisesSystemExit(1)unconditionally.release()restores the saved handlers (only when the currently installed handler is still ours, so an application that registered its own afterwards is not clobbered) and unregisters theatexithook.atexitcallback is stored once asself._atexit_hook, becauseatexit.unregister()will not match a freshly-created bound method.Why
Lockis constructed wheneverallow_multiple_runs=False. Because it overwrote the process signal disposition permanently, restoring the default SIGINT handler never happened andKeyboardInterruptstopped being raised at all — everyexcept KeyboardInterrupt:in the embedding application became dead code, including CodeCarbons own incodecarbon/cli/monitor.py:95. Applications that had registered a graceful-shutdown SIGTERM handler lost it silently.Verification
Two new tests in
tests/test_lock.py(TestLockSignalHandlers):test_release_restores_previous_handlers— a sentinel SIGTERM handler is installed, aLockis built, and the sentinel (plus the original SIGINT handler) must be back afterrelease().test_signal_is_forwarded_to_previous_handler— raises a real SIGTERM and asserts the applications handler ran.Both fail on
masterand pass with this change;uv run pytest tests/test_lock.py -qis green (7 passed).uv run task format/lintreformat the whole repository with the current tool versions, so only the two touched files were formatted and checked (ruff/blackclean apart from pre-existing findings intests/test_lock.py).Ordering note
The
stop()idempotency fix should land first. Now that_handle_exitchains to the previous handler, the CLIs own SIGINT handler (codecarbon/cli/main.py) runs after the lock releases and callstracker.stop(); on a session that already stopped the tracker that becomes a secondstop(), which would turn a hijacked signal into a duplicate emissions row.Behaviour change
Previously any SIGINT/SIGTERM ended in
SystemExit(1). Now the process does whatever the application asked for. That is the intent of the fix, but anyone relying on CodeCarbon force-exiting on Ctrl-C will notice.🤖 Generated with Claude Code