Add tests for retry logic based on response status code

Author: Drvi

Project.toml

@@ -4,14 +4,18 @@ version = "0.1.0"
 
 [compat]
 CloudBase = "1"
+HTTP = "1"
 ReTestItems = "1"
+Sockets = "1"
 Test = "1"
 julia = "1.8"
 
 [extras]
 CloudBase = "85eb1798-d7c4-4918-bb13-c944d38e27ed"
+HTTP = "cd3eb016-35fb-5094-929b-558a96fad6f3"
 ReTestItems = "817f1d60-ba6b-4fd5-9520-3cf149f6a823"
+Sockets = "6462fe0b-24de-5631-8697-dd941f90decc"
 Test = "8dfed614-e22c-5e08-85e1-65c5234f0b40"
 
 [targets]
-test = ["CloudBase", "ReTestItems", "Test"]
+test = ["CloudBase", "HTTP", "ReTestItems", "Sockets", "Test"]

test/azure_blobs_exception_tests.jl

@@ -41,7 +41,7 @@
                 @test false # Should have thrown an error
             catch e
                 @test e isa ErrorException
-                @test occursin("400 Bad Request", e.msg)
+                @test occursin("400 Bad Request", e.msg) # Should this be 403 Forbidden? We've seen that with invalid SAS tokens
                 @test occursin("Authentication information is not given in the correct format", e.msg)
             end
 
@@ -151,3 +151,141 @@
         @test res == 1 # Rust CResult::Error
     end
 end # @testitem
