Skip to content

fix: isolate scheduled wake dispatches - #349

Merged
mostlydev merged 3 commits into
masterfrom
issue-347-scheduler-fairness
Aug 3, 2026
Merged

fix: isolate scheduled wake dispatches#349
mostlydev merged 3 commits into
masterfrom
issue-347-scheduler-fairness

Conversation

@mostlydev

Copy link
Copy Markdown
Owner

Summary

  • dispatch due runner targets concurrently so one slow wake cannot stall unrelated targets or the minute loop
  • preserve strict FIFO ordering per runner target across scheduled ticks and manual fires
  • coalesce overlapping slots per invocation, keep next-fire state monotonic, and return HTTP 409 for duplicate manual fires
  • treat queue, lookup, and execution cancellation neutrally and drain active scheduler dispatches during shutdown

Boundary

This stays entirely in claw-api scheduler transport/state. It adds no runner prompt behavior, downstream wrapper, or cllama feature. PR #346 remains draft and should depend on this fairness correction before extending a Hermes wake budget.

Verification

  • git diff --check
  • DOCKER_HOST=unix:///tmp/clawdapus-no-docker.sock go test -count=1 ./...
  • DOCKER_HOST=unix:///tmp/clawdapus-no-docker.sock go vet ./...
  • go test -race -count=20 ./cmd/claw-api
  • independent adversarial review found no blocking correctness issue

Closes #347

Overlap suppression was audit-invisible: tick() advanced and persisted
next-fire for a suppressed entry and logged "overlap-suppressed", but
wrote nothing to InvocationState. Three due slots with one execution
left state byte-identical to a schedule that only had one slot due, so
clawdash and `claw api schedule` rendered a coalescing schedule as
perfectly healthy. That misses requirement 5 of #347 (state must stay
auditable for each fire slot), and it starts to bite once a runner wake
budget can exceed the schedule cadence.

- add cumulative `suppressed_slots` + `last_suppressed_at` to
  InvocationState, persisted in the same state write that advances
  next-fire so a completing wake cannot erase the record
- include the dropped slot timestamp in the scheduler log line
- note coalesced slots on the clawdash schedule card
- drop Run()'s unreachable `case <-s.stopCtx.Done()` select arm;
  stopCancel() is only reached from Run()'s own ctx.Done branch, which
  returns immediately after. stopCtx itself stays, since FireNow uses it
  to cancel in-flight manual fires at shutdown.

go test ./... , go vet ./... , go test -race -count=20 ./cmd/claw-api,
and git diff --check all pass.
@mostlydev

Copy link
Copy Markdown
Owner Author

Pushed two scoped corrections to this branch (2c8ec25) after independent adversarial review. Full review notes are in the talking-stick room; summary here.

No correctness bug found. Verified independently rather than trusting CI: go test -race -count=20 ./cmd/claw-api clean, lock order is consistently s.mu -> q.mu with no inversion anywhere, the reservation state machine handles waiting/granted/released/canceled including the double-cancel and cancel-while-granted paths, and dispatchWG.Add happens under s.mu before goroutine launch so the shutdown drain is sound.

Correction 1 — coalesced slots were audit-invisible (requirement 5 of #347).

tick() advanced and persisted nextFireUTC for a suppressed entry and logged overlap-suppressed, but wrote nothing to InvocationState. Proved with a test: three due slots with one execution left LastStatus=fired, LastFiredAt=T0, NextFireAt=T0+3m, ConsecutiveFailures=0 — byte-identical to a schedule that only ever had one slot due. cmd/clawdash/schedule_page.go and claw api schedule read only that state, so a schedule quietly firing at a third of its cron rate rendered as perfectly healthy, with the only evidence in container logs.

The suppression itself is correct and intended — the gap was that it left no operator-visible trace. Fix keeps the existing model rather than inventing a new one:

  • cumulative suppressed_slots + last_suppressed_at on InvocationState, written in the same state update that advances next-fire, so a completing wake cannot erase the record (regression-tested)
  • deliberately did not overload LastSkippedAt / LastStatusskipped already means the when: guard declined the slot, which is a different thing from a dropped slot
  • the log line now carries the dropped slot timestamp
  • clawdash schedule card gains a note; a clean dispatch still produces no note (both directions tested)

This matters specifically because #346 gives Hermes a 2-minute budget, which exceeds many cron cadences and makes coalescing an ordinary occurrence rather than an edge case.

Correction 2 — dead select arm. Run()'s case <-s.stopCtx.Done() is unreachable: stopCancel() is only called by stopScheduledDispatches(), whose only caller is Run()'s own ctx.Done branch, which returns immediately after. Removed the arm. stopCtx itself stays — FireNow legitimately uses context.AfterFunc(s.stopCtx, cancel) to cancel in-flight manual fires at shutdown.

Withdrawn finding, recorded for honesty. I first measured "6 due slots produce 1 dispatch" and was going to report it as a regression. It is not one. Master's tick is synchronous, so a slow wake blocks the loop and later slots coalesce there too; my harness drove six back-to-back ticks in a way master structurally cannot. This PR does not reduce executed wake count, it decouples targets. Withdrawn.

Verification on the updated branch: go test ./... (all packages), go vet ./..., go test -race -count=20 ./cmd/claw-api, git diff --check — all pass. Changelog edit stays inside ## Unreleased; no pins, badges, nav, or submodule movement.

@mostlydev
mostlydev marked this pull request as ready for review August 3, 2026 00:48
@mostlydev
mostlydev merged commit de18d14 into master Aug 3, 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.

Prevent slow scheduled wakes from blocking unrelated invocations

1 participant