Maas client tests and retry cases improvement

[Ksiona]

MaaS client: retry behaviour for a storage failover

A database leader switchover arrives at the client as 405, not as 5xx, so the usual
"retry 5xx, fail fast on 4xx" rule does not survive it. This adds the retry policy that does,
and fixes the watch loop that spun instead of backing off.

Scope. This covers the control plane — the REST channel between the client and maas-agent.
The data plane is untouched: rabbit-blue-green opens a channel per binding with no retry, and
kafka-blue-green-consumer has no failover test. The ticket is not fully closed by this PR.

What changed

Retries now key on the response, not only on IOException. Retryable: 5xx, 429, and
405 when the body carries a maas-service error naming a read-only database. A plain 405 from
an ingress or a removed route stays permanent and fails fast.

401 is not retried. The token source refreshes on its own polling interval, so a retry inside
the backoff re-sends the same token, and M2MInterceptor has already made its own 401 round trip
by the time the response reaches us.

One knob bounds the whole call: maas.http.retry.max-total-duration-ms, 60s by default.
Attempt count and backoff growth are derived from it, and each attempt is capped by what is left
via Call.timeout(), so the worst case a caller sees is that duration rather than the duration
plus one maas.http.timeout. Zero disables retries. Backoff is exponential with jitter, delegated
to Failsafe.

deleteTopic is deliberately not retried. It is not idempotent: a lost response after a
completed delete would make the next attempt return empty lists, and the method would report the
topic as still present.

The watch long poll is excluded from the policy through HttpExecution.noRetry(): it owns its
own loop, which got a capped backoff instead of retrying with no delay. Its window is derived from
maas.http.timeout (25s with the defaults) instead of a fixed 60s that outlived the read timeout,
so every quiet poll used to die locally instead of returning an empty 200. The window now
travels in milliseconds, so it stays below the read timeout for small timeouts too.

Watch thread lifecycle. Parking moved from the thread object to a private monitor — join()
in close() waited on the same monitor and could consume the notification meant for the loop. The
wait is guarded against spurious wakeups, the backoff waits on that monitor so close() cuts it
short, and watchTopicCreate after close() throws instead of registering a callback that can
never fire.

Failures throw MaaSHttpException instead of a bare RuntimeException. It extends
MaaSException, so existing catch blocks keep working — note the widening: they now also catch
transport failures.

Two pre-existing NPE/NoSuchElementException paths fixed along the way: an empty body in
deleteTopic and in search.

New dependency

dev.failsafe:failsafe 3.3.2, compile scope, no transitive dependencies. It replaces a
hand-written loop with backoff, jitter, attempt counting and deadline handling. Services with
dependency convergence rules will see it appear.

How to verify

cd maas-client && mvn -B -pl client -am clean test

The backoff and watch-window tests fail against main.

Tests

HttpExecutionFailoverTest

Test	What it pins down
testFailover_RetryableResponseSucceedsOnRetry	405 read-only, 500 and 429 are retried and the call then succeeds
testFailover_PermanentResponseNotRetried	400, 401, a bare 405 and a 405 whose maas-service reason is unrelated go out exactly once
testMaxTotalDuration_BoundsAHangingAttempt	an agent that accepts the connection and never answers cannot stretch the call
testNoRetry_SendsExactlyOneAttempt	noRetry() sends one request
testZeroTotalDuration_SendsOneAttemptAndDoesNotRetry	the same through the property rather than the method
testInterrupt_RestoresFlagAndAbortsRetryLoop	an interrupt during a retry wait restores the flag and aborts
testTotalDurationExceeded_CarriesTheLastFailureAsCause	the terminal exception names what kept failing, and the time went on retries
testMaxTotalDuration_AbortsBeforeAttemptsExhausted	a tight duration stops the loop early

RabbitFailoverTest

Test	What it pins down
testFailover_RetryableResponseSucceedsOnRetry	405 and 500 are retried on the vhost path too
testFailover_400NotRetried	a permanent 400 is not

KafkaMaaSClientWatchBackoffTest

Test	What it pins down
watchWindowStaysBelowTheReadTimeout	the poll window stays under maas.http.timeout, down to a one-second timeout
backoffGrowsWithConsecutiveFailures	the pause grows per failure and saturates at the cap
failingWatchPollIsBackedOffInsteadOfHotLooping	a down agent is not polled in a hot loop

Elsewhere

EnvTest covers the new property: default, explicit value, zero, and the fallback for a
non-numeric, empty or negative one. KafkaMaaSClientImplTest adds the empty-body delete that used
to throw NullPointerException.

[alsergs]

Review: PR #173 — maas client tests and retry cases improvement

Reviewed at 895b45687, after the rebase onto main and the move to Failsafe.

Verdict

The retry policy and the watch-loop rework fix real failures that show up during a rolling node replacement, and the
backoff has a regression test that fails against main. Replacing the hand-rolled retry loop with Failsafe removed a
class of arithmetic bugs and most of the bookkeeping. Three items should be settled before merge: a retried DELETE
that reports a completed delete as a failure, a stray interrupt that kills the watch thread for good, and a narrowed
catch in Env that lets a config-provider failure escape.

Scope note. The ticket asks for coverage of leader failover and switchover in the maas client libs. This PR covers the
control plane, meaning the REST channel between the client and maas-agent. The data plane, where a broker leader change
actually lands, is untouched: rabbit-blue-green opens a channel per binding through ChannelSupplier with no retry,
and kafka-blue-green-consumer has no failover test. Worth stating in the PR body so the ticket does not look closed.

Fixed issues

1. Hot spin on a failed watch poll. watchTenantCreateTopics caught the exception, logged it and immediately
   looped. With maas-agent unreachable the connection is refused in microseconds, so the loop ran thousands of times
   per second, each iteration writing an ERROR with a stack trace. awaitWatchBackoff now applies a linear backoff
   capped at 30s, reset on success. Waiting on watchLock rather than Thread.sleep also lets close() cut the
   backoff short instead of waiting out the full pause.

2. NullPointerException in deleteTopic. The old code null-checked resp in the if, then dereferenced it on
   the next line. A 200 with an empty body reaches that path, because sendAndReceive filters an empty body to
   Optional.empty(). Now handled at KafkaMaaSClientImpl.java:116.

3. Missed signal in the watch park path. The old wait() was not guarded by a condition. Between the isEmpty()
   check that ended the poll loop and the wait() there was a closed check and a logging call. A watchTopicCreate
   landing in that window sent its notify() before anyone waited, and the watch thread parked forever holding a
   non-empty listener map, so the callback never fired. The guarded wait at :223 closes it.

4. Lock held on the Thread object. synchronized (watchThread.get()) shares a monitor with Thread.join(), which
   close() calls. A single notify() could wake the joiner instead of the watch loop. Now a private watchLock, and
   notifyAll() rather than notify().

5. Watch window longer than the read timeout. The window was a fixed 60s against a 30s default read timeout, so
   every quiet long poll died locally and counted as a failure. watchTimeout() now derives the window from
   maas.http.timeout, giving 25s by default.

6. No retry on transient failover responses. HttpExecution retried only on IOException. It now also retries
   5xx, 429, a read-only 405 from maas-service, and refreshes once on 401, under a Failsafe RetryPolicy bounded by
   maas.http.retry.max-total-duration-ms with a 60s default. Per-attempt bounding uses Call.timeout(), so no client
   is rebuilt per attempt.

7. watchTopicCreate after close(). It used to register a callback that could never fire. It now throws
   IllegalStateException.

Test coverage

Covered, and the tests fail against main:

• Issue 1: KafkaMaaSClientWatchBackoffTest.failingWatchPollIsBackedOffInsteadOfHotLooping stands up an HTTP stub that
  answers every poll with 500, timestamps the arrivals, and asserts the first pause exceeds 500ms and the second
  exceeds the first. Good test.
• Issue 5: watchWindowStaysBelowTheReadTimeout, with the gap noted below.
• Issue 6: HttpExecutionFailoverTest, now parameterized, covering the retryable statuses, the permanent ones, the 401
  cap, noRetry(), a zero budget, a hanging attempt, interrupt handling and cause chaining. RabbitFailoverTest adds
  the same shape end to end through getOrCreateVirtualHost.
• KafkaMaaSClientCloseTest.closeStopsWatchThread is a good addition: it asserts the named thread is no longer alive
  after close(), which pins the shutdown path that issues 3 and 4 sit on.

Not covered, and worth adding:

• Issue 2 needs one line. No test sends a 200 with an empty body, so the fixed NullPointerException has no
  regression test. testTopicDeleteSuccess uses {"deletedSuccessfully": [], "failedToDelete": []}, which is a
  different path. Add a .respond(response()) case.
• Issue 7 has no test. One assertThrows(IllegalStateException.class, …) after close() covers it.
• Issues 3 and 4 have no test. Nothing exercises a notify arriving between the isEmpty() check and the wait(),
  or a notify() waking a join(). closeStopsWatchThread and testWatchTopicCreate walk the park and wake path,
  but both pass on main too, so neither would have caught either bug. Both races are hard to drive deterministically
  without a hook at the park point, which is an argument for changing the mechanism rather than for writing the test.
  See the first suggestion.
• The boundary in issue 5 is not covered. watchWindowStaysBelowTheReadTimeout checks
  {2, 5, 6, 10, 30, 60, 120}, but the invariant breaks at maas.http.timeout=1, where the window comes out at 1s and
  equals the read timeout, and for sub-second timeouts, where Duration.getSeconds() truncates to 0. Add 1 to the set
  and fix the clamp.

Issues to fix before merge

• A retried DELETE reports a successful delete as a failure. deleteTopic is not idempotent, and retrying on
  5xx makes it at-least-once. If maas-service deletes the topic and then the response is lost, the second attempt
  returns empty lists and deleteTopic returns false, telling the caller the topic still exists. getOrCreateTopic
  with OnTopicExists.FAIL has the same shape. noRetry() is currently used only by the watch poll; both of these
  need it, or explicit duplicate handling.
• A stray interrupt kills the watch thread permanently. When the thread is interrupted while closed is false,
  pollWhileThereIsSomethingToWatch returns false and the thread exits. watchThread is a Lazy that stays
  initialized, so the next watchTopicCreate passes the closed guard, registers a callback, gets the dead thread
  back, and notifies a monitor nobody waits on. Nothing restarts the thread and nothing logs an error.
• Env no longer catches a config-provider failure. The probe at Env.java:236 narrowed from Throwable to
  ClassNotFoundException | NoClassDefFoundError. If MicroProfile Config is present but its provider fails during
  class initialization, a duplicate or broken SmallRye ConfigSource being the common case, the resulting
  ExceptionInInitializerError is an Error that is neither of those. It now escapes stringProperty, so apiUrl(),
  httpTimeout() and httpRetryMaxTotalDuration() all throw, where the client previously fell back to system
  properties and environment variables. Catching LinkageError as well restores the old behavior.

Suggestions

Replace the monitor with a Semaphore. The guarded wait is correct, but its correctness rests on an invariant that
a later edit can break silently: move the isEmpty() check out of the loop, or notify before mutating the map, and the
missed signal returns with no test failure. A Semaphore remembers a permit released before anyone waits, so the race
cannot exist:

private final Semaphore watchSignal = new Semaphore(0);

// park:
watchSignal.drainPermits();
watchSignal.acquire();

// wake, from watchTopicCreate and close():
watchSignal.release();

This also removes the third coordination mechanism from the thread. Right now closed, the watchLock monitor and
Thread.interrupt() all steer the same thread, and the interrupt being load-bearing in shutdown is exactly why a stray
interrupt is indistinguishable from a close. A timed acquire covers the backoff wait as well, so both uses of
watchLock go away together.

Reconsider the 401 retry, and move it to an OkHttp Authenticator. Capping at one is the right call, and the
reasoning in the comment holds: nothing in this stack can invalidate a token, so attempt N+1 sends whatever the
supplier returns, and a second retry is provably useless. The cap itself matters more than the retry, because 401 is in
isRetryableStatus and without the cap a bad credential would spin for the whole 60s budget on every call. Three
things are worth changing around it.

First, the retry buys less than it looks like. M2MInterceptor calls the token supplier on every request, immediately
before chain.proceed, so the gap between minting the header and the server validating it is one network round trip.
The only case the retry covers is a token crossing its expiry boundary inside that gap, where the supplier's own
expiry check fires on the next call. Real, but narrow.

