Skip to content

Race condition on merge? #15

@jonchurch

Description

@jonchurch

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 jonchurch/semversyphus#6 and then also saw jonchurch/semversyphus#7 opened.

Checking the action logs, what I think happened is that the pr job saw a push (the merge commit from jonchurch/semversyphus#6), it ran isReleaseMergeCommit 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

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. 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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions