Retry failed harvest requests, and fail loudly when they don't recover

Both response classes rescued every error while paging, nil'd the
response, and left their loop, so #index returned normally after a single
transient failure: a harvest that indexed 6 of 15 documents exited 0 and
looked like a success. Individual document failures were dropped the same
way, by compacting them out of the page.

Requests now go through retries with an exponential backoff for failures
that tend to be transient -- connection resets, broken pipes, timeouts,
TLS errors, 5xx, rate limiting -- and a request that still can't be
completed raises GeoCombine::Exceptions::HarvestError. A 404 for a single
document is logged and skipped instead, since a record can be indexed but
unreadable.

Responses are checked rather than trusted, since a status code alone
can't tell a page of records from a rejection:

  - a 200 carrying HTML, or any body that isn't a JSON object, is treated
    as a failure worth retrying rather than parsed as a document
  - every page is validated as a page of search results, not just the
    first one through the response factory; a 200 carrying JSON that
    isn't one used to read as "no more results" and end the harvest
  - a result with no link to itself, and a page whose every document was
    skipped, are logged rather than quietly dropped

Closes #208

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Author: thatbudakguy

README.md

@@ -171,6 +171,8 @@ GeoCombine::GeoBlacklightHarvester.configure do
     crawl_delay: 1, # All sites
     debug: true,
     headers: { 'User-Agent' => 'GeoCombine harvester (you@example.edu)' }, # All sites
+    max_retries: 3, # All sites
+    retry_delay: 2, # All sites
     SITE1: {
       crawl_delay: 2, # SITE1 only
       headers: { 'X-Api-Key' => 'secret' }, # SITE1 only
@@ -201,6 +203,12 @@ Be aware that this makes a harvest take considerably longer than it did when the
 
 Headers can be configured either globally for all sites or on a per-site basis, and are sent with every request the harvester makes; headers configured for a site are merged over the global ones. This is one way to get the harvester past a firewall or bot detection (at Stanford, for example, requests carrying a particular header skip Turnstile), and it can also be used to authenticate the harvester. Configuring a `User-Agent` is worthwhile even if you need neither: it identifies your harvester to the sites you harvest, and lets GeoBlacklight's `crawler_detector` recognize it as a bot.
 
+##### Retries (default: 3 retries, starting with a 2 second delay)
+
+Requests that fail in ways that tend to be transient are retried with an exponential backoff: connection resets, broken pipes, timeouts, TLS errors, 5xx responses, rate limiting, and responses that come back with a 200 but aren't JSON (bot mitigation often answers a crawler with a page of HTML). How many times to retry and the delay to start doubling from can be configured globally or per site with `max_retries` and `retry_delay`.
+
+A request that still can't be completed raises `GeoCombine::Exceptions::HarvestError`, so a harvest that couldn't finish exits non-zero instead of looking like a success that happened to index part of the site. Every page of results is checked, not just the first: a 200 whose JSON isn't a page of search results, which is how a WAF or API gateway rejection often arrives, raises rather than reading as the end of the results. The one failure that is logged and skipped instead is a 404 for an individual document, since a record can be indexed but unreadable.
+
 ##### Solr's commitWithin (default: 5000 milliseconds)
 
 Solr's commitWithin option can be configured (in milliseconds) by passing a value under the commit_within key.

lib/geo_combine/exceptions.rb

@@ -10,5 +10,13 @@ class InvalidGeometry < StandardError
 
     class InvalidSchemaVersion < StandardError
     end
+
+    # A harvest could not be completed and should not be treated as a success
+    class HarvestError < StandardError
+    end
+
+    # An individual document could not be found and can be skipped
+    class DocumentNotFound < HarvestError
+    end
   end
 end

lib/geo_combine/geo_blacklight_harvester.rb

@@ -1,6 +1,8 @@
 # frozen_string_literal: true
 
 require 'net/http'
+require 'openssl'
+require 'geo_combine/exceptions'
 require 'geo_combine/logger'
 
 module GeoCombine
@@ -16,6 +18,7 @@ module GeoCombine
   #  - A debug parameter to print out details of what is being harvested and indexed
   #  - crawl delays between requests (globally or on a per site basis)
   #  - headers to send with every request (globally or on a per site basis)
+  #  - retries and backoff for failed requests (globally or on a per site basis)
   #  - Solr's commitWithin parameter (defaults to 5000)
   #  - A document transformer proc to modify a document before indexing (defaults to removing _version_, score, and timestamp)
   # Example: GeoCombine::GeoBlacklightHarvester.new('SITE').index
@@ -82,25 +85,46 @@ def each_document(&block)
     # one; a request that is both paced and freshly connected is much less
     # likely to be turned away by a WAF or other bot mitigation.
     class HttpClient
-      attr_reader :crawl_delay, :headers
-
-      def initialize(crawl_delay: nil, headers: {}, logger: GeoCombine::Logger.logger)
+      # Failures that are worth retrying rather than ending a harvest over
+      RETRIABLE_ERRORS = [
+        Errno::ECONNRESET,
+        Errno::EPIPE,
+        EOFError,
+        Net::OpenTimeout,
+        Net::ReadTimeout,
+        OpenSSL::SSL::SSLError,
+        SocketError
+      ].freeze
+
+      DEFAULT_MAX_RETRIES = 3
+      DEFAULT_RETRY_DELAY = 2
+
+      # Raised internally to signal a response that is worth retrying
+      class RetriableResponse < StandardError; end
+
+      attr_reader :crawl_delay, :headers, :max_retries, :retry_delay
+
+      def initialize(crawl_delay: nil, headers: {}, max_retries: nil, retry_delay: nil,
+                     logger: GeoCombine::Logger.logger)
         @crawl_delay = crawl_delay&.to_f
         @headers = headers.to_h { |name, value| [name.to_s, value.to_s] }
+        @max_retries = (max_retries || DEFAULT_MAX_RETRIES).to_i
+        @retry_delay = (retry_delay || DEFAULT_RETRY_DELAY).to_f
         @logger = logger
       end
 
-      # Fetch a URL and parse the JSON response body
+      # Fetch a URL and parse the JSON response body, retrying failures that
+      # look transient and raising if the harvest cannot go on
       def get_json(url)
-        JSON.parse(get(url))
+        with_retries(url) { parse_json(get(url), url) }
       end
 
       private
 
-      # Fetch a URL and return the response body
+      # Fetch a URL and return the response
       def get(url)
         throttle
-        Net::HTTP.get_response(URI(url), headers).body
+        Net::HTTP.get_response(URI(url), headers)
       end
 
       # Wait out the crawl delay, if one is configured
@@ -110,6 +134,51 @@ def throttle
         @logger.debug "waiting #{crawl_delay}s before the next request"
         sleep(crawl_delay)
       end
+
+      # Retry transient failures with an exponential backoff. A request that
+      # still cannot be completed ends the harvest rather than quietly
+      # truncating it.
+      def with_retries(url)
+        attempts = 0
+
+        begin
+          attempts += 1
+          yield
+        rescue *RETRIABLE_ERRORS, RetriableResponse => e
+          raise Exceptions::HarvestError, "request for #{url} failed after #{attempts} attempts: #{e.message}" if attempts > max_retries
+
+          delay = retry_delay * (2**(attempts - 1))
+          @logger.warn "request for #{url} failed with #{e.message}; retrying in #{delay}s"
+          sleep(delay)
+          retry
+        end
+      end
+
+      # Parse a response body, or raise if it isn't a document we can use.
+      # Bot mitigation often answers with a 200 and a page of HTML, so the
+      # status code alone isn't enough to tell a good response from a bad one.
+      def parse_json(response, url)
+        raise Exceptions::DocumentNotFound, "#{url} was not found" if response.is_a?(Net::HTTPNotFound)
+        raise RetriableResponse, status(response) if retriable?(response)
+
+        raise Exceptions::HarvestError, "request for #{url} failed with #{status(response)}" unless response.is_a?(Net::HTTPSuccess)
+
+        parsed = JSON.parse(response.body.to_s)
+        raise RetriableResponse, 'a JSON body that is not an object' unless parsed.is_a?(Hash)
+
+        parsed
+      rescue JSON::ParserError
+        raise RetriableResponse, "a body that is not JSON (content type: #{response.content_type || 'none'})"
+      end
+
+      # Server errors and rate limiting are worth waiting out and retrying
+      def retriable?(response)
+        response.is_a?(Net::HTTPServerError) || response.is_a?(Net::HTTPTooManyRequests)
+      end
+
+      def status(response)
+        "#{response.code} #{response.message}"
+      end
     end
 
     ##
@@ -150,18 +219,23 @@ def documents
 
           self.page += 1
           @logger.debug "fetching page #{page} @ #{url}"
-
-          begin
-            self.response = client.get_json(url)
-          rescue StandardError => e
-            @logger.error "request for #{url} failed with #{e}"
-            self.response = nil
-          end
+          self.response = client.get_json(url)
+          validate_page!
         end
       end
 
       private
 
+      # Only the first page of a harvest goes through the response factory. A
+      # 200 carrying JSON that isn't a page of search results -- a WAF or API
+      # gateway rejection, say -- would otherwise read as the end of the
+      # results and finish the harvest as though nothing had gone wrong.
+      def validate_page!
+        return if current_page && total_pages
+
+        raise Exceptions::HarvestError, "response for #{url} was not a page of search results"
+      end
+
       def url
         "#{base_url}&page=#{page}"
       end
@@ -193,7 +267,7 @@ def documents
         return enum_for(:documents) unless block_given?
 
         while response && response['data'].any?
-          document_urls = response['data'].collect { |data| data.dig('links', 'self') }.compact
+          document_urls = document_urls_from(response['data'])
 
           yield documents_from_urls(document_urls)
 
@@ -203,26 +277,47 @@ def documents
           url = "#{url}&format=json"
           self.page += 1
           @logger.debug "fetching page #{page} @ #{url}"
-          begin
-            self.response = client.get_json(url)
-          rescue StandardError => e
-            @logger.error "Request for #{url} failed with #{e}"
-            self.response = nil
-          end
+          self.response = client.get_json(url)
+          validate_page!(url)
         end
       end
 
       private
 
+      # Only the first page of a harvest goes through the response factory. A
+      # 200 carrying JSON that isn't a page of search results -- a WAF or API
+      # gateway rejection, say -- would otherwise read as the end of the
+      # results and finish the harvest as though nothing had gone wrong.
+      def validate_page!(url)
+        return if response['data'].is_a?(Array)
+
+        raise Exceptions::HarvestError, "response for #{url} was not a page of search results"
+      end
+
+      # A result with no link to itself can't be fetched; say which one rather
+      # than letting it disappear from the harvest
+      def document_urls_from(data)
+        data.filter_map do |result|
+          url = result.dig('links', 'self')
+          @logger.warn "skipping result with no self link: #{result.inspect}" unless url
+
+          url
+        end
+      end
+
       def documents_from_urls(urls)
         @logger.debug "fetching #{urls.count} documents for page #{page}"
-        urls.map do |url|
+        documents = urls.map do |url|
           client.get_json("#{url}/raw")
-        rescue StandardError => e
-          @logger.error "fetching \"#{url}/raw\" failed with #{e}"
+        rescue Exceptions::DocumentNotFound => e
+          # A record can be indexed but unreadable; log it and move on
+          @logger.warn "skipping document: #{e.message}"
 
           nil
         end.compact
+        @logger.error "skipped every document on page #{page}" if documents.empty? && urls.any?
+
+        documents
       end
     end
 
@@ -243,7 +338,7 @@ def each_page
 
     # The client used to make requests for this site
     def client
-      @client ||= HttpClient.new(crawl_delay:, headers:, logger: @logger)
+      @client ||= HttpClient.new(crawl_delay:, headers:, max_retries:, retry_delay:, logger: @logger)
     end
 
     def base_url
@@ -264,6 +359,16 @@ def crawl_delay
       site[:crawl_delay] || self.class.config[:crawl_delay]
     end
 
+    # How many times to retry a request that fails transiently
+    def max_retries
+      site[:max_retries] || self.class.config[:max_retries]
+    end
+
+    # How long to wait before the first retry; it doubles with each attempt
+    def retry_delay
+      site[:retry_delay] || self.class.config[:retry_delay]
+    end
+
     # Headers to send with every request, e.g. to identify the harvester to a
     # WAF or bot detection, or to authenticate it. Headers configured for the
     # site are merged over any configured globally.