Add circuit breaker by devinbileck · Pull Request #15 · bisq-network/bisq-relay

devinbileck · 2026-04-19T05:24:44Z

Resolves #7

Update to the latest LTS version.

The previous approach of copying app start scripts from build directories to the root and adjusting them no longer works in newer Gradle versions. Therefore, there is no longer a need to depend on the bisq-gradle module.

Previously, the masked string would have the same length as the original string. This caused unnecessarily long masked strings to be logged. Now the masked string is reduced with a fixed mask length.

All push messages should contain encrypted content.

This attempts to reduce user-visible duplicates caused by retries of the same logical push notification.

… classes

- Rename integration test classes - Use MockitoBean rather than deprecated MockBean

rodvar

Very solid implementation overall @devinbileck - it feels like a big upgrade.

I see the PR resolves the issue referred in the description + implementing a full retry mechanism

Just weary how would this play out with the circuit breaking? Can you confirm / document how this is intended to work?

Each retry counts as a separate failure against the breaker. With maxAttempts=3 and minimumNumberOfCalls=5, a real outage trips the breaker after ~2 logical sends instead of 5 — could be desirable, but it's not what the threshold values suggest..?
Retry can amplify load during a partial outage (the very thing the breaker is trying to prevent) until the breaker opens.
The non-goal wording in the issue suggests we wanted to address retry separately or not at all?

And also added some minor comments on the code hope you find them helpful !

rodvar · 2026-04-29T02:05:21Z