Second, the retry costs more than one request. M2MInterceptor already handles 401 by re-sending with the Keycloak
token, and alterRequest(…, useFallbackUrl=true) rebases the URL, so the fallback goes to a different host and port.
UrlCache.store runs only when the fallback succeeds, so a URL that keeps failing is never cached and every attempt
repeats Kubernetes token, 401, Keycloak token, 401. One 401 retry then puts four requests on the wire rather than two.
The tests do not show this because they run with KUBERNETES_M2M_ENABLED unset.

Third, the Failsafe policy applies the same backoff to every retryable failure, 401 included. Auth has no
rate-limiting semantics here: either the supplier can mint a fresh token now or it cannot. The delay adds a second of
latency to the common auth-failure path for nothing.

An OkHttp Authenticator addresses all three. It runs inside the call, below the interceptor, so it retries the auth
without re-running the fallback and rebase; it is handed the failed response and must build a new request, which makes
the refresh explicit rather than incidental; and it caps follow-ups itself. Worth checking with the security library
owners whether refresh belongs there rather than here, given that M2MInterceptor already owns 401 and maas-client
does not own the token supplier.

While in this code, the give-up message does not say that credentials were the problem. A dedicated message for the
exhausted-auth case saves a debugging session.

Move authAttempts into the execution. HttpExecution is a single-use, thread-confined builder, and every call
site consumes it in one chained expression, so the mutable fields cause no problem today. authAttempts is the
awkward one: it is an instance field reset at the top of sendAndReceive() and then incremented from inside the
Failsafe handleIf predicate, which means per-call retry state now lives outside the retry policy that owns
everything else. Holding it in a small per-call object captured by the policy lambda keeps the reset from being
load-bearing.

Worth noting that expectedCodes, errorHandler, retryEnabled and req are mutated after construction with no
synchronization, and final freezes the reference rather than the contents. Building an HttpExecution on one thread
and running it on another would have no happens-before edge. Nothing does that, and the class does not need to support
it, but the builder chain looks like something a caller might reasonably cache in a field. A sentence on the class
saying it is single-use and confined to one thread would settle it.

Tighten the read-only 405 match. Every maas-service error envelope carries MAAS-0600, so the decision rests on a
substring search for read-only anywhere in the body. A permanent 405 whose message mentions a read-only field gets
retried for the full budget. Match the reason field, or the Postgres code 25006.

Consider an upper clamp on the retry budget. parseRetryDurationMillis rejects a negative value but not an absurd
one, so maas.http.retry.max-total-duration-ms=9223372036854775807 reaches Failsafe as a Duration of roughly 292
million years. The nanosecond overflow that this used to cause in the hand-rolled deadline is gone, and I did not
verify how Failsafe handles a Duration that large, so this is cheap insurance rather than a known defect.

Relax one timing assertion. testMaxTotalDuration_AbortsBeforeAttemptsExhausted allows 800ms for a 200ms budget
while running real requests against MockServer. The margin is 600ms across several attempts, backoff delays and JVM
scheduling; one GC pause makes it fail. Asserting the exception rather than the clock removes the flake.

Interceptor, optional. Moving the Failsafe policy into an OkHttp Interceptor would cover calls that bypass
HttpExecution, such as the websocket client from HttpClient.getClient(), and would turn noRetry() into a
Request.tag() rather than mutable state. Lower value than it was before Call.timeout() replaced the per-attempt
client rebuild. Fine to leave for later.

Minor

• The CHANGELOG and README use behaviour; the rest of the maas-client docs use American spelling.
• The PR body is empty. Given that the change spans a new dependency, a rewritten retry path and a reworked watch
  thread, a short Why, What and How to verify would help the reviewer.
• Adding Failsafe is a new runtime dependency for every consumer of maas-client. Worth one line in the CHANGELOG, since
  downstream services with dependency convergence rules will see it.

[Ksiona]

Not fixed after review, with reasons

• getOrCreateTopic with OnTopicExists.FAIL. Disabling retry there would drop the exact
  scenario this PR exists for. It is a rarely used flag; I would document the at-least-once
  semantics rather than fix it silently.
• Match 25006. That code does not appear in the 405 body — it carries MAAS-0600 and prose.
  Matching the reason field rather than the whole body is the right improvement, but it is a
  separate change.
• Upper clamp on the budget. I added one, then removed it: it is speculative, and it would
  silently turn a deliberate two-hour value into 60s.
