Job Management: Preserve llm_call details#823
Open
Prajna1999 wants to merge 6 commits into
Open
Conversation
📝 WalkthroughWalkthroughThis PR implements two-phase LLM call lifecycle (pending → resolved) with audio URL support. The schema is loosened to allow nullable fields, audio inputs can now be URLs or base64, and job execution defers provider/model determination. Audio is downloaded from URLs, uploaded to S3, and served via presigned URLs. ChangesAudio URL Support and Call Lifecycle
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Prajna1999
changed the title
vprashrex
approved these changes
May 12, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
backend/app/core/storage_utils.py (1)
211-222: ⚡ Quick winAdd m4a MIME aliases to extension mapping.
audio/m4a(and common variantaudio/x-m4a) currently falls back towav, which can produce wrong file extensions.Suggested mapping update
_MIME_TO_EXT: dict[str, str] = { @@ "audio/mp4": "mp4", + "audio/m4a": "m4a", + "audio/x-m4a": "m4a", "audio/aac": "aac", "audio/flac": "flac", }🤖 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 `@backend/app/core/storage_utils.py` around lines 211 - 222, The _MIME_TO_EXT mapping in storage_utils.py is missing aliases for m4a mime types so audio/m4a and audio/x-m4a fall back incorrectly; update the _MIME_TO_EXT dict to include entries mapping "audio/m4a" and "audio/x-m4a" to "m4a" (alongside the existing audio/* entries) so functions that rely on _MIME_TO_EXT return the correct .m4a extension.
🤖 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 `@backend/app/alembic/versions/058_make_llm_call_fields_nullable.py`:
- Around line 25-28: The downgrade() changes try to set llm_call.model,
llm_call.provider and llm_call.input_type to nullable=False which will fail if
any rows contain NULL; modify downgrade() to first backfill NULL values for
these columns (or abort with a clear error) before calling op.alter_column, e.g.
run UPDATE statements against the "llm_call" table to set sensible defaults for
model, provider and input_type or raise a RuntimeError if NULLs exist, then call
op.alter_column("llm_call", "model", nullable=False),
op.alter_column("llm_call", "provider", nullable=False) and
op.alter_column("llm_call", "input_type", nullable=False).
In `@backend/app/crud/llm.py`:
- Around line 284-291: get_llm_call_by_job_id is non-deterministic because it
uses .first() without filtering or ordering, which can return the wrong
standalone LlmCall when multiple rows exist; update the query in
get_llm_call_by_job_id to restrict to the intended active/pending row (e.g., add
a predicate such as LlmCall.status == "pending" or LlmCall.is_active) and add a
deterministic ORDER BY (for example LlmCall.created_at.desc() or
LlmCall.id.desc()) before taking the first result so updates always target the
correct record.
In `@backend/app/services/llm/chain/executor.py`:
- Line 73: The _resolve_presigned_url function's parameter is untyped; update
the signature to include a concrete type (e.g., change def
_resolve_presigned_url(self, output) -> None: to def
_resolve_presigned_url(self, output: dict[str, Any]) -> None:) and import Any
from typing at the top of the module; if a more specific type exists in your
codebase (e.g., a Response or Output model), use that type instead of dict[str,
Any] and keep the return as -> None.
In `@backend/app/utils.py`:
- Around line 595-607: The download_audio_bytes function is vulnerable to SSRF
and memory exhaustion; harden it by validating the input URL and streaming with
size caps: ensure the scheme is http/https, resolve the hostname to an IP and
reject private/loopback/metadata or other disallowed ranges (re-check after
redirects), enforce and check Content-Length against a configured
MAX_AUDIO_BYTES before allocating, use requests.get(stream=True) with short
timeouts and iterate response.iter_content(chunk_size) accumulating up to the
max and aborting if exceeded, and return clear error messages on validation
failures, redirect-caused host changes, timeouts, or oversized payloads; keep
all checks and limits inside download_audio_bytes and reference it when
implementing these changes.
---
Nitpick comments:
In `@backend/app/core/storage_utils.py`:
- Around line 211-222: The _MIME_TO_EXT mapping in storage_utils.py is missing
aliases for m4a mime types so audio/m4a and audio/x-m4a fall back incorrectly;
update the _MIME_TO_EXT dict to include entries mapping "audio/m4a" and
"audio/x-m4a" to "m4a" (alongside the existing audio/* entries) so functions
that rely on _MIME_TO_EXT return the correct .m4a extension.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aca2da0e-ba34-4944-b3f4-9eac2b41990d
📒 Files selected for processing (8)
backend/app/alembic/versions/058_make_llm_call_fields_nullable.pybackend/app/core/storage_utils.pybackend/app/crud/llm.pybackend/app/crud/llm_chain.pybackend/app/models/llm/request.pybackend/app/services/llm/chain/executor.pybackend/app/services/llm/jobs.pybackend/app/utils.py
Comment on lines
+25
to
+28
| def downgrade() -> None: | ||
| op.alter_column("llm_call", "model", nullable=False) | ||
| op.alter_column("llm_call", "provider", nullable=False) | ||
| op.alter_column("llm_call", "input_type", nullable=False) |
There was a problem hiding this comment.
Downgrade can fail once NULL rows exist.
Setting these columns back to nullable=False without backfilling will fail on rollback if any row has NULL in input_type, provider, or model.
Suggested migration hardening
def downgrade() -> None:
+ op.execute(
+ """
+ UPDATE llm_call
+ SET input_type = COALESCE(input_type, 'text'),
+ provider = COALESCE(provider, 'unknown'),
+ model = COALESCE(model, 'unknown')
+ """
+ )
op.alter_column("llm_call", "model", nullable=False)
op.alter_column("llm_call", "provider", nullable=False)
op.alter_column("llm_call", "input_type", nullable=False)🤖 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 `@backend/app/alembic/versions/058_make_llm_call_fields_nullable.py` around
lines 25 - 28, The downgrade() changes try to set llm_call.model,
llm_call.provider and llm_call.input_type to nullable=False which will fail if
any rows contain NULL; modify downgrade() to first backfill NULL values for
these columns (or abort with a clear error) before calling op.alter_column, e.g.
run UPDATE statements against the "llm_call" table to set sensible defaults for
model, provider and input_type or raise a RuntimeError if NULLs exist, then call
op.alter_column("llm_call", "model", nullable=False),
op.alter_column("llm_call", "provider", nullable=False) and
op.alter_column("llm_call", "input_type", nullable=False).
Comment on lines
+284
to
+291
| def get_llm_call_by_job_id(session: Session, job_id: UUID) -> LlmCall | None: | ||
| """Return the single active LlmCall for a standalone job (no chain_id).""" | ||
| statement = select(LlmCall).where( | ||
| LlmCall.job_id == job_id, | ||
| LlmCall.chain_id.is_(None), | ||
| LlmCall.deleted_at.is_(None), | ||
| ) | ||
| return session.exec(statement).first() |
There was a problem hiding this comment.
Make get_llm_call_by_job_id deterministic for pending-row updates.
Using .first() without ordering/pending filters can pick the wrong row when multiple standalone calls exist for the same job, causing updates to land on an incorrect record.
Suggested query tightening
def get_llm_call_by_job_id(session: Session, job_id: UUID) -> LlmCall | None:
"""Return the single active LlmCall for a standalone job (no chain_id)."""
statement = select(LlmCall).where(
LlmCall.job_id == job_id,
LlmCall.chain_id.is_(None),
LlmCall.deleted_at.is_(None),
+ LlmCall.provider.is_(None),
+ LlmCall.model.is_(None),
+ LlmCall.content.is_(None),
)
- return session.exec(statement).first()
+ statement = statement.order_by(LlmCall.created_at.desc())
+ return session.exec(statement).first()🤖 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 `@backend/app/crud/llm.py` around lines 284 - 291, get_llm_call_by_job_id is
non-deterministic because it uses .first() without filtering or ordering, which
can return the wrong standalone LlmCall when multiple rows exist; update the
query in get_llm_call_by_job_id to restrict to the intended active/pending row
(e.g., add a predicate such as LlmCall.status == "pending" or LlmCall.is_active)
and add a deterministic ORDER BY (for example LlmCall.created_at.desc() or
LlmCall.id.desc()) before taking the first result so updates always target the
correct record.
| self._context.project_id, self._context.organization_id | ||
| ) | ||
|
|
||
| def _resolve_presigned_url(self, output) -> None: |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add a type hint for _resolve_presigned_url parameter.
output is currently untyped; this breaks the project’s typing rule and weakens static checks.
As per coding guidelines: “Always add type hints to all function parameters and return values in Python code”.
🤖 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 `@backend/app/services/llm/chain/executor.py` at line 73, The
_resolve_presigned_url function's parameter is untyped; update the signature to
include a concrete type (e.g., change def _resolve_presigned_url(self, output)
-> None: to def _resolve_presigned_url(self, output: dict[str, Any]) -> None:)
and import Any from typing at the top of the module; if a more specific type
exists in your codebase (e.g., a Response or Output model), use that type
instead of dict[str, Any] and keep the return as -> None.
Comment on lines
+595
to
+607
| def download_audio_bytes(url: str) -> tuple[bytes | None, str | None]: | ||
| """Download audio from a public URL. Returns (bytes, error).""" | ||
| try: | ||
| response = requests.get(url, timeout=30) | ||
| response.raise_for_status() | ||
| return response.content, None | ||
| except requests.exceptions.Timeout: | ||
| return None, f"Timed out downloading audio from URL: {url}" | ||
| except requests.exceptions.HTTPError as e: | ||
| return None, f"HTTP {e.response.status_code} downloading audio from URL: {url}" | ||
| except Exception as e: | ||
| return None, f"Failed to download audio from URL: {str(e)}" | ||
|
|
There was a problem hiding this comment.
Harden URL audio download against SSRF and oversized payloads.
This directly fetches user-controlled URLs without host/IP validation, and reads unbounded content into memory. That is exploitable for SSRF and resource exhaustion.
Suggested hardening pattern
def download_audio_bytes(url: str) -> tuple[bytes | None, str | None]:
"""Download audio from a public URL. Returns (bytes, error)."""
try:
- response = requests.get(url, timeout=30)
+ validate_callback_url(url) # or dedicated outbound-url validator for media fetch
+ with requests.Session() as session:
+ session.trust_env = False
+ response = session.get(
+ url,
+ timeout=(5, 30),
+ allow_redirects=False,
+ stream=True,
+ )
response.raise_for_status()
- return response.content, None
+ max_bytes = 25 * 1024 * 1024
+ chunks: list[bytes] = []
+ total = 0
+ for chunk in response.iter_content(chunk_size=64 * 1024):
+ if not chunk:
+ continue
+ total += len(chunk)
+ if total > max_bytes:
+ return None, "Audio file exceeds maximum allowed size"
+ chunks.append(chunk)
+ return b"".join(chunks), None🤖 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 `@backend/app/utils.py` around lines 595 - 607, The download_audio_bytes
function is vulnerable to SSRF and memory exhaustion; harden it by validating
the input URL and streaming with size caps: ensure the scheme is http/https,
resolve the hostname to an IP and reject private/loopback/metadata or other
disallowed ranges (re-check after redirects), enforce and check Content-Length
against a configured MAX_AUDIO_BYTES before allocating, use
requests.get(stream=True) with short timeouts and iterate
response.iter_content(chunk_size) accumulating up to the max and aborting if
exceeded, and return clear error messages on validation failures,
redirect-caused host changes, timeouts, or oversized payloads; keep all checks
and limits inside download_audio_bytes and reference it when implementing these
changes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Target issue is #819
Explain the motivation for making this change. What existing problem does the pull request solve?
Checklist
Before submitting a pull request, please ensure that you mark these task.
fastapi run --reload app/main.pyordocker compose upin the repository root and test.Notes
Please add here if any other information is required for the reviewer.
Summary by CodeRabbit
New Features
Improvements