Skip to content

fix(ci): scope minimize-reviews concurrency by event name#4174

Merged
jlowin merged 3 commits into
PrefectHQ:mainfrom
strawgate:fix-minimize-reviews-concurrency
May 20, 2026
Merged

fix(ci): scope minimize-reviews concurrency by event name#4174
jlowin merged 3 commits into
PrefectHQ:mainfrom
strawgate:fix-minimize-reviews-concurrency

Conversation

@strawgate
Copy link
Copy Markdown
Collaborator

A single review action fires two events at the same instant — pull_request_review (submitted) and pull_request_review_comment (created). Because the minimize-resolved-reviews concurrency group was keyed only on PR number, both sibling runs landed in the same group with cancel-in-progress: true and cancelled each other. Usually one survives, but under a burst both siblings can end cancelled, leaving a commit with no successful run of this workflow at all (observed on #4060's codex/keycloak-fix branch — e.g. runs 2767 and 2768 for the same SHA both cancelled).

Appending github.event_name to the concurrency group keeps the two sibling event types in separate groups so they no longer cancel each other. Same-PR runs of the same event still supersede cleanly, and the last run of each type always completes.

group: minimize-reviews-${{ github.event.pull_request.number || github.event.issue.number }}-${{ github.event_name }}

🤖 Generated with Claude Code

A single review action fires pull_request_review and
pull_request_review_comment at the same instant; sharing one
PR-scoped concurrency group made the siblings cancel each other,
sometimes leaving a SHA with only cancelled runs.

🤖 Generated with Claude Code
Copy link
Copy Markdown

@claude claude Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@strawgate
Copy link
Copy Markdown
Collaborator Author

@claude review

@marvin-context-protocol marvin-context-protocol Bot added bug Something isn't working. Reports of errors, unexpected behavior, or broken functionality. tests labels May 17, 2026
@marvin-context-protocol
Copy link
Copy Markdown
Contributor

tl;dr: CI failure is a pre-existing Windows-only flake in tests/server/auth/providers/test_azure_scopes.py::TestAzureOBOIntegration::test_get_obo_credential_returns_configured_credential — unrelated to this PR's workflow-file change. Safe to re-run.

Root cause: The test timed out at the 5s thread-timeout while inside with patch("azure.identity.aio.OnBehalfOfCredential", ...). The traceback ends in Python's import machinery (_dot_lookup__import__find_spec_path_join), i.e. the mock.patch import resolution for azure.identity.aio got stuck during module discovery on the Windows runner. This is environmental (slow Windows file I/O during import), not caused by anything in this PR. The PR diff touches exactly one file — .github/workflows/minimize-resolved-reviews.yml — to scope the concurrency group by github.event_name. No Python source or test code was changed.

Fix: Re-run the Tests: Python 3.10 on windows-latest job. If the same test hangs again on retry, that's a separate flake worth opening an issue for (the test does with patch(\"azure.identity.aio.OnBehalfOfCredential\", ...) at tests/server/auth/providers/test_azure_scopes.py:617, which forces a real import of azure.identity.aio under a 5s timeout — vulnerable to slow Windows imports).

Log excerpt

```
File "D:\a\fastmcp\fastmcp\tests\server\auth\providers\test_azure_scopes.py", line 617, in test_get_obo_credential_returns_configured_credential
with patch(
File "...\unittest\mock.py", line 1431, in enter
self.target = self.getter()
File "...\unittest\mock.py", line 1261, in _importer
thing = _dot_lookup(thing, comp, import_path)
File "...\unittest\mock.py", line 1250, in _dot_lookup
import(import_path)
File "", line 1562, in find_spec
File "", line 104, in _path_join
+++++++++++++++++++++++++++++++++++ Timeout +++++++++++++++++++++++++++++++++++
##[error]Process completed with exit code 1.
```

All other jobs in run 26002021291 (ubuntu 3.10/3.13, lowest-direct deps, MCP conformance, integration, smoke) passed.

PR diff (for context)

```diff

  • group: minimize-reviews-${{ github.event.pull_request.number || github.event.issue.number }}
  • group: minimize-reviews-${{ github.event.pull_request.number || github.event.issue.number }}-${{ github.event_name }}
    ```

Workflow-only change; cannot affect Python unit tests.

@jlowin jlowin merged commit b4c3502 into PrefectHQ:main May 20, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working. Reports of errors, unexpected behavior, or broken functionality. tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants