Skip to content

fix: make tracker.stop() idempotent - #1336

Open
davidberenstein1957 wants to merge 1 commit into
masterfrom
fix/stop-idempotent
Open

fix: make tracker.stop() idempotent#1336
davidberenstein1957 wants to merge 1 commit into
masterfrom
fix/stop-idempotent

Conversation

@davidberenstein1957

Copy link
Copy Markdown
Collaborator

Calling stop() twice wrote a second complete emissions row (two CSV rows, two API POSTs, two handler.exit() calls) for a single run, because nothing recorded that the tracker had stopped — _start_time stays set and both schedulers being None was only used to emit an advisory warning.

What changed

  • _initialize_runtime_state() now initializes _is_stopped, final_emissions and final_emissions_data.
  • stop() returns early on _is_stopped, returning the cached final_emissions.
  • The lock release moved after the guard, so a repeat stop() no longer retries os.remove on an already-removed lock file.
  • The misplaced else: logger.warning("Tracker already stopped !") (bound to the _scheduler_monitor_power check) is replaced by the real terminal-state guard.

Why

__exit__ calls stop() unconditionally, so a with block plus an explicit stop() — the usual way to get the return value — double counts the whole run.

Verification

tests/test_emissions_tracker.py::TestCarbonTracker::test_offline_tracker_stop_is_idempotent asserts one CSV row and an identical return value after a double stop. It fails on master (2 rows) and passes with this change. Full file: 32 passed.

Closes #1307

🤖 Generated with Claude Code

@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 (3ec31a0) to head (334f76d).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1336      +/-   ##
==========================================
+ Coverage   91.43%   91.51%   +0.07%     
==========================================
  Files          49       49              
  Lines        5057     5066       +9     
==========================================
+ Hits         4624     4636      +12     
+ Misses        433      430       -3     

☔ 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
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
stop() used the schedulers as its state flag, so a second call re-ran the
final measurement, wrote a second row and released the lock twice -- by
then the lock may already belong to another tracker.

Add `_stopped_at` as the single state flag: `_start_time is None` means
never started, `_stopped_at is not None` means stopped. A second stop()
returns the memoised emissions; a start() after stop() is refused with an
error instead of half-restarting a tracker whose output handlers, lock and
schedulers are already gone.

Folds in #1337, which inferred the same state from `self._scheduler`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
davidberenstein1957 added a commit that referenced this pull request Aug 19, 2026
Handlers were installed before acquire() could fail. On the "another
instance is already running" path acquire() raises, the tracker sets
_another_instance_already_running and stop() returns at its early guard
without ever reaching release() -- so the host application's SIGINT and
SIGTERM stayed hijacked for the life of the process.

Install them after open(LOCKFILE, "x") succeeds instead, and drop the
_atexit_hook indirection: atexit.unregister() compares with ==, not
identity, so a bound method unregisters fine.

Also make release() idempotent (moved here from #1336): it edits the same
few lines of release() this branch already rewrites.

The deadlock test now unregisters its atexit hook, so a reverted lock.py
fails the suite instead of wedging the interpreter at exit.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
davidberenstein1957 added a commit that referenced this pull request Aug 20, 2026
`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()`, unregistering the atexit hook
so a released lock is not pinned.

Handlers are installed in `acquire()` after `open(LOCKFILE, "x")` succeeds,
not in `__init__`. On the "another instance is already running" path
`acquire()` raises, the tracker sets `_another_instance_already_running`, and
`stop()` returns at its early guard without ever reaching `release()` -- so
handlers installed in the constructor stayed hijacked for the life of the
process.

The thread lock is reentrant: `_handle_exit` calls `release()`, which takes
`_thread_lock`, so a signal delivered while the same thread was inside
`acquire()`/`release()` deadlocked on a plain `Lock`. `release()` is also
idempotent now (moved here from #1336, since it edits the same few lines this
branch already rewrites), and the `_atexit_hook` indirection is dropped:
`atexit.unregister()` compares with `==`, not identity, so a bound method
unregisters fine.

Tests cover the default and ignored signal dispositions, and the deadlock test
unregisters its atexit hook so a reverted lock.py fails the suite instead of
wedging the interpreter at exit.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

tracker.stop() is not idempotent: a second call writes a duplicate emissions row

1 participant