Release lock in AckEvent in base capability - #2358
Conversation
|
👋 yashnevatia, 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! |
📊 API Diff Results
|
There was a problem hiding this comment.
🟡 Changes recommended
The concurrency/deadlock fix should be accompanied by a targeted regression test to ensure AckEvent cannot reintroduce lock-holding behavior during capability updates/removal.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates the base capabilities registry wrappers to avoid holding the wrapper mutex while forwarding AckEvent calls to the underlying capability, reducing the risk of lock contention/deadlocks during acknowledgements.
Changes:
- Update
atomicTriggerCapability.AckEventto snapshot the underlying capability underRLockand callAckEventwithout holding the mutex. - Update
atomicExecuteAndTriggerCapability.AckEventwith the same lock-release/snapshot pattern.
File summaries
| File | Description |
|---|---|
| pkg/capabilities/registry/base.go | Releases wrapper lock before calling underlying AckEvent by snapshotting cap under RLock |
Review details
Suppressed comments (1)
pkg/capabilities/registry/base.go:571
- Same as the trigger-only wrapper: this lock-release change in AckEvent should be covered by a regression test that simulates a blocked underlying AckEvent and verifies capability updates/removal don’t block (to prevent deadlocks from being reintroduced).
func (a *atomicExecuteAndTriggerCapability) AckEvent(ctx context.Context, triggerID string, eventID string, method string) error {
a.mu.RLock()
cap := a.cap
a.mu.RUnlock()
if cap == nil {
return errors.New("capability unavailable")
}
return cap.AckEvent(ctx, triggerID, eventID, method)
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| func (a *atomicTriggerCapability) AckEvent(ctx context.Context, triggerID string, eventID string, method string) error { | ||
| a.mu.Lock() | ||
| defer a.mu.Unlock() | ||
| if a.cap == nil { | ||
| a.mu.RLock() | ||
| cap := a.cap | ||
| a.mu.RUnlock() | ||
| if cap == nil { | ||
| return errors.New("capability unavailable") | ||
| } | ||
| return a.cap.AckEvent(ctx, triggerID, eventID, method) | ||
| return cap.AckEvent(ctx, triggerID, eventID, method) |
Requires
Supports