ci: Checkout latest merged pulls #33303

pull maflcko wants to merge 1 commits into bitcoin:master from maflcko:2509-ci-co-pull changing 1 files +10 −9
  1. maflcko commented at 9:25 am on September 4, 2025: member

    Currently, the actions/checkout@v5 checks out pull requests merged against master, which is what we want.

    However, it checks out ancient/stale merge commits on a re-run. This is documented (https://docs.github.com/en/actions/how-tos/manage-workflow-runs/re-run-workflows-and-jobs):

    Re-run workflows […] will also use the same GITHUB_SHA (commit SHA) and GITHUB_REF (git ref) of the original event that triggered the workflow run.

    For example:

    This is problematic, because:

    • Unrelated CI failures and intermittent issues, which are fixed or worked around in latest master can not be cleaned by re-running the task. The author has to actively go out and (force-)push the branch, invalidating review.
    • It is odd to have a recent CI run, but it uses code and config from the past.
    • Detecting silent merge conflicts by re-running the CI task is impossible.

    Fix all issues by checking out the latest merged state of the pull request. The behavior is unchanged for non-pull-request actions. This patch changes the “re-run” default behaviour. Forcing it to use the new state instead of running the old state again.

  2. DrahtBot added the label Tests on Sep 4, 2025
  3. DrahtBot commented at 9:26 am on September 4, 2025: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33303.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK janb84, hebasto
    Stale ACK willcl-ark

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

  4. stickies-v commented at 11:36 am on September 4, 2025: contributor

    If I understand correctly - the expected behaviour for actions/checkout@v5 for pull_request is to checkout a temporary merge commit onto the merge branch, which should be re-evaluated every time the workflow is reran (e.g. a manual trigger), which is what we want to avoid silent merge conflicts. But you’ve observed that in some cases, the merge branch (GITHUB_REF aka refs/pull/PULL_REQUEST_NUMBER/merge) is stale. Do you know why that is happening, and is this an upstream bug? I’ve browsed the documentation and https://github.com/actions/checkout Issues and Discussion but couldn’t find any relevant conversation.

    If my above understanding is correct, I’m not sure how the fix proposed here can work.

    The docs define GITHUB_REF as refs/pull/PULL_REQUEST_NUMBER/merge. This PR sets the ref based on format('refs/pull/{0}/merge', github.event.pull_request.number). Aren’t they doing the exact same thing?

  5. janb84 commented at 11:52 am on September 4, 2025: contributor

    If I understand correctly - the expected behaviour for actions/checkout@v5 for pull_request is to checkout a temporary merge commit onto the merge branch, which should be re-evaluated every time the workflow is reran (e.g. a manual trigger), which is what we want to avoid silent merge conflicts. But you’ve observed that in some cases, the merge branch (GITHUB_REF aka refs/pull/PULL_REQUEST_NUMBER/merge) is stale. Do you know why that is happening, and is this an upstream bug? I’ve browsed the documentation and https://github.com/actions/checkout Issues and Discussion but couldn’t find any relevant conversation.

    If my above understanding is correct, I’m not sure how the fix proposed here can work.

    The docs define GITHUB_REF as refs/pull/PULL_REQUEST_NUMBER/merge. This PR sets the ref based on format('refs/pull/{0}/merge', github.event.pull_request.number). Aren’t they doing the exact same thing?

    I came to the same conclusion. Can it be that it’s not on pull-request event but on push event ? (workflow triggers on both)

  6. maflcko commented at 12:08 pm on September 4, 2025: member

    I am not familiar with the action written in typescript and I don’t want to touch it upstream. However, I am happy to close this pull, if someone else does.

    The issue is that the action does not use the ref, but the commit to fetch: https://github.com/actions/checkout/blob/ff7abcd0c3c05ccf6adc123a8cd1fd4fb30fb493/src/ref-helper.ts#L96

    You can also see this here: https://github.com/bitcoin/bitcoin/actions/runs/17458152407/job/49579638898?pr=29641#step:3:59:

    0  /usr/bin/git -c protocol.version=2 fetch --no-tags --prune --no-recurse-submodules --depth=1 origin +ee059f57f059c7210da3a3f63ec433b29a35da0e:refs/remotes/pull/29641/merge

    This is obviously wrong. The correct command can be seen in this pull request: https://github.com/bitcoin/bitcoin/actions/runs/17459448857/job/49580495254?pr=33303#step:3:60. Adjusted:

    0  /usr/bin/git -c protocol.version=2 fetch --no-tags --prune --no-recurse-submodules --depth=1 origin +refs/pull/29641/merge:refs/remotes/pull/29641/merge

    You can also run it locally, to confirm.

    You can also refer to the old cirrus yaml, which did the same: https://github.com/bitcoin/bitcoin/commit/ba0b4304eceeec52eb3ffd2e7f8bb831ba1ec278#diff-62587956f943bb2503db7bc6dd27d0d888074a1c0ecaab3f570ad611aff0f7bbL77:

    0    - git fetch --depth=1 $CIRRUS_REPO_CLONE_URL "pull/${CIRRUS_PR}/merge"
  7. hebasto commented at 12:16 pm on September 4, 2025: member

    Currently, the actions/checkout@v5 checks out pull requests merged against master, which is what we want.

    However, sometimes, it checks out ancient/stale merge commits. For example:

    This appears to be an issue with GitHub itself rather than actions/checkout@v5, as the CI logs show it switching to a refs/remotes/pull/<PR_NUMBEER>/merge branch.

  8. maflcko commented at 12:19 pm on September 4, 2025: member

    This appears to be an issue with GitHub itself rather than actions/checkout@v5, as the CI logs show it switching to a refs/remotes/pull/<PR_NUMBEER>/merge branch.

    The refs/remotes/... is just the local name used by the checkout action. It does not reflect the stuff that was fetched, see my previous comment.

  9. hebasto commented at 12:47 pm on September 4, 2025: member

    The issue is that the action does not use the ref, but the commit to fetch: https://github.com/actions/checkout/blob/ff7abcd0c3c05ccf6adc123a8cd1fd4fb30fb493/src/ref-helper.ts#L96

    You can also see this here: https://github.com/bitcoin/bitcoin/actions/runs/17458152407/job/49579638898?pr=29641#step:3:59:

    0  /usr/bin/git -c protocol.version=2 fetch --no-tags --prune --no-recurse-submodules --depth=1 origin +ee059f57f059c7210da3a3f63ec433b29a35da0e:refs/remotes/pull/29641/merge

    This is obviously wrong.

    Why is this wrong? ee059f57f059c7210da3a3f63ec433b29a35da0e (i.e., GITHUB_SHA) is expected to be:

    Last merge commit on the GITHUB_REF branch

  10. janb84 commented at 12:48 pm on September 4, 2025: contributor

    ACK fa8f944eaa1955e4e2c376ce36f1b1cbb1897769

    The example given in the PR description https://github.com/bitcoin/bitcoin/actions/runs/17458152407/job/49579638898?pr=29641#step:9:914 is a “re-run”

    Re-run workflows use the privileges of the actor who initially triggered the workflow, not the privileges of the actor who initiated the re-run. The workflow will also use the same GITHUB_SHA (commit SHA) and GITHUB_REF (git ref) of the original event that triggered the workflow run.

    At a first glance the PR does nothing that should already happen, but The PR changes “re-run” default behaviour. Forcing it to use the new state instead of running the old state again.

  11. hebasto commented at 12:54 pm on September 4, 2025: member

    The PR changes “re-run” default behaviour. Forcing it to use the new state instead of running the old state again.

    I suggest making this clearer in the PR description.

  12. hebasto commented at 1:40 pm on September 4, 2025: member

    I’ve tested this in my personal repo and can confirm the re-run job behaviour described by @janb84, which is also documented here.

    In my opinion, the current behavior is useful for reproducing issues in the CI.

    However, before merging a PR, it’s much easier to re-run the CI to check for merge conflicts than to rebase the PR.

    Concept ACK.

  13. maflcko commented at 2:12 pm on September 4, 2025: member

    documented

    thx, added to description

    In my opinion, the current behavior is useful for reproducing issues in the CI.

    If there is a hypothetical issue to reproduce by re-running, which is no longer present with current master, it seems best to reproduce with an exact commit outside this repo. Also, I can’t really remember a single instance where this was ever needed. The inverse is much more common: Re-run to get rid of an unrelated or fixed issue.

  14. hebasto commented at 2:14 pm on September 4, 2025: member

    While testing this PR, I noticed that GitHub’s behaviour seems unreliable.

    In my personal repo, I opened a PR, then pushed a commit to the default branch. After that, I checked it out with:

    0git fetch origin pull/4/merge
1git rev-parse FETCH_HEAD

    The output still refers to the branch based on the previous state of the default branch.

    It took GitHub a few minutes to update the references.

  15. maflcko commented at 2:20 pm on September 4, 2025: member

    It took GitHub a few minutes to update the references.

    Correct. See https://docs.github.com/en/rest/guides/using-the-rest-api-to-interact-with-your-git-database?apiVersion=2022-11-28#checking-mergeability-of-pull-requests for the docs

  16. in .github/workflows/ci.yml:138 in fa8f944eaa outdated 
    133@@ -134,6 +134,8 @@ jobs:
134     steps:
135       - name: Checkout
136         uses: actions/checkout@v5
137+        with:
138+          ref: ${{ github.event.pull_request.number && format('refs/pull/{0}/merge', github.event.pull_request.number) || '' }}
    hebasto commented at 2:33 pm on September 4, 2025:
    1. The github.ref can be used directly:
    0          ref: ${{ github.event.pull_request.number && github.ref || '' }}
    1. When combined with suggestion 1, the check can be made more explicit:
    0          ref: ${{ github.event_name == 'pull_request' && github.ref || '' }}
    1. Since this PR changes the documented behavior when re-running a CI job, it seems reasonable to add a comment to document:
    • the new behavior;
    • a caution not to rely on github.sha or GITHUB_SHA, as they may be irrelevant.
    maflcko commented at 5:05 pm on September 4, 2025:
    thx, all done
    hebasto commented at 2:35 pm on September 5, 2025:

    The helpful explanatory comment was removed in the recent push (faeb3320952906b6147b06170059e71d7d59f4bd).

    Could it be re-added, if it’s not too much trouble?

    maflcko commented at 9:36 am on September 7, 2025:
    ah, restored doc
  17. hebasto approved
  18. hebasto commented at 2:37 pm on September 4, 2025: member

    ACK fa8f944eaa1955e4e2c376ce36f1b1cbb1897769, tested in my personal repo.

    At some point, it might be useful to use YAML anchor and aliases for repeated “Checkout” steps. For example: https://github.com/bitcoin-core/secp256k1/pull/1719/commits/50e73c6eee8be094ec75920b5b71cdc5984452de.

    The new re-run behaviour will only be available for PRs that are rebased after this one is merged.

  19. willcl-ark commented at 2:40 pm on September 4, 2025: member

    At some point, it might be useful to use YAML anchor and aliases for repeated “Checkout” steps. For example: bitcoin-core/secp256k1@50e73c6.

    Wow, I’m not sure why but I had in my head that GitHub did not support anchors (annoyingly so). And their alternative was basically “make your own action for it”.

    Will look at using a few anchors now I know they work, thanks.

  20. hebasto commented at 2:45 pm on September 4, 2025: member

    At some point, it might be useful to use YAML anchor and aliases for repeated “Checkout” steps. For example: bitcoin-core/secp256k1@50e73c6.

    Wow, I’m not sure why but I had in my head that GitHub did not support anchors (annoyingly so). And their alternative was basically “make your own action for it”.

    It was introduced fairly recently.

  21. maflcko commented at 2:54 pm on September 4, 2025: member

    Will look at using a few anchors now I know they work, thanks.

    The qa-assets repo actually uses the GitHub actions shipped by Bitcoin Core, so I think keeping them is preferable than anchors in this instance. Ref:

    https://github.com/bitcoin-core/qa-assets/blob/fc42e64f6e664e15549f608e2cc90d8af9a515b1/.github/workflows/ci.yml#L56-L75

  22. hebasto commented at 3:00 pm on September 4, 2025: member

    Will look at using a few anchors now I know they work, thanks.

    The qa-assets repo actually uses the GitHub actions shipped by Bitcoin Core, so I think keeping them is preferable than anchors in this instance. Ref:

    https://github.com/bitcoin-core/qa-assets/blob/fc42e64f6e664e15549f608e2cc90d8af9a515b1/.github/workflows/ci.yml#L56-L75

    Then perhaps encapsulate customized checkout action as well?

  23. maflcko commented at 3:18 pm on September 4, 2025: member

    Then perhaps encapsulate customized checkout action as well?

    I don’t think it matters much for other repos, but I am happy to push a commit, if someone writes one, or close this pull in favour of one that refactors this one :)

  24. maflcko force-pushed on Sep 4, 2025
  25. maflcko force-pushed on Sep 4, 2025
  26. janb84 commented at 5:37 pm on September 4, 2025: contributor

    ACK fab00ebbc74fe55c41b2d531a964cdb083b02ab0

    changes sinds last ACK:

    • changes related to use YAML KEYS / templating.

    Kinda in doubt about the use of YAML_KEY_01, in code it makes it clear that it is a YAML key but in the action execution it is visible as a env variable :

    0• Run actions/checkout@v5 with:
1ref: refs/pull/33303/merge repository: bitcoin/bitcoin token: ***
2ssh-strict: true ssh-user: git
3persist-credentials: true clean: true
4sparse-checkout-cone-mode: true fetch-depth: 1
5fetch-tags: false show-progress: true lfs: false
6submodules: false set-safe-directory: true
7env:
8YAML_KEY_01: refs/pull/33303/merge
  27. DrahtBot requested review from hebasto on Sep 4, 2025
  28. hebasto approved
  29. hebasto commented at 5:56 pm on September 4, 2025: member

    ACK fab00ebbc74fe55c41b2d531a964cdb083b02ab0.

    This time, it took GitHub ~20min to update the reference.

  30. maflcko commented at 6:16 pm on September 4, 2025: member

    Kinda in doubt about the use of YAML_KEY_01, in code it makes it clear that it is a YAML key but in the action execution it is visible as a env variable :

    I know, but I don’t know any alternative. See also the GitHub schema validator on a previous run: https://github.com/bitcoin/bitcoin/actions/runs/17471191651/workflow:

    0.github/workflows/ci.yml
1
2Invalid workflow file
3
4(Line: 21, Col: 1): Unexpected value 'checkout_ref_tmpl'
  31. janb84 commented at 6:25 pm on September 4, 2025: contributor

    Kinda in doubt about the use of YAML_KEY_01, in code it makes it clear that it is a YAML key but in the action execution it is visible as a env variable :

    I know, but I don’t know any alternative. See also the GitHub schema validator on a previous run: https://github.com/bitcoin/bitcoin/actions/runs/17471191651/workflow:

    0.github/workflows/ci.yml
1
2Invalid workflow file
3
4(Line: 21, Col: 1): Unexpected value 'checkout_ref_tmpl'

    Isn’t this because of out of env declaration ? It’s also fine to do as a followup when the documentation is available (if the change is wanted)

  32. willcl-ark commented at 7:22 pm on September 4, 2025: member

    Kinda in doubt about the use of YAML_KEY_01, in code it makes it clear that it is a YAML key but in the action execution it is visible as a env variable :

    I know, but I don’t know any alternative. See also the GitHub schema validator on a previous run: bitcoin/bitcoin/actions/runs/17471191651/workflow:

    0.github/workflows/ci.yml
1
2Invalid workflow file
3
4(Line: 21, Col: 1): Unexpected value 'checkout_ref_tmpl'

    I think you should be able to use this patch to avoid setting it in the env vars:

     0commit 186895413f578ccb06dace46b43784ebfae171a3
 1Author: will <will@256k1.dev>
 2Date:   Thu Sep 4 20:16:14 2025 +0100
 3
 4    fixup! ci: Checkout latest merged pulls
 5
 6diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
 7index 495c9f89584..68224792f30 100644
 8--- a/.github/workflows/ci.yml
 9+++ b/.github/workflows/ci.yml
10@@ -18,9 +18,6 @@ concurrency:
11   cancel-in-progress: true
12 
13 env:
14-  # Ensure the latest merged pull request state is used, even on re-runs.
15-  YAML_KEY_01: &CHECKOUT_REF_TMPL ${{ github.event_name == 'pull_request' && github.ref || '' }}
16-
17   CI_FAILFAST_TEST_LEAVE_DANGLING: 1  # GHA does not care about dangling processes and setting this variable avoids killing the CI script itself on error
18   CIRRUS_CACHE_HOST: http://127.0.0.1:12321/ # When using Cirrus Runners this host can be used by the docker `gha` build cache type.
19   REPO_USE_CIRRUS_RUNNERS: 'bitcoin/bitcoin' # Use cirrus runners and cache for this repo, instead of falling back to the slow GHA runners
20@@ -135,10 +132,11 @@ jobs:
21       BASE_ROOT_DIR: ${{ github.workspace }}
22 
23     steps:
24-      - name: Checkout
25+      - &Checkout
26+        name: Checkout
27         uses: actions/checkout@v5
28         with:
29-          ref: *CHECKOUT_REF_TMPL
30+          ref: &CHECKOUT_REF_TMPL ${{ github.event_name == 'pull_request' && github.ref || '' }}
31 
32       - name: Clang version
33         run: |
34@@ -204,10 +202,7 @@ jobs:
35             job-name: 'Windows native, fuzz, VS 2022'
36 
37     steps:
38-      - name: Checkout
39-        uses: actions/checkout@v5
40-        with:
41-          ref: *CHECKOUT_REF_TMPL
42+      - *Checkout
43 
44       - name: Configure Developer Command Prompt for Microsoft Visual C++
45         # Using microsoft/setup-msbuild is not enough.
46@@ -317,10 +312,7 @@ jobs:
47       DANGER_CI_ON_HOST_FOLDERS: 1
48 
49     steps:
50-      - name: Checkout
51-        uses: actions/checkout@v5
52-        with:
53-          ref: *CHECKOUT_REF_TMPL
54+      - *Checkout
55 
56       - name: Configure environment
57         uses: ./.github/actions/configure-environment
58@@ -360,10 +352,7 @@ jobs:
59       TEST_RUNNER_TIMEOUT_FACTOR: 40
60 
61     steps:
62-      - name: Checkout
63-        uses: actions/checkout@v5
64-        with:
65-          ref: *CHECKOUT_REF_TMPL
66+      - *Checkout
67 
68       - name: Download built executables
69         uses: actions/download-artifact@v4
70@@ -504,10 +493,7 @@ jobs:
71             file-env: './ci/test/00_setup_env_native_msan.sh'
72 
73     steps:
74-      - name: Checkout
75-        uses: actions/checkout@v5
76-        with:
77-          ref: *CHECKOUT_REF_TMPL
78+      - *Checkout
79 
80       - name: Configure environment
81         uses: ./.github/actions/configure-environment

    Running over in https://github.com/willcl-ark/bitcoin/actions/runs/17474469573

  33. maflcko force-pushed on Sep 4, 2025
  34. willcl-ark approved
  35. willcl-ark commented at 12:59 pm on September 5, 2025: member

    ACK faeb3320952906b6147b06170059e71d7d59f4bd

    This looks correct to me.

    For pushes we get the default behaviour, and for pull requests it evaluates to github.ref (e.g., refs/pull/<pr-num>/merge, ensuring the checkout uses the dynamic merge ref for the PR.

    This avoids pinning to a specific commit SHA (on re-runs) and fetches the latest merge state.

  36. DrahtBot requested review from janb84 on Sep 5, 2025
  37. DrahtBot requested review from hebasto on Sep 5, 2025
  38. janb84 commented at 1:36 pm on September 5, 2025: contributor

    re ACK faeb3320952906b6147b06170059e71d7d59f4bd

    Looks good !

    changes since last ACK:

    • using YAML_KEYS for templating with descriptive naming (moved from env)
  39. in .github/workflows/ci.yml:140 in faeb332095 outdated 
    131@@ -132,8 +132,11 @@ jobs:
132       BASE_ROOT_DIR: ${{ github.workspace }}
133 
134     steps:
135-      - name: Checkout
136+      - &CHECKOUT
137+        name: Checkout
138         uses: actions/checkout@v5
139+        with:
140+          ref: &CHECKOUT_REF_TMPL ${{ github.event_name == 'pull_request' && github.ref || '' }}
    kevkevinpal commented at 6:42 pm on September 6, 2025:

    What’s the reason for running the CI on the default branch (master) when we push to any branch on the repo?

    I think it makes sense to set the ref to the PR or the branch?

    maflcko commented at 9:35 am on September 7, 2025:
    Not sure what your question is, can you link to logs that explain what you are observing and asking about?
  40. ci: Checkout latest merged pulls fa8f081af3
  41. maflcko force-pushed on Sep 7, 2025
  42. maflcko commented at 9:36 am on September 7, 2025: member
    (doc-only force push to restore the explanation)
  43. janb84 commented at 6:16 pm on September 8, 2025: contributor

    re ACK fa8f081af31cd99155c7545643e7b10beb26714d

    changes since last ACK:

    • Restore of comment in code
  44. DrahtBot requested review from willcl-ark on Sep 8, 2025
  45. hebasto approved
  46. hebasto commented at 8:47 pm on September 8, 2025: member
    ACK fa8f081af31cd99155c7545643e7b10beb26714d.
  47. fanquake merged this on Sep 9, 2025
  48. fanquake closed this on Sep 9, 2025


maflcko DrahtBot stickies-v janb84 hebasto willcl-ark kevkevinpal


willcl-ark

Labels
Tests

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-09-15 06:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me