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

[JacekLach]

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:

release-please/src/github.ts

Line 148 in 05c6a4f

this.fileCache = new RepositoryFileCache(this.octokit, this.repository);

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

release-please/src/github.ts

Lines 604 to 617 in 05c6a4f

	async getFileContentsOnBranch(
	path: string,
	branch: string
	): Promise<GitHubFileContents> {
	this.logger.debug(`Fetching ${path} from branch ${branch}`);
	try {
	return await this.fileCache.getFileContents(path, branch);
	} catch (e) {
	if (e instanceof MissingFileError) {
	throw new FileNotFoundError(path);
	}
	throw e;
	}
	}

​
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:

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:

release-please/src/github.ts

Lines 838 to 879 in 05c6a4f

	async buildChangeSet(
	updates: Update[],
	defaultBranch: string
	): Promise<ChangeSet> {
	// Sometimes multiple updates are proposed for the same file,
	// such as when the manifest file is additionally changed by the
	// node-workspace plugin. We need to merge these updates.
	const mergedUpdates = mergeUpdates(updates);
	const changes = new Map();
	for (const update of mergedUpdates) {
	let content: GitHubFileContents | undefined;
	try {
	content = await this.getFileContentsOnBranch(
	update.path,
	defaultBranch
	);
	} catch (err) {
	if (!(err instanceof FileNotFoundError)) throw err;
	// if the file is missing and create = false, just continue
	// to the next update, otherwise create the file.
	if (!update.createIfMissing) {
	this.logger.warn(`file ${update.path} did not exist`);
	continue;
	}
	}
	const contentText = content
	? Buffer.from(content.content, 'base64').toString('utf8')
	: undefined;
	const updatedContent = update.updater.updateContent(
	contentText,
	this.logger
	);
	if (updatedContent) {
	changes.set(update.path, {
	content: updatedContent,
	originalContent: content?.parsedContent || null,
	mode: content?.mode || DEFAULT_FILE_MODE,
	});
	}
	}
	return changes;
	}

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

release-please/src/github.ts

Lines 725 to 745 in 05c6a4f

	async createPullRequest(
	pullRequest: PullRequest,
	targetBranch: string,
	message: string,
	updates: Update[],
	options?: CreatePullRequestOptions
	): Promise<PullRequest> {
	const changes = await this.buildChangeSet(updates, targetBranch);
	const prNumber = await suggesterCreatePullRequest(this.octokit, changes, {
	upstreamOwner: this.repository.owner,
	upstreamRepo: this.repository.repo,
	title: pullRequest.title,
	branch: pullRequest.headBranchName,
	description: pullRequest.body,
	primary: targetBranch,
	force: true,
	fork: !!options?.fork,
	message,
	logger: this.logger,
	draft: !!options?.draft,
	labels: pullRequest.labels,

release-please/src/github.ts

Lines 784 to 818 in 05c6a4f

	async updatePullRequest(
	number: number,
	releasePullRequest: ReleasePullRequest,
	targetBranch: string,
	options?: ScmUpdatePullRequestOptions
	): Promise<PullRequest> {
	const changes = await this.buildChangeSet(
	releasePullRequest.updates,
	targetBranch
	);
	let message = releasePullRequest.title.toString();
	if (options?.signoffUser) {
	message = signoffCommitMessage(message, options.signoffUser);
	}
	const title = releasePullRequest.title.toString();
	const body = (
	options?.pullRequestOverflowHandler
	? await options.pullRequestOverflowHandler.handleOverflow(
	releasePullRequest
	)
	: releasePullRequest.body
	)
	.toString()
	.slice(0, MAX_ISSUE_BODY_SIZE);
	const prNumber = await suggesterCreatePullRequest(this.octokit, changes, {
	upstreamOwner: this.repository.owner,
	upstreamRepo: this.repository.repo,
	title,
	branch: releasePullRequest.headRefName,
	description: body,
	primary: targetBranch,
	force: true,
	fork: options?.fork === false ? false : true,
	message,

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

release-please/src/util/code-suggester/github/branch.ts

Lines 122 to 143 in 05c6a4f

	export async function branch(
	octokit: Octokit,
	origin: RepoDomain,
	upstream: RepoDomain,
	name: string,
	baseBranch: string = DEFAULT_PRIMARY_BRANCH
	): Promise<string> {
	// create branch from primary branch HEAD SHA
	try {
	const baseSha = await getBranchHead(octokit, upstream, baseBranch);
	const duplicate: boolean = await existsBranchWithName(
	octokit,
	origin,
	name
	);
	await createBranch(octokit, origin, name, baseSha, duplicate);
	return baseSha;
	} catch (err) {
	logger.error('Error when creating branch');
	throw err;
	}
	}

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.