Put the whole unfurl, lookups included, under one deadline

The deadline in Opengraph::Fetch started after the lookup of the pasted
host, and an unfurl looks up more hosts outside any fetch: the canonical
URL's, and the image's, once to check its content type and again to
validate it. Each of those waits as long as the resolver takes to give
up. Move the deadline up to UnfurlLinksController, around everything the
link leads to; Opengraph::Fetch keeps its per-operation timeouts and
makes no retries.

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

app/controllers/unfurl_links_controller.rb

@@ -1,8 +1,9 @@
 class UnfurlLinksController < ApplicationController
-  def create
-    opengraph = Opengraph::Metadata.from_url(url_param)
+  # The pasted link decides which hosts an unfurl looks up and fetches from, and how quickly they answer.
+  DEADLINE = 10.seconds
 
-    if opengraph.valid?
+  def create
+    if opengraph = unfurl(url_param)
       render json: opengraph
     else
       head :no_content
@@ -13,4 +14,14 @@ class UnfurlLinksController < ApplicationController
     def url_param
       params.require(:url)
     end
+
+    # One deadline for everything the link leads to: DNS lookups, redirects, the page, the image check.
+    def unfurl(url)
+      Timeout.timeout(DEADLINE) do
+        Opengraph::Metadata.from_url(url).then { |opengraph| opengraph if opengraph.valid? }
+      end
+    rescue Timeout::Error
+      Rails.logger.warn "Gave up unfurling #{url} after #{DEADLINE.inspect}"
+      nil
+    end
 end

app/models/opengraph/fetch.rb

@@ -5,8 +5,7 @@ class Opengraph::Fetch
   ALLOWED_DOCUMENT_CONTENT_TYPE = "text/html"
   MAX_BODY_SIZE = 5.megabytes
   MAX_REDIRECTS = 10
-  TIMEOUT = 7.seconds   # to connect (TLS included), and for each read and write, as Webhook does
-  DEADLINE = 10.seconds # for the whole fetch: redirects, headers and body
+  TIMEOUT = 7.seconds # to connect (TLS included), and for each read and write, as Webhook does
 
   class TooManyRedirectsError < StandardError; end
   class RedirectDeniedError < StandardError; end
@@ -24,25 +23,22 @@ class Opengraph::Fetch
   end
 
   private
-    # A member triggers this from a web request, and the host on the other end decides how fast it answers.
-    # The timeouts bound each operation, but Net::HTTP reads headers and body in as many reads as the host
-    # likes, so only a deadline over the whole fetch stops one that sends a byte at a time.
+    # The timeouts bound each operation. A host can still send its headers or body a byte at a time,
+    # in as many reads as it likes: UnfurlLinksController puts the whole unfurl under one deadline.
     def request(url, request_class, ip:)
-      Timeout.timeout(DEADLINE) do
-        MAX_REDIRECTS.times do
-          Net::HTTP.start(url.host, url.port, ipaddr: ip, use_ssl: url.scheme == "https", **timeouts) do |http|
-            http.request request_class.new(url) do |response|
-              if response.is_a?(Net::HTTPRedirection)
-                url, ip = resolve_redirect(response["location"])
-              else
-                yield response
-              end
+      MAX_REDIRECTS.times do
+        Net::HTTP.start(url.host, url.port, ipaddr: ip, use_ssl: url.scheme == "https", **timeouts) do |http|
+          http.request request_class.new(url) do |response|
+            if response.is_a?(Net::HTTPRedirection)
+              url, ip = resolve_redirect(response["location"])
+            else
+              yield response
             end
           end
         end
-
-        raise TooManyRedirectsError
       end
+
+      raise TooManyRedirectsError
     end
 
     # Without max_retries: 0, Net::HTTP sends a GET or HEAD that timed out a second time.

test/controllers/unfurl_links_controller_test.rb

@@ -75,7 +75,94 @@ class UnfurlLinksControllerTest < ActionDispatch::IntegrationTest
     assert_equal "Hey!", JSON.parse(response.body)["title"]
   end
 
+  test "create gives up on a host that accepts the connection and never answers" do
+    with_local_host do |url, connections|
+      stub_const(Opengraph::Fetch, :TIMEOUT, 0.2.seconds) do
+        assert_unfurls_nothing_within(1.second) { post unfurl_link_url, params: { url: url } }
+      end
+      assert_equal 1, connections.size, "the GET is not retried"
+    end
+  end
+
+  test "create gives up on headers that trickle in past the deadline" do
+    trickle_headers = ->(client) do
+      client.write "HTTP/1.1 200 OK\r\nX-Padding: "
+      100.times { client.write "x"; sleep 0.05 }
+      client.write "\r\nContent-Type: text/html\r\nContent-Length: 2\r\n\r\nok"
+    end
+
+    with_local_host(trickle_headers) do |url|
+      stub_const(UnfurlLinksController, :DEADLINE, 0.5.seconds) do
+        assert_unfurls_nothing_within(1.5.seconds) { post unfurl_link_url, params: { url: url } }
+      end
+    end
+  end
+
+  test "create gives up on a body that trickles in past the deadline" do
+    trickle_body = ->(client) do
+      client.write "HTTP/1.1 200 OK\r\nContent-Type: text/html\r\nContent-Length: 100\r\n\r\n"
+      100.times { client.write "x"; sleep 0.05 }
+    end
+
+    with_local_host(trickle_body) do |url|
+      stub_const(UnfurlLinksController, :DEADLINE, 0.5.seconds) do
+        assert_unfurls_nothing_within(1.5.seconds) { post unfurl_link_url, params: { url: url } }
+      end
+    end
+  end
+
+  test "create gives up on redirects that keep coming past the deadline" do
+    stub_dns_resolution("1.2.3.4")
+    WebMock.stub_request(:get, "https://www.example.com/").to_return do
+      sleep 0.2
+      { status: 302, headers: { location: "https://www.example.com/" } }
+    end
+
+    stub_const(UnfurlLinksController, :DEADLINE, 0.5.seconds) do
+      assert_unfurls_nothing_within(1.second) { post unfurl_link_url, params: { url: "https://www.example.com" } }
+    end
+  end
+
+  test "create gives up on a host name that takes too long to look up" do
+    Resolv.stubs(:getaddresses).with { sleep 2 }.returns([ "1.2.3.4" ]) # each lookup takes 2 s
+    WebMock.stub_request(:get, "https://www.example.com/").to_return(status: 200, body: "<html></html>", headers: { content_type: "text/html" })
+
+    stub_const(UnfurlLinksController, :DEADLINE, 0.5.seconds) do
+      assert_unfurls_nothing_within(1.second) { post unfurl_link_url, params: { url: "https://www.example.com" } }
+    end
+  end
+
   private
+    # A real socket, because WebMock reads the whole response before the code under test sees any of it.
+    # The guard lets the local address through, as it would a public one.
+    def with_local_host(respond = ->(client) { })
+      server = TCPServer.new("127.0.0.1", 0)
+      connections = Queue.new
+      Thread.new do
+        loop do
+          client = server.accept
+          connections << client
+          client.readpartial(1024)
+          respond.call(client)
+        end
+      rescue IOError, SystemCallError
+      end
+
+      RestrictedHTTP::PrivateNetworkGuard.stubs(:resolve).returns("127.0.0.1")
+      WebMock.disable!
+      yield "http://www.example.com:#{server.addr[1]}/", connections
+    ensure
+      WebMock.enable!
+      server&.close
+    end
+
+    def assert_unfurls_nothing_within(limit)
+      started = Process.clock_gettime(Process::CLOCK_MONOTONIC)
+      yield
+      assert_response :no_content
+      assert_operator Process.clock_gettime(Process::CLOCK_MONOTONIC) - started, :<, limit
+    end
+
     def stub_successful_request(url: "https://www.example.com/", title: "Hey!", description: "desc..")
       WebMock.stub_request(:get, url).to_return(
         status: 200,

test/models/opengraph/fetch_test.rb

@@ -74,54 +74,6 @@ class Opengraph::FetchTest < ActiveSupport::TestCase
     end
   end
 
-  test "#fetch_document gives up on a host that accepts the connection and never answers" do
-    with_local_host do |url, connections|
-      stub_const(Opengraph::Fetch, :TIMEOUT, 0.2.seconds) do
-        assert_gives_up_within(1.second, Net::ReadTimeout) { @fetch.fetch_document(url, ip: "127.0.0.1") }
-      end
-      assert_equal 1, connections.size, "the GET is not retried"
-    end
-  end
-
-  test "#fetch_document gives up on headers that trickle in past the deadline" do
-    trickle_headers = ->(client) do
-      client.write "HTTP/1.1 200 OK\r\nX-Padding: "
-      100.times { client.write "x"; sleep 0.05 }
-      client.write "\r\nContent-Type: text/html\r\nContent-Length: 2\r\n\r\nok"
-    end
-
-    with_local_host(trickle_headers) do |url|
-      stub_const(Opengraph::Fetch, :DEADLINE, 0.5.seconds) do
-        assert_gives_up_within(1.5.seconds, Timeout::Error) { @fetch.fetch_document(url, ip: "127.0.0.1") }
-      end
-    end
-  end
-
-  test "#fetch_document gives up on a body that trickles in past the deadline" do
-    trickle_body = ->(client) do
-      client.write "HTTP/1.1 200 OK\r\nContent-Type: text/html\r\nContent-Length: 100\r\n\r\n"
-      100.times { client.write "x"; sleep 0.05 }
-    end
-
-    with_local_host(trickle_body) do |url|
-      stub_const(Opengraph::Fetch, :DEADLINE, 0.5.seconds) do
-        assert_gives_up_within(1.5.seconds, Timeout::Error) { @fetch.fetch_document(url, ip: "127.0.0.1") }
-      end
-    end
-  end
-
-  test "#fetch_document gives up on redirects that keep coming past the deadline" do
-    stub_dns_resolution("1.2.3.4")
-    WebMock.stub_request(:get, "https://www.example.com/").to_return do
-      sleep 0.2
-      { status: 302, headers: { location: "https://www.example.com/" } }
-    end
-
-    stub_const(Opengraph::Fetch, :DEADLINE, 0.5.seconds) do
-      assert_gives_up_within(1.second, Timeout::Error) { @fetch.fetch_document(@url, ip: "1.2.3.4") }
-    end
-  end
-
   test "#fetch_document is empty following redirects that never finish" do
     WebMock.stub_request(:get, "https://www.example.com/")
       .to_return(status: 302, headers: { location: "https://www.example.com/" })
@@ -163,31 +115,4 @@ class Opengraph::FetchTest < ActiveSupport::TestCase
     def large_body_content
       "x" * (Opengraph::Fetch::MAX_BODY_SIZE + 1)
     end
-
-    # A real socket, because WebMock reads the whole response before the code under test sees any of it.
-    def with_local_host(respond = ->(client) { })
-      server = TCPServer.new("127.0.0.1", 0)
-      connections = Queue.new
-      Thread.new do
-        loop do
-          client = server.accept
-          connections << client
-          client.readpartial(1024)
-          respond.call(client)
-        end
-      rescue IOError, SystemCallError
-      end
-
-      WebMock.disable!
-      yield URI.parse("http://www.example.com:#{server.addr[1]}/"), connections
-    ensure
-      WebMock.enable!
-      server&.close
-    end
-
-    def assert_gives_up_within(limit, error, &block)
-      started = Process.clock_gettime(Process::CLOCK_MONOTONIC)
-      assert_raises(error, &block)
-      assert_operator Process.clock_gettime(Process::CLOCK_MONOTONIC) - started, :<, limit
-    end
 end