Skip to content

fix: guard Appwrite source reportSites/reportMessaging against source failures#206

Merged
abnegate merged 4 commits into
mainfrom
fix/appwrite-source-report-crash
Jul 8, 2026
Merged

fix: guard Appwrite source reportSites/reportMessaging against source failures#206
abnegate merged 4 commits into
mainfrom
fix/appwrite-source-report-crash

Conversation

@HarshMN2345

Copy link
Copy Markdown
Member

Summary

  • reportSites() and reportMessaging() in Sources\Appwrite had no error handling around their API calls, unlike sibling methods (reportDomains, reportProjects) which already degrade to a 0 count when the source can't serve that resource.
  • If the source project fails to serve Sites or Messaging data (missing API key scope, or an older self-hosted version that predates the feature), the exception propagated uncaught through report(), aborting the entire pre-flight report before any resource was counted or processed — even resources unrelated to Sites/Messaging.
  • This surfaced as a generic "Migration failed due to an unexpected error" (500) with empty resourceId/statusCounters/resourceData, reported by a user migrating a self-hosted 1.9.5 Appwrite project to Cloud via the Console, where the resource list defaulted to including site/site-deployment/site-variable.

Fix

Wrapped each resource-type branch in reportSites() and reportMessaging() in try/catch (\Throwable), defaulting the count to 0 on failure — the same pattern already used in reportDomains() and reportProjects().

Test plan

  • Reproduced the original crash with a local stub server returning 404 on /v1/sites and 401 on /v1/messaging/*, confirmed reportSites()/reportMessaging() threw uncaught before the fix.
  • Re-ran the same reproduction against the patched methods — both now return gracefully with the affected resource counted as 0, and report() no longer aborts.
  • php -l and vendor/bin/pint --test pass on the changed file.

… failures

reportSites() and reportMessaging() had no error handling around their
API calls, unlike sibling methods (reportDomains, reportProjects) which
already degrade to a 0 count on failure. If the source project can't
serve Sites or Messaging (missing scope, older version without the
feature), the exception propagated uncaught through report(), aborting
the entire pre-flight report before any resource was processed.
@greptile-apps

greptile-apps Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR wraps the unguarded API calls in reportSites() and reportMessaging() in try/catch blocks to prevent a crash when a self-hosted Appwrite instance doesn't support Sites or Messaging. It also removes the @throws AppwriteException PHPDoc tag from reportMessaging and narrows the pre-existing catches in reportDomains()/reportProjects() to only swallow 5xx errors.

  • The catch blocks gate on $e->getCode() >= 500 && $e->getCode() < 600, but the PR description identifies the real failure modes as HTTP 404 (feature absent on older Appwrite) and HTTP 401/403 (missing API key scope) — both 4xx, so they are still re-thrown and the original crash is not resolved.
  • The pre-existing unconditional \Throwable catches in reportDomains() and reportProjects() are also narrowed to 5xx-only, regressing their previously broader error tolerance.

Confidence Score: 3/5

The fix does not actually resolve the reported crash — the 5xx-only error filter re-throws the 404 and 401 responses that trigger the bug, and it also makes the existing domain/project catches stricter than they were before.

The catch blocks introduced for Sites and Messaging only silence 5xx errors, but the crash described in the PR is caused by 404 (missing endpoint on older Appwrite) and 401 (missing scope), which are still re-thrown. Additionally, the pre-existing unconditional catches in reportDomains and reportProjects have been narrowed to 5xx-only, regressing their error handling for 4xx responses they previously absorbed.

src/Migration/Sources/Appwrite.php — the error-code filter in every new and modified catch block needs revisiting.

Important Files Changed

Filename Overview
src/Migration/Sources/Appwrite.php Adds try/catch guards to reportSites() and reportMessaging(), and narrows existing catches in reportDomains()/reportProjects() — but the 5xx-only filter re-throws the 404/401 errors that are the documented failure cases, leaving the original bug unfixed.

Reviews (3): Last reviewed commit: "Inline Appwrite report server error chec..." | Re-trigger Greptile

Comment thread src/Migration/Sources/Appwrite.php Outdated
…reportMessaging

Addresses review feedback: the site-variable catch was leaving a
partially-accumulated count instead of resetting to 0 like every other
catch in these methods, and reportMessaging no longer lets any
exception escape, so the @throws AppwriteException tag was stale.
@abnegate abnegate merged commit 2110072 into main Jul 8, 2026
4 checks passed
@abnegate abnegate deleted the fix/appwrite-source-report-crash branch July 8, 2026 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants