fix(breeze_buddy): guard handle_call_completion against duplicate finished-lead callbacks - #938
fix(breeze_buddy): guard handle_call_completion against duplicate finished-lead callbacks#938Tara-ag wants to merge 2 commits into
Conversation
…dy-finished leads Adds a status guard in handle_call_completion right after the lead lookup so that concurrent completion callbacks for the same lead short-circuit once the lead is already FINISHED, avoiding redundant DB updates, redundant outbound number releases, and duplicate retry scheduling under DB row-locking contention.
WalkthroughAdds a concurrency guard to call completion handling so callbacks for already finished leads exit before repeating updates, number releases, or retry scheduling. ChangesCall completion handling
Estimated code review effort: 2 (Simple) | ~5 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/ai/voice/agents/breeze_buddy/managers/calls.py`:
- Around line 501-509: Move the lead.status == LeadCallStatus.FINISHED guard in
the completion callback below the outbound number release block, ensuring
_release_number executes before any early return. Preserve the guard’s
duplicate-completion behavior and logging after the release so channel tokens
are always reclaimed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bae7c709-becb-48bc-a5a5-2e986ee17bc1
📒 Files selected for processing (1)
app/ai/voice/agents/breeze_buddy/managers/calls.py
| # Guard: if a concurrent callback already finished this lead, skip to avoid | ||
| # redundant DB updates, redundant outbound number releases, and duplicate | ||
| # retry scheduling caused by racing completion callbacks for the same lead. | ||
| if lead.status == LeadCallStatus.FINISHED: | ||
| logger.info( | ||
| f"Lead {lead.id} is already FINISHED for call_id: {call_id}, skipping duplicate completion callback." | ||
| ) | ||
| return lead | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Relocate the FINISHED guard below the outbound number release to prevent channel token leaks.
Although the PR description explicitly mentions preventing "redundant outbound number releases," this contradicts the established resource-management pattern documented in the adjacent handle_unanswered_calls function (lines 615-617).
If a duplicate call was actually initiated, it acquired its own channel token. Returning early before releasing the number will leak that token. As noted in the existing comments, _release_number is idempotent, and releasing tokens is safe because over-counts are trimmed by the periodic reconciliation, whereas skipping the release results in a leaked channel allocation.
Consider moving this guard immediately after the outbound number release block.
🔄 Proposed relocation
- # Guard: if a concurrent callback already finished this lead, skip to avoid
- # redundant DB updates, redundant outbound number releases, and duplicate
- # retry scheduling caused by racing completion callbacks for the same lead.
- if lead.status == LeadCallStatus.FINISHED:
- logger.info(
- f"Lead {lead.id} is already FINISHED for call_id: {call_id}, skipping duplicate completion callback."
- )
- return lead
-
# Always release outbound number (including transfers — bot leaves, cleanup happens here)
if lead.outbound_number_id:
outbound_number = await get_outbound_number_by_id(lead.outbound_number_id)
if outbound_number:
await _release_number(outbound_number.id, outbound_number.provider)
# Event-driven dispatch: return a token to the channel semaphore.
# Idempotent in aggregate — reconcile_channel_tokens trims any
# over-count caused by duplicate webhooks within 60s.
await release_channel_token(outbound_number.id)
else:
logger.error(
f"Could not find outbound number with id: {lead.outbound_number_id} to release."
)
else:
logger.info(f"No outbound number id for lead: {lead.id}")
+
+ # Guard: if a concurrent callback already finished this lead, skip to avoid
+ # redundant DB updates and duplicate retry scheduling caused by racing
+ # completion callbacks for the same lead.
+ if lead.status == LeadCallStatus.FINISHED:
+ logger.info(
+ f"Lead {lead.id} is already FINISHED for call_id: {call_id}, skipping duplicate completion callback."
+ )
+ return lead🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/ai/voice/agents/breeze_buddy/managers/calls.py` around lines 501 - 509,
Move the lead.status == LeadCallStatus.FINISHED guard in the completion callback
below the outbound number release block, ensuring _release_number executes
before any early return. Preserve the guard’s duplicate-completion behavior and
logging after the release so channel tokens are always reclaimed.
There was a problem hiding this comment.
Pull request overview
This PR aims to make Breeze Buddy’s telephony completion path (handle_call_completion) idempotent when multiple “call finished” callbacks race for the same lead, by short-circuiting when the lead is already FINISHED to reduce DB contention and duplicate side effects.
Changes:
- Add an early-return guard in
handle_call_completionwhenlead.status == LeadCallStatus.FINISHED. - Log and return the already-finished lead to avoid duplicate completion processing.
| if lead.status == LeadCallStatus.FINISHED: | ||
| logger.info( | ||
| f"Lead {lead.id} is already FINISHED for call_id: {call_id}, skipping duplicate completion callback." | ||
| ) | ||
| return lead |
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review Summary
Files reviewed: 1 (app/ai/voice/agents/breeze_buddy/managers/calls.py)
New issues raised this run: 0
I inspected the single-file diff and the existing review threads. The concurrency guard itself is a reasonable fix for duplicate completion callbacks, but the placement before the outbound-number/channel release is already flagged by two existing inline comments (CodeRabbit and Copilot). I concur with that feedback: handle_unanswered_calls in the same file releases the outbound number before its FINISHED guard, and the release helpers are idempotent, so moving the guard below the release block avoids leaking channel tokens when a duplicate callback arrives after the lead is already marked FINISHED.
No additional blocking issues (hardcoded secrets, SQL injection, auth/authorization flaws, SSRF, PII exposure, or migration edits) were found in this change. The existing feedback should be addressed before merge.
Summary
Adds a status guard in
handle_call_completion(app/ai/voice/agents/breeze_buddy/managers/calls.py), placed immediately after the lead lookup. Iflead.status == LeadCallStatus.FINISHED, the function logs an info message and returns the lead immediately, short-circuiting before:This resolves DB row-locking and contention issues seen during concurrent call completion callbacks for the same lead (racing webhooks/callbacks arriving independently for a lead that has already been marked FINISHED).
Files changed
app/ai/voice/agents/breeze_buddy/managers/calls.py— added early-return guard inhandle_call_completionVerification
uv run black --check .— passuv run isort . --profile black --check-only— passuv run pyrefly check— pass (no new errors introduced)Note for reviewer
This branch currently has 2 commits ahead of
release: an empty auto-generatedchore: start tara task ...commit created when the branch was provisioned, and this fix commit. Per repo convention, PRs must contain exactly 1 commit (enforced in CI) — squashing would require a force-push, which is outside this automation's permitted actions. Please squash-merge, or let me know if you'd like the branch squashed before merge.Discussion
Original Slack thread: https://slack.com/archives/C09ST3HSDT6/p1784636128369249
Summary by CodeRabbit