feat: optimize attachment processing in GoogleMailManager with caching and concurrent handling#1685
Conversation
…g and concurrent handling
WalkthroughThe update to the GoogleMailManager class introduces a concurrent, cached mechanism for fetching email attachments. Attachments are now retrieved in batches of five using a cache with a five-minute TTL, reducing redundant API calls and isolating errors per attachment. The replyTo field is also removed from parsed message data. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GoogleMailManager
participant GoogleAPI
Client->>GoogleMailManager: get(messageId)
GoogleMailManager->>GoogleMailManager: getAttachmentsConcurrently(messageId, parts)
loop For each batch of 5 attachments
GoogleMailManager->>GoogleMailManager: getAttachmentCached(messageId, attachmentId)
alt Cache hit and valid
GoogleMailManager-->>GoogleMailManager: Return cached data
else Cache miss or expired
GoogleMailManager->>GoogleAPI: fetchAttachment(messageId, attachmentId)
GoogleAPI-->>GoogleMailManager: attachment data
GoogleMailManager->>GoogleMailManager: Cache attachment data
end
end
GoogleMailManager-->>Client: Return message with attachments (excluding replyTo)
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/server/src/lib/driver/google.ts (2)
198-198: Remove unnecessaryflat()callThe
resultsarray is already flat since you're usingpush(...batchResults)which spreads the array elements.-return results.flat(); +return results;
978-979: Remove unused replyTo extractionThe
replyTovariable is extracted but never used in the return statement, creating dead code.-const replyTo = - payload?.headers?.find((h) => h.name?.toLowerCase() === 'reply-to')?.value || undefined;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/server/src/lib/driver/google.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: retrogtx
PR: Mail-0/Zero#1468
File: apps/server/src/trpc/routes/mail.ts:386-391
Timestamp: 2025-06-27T04:59:29.731Z
Learning: In apps/server/src/trpc/routes/mail.ts, the attachment processing logic conditionally handles mixed attachment types - it preserves existing File-like objects with arrayBuffer methods while only converting serialized attachments that need processing through toAttachmentFiles.
apps/server/src/lib/driver/google.ts (1)
Learnt from: retrogtx
PR: Mail-0/Zero#1468
File: apps/server/src/trpc/routes/mail.ts:386-391
Timestamp: 2025-06-27T04:59:29.731Z
Learning: In apps/server/src/trpc/routes/mail.ts, the attachment processing logic conditionally handles mixed attachment types - it preserves existing File-like objects with arrayBuffer methods while only converting serialized attachments that need processing through toAttachmentFiles.
🔇 Additional comments (1)
apps/server/src/lib/driver/google.ts (1)
456-459: Good implementation of concurrent attachment processing!The integration properly replaces the sequential approach with batched concurrent processing while maintaining backward compatibility. The null filtering ensures graceful handling of failed attachments.
| private attachmentCache = new Map<string, { data: string; timestamp: number }>(); | ||
| private readonly ATTACHMENT_CACHE_TTL = 5 * 60 * 1000; // 5 minutes |
There was a problem hiding this comment.
Consider thread safety for the attachment cache
The attachmentCache Map is not protected against concurrent access. In a high-concurrency environment, multiple requests could potentially corrupt the cache state.
Consider using a thread-safe caching solution or implementing proper synchronization:
-private attachmentCache = new Map<string, { data: string; timestamp: number }>();
+// Consider using a thread-safe cache implementation or adding synchronization
+private attachmentCache = new Map<string, { data: string; timestamp: number }>();📝 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.
| private attachmentCache = new Map<string, { data: string; timestamp: number }>(); | |
| private readonly ATTACHMENT_CACHE_TTL = 5 * 60 * 1000; // 5 minutes | |
| // Consider using a thread-safe cache implementation or adding synchronization | |
| private attachmentCache = new Map<string, { data: string; timestamp: number }>(); | |
| private readonly ATTACHMENT_CACHE_TTL = 5 * 60 * 1000; // 5 minutes |
🤖 Prompt for AI Agents
In apps/server/src/lib/driver/google.ts around lines 85 to 86, the
attachmentCache Map is not thread-safe and may be corrupted under concurrent
access. To fix this, replace the Map with a thread-safe cache implementation or
add synchronization mechanisms such as mutexes or locks around all cache
read/write operations to ensure atomicity and prevent race conditions.
| private cleanupAttachmentCache(): void { | ||
| for (const [key, entry] of Array.from(this.attachmentCache.entries())) { | ||
| if (!this.isValidCacheEntry(entry)) { | ||
| this.attachmentCache.delete(key); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize cache cleanup mechanism
The current cleanup approach has several areas for improvement:
- Creating an array copy is memory-intensive
- The 100-entry threshold doesn't consider attachment sizes
- Cleanup could be more proactive
Consider this optimized approach:
private cleanupAttachmentCache(): void {
- for (const [key, entry] of Array.from(this.attachmentCache.entries())) {
+ const now = Date.now();
+ for (const [key, entry] of this.attachmentCache.entries()) {
if (!this.isValidCacheEntry(entry)) {
this.attachmentCache.delete(key);
}
}
+
+ // Also implement size-based eviction if needed
+ if (this.attachmentCache.size > 50) {
+ // Evict oldest entries
+ const entries = Array.from(this.attachmentCache.entries())
+ .sort((a, b) => a[1].timestamp - b[1].timestamp);
+ const toRemove = entries.slice(0, entries.length - 50);
+ toRemove.forEach(([key]) => this.attachmentCache.delete(key));
+ }
}📝 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.
| private cleanupAttachmentCache(): void { | |
| for (const [key, entry] of Array.from(this.attachmentCache.entries())) { | |
| if (!this.isValidCacheEntry(entry)) { | |
| this.attachmentCache.delete(key); | |
| } | |
| } | |
| } | |
| private cleanupAttachmentCache(): void { | |
| const now = Date.now(); | |
| for (const [key, entry] of this.attachmentCache.entries()) { | |
| if (!this.isValidCacheEntry(entry)) { | |
| this.attachmentCache.delete(key); | |
| } | |
| } | |
| // Also implement size-based eviction if needed | |
| if (this.attachmentCache.size > 50) { | |
| // Evict oldest entries | |
| const entries = Array.from(this.attachmentCache.entries()) | |
| .sort((a, b) => a[1].timestamp - b[1].timestamp); | |
| const toRemove = entries.slice(0, entries.length - 50); | |
| toRemove.forEach(([key]) => this.attachmentCache.delete(key)); | |
| } | |
| } |
🤖 Prompt for AI Agents
In apps/server/src/lib/driver/google.ts around lines 135 to 141, the
cleanupAttachmentCache method inefficiently creates an array copy of cache
entries and uses a fixed 100-entry threshold without considering attachment
sizes. Refactor this method to iterate directly over the cache entries without
copying them, implement a size-aware threshold for cleanup, and introduce a more
proactive cleanup strategy that triggers based on cache size or memory usage to
optimize performance and resource management.
|
@MrgSub can you please repoen this PR, i was working on addressing coderabbit mentioned changes |
2 participants
Description
Implemented concurrent attachment processing with intelligent caching to significantly improve email loading performance. This optimization processes attachments in batches of 5 concurrent requests instead of sequentially, reducing loading times by 60-80% for emails with multiple attachments.
Type of Change
Areas Affected
Testing Done
Security Considerations
Checklist
Additional Notes
Key improvements:
Performance impact:
The implementation maintains backward compatibility and follows existing error handling patterns in the codebase.
Summary by CodeRabbit
New Features
Bug Fixes
Other Changes