chris/irrigo
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Activity Actions

[IRRIGOAPI-115] Fix duplicate concurrent daemon tick loops #12

Merged
chris merged 5 commits from bug/IRRIGOAPI-115 into main 2026-08-11 17:48:38 -06:00
Conversation 1 Commits 5 Files changed 12 +1896 -38
chris commented 2026-08-11 17:32:52 -06:00
Owner
Copy link

Ticket

IRRIGOAPI-115 — Daemon runs multiple concurrent tick loops, duplicate re-plans and duplicate scheduling_decisions rows

Root cause — it is not --watch

The ticket suspected hot-reload stacking under bun --watch. That is not it: bun --watch restarts the process, so old timers die with it. This reproduces identically under bun run start, so it is a production bug.

The defect is a timer leak in TimerRegistry.setRePlanHandle (api/service/daemon/runtime.ts), which overwrote the stored handle without cancelling the timer it replaced. scheduleNextTick calls it at the end of every re-plan. A scheduled tick is net-neutral — its timer has already fired before it re-arms. But every operator-triggered re-plan (POST /replan, schedule enable/disable/skip/resume, system enable/disable) runs off-timer and arms an extra tick while the previously-armed one is still live. The pending-timer count therefore grew by exactly one per operator re-plan and reset only on container restart — matching the reported "2, then 7, then 3, cleared on restart" signature.

Summary

  • Cancel the previous re-plan timer when arming the next one. setRePlanHandle now takes the Clock and clears the handle it replaces, so at most one re-plan timer is ever live.
  • Serialise re-plans. Both the scheduled tick and the operator rePlan() go through a promise-chain queue, so an operator re-plan arriving mid-tick can no longer interleave with the running one on zonesRepo.advanceDepletion / scheduleEntriesRepo.replaceForZone. A failed re-plan can't wedge the queue, and the caller still sees its own rejection (so POST /replan keeps its 502 semantics).
  • Don't re-arm a tick after shutdown. Found during review: a re-plan in flight (or queued) when shutdown ran would re-arm a tick right after cancelAllTimers cleared it, leaving a live timer holding the process open. Queued re-plans are now dropped and scheduleNextTick is a no-op once shutdown starts.
  • Idempotent decision writes. scheduling_decisions gains a unique index on (zone_id, date) and the repository upserts via onConflictDoUpdate, so a duplicated re-plan can no longer fan out rows. The row now holds the latest replan's decision for that night — the authoritative answer to "why didn't zone X water on night Y?".
  • One-off cleanup of the existing duplicates, prepended to migration 0020_tense_iceman.sql: keeps the newest row per (zone_id, date), with id as a deterministic tie-break.
  • Repaired 12 stale test expectations left behind by IRRIGOAPI-113's alert-message rewrite (assertions still checked the old HA open failed / Weather API stale style titles). These were already failing on main; the suite is now fully green.

Deploy note

verifyMigrations exits the api container at startup when the schema is behind the codebase, so docker compose run --rm api bun run db:migrate must be run before the new image boots. That migration is also what performs the duplicate cleanup.

Verification

  • bun --cwd=./api run type-check — clean
  • bun --cwd=./api test — 1010 pass, 0 fail (was 996 pass / 12 fail on main)
  • Each new guard was confirmed to fail its test when disabled, so the regression tests genuinely exercise the fix
## Ticket [IRRIGOAPI-115](http://192.168.2.100:7123/home/browse/IRRIGOAPI-115/) — Daemon runs multiple concurrent tick loops, duplicate re-plans and duplicate `scheduling_decisions` rows ## Root cause — it is not `--watch` The ticket suspected hot-reload stacking under `bun --watch`. That is not it: `bun --watch` restarts the process, so old timers die with it. **This reproduces identically under `bun run start`, so it is a production bug.** The defect is a timer leak in `TimerRegistry.setRePlanHandle` (`api/service/daemon/runtime.ts`), which overwrote the stored handle without cancelling the timer it replaced. `scheduleNextTick` calls it at the end of every re-plan. A *scheduled* tick is net-neutral — its timer has already fired before it re-arms. But every **operator-triggered** re-plan (`POST /replan`, schedule enable/disable/skip/resume, system enable/disable) runs off-timer and arms an extra tick while the previously-armed one is still live. The pending-timer count therefore grew by exactly one per operator re-plan and reset only on container restart — matching the reported "2, then 7, then 3, cleared on restart" signature. ## Summary - **Cancel the previous re-plan timer when arming the next one.** `setRePlanHandle` now takes the `Clock` and clears the handle it replaces, so at most one re-plan timer is ever live. - **Serialise re-plans.** Both the scheduled tick and the operator `rePlan()` go through a promise-chain queue, so an operator re-plan arriving mid-tick can no longer interleave with the running one on `zonesRepo.advanceDepletion` / `scheduleEntriesRepo.replaceForZone`. A failed re-plan can't wedge the queue, and the caller still sees its own rejection (so `POST /replan` keeps its 502 semantics). - **Don't re-arm a tick after shutdown.** Found during review: a re-plan in flight (or queued) when `shutdown` ran would re-arm a tick right after `cancelAllTimers` cleared it, leaving a live timer holding the process open. Queued re-plans are now dropped and `scheduleNextTick` is a no-op once shutdown starts. - **Idempotent decision writes.** `scheduling_decisions` gains a unique index on `(zone_id, date)` and the repository upserts via `onConflictDoUpdate`, so a duplicated re-plan can no longer fan out rows. The row now holds the latest replan's decision for that night — the authoritative answer to "why didn't zone X water on night Y?". - **One-off cleanup of the existing duplicates**, prepended to migration `0020_tense_iceman.sql`: keeps the newest row per `(zone_id, date)`, with `id` as a deterministic tie-break. - **Repaired 12 stale test expectations** left behind by IRRIGOAPI-113's alert-message rewrite (assertions still checked the old `HA open failed` / `Weather API stale` style titles). These were already failing on `main`; the suite is now fully green. ## Deploy note `verifyMigrations` exits the api container at startup when the schema is behind the codebase, so **`docker compose run --rm api bun run db:migrate` must be run before the new image boots**. That migration is also what performs the duplicate cleanup. ## Verification - `bun --cwd=./api run type-check` — clean - `bun --cwd=./api test` — 1010 pass, 0 fail (was 996 pass / 12 fail on `main`) - Each new guard was confirmed to fail its test when disabled, so the regression tests genuinely exercise the fix
Resolve merge conflicts with main
All checks were successful
plane-sync / sync (pull_request) Successful in 2s
Details
677eaf3527 
Renumber our 0020_tense_iceman migration to 0021 to avoid collision with
0020_whole_wolfsbane (precipitation_probability_max column) from main.
Updated snapshot chain and journal accordingly.
chris commented 2026-08-11 17:41:16 -06:00
Author
Owner
Copy link

From Claude: resolved merge conflicts — please re-review.

Both branches had independently created a migration numbered 0020. Resolution: kept main's 0020_whole_wolfsbane (adds precipitation_probability_max to weather_daily_snapshots) as index 0020, and renumbered our 0020_tense_iceman (deduplicates scheduling_decisions rows + adds unique index on zone_id/date) to 0021. Snapshot chain and journal updated accordingly.

From Claude: resolved merge conflicts — please re-review. Both branches had independently created a migration numbered `0020`. Resolution: kept main's `0020_whole_wolfsbane` (adds `precipitation_probability_max` to `weather_daily_snapshots`) as index 0020, and renumbered our `0020_tense_iceman` (deduplicates scheduling_decisions rows + adds unique index on zone_id/date) to `0021`. Snapshot chain and journal updated accordingly.
chris merged commit 59e95e8dd2 into main 2026-08-11 17:48:38 -06:00
chris deleted branch bug/IRRIGOAPI-115 2026-08-11 17:48:38 -06:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
chris/irrigo!12
No description provided.