Retry transient transaction failures in the Mongo adapter#889
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesMongoDB Transaction Resilience
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Greptile SummaryThis PR gives
Confidence Score: 5/5Safe to merge — the change is a faithful port of the base adapter's retry loop into the Mongo override, with no altered semantics beyond adding the retry path. The retry loop, backoff math, non-retryable exception list, and per-attempt state cleanup are all verified to match the base Adapter::withTransaction. Session cleanup via the finally block correctly nulls $this->session before startTransaction() is called on the next attempt, and rollbackTransaction() already ends the session internally in all paths, so the if ($this->session) guard in withTransaction's finally avoids any double-end. No logic divergence from the base adapter was found. No files require special attention. Important Files Changed
Reviews (2): Last reviewed commit: "(fix): retry transient transaction failu..." | Re-trigger Greptile |
Load test results (local appwrite/appwrite stack, MongoDB 8.2.5 single-node replica set)Validated this branch inside the full Appwrite stack (appwrite/appwrite#12587 runs it through CI in parallel). Hammer test — for each iteration, create a document then fire 2×
Every 5xx on Scenario-faithful test — reproduction of the flaky
The previously flaky |
What does this PR do?
Gives
Mongo::withTransactionthe same retry-with-backoff behavior every other adapter already inherits fromAdapter::withTransaction: up to 2 retries with 50ms/100ms backoff on transient failures.Mongo is the only adapter that overrides
withTransactionwithout reproducing the base retry loop, so transient MongoDB errors — most notablyE112 WriteConflict, which MongoDB documents as retryable ("Please retry your operation or multi-document transaction") — surface immediately as 500s instead of being retried.Non-transient typed exceptions (
Duplicate,Restricted,Authorization,Relationship,Conflict,Limit) still throw immediately without retrying, matching the base adapter's exclusion list. The existing rollback/session-cleanup semantics are preserved as-is and now run before each retry, so every attempt starts with a fresh session.Test Plan
composer lintpasses.Related PRs and Issues
Root cause of the flaky
testInvalidSSRSourceSites E2E failures on appwrite/appwrite MongoDB CI jobs (e.g. https://github.com/appwrite/appwrite/actions/runs/27395946536/job/80963312794). The builds worker updates the site document'slatestDeployment*attributes at the same time the test's cleanup issuesDELETE /v1/sites/:siteId; the delete transaction hitsE112 WriteConflictand bubbles up as500 general_unknown. SQL adapters don't flake because InnoDB row locks make the delete wait, and because their inheritedwithTransactionretries.Checklist
Summary by CodeRabbit