fix: recover from llama server segfault and prevent model eviction timeouts

[AngeloDanducci]

Pull Request

Issue

Fixes #1388

Description

We've had a recent uptick in CI flakes due to an ollama segfault (likely resource binding) this PR attempts to address that by rerunning when the issue by pinning the model and only allowing 1 model to be loaded at once for the quality check. Additionally it will retry flaky marked tests if the cause is core dump / segfault / readtimeout instead of just failing.

Testing

• Tests added to the respective file if code was changed
• New code has 100% coverage if code was added
• Ensure existing tests and github automation passes (a maintainer will kick off the github automation when the rest of the PR is populated)

Attribution

• AI coding assistants used

---

Adding a new component, requirement, sampling strategy, or tool?

If your PR adds or modifies one of the types below, check the matching box. A checklist of type-specific review items will be posted as a comment.

• Component
• Requirement
• Sampling Strategy
• Tool

NOTE: Please ensure you have an issue that has been acknowledged by a core contributor and routed you to open a pull request against this repository. Otherwise, please open an issue before continuing with this pull request.

[markstur]

This look reasonable and worth trying.

Sounds like it is still w-i-p

[ajbozarth]

I'm still digging into this, but I'm not convinced this is the right solution yet

[markstur]

Looks even better. I have no notes except CI hasn't passed yet.

Review bot has notes:

Remaining question: The warmup only happens once at collection time. If the server segfaults mid-test-run, will the retry logic successfully recover, or does the warmup need to happen again after a crash?

The PR author should monitor whether retries succeed after a segfault, or if the model needs re-warming after a crash.

[AngeloDanducci]

Good question, it currently does not re-warm.

It seemed like the segfault issue was already addressed and surfaced the new failure (requiring warmup) and no retries were used.

However this is a "let's fix this now" approach. I have some additional changes we can do if we end up deciding the best route is standing this back up after a failure as opposed to just a retry (restarting ollama serve, fast-fail on 500).

[markstur]

I'm still digging into this, but I'm not convinced this is the right solution yet

I'll defer to Alex's diggings. Otherwise, I'd approve as-in it looks like it is worth a try.

[ajbozarth]

Dug into this against #1388, #1318, #599, and the reverted #1381/#1387. The warmup refactor is clean and the rerun net is a sound safety mechanism — but there are a few things worth resolving before merge, and one that's structural. Inline comments cover the diff-anchored points; the rest are collected here because they touch code outside the diff.

1. The three pinning/eviction mechanisms disagree (structural). After this PR, three things set keep-alive and they don't agree:

• OLLAMA_KEEP_ALIVE=-1 (quality.yml) — pin forever, server default
• warm_ollama_models() — keep_alive=-1, once at collection
• pytest_runtest_teardown → evict_ollama_models() (conftest.py:685, not in this diff) — keep_alive=0 at every ollama module boundary

A per-request keep_alive=0 overrides the server default, and the teardown isn't gated on --group-by-backend, so it runs in CI. Net effect: models still get evicted + cold-reloaded at each module boundary. So the KEEP_ALIVE=-1 "never evict → no slow reload" benefit (the #1318 fix) is mostly nullified past the first module, and the collection warmup only protects the first ollama module. Since MAX_LOADED_MODELS=1 now bounds the memory that eviction was protecting against (#798 was model accumulation, impossible at a cap of 1), the teardown eviction is arguably redundant in CI — consider relaxing it, or reframe the "never evict" comment to acknowledge eviction still governs.

2. Worst-case runtime can regress to the exact problem #1388 describes. If MAX_LOADED_MODELS=1 doesn't actually prevent the crash (see inline note on the wrong-axis concern), a bad run reruns ~56 ollama tests up to 3× with 10s delays — potentially worse than the 40-min merge-queue stall this is trying to kill. The reruns are a good safety net only if the crash is now genuinely rare; they amplify a bad run rather than bound it. Worth a cap or a fast-fail path so one segfault can't balloon the run.

