From f61ed838cded3d6b0f243c98722ef32b144de052 Mon Sep 17 00:00:00 2001 From: Yukihiro Okada Date: Mon, 8 Jun 2026 11:46:00 +0900 Subject: [PATCH 1/3] fix: improve POST retry tests and document idempotency assumption - Add comment explaining duplicate-query risk when retrying POST on connection-level errors (Faraday::TimeoutError, ConnectionFailed) - Fix test name: Timeout::Error -> Faraday::TimeoutError - Parameterize retryable status test to cover 502, 503, and 504 - Add test for retry_timeout exhaustion raising TrinoHttpError(408) --- lib/trino/client/statement_client.rb | 4 +++ spec/statement_client_spec.rb | 40 ++++++++++++++++++---------- 2 files changed, 30 insertions(+), 14 deletions(-) diff --git a/lib/trino/client/statement_client.rb b/lib/trino/client/statement_client.rb index 9d6bba91..868471b0 100644 --- a/lib/trino/client/statement_client.rb +++ b/lib/trino/client/statement_client.rb @@ -77,6 +77,10 @@ def post_query_request! init_request(req) end rescue Faraday::TimeoutError, Faraday::ConnectionFailed + # POST /v1/statement is not idempotent; retrying connection-level errors + # risks submitting duplicate queries. This is intentional under the + # assumption that a connection failure means Trino never received the + # request (e.g. rejected by a load balancer before reaching the server). throw :retry_with_backoff rescue => e exception! e diff --git a/spec/statement_client_spec.rb b/spec/statement_client_spec.rb index b83754ed..0f6df5be 100644 --- a/spec/statement_client_spec.rb +++ b/spec/statement_client_spec.rb @@ -174,7 +174,7 @@ } end - it "retries POST on Timeout::Error" do + it "retries POST on Faraday::TimeoutError" do attempts = 0 stub_request(:post, "localhost/v1/statement"). with(body: query, headers: headers). @@ -210,22 +210,34 @@ expect(attempts).to eq 2 end - it "retries POST on 503 response" do - attempts = 0 + [502, 503, 504].each do |status| + it "retries POST on #{status} response" do + attempts = 0 + stub_request(:post, "localhost/v1/statement"). + with(body: query, headers: headers). + to_return(lambda { |req| + attempts += 1 + if attempts < 2 + {status: status, body: "service unavailable"} + else + {status: 200, body: response_json.to_json} + end + }) + + sc = StatementClient.new(faraday, query, options) + expect(sc.query_id).to eq "queryid" + expect(attempts).to eq 2 + end + end + + it "raises TrinoHttpError after retry_timeout is exhausted on POST" do stub_request(:post, "localhost/v1/statement"). with(body: query, headers: headers). - to_return(lambda { |req| - attempts += 1 - if attempts < 2 - {status: 503, body: "service unavailable"} - else - {status: 200, body: response_json.to_json} - end - }) + to_return(status: 503, body: "service unavailable") - sc = StatementClient.new(faraday, query, options) - expect(sc.query_id).to eq "queryid" - expect(attempts).to eq 2 + expect do + StatementClient.new(faraday, query, options.merge(retry_timeout: 0)) + end.to raise_error(Trino::Client::TrinoHttpError, "Trino API error due to timeout") end it "does not retry POST on deterministic 4xx errors" do From 795c01e1974fd001add2aa98f7282d24b020d8a6 Mon Sep 17 00:00:00 2001 From: "Yukihiro Okada (Yuki)" Date: Mon, 8 Jun 2026 12:12:39 +0900 Subject: [PATCH 2/3] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- lib/trino/client/statement_client.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/trino/client/statement_client.rb b/lib/trino/client/statement_client.rb index 868471b0..da9c32db 100644 --- a/lib/trino/client/statement_client.rb +++ b/lib/trino/client/statement_client.rb @@ -77,10 +77,10 @@ def post_query_request! init_request(req) end rescue Faraday::TimeoutError, Faraday::ConnectionFailed - # POST /v1/statement is not idempotent; retrying connection-level errors - # risks submitting duplicate queries. This is intentional under the - # assumption that a connection failure means Trino never received the - # request (e.g. rejected by a load balancer before reaching the server). + # POST /v1/statement is not idempotent; retrying transport-level errors + # (timeouts/connection failures) can submit duplicate queries. This is + # intentional under the assumption the failure happened before Trino + # received the request (e.g. rejected by a load balancer upstream). throw :retry_with_backoff rescue => e exception! e From 9d43c15b35a9d08fdeaf7bd2781d69e6eb3b0c30 Mon Sep 17 00:00:00 2001 From: Yukihiro Okada Date: Mon, 8 Jun 2026 12:20:13 +0900 Subject: [PATCH 3/3] fix: also assert status == 408 in retry_timeout exhaustion test --- spec/statement_client_spec.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/spec/statement_client_spec.rb b/spec/statement_client_spec.rb index 0f6df5be..b2895bec 100644 --- a/spec/statement_client_spec.rb +++ b/spec/statement_client_spec.rb @@ -237,7 +237,9 @@ expect do StatementClient.new(faraday, query, options.merge(retry_timeout: 0)) - end.to raise_error(Trino::Client::TrinoHttpError, "Trino API error due to timeout") + end.to raise_error(Trino::Client::TrinoHttpError, "Trino API error due to timeout") do |e| + expect(e.status).to eq 408 + end end it "does not retry POST on deterministic 4xx errors" do