Persistent photo cache for chat attachments

[yewreeka]

Add design for persistent photo cache for chat attachments in persistent-photo-cache.md

Introduce a planning document detailing a two-tier disk storage model, ImageCache API changes with ImageStorageTier, new cache/remove methods accepting a storage tier, and cleanup behavior on conversation delete/explode in persistent-photo-cache.md.

📍Where to Start

Start with the design overview in persistent-photo-cache.md.

^{Macroscope summarized dbbbfe7.}

[claude]

Claude finished @yewreeka's task —— View job

---

Review of PR #510: Persistent Photo Cache Design

Tasks

• Read the design document
• Review design quality and best practices
• Identify potential issues
• Evaluate performance considerations
• Assess security concerns
• Consider test coverage requirements
• Provide comprehensive feedback

---

Overall Assessment

This is a well-designed solution that addresses the core problem elegantly. The two-tier storage model is the right approach, and the design document is clear and comprehensive. Below are specific observations organized by review criteria.

---

1. Design Quality & Best Practices

Strengths:

• Clear separation of concerns: The two-tier model (persistent vs cache) cleanly separates re-fetchable images from non-recoverable chat attachments
• Minimal API surface changes: Using an enum parameter (ImageStorageTier) is a clean extension that maintains backward compatibility with .cache as default
• Appropriate use of iOS storage directories: Application Support for persistent data and Caches for evictable data follows iOS conventions
• Well-scoped: Explicitly states what's in/out of scope, avoiding scope creep
• Good integration points: Leverages existing cleanupInboxData flow rather than inventing new cleanup mechanisms

Suggestions:

• Consider protocol extension for default tier: The document mentions "Existing methods default to .cache tier" but doesn't show how. Consider explicitly defining this in the API section:

  extension ImageCacheProtocol {
      func cacheImage(_ image: ImageType, for identifier: String) {
          cacheImage(image, for: identifier, storageTier: .cache)
      }
  }

---

2. Potential Issues

Critical Issues:

1. Missing attachment key collection before message deletion

In InboxStateMachine.swift:878, messages are deleted directly:

try DBMessage.filter(DBMessage.Columns.conversationId == conversationId).deleteAll(db)

The design document proposes collecting attachment keys before deletion at line 72-78, but the current cleanupInboxData doesn't do this. This is a critical implementation detail that must be added, or photos will become orphaned on disk.

Recommendation: The implementation PR must modify cleanupInboxData to:

1. Query attachment keys before deleting messages
2. Call the new removeAttachments method
3. Then delete the message rows

2. Potential race condition during "Delete All Data"

The design states: "The app's 'Delete All Data' flow already calls cleanupInboxData for each inbox" (line 82). However, if multiple inboxes are cleaned up concurrently, and they share the same ImageCache instance, there could be concurrent file deletions in the PhotoStore directory.

Recommendation: Ensure removeAttachments uses the existing diskCacheQueue serial queue (like other disk operations in ImageCache.swift) to serialize file I/O.

3. Identifier scheme not fully specified

Line 35 states: "The identifier is the attachment key (either the local URL string for outgoing, or the stored remote attachment JSON hash for incoming)."

This is vague. The implementation needs a concrete identifier scheme:

• Outgoing: localURL.absoluteString? Just the path component?
• Incoming: SHA256 of the remote attachment JSON? Which fields are included in the hash?

Recommendation: Add a subsection defining the exact identifier format, or reference where this is already defined in the codebase (e.g., in RemoteAttachmentLoader or OutgoingMessageWriter).

Moderate Issues:

4. No size limits on persistent storage

Line 98 acknowledges this risk: "Persistent photos accumulate without a size cap." The mitigation relies on "cleanup on delete/explode" and the 30-day S3 lifecycle window.

Concern: Power users with many conversations could accumulate hundreds of MB of photos over time. If they rarely delete conversations, this could grow unbounded.

Recommendation: Consider adding observability (e.g., log total persistent storage size periodically) or a future enhancement for a soft cap with user notification when exceeded.

5. Missing migration strategy for edge case

Line 91 states: "Migration of existing cached attachments (photos have not shipped to the App Store yet...)"

However, internal builds already have photos in the evictable cache pool (Caches/ImageCache/). When the new implementation starts writing to PhotoStore/, old photos will remain in the evictable cache and could be purged by iOS.

Recommendation: Add a one-time migration in ImageCache.init() that:

• Checks for a migration flag (e.g., UserDefaults key)
• If not migrated, scans for attachment identifiers in Caches/ImageCache/
• Moves them to PhotoStore/
• Sets the migration flag

Alternatively, explicitly accept this as tech debt and document it in the "Out of scope" section.

---

3. Performance Considerations

Strengths:

• Disk I/O is already serialized: The existing diskCacheQueue in ImageCache.swift:213 handles disk operations on a background queue, so adding persistent storage won't block the main thread
• No network implications: The design only affects local storage, not network fetching
• Reuses existing compression: No additional image processing overhead

Concern:

• Cleanup query performance: The proposed query at line 72-78 collects all attachment keys for a conversation:

  let attachmentKeys = try DBMessage
      .filter(DBMessage.Columns.conversationId == conversationId)
      .filter(DBMessage.Columns.attachmentKey != nil)
      .fetchAll(db)
      .compactMap { $0.attachmentKey }

  For conversations with thousands of messages (e.g., group chats), this could fetch many rows just to extract the attachmentKey column.

Recommendation: Use a more efficient query that projects only the attachmentKey column:

let attachmentKeys = try String
    .fetchAll(db, sql: """
        SELECT attachmentKey FROM DBMessage 
        WHERE conversationId = ? AND attachmentKey IS NOT NULL
    """, arguments: [conversationId])

Or use GRDB's select method:

let attachmentKeys = try DBMessage
    .select(DBMessage.Columns.attachmentKey)
    .filter(DBMessage.Columns.conversationId == conversationId)
    .filter(DBMessage.Columns.attachmentKey != nil)
    .fetchAll(db)

---

4. Security Concerns

Strengths:

• Proper use of Application Support: This directory is backed up by the system (iCloud backups), which is appropriate for user data
• No new encryption requirements: Relies on iOS file-level encryption
• Follows existing patterns: Doesn't introduce new security vectors beyond the existing ImageCache

Concerns:

1. Backup implications

Application Support is backed up to iCloud by default. If users have iCloud Backup enabled, all persistent photos will be backed up. This could:

• Consume significant iCloud storage for users with many photos
• Expose photos to Apple's cloud infrastructure (not end-to-end encrypted unless Advanced Data Protection is enabled)

Recommendation: Consider marking the PhotoStore directory as excluded from backups:

var resourceValues = URLResourceValues()
resourceValues.isExcludedFromBackup = true
try photoStoreURL.setResourceValues(resourceValues)

Document this decision in the design (either way).

2. No explicit file permissions

The design doesn't mention file permissions for the persistent storage directory.

Recommendation: Ensure the implementation sets appropriate file protection (e.g., .completeUntilFirstUserAuthentication) when creating the directory, consistent with sensitive user data.

---

5. Test Coverage Requirements

The design document doesn't include a testing strategy. For implementation, I recommend:

Unit Tests:

• ImageCache with two storage tiers (cache vs persistent)
• LRU eviction only affects .cache tier, not .persistent
• removeAttachments(forConversationId:attachmentKeys:) removes correct files
• Default tier behavior (backward compatibility)

Integration Tests:

• cleanupInboxData collects attachment keys and calls removeAttachments
• Photos are cleaned up when conversations are deleted/exploded
• "Delete All Data" flow cleans up all persistent photos

Filesystem Tests:

• Verify files are written to correct directories:
  • .cache → Caches/ImageCache/
  • .persistent → Application Support/PhotoStore/
• Verify cleanup removes orphaned files

---

6. Additional Observations

Documentation:

• The document references ADR 008 and ADR 009, which provide good context
• Consider adding a sequence diagram showing the flow of an attachment from upload → cache → cleanup

API Naming:

• removeAttachments(forConversationId:attachmentKeys:) is clear, but forConversationId is only used for logging. Consider simplifying to:

  func removeImages(for identifiers: [String], storageTier: ImageStorageTier)

  This is more generic and already proposed in the design at line 55.

Future Enhancements:

• The design mentions "Re-upload recovery for expired assets" (line 92). Consider adding a note about how persistent storage would enable this feature.

---

Summary

This is a solid, well-thought-out design that solves the problem with minimal API changes and good separation of concerns. The main action items for implementation are:

1. Critical: Modify cleanupInboxData to collect attachment keys before message deletion
2. Critical: Define the exact attachment key identifier scheme
3. Important: Optimize the attachment key collection query for large conversations
4. Important: Decide on iCloud backup policy and set isExcludedFromBackup accordingly
5. Important: Add comprehensive test coverage for the two-tier storage model
6. Nice-to-have: Consider migration for existing cached attachments in internal builds

Overall, I'd approve this design with the understanding that the critical issues above will be addressed during implementation. Great work! 🚀

---

[yewreeka]

• Persistent photo cache: two-tier disk storage for chat attachments #511
• Persistent photo cache for chat attachments #510 👈 (View in Graphite)
• dev

---

How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

[graphite-app]

Merge activity

• Feb 24, 9:54 PM UTC: yewreeka added this pull request to the Graphite merge queue.
• Feb 24, 9:57 PM UTC: Merged by the Graphite merge queue.