fix(outbox): stop a broker outage from spending the attempt budget - #33
Merged
Merged
Conversation
A relay whose broker does not answer used to spend one attempt on every pending row every cycle, so an outage longer than `max_attempts` cycles turned the whole backlog into terminal `failed` rows: when the broker came back the relay published nothing, because `fetch_and_lock_pending` filters on `attempts_made < max_attempts`, and only `requeue_failed` on each row brought them back. The attempt budget belongs to a row that cannot be published -- a payload the broker rejects, a topic that does not exist, a serialization error. A broker that is not there is a property of the cycle. `TransientError` is the exception-shaped twin of `handler_retry(count_as_attempt=False)`: a publisher or a handler raises it to say the failure is the environment's, not the event's. `HandlerExecutionStep` records it without spending an attempt and puts the event a second ahead. `KafkaEventPublisher` raises it once its own retries are spent on a broker that does not answer -- connection, node-not-ready, request and client timeouts, and an unknown topic only when a forced metadata refresh fails as well, so a typo in a topic still ends in `failed`. `PublisherExecutionStep`, which `create_outbox_processor` now installs, goes two steps further, because everything in an outbox batch goes to the same broker: the publish timeout is transient too, and the first transient failure ends the cycle's publishing -- the remaining rows are recorded the same way with their schedule untouched. A dead broker costs one probe per cycle instead of a batch of them, and the rows publish on the first cycle after it answers.
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.
Summary
A relay whose broker does not answer used to spend one attempt on every pending row every
cycle. After
max_attemptscycles the whole backlog wasfailed, which is terminal: whenKafka came back the relay published nothing, because
fetch_and_lock_pendingfilters onattempts_made < max_attempts, and the events came back only when somebody ranrequeue_failedon each row. A four-minute outage against a relay ticking every secondturned into a backlog that never drained on its own.
The attempt budget is the right tool for a row that cannot be published — a payload the
broker rejects, a topic that does not exist, a serialization error. It is the wrong tool for
a broker that is not there, which is a property of the cycle and not of any row.
What the fix is
TransientError(omni_box.core.exceptions, re-exported at the top level) — theexception-shaped twin of
handler_retry(count_as_attempt=False). A publisher or a handlerraises it to say the failure is the environment's, not the event's.
HandlerExecutionSteprecords it without spending an attempt and puts the row a secondahead. Every other exception, the handler timeout included, still counts.
KafkaEventPublisherraises it oncemax_infra_retriesare spent on a broker that doesnot answer: connection and node errors, request and client timeouts, and
UnknownTopicOrPartitionErroronly when a forced metadata refresh fails as well — atopic the broker answers about and still does not know keeps counting, so a typo in a topic
name still ends in
failed. aiokafka's ownretriableflag is wider than this, which iswhy the check is explicit.
PublisherExecutionStep(new, installed bycreate_outbox_processor) goes two stepsfurther, because everything in an outbox batch goes to the same broker: the publish timeout
is transient too — a payload the broker cannot ack in 30 s is not the row's fault — and the
first transient failure ends this cycle's publishing. The remaining rows are recorded the
same way, unsent, with their schedule untouched. A dead broker costs one probe per cycle
instead of a batch of them.
Nothing is added to any signature and no default changes for anyone whose broker is up.
Why aiokafka's errors never reached the retry loop
Measured on 0.2.0 with aiokafka 0.14.0 and the container paused: the first publish of a topic
the producer has never seen ends in
UnknownTopicOrPartitionErrorafterrequest_timeout_ms,because
_wait_on_metadataswallows the failed refresh and reports "no such topic"; oncemetadata is cached it is
RequestTimedOutError, thenNodeNotReadyError. None of thosesubclass the builtin
ConnectionErrororTimeoutError, soErrorClassifiercalled them allpermanent and
max_infra_retriesnever ran for aiokafka's own errors at all.What I rejected
ErrorClassifier— core cannot seeaiokafka's classes, and it would silently change how an inbox handler raising
ConnectionErroris counted.publishreturn a handler result — that changes the publisher contract for everyadapter.
UnknownTopicOrPartitionErroras transient unconditionally — a missing topic wouldthen never fail, and would cost a probe every cycle forever.
every row behind it.
any row-level delay beyond a tick is pure lag after the broker returns, and growth would
need state that outlives a cycle. If it is ever wanted, a
retry_afteron the exception iswhere it goes.
Type of change
Checklist
make checkpasses locally (ruff+mypy)CHANGELOG.mdupdated under[Unreleased]— n/a, release-please writes it from the commitdocs/agents.mdincludedVerification
tests/integration/postgres/test_broker_outage.pyis the report against the real outboxtable: 20 rows,
max_attempts=6, aKafkaEventPublisherover a producer that raises what apaused broker raises, and seven relay cycles. On
masterit fails exactly the way the reportreads —
{(PENDING, 1)}after cycle 1,{(FAILED, 6)}at the end, and nothing published whenthe broker comes back. A broker that answers and rejects the record still spends an attempt
per row, which the third test pins.
The reporter's own
kafka_down_lab.py, unmodified, against this branch:The outage is also 35 s instead of 240 s, because a cycle is now one probe rather than twenty.
Full gate:
make checkclean,make test758 passed / 98.61% coverage,make test-integration95 passed.
Related issues
Closes #32