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

Bug 43033: Sort unallocated task indices before compacting the retry matrix 

The previous two commits on this bug fixed two ordering assumptions in
the RETRY loop of _allocateWithTransportCostMatrix(): resetting $inf at
the start of each attempt, and splicing unallocated task indices out of
@requests in descending order (removing a lower index first would shift
the position of any higher index still waiting to be removed).

This is a third instance of the same underlying problem, a few lines
earlier in the loop that compacts the working cost matrix $m before
that splice runs. @unallocated is built by iterating over agents (item
rows), pushing the column index Munkres assigned to each row that
turned out infeasible - and Munkres' row-to-column assignment is a
permutation, not a monotonic function of the row index, so
@unallocated is no more reliably sorted here than it was for the
splice.

That compaction loop scans matrix columns in ascending order and
advances a single pointer into @unallocated, expecting to meet each
entry in that same ascending order. When @unallocated arrives out of
order, the pointer never catches up: columns that should have been
dropped are left in place, and the columns after them are never
shifted left to fill the gap. From that point on, $m no longer lines
up with @requests, which *is* correctly reduced by the splice - so on
the next retry pass a hold's transport cost gets evaluated using a
different hold's matrix row. Depending on the exact misalignment, this
can silently drop a fillable hold from the queue while a genuinely
available item sits unused, or fill a hold via the wrong-cost item.

It didn't show up in the "many holds" regression test added earlier on
this bug, because that test's transport cost matrix is uniform (every
disabled pair costs the same); this defect only surfaces once the
columns competing for a scarce item have distinct costs.

Sort @unallocated once, ascending, and use that copy for the
compaction pass; reverse it for the splice, which needs descending
order.

Test plan:

1. Run the new t/db_dependent/HoldsQueue/RetryAllocationOrdering.t -
   it calls _allocateWithTransportCostMatrix() directly with a small
   hand-built scarce transport cost matrix and a pinned srand() (the
   specific item-to-branch permutation needed to trigger the bug
   depends on the shuffle() inside the routine). Reverting just this
   patch makes 3 of its 6 subtests fail: the L2 pickup hold is dropped
   even though an available item could have filled it.
2. Run t/db_dependent/HoldsQueue/TransportCostOptimizations.t to
   confirm no regression in the existing retry-loop coverage.

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This commit is contained in:
Martin Renvoize 2026-07-09 17:07:28 +01:00 committed by Lucas Gass
parent 4d4bc5b18f
commit 76e719964c
No known key found for this signature in database
GPG Key ID: 60B447398B0D36CA
2 changed files with 148 additions and 3 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

16
C4/HoldsQueue.pm
View File

@ -668,12 +668,20 @@ RETRY:
if ( $retries-- > 0 && @unallocated && @remaining ) {
# @unallocated is built by iterating agents (rows), so its
# entries are not necessarily in column order: Munkres'
# row-to-column assignment is a permutation, not a monotonic
# mapping. The matrix compaction below scans columns in
# ascending order and expects to consume @unallocated in that
# same order, so it must be sorted first.
my @unallocated_sorted = sort { $a <=> $b } @unallocated;
# Remove the transport cost of unfilled holds and compact the matrix.
# Also remove the hold request from the array.
for ( my $i = 0 ; $i < $num_agents ; $i++ ) {
my $u = 0;
for ( my $j = 0 ; $j < $num_tasks ; $j++ ) {
if ( $u < scalar(@unallocated) && $unallocated[$u] == $j ) {
if ( $u < scalar(@unallocated_sorted) && $unallocated_sorted[$u] == $j ) {
$u++;
} elsif ( $u > 0 ) {
$m[$i][ $j - $u ] = $m[$i][$j];
@ -681,8 +689,10 @@ RETRY:
}
}
# Indices must be spliced out reverse order.
for my $u ( sort { $b <=> $a } @unallocated ) {
# Indices must be spliced out in reverse order, otherwise removing
# a lower index shifts the position of any higher index still to
# be removed.
for my $u ( reverse @unallocated_sorted ) {
splice @requests, $u, 1;
}
$num_tasks = scalar(@requests);

135
t/db_dependent/HoldsQueue/RetryAllocationOrdering.t Executable file
View File

@ -0,0 +1,135 @@
#!/usr/bin/perl
# Test that C4::HoldsQueue::_allocateWithTransportCostMatrix() correctly
# compacts its working matrix between retries, regardless of the order in
# which Munkres' row-to-column assignment happens to report unfilled holds.
#
# This calls the (private) allocation routine directly, with hand-built
# fixtures, rather than going through CreateQueue()/the database: the
# defect being tested only manifests for a specific item-to-branch
# permutation, and List::Util::shuffle() inside the routine makes that
# permutation depend on the interpreter's random state. Pinning srand()
# here keeps the test deterministic without depending on how many random
# numbers TestBuilder/Faker happen to consume building DB fixtures.
use Modern::Perl;
use Test::More tests => 6;
use Test::NoWarnings;
use C4::HoldsQueue;
# A minimal stand-in for a Koha::Item: only the methods this routine calls.
package t::lib::FakeTransferableItem;
sub new { my $class = shift; return bless {}, $class; }
sub can_be_transferred { return 1; }
sub item_group { return; }
package main;
# Branches double as both holding branches (agents) and pickup branches
# (tasks). The transport cost matrix is deliberately scarce:
# - L0 is the *only* holding branch that can serve pickups L0, L1 and L3.
# - L1, L2 and L3 can each *only* serve pickup L2.
# So among {L0, L1, L3} pickups, at most one can ever be filled (by L0);
# the other two are genuinely unfillable and must both be reported as
# unallocated in the same retry pass - this is what makes the ordering of
# @unallocated matter, since the matrix compaction assumes ascending order.
my @branches = qw(L0 L1 L2 L3);
my %cost = (
L0 => { L0 => 10, L1 => 11, L2 => undef, L3 => 12 },
L1 => { L0 => undef, L1 => undef, L2 => 5, L3 => undef },
L2 => { L0 => undef, L1 => undef, L2 => 6, L3 => undef },
L3 => { L0 => undef, L1 => undef, L2 => 7, L3 => undef },
);
sub build_transport_cost_matrix {
my %tcm;
for my $holding (@branches) {
for my $pickup (@branches) {
my $cost = $cost{$holding}{$pickup};
$tcm{$pickup}{$holding} =
defined $cost
? { cost => $cost, disable_transfer => 0 }
: { disable_transfer => 1 };
}
}
return \%tcm;
}
sub build_request {
my ( $branch, $reserve_id, $note ) = @_;
return {
itemnumber => undef,
allocated => undef,
branchcode => $branch,
borrowerbranch => $branch,
itemtype => undef,
item_group_id => undef,
borrowernumber => 1000 + $reserve_id,
biblionumber => 1,
reserve_id => $reserve_id,
item_level_hold => 0,
reservedate => '2024-01-01',
reservenotes => $note,
};
}
# One item per branch, and one initial hold request per branch, plus two
# extra pending requests for the always-fillable L2 pickup: this forces a
# retry (there are more requests than items) that reuses whatever the first
# pass left behind in the compacted matrix.
srand(80); # known to trigger a non-ascending @unallocated on the first pass
my @available_items;
my %items_by_itemnumber;
my $itemnumber = 1;
for my $branch (@branches) {
my $item = {
itemnumber => $itemnumber,
holdingbranch => $branch,
holdallowed => 'yes',
hold_fulfillment_policy => '',
itype => undef,
_object => t::lib::FakeTransferableItem->new,
};
push @available_items, $item;
$items_by_itemnumber{$itemnumber} = { _object => $item->{_object} };
$itemnumber++;
}
my $reserve_id = 1;
my @hold_requests = map { build_request( $_, $reserve_id++, "req_$_" ) } @branches;
push @hold_requests, build_request( 'L2', $reserve_id++, 'extra_0' );
push @hold_requests, build_request( 'L2', $reserve_id++, 'extra_1' );
my %libraries = map { $_ => { branchcode => $_ } } @branches;
my $allocated = C4::HoldsQueue::_allocateWithTransportCostMatrix(
\@hold_requests, \@available_items, \@branches, \%libraries,
build_transport_cost_matrix(), {}, \%items_by_itemnumber
);
my %holding_branch_used_for_note = map { $_->[1]{reservenotes} => $_->[1]{holdingbranch} } @$allocated;
ok( $holding_branch_used_for_note{req_L2}, 'the original L2 pickup request was allocated an item' );
my @l2_holding_branches = sort grep { defined } @holding_branch_used_for_note{qw(req_L2 extra_0 extra_1)};
is_deeply(
\@l2_holding_branches,
[qw(L1 L2 L3)],
'all three L2 pickups were filled, one from each of the three branches that can supply L2 (no idle item, no dropped hold)'
);
ok(
!defined $holding_branch_used_for_note{req_L1},
'req_L1 is genuinely unfillable: L0 is the only item that could serve it, and it is cheaper to use L0 for req_L0'
);
ok(
!defined $holding_branch_used_for_note{req_L3},
'req_L3 is genuinely unfillable for the same reason'
);
my $filled = grep { defined } values %holding_branch_used_for_note;
is( $filled, 4, 'exactly 4 of the 6 pending requests could be filled with the 4 available items' );