fix(brpc): Skip backup when retry budget is exhausted

docs/cn/client.md

@@ -721,6 +721,8 @@ options.retry_policy = &g_my_retry_policy;
 
 工作机制如下：如果response没有在backup_request_ms内返回，则发送另外一个请求，哪个先回来就取哪个。新请求会被尽量送到不同的server。注意如果backup_request_ms大于超时，则backup request总不会被发送。backup request会消耗一次重试次数。backup request不意味着server端cancel。
 
+如果没有剩余的重试次数，则跳过backup request，继续等待当前请求完成或整体RPC超时。
+
 ChannelOptions.backup_request_ms影响该Channel上所有RPC，单位毫秒，默认值-1（表示不开启）。Controller.set_backup_request_ms()可修改某次RPC的值。
 
 用户可以通过继承[brpc::BackupRequestPolicy](https://github.com/apache/brpc/blob/master/src/brpc/backup_request_policy.h)自定义策略（backup_request_ms和熔断backup request）。 比如根据延时调节backup_request_ms或者根据错误率熔断部分backup request。

docs/en/client.md

@@ -601,6 +601,8 @@ If the server does not respond and connection is good, retry is not triggered. I
 
 How it works: If response does not return within the timeout specified by backup_request_ms, send another request, take whatever the first returned. New request will be sent to a different server that never tried before by best efforts. NOTE: If backup_request_ms is greater than timeout_ms, backup request will never be sent. backup request consumes one retry. backup request does not imply a server-side cancellation.
 
+If no retry budget remains, the backup request is skipped. The in-flight request continues until it completes or the overall RPC timeout expires.
+
 ChannelOptions.backup_request_ms affects all RPC via the Channel, unit is milliseconds, Default value is -1(disabled), Controller.set_backup_request_ms() overrides value for one RPC.
 
 ### Timeout is not reached

src/brpc/controller.cpp

@@ -869,12 +869,14 @@ void Controller::OnVersionedRPCReturned(const CompletionInfo& info,
         return;
     }
 
+    const bool retry_limit_reached = _current_call.nretry >= _max_retry;
     if ((!_error_code && _retry_policy == nullptr) ||
-        _current_call.nretry >= _max_retry) {
+        (_error_code != EBACKUPREQUEST && retry_limit_reached)) {
         goto END_OF_RPC;
     }
     if (_error_code == EBACKUPREQUEST) {
-        if (nullptr != _backup_request_policy && !_backup_request_policy->DoBackup(this)) {
+        if (!retry_limit_reached && nullptr != _backup_request_policy &&
+            !_backup_request_policy->DoBackup(this)) {
             // No need to do backup request.
             _error_code = saved_error;
             CHECK_EQ(0, bthread_id_unlock(info.id));
@@ -893,6 +895,13 @@ void Controller::OnVersionedRPCReturned(const CompletionInfo& info,
             SetFailed(rc, "Fail to add timer");
             goto END_OF_RPC;
         }
+        if (retry_limit_reached) {
+            // The backup timer is not an RPC failure. Keep the in-flight call
+            // and its original deadline even when no retry budget remains.
+            _error_code = saved_error;
+            CHECK_EQ(0, bthread_id_unlock(info.id));
+            return;
+        }
         if (!SingleServer()) {
             if (_accessed == nullptr) {
                 _accessed = ExcludedServers::Create(

test/brpc_channel_unittest.cpp

@@ -2238,6 +2238,57 @@ class ChannelTest : public ::testing::Test{
         StopAndJoin();
     }
 
+    void TestBackupRequestWithoutRetryBudget(bool single_server, bool async,
+                                            int timeout_ms,
+                                            bool consume_retry_first) {
+        ASSERT_EQ(0, StartAccept(_ep));
+        brpc::Channel channel;
+        SetUpChannel(&channel, single_server, false);
+
+        // Establish the connection before testing backup/retry ordering.
+        test::EchoRequest req;
+        test::EchoResponse res;
+        req.set_message(__FUNCTION__);
+        brpc::Controller warmup;
+        warmup.set_timeout_ms(1000);
+        CallMethod(&channel, &warmup, &req, &res, async);
+        ASSERT_FALSE(warmup.Failed()) << warmup.ErrorText();
+
+        auto attempts = std::make_shared<butil::atomic<int>>(0);
+        _svc.SetMockFunc([attempts, consume_retry_first](
+                google::protobuf::RpcController* controller,
+                const test::EchoRequest*, test::EchoResponse*,
+                google::protobuf::Closure*) {
+            if (attempts->fetch_add(1, butil::memory_order_relaxed) == 0 &&
+                consume_retry_first) {
+                static_cast<brpc::Controller*>(controller)->SetFailed(
+                    brpc::EOVERCROWDED, "Consume the only retry before backup");
+                return;
+            }
+            bthread_usleep(100000);
+        });
+
+        brpc::Controller cntl;
+        cntl.set_max_retry(consume_retry_first ? 1 : 0);
+        cntl.set_backup_request_ms(20);
+        cntl.set_timeout_ms(timeout_ms);
+        CallMethod(&channel, &cntl, &req, &res, async);
+        if (timeout_ms < 0 || timeout_ms > 100) {
+            EXPECT_EQ(0, cntl.ErrorCode()) << cntl.ErrorText();
+            EXPECT_EQ("received " + req.message(), res.message());
+        } else {
+            EXPECT_EQ(brpc::ERPCTIMEDOUT, cntl.ErrorCode()) << cntl.ErrorText();
+        }
+        EXPECT_FALSE(cntl.has_backup_request());
+        EXPECT_EQ(consume_retry_first ? 1 : 0, cntl.retried_count());
+        // Also finish delayed server work when the client has timed out.
+        bthread_usleep(150000);
+        EXPECT_EQ(consume_retry_first ? 2 : 1,
+                  attempts->load(butil::memory_order_relaxed));
+        StopAndJoin();
+        _svc.SetMockFunc({});
+    }
+
     class BackupRequestPolicyImpl : public brpc::BackupRequestPolicy {
     public:
         int32_t GetBackupRequestMs(const brpc::Controller*) const override {
@@ -3396,6 +3447,25 @@ TEST_F(ChannelTest, backup_request_policy) {
     }
 }
 
+TEST_F(ChannelTest, backup_request_without_retry_budget) {
+    const int timeouts_ms[] = {1000, 60, -1};
+    for (bool single_server : {false, true}) {
+        for (bool async : {false, true}) {
+            for (int timeout_ms : timeouts_ms) {
+                for (bool consume_retry_first : {false, true}) {
+                    SCOPED_TRACE(testing::Message()
+                        << "single_server=" << single_server
+                        << " async=" << async
+                        << " timeout_ms=" << timeout_ms
+                        << " consume_retry_first=" << consume_retry_first);
+                    TestBackupRequestWithoutRetryBudget(
+                        single_server, async, timeout_ms, consume_retry_first);
+                }
+            }
+        }
+    }
+}
+
 TEST_F(ChannelTest, selective_channel_ignores_late_subdone_after_timeout) {
     DelayedCloseEchoService service;
     brpc::Server server;