fix(client): harden artifacts.get and follow Modal 303 long requests - #201
fix(client): harden artifacts.get and follow Modal 303 long requests#201dineshreddy91 wants to merge 1 commit into
Conversation
Strip optional extensions from artifact IDs, stop asserting a single image Content-Type, and poll Location after a 303 so slow Orion completions do not fail in the OpenAI-compatible client. Co-authored-by: Cursor <cursoragent@cursor.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements support for Modal's long-request polling flow (handling 303 See Other redirects) during chat completions, adds a dedicated helper module for polling, and refactors artifact retrieval to normalize object IDs and handle content types more robustly. The review feedback identifies several key issues: a runtime error from directly instantiating Pydantic v2's AnyHttpUrl, potential MissingSchema exceptions from relative redirect URLs, an infinite loop risk on empty 200/201 responses, Python 3.8 incompatibility with asyncio.to_thread, case-sensitive header lookups, and a duplicated file extension bug when caching URL artifacts.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| return Image.open(io.BytesIO(response)).convert("RGB") | ||
| elif obj_type == "url": | ||
| # Get the filename including extension frm the URL by stripping any query parameters | ||
| url: AnyHttpUrl = AnyHttpUrl(response.decode("utf-8")) |
There was a problem hiding this comment.
In Pydantic v2, AnyHttpUrl is a type alias and cannot be instantiated directly like a class (e.g., AnyHttpUrl(...)). This will raise a TypeError at runtime. Use TypeAdapter to validate and instantiate it properly.
| url: AnyHttpUrl = AnyHttpUrl(response.decode("utf-8")) | |
| from pydantic import TypeAdapter | |
| url = TypeAdapter(AnyHttpUrl).validate_python(response.decode("utf-8")) |
| location = extract_location(_headers_from_exc(exc)) | ||
| if not location: | ||
| raise exc |
There was a problem hiding this comment.
Relative redirect URLs in the Location header will cause requests to raise a MissingSchema exception. To prevent this, resolve the Location URL against the original request or response URL.
location = extract_location(_headers_from_exc(exc))
if not location:
raise exc
response = getattr(exc, "response", None)
if response is not None:
request = getattr(response, "request", None)
base_url = getattr(request, "url", None) or getattr(response, "url", None)
if base_url:
from urllib.parse import urljoin
location = urljoin(str(base_url), location)| response.raise_for_status() | ||
| time.sleep(poll_interval) |
There was a problem hiding this comment.
If the server returns a 200 or 201 status code with an empty body, it is neither a result status nor a pending status. Without an explicit check, response.raise_for_status() will not raise an error, causing the client to loop indefinitely. Raise a ValueError to prevent this hang.
| response.raise_for_status() | |
| time.sleep(poll_interval) | |
| if status in (200, 201): | |
| raise ValueError(f"Received empty response body with status {status}") | |
| response.raise_for_status() | |
| time.sleep(poll_interval) |
| return await asyncio.to_thread( | ||
| _follow_long_request_303, | ||
| exc, | ||
| api_key=api_key, | ||
| timeout=max(float(timeout or 0), _DEFAULT_LONG_REQUEST_TIMEOUT), | ||
| ) |
There was a problem hiding this comment.
asyncio.to_thread was introduced in Python 3.9. To maintain compatibility with Python 3.8, use loop.run_in_executor with functools.partial instead.
| return await asyncio.to_thread( | |
| _follow_long_request_303, | |
| exc, | |
| api_key=api_key, | |
| timeout=max(float(timeout or 0), _DEFAULT_LONG_REQUEST_TIMEOUT), | |
| ) | |
| loop = asyncio.get_running_loop() | |
| return await loop.run_in_executor( | |
| None, | |
| functools.partial( | |
| _follow_long_request_303, | |
| exc, | |
| api_key=api_key, | |
| timeout=max(float(timeout or 0), _DEFAULT_LONG_REQUEST_TIMEOUT), | |
| ), | |
| ) |
| raw = headers.get("Content-Type") or headers.get("content-type") or "" | ||
| return raw.split(";", 1)[0].strip().lower() |
There was a problem hiding this comment.
Since headers is converted to a standard dict in APIRequestor.request, it is no longer case-insensitive. A case-insensitive lookup using a generator expression is more robust to handle any casing of the Content-Type header.
| raw = headers.get("Content-Type") or headers.get("content-type") or "" | |
| return raw.split(";", 1)[0].strip().lower() | |
| raw = next((v for k, v in headers.items() if k.lower() == "content-type"), "") | |
| return raw.split(";", 1)[0].strip().lower() |
| url_filename: str = path.name.split("?")[0] | ||
| ext: str = url_filename.split(".")[-1].lower() | ||
| tmp_path: Path = artifacts_dir / f"{url_filename}.{ext}" |
There was a problem hiding this comment.
Extracting the extension from url_filename and then appending it again results in a duplicated file extension (e.g., image.jpg.jpg). Since url_filename already contains the extension, you can use it directly.
| url_filename: str = path.name.split("?")[0] | |
| ext: str = url_filename.split(".")[-1].lower() | |
| tmp_path: Path = artifacts_dir / f"{url_filename}.{ext}" | |
| url_filename: str = path.name.split("?")[0] | |
| tmp_path: Path = artifacts_dir / url_filename |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| class LongRequestTimeoutError(TimeoutError): | ||
| """Raised when polling a long-request Location URL times out.""" |
There was a problem hiding this comment.
🟡 Custom timeout error bypasses the project's exception hierarchy, so callers catching SDK errors will miss it
The new timeout error class inherits from Python's builtin TimeoutError (LongRequestTimeoutError(TimeoutError) at vlmrun/client/long_request.py:26) instead of the project's RequestTimeoutError or VLMRunError, so user code that catches the documented SDK exception types will not catch this error.
Impact: Users relying on except VLMRunError or except RequestTimeoutError will get an unhandled exception when a long-running agent request times out.
Exception hierarchy violation and missing suggestion field
The AGENTS.md rules state:
- "Use the existing exception hierarchy in
vlmrun/client/exceptions.py. Do not raise genericExceptionorValueErrorfor API-related errors." - "Every custom exception should include a helpful
suggestionfield guiding the user toward resolution." - "Preserve the pattern:
VLMRunError→APIError/ClientError→ specific error types."
The project already has RequestTimeoutError at vlmrun/client/exceptions.py:154 which inherits from APIError → VLMRunError and includes a suggestion field. LongRequestTimeoutError should either inherit from RequestTimeoutError or at minimum from VLMRunError/ClientError, and should include a suggestion field.
The error is raised at vlmrun/client/long_request.py:87 and propagates through _follow_long_request_303 (vlmrun/client/agent.py:95-100) to the user.
Prompt for agents
The LongRequestTimeoutError class at vlmrun/client/long_request.py:26 inherits from Python's builtin TimeoutError. Per the project's AGENTS.md rules, it should use the existing exception hierarchy from vlmrun/client/exceptions.py. It should either inherit from RequestTimeoutError (which already exists for timeout scenarios) or at minimum from ClientError/VLMRunError, and should include a suggestion field. You'll need to import the appropriate base class from vlmrun.client.exceptions and add a suggestion field like 'Increase the timeout or check if the agent job is still running'.
Was this helpful? React with 👍 or 👎 to provide feedback.
| def poll_location( | ||
| location: str, | ||
| *, | ||
| headers: Optional[Mapping[str, str]] = None, |
There was a problem hiding this comment.
🟡 New file uses legacy Optional type syntax instead of the mandated pipe-union style
Type annotations use Optional[X] (Optional[Mapping[str, str]] at vlmrun/client/long_request.py:54) instead of the required X | None syntax in a brand-new file that already has from __future__ import annotations.
Impact: Inconsistency with the codebase style rules; no runtime effect but violates mandatory coding standards.
Multiple Optional usages in the new long_request.py file
AGENTS.md states: "Use X | None instead of Optional[X] for type hints (PEP 604)."
Violations in the new file vlmrun/client/long_request.py:
- Line 54:
headers: Optional[Mapping[str, str]] = None→ should beMapping[str, str] | None = None - Line 58:
session: Optional[requests.Session] = None→ should berequests.Session | None = None - Line 131:
-> Optional[str]:→ should be-> str | None:
The file already has from __future__ import annotations at line 14, so the | syntax works without issue.
| headers: Optional[Mapping[str, str]] = None, | |
| headers: Mapping[str, str] | None = None, | |
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary
img_abc123.jpg→img_abc123)image/*andapplication/octet-stream303 See Otheron chat completions and pollLocationuntil200/201with a body204/ transient5xx/ connection holds as pending while pollingTest plan
pytest tests/test_artifacts.py(16 tests)artifacts.get(object_id="img_xxxx.jpg", …)succeeds when API returnsapplication/octet-streamagent.completions.createthat returns 303 resolves via Location polling instead of raisingMade with Cursor