control_plane: retry transient postgres errors in control plane jobs - #721
Open
Mwea wants to merge 1 commit into
Open
control_plane: retry transient postgres errors in control plane jobs#721Mwea wants to merge 1 commit into
Mwea wants to merge 1 commit into
Conversation
Read fetches served by a hot-standby replica can fail with recovery
conflicts ("canceling statement due to conflict with recovery" and its
escalated "terminating connection due to conflict with recovery" form).
These are transient and rolled back by the server, but JobUtils had no
retry (only a TODO), so a single conflict surfaced to consumers as
UNKNOWN_SERVER_ERROR instead of being transparently retried.
Retry the whole job with jittered exponential backoff on rollback-guaranteed
SQLStates (40001 serialization/recovery-conflict, 40P01 deadlock). The retry
wraps the entire transaction, so it obtains a fresh pooled connection and
recovers even when the conflict killed the previous connection. Generic
connection-loss states are intentionally not retried, since a drop during
commit leaves a write in-doubt.
Because this retry sits on the fetch hot path shared by every control plane
job, it is designed to stay safe under a conflict storm:
- Only the decisive attempt (success, or the final failure) is reported to
the duration callback. measureDurationMs fires in a finally, so timing each
attempt directly would record N samples per run and pollute the latency/rate
metrics with failed-attempt durations exactly when an operator relies on them.
- Backoff uses ExponentialBackoff with jitter, so many jobs failing against the
same struggling standby do not retry in lockstep and re-collide on the same
recovery-conflict window.
- The loop aborts when the thread is interrupted during backoff: Utils.sleep
restores the interrupt flag but returns normally, so without an explicit check
the loop would spin through remaining attempts and delay broker shutdown.
- Retries exhaustion is logged once, so the final user-visible failure is
greppable rather than only appearing as intermediate "retrying" warnings.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Mwea
force-pushed
the
fix/find-batches-retry-recovery-conflict
branch
from
July 25, 2026 17:05
f8ee902 to
ba3c118
Compare
| throw e; | ||
| } | ||
| final long backoffMs = BACKOFF.backoff(attempt - 1); | ||
| LOGGER.warn("Transient database error on attempt {}/{}, retrying in {} ms", |
Contributor
There was a problem hiding this comment.
If it's a transient error, maybe the warn level is a bit too harsh. I think this ideally should be debug or maybe info level. The final attempt is fair to be warn though.
Comment on lines
+56
to
+76
| /** | ||
| * PostgreSQL {@code SQLState}s that are safe to retry because they guarantee the transaction did | ||
| * <em>not</em> commit (the server rolled it back), so replaying the whole transaction cannot | ||
| * double-apply a write. | ||
| * | ||
| * <ul> | ||
| * <li>{@code 40001} — {@code serialization_failure}. PostgreSQL also raises this | ||
| * ({@code ERRCODE_T_R_SERIALIZATION_FAILURE}) for hot-standby recovery conflicts, in both | ||
| * observed forms: the statement cancellation | ||
| * ("canceling statement due to conflict with recovery") <em>and</em> the connection | ||
| * termination ("terminating connection due to conflict with recovery"). The latter closes | ||
| * the JDBC connection, but because {@code readJooqCtx}/{@code writeJooqCtx} are backed by a | ||
| * connection pool, the retry transparently obtains a fresh connection.</li> | ||
| * <li>{@code 40P01} — {@code deadlock_detected}. The victim transaction is rolled back.</li> | ||
| * </ul> | ||
| * | ||
| * <p>We deliberately do <b>not</b> retry generic connection-loss states (class {@code 08*}, | ||
| * {@code 57P01}) here: a connection dropped <em>during commit</em> leaves the transaction | ||
| * in-doubt, so blindly replaying a write job could apply it twice. The recovery-conflict family | ||
| * that motivated this retry logic is fully covered by the rollback-guaranteed states above. | ||
| */ |
Contributor
There was a problem hiding this comment.
The comment seems to be a bit too verbose. Perhaps:
Suggested change
| /** | |
| * PostgreSQL {@code SQLState}s that are safe to retry because they guarantee the transaction did | |
| * <em>not</em> commit (the server rolled it back), so replaying the whole transaction cannot | |
| * double-apply a write. | |
| * | |
| * <ul> | |
| * <li>{@code 40001} — {@code serialization_failure}. PostgreSQL also raises this | |
| * ({@code ERRCODE_T_R_SERIALIZATION_FAILURE}) for hot-standby recovery conflicts, in both | |
| * observed forms: the statement cancellation | |
| * ("canceling statement due to conflict with recovery") <em>and</em> the connection | |
| * termination ("terminating connection due to conflict with recovery"). The latter closes | |
| * the JDBC connection, but because {@code readJooqCtx}/{@code writeJooqCtx} are backed by a | |
| * connection pool, the retry transparently obtains a fresh connection.</li> | |
| * <li>{@code 40P01} — {@code deadlock_detected}. The victim transaction is rolled back.</li> | |
| * </ul> | |
| * | |
| * <p>We deliberately do <b>not</b> retry generic connection-loss states (class {@code 08*}, | |
| * {@code 57P01}) here: a connection dropped <em>during commit</em> leaves the transaction | |
| * in-doubt, so blindly replaying a write job could apply it twice. The recovery-conflict family | |
| * that motivated this retry logic is fully covered by the rollback-guaranteed states above. | |
| */ | |
| /** | |
| * PostgreSQL failures that guarantee transaction rollback and are safe to retry: | |
| * {@code 40001} for serialization or recovery conflicts, and {@code 40P01} for deadlocks. | |
| * Connection-loss states are excluded because the commit outcome may be unknown. | |
| */ |
Comment on lines
+160
to
+165
| if (throwable instanceof SQLException) { | ||
| final String sqlState = ((SQLException) throwable).getSQLState(); | ||
| if (sqlState != null && RETRIABLE_SQL_STATES.contains(sqlState)) { | ||
| return true; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Would it make sense to take a look at the whole chain by iterating getNextException()?
| final T result = TimeUtils.measureDurationMs(time, callable, captureDuration); | ||
| durationCallback.accept(lastAttemptDurationMs[0]); | ||
| return result; | ||
| } catch (final Exception e) { |
Contributor
There was a problem hiding this comment.
Can't we just catch SQLException here?
| * cause is typically wrapped several layers deep (e.g. {@code ControlPlaneException} -> | ||
| * {@code DataAccessException} -> {@code PSQLException}), so the whole tree is scanned. | ||
| */ | ||
| static boolean isRetriable(final Throwable throwable) { |
Contributor
There was a problem hiding this comment.
The parameter could be narrowed down to SQLException based on its behavior and the javadoc on the function too.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A production cluster (
cdip-diskless-prod-spoke-*) threw a burst ofFetchError/Read future failedexceptions. Every trace bottoms out in the same PostgreSQL condition on the read replica that servesfind_batches_v2, in two escalating forms:server conn crashed?,Cannot commit transaction, and a suppressedConnection is closed/Cannot get autoCommit.Both are hot-standby recovery conflicts: a read query holds a shared buffer pin while WAL replay needs the same page. They are transient and the server rolls the transaction back — but
JobUtilshad no retry (only a// TODO add retry with backoff), so a single conflict propagated all the way up asControlPlaneException("Error finding batches")→FindBatchesExceptionandFetchHandlerreturnedUNKNOWN_SERVER_ERRORfor every partition in the fetch.Change
Implement the missing retry in
JobUtils: retry the whole job with jittered exponential backoff (3 attempts, ~50 ms → ~100 ms with 0.2 jitter, capped at 1 s; via the injectedTimeso it is fully virtual in tests).Exceptions handled — retry is keyed on rollback-guaranteed PostgreSQL
SQLStates:40001serialization_failureERRCODE_T_R_SERIALIZATION_FAILURE) for both recovery-conflict forms above — the statement cancellation and the connection termination. Server guarantees rollback.40P01deadlock_detectedKey design points:
DSLContexts are Hikari-pool-backed, a retry pulls a fresh connection — this is what recovers from the connection-terminated form where the previous connection is dead.PSQLExceptionis buried underControlPlaneException→DataAccessException; in the connection-killed form it appears as a suppressed throwable (the failed rollback), exactly as in the traces.08*,57P01) are intentionally NOT retried — a drop during commit leaves a write in-doubt, so blindly replaying could double-apply. The recovery-conflict family that caused this incident is fully covered by the rollback-guaranteed states, so the retry is safe for both read and write jobs and can live in the shared helper.ControlPlaneException/ wrappedRuntimeExceptionas before.Hot-path safety
This retry sits on the fetch hot path shared by all 16 control-plane jobs, so it is designed to stay safe under a conflict storm rather than amplify one:
measureDurationMsfires the duration callback in afinally, so timing each attempt directly would record N samples per run and pollute the latency/rate percentiles with failed-attempt durations. Only the decisive attempt (success, or the final failure) is reported to the callback → exactly one sample perrun().ExponentialBackoff, reused fromclients) prevents many jobs failing against the same struggling standby from retrying in lockstep and re-colliding on the same recovery-conflict window.Utils.sleeprestores the interrupt flag but returns normally; the loop checksThread.isInterrupted()after backoff and aborts (keeping the flag set) rather than spinning through remaining attempts and delaying broker shutdown.Tests
JobUtilsTest(pure unit — no DB container, usesMockTime): happy path, recovery-conflict-then-succeed (reproduces the incident with the exact wrapping from the traces), deadlock retry, max-attempts exhaustion preservingControlPlaneException, non-retriable SQLState (23505) and plain runtime exceptions not retried, suppressed-throwable scanning, self-referential cause chain (terminates, noStackOverflow), plus: jittered backoff timing asserted viaMockTimedeltas, a single duration-callback per run (guards the metrics contract), no sleep on non-retriable failures, and interrupt-driven abort.Follow-up (not in this PR)
UNKNOWN_SERVER_ERROR. Surfacing exhausted transient failures as a retriable Kafka error would be a separate change in theFetchHandlererror-mapping path.40001; there is a residual driver-level race where a socket EOF beating the FATAL message could downgrade to08006. Confirming this end-to-end warrants a hot-standby integration test.🤖 Generated with Claude Code