backend-worker: treat codes.NotFound on job completion as terminal, not infinitely retried - #7893
Open
DeviousCardi wants to merge 3 commits into
Open
DeviousCardi wants to merge 3 commits into
DeviousCardi wants to merge 3 commits into
Conversation
DeviousCardi
requested review from
carles-grafana,
electron0zero,
ie-pham,
javiermolinar,
knylander-grafana,
mapno,
mattdurham,
mdisibio,
ruslan-mikhailov,
stoewer,
yvrhdn,
zalegrala and
zhxiaogg
as code owners
September 15, 2026 08:47
Contributor
Signed commits reportAll 3 commits between |
|
|
The Next() polling path already special-cased codes.NotFound from the scheduler, but the four completion call sites (UpdateJob in processCompactionJob and processRetentionJob, completeRedactionJob, and failJob) did not. All four route through callSchedulerWithBackoff, whose retry loop is effectively infinite because Backoff.MaxRetries defaults to 0. A NotFound response on completion (e.g. after a scheduler restart loses the in-flight assignment) therefore retried forever and the worker never called Next() again, starving itself of new work. Factor the NotFound check into a small isNotFound helper, apply it inside callSchedulerWithBackoff so a NotFound short-circuits the retry loop instead of looping until MaxRetries, and have each completion call site log a warning and drop the job (returning nil / the original failure reason) instead of escalating to another doomed completion call. Fixes grafana#7879 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…dedupe NotFound wrapping
completeRedactionJob was the only one of the three job-completion functions
that did not call failJob on a genuine (non-NotFound) UpdateJob error. It
just returned the wrapped error, leaving the redaction job stuck as
in-progress/leased to this worker forever instead of being marked failed
and made available for retry/reassignment. Bring it in line with
processCompactionJob and processRetentionJob.
Also extract the repeated "if isNotFound(err) { return err }; return
fmt.Errorf(...)" branch (needed because gogo/status's FromError uses a
plain type assertion rather than errors.As, so %w-wrapping a NotFound
error would hide it from later detection) into a wrapSchedulerErr helper,
used at all 5 callSchedulerWithBackoff call sites instead of duplicating
it.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The prior two commits made callSchedulerWithBackoff short-circuit on any codes.NotFound scheduler error, intending to make job-completion NotFound (UpdateJob/failJob/completeRedactionJob) terminal instead of retried forever. But callSchedulerWithBackoff also wraps the Next() polling call in processJobs, where NotFound just means "no jobs queued right now" after a routine long-poll timeout -- not a failure. That made every idle poll cycle propagate up through running() as a level.Error "error processing jobs" log and skip metricWorkerCallRetries, misrepresenting normal operation as a failure. processJobs now recognizes isNotFound on the Next() call specifically and returns quietly (nil) so running() resets its backoff and polls again, leaving the terminal short-circuit behavior for the four completion call sites untouched. Also extract the near-identical NotFound-drop-and-log block duplicated across processCompactionJob, processRetentionJob, and completeRedactionJob into a shared handleCompletionErr helper, keeping each call site's log message distinguishable by job kind. Adds TestNextNotFoundIsQuietIdlePolling, which asserts an idle Next() NotFound does not produce an Error-level log and does not touch metricWorkerCallRetries, distinguishing it from TestCompletionNotFoundIsTerminal's genuinely terminal completion case. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
DeviousCardi
force-pushed
the
fix/7879-backendworker-notfound-completion-retry
branch
from
September 15, 2026 09:01
2068aca to
1cf3a48
Compare
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.
Summary
Fixes #7879.
When a backend-worker finished a job and called a completion RPC (
UpdateJob,completeRedactionJob,failJob) and the backend-scheduler returnedcodes.NotFound(e.g. after a scheduler restart lost the in-flight assignment), the worker retried the same job forever insidecallSchedulerWithBackoff—Backoff.MaxRetriesdefaults to0, which dskit treats as infinite retries. The worker never returned toNext(), permanently starving itself of new work.Next()'s own NotFound handling turned out to be broken too: it checked forcodes.NotFoundin its callback, but the actual infinite-retry loop lived insidecallSchedulerWithBackoffitself, so the check never had a chance to stop anything — every idle "no jobs queued" poll cycle was silently retrying via the same unbounded loop.Changes
callSchedulerWithBackoffnow short-circuits oncodes.NotFoundvia a smallisNotFound/wrapSchedulerErrhelper pair (the latter avoidsfmt.Errorf's%wwrapping, which would otherwise hide the status code fromstatus.FromError's plain type assertion).processJobs'sNext()call site now treats aNotFoundresult as the expected "no jobs queued" case: it returns quietly and resumes polling, instead of the previous behavior of never terminating the retry loop or producing spuriouslevel.Errorlogs.processCompactionJob's andprocessRetentionJob'sUpdateJob,completeRedactionJob,failJob) now treatNotFoundas terminal: warn-log, drop the job, and resume polling instead of retrying forever.completeRedactionJobwas also missing thefailJobescalation thatprocessCompactionJob/processRetentionJobalready had for genuine (non-NotFound) errors — fixed to match.handleCompletionErrhelper shared by all three completion functions..chloggenentry per this repo's changelog convention.Test plan
go build ./modules/backendworker/...go vet ./modules/backendworker/...go test ./modules/backendworker/...— including new tests:TestCompletionNotFoundIsTerminal,TestNextNotFoundIsQuietIdlePolling(asserts idle polling produces no Error-level log and doesn't touch the retry-count metric),TestCompleteRedactionJobFailsOnGenuineError,TestIsNotFound.🤖 Generated with Claude Code