Tell a failed reconnect from a dead idle connection

Only a failure while Net::HTTP checks a reused idle socket means the
push service closed it, and a new connection can take the push. A
failure while connecting again, such as a certificate for another
name, or before the request is written on a new connection, is raised
as it is, so the subscription is invalidated as before. After the
request is written, a dropped connection raises ConnectionLost on new
connections too.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0142qgjggdJ2KDdGk7RF9Xm9

lib/web_push/connections.rb

@@ -6,16 +6,24 @@
 # same address for the same host. Net::HTTP reconnects to its pinned address, never to a new lookup of the host.
 class WebPush::Connections
   class ConnectionLost < StandardError; end
+  class StaleConnection < StandardError; end
 
-  # Net::HTTP checks an idle connection, and reconnects if the push service closed it, just before writing the
-  # request. Until then a dead connection can be replaced without sending the push twice.
-  module WriteTracking
-    attr_reader :request_written
+  # Where a request failed. Before writing it, Net::HTTP checks an idle connection (:checking) and connects again
+  # if the push service closed it (:connecting); then it writes (:sent). Only a failed check means a dead idle
+  # connection that a new one can replace without sending the push twice.
+  module Stages
+    attr_reader :stage
 
-    private def begin_transport(...)
-      @request_written = false
-      super.tap { @request_written = true }
-    end
+    private
+      def begin_transport(...)
+        @stage = :checking
+        super.tap { @stage = :sent }
+      end
+
+      def connect(...)
+        @stage = :connecting if @stage == :checking
+        super
+      end
   end
 
   def initialize(keep_alive_timeout: 30)
@@ -32,20 +40,17 @@ class WebPush::Connections
     address = [ http.address, http.port, http.ipaddr ]
 
     if idle = checkout(address)
-      response = begin
-        idle.request(request)
-      rescue IOError, SystemCallError, OpenSSL::SSL::SSLError => error
-        close(idle)
-        # The push service may have it: don't send it twice, and don't report a closed connection as a TLS failure
-        raise ConnectionLost, "#{error.class}: #{error.message}" if idle.request_written
+      begin
+        return send_over(idle, request, reused: true).tap { checkin(address, idle) }
+      rescue StaleConnection
+        # The push service had closed it: a new connection takes the push
       end
-      return response.tap { checkin(address, idle) } if response
     end
 
-    http.extend WriteTracking
+    http.extend Stages
     http.keep_alive_timeout = @keep_alive_timeout
     http.start
-    http.request(request).tap { checkin(address, http) }
+    send_over(http, request, reused: false).tap { checkin(address, http) }
   end
 
   def shutdown
@@ -58,6 +63,17 @@ class WebPush::Connections
   end
 
   private
+    def send_over(http, request, reused:)
+      http.request(request)
+    rescue IOError, SystemCallError, OpenSSL::SSL::SSLError => error
+      close(http)
+      # Only a connection that was idle can have been dead already: on a new one the error is the push service's
+      raise StaleConnection if reused && http.stage == :checking
+      # The push service may have it: don't send it twice, and don't report a dropped connection as a TLS failure
+      raise ConnectionLost, "#{error.class}: #{error.message}" if http.stage == :sent
+      raise
+    end
+
     def checkout(address)
       @mutex.synchronize do
         forget_after_fork

test/lib/web_push/connections_test.rb

@@ -68,6 +68,35 @@ class WebPush::ConnectionsTest < ActiveSupport::TestCase
     end
   end
 
+  test "a push the service may have received on a new connection isn't reported as a TLS failure either" do
+    with_push_service(drop_request: ->(number) { number == 1 }) do |server|
+      error = assert_raises(WebPush::Connections::ConnectionLost) { @connections.request(pinned_connection(server), push_request) }
+      assert_not_kind_of OpenSSL::OpenSSLError, error
+      assert_equal 1, server.requests.size
+    end
+  end
+
+  test "a TLS failure on a new connection before the push is written is reported as it is" do
+    failing_check = Module.new { private def begin_transport(*) = raise(OpenSSL::SSL::SSLError, "alert right after the handshake") }
+
+    with_push_service do |server|
+      assert_raises(OpenSSL::SSL::SSLError) { @connections.request(pinned_connection(server).extend(failing_check), push_request) }
+      assert_empty server.requests
+    end
+  end
+
+  test "a certificate for another name when reconnecting a cleanly closed connection is reported, even if a new one would work" do
+    certificates = { 2 => "other.test" }
+    with_push_service(hang_up_after_response: :close_notify, certificate: ->(connection) { certificates.fetch(connection, HOST) }) do |server|
+      @connections.request(pinned_connection(server), push_request)
+      assert server.hung_up?
+
+      assert_raises(OpenSSL::SSL::SSLError) { @connections.request(pinned_connection(server), push_request) }
+      assert_equal 1, server.requests.size
+      assert_equal 2, server.connections
+    end
+  end
+
   test "only new, direct TLS connections pinned to an address are pooled" do
     with_push_service do |server|
       unpinned = Net::HTTP.new(HOST, server.port, nil).tap { it.use_ssl = true }