fix: resume the scheduler in stop_task only if start_task paused it - #1375
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1375 +/- ##
==========================================
+ Coverage 91.44% 91.55% +0.11%
==========================================
Files 49 49
Lines 5074 5082 +8
==========================================
+ Hits 4640 4653 +13
+ Misses 434 429 -5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
davidberenstein1957
marked this pull request as ready for review
August 13, 2026 05:05
davidberenstein1957
force-pushed
the
fix/stop-task-scheduler-resume
branch
from
August 19, 2026 09:18
3609604 to
70b7d66
Compare
davidberenstein1957
force-pushed
the
fix/stop-task-scheduler-resume
branch
from
August 19, 2026 14:29
70b7d66 to
f57ceb6
Compare
davidberenstein1957
added a commit
that referenced
this pull request
Aug 19, 2026
Adds an ASGI middleware that gives each HTTP request its share of a long-running tracker's energy, plus the attribution model behind it. One tracker runs for the app's lifetime. Each completed sampling window (t_prev, t_now, dE) is split across the requests in flight during it, weighted by their overlap with the window and normalised by the sum of the weights. Windows with nothing in flight are recorded as unattributed. The invariant attributed + unattributed == settled holds exactly after every window, and is what the concurrency test pins down. Why not per-request start/stop energy snapshots: with N requests in flight each request observes the whole machine's delta, so the sum overcounts by roughly N - measured up to 88x at 100 concurrent requests. Fair-share weighting is the only split that conserves the run total. A request's share is only known one or more sampling windows after its response was sent, so results are reported then, via a callback. A request that never covered a completed window reports energy_kwh=None rather than zero: there is no honest number for it. Tracker side: add_energy_window_observer / remove_energy_window_observer expose the sampling windows, and http_request_emissions() scales the run's EmissionsData down to one attributed share using the run's accumulated component ratios and carbon intensity. Depends on #1374 (duration int -> float in the emissions schemas, and dropping the duration < 1 send guard) and #1375 (scheduler pause handling around tasks). Both are carried by their own PRs rather than duplicated here, so this should merge after them. Deliberately left out, to keep the diff reviewable: hardware-tier gating of which backends can resolve a sampling window, include/exclude path filtering (endpoint labelling is two lines inline), idle-baseline subtraction, per-endpoint aggregation, routing per-request rows into the tracker's own CSV/API output handlers, a lifespan helper, and a dedicated docs page. Each is additive on top of this and can follow if there is demand. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
davidberenstein1957
added a commit
that referenced
this pull request
Aug 19, 2026
Adds an ASGI middleware that gives each HTTP request its share of a long-running tracker's energy, plus the attribution model behind it. One tracker runs for the app's lifetime. Each completed sampling window (t_prev, t_now, dE) is split across the requests in flight during it, weighted by their overlap with the window and normalised by the sum of the weights. Windows with nothing in flight are recorded as unattributed. The invariant attributed + unattributed == settled holds exactly after every window, and is what the concurrency test pins down. Why not per-request start/stop energy snapshots: with N requests in flight each request observes the whole machine's delta, so the sum overcounts by roughly N - measured up to 88x at 100 concurrent requests. Fair-share weighting is the only split that conserves the run total. A request's share is only known one or more sampling windows after its response was sent, so results are reported then, via a callback. A request that never covered a completed window reports energy_kwh=None rather than zero: there is no honest number for it. Tracker side: add_energy_window_observer / remove_energy_window_observer expose the sampling windows, and http_request_emissions() scales the run's EmissionsData down to one attributed share using the run's accumulated component ratios and carbon intensity. Depends on #1374 (duration int -> float in the emissions schemas, and dropping the duration < 1 send guard) and #1375 (scheduler pause handling around tasks). Both are carried by their own PRs rather than duplicated here, so this should merge after them. Deliberately left out, to keep the diff reviewable: hardware-tier gating of which backends can resolve a sampling window, include/exclude path filtering (endpoint labelling is two lines inline), idle-baseline subtraction, per-endpoint aggregation, routing per-request rows into the tracker's own CSV/API output handlers, a lifespan helper, and a dedicated docs page. Each is additive on top of this and can follow if there is demand. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
benoit-cty
force-pushed
the
fix/stop-task-scheduler-resume
branch
from
August 27, 2026 15:54
273ee24 to
c296fdd
Compare
start_task stops the periodic scheduler but nothing ever restarted it, so a tracker started with start() lost its periodic measurements after the first task. Restarting it unconditionally would instead leave a 1s scheduler running for pure start_task/stop_task users, so track whether start_task actually paused a running scheduler. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sticky flag in start_task (:755-763) — self._scheduler_paused_by_task or not self._scheduler._stopped, so a second start_task() on an already-measuring tracker can't clear it.
New _resume_scheduler_if_paused_by_task() helper (:803-812), called at both stop_task exit points — the unknown-task early return and the normal path.
I did not move the resume to the top of stop_task as I originally suggested. That would have restarted the timer before stop_task's own _measure_power_and_energy() runs, letting the scheduler thread mutate _last_measured_time/_previous_emissions concurrently with the task-delta computation — narrow at the 15s default, but real at low measure_power_secs. Calling the helper at each exit point keeps the original placement's safety and covers the early return.
Test (tests/test_emissions_tracker.py:786-802) — two cases added: double start_task, and stop_task("unknown-task"). I confirmed both fail against the pre-fix source (AssertionError: True is not false) and pass with it, so they're real regression guards rather than assertions that hold either way.
Verification: pre-commit clean on both files. Full suite: 5 failed, 644 passed, 3 skipped. The 5 failures are all in tests/test_config.py and tests/test_electricitymaps_config_backward_compatibility.py — I re-ran them with my changes stashed and got the identical 5, so they're pre-existing locally (they read your real .codecarbon.config), not caused by this work. Worth noting your PR description's "627 passed, 21 skipped, clean" doesn't match this machine's current state either way.
benoit-cty
force-pushed
the
fix/stop-task-scheduler-resume
branch
from
August 28, 2026 08:07
c296fdd to
080ed19
Compare
benoit-cty
approved these changes
Aug 28, 2026
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
2 participants
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.
Extracted from #1203 (
feat/add-fastapi-middleware), which bundled this with unrelated FastAPI middleware work. It changes behaviour for everystart_taskuser, so it deserves review on its own rather than buried in a 3,700-line feature branch. #1203 will be rebased to drop the duplicated hunks.What
start_taskstops the periodic measurement scheduler so it does not interfere with the task measurement, but nothing ever restarted it. A tracker started withstart()therefore lost its periodic measurements permanently after the first task.Restarting it unconditionally in
stop_taskwould be wrong in the other direction: users who only ever callstart_task/stop_task(neverstart()) would be left with a 1s scheduler running that nobody asked for and nothing stops.So
start_taskrecords whether it actually paused a running scheduler (_scheduler_paused_by_task = not self._scheduler._stopped), andstop_taskresumes only in that case.Behaviour change
start()+start_task()/stop_task(): periodic measurement now continues after the task, as it did before the task started. Previously it stayed dead.start_task()/stop_task()only: unchanged — no scheduler left running.Tests
tests/test_emissions_tracker.py::TestCarbonTracker::test_stop_task_resumes_scheduler_only_if_start_task_paused_itcovers both directions. It fails onmasterand passes with the fix.uv run pytest tests/ -q --ignore=tests/test_viz_data.py→ 627 passed, 21 skipped.pre-commit run --all-filesclean.🤖 Generated with Claude Code