refactor(scheduler): run ticks on one daemon thread instead of chained Timers - #1339
Open
davidberenstein1957 wants to merge 1 commit into
Open
refactor(scheduler): run ticks on one daemon thread instead of chained Timers#1339davidberenstein1957 wants to merge 1 commit into
davidberenstein1957 wants to merge 1 commit into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 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. 🚀 New features to boost your workflow:
|
davidberenstein1957
marked this pull request as ready for review
August 12, 2026 19:14
This was referenced Aug 16, 2026
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
force-pushed
the
scaling/04-scheduler-single-thread
branch
from
August 19, 2026 14:19
611879b to
1e90504
Compare
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.
Part of #1338.
PeriodicSchedulerchained a freshthreading.Timerper tick and armed the successor before running the payload. Replaced with a single daemon thread waiting on anEventagainst an absolutetime.monotonic()deadline, with a catch-up guard, in-loop exception logging, and astop()that sets the event and joins the worker.Measured, master vs. this branch
stop()(gated race repro)stop()latency @ 10s intervalThe 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) whenpowermetricsor RAPL is slow, and two concurrent mutations of_total_energy/_last_measured_timecorrupt the numbers users report. Thestop()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.
tracker.stop()can now block for up tomin(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 aponytail:comment.tracker.start_task()inherits the same bound.start_taskcallsself._scheduler.stop()(emissions_tracker.py:754-755), so it too can now block for up tomin(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.Wedged-thread handling
If the bounded join in
stop()times out,stop()keeps its reference to the thread instead of settingself._thread = None. Nulling it was unsafe:_stoppedisself._thread is None or not self._thread.is_alive(), so a forgotten-but-alive thread made the nextstart()clear_stop_eventand arm a second thread — and the original, unblocked a moment later, resumed ticking from the cleared event. Two live schedulers mutating_total_energyand_last_measured_time, which is exactly what this PR set out to prevent.Holding the reference makes
_stoppedreport the truth, sostart()stays a no-op until the wedged thread actually exits (it exits on its nextwait(), since_stop_eventis still set).stop()logs a warning when the join gives up, so the no-op is not silent.Other notes
from_runis gone rather than kept as an ignored parameter — nothing outside the removed_runever passed it, andPeriodicScheduleris not re-exported fromcodecarbon/__init__.py._stoppedis kept as a property so its two existing readers need no change.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_loopsguards the wedged-thread case: it blocks the callback, letsstop()'s join time out, callsstart(), then asserts only one thread ident ever ran the callback. Reverting the one-linestop()change fails it withtwo scheduler loops ran: {...}(verified).Full suite
633 passed, 21 skipped(excludingtests/test_viz_data.py, which fails to import on master too —dashnot installed).uv run pre-commit run --all-filespasses.Not addressed here: nothing restarts
_schedulerafterstart_taskstops it (emissions_tracker.py:754-755) — a real pre-existing bug, but a separate one.🤖 Generated with Claude Code