Add force re-upload from cache for expired assets

[lourou]

Add force re-upload from cache for expired assets

Adds manual re-upload capability for expired assets from the debug view:

• Enhanced ExpiredAssetRecoveryHandler with optional cache recovery support
• Added forceReuploadAssetFromCache to SessionManagerProtocol
• Implemented in SessionManager with proper writer initialization
• Wired through debug view hierarchy with context menu action

Fix asset reupload compile regressions and start assessment log

Assess renewal accounting and document asset renewal findings

Fix renewal timestamp accounting using explicit renewed keys

Preserve avatar renewal timestamps during conversation sync

Defer expired asset recovery from startup via queued foreground processing

Add tests for deferred asset recovery queue and handler behavior

Update assessment log with deferred recovery implementation status

Update recovery handler test cache stub for latest image cache protocol

Harden deferred asset recovery flow with capped retries and serialized processing

Expand deferred recovery tests for queue semantics and defer result

Align app settings and debug asset view with project style conventions

Add group image recovery-path coverage for expired asset handler

Note

Recover expired assets from cache in ExpiredAssetRecoveryHandler with deferred processing

• ExpiredAssetRecoveryHandler.handleExpiredAsset now attempts to recover assets from cache using MyProfileWriter or ConversationMetadataWriter, defers processing when writers are unavailable via DeferredAssetRecoveryQueue, and only clears URLs as a last resort
• SessionManager processes deferred assets in background on app foreground with retry logic (max 3 attempts), and exposes forceReuploadAssetFromCache to manually trigger cache recovery for a specific asset
• AssetRenewalResult includes renewedKeys field to track which keys were successfully renewed, fixing timestamp updates to only apply to explicitly renewed assets
• ConversationWriter.store preserves avatarLastRenewed when avatar URLs remain unchanged during conversation sync
• Debug UI adds "Force Re-upload from Cache" action in DebugAssetRenewalView.swift
• Behavioral Change: expired assets are now recovered from cache when possible instead of being cleared immediately

📊 Macroscope summarized f83030c. 13 files reviewed, 8 issues evaluated, 7 issues filtered, 0 comments posted

🗂️ Filtered Issues

Convos/Debug View/DebugAssetRenewalView.swift — 0 comments posted, 1 evaluated, 1 filtered

• [line 164](https://github.com/xmtplabs/convos-ios/blob/f83030c7ac3c26672253bbc57bb7a3cc9e698f80/Convos/Debug View/DebugAssetRenewalView.swift#L164): The "Force Re-upload from Cache" button's .disabled(isReuploading) modifier doesn't account for when isRenewing is true, and similarly the renew button doesn't check isReuploading. This allows both operations to run concurrently on the same asset, potentially causing race conditions or inconsistent state when both renewSingleAsset() and forceReuploadFromCache() execute simultaneously. [ Low confidence ]

ConvosCore/Sources/ConvosCore/Assets/AssetRenewalManager.swift — 0 comments posted, 1 evaluated, 1 filtered

• line 122: In performRenewal, the returned AssetRenewalResult at line 122 does not include accumulated renewedKeys. While allExpiredKeys is properly accumulated across batches, there is no corresponding allRenewedKeys accumulation. The result uses the default empty array for renewedKeys, meaning callers of forceRenewal() will always receive an empty renewedKeys array even when assets were successfully renewed. This is inconsistent with how expiredKeys is handled and could break callers that depend on knowing which keys were renewed. [ Out of scope ]

ConvosCore/Sources/ConvosCore/Assets/ExpiredAssetRecoveryHandler.swift — 0 comments posted, 1 evaluated, 1 filtered

• line 152: The clearAssetUrl method swallows database write errors silently (lines 152-154), but handleExpiredAsset returns .cleared regardless of whether the clear operation succeeded (lines 37-38, 43-44). This could mislead callers into believing the asset URL was cleared when the database write actually failed. [ Out of scope ]

ConvosCore/Sources/ConvosCore/Inboxes/MockInboxesService.swift — 0 comments posted, 1 evaluated, 1 filtered

• line 138: In makeAssetRenewalManager, the ExpiredAssetRecoveryHandler is created with imageCache but without myProfileWriter, conversationMetadataWriter, or onRecoveryDeferred. If attemptRecoveryFromCache finds a cached image but no writer is available, it returns .deferred. Since onRecoveryDeferred is nil, handleExpiredAsset will then clear the asset URL even though the cached image exists and recovery might have been possible with proper writers configured. This could result in unnecessary asset loss in mock scenarios. [ Low confidence ]

ConvosCore/Sources/ConvosCore/Sessions/SessionManager.swift — 0 comments posted, 4 evaluated, 3 filtered

• line 569: In processDeferredAssetRecoveries, when forceReuploadAssetFromCache returns false because the recovery handler inside it already returned .cleared (meaning the asset URL was already cleared in the database), the code at lines 569-570 creates another ExpiredAssetRecoveryHandler and calls handleExpiredAsset again. This results in redundant database operations attempting to clear an already-cleared asset URL. While not a crash, this wastes database writes. [ Low confidence ]
• line 617: In forceReuploadAssetFromCache for .profileAvatar assets (line 617-624), the code uses assetInboxId from the RenewableAsset tuple to obtain the messaging service. If the RenewableAsset.profileAvatar was created for another user's cached avatar (not the current user's profile), this would attempt to get a service for an inbox that doesn't belong to us. The call to messagingService(for:inboxId:) would fail, causing retries until exhaustion and eventual clearing of a URL that may have been legitimate. This depends on whether AssetRenewalURLCollector only collects the current user's avatars. [ Low confidence ]
• line 652: The ExpiredAssetRecoveryHandler created in forceReuploadAssetFromCache at lines 652-657 does not include an onRecoveryDeferred callback. If for any reason the recovery returns .deferred (though currently the writers are provided so this shouldn't happen), the handler will fall through to clearing the asset with a warning log rather than properly deferring it for later processing. [ Low confidence ]

[lourou]

• Add force re-upload from cache for expired assets #530 👈 (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.

[claude]

Claude finished @lourou's task —— View job

---

Code Review Complete ✅

I've completed the code review of PR #530. Here are my findings organized by category:

---

Critical Issues 🔴

1. Race Condition in Debug UI (DebugAssetRenewalView.swift:156, 164)

The "Renew Asset" and "Force Re-upload from Cache" buttons can run concurrently on the same asset. The renew button checks isRenewing || isReuploading || asset.key == nil, but the reupload button only checks isRenewing || isReuploading. Both operations modify the same database records and could cause inconsistent state.

Recommendation:

// Line 156 and 164 - ensure both operations are mutually exclusive
.disabled(isRenewing || isReuploading || asset.key == nil)

2. Missing renewedKeys Accumulation (AssetRenewalManager.swift:122)

In performRenewal(), the method accumulates allExpiredKeys across batches (line 97) but returns AssetRenewalResult with an empty renewedKeys array. Line 90 declares allRenewedKeys and line 98 populates it, but it's not passed to the result at line 124.

Recommendation:

// Line 124 - include accumulated renewedKeys
return AssetRenewalResult(renewed: totalRenewed, failed: totalFailed, expiredKeys: allExpiredKeys, renewedKeys: allRenewedKeys)

---

Medium Issues 🟡

3. Deferred Recovery Falls Through to Clear (SessionManager.swift:577-582)

When forceReuploadAssetFromCache returns false (line 577), the code creates a "clear-only" recovery handler and immediately clears the asset. This happens even if the asset was already cleared or deferred. The redundant clear operation wastes database writes.

Recommendation: Check the actual recovery result before creating a second handler. If forceReuploadAssetFromCache returned false due to .cleared, skip the redundant operation.

4. Silent Clear Failures (ExpiredAssetRecoveryHandler.swift:46-49)

The clearExpiredAsset method returns .clearFailed if the database write fails (line 152), but handleExpiredAsset returns this to callers without logging the failure at the call site. Callers in SessionManager.processDeferredAssetRecoveries don't distinguish between .cleared and .clearFailed.

Recommendation: Log clear failures explicitly in handleExpiredAsset or ensure callers check for .clearFailed.

5. Missing onRecoveryDeferred in Manual Recovery (SessionManager.swift:664-673)

The ExpiredAssetRecoveryHandler created in forceReuploadAssetFromCache (line 664) doesn't include an onRecoveryDeferred callback. If recovery returns .deferred (unlikely with writers provided), it will fall through to clearing the asset with a warning rather than re-queuing.

Recommendation: Either add the callback or add a comment explaining why deferred is impossible in this context.

---

Low Priority Issues 🟢

6. Profile Avatar Recovery for Other Users (SessionManager.swift:617-636)

For .profileAvatar assets, the code uses assetInboxId to look up the messaging service (line 636). If the RenewableAsset represents another user's cached avatar (not the current user's), this would fail. Review AssetRenewalURLCollector to confirm it only collects the current user's avatars.

7. MockInboxesService Missing Writers (MockInboxesService.swift:138)

The mock creates ExpiredAssetRecoveryHandler with imageCache but without myProfileWriter or conversationMetadataWriter. If a test triggers cache recovery, it will defer and then clear the asset even though the cached image exists. Consider providing mock writers for more realistic test behavior.

---

Positive Observations ✅

1. Excellent test coverage: Three new test files cover deferred queue semantics, recovery handler behavior, and timestamp preservation logic. The tests are well-structured and use realistic scenarios.

2. Robust retry logic: The deferred recovery queue implements capped retries (max 3) with serialized processing, preventing infinite loops.

3. Proper timestamp preservation: ConversationWriter.preservingAvatarLastRenewed correctly preserves renewal timestamps when avatar URLs remain unchanged during conversation sync.

4. Good separation of concerns: DeferredAssetRecoveryQueue actor provides thread-safe queue management with clear semantics (deduplication, processing lock).

5. API client correctly extracts renewedKeys: Lines 531-533 of ConvosAPIClient.swift properly filter successful results to build the renewedKeys array.

---

Performance Considerations ⚡

• Batch processing: The 100-item batch size for asset renewal (AssetRenewalManager.swift:6) is appropriate for API limits.
• Serial processing: Deferred recovery processes items serially (SessionManager.swift:567), which prevents database contention but may be slow for large queues. Consider parallel processing with a concurrency limit if this becomes a bottleneck.

---

Security Considerations 🔒

No security concerns identified. The implementation:

• Uses proper authentication for API calls
• Doesn't expose sensitive data in logs
• Handles expired assets safely by clearing URLs

---

Test Coverage Assessment 📊

Strengths:

• Core recovery logic tested (deferred, recovered, cleared states)
• Queue semantics thoroughly tested (deduplication, processing lifecycle)
• Timestamp preservation edge cases covered

Gaps:

• No tests for SessionManager.processDeferredAssetRecoveries retry logic
• No tests for race conditions in concurrent recovery attempts
• Group image recovery path less tested than profile avatar recovery

---

Recommendations Summary

1. Fix Critical Issue Contact Card Creation UI #2 (missing renewedKeys) - this breaks the renewal result contract
2. Fix Critical Issue [Screen] Onboarding #1 (race condition) - protect against concurrent operations
3. Add explicit logging or handling for .clearFailed cases
4. Consider adding integration tests for the full deferred recovery flow
5. Document why onRecoveryDeferred is omitted in manual recovery (or add it)

Overall, this is a well-architected feature with good test coverage. The issues identified are mostly edge cases and minor bugs that should be straightforward to fix.

---