Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
justdave wants to merge 6 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6ec0eed
Bug 2058190: [GitHub] fix race condition on adding PRs to bugs
justdave Jul 27, 2026
93ddb23
Address review comments
justdave Jul 27, 2026
03067ba
Address additional review comments
justdave Jul 27, 2026
39a351f
Add an additional index to the attachments table to make the lock acq…
justdave Jul 27, 2026
c62e1f5
Add FOR UPDATE clause to SQL query in pull_request method to address …
justdave Jul 27, 2026
8679246
Implement advisory locking for pull requests
justdave Jul 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
232 changes: 165 additions & 67 deletions Bugzilla/API/V1/Github.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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 = ?
Comment thread
justdave marked this conversation as resolved.
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;
Expand All @@ -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,
Expand Down Expand Up @@ -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') {
Expand Down
1 change: 1 addition & 0 deletions Bugzilla/DB/Schema.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'],
],
Expand Down
5 changes: 5 additions & 0 deletions Bugzilla/Install/DB.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
Loading
Loading