fix(scheduler): claim retry queue (rebased, ladder bug fixed) — supersedes #101#103
Closed
ephpm-claude[bot] wants to merge 5 commits into
Closed
fix(scheduler): claim retry queue (rebased, ladder bug fixed) — supersedes #101#103ephpm-claude[bot] wants to merge 5 commits into
ephpm-claude[bot] wants to merge 5 commits into
Conversation
…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.
fireDue popped a due item and delete()d it from q.index before spawning runOne. On handler failure runOne re-enqueued via Add, which found an empty index and rebuilt a FRESH item (attempts reset to 1, firstFailure reset to now). The backoff ladder therefore never advanced past schedule[0] (~30s forever) and the MaxAge give-up never triggered. Fix: fireDue no longer deletes the popped item from q.index. Pop already sets it.index = -1, so the item is out of the heap but stays registered; the re-Add in runOne now hits the existed branch, incrementing attempts and preserving firstFailure so the ladder advances and give-up fires. On handler SUCCESS runOne calls Drop(key) to clean up the lingering index entry (removeLocked tolerates index < 0, so a concurrent Drop is a no-op). Add TestRetryQueue_LadderAdvancesThroughFirePath, which drives fireDue -> runOne -> re-Add with an always-failing handler and asserts the delays grow 30s -> 1m -> 2m and that the item is dropped once age exceeds MaxAge. It fails against the un-fixed code (ladder pinned at 30s).
Two follow-on fixes to the claim retry queue: 1. A retry that fires and fails AGAIN was being dropped instead of re-scheduled, so during a real rate-limit window (which needs several retries until the hourly reset) a job self-healed at most once then was lost. Cause: retryHandler always returned nil, but runOne now Drops on nil — and handleQueued's own enqueueRetryIfEligible had already re-added the item synchronously, so the Drop removed it. Fix: retryHandler now returns the real claim error (captured via a context pointer that enqueueRetryIfEligible fills in, which also suppresses the duplicate enqueue on the retry path). runOne then drops on success and advances the ladder on failure — with the original error class preserved, so rate-aware snap-to-reset still applies on later attempts. New test TestRetryQueue_StillFailingRetry- StaysEnqueued drives the real fireDue->retryHandler->claim path and asserts the job stays enqueued; it fails before this change. 2. handleNativeMacOSJob did not enqueue a retry on claim failure, so native-mode macOS claim failures didn't self-heal (Windows already routes through handleLocalJob, which does). Wired it up to match. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ChVRC9ZrvQarrctDwSy1S5
Contributor
Author
|
Pushed two follow-on fixes after finding a deeper interaction bug during review:
Verified: full scheduler suite green, |
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rebased, bug-fixed successor to #101. Same feature (self-healing claim retries so orphaned jobs don't need manual cancel/rerun), with the review findings addressed.
Fixes vs #101
fireDuedeleted the item from the index on pop, sorunOne's re-Addbuilt a fresh item —attemptsnever advanced pastschedule[0](~30s forever) andfirstFailurereset every fire (MaxAge never triggered). Fixed:fireDuekeeps the item in the index across a fire (heapindexis already -1 afterPop);runOnecallsDrop(key)on success to clean up. So the re-Addincrementsattemptsand preservesfirstFailure.TestRetryQueue_LadderAdvancesThroughFirePathuses a 3-rung schedule + fake clock, fires throughfireDue/runOne, and asserts delays grow 30s→1m→2m (not reset to 30s),firstFailureis preserved, and the item drops at MaxAge. Fails against the un-fixed code, passes after. (The old ladder/give-up tests calledAdd()directly, bypassing the fire cycle — which is why the bug shipped.)port+ thetunnel = "external"case). Reconciledscheduler.go/config.go/main.goso fix(scheduler): stop re-provisioning undispatchable zombie jobs #95's zombie guard and the native macОS handler are untouched — union merges, nothing reverted.Verification
go build ./...,go vet ./...clean;go test ./pkg/scheduler/... ./pkg/github/... ./pkg/config/...green (minus the pre-existing arch-dependentTestCanHandleJob/no_OS_label_accepts, which also fails onmain);-raceclean on the retry queue.Known follow-up (not a regression)
handleNativeMacOSJob) doesn't call the retry helper — fix(scheduler): claim retry queue, rate-aware backoff, seen-on-skip, fresh rate gauges #101 only wired retry into the linux/vm-macos/local claim paths. Native claim failures won't self-heal yet; trivial to extend. Flagged, not fixed here.Closes #101.
🤖 Generated with Claude Code