fix: harden album buffer error paths - #169
Merged
Merged
Conversation
- Resolve album.future inside _handle_album_turn itself, not only via the caller's finally block - Roll back the buffer entry if scheduling the max-wait watchdog task raises during add_message - Log the suppressed album-wait exception at warning, not debug - Document the sync invariant that keeps debounce/max-wait flush race-free Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Hardens four latent robustness gaps in the Telegram photo-album buffering state machine (
telegram/album_buffer.pyandtelegram/bot.py), found during a code-reduction audit and tracked as separate items in #163.Why
None of the four gaps caused an observed failure, but two were "safe by accident": correct only because of how the code happens to be called or scheduled today, not because anything enforces it. A future refactor could silently reintroduce a stuck
asyncio.Future(a hung Telegram turn) or a double-processed album. Fixing all four together keeps the album-buffer state machine self-consistent in one small, reviewable change rather than four disconnected diffs against the same file.How
_handle_album_turnnow resolvesalbum.futurein its ownfinallyblock, so every validation early-return (too many photos, missing photo, oversized photo/album) is covered even if the method is ever called outside_process_flushed_album.AlbumBuffer.add_messagenow rolls back theself._buffersentry ifasyncio.create_taskfor the max-wait watchdog raises, so a failed schedule can't leave an album with no timer able to flush it._safe_handle_update(album-wait path) is now logged atwarninginstead ofdebug, with a comment explaining why it must not re-raise (the album turn already reports its own user-facing error).AlbumBuffer._flushnow documents the synchronous, no-awaitinvariant that keeps the debounce/max-wait race safe, so a future change that adds anawaitbeforealbum.processed = Truedoesn't silently reintroduce a double-flush.Tests
_handle_album_turnresolves a pendingalbum.futureon a validation early-return, called directly (not via_process_flushed_album)create_taskfailure duringadd_messagedoes not leave the album orphaned inself._bufferswarninglevel_flushtwice back-to-back (simulating the debounce/max-wait race) flushes exactly oncepytest tests/test_telegram_album_buffer.py tests/test_telegram_bot.py -qpasses (245 passed)ruff check,ruff format --checkpass with no new warningsmypy src/blacki/telegram/passes with no new warningsRelated Issues
Closes #163