Conversation
Review Summary by Qodo(Agentic_describe updated until commit e386835)Add merge-base lane fingerprint guard for parallel PR race condition detection
WalkthroughsDescription• Implement merge-base lane fingerprint guard to prevent race conditions when multiple parallel PRs modify the same operator/catalog paths • Create snapshot of operator/catalog file-level fingerprints at pipeline start using live target branch tip OID • Verify fingerprint at merge time; block merge if base branch advanced and lane paths changed, with exit code 10 for pipeline re-run • Add CLI tool and Tekton tasks for recording and verifying merge-base snapshots with SSH/HTTPS credential support Diagramflowchart LR
detect["detect-changes"] --> record["record-merge-base-lane-snapshot"]
record --> snapshot["snapshot.json<br/>base_oid + lane_fp"]
snapshot --> merge["merge-pr task"]
merge --> verify["merge-base-lane-fingerprint verify"]
verify -->|tip unchanged| allow["Allow merge"]
verify -->|tip changed, fp match| allow
verify -->|tip changed, fp mismatch| block["Block merge<br/>exit 10"]
File Changes1. operatorcert/merge_base_lane.py
|
Code Review by Qodo
Context used 1. Guard misses pre-snapshot drift
|
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
Persistent review updated to latest commit e386835 |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+110
to
+134
| TIP_REF="refs/heads/$(params.git_base_branch)" | ||
| TIP_OID_REMOTE="$(git ls-remote "${REMOTE_URL}" "${TIP_REF}" | awk '{print $1; exit}')" | ||
| if [[ -z "${TIP_OID_REMOTE}" ]]; then | ||
| echo "ERROR: could not resolve ${TIP_REF} on remote" | ||
| exit 1 | ||
| fi | ||
|
|
||
| # shared git config no matter which base used | ||
| export GIT_CONFIG_COUNT=1 | ||
| export GIT_CONFIG_KEY_0=safe.directory | ||
|
|
||
| if [[ "${TIP_OID_REMOTE}" == "$(params.git_commit_base)" ]]; then | ||
| echo "Live target tip matches git_commit_base; reusing base workspace clone." | ||
| rm -rf "${TIP_CLONE}" | ||
| TIP_CLONE="${BASE_CLONE}" | ||
| TIP_OID="${TIP_OID_REMOTE}" | ||
| # safe.directory for base workspace (merge-base-lane-fingerprint); env only, no $HOME writes. | ||
| export GIT_CONFIG_VALUE_0="${TIP_CLONE}" | ||
| else | ||
| rm -rf "${TIP_CLONE}" | ||
| # safe.directory before git -C on the new clone (ownership vs step user). | ||
| export GIT_CONFIG_VALUE_0="${TIP_CLONE}" | ||
| git clone --depth 1 -b "$(params.git_base_branch)" "${REMOTE_URL}" "${TIP_CLONE}" | ||
| TIP_OID="$(git -C "${TIP_CLONE}" rev-parse HEAD)" | ||
| fi |
There was a problem hiding this comment.
1. Guard misses pre-snapshot drift 📎 Requirement gap ☼ Reliability
The merge-base guard snapshots the live target-branch tip even when it already differs from git_commit_base, and the merge check only guards further drift after that snapshot. This can let the hosted pipeline pass and merge even though another concurrent bundle merge already changed the operator/catalog state the PR was validated against, risking post-merge incompatibility and multiple channel heads.
Agent Prompt
## Issue description
The merge-base lane guard only blocks merges when the base tip changes *after* the snapshot is recorded; if the base tip already advanced away from `git_commit_base` before the snapshot task runs, the pipeline can still proceed/merge without re-validating against the new base state.
## Issue Context
`detect-changes` uses a clone at `git_commit_base`, but the snapshot task intentionally fingerprints the live target-branch tip when it differs from `git_commit_base`. This means concurrent merges that occur earlier in the pipeline run may not be treated as a compatibility risk per the checklist objectives.
## Fix Focus Areas
- README.md[148-164]
- ansible/roles/operator-pipeline/templates/openshift/tasks/record-merge-base-lane-snapshot.yml[79-137]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
Author
There was a problem hiding this comment.
The snapshot step always resolves the live refs/heads/<git_base_branch> tip via ls-remote and records base_oid from that tip (reusing the base workspace only when that OID equals git_commit_base; otherwise it shallow-clones that tip and fingerprints HEAD). merge-base-lane-fingerprint verify compares merge-time ls-remote to base_oid from the snapshot, so drift after the snapshot is detected relative to the tip we actually fingerprinted, including the case where the branch was already ahead of git_commit_base before the snapshot ran.
What stays at git_commit_base is detect-changes and the base workspace used there—that’s the intentional scope called out in the README: lane-level fingerprinting, not a second full validation pass on the newer tip.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes ISV-7093
Handles situation when multiple parallel PRs (and pipelines) are touching the same operator or catalog and merging one of them can make the other PR(s) invalid (e.g. multiple channel heads).
The solution creates fingerprint (hash) of the updated operator/catalog tree-ish in a new task right after
detect-changesand is then used as a guard inmerge-pr:Known limitations: as a naive solution, there is no actual
opm validate, leaving some space for false positives (stated in the PR comment if guarded, pipeline re-run should fix it). This trade off is time effective, regular validation with catalog/index build will introduce another time-window for race condition.Example guarded merge is here.
Merge Request Checklists