Race condition on merge?
- Dominant language
- TypeScript
- Stars
- 171
- Forks
- 7
- Avg merge
- 1d 11h
- Merged PRs (30d)
- 8
Description
I ran into something quirky but I haven't been able to reproduce it again (have tried only like 3 times).
**tldr; there could be a race condition on the `pr` job trigger after a `release/v*` PR merge between Github updating the API, and the order of the `pr` and `release` jobs completing**
Specifically, I merged https://github.com/jonchurch/semversyphus/pull/6 and then also saw https://github.com/jonchurch/semversyphus/pull/7 opened.
Checking the action logs, what I think happened is that the pr job saw a push (the merge commit from https://github.com/jonchurch/semversyphus/pull/6), it ran [`isReleaseMergeCommit`](https://github.com/danielroe/uppt/blob/d3f301672a592a0889617ca1c5f2c625dd31875b/scripts/update-changelog.ts#L281) which hits the GH API to check for a PR associated with said merge.
It must have either hit an error (which is swallowed in the catch block) or not received a result from the API (aka the db row wasnt wired up yet or eventual consistency hadnt caught up yet).
The `pr` job will also attempt to check if there are zero commits between the latest tag and the HEAD so it can bail, which will be true if the `release` job has already pushed the latest tag, but false in the bug I saw bc the release job hadnt pushed the new tag yet when the pr job read the tag list. Which is a second race.
Finally, `pr` job said "okay, lets open/update a PR since this isnt a release merge and I see a diff between HEAD and latest tag".
Bingo bango, as they say
## corkboard with red string
- HEAD in [that pr job run](https://github.com/jonchurch/semversyphus/actions/runs/26316586457) was `9b1897ce4e5a4028fed52cf1b90c693584df1a8c`, which is the `merge_commit_sha` of #6. So uppt was looking at the right commit and didn't recognize it.
- That run's log skipped the `HEAD (xxx) is the merge of a release PR; skipping.` line and went straight to `Current: 3.2.0 -> 3.3.0 (minor)` and `Created PR #7`. Only possible if `isReleaseMergeCommit` returned false.
- PR #7's body uses `v3.1.5...release/v3.3.0` in the compare URL, the same `v3.1.5` base #6 uses. So the `v3.2.0` tag the parallel `release` job was supposed to push wasn't visible to the pr job either. So there's a race between the two jobs as well!
- The 2 release PRs I merged after this (#8, #9) both correctly hit the skip path. AKA I couldnt repro it on demand, so Im either mistaken or there's something racey happening.
## the job to be done
The `pr` job basically does one thing: opens or updates a PR for every push it sees. Very simple, ignoring the changelog bits.
But the **real job** is more like "open or update a PR for every push I see EXCEPT for ones generated by some merge strategy to integrate `release/v*` branches". Which is, surprisingly, a lot harder.
Without Github as a DB layer, I dont even know if determining that w/ git alone is possible for all merge strategies. I tried to think of how to remove the API lookup entirely and I dont love the answers (sentinel string in the commit message at merge time I'm just going to sail right past, doesnt feel clean).
## proposal
Well I do like one answer, but it requires reframing the question.
The current code asks *"was this push caused by a release-PR merge?"* which inherently requires reconstructing the past (commit metadata, merge-strategy reasoning) or the API lookup that I believe is racey.
The lateral question *"is the repo currently in a state where a release is mid-flight?"* might be answerable from reading the pkg.version and git tags.
Beating the race in the API reduces to one check, are pkg version and latest tag out of sync:
> **`pkg.version !== latest tag` → skip**
This is true between when a release PR's merge updates `package.json` on main and when the `release` job pushes the corresponding tag. Its still a race condition, I think those two jobs will always race, but it is one that can be coded around. (it may also be true if someone is doing manual release prep, updating pkg.version without uppt, in which case we'd also want uppt to ignore that commit IMO)
The other side of the race - tag is pushed before pr job runs - is already handled by the existing [`!commits.length` early exit](https://github.com/danielroe/uppt/blob/d3f301672a592a0889617ca1c5f2c625dd31875b/scripts/update-changelog.ts#L313). When the tag points to HEAD, `getCommitsSince(tag)` returns empty and uppt exits with `No release-worthy commits since v3.x.x`. Both halves of the race are covered, and `isReleaseMergeCommit`'s racey API call can be dropped.
Contributor guide
Research direction
Start in scripts/update-changelog.ts, especially isReleaseMergeCommit and the pr/release job flow, then inspect the linked action run and merge details. Compare the package version with the latest git tag during the interval described in the issue, while preserving the existing no-commits early exit. Done means release-merge pushes are not opened as duplicate PRs despite either job completing first.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- github-actions, typescript
- Domain
- ci-cd, release
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 45/100