@codex review

Codex Review: Didn't find any major issues. Hooray!

Reviewed commit: 5c5cde030f

This fixes the ROCm library mismatch, but it shares the same two-attempt budget as the existing --fit retry. Could we give the ROCm fallback its own retry and add a behavioral test for that sequence?

@Etherll Agreed. The ROCm env retry was sharing the same (0, 1) slot as --fit

Pushed a follow-up: ROCm and --fit each gets their own retry (three launches max). Also added a behavioural test for mix → bundled HIP → --fit on, and one that a healthy bundled-HIP retry does not flip --fit

@codex review

@codex review

Codex Review: Didn't find any major issues. What shall we delve into next?

Reviewed commit: bca0bcfc74

@indrajeetapache thank you so much for building out this fix, am pushing my review changes, cleaned up a few tests and methods

Reviewed at 7d3b762. Diagnosis and shape are right: this is a loader version mix, --fit cannot fix a missing symbol, and giving the ROCm correction its own flag rather than sharing the --fit slot is the correct answer to the earlier P2. The fragile parts hold up: _stdout_lines resets per attempt so one crash cannot classify the next, and the WSL librocdxg prepend is genuinely untouched because /dev/dxg short-circuits _native_linux_system_rocm_lib_dirs first.

I pushed four corrections plus a merge of latest main to your branch rather than asking you to round-trip. The first one does not show up in a normal run, and your test plan does not include test_gpu_init_crash_message.py at all, so there was no way for you to see it.

1. The behavioral test asserted against the wrong process. _run_full_offload_spawns patches subprocess.Popen globally, so on a host with the rocm_sdk wheel it also captures the offload-arch probe that discover_current_target_family() shells out to inside load_model. Running your test on its own:

0 LD=None                        cmd0=offload-arch
1 LD='/opt/rocm/lib:/bundle/bin' cmd0=/fake/llama-server  --fit off
2 LD='/opt/rocm/lib:/bundle/bin' cmd0=/fake/llama-server  --fit on

That lands at index 0, shifts everything, and eats returncodes[0] = 127. error is None, loaded, and len(launches) == 3 still pass, so the test looks alive while the retry never ran (launch 2 still carries /opt/rocm/lib). rocm_sdk caches the lookup per process, so whichever test hits load_model first absorbs the probe and everything after it in that process is clean, which is why a full-file run passes and an isolated one does not. The harness now records only the fake server and delegates the rest to the real Popen.

2. The detector could not match the other object this failure has. _native_linux_system_rocm_lib_dirs prepends the whole system ROCm dir on purpose, and its own docstring says mixing the bundle's rocBLAS with a different-version system HIP "risks missing symbols". So librocblas or libhipblaslt failing the same lookup is predicted by our own code, and it was falling through to the generic 127 "reinstall llama-server" plus a wasted --fit retry. It now parses glibc's format and matches the ROCm family on the object's basename, so a path like /opt/librocm-vendor/lib/ cannot stand in for the object and libggml-hip.so still does not match.

3. The message named a symbol the predicate did not require. It hardcoded hsa_amd_queue_create while the check also fired on any rocr_ match. It now interpolates what the loader actually reported.

4. Small one: _retry_ld != env.get("LD_LIBRARY_PATH") is "" != None on Windows and macOS, which would set an empty LD_LIBRARY_PATH and spend an attempt. Unreachable today, but if _retry_ld and ... closes it.

I also added a test for when bundle-only does not help. Both and not _hip_rocr_mismatch guards had no coverage: deleting them broke nothing. That test pins them and stops at two launches instead of burning --fit on a symbol error. And I dropped the three source-text assertions in test_hip_rocr_retry_is_checked_before_the_fit_on_retry, since the behavioral tests already prove ordering and budget while assert "for _spawn_attempt in (0, 1, 2)" in body breaks on a no-op refactor.

Two things still open that are not code review. No workflow has run here, so pre-commit is the only green check. And the hardware line in your test plan is unchecked, so nobody has run this on a host that reproduces the crash. Worth asking the reporter, since the premise is one line on their machine: if UNSLOTH_LLAMA_NO_SYSTEM_ROCM=1 makes the same load succeed on main, this PR automates exactly that.

For the record, two follow-ups, not asks for this PR: the durable fix is comparing system ROCm against the bundle's version before launching rather than after a crash, and the correction does not outlive the load, so the STT sidecar builds the same env with no retry and stays broken on this host.

Confirmed and fixed in c4af9a8. This was a regression I introduced: the old text hardcoded the strings, so it was bounded by construction, and interpolating the captures removed that. _drain_stdout only does rstrip() per line with no cap, so an oversized token does reach _stdout_lines whole, _short is documented for exactly this, and every other capture in _classify_start_failure_text already uses it. Both interpolations are now wrapped, with a test covering an 8 KB object and an 8 KB symbol.

Also pushed a comment pass in 07c4f0cc, separate commit. The fix had accumulated twelve issue citations across four files, several of them restating the line underneath. Kept #8998 where it identifies the crash signature and #7233 where it names the prepend being undone, dropped the other ten, and cut the prose that repeated the code. One was already stale: the ggml test said the ROCm branch "names libamdhip64", which stopped being true once the detector started matching the whole ROCm family.

213 pass, ruff clean, formatted with the pinned ruff 0.6.9.

Follow-up in 6d9e83c, closing the gap I flagged as out of scope last time.

The problem it fixes. The retry repaired one launch and recorded nothing. So on a host that hits the mix, every load rebuilt the prepend and crashed into it again before recovering, and the STT sidecar never recovered at all: stt_mtmd_sidecar.py:156 builds its child env through the same _llama_server_env_for_binary and has no retry of its own, which left dictation broken on the host chat had just been fixed on.

The fix. The build dir is recorded once a bundle-only launch reaches health, and the prepend is skipped there from then on. The sidecar needed no code of its own: it calls the same helper, so it inherits the correction. Three constraints keep it safe. Written only from that proof, so a host the #7233 prepend is right for cannot land in it. Keyed on the resolved build/bin dir, so a second install tree still gets the prepend. In-process only, so a ROCm or driver upgrade re-tests on restart. Locked rather than a bare set, since the sidecar spawns on its own thread and can overlap a chat load.

Tests cover both directions, including that a retry which does not fix the crash leaves no record, and the harness resets the state per run so a correction cannot leak between tests. Still unverified on real hardware, same as the rest of the PR.

Confirmed and fixed in f525e59. The repo had already solved this 200 lines up: the missing-library branch at llama_cpp.py:11544 splits on the ": " that introduces the diagnostic for exactly this reason, and its comment uses the same /opt/My Runtime/ example. Mine was the inconsistent one.

One thing worth recording, since the obvious fix has a cost. Widening \S+? to [^\r\n]+? restores spaced paths but also lets the lazy capture expand to end of line on every symbol lookup error: occurrence, and _drain_stdout keeps an unterminated line whole. On a 400 KB line carrying 200 of those prefixes the classifier went from 3 ms to 803 ms. Bounding the object to PATH_MAX keeps spaced paths and brings that to 16 ms, better than the original, since no longer path can exist anyway.

Tests: a bundle under /opt/My Runtime/ now classifies as the mix, and an object past PATH_MAX does not.

Confirmed and fixed in 64c0899. _did_rocm_retry was local to _spawn_and_wait, but the correction edits the env dict built in the enclosing scope, so it outlives that call: -archfallback, -noflash, -noflash-mtp, -retry and -cpu all call _spawn_and_wait again with the corrected env and a fresh flag. A load that recovered through any of them came up healthy and recorded nothing.

Reproduced before fixing. Driving load_model with mismatch, signal crash, signal crash, healthy gives four launches through two _spawn_and_wait calls: the correction is applied on launch 0 and the load succeeds on launch 3, and the preference was not recorded. Hoisting the flag to the load scope records it. The mutation holds in both directions, so the new test fails if the flag goes back to per-call.

Worth noting the flag only widens where a success is recorded. It cannot cause a second correction, since the retry is still guarded on the rebuilt LD_LIBRARY_PATH differing from the one in use, which it no longer does once applied.

Confirmed this hits the path in studio/backend/core/inference/llama_cpp.py where the system ROCm prepend is applied unconditionally and the resulting 127 is then treated as a VRAM problem and sent into the --fit retries. Will get this reviewed.

@codex review

@codex review

Codex Review: Didn't find any major issues. Already looking forward to the next diff.

Reviewed commit: b4ee47b519

@codex review

Codex Review: Didn't find any major issues. Bravo.

Reviewed commit: b38490f0a2