Conversation
product-security-plaid-production
bot
requested review from
DylanTinianov and
ettec
|
👋 cedric-cordenier, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
|
I see you updated files related to
|
|
✅ No conflicts with other open PRs targeting |
There was a problem hiding this comment.
Pull request overview
Risk Rating: MEDIUM (concurrency change in core capabilities launcher shutdown/service lifecycle)
This PR addresses a data race in the capabilities launcher by synchronizing access to the subServices slice, which is used to track started sub-services for later shutdown.
Changes:
- Add a mutex (
subServicesMux) to guard concurrent reads/writes tolauncher.subServices. - Route all
subServicesappends through a new helper (appendSubService) that takes the write lock. - Add locking in
Close()to prevent concurrent modification while iterating sub-services.
| defer w.subServicesMux.RUnlock() | ||
| for _, s := range w.subServices { |
There was a problem hiding this comment.
Close() holds subServicesMux.RLock() while calling s.Close() on each sub-service. Calling into external code while holding a lock can lead to lock contention and potential deadlocks; it also blocks concurrent appendSubService calls for the duration of every sub-service close. Consider copying w.subServices under the lock, releasing the lock, and then iterating/closing the snapshot.
| defer w.subServicesMux.RUnlock() | |
| for _, s := range w.subServices { | |
| subServices := make([]services.ServiceCtx, 0, len(w.subServices)) | |
| subServices = append(subServices, w.subServices...) | |
| w.subServicesMux.RUnlock() | |
| for _, s := range subServices { |
| func (w *launcher) appendSubService(svc services.Service) { | ||
| w.subServicesMux.Lock() | ||
| defer w.subServicesMux.Unlock() | ||
| w.subServices = append(w.subServices, svc) | ||
| } |
There was a problem hiding this comment.
This PR introduces new synchronization around subServices, but there isn't a regression test covering concurrent Close() vs. sub-service registration. Since the repo runs Go race tests, consider adding a unit test that concurrently calls appendSubService/receiver start paths and Close() to ensure this race stays fixed (and to avoid future refactors reintroducing it).
|
Flagged by @jmank88 , there's a potential race in the launcher where we add to subservices and try to read them at the same time.