Skip to content

Merge Task Retry Handling & Performance#860

Open
EvanDietzMorris wants to merge 2 commits into
masterfrom
task-retry-performance
Open

Merge Task Retry Handling & Performance#860
EvanDietzMorris wants to merge 2 commits into
masterfrom
task-retry-performance

Conversation

@EvanDietzMorris

@EvanDietzMorris EvanDietzMorris commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

This PR aims to improve performance, resource utilization, and error reporting for the merge_and_post_process celery task queue implementation.

Issues Addressed
Currently, merge_and_post_process celery tasks are retried for three broad reasons: a failure to acquire an 'expensive token', failure to acquire a merge db lock, or when any Exception is raised during merge_and_post_process. All three of these share the same retry implementation, retry counter, and an exponential back off behavior that delays the next attempt by increasing amounts. The exponential back off delay is tuned to be quite slow, in an attempt to give downstream services time to recover in event of an error, but for normal 'expensive token' or db lock contention this isn't appropriate and results in unnecessary downtime when long back off delays occur while resources might already be available. Additionally, if deterministic errors occur, the current implementation still retries them up to the shared limit of retries even though they will fail again, wasting time and resources, with long delays in between.

The Solution
This PR separates the retry logic for Exceptions from retries due to contention, allowing us to tune them differently. By default it attempts retries due to contention much faster and for more attempts (every 2-5 seconds up to 100 times) and retries due to exceptions with the existing exponential back off (up to 30 seconds) but only for 5 attempts.

DB Lock Silent Failures
Here we also address a mismatch for how db lock retries were handled vs token contention. Previously the db lock had hard coded retry values inside the merge_and_post_process function (with a cap of 10 instead of 20), and when it was exhausted and gave up it silently caught the error with only a log message. Now the db lock contention retry shares the implementation of the token contention retry, and raises the failure when it occurs, allowing Celery and OTEL to recognize it.

New ENV Vars
This introduces two new env vars for tuning this retry logic, but does not attempt to alter the helm chart due to settings being applied in jenkins outside of this repo. That should be addressed in future work.

ARS_EXPENSIVE_TASK_MAX_RETRIES (the overall shared retry limit, default 100)
ARS_MERGE_ERROR_MAX_RETRIES (retry limit for real errors, default 5)

Misc.
This also changes a logging call in expensive_gate indicating a token acquisition is being attempted from warning to debug, to avoid clogging up logs and unnecessary log writes, because this is a normal operation that should happen all the time, and more frequently with these changes.

Future work
It is important to note that there are still significant improvements that could be made for error handling here. All exceptions are handled in the same way. Except for other logs outside of merge_and_post_process it is not apparent what kind of issue caused Exceptions or retries for them, which still causes deterministic errors to be retried unnecessarily, and hurts our ability to diagnose issues. Perhaps more importantly, according to Claude, errors that occur when hitting the node annotator or appraiser are swallowed and don't even cause Exceptions to be raised into the merge_and_post_process - so at least some of the kinds of errors that it is most appropriate to retry with exponential back off on probably aren't even being retried.

- separate retry functionality for errors from retries due to contention (failure to acquire a token or a db lock)
- allow different max retry settings for each
- make the backoff timing for normal retries constant/faster vs the exponential backoff for real errors
- fix silent handling of merge lock retry exhaustion
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.

1 participant