Skip to content

Album buffering: future/buffer leaks on error paths #163

Description

@QueryPlanner

What

Found during a code-reduction audit of telegram/bot.py's photo-album buffering (now in telegram/album_buffer.py after #160/#161/#162). Four related edge cases in the debounce/max-wait/flush state machine, none reproduced with a failing test yet — reported for triage, not yet confirmed as a live incident.

  1. Every early return in _handle_album_turn skips resolving album.future. Validation failures (too many photos, missing photo, oversized photo/album) return without calling album.future.set_result(None). This only stays safe today because the caller, _process_flushed_album's finally block, resolves the future regardless of how _handle_album_turn exits — but any future refactor that invokes _handle_album_turn directly (a test, a new call path) would silently leak that future forever, stalling any concurrent _safe_handle_update doing await asyncio.shield(active_album.future).
  2. A buffer can be leaked if scheduling the max-wait watchdog fails. In AlbumBuffer.add_message, the album is stored in self._buffers before asyncio.create_task(self._max_wait(album)) succeeds. If that task creation raises, the album has no watchdog and only the debounce timer can flush it — if the debounce timer is also lost (e.g. a subsequent exception before it's set), the buffered album lingers until shutdown().
  3. Silent exception swallow in _safe_handle_update (bot.py:305): after await asyncio.shield(active_album.future), a bare except Exception as exc: logger.debug(...) downgrades any real turn-processing exception to a debug log with no user-facing error message.
  4. Fragile (currently safe) race between the debounce callback and the max-wait callback: both call AlbumBuffer._flush, guarded by album.processed. The check-then-set is not atomic against the event loop scheduling both callbacks in the same iteration — currently safe only because neither callback awaits between the check and the set. If _flush is ever made async or gains an await before setting processed, this becomes a real race.

Why it matters

These are latent robustness gaps in code that runs on every multi-photo album a user sends. None currently cause an observed failure, but items 1 and 4 are "safe by accident" — a future change elsewhere in the file could silently reintroduce a stuck asyncio.Future (hung task) or a double-flush.

Priority

Low — no known live incident; these are defensive-robustness gaps in already-shipped code.

Level of Effort

Small (S) — each fix is a few lines: resolve the future in a finally inside _handle_album_turn itself (or before every early return), guard the max-wait task creation, restore user visibility on the swallowed exception, or convert the processed-check into a single atomic operation.

Sources

Passing criteria / definition of done

  • A new test in tests/test_telegram_album_buffer.py directly calls _handle_album_turn (or an equivalent isolated path) with a validation failure and asserts album.future is resolved (not left pending).
  • A new test simulates asyncio.create_task raising during add_message and asserts the album is not left in self._buffers indefinitely (either scheduling succeeds or the album is cleaned up).
  • The swallowed exception at _safe_handle_update is either surfaced as a user-facing error message or explicitly documented with a comment explaining why silent debug-logging is intentional; a test asserts the chosen behavior.
  • pytest tests/test_telegram_album_buffer.py tests/test_telegram_bot.py -q passes.
  • ruff check, ruff format --check, and mypy src/blacki/telegram/ all pass with no new warnings.

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions