throw exception when download of first diff encounters client error

Author: lonvia

src/osmium/replication/server.py

@@ -144,8 +144,14 @@ def collect_diffs(self, start_id: int, max_size: Optional[int] = None,
             contains the MergeInputReader with the data and `newest` is a
             sequence id of the most recent diff available.
 
-            Returns None if there was an error during download or no new
-            data was available.
+            Returns None if there was no new data was available.
+
+            If there is an error during the download, then the function will
+            simply return the already downloaded data. If the reported
+            error is a client error (HTTP 4xx) and happens during the download
+            of the first diff, then a ::request.HTTPError:: is raised: this
+            condition is likely to be permanent and the caller should not
+            simply retry without investigating the cause.
         """
         # must not read data newer than the published sequence id
         # or we might end up reading partial data
@@ -168,8 +174,19 @@ def collect_diffs(self, start_id: int, max_size: Optional[int] = None,
                 and current_id <= newest.sequence:
             try:
                 diffdata = self.get_diff_block(current_id)
-            except:  # noqa: E722
-                LOG.error("Error during diff download. Bailing out.")
+            except requests.RequestException as ex:
+                if start_id == current_id \
+                        and ex.response is not None \
+                        and (ex.response.status_code % 100 == 4):
+                    # If server directly responds with a client error,
+                    # reraise the exception to signal a potentially permanent
+                    # error.
+                    LOG.error("Permanent server error: %s", ex.response)
+                    raise ex
+                # In all other cases, process whatever diffs we have and
+                # encourage a retry.
+                LOG.error("Error during diff download: %s", ex)
+                LOG.error("Bailing out.")
                 diffdata = ''
             if len(diffdata) == 0:
                 if start_id == current_id:

test/test_replication.py

@@ -10,6 +10,7 @@
 import uuid
 
 import pytest
+import requests
 
 from werkzeug.wrappers import Response
 
@@ -391,6 +392,23 @@ def test_apply_diffs_permanent_error(httpserver, caplog):
     httpserver.expect_ordered_request('/000/000/100.opl')\
               .respond_with_data('not a file', status=404)
 
+    with caplog.at_level(logging.ERROR):
+        with rserv.ReplicationServer(httpserver.url_for(''), "opl") as svr:
+            h = CountingHandler()
+            with pytest.raises(requests.HTTPError, match='404'):
+                svr.apply_diffs(h, 100, 10000)
+
+    assert 'Permanent server error' in caplog.text
+
+
+def test_apply_diffs_transient_error_first_diff(httpserver, caplog):
+    httpserver.expect_ordered_request('/state.txt').respond_with_data("""\
+        sequenceNumber=100
+        timestamp=2017-08-26T11\\:04\\:02Z
+    """)
+    httpserver.expect_request('/000/000/100.opl')\
+              .respond_with_data('not a file', status=503)
+
     with caplog.at_level(logging.ERROR):
         with rserv.ReplicationServer(httpserver.url_for(''), "opl") as svr:
             h = CountingHandler()
@@ -421,7 +439,7 @@ def test_apply_diffs_permanent_error_later_diff(httpserver, caplog):
     assert 'Error during diff download' in caplog.text
 
 
-def test_apply_diffs_transient_error(httpserver, caplog):
+def test_apply_diffs_transient_error_later_diff(httpserver, caplog):
     httpserver.expect_ordered_request('/state.txt').respond_with_data("""\
         sequenceNumber=101
         timestamp=2017-08-26T11\\:04\\:02Z