Skip to content

scheduler: correctness and robustness fixes from two live nights - #255

Merged
phsilva merged 13 commits into
astroufsc:masterfrom
wschoenell:fix/scheduler-altaz-and-lost-start
Jul 29, 2026
Merged

scheduler: correctness and robustness fixes from two live nights#255
phsilva merged 13 commits into
astroufsc:masterfrom
wschoenell:fix/scheduler-altaz-and-lost-start

Conversation

@wschoenell

@wschoenell wschoenell commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Scheduler fixes found operating the LNA 0.4 m robotically across 2026-07-21/22. All are reachable on current master; every fix carries a test verified to fail without it.

Commit 1 — alt/az pointing, lost START, duplicate runs

point with alt/az always crashed. Position.alt/az are floats (Coord.deg) while .ra/.dec are Coords; .to_d() on all four raised 'float' object has no attribute 'to_d' on every alt/az point (chimera-robobs parks with one, so it failed whenever its queue emptied).

A --start during executor.stop() was discarded by the unconditional state(OFF) after the blocking stop.

The same program could run several times concurrently. START overwrites BUSY; the machine re-entered IDLE, next(scheduler) returned the same unfinished program, and _process forked another thread — five concurrent autofocus runs and four concurrent sky flats on one camera, presenting as an endless focus loop.

Commit 2 — worker exit, cancellable waits, bounded abort, time order

Lost wakeup: the duplicate guard slept on the condition variable, but the worker signals IDLE from inside itself just before exiting — the wakeup was consumed before the sleep and the machine parked forever. It now polls the worker (join(1.0)).

Frozen state machine during aborts: executor.stop() blocks until the running action gives up (280 s observed for a camera readout) and ran on the machine thread. The abort now runs on its own thread bounded by STOP_ABORT_TIMEOUT (30 s); the abort itself continues in the background.

Uninterruptible pre-slew waits: a program waiting for a future start_at sat in time.sleep(); --stop could not reach it (no current action to abort) — a focus queued for 07:50 held the machine 90 minutes. The wait is now a cancellable event; STOP/SHUTDOWN release it and the program completes ABORTED.

Priority-only ordering starved the night: SequentialScheduler ordered by desc(priority) with no regard for start_at, so a high-priority program timed for dawn was picked at 22:00 and waited 10 h with the whole night queued behind it. Execution now follows start_at (priority breaks ties); programs without start times keep the old ordering.

Inline tracking stop (stop_tracking_on_program_end, default on): stopping tracking at program end used to live in the robobs plugin on a detached thread; stop_tracking() is locked, so the stale stop queued behind the next program's slew and untracked the freshly acquired target (TheSkyX re-enables tracking at slew end). It now runs inline on the program thread, ordered before the next program can start.

Tests

tests/chimera/controllers/ — one test per fix; each was checked against a reverted copy of its own source file. Notable reproductions: program forked 6 concurrent executions, machine parked after the worker exited, the START was not acted on during the abort, morning runs blockid 2 before 1.

Known limitation

STOP_ABORT_TIMEOUT bounds how long the machine is deaf during an abort, but after the timeout the machine reports OFF while the abort still runs — an honest STOPPING state is follow-up work. Resolved by the 2026-07-25 update below.

Update 2026-07-23: adds the stale-queue-entry fix from the third live night: reschedule() re-enqueues the currently RUNNING program (its finished flag is only written at completion), and the pop replayed it once it finished — a focus ran twice back to back. next() now re-checks each popped entry against the database and the worker commits the finished flag before the IDLE transition.

Update 2026-07-23 (2): a program past its valid_for window was reported complete ("not valid anymore") and then executed anywayprogram_begin fired, the actions ran, and a second contradictory program_complete landed at the end (robobs advances its queue on the first one). The expired program is now finished without executing; programs with valid_for < 0 keep running late as before. Test: test_scheduler_expired_program.py, verified to fail without the fix.

Update 2026-07-25 — OFF only when the worker is dead. A live escalation on the fourth night (three stops survived, 154 dark frames on a closed dome over 52 min) showed the remaining gap: OFF was still declared with the worker alive, and when the surviving program finally died with an error its completion path flipped the stopped machine back to IDLE, re-arming the night. This update replaces the STOP_ABORT_TIMEOUT give-up with the honest STOPPING state: STOP aborts off-thread, joins the worker with a heartbeat log line, and only then reports OFF; terminal transitions from the worker are compare-and-set on BUSY, so a program's own completion or error can never re-arm a stopped machine. executor.stop() now sets must_stop even between actions and execute() clears its current handler on exit (a later stop cannot abort an action of a finished program); SequentialScheduler.done() survives the queue being rebuilt underneath it. Tests: an end-to-end rig (real bus, fake camera) replaying the incident sequence — start, re-start during BUSY, stop — asserting the camera stops and stays stopped after OFF, plus unit tests for worker-death-gated OFF and the sticky transition. The suite's scheduler database is now redirected to a temp file at conftest import (the model engine binds at import time; a full-suite run used to write into the user's real ~/.chimera/scheduler.db).

Update 2026-07-28 — a deleted program row must not kill the program thread. Aborting a program whose row was deleted while it ran killed the scheduler-program thread, and with it the completion event the rest of the stack waits for:

machine.py:293  log.debug(f"[aborted by user] {str(task)}")
ObjectDeletedError: Instance '<Program ...>' has been deleted

The abort itself completed correctly; the casualty was the logging. session.commit() expires every instance, so str(task) after it reloads the row — and the row is routinely gone by then: robobs cleans the chimera queue on start and on stop, and a replan rebuilds it. Every operator lock during a program hit this on opd-40 2026-07-28 (the abort landed 391 s after the delete, so it is not a tight race). Three defences: Program.__str__ falls back to the ORM identity instead of raising (a __str__ that can raise is a trap wherever it is used, and every scheduler log line formats a task); the program is described once, up front, while its row is certainly there; and the terminal bookkeeping is wrapped so a failure to record the end of a program cannot cost the completion event or the state transition. Test: test_scheduler_deleted_program.py (both halves verified to fail without the fix). The robobs-side half — the queue clean no longer deletes the row of the program the scheduler is executing — is in astroufsc/chimera-robobs#3.

Two smaller items from the same nights, in a second commit:

  • Heartbeat. The machine waited in silence while a program ran (BUSY sleeps on the wake-up Condition; a worker counting down to its slew time or running a 10 min autofocus prints nothing), and twice that silence was diagnosed as "the scheduler is wedged" from the log alone. It now reports what it is running, for how long, and what it is waiting for, once a minute. The BUSY wait is bounded to make room for it; the Condition is still notified on every state change, so a STOP lands as immediately as before.
  • File order for untimed queues. A hand-written chimera-sched --new queue (no start_at, one priority) ran in whatever order the database returned — in practice the reverse of the YAML. Insertion order is now the last tie-break.

Two independent faults, both hit in production on the LNA 0.4 m tonight.

PointHandler crashed on every alt/az point action with "'float' object
has no attribute 'to_d'". Position.alt/az return floats (they are
Coord.deg) while .ra/.dec return Coords, so the .to_d() applied to all
four is only correct for the ra/dec pair. The ra/dec path is the common
one, which is why this survived; robobs parks with an alt/az point, so
it failed every time its queue emptied.

Machine discarded a START requested while executor.stop() was running.
That call blocks until the running action gives up - for a camera the
rest of the exposure plus its readout, minutes - and the machine thread
is inside it, not reading the state. A chimera-sched --start in that
window only set the variable, and the unconditional state(OFF) after the
stop then threw it away: the scheduler stayed dead and the CLI appeared
to hang. Fall through to OFF only if nothing else was requested.

Both are covered by tests that fail without the corresponding fix.
A start() arriving while a program was executing forked a second
execution of it. START overwrites BUSY, so the machine dropped back
through IDLE, next(scheduler) handed back the SAME program - still
unfinished, so still first in the queue - and _process spawned another
thread for it.

chimera-robobs calls scheduler.start() once per program it queues, so
this compounds fast. Seen on the LNA 0.4 m: five concurrent autofocus
runs and four concurrent sky flats racing on one camera, all surfacing
together at shutdown as repeated "[error] astroufsc#4 ... (Error while
autofocusing)" for a single program id. From the outside it looks like
an endless focus loop.

Keep a handle on the worker thread and stay BUSY while it is alive.
@wschoenell wschoenell changed the title scheduler: fix alt/az pointing and a START lost during stop scheduler: fix alt/az pointing, a lost START, and duplicate concurrent runs Jul 21, 2026
…e order

Four further machine fixes and the inline tracking stop, all from live
nights on the LNA 0.4 m (2026-07-22), each with a discriminating test:

- The duplicate guard slept on the condition variable, but the worker
  signals IDLE from inside itself just before exiting - the wakeup was
  consumed before the sleep and the machine parked forever. Poll the
  worker (join 1s) instead.
- executor.stop() blocks until the running action gives up (280 s for a
  camera readout) and ran on the machine thread: the state machine froze
  and a --start during the abort was invisible. The abort now runs on
  its own thread bounded by STOP_ABORT_TIMEOUT.
- A program waiting for a future slew_at sat in an uninterruptible
  time.sleep(); --stop could not reach it (no current action to abort).
  The wait is a cancellable event; STOP/SHUTDOWN release it and the
  program completes ABORTED.
- SequentialScheduler ordered by priority alone, so a high-priority
  program with a future start_at was picked hours early and waited on
  the machine with the whole queue behind it. Execution follows
  start_at; priority breaks ties; programs without start times keep the
  old ordering.
- The end-of-program tracking stop moves here from the robobs plugin,
  inline on the program thread (stop_tracking_on_program_end, default
  on): the detached-thread stop raced the next program's slew and
  untracked the freshly acquired target.
@wschoenell wschoenell changed the title scheduler: fix alt/az pointing, a lost START, and duplicate concurrent runs scheduler: correctness and robustness fixes from two live nights Jul 22, 2026
reschedule() rebuilds the run queue from every unfinished row - including
the program RUNNING at that moment, whose finished flag is only written at
completion. robobs hands the night over one program at a time and each
hand-over triggers a reschedule, so the queue routinely held a stale entry
for the running program; when it finished, the pop replayed it (2026-07-23
02:21: the SAO 189035 focus ran twice back to back, slew and all).

next() now re-checks each popped entry against the database and skips
finished rows, and the worker commits the finished flag right after
scheduler.done() - before tracking stop, completion event and the IDLE
transition - so the check cannot read a stale flag while the machine is
being released.
Comments trimmed to the why; the live-night detail already lives in the
PR description and commit messages.

Remove the time_speedup() wait scaling and the autoguider config key:
both belong to feature/fast-forward-clock-and-autoguide. On master
Site.time_speedup does not exist, so the call raised and fell back to
1.0 on every timed program.

sequential: use SQLAlchemy 1.4 session.get() instead of legacy
Query.get().
The expired branch reported program_complete(OK, 'not valid anymore')
and then fell through to run the program anyway: program_begin fired,
the actions executed, and a second contradictory program_complete
landed at the end. robobs advances its queue on the first completion,
so it raced a program that was in fact still running.

The expired program is now marked done, committed, and completed
without executing, releasing the machine to IDLE like every other
exit. Programs with valid_for < 0 (no window) keep running late, as
before. Also fixes the missing f-prefix on the expiry log line by
switching it to lazy %-formatting.
Reconcile the slew-time wait with the fast-forward Site clock (astroufsc#253):
keep master's site.mjd() poll loop on the wake-up Condition and drop the
PR's redundant _cancel_wait Event, but preserve this branch's behaviour of
reporting the program ABORTED when a STOP/SHUTDOWN cuts the wait short, plus
the expired-program branch that finishes a stale program without running it.
wschoenell added a commit to wschoenell/chimera that referenced this pull request Jul 25, 2026
…stroufsc#253 over master

Audit follow-up. Reset every file that was diverging from upstream/master
without an open PR back to master, per review decisions:
- imagerequest.py: revert the dome-sync blast-radius containment (astroufsc#261
  closed unmerged - a dome that cannot sync SHOULD fail the program);
- DomeWindScreen + sextractor source-extractor fix: moved to their own
  draft PRs (astroufsc#267, astroufsc#268), dropped here (not needed on opd-40);
- bus.py _outbound_dead vestige: dead after the merged bus refactor;
- cli/guide.py: superseded by cli/guider.py (merged astroufsc#246);
- misc core (constants, manager, proxy, fakes, old tests, AGENTS): master.

Deploy branch now = upstream/master + astroufsc#255 + astroufsc#259 + astroufsc#253 content only.
wschoenell added a commit to wschoenell/chimera that referenced this pull request Jul 25, 2026
astroufsc#253/astroufsc#259 merged upstream: their files track master again; deploy delta
is now astroufsc#255 (scheduler) only. Slew-wait resolved to master's merged
fast-forward-aware polling abort, dropping astroufsc#255's superseded speedup
scaffolding; astroufsc#255's worker guard, bounded STOP abort and stale-entry
guard retained.
The opd-40 2026-07-25 escalation (lna40 astroufsc#19): the machine declared OFF
while an aborting exposure loop survived, and when that program finally
died with an error its completion path flipped the stopped machine back
to IDLE. STOP now moves to a STOPPING state, runs the abort off-thread,
and joins the worker with a heartbeat before declaring OFF; terminal
transitions from the worker are compare-and-set on BUSY so a program's
own completion or error can never re-arm a machine that was told to
stop. executor.stop() sets must_stop even with no action in flight and
execute() clears its current handler on exit, so a later stop cannot
abort an action of a finished program. SequentialScheduler.done()
survives the queue having been rebuilt underneath it.

Replaces the 30 s STOP_ABORT_TIMEOUT give-up: waiting on the stopper
thread bounded the wait for the abort *call*, but OFF was still declared
with the worker alive - the machine's word is now tied to the only thing
that matters, worker death.

Tests: an end-to-end stop regression rig (fake camera, real bus) covering
the incident sequence, unit tests for worker-death-gated OFF and the
sticky terminal transition, and the scheduler database redirected to a
temp file at conftest import time - the model engine binds at import, and
a suite-wide run used to write into the user's real ~/.chimera database.
reschedule() rebuilds run_queue and re-enqueues the still-running program,
so a program's slot could be task_done()'d twice -- once by done(), once by
__next__ when it pops the now-finished stale entry. __next__'s call was
unguarded and, once the counter went negative, raised "task_done() called
too many times", killing the machine thread mid-night (seen on opd-40).

The queue is only ever used as a thread-safe FIFO -- nothing join()s it --
so the task_done() bookkeeping served no purpose. Drop it entirely from both
__next__ and done() and note why, and add a single-program regression that
drove the counter negative under the old code.
…az-and-lost-start

# Conflicts:
#	tests/chimera/conftest.py
@codecov

codecov Bot commented Jul 28, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 80.51948% with 30 lines in your changes missing coverage. Please review.
✅ Project coverage is 54.48%. Comparing base (5599e42) to head (1c5b2c9).

Files with missing lines Patch % Lines
src/chimera/controllers/scheduler/machine.py 78.57% 21 Missing and 3 partials ⚠️
src/chimera/controllers/scheduler/executor.py 76.92% 4 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #255      +/-   ##
==========================================
+ Coverage   51.99%   54.48%   +2.48%     
==========================================
  Files         102      102              
  Lines        8626     8720      +94     
  Branches      999     1008       +9     
==========================================
+ Hits         4485     4751     +266     
+ Misses       3933     3737     -196     
- Partials      208      232      +24     

☔ 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.

Aborting a program whose row was deleted while it ran killed the
scheduler-program thread — and with it the completion event the rest of
the stack waits for:

    machine.py:293  log.debug(f"[aborted by user] {str(task)}")
    ObjectDeletedError: Instance '<Program ...>' has been deleted

The abort itself had completed correctly; the casualty was the LOGGING.
`session.commit()` expires every instance, so `str(task)` after it is not
reading cached values - it reloads the row, and the row is routinely gone
by then: robobs cleans the chimera queue on start AND on stop, and
plan_robobs rebuilds it. Every operator lock during a program hit this on
opd-40 2026-07-28 (the abort landed 391 s after the delete, so this is not
a tight race).

Three defences, in order of generality: `Program.__str__` falls back to
the ORM identity instead of raising - a __str__ that can raise is a trap
wherever it is used, and every scheduler log line formats a task; the
program is described ONCE, up front, while its row is certainly there;
and the terminal bookkeeping is wrapped so a failure to record the end of
a program cannot cost the completion event or the state transition. The
program is over by then - the machine must survive to pick up the next
one.
…timed queues

Two smaller papercuts from the same nights:

* the machine waits in silence while a program runs (BUSY sleeps on the
  wake-up Condition; a worker counting down to its slew time or running a
  10 min autofocus prints nothing), and twice that silence was diagnosed
  as 'the scheduler is wedged' from the log alone. It now says what it is
  running, for how long, and what it is waiting for, once a minute. The
  BUSY wait is bounded to make room for it; the Condition is still
  notified on every state change, so a STOP lands as immediately as
  before.

* an untimed hand-written queue ('chimera-sched --new', no start_at, one
  priority) ran in whatever order the database returned - in practice the
  reverse of the YAML. Insertion order is now the last tie-break, so a
  file runs in the order it was written.
wschoenell added a commit to wschoenell/chimera-bisque that referenced this pull request Jul 29, 2026
The slew and homing loops slept through their poll interval, so an
abort_slew() was only noticed on the next tick. self._abort is already a
threading.Event, so wait on it: the abort releases the poll at once and
the poll interval stops being a floor on abort latency.

Same idiom astroufsc/chimera#255 applies to the scheduler's pre-slew
wait, where an uninterruptible time.sleep() held the machine for 90
minutes because --stop could not reach it.

At the default 0.1 s interval this buys little, but it stops the interval
from being a latency knob traded against TheSkyX socket traffic -- a
2 s poll would have made every abort take up to 2 s. Test drives homing
at a 30 s interval and aborts it; it fails (30.5 s) against the sleep.
@phsilva
phsilva merged commit 2b1f59c into astroufsc:master Jul 29, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants