Refactoring the call managers into separate modules#501
Refactoring the call managers into separate modules#501Swetha-160303 wants to merge 1 commit intojuspay:releasefrom
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 Use the checkbox below for a quick retry:
WalkthroughThe change refactors Breeze Buddy call management by decomposing a monolithic Changes
Sequence DiagramsequenceDiagram
participant Cron as Cron Task
participant Init as initiate_calls()
participant Config as Configuration
participant Number as Number Manager
participant Provider as Call Provider<br/>(Twilio/Exotel)
participant Webhook as Webhook Service
participant Retry as Retry Manager
participant Record as Recording Manager
Cron->>Init: Trigger backlog processing
Init->>Init: Clean up timeout leads (>10 min)
loop For each pending lead
Init->>Config: Get lead config
Config-->>Init: CallExecutionConfig
Init->>Init: Validate calling hours
Init->>Number: Get available number
Number-->>Init: OutboundNumber
Init->>Number: Acquire number (mark IN_USE)
Number-->>Init: Acquisition success
Init->>Provider: Initiate call
alt Call succeeds
Provider-->>Init: Call initiated
else Call fails
Init->>Webhook: Send failure webhook
Webhook-->>Init: Webhook sent
Init->>Retry: Attempt fallback provider
alt Retry succeeds
Retry->>Provider: Initiate call with alt provider
Provider-->>Retry: Call initiated
else Retry fails
Retry->>Number: Release number
Number-->>Retry: Number released
end
end
end
Init-->>Cron: Processing complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 6
🤖 Fix all issues with AI agents
In `@app/ai/voice/agents/breeze_buddy/managers/__init__.py`:
- Around line 1-15: The file
app/ai/voice/agents/breeze_buddy/managers/__init__.py fails Black formatting;
run the formatter (e.g., black . or black --check --diff .) and commit the
reformatted file so CI passes, ensuring the module docstring in __init__.py
remains intact and properly formatted after Black runs.
- Around line 244-366: The try/except in initiate_calls can leave a lead locked
if an exception occurs after acquire_lock_on_lead_by_id; wrap the per-lead work
in a try/finally (or ensure in the except you call release_lock_on_lead_by_id)
so release_lock_on_lead_by_id(locked_lead.id) is always called when locked_lead
was acquired; use the locked_lead variable (or a boolean flag) to check whether
a lock was obtained before releasing to avoid releasing nonexistent locks.
In `@app/ai/voice/agents/breeze_buddy/managers/post_call_handlers.py`:
- Around line 37-43: lead.outbound_number_id is optional, so avoid calling
get_outbound_number_by_id with None; first check if lead.outbound_number_id is
truthy and only then await get_outbound_number_by_id(lead.outbound_number_id)
and call release_number(outbound_number.id, outbound_number.provider); if it is
None, skip the lookup and call and emit a clear debug/info log via logger
indicating there was no outbound_number_id to release. Ensure you still handle
the case where get_outbound_number_by_id returns None by logging the existing
error message.
In `@app/ai/voice/agents/breeze_buddy/managers/recording.py`:
- Around line 100-103: The except Exception as e block that currently only calls
logger.error for the call_id should also perform the same fallback lead update
used in the earlier failure paths (the logic that sets the lead's provider
recording URL when processing fails) so the lead isn't left without any
recording URL; locate the except block that calls logger.error and invoke the
existing fallback function or replicate the same update (the same code used on
the other failure paths) to set the provider recording URL for the lead, and
wrap that fallback call in its own try/except to log any secondary errors
without swallowing the original exception.
In `@app/ai/voice/agents/breeze_buddy/managers/webhook.py`:
- Around line 38-78: The code in send_no_answer_webhook accesses
lead.payload.get(...) without guarding for lead.payload being None
(LeadCallTracker.payload is optional); update the function to first handle a
missing payload (e.g., treat payload as empty dict or check if lead.payload is
truthy) before extracting reporting_webhook_url, order_id, and any other fields
used to build summary_data so that calls like reporting_webhook_url =
lead.payload.get("reporting_webhook_url") and lead.payload.get("order_id") are
safe; ensure the null-safe approach is applied consistently when building
summary_data and logging.
In `@app/api/routers/breeze_buddy/telephony/callbacks/handlers.py`:
- Around line 140-142: The current handlers.py routes call_status "failed" and
"busy" to handle_call_no_answer which hardcodes outcome="NO_ANSWER"; change the
branching so only "no-answer" calls invoke handle_call_no_answer(call_sid) and
route "failed" and "busy" to handle_call_completion with explicit outcomes
(e.g., await handle_call_completion(call_sid, outcome="FAILED") and await
handle_call_completion(call_sid, outcome="BUSY")); update the logger messages to
reflect the exact outcome and ensure the call_sid and any other required params
match handle_call_completion's signature so retries/DB records use the correct
outcome instead of NO_ANSWER.
🧹 Nitpick comments (6)
app/ai/voice/agents/breeze_buddy/managers/utils.py (1)
14-29: Consider making timezone configurable.The IST timezone is hardcoded, which may limit flexibility if merchants operate across different timezones. Consider accepting timezone as a parameter or reading it from the config.
Additionally, the static analysis flagged ambiguous EN DASH characters (
–) in comments on lines 22 and 25. Replace with standard HYPHEN-MINUS (-).Suggested fix for EN DASH characters
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/recording.py (2)
43-47: Moveprovider.lower()before the lead lookup.The
providervariable is lowercased on line 44 after the lead lookup. If the lead lookup fails and returns early, this is fine, but for consistency and clarity, consider normalizing the provider string earlier or after the null check.Suggested reordering
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}") return + + provider = provider.lower()
83-91: Wrap synchronous GCS upload to avoid blocking the event loop.The
upload_file_to_gcscall at line 83 is a synchronous I/O operation (usesblob.upload_from_file()) executed in an async context (update_call_recording), which will block the event loop during uploads and cause latency issues under load.Use
asyncio.to_thread()to offload this to a thread pool, following the existing pattern elsewhere in the codebase:Suggested fix
import asyncio gcs_url = await asyncio.to_thread( upload_file_to_gcs, file_obj=audio_file, destination_path=f"breeze-buddy/recordings/{call_id}.{file_extension}", content_type=content_type, metadata={ "call_id": call_id, "original_url": provider_recording_url, }, )app/ai/voice/agents/breeze_buddy/managers/post_call_handlers.py (2)
84-90: Same issue: Handle potential Noneoutbound_number_id.This duplicates the pattern from
handle_call_completion. Consider extracting the number release logic into a helper function to reduce duplication and ensure consistent handling.Example helper function
async def _release_outbound_number(lead: LeadCallTracker) -> None: """Release the outbound number for a lead if present.""" if not lead.outbound_number_id: logger.warning(f"Lead {lead.id} has no outbound_number_id to release.") return outbound_number = await get_outbound_number_by_id(lead.outbound_number_id) if outbound_number: await release_number(outbound_number.id, outbound_number.provider) else: logger.error( f"Could not find outbound number with id: {lead.outbound_number_id} to release." )
57-58: Consider creating an enum for call outcomes instead of using string literals.The values
"BUSY"and"NO_ANSWER"are hardcoded strings used throughout the codebase for comparison. No centralized enum or constants currently define these outcome values. Creating aCallOutcomeenum inapp/schemas/breeze_buddy/core.py(similar to the existingLeadCallStatusenum) would reduce the risk of typos, improve consistency across the codebase, and make it easier to track valid outcome values.app/ai/voice/agents/breeze_buddy/managers/retry.py (1)
16-48: Preserve existingmetaDatawhen scheduling retries.
meta_datais recreated with onlyuse_template_flow, so any other keys inlead.metaDataare lost on retry. If those keys are needed downstream, merge them instead of replacing.♻️ Suggested fix
- meta_data={ - "use_template_flow": ( - lead.metaData.get("use_template_flow", False) - if lead.metaData - else False - ) - }, + meta_data={ + **(lead.metaData or {}), + "use_template_flow": ( + lead.metaData.get("use_template_flow", False) + if lead.metaData + else False + ), + },
app/ai/voice/agents/breeze_buddy/managers/post_call_handlers.py
Outdated
Show resolved
Hide resolved
Swetha-160303
force-pushed
the
refactor/calls-manager-modularization
branch
4 times, most recently
from
233be8e to
b5c33d3
Compare
|
@CodeRabbit please review |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@app/ai/voice/agents/breeze_buddy/managers/number.py`:
- Around line 38-86: The current branch guards the entire fallback block behind
an `else` so when template.outbound_number_id exists but the retrieved outbound
number (via get_outbound_number_by_id) is unavailable or at capacity you never
run the fallback; change the control flow so you try the template-specific
number first (call get_outbound_number_by_id and validate availability/provider
logic in that block using OutboundNumberStatus and CallProvider.EXOTEL) and if
that lookup does not yield a usable number (i.e., number is still None) then run
the existing fallback logic that calls
get_outbound_number_based_on_status_and_provider to select a matching number;
effectively remove the outer else and instead guard the fallback with "if number
is None" after the template-handling block so fallback executes when template
number is unavailable.
In `@app/ai/voice/agents/breeze_buddy/managers/post_call_handlers.py`:
- Around line 50-63: Currently the code returns early when get_lead_config(lead)
is falsy, which prevents update_lead_call_completion_details(...) from running
and leaves leads stuck in PROCESSING; remove the early return and always call
update_lead_call_completion_details(id=lead.id, status=LeadCallStatus.FINISHED,
outcome=outcome, meta_data=meta_data, call_end_time=call_end_time), then
conditionally call retry_call(lead, config, outcome) only if config is truthy
and outcome is in ["BUSY", "NO_ANSWER"]; apply the same change for both
occurrences surrounding get_lead_config, update_lead_call_completion_details,
and retry_call.
In `@app/ai/voice/agents/breeze_buddy/managers/recording.py`:
- Around line 67-69: The code currently logs unsupported providers using
logger.error(f"Unsupported provider: {provider}") and returns early, but it must
persist the provider URL so the lead still has a recording reference; before
returning, persist provider_url for the lead by reusing the same persistence
logic used for supported providers (the block that writes the recording
metadata/URL), e.g., set the lead's recording URL field and call the existing
save/persist function used elsewhere in this function/class (the same code path
used for supported providers) so provider_url is stored even when provider is
unexpected.
In `@app/ai/voice/agents/breeze_buddy/managers/retry.py`:
- Around line 32-47: The retry scheduling call to create_lead_call_tracker is
not preserving the original lead's execution_mode, causing retries to default to
TELEPHONY; update the create_lead_call_tracker invocation (in retry logic inside
managers/retry.py) to pass execution_mode=lead.execution_mode (or fallback to
the existing default only if lead.execution_mode is None) so the retried Lead
retains TELEPHONY_TEST/DAILY/etc.; ensure any other places that construct a new
lead for retry (e.g., where attempt_count and next_attempt_at are set) also
propagate execution_mode from the original lead.
In `@app/ai/voice/agents/breeze_buddy/managers/utils.py`:
- Around line 22-25: Replace the EN DASH characters in the inline comments that
show time ranges (the comments containing "09:00–17:00" and "22:00–06:00") with
a standard hyphen, i.e., change "09:00–17:00" to "09:00-17:00" and "22:00–06:00"
to "22:00-06:00" in the vicinity of the logic using config.call_start_time and
config.call_end_time so Ruff RUF003 is satisfied.
🧹 Nitpick comments (3)
app/ai/voice/agents/breeze_buddy/managers/recording.py (1)
83-91: Avoid blocking GCS upload on the event loop.Line 83 calls synchronous
upload_file_to_gcsinside an async function. That blocks the event loop during network I/O. Considerasyncio.to_thread(...)(or an async GCS client).♻️ Suggested change
+import asyncio ... - gcs_url = upload_file_to_gcs( - file_obj=audio_file, - destination_path=f"breeze-buddy/recordings/{call_id}.{file_extension}", - content_type=content_type, - metadata={ - "call_id": call_id, - "original_url": provider_recording_url, - }, - ) + gcs_url = await asyncio.to_thread( + upload_file_to_gcs, + file_obj=audio_file, + destination_path=f"breeze-buddy/recordings/{call_id}.{file_extension}", + content_type=content_type, + metadata={ + "call_id": call_id, + "original_url": provider_recording_url, + }, + )app/ai/voice/agents/breeze_buddy/managers/__init__.py (2)
54-60: Consider sorting__all__alphabetically.Ruff flags that
__all__is not sorted. While not critical, alphabetical sorting improves readability and makes it easier to spot missing/duplicate exports.♻️ Suggested fix
# Export all public functions __all__ = [ + "handle_call_completion", + "handle_call_no_answer", "initiate_calls", - "handle_call_completion", - "handle_call_no_answer", "update_call_recording", ]
361-362: Broad exception catch is acceptable here but consider logging the traceback.Catching
Exceptionin batch processing prevents one lead's failure from blocking others. However, you may want to include the traceback for better debugging:♻️ Suggested improvement
except Exception as e: - logger.error(f"Error processing lead {lead.id}: {e}") + logger.error(f"Error processing lead {lead.id}: {e}", exc_info=True)
| config = await get_lead_config(lead) | ||
| if not config: | ||
| return | ||
|
|
||
| updated_lead = await update_lead_call_completion_details( | ||
| id=lead.id, | ||
| status=LeadCallStatus.FINISHED, | ||
| outcome=outcome, | ||
| meta_data=meta_data, | ||
| call_end_time=call_end_time, | ||
| ) | ||
|
|
||
| if outcome in ["BUSY", "NO_ANSWER"]: | ||
| await retry_call(lead, config, outcome) |
There was a problem hiding this comment.
Don’t skip lead completion updates when config is missing.
Lines 50–53 and 102–104 return before update_lead_call_completion_details, leaving leads stuck in PROCESSING when config is missing. Updates should happen regardless; only the retry should be conditional.
🛠️ Proposed fix
- config = await get_lead_config(lead)
- if not config:
- return
-
updated_lead = await update_lead_call_completion_details(
id=lead.id,
status=LeadCallStatus.FINISHED,
outcome=outcome,
meta_data=meta_data,
call_end_time=call_end_time,
)
if outcome in ["BUSY", "NO_ANSWER"]:
- await retry_call(lead, config, outcome)
+ config = await get_lead_config(lead)
+ if config:
+ await retry_call(lead, config, outcome)
+ else:
+ logger.warning(
+ f"No call execution config found for lead {lead.id}; skipping retry."
+ )
...
- config = await get_lead_config(lead)
- if not config:
- return
-
await update_lead_call_completion_details(
id=lead.id,
status=LeadCallStatus.FINISHED,
outcome="NO_ANSWER",
meta_data={},
call_end_time=datetime.now(timezone.utc),
)
- await retry_call(lead, config, "NO_ANSWER")
+ config = await get_lead_config(lead)
+ if config:
+ await retry_call(lead, config, "NO_ANSWER")
+ else:
+ logger.warning(
+ f"No call execution config found for lead {lead.id}; skipping retry."
+ )Also applies to: 102-114
🤖 Prompt for AI Agents
In `@app/ai/voice/agents/breeze_buddy/managers/post_call_handlers.py` around lines
50 - 63, Currently the code returns early when get_lead_config(lead) is falsy,
which prevents update_lead_call_completion_details(...) from running and leaves
leads stuck in PROCESSING; remove the early return and always call
update_lead_call_completion_details(id=lead.id, status=LeadCallStatus.FINISHED,
outcome=outcome, meta_data=meta_data, call_end_time=call_end_time), then
conditionally call retry_call(lead, config, outcome) only if config is truthy
and outcome is in ["BUSY", "NO_ANSWER"]; apply the same change for both
occurrences surrounding get_lead_config, update_lead_call_completion_details,
and retry_call.
There was a problem hiding this comment.
If the lead is in the Processing state, we have a separate function to handle this case _handle_processing_timeout_leads(), so anyways the lead will be updated.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for the _handle_processing_timeout_leads function to understand the cleanup mechanism
rg -n "_handle_processing_timeout_leads" -A 10 -B 2
Length of output: 2356
🏁 Script executed:
#!/bin/bash
# Get the complete implementation of _handle_processing_timeout_leads
ast-grep --pattern $'async def _handle_processing_timeout_leads() {
$$$
}'
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Get more context around the _handle_processing_timeout_leads function
rg -n "_handle_processing_timeout_leads" -A 40 app/ai/voice/agents/breeze_buddy/managers/__init__.py | head -50
Length of output: 2055
🏁 Script executed:
#!/bin/bash
# Get the rest of the _handle_processing_timeout_leads function to see if it handles retries
rg -n "_handle_processing_timeout_leads" -A 60 app/ai/voice/agents/breeze_buddy/managers/__init__.py | head -70
Length of output: 2621
@Swetha-160303, thank you for the clarification! I can see that _handle_processing_timeout_leads() does indeed handle this scenario by cleaning up leads that have been in PROCESSING state for more than 10 minutes (lines 63-114 in __init__.py).
You're right that this provides a safety net. The tradeoff is that leads will remain in PROCESSING state for up to 10 minutes before being cleaned up and retried, whereas updating them immediately in handle_call_completion() and handle_call_no_answer() would avoid that delay.
Since you have a working cleanup mechanism in place, I'll leave the architectural decision to you. The timeout-based cleanup is a valid pattern for handling edge cases like missing configurations.
Swetha-160303
force-pushed
the
refactor/calls-manager-modularization
branch
from
b5c33d3 to
3db08d7
Compare
|
@CodeRabbit Do the review |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/api/routers/breeze_buddy/telephony/callbacks/__init__.py`:
- Around line 130-133: Update the status-handler docstring to reflect current
routing: change the Behavior section so that only "no-answer" triggers
handle_call_no_answer(), while "failed" and "busy" are documented as routed to
handle_call_completion(); keep the note that other statuses are logged and
acknowledged. Locate the docstring associated with the status handler
(referencing handle_call_no_answer and handle_call_completion in the string) and
edit the bullets to match this routing and current terminology.
♻️ Duplicate comments (1)
app/ai/voice/agents/breeze_buddy/managers/post_call_handlers.py (1)
50-52: Confirm early return on missing config is intentional.
Both handlers return before marking the lead FINISHED when config is missing, leaving the lead in PROCESSING until timeout cleanup. If that’s intended, all good; otherwise consider updating the lead before returning.Also applies to: 102-104
🧹 Nitpick comments (1)
app/ai/voice/agents/breeze_buddy/managers/recording.py (1)
85-93: Avoid blocking the event loop during GCS upload.
upload_file_to_gcsis synchronous; consider offloading it to a thread (or using an async variant) to keep this coroutine non‑blocking under load.♻️ Suggested change (thread offload)
+import asyncio @@ - gcs_url = upload_file_to_gcs( - file_obj=audio_file, - destination_path=f"breeze-buddy/recordings/{call_id}.{file_extension}", - content_type=content_type, - metadata={ - "call_id": call_id, - "original_url": provider_recording_url, - }, - ) + gcs_url = await asyncio.to_thread( + upload_file_to_gcs, + file_obj=audio_file, + destination_path=f"breeze-buddy/recordings/{call_id}.{file_extension}", + content_type=content_type, + metadata={ + "call_id": call_id, + "original_url": provider_recording_url, + }, + )
Swetha-160303
force-pushed
the
refactor/calls-manager-modularization
branch
from
3db08d7 to
eca0cb6
Compare
|
@CodeRabbit do the review |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/ai/voice/agents/breeze_buddy/managers/__init__.py`:
- Around line 98-103: The code calls get_outbound_number_by_id with
locked_lead.outbound_number_id without checking for None; guard the lookup by
first checking that locked_lead.outbound_number_id is not None/falsey before
calling get_outbound_number_by_id, and only call release_number when an
outbound_number was retrieved (existing code already checks outbound_number),
referencing the symbols locked_lead.outbound_number_id,
get_outbound_number_by_id, and release_number.
🧹 Nitpick comments (3)
app/ai/voice/agents/breeze_buddy/managers/webhook.py (1)
14-34: Consider API consistency for session handling.
send_failure_webhookaccepts an externalsessionparameter, whilesend_no_answer_webhookcreates its own session internally. This inconsistency may lead to confusion for callers.Consider aligning both functions to either accept a session or create one internally for consistency.
app/ai/voice/agents/breeze_buddy/managers/number.py (1)
131-157: Consider extracting channel validation logic.The Exotel channel capacity check (
channels is not None and maximum_channels is not None and channels < maximum_channels) is duplicated inget_available_number(lines 46-49, 77-80) and here (lines 148-151). Consider extracting this into a helper function likehas_available_capacity(number: OutboundNumber) -> boolto reduce duplication and centralize the logic.♻️ Example helper
def _has_available_capacity(number: OutboundNumber) -> bool: """Check if an Exotel number has available channel capacity.""" if number.provider != CallProvider.EXOTEL: return True return ( number.channels is not None and number.maximum_channels is not None and number.channels < number.maximum_channels )app/ai/voice/agents/breeze_buddy/managers/recording.py (1)
78-83: Consider explicit provider handling for clarity.Since unsupported providers return early (line 71), the
elseclause here safely assumes Exotel. However, for explicit clarity and future-proofing, consider usingelif provider == "exotel"with an explicit else that raises or logs an unexpected state.♻️ Optional: More explicit handling
if provider == "twilio": content_type = "audio/wav" file_extension = "wav" - else: # exotel + elif provider == "exotel": content_type = "audio/mp3" file_extension = "mp3" + else: + # Shouldn't reach here due to early return, but defensive + logger.warning(f"Unexpected provider in content type block: {provider}") + content_type = "audio/mpeg" + file_extension = "mp3"
| handle_unanswered_calls, | ||
| from app.ai.voice.agents.breeze_buddy.managers import ( | ||
| handle_call_completion, | ||
| handle_call_no_answer, |
There was a problem hiding this comment.
handle_call_no_answer -> handle_unanswered_call
| @@ -0,0 +1,114 @@ | |||
| """ | |||
There was a problem hiding this comment.
post_call_handlers.py -> call_completion.py
| async def get_retry_number( | ||
| retry_calling_provider: CallProvider, | ||
| ) -> Optional[OutboundNumber]: | ||
| """ |
There was a problem hiding this comment.
why do we need this function?
There was a problem hiding this comment.
This is for the provider fallback during retries. When a call fails we will try it with the different provider. This function will get the number from the alternate provider. Previously in calls.py it is inline, now segregated into a separate function
| await release_lock_on_lead_by_id(locked_lead.id) | ||
|
|
||
|
|
||
| async def _attempt_fallback_call( |
There was a problem hiding this comment.
break this function into multiple functions, its still so large
Swetha-160303
force-pushed
the
refactor/calls-manager-modularization
branch
3 times, most recently
from
51d7d42 to
17e100c
Compare
| if call_status in ("no-answer", "failed", "busy"): | ||
| logger.info(f"Call with SID {call_sid} failed with status: {call_status}") | ||
| await handle_unanswered_calls(call_sid) |
| - If call_status is "no-answer": | ||
| Triggers retry logic via handle_unanswered_call() | ||
| - If call_status is "busy" or "failed": | ||
| Routes to handle_call_completion() with appropriate outcome |
Swetha-160303
force-pushed
the
refactor/calls-manager-modularization
branch
4 times, most recently
from
b8ceb4f to
b2106c1
Compare
Swetha-160303
force-pushed
the
refactor/calls-manager-modularization
branch
from
b2106c1 to
6f661fb
Compare
Swetha-160303
force-pushed
the
refactor/calls-manager-modularization
branch
2 times, most recently
from
b103ed6 to
ca44c51
Compare
|
@coderabbitai please review |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@app/ai/voice/agents/breeze_buddy/managers/__init__.py`:
- Around line 299-314: When _acquire_retry_number returns falsy, the code
currently returns False without performing failure handling; update this branch
so it mirrors other failure paths by updating the lead's status and sending the
merchant webhook before returning. Concretely, in the block where
retry_number_to_use is falsy (after calling _acquire_retry_number), set the
locked_lead.status to the appropriate terminal state (e.g., FINISHED if you
intend to mark as failed) and invoke the same webhook/notification routine used
elsewhere in this module (use the identical logic used in other failure handlers
in this file), then persist the lead change and finally return False; reference
_acquire_retry_number, retry_number_to_use, locked_lead, and _execute_retry_call
when locating the code to modify.
In `@app/ai/voice/agents/breeze_buddy/managers/number.py`:
- Around line 102-129: The code currently ignores PLIVO in acquire_number() and
release_number(), causing acquisition failures or leaked capacity when
get_available_number() can return PLIVO; add explicit PLIVO handling or ensure
PLIVO is never returned by get_available_number(). Fix option A: treat PLIVO
like TWILIO by adding a provider branch in acquire_number() that calls
update_outbound_number_status(number.id, OutboundNumberStatus.IN_USE) and in
release_number() that sets status to OutboundNumberStatus.AVAILABLE (using the
existing update_outbound_number_status function). Option B: prevent PLIVO from
being selected by get_available_number() (ensure it never returns PLIVO and log
a warning) if PLIVO channel semantics aren’t supported; pick one and implement
consistently across get_available_number(), acquire_number(), and
release_number().
In `@app/ai/voice/agents/breeze_buddy/managers/webhook.py`:
- Around line 32-35: The FAILED branch calls send_failure_webhook(session, lead,
failure_reason) without ensuring session is present; mirror the NO_ANSWER
branch's behavior by creating a session when session is None before calling
send_failure_webhook. Locate the send_call_webhook handling of outcome ==
"FAILED" and add the same session initialization used in the outcome ==
"NO_ANSWER" path so send_failure_webhook always receives a valid session object.
🧹 Nitpick comments (1)
app/ai/voice/agents/breeze_buddy/managers/__init__.py (1)
54-59: Consider sorting__all__for consistency.Static analysis flags that
__all__is not sorted. This is a minor style improvement.♻️ Suggested sorted order
__all__ = [ - "initiate_calls", "handle_call_completion", "handle_unanswered_calls", + "initiate_calls", "update_call_recording", ]
| retry_number_to_use = await _acquire_retry_number( | ||
| retry_calling_provider, locked_lead | ||
| ) | ||
| if not retry_number_to_use: | ||
| return False | ||
|
|
||
| # Execute the retry call | ||
| return await _execute_retry_call( | ||
| session, | ||
| locked_lead, | ||
| retry_number_to_use, | ||
| retry_calling_provider, | ||
| config, | ||
| use_template_flow, | ||
| ) | ||
|
|
There was a problem hiding this comment.
Missing failure handling when retry number is unavailable.
When _acquire_retry_number returns None (lines 302-303), the function returns False without updating the lead status or sending a failure webhook. While the lead remains in BACKLOG and will be retried, the merchant receives no notification about this temporary failure.
Consider adding a status update and webhook for consistency with other failure paths.
🛠️ Suggested improvement
# Try to acquire a retry number
retry_number_to_use = await _acquire_retry_number(
retry_calling_provider, locked_lead
)
if not retry_number_to_use:
+ logger.warning(
+ f"No retry number available for lead {locked_lead.id}, will retry on next backlog run."
+ )
return FalseAlternatively, if this scenario should mark the lead as failed (rather than leaving it for retry), update the status to FINISHED and send a webhook similar to the other failure handlers.
🤖 Prompt for AI Agents
In `@app/ai/voice/agents/breeze_buddy/managers/__init__.py` around lines 299 -
314, When _acquire_retry_number returns falsy, the code currently returns False
without performing failure handling; update this branch so it mirrors other
failure paths by updating the lead's status and sending the merchant webhook
before returning. Concretely, in the block where retry_number_to_use is falsy
(after calling _acquire_retry_number), set the locked_lead.status to the
appropriate terminal state (e.g., FINISHED if you intend to mark as failed) and
invoke the same webhook/notification routine used elsewhere in this module (use
the identical logic used in other failure handlers in this file), then persist
the lead change and finally return False; reference _acquire_retry_number,
retry_number_to_use, locked_lead, and _execute_retry_call when locating the code
to modify.
| async def acquire_number(number: OutboundNumber) -> bool: | ||
| """ | ||
| Marks an outbound number as in use. | ||
| Uses atomic increment to avoid race conditions. | ||
| For Exotel, only succeeds if channels < maximum_channels. | ||
| Returns True if acquisition succeeded, False if at capacity. | ||
| """ | ||
| if number.provider == CallProvider.TWILIO: | ||
| result = await update_outbound_number_status( | ||
| number.id, OutboundNumberStatus.IN_USE | ||
| ) | ||
| return result is not None | ||
| elif number.provider == CallProvider.EXOTEL: | ||
| result = await increment_outbound_number_channels(number.id) | ||
| return result is not None | ||
| return False | ||
|
|
||
|
|
||
| async def release_number(number_id: str, provider: CallProvider): | ||
| """ | ||
| Releases an outbound number, making it available for other calls. | ||
| Uses atomic decrement to avoid race conditions. | ||
| """ | ||
| if provider == CallProvider.TWILIO: | ||
| await update_outbound_number_status(number_id, OutboundNumberStatus.AVAILABLE) | ||
| elif provider == CallProvider.EXOTEL: | ||
| await decrement_outbound_number_channels(number_id) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find PLIVO usage in Breeze Buddy and check CallProvider enum
echo "=== Searching for PLIVO in Breeze Buddy ==="
rg -n --type=py '\bPLIVO\b|CallProvider\.PLIVO' app/ai/voice/agents/breeze_buddy 2>/dev/null || echo "No direct PLIVO matches found"
echo -e "\n=== Locating call sites for number management functions ==="
rg -n --type=py 'get_available_number|acquire_number|release_number' app/ai/voice/agents/breeze_buddy 2>/dev/null
echo -e "\n=== Finding CallProvider enum definition ==="
rg -n --type=py 'class CallProvider|enum.*CallProvider' --max-count=5 2>/dev/null
echo -e "\n=== Checking where get_available_number is defined ==="
rg -n --type=py 'def get_available_number' 2>/dev/nullRepository: juspay/clairvoyance
Length of output: 2301
🏁 Script executed:
#!/bin/bash
# View get_available_number implementation
echo "=== get_available_number implementation ==="
sed -n '25,101p' app/ai/voice/agents/breeze_buddy/managers/number.py
echo -e "\n=== CallProvider enum ==="
cat app/schemas/breeze_buddy/core.py
echo -e "\n=== PLIVO usage in telephony/utils.py ==="
sed -n '15,35p' app/ai/voice/agents/breeze_buddy/services/telephony/utils.pyRepository: juspay/clairvoyance
Length of output: 7999
Add PLIVO handling or prevent PLIVO selection in number management.
get_available_number() can return PLIVO numbers—both in the new approach (template with outbound_number_id) and fallback (when calling_provider=PLIVO). However, acquire_number() returns False for PLIVO and release_number() is a no-op for PLIVO. This will cause call acquisition to fail or leave capacity untracked. Either add PLIVO support with channel tracking to acquire_number() and release_number(), or prevent PLIVO numbers from being selected by get_available_number() with a warning.
🤖 Prompt for AI Agents
In `@app/ai/voice/agents/breeze_buddy/managers/number.py` around lines 102 - 129,
The code currently ignores PLIVO in acquire_number() and release_number(),
causing acquisition failures or leaked capacity when get_available_number() can
return PLIVO; add explicit PLIVO handling or ensure PLIVO is never returned by
get_available_number(). Fix option A: treat PLIVO like TWILIO by adding a
provider branch in acquire_number() that calls
update_outbound_number_status(number.id, OutboundNumberStatus.IN_USE) and in
release_number() that sets status to OutboundNumberStatus.AVAILABLE (using the
existing update_outbound_number_status function). Option B: prevent PLIVO from
being selected by get_available_number() (ensure it never returns PLIVO and log
a warning) if PLIVO channel semantics aren’t supported; pick one and implement
consistently across get_available_number(), acquire_number(), and
release_number().
| if outcome == "FAILED": | ||
| await send_failure_webhook(session, lead, failure_reason) | ||
| elif outcome == "NO_ANSWER": | ||
| reporting_webhook_url = ( |
There was a problem hiding this comment.
Guard the FAILED path when session is None.
If send_call_webhook(..., outcome="FAILED") is called without a session, send_failure_webhook will receive None and fail at runtime. Create a session when missing (as you already do for NO_ANSWER).
🛠️ Proposed fix
- if outcome == "FAILED":
- await send_failure_webhook(session, lead, failure_reason)
+ if outcome == "FAILED":
+ if session is None:
+ async with create_aiohttp_session() as session:
+ await send_failure_webhook(session, lead, failure_reason)
+ else:
+ await send_failure_webhook(session, lead, failure_reason)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if outcome == "FAILED": | |
| await send_failure_webhook(session, lead, failure_reason) | |
| elif outcome == "NO_ANSWER": | |
| reporting_webhook_url = ( | |
| if outcome == "FAILED": | |
| if session is None: | |
| async with create_aiohttp_session() as session: | |
| await send_failure_webhook(session, lead, failure_reason) | |
| else: | |
| await send_failure_webhook(session, lead, failure_reason) | |
| elif outcome == "NO_ANSWER": | |
| reporting_webhook_url = ( |
🤖 Prompt for AI Agents
In `@app/ai/voice/agents/breeze_buddy/managers/webhook.py` around lines 32 - 35,
The FAILED branch calls send_failure_webhook(session, lead, failure_reason)
without ensuring session is present; mirror the NO_ANSWER branch's behavior by
creating a session when session is None before calling send_failure_webhook.
Locate the send_call_webhook handling of outcome == "FAILED" and add the same
session initialization used in the outcome == "NO_ANSWER" path so
send_failure_webhook always receives a valid session object.
ca44c51 to
7eb4a65
Compare
Swetha-160303
force-pushed
the
refactor/calls-manager-modularization
branch
from
7eb4a65 to
b3e4ac7
Compare
There was a problem hiding this comment.
Pull request overview
This pull request refactors the monolithic calls.py module (840+ lines) into a well-organized set of smaller, focused modules for better maintainability and separation of concerns. The original single-file implementation has been split into 7 specialized modules: number.py, configuration.py, retry.py, webhook.py, call_completion.py, recording.py, and utils.py, with the main orchestration logic remaining in __init__.py.
Changes:
- Refactored monolithic call manager into separate modules by responsibility (number management, retry logic, webhooks, recording, etc.)
- Renamed
process_backlog_leads()toinitiate_calls()for clarity - Enhanced webhook payload with
isLastAttemptfield for NO_ANSWER outcomes - Improved Plivo provider handling to include channel capacity checks (similar to Exotel)
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| app/ai/voice/agents/breeze_buddy/managers/init.py | Main entry point with call initiation orchestration and backward-compatible exports |
| app/ai/voice/agents/breeze_buddy/managers/number.py | Outbound number acquisition, release, and multi-provider fallback logic |
| app/ai/voice/agents/breeze_buddy/managers/configuration.py | Lead configuration retrieval utilities |
| app/ai/voice/agents/breeze_buddy/managers/retry.py | Call retry scheduling and webhook notifications |
| app/ai/voice/agents/breeze_buddy/managers/webhook.py | Webhook delivery for call outcomes (FAILED, NO_ANSWER) |
| app/ai/voice/agents/breeze_buddy/managers/call_completion.py | Call completion and unanswered call event handlers |
| app/ai/voice/agents/breeze_buddy/managers/recording.py | Call recording download and cloud storage upload |
| app/ai/voice/agents/breeze_buddy/managers/utils.py | Utility functions for calling hours validation and greeting preparation |
| app/api/routers/breeze_buddy/websocket.py | Updated import to use new module structure |
| app/api/routers/breeze_buddy/telephony/callbacks/handlers.py | Updated import to use new module structure |
| app/api/routers/breeze_buddy/cron.py | Updated to call renamed initiate_calls() function |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| call_provider = get_voice_provider( | ||
| config.calling_provider.value, | ||
| session, | ||
| use_template_flow, | ||
| ) | ||
| call = call_provider.make_call( | ||
| locked_lead.payload.get("customer_mobile_number"), | ||
| number_to_use.number, | ||
| ) | ||
|
|
||
| if call and call.get("sid"): | ||
| await update_lead_call_details( | ||
| locked_lead.id, | ||
| LeadCallStatus.PROCESSING, | ||
| call.get("sid"), | ||
| datetime.now(timezone.utc), | ||
| number_to_use.id, | ||
| ) | ||
| else: | ||
| # Handle call initiation failure with retry logic | ||
| await _attempt_fallback_call( | ||
| session, | ||
| locked_lead, | ||
| number_to_use, | ||
| config, | ||
| template, | ||
| use_template_flow, | ||
| ) | ||
|
|
There was a problem hiding this comment.
Potential resource leak if exception occurs after acquiring number. If an exception is raised between lines 399-437 (after successfully acquiring a number but before releasing it via _attempt_fallback_call or normal flow), the number will remain locked because there's no try-finally block to ensure cleanup. Consider wrapping the code after number acquisition in a try-finally block that releases the number on exception, similar to how the lead lock is handled.
| call_provider = get_voice_provider( | |
| config.calling_provider.value, | |
| session, | |
| use_template_flow, | |
| ) | |
| call = call_provider.make_call( | |
| locked_lead.payload.get("customer_mobile_number"), | |
| number_to_use.number, | |
| ) | |
| if call and call.get("sid"): | |
| await update_lead_call_details( | |
| locked_lead.id, | |
| LeadCallStatus.PROCESSING, | |
| call.get("sid"), | |
| datetime.now(timezone.utc), | |
| number_to_use.id, | |
| ) | |
| else: | |
| # Handle call initiation failure with retry logic | |
| await _attempt_fallback_call( | |
| session, | |
| locked_lead, | |
| number_to_use, | |
| config, | |
| template, | |
| use_template_flow, | |
| ) | |
| try: | |
| call_provider = get_voice_provider( | |
| config.calling_provider.value, | |
| session, | |
| use_template_flow, | |
| ) | |
| call = call_provider.make_call( | |
| locked_lead.payload.get("customer_mobile_number"), | |
| number_to_use.number, | |
| ) | |
| if call and call.get("sid"): | |
| await update_lead_call_details( | |
| locked_lead.id, | |
| LeadCallStatus.PROCESSING, | |
| call.get("sid"), | |
| datetime.now(timezone.utc), | |
| number_to_use.id, | |
| ) | |
| else: | |
| # Handle call initiation failure with retry logic | |
| await _attempt_fallback_call( | |
| session, | |
| locked_lead, | |
| number_to_use, | |
| config, | |
| template, | |
| use_template_flow, | |
| ) | |
| except Exception as call_err: | |
| # Ensure the acquired number is released if an error occurs after acquisition | |
| try: | |
| await release_number(number_to_use.id) | |
| except Exception as release_err: | |
| logger.error( | |
| f"Failed to release number {getattr(number_to_use, 'id', 'unknown')} " | |
| f"after exception: {release_err}" | |
| ) | |
| raise call_err |
| "attemptCount": lead.attempt_count + 1, | ||
| "callDuration": call_duration, | ||
| "orderId": lead.payload.get("order_id") if lead.payload else None, | ||
| "isLastAttempt": is_last_attempt, |
There was a problem hiding this comment.
New field 'isLastAttempt' added to NO_ANSWER webhook payload. The webhook now includes an 'isLastAttempt' boolean field (line 58) that was not present in the original implementation. While this is likely an enhancement, it changes the webhook payload structure which could affect downstream consumers that have strict schema validation. This should be documented in the PR description or release notes as an API change.
| "outcome": "NO_ANSWER", | ||
| "attemptCount": lead.attempt_count + 1, | ||
| "callDuration": call_duration, | ||
| "orderId": lead.payload.get("order_id") if lead.payload else None, |
There was a problem hiding this comment.
Inconsistent orderId field in NO_ANSWER webhook. The old code used lead.request_id for the orderId field in the webhook payload, but the new code uses lead.payload.get("order_id"). This changes the behavior and may break downstream webhook consumers expecting the request_id. The field should be consistent with the original implementation using lead.request_id.
| "orderId": lead.payload.get("order_id") if lead.payload else None, | |
| "orderId": lead.request_id, |
| call_provider = get_voice_provider( | ||
| config.calling_provider.value, | ||
| session, | ||
| use_template_flow, | ||
| ) | ||
| call = call_provider.make_call( | ||
| locked_lead.payload.get("customer_mobile_number"), |
There was a problem hiding this comment.
Missing validation for customer_mobile_number before initiating the call. The old code validated that customer_mobile_number exists and is a string before making the call. Without this validation, the code could pass None or non-string values to make_call, leading to potential errors. This validation should be restored before line 414 to ensure the number is properly released and the lead is unlocked if validation fails.
| call_provider = get_voice_provider( | |
| config.calling_provider.value, | |
| session, | |
| use_template_flow, | |
| ) | |
| call = call_provider.make_call( | |
| locked_lead.payload.get("customer_mobile_number"), | |
| customer_mobile_number = locked_lead.payload.get("customer_mobile_number") | |
| if not isinstance(customer_mobile_number, str) or not customer_mobile_number.strip(): | |
| logger.warning( | |
| f"Invalid or missing customer_mobile_number for lead {locked_lead.id}: " | |
| f"{customer_mobile_number!r}. Releasing acquired number and skipping call." | |
| ) | |
| try: | |
| await release_number(number_to_use.id) | |
| except Exception as release_err: | |
| logger.error( | |
| f"Failed to release number {number_to_use.id} after invalid customer_mobile_number " | |
| f"for lead {locked_lead.id}: {release_err}" | |
| ) | |
| # Reset number_to_use as it has been released and must not be used | |
| number_to_use = None | |
| continue | |
| call_provider = get_voice_provider( | |
| config.calling_provider.value, | |
| session, | |
| use_template_flow, | |
| ) | |
| call = call_provider.make_call( | |
| customer_mobile_number, |
| ) | ||
|
|
||
| retry_call_result = retry_call_provider.make_call( | ||
| locked_lead.payload.get("customer_mobile_number"), |
There was a problem hiding this comment.
Missing None check for locked_lead.payload before accessing it. At line 234, the code calls locked_lead.payload.get("customer_mobile_number") without checking if payload is None first. If payload is None, this will raise an AttributeError. The code should use (locked_lead.payload or {}).get(...) or validate that payload is not None before accessing it.
| locked_lead.payload.get("customer_mobile_number"), | |
| (locked_lead.payload or {}).get("customer_mobile_number"), |
| if outcome in ["BUSY", "NO_ANSWER"]: | ||
| await retry_call(lead, config, outcome) |
There was a problem hiding this comment.
Missing call_direction check before retry. The old code checked "if outcome in ['BUSY', 'NO_ANSWER'] and lead.call_direction == CallDirection.OUTBOUND" before scheduling a retry, ensuring only outbound calls are retried. The refactored code unconditionally retries all calls with BUSY or NO_ANSWER outcomes, which means inbound calls will now be incorrectly retried. This check needs to be restored.
| retry_call_result = retry_call_provider.make_call( | ||
| locked_lead.payload.get("customer_mobile_number"), | ||
| retry_number_to_use.number, | ||
| ) |
There was a problem hiding this comment.
Missing validation for customer_mobile_number in retry call execution. The old code validated that customer_mobile_number exists and is a string before making retry calls. This validation is missing in the new code at line 234, which could lead to passing invalid values to make_call. The validation should check if the payload contains a valid customer_mobile_number and handle the error appropriately if not.
| await prepare_and_store_initial_greeting( | ||
| lead_id=locked_lead.id, | ||
| payload=locked_lead.payload, | ||
| template=template, | ||
| ) |
There was a problem hiding this comment.
Potential None payload issue in prepare_and_store_initial_greeting call. At line 391, locked_lead.payload is passed directly to prepare_and_store_initial_greeting without checking if it's None. While the function handles None payloads internally, the old code passed (locked_lead.payload or {}) which ensured a dict was always provided. This inconsistency could lead to unexpected behavior if the function is modified or other parts of the code rely on receiving a dict.
| use_template_flow, | ||
| ) | ||
| call = call_provider.make_call( | ||
| locked_lead.payload.get("customer_mobile_number"), |
There was a problem hiding this comment.
Missing None check for locked_lead.payload before accessing it. At line 415, the code calls locked_lead.payload.get("customer_mobile_number") without checking if payload is None first. If payload is None, this will raise an AttributeError. The code should use (locked_lead.payload or {}).get(...) or validate that payload is not None before accessing it.
| locked_lead.payload.get("customer_mobile_number"), | |
| (locked_lead.payload or {}).get("customer_mobile_number"), |
| Args: | ||
| call_id: The call SID | ||
| provider_recording_url: The URL of the recording from the provider | ||
| provider: The provider name ('twilio' or 'exotel') |
There was a problem hiding this comment.
Incomplete docstring for update_call_recording function. The docstring at line 38 states "provider: The provider name ('twilio' or 'exotel')" but the function should also support 'plivo' provider (which needs to be added back to the implementation). The docstring should be updated to reflect all supported providers.
Swetha-160303
force-pushed
the
refactor/calls-manager-modularization
branch
2 times, most recently
from
f85f366 to
c8d0cbf
Compare
Swetha-160303
force-pushed
the
refactor/calls-manager-modularization
branch
from
c8d0cbf to
9db7e07
Compare
Swetha-160303
force-pushed
the
refactor/calls-manager-modularization
branch
from
9db7e07 to
b8f0d21
Compare
Swetha-160303
force-pushed
the
refactor/calls-manager-modularization
branch
from
c200585 to
cc7f2ed
Compare
Swetha-160303
force-pushed
the
refactor/calls-manager-modularization
branch
from
cc7f2ed to
a7ec254
Compare
Summary by CodeRabbit
Release Notes
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.