[bot-detection] Fix leak of background pre-warmer goroutine - #4589
Conversation
|
@buixor: There are no 'kind' label on this PR. You need a 'kind' label to generate the release automatically.
DetailsI am a bot created to help the crowdsecurity developers manage community feedback and contributions. You can check out my manifest file to understand my behavior and what I can do. If you want to use this for your project, you can check out the BirthdayResearch/oss-governance-bot repository. |
|
@buixor: There are no area labels on this PR. You can add as many areas as you see fit.
DetailsI am a bot created to help the crowdsecurity developers manage community feedback and contributions. You can check out my manifest file to understand my behavior and what I can do. If you want to use this for your project, you can check out the BirthdayResearch/oss-governance-bot repository. |
There was a problem hiding this comment.
🟢 Ready to approve
The change cleanly addresses the reported goroutine leak with minimal surface area and adds a targeted regression test to prevent recurrence.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR fixes a goroutine leak in the AppSec challenge runtime where dynamicModulePreWarmer could keep ticking after a reload closed the underlying wazero runtime, resulting in repeated warning logs and accumulating background goroutines.
Changes:
- Run the dynamic-module pre-warmer under a
Close()-owned cancellable context and cancel it during shutdown. - Suppress the expected “runtime closed” instantiation error during shutdown by exiting the pre-warmer when its context is canceled.
- Add a regression test using
go.uber.org/goleakto ensure the pre-warmer goroutine is not left behind afterClose().
File summaries
| File | Description |
|---|---|
| pkg/appsec/challenge/dynamic_module.go | Exits pre-warmer quietly on shutdown cancellation to avoid noisy warnings. |
| pkg/appsec/challenge/challenge.go | Introduces preWarmCancel, wires a cancelable pre-warmer context, cancels it in Close(). |
| pkg/appsec/challenge/close_test.go | Adds goleak-based regression test ensuring Close() stops the pre-warmer goroutine. |
| go.mod | Adds go.uber.org/goleak as a direct dependency for the new test. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4589 +/- ##
==========================================
- Coverage 64.36% 63.84% -0.52%
==========================================
Files 499 516 +17
Lines 38544 39284 +740
==========================================
+ Hits 24807 25079 +272
- Misses 11413 11835 +422
- Partials 2324 2370 +46
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
/kind enhancement |
reloads lead to a leak of
dynamicModulePreWarmer(wasm runtime was shutdown, but not the associated go routine). Leads to increasing spam of this error in logs: