fix(ci_visibility): git multi-process issues#18156
Conversation
…vironments In multi-process (xdist) runs each worker invokes git commands concurrently. Two issues caused spurious warnings: 1. Simultaneous `git fetch --update-shallow` calls compete for `.git/shallow.lock`. Fix: retry up to 5 times with exponential back-off + jitter via a new `_call_git_with_lock_retry()` helper. 2. `git merge-base` was called inside `get_env_tags()`, before the repository was unshallowed, so the required commits were not yet available locally. Fix: defer the call to `upload_git_data()`, after any unshallowing has completed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Merge-base is now computed directly via Git.get_merge_base() inside SessionManager._update_pr_merge_base(), so this standalone wrapper is dead code. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Codeowners resolved as |
|
gnufede
changed the title
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Pull the initial call outside the loop so the loop body is only retry logic, eliminating the unreachable return and making control flow obvious. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
gnufede
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06626a1ff0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…its known _update_pr_merge_base() was called after the "all commits already in backend, skip pack upload" early return, so PR sessions where search_commits found every recent commit already known never populated git.pull_request.base_branch_sha. Move the merge-base update before the commits_not_in_backend == 0 check so it runs on every path where unshallowing has already completed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ock TypeError Unshallowing was moved before the commits_not_in_backend==0 early return, which caused tests that mock all commits as known to hit git.get_git_version() >= (2, 27, 0) with a Mock object and raise TypeError. Gate the is_shallow_repository() / get_git_version() block on len(commits_not_in_backend) > 0, restoring the original invariant that unshallowing only runs when there are commits to upload, while keeping _update_pr_merge_base() before the early return. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 838253fc64
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| return | ||
|
|
||
| if git.is_shallow_repository() and git.get_git_version() >= (2, 27, 0): | ||
| if len(commits_not_in_backend) > 0 and git.is_shallow_repository() and git.get_git_version() >= (2, 27, 0): |
There was a problem hiding this comment.
When commits_not_in_backend is empty, this new guard skips unshallowing even if the checkout is shallow, but _update_pr_merge_base() still runs later and calls git merge-base. In shallow CI clones that do not contain the base ancestry, get_merge_base() fails and now emits a warning, so git.pull_request.base_branch_sha is not populated in exactly the “all commits already known” path this change intended to support.
Useful? React with 👍 / 👎.
…nown On shallow clones where all recent commits are already in the backend (commits_not_in_backend == 0), unshallowing was skipped, so _update_pr_merge_base() called git merge-base without the required ancestry and emitted a spurious warning. Remove the commits_not_in_backend > 0 guard from the outer shallow check so the repo is always unshallowed first. Keep the re-check gated on commits_not_in_backend > 0 since there is nothing to re-query when all commits are already known. Add is_shallow_repository.return_value = False to get_mock_git_instance() so tests that use the shared Git mock don't enter the shallow block and hit get_git_version() >= (2, 27, 0) on a Mock object (TypeError). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2 participants
Description
Fixes two sources of spurious git warning logs that customers see when using the pytest plugin under pytest-xdist (or other multi-process) environments.
Problem 1: shallow lock contention
Multiple xdist workers each call
git fetch --update-shallowduring startup. They all compete for.git/shallow.lock, causing repeatedfatal: Unable to create '.../.git/shallow.lock': File existswarnings. Fixed by adding_call_git_with_lock_retry()in git.py, which retries up to 5 times with exponential back-off + jitter when lock contention is detected.Problem 2: merge-base called on a shallow repo
git merge-basewas called insideget_env_tags(), which runs at plugin startup before any unshallowing. On shallow clones the commits aren't locally available yet, so the call always failssilently. Fixed by removing the call from
get_env_tags()and deferring it toSessionManager.upload_git_data(), after unshallowing has completed. The standalone helperget_pr_base_commit_sha()that was the only caller is removed as dead code.Testing
New tests added:
TestGitLockRetry— unit tests for_call_git_with_lock_retry: success path, non-lock errors skip retry, lock error retries then succeeds, retry exhaustion returns last failure,unshallow_repositoryuses the retry helper,get_merge_basedoes not (it's read-only).TestUpdatePrMergeBase— unit tests forSessionManager._update_pr_merge_base: skips when SHA already set, skips when inputs are missing, sets the tag when both SHAs are present, skips whenmerge-base returns empty, and two wiring tests verifying it is called from
upload_git_data()in both shallow and non-shallow scenarios.TestGitUnshallowassertions to reflect that_call_git_with_lock_retrynow passesinput_stringas an explicit second argument to_call_git.No regression tests (real shallow clone + full plugin run) were added; that would require a more involved fixture setup.
Risks
Low. The changes are confined to the pytest plugin internals and only affect CI Visibility data collection, not any tracer hot paths.
PULL_REQUEST_BASE_BRANCH_SHAwas already an optional tag — ifmerge-basefails it is simply absent, same as before.get_pr_base_commit_shais safe; grep confirms it had no callers outside its own definition.