Skip to content

refactor(scheduler): run ticks on one daemon thread instead of chained Timers - #1339

Open
davidberenstein1957 wants to merge 1 commit into
masterfrom
scaling/04-scheduler-single-thread
Open

refactor(scheduler): run ticks on one daemon thread instead of chained Timers#1339
davidberenstein1957 wants to merge 1 commit into
masterfrom
scaling/04-scheduler-single-thread

Conversation

@davidberenstein1957

@davidberenstein1957 davidberenstein1957 commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Part of #1338.

PeriodicScheduler chained a fresh threading.Timer per tick and armed the successor before running the payload. Replaced with a single daemon thread waiting on an Event against an absolute time.monotonic() deadline, with a catch-up guard, in-loop exception logging, and a stop() that sets the event and joins the worker.

Measured, master vs. this branch

master branch
overlapping entries (payload 2.4x interval, ~18 calls) 17 0
extra calls after stop() (gated race repro) 8, and never stops 0
drift per tick @ 1s +4.3 ms (0.4%) +0.1 ms
distinct worker thread idents over 30 ticks 2 1
live threads during a run 3 2
stop() latency @ 10s interval 0.0 ms 0.1 ms

The case for merging is the first two rows. Re-entrancy needs no race window — just a measurement slower than the interval, which is routine for _scheduler_monitor_power (1s, hard-coded) when powermetrics or RAPL is slow, and two concurrent mutations of _total_energy / _last_measured_time corrupt the numbers users report. The stop() race is unlikely per tick but unbounded in consequence: measurements and API pushes continue after the tracker has written its final row.

Two arguments against overselling this, both from the same measurements: thread count never grows (each Timer dies as its successor starts), and drift is 0.4%, invisible because energy is computed from measured elapsed time. Neither would justify the change alone.

⚠️ Behaviour changes (no CHANGELOG file exists in this repo, so recording them here)

  1. tracker.stop() can now block for up to min(measure_power_secs, 5.0) seconds. It previously returned immediately. Normally the wait is ~0.1 ms; the bound is only reached if a measurement is genuinely wedged. That wait is the fix for "stop() returns while the callback is still running", but it is the change most likely to be noticed in the wild. The 5s cap is a judgement call and carries a ponytail: comment.
  2. tracker.start_task() inherits the same bound. start_task calls self._scheduler.stop() (emissions_tracker.py:754-755), so it too can now block for up to min(measure_power_secs, 5.0) where it previously returned at once. Same normal-case cost (~0.1 ms); worth knowing for callers that start many short tasks in a loop.
  3. A slow callback now yields missing measurements instead of overlapping ones, so the existing "Background scheduler didn't run for a long period" warning will fire where master silently produced corrupted concurrent measurements.
  4. A wedged scheduler now refuses to restart rather than double-starting. See below.

Wedged-thread handling

If the bounded join in stop() times out, stop() keeps its reference to the thread instead of setting self._thread = None. Nulling it was unsafe: _stopped is self._thread is None or not self._thread.is_alive(), so a forgotten-but-alive thread made the next start() clear _stop_event and arm a second thread — and the original, unblocked a moment later, resumed ticking from the cleared event. Two live schedulers mutating _total_energy and _last_measured_time, which is exactly what this PR set out to prevent.

Holding the reference makes _stopped report the truth, so start() stays a no-op until the wedged thread actually exits (it exits on its next wait(), since _stop_event is still set). stop() logs a warning when the join gives up, so the no-op is not silent.

Other notes

  • from_run is gone rather than kept as an ignored parameter — nothing outside the removed _run ever passed it, and PeriodicScheduler is not re-exported from codecarbon/__init__.py. _stopped is kept as a property so its two existing readers need no change.
  • The new tests use real sleeps at a 0.05s interval. Stable here, but they are wall-clock tests on shared runners; if they flake, the right fix is injecting a clock, not loosening tolerances.
  • Dropped from the original proposal: a drift-assertion test (it flaked under full-suite load while passing standalone, and 0.4% drift isn't worth guarding) and a frozen-clock test for the three-line catch-up guard.

Tests

New tests/test_scheduler.py, 7 tests, 7 passed. Against master's scheduler, 4 of them fail. Three are fail-before/pass-after for the threading rewrite: test_ticks_run_on_a_single_thread, test_slow_function_is_never_re_entered, test_start_is_idempotent_and_restartable.

test_start_after_a_timed_out_stop_does_not_run_two_loops guards the wedged-thread case: it blocks the callback, lets stop()'s join time out, calls start(), then asserts only one thread ident ever ran the callback. Reverting the one-line stop() change fails it with two scheduler loops ran: {...} (verified).

Full suite 633 passed, 21 skipped (excluding tests/test_viz_data.py, which fails to import on master too — dash not installed). uv run pre-commit run --all-files passes.

Not addressed here: nothing restarts _scheduler after start_task stops it (emissions_tracker.py:754-755) — a real pre-existing bug, but a separate one.

🤖 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.44%. Comparing base (3ec31a0) to head (1e90504).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1339      +/-   ##
==========================================
+ Coverage   91.43%   91.44%   +0.01%     
==========================================
  Files          49       49              
  Lines        5057     5064       +7     
==========================================
+ Hits         4624     4631       +7     
  Misses        433      433              

☔ 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

Copy link
Copy Markdown
Collaborator Author

Note for whoever reviews this: #1324 fixes the same scheduler defect by a different route. It is a narrow fix to the existing chained-Timer design, while this PR replaces the design with a single daemon thread. Worth picking one before reviewing either, otherwise they conflict on merge.

…d Timers

Every tick spawned a fresh threading.Timer and the next one was armed
before the callback ran, so a callback slower than the interval overlapped
with itself and the thread count grew with the run.

Run the loop on a single daemon thread waiting on an Event, with an
absolute deadline so the cadence does not drift and an overrun skips ahead
instead of firing catch-up ticks. Each run owns its Event, so a thread left
behind by a timed-out stop() keeps its own set event and exits after its
callback returns, while start() takes effect immediately on a new thread.

Note: stop() now blocks up to min(interval, 5.0) waiting for the in-flight
call. start_task() calls _scheduler.stop() (emissions_tracker.py:755), so
start_task() becomes potentially multi-second where it used to be instant.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@davidberenstein1957
davidberenstein1957 force-pushed the scaling/04-scheduler-single-thread branch from 611879b to 1e90504 Compare August 19, 2026 14:19
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.

1 participant