3. Bet worth stating explicitly. KEEP_ALIVE=-1 and MAX_LOADED_MODELS=1 are a matched pair — KEEP_ALIVE=-1 alone would pin every model touched (higher peak memory, more segfault risk); MAX_LOADED_MODELS=1 counterbalances. So the pair primarily fixes reload-churn (#1318), not the segfault. The actual segfault recovery is the rerun list. Framing the PR that way sets the right expectation if the segfault recurs.

[ajbozarth]

MAX_LOADED_MODELS=1 caps distinct resident models, not the parallel runner slots spawned for a single model — but #1381 blamed concurrent same-model requests (e.g. test_async_parallel_requests) for the segfault, which is the OLLAMA_NUM_PARALLEL axis, not this one. If that hypothesis holds, this env var won't prevent the crash and the reruns are what recover. See top-level point 1 re: the eviction teardown overriding this pin in CI.

[AngeloDanducci]

I don't think the hypothesis held as we reverted that PR.

[ajbozarth]

Claude says: Agreed — with #1381 reverted that axis is moot for this PR, and the new comment framing (pair fixes churn, reruns handle the crash) is exactly right. I confirmed the crash-signature approach still covers the openai/litellm clients: a real segfault surfaces as InternalServerError: Error code: 500 - {...segmentation fault (core dumped)...}, so segmentation fault matches all three variants — dropping bare 500/InternalServerError loses generic-500 coverage only, which is the intent.

[ajbozarth]

status code: 500 and InternalServerError are broad — a deterministic 500 from a genuinely malformed payload mellea constructs would now be retried 3× and could flake to green, eroding test signal. Consider anchoring to the crash signature (terminated / segmentation fault) rather than any 500. Minor: segmentation fault and core dumped are the same message, so one is redundant (harmless).

[AngeloDanducci]

Removed the 500/ISE, pending.

[ajbozarth]

Two things: (a) because the teardown evicts at each module boundary (top-level point 1), this only protects the first ollama module — later modules re-hit the cold-load path (#599) this is meant to prevent. (b) test_sofai_sampling.py::TestSOFAIIntegration is @ollama @e2e but not @qualitative, so it runs in CI with llama3.2:1b, which CI never pulls (ollama pull granite4.1:3b only). It likely 500s today and now burns 3 reruns × 10s before failing — does that test currently pass in CI? If it's meant to be CI-skipped, it needs @qualitative or a model-availability guard.

[AngeloDanducci]

https://github.com/generative-computing/mellea/blob/main/test/stdlib/sampling/test_sofai_sampling.py#L269

Looks like it's marked as qualitative to me.

[ajbozarth]

Claude says: You're right — L269 has @pytest.mark.qualitative, I misread by starting one line too low. Disregard (b). And (a) is resolved now that the teardown no-ops under the pin.

[AngeloDanducci]

Feedback addressed.

1. skips when pinned for the whole job now
2. I mentioned a fast fail but if this does solve the issue then it's not needed - willing to wait and see on this one
3. added a note about the matched pair.

[AngeloDanducci]

Added a restart, not sure this is something we want to keep long term but should help narrow down the root cause as the segfault will not cascade - investigation ongoing.

[planetf1]

Two questions on the recovery path, then a nit — otherwise this looks a sensible step forward, and the MAX_LOADED_MODELS=1 cap in particular should take real pressure off the runner.

1. Does /api/ps actually go non-200 when the runner dies? The restart_ollama_if_dead docstring describes the case where "the supervisor survives but fails to cleanly respawn the runner, leaving every subsequent request 500-ing" — but _ollama_healthy only probes GET /api/ps, which the supervisor answers from its own state without touching a runner. So in that exact scenario I'd expect /api/ps to still return 200 and the restart to no-op. The probe looks like it reliably catches a dead supervisor (port down → restart), which is the OOM-kills-everything case, rather than the live-supervisor/dead-runner case the docstring names. If you've got a crash log from #1388 handy, could you confirm what /api/ps returns at that point? If it stays 200, a tiny POST /api/generate against the pinned model would be a truer liveness check.

2. Would an OOM-kill match the rerun list? Memory-pressure crashes often surface as an OS OOM-kill (SIGKILL), which Ollama reports as signal: killed rather than segmentation fault / core dumped — so that variant wouldn't be reran. Probably fine in practice now that MAX_LOADED_MODELS=1 makes OOM far less likely, but if you want the reruns robust to memory crashes and not just SIGSEGV, adding signal: killed would cover it (and it's still a genuine crash signature, so it doesn't reopen the bare-500 concern).

Nit (not for this PR, just an observation): the new warm_ollama_models() warms one model while the pre-existing --group-by-backend warmup block warms two, so they could drift — might be worth unifying on OLLAMA_WARMUP_MODEL down the line.