Repository navigation
fix(mqtt sink): stop event loop after the sink stops - #26793
thomasqueirozb wants to merge 26 commits into
Conversation
…nto mqtt-drop-rustls-webpki-0.102 # Conflicts: # changelog.d/mqtt_native_tls.security.md
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
More details
Whether RequestsDone waits for outstanding QoS 2 acknowledgments remains unverified. The available evidence does not establish a delivery regression in that shutdown scenario.
🤖 Bits Code Review · Commit 34f14fa
There was a problem hiding this comment.
Stopping an MQTT sink configured for exactly-once delivery can close its connection before in-flight QoS 2 handshakes finish, leaving messages undelivered before the five-second flush allowance expires.
🤖 Bits Code Review · Commit 34f14fa
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5118bc7ab4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let (result, drained) = tokio::join!( | ||
| async { | ||
| let result = driver.await; | ||
| input_done.cancel(); | ||
| result | ||
| }, | ||
| drive_event_loop(connection, input_done.clone()), |
There was a problem hiding this comment.
Bound the drain during configuration reloads
When input ends while a QoS 1/2 publish is awaiting PUBACK/PUBCOMP, this join cannot complete until the broker acknowledges it or the connection errors. For a changed sink whose buffer is reused, src/topology/running.rs:723-726 awaits the old sink without the process-shutdown deadline, so a broker that keeps the connection healthy but withholds the acknowledgement makes the configuration reload hang indefinitely and prevents the replacement sink from starting. Add a bounded/cancellable drain path for reloads rather than relying solely on the process shutdown timeout.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not bounding this by default is intentional. Reloads that wait for a sink already have no time limit on master. shutdown_diff waits for sinks that reuse their buffer, have a resource conflict, or get a new disk buffer, and it has no timeout. With the default request.retry_attempts, an HTTP-based sink that keeps getting retriable errors already blocks such a reload forever.
With this change, the MQTT sink finishes its in-flight QoS exchanges before it stops. Before, it dropped them. This matches what other sinks do. The drain still ends if the broker closes the connection, if keepalive fails, or if a new connection uses the same client ID.
A default limit would change reload behavior for every sink, so the limit is opt-in through --reload-sink-drain-limit-secs in #26834.
JinchengCao
left a comment
There was a problem hiding this comment.
LGTM,you can update the description and test command to reflect the new dependency and the shared reload deadline, which now applies to all sinks
There was a problem hiding this comment.
Summary
The MQTT sink's event loop ran in a task detached from the sink. When a reload removed or replaced the sink, that task kept polling and reconnecting, so the old client ID stayed connected to the broker. With
rumqttc-next, dropping the lastAsyncClientends the request channel, but the sink merely logged the resultingRequestsDoneerror and kept polling.Changes:
ConnectionError::RequestsDone. The loop is dropped together with the driver's clients, so queued publishes and QoS handshakes complete first.EventLoop::diagnosticsto check that its queues are empty.rumqttc-next0.26.1 is replaced byrumqttc-v4-next0.34.0. It reports the queue and outbound state used above, so the previous hand-rolled tracking is gone. MQTT 3.1.1 support is kept and TLS stays on Rustls with the Ring provider.RunningTopologynow share one deadline derived from--graceful-shutdown-limit-secs, so a reload no longer waits forever for a sink stuck in a QoS exchange. When the deadline passes, only the sink future is dropped, so unread buffered events remain available to the replacement.--no-graceful-shutdown-limitstill waits without a deadline. This applies to every sink, not only MQTT.References
Related: #26791 (this PR is based on it)
Vector configuration
Manual reload check against a local broker:
How did you test this PR?
src/sinks/mqtt/sink.rsagainst a mock broker:sink_drains_before_returning(QoS 0/1/2),idle_sink_closes_when_input_ends,cancelling_sink_closes_unacknowledged_connection,drain_connection_error_finishes_sink_without_reconnecting,reload_drains_with_vector_shutdown_policy(bounded deadline, unlimited, broker disconnect),drain_handles_queued_publishes_and_reused_ids,drain_after_reconnect(QoS 1/2 resume and reset), andrejected_publish_does_not_block_draining.sink_reload_deadline_preserves_unread_buffersinsrc/topology/running.rs: two stalled sinks are cancelled at the same deadline, and their unread buffered events reach the replacements.cargo vdev runagainst a TCP stub broker and SIGHUP reloads:--graceful-shutdown-limit-secs 2closed an unacknowledged QoS 2 connection after 2s and started the replacement, which then published on its own connection.--no-graceful-shutdown-limitkept waiting for PUBCOMP and then exited cleanly.make test FEATURES="sources-mqtt,sinks-mqtt,sources-prometheus,sinks-prometheus,sources-internal_metrics,sources-splunk_hec,enrichment-tables-memory" SCOPE="-p vector --lib -E 'test(mqtt) or test(topology::)'"passes.make check-clippy,make check-markdown,make check-changelog-fragments, andmake generate-docspass.Does this PR include user facing changes?
no-changeloglabel to this PR.