From 76e719964c90393472014aa0be18d22b8f723081 Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Thu, 9 Jul 2026 17:07:28 +0100 Subject: [PATCH] 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 --- C4/HoldsQueue.pm | 16 ++- .../HoldsQueue/RetryAllocationOrdering.t | 135 ++++++++++++++++++ 2 files changed, 148 insertions(+), 3 deletions(-) create mode 100755 t/db_dependent/HoldsQueue/RetryAllocationOrdering.t diff --git a/C4/HoldsQueue.pm b/C4/HoldsQueue.pm index f2615038e8..33b6eb6873 100644 --- a/C4/HoldsQueue.pm +++ b/C4/HoldsQueue.pm @@ -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); diff --git a/t/db_dependent/HoldsQueue/RetryAllocationOrdering.t b/t/db_dependent/HoldsQueue/RetryAllocationOrdering.t new file mode 100755 index 0000000000..3979624916 --- /dev/null +++ b/t/db_dependent/HoldsQueue/RetryAllocationOrdering.t @@ -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' );