Race condition can cause big checklists

[roryabraham]

Problem

Sometimes we get big checklists like this one that include all the PRs from the previous checklist.

The root cause is a race condition between:

• createNewStagingDeployCash
• deployStaging

Both are triggered at the same time after updateStaging finishes. If deployStaging finishes (i.e: the tag for the new staging version is pushed to the remote) before the checklist is created, then it works. If not, then we get a huge checklist like this will all the PRs that were deployed in the previous version.

Solution

TBD – probably change the deploy system a bit such that staging deploys are triggered by the creation of a prerelease and production deploys by the creation of a "full" release.

That way we can push the tag as soon as it's created without triggering a deploy

[roryabraham]

No update right now

[roryabraham]

No update

[mountiny]

Looking into this one now

Noting that the createNewDeployChecklist was moved to the platformDeploy.yml, which feels like the race condition is not happening anymore and the bug might be somewhere else.

When the updateStaging finished, it force pushes to staging branch and then the deploy.yml runs which pushes the tags and finally we trigger the platformDeploy.yml which would create the checklist but the correct tags should be available

Looking more into why this is failing

[mountiny]

Created a PR which should hopefully help with this

[roryabraham]

Ok, it seems like there's some way the git history can change in production that's different from what we have represented in the CIGitLogicTest.sh, just judging by running this command on staging w/ full git history:

Neither is correct, because the only version-bump that we expect to be included between 1.3.76-6 and 1.3.77-0 is Update version to 1.3.77-0

[roryabraham]

It looks like the last checklist that was created without being affected by this error is this one, so we should try to figure out what was different between 1.3.74-3 and 1.3.75-0 vs 1.3.76-6 and 1.3.77-0

[roryabraham]

Looks like the history is maybe the same?

[melvin-bot]

This issue has not been updated in over 15 days. @roryabraham eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[Beamanator]

FYI I wrote a handy little JS script to mark off items in the new checklist that were on the old one: #30560 (comment) - just a temporary workaround

[roryabraham]

Moving this back to weekly since it's a PITA

[roryabraham]

Planning to investigate this further this week

[roryabraham]

I have a hunch that this has something to do with force-pushes.

[roryabraham]

Spent several hours trying to dig into this, but so far no success in finding a minimal reproduction. I tried something like this:

1. Create a PR A
2. Create a PR B that conflicts with A
3. Merge PR B
4. Release app
5. Merge main into pr-A branch and resolve conflicts
6. Merge PR A

That worked as expected, so I think the next variation should be something like this:

1. Create PR A
2. Create PR B that conflicts with A
3. Merge PR B
4. Release app
5. Rebase pr-A branch and resolve conflicts. Force-push to PR A
6. Merge PR A

My thinking is that by rebasing onto a commit from the previous release then force-pushing we'll somehow include the commits from PR B in the new checklist

[roryabraham]

The other thing maybe worth looking into is that our tests always checkout 1 commit on main before fetching the other tags used in the git log command.

Not sure if that matters, but it's a difference between our local testing setup and the production environment (this may change if/when I create a new callable workflow to wrap createOrUpdateStagingDeploy so we can retry / refresh it at will)

[roryabraham]

This should be fixed by #31458. I hope so 😅

[roryabraham]

That didn't work perfectly: https://github.com/Expensify/App/actions/runs/6910552568/job/18803863115. Going to retry

[github-actions]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[melvin-bot]

📣 @github-actions[bot]! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Make sure you've read and understood the contributing guidelines.
2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[roryabraham]

Nice, that worked as expected (well, maybe not the upwork bit, that's weird).

Anyways, for context I'm marking this as a deploy blocker not because we actually need to block deploy on it, but because I think we should merge #31458 before the next deploy, because it should fix this big checklist issue