Skip to content

fix: restore signal handlers on lock release - #1329

Open
davidberenstein1957 wants to merge 3 commits into
masterfrom
fix/lock-signal-handler-restore
Open

fix: restore signal handlers on lock release#1329
davidberenstein1957 wants to merge 3 commits into
masterfrom
fix/lock-signal-handler-restore

Conversation

@davidberenstein1957

Copy link
Copy Markdown
Collaborator

Closes #1310

What changed

codecarbon/lock.py:

  • Lock.__init__ now keeps the handlers returned by signal.signal() in self._previous_handlers instead of discarding them.
  • _handle_exit releases the lock and then delegates to the handler it replaced: it calls a previous Python handler, or reproduces the default disposition with os.kill(os.getpid(), signum) when it was SIG_DFL, or does nothing for SIG_IGN. It no longer raises SystemExit(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 the atexit hook.
  • The atexit callback is stored once as self._atexit_hook, because atexit.unregister() will not match a freshly-created bound method.

Why

Lock is constructed whenever allow_multiple_runs=False. Because it overwrote the process signal disposition permanently, restoring the default SIGINT handler never happened and KeyboardInterrupt stopped being raised at all — every except KeyboardInterrupt: in the embedding application became dead code, including CodeCarbons own in codecarbon/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, a Lock is built, and the sentinel (plus the original SIGINT handler) must be back after release().
  • test_signal_is_forwarded_to_previous_handler — raises a real SIGTERM and asserts the applications handler ran.

Both fail on master and pass with this change; uv run pytest tests/test_lock.py -q is green (7 passed). uv run task format/lint reformat the whole repository with the current tool versions, so only the two touched files were formatted and checked (ruff/black clean apart from pre-existing findings in tests/test_lock.py).

Ordering note

The stop() idempotency fix should land first. Now that _handle_exit chains to the previous handler, the CLIs own SIGINT handler (codecarbon/cli/main.py) runs after the lock releases and calls tracker.stop(); on a session that already stopped the tracker that becomes a second stop(), 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

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

codecov Bot commented Aug 12, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.51%. Comparing base (065d0e6) to head (a91020f).

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.
📢 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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

davidberenstein1957 and others added 2 commits August 12, 2026 17:43
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
davidberenstein1957 marked this pull request as ready for review August 12, 2026 19:14
@davidberenstein1957
davidberenstein1957 requested a review from a team as a code owner August 12, 2026 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Lock permanently replaces the host application's SIGINT/SIGTERM handlers

1 participant