fix(scheduler): claim retry queue, rate-aware backoff, seen-on-skip, fresh rate gauges#101
fix(scheduler): claim retry queue, rate-aware backoff, seen-on-skip, fresh rate gauges#101luthermonson wants to merge 4 commits into
Conversation
The startup log unconditionally printed 'polling mode enabled (tunnel disabled)' whenever no tunnel provider was configured, even when the scheduler was running in webhook mode via an external ingress (cfg.Webhook.Secret set). scheduler.Run gates webhook mode on Tunnel != nil OR WebhookSecret != "", so the two paths must match. Now: log 'webhook mode enabled (external ingress, no tunnel provider)' when a secret is set without a tunnel, and 'polling mode enabled (no tunnel, no webhook secret)' only when neither is configured.
…ication handleQueued ran the canHandleJob label check BEFORE the seen/pending dedup writes, so label-mismatched jobs (e.g. self-hosted+macos+arm64 while the Mac host is down) returned without leaving any trace. On the next poll cycle the same queued job re-appeared and re-triggered the skip path — logs showed 'skipping job, OS labels don't match this platform' repeating every ~15s for hours, and every poll cycle still paid the API cost of listing runs + listing jobs for the run. Reorder handleQueued so the dedup check runs first; on label mismatch, write the key into seen (10min TTL) so re-deliveries short-circuit at the dedup gate. No pending-slot semantics change: we never took the semaphore, so pending stays unset. Update TestHandleQueued_SkipsMismatchedLabels to encode the new contract (seen IS populated, pending is NOT) and add a re-delivery assertion that pins the O(1)-per-event guarantee.
ephemerd_github_api_rate_remaining was defined but never written, so it
sat at 0 forever. Operators looking at Grafana couldn't tell 'budget
truly exhausted' from 'no data yet' from 'stale reading after window
reset'.
Wrap the GitHub HTTP client in a rateTrackingTransport that parses
X-RateLimit-{Limit,Remaining,Reset} on EVERY response (200 or 429/5xx
error — errors carry rate headers too, and that's exactly when we most
need them). Introduce three companion gauges:
ephemerd_github_api_rate_limit (ceiling for the window)
ephemerd_github_api_rate_reset_seconds (unix ts of next reset)
ephemerd_github_api_rate_updated_seconds (when we last observed)
Use rate_updated_seconds vs time() to age out a stale reading in
dashboards / alerts.
Also expose Client.RateSnapshot() for callers that want to bias
retry backoff based on the live rate state (used by the upcoming
scheduler claim-retry queue).
Guard against responses that don't emit rate headers (some GraphQL
paths) — those must NOT clobber the last known value to zero.
…re scheduling
GitHub does not re-deliver workflow_job webhooks. When ephemerd's
initial claim/JIT-runner-registration attempt failed with a transient
error (rate-limit exhausted, transient 5xx, network blip), the queued
job was dropped forever - operators noticed only via missing runs and
manually re-queued each job by hand.
Add an in-memory priority-heap retry queue keyed by jobKey. Failed
claims are re-scheduled on a jittered ladder (30s, 1m, 2m, 5m, 10m by
default), giving up after MaxAge (default 90m).
Hooks:
- handleLocalJob, handleLinuxJob, handleMacOSJob classify the claim
error via classifyErr and enqueue on retryable failures. The old
blind time.Sleep(backoffDuration) and 5s sleep are gone - the sem is
released first so we do not hold a slot idle across the wait.
- handleCompleted Drops the retry key so a job picked up elsewhere
(peer daemon sharing the installation token, or manual cancel) stops
getting reattempted.
- The event loop also Drops on 'in_progress', matching the same intent.
Rate-aware backoff: when the last GitHub API response reported
remaining=0 with a fresh (< 5m old) update timestamp and reset > now,
the next attempt is snapped to reset + [5s, 25s) jitter. This avoids
burning attempts against a provably-exhausted budget, and the jitter
prevents multiple daemons sharing the installation token from
stampeding at the exact same second.
Error classification (classifyErr) sorts errors into
non_retryable / rate_limit / server_side / network / unknown_retryable.
404/422/401/permission-denied are non-retryable; 429 and the go-github
RateLimitError/AbuseRateLimitError typed errors are rate_limit; 5xx
plus string-match fallbacks ("bad gateway", "gateway timeout") are
server_side; net.Error is network. Unknown errors are treated as
retryable within backoff/MaxAge - the whole point is to stop losing
jobs to unexpected transient failures.
Wiring:
- New RetryConfig on scheduler.Config, plumbed from [runner.claim_retry]
in the TOML (enabled defaults to true - losing jobs is worse than
extra retries).
- RateHint closure in cmd/ephemerd/main.go walks activeProviders for a
GitHub provider and binds to its RateSnapshot; nil when no rate-aware
provider is present.
- providers/github.Provider exposes RateSnapshot() forwarding to the
underlying github.Client (added in the earlier rate-transport commit).
Tests: retry_queue_test covers backoff progression (schedule ladder,
clamp past end), MaxAge give-up, Drop on completion, non-retryable
classification, rate-aware snap-to-reset with a fake clock, and the
full classifyErr table. retry_integration_test uses the existing
claimErrorProvider to end-to-end verify that a retryable failure in
handleLocalJob puts one entry in the queue and that a completed webhook
drops it. All new tests use table-driven or deterministic clock
patterns; no time.Sleep in scheduling logic.
![ephpm-claude[bot]](https://avatars.githubusercontent.com/u/272061596?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed in depth — thoughtful PR solving a real problem (self-healing claims), with good error classification, rate-aware backoff, jitter for the shared-token fleet, and a clean heap. Two things block merge; details inline.
Blocking: the backoff ladder and MaxAge give-up don't work on the real path
The retry loop resets its own state every time it fires:
Add()creates an item (attempts=1,firstFailure=T0), scheduled atschedule[0](~30s).fireDue()pops it anddelete(q.index, it.key), thenrunOne()runs the handler.- Handler fails →
runOnecallsAdd(event, …)→ index entry was just deleted →existed=false→ fresh item,attempts=1,firstFailure=now.
So in production:
- Ladder never advances — every retry is
schedule[0](~30s); the 1m/2m/5m/10m rungs are unreachable. MaxAgegive-up never fires —firstFailureresets each attempt, sonow - firstFailure ≈ 0forever. Jobs retry every 30s indefinitely instead of stopping at 90m.
The unit tests miss this because TestRetryQueue_BackoffLadder / GiveUpOnMaxAge call q.Add() directly in a loop, which finds the existing index entry and increments — bypassing the fireDue → runOne → Add cycle that runs in production.
Fix: preserve item state across a fire — e.g. don't delete from the index in fireDue (leave the item, index=-1, so the re-Add finds it, increments attempts, keeps firstFailure), and have runOne Drop(key) on success. Then add a test that drives the real Run()/fireDue loop on the fake clock and asserts the delay grows 30s→1m→2m and that the item drops after MaxAge.
Must rebase — branch predates #95/#97/#98/#99 and would regress them
mergeable_state is dirty. It was cut before today's merges:
- Item 3 (startup log) duplicates and regresses the merged #98 —
mainalready has the richer version (with theportfield and a distincttunnel = "external"case). Drop item 3 on rebase. - Item 2 (seen-on-skip) and the large
scheduler.gohunks (incl. a ~131-line deletion nearhandleMacOSJob) overlap #95'shandleQueued/zombie-guard and the native handler. Rebase onto currentmainand reconcile theseenhandling with #95 so nothing is clobbered.
Minor
classifyErr: a 403 *gh.ErrorResponse returns errNonRetryable before the string fallback, so a secondary rate limit that surfaces as a plain ErrorResponse (not AbuseRateLimitError) is dropped. Low risk, but consider checking Retry-After/message on 403 first.
Good
Rate-aware snap-to-reset with jitter, sensible error taxonomy, config-gated + documented, fresh rate gauges, and the seen-on-skip idea is correct. The design is right and worth landing once the loop actually advances and it's rebased.
| q.mu.Lock() | ||
| for len(q.heap) > 0 && !q.heap[0].nextAttempt.After(now) { | ||
| it := heap.Pop(&q.heap).(*retryItem) | ||
| delete(q.index, it.key) |
There was a problem hiding this comment.
Blocking bug source. Deleting the index entry here means the re-enqueue in runOne→Add (line 417) can't find the item, so Add builds a fresh one with attempts=1 and firstFailure=now. Net: the backoff ladder never advances past schedule[0] and MaxAge never triggers.
Suggest: don't delete here — leave the popped item in index (with index=-1) so the re-Add at line 207 hits the existed branch and increments. Then runOne must Drop(key) on success to clean up.
| return | ||
| } | ||
| // Re-enqueue on failure. Add() decides retry/give-up. | ||
| q.Add(it.event, it.handler, err) |
There was a problem hiding this comment.
This re-enqueues by event, not by the item, so attempts/firstFailure are lost (see line 393). A test that drives the real Run()/fireDue cycle on the fake clock — asserting delay grows 30s→1m→2m and the item drops after MaxAge — would have caught this; the current ladder/give-up tests call Add() directly and bypass fireDue.
| // 401/400/422 validation: fixing requires config change, | ||
| // not time. | ||
| // (Conflict 409 is handled inside claimJob by name-retry.) | ||
| return errNonRetryable |
There was a problem hiding this comment.
A 403 *gh.ErrorResponse returns errNonRetryable here before the string-fallback runs. A secondary rate limit that arrives as a plain ErrorResponse (not AbuseRateLimitError) would be dropped rather than retried. Consider checking Retry-After/the message on 403 before classifying non-retryable.
| // running webhooks via an external ingress (reverse proxy, LB, etc.). | ||
| // Scheduler treats this as webhook mode (see scheduler.Run: | ||
| // useWebhook = Tunnel != nil || WebhookSecret != ""). | ||
| log.Info("webhook mode enabled (external ingress, no tunnel provider)") |
There was a problem hiding this comment.
|
Superseded by the rebased + bug-fixed successor. The review findings are addressed there: the fireDue/runOne state-reset bug (ladder stuck at 30s / MaxAge never firing) is fixed with a regression test that drives the real fire path, the branch is rebased onto current main, and the startup-log edit (item 3) is dropped since it regressed the merged tunnel PR. Closing this in favor of that. |
1 participant
Summary
Four reliability fixes from a night of production forensics (2026-07-07/08, webhook-mode fleet sharing one GitHub App installation token):
pkg/scheduler/retry_queue.go) — claim/JIT-registration failures on retryable errors (429/rate 403/5xx/network) no longer orphan jobs. Heap-based single-goroutine queue, backoff 30s/1m/2m/5m/10m with ±20% jitter, give-up after 90m with WARN, dropped onin_progress/completedwebhooks. Rate-aware: when the last API response shows remaining=0 with a fresh reset timestamp, the next attempt snaps to just past the reset (jittered to prevent multi-daemon stampede). Config:[runner.claim_retry](enabled by default, documented in config.example.toml).handleQueuedrecorded jobs in the dedup map only AFTER the label check, so a still-queued job with foreign labels (e.g. macos/arm64 while that host is down) was re-adjudicated at full API cost on every poll/catch-up emission — the observed 15sskipping job, OS labels don't matchloop that drained the shared installation budget and starved every daemon. Label-mismatch skips now dedup like everything else.polling mode enabled (tunnel disabled)printed whenever no tunnel provider existed, even in webhook mode (secret-gated); caused a real misdiagnosis. Message now reflects the actual discovery mode.rateTrackingTransportupdatesephemerd_github_api_rate_{remaining,limit,reset_seconds,updated_seconds}from response headers on every API call, so "0" is distinguishable from "stale 0".Test plan
go test ./pkg/scheduler/... ./pkg/github/...green on Windows;go vetclean (GOOS=linux CGO_ENABLED=0)Not covered (follow-ups)
ephemerd_retry_queue_depthgauge