Repository navigation
Enforce fail_after in Batch#mark_retryable - #12
Merged
Merged
Conversation
…ry path shares. connection_lost, service_unavailable, and the batch-level ECONNRESET/timeout rescues (apnsp8 and apns2 alike) call Batch#mark_retryable directly, bypassing Delivery#mark_retryable's own fail_after check entirely — only FCM's retry path reaches that check today. A notification whose connection keeps dropping retries every ~10s forever, with no way to stop it: neither the per-notification nor the batch-level entry point ever consults fail_after. mark_all_retryable already loops back into mark_retryable per-notification, so moving the check here reaches every transport uniformly instead of duplicating it across each Delivery subclass's rescue blocks. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015ENCThkiQAQ4zF699fuaHG
|
Warning Review limit reachedNext included review available in 40 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. Your 54 included PR review attempts over the past 7 days set your current allowance at 2 reviews per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
drn
approved these changes
Aug 23, 2026
drn
approved these changes
Aug 23, 2026
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
2 participants
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.
A notification retrying a dropped connection could retry forever with no way to stop it — neither entry point that most transports actually use ever checked
fail_after. ORD-867 root cause; nexus side setsfail_afterat creation in nexus#1512, but that value was never being read for apnsp8/apns2.The gap:
connection_lost,service_unavailable, and the batch-levelECONNRESET/timeout rescues (apnsp8 and apns2 alike) callBatch#mark_retryabledirectly. OnlyDelivery#mark_retryablechecksfail_after— and only FCM's retry path reaches that method. Every iOS retry path bypasses the check entirely.The fix: move the check into
Batch#mark_retryableitself — the one placemark_all_retryablealso loops back into, so every transport's retry path reaches it uniformly instead of needing the check duplicated across eachDeliverysubclass.✅ Verified:
ADAPTER=sqlite3 bundle exec rspec spec/unit/daemon/— 454 examples, 0 failures, 1 pre-existing pending (unrelated FCM spec, already pending before this change).