From 9508004e8eef7bf3d0ef88ae0407f9a97a4c5692 Mon Sep 17 00:00:00 2001 From: Masaori Koshiba Date: Tue, 28 Jul 2026 17:20:18 +0900 Subject: [PATCH] Only restore a retried parent when the retry succeeds A parent that accepts connections but never sends a response was marked down and then restored to the pool by the very next retry probe, even when that probe also failed, so it kept receiving traffic indefinitely while healthy peers took none of the load. handle_response_from_parent() called markParentUp() on entry, before the switch on current.state, so a parent selected as a retry candidate was restored before ATS knew the retry's outcome. markParentUp() zeroes failedAt and failCount; the markParentDown() that follows a failed probe then takes its result->retry branch, which leaves new_fail_count at 0, so available is never re-cleared. The parent therefore returned to the pool with a cleared failure count after every retry_time window, no matter how long it stayed degraded. The CONNECTION_ALIVE branch already calls markParentUp() for a retry that actually succeeded, which is the correct restore point. Removing the entry call restores the behavior that predates #8546: that commit backed out the retry-limiting work and replaced a retryComplete() call -- which only decremented an in-flight retry counter and never touched availability -- with markParentUp(), silently turning "the retry finished" into "the parent is healthy". Two autests pin both outcomes of the retry path. A parent that is still silent when probed stays down; a parent that has recovered is restored. Each was negative-controlled against its own call site: reverting this change fails the first, and removing the CONNECTION_ALIVE call fails only the second. --- doc/admin-guide/files/records.yaml.en.rst | 4 + src/proxy/http/HttpTransact.cc | 6 - .../parent_retry_availability.test.py | 26 +++ ...arent_retry_failure_stays_down.replay.yaml | 215 ++++++++++++++++++ .../parent_retry_success_restores.replay.yaml | 177 ++++++++++++++ 5 files changed, 422 insertions(+), 6 deletions(-) create mode 100644 tests/gold_tests/parent_proxy/parent_retry_availability.test.py create mode 100644 tests/gold_tests/parent_proxy/replays/parent_retry_failure_stays_down.replay.yaml create mode 100644 tests/gold_tests/parent_proxy/replays/parent_retry_success_restores.replay.yaml diff --git a/doc/admin-guide/files/records.yaml.en.rst b/doc/admin-guide/files/records.yaml.en.rst index 043ed594b65..24866ceb2da 100644 --- a/doc/admin-guide/files/records.yaml.en.rst +++ b/doc/admin-guide/files/records.yaml.en.rst @@ -1532,6 +1532,10 @@ Parent Proxy Configuration The amount of time allowed between connection retries to a parent cache that is unavailable. + Once this time has elapsed the parent is selected again as a retry candidate. It is + restored to the pool only if that retry actually succeeds; if the retry fails, the parent + remains unavailable and a further ``retry_time`` must elapse before it is tried again. + .. ts:cv:: CONFIG proxy.config.http.parent_proxy.max_trans_retries INT 2 Limits the number of simultaneous transactions that may retry a parent once the parents diff --git a/src/proxy/http/HttpTransact.cc b/src/proxy/http/HttpTransact.cc index b87412d7b29..4aaa8b81a5b 100644 --- a/src/proxy/http/HttpTransact.cc +++ b/src/proxy/http/HttpTransact.cc @@ -3836,12 +3836,6 @@ HttpTransact::handle_response_from_parent(State *s) TxnDbg(dbg_ctl_http_trans, "(hrfp)"); HTTP_RELEASE_ASSERT(s->current.server == &s->parent_info); - // if this parent was retried from a markdown, then - // notify that the retry has completed. - if (s->parent_result.retry) { - markParentUp(s); - } - simple_or_unavailable_server_retry(s); s->parent_info.state = s->current.state; diff --git a/tests/gold_tests/parent_proxy/parent_retry_availability.test.py b/tests/gold_tests/parent_proxy/parent_retry_availability.test.py new file mode 100644 index 00000000000..51a7b1448d1 --- /dev/null +++ b/tests/gold_tests/parent_proxy/parent_retry_availability.test.py @@ -0,0 +1,26 @@ +""" +Verify the retry path restores a parent only when the retry actually succeeds. +""" +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +Test.Summary = ''' +Both outcomes of a parent retry: a parent that is still up but not serving stays +marked down, and a parent that has recovered is restored to the pool. +''' + +Test.ATSReplayTest(replay_file='replays/parent_retry_failure_stays_down.replay.yaml') +Test.ATSReplayTest(replay_file='replays/parent_retry_success_restores.replay.yaml') diff --git a/tests/gold_tests/parent_proxy/replays/parent_retry_failure_stays_down.replay.yaml b/tests/gold_tests/parent_proxy/replays/parent_retry_failure_stays_down.replay.yaml new file mode 100644 index 00000000000..d2c5fc5bd88 --- /dev/null +++ b/tests/gold_tests/parent_proxy/replays/parent_retry_failure_stays_down.replay.yaml @@ -0,0 +1,215 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# +# A parent that accepts the connection but never sends a response ("up but not +# serving") must stay marked down when a retry probe also fails. +# +# A retried parent must only be restored once a retry actually succeeds. +# markParentUp() zeroes failedAt and failCount, and the markParentDown() that +# follows a failed probe takes its result->retry branch, which leaves +# new_fail_count at 0 and so never re-clears available. Restoring the parent +# before the probe's outcome is known therefore returns it to the pool with a +# clean counter after every retry_time window, no matter how long it stays +# degraded. That is why the restore belongs in the CONNECTION_ALIVE branch of +# HttpTransact::handle_response_from_parent and not at function entry. +# +# The verifier server plays the parent: server-response.delay (4s) exceeds the +# child's transaction_no_activity_timeout_out (2s), so ATS reads zero bytes and +# times out (INACTIVE_TIMEOUT), which enable_parent_timeout_markdowns=1 admits +# as a markdown-eligible failure. +# + +meta: + version: "1.0" + +autest: + description: 'A failed retry probe must not restore a degraded parent' + + server: + name: 'server-degraded' + # Every transaction is abandoned by ATS mid-flight (the response is still + # being held when the timeout fires), so the server may report an incomplete + # transaction cycle. The assertions live in the client statuses and the ATS + # diags log. + return_code: [0, 1] + + client: + name: 'client-degraded' + process_config: + # Each request waits out a 2s ATS timeout, and one waits a further 3s for + # retry_time. The default 5s poll timeout would abandon a transaction + # mid-flight and report no status violation at all. + other_args: '--poll-timeout 30000' + + # Resolve every hostname to loopback. The origin is never actually contacted + # (go_direct=false keeps every request on the parent), but ATS may still run + # an origin DNS lookup on the way to parent selection, and an unresolvable + # name would fail the request before selection runs. + dns: + name: 'dns-degraded' + + ats: + name: 'ts-degraded' + process_config: + enable_cache: false + + records_config: + proxy.config.diags.debug.enabled: 1 + proxy.config.diags.debug.tags: 'parent_select|http_trans' + # NOTE: no_dns_just_forward_to_parent is deliberately NOT set here. It + # routes the request through the parentExists() shortcut in + # HttpTransact::HandleRequest, which tests pRecord::available directly and + # so short-circuits to an error as soon as the parent is marked down -- + # before ParentRoundRobin::selectParent runs. The retry-window logic under + # test lives inside selectParent, so the request has to reach it. + # + # With the cache disabled every request is non-lookupable, and this + # setting (default 1) would then send each one direct to the origin + # instead of the parent -- "request not cacheable, so bypass parent". + proxy.config.http.uncacheable_requests_bypass_parent: 0 + # A read timeout only counts toward markdown when this is enabled. It is + # off by default upstream but enabled in the production config this + # reproduces. + proxy.config.http.parent_proxy.enable_parent_timeout_markdowns: 1 + # One timeout demotes the parent, keeping the test to four requests. + proxy.config.http.parent_proxy.fail_threshold: 1 + # Short enough that a single in-replay delay outlasts the down window. + proxy.config.http.parent_proxy.retry_time: 2 + # No intra-request retry looping; one attempt per request keeps the + # failure accounting one-to-one with the transactions below. + proxy.config.http.parent_proxy.total_connect_attempts: 1 + proxy.config.http.parent_proxy.per_parent_connect_attempts: 1 + # Keep the parent out of the HostStatus map so availability is governed + # purely by retry_time. + proxy.config.http.parent_proxy.self_detect: 0 + # Bound the wait on the silent parent. + proxy.config.http.transaction_no_activity_timeout_out: 2 + proxy.config.url_remap.remap_required: 0 + + # A single parent with go_direct=false: once it is unavailable there is + # nowhere else to go, so selection-time skipping is visible as a 502. + parent_config: + - 'dest_domain=. parent="127.0.0.1:{SERVER_HTTP_PORT}" go_direct=false parent_is_proxy=true' + + log_validation: + diags_log: + contains: + # The retry probe fails, so the parent must be re-marked down rather + # than left available. + - expression: 'Parent retry marked as down 127\.0\.0\.1:\d+' + description: 'The failed retry probe re-marks the parent down' + excludes: + # Note() emitted only by ParentSelectionStrategy::markParentUp. Every + # request in this replay fails, so no retry ever succeeds and the + # parent must never be restored. + - expression: 'http parent proxy 127\.0\.0\.1:\d+ restored with request' + description: 'A failed retry probe must not restore the parent' + +sessions: +- transactions: + # 1: the parent accepts but never responds. ATS times out (504) and the + # failure count reaches fail_threshold, so the parent is marked down as this + # request finishes. + - all: { headers: { fields: [[ uuid, degrade1 ]]}} + client-request: + method: GET + version: "1.1" + scheme: http + url: /degrade1 + headers: + fields: + - [ Host, example.com ] + - [ Content-Length, "0" ] + server-response: + delay: 4s + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, "0" ] + proxy-response: + status: 504 + + # 2: still inside retry_time. The parent is skipped at selection and, with no + # other parent and go_direct=false, ATS returns 502 without touching the leg. + # This confirms the markdown from request 1 actually took effect. + - all: { headers: { fields: [[ uuid, downcheck ]]}} + client-request: + method: GET + version: "1.1" + scheme: http + url: /downcheck + headers: + fields: + - [ Host, example.com ] + - [ Content-Length, "0" ] + server-response: + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, "0" ] + proxy-response: + status: 502 + + # 3: the retry probe. The client delay pushes this request past retry_time, so + # parent selection picks the parent as a retry candidate. The probe reaches the + # still-silent parent and times out (504). The parent must not be restored: + # markParentUp belongs to the CONNECTION_ALIVE branch, which this never takes. + - all: { headers: { fields: [[ uuid, retryprobe ]]}} + client-request: + method: GET + version: "1.1" + scheme: http + url: /retryprobe + delay: 3s + headers: + fields: + - [ Host, example.com ] + - [ Content-Length, "0" ] + server-response: + delay: 4s + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, "0" ] + proxy-response: + status: 504 + + # 4: the retry probe failed, so the parent must still be down. Selection skips + # it and ATS returns 502 without touching the leg. A 504 here would mean the + # failed probe had restored the parent and it was attempted again. + - all: { headers: { fields: [[ uuid, aftercheck ]]}} + client-request: + method: GET + version: "1.1" + scheme: http + url: /aftercheck + headers: + fields: + - [ Host, example.com ] + - [ Content-Length, "0" ] + server-response: + delay: 4s + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, "0" ] + proxy-response: + status: 502 diff --git a/tests/gold_tests/parent_proxy/replays/parent_retry_success_restores.replay.yaml b/tests/gold_tests/parent_proxy/replays/parent_retry_success_restores.replay.yaml new file mode 100644 index 00000000000..91a8942d53e --- /dev/null +++ b/tests/gold_tests/parent_proxy/replays/parent_retry_success_restores.replay.yaml @@ -0,0 +1,177 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# +# A parent that recovers must be restored to the pool by a successful retry. +# +# This is the positive counterpart to parent_retry_failure_stays_down.replay.yaml, +# which covers the case where the retry probe also fails. Together they pin both +# outcomes of the retry path: restore only on success, stay down on failure. +# +# The parent stops being silent partway through the replay, so the retry probe +# sent after retry_time elapses actually succeeds. markParentUp() must then run +# from the CONNECTION_ALIVE branch of handle_response_from_parent, clearing the +# failure count and returning the parent to the pool. +# + +meta: + version: "1.0" + +autest: + description: 'A successful retry restores a parent that had been marked down' + + server: + name: 'server-recovered' + # The first transaction is abandoned by ATS mid-flight (the response is + # still being held when the timeout fires), so the server may report an + # incomplete transaction cycle. + return_code: [0, 1] + + client: + name: 'client-recovered' + process_config: + # The first request waits out a 2s ATS timeout and the retry probe waits a + # further 3s for retry_time, both well past the 5s default. + other_args: '--poll-timeout 30000' + + # Resolve every hostname to loopback, as in the failing-retry scenario. + dns: + name: 'dns-recovered' + + ats: + name: 'ts-recovered' + process_config: + enable_cache: false + + records_config: + proxy.config.diags.debug.enabled: 1 + proxy.config.diags.debug.tags: 'parent_select|http_trans' + # See parent_retry_failure_stays_down.replay.yaml for why + # no_dns_just_forward_to_parent must stay unset and why this must be 0. + proxy.config.http.uncacheable_requests_bypass_parent: 0 + proxy.config.http.parent_proxy.enable_parent_timeout_markdowns: 1 + proxy.config.http.parent_proxy.fail_threshold: 1 + proxy.config.http.parent_proxy.retry_time: 2 + proxy.config.http.parent_proxy.total_connect_attempts: 1 + proxy.config.http.parent_proxy.per_parent_connect_attempts: 1 + proxy.config.http.parent_proxy.self_detect: 0 + proxy.config.http.transaction_no_activity_timeout_out: 2 + proxy.config.url_remap.remap_required: 0 + + parent_config: + - 'dest_domain=. parent="127.0.0.1:{SERVER_HTTP_PORT}" go_direct=false parent_is_proxy=true' + + log_validation: + diags_log: + contains: + # Note() from ParentSelectionStrategy::markParentUp, emitted only when + # a retry succeeds against a parent with a non-zero failure count. + - expression: 'http parent proxy 127\.0\.0\.1:\d+ restored with request' + description: 'A successful retry restores the parent' + excludes: + # The retry succeeds, so it must never be re-marked down. + - expression: 'Parent retry marked as down' + description: 'A successful retry must not re-mark the parent down' + +sessions: +- transactions: + # 1: the parent accepts but never responds. ATS times out (504) and marks it + # down, since fail_threshold is 1. + - all: { headers: { fields: [[ uuid, degrade1 ]]}} + client-request: + method: GET + version: "1.1" + scheme: http + url: /degrade1 + headers: + fields: + - [ Host, example.com ] + - [ Content-Length, "0" ] + server-response: + delay: 4s + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, "0" ] + proxy-response: + status: 504 + + # 2: still inside retry_time, so the parent is skipped at selection and ATS + # returns 502 without touching the leg. + - all: { headers: { fields: [[ uuid, downcheck ]]}} + client-request: + method: GET + version: "1.1" + scheme: http + url: /downcheck + headers: + fields: + - [ Host, example.com ] + - [ Content-Length, "0" ] + server-response: + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, "0" ] + proxy-response: + status: 502 + + # 3: the retry probe. The client delay pushes it past retry_time so the parent + # is selected as a retry candidate, and this time the parent responds promptly + # (no server delay). The 200 proves the retry succeeded and the parent was + # restored from the CONNECTION_ALIVE branch. + - all: { headers: { fields: [[ uuid, retryprobe ]]}} + client-request: + method: GET + version: "1.1" + scheme: http + url: /retryprobe + delay: 3s + headers: + fields: + - [ Host, example.com ] + - [ Content-Length, "0" ] + server-response: + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, "0" ] + proxy-response: + status: 200 + + # 4: the parent is available again, so this request goes straight through + # without waiting for another retry_time window. + - all: { headers: { fields: [[ uuid, aftercheck ]]}} + client-request: + method: GET + version: "1.1" + scheme: http + url: /aftercheck + headers: + fields: + - [ Host, example.com ] + - [ Content-Length, "0" ] + server-response: + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, "0" ] + proxy-response: + status: 200