Skip to content

fix: resume the scheduler in stop_task only if start_task paused it - #1375

Open
davidberenstein1957 wants to merge 1 commit into
masterfrom
fix/stop-task-scheduler-resume
Open

fix: resume the scheduler in stop_task only if start_task paused it#1375
davidberenstein1957 wants to merge 1 commit into
masterfrom
fix/stop-task-scheduler-resume

Conversation

@davidberenstein1957

Copy link
Copy Markdown
Collaborator

Extracted from #1203 (feat/add-fastapi-middleware), which bundled this with unrelated FastAPI middleware work. It changes behaviour for every start_task user, 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_task stops the periodic measurement scheduler so it does not interfere with the task measurement, but nothing ever restarted it. A tracker started with start() therefore lost its periodic measurements permanently after the first task.

Restarting it unconditionally in stop_task would be wrong in the other direction: users who only ever call start_task/stop_task (never start()) would be left with a 1s scheduler running that nobody asked for and nothing stops.

So start_task records whether it actually paused a running scheduler (_scheduler_paused_by_task = not self._scheduler._stopped), and stop_task resumes 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_it covers both directions. It fails on master and passes with the fix.

uv run pytest tests/ -q --ignore=tests/test_viz_data.py → 627 passed, 21 skipped. pre-commit run --all-files clean.

🤖 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 (f57ceb6).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1375   +/-   ##
=======================================
  Coverage   91.43%   91.44%           
=======================================
  Files          49       49           
  Lines        5057     5062    +5     
=======================================
+ Hits         4624     4629    +5     
  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
davidberenstein1957 marked this pull request as ready for review August 13, 2026 05:05
@davidberenstein1957
davidberenstein1957 requested a review from a team as a code owner August 13, 2026 05:05
@davidberenstein1957
davidberenstein1957 force-pushed the fix/stop-task-scheduler-resume branch from 3609604 to 70b7d66 Compare August 19, 2026 09:18
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>
@davidberenstein1957
davidberenstein1957 force-pushed the fix/stop-task-scheduler-resume branch from 70b7d66 to f57ceb6 Compare August 19, 2026 14:29
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>
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