Add Files API support for image attachments #1462
Conversation
|
https://platform.openai.com/docs/api-reference/files should be similar in concept to the anthropic process implemented |
| URL: part.ImageURL.URL, | ||
| })) | ||
| // Use the image converter which handles file refs, data URLs, and HTTP URLs | ||
| if imgBlock := convertImagePart(context.Background(), nil, part.ImageURL); imgBlock != nil { |
There was a problem hiding this comment.
We should use a real context here so that we can cancel the operation
Signed-off-by: Jean-Laurent de Morlhon <jeanlaurent@morlhon.net>
jeanlaurent
force-pushed
the
attach-fileid
branch
from
e1fb4a4 to
db0ff8b
Compare
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
Found 4 potential issues in the Files API implementation:
- Context cancellation handling - Files may be uploaded but not cached if context is canceled
- Silent image drops - Images silently skipped when using FileID with standard Anthropic API
- Resource leaks - Uploaded files to Gemini are never cleaned up
- Missing cleanup mechanism - No TTL or deletion for cached file uploads
The core functionality looks solid, but these issues could cause resource leaks and unexpected behavior.
| // Try to upload via Files API | ||
| if client != nil { | ||
| fileID, err := uploadFileToAnthropic(ctx, client, localPath) | ||
| if err == nil { |
There was a problem hiding this comment.
Race condition: File uploaded but not cached if context canceled
If the context is canceled after uploadFileToAnthropic() succeeds but before the cache write completes, the file will exist on Anthropic's servers but won't be in the local cache. This causes:
- Resource leak on Anthropic's servers
- Duplicate uploads on retry (cache miss)
Consider checking ctx.Err() after upload and still caching the result:
fileID, err := uploadFileToAnthropic(ctx, client, localPath)
if err == nil {
// Always cache successful uploads, even if context canceled
fileUploadCache.Lock()
fileUploadCache.cache[localPath] = fileID
fileUploadCache.Unlock()
if ctx.Err() != nil {
slog.Debug("Context canceled after upload, but file was cached", "file_id", fileID)
}
...
}| // The standard API will fall back to base64 | ||
| slog.Debug("Using existing file ID", "file_id", fileRef.FileID) | ||
| // Standard API doesn't support file IDs, so we need to fall back | ||
| // For now, log a warning and return nil (the beta converter handles this) |
There was a problem hiding this comment.
Silent image drop when FileID used with standard API
When a FileReference with SourceType = FileSourceTypeFileID is encountered, the function logs a warning and returns nil. This silently drops the image from the message without any error to the caller.
This could happen when:
- Deserializing messages created for beta API but used with standard API
- Cross-provider message reuse
Consider returning an error or attempting to fall back to the original file if LocalPath is still available in the FileReference.
| fileURI, err := uploadFileToGemini(ctx, client, localPath, mimeType) | ||
| if err == nil { | ||
| // Cache the file URI | ||
| fileUploadCache.Lock() |
There was a problem hiding this comment.
Race condition: File uploaded but not cached if context canceled
Same issue as Anthropic - if context is canceled after uploadFileToGemini() succeeds but before cache write completes, the file will be uploaded to Gemini's servers but not cached locally. This leads to:
- Resource leak on Gemini's servers
- Duplicate uploads on retry
Consider checking ctx.Err() and still caching successful uploads to prevent resource leaks.
| } | ||
|
|
||
| file, err := client.Files.UploadFromPath(ctx, localPath, config) | ||
| if err != nil { |
There was a problem hiding this comment.
Resource leak: Uploaded files never deleted from Gemini's servers
The uploadFileToGemini() function uploads files to Gemini's Files API and caches the URI, but there's no cleanup mechanism. Over time, this will accumulate potentially large amounts of data on Gemini's servers.
The cache is never cleared, so even old/unused files remain uploaded indefinitely.
Consider implementing:
- TTL-based cleanup for cache entries
- Explicit file deletion when files are no longer referenced
- Cache eviction policy (LRU, size-based, etc.)
Gemini's Files API likely has quotas that could be exceeded without cleanup.
|
not sure how relevant the review is right now, but i figured we should test this our bot as much as possible to make it good |
3 participants
Add Files API support for image attachments
Images attached via /attach now use provider-specific Files APIs instead of
embedding base64 data in every message.
Changes:
• Store local file paths in messages instead of immediate base64 conversion
• Anthropic: Upload via Files API (beta), reference by file_id , with caching to
avoid re-uploads
• Gemini: Upload via Files API, reference by URI, with caching
• OpenAI/Bedrock: Convert to base64 at send time (no Files API for chat vision)
Benefits:
• Significantly reduces token usage for Anthropic (file IDs vs ~3000+ tokens per
image)
• Fixes "prompt too long" errors when attaching large images
• Each provider handles images optimally for its API