Create PR comment on branch update failures

[fgblomqvist]

What Renovate type, platform and version are you using?

Latest, self-hosted Renovate, on GitLab.

Describe the bug

If Renovate fails to update a branch for an MR, it doesn't add a comment to that MR.
E.g. I think the expected behavior would be this:

1. Renovate creates a branch and PR successfully, all is good
2. On another run, it wants to make changes to the branch
3. It fails to make those changes (irrelevant of what the error is)
4. Now it needs to check if an PR already exists, and if it does, add a comment explaining that it failed to update the branch so the MR should probably not be merged as-is (similar to the artifact update error).

At the moment, Renovate just returns if it fails with any of the branch updating logic:

renovate/lib/workers/branch/index.ts

Lines 581 to 653 in 5f84737

	} catch (err) /* istanbul ignore next */ {
	if (err.statusCode === 404) {
	logger.debug({ err }, 'Received a 404 error - aborting run');
	throw new Error(REPOSITORY_CHANGED);
	}
	if (err.message === PLATFORM_RATE_LIMIT_EXCEEDED) {
	logger.debug('Passing rate-limit-exceeded error up');
	throw err;
	}
	if (err.message === REPOSITORY_CHANGED) {
	logger.debug('Passing repository-changed error up');
	throw err;
	}
	if (err.message?.startsWith('remote: Invalid username or password')) {
	logger.debug('Throwing bad credentials');
	throw new Error(PLATFORM_BAD_CREDENTIALS);
	}
	if (
	err.message?.startsWith(
	'ssh_exchange_identification: Connection closed by remote host'
	)
	) {
	logger.debug('Throwing bad credentials');
	throw new Error(PLATFORM_BAD_CREDENTIALS);
	}
	if (err.message === PLATFORM_BAD_CREDENTIALS) {
	logger.debug('Passing bad-credentials error up');
	throw err;
	}
	if (err.message === PLATFORM_INTEGRATION_UNAUTHORIZED) {
	logger.debug('Passing integration-unauthorized error up');
	throw err;
	}
	if (err.message === MANAGER_LOCKFILE_ERROR) {
	logger.debug('Passing lockfile-error up');
	throw err;
	}
	if (err.message?.includes('space left on device')) {
	throw new Error(SYSTEM_INSUFFICIENT_DISK_SPACE);
	}
	if (err.message === SYSTEM_INSUFFICIENT_DISK_SPACE) {
	logger.debug('Passing disk-space error up');
	throw err;
	}
	if (err.message.startsWith('Resource not accessible by integration')) {
	logger.debug('Passing 403 error up');
	throw err;
	}
	if (err.message === WORKER_FILE_UPDATE_FAILED) {
	logger.warn('Error updating branch: update failure');
	} else if (err.message.startsWith('bundler-')) {
	// we have already warned inside the bundler artifacts error handling, so just return
	return ProcessBranchResult.Error;
	} else if (
	err.messagee &&
	err.message.includes('fatal: Authentication failed')
	) {
	throw new Error(PLATFORM_AUTHENTICATION_ERROR);
	} else if (err.message?.includes('fatal: bad revision')) {
	logger.debug({ err }, 'Aborting job due to bad revision error');
	throw new Error(REPOSITORY_CHANGED);
	} else if (err.message === CONFIG_VALIDATION) {
	logger.debug('Passing config validation error up');
	throw err;
	} else if (err.message === TEMPORARY_ERROR) {
	logger.debug('Passing TEMPORARY_ERROR error up');
	throw err;
	} else if (!(err instanceof ExternalHostError)) {
	logger.warn({ err }, `Error updating branch`);
	}
	// Don't throw here - we don't want to stop the other renovations
	return ProcessBranchResult.Error;
	}

I think the way to approach this would be to refactor this processing a bit. First of all, probably move out the check for whether a PR already exists or not. Then you'd at least need to identify which errors to simply return on (things like invalid credentials and repo 404's). Finally, on all other errors, attempt to post the error to the PR (if it exists). Something like that.

Relevant debug logs
Here's an example that involves an LFSd package-lock.json, however Renovate's docker image does not yet support LFS so GitLab returns an error when Renovate tries to push a commit for this file:

Click me to see logs

[07:11:30] DEBUG: 2 file(s) to commit (repository=<snip>, branch=<snip>)
[07:11:30] DEBUG: Committing files to branch <snip> (repository=<snip>, branch=<snip>)
[07:11:31] DEBUG: git commit (repository=<snip>, branch=<snip>)
[07:11:31] "result": {
[07:11:31] "author": null,
[07:11:31] "branch": "<snip>",
[07:11:31] "commit": "26c827ab",
[07:11:31] "root": false,
[07:11:31] "summary": {"changes": 2, "insertions": 44781, "deletions": 85}
[07:11:31] }
[07:11:35] DEBUG: Unknown error committing files (repository=<snip>, branch=<snip>)
[07:11:35] "err": {
[07:11:35] "task": {
[07:11:35] "commands": [
[07:11:35] "push",
[07:11:35] "origin",
[07:11:35] "<snip>:<snip>",
[07:11:35] "--force",
[07:11:35] "-u",
[07:11:35] "--no-verify",
[07:11:35] "--verbose",
[07:11:35] "--porcelain"
[07:11:35] ],
[07:11:35] "format": "utf-8"
[07:11:35] },
[07:11:35] "message": "Pushing to <snip>\nPOST git-receive-pack (237751 bytes)\nremote: GitLab: File \"<snip>/package-lock.json\" is larger than the allowed size of 1 MB\nerror: failed to push some refs to '<snip>'\n",
[07:11:35] "stack": "Error: Pushing to <snip>\nPOST git-receive-pack (237751 bytes)\nremote: GitLab: File \"<snip>/package-lock.json\" is larger than the allowed size of 1 MB\nerror: failed to push some refs to '<snip>'\n\n    at GitExecutorChain.onFatalException (/usr/src/app/node_modules/simple-git/src/lib/runners/git-executor-chain.ts:76:77)\n    at GitExecutorChain.<anonymous> (/usr/src/app/node_modules/simple-git/src/lib/runners/git-executor-chain.ts:68:21)\n    at Generator.throw (<anonymous>)\n    at rejected (/usr/src/app/node_modules/simple-git/src/lib/runners/git-executor-chain.js:6:65)\n    at runMicrotasks (<anonymous>)\n    at processTicksAndRejections (internal/process/task_queues.js:93:5)"
[07:11:35] }
[07:11:35] WARN: Error updating branch (repository=<snip>, branch=<snip>)
[07:11:35] "err": {
[07:11:35] "task": {
[07:11:35] "commands": [
[07:11:35] "push",
[07:11:35] "origin",
[07:11:35] "<snip>:<snip>",
[07:11:35] "--force",
[07:11:35] "-u",
[07:11:35] "--no-verify",
[07:11:35] "--verbose",
[07:11:35] "--porcelain"
[07:11:35] ],
[07:11:35] "format": "utf-8"
[07:11:35] },
[07:11:35] "message": "Pushing to <snip>\nPOST git-receive-pack (237751 bytes)\nremote: GitLab: File \"<snip>/package-lock.json\" is larger than the allowed size of 1 MB\nerror: failed to push some refs to '<snip>'\n",
[07:11:35] "stack": "Error: Pushing to <snip>\nPOST git-receive-pack (237751 bytes)\nremote: GitLab: File \"<snip>/package-lock.json\" is larger than the allowed size of 1 MB\nerror: failed to push some refs to '<snip>'\n\n    at GitExecutorChain.onFatalException (/usr/src/app/node_modules/simple-git/src/lib/runners/git-executor-chain.ts:76:77)\n    at GitExecutorChain.<anonymous> (/usr/src/app/node_modules/simple-git/src/lib/runners/git-executor-chain.ts:68:21)\n    at Generator.throw (<anonymous>)\n    at rejected (/usr/src/app/node_modules/simple-git/src/lib/runners/git-executor-chain.js:6:65)\n    at runMicrotasks (<anonymous>)\n    at processTicksAndRejections (internal/process/task_queues.js:93:5)"
[07:11:35] }

Have you created a minimal reproduction repository?

• I have provided a minimal reproduction repository
• I don't have time for that, but it happens in a public repository I have linked to
• I don't have time for that, and cannot share my private repository
• The nature of this bug means it's impossible to reproduce publicly

Additional context

I don't think it should be necessary to create a repro repo. I think the relevant logic is here:

renovate/lib/workers/branch/index.ts

Lines 581 to 653 in 5f84737

	} catch (err) /* istanbul ignore next */ {
	if (err.statusCode === 404) {
	logger.debug({ err }, 'Received a 404 error - aborting run');
	throw new Error(REPOSITORY_CHANGED);
	}
	if (err.message === PLATFORM_RATE_LIMIT_EXCEEDED) {
	logger.debug('Passing rate-limit-exceeded error up');
	throw err;
	}
	if (err.message === REPOSITORY_CHANGED) {
	logger.debug('Passing repository-changed error up');
	throw err;
	}
	if (err.message?.startsWith('remote: Invalid username or password')) {
	logger.debug('Throwing bad credentials');
	throw new Error(PLATFORM_BAD_CREDENTIALS);
	}
	if (
	err.message?.startsWith(
	'ssh_exchange_identification: Connection closed by remote host'
	)
	) {
	logger.debug('Throwing bad credentials');
	throw new Error(PLATFORM_BAD_CREDENTIALS);
	}
	if (err.message === PLATFORM_BAD_CREDENTIALS) {
	logger.debug('Passing bad-credentials error up');
	throw err;
	}
	if (err.message === PLATFORM_INTEGRATION_UNAUTHORIZED) {
	logger.debug('Passing integration-unauthorized error up');
	throw err;
	}
	if (err.message === MANAGER_LOCKFILE_ERROR) {
	logger.debug('Passing lockfile-error up');
	throw err;
	}
	if (err.message?.includes('space left on device')) {
	throw new Error(SYSTEM_INSUFFICIENT_DISK_SPACE);
	}
	if (err.message === SYSTEM_INSUFFICIENT_DISK_SPACE) {
	logger.debug('Passing disk-space error up');
	throw err;
	}
	if (err.message.startsWith('Resource not accessible by integration')) {
	logger.debug('Passing 403 error up');
	throw err;
	}
	if (err.message === WORKER_FILE_UPDATE_FAILED) {
	logger.warn('Error updating branch: update failure');
	} else if (err.message.startsWith('bundler-')) {
	// we have already warned inside the bundler artifacts error handling, so just return
	return ProcessBranchResult.Error;
	} else if (
	err.messagee &&
	err.message.includes('fatal: Authentication failed')
	) {
	throw new Error(PLATFORM_AUTHENTICATION_ERROR);
	} else if (err.message?.includes('fatal: bad revision')) {
	logger.debug({ err }, 'Aborting job due to bad revision error');
	throw new Error(REPOSITORY_CHANGED);
	} else if (err.message === CONFIG_VALIDATION) {
	logger.debug('Passing config validation error up');
	throw err;
	} else if (err.message === TEMPORARY_ERROR) {
	logger.debug('Passing TEMPORARY_ERROR error up');
	throw err;
	} else if (!(err instanceof ExternalHostError)) {
	logger.warn({ err }, `Error updating branch`);
	}
	// Don't throw here - we don't want to stop the other renovations
	return ProcessBranchResult.Error;
	}

I guess you could argue that this is more of a feature request than a bug report, so feel free to change if needed.

I'm not sure when Renovate decides to add comments, but if it e.g. fails to update an Artifact it does (which is very helpful). I think it would make sense/be expected that Renovate adds a comment in any scenario where it didn't successfully manipulate/create/delete the MR. Otherwise, the MR could look good and might be merged, even though something is wrong with it. If you check the Dep Dashboard, you will see that this MR had an error (but you can't expect everyone to do that, and some people don't even have the Dashboard enabled).

[fgblomqvist]

@viceice This is not a duplicate of those since this is about the fact that Renovate doesn't put a comment when it fails to update a branch. The error could be anything. What if GitLab returns "Error: Something went wrong"? It doesn't matter if the error is LFS-related or not, the behavior is still the same 🙂 .

[fgblomqvist]

To elaborate, this is the scenario:

1. Renovate creates a branch and MR successfully, all is good
2. On another run, it wants to make changes to the branch
3. It fails to make those changes
4. Now it needs to check if an MR already exists, and if it does, add a comment explaining that it failed to update the branch so the MR should probably not be merged

EDIT: Added this to the issue description.

[viceice]

Sure but the cause is the same. So feel free to add another pr to handle this specific git error in our git layer. 🤗

[viceice]

You should at least see a warning on the dashboard

[fgblomqvist]

Sure but the cause is the same. So feel free to add another pr to handle this specific git error in our git layer. 🤗

But what about the general case? Are you saying that for general errors it's better to assume that users will verify on the dashboard whether there are any errors with the MR? 🤔

[viceice]

Ok, can you please update the PR description to reflect what should be happen? Then reopen the issue.

[fgblomqvist]

Updated the description, let me know what you think! Idk if this is an easy task or not, but it may not be too bad.

[rarkins]

Should be easy to find and add the comment. The challenge may be removing it after without adding unnecessary API calls in the other 99.9% of cases