Repository navigation
fix(prometheus_exporter sink): close listener before the sink finishes - #26831
thomasqueirozb wants to merge 3 commits into
Conversation
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c99a14d4f
ℹ️ 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".
There was a problem hiding this comment.
The Prometheus exporter's graceful server shutdown await in exporter.rs has no timeout, meaning a stalled scrape client or an incomplete TLS handshake can block RunningTopology::shutdown_diff indefinitely during a configuration reload. The fix requires bounding the drain wait and aborting/joining the server on expiry, or decoupling listener-close completion from connection draining.
🤖 Bits Code Review · Commit 0c99a14
Summary
The
prometheus_exportersink runs its HTTP server in a detached task, andrun()returned before that task closed the listener. During a reload,shutdown_diffwaits for a sink with a conflicting resource to finish and then treats that resource as free. For this sink, only task scheduling made the address free in time. The sink now waits for its server to stop before it finishes.No user-visible failure was reproduced (see below), so this PR has no changelog fragment. #26832, which removes the Windows reload sleep, depends on this change.
References
Related: #3035, #26832
Vector configuration
How did you test this PR?
flush_period_secschanged before each reload. The setups were default threads with health checks on,--threads 1with health checks on, and--threads 1with the config above. All 300 reloads succeeded, with noAddrInUseorFailed to start Prometheus exportererrors. The race does not show up in practice, because the old server task runs before the new sink binds.finished_sink_releases_addressruns the sink to completion and then binds its address. Onorigin/masterit fails every time withAddrInUse(4/4 tries); with this change it passes.Does this PR include user facing changes?
no-changeloglabel to this PR.