Skip to content

Pin kombu to unreleased commit to fix worker hang after Redis connection reset - #3640

Draft
jonathangreen wants to merge 2 commits into
mainfrom
bugfix/pin-kombu-redis-reconnect-fix
Draft

Pin kombu to unreleased commit to fix worker hang after Redis connection reset#3640
jonathangreen wants to merge 2 commits into
mainfrom
bugfix/pin-kombu-redis-reconnect-fix

Conversation

@jonathangreen

@jonathangreen jonathangreen commented Aug 11, 2026

Copy link
Copy Markdown
Member

Description

Pins kombu to an unreleased upstream commit (2c8372c, the merge commit of celery/kombu#2498) via a direct dependency and a [tool.uv.sources] entry pointing at GitHub's commit archive tarball. Using the tarball URL rather than a git source lets uv fetch it over plain HTTPS, so no git executable is needed in the docker images. kombu still identifies as 5.6.2 at this commit, so celery's own dependency constraint remains satisfied.

This pin is a temporary bandaid: it should be removed once a kombu release containing the fix (5.7.0 milestone) is published. The comment in pyproject.toml documents the removal condition.

Motivation and Context

When a celery worker's Redis broker connection is reset (e.g. by a Redis failover or restart), the worker logs consumer: Connection to broker lost. Trying to re-establish the connection... and then permanently stops consuming from all queues, while beat and already-running tasks continue. We hit this in production on 2026-08-11 (~10:54 UTC) on fl-florida-scripts and at least one other instance; the only recovery is restarting the worker.

This is celery/celery#10205: kombu's redis transport timer callbacks (maybe_restore_messages, maybe_check_subclient_health) raise ConnectionError through the event loop while the connection is down, and each reconnect attempt registers new timers without cancelling the old ones, so stale timers accumulate and repeatedly crash the loop. The celery half of the fix (celery/celery#10218) is already in celery 5.6.3, which we run — our incident shows it is insufficient on its own. The kombu half (celery/kombu#2498) is merged but unreleased.

Opened as a draft: we may prefer to wait for the kombu release. This PR is ready to merge if the hang recurs before that release ships.

How Has This Been Tested?

  • Verified the installed kombu at the pinned commit contains both halves of the fix: the connection_errors guards inside maybe_restore_messages / maybe_check_subclient_health, and the timer references (_restore_messages_tref / _subclient_health_tref) that are cancelled on reconnect.
  • tox -e py312-docker -- tests/manager/celery/ tests/manager/service/celery/ — 445 passed.
  • Built the common stage of docker/Dockerfile locally (both uv sync steps succeed with no git in the image) and confirmed the installed kombu inside the image contains the fix.

Checklist

  • I have updated the documentation accordingly.
  • All new and existing tests passed.

When a celery worker's redis broker connection is reset, kombu's redis
transport timer callbacks raise ConnectionError through the event loop
and stale timers accumulate across reconnect attempts, leaving the
worker permanently unable to consume tasks (celery/celery#10205).

The fix (celery/kombu#2498) is merged upstream but not yet in a kombu
release, so pin kombu to the merge commit until a release containing it
is published.
@jonathangreen jonathangreen added the bug Something isn't working label Aug 11, 2026
@claude

claude Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Claude finished @jonathangreen's task in 4m 43s —— View job


Claude Code Review

  • Gather context (read PR diff, changed files)
  • Review pyproject.toml dependency pin
  • Review uv.lock consistency
  • Check build/deploy/CI paths for compatibility with the new source type
  • Verify pinned tarball contents & hash (network egress blocked — see Summary)
  • Post review

Summary

The pin is well-executed and the diagnosis in the description matches the code: src/palace/manager/celery/monitoring.py:15 is the only place we touch kombu.transport.redis, and it subclasses PrefixedStrictRedis, which celery/kombu#2498 doesn't touch — so there's no interaction with the fix beyond getting it. uv.lock is internally consistent (kombu still resolves at 5.6.2 with its redis/sqs extras intact, satisfying celery 5.6.3's constraint), and every install path in the repo goes through uv sync --frozen — the Dockerfiles, tox.ini's uv-venv-lock-runner, docker/ci/test_backwards_compatibility.sh, and all CI workflows — so [tool.uv.sources] is honored everywhere and nothing does a bare pip install. The second commit's switch from a git source to a commit tarball is the right call given docker/Dockerfile.baseimage never installs git. Two minor points below. Note: I could not independently verify the recorded sdist hash or the pinned commit's contents — outbound network to github.com is blocked in this environment — so I'm taking the PR description's verification at face value there.

Details

Minor: pyproject.toml:403

GitHub's archive/<sha>.tar.gz endpoint generates the tarball on the fly rather than serving a stored artifact, so its bytes are not guaranteed stable across time — GitHub has changed the compression before and invalidated these checksums ecosystem-wide. Because uv records sha256:9cc46518… for it in the lock, a regeneration would make every uv sync --frozen fail on a hash mismatch at once: base image, app image, CI, and dev machines, with no code change to explain it. A git source is content-addressed by the commit and immune to this; it was dropped because the base image has no git, which is one line in the install_clean list at docker/Dockerfile.baseimage:33-44. Since the baseimage builds on its own schedule, adding git there now (even keeping the tarball for this PR) would let the pin be moved to the immune form without blocking the hotfix:

RUN install_clean \
      nginx \
      git \
      python3.12 \

kombu = {url = "https://github.com/celery/kombu/archive/2c8372c6b35b6c61bb6cf93470c57d9eab8836b8.tar.gz"}

Minor: pyproject.toml:70-75

The removal note points only at [tool.uv.sources], but deleting just that entry leaves "kombu" behind in [project.dependencies] as an unconstrained direct dependency — the one entry in this list with no version bound. Worth naming both halves in the comment, and giving the requirement a specifier now (uv validates the source's version against it, and 5.6.2 satisfies it) so the leftover is harmless if the source removal happens first:

    # ... This pin should be removed — both this requirement and the source in
    # [tool.uv.sources] — once a kombu release containing that fix is published
    # and picked up by our celery dependency.
    "kombu>=5.6.2,<6",

# kombu is a dependency of celery. We pin it to an unreleased commit here, because we
# need the fix from https://github.com/celery/kombu/pull/2498, so that celery workers
# recover when their redis broker connection is reset. This pin should be removed
# (see the source in [tool.uv.sources]) once a kombu release containing that fix
# is published and picked up by our celery dependency.
"kombu",

| Branch

@greptile-apps

greptile-apps Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR pins Kombu 5.6.2 to a commit-addressed upstream archive containing the Redis reconnection fix.

  • Adds Kombu as an explicit root dependency backed by a GitHub archive source.
  • Updates the lockfile to install and verify the pinned source artifact.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
pyproject.toml Adds the documented direct Kombu dependency and commit-addressed URL source.
uv.lock Locks Kombu to the selected archive and records its resolved dependency metadata and artifact hash.

Reviews (2): Last reviewed commit: "Install pinned kombu from GitHub commit ..." | Re-trigger Greptile

@codecov

codecov Bot commented Aug 11, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.54%. Comparing base (1c7746f) to head (bb5a739).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3640   +/-   ##
=======================================
  Coverage   93.54%   93.54%           
=======================================
  Files         509      513    +4     
  Lines       46854    46883   +29     
  Branches     6406     6406           
=======================================
+ Hits        43829    43859   +30     
+ Misses       1955     1954    -1     
  Partials     1070     1070           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

The docker images do not include a git executable, so uv cannot fetch a
git source there. GitHub's commit archive tarball has the same content
and uv downloads it over HTTPS with its own fetcher, no git required.

@tdilauro tdilauro left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤕

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants