fix: close consumer and producer when initial broker connect fails#31
Merged
Merged
Conversation
KafkaTable.start() and KafkaTableWriter.start() constructed their AIOKafkaConsumer/AIOKafkaProducer and connected BEFORE recording the handle on the instance, with the failure-cleanup guard beginning only AFTER the connect. When the initial broker connect raised (the common 'broker not up yet' case), the resource was orphaned: self._consumer/self._producer stayed None, so a follow-up stop() closed nothing, and aiokafka logged 'Unclosed AIOKafka*' and leaked the socket when the object was garbage-collected. Refactor KafkaTable.start() onto an AsyncExitStack that registers each resource's cleanup the instant it is constructed (before the fallible connect) and commits with pop_all() only on full success. Extract teardown into _detach_task()/_detach_consumer() so start()'s failure rollback and stop() share the exact same steps in the same LIFO order. Fix the writer with a proportionate try/except guard around producer.start() (single resource, single step). Also correct a stale ktables version in uv.lock (0.3.0 -> 1.1.0, matching pyproject). Closes #30
Contributor
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||
Deep-review round on PR #31. One behavior fix plus test/comment hardening driven by the review agents. fix: KafkaTableWriter.start() guarded its rollback stop() with a bare 'await producer.stop()'; if that stop() raised, it replaced the actionable KafkaConnectionError and the bare 'raise' never ran. Guard it like KafkaTable._detach_consumer (log-and-swallow) so the original connect error always propagates. docs: reword the AsyncExitStack comment that still described the pre-refactor 'assigned only after connect' ordering (it would have misled a maintainer into reintroducing #30), and tighten stop()'s LIFO wording (push order is reversed to yield a task-first unwind). test: add review-driven coverage — an async-with test that ISOLATES start()'s internal rollback (the old try/finally test passes even with the rollback removed, since the trailing stop() masks it); cancellation-during-catch-up (the only path that pins the _detach_task stack registration); degraded-timeout survival (asserts live resources, not just status); seek failure; retry after failed connect; and consumer/producer stop-also-fails masking. Enhance the fakes with catch_up and stop_error knobs. All new tests mutation-validated against the specific production line each pins.
ryan-yuuu
pushed a commit
that referenced
this pull request
Jul 3, 2026
🤖 I have created a release *beep* *boop* --- ## [1.1.1](v1.1.0...v1.1.1) (2026-07-03) ### Bug Fixes * close consumer and producer when initial broker connect fails ([#31](#31)) ([6aa3a3b](6aa3a3b)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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
Fixes a resource leak in
KafkaTable.start()(and the same latent bug inKafkaTableWriter.start()): when the initial broker connect fails — the common "broker not up yet" case — theAIOKafkaConsumer/AIOKafkaProducerthatstart()created is orphaned. The caller's idiomatictry: await start() ... finally: await stop()cannot help, becausestop()readsself._consumer/self._producer, which is stillNone(the handle is only recorded on success). The orphaned object is garbage-collected unclosed, loggingUnclosed AIOKafka*and leaking the socket.Closes #30.
Root cause
The resource armed aiokafka's
__del__unclosed-resource guard at construction, but the failure-cleanup only became reachable afterawait consumer.start()/await producer.start():The violated invariant: a resource's cleanup guard must span the entire window from construction to ownership-transfer.
ensure_topic()already did this correctly (itsadmin.start()is inside atry/finally: close()); the twostart()paths did not.Fix
KafkaTable.start()→ refactored onto acontextlib.AsyncExitStack. Each resource's cleanup (_detach_consumer, then_detach_task) is registered the instant the resource is constructed — before the fallible connect — so any failure or cancellation unwinds them in LIFO order. Full success callspop_all()to commit ownership to the instance and disarm the stack. An orphaned, never-stopped consumer is now structurally impossible.stop()unified with the rollback → teardown extracted into_detach_task()/_detach_consumer();stop()is now justawait self._detach_task(); await self._detach_consumer(), andstart()'s stack registers those same callbacks in the matching order — so the two teardown paths can never diverge.KafkaTableWriter.start()→ the same bug for the producer (not in the original issue report, same root cause), fixed with a proportionatetry/except BaseException: await producer.stop(); raise(single resource, single step — anAsyncExitStackthere would be overkill).Behavior is preserved exactly on every path, including the documented "reader died during catch-up counts as started (
status == "failed")" contract and the degraded catch-up-timeout path.Tests
New broker-free, deterministic regression tests in
TestStartFailureCleanup(a configurable fake consumer/producer drives every lifecycle path — no GC-timing or__del__-warning dependence):test_start_stops_consumer_when_initial_connect_failstest_writer_start_stops_producer_when_initial_connect_failstest_start_stops_consumer_when_no_partitions_assignedtest_start_cleans_up_when_reader_dies_during_catchupstarted is True,status == "failed"test_successful_start_commits_consumer_without_stopping_itpop_all()commit does not over-cleanWritten test-first: the two bug tests were confirmed failing for the exact cause before any implementation change.
Verification
barrier()paths the refactor touches.ktables/kafka_table.pymaintained.KafkaTable.start()andKafkaTableWriter.start()now emit noUnclosed AIOKafka*signal.Note
uv.lockcarried a stalektablesversion (0.3.0);uvcorrected it to1.1.0to matchpyproject.toml. Unrelated to the fix, but a correct sync — happy to drop it if a minimal diff is preferred.