otelcol: synchronize Run and Shutdown lifecycle - #14989
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the otelcol.Collector lifecycle so Shutdown() blocks until Run() has fully completed cleanup, aligning behavior with the expectations in #4947 and avoiding callers racing on resource teardown.
Changes:
- Add
donesignaling and arunStartedflag to synchronizeRun()completion withShutdown(). - Update/extend
otelcoltests to account for the new blockingShutdown()behavior and add coverage for the blocking guarantee. - Add a changelog entry describing the lifecycle semantics change.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
otelcol/collector.go |
Adds done channel + runStarted flag; Shutdown() waits for Run() completion; Run() errors immediately if already shut down. |
otelcol/collector_test.go |
Updates existing tests to avoid races and adds TestShutdownBlocksUntilRunCompletes. |
.chloggen/sync-run-shutdown-lifecycle.yaml |
Documents the lifecycle synchronization change for release notes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Sets up the control logic for config reloading and shutdown. | ||
| // If Shutdown was called before Run, Run returns an error without starting. | ||
| func (col *Collector) Run(ctx context.Context) error { | ||
| col.runStarted.Store(true) |
There was a problem hiding this comment.
Run defers close(col.done), which will panic if Run is ever called more than once (including concurrent calls). Even though the doc comment says consecutive calls are not allowed, a public API should return a deterministic error instead of panicking. Consider guarding entry with something like CompareAndSwap(false, true) (or a sync.Once/state check) and return an error when Run is invoked after it has already started/returned, and ensure done is only closed once.
| col.runStarted.Store(true) | |
| if !col.runStarted.CompareAndSwap(false, true) { | |
| return errors.New("collector server Run was already called") | |
| } |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35120ccfea
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| case <-col.shutdownChan: | ||
| col.setCollectorState(StateClosed) | ||
| return errors.New("collector server was already shut down") |
There was a problem hiding this comment.
This early-return branch skips col.shutdown(...), so when Shutdown() was called before Run(), Run exits without ever calling col.configProvider.Shutdown. NewCollector has already instantiated confmap providers, and provider lifecycle requires Shutdown to release resources/goroutines; in this new pre-shutdown flow, custom providers can leak background resources for the rest of the process.
Useful? React with 👍 / 👎.
35120cc to
537059d
Compare
|
Rebased and pushed — the implementation addresses both the CompareAndSwap guard for double-Run prevention and the config provider shutdown on early return when CI is waiting on first-time contributor approval from a maintainer — @axw would you mind approving the workflow runs when you get a chance? Happy to iterate on anything once CI results are in. |
537059d to
479a943
Compare
|
Addressed @axw's review: Shutdown-before-Run no longer returns an error — |
|
Switched to axw's point about the configProvider.Shutdown wrapping is addressed in the same commit. Pushed. |
bb20b84 to
08152b1
Compare
|
Addressed review — fixed component path to |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #14989 +/- ##
==========================================
- Coverage 91.24% 90.30% -0.94%
==========================================
Files 699 699
Lines 44913 53703 +8790
==========================================
+ Hits 40979 48497 +7518
- Misses 2786 4060 +1274
+ Partials 1148 1146 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
08152b1 to
548c4dd
Compare
|
added a test for the configProvider.Shutdown error path, should bring patch coverage to 100% |
This comment was marked as outdated.
This comment was marked as outdated.
|
@Rajneesh180 please fix the lint issues |
|
Fixed the lint — |
dmathieu
added
the
ready-to-merge
638babd to
2fb61e8
Compare
2fb61e8 to
ac54c9e
Compare
Shutdown() currently returns before Run() finishes its cleanup, so callers that rely on Shutdown returning to mean "all resources freed" can hit goroutine leaks or use-after-close bugs (open-telemetry#4947). Fix: a done channel is closed via defer at the end of Run(), and Shutdown waits on it when Run has been called. An atomic bool tracks whether Run was called so Shutdown knows if it needs to block. This avoids the WaitGroup approach from open-telemetry#8811 that deadlocks when Shutdown precedes Run. Also guards against double-Run with CompareAndSwap (returns an error instead of panicking on double close), and shuts down the config provider in the early-return path when Shutdown was called before Run. Fixes open-telemetry#4947 Signed-off-by: Rajneesh Chaudhary <rajneeshrehsaan48@gmail.com>
Replace done channel + runStarted atomic.Bool with sync.WaitGroup per review feedback. Remove CompareAndSwap double-call guard — consecutive calls to Run are documented as unsupported behavior rather than returning an error. Addresses review comments from bogdandrutu and axw. Signed-off-by: Rajneesh Chaudhary <rajneeshrehsaan48@gmail.com>
Signed-off-by: Rajneesh Chaudhary <rajneeshrehsaan48@gmail.com>
Signed-off-by: Rajneesh Chaudhary <rajneeshrehsaan48@gmail.com>
ac54c9e to
ddc1211
Compare
|
@bogdandrutu This has been ready for a couple of weeks now with approvals from @axw and @dmathieu, and CI is green on the latest rebase. Would you be able to merge when you get a chance? Thanks! |
6caf258
|
Thank you for your contribution @Rajneesh180! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey. |
|
@Rajneesh180 thanks! Please keep an eye out for "help wanted" issues: https://github.com/open-telemetry/opentelemetry-collector/issues?q=is%3Aissue%20state%3Aopen%20label%3A%22help%20wanted%22 |
Right now `Shutdown()` returns before `Run()` is done cleaning up, which means callers that treat "Shutdown returned" as "everything is freed" can run into goroutine leaks and use-after-close problems. This is the root of open-telemetry#4947. The fix adds a `done` channel that `Run()` closes via `defer` when it finishes, and an `atomic.Bool` so `Shutdown()` knows whether `Run()` was called. If it was, `Shutdown()` blocks on `<-col.done` until `Run()` is fully done. This sidesteps the WaitGroup idea from open-telemetry#8811 that deadlocks when `Shutdown` happens before `Run`. I also added a `CompareAndSwap` guard on the `Run()` entry so calling it twice returns an error instead of panicking on the double-close of the done channel, and the early-return path (when Shutdown was called before Run) now properly shuts down the config provider so we don't leak confmap resources. Tests cover shutdown-before-run, shutdown-during-run, double-run, and the blocking guarantee. Fixes open-telemetry#4947 --------- Signed-off-by: Rajneesh Chaudhary <rajneeshrehsaan48@gmail.com>
Right now
Shutdown()returns beforeRun()is done cleaning up, which means callers that treat "Shutdown returned" as "everything is freed" can run into goroutine leaks and use-after-close problems. This is the root of #4947.The fix adds a
donechannel thatRun()closes viadeferwhen it finishes, and anatomic.BoolsoShutdown()knows whetherRun()was called. If it was,Shutdown()blocks on<-col.doneuntilRun()is fully done. This sidesteps the WaitGroup idea from #8811 that deadlocks whenShutdownhappens beforeRun.I also added a
CompareAndSwapguard on theRun()entry so calling it twice returns an error instead of panicking on the double-close of the done channel, and the early-return path (when Shutdown was called before Run) now properly shuts down the config provider so we don't leak confmap resources.Tests cover shutdown-before-run, shutdown-during-run, double-run, and the blocking guarantee.
Fixes #4947