Skip to content

Deduplicate CI runs and finish full matrices - #1003

Draft
caspervonb wants to merge 1 commit into
mainfrom
fix-ci-duplicate-runs
Draft

Deduplicate CI runs and finish full matrices#1003
caspervonb wants to merge 1 commit into
mainfrom
fix-ci-duplicate-runs

Conversation

@caspervonb

Copy link
Copy Markdown
Collaborator

Every PR was running the whole matrix twice against the same commit, because both workflows trigger on push and pull_request for all branches. On #1002 that produced two test runs for the same SHA 12 seconds apart, one of which showed up as a confusing cancelled.

Restricting push to main leaves pull_request as the single trigger for branch work. The concurrency group additionally retires a PR's earlier runs when it is pushed again; main pushes key on run_id so they always finish.

Separately, the project and build matrices had no fail-fast setting, so one failing leg cancelled its siblings — a single flaky ubuntu job took the macOS and Windows results with it (10-15 cancelled jobs on #938, #941 and #1002). The legacy nats matrix already sets fail-fast: false.

Note the tradeoff: pushes to a branch with no open PR no longer get CI.

Both workflows triggered on push and pull_request for every branch, so
each pull request ran the whole matrix twice against the same commit:
one run per event, 50+ jobs each. Restricting push to main leaves pull
requests as the single trigger for branch work. A concurrency group
keyed on head_ref also retires a pull request's earlier runs when it is
pushed again; pushes to main key on run_id so they always finish.

The project and build matrices had no fail-fast setting, so one failing
leg cancelled its siblings -- a single flaky ubuntu job would take the
macOS and Windows results with it and hide whether they passed. The
legacy nats matrix already set fail-fast: false; do the same for the
other two.
@synadia-claude-reviewer

synadia-claude-reviewer Bot commented Aug 17, 2026

Copy link
Copy Markdown

Claude finished @caspervonb's task in 1m 11s —— View job


Code Review

  • Read PR diff and existing workflow files
  • Analyze changes for correctness, security, performance
  • Post review findings

The changes are correct and well-structured. A few observations:

Concurrency group formula (${{ github.workflow }}-${{ github.head_ref || github.run_id }}): this is the standard recommended pattern. github.head_ref is set on pull_request events (PR branch name) and is an empty string on push events — GitHub Actions expressions treat empty string as falsy in ||, so main pushes correctly fall through to run_id and are never cancelled. No issue here.

push branch filter change in check.yml: old value was "*" (single star, doesn't match slashes), new pull_request value is "**". This is strictly correct — "*" would silently skip PRs from branches like feat/my-feature. The test.yml already used "**" for pull_request, so this makes the two files consistent.

fail-fast: false: both the build matrix in check.yml and project matrix in test.yml needed this. The nats matrix already had it. No gaps remain.

Tradeoff (no CI for pushes to branches without an open PR) is the right call for a repo that uses PRs as the standard review gate. It's documented clearly in the PR description and in the inline comment.

No correctness issues, security concerns, or performance implications. LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant