diff --git a/Bugzilla/API/V1/Github.pm b/Bugzilla/API/V1/Github.pm index 7e71c465a9..d9edb8b010 100644 --- a/Bugzilla/API/V1/Github.pm +++ b/Bugzilla/API/V1/Github.pm @@ -59,7 +59,7 @@ sub pull_request { return $self->render(json => {error => 0}); } - # Validate JSON input + # Validate JSON input my $payload = $self->req->json; my @errors = joi->object->props( action => joi->string->required, @@ -101,83 +101,181 @@ sub pull_request { return $self->render(json => {error => 1, message => $message}); } - # Check if bug already has this pull request attached (non-fatal) - foreach my $attachment (@{$bug->attachments}) { - next if $attachment->contenttype ne 'text/x-github-pull-request'; - if ($attachment->data eq $html_url) { - $template->process('global/code-error.html.tmpl', - {error => 'github_pr_attachment_exists'}, \$message) - || die $template->error(); - return $self->render(json => {error => 1, message => $message}); - } - } - - # Create new attachment using pull request URL as attachment content + # Create new attachment using pull request URL as attachment content. + # This path must be serialized per bug because concurrent webhook deliveries + # can otherwise race between duplicate-check and insert. my $auto_user = Bugzilla::User->check({name => 'github-automation@bmo.tld'}); $auto_user->{groups} = [Bugzilla::Group->get_all]; $auto_user->{bless_groups} = [Bugzilla::Group->get_all]; Bugzilla->set_user($auto_user); - my $timestamp = Bugzilla->dbh->selectrow_array("SELECT NOW()"); - - my $attachment = Bugzilla::Attachment->create({ - bug => $bug, - creation_ts => $timestamp, - data => $html_url, - description => "[$repository] $title (#$pr_number)", - filename => "github-$repo_filename-$pr_number-url.txt", - ispatch => 0, - isprivate => 0, - mimetype => 'text/x-github-pull-request', - }); - - # Insert a comment about the new attachment into the database. - $bug->add_comment( - '', - { - type => CMT_ATTACHMENT_CREATED, - extra_data => $attachment->id, - is_markdown => (Bugzilla->params->{use_markdown} ? 1 : 0) - } - ); - $bug->update($timestamp); - - # Fixup attachments with same github pull request but on different bugs - my %other_bugs; - my $other_attachments = Bugzilla::Attachment->match({ - mimetype => 'text/x-github-pull-request', - filename => "github-$repo_filename-$pr_number-url.txt", - WHERE => {'bug_id != ? AND NOT isobsolete' => $bug->id} - }); - foreach my $attachment (@$other_attachments) { - - # data doesn't match this URL, skip it - next if $attachment->data ne $html_url; - - $other_bugs{$attachment->bug_id}++; - my $moved_comment - = "GitHub pull request attachment was moved to bug " - . $bug->id - . ". Setting attachment " - . $attachment->id - . " to obsolete."; - $attachment->set_is_obsolete(1); - $attachment->bug->add_comment( - $moved_comment, - { - type => CMT_ATTACHMENT_UPDATED, - extra_data => $attachment->id, - is_markdown => (Bugzilla->params->{use_markdown} ? 1 : 0) + my $dbh = Bugzilla->dbh; + my $pr_lock_name = _acquire_pr_lock($dbh, $repository, $pr_number); + if ($dbh->isa('Bugzilla::DB::Mysql') && !$pr_lock_name) { + return $self->render( + json => { + error => 1, + message => 'Unable to acquire a lock for this pull request. Please retry.' } ); - $attachment->bug->update($timestamp); - $attachment->update($timestamp); + } + + my ($attachment, $duplicate); + eval { + $dbh->bz_start_transaction; + + # Lock all potentially-affected bugs in sorted order before we read/write + # attachments. This avoids deadlocks when two webhook deliveries move + # different PR attachments between the same pair of bugs in opposite + # directions. + my $candidate_bug_ids = $dbh->selectcol_arrayref( + 'SELECT DISTINCT bug_id + FROM attachments + WHERE mimetype = ? + AND filename = ? + AND NOT isobsolete', + undef, + 'text/x-github-pull-request', + "github-$repo_filename-$pr_number-url.txt" + ); + + my %bug_ids_to_lock = map { $_ => 1 } @{$candidate_bug_ids // []}; + $bug_ids_to_lock{$bug->id} = 1; + my @lock_bug_ids = sort { $a <=> $b } keys %bug_ids_to_lock; + my $lock_placeholders = join(', ', ('?') x @lock_bug_ids); + + $dbh->selectcol_arrayref( + "SELECT bug_id + FROM bugs + WHERE bug_id IN ($lock_placeholders) + ORDER BY bug_id + FOR UPDATE", + undef, + @lock_bug_ids + ); + + my $existing_attachments = Bugzilla::Attachment->match({ + bug_id => $bug->id, + mimetype => 'text/x-github-pull-request', + }); + + foreach my $existing_attachment (@$existing_attachments) { + next if $existing_attachment->data ne $html_url; + $template->process('global/code-error.html.tmpl', + {error => 'github_pr_attachment_exists'}, \$message) + || die $template->error(); + $duplicate = 1; + last; + } + + if ($duplicate) { + $dbh->bz_commit_transaction; + 1; + } + else { + + my $timestamp = $dbh->selectrow_array("SELECT NOW()"); + + $attachment = Bugzilla::Attachment->create({ + bug => $bug, + creation_ts => $timestamp, + data => $html_url, + description => "[$repository] $title (#$pr_number)", + filename => "github-$repo_filename-$pr_number-url.txt", + ispatch => 0, + isprivate => 0, + mimetype => 'text/x-github-pull-request', + }); + + # Insert a comment about the new attachment into the database. + $bug->add_comment( + '', + { + type => CMT_ATTACHMENT_CREATED, + extra_data => $attachment->id, + is_markdown => (Bugzilla->params->{use_markdown} ? 1 : 0) + } + ); + $bug->update($timestamp); + + # Fixup attachments with same github pull request but on different bugs + my %other_bugs; + my $other_attachments = Bugzilla::Attachment->match({ + mimetype => 'text/x-github-pull-request', + filename => "github-$repo_filename-$pr_number-url.txt", + WHERE => {'bug_id != ? AND NOT isobsolete' => $bug->id} + }); + foreach my $attachment (@$other_attachments) { + + # data doesn't match this URL, skip it + next if $attachment->data ne $html_url; + + $other_bugs{$attachment->bug_id}++; + my $moved_comment + = "GitHub pull request attachment was moved to bug " + . $bug->id + . ". Setting attachment " + . $attachment->id + . " to obsolete."; + $attachment->set_is_obsolete(1); + $attachment->bug->add_comment( + $moved_comment, + { + type => CMT_ATTACHMENT_UPDATED, + extra_data => $attachment->id, + is_markdown => (Bugzilla->params->{use_markdown} ? 1 : 0) + } + ); + $attachment->bug->update($timestamp); + $attachment->update($timestamp); + } + + $dbh->bz_commit_transaction; + 1; + } + } or do { + my $error = $@; + $dbh->bz_rollback_transaction; + _release_pr_lock($dbh, $pr_lock_name); + die $error; + }; + + _release_pr_lock($dbh, $pr_lock_name); + + if ($duplicate) { + return $self->render(json => {error => 1, message => $message}); } # Return new attachment id when successful return $self->render(json => {error => 0, id => $attachment->id}); } +sub _acquire_pr_lock { + my ($dbh, $repository, $pr_number) = @_; + + # Named advisory locks are MySQL-specific. bmo uses MySQL in production. + return undef if !$dbh->isa('Bugzilla::DB::Mysql'); + + my $lock_name + = 'github-pr:' + . substr(hmac_sha256_hex("$repository#$pr_number", 'github_pr_lock'), 0, 54); + + my ($locked) = $dbh->selectrow_array( + 'SELECT GET_LOCK(?, ?)', + undef, + $lock_name, + 30 + ); + + return $locked ? $lock_name : undef; +} + +sub _release_pr_lock { + my ($dbh, $lock_name) = @_; + return if !$lock_name; + $dbh->selectrow_array('SELECT RELEASE_LOCK(?)', undef, $lock_name); +} + sub push_comment { my ($self) = @_; my $template = Bugzilla->template; @@ -204,7 +302,7 @@ sub push_comment { return $self->render(json => {error => 0}); } - # Validate JSON input + # Validate JSON input my $payload = $self->req->json; my @errors = joi->object->props( ref => joi->string->required, @@ -435,7 +533,7 @@ sub _set_status_flag { # In order to determine the appropriate status flag for the default # branch, we have to find out what the current *nightly* Firefox version is. - # fetch_product_versions() calls an API endpoint maintained by rel-eng that + # fetch_product_versions() calls an API endpoint maintained by rel-eng that # returns all of the current product versions so we can use that. my $version; if ($branch eq 'main' || $branch eq 'master') { diff --git a/Bugzilla/DB/Schema.pm b/Bugzilla/DB/Schema.pm index 477c65f92c..fadbc7c241 100644 --- a/Bugzilla/DB/Schema.pm +++ b/Bugzilla/DB/Schema.pm @@ -572,6 +572,7 @@ use constant ABSTRACT_SCHEMA => { attachments_bug_id_idx => ['bug_id'], attachments_creation_ts_idx => ['creation_ts'], attachments_modification_time_idx => ['modification_time'], + attachments_filename_isobsolete_bug_id_idx => ['filename', 'isobsolete', 'bug_id'], attachments_submitter_id_idx => ['submitter_id', 'bug_id'], attachments_ispatch_idx => ['ispatch'], ], diff --git a/Bugzilla/Install/DB.pm b/Bugzilla/Install/DB.pm index 3da8b9a3d1..26d4befa0d 100644 --- a/Bugzilla/Install/DB.pm +++ b/Bugzilla/Install/DB.pm @@ -698,6 +698,11 @@ sub update_table_definitions { # 2013-08-16 glob@mozilla.com - Bug 905925 $dbh->bz_add_index('attachments', 'attachments_ispatch_idx', ['ispatch']); + # 2026-07-27 justdave@bugzilla.org - Bug 2058190 + $dbh->bz_add_index('attachments', + 'attachments_filename_isobsolete_bug_id_idx', + [qw(filename isobsolete bug_id)]); + # 2014-06-09 dylan@mozilla.com - Bug 1022923 $dbh->bz_add_index('bug_user_last_visit', 'bug_user_last_visit_last_visit_ts_idx', diff --git a/qa/t/rest_github_pull_request.t b/qa/t/rest_github_pull_request.t index cc6360bf71..f18594f1d6 100644 --- a/qa/t/rest_github_pull_request.t +++ b/qa/t/rest_github_pull_request.t @@ -11,6 +11,7 @@ use 5.10.1; use lib qw(lib ../../lib ../../local/lib/perl5); use Bugzilla; +use Bugzilla::Attachment; use Bugzilla::Logging; use MIME::Base64 qw(decode_base64); @@ -18,6 +19,23 @@ use Mojo::JSON 'true'; use QA::Util qw(get_config generate_payload_signature); use Test::Mojo; use Test::More; +use Time::HiRes qw(usleep); + +{ + no warnings 'redefine'; + my $original_create = \&Bugzilla::Attachment::create; + *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); + } + return $original_create->(@_); + }; +} my $config = get_config(); my $api_key = $config->{admin_user_api_key}; @@ -167,6 +185,87 @@ $t->post_ok( ->json_like('/message' => qr/The pull request contained a bug ID that already has an attachment/); +# Verify the bug still has exactly one active attachment for this PR URL. +$t->get_ok( + $url . "rest/bug/$bug_id/attachment" => {'X-Bugzilla-API-Key' => $api_key}) + ->status_is(200); + +my $bug_attachments = $t->tx->res->json->{bugs}->{$bug_id} // []; +my @matching_pr_attachments + = grep { ($_->{content_type} // '') eq 'text/x-github-pull-request' } + @$bug_attachments; +is(scalar @matching_pr_attachments, 1, + 'Repeated delivery keeps only one PR attachment on the bug'); + +# Create another bug and deterministically overlap two identical webhook +# deliveries. The monkeypatch above delays attachment create so both deliveries +# are in-flight at the same time in unlocked implementations. +$t->post_ok( + $url . 'rest/bug' => {'X-Bugzilla-API-Key' => $api_key} => json => $new_bug) + ->status_is(200)->json_has('/id'); + +my $race_bug_id = $t->tx->res->json->{id}; +my $race_payload = { + action => 'opened', + pull_request => { + html_url => 'https://github.com/mozilla-bteam/bmo/pull/2', + title => "Bug $race_bug_id - Test GitHub PR Linking", + number => 2, + }, + repository => {full_name => 'foo/bar'} +}; + +my @child_pids; +my %child_exit_counts; +local $ENV{BMO_GITHUB_PR_TEST_CREATE_DELAY_US} = 500_000; + +foreach my $i (1 .. 2) { + my $pid = fork(); + die "Unable to fork for race test" unless defined $pid; + + if ($pid == 0) { + my $child_t = Test::Mojo->new(); + my $tx = $child_t->ua->post( + $url + . 'rest/github/pull_request' => { + 'X-Hub-Signature-256' => generate_payload_signature($secret, $race_payload), + 'X-GitHub-Event' => 'pull_request' + } => json => $race_payload + ); + + my $res = $tx->result; + exit 2 unless $res && $res->is_success; + + my $json = $res->json // {}; + my $error = defined $json->{error} ? $json->{error} : 99; + exit($error == 0 ? 0 : ($error == 1 ? 1 : 3)); + } + + push @child_pids, $pid; +} + +foreach my $pid (@child_pids) { + waitpid($pid, 0); + my $exit_code = $? >> 8; + $child_exit_counts{$exit_code}++; +} + +is($child_exit_counts{0} // 0, 1, + 'One overlapping delivery creates the GitHub PR attachment'); +is($child_exit_counts{1} // 0, 1, + 'Second overlapping delivery is rejected as duplicate'); + +$t->get_ok( + $url . "rest/bug/$race_bug_id/attachment" => {'X-Bugzilla-API-Key' => $api_key}) + ->status_is(200); + +my $race_bug_attachments = $t->tx->res->json->{bugs}->{$race_bug_id} // []; +my @race_pr_attachments + = grep { ($_->{content_type} // '') eq 'text/x-github-pull-request' } + @$race_bug_attachments; +is(scalar @race_pr_attachments, 1, + 'Overlapping deliveries commit exactly one PR attachment'); + # Create a second bug for testing attaching the same github pr but to a # different bug. For example if someone changes the bug ID in the title # of an existing pull request. The first attachment should be obsoleted