Use the thread-safe aliases in the kZeroObjSingletonContinuousCol undo case - #3306
Conversation
…o case HighsPostsolveStack::undo aliases reductionValues/colValues/rowValues to thread-local copies when thread_safe is set. The kZeroObjSingletonContinuousCol case pops from the members instead, so with thread_safe = true it reads the member stack at a position no one reset and mutates state shared with the other workers. HighsDataStack::pop(std::vector<T>&) reads the element count from the stack at that position and resizes to it, so a stale position yields an arbitrary count: std::length_error when it exceeds max_size(), std::bad_alloc when it does not, and an out-of-bounds read when position underflows. HighsMipWorker::transformNewIntegerFeasibleSolution is the only caller that passes thread_safe = true, which is why this only shows up in a parallel MIP search, and it throws on a task-executor worker thread with no handler above it, so the process terminates.
|
Thanks, even to me this looks OK and acceptable. Pattern-matching (at least) tells me that I see that |
|
@jajhall I should've spotted this when we merged @MathieuDutSik great spot! That bug currently completely corrupts some parallel MIP runs. Can you please remove the new test? I believe it is a bit overkill for this specific bit of the code and the parallel interaction. I'd then happily merge the change! |
fa9e648 to
5ee233d
Compare
Done. |
|
Spurious CI failures |
3 participants
Description
Fixes a crash in the parallel MIP search:
kZeroObjSingletonContinuousColis the oneHighsPostsolveStack::undocase that does not use the thread-safe aliases.undoaliases its three mutable members to thread-local copies whenthread_safeis set (HighsPostsolveStack.h:1069-1071):Fifteen of the sixteen
switchcases go through those aliases.kZeroObjSingletonContinuousColpops from the members instead, so withthread_safe = trueit reads the member stack at a position nobody reset —undoPrimalcallsresetPosition()on the member stack only in the serial path — and mutates state shared with the other workers.HighsDataStack::pop(std::vector<T>&)reads the element count from the stack at the current position and resizes to it, so a stale position yields an arbitrary count:std::length_errorwhen it exceedsmax_size(),std::bad_allocwhen it does not, and an out-of-bounds read whenpositionunderflows past zero. When it does not throw, the singleton comes back as 0 and the ranged row it was removed from is left violated.HighsMipWorker::transformNewIntegerFeasibleSolutionis the only caller passingthread_safe = true, which is why this appears only in a parallel MIP search. The throw lands on aHighsTaskExecutorworker with no handler above it, sostd::terminateruns and the process dies;Highs::optimizeModelTryCatchonly guards the main-thread chain, which is why the same corruption sometimes surfaces as a returnedErrorinstead.The case was added in
9cce7235d9("Remove continuous singletons from double-sided rows", 2026-04-10), seven months after848ced1785("Add thread safe undoPrimal option", 2025-09-11) introducedthread_safe, so the new case body was written against the pre-thread_safeshape.The fix is three lines, using the aliases already in scope.
Verification
clang-format --dry-run -Werroris clean on the changed file.Separately, on our fork (upstream
latestat939eba361dplus some local MIP-option commits, so not a cleanlatestbuild), a 2,982-row / 3,450-column / 540-integer dispatch model that aborted withstd::length_error3/3 at the shippedparallel=on,threads=0default now solves to optimality 5/5, and a larger model that aborted withstd::bad_allocviaRINS→solveSubMip→trySolutionsolves 3/3. The patched objectives match thethreads=1andparallel=offruns, so this restores correct postsolve rather than only suppressing the throw.Checklist
latestbranchThe analysis and patch were AI-assisted; I have reviewed and verified them. Leaving that box unchecked deliberately rather than misreporting it — the source change is the three-line alias swap, so please take it directly if that is the easier path for you.
Related issue
Closes #3305
Secondary and out of scope here:
HighsTaskExecutor::run_worker/HighsSplitDeque::runStolenTasklet anystd::exceptionescape a task tostd::terminate, which is what turns this bug into a process death rather than an error return.