Skip to content

fix(scheduler): survive stop/start races and self-heal a dead scheduler thread - #2342

Open
federicodeponte wants to merge 4 commits into
mainfrom
fix/scheduler-catchup-restart
Open

fix(scheduler): survive stop/start races and self-heal a dead scheduler thread#2342
federicodeponte wants to merge 4 commits into
mainfrom
fix/scheduler-catchup-restart

Conversation

@federicodeponte

Copy link
Copy Markdown
Member

Scope, so nobody misreads it

The PRIMARY cause of the 2026-08-02 incident was not in this repo. It was restartPolicyType = "on_failure" in the cloud repo's railway.toml: a clean SIGTERM meant Railway never restarted the service, and it stayed down 5h33m. That is fixed in floomhq/workeros-cloud#1287.

This PR fixes a separate latent bug in the engine with the same symptom: a scheduler that stops firing and never comes back. It was reproduced deterministically, it is real, and it would have produced the same customer-visible outcome on its own. It is not the incident's root cause.

The reproduced race

apps/api/scheduler.py controlled the scheduler thread with one MODULE-GLOBAL _stop_event shared across thread generations:

  • The cloud wrapper calls stop_scheduler() and, milliseconds later, start_scheduler() again when its Postgres advisory-lock connection blips.
  • The old thread is still sleeping in _stop_event.wait(60), so it is is_alive().
  • start_scheduler() early-returned WITHOUT clearing _stop_event.
  • The old thread then woke, saw the flag set, and exited.

Net result: no scheduler thread, _stop_event stuck set, and nothing ever restarted it. Reproduced as alive=False stop_set=True.

Changes

  1. Per-generation stop event. Every start_scheduler() binds a FRESH threading.Event and hands it to the thread it launches by closure, so a stop aimed at generation N can never terminate generation N+1. stop_scheduler() still stops the current generation. The module-level names _stop_event, _scheduler_thread and _scheduler_lock are preserved (existing tests read them); _scheduler_lock became an RLock so the watchdog can check and restart under one lock.

  2. start_scheduler() no longer early-returns while a stop is pending. If the existing thread is alive but stopping, it is joined (bounded, 5s) and replaced. If it is alive and healthy, the call stays the no-op it always was.

  3. The poll loop is unkillable by incidental exceptions. _record_scheduler_heartbeat(), _tick() and the sleep are all inside guards, and logging goes through a _safe_log helper, so a broken stdout pipe (seen after a container restart) cannot end the scheduler for the lifetime of the process. Only the generation's own stop event ends the loop.

  4. ensure_scheduler_running(), an idempotent watchdog. Three states, only one of which it recovers:

    • ok: live generation, fresh heartbeat. No-op.
    • dead: no thread, thread not alive, or stopping and gone within the bounded join. Recovered by launching a fresh generation.
    • wedged: alive, not stopping, heartbeat stale. Logged loudly and left alone. Python cannot kill a thread, so a replacement would leave two schedulers firing the same triggers and duplicating side effects (emails, CRM writes), which is worse than the delay. A stale heartbeat is also not proof of death: a recovery tick working through many overdue triggers legitimately runs long. This matches the three-state model in the cloud PR.

    It holds _scheduler_lock for the whole check-and-restart, so it never spawns a duplicate thread, and it only ever RE-starts: a process that never started a scheduler (WORKEROS_ROLE=web) cannot grow one by calling it. It is wired as a coarse backstop into the hourly sweep loop in main.py, off the event loop via asyncio.to_thread.

  5. Health surfacing (additive only). _health_check_scheduler() now adds alive, heartbeat_age_seconds, stale and stale_after_seconds to the existing scheduler_status() payload, so an alive-but-wedged scheduler is visible. ok deliberately keeps its old meaning and no HTTP status-code behaviour changes.

    scheduler_heartbeat_status() also gained an explicit alive. running is alive and not stopping, so a live thread winding down used to look identical to no thread at all, and a supervisor must tell those apart: restarting or releasing a leader lock while a live thread may still be firing triggers double-fires runs. scheduler_status() is deliberately untouched, since its payload is asserted by exact dict equality in existing tests.

    Known gap, deliberately out of scope: the cloud runs leader election, so a follower legitimately has no scheduler thread and is perfectly healthy. Making "no scheduler thread" unhealthy would mark every follower unhealthy. Leader-aware health semantics are a follow-up.

  6. Honest user-facing failure copy in apps/api/services/public_view.py. scheduler_missed, executor_lost_mid_run and run_claimed_without_dispatch had no headline entry, so a customer's platform-abandoned run fell back to unknown_error copy: "This worker failed to run. Check the run logs for details, then edit or re-run the worker." That blames the customer for our restart. They now read:

    • scheduler_missed: Floom's scheduler was delayed, so this scheduled run started later than its scheduled time. The worker itself is fine and no action is needed.
    • executor_lost_mid_run: Floom's execution service stopped while this run was in progress. This is a platform fault, not a problem with the worker. Re-run it if the work did not complete.
    • run_claimed_without_dispatch: Floom picked this run up but the platform stopped before the worker started. Nothing in the worker ran. This is a platform fault; re-run it.

Tests

New: apps/api/tests/test_scheduler_stop_start_race.py, apps/api/tests/test_platform_fault_operator_copy.py. Deterministic: POLL_INTERVAL_SECONDS is monkeypatched and ticks are awaited on an Event, no fixed sleeps, no network.

BEFORE (the new tests run against origin/main in a scratch worktree)

$ cd apps/api && python3 -m pytest tests/test_scheduler_stop_start_race.py::test_start_after_stop_leaves_a_live_ticking_scheduler -q

