Skip to content

fix(sync): retry failed screenshot fetches instead of caching forever#23

Merged
edward-ly merged 1 commit into
masterfrom
fix/retry-failed-screenshot-fetches
Jul 21, 2026
Merged

fix(sync): retry failed screenshot fetches instead of caching forever#23
edward-ly merged 1 commit into
masterfrom
fix/retry-failed-screenshot-fetches

Conversation

@miaulalala

Copy link
Copy Markdown
Contributor

Summary

  • Investigated App Store usercontent proxy returns "File not found" for valid CoBudget screenshots appstore#1843 (CoBudget screenshots showing "File not found" through the proxy while working fine directly on GitHub).
  • Found that sync.php writes the failure placeholder (generateWarningImage()) into the exact same cache path that file_exists($cacheUrl) uses to decide "already handled, skip forever" — so any transient fetch failure (timeout, temporary origin error, unrecognized mime type) becomes permanent and is never retried.
  • Also found that a single uncaught error while processing one screenshot (e.g. mime_content_type() returning false, which then fails str_starts_with()'s type check) aborts the entire sync run, leaving every app later in the list unsynced until the same bug is hit again next run.
  • This PR adds a cache-failed/ marker directory that tracks failed URLs with a 6h backoff, so failures get retried instead of being stuck forever, and wraps per-screenshot handling in handleApps() so one bad entry can't kill the whole run.

Test plan

  • php -l sync.php passes
  • Manually exercised the extracted handleScreenshot() logic against a real failing URL: confirmed a fail marker is created on failure, a retry within the backoff window is a no-op, and a retry after the backoff window elapses re-attempts the fetch
  • Confirmed a subsequent successful fetch clears the fail marker and replaces the cached placeholder with the real image
  • Would appreciate a maintainer sanity-check against the production cache layout / cron invocation before merge

🤖 Generated with Claude Code

Previously, a failed screenshot fetch (timeout, transient origin error,
unrecognized mime type, etc.) wrote a warning placeholder directly into
the same cache path that gates re-fetching, so any failure became
permanent - the URL was never retried again. A single uncaught error
per screenshot (e.g. mime_content_type() returning false) could also
abort the entire sync run, leaving everything later in the apps list
unsynced.

This adds a separate cache-failed/ marker per URL with a backoff
window, so failed fetches are retried periodically instead of being
stuck forever, and wraps per-screenshot handling so one bad entry
can't kill the whole run.

Assisted-by: claude:claude-sonnet-5
Signed-off-by: Anna Larch <anna@nextcloud.com>

@edward-ly edward-ly left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@edward-ly
edward-ly merged commit c70bd33 into master Jul 21, 2026
1 check passed
@edward-ly
edward-ly deleted the fix/retry-failed-screenshot-fetches branch July 21, 2026 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants