fix: make the Reactant-compiled solve loop terminate and restore reporting

[gdalle-bot]

This PR was opened by a coding agent, please disregard it until @gdalle has reviewed it

Targets gd/reac2 (#150), not main.

Fixes the two Core failures on that branch, and a third problem they were hiding: the compiled solve! never returns.

1. The compiled solve loop never terminates

termination_check! returned stats.termination_status !== MOI.OPTIMIZE_NOT_CALLED, but termination_status is a plain MOI.TerminationStatusCode enum, so under Reactant that comparison is folded to a constant at trace time. The generated HLO shows it plainly — the outer loop's predicate is stablehlo.not %iterArg, and the body returns a constant false into that same slot:

%25:16 = stablehlo.while(%iterArg = %c_10, ...)
 cond {
   %26 = stablehlo.not %iterArg : tensor<i1>
   stablehlo.return %26 : tensor<i1>
 } do {
   ...
   stablehlo.return %c_10, ...     // %c_10 = stablehlo.constant dense<false>
 }

So the predicate is always true. Compilation itself was fine (~90s); execution simply never came back — which is why the Reactant job hung rather than failed.

set_termination_status!! now also returns the decision as a possibly-traced boolean, and that is what the solve loops branch on. The dead should_terminate!! was already exactly this shape and is retired in favour of it.

While reordering those branches I also restored the precedence that the pre-refactor if/elseif had. The three @trace ifs were in order optimal → time → iteration, and since the last write wins, ITERATION_LIMIT was silently overriding OPTIMAL whenever both held. They are now ordered by increasing priority.

2. The error history is never filled

This is what test/tutorial.jl:176 tripped on:

Expression: last_err < first_err
 Evaluated: 3.8440421930617714e-7 < 3.8440421930617714e-7

The optimization is not stuck — 3.84e-7 is below the requested 1e-6. The two values are identical because error_history held exactly one element, so first and last returned the same tuple.

Two causes. The push! in termination_check! was commented out, so nothing was ever appended. And the ConvergenceStats default seeded the vector with the same mutable KKTErrors object stored in stats.err, so that one entry was overwritten in place by kkt_errors! on every check.

The seed itself is worth keeping — it is what gives to_rarray a non-empty vector to convert, so the field type and its value agree under tracing. It just needs to be a snapshot rather than an alias:

error_history = [(kkt_passes, copy(err))]

That alone is not enough, because KKTErrors(sol) fills with NaN and initialize passed exactly that, which would leave first_err == NaN and the comparison false. So initialize now fills the errors of the starting point before building the stats. The history therefore starts at a genuine measurement at 0 KKT passes, which is also what a convergence plot wants. For PDLP this is the same single kkt_errors! call it already made for err_restart (now copied from it); for PDHG it adds one evaluation at initialization.

New tests

The tutorial should not have been the only thing standing between this and main, so test/components/termination.jl gets an Error history testset covering each failure mode independently: that the history actually grows, that it is indexed by KKT passes and ends at stats.kkt_passes, that consecutive entries differ, that the last entry matches the final live errors, that entries are independent snapshots (mutating stats.err afterwards must not rewrite them), that the seed is finite rather than NaN, and that record_error_history = false leaves just the seed.

Against the code on gd/reac2 these fail 10 ways, on five independent assertions:

Expression: length(history) > 1                              Evaluated: 1 > 1
Expression: length(history) == 1 + div(max_kkt_passes, check_every)   Evaluated: 1 == 11
Expression: last(passes) == stats.kkt_passes                 Evaluated: 0 == 100
Expression: first(recorded) != last(recorded)                Evaluated: 0.786… != 0.786…
Expression: all(err -> err !== stats.err, errors)            (aliasing)

3. Stale ProgressMeter imports

The other Core failure: the progress bar was dropped from both solve! methods, leaving ProgressUnknown, finish! and next! imported but unused. Deleting the import only trades it for an Aqua stale-dependency failure, and dropping the dependency would silently turn the public show_progress option (used by the MOI wrapper via :show_progress => !dest.silent) into a no-op.

So instead of deleting either feature, the progress bar and the history recording are gated on ReactantCore.within_compile() — no-ops inside a compiled region, unchanged on the CPU path:

function init_progress(desc::String, show_progress)
    within_compile() && return nothing
    return ProgressUnknown(; desc, enabled = show_progress)
end

This costs nothing when Reactant is not compiling: within_compile() is @inlined to false, so Base.infer_return_type(init_progress, Tuple{String, Bool}) is ProgressUnknown, not a Union.

4. CI

timeout-minutes: 45 on the test job — it inherited the 6-hour default, which is how the Reactant job ran 1h17m before the concurrency rule cancelled the workflow. Coverage is also skipped for the Reactant group, since the instrumentation covers Reactant's tracing machinery: 4m10s with coverage vs 2m28s without, measured locally.

On making Reactant compilation faster

Measured rather than assumed, and two of the obvious levers turned out not to work:

lever	result
smaller instance (afiro 32×27 vs 25fv47 1571×821)	no effect — PDHG 92.5s vs 90.6s. Compile time tracks graph structure, not tensor shapes. Not worth changing the test.
@compile optimize = :none	fails — 'enzyme.batch' op unsupported op for export to XLA. The optimization passes are what lower Enzyme ops into exportable StableHLO.
skip coverage in CI	1.7× on that job (above)

I did not lower Julia's own optimization level: Reactant traces through Julia's abstract interpreter, so it depends on inference running normally, and --compile=min in particular tends to break that.

The honest summary is that compilation was never the problem — it is ~90s for PDHG and ~107s for PDLP regardless. The group appeared to take forever because it was executing an infinite loop. It now runs in 2m28s.

Verification

All run locally on top of b75745b:

check	before	after
Core	5649 pass, 2 fail	5654 pass, 0 fail
Reactant	hangs (killed at 50 min)	4 pass, 2m28s
MOI	—	1827 pass
termination.jl (new testset)	10 fail	65 pass
runic --check src/	clean	clean

(The exact totals wobble between runs: the milp.jl testset at line 135 samples 20 random Netlib instances via randperm, and instances on its skip list contribute one @test_skip instead of three passes. Nothing to do with these changes.)

error_history now matches main exactly on the same random instance (1000 entries, first_err = 0.972…, last_err = 192.20…, no aliasing).

🤖 Generated with Claude Code

https://claude.ai/code/session_01WdrfQSAANYeyin33qaW6F4

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

Files with missing lines	Coverage Δ
src/CoolPDLP.jl	100.00% <ø> (ø)
src/algorithms/common.jl	98.66% <100.00%> (+22.39%)	⬆️
src/algorithms/pdhg.jl	95.55% <100.00%> (+3.05%)	⬆️
src/algorithms/pdlp.jl	97.82% <100.00%> (+3.57%)	⬆️
src/components/termination.jl	100.00% <100.00%> (+40.62%)	⬆️

... and 17 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.