googleapis / googleapis/release-please

Release PR can silently revert file changes if the target branch advances mid-run

Open
#2,890 0 comments 0 reactions 0 assignees View on GitHub
priority: p3 type: bug
Dominant language
TypeScript
Stars
7.5k
Forks
588
Avg merge
12h 16m
Merged PRs (30d)
7

Description

#### Summary

If the target branch advances while release-please is running, the resulting release PR can contain *out-of-date content* for *every file release-please rewrites*, while its changelog and PR base commit are created on top of the newest develop. This is in effect a silent revert of whatever the concurrent changes to those files were.

Because release-please creates the PR on top of the concurrent merge, this does not show up as a merge conflict; release-please writes whole-file blobs rather than patches, so git does not see this as a three-way merge but instead a 'hand-written' revert.

Initially found on release-please 17.6.0 (via release-please-action@v5), against a repository using a GitHub merge queue.

#### Impact

The severity depends on which files release-please is configured to write. Changelogs and .release-please-manifest.json are usually owned by release-please, so stale reads there are cosmetic. But extra-files entries and the node/rust/etc. strategies point release-please at manifests that contributors edit constantly — Cargo.toml, package.json, pom.xml. A stale read of one of those silently reverts a contributor's change to that file.

If the separately merged PR runs its own release-please action before the pending PR is merged, it will undo this revert and everything is well. However, if it either fails to run the action, or the pending release PR is merged quickly, the revert becomes permanent.

#### Environment details
- `release-please` version: 17.6.0

#### Steps to reproduce
I will use 'develop' for target branch name, commit N (in develop) as the commit the release-please action is running on, and commit N+1 as the next merged commit in develop (newest commit in develop when the action completes). Commit N+1 contains independent changes to a file that release-please modifies (like package.json or Cargo.toml)

1. Trigger a release-please run on commit N
2. While release-please is running, between the step where it fetches and caches file content [code reference, log line?]; and the step where it writes the changeset and creates a PR, merge commit N+1 into develop
3. Observe that the resulting PR reverts the change from N+1 instead of simply bumping versions

#### Root cause

Root cause

1. Reads go through a cache keyed by the branch name, so the first read pins a tree for the whole run.
GitHub constructs one RepositoryFileCache per instance, so its lifetime is the run:

https://github.com/googleapis/release-please/blob/05c6a4f71022304d4edad24ea90c1c16324503d5/src/github.ts#L148

Every file read goes through it, keyed by a branch *name*:

https://github.com/googleapis/release-please/blob/05c6a4f71022304d4edad24ea90c1c16324503d5/src/github.ts#L604-L617

In `@google-automations/git-file-utils` (3.2.0 at HEAD, 3.0.0 at 17.6.0 — identical in both), `BranchFileCache` resolves every path through a cached tree, and `getTree` caches by the string it is handed. [googleapis/repo-automation-bots](https://github.com/googleapis/repo-automation-bots):

```js
async fetchFileContents(path) {
const treeEntries = await this.getFullTree(); // → getTree(this.branch)
const found = treeEntries.find(e => e.path === path);
return await this.fetchContents(found.sha, found); // blob SHA from the cached tree
}

async getTree(sha) {
const cached = this.treeCache.get(sha); // key is "main", not a commit SHA
if (cached) return cached;
...
}
```

Because the key is a moving ref, the first read of any file fixes the tree — and therefore the blob SHAs — for every later read in the run. The library itself is fine; caching by SHA is correct. The problem is that release-please hands it a branch name.

2. The commit base is resolved separately, and later.

`buildChangeSet` takes a branch *name*, reads each file through the cache above, and stores the updater's output as a **whole-file blob** — there is no patch anywhere:

https://github.com/googleapis/release-please/blob/05c6a4f71022304d4edad24ea90c1c16324503d5/src/github.ts#L838-L879

`createPullRequest` and `updatePullRequest` pass `targetBranch` to both `buildChangeSet` and code-suggester's `primary`:

https://github.com/googleapis/release-please/blob/05c6a4f71022304d4edad24ea90c1c16324503d5/src/github.ts#L725-L745

https://github.com/googleapis/release-please/blob/05c6a4f71022304d4edad24ea90c1c16324503d5/src/github.ts#L784-L818

code-suggester then resolves the base independently, at line 131, and `commitAndPush` commits on top of whatever that returns:

https://github.com/googleapis/release-please/blob/05c6a4f71022304d4edad24ea90c1c16324503d5/src/util/code-suggester/github/branch.ts#L122-L143

So reads and the base can disagree by any number of commits, and the changeset overwrites files wholesale on top of that base.

#### Suggested resolution

I see two ways forward:

1. You could pin the develop reference at start of the run, and create the release-please action against that base. This leads to a PR that does not revert any third-party changes, but it might skip changelog generation for the merged 'N+1' PR. The generated release PR will be mergable if the conflicts can be auto-resolved, but merging it will put a release on 'top' of N+1 that doesnt actually include its changelog. However, this issue is unavoidable; if N+1 merges _after_ the release PR is created, and then the release PR is merged before it's replaced by a new release-please run, you get the same issue

2. You could fail the release-please run if the develop reference has moved between start of run and when it attempts to create the PR.

Contributor guide

Open the contributing guide

Research direction

Start in src/github.ts at buildChangeSet, createPullRequest, and updatePullRequest, then follow the base resolution in src/util/code-suggester/github/branch.ts. Reproduce the race with a moving target branch and compare the cached file reads with the commit base. Done means concurrent branch advancement no longer silently overwrites unrelated file changes, with regression coverage for the observed scenario.

Written by the indexing model from the issue text.

Assessment

Tech stack
git, github, typescript
Domain
ci-cd, devtools, release
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
38/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.