Retry mechanism with retry breaking system#797
Conversation
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8bc993cafd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
# Conflicts: # lib/OnyxUtils.ts # lib/storage/providers/IDBKeyValProvider/createStore.ts
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23bc519d72
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: edc6ec8dbb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| StorageCircuitBreaker.recordEviction(); | ||
|
|
||
| // @ts-expect-error No overload matches this call. | ||
| return remove(keyForRemoval).then(() => onyxMethod(defaultParams, nextRetryAttempt)); |
There was a problem hiding this comment.
Move eviction marker after deletion completes
Fresh evidence beyond the earlier stale-marker issue is that this records an eviction before remove(keyForRemoval) has resolved. When several writes hit quota concurrently, the next write's rejection can run while the first deletion transaction is still pending, so recordCapacityFailure() counts it as a no-progress eviction even though nothing has been freed yet; after five such racing failures the breaker opens and subsequent storage writes are skipped for 60s despite the evictions potentially succeeding. Record the eviction only after the deletion completes, or associate the no-progress verdict with the retry that follows that deletion.
Useful? React with 👍 / 👎.
Details
Replace the brittle, string-matching storage-error retry logic with a layered design — classify the error → route it to exactly one recovery owner → stop session-wide storms with a circuit breaker.
Related Issues
Expensify/App#94069
Linked E/App PR
Expensify/App#92931
Automated Tests
Manual Tests
Prerequisites: put Chrome into insufficient storage situation by going to Console -> Application -> Storage and enable
Simulate custom storage quota. Set a value lower than used by the application.OnyxStorage circuit breaker tripped: 5 consecutive evictions freed no usable space. Halting eviction/retry for 60s to stop a storage failure storm.Author Checklist
### Related Issuessection above### Linked E/App PRsection above, and verified this change against it (E/App CI passed and manual testing completed)TestssectiontoggleReportand notonIconClick)myBool && <MyComponent />.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)Avataris modified, I verified thatAvataris working as expected in all cases)mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
before:
Nagranie.z.ekranu.2026-06-22.o.15.05.45.mov
after:
Nagranie.z.ekranu.2026-06-22.o.15.07.34.mov