Skip to content

fix(TaskProducer): call close() on shutdown#718

Merged
bmckerry merged 1 commit into
mainfrom
ben/taskproducer-close
Jun 17, 2026
Merged

fix(TaskProducer): call close() on shutdown#718
bmckerry merged 1 commit into
mainfrom
ben/taskproducer-close

Conversation

@bmckerry

Copy link
Copy Markdown
Member

This PR adds a new CloseableProducerProtocol interface for TaskProducer.
Previously, TaskProducer was never calling the arroyo producer close() method, meaning the inner Confluent producer would never flush().

@bmckerry bmckerry requested a review from a team as a code owner June 17, 2026 17:00
) -> ProducerFuture[BrokerValue[KafkaPayload]]: ...


class CloseableProducerProtocol(Protocol):

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a new type rather than updating ProducerProtocol because that's used by TaskbrokerApp's producer factory, which returns a SingletonProducer in sentry, which has a different name for its shutdown method.

Comment thread clients/python/src/taskbroker_client/worker/producer.py Outdated
@bmckerry bmckerry force-pushed the ben/taskproducer-close branch from dff0895 to bc90e93 Compare June 17, 2026 17:11

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit bc90e93. Configure here.

Comment thread clients/python/tests/worker/test_producer.py Outdated
Comment thread clients/python/tests/worker/test_producer.py Outdated
Comment thread clients/python/src/taskbroker_client/worker/producer.py
@bmckerry bmckerry force-pushed the ben/taskproducer-close branch from bc90e93 to aa29295 Compare June 17, 2026 17:16
)
)

def _shutdown(self) -> None:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The call to self._inner_producer.close().result() in the _shutdown handler lacks a timeout, which can cause the process to hang indefinitely if the Kafka broker is unreachable.
Severity: HIGH

Suggested Fix

Add a timeout to the self._inner_producer.close().result() call within the _shutdown method. This will prevent the process from hanging indefinitely if the Kafka broker is unresponsive during shutdown. For example, use self._inner_producer.close().result(timeout=30).

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: clients/python/src/taskbroker_client/worker/producer.py#L105

Potential issue: The `_shutdown` function, registered via `atexit`, calls
`self._inner_producer.close().result()` without a timeout. The underlying
`arroyo.KafkaProducer.close()` method flushes pending messages and blocks until they are
acknowledged by the Kafka broker. If the broker is unreachable or slow during process
shutdown (e.g., due to a network partition or rolling restart), the `.result()` call
will block indefinitely. Since this occurs within an `atexit` handler, the process will
never terminate cleanly.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if its unreachable we have bigger problems

@bmckerry bmckerry merged commit 8cf2b61 into main Jun 17, 2026
27 checks passed
@bmckerry bmckerry deleted the ben/taskproducer-close branch June 17, 2026 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants