Describe the bug
When a previously merged image-updater PR/MR leaves its head branch behind on the remote, the updater attempts to open a new PR/MR against a branch that has no commits ahead of the base, on every reconcile.
skipIfPRExists only looks for open PRs:
- GitHub —
State: "open" (pkg/argocd/pr_github.go:102)
- GitLab —
State: new("opened") (pkg/argocd/mr_gitlab.go:71)
- Azure DevOps —
searchCriteria.status = "active" (pkg/argocd/pr_azuredevops.go:88)
so a merged PR is invisible to it. The head branch still exists on the remote, ShallowFetch succeeds, the write-back finds the target already at the desired value and skips, and the PR API is called anyway.
On GitHub the result is 422 Validation Failed — "No commits between <base> and <head>". isAlreadyExistsError (pkg/argocd/pr_github.go:82) does not match that message, so it propagates: update.go:331-334 logs at ERROR and does result.NumErrors += 1, which feeds metrics.ImageUpdaterCR().IncreaseUpdateErrors (internal/controller/reconcile.go:116) and the CR condition "%d error(s) occurred during image update checks." (internal/controller/status.go:172-184).
Nothing is corrupted and no update is lost — the cost is an unclearable false-positive error state that repeats every cycle, makes the error metric useless for alerting, and buries real write-back failures.
To Reproduce
Requires forceUpdate: true (or the empty .status.summary.images condition described in #1805), writeBackConfig.gitConfig.pullRequest, and a repository without "automatically delete head branches" enabled.
- Let the updater open a PR and merge it. The head branch
image-updater-<targetKey>-<sha256> remains on the remote.
- On the next reconcile
forceUpdate re-enters the write-back path. skipIfPRExists finds no open PR. ShallowFetch of the head branch succeeds, so the branch is not recreated locally. The target already holds the desired tag, so the change writer skips.
- PR creation is attempted against a head with no commits ahead of base and is rejected.
- Repeats indefinitely: the head branch name hashes
imageName-oldTag-newTag (pkg/argocd/git.go:111), which is stable across cycles.
Expected behavior
If the change carried by a content-hashed head branch has already been merged, the updater should recognise it as already landed and skip cleanly, without an error.
Additional context
Priority: medium. This is the sibling of #1805, differing only in a repository setting. That issue's log shows fatal: couldn't find remote ref image-updater-…, i.e. the reporter's head branch was auto-deleted on merge, and their kustomization.yaml "already reads newTag: main-<sha>" is exactly the post-merge state. With branch auto-deletion switched off, the same configuration produces the same 422-every-two-minutes symptom.
#1806 fixes the deleted-branch half of this (commitChangesGit returns pushBranchCreated && skip, so a locally-created branch takes the early return). The merged-but-retained-branch half is not covered: there pushBranchCreated is false, which is correct — a head branch present on the remote must still be able to get a PR so that a create() that failed after a successful push is retried on the next cycle. The two cases cannot be told apart by "did the writer skip".
Reported by @pujitha24 in review of #1806.
Recommended fix
Treat a merged PR/MR for the same content-hashed head as "already landed" in the existence check, alongside open ones. Because the head branch name hashes the image name and the old/new tags, a merged PR with that head means this exact change has already been applied to the base branch, so there is nothing left to propose.
Concretely, in each provider's exists():
- GitHub — list with
State: "all"; report true when a PR is open or has a non-nil MergedAt.
- GitLab — drop the
"opened" filter; report true when mr.State is "opened" or "merged".
- Azure DevOps —
searchCriteria.status = "all"; report true when status is "active" or "completed". Note exists() currently sets $top=1 and decodes into []json.RawMessage counting only the length; it needs to decode status and raise $top, otherwise a closed PR could mask a merged one.
Closed-but-unmerged PRs must deliberately not count. Today those are reopened on the next cycle, and preserving that keeps this change a pure bug fix. Whether to suppress deliberately-closed PRs is a separate product decision and should not ride along here.
This also short-circuits in skipIfPRExists before the clone, so the cycle gets cheaper rather than just quieter.
Why not per-provider error mapping
The alternative is to classify the rejection after the fact, the way isAlreadyExistsError / ErrMRAlreadyExists / GitPullRequestExistsException already do. That was considered and rejected:
- The three providers signal "already exists" three different ways — GitHub by substring match on a 422 message, GitLab by HTTP 409, Azure DevOps by JSON
typeKey — so there is no shared helper to extend and each needs its own brittle matcher.
- It only converts the error to a no-op; the wasted clone, fetch, checkout and API call still happen every cycle.
- It is unclear that GitLab errors at all here. GitLab may accept an MR with no commits, in which case the current code creates a junk empty MR rather than failing — a different bug needing a different fix. This needs verifying against a real instance before either approach is considered complete.
Test
Per provider, with the change writer reporting no changes and ShallowFetch succeeding for the head branch (so pushBranchCreated is false):
Version
master, including #1806.
Regression
PR/MR mode is CRD-only, so only CRD-based configuration is affected.
Describe the bug
When a previously merged image-updater PR/MR leaves its head branch behind on the remote, the updater attempts to open a new PR/MR against a branch that has no commits ahead of the base, on every reconcile.
skipIfPRExistsonly looks for open PRs:State: "open"(pkg/argocd/pr_github.go:102)State: new("opened")(pkg/argocd/mr_gitlab.go:71)searchCriteria.status = "active"(pkg/argocd/pr_azuredevops.go:88)so a merged PR is invisible to it. The head branch still exists on the remote,
ShallowFetchsucceeds, the write-back finds the target already at the desired value and skips, and the PR API is called anyway.On GitHub the result is
422 Validation Failed — "No commits between <base> and <head>".isAlreadyExistsError(pkg/argocd/pr_github.go:82) does not match that message, so it propagates:update.go:331-334logs at ERROR and doesresult.NumErrors += 1, which feedsmetrics.ImageUpdaterCR().IncreaseUpdateErrors(internal/controller/reconcile.go:116) and the CR condition"%d error(s) occurred during image update checks."(internal/controller/status.go:172-184).Nothing is corrupted and no update is lost — the cost is an unclearable false-positive error state that repeats every cycle, makes the error metric useless for alerting, and buries real write-back failures.
To Reproduce
Requires
forceUpdate: true(or the empty.status.summary.imagescondition described in #1805),writeBackConfig.gitConfig.pullRequest, and a repository without "automatically delete head branches" enabled.image-updater-<targetKey>-<sha256>remains on the remote.forceUpdatere-enters the write-back path.skipIfPRExistsfinds no open PR.ShallowFetchof the head branch succeeds, so the branch is not recreated locally. The target already holds the desired tag, so the change writer skips.imageName-oldTag-newTag(pkg/argocd/git.go:111), which is stable across cycles.Expected behavior
If the change carried by a content-hashed head branch has already been merged, the updater should recognise it as already landed and skip cleanly, without an error.
Additional context
Priority: medium. This is the sibling of #1805, differing only in a repository setting. That issue's log shows
fatal: couldn't find remote ref image-updater-…, i.e. the reporter's head branch was auto-deleted on merge, and theirkustomization.yaml"already readsnewTag: main-<sha>" is exactly the post-merge state. With branch auto-deletion switched off, the same configuration produces the same 422-every-two-minutes symptom.#1806 fixes the deleted-branch half of this (
commitChangesGitreturnspushBranchCreated && skip, so a locally-created branch takes the early return). The merged-but-retained-branch half is not covered: therepushBranchCreatedis false, which is correct — a head branch present on the remote must still be able to get a PR so that acreate()that failed after a successful push is retried on the next cycle. The two cases cannot be told apart by "did the writer skip".Reported by @pujitha24 in review of #1806.
Recommended fix
Treat a merged PR/MR for the same content-hashed head as "already landed" in the existence check, alongside open ones. Because the head branch name hashes the image name and the old/new tags, a merged PR with that head means this exact change has already been applied to the base branch, so there is nothing left to propose.
Concretely, in each provider's
exists():State: "all"; report true when a PR isopenor has a non-nilMergedAt."opened"filter; report true whenmr.Stateis"opened"or"merged".searchCriteria.status = "all"; report true whenstatusis"active"or"completed". Noteexists()currently sets$top=1and decodes into[]json.RawMessagecounting only the length; it needs to decodestatusand raise$top, otherwise a closed PR could mask a merged one.Closed-but-unmerged PRs must deliberately not count. Today those are reopened on the next cycle, and preserving that keeps this change a pure bug fix. Whether to suppress deliberately-closed PRs is a separate product decision and should not ride along here.
This also short-circuits in
skipIfPRExistsbefore the clone, so the cycle gets cheaper rather than just quieter.Why not per-provider error mapping
The alternative is to classify the rejection after the fact, the way
isAlreadyExistsError/ErrMRAlreadyExists/GitPullRequestExistsExceptionalready do. That was considered and rejected:typeKey— so there is no shared helper to extend and each needs its own brittle matcher.Test
Per provider, with the change writer reporting no changes and
ShallowFetchsucceeding for the head branch (sopushBranchCreatedis false):Version
master, including #1806.Regression
0, e.g.,0.17.0)1, e.g.,1.0.0)PR/MR mode is CRD-only, so only CRD-based configuration is affected.