fix(worker): cap retry countdown to visibility timeout across all tasks

[drazisil-codecov]

Any `self.retry(countdown=N)` where `N > TASK_VISIBILITY_TIMEOUT_SECONDS` causes Redis to redeliver the task from `unacked` before the ETA fires, creating duplicate executions that multiply exponentially. This is the root cause of the bundle analysis queue explosions documented in CODECOV-59.

What was broken

Two retry paths were affected across multiple tasks:

Lock-contention retries (`LockRetry.countdown`) — `LockManager` uses `200 * 3**retry_num`, which reaches ~1800s at `retry_num=2` (above the 900s visibility timeout). This formula predates PR #589 for bundle analysis tasks, but #589 extended `LockManager` to cover the entire upload pipeline (`UPLOAD`, `UPLOAD_PROCESSING`, `UPLOAD_FINISHER`, `PREPROCESS_UPLOAD`, `MANUAL_TRIGGER`), significantly expanding the blast radius.

Exponential backoff retries — Various tasks use formulas like `30 * 2retries`, `20 * 2retries`, `60 * 3**retries`. These all exceed 900s within a handful of retries and had no upper bound.

Fix

Two layers of defense:

1. `LockManager.MAX_RETRY_COUNTDOWN_SECONDS` is now bounded at `TASK_RETRY_COUNTDOWN_MAX_SECONDS` (was 5 hours).

2. `BaseCodecovTask.clamp_retry_countdown()` caps any explicitly-provided countdown before it is passed to Celery. Parameterless `self.retry()` calls (`countdown=None`) are left alone so Celery's built-in 180s default is preserved — 180s is already within the safe window, and overriding it would silently change retry cadence for cleanup tasks that rely on that default.

The cap is derived from the task's own `hard_time_limit_task`: a task calling `retry()` may have been running for up to that many seconds, so the safe ceiling is `TASK_VISIBILITY_TIMEOUT_SECONDS - hard_time_limit_task`. This is the same reasoning already used in `get_lock_timeout()`. Tasks with no configured hard limit fall back to `TASK_RETRY_COUNTDOWN_MAX_SECONDS` (visibility timeout − 30 s).

# In BaseCodecovTask
def clamp_retry_countdown(self, countdown: int) -> int:
    hard_limit = self.hard_time_limit_task
    if hard_limit > 0:
        return min(countdown, TASK_VISIBILITY_TIMEOUT_SECONDS - hard_limit)
    return min(countdown, TASK_RETRY_COUNTDOWN_MAX_SECONDS)

Fixes CODECOV-59

Follows up on CCMRG-2042 and #589.

---

Note

Medium Risk
Changes core retry timing across many worker tasks; while intended to prevent Redis redelivery duplication, it could alter retry cadence (and edge-case countdown values) for long-running or misconfigured time-limit tasks.

Overview
Prevents runaway duplicate task execution by capping all explicit self.retry(countdown=...) delays to fit within the remaining Redis TASK_VISIBILITY_TIMEOUT_SECONDS window.

Adds BaseCodecovTask.clamp_retry_countdown() and applies it automatically in the overridden BaseCodecovTask.retry(), while introducing a shared TASK_RETRY_COUNTDOWN_MAX_SECONDS fallback for tasks without a hard time limit.

Also reduces LockManager’s exponential backoff cap from a hardcoded 5 hours to TASK_RETRY_COUNTDOWN_MAX_SECONDS, and updates/extends unit tests to assert the new clamping behavior.

^{Written by Cursor Bugbot for commit 8a3a18a. This will update automatically on new commits. Configure here.}

[linear]

CODECOV-59 Cap bundle analysis retry countdown to task visibility timeout

[sentry]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.24%. Comparing base (aae0aa4) to head (8a3a18a).
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #720   +/-   ##
=======================================
  Coverage   92.24%   92.24%           
=======================================
  Files        1302     1302           
  Lines       47888    47896    +8     
  Branches     1628     1628           
=======================================
+ Hits        44175    44183    +8     
  Misses       3404     3404           
  Partials      309      309           

Flag	Coverage Δ
apiunit	96.36% <ø> (ø)
sharedintegration	37.01% <100.00%> (+<0.01%)	⬆️
sharedunit	84.89% <100.00%> (+<0.01%)	⬆️
workerintegration	58.63% <76.92%> (+0.01%)	⬆️
workerunit	90.33% <84.61%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[codecov-notifications]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

📢 Thoughts on this report? Let us know!

[codspeed-hq]

Merging this PR will not alter performance

✅ 9 untouched benchmarks

---

_{Comparing joebecher/cap-bundle-analysis-retry-countdown (8a3a18a) with main (afa4356)¹}

Footnotes

1. No successful run was found on main (aae0aa4) during the generation of this report, so afa4356 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

[cursor]

Cursor Bugbot has reviewed your changes and found 2 potential issues.

^{Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.}

[cursor]

Negative countdown when hard_time_limit exceeds visibility timeout

High Severity

clamp_retry_countdown computes TASK_VISIBILITY_TIMEOUT_SECONDS - hard_limit without guarding against hard_limit >= TASK_VISIBILITY_TIMEOUT_SECONDS. Several tasks have time_limit well above 900s (e.g., delete_owner at 2880s, mark_owner_for_deletion and sync_repos at 1440s). For those tasks the subtraction yields a large negative number (e.g., 900 − 2880 = −1980), and min(countdown, −1980) returns −1980. A negative Celery countdown schedules the retry in the past, causing immediate re-execution — the exact runaway-retry behavior this PR is trying to prevent.

 

[cursor]

Type check excludes float hard time limits

Medium Severity

isinstance(hard_limit, int) rejects float values, but hard_time_limit_task can return floats via self.request.timelimit[0] (Celery stores time limits as int or float). When the hard limit is a float like 720.0, the per-task visibility cap is silently bypassed and the method falls through to the generic TASK_RETRY_COUNTDOWN_MAX_SECONDS. This is inconsistent with get_lock_timeout, which uses hard_time_limit_task without any type check.

 

[thomasrockhu-codecov]

we should probably just get rid of this variable

[thomasrockhu-codecov]

I think we should force this to int instead of doing the check later

[drazisil-codecov]

Can a MagicMock be forced into an int? I will look.

[thomasrockhu-codecov]

if we do the above comment, you can just do if self.hard_time_limit_task

[drazisil-codecov]

The isinstance check is required for tests, where self.app is mocked and tests fail due to it not being an int. That is the only time the limit is ignored

[thomasrockhu-codecov]

I think that means the way we mock this needs to change, we shouldn't build based on how we expect tests to be run

[drazisil-codecov]

Fair. I don't know if we xan, since iirc, self.app is assigned by celery at runtime, but i will investigate. I remember quite a bit of trouble getting that to exist when i started, i almost want to say its a read-only property.

[thomasrockhu-codecov]

godspeed, mocks are painful, but it's probably the right way to handle this if it's possible. if not, we should rethink how these are tested

[drazisil-codecov]

This property only exists while the task is being executed, unless i am misunderstanding https://docs.celeryq.dev/en/stable/userguide/tasks.html#task-request

[drazisil-codecov]

I honestly can not remember if we were even testing retries prior the start of all my work. I will have to dig back.

[drazisil-codecov]

Yes, these tests were created by me. This wasn't being tested before. I don't know if there is another way to do this, you will note that I'm patching it in to begin with 427ec64

I'll look though my prior tries.

[thomasrockhu-codecov]

seems like there a logic disparity here, can you explain how we'd have hard_limit actually be 0 and why we don't care about the visibility timeout here?

[drazisil-codecov]

Per comment above, this is only for tests where the timeout is not a number.