• Interceptor. Out of scope: the retry has always lived in HttpExecution — main has a
  30-attempt loop with a fixed 1s sleep in the same place. This PR changed the mechanism, not the
  location, so moving the policy into an interceptor is a restructuring on top of pre-existing
  code rather than a fix to this change. Worth a ticket, since it would also cover callers that
  bypass HttpExecution, such as the websocket client from HttpClient.getClient().

[Ksiona]

Reverted: the Semaphore. Rolled back to watchLock — the regression it introduced outweighs
the improvement it offered.

Introduced: a busy-wait for the whole pause. tryAcquire returns immediately while a permit is
available and the release puts it straight back, so the loop never blocks. A permit is always
available after the first watchTopicCreate, since only the park consumes one — this hits the
ordinary path of one subscription against an agent returning 500, burning a core for up to 30s
between polls. Also lost the guard in the park path, leaving idle outer-loop spins after delivery.

Offered: nothing. The missed-signal race does not exist in the monitor version — isEmpty() and
wait() are atomic under the lock, and watchTopicCreate notifies under the same lock. The
JEP 491 pinning point is factually right but concerns one daemon thread and disappears on JDK 24.

[alsergs]

Re-reviewed at 4774f001, against my previous review at 895b45687.

Addressed

• Env catch narrowing. Fixed, and better than what I suggested: catches Throwable (not just
  adding LinkageError), with a comment explaining why an Error has to be caught here.
• Retried DELETE. deleteTopic now calls .noRetry(), with a CHANGELOG entry explaining why it
  is deliberately not retried.
• Missing empty-body delete test. Added — testTopicDeleteEmptyBody covers the NullPointerException
  fix directly.
• keySet() handed to Jackson without the map's lock. Fixed: watchedClassifiers() now copies the
  key set under synchronized (topicCreateListeners) before it is serialized.
• Flaky timing assertion in testMaxTotalDuration_AbortsBeforeAttemptsExhausted. Fixed as suggested:
  the elapsedMs < 800 clock check is replaced with VerificationTimes.atMost(10) on the attempt count.
• 401 retry. Removed entirely rather than moved to an Authenticator. The new comment matches what I
  found tracing M2MInterceptor: it already makes its own 401 round trip, and nothing in this stack can
  force a token refresh, so the retry only ever covered a narrow expiry-boundary window. Dropping it is a
  reasonable, more conservative resolution than the Authenticator move I suggested — no objection to it.
• Failsafe as a new dependency. Now called out in the CHANGELOG, including that it pulls no
  transitives but will still show up under dependency convergence rules.
• watchBackoffMillis is now extracted and unit-tested on its own, and Env's retry-duration parsing
  (zero, negative, unparseable, whitespace) has direct test coverage.

Still open

• A stray interrupt kills the watch thread permanently. Not fixed. The log line improved — it is now
  log.error, names the affected classifiers, and tells the operator to recreate the client — but the
  thread still exits with nothing that restarts it and no state that later calls can detect. Better
  diagnostics, same underlying bug: watchThread stays initialized, so a later watchTopicCreate
  silently registers a callback that will never fire.
• getOrCreateTopic with OnTopicExists.FAIL is still retried. Same shape as the deleteTopic issue
  that was fixed: a lost response after a completed create makes the retry hit a topic that already
  exists and fail permanently. deleteTopic got .noRetry(); this call site did not.
• The watch-window boundary is still untested. watchWindowStaysBelowTheReadTimeout checks
  {2, 5, 6, 10, 30, 60, 120}. The invariant it exists to protect breaks at maas.http.timeout=1, where
  the window equals the read timeout, and for sub-second timeouts, where Duration.getSeconds()
  truncates to 0. Adding 1 to the data set would catch it.
• No test for watchTopicCreate throwing after close(). One assertThrows(IllegalStateException.class, …)
  would cover it.

Net

Six of eight points from the previous review landed cleanly, one (401) was resolved differently but
soundly. The two remaining items — the permanently dead watch thread after a stray interrupt, and the
retried getOrCreateTopic — are worth closing before merge; the two test gaps are cheap follow-ups.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
85.1% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[TaurMorchant]

Why does deleteTopicTemplate still have retries? Shouldn't we keep them consistent?