+     */
+    private static boolean isBreakerRelevantApnsRejection(final String errorCode) {
+        return switch (errorCode) {
+            case "TooManyRequests", "ServiceUnavailable", "InternalServerError" -> true;


nit - use enum instead of hardcoded strings?

rodvar · 2026-04-29T02:05:47Z

+                false,
+                errorCode,
+                errorMessage,
+                messagingErrorCode == MessagingErrorCode.UNREGISTERED


can we add a test for this case?
same for auth-failure case

Sure, I can take a look.

rodvar · 2026-04-29T02:07:31Z

+@SpringBootTest
+@AutoConfigureMockMvc
+@ActiveProfiles("integrationtest")
+class RelayControllerIT {


With the removal of RelayController into this class the RelayControllerTest has been lost with important validations (hex decoding, Bisq v1/v2 token format detection, missing-param errors) - consider bringing that back in?

I thought I covered all the various validations here, but I will review it again.

rodvar · 2026-04-29T02:09:54Z

+resilience4j.retry.configs.default.waitDuration=500ms
+resilience4j.retry.configs.default.enableExponentialBackoff=true
+resilience4j.retry.configs.default.exponentialBackoffMultiplier=2
+resilience4j.retry.configs.default.retryExceptions=\


overlaps with recordExceptions is that intentional?

Yes, the overlap is intentional. retryExceptions and recordExceptions serve different purposes: retry decides which failures are worth another attempt within the same logical send, while the circuit breaker decides which final failures should contribute to opening the breaker. The shared exceptions are transient transport/async failures, so they should be retried and, if still failing after retries/time limit, recorded by the breaker. ProviderFailureException is intentionally only in recordExceptions, not retryExceptions, so provider outage/throttle/server-failure signals count against the breaker without adding retry load.

rodvar · 2026-04-29T02:11:38Z

+        this.meterRegistryProvider = meterRegistryProvider;
+    }
+
+    @PostConstruct


I think this is minor given Spring lifecycle but worth noting: A CB transition that fires before the listener is attached is missed.

Yes, a circuit breaker event emitted before the observability listener is attached would be missed. In this case the impact is limited to observability: state transition logs and the custom short-circuit counter could miss very early startup events, but breaker behavior itself is unaffected. In normal Spring lifecycle this should be unlikely because registration happens during context initialization before requests are served.

rodvar · 2026-04-29T02:21:16Z

+
+        // Wait for the OPEN wait duration to elapse
+        // This assumes waitDurationInOpenState <= 3s (in test properties)
+        Awaitility.await().pollDelay(Duration.ofSeconds(3)).until(() -> true);


consider changing to something like:

Awaitility.await() .atMost(Duration.ofSeconds(5)) .until(() -> cb.tryAcquirePermission());

in all occurrences to avoid forcing a Thread.sleep(3000) when its not necessary

Good point. The existing Awaitility usage was just a fixed delay and always waited the full 3 seconds. I'll replace it with a condition-based wait. I'm avoiding tryAcquirePermission() in the wait condition because it consumes a HALF_OPEN permit, which can affect the actual send being tested.

devinbileck · 2026-05-01T04:39:10Z

I see the PR resolves the issue referred in the description + implementing a full retry mechanism

Just weary how would this play out with the circuit breaking? Can you confirm / document how this is intended to work?

Each retry counts as a separate failure against the breaker. With maxAttempts=3 and minimumNumberOfCalls=5, a real outage trips the breaker after ~2 logical sends instead of 5 — could be desirable, but it's not what the threshold values suggest..?

Retry can amplify load during a partial outage (the very thing the breaker is trying to prevent) until the breaker opens.

The current composition is CircuitBreaker --> TimeLimiter --> Retry --> provider call.
Since the breaker is the outermost decorator, it observes one final outcome per logical send, not each retry attempt. So maxAttempts=3 does not make minimumNumberOfCalls=5 trip after ~2 logical sends; it still takes about 5 failed logical sends before the breaker evaluates/opens.
Retry can still amplify provider traffic while the breaker is closed, because a logical send can perform up to 3 attempts. That is intentional but bounded by retry max attempts, exponential backoff, the 5s time limiter, and the breaker preventing calls entirely once open.
I can add documentation near the composition/config to make that explicit.

The non-goal wording in the issue suggests we wanted to address retry separately or not at all?

I will re-word that.

And also added some minor comments on the code hope you find them helpful !

Yes! Thank you.

devinbileck · 2026-07-02T23:24:47Z

@rodvar I believe I have addressed your feedback. Please review again when you get a chance.

rodvar

utACK

all concerns have been addressed 💪

just added a small nit for your consideration, great stuff!

rodvar · 2026-07-03T02:01:24Z

+        // This assumes waitDurationInOpenState <= 3s (in test properties)
+        Awaitility.await()
+                .atMost(Duration.ofSeconds(3))
+                .until(cb::tryAcquirePermission);


nit - wouldn't it be better to use cb.transitionToHalfOpenState and then do the probe?

That would change the intent of the test - it should verify the OPEN wait-duration path. I’ll replace it with a passive wait for the configured open duration.

rodvar · 2026-07-03T07:08:13Z

LGTM!

devinbileck added 14 commits April 22, 2026 15:52

Bump Java to 25

7dedcc8

Update to the latest LTS version.

Fix broken Gradle task

0729693

The previous approach of copying app start scripts from build directories to the root and adjusting them no longer works in newer Gradle versions. Therefore, there is no longer a need to depend on the bisq-gradle module.

Bump dependencies to latest versions

6946a00

Misc cleanup

4f513ab

Adjust masking logic to reduce masked string length

4cd88e9

Previously, the masked string would have the same length as the original string. This caused unnecessarily long masked strings to be logged. Now the masked string is reduced with a fixed mask length.

PushNotificationMessage.encrypted should be NotBlank

90eb90a

All push messages should contain encrypted content.

Implement PushNotificationMessage coalescingKey

c10e5c8

This attempts to reduce user-visible duplicates caused by retries of the same logical push notification.

Use AnnotationUtils for PushProvider lookup to handle inherited/proxy…

231fec3

… classes

Update integration tests

4156d22

- Rename integration test classes - Use MockitoBean rather than deprecated MockBean

Clean up RelayController to remove use of Optional parameters

a2dfe34

Implement circuit breaker

b5a36fc

Update grafana dashboards

8919d90

Add data-only flag info to FCM client initialization log

256a0ea

Bump version to 2.1.0

d355fbf

devinbileck force-pushed the add-circuit-breaker branch from 86f811e to d355fbf Compare April 23, 2026 04:51

devinbileck marked this pull request as ready for review April 28, 2026 14:45

rodvar suggested changes Apr 29, 2026

View reviewed changes

Resolve code review feedback

fde3241

rodvar approved these changes Jul 3, 2026

View reviewed changes

Update resilience tests to use dynamic CB wait durations

d038e3a

This was referenced Jul 15, 2026

Hardening #20

Open

For Cycle 83 bisq-network/compensation#2075

Closed

Uh oh!

Conversation

devinbileck commented Apr 19, 2026

Uh oh!

rodvar 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!

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!

devinbileck commented May 1, 2026

Uh oh!

devinbileck commented Jul 2, 2026

Uh oh!

rodvar 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!

rodvar commented Jul 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants