Do not open the circuit breaker when only one host failed by justinpersaud · Pull Request #267 · Expensify/Bedrock-PHP

justinpersaud · 2026-08-04T14:54:03Z

Explanation of Change

The circuit breaker opened when a single node was down. We don't need the breaker to jump in here, only when many hosts are down or the cluster isn't responding.

Related Issues

For #258

For https://github.com/Expensify/Expensify/issues/657702

Tests

AI tests below

No automated suite in this repo, so I ran a manual script in the Expensidev VM (dead loopback ports for unreachable nodes, local Bedrock on 8888 for healthy ones). 6/6:

 [PASS] 1: one failed host never opens it, failover still serves
 [PASS] 2: two failed hosts never open it
 [PASS] 3: three failed hosts open it at the threshold
 [PASS] 4: a dead cluster opens it at the threshold
 [PASS] 5: one dead host IS the whole cluster, so it opens
 [PASS] 6: a healthy call clears both keys

Phase 1 is the 2026-08-04 shape and fails on main. Happy to attach the script.

Deployment

I followed the steps in the README to ensure this PR is deployed properly

Version bump and tag still to come.

A call counted toward the breaker whenever it exhausted its own candidate host list, and the blacklist routinely narrows that list to a single host, so one dead node was indistinguishable from a dead cluster. On 2026-08-04 a single dead auth node (db1.sjc) opened the breaker on 8 web boxes while the other 5 nodes were healthy and db2.sjc had been leading for 5 minutes. Because call() checks the breaker before host selection, the ~880 requests that failed fast never tried a healthy node. Of the 1,432 requests that threw, 1,321 had failed against exactly one host. Blacklisting already routes around a single bad host, so only count a failure once the call failed against more than one distinct host. Capped at the number of configured hosts so a single-host cluster still trips.

A cluster keeps its leader while a majority of full peers are reachable (numLoggedInFullPeers * 2 >= numFullPeers), so auth's 5 full nodes plus one permafollower still serve with 3 nodes up. Failing against 2 hosts therefore proves nothing: it happens when the blacklist has narrowed a call's candidate list to two dead hosts while the cluster is fine, which 75 requests hit on 2026-08-04.

iwiznia

Have to be honest, I am not sure what the circuit breaker is doing or is supposed to be doing...
Like if one command is bad and gets attempted in all hosts and fails, it will stop processing all requests?

iwiznia · 2026-08-05T18:59:41Z

        $rawRequest .= $body;

        $response = null;
+        $this->failedHosts = [];


Probably better in getPossibleHosts?

Why is that? Happy to move but not sure why

iwiznia · 2026-08-05T19:01:31Z

            return;
        }
+        // A cluster only loses its leader once 3 nodes are unreachable; below that, blacklisting routes around them.
+        if (count($this->failedHosts) < min(3, count($this->mainHostConfigs + $this->failoverHostConfigs))) {


Why are you adding main + failovers? That count will be 8 in production.. I think you only want failoverHostConfigs here.

This is an array union, so it doesn't add both together. It should still be 6.

Oh, TIL that's how it works

iwiznia · 2026-08-05T19:02:22Z

        if ($this->circuitBreakerThreshold <= 0 || !$this->isApcuAvailable()) {
            return;
        }
+        // A cluster only loses its leader once 3 nodes are unreachable; below that, blacklisting routes around them.


Not sure I get this, we can have a cluster with 3 nodes and have 6 in total, so with 3 nodes down (and 3 up) we have a working cluster.
Shouldn't we be circuit breaking when all hosts are down?

fair, i will move it to all hosts

justinpersaud · 2026-08-05T21:10:04Z

I am not sure what the circuit breaker is doing or is supposed to be doing...

The goal of the circuit breaker is to stop creating new requests up to the readTimeout when the auth cluster cannot handle any additional requests due to whatever reason. It prevents overwhelming an overloaded cluster during fires. It is not one bad command, but rather, 10 failures. Any success within that clears it.

…ing it Failing against 3 hosts assumed auth's node count and still left hosts that could serve the call. Require every configured host to have failed instead, which is the condition the breaker exists for and needs no per-cluster tuning. Timeouts are never retried on another host, so they can never fail against all of them. They bypass the check and count on their own, keeping the behaviour from Bedrock-PHP #266.

justinpersaud · 2026-08-06T21:26:19Z

We have a tag conflict here now from another PR by @Valforte. I will have to move this over to 2.3.7.. and then we skip 2.3.6 I guess?

justinpersaud · 2026-08-06T21:33:18Z

actually since this one was already here first @Valforte I am going to use the 2.3.6 tag here and you can use 2.3.7 on yours when it is ready

iwiznia · 2026-08-06T22:27:26Z

ok, make sure that the tag points to the correct commit when you create it.

iwiznia · 2026-08-06T22:28:02Z

Oh you already created it and it's pointing to the correct commit...

justinpersaud · 2026-08-07T00:25:04Z

Yeah we should really just add the tag automation to this repo

flodnv

The comments are more confusing than the code 😅

flodnv · 2026-08-07T10:20:32Z

        try {
            $response = $this->doCall($method, $headers, $body);
+        } catch (TimeoutError $e) {
+            // A timeout doesn't mark the host as failed, so it records none and could never pass the check below.


What? This seems like an AI comment that taken out of its context, is meaningless (and very confusing)

rewording it

flodnv · 2026-08-07T10:22:26Z

        if ($this->circuitBreakerThreshold <= 0 || !$this->isApcuAvailable()) {
            return;
        }
+        // While any host can still serve the call, blacklisting the bad ones is enough.


I also don't understand this comment. Why is it talking about blacklisting, when what we're doing is an early return?

Blacklisting prevents requests to a host that have failed for some reason already, but the early return here is because there are still candidate hosts that can serve the request and we don't want to prevent a request from going to them

flodnv

To be honest, I don't think I fully understand this PR. Does it mean that if we get 10 different command timeouts in 1 minute, the circuit breaker will kick in? So if a user causes 10 timeouts in 1 minute, the circuit breaker will kick in and prevent all other requests? If so, that feels off 🤔

Co-authored-by: Florent De'Neve <florent@expensify.com>

justinpersaud · 2026-08-07T13:31:41Z

Does it mean that if we get 10 different command timeouts in 1 minute, the circuit breaker will kick in? So if a user causes 10 timeouts in 1 minute, the circuit breaker will kick in and prevent all other requests?

Yes, but this behaviour was introduced in the other PR Ionatan made regarding timeouts. Also, this assumes zero requests succeed in that time period. Remember, any successful request resets the counter.

justinpersaud · 2026-08-07T18:50:56Z

We may not need this PR after all. I misdiagnosed the original issue I was looking at earlier in the week. Details here https://expensify.slack.com/archives/C05CBC62HGW/p1786125723436259?thread_ts=1783698356.886189&cid=C05CBC62HGW

If there are no issues over the weekend with the LAX maintenance, then I think we just close this.

justinpersaud · 2026-08-10T12:36:06Z

No issues, so closing this as not needed

justinpersaud · 2026-08-10T12:36:14Z

(noting I will delete the tags now too)

justinpersaud self-assigned this Aug 4, 2026

justinpersaud marked this pull request as ready for review August 4, 2026 17:23

justinpersaud changed the title ~~Trip the circuit breaker only when the cluster lost quorum~~ Do not open the circuit breaker when only one host failed Aug 5, 2026

justinpersaud force-pushed the jpersaud-breaker-cluster-threshold branch from 4ee9a82 to f4fc3e0 Compare August 5, 2026 15:45

justinpersaud added 3 commits August 5, 2026 11:50

Reword the failed host check comment

dcb770f

Merge branch 'main' into jpersaud-breaker-cluster-threshold

47e16c9

justinpersaud requested review from flodnv and iwiznia August 5, 2026 17:45

Bump version to 2.3.6

e23d1d1

iwiznia requested changes Aug 5, 2026

View reviewed changes

justinpersaud added 2 commits August 5, 2026 17:13

Name the actual reason timeouts bypass the host check

b4c4088

iwiznia previously approved these changes Aug 6, 2026

View reviewed changes

bump tag to 2.3.7

98177ca

justinpersaud dismissed iwiznia’s stale review via 98177ca August 6, 2026 21:27

Set version back to 2.3.6

610d4b6

justinpersaud requested a review from iwiznia August 6, 2026 21:33

iwiznia previously approved these changes Aug 6, 2026

View reviewed changes

flodnv reviewed Aug 7, 2026

View reviewed changes

update comments

7e73ee6

justinpersaud dismissed iwiznia’s stale review via 7e73ee6 August 7, 2026 12:17

flodnv reviewed Aug 7, 2026

View reviewed changes

Comment thread src/Client.php Outdated

Comment thread src/Client.php Outdated

justinpersaud and others added 2 commits August 7, 2026 09:23

Update src/Client.php

382ebac

Co-authored-by: Florent De'Neve <florent@expensify.com>

Update src/Client.php

a366efe

Co-authored-by: Florent De'Neve <florent@expensify.com>

Remove trailing whitespace

addef7a

iwiznia approved these changes Aug 7, 2026

View reviewed changes

justinpersaud closed this Aug 10, 2026

Uh oh!

Conversation

justinpersaud commented Aug 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Explanation of Change

Related Issues

Tests

Deployment

Uh oh!

iwiznia left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinpersaud commented Aug 5, 2026

Uh oh!

justinpersaud commented Aug 6, 2026

Uh oh!

justinpersaud commented Aug 6, 2026

Uh oh!

iwiznia commented Aug 6, 2026

Uh oh!

iwiznia commented Aug 6, 2026

Uh oh!

justinpersaud commented Aug 7, 2026

Uh oh!

flodnv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flodnv left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

justinpersaud commented Aug 7, 2026

Uh oh!

justinpersaud commented Aug 7, 2026

Uh oh!

justinpersaud commented Aug 10, 2026

Uh oh!

justinpersaud commented Aug 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

justinpersaud commented Aug 4, 2026 •

edited

Loading

flodnv left a comment •

edited

Loading