Refactor call manager in buddy #454
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis PR refactors the Breeze Buddy call management system by extracting lead processing, configuration management, outbound number handling, retry scheduling, call recordings, webhook delivery, and stuck-lead cleanup logic into seven dedicated manager modules, updating the main calls.py to depend on these new public interfaces. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
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 |
badri-singhal
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
app/ai/voice/agents/breeze_buddy/managers/config.py (1)
32-47: Replace EN DASH with HYPHEN-MINUS in comments.The static analysis tool flagged ambiguous Unicode characters. Use standard ASCII hyphens for consistency.
🔎 Proposed fix
if config.call_start_time <= config.call_end_time: - # Normal case (e.g., 09:00–17:00) + # Normal case (e.g., 09:00-17:00) return config.call_start_time <= current_time <= config.call_end_time else: - # Overnight case (e.g., 22:00–06:00) + # Overnight case (e.g., 22:00-06:00) return ( current_time >= config.call_start_time or current_time <= config.call_end_time )app/ai/voice/agents/breeze_buddy/managers/recordings.py (1)
39-43: Moveprovider.lower()before the lead lookup.The provider normalization at line 40 happens after the async call but before the
if not leadcheck. While functionally correct, it would be cleaner to normalize the provider at the start of the function for clarity and to avoid unnecessary processing if an early return happens.🔎 Proposed fix
logger.info( f"Processing call recording for call_id: {call_id} from provider: {provider}" ) + provider = provider.lower() lead = await get_lead_by_call_id(call_id) - provider = provider.lower() if not lead: logger.error(f"Could not find lead for call_id: {call_id}") returnapp/ai/voice/agents/breeze_buddy/managers/outbound_numbers.py (1)
144-148: Potential negative channel count.If
release_numberis called whenchannelsis already 0 (or due to a race condition), the channel count could become negative. Consider adding a floor check.🔎 Proposed fix
elif provider == CallProvider.EXOTEL: outbound_number = await get_outbound_number_by_id(number_id) if outbound_number: + new_channels = max(0, outbound_number.channels - 1) if outbound_number.channels else 0 await update_outbound_number_channels( - number_id, outbound_number.channels - 1 + number_id, new_channels )app/ai/voice/agents/breeze_buddy/managers/calls.py (1)
63-65: Explicitreturn Nonefor clarity.The function signature declares
-> Optional[LeadCallTracker], but line 65 uses barereturn. While functionally equivalent, explicitreturn Noneimproves readability.🔎 Proposed fix
lead = await get_lead_by_call_id(call_id) if not lead: logger.error(f"Could not find lead for call_id: {call_id}") - return + return Noneapp/ai/voice/agents/breeze_buddy/managers/processor.py (1)
291-296: Remove unusedretry_successvariable.The variable is assigned but never used, as flagged by static analysis. If the retry outcome doesn't affect subsequent logic, remove the assignment.
🔎 Proposed fix
# Attempt retry with alternate provider - retry_success = await _handle_call_initiation_failure_with_retry( + await _handle_call_initiation_failure_with_retry( session, locked_lead, number_to_use, config, use_template_flow )
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/ai/voice/agents/breeze_buddy/managers/calls.pyapp/ai/voice/agents/breeze_buddy/managers/cleanup.pyapp/ai/voice/agents/breeze_buddy/managers/config.pyapp/ai/voice/agents/breeze_buddy/managers/outbound_numbers.pyapp/ai/voice/agents/breeze_buddy/managers/processor.pyapp/ai/voice/agents/breeze_buddy/managers/recordings.pyapp/ai/voice/agents/breeze_buddy/managers/retry.pyapp/ai/voice/agents/breeze_buddy/managers/webhooks.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-24T07:52:29.756Z
Learnt from: badri-singhal
Repo: juspay/clairvoyance PR: 445
File: app/database/migrations/009_add_merchant_template_to_outbound_number.sql:38-38
Timestamp: 2025-12-24T07:52:29.756Z
Learning: In the outbound_number table in app/database/migrations/009_add_merchant_template_to_outbound_number.sql, duplicate phone numbers across merchant_id and shop_identifier combinations are intentionally allowed. No composite unique constraint on (number, merchant_id, shop_identifier) is required.
Applied to files:
app/ai/voice/agents/breeze_buddy/managers/outbound_numbers.py
🧬 Code graph analysis (7)
app/ai/voice/agents/breeze_buddy/managers/recordings.py (3)
app/database/accessor/breeze_buddy/lead_call_tracker.py (2)
get_lead_by_call_id(193-212)update_lead_call_recording_url(263-286)app/services/gcp/storage/storage.py (1)
upload_file_to_gcs(75-110)app/ai/voice/agents/breeze_buddy/managers/calls.py (1)
update_call_recording(126-145)
app/ai/voice/agents/breeze_buddy/managers/webhooks.py (3)
app/ai/voice/agents/breeze_buddy/utils/common.py (1)
send_webhook_with_retry(57-99)app/schemas/breeze_buddy/core.py (1)
LeadCallTracker(34-57)app/ai/voice/agents/breeze_buddy/template/context.py (1)
reporting_webhook_url(97-99)
app/ai/voice/agents/breeze_buddy/managers/config.py (2)
app/database/accessor/breeze_buddy/call_execution_config.py (1)
get_call_execution_config_by_merchant_id(91-134)app/schemas/breeze_buddy/core.py (2)
CallExecutionConfig(118-134)LeadCallTracker(34-57)
app/ai/voice/agents/breeze_buddy/managers/cleanup.py (6)
app/ai/voice/agents/breeze_buddy/managers/config.py (1)
get_lead_config(13-29)app/ai/voice/agents/breeze_buddy/managers/outbound_numbers.py (1)
release_number(138-149)app/ai/voice/agents/breeze_buddy/managers/retry.py (1)
schedule_retry(17-56)app/database/accessor/breeze_buddy/lead_call_tracker.py (4)
acquire_lock_on_lead_by_id(119-138)get_leads_by_status_and_time_before(361-377)release_lock_on_lead_by_id(141-160)update_lead_call_completion_details(289-319)app/database/accessor/breeze_buddy/outbound_number.py (1)
get_outbound_number_by_id(77-99)app/schemas/breeze_buddy/core.py (1)
LeadCallStatus(25-31)
app/ai/voice/agents/breeze_buddy/managers/retry.py (4)
app/ai/voice/agents/breeze_buddy/managers/webhooks.py (1)
send_no_answer_webhook(46-85)app/core/transport/http_client.py (1)
create_aiohttp_session(58-77)app/database/accessor/breeze_buddy/lead_call_tracker.py (1)
create_lead_call_tracker(43-95)app/schemas/breeze_buddy/core.py (2)
CallExecutionConfig(118-134)LeadCallTracker(34-57)
app/ai/voice/agents/breeze_buddy/managers/processor.py (8)
app/ai/voice/agents/breeze_buddy/managers/config.py (2)
get_lead_config(13-29)is_within_calling_hours(32-47)app/ai/voice/agents/breeze_buddy/managers/outbound_numbers.py (4)
acquire_number(128-135)get_available_number(23-96)get_available_number_by_provider(99-125)release_number(138-149)app/ai/voice/agents/breeze_buddy/managers/webhooks.py (1)
send_call_failure_webhook(14-43)app/ai/voice/agents/breeze_buddy/services/telephony/utils.py (1)
get_voice_provider(13-20)app/database/accessor/breeze_buddy/lead_call_tracker.py (4)
acquire_lock_on_lead_by_id(119-138)release_lock_on_lead_by_id(141-160)update_lead_call_completion_details(289-319)update_lead_call_details(163-190)app/database/accessor/breeze_buddy/template.py (1)
get_template_by_merchant(35-77)app/schemas/breeze_buddy/core.py (3)
CallProvider(18-22)LeadCallStatus(25-31)LeadCallTracker(34-57)app/ai/voice/agents/breeze_buddy/template/context.py (1)
provider(117-119)
app/ai/voice/agents/breeze_buddy/managers/calls.py (11)
app/helpers/automatic/process_pool.py (1)
cleanup(677-707)app/ai/voice/agents/breeze_buddy/managers/cleanup.py (1)
cleanup_stuck_leads(23-72)app/ai/voice/agents/breeze_buddy/managers/config.py (1)
get_lead_config(13-29)app/ai/voice/agents/breeze_buddy/managers/outbound_numbers.py (1)
release_number(138-149)app/ai/voice/agents/breeze_buddy/managers/processor.py (1)
process_single_lead(208-300)app/ai/voice/agents/breeze_buddy/managers/recordings.py (1)
update_call_recording(17-100)app/ai/voice/agents/breeze_buddy/managers/retry.py (1)
schedule_retry(17-56)app/core/transport/http_client.py (1)
create_aiohttp_session(58-77)app/database/accessor/breeze_buddy/lead_call_tracker.py (3)
get_lead_by_call_id(193-212)get_leads_based_on_status_and_next_attempt(98-116)update_lead_call_completion_details(289-319)app/database/accessor/breeze_buddy/outbound_number.py (1)
get_outbound_number_by_id(77-99)app/schemas/breeze_buddy/core.py (2)
LeadCallStatus(25-31)LeadCallTracker(34-57)
🪛 Ruff (0.14.10)
app/ai/voice/agents/breeze_buddy/managers/recordings.py
96-96: Do not catch blind exception: Exception
(BLE001)
app/ai/voice/agents/breeze_buddy/managers/webhooks.py
42-42: Do not catch blind exception: Exception
(BLE001)
84-84: Do not catch blind exception: Exception
(BLE001)
app/ai/voice/agents/breeze_buddy/managers/config.py
40-40: Comment contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF003)
43-43: Comment contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF003)
app/ai/voice/agents/breeze_buddy/managers/cleanup.py
67-67: Do not catch blind exception: Exception
(BLE001)
app/ai/voice/agents/breeze_buddy/managers/processor.py
291-291: Local variable retry_success is assigned to but never used
Remove assignment to unused variable retry_success
(F841)
296-296: Consider moving this statement to an else block
(TRY300)
298-298: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (9)
app/ai/voice/agents/breeze_buddy/managers/config.py (1)
13-29: LGTM!The function correctly fetches configs by merchant/shop and filters by template. The early return pattern with appropriate warning logs is clean.
app/ai/voice/agents/breeze_buddy/managers/recordings.py (1)
45-100: LGTM with note on error handling.The fallback behavior (storing provider URL on download/upload failure) is a sensible degradation pattern. The broad
Exceptioncatch is acceptable here as a top-level handler that logs with stack trace viaexc_info=True.One minor observation: there's no return or database update in the exception handler at line 96-100, so if an exception occurs mid-processing, the lead's recording URL may remain unset. Consider whether a fallback to the provider URL should be attempted in the exception path as well.
app/ai/voice/agents/breeze_buddy/managers/cleanup.py (1)
23-72: LGTM overall.The cleanup logic is well-structured with proper lock management via try/finally. The per-lead error handling ensures one failed cleanup doesn't block others.
app/ai/voice/agents/breeze_buddy/managers/retry.py (1)
17-56: LGTM!The retry logic correctly handles:
- Scheduling retries when attempts remain (using
attempt_count < max_retry - 1)- Sending NO_ANSWER webhook when max retries are exhausted
- Propagating
use_template_flowmetadata to the new leadThe session management with
async with create_aiohttp_session()ensures proper cleanup.app/ai/voice/agents/breeze_buddy/managers/outbound_numbers.py (1)
23-96: LGTM on the number selection logic.The dual-path approach (template-based vs backward-compatible) is well-documented and correctly handles provider-specific constraints (EXOTEL channel limits vs TWILIO availability).
app/ai/voice/agents/breeze_buddy/managers/calls.py (2)
35-49: LGTM!The backlog processing flow is clean: cleanup stuck leads first, then process each lead through the dedicated processor. The session management via
async withensures proper resource cleanup.
126-145: LGTM!The recording handler is now a thin wrapper that delegates to the extracted
update_recordingfunction, maintaining the same public API.app/ai/voice/agents/breeze_buddy/managers/processor.py (2)
34-54: LGTM on helper structure.The helper functions are well-factored with clear single responsibilities. The call initiation logic cleanly separates success and failure handling paths.
107-205: LGTM on retry with alternate provider logic.The provider flip logic (Twilio ↔ Exotel) with proper international calling constraints and failure webhooks is well-implemented.
badri-singhal
force-pushed
the
refactor-call-manager-in-buddy
branch
from
81dfd99 to
0115ebd
Compare
|
@coderabbitai please review thoroughly and check for each edge cases. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 20737 🏁 Script executed: Length of output: 11711 @badri-singhal, I've completed a thorough review of all 8 files in this refactoring. Here's my comprehensive edge case analysis organized by severity: 🔴 Critical Issues1. Non-Atomic Number Acquisition Pattern (All modules using numbers)Risk: Race condition where multiple processes could acquire the same number simultaneously. Location: number_to_use = await get_available_number(config, template) # Check
if not number_to_use:
return True
await acquire_number(number_to_use) # Acquire (separate operation)Issue: Between checking availability and acquiring, another process could grab the same number. This is especially problematic for Twilio numbers where status changes from AVAILABLE → IN_USE, but two processes could both see it as AVAILABLE. Recommendation: Consider database-level atomic operations (SELECT FOR UPDATE) or optimistic locking with version numbers. 2. Exotel Channel Management with None Values (
|
1 participant
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.