gitea
/
Koha-clms
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Bug 41801: (QA follow-up) Prevent race condition with txn_do and FOR UPDATE 

FixPriority has a read-modify-write structure: it SELECTs all active
holds for a biblionumber, computes new priority order in Perl, then
issues N UPDATEs.  Two concurrent callers (e.g. two patrons adjusting
holds on a popular title at the same moment) could both read the same
snapshot and then overwrite each other's renumbering, leaving duplicate
or incorrect priority values.

Fix by:

1. Wrapping the entire function body in txn_do so it runs atomically.
   txn_do uses MySQL savepoints when called inside an existing
   transaction, so callers that already hold a transaction are unaffected.

2. Adding FOR UPDATE to the initial SELECT so the first caller
   acquires row-level locks on all holds for that bib.  A second
   concurrent caller will block at the SELECT until the first commits,
   then reads the already-corrected priorities and produces a consistent
   result.

Note: FOR UPDATE locks only rows that exist at read time; a concurrent
INSERT of a brand-new hold can still slip through, but priority
assignment for new holds goes via AddReserve → FixPriority, which will
also acquire the lock and renumber correctly.
This commit is contained in:
Martin Renvoize 2026-03-09 10:13:33 +00:00
parent 4d61ac803f
commit e0bdf3e859
No known key found for this signature in database
GPG Key ID: 2D86E0EAEC83711B
1 changed files with 98 additions and 91 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

189
C4/Reserves.pm
View File

@ -1665,101 +1665,108 @@ priority adjusted to ensure that they remain at the end of the line.
=cut
sub FixPriority {
my ($params) = @_;
my $reserve_id = $params->{reserve_id};
my $rank = $params->{rank} // '';
my $biblionumber = $params->{biblionumber};
my ($params) = @_;
my $dbh = C4::Context->dbh;
Koha::Database->new->schema->txn_do(
sub {
my $reserve_id = $params->{reserve_id};
my $rank = $params->{rank} // '';
my $biblionumber = $params->{biblionumber};
my $hold;
if ($reserve_id) {
$hold = Koha::Holds->find($reserve_id);
if ( !defined $hold ) {
my $dbh = C4::Context->dbh;
# may have already been checked out and hold fulfilled
require Koha::Old::Holds;
$hold = Koha::Old::Holds->find($reserve_id);
my $hold;
if ($reserve_id) {
$hold = Koha::Holds->find($reserve_id);
if ( !defined $hold ) {
# may have already been checked out and hold fulfilled
require Koha::Old::Holds;
$hold = Koha::Old::Holds->find($reserve_id);
}
return unless $hold;
}
unless ($biblionumber) { # FIXME This is a very weird API
$biblionumber = $hold->biblionumber;
}
if ( $rank eq "del" ) { # FIXME will crash if called without $hold
$hold->cancel;
} elsif ( $reserve_id && ( $rank eq "W" || $rank eq "0" ) ) {
# make sure priority for waiting or in-transit items is 0
my $query = "
UPDATE reserves
SET priority = 0
WHERE reserve_id = ?
AND found IN ('W', 'T', 'P')
";
my $sth = $dbh->prepare($query);
$sth->execute($reserve_id);
}
my @priority;
# Lock all active holds for this bib so concurrent FixPriority calls
# for the same hold set cannot interleave their read-modify-write cycle.
# FOR UPDATE is effective inside the enclosing txn_do transaction.
my $query = "
SELECT reserve_id, borrowernumber, reservedate, lowestPriority
FROM reserves
WHERE biblionumber = ?
AND ((found <> 'W' AND found <> 'T' AND found <> 'P') OR found IS NULL)
ORDER BY lowestPriority ASC, priority ASC
FOR UPDATE
";
my $sth = $dbh->prepare($query);
$sth->execute($biblionumber);
while ( my $line = $sth->fetchrow_hashref ) {
push( @priority, $line );
}
# FIXME This whole sub must be rewritten, especially to highlight what is done when reserve_id is not given
# To find the matching index
my $i;
my $key = -1; # to allow for 0 to be a valid result
for ( $i = 0 ; $i < @priority ; $i++ ) {
if ( $reserve_id && $reserve_id == $priority[$i]->{'reserve_id'} ) {
$key = $i; # save the index
last;
}
}
# if this hold is marked lowest priority, we can only move it so far;
# cap rank to just after the last non-lowestPriority hold using the
# already-fetched @priority array (avoids a second DB query and stale data)
if ( $hold && $hold->lowestPriority && $rank ne 'del' && $rank > 0 ) {
my $non_lowest_count = scalar grep { !$_->{lowestPriority} } @priority;
$rank = $non_lowest_count + 1 if $non_lowest_count && $rank <= $non_lowest_count;
}
# if index exists in array then move it to new position
if ( $key > -1 && $rank ne 'del' && $rank > 0 ) {
my $new_rank = $rank - 1; # $new_rank is what you want the new index to be in the array
my $moving_item = splice( @priority, $key, 1 );
$new_rank = scalar @priority if $new_rank > scalar @priority;
splice( @priority, $new_rank, 0, $moving_item );
}
# now fix the priority on those that are left....
# only updating if changed
$query = "
UPDATE reserves
SET priority = ?
WHERE reserve_id = ? AND priority != ?
";
$sth = $dbh->prepare($query);
for ( my $j = 0 ; $j < @priority ; $j++ ) {
$sth->execute(
$j + 1,
$priority[$j]->{'reserve_id'}, $j + 1
);
}
}
return unless $hold;
}
unless ($biblionumber) { # FIXME This is a very weird API
$biblionumber = $hold->biblionumber;
}
if ( $rank eq "del" ) { # FIXME will crash if called without $hold
$hold->cancel;
} elsif ( $reserve_id && ( $rank eq "W" || $rank eq "0" ) ) {
# make sure priority for waiting or in-transit items is 0
my $query = "
UPDATE reserves
SET priority = 0
WHERE reserve_id = ?
AND found IN ('W', 'T', 'P')
";
my $sth = $dbh->prepare($query);
$sth->execute($reserve_id);
}
my @priority;
# get what's left, sorting lowestPriority holds to the bottom
my $query = "
SELECT reserve_id, borrowernumber, reservedate, lowestPriority
FROM reserves
WHERE biblionumber = ?
AND ((found <> 'W' AND found <> 'T' AND found <> 'P') OR found IS NULL)
ORDER BY lowestPriority ASC, priority ASC
";
my $sth = $dbh->prepare($query);
$sth->execute($biblionumber);
while ( my $line = $sth->fetchrow_hashref ) {
push( @priority, $line );
}
# FIXME This whole sub must be rewritten, especially to highlight what is done when reserve_id is not given
# To find the matching index
my $i;
my $key = -1; # to allow for 0 to be a valid result
for ( $i = 0 ; $i < @priority ; $i++ ) {
if ( $reserve_id && $reserve_id == $priority[$i]->{'reserve_id'} ) {
$key = $i; # save the index
last;
}
}
# if this hold is marked lowest priority, we can only move it so far;
# cap rank to just after the last non-lowestPriority hold using the
# already-fetched @priority array (avoids a second DB query and stale data)
if ( $hold && $hold->lowestPriority && $rank ne 'del' && $rank > 0 ) {
my $non_lowest_count = scalar grep { !$_->{lowestPriority} } @priority;
$rank = $non_lowest_count + 1 if $non_lowest_count && $rank <= $non_lowest_count;
}
# if index exists in array then move it to new position
if ( $key > -1 && $rank ne 'del' && $rank > 0 ) {
my $new_rank = $rank - 1; # $new_rank is what you want the new index to be in the array
my $moving_item = splice( @priority, $key, 1 );
$new_rank = scalar @priority if $new_rank > scalar @priority;
splice( @priority, $new_rank, 0, $moving_item );
}
# now fix the priority on those that are left....
# only updating if changed
$query = "
UPDATE reserves
SET priority = ?
WHERE reserve_id = ? AND priority != ?
";
$sth = $dbh->prepare($query);
for ( my $j = 0 ; $j < @priority ; $j++ ) {
$sth->execute(
$j + 1,
$priority[$j]->{'reserve_id'}, $j + 1
);
}
);
}
=head2 _Findgroupreserve