Repository navigation
Conversation
den-dw
requested review from
aiivashchenko,
arttor,
fa-at-pulsit and
schucly
as code owners
October 27, 2026 08:18
arttor
reviewed
Oct 27, 2026
| if err != nil && !errors.As(err, &rlErr) { | ||
| failedCounter.WithLabelValues(queue, t.Type()).Inc() | ||
| } | ||
| processedCounter.WithLabelValues(queue, t.Type()).Inc() |
Contributor
There was a problem hiding this comment.
should then ratelimited task not counted as processed?
Contributor
Author
There was a problem hiding this comment.
Good point — a postponed task goes back to the queue and is processed later, so it shouldn't count as processed either. The middleware now skips both counters for dom.ErrRateLimitExceeded, and the test and the README metric descriptions cover it. I also rebased onto main; the CI failure was make proto-gen changing go.mod on the old base.
A task that hits a storage rate limit or cannot obtain the object lock returns dom.ErrRateLimitExceeded and is retried after a short delay; the worker error handler logs it at debug level, not as a failure. The worker middleware still counted it in worker_failed_tasks_total. CompleteMultipartUpload (and PutObject with ACL or tagging headers) enqueues object sync, object ACL sync and object tags sync tasks of the same object at once, and all of them take the same object lock, so some of them wait and are counted as failed. A burst of uploads (e.g. a database backup) produces hundreds of "failed" tasks per 15 minutes with no errors in the logs, which makes the metric useless for alerting. Signed-off-by: Denis Ivanov <dendw.work@gmail.com>
A postponed task returns to the queue and is processed later, so counting each postponed attempt inflated worker_processed_tasks_total as well. The middleware now skips both counters for dom.ErrRateLimitExceeded; the metric descriptions in the chart README say so. Signed-off-by: Denis Ivanov <dendw.work@gmail.com>
den-dw
force-pushed
the
fix/metrics-postponed-tasks
branch
from
October 27, 2026 12:51
eeb6dc1 to
fecf67d
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
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.
Problem
A task that hits a storage rate limit or can't get the object lock returns
dom.ErrRateLimitExceededand is retried after a short delay. The worker error handler treats this as a postponed task and logs it at debug level.WorkerMiddlewarestill counts it inworker_failed_tasks_total.The most common case is lock contention between tasks of the same object.
CompleteMultipartUpload(andPutObjectwith ACL or tagging headers) enqueuesobject:sync,object:sync:aclandobject:sync:tagsfor the same object at once. All three take the same object lock, so some of them wait.We replicate pgBackRest repositories, which are mostly multipart uploads. While a backup runs,
worker_failed_tasks_totalgrows by up to ~490 per 15 minutes forobject:sync:aclandobject:sync:tags(ACL sync is disabled in our config), and the worker logs show no errors at all. An alert on this metric can't tell these delays from real failures.Fix
WorkerMiddlewaredoesn't countdom.ErrRateLimitExceededas a failed task.worker_processed_tasks_totalis unchanged.Testing
New
pkg/metrics/worker_middleware_test.go: success, a regular error, and a wrappedErrRateLimitExceeded("lock not obtained"). On main the test fails: the postponed task is counted (expected: 1, actual: 2). With the change it passes.