From 69f239e67455d305b7a38d9d17239a2135d3b112 Mon Sep 17 00:00:00 2001
From: Hua Jie Chen <clark@thanx.com>
Date: Sun, 23 Aug 2026 18:29:30 -0400
Subject: [PATCH] Enforce fail_after in Batch#mark_retryable, the choke point
 every retry path shares.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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
---
 lib/rpush/daemon/batch.rb      |  8 ++++++
 spec/unit/daemon/batch_spec.rb | 47 ++++++++++++++++++++++++++++++++--
 2 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/lib/rpush/daemon/batch.rb b/lib/rpush/daemon/batch.rb
index c7cfa22b..7037b4a6 100644
--- a/lib/rpush/daemon/batch.rb
+++ b/lib/rpush/daemon/batch.rb
@@ -39,7 +39,15 @@ def unresolved
         end
       end
 
+      # Most transports bypass Delivery#mark_retryable's own fail_after check by calling
+      # this directly, so it's enforced here too rather than assuming it's redundant.
       def mark_retryable(notification, deliver_after)
+        if notification.fail_after && notification.fail_after < Time.now
+          mark_failed(notification, nil,
+            "Notification failed to be delivered before #{notification.fail_after.strftime('%Y-%m-%d %H:%M:%S')}.")
+          return
+        end
+
         @mutex.synchronize do
           @retryable[deliver_after] ||= []
           @retryable[deliver_after] << notification
diff --git a/spec/unit/daemon/batch_spec.rb b/spec/unit/daemon/batch_spec.rb
index 3df19752..4a8ffdc5 100644
--- a/spec/unit/daemon/batch_spec.rb
+++ b/spec/unit/daemon/batch_spec.rb
@@ -1,8 +1,8 @@
 require 'unit_spec_helper'
 
 describe Rpush::Daemon::Batch do
-  let(:notification1) { double(:notification1, id: 1, delivered: false, failed: false) }
-  let(:notification2) { double(:notification2, id: 2, delivered: false, failed: false) }
+  let(:notification1) { double(:notification1, id: 1, delivered: false, failed: false, fail_after: nil) }
+  let(:notification2) { double(:notification2, id: 2, delivered: false, failed: false, fail_after: nil) }
   let(:batch) { Rpush::Daemon::Batch.new([notification1, notification2]) }
   let(:store) { double.as_null_object }
   let(:time) { Time.now }
@@ -96,6 +96,22 @@
         expect(batch.retryable).to eq(time => [notification1])
       end
     end
+
+    context 'when one of the notifications has an expired fail_after' do
+      let(:notification2) {
+        double(:notification2, id: 2, delivered: false, failed: false, fail_after: time - 1)
+      }
+
+      it 'fails that notification instead of retrying it' do
+        batch.mark_all_retryable(time, error)
+        expect(batch.failed.values.flatten).to eq [notification2]
+      end
+
+      it 'still retries the other notification' do
+        batch.mark_all_retryable(time, error)
+        expect(batch.retryable).to eq(time => [notification1])
+      end
+    end
   end
 
   describe 'mark_failed' do
@@ -133,6 +149,33 @@
       batch.mark_retryable(notification1, time)
       expect(batch.retryable).to eq(time => [notification1])
     end
+
+    context 'when fail_after has already passed' do
+      let(:notification1) {
+        double(:notification1, id: 1, delivered: false, failed: false, fail_after: time - 1)
+      }
+
+      it 'marks it failed instead of retryable' do
+        batch.mark_retryable(notification1, time)
+        expect(batch.failed).to eq([nil, "Notification failed to be delivered before #{(time - 1).strftime('%Y-%m-%d %H:%M:%S')}."] => [notification1])
+      end
+
+      it 'does not add it to the retryable set' do
+        batch.mark_retryable(notification1, time)
+        expect(batch.retryable).to be_empty
+      end
+    end
+
+    context 'when fail_after has not yet passed' do
+      let(:notification1) {
+        double(:notification1, id: 1, delivered: false, failed: false, fail_after: time + 60)
+      }
+
+      it 'still marks it retryable' do
+        batch.mark_retryable(notification1, time)
+        expect(batch.retryable).to eq(time => [notification1])
+      end
+    end
   end
 
   describe 'unresolved' do