[davidberenstein1957]

Description

codecarbon/lock.py now saves the signal handlers replaced by signal.signal() in self._previous_handlers (main thread only, once the lock file is acquired) instead of discarding them. _handle_exit releases the lock and then delegates to the handler it replaced: it calls a previous Python handler, does nothing for SIG_IGN, and for the default disposition raises SystemExit(128 + signum) or KeyboardInterrupt for SIGINT, so finally / __exit__ / @track_emissions still write the final emissions. release() removes the lock file first, then restores the saved handlers, but only when the currently installed handler is still the one it set, so an application that registered its own handler afterward is not clobbered; restoring is skipped off the main thread, where signal.signal() raises, so stop() from a worker thread still writes its CSV and removes the lock file. The internal thread lock is now an RLock, since _handle_exit can call release() on a thread already holding it.

Related Issue

Fixes #1310

Motivation and Context

Lock is constructed whenever allow_multiple_runs=False. Because it overwrote the process signal disposition permanently, restoring the default SIGINT handler never happened, so KeyboardInterrupt stopped being raised at all — every except KeyboardInterrupt: in an embedding application became dead code, including CodeCarbon's own in codecarbon/cli/monitor.py. Applications that had registered a graceful-shutdown SIGTERM handler also lost it silently.

Behavior change: previously any SIGINT/SIGTERM ended in SystemExit(1). Now the process does whatever the application asked for; with no application handler, SIGTERM exits with the conventional code 143 (after cleanup) and SIGINT raises KeyboardInterrupt.

How Has This Been Tested?

tests/test_lock.py (TestLockSignalHandlers) covers: handlers restored on release, forwarding to a previous Python handler, SystemExit(143) on SIGTERM with the default disposition, SIG_IGN kept, release from a worker thread, a None previous handler, no deadlock on re-entrant release, and a failed acquire() leaving handlers alone.

Screenshots (if appropriate):

N/A

Release notes (required)

This changes public behaviour and needs a line in the release notes:

• SIGTERM with the default handler now exits with status 143 (128 + 15) instead of 1.
• Ctrl+C (SIGINT) now raises KeyboardInterrupt instead of SystemExit(1), so except KeyboardInterrupt and finally blocks in user code run.
• Handlers the application installed before CodeCarbon are called and restored.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

AI Usage Disclosure

• 🟥 AI-vibecoded
• 🟠 AI-generated
• ⭐ AI-assisted
• ♻️ No AI used

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the docs/how-to/contributing.md document.
• I have added tests to cover my changes.
• All new and existing tests passed.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.62%. Comparing base (dfefae0) to head (7cc21c2).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1329      +/-   ##
==========================================
+ Coverage   93.47%   93.62%   +0.15%     
==========================================
  Files          50       50              
  Lines        5531     5556      +25     
==========================================
+ Hits         5170     5202      +32     
+ Misses        361      354       -7     

☔ 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.

[benoit-cty]

🤖 This review comment was written and posted by Claude Opus 5.5 (AI assistant), at the request of @benoit-cty. Findings were checked by reading the code and running tests locally (merged with current master where relevant), but please double-check before acting on them.

Verdict: 🔧 Request changes

The bug is real. Lock permanently took over SIGINT/SIGTERM and always raised SystemExit(1). The fix saves the previous handlers, restores them on release, forwards the signal to them, and uses an RLock.

• Nested trackers are fine: a second lock never acquires, so it installs no handlers.
• Handlers the user sets after acquire() are kept (the getsignal check).

Must fix:

1. Calling release() off the main thread loses the final emissions and leaves the lock file behind (codecarbon/lock.py L71-75).
   • signal.signal() only works on the main thread, and release() now calls it with no thread check, so it raises ValueError.
   • stop() calls _lock.release() before saving anything. @suppress swallows the exception, so the rest of stop() never runs.
   • I verified this: with OfflineEmissionsTracker(allow_multiple_runs=False) and stop() called from a worker thread, master writes the CSV and removes the lock. With this PR, stop() returns None, no CSV is written, and the lock file stays until the process exits.
   • Stopping from a callback or worker thread is a common pattern.
   • Fix: only restore handlers when threading.current_thread() is threading.main_thread(). Remove the lock file first, and wrap the restore in try/except. Please add a test that stops from a worker thread.
2. SIGTERM now kills the process with no final write (lock.py L42-43).
   • When the previous SIGTERM handler was SIG_DFL, the PR restores it and re-sends the signal with os.kill. The process then dies immediately: no finally, no __exit__, no tracker.stop().
   • On master, SystemExit(1) let with / @track_emissions write the final emissions. I verified this: master exits with code 1 and finally runs; the PR exits with 143 and finally does not run.
   • On SLURM/Kubernetes shutdowns this loses the final row that users get today.
   • Suggested fix: forward only to callable previous handlers. When the previous handler is SIG_DFL, raise SystemExit(128 + signum) (or KeyboardInterrupt for SIGINT), so cleanup runs and the exit code stays conventional.

Nits:

• If the previous handler was installed from C code, signal.getsignal() returns None. release() then calls signal.signal(sig, None), which raises TypeError, and _handle_exit swallows the signal. Treat None as SIG_DFL.
• The description refers to an _atexit_hook that isn't in the code, and the ordering note about stop idempotency is already covered by fix: make tracker.stop() idempotent #1408. Please update it.

[davidberenstein1957]

Made the changes in f132178: worker-thread release and SIGTERM cleanup fixed.

• release() removes the lock file first and only restores handlers on the main thread (new worker-thread test).
• A default previous handler now raises SystemExit(128 + signum) / KeyboardInterrupt instead of os.kill, so finally and the final CSV row still happen (checked: exit 143, row written).
• A None previous handler is restored as SIG_DFL. Updated the description too.

[benoit-cty]

Thanks, both points from the previous review are fixed:

• stop() from a worker thread now writes the CSV and removes the lock file.
• SIGTERM with the default handler now exits with code 143 after finally has run.

I checked this end to end with an OfflineEmissionsTracker(allow_multiple_runs=False).

I found three more problems in the same area. All three remove the lock file, or skip the application's handler, while the run is still going. I added one commit per problem, each with a test that fails on the current branch:

1. An ignored signal removes the lock file and the tracker keeps running (4b9e24b)

• acquire() installs _handle_exit even when the signal was SIG_IGN.
• A common case is SIGINT for a background job started from a non-interactive shell (python train.py & inside a script).
• When that signal arrives, _handle_exit calls release(): the lock file is deleted, and then the handler returns because the signal was ignored. The process keeps tracking with no lock, so a second CodeCarbon instance can start.
• Fix: do not install a handler over SIG_IGN at all. The SIG_IGN branch in _handle_exit is no longer needed, so I removed it. test_ignored_signal_stays_ignored now checks that the lock is kept.

2. A graceful-shutdown handler loses the lock as soon as it is called (d7d66d9)

• _handle_exit released the lock, and restored the handlers, before calling the previous handler.
• Your PR description gives the typical case: an application handler that sets a should_stop flag and returns. The app then finishes its current step while the tracker is still measuring, but the lock file is already gone.
• Fix: call the previous handler first, and release only if it raises (KeyboardInterrupt, SystemExit, …), then re-raise. If the handler returns, the run goes on and stop() / atexit release the lock later. Our handler stays installed, so a second signal is forwarded too.
• Tests: test_signal_is_forwarded_to_previous_handler was updated, and test_previous_handler_that_exits_releases_the_lock is new.

3. A signal during release() skips the application's handler (242cb14)

• _restore_signal_handlers used popitem() before calling signal.signal().
• If a signal arrives in that window, which is likely for a SIGTERM during stop(), _handle_exit no longer finds the saved handler. It then falls back to SIG_DFL, so the application's handler is skipped and the process gets SystemExit(143) / KeyboardInterrupt instead.
• Fix: restore each handler first, then remove it from _previous_handlers, with pop(sig, None) so a re-entrant release is safe.
• Test: test_signal_during_restore_is_forwarded.