[No QA] Log deferred WRITE Onyx flush failures after the watermark advanced#96290
Draft
elirangoshen wants to merge 2 commits into
Draft
[No QA] Log deferred WRITE Onyx flush failures after the watermark advanced#96290elirangoshen wants to merge 2 commits into
elirangoshen wants to merge 2 commits into
Conversation
Follow-up to Expensify#96039: WRITE requests queue their updates and resolve before the deferred flush applies them, so the lastUpdateID watermark advances before the real write. Surface a flush failure in Sentry to confirm the issue before adding a behavioural fix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…-write-flush-failure
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Explanation of Change
Follow-up to #96039. That PR advanced the
lastUpdateIDwatermark (ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT) only after updates apply successfully — but that guarantee only holds for Pusher, Airship, and READ API commands.As raised in review and confirmed here, WRITE requests still have the original bug. In
applyHTTPSOnyxUpdates, WRITE requests usequeueOnyxUpdatesas their handler, which only appends the updates to an in-memory array and resolves immediately — the real write to Onyx happens later inQueuedOnyxUpdates.flushQueue()(driven bySequentialQueue). So the promise the watermark now keys off resolves before the deferred apply, and if that laterOnyx.updatefails, the updates are silently lost while the watermark already claims the client is caught up.Following the same workflow as before (log first, add the behavioural fix only if the logs confirm the problem), this PR is logging only. It adds an
[OnyxUpdateManagerError]Sentry alert when the deferred flush'sOnyx.updaterejects, then rethrows — no behavioural change. By the timeflushQueueruns, the watermark has unconditionally already advanced, so any rejection here is by definition "after the watermark advanced."Fixed Issues
$ #94877
PROPOSAL:
Tests
This PR is logging only — it adds a Sentry alert but changes no behaviour, so there is nothing user-facing to QA. It ships this way to confirm in production whether the deferred WRITE flush actually fails before we commit to a behavioural fix. The failure it logs is a storage-layer error path that can't be triggered through normal UI use.
[OnyxUpdateManagerError]noise appears during normal use (the alert should only fire on an actual storage write failure).Offline tests
Go offline, perform actions (e.g. create an expense), then come back online. Verify the app syncs as before and no unexpected errors appear — this change does not alter offline behaviour.
QA Steps
Same as Tests (regression only). This is an internal reliability/observability change to how queued Onyx updates are flushed, with no user-facing UI. QA only needs to confirm there are no regressions in report/transaction syncing during normal use.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.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