fix: guard semantic cache Cleanup with sync.Once to prevent double-close panic - #3532
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe Plugin struct now includes a ChangesCleanup Idempotency Implementation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
Confidence Score: 4/5Safe to merge; the fix is minimal, correct, and well-scoped to the panic scenario described. The change is a single, focused addition of sync.Once around an already-correct cleanup sequence. The only gap is that no regression test was added to lock in the new behavior, so the guard could silently disappear in a future refactor. plugins/semanticcache/main.go — verify the lack of a regression test is acceptable given the existing Cleanup test suite. Important Files Changed
Reviews (1): Last reviewed commit: "fix: cleanup once check added" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
|
Merge activity
|
…e-close panic (#3532) ## Summary The `Cleanup` method on the semantic cache plugin could panic if called more than once, because `close(stopCh)` would be invoked on an already-closed channel. This happens when the plugin is registered against multiple interface caches, causing the harness to invoke `Cleanup` multiple times. ## Changes - Introduced a `sync.Once` field (`cleanupOnce`) on the `Plugin` struct to guard the body of `Cleanup`, ensuring `close(stopCh)` and the subsequent wait/sweep logic execute exactly once regardless of how many times `Cleanup` is called. ## Type of change - [x] Bug fix ## Affected areas - [x] Plugins ## How to test Register the semantic cache plugin against multiple interface caches and trigger shutdown. Verify that `Cleanup` is called more than once without panicking. ```sh go test ./plugins/semanticcache/... ``` ## Breaking changes - [x] No ## Security considerations None. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
…e-close panic (#3532) ## Summary The `Cleanup` method on the semantic cache plugin could panic if called more than once, because `close(stopCh)` would be invoked on an already-closed channel. This happens when the plugin is registered against multiple interface caches, causing the harness to invoke `Cleanup` multiple times. ## Changes - Introduced a `sync.Once` field (`cleanupOnce`) on the `Plugin` struct to guard the body of `Cleanup`, ensuring `close(stopCh)` and the subsequent wait/sweep logic execute exactly once regardless of how many times `Cleanup` is called. ## Type of change - [x] Bug fix ## Affected areas - [x] Plugins ## How to test Register the semantic cache plugin against multiple interface caches and trigger shutdown. Verify that `Cleanup` is called more than once without panicking. ```sh go test ./plugins/semanticcache/... ``` ## Breaking changes - [x] No ## Security considerations None. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
2 participants
Summary
The
Cleanupmethod on the semantic cache plugin could panic if called more than once, becauseclose(stopCh)would be invoked on an already-closed channel. This happens when the plugin is registered against multiple interface caches, causing the harness to invokeCleanupmultiple times.Changes
sync.Oncefield (cleanupOnce) on thePluginstruct to guard the body ofCleanup, ensuringclose(stopCh)and the subsequent wait/sweep logic execute exactly once regardless of how many timesCleanupis called.Type of change
Affected areas
How to test
Register the semantic cache plugin against multiple interface caches and trigger shutdown. Verify that
Cleanupis called more than once without panicking.go test ./plugins/semanticcache/...Breaking changes
Security considerations
None.
Checklist
docs/contributing/README.mdand followed the guidelines