Skip to content

Re-arm the flush timer after a broker reconnect - #108

Merged
clokep merged 4 commits into
clokep:mainfrom
ShuaiShao93:fix/rearm-flush-timer-on-reconnect
Aug 26, 2026
Merged

Re-arm the flush timer after a broker reconnect#108
clokep merged 4 commits into
clokep:mainfrom
ShuaiShao93:fix/rearm-flush-timer-on-reconnect

Conversation

@ShuaiShao93

Copy link
Copy Markdown
Contributor

Summary

Fixes the long-standing "RMQ reconnection" wedge (#99): after the broker connection is lost and re-established, a Batches worker can stop flushing batches entirely and sit holding its prefetched messages unacked until it is restarted. I dug into the root cause and have a one-spot fix in Batches.Strategy(). Full analysis (with a minimal repro) is in #107.

Root cause

Strategy() runs every time the consumer (re)starts, including after a reconnect. The flush timer is armed lazily inside the message handler:

if self._tref is None:  # first request starts flush timer.
    self._tref = timer.call_repeatedly(self.flush_interval, flush_buffer)

self._tref lives on the task instance, which survives reconnects, but the timer it points at is bound to the consumer's old event-loop hub. After a reconnect Celery rebuilds the consumer (new hub, new channel) and calls Strategy() again, but self._tref is still set, so the if self._tref is None guard never re-arms the timer on the new hub.

The interval-based flush is then dead. The only remaining flush path is the count-based one (flush_every). With the common broker setup worker_prefetch_multiplier=1 (so the broker delivers fewer than flush_every messages and waits for acks), that count threshold is never reached either, so the worker buffers its prefetched messages forever without acking and looks "connected but idle". This matches the symptoms reported in #99.

Fix

Reset the per-consumer state (timer, buffers, count) at the start of Strategy(), before the new message handler closes over self._buffer. The next message then re-arms the flush timer on the new hub via the existing guard. Buffered requests belong to the old connection and are redelivered by the broker, so they are dropped.

Because Strategy() only runs at consumer (re)start — not per message — there is no steady-state overhead, and on a normal first start the reset is a no-op (the buffers are already empty and _tref is None).

Testing

  • Added t/unit/test_strategy.py covering both the reconnect reset (stale timer cancelled, _tref cleared so it re-arms, buffers/count reset) and the first-start no-op path.
  • pytest, mypy pass locally.
  • Verified end-to-end on RabbitMQ: a Batches worker with worker_prefetch_multiplier=1 wedges on main after a broker restart and recovers on its own with this change, including under sustained memory/disk alarms.

Closes #99

Strategy() runs every time the consumer (re)starts, including after a
lost broker connection is re-established. The flush timer (self._tref)
is bound to the consumer's event-loop hub and the buffered requests
reference the consumer's channel, both of which are replaced on
reconnect. The timer reference was kept across reconnects, so it was
never re-armed on the new hub; when the broker prefetch is smaller than
flush_every the worker silently stops flushing batches and wedges,
holding its prefetched messages unacked until it is restarted.

Reset the per-consumer state (timer, buffers, count) at the start of
Strategy() so the next message re-arms the flush timer on the new hub.
Buffered requests belong to the old connection and are redelivered by
the broker, so they are dropped.

Fixes clokep#99

Co-authored-by: Cursor <cursoragent@cursor.com>
Comment thread celery_batches/__init__.py Outdated
Comment thread CHANGELOG.rst Outdated
Co-authored-by: Patrick Cloke <clokep@users.noreply.github.com>
clokep and others added 2 commits August 26, 2026 08:33
Two failures under the repo's mypy config (warn_return_any,
warn_unreachable):

- Celery's task decorator is untyped, so the decorated function is Any and
  returning it from a helper annotated `-> Batches` triggers no-any-return.
  Make the conversion explicit with a cast.

- `_tref` is declared `Optional[Timer]`, but Timer is untyped so the
  declared type is `Union[Any, None]`. Assigning a MagicMock narrowed the
  attribute to MagicMock, making `assert task._tref is None` provably false
  and every statement after it unreachable. Typing the mock as Any keeps the
  attribute at its declared Optional type.
@clokep

clokep commented Aug 26, 2026

Copy link
Copy Markdown
Owner

Looks like GitHub Actions is having a day again. 😞

@clokep
clokep merged commit 9d72b3f into clokep:main Aug 26, 2026
10 checks passed
@clokep

clokep commented Aug 26, 2026

Copy link
Copy Markdown
Owner

Thank you!

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.

RMQ Reconnection issue

2 participants