>       assert ticks.wait_for_first(), "a fresh generation must tick after a stop/start race"
E       AssertionError: a fresh generation must tick after a stop/start race
E       assert False
E        +  where False = wait_for_first()

tests/test_scheduler_stop_start_race.py:86: AssertionError
=========================== short test summary info ============================
FAILED tests/test_scheduler_stop_start_race.py::test_start_after_stop_leaves_a_live_ticking_scheduler
1 failed in 5.74s

Both new files against origin/main: 22 failed, 2 passed.

AFTER (this branch)

$ cd apps/api && python3 -m pytest tests/test_scheduler_stop_start_race.py tests/test_platform_fault_operator_copy.py -q
24 passed

Existing suites, unbroken

$ cd apps/api && python3 -m pytest tests/test_scheduler_missed_fire.py tests/test_scheduler_source_tag.py \
    tests/test_run_metrics.py tests/test_1067_worker_limits_and_cron.py \
    tests/db/test_worker_triggers_normalize.py tests/test_backend_p2_reliability.py \
    tests/test_health_info_disclosure.py tests/test_scheduler_stop_start_race.py \
    tests/test_platform_fault_operator_copy.py -q
56 passed

and from the repo root:

$ python3 -m pytest tests/test_operator_hygiene.py tests/test_ops_alerting.py -q
40 passed

ruff check is clean on every touched file. The full apps/api/tests suite is left to CI.

Related: floomhq/workeros-cloud#1287 (the primary fix: Railway restart policy).

🤖 Generated with Claude Code

claude added 3 commits August 2, 2026 18:23
…er thread

The scheduler thread was controlled by one module-global stop event shared
across thread generations. A stop_scheduler() immediately followed by a
start_scheduler() (what the cloud wrapper does when its Postgres advisory-lock
connection blips) left the process with NO scheduler: the old thread was still
sleeping in wait(60) so it was is_alive(), start_scheduler() early-returned
without clearing the flag, and the old thread then woke, saw the flag and
exited. Reproduced deterministically (alive=False stop_set=True).

- Per-generation stop event, bound fresh by every start and handed to the
  thread by closure, so a stop aimed at generation N cannot kill N+1.
- start_scheduler() joins a stopping generation (bounded) and replaces it
  instead of early-returning. A healthy generation is still a no-op.
- The poll loop contains the heartbeat, the tick and the sleep, so a broken
  logging handler cannot end the scheduler for the process lifetime.
- ensure_scheduler_running(): idempotent watchdog that restarts a dead or
  stale-heartbeat scheduler under _scheduler_lock, never cold-starts one in a
  process that never had a scheduler, and never spawns a duplicate thread.
  Called as a coarse backstop from the hourly sweep loop.
- /health now reports scheduler_heartbeat_status(), so an alive-but-wedged
  scheduler is visible. The payload stays a superset of the old keys.
- Honest operator copy for scheduler_missed, executor_lost_mid_run and
  run_claimed_without_dispatch: these are platform faults and no longer fall
  back to copy that tells the customer to edit their worker.

Co-Authored-By: Claude <noreply@anthropic.com>
Codex review of the design: Python cannot kill a thread, so restarting on a
stale heartbeat while the old generation is still alive would leave TWO
schedulers firing the same triggers and duplicating side effects (emails, CRM
writes). A stale heartbeat is also not proof of death: a recovery tick working
through many overdue triggers legitimately runs long.

- ensure_scheduler_running() now recovers only the dead state (no thread, not
  alive, or stopping and gone within the bounded join). Alive plus stale is
  logged loudly and left alone.
- _start_scheduler_locked(replace_wedged=False) refuses to launch a successor
  while the outgoing generation is still alive, so an automatic caller can
  never double the scheduler. The explicit start_scheduler() path keeps
  replacing, which is what the cloud wrapper asks for.
- /health surfaces heartbeat_age_seconds, stale and stale_after_seconds
  ADDITIVELY and no longer changes what `ok` means. The cloud runs leader
  election, where a follower legitimately has no scheduler thread, so
  leader-aware health semantics stay a separate follow-up.

Co-Authored-By: Claude <noreply@anthropic.com>
`running` is `alive and not stopping`, so a live thread that is winding down
reported running=False and was indistinguishable from "no thread at all". A
supervisor has to tell those apart: restarting or releasing a leader lock while
a live thread may still be firing triggers double-fires runs.

Purely additive. scheduler_status() is deliberately untouched, since its
payload is asserted by exact dict equality in existing tests.

Co-Authored-By: Claude <noreply@anthropic.com>
…r stub

The additive heartbeat fields were read with a from-import, which raises on the
SimpleNamespace scheduler stub several tests inject and made the whole check
report ok=False, turning /health/details into "degraded" (caught by CI in
test_s35_observability). Read the heartbeat through getattr and never fail the
check on it: the scheduler_status() payload is returned either way.

Co-Authored-By: Claude <noreply@anthropic.com>
@federicodeponte

Copy link
Copy Markdown
Member Author

CI on ad11b1430: Runtime tests pass on ubuntu-latest and windows-latest, Python lint, MCP tests, Secret scan and Dependency review all pass.

Web lint fails on the Audit production dependencies step (npm advisories in hono / @hono/node-server / brace-expansion / DOMPurify / fast-uri). That step is already red on main at the base commit 1d7b841d9, and this PR touches Python only. Unrelated, not introduced here.

One regression this PR did introduce was caught by CI and fixed in ad11b1430: reading the new heartbeat fields with a from-import broke _health_check_scheduler() against the minimal SimpleNamespace scheduler stub several tests inject, which flipped /health/details to degraded (test_s35_observability.py::test_health_runs_dependency_checks_and_uses_cache). The heartbeat is now read through getattr and never fails the check.

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