+
+@testitem "BlobStorage retries" setup=[InitializeRustStore] begin
+    using CloudBase.CloudTest: Azurite
+    import CloudBase
+    using ObjectStore: blob_get!, blob_put, AzureCredentials
+    import HTTP
+    import Sockets
+
+    max_retries = InitializeRustStore.max_retries
+
+    function test_status(method, response_status, headers=nothing)
+        @assert method === :GET || method === :PUT
+        nretries = Ref(0)
+        response_body = "response body from the dummy server"
+        account = "myaccount"
+        container = "mycontainer"
+        shared_key_from_azurite = "Eby8vdM02xNOcqFlqUwJPLlmEtlCDXJ1OUzFT50uSRZ6IFsuFq2UVErCz4I6tq/K1SZFPTOtr/KBHBeksoGMGw=="
+
+        (port, tcp_server) = Sockets.listenany(8081)
+        http_server = HTTP.serve!(tcp_server) do request::HTTP.Request
+            if request.method == "GET" && request.target == "/$account/$container/_this_file_does_not_exist"
+                # This is the exploratory ping from connect_and_test in lib.rs
+                return HTTP.Response(404, "Yup, still doesn't exist")
+            end
+            nretries[] += 1
+            response = isnothing(headers) ? HTTP.Response(response_status, response_body) : HTTP.Response(response_status, headers, response_body)
+            return response
+        end
+
+        baseurl = "http://127.0.0.1:$port/$account/$container/"
+        creds = AzureCredentials(account, container, shared_key_from_azurite, baseurl)
+
+        try
+            method === :GET && blob_get!(joinpath(baseurl, "blob"), zeros(UInt8, 5), creds)
+            method === :PUT && blob_put(joinpath(baseurl, "blob"), codeunits("a,b,c"), creds)
+            @test false # Should have thrown an error
+        catch e
+            @test e isa ErrorException
+            @test occursin(string(response_status), e.msg)
+            response_status < 500 && (@test occursin("response body from the dummy server", e.msg))
+        finally
+            close(http_server)
+        end
+        wait(http_server)
+        return nretries[]
+    end
+
+    # See https://learn.microsoft.com/en-us/rest/api/searchservice/http-status-codes
+
+    @testset "400: Bad Request" begin
+        # Returned when there's an error in the request URI, headers, or body. The response body
+        # contains an error message explaining what the specific problem is.
+        nretries = test_status(:GET, 400)
+        @test nretries == 1 broken=true
+        nretries = test_status(:PUT, 400)
+        @test nretries == 1 broken=true
+    end
+
+    @testset "403: Forbidden" begin
+        # Returned when you pass an invalid api-key.
+        nretries = test_status(:GET, 403)
+        @test nretries == 1 broken=true
+        nretries = test_status(:PUT, 403)
+        @test nretries == 1 broken=true
+    end
+
+    @testset "404: Not Found" begin
+        nretries = test_status(:GET, 404)
+        @test nretries == 1
+    end
+
+    @testset "405: Method Not Supported" begin
+        nretries = test_status(:GET, 405, ["Allow" => "PUT"])
+        @test nretries == 1 broken=true
+        nretries = test_status(:PUT, 405, ["Allow" => "GET"])
+        @test nretries == 1 broken=true
+    end
+
+    @testset "409: Conflict" begin
+        # Returned when write operations conflict.
+        # NOTE: We currently don't retry but maybe we should? This is probably a case where the
+        # retry logic should add more noise to the backoff so that multiple writers don't collide on retry.
+        nretries = test_status(:GET, 409)
+        @test nretries == max_retries broken=true
+        nretries = test_status(:PUT, 409)
+        @test nretries == max_retries broken=true
+    end
+
+    @testset "412: Precondition Failed" begin
+        # Returned when an If-Match or If-None-Match header's condition evaluates to false
+        nretries = test_status(:GET, 412)
+        @test nretries == 1
+        nretries = test_status(:PUT, 412)
+        @test nretries == 1
+    end
+
+    @testset "413: Content Too Large" begin
+        # https://learn.microsoft.com/en-us/rest/api/storageservices/put-blob?tabs=shared-access-signatures#remarks
+        nretries = test_status(:PUT, 413)
+        @test nretries == 1 broken=true
+    end
+
+    @testset "429: Too Many Requests" begin
+        # TODO: We probably should respect the Retry-After header, but we currently don't
+        # (and we don't know if Azure actually sets it)
+        # NOTE: This can happen when Azure is throttling us, so it might be a good idea to retry with some
+        # larger initial backoff (very eager retries probably only make the situation worse).
+        nretries = test_status(:GET, 429, ["Retry-After" => 10])
+        @test nretries == max_retries broken=true
+        nretries = test_status(:PUT, 429, ["Retry-After" => 10])
+        @test nretries == max_retries broken=true
+    end
+
+    @testset "502: Bad Gateway" begin
+        # This error occurs when you enter HTTP instead of HTTPS in the connection.
+        nretries = test_status(:GET, 502)
+        @test nretries == 1 broken=true
+        nretries = test_status(:PUT, 502)
+        @test nretries == 1 broken=true
+    end
+
+    @testset "503: Service Unavailable" begin
+        # NOTE: This seems similar to 429 and the Azure docs specifically say:
+        #    Important: In this case, we highly recommend that your client code back off and wait before retrying
+        nretries = test_status(:GET, 503)
+        @test nretries == max_retries broken=true
+        nretries = test_status(:PUT, 503)
+        @test nretries == max_retries broken=true
+    end
+
+    @testset "504: Gateway Timeout" begin
+        # Azure AI Search listens on HTTPS port 443. If your search service URL contains HTTP instead of HTTPS, a 504 status code is returned.
+        nretries = test_status(:GET, 504)
+        @test nretries == 1 broken=true
+        nretries = test_status(:PUT, 504)
+        @test nretries == 1 broken=true
+    end
+end

test/common_testsetup.jl

@@ -2,7 +2,8 @@
     using ObjectStore
     # Since we currently only support centralized configs, we need to have one that is compatible
     # with all the tests (some of the tests would take too long if we use default values).
-    max_retries = 5
-    retry_timeout_sec = 5
+    max_retries = 2
+    retry_timeout_sec = 2
+    ObjectStore.init_rust_store(ObjectStore.RustStoreConfig(max_retries, retry_timeout_sec))
     init_object_store(ObjectStoreConfig(max_retries, retry_timeout_sec))
 end

test/runtests.jl

@@ -2,5 +2,5 @@ using ReTestItems
 using ObjectStore
 
 withenv("RUST_BACKTRACE"=>1) do
-    runtests(ObjectStore, testitem_timeout=120, nworkers=1)
-end
+    runtests(ObjectStore, testitem_timeout=180, nworkers=1)
+end