-
Notifications
You must be signed in to change notification settings - Fork 3.6k
[No QA] Fix version sync race condition between App and Mobile-Expensify #81369
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This addresses the issue where cherry-pick workflows fail because Mobile-Expensify version gets bumped but E/App does not, leaving the repos out of sync. Changes: - createNewVersion.yml: Replace single retry with 5-attempt retry loop with exponential backoff for E/App push - createNewVersion.yml: Add version verification step after pushes - cherryPick.yml: Fail fast with exit 1 when version bump commits can't be found, with helpful error messages - syncVersions.yml: New recovery workflow for manual intervention when versions get out of sync (restricted to mobile deployers)
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
Beamanator
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's try it!!
Opus 4.6 recommendation:
The PR is a meaningful improvement and worth merging with fixes, but to truly never need #81763-style PRs again, consider:
- Fix the single-retry in syncVersions.yml (use the same 5-retry loop)
- Have createNewVersion.yml auto-trigger recovery on final failure rather than waiting for manual dispatch
- Remove or make the "Verify version sync" step actually useful
I'm down to take those as followups
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/Beamanator in version: 9.3.16-0 🚀
|
|
🚀 Deployed to production by https://github.com/lakchote in version: 9.3.16-9 🚀
|
Explanation of Change
This PR addresses a race condition in the cherry-pick workflows where Mobile-Expensify version gets auto-bumped but the E/App version does not, leaving the repositories out of sync. This happens when the E/App push fails due to a race condition (another PR merged to main while the workflow was running).
Root cause: Mobile-Expensify must be pushed first (since E/App's submodule references it), but if the subsequent E/App push fails, there's no rollback mechanism, leaving the repos with different versions.
Solution: Since we cannot achieve true atomicity across two Git repositories, this PR implements a multi-pronged approach:
createNewVersion.yml): Replace single retry with 5-attempt retry loop with exponential backoff (2s, 4s, 8s, 16s, 32s delays)createNewVersion.yml): Add a verification step after pushes to detect any mismatchcherryPick.yml): Exit immediately withexit 1when version bump commits can't be found, with helpful error messages pointing to the recovery workflowsyncVersions.yml): New manual workflow that deployers can trigger to sync versions when they get out of sync (restricted to mobile deployers)Fixed Issues
$ https://github.com/Expensify/Expensify/issues/592093
Tests
This is a GitHub Actions workflow change. Testing requires:
Offline tests
N/A - This is a GitHub Actions workflow change, not a frontend change.
QA Steps
N/A - This is a GitHub Actions infrastructure change that doesn't affect the app UI or functionality directly.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, 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.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
N/A - This is a GitHub Actions workflow change with no UI changes.