Skip to content

refactor: replace 2 DB-busy retry loops with tenacity (#1132)#1282

Open
axisrow wants to merge 1 commit into
mainfrom
ao/tg_content_factory_5863f66be3-64/tenacity-db-busy-retry
Open

refactor: replace 2 DB-busy retry loops with tenacity (#1132)#1282
axisrow wants to merge 1 commit into
mainfrom
ao/tg_content_factory_5863f66be3-64/tenacity-db-busy-retry

Conversation

@axisrow

@axisrow axisrow commented Jul 10, 2026

Copy link
Copy Markdown
Owner

Part of #1132 (axis 2/7 of #1130). Registry: #782 — retry batch, DB-busy group. Follows up #1198 (Counter/base64 safe subset), which explicitly deferred all retry findings for per-case owner review.

What

Replaces the two genuine hand-rolled retry loops among the 14 detector findings with tenacity.AsyncRetrying (already a direct dependency; same style as error_recovery_service.py / production_limits_service.py). The other 12 findings are poll/pacing/work-distribution loops — adjudication table below, no code changes.

1. Database._with_busy_retry (src/database/facade.py)

SQLite-busy retry on the shared write path. Behaviour preserved exactly:

  • fixed delay ladder from self._busy_retry_delays_sec via wait_chain(*(wait_fixed(d) ...)) — no jitter, no exponent;
  • attempts = len(delays) + 1 via stop_after_attempt;
  • non-busy OperationalError re-raised immediately (retry_if_exception(_is_retryable_busy_error));
  • same per-retry and final warning logs; final failure raises DatabaseBusyError chained (from) to the last busy error.

2. TelegramCommandDispatcher._update_command_safely (src/services/telegram_command_dispatcher.py)

Unbounded exponential backoff on DatabaseBusyError. Behaviour preserved exactly:

  • ladder INITIAL, 2×, 4×, … capped at MAX via wait_exponential(multiplier=INITIAL, max=MAX) — bit-identical float sequence (doubling is exact in binary);
  • retry_busy=False → single attempt, busy swallowed with the same log line;
  • non-busy exceptions swallowed with the same log line (status updates must never kill the run loop);
  • sleep=asyncio.sleep keeps the existing test seam — suite patches of mod.asyncio.sleep still observe the retry sleeps;
  • CancelledError still propagates (tenacity re-raises BaseExceptions, except Exception doesn't catch them) — test_run_loop_cancelled_reraises_when_update_busy stays green.

tenacity trap discovered (for the #782 registry)

AsyncRetrying awaits the result of fn(), so fn must be an async callable. _with_busy_retry receives a sync Callable returning a coroutine (lambda: begin_immediate(...)). Without the _wrap_async adapter the coroutine was never awaited → BEGIN IMMEDIATE never ran → transaction() hit cannot COMMIT - no transaction is active, leaking out as PytestUnraisableExceptionWarning (which is an error under the project's filterwarnings = ["error"], so it silently broke every DB write and red the whole suite). The adapter is one line (async def _runner(): return await action()); the dispatcher path is unaffected because its _update is already async def. Worth a line in #782 so future retry→tenacity conversions don't repeat it.

Retry/lifecycle check (#958 trap)

Both loops are local SQLite retries — no network, no billing. SQLITE_BUSY means the statement did not execute, so a retry cannot double-apply a write. tenacity adds no hidden extra retry layer here (no SDK involved); attempt counts and delay sequences are pinned by tests.

TDD / behavioural equivalence

7 characterisation tests were added first and verified green on the old implementation, then the swap landed with the tests untouched:

  • test_database.py: exact sleep ladder on exhaustion; result returned after transient busy (slept exactly delays[0]); non-busy OperationalError raise-through with zero sleeps; DatabaseBusyError.__cause__ is the last busy error.
  • test_telegram_command_dispatcher.py: exponential ladder doubles and caps (computed from module constants); retry_busy=False single-shot without sleep; generic error swallowed without retry.

Detector / baseline

  • Findings for both converted functions are gone; --fail-on-new green.
  • Baseline refreshed (13 → 12 entries): −2 fixed; pure path renames folded in (agent/manager.pyagent/backends/_stream.py, telegram/collector.pytelegram/collector_mixins/* — same findings, files moved by earlier refactors); +1 genuinely new finding recorded (web/scheduler/context.py:236, see table).

Adjudication of the remaining 12 findings (proposal — owner decides, #782)

Finding Class Proposal
agent/backends/_stream.py:463 _await_with_countdown deadline-extension ticker for streaming UX — no operation is ever re-attempted keep (false positive)
services/task_handlers/stats.py handle_stats_all worker-pool batch loop with flood-defer; retries delegated keep (FP, same class as pool_dialogs #1114)
telegram_command_dispatcher.py _run_loop infinite service poll loop (claim → dispatch); sleeps are idle pacing keep
unified_dispatcher.py _run_loop same keep
collector_mixins/collection.py collect_all_channels iterates different channels with inter-channel stagger; errors break/continue, never re-attempt keep (FP)
collector_mixins/stats.py collect_all_stats worker-pool with defer keep (FP)
pool_dialogs.py warm_all_dialogs, leave_channels already adjudicated FP — #1114, AST guards in #1149/#1176 keep (guarded)
pool_dialogs.py delete_dialogs same pattern as leave_channels: retry delegated to run_with_flood_wait_retry, loop iterates different dialogs keep (FP)
web/bootstrap.py _retry_telegram_pool_until_connected condition-driven reconnect loop with cooperative-shutdown checks between waits; tenacity is exception-driven — conversion would bury the shutdown logic keep
web/search/handlers.py _telegram_search_via_worker poll-until-deadline for a DB command status, not a failure retry keep (FP)
web/scheduler/context.py:236 _dedupe_recent_unavailability_events (NEW, counter) group-by with two aggregates (count + max timestamp); collections.Counter covers only the count and would split a single-pass aggregation into two structures propose keep

Registry row (#782 journal):

Дата Находка (файл) Стандартная замена Решение PR/issue
2026-07-10 database/facade.py _with_busy_retry; telegram_command_dispatcher.py _update_command_safely (DB-busy retry) tenacity.AsyncRetrying заменено (этот PR) this PR
2026-07-10 остальные 12 retry/counter-находок предложение «оставить» (таблица выше), финальное слово за владельцем this PR

Not merging — owner reviews (retry→library is in the dual-review category per the task brief).

🤖 Generated with Claude Code

Part of #1132 (axis 2/7 of #1130). Registry: #782 — retry batch, DB-busy
group. Follows up #1198 (Counter/base64 safe subset), which deferred all
retry findings for per-case owner review.

Replaces the two genuine hand-rolled retry loops among the 14 detector
findings with tenacity.AsyncRetrying (already a direct dependency; same
style as error_recovery_service.py / production_limits_service.py). The
other 12 findings are poll/pacing/work-distribution loops — adjudication
table in the PR, no code changes.

1. Database._with_busy_retry (src/database/facade.py) — SQLite-busy retry
   on the shared write path. Behaviour preserved exactly:
   - fixed delay ladder from self._busy_retry_delays_sec via wait_chain
     (no jitter, no exponent);
   - attempts = len(delays)+1 via stop_after_attempt;
   - non-busy OperationalError re-raised immediately;
   - same per-retry/final warning logs; final failure raises
     DatabaseBusyError chained to the last busy error.

2. TelegramCommandDispatcher._update_command_safely — unbounded
   exponential backoff on DatabaseBusyError. Behaviour preserved exactly:
   - ladder INITIAL, 2x, 4x... capped at MAX via wait_exponential;
   - retry_busy=False -> single attempt, busy swallowed;
   - non-busy exceptions swallowed (status updates never kill the loop);
   - sleep=asyncio.sleep keeps the existing test seam; CancelledError
     still propagates (tenacity re-raises BaseException).

tenacity trap discovered (#1132): AsyncRetrying awaits the result of
fn(), so fn must be an async callable. _with_busy_retry receives a
SYNC Callable returning a coroutine (lambda: begin_immediate(...));
without the _wrap_async adapter the coroutine was never awaited, the
BEGIN never ran, and "cannot COMMIT - no transaction is active" leaked
out via PytestUnraisableExceptionWarning (errors under filterwarnings=
["error"]). Recorded for the #782 registry.

Retry/lifecycle check (#958 trap): both loops are local SQLite retries
(SQLITE_BUSY = statement did NOT execute), so a retry cannot double-apply
a write. No network, no billing, no SDK retry layer added.

TDD: 7 characterisation tests added first (green on old impl), then the
swap landed with the tests untouched:
- test_database.py: exact sleep ladder on exhaustion; result after
  transient busy; non-busy OperationalError raise-through (zero sleeps);
  DatabaseBusyError.__cause__ is the last busy error.
- test_telegram_command_dispatcher.py: exponential ladder doubles+caps;
  retry_busy=False single-shot; generic error swallowed.

Detector: findings for both functions gone; --fail-on-new green.
Baseline refreshed (13 -> 12): -2 fixed; pure path renames folded in
(agent/manager.py -> agent/backends/_stream.py, telegram/collector.py
-> collector_mixins/*); +1 new finding recorded
(web/scheduler/context.py:236, counter with two aggregates -> propose
keep).

Co-Authored-By: Claude <noreply@anthropic.com>
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