Count in-flight Pusher applies as applied when detecting update gaps by adhorodyski · Pull Request #99774 · Expensify/App

adhorodyski · 2026-08-28T15:24:00Z

Explanation of Change

When a Pusher update is still being written to disk, the next Pusher event that follows it stops being mistaken for a missing update, so the client no longer pauses its write queue and re-downloads what it is already writing.

Fixed Issues

$ #100313
PROPOSAL: N/A

Follow-up, not fixed here: #100174

Tests

Check out this branch and run npm run test -- tests/unit/OnyxUpdatesTest.ts tests/unit/OnyxUpdatesPusherChainFailureTest.ts. 20 pass.

Six tests were added, five beside their existing WRITE-path siblings in OnyxUpdatesTest.ts. Only the first two fail on main; the rest are green there because main has no marker at all, so each one names the change that turns it red instead:

Test	Fails when
`does not report a gap for a Pusher update that is still applying`	on `main` — this is the behaviour the PR changes
`relies on pusherEventsPromise staying rejected to stop a follower whose gap check the failed update had suppressed`	on `main`, and on this branch if `pusherEventsPromise` is made to recover from a rejection
`reports a gap for an HTTPS response chained on a Pusher update that is still applying`	the `updateType` gate is dropped and every transport reads the marker
`clears the Pusher pending apply marker when an unrelated apply fails below it, so a real gap is never masked`	`lastUpdateIDPendingApply = 0` is deleted from the shared `.catch`
`stops counting a Pusher update as in flight once its apply has settled`	the clear in the success `.then()` is deleted
`clears the pending apply watermark on sign-out`	the sign-out reset is deleted

Adding the marker to getEffectiveLastUpdateID() reddens four tests, including keeps a Pusher update that is still applying out of the catch-up fetch range, which is what guards that decision.

relies on pusherEventsPromise staying rejected to stop a follower whose gap check the failed update had suppressed lives in its own file. A rejected Pusher apply leaves the module-scoped pusherEventsPromise rejected for the rest of the module's life, so it cannot share an instance with the other tests. It is also the test that pins the coupling tracked in A single rejected Pusher apply poisons pusherEventsPromise, stopping all Pusher Onyx updates for the session #100174: nothing checks that update IDs are contiguous before advancing the watermark, so serializing on pusherEventsPromise is the only thing stopping the follower from moving the watermark past the failed update's range.
Run the regression set: npm run test -- tests/unit/OnyxUpdatesTest.ts tests/unit/OnyxUpdatesPusherChainFailureTest.ts tests/unit/OnyxUpdateManagerTest.ts tests/actions/OnyxUpdateManagerTest.ts tests/unit/SequentialQueueTest.ts tests/actions/QueuedOnyxUpdatesTest.ts. 107 pass.
Manual, web: open the app with the JS console filtered to [OnyxUpdateManager], then send several messages in quick succession in a busy chat while a second device posts to the same chat.
Verify lastUpdateIDFromClient is less than the previousUpdateID received no longer fires for an update ID that a preceding [OnyxUpdateManager] Applying pusher update line already named.
Verify messages still arrive in order and no message is lost or duplicated.

Verify that no errors appear in the JS console

Offline tests

Go offline, send two messages, and verify both queue and show as pending.
Go back online and verify both send, the queue drains, and the chat matches the server state after a refresh.
Verify that a GetMissingOnyxMessages or ReconnectApp still fires when the client is genuinely behind, which is the reconnect path this change deliberately leaves intact.

QA Steps

No user-visible behaviour changes on its own. It removes redundant GetMissingOnyxMessages fetches and the write-queue pauses that accompany them, so the observable effect is fewer stalls under load.

Sign in on two devices to the same account and open the same chat on both.
Send messages rapidly from both devices for about a minute.
Verify every message appears on both devices, in order, with no duplicates and no lost messages.
Verify the composer stays responsive and no message sticks in a pending state.

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

N/A — no UI surface. This changes update-gap bookkeeping in src/libs/actions/OnyxUpdates.ts only.

Android: mWeb Chrome

N/A — no UI surface.

iOS: Native

N/A — no UI surface.

iOS: mWeb Safari

N/A — no UI surface.

MacOS: Chrome / Safari

N/A — no UI surface.

adhorodyski · 2026-08-31T11:06:35Z

@codex review

adhorodyski · 2026-08-31T11:06:43Z

@MelvinBot review this PR please

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 290ff85452

ℹ️ 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".

MelvinBot · 2026-08-31T11:34:00Z

Review

The core change is sound. One test-quality problem should be fixed before merge.

Two of the four tests you list as "confirmed red before the fix was kept" are green on main. I checked out main's OnyxUpdates.ts and ran each test individually with -t:

Test	On `main`, in isolation
`does not pause the queue for an event chained on an update that is still being applied`	✅ fails — `Expected: false, Received: true`
`counts a Pusher update that is still applying in the catch-up fetch range`	✅ fails — `Expected: 20, Received: 10`
`does not let an update left mid-apply by the previous session mask a gap after signing back in`	❌ passes in 6 ms
`pauses the queue when the update it was waiting on failed to apply`	❌ passes in 7 ms

Both are expected to pass on main: without the Pusher pending marker, gap detection already pauses the queue in those scenarios. They're still useful as guards on this branch, but they aren't evidence the fix is needed — worth correcting in the description so a reviewer isn't given a false signal.

They only looked red because the file cascades. In ApplyOnyxUpdatesReliablyTest.ts:44, the failing expect throws before releaseApply() runs, so the mocked handler promise never settles. That wedges the module-level pusherEventsPromise, and the next two tests die on the 240 s Jest timeout instead of asserting anything. The full file took 481 s on main and reported a timeout, not a diff.

That's the real risk: any future regression in the first test costs ~8 minutes of CI and reports a misleading failure. The // keep this case last and add new ones above it comment at line 84 documents the fragility rather than removing it. Hoist releaseApply to describe scope and drain it in afterEach so a failed assertion can't hang the chain:

afterEach(() => {
    releaseApply?.();
    jest.restoreAllMocks();
});

Once failures stay local, the ordering comment can go.

Rest of the review — what I verified, and two minor notes

Verified

npm run test -- tests/actions/ApplyOnyxUpdatesReliablyTest.ts tests/unit/OnyxUpdatesTest.ts → 17 pass.
Regression set (step 3) → 91 pass, as claimed.
npx eslint on all three changed files → clean (the one warning on OnyxUpdates.ts:125 is pre-existing and untouched here).

The logic itself looks right

All three exits from the pending marker are covered: the watermark catching up (line 206), the apply failing (line 212), and sign-out (line 44).

Two things I specifically checked that hold up:

isCatchUpRequest at line 158 still reads the persisted watermark only, so GetMissingOnyxMessages responses aren't discarded as stale by the marker they'd otherwise be compared against.
Collapsing the WRITE reset into the shared .catch is behaviour-preserving. The catch is attached to applyPromise.then(() => getCurrentFlushPromise()), so a flush rejection still reaches it — the now-empty .catch(() => {}) at line 236 isn't dropping the reset, it's just swallowing the rethrow.

Note 1: say why the reset is unconditional

lastUpdateIDPending = 0 in the catch is shared by every update type, so an Airship or non-WRITE HTTPS failure clears a marker belonging to an unrelated in-flight Pusher apply. That's the correct choice — scoping it to pending <= lastUpdateID would let a higher, unrelated marker mask the gap left by the update that just failed, which is a data-loss bug rather than an extra fetch. But the comment on line 24 only says "reset if the apply fails", which reads like an oversight. Worth one line saying the blanket reset is deliberate and which way it errs.

Note 2: Airship has the same shape

applyAirshipOnyxUpdates serializes through a single airshipEventsPromise exactly like the Pusher path, so its watermark trails the same way. Fine to leave out of scope, but it's the same bug and it's now the only path without a marker.

Agreed that the pusherEventsPromise poisoning you flagged belongs in its own issue. For what it's worth, this PR doesn't make it worse: a poisoned chain rejects every later apply, which resets the marker and lets gap detection fire.

Web smoke test

Result: partial. No UI surface here, so this only checks that nothing obvious broke.

Step	Status	Observation
Reach Home and open a chat	pass	Landed on Home, Inbox auto-opened Concierge.
Send 5 messages rapidly — in order, no duplicates, none stuck pending	pass	All 5 present in order with timestamps, no duplicates, no pending indicator.
After reload, all 5 still present in order	fail	Not run. Reloading drops client auth in this harness and can't be recovered, so the tester declined. Needs a human.
Console errors / `OnyxUpdateManager` lines	partial	No console capture verb exists for web in this harness, so the console stream couldn't be read. Nothing surfaced in the network dump or accessibility tree.

One observation rather than a finding: the network dump showed 5x POST /api/GetMissingOnyxMessages, all 200. Single client with no concurrent writer, so it says nothing either way about the 24.5% reduction — but the two-device QA step is what would actually demonstrate that, and it's still unrun.

Evidence (1)

Concierge chat showing 5 rapid test messages sent in order

The high-traffic-account test you called out as the one that matters most is still the gap I can't close — I can't reproduce the concurrent-writer conditions that produce the gap detections you measured.

view run

applyPusherOnyxUpdates serializes every event through one module-level promise, and the watermark only advances once that chain settles. So the next event, chained on an update the client already holds, reads as a gap: the client pauses its own write queue and refetches data it is part-way through writing. Measured over 2026-08-26 production logs, 24.5% of gap detections had the missing update delivered before the gap fired and applied after it, at p50 209ms arrival to detection. lastUpdateIDPendingApply records the highest update ID accepted into the Pusher apply chain, and getLastUpdateIDForGapCheck counts it as applied. It is deliberately absent from getEffectiveLastUpdateID, which is the lower bound of the catch-up fetch range. An accepted update is not a written one. If the apply then rejects, a range fetched from it skips that update for good: the catch-up response is exempt from gap detection, so it advances the persisted watermark past the hole and nothing detects it again. Fetching from the persisted watermark refetches the in-flight range instead, which only costs bandwidth. The test named "keeps a Pusher update that is still applying out of the catch-up fetch range" holds that line. lastUpdateIDPendingFlush already carries the same hazard for the WRITE path, since it does sit in getEffectiveLastUpdateID. Left as-is here; it wants its own issue. Internal tracking: callstack-internal/expensify-issues#2882 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

applyPusherOnyxUpdates reassigns the module-level pusherEventsPromise from itself, so a held apply blocks every later Pusher apply in the file. The held promise was only released on each test's happy path, after its assertions. A failing assertion threw first, the mocked handler never settled, and the following tests hit the 240s Jest timeout instead of asserting -- the file reported a timeout, not a diff, and --testTimeout did not bound it. Releasing it from afterEach turns that into one local failure: with the first Pusher assertion inverted the suite now reports 1 failed, 16 passed in 1.5s. The rejection case still has to run last. afterEach releases a held apply, it cannot un-reject a rejected chain, so that test keeps its name. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

adhorodyski · 2026-09-01T14:24:31Z

Note 2: Airship has the same shape

applyAirshipOnyxUpdates serializes through a single airshipEventsPromise exactly like the Pusher path, so its watermark trails the same way.

Agreed, the shape is identical: applyAirshipOnyxUpdates serializes on airshipEventsPromise the same way. Leaving it out here because I have not measured the Airship path's share of gap detections. The 24.5% in the description is Pusher-only. I will check the logs and either add the marker or open an issue.

adhorodyski · 2026-09-01T16:25:21Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0d70c5192c

ℹ️ 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".

The reset in the shared catch was bare, and a reviewer read it as an oversight and asked for it to be scoped to Pusher failures. Scoping it that way is a data-loss path: the marker is read as a flat max, so a lower-ID non-Pusher failure would stay masked once the in-flight Pusher apply advances the watermark, whereas today it costs one redundant GetMissingOnyxMessages. Name the trade at the reset, and pin it with a test that fails an unrelated Airship apply while a Pusher apply is held. Carry the rest by naming: getLastUpdateIDForGapCheck said when it is called, not how it differs from getEffectiveLastUpdateID, so the difference needed a comment on the marker to explain it. Renamed to getLastUpdateIDIncludingInFlightApplies, which states the difference and leaves the declaration to speak for itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Both tests rejected an apply whose promise chain lives in module scope, so the rejection outlived them and only the next test's position kept the file green. Adding an Airship or Pusher test below either one would have failed on a poisoned chain rather than on a diff, which is the fragility that already had to be fixed once for the held-apply helper. Fail a READ request's apply instead of an Airship one: applyHTTPSOnyxUpdates returns a fresh promise per call, so nothing leaks, and failing it at update 15 under a Pusher apply held at 20 is the case that argues for the unconditional reset — the marker masks 15 if the reset is scoped by type. The Pusher chain has no such escape, so that test loads its own copy of the module and passes the watermark in, keeping it independent of both the chain and the Onyx instance the fresh copy connects to. Verified by deleting the reset: these two go red, the other sixteen stay green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The describe now says the event chain is module-scoped, which is the whole reason the block resets modules and imports its own copy. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

adhorodyski · 2026-09-02T11:51:39Z

@MelvinBot this PR is aimed to close the gap on excess GetMissingOnyxMessages API calls so we fire less of them when there is no need to do so. Please review this PR.

MelvinBot · 2026-09-02T12:12:20Z

Review

The core idea is right and the premise checks out: nothing at the Pusher ingress serializes the gap check, so update B genuinely arrives while update A's apply is still in flight and falsely reads as a gap on main. Keeping the marker out of getEffectiveLastUpdateID() is the correct call and addresses the P1 bot comment. I also agree with your pushback on the P2 comment — scoping the reset to Pusher failures would lose data.

Two things to fix before this ships.

1. The marker suppresses gaps for HTTPS too, but the thing that makes that safe is Pusher-only

doesClientNeedToBeUpdated is also the gap check for HTTPS responses at SaveResponseInOnyx.ts:50. So an in-flight Pusher apply now suppresses gaps on a transport whose apply runs on a completely independent promise chain (OnyxUpdates.ts:252, Onyx.update).

Verified empirically against your branch: watermark 10, Pusher A (10→20) held mid-apply, then an HTTPS READ response {previousUpdateID: 20, lastUpdateID: 30} arrives. The marker says "no gap", B applies, watermark advances to 30. A then rejects, clears the marker — too late. Updates 11–20 are gone with nothing left to trigger recovery.

Since the 24.5% you measured is Pusher-chained-on-Pusher, scoping the marker's use to the Pusher call site (applyOnyxUpdatesReliably.ts:51) keeps the whole win and closes this. Note Airship goes through the same call site on airshipEventsPromise, a third independent chain — scope to Pusher, not to applyOnyxUpdatesReliably.

2. The `pusherEventsPromise` bug you flagged is load-bearing for this PR

For the pure-Pusher failure case, nothing checks ID continuity — OnyxUpdates.ts:208 only checks monotonicity, so 30 > 10 advances the watermark past a never-applied 11–20. The only reason it doesn't is that A's rejection propagates through the shared pusherEventsPromise, so B rejects too and takes the .catch. I confirmed this: handlerCallCount === 1 — B's handler never runs, and the watermark stays at 10.

That means the "known issue" in your description isn't separable. Whoever fixes pusherEventsPromise by resetting it to Promise.resolve() in a catch silently turns this PR into data loss, and no test in this file will catch it. Either fix both together, or add a test that pins the coupling and a comment at OnyxUpdates.ts:256 saying so.

Smaller things

lastUpdateIDPendingApply is never cleared on success, unlike lastUpdateIDPendingFlush at OnyxUpdates.ts:214. It's a session high-water mark, not a pending marker, and the name says otherwise. Benign today (a successful apply moves the watermark to the same value, so Math.max is a no-op), but it does override an explicitly-passed lower clientLastUpdateID, which main would have reported as a gap. Clear it in the .then() for symmetry.
afterEach(() => releaseHeldApply?.()) never resets releaseHeldApply to undefined, so every later test re-invokes a stale resolver. Harmless, but reset it.
clears the pending apply watermark on sign-out releases the held apply after Onyx.clear(), so the apply then advances lastUpdateIDAppliedToClient to 20 in the post-sign-out state. Only the beforeEach clear saves the next test.
The jest.resetModules() describe re-imports OnyxUpdates, which registers a second Onyx.connectWithoutView on the same key that leaks for the rest of the file.

What I checked, and what I didn't

Ingress ordering (confirms the premise). There is no await, queue, or mutex between the websocket message and the gap check: pusher-js emit is a synchronous loop ignoring return values, PusherUtils.ts:47-50 ignores the return, and User.ts:913 discards the promise from applyOnyxUpdatesReliably. So B's check runs synchronously inside its own callback while A is unsettled. The marker is set at OnyxUpdates.ts:256 before applyPusherOnyxUpdates is entered, so it's visible to B.

Recovery after a pure-Pusher failure works. Both A and B hit the .catch, marker resets to 0, the next update sees max(10, 0, 0) = 10 < 30, and the normal fetchMissingUpdates → GetMissingOnyxMessages(10 → N) path runs.

Refetch ranges are not shrunk. getEffectiveLastUpdateID() excludes the marker, and OnyxUpdateManager/index.ts:213 and utils/index.ts:110,136 all use it, so the P1 concern doesn't apply to the current diff.

CI. test (job 3) is red on NextStepUtilsTest → scheduled submit enabled › monthly on the 2nd, a date-dependent failure on Sept 2. Unrelated to this PR. Everything else passes. I could not run jest locally in this environment.

Not checked: the base rate of Pusher applies actually rejecting in production. Both failure scenarios above only matter at that rate, and your description suggests it's low. Worth pulling [OnyxUpdateManagerError] counts filtered to type: 'pusher' to size it. Also not checked: whether the backend actually emits an HTTPS response whose previousUpdateID matches a mid-apply Pusher update — the code path is reachable and the loss is confirmed, but I didn't verify the server produces that pairing.

Web smoke test on this branch (supporting only — not the test that matters)

Ran dev NewDot on 2882-pusher-gap-detection. This is a fresh single-device account, so it does not exercise the concurrency this PR targets. The high-traffic two-device run you asked for in the checklist is still the one that matters.

Step	Status
Home loads and the LHN shows chats Inbox LHN rendered with the Concierge chat, nav intact.	✅
Send 3 messages in quick succession — in order, no duplicates, none pending All three present in order; LHN preview updated; no failed/pending state; all AddComment calls returned 200.	✅
Reload and confirm messages persist Not run — a page reload drops client auth in this harness and can hit an unrecoverable magic-code wall.	⚠️
Report JS console errors and any gap-detection log lines Could not verify — the web harness does not expose browser console output, so I cannot confirm or deny the `lastUpdateIDFromClient is less than the previousUpdateID` / `[OnyxUpdateManagerError]` lines. No error banners or toasts appeared and no request failed.	⚠️

One observation: two GetMissingOnyxMessages calls still fired during the 3-message burst (both 200). On a brand-new onboarding account that is likely legitimate, but it's a reminder that this closes one specific source of the excess calls rather than all of them.

view run

mountiny · 2026-09-02T13:23:33Z

@MelvinBot van you create an issue for this or and link it in the pr body. Assign mountiny, pr author to the issue and make it Daily, Task

MelvinBot · 2026-09-02T13:30:46Z

Created #100174 and linked it in the PR body under "Known issue found while writing this, not fixed here".

Assignees: mountiny, adhorodyski
Labels: Daily, Task

I filed it for the pusherEventsPromise poisoning bug — the "Worth its own issue" item already flagged in the description, and finding #2 from my review. No existing issue covers it.

The issue body records the ordering constraint, which is the part that matters: this bug is currently load-bearing for this PR. Fixing pusherEventsPromise in isolation would turn this PR into data loss, because the poisoned chain is the only thing stopping a chained follow-up update from advancing the watermark past a failed earlier apply. It needs an explicit continuity guard in advanceLastUpdateIDAfterApply to land alongside it. The proposed fix in the issue covers Airship too, since airshipEventsPromise has the identical shape.

If you meant a different item — finding #1 (the cross-transport hole) or the Airship marker adhorodyski mentioned earlier — say which and I'll file that instead. I read finding #1 as a change for this PR rather than a follow-up.

view run

mountiny · 2026-09-03T07:33:19Z

@MelvinBot can you make sure the linked issue reflects what this PR is about #100174

MelvinBot · 2026-09-03T07:38:41Z

No — #100174 is a different bug, and the issue itself says so.

What the issue is about: a single rejected Pusher apply leaves the module-scoped pusherEventsPromise permanently rejected, so every later Pusher update dies on the poisoned chain. It is pre-existing on main. Its own body states: "This is pre-existing on main and not introduced by #99774" and "Fixing this in isolation would silently turn #99774 into data loss."

What this PR is about: adding lastUpdateIDPendingApply so an in-flight Pusher apply counts as applied during gap detection, cutting the redundant GetMissingOnyxMessages fetches and write-queue pauses (24.5% of gap detections).

Nothing in the diff touches pusherEventsPromise rejection handling, so merging this will not close #100174 — and #100174 currently carries the Reviewing label as if it does.

Recommendation: follow the precedent from the sibling change — #98777 has its own issue, #98984, titled the same as the PR. Create a dedicated issue for this PR, point ### Fixed Issues at it, and keep #100174 listed only as a related/blocking issue. Say the word and I'll open it and re-point the link.

Separate problem: the PR description no longer matches its own diff

Checked against head 20934efd:

"doesClientNeedToBeUpdated now takes the transport the update arrived on" — it doesn't. OnyxUpdates.ts:294 still destructures only {previousUpdateID, clientLastUpdateID}, and line 34 reads the marker for every caller, HTTPS included. That gate was the fix for the data-loss path I flagged on 2026-09-02 — it isn't in the code yet.
"A successful apply clears it too" — it doesn't. The marker is cleared only at sign-out (line 51) and in the shared .catch (line 221). There is no clear in the success .then().
tests/unit/OnyxUpdatesPusherChainFailureTest.ts — referenced in Tests steps 1, 3 and 4, but the file doesn't exist at head or on main. The changed files are src/libs/actions/OnyxUpdates.ts and tests/unit/OnyxUpdatesTest.ts only, and the Tests section's claim of "20 pass" / "97 pass" can't be reproduced as written.

Reads like the description was written ahead of the code. Worth pushing those commits or trimming the description back to what's actually there, so a reviewer isn't reviewing a version that doesn't exist.

view run

An in-flight Pusher apply is only safe to count as applied for callers serialized behind it on pusherEventsPromise. HTTPS and Airship applies run on their own chains, so reading the marker there could advance the watermark past updates the held apply has not written, leaving no gap to recover from. Also clear the marker once its apply settles, so it stops standing for an update that is no longer in flight and cannot override a lower clientLastUpdateID that a caller passed explicitly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

melvin-bot · 2026-09-03T15:24:05Z

@DylanDylann Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

DylanDylann · 2026-09-08T05:47:49Z

Consider updating getEffectiveLastUpdateID to include lastUpdateIDPendingApply. If you think it's unnecesary, pleae add a comment

DylanDylann · 2026-09-08T06:46:50Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified that the composer does not automatically focus or open the keyboard on mobile unless explicitly intended. This includes checking that returning the app from the background does not unexpectedly open the keyboard.
I verified tests pass on all platforms & I tested again on:
- Android: HybridApp
- Android: mWeb Chrome
- iOS: HybridApp
- iOS: mWeb Safari
- MacOS: Chrome / Safari
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
- I verified that all the inputs inside a form are aligned with each other.
- I added Design label and/or tagged @Expensify/design so the design team can review the changes.
For any bug fix or new feature in this PR, I verified that sufficient unit tests are included to prevent regressions in this flow.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Screen.Recording.2026-09-08.at.13.51.14.mov

DylanDylann · 2026-09-08T06:52:16Z

@adhorodyski Some minor NITs

adhorodyski · 2026-09-08T12:24:57Z

re: lastUpdateIDPendingApply on getEffectiveLastUpdateID, there is a test written here that fails exactly on adding it there. leaving this suggestion as is.

mountiny

Thank you

github-actions · 2026-09-09T10:28:38Z

🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here.

OSBotify · 2026-09-09T10:31:24Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

github-actions · 2026-09-09T10:32:47Z

🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
Built from App PR #99774.

Android 🤖	iOS 🍎
https://ad-hoc-expensify-cash.s3.us-east-1.amazonaws.com/rock-artifacts/ad-hoc/rock-android-Adhoc-a3decd0-57708e6-cf3e978c44dc834d23c525bd21b0d0a80a832fee/index.html	https://ad-hoc-expensify-cash.s3.us-east-1.amazonaws.com/rock-artifacts/ad-hoc/rock-ios-device-AdHoc-a3decd0-57708e6-f18141dddde2c57b3ba8f9a52e9b3c16916d07c2/index.html

Web 🕸️
https://99774.pr-testing.expensify.com

👀 View the workflow run that generated this build 👀

OSBotify · 2026-09-09T19:44:18Z

🚀 Deployed to staging by https://github.com/mountiny in version: 9.4.75-0 🚀

platform	result
🕸 web 🕸	success ✅
🤖 android 🤖	success ✅
🍎 iOS 🍎	success ✅

MelvinBot · 2026-09-09T19:48:37Z

No help site changes are required for this PR, so I did not create a draft PR.

This change is internal client sync bookkeeping. It splits the pending-update watermark into lastUpdateIDPendingWriteFlush and lastUpdateIDPendingPusherApply, and gates the new marker to Pusher updates in doesClientNeedToBeUpdated(). Nothing in the diff adds, renames, or removes a feature, setting, tab, menu, or button.

What I checked

Files changed — all four are library or test code, none is user-facing:

File	Nature
`src/libs/actions/OnyxUpdates.ts`	Gap-detection watermark logic
`src/libs/actions/applyOnyxUpdatesReliably.ts`	Passes `updateType` through
`tests/unit/OnyxUpdatesTest.ts`	Tests
`tests/unit/OnyxUpdatesPusherChainFailureTest.ts`	Tests

No user-facing surface. The diff touches no src/languages/*.ts file, no component, and no copy string. Your own QA steps say it best: "No user-visible behaviour changes on its own." The only observable effect is fewer redundant GetMissingOnyxMessages fetches and fewer write-queue pauses under load — a performance characteristic, not documented behavior.

No matching articles. I searched all 947 files in docs/articles for sync, update, and offline-behavior content. Every "sync" match is accounting-integration sync (QuickBooks, NetSuite, Xero, payroll connections), which this change does not affect. The help site does not document Onyx update-gap detection, Pusher transport, or client watermarking, and it should not — these are implementation details with no user-facing control or symptom to describe.

Per docs/HELP_AUTHORING_GUIDELINES.md, help articles describe what a customer can see and do in the product. There is no such change here to describe.

Since no docs changes are needed, there is no linked help site PR to review, and no HelpDot label or assignment to apply. If you expected this PR to change something a customer can observe, tell me what that is and I will re-check the relevant articles.

view run · view recording

OSBotify · 2026-09-10T22:30:35Z

🚀 Deployed to production by https://github.com/mountiny in version: 9.4.75-3 🚀

platform	result
🕸 web 🕸	success ✅
🤖 android 🤖	failure ❌
🍎 iOS 🍎	success ✅

Bundle Size Analysis (Sentry):

Android

OSBotify · 2026-09-11T09:18:32Z

🚀 Deployed to production by https://github.com/mountiny in version: 9.4.75-3 🚀

platform	result
🕸 web 🕸	success ✅
🤖 android 🤖	failure ❌
🍎 iOS 🍎	success ✅

Bundle Size Analysis (Sentry):

Android

melvin-bot Bot assigned adhorodyski Aug 28, 2026

chatgpt-codex-connector Bot reviewed Aug 31, 2026

View reviewed changes

Comment thread src/libs/actions/OnyxUpdates.ts Outdated

adhorodyski force-pushed the 2882-pusher-gap-detection branch 2 times, most recently from e2587cf to 9a564f7 Compare August 31, 2026 12:21

adhorodyski force-pushed the 2882-pusher-gap-detection branch from 9a564f7 to a77a3ce Compare August 31, 2026 12:23

adhorodyski added 2 commits September 1, 2026 17:26

Merge branch 'Expensify:main' into 2882-pusher-gap-detection

61e6e1c

Merge branch 'Expensify:main' into 2882-pusher-gap-detection

0d70c51

chatgpt-codex-connector Bot reviewed Sep 1, 2026

View reviewed changes

Comment thread src/libs/actions/OnyxUpdates.ts Outdated

adhorodyski and others added 4 commits September 2, 2026 12:26

Merge branch 'Expensify:main' into 2882-pusher-gap-detection

f1f9591

Name the reason for resetModules instead of commenting it

77bd7e1

The describe now says the event chain is module-scoped, which is the whole reason the block resets modules and imports its own copy. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

MelvinBot mentioned this pull request Sep 2, 2026

A single rejected Pusher apply poisons pusherEventsPromise, stopping all Pusher Onyx updates for the session #100174

Open

adhorodyski added 2 commits September 2, 2026 17:23

Merge branch 'Expensify:main' into 2882-pusher-gap-detection

1774103

Merge branch 'Expensify:main' into 2882-pusher-gap-detection

20934ef

adhorodyski and others added 2 commits September 3, 2026 13:21

Merge branch 'Expensify:main' into 2882-pusher-gap-detection

4bef1ec

melvin-bot Bot removed the request for review from a team September 3, 2026 15:24

adhorodyski mentioned this pull request Sep 3, 2026

Treat an in-flight reconnect as covering the gap it is already fetching #100320

Merged

39 tasks

garrettmknight removed their request for review September 4, 2026 11:34

mountiny mentioned this pull request Sep 7, 2026

[Due for payment 2026-09-16] Treat an in-flight reconnect as covering the gap it is already fetching #100471

Open

Merge branch 'Expensify:main' into 2882-pusher-gap-detection

554bec0

DylanDylann reviewed Sep 8, 2026

View reviewed changes

Comment thread src/libs/actions/OnyxUpdates.ts Outdated

DylanDylann reviewed Sep 8, 2026

View reviewed changes

Comment thread src/libs/actions/OnyxUpdates.ts

Name the Pusher gap-check gate and restore the deferred-write note

275c1a5

adhorodyski requested a review from DylanDylann September 8, 2026 12:47

DylanDylann approved these changes Sep 9, 2026

View reviewed changes

Comment thread src/libs/actions/OnyxUpdates.ts Outdated

melvin-bot Bot requested a review from mountiny September 9, 2026 04:55

rename variables to address the review round

9c0c59c

mountiny approved these changes Sep 9, 2026

View reviewed changes

mountiny merged commit a3decd0 into Expensify:main Sep 9, 2026
35 of 36 checks passed

adhorodyski deleted the 2882-pusher-gap-detection branch September 9, 2026 11:40

adhorodyski mentioned this pull request Sep 9, 2026

Recover the Pusher apply chain after a rejection, behind a watermark continuity guard #100594

Open

40 tasks

OSBotify mentioned this pull request Sep 9, 2026

Deploy Checklist: New Expensify 2026-09-09 #100739

Closed

83 tasks

applause-bot Bot mentioned this pull request Sep 10, 2026

[Exploratory] Lost chat messages when login same account login on third device #100791

Open

7 tasks

Uh oh!

Conversation

adhorodyski commented Aug 28, 2026 • edited by MelvinBot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Explanation of Change

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Uh oh!

adhorodyski commented Aug 31, 2026

Uh oh!

adhorodyski commented Aug 31, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

MelvinBot commented Aug 31, 2026

Review

Verified

The logic itself looks right

Note 1: say why the reset is unconditional

Note 2: Airship has the same shape

Web smoke test

Concierge chat showing 5 rapid test messages sent in order

Uh oh!

adhorodyski commented Sep 1, 2026

Note 2: Airship has the same shape

Uh oh!

adhorodyski commented Sep 1, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

adhorodyski commented Sep 2, 2026

Uh oh!

MelvinBot commented Sep 2, 2026

Review

1. The marker suppresses gaps for HTTPS too, but the thing that makes that safe is Pusher-only

2. The pusherEventsPromise bug you flagged is load-bearing for this PR

Smaller things

Uh oh!

mountiny commented Sep 2, 2026

Uh oh!

MelvinBot commented Sep 2, 2026

Uh oh!

mountiny commented Sep 3, 2026

Uh oh!

MelvinBot commented Sep 3, 2026

Uh oh!

melvin-bot Bot commented Sep 3, 2026

Uh oh!

DylanDylann commented Sep 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DylanDylann commented Sep 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviewer Checklist

Screenshots/Videos

Uh oh!

DylanDylann commented Sep 8, 2026

Uh oh!

adhorodyski commented Sep 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

mountiny left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions Bot commented Sep 9, 2026

Uh oh!

adhorodyski commented Aug 28, 2026 •

edited by MelvinBot

Loading

2. The `pusherEventsPromise` bug you flagged is load-bearing for this PR

DylanDylann commented Sep 8, 2026 •

edited

Loading

DylanDylann commented Sep 8, 2026 •

edited

Loading

adhorodyski commented Sep 8, 2026 •

edited

Loading

MelvinBot commented Sep 9, 2026 •

edited

Loading