Repository navigation
[noqa] fix: reconnect Pusher when a PONG is missed instead of only logging - #96883
Conversation
The one-shot latch disarmed the watchdog after a single reconnect, so a socket that stayed dead (e.g. a network that throttles websockets) went silently stale again. The PONG-clock reset now paces retries on its own (~1 reconnect every 2 minutes), and the ping-send timestamp is left alone so the fresh socket gets probed by the very next PING. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
|
@Krishna2323 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] |
There was a problem hiding this comment.
Pull request overview
This PR improves the resiliency of the app’s Pusher “PING/PONG watchdog” by attempting to recover from stalled/dead websocket connections (e.g., after sleep/wake or websocket throttling) via Pusher.reconnect(), rather than only emitting a misleading “going offline” log.
Changes:
- Update the Pusher PING/PONG watchdog to call
Pusher.reconnect()when PONGs have been missing beyond the threshold. - Adjust watchdog log messaging to reflect the “socket presumed dead” behavior rather than “going offline”.
- Add a new unit test covering the watchdog reconnect behavior and how a fresh PONG defers reconnects.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/libs/actions/User.ts |
Changes watchdog behavior to reconnect Pusher when PONGs are missing past the threshold and updates related logging. |
tests/unit/PusherPingPongTest.ts |
Adds unit coverage for watchdog reconnect timing and PONG-reset behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…uite Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s deterministic The previous approach reset the PONG clock on reconnect, which made the next check tick a photo finish against the 60s threshold: timer jitter (especially in throttled background tabs) could make the watchdog retry every check tick (~1 minute) instead of every second one (~2 minutes), doubling the ReconnectApp volume during an outage. Skipping exactly one check has no time comparison to race, and keeping the PONG clock untouched means logs now report the true age of the outage. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
JmillsExpensify
left a comment
There was a problem hiding this comment.
No product review required.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
This comment was marked as resolved.
This comment was marked as resolved.
|
@Krishna2323 does not repro for me |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.4.46-0 🚀
|
|
🤖 Help site review: no changes required I reviewed the changes in this PR against Expensify's help site files under Why: This is a purely internal networking-resilience change. The Pusher PING/PONG watchdog now calls There is no user-facing change here — no new or modified feature, setting, tab, label, button, or workflow that a customer would read about. The effect is entirely behind the scenes: chat and report updates silently recover on their own instead of stalling until a manual reload. The help site documents product features and workflows, not the app's WebSocket connection internals, so there's nothing to add or update. I also searched If you believe a customer-facing behavior did change and should be documented, let me know what to cover and I'll draft the article. |
|
🚀 Deployed to production by https://github.com/marcaaron in version: 9.4.46-10 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
The app sends a PING to Pusher every 30 seconds and expects a PONG back. A watchdog checks for missing PONGs every 60 seconds. Today, when the PONG never comes back (for example after the machine sleeps, or on a network that throttles websockets), the watchdog only writes a misleading "going offline" log line and does nothing — so chat and report updates silently stop until the user reloads the app.
Now the watchdog calls
Pusher.reconnect()when the PONG has been missing for over 60 seconds. Each reconnect resets the PONG clock, so the new socket gets a fresh grace period and retries space out naturally to about one reconnect every 2 minutes. Retries continue for as long as PONGs stay missing and stop the moment one arrives. Reconnecting also re-subscribes the user channel, which triggersreconnectApp, so the messages missed while the socket was dead are fetched too. The misleading log text is fixed.Fixed Issues
$ #93966
PROPOSAL:
Tests
npx jest tests/unit/PusherPingPongTest.ts— 2 tests pass: the watchdog reconnects once the PONG goes missing and keeps retrying about every 2 minutes while PONGs stay missing, and a fresh PONG defers the next reconnect.Offline tests
The watchdog already early-returns while offline, so this change has no effect offline; behavior offline is unchanged.
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same as tests
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
Non-UI change — verified via unit tests (
tests/unit/PusherPingPongTest.ts), no visual changes.Android: mWeb Chrome
Non-UI change — verified via unit tests (
tests/unit/PusherPingPongTest.ts), no visual changes.iOS: Native
Non-UI change — verified via unit tests (
tests/unit/PusherPingPongTest.ts), no visual changes.iOS: mWeb Safari
Non-UI change — verified via unit tests (
tests/unit/PusherPingPongTest.ts), no visual changes.MacOS: Chrome / Safari
Non-UI change — verified via unit tests (
tests/unit/PusherPingPongTest.ts), no visual changes.