Bug 2058190: [GitHub] fix race condition on adding PRs to bugs

[justdave]

• Bug 2058190

Turns out there was already an attempt to avoid duplicates in the code, but it did not lock the bug around the duplicate check, which meant if you received two notifications in a row for the same PR, they could both pass the duplicate check before either one wrote to the database.

This PR fixes it by grabbing a row-level lock on the bugs table around the duplicate check, so that the second one coming in has to wait in line for the first one to finish before it checks if there's a duplicate.

[Copilot]

Pull request overview

Adds per-bug transaction locking to prevent duplicate GitHub pull-request attachments during concurrent webhook delivery.

Changes:

• Locks the bug row before duplicate detection and attachment creation.
• Adds duplicate-attachment regression coverage.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
Bugzilla/API/V1/Github.pm	Serializes PR attachment creation per bug.
qa/t/rest_github_pull_request.t	Verifies repeated webhook delivery preserves one attachment.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

qa/t/rest_github_pull_request.t:27

• This monkeypatch cannot delay the API handler exercised by this test. The test sends requests to the absolute urlbase, while cmd_test_qa starts bugzilla.pl daemon as a separate process before prove (scripts/entrypoint.pl:162-179), so neither this symbol replacement nor the environment variable assigned later reaches that server. The QA runner also selects the single-process simple backend, so forked clients do not guarantee overlapping handlers. Consequently the old unlocked implementation can pass this regression test; the synchronization hook must run in a concurrently serving API process.

{
  no warnings 'redefine';
  my $original_create = \&Bugzilla::Attachment::create;
  *Bugzilla::Attachment::create = sub {

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

qa/t/rest_github_pull_request.t:34

• This monkeypatch runs only in the test process, while every request is sent to the absolute urlbase and handled by the separately running web server. Consequently the server never executes this wrapper or sees the localized environment variable, so the 500 ms delay does not force the deliveries to overlap and this regression test is timing-dependent. Run these requests against an in-process app that inherits the patch, or add a synchronization mechanism that executes in the server workers.

  *Bugzilla::Attachment::create = sub {
    my ($invocant, $params) = @_;
    my $delay_us = int($ENV{BMO_GITHUB_PR_TEST_CREATE_DELAY_US} // 0);
    if ($delay_us > 0
      && ref $params eq 'HASH'
      && ($params->{mimetype} // '') eq 'text/x-github-pull-request')
    {
      usleep($delay_us);

[justdave]

I see ! vs eq precedence issues being reported in the test, you can fix that by cherry-picking bugzilla/harmony@2be0c4c out of Bugzilla Harmony (we just fixed that last week)

[justdave]

The remaining nit from CoPilot that I didn't fix yet is a timing issue with the test... and due to the nature of the test, there will always be timing issues with it, and I don't see a way to force that to be concurrent. You get lucky sometimes, just like in production.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

Bugzilla/API/V1/Github.pm:127

• This locking order can deadlock the exact first-delivery race on MySQL/InnoDB. When no attachment exists, both transactions can take compatible gap locks for this absent filename via the new index; one then holds the bug row and waits to insert through the other's gap lock while the other waits for that bug row. MySQL aborts one request, so the duplicate delivery can return a database error instead of the intended duplicate response. Serialize before this range scan (for example with a dedicated per-PR lock), or otherwise redesign the lock order and retry deadlock victims.

    my $candidate_bug_ids = $dbh->selectcol_arrayref(
      'SELECT DISTINCT bug_id
         FROM attachments
        WHERE mimetype = ?
          AND filename = ?
          AND NOT isobsolete
        FOR UPDATE',

qa/t/rest_github_pull_request.t:34

• This monkeypatch runs only in the QA process, but the test posts to urlbase, which is served by the separate dev_httpd process. Neither this symbol replacement nor the child-local environment value reaches the server, so Attachment::create is never delayed and the test does not deterministically exercise the race it claims to cover. The synchronization hook must run in the server process (or the endpoint must be exercised in-process with an equivalent barrier).

  *Bugzilla::Attachment::create = sub {
    my ($invocant, $params) = @_;
    my $delay_us = int($ENV{BMO_GITHUB_PR_TEST_CREATE_DELAY_US} // 0);
    if ($delay_us > 0
      && ref $params eq 'HASH'
      && ($params->{mimetype} // '') eq 'text/x-github-pull-request')
    {
      usleep($delay_us);