[JENKINS-55927] Hook event should not trigger Branch Indexing

[Dohbedoh]

JENKINS-55927

Noticed this issue while troubleshooting an environment where Bitbucket event were triggering branch indexing. Haven't yet identified exactly what kind of push events has empty changes. Though per my understanding of the design of the Branch API / SCM API and Pipeline Multibranch, SCM events should not trigger branch indexing ? And this is rather unexpected. Unless I am missing something ?

cc @jenkinsci/bitbucket-branch-source-plugin-developers

Your checklist for this pull request

• Make sure you are requesting to pull a topic/feature/bugfix branch (right side) and not your master branch!
• Ensure that the pull request title represents the desired changelog entry
• Please describe what you did
• Link to relevant issues in GitHub or in Jenkins JIRA
• Link to relevant pull requests, esp. upstream and downstream changes
• Did you provide a test-case? That demonstrates feature works or fixes the issue.

[nfalco79]

I think, but I need to check, Approved, Approved Removed, Declined event could have not changes. For example declined will trigger the job deletion in multibranch pipeline

[Dohbedoh]

@nfalco79 Actually according to the code and the HookProcessor that do this, it should really be a push event (could also be a mirror synchronized) but not pull requests... Unless declining a pull requests somehow generate a push event on the base branch..

For example declined will trigger the job deletion in multibranch pipeline

Right, so per my understanding of the SCM API, a decline should just be a REMOVED event and mark the branch job as dead. Branch Indexing will be responsible later on for the cleanup of orphaned items.

• https://github.com/jenkinsci/scm-api-plugin/blob/698.v8e3b_c788f0a_6/docs/implementation.adoc?plain=1#L153-L157
• https://github.com/jenkinsci/scm-api-plugin/blob/698.v8e3b_c788f0a_6/docs/implementation.adoc?plain=1#L1945-L1950

Here the behavior of triggering a full scan on a SCMHead event (single branch) can be detrimental. A full scan can be long and expensive (resources and BranchIndexing thread pool on the server but also API requests against Bitbucket).

That is why I think this might be a mistake.

[nfalco79]

Here the behavior of triggering a full scan on a SCMHead event (single branch) can be detrimental. A full scan can be long and expensive (resources and BranchIndexing thread pool on the server but also API requests against Bitbucket).

I agree but if you do not perform a BranchIndexing you do not know what are branches involved in changes.

[Dohbedoh]

Looking at the history of that change, it was here from the start, before we implemented the SCM API Event mechanism and preserved when we added it https://github.com/jenkinsci/bitbucket-branch-source-plugin/pull/37/files#diff-a5efe0e74b0f75735db75b177d0b8da3426c13be4a855727e8eeb8193f74ff33. Though we were relying on a custom made plugin at that time that does not exist anymore: https://github.com/topicusfinan/bitbucket-webhooks-plugin/

I think we really need to figure out what scenario causes an empty changes. I am quite convinced triggering indexing instead of using a particular Event is wrong.. If we wanted to have indexing triggered, I would think we'd create a SCMSourceEvent instead. Maybe someone closer to the design of SCM API / Branch API could confirm cc @rsandell @amuniz

Per the JIRA issue, this could be a remote merge, that is we have an Open PR, but instead of merging it from Bitbucket, we merge it by hand. Hence we end up with a PR without changes. If that is the case, this really shouldn't trigger an indexing. I would be surprised if someone was relying on such a behavior.

[nfalco79]

I would take time to test some supported webhook event (I'm collecting them) like:
new tag, that must automatically trigger the tag discover branch
PR merged trigger removal
New PR
Approved
Disapproved

The behaviour to trigger a new build for all the PRs with Merge strategy (build PR commit with merge into master) is by what who setup this expect to ensure the PR is good for merge and avoid trigger manul build. Who do not want -> build strategy to inhibit builds (but not indexing)

[amuniz]

It's a really long time I worked on this plugin, so I don't really know. But looking at the change introducing the empty check I think they were interpreted as an empty changeset, so it corresponds to a PR creation/deletion which actually requires a re-index to create/remove the job in Jenkins. Whether that's still needed or not, I don't know, but I think if all the known cases are manually tested and they work, then we are ok.

[Dohbedoh]

But per the design of the API (and like all implementation do) those events do not run branch indexing... A CREATE SCMEvent would create the PR job. A DELETED SCMEvent would mark it as stale...
And those event with empty changes are actually push events, they originate from activity on branches.

Just need to figure out what a push event with empty changes originate from. I think I have a reproducer with Bitbucket Server with Native webhook and also with the add on. Will update this in the JIRA shortly.

[Dohbedoh]

Found 2 scenario where you get a push event with empty changes, in Bitbucket Server with the Native webhook as well as with the Bitbucket add-on. I found 2 scenarios where the empty changes may happen:

Rebase a target branch to a source branch while a PR is open

git checkout main
git checkout -B source-1
echo "test" >> source-1.md
git add . && git commit -m "source-1: $(date)" && git push --set-upstream origin source-1
git push --set-upstream origin source-1
# Open a PR for from the source branch `source-1` to the target branch `main`
git checkout main
git rebase -i source-1
git push -f

This will generate 2 events. The first one with empty changes and the next one with the correct changes made to the target branch. (The PR is automatically merged because there is no diff and that's a default behavior of Bitbucket server).. So the first event is useless. It does not give any clue about what just happened.

Auto-Sync of Fork when a branch cannot be synchronized

When the Multibranch Pipeline is a fork and Automatic Fork Syncing is enabled. If a branch on a fork cannot be synchronized, the attempt from Bitbucket to synchronize it generates a push event with empty changes. This seems also useless. The event does not contain information that allow us to know what just happened, and the refs is actually unchanged...

---

Both the add-on and the native webhook results behave the same. Per my understanding the add-on relies on the native mechanism but add some extra configuration..

So far, I don't see a valid scenario where a push event with empty changes should be considered. Or why it would trigger branch indexing. If we are skeptical about whether anybody expect this behavior, I propose to add a system property to allow use to re-enable this.

[nfalco79]

The behaviour to trigger a new build for all the PRs with Merge strategy (build PR commit with merge into master) is by what who setup this expect to ensure the PR is good for merge and avoid trigger manual build.

Issue #316

[Dohbedoh]

Actually this behavior is not what happens with Bitbucket Branch Source at the moment. Those events that trigger branch indexing are happening on very specific scenario where you would not expect it (see my previous comment).
What people are asking in #316 are IMO a feature for SCM API / Branch API. By default the API does not go and observe source branches when a target one is updated.

[Dohbedoh]

I still don't have an answer from Atlassian at https://community.atlassian.com/t5/Bitbucket-questions/Webhook-repo-refs-changed-events-with-empty-changes-array/qaq-p/2868488#M109342. But note that the plugin maintained by Atlassian only move forward if the event contains changes:

• https://github.com/jenkinsci/atlassian-bitbucket-server-integration-plugin/blob/7da79af5f2f137ed38b1f31828b45f935a9d86f4/src/main/java/com/atlassian/bitbucket/jenkins/internal/trigger/BitbucketWebhookConsumer.java#L59-L61
• https://github.com/jenkinsci/atlassian-bitbucket-server-integration-plugin/blob/7da79af5f2f137ed38b1f31828b45f935a9d86f4/src/main/java/com/atlassian/bitbucket/jenkins/internal/trigger/BitbucketWebhookConsumer.java#L101C44-L106

Maybe that can help convinced maintainers JENKINS-55927 is truly a bug and this fix is valid.
cc @nfalco79

[nfalco79]

it's not possible, this will have a worster impact on issues like #539

[Dohbedoh]

The impact really depend on a the environment. User that have large repositories with hundreds of branches / PRs cannot afford frequent branch indexing.
In case of #539, branch indexing is not a good solution IMO. A pull request decline event should not trigger branch indexing, but rather update the source and target based on discovery criteria through SCMHeadEvents.
Maybe the description could be improved.

[nfalco79]

but rather update the source and target based on discovery criteria through SCMHeadEvents.

This is hard because you should apply any registered trait (also external to this plugin) and simulate if any existing job is related to the target branch and in case delete or create.
In master I start to add unit test collecting some bitbucket cloud events and I'm providing test on how I expect they should do

[nfalco79]

Is this a real payload webhook?

[Dohbedoh]

Nope, taken from https://github.com/jenkinsci/bitbucket-branch-source-plugin/blob/master/src/test/resources/com/cloudbees/jenkins/plugins/bitbucket/client/events/BitbucketCloudPushEventTest/emptyPayload.json. I don't have a non native server instance to test and get a payload.
Maybe I can use https://github.com/jenkinsci/bitbucket-branch-source-plugin/blob/master/src/test/resources/com/cloudbees/jenkins/plugins/bitbucket/server/events/BitbucketServerPushEventTest/updatePayload.json and change to an empty array of changes.

[nfalco79]

I would real example (masked) instead to provide payload that support what would do.

[nfalco79]

Further details about server empty changes:
https://docs.atlassian.com/bitbucket-server/javadoc/5.7.0/api/reference/com/atlassian/bitbucket/event/repository/RepositoryRefsChangedEvent.html#getRepository()

Note: While the returned collection will never be null, it may be empty. For example, a remotely merged pull request will have an empty set of changes.

[Dohbedoh]

@nfalco79 Yes, as mentioned above and in the JIRA. In that case, you receives 2 events. One without changes and the second one with the commits from the remote merge. The second can just be discarded.

[jtnord]

Haven't yet identified exactly what kind of push events has empty changes

A mirror synchronized event with too many refs to list will set refLimitExceeded = true and have an empty changes.

http://confluence.atlassian.com/bitbucketserver/event-payload-938025882.html#Eventpayload-Mirrorsynchronized

Thus if mirrors are being used, a branch index would appear to be correct for this?

[Dohbedoh]

Thnaks @jtnord. I have update the PR and added tests for this scenario.

[jtnord]

Seems reasonable, however I would wonder if the boolean should be inverted so that you opt into the new behaviour rather than out of it.

At the end of the day triggering a scan should be fine, and if that is an issue, then scanning should be improved to rate limit or coalesce full repository scans.

[nfalco79]

Could we use a single property "bitbucket.hooks.processor.scanOnEmptyChanges" instead to have two different property for the same scope? I mean would you want enable the feature in a system that use different server with different webhook with different behaviour?

[nfalco79]

In my roadmap I would create an extension point to allow other plugin to implement a custom web hook processor so I would not have a property named like class because of refactoring and expose a method like isEmptyChangesAllowed returns true/false so implementations could ignore or respect this flag

[Dohbedoh]

Got it. Added the property definition in the HooKProcessor for now.

[nfalco79]

@Dohbedoh could you provide a single commit with a description of changes ?