From 823dd829ff1b87e10b3f8186570e8adb66e01016 Mon Sep 17 00:00:00 2001 From: Mikko Numminen Date: Thu, 16 Jul 2026 16:45:27 +0300 Subject: [PATCH] =?UTF-8?q?fix(gpu):=20harden=20multi-user=20GPU=20content?= =?UTF-8?q?ion=20=E2=80=94=20gate=20/health,=20align=20Ollama,=20jittered?= =?UTF-8?q?=20desk=20retry=20(ADR-0040)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Shed-not-queue is kept (it is right for an interactive desk on one 8B GPU), but the audit found three sharp edges under a crowd: 1. /health bypassed the LlmGate — it fired a real 1-token completion on every probe with no slot, adding uncontrolled concurrent GPU calls and able to steal a slot from a live request. Now it acquires a slot NON-BLOCKING (LlmGate.TryRunAsync / WaitAsync(0)): runs the probe only if a slot is free, and treats a saturated gate as healthy (busy => the model is demonstrably loaded and generating) without a completion. A genuine failure still 503s. 2. Ollama ran at defaults (OLLAMA_MAX_QUEUE=512) UNDER the app's fast-shed gate, so an admitted call could stall for minutes in a hidden deep queue. Compose now sets OLLAMA_NUM_PARALLEL=2 (both admitted slots run in parallel) and a shallow OLLAMA_MAX_QUEUE=8, so the two layers express the same policy. This is why we do NOT lean on Ollama's queue: we want no deep queue at all, and the app gate carries per-app policy Ollama can't. 3. A desk 503 dropped straight to the busy message with no resilience. Interpret now retries a bounded few times with JITTERED backoff so transient sheds self-heal and a roomful of retries desynchronize; a 429 (shared rate bucket, not the GPU) shows a distinct wait message and is never retried. Unit-tested (LlmGate.TryRunAsync: runs when free, skips-without-blocking when saturated, releases on throw; RunAsync sheds). Runtime-verified on the failure path (Ollama down => /health 503 llm_unavailable across repeated probes, proving the slot is released each time; /interpret 503; /live/feedback still 200). The real-GPU idle-probe path was not driven live because the sibling RAG stack is up and owns the GPU (isolation rule). Restructure per-item slotting and the shared browser rate bucket are documented as accepted tradeoffs. --- docker-compose.yml | 11 +++ .../0040-gpu-contention-hardening.md | 88 ++++++++++++++++++ docs/decisions/README.md | 1 + docs/operations.md | 2 +- src/FeedbackIntelligence.Api/LlmGate.cs | 49 +++++++--- src/FeedbackIntelligence.Api/Program.cs | 21 ++++- .../wwwroot/desk.html | 25 +++-- .../LlmGateTests.cs | 92 +++++++++++++++++++ 8 files changed, 265 insertions(+), 24 deletions(-) create mode 100644 docs/decisions/0040-gpu-contention-hardening.md create mode 100644 tests/FeedbackIntelligence.Api.Tests/LlmGateTests.cs diff --git a/docker-compose.yml b/docker-compose.yml index 7020d6b..6223294 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -20,6 +20,17 @@ services: # set anywhere). 4096 fits Phase 0 structuring prompts; revisit for the # Phase 4 synthesis window. Larger costs VRAM per loaded model. OLLAMA_CONTEXT_LENGTH: ${OLLAMA_CONTEXT_LENGTH:-4096} + # Concurrency MUST agree with the app-side LlmGate (Ingest:LlmMaxConcurrency + # = 2, shed-not-queue), or the two layers fight: the gate fast-sheds at 2 + # while Ollama silently deep-queues under it (default OLLAMA_MAX_QUEUE=512), + # so a call the gate admits can still stall for minutes in a hidden backlog + # bounded only by Ingest:LlmCallTimeoutMs. NUM_PARALLEL=2 lets both admitted + # slots actually run in parallel instead of being serialized; a SHALLOW + # queue makes Ollama shed too rather than hide a deep one (ADR-0040). Safe + # on VRAM because this container runs only while the shared RAG is down, so + # the whole GPU is ours. Tunable via env if a future GPU is tighter. + OLLAMA_NUM_PARALLEL: ${OLLAMA_NUM_PARALLEL:-2} + OLLAMA_MAX_QUEUE: ${OLLAMA_MAX_QUEUE:-8} ports: - "11434:11434" healthcheck: diff --git a/docs/decisions/0040-gpu-contention-hardening.md b/docs/decisions/0040-gpu-contention-hardening.md new file mode 100644 index 0000000..d125493 --- /dev/null +++ b/docs/decisions/0040-gpu-contention-hardening.md @@ -0,0 +1,88 @@ +# ADR-0040 — GPU contention under many simultaneous users: shed, don't queue (and make the layers agree) + +- **Status:** Accepted (2026-07-14) +- **Deciders:** Mikko + +## Context + +The app runs one 8B model (Poro-2-8B, Q4) on a single GPU that is time-shared +with a sibling project's RAG. A demo is shown to a room, so "many people use it +at once" is the expected load, not an edge case. The question: what happens when +concurrent users all need the GPU, and do we queue? + +The existing design already sheds rather than queues (`LlmGate`, a 2-slot +semaphore: a caller waits at most `LlmAcquireTimeoutMs = 500 ms` for a slot, +then gets `LlmBusyException` → HTTP 503). A concurrency audit confirmed the +shed is the right call for an interactive desk, but found three sharp edges: + +1. **`/health` bypassed the gate.** It fired a real 1-token completion on every + probe with no slot acquisition, so health checks added uncontrolled + concurrent GPU calls on top of the 2 slots and could steal a slot from a + live request under load. +2. **Ollama's own concurrency was unconfigured.** Beneath the app's fast-shed + gate, `ollama serve` ran at defaults — `OLLAMA_MAX_QUEUE = 512`. A call the + gate admits could then stall for minutes in a *hidden* deep queue bounded + only by `LlmCallTimeoutMs = 120 s`. The two layers disagreed: the app + fast-sheds, Ollama deep-queues. +3. **A shed on the desk had no client resilience.** A single transient 503 + dropped the clerk straight to the busy message, and a roomful of manual + re-clicks could re-collide in lockstep and also synchronize into the shared + rate-limit bucket (one Azure egress IP → one 240/60 s bucket, ADR-0025). + +### Why not just use Ollama's queue? + +Because we do not want a deep queue at all. On one 8B GPU a 512-deep queue means +an interactive "Tulkitse" click could wait minutes; a fast "busy, try again" +beats a spinner that resolves in three minutes. And the app-level gate expresses +policy Ollama cannot: cap *this* app at 2 concurrent so it stays a good neighbour +to the sibling RAG, distinguish shed-vs-degrade per endpoint (ingest sheds and +stores nothing so the client retries; the report swallows the shed and renders +its deterministic layer), and enforce the per-call timeout. Ollama's queue is +tuned for batch throughput, not latency-bounded interactivity. The fix is not to +adopt Ollama's queue but to make Ollama *agree* with the gate. + +## Decision + +1. **Gate `/health`.** The probe acquires a slot **non-blocking** + (`LlmGate.TryRunAsync`, `WaitAsync(0)`): if a slot is free it runs the + 1-token completion; if the gate is saturated it returns `status: "ok"` + without a completion, because a full gate already proves the model is loaded + and generating. Health can never add GPU load beyond the 2-slot bound nor + steal a slot from a live request. A genuine failure (unreachable, or slower + than `HealthTimeoutSeconds = 10 s`) still returns `503 llm_unavailable`. +2. **Make Ollama agree with the gate** (`docker-compose.yml`): + `OLLAMA_NUM_PARALLEL = 2` (both slots the gate admits actually run in + parallel instead of being serialized) and `OLLAMA_MAX_QUEUE = 8` (shallow, so + Ollama sheds too rather than hiding a 512-deep backlog under the app's + fast-shed design). Both env-overridable; `NUM_PARALLEL = 2` is safe on VRAM + because this container runs only while the shared RAG is down, so the whole + GPU is ours. +3. **Client resilience on the desk** (`desk.html`): a 503 on interpret retries a + few times with **jittered** backoff (`300 + attempt·400 + random·500 ms`, up + to 2 retries) so transient sheds self-heal and a roomful of retries + desynchronize instead of re-colliding; then it falls to the manual busy + message. A 429 (shared rate bucket, not the GPU) is never retried — it shows + a distinct "too many requests, wait" message. + +## Consequences + +- The two concurrency layers now express the same policy (2 parallel, shallow + queue, shed fast) instead of fighting; there is no hidden deep queue under the + gate. +- Health is honest under load: busy → healthy without adding load; idle → real + probe; dead/slow → `503 llm_unavailable`. `/health` consumers + (`feedctl` board, the public `/api/health` proxy probe) read `status == "ok"` + and need no change. +- A transient GPU shed on the desk mostly self-heals silently; a sustained one + still shows the busy message quickly. Auto-retry is **bounded** (≤2, jittered) + so it smooths sheds without becoming a retry storm. +- **Accepted, not fixed:** `POST /live/restructure` still holds a slot per item + in a loop with no priority over live visitors. It is operator-only + (loopback-exempt, absent from the public proxy allowlist), so only the + operator can trigger it and they know a batch is running; a per-caller slot + reservation would be a larger change than the risk warrants. +- **Accepted, not fixed:** the browser-path rate-limit bucket is shared across + all public visitors (Azure egress IP). At demo scale (240/60 s ≈ 4 req/s + sustained) this is ample; the client-side 429 message + jittered 503 retry are + the mitigation. A per-visitor bucket would require the proxy to forward a + stable per-client key, which the SWA managed function does not. diff --git a/docs/decisions/README.md b/docs/decisions/README.md index cf06165..d8fc84c 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -50,3 +50,4 @@ these ADRs relocate that reasoning into durable records. | [0037](0037-category-keywords-service-premises.md) | Extend the category-keyword override to service/premises (kassa_palvelu, tilat_siisteys) declared last, so products always win and service is the fallback | Accepted | | [0038](0038-unrated-no-trend-no-model-narrative.md) | Unrated content carries no trend either, and demoted categories get a deterministic count-only moderation theme (no model narrative) | Accepted | | [0039](0039-graded-vulgarity-density-asiaton.md) | Graded vulgarity: a density-gated deterministic scorer forces dense, non-substantive profanity to `asiaton` (a lone swear in real feedback stays rated) | Accepted | +| [0040](0040-gpu-contention-hardening.md) | GPU contention: shed-not-queue is kept; gate the `/health` probe, make Ollama's parallelism/queue agree with the 2-slot gate, jittered client retry on 503 | Accepted | diff --git a/docs/operations.md b/docs/operations.md index 1ff21bb..67f86e3 100644 --- a/docs/operations.md +++ b/docs/operations.md @@ -89,7 +89,7 @@ Ported rather than reinvented; each was measured there. |---|---| | Input length cap | 800 chars | | Request-body cap | 16 KB | - | LLM concurrency | 2, with 0.5 s acquire-then-**shed** (never queue behind a busy GPU) | + | LLM concurrency | 2, with 0.5 s acquire-then-**shed** (never queue behind a busy GPU). Ollama is configured to match — `OLLAMA_NUM_PARALLEL=2`, shallow `OLLAMA_MAX_QUEUE=8` — so no hidden deep queue sits under the gate, and `/health` probes take a slot only if one is free ([ADR-0040](decisions/0040-gpu-contention-hardening.md)) | | Per-IP rate limit | 30 requests / 60 s | | Output-token cap | `num_predict` / `MaxOutputTokens` | diff --git a/src/FeedbackIntelligence.Api/LlmGate.cs b/src/FeedbackIntelligence.Api/LlmGate.cs index b0e8583..aabf276 100644 --- a/src/FeedbackIntelligence.Api/LlmGate.cs +++ b/src/FeedbackIntelligence.Api/LlmGate.cs @@ -26,17 +26,7 @@ public async Task RunAsync(Func> work, Cancella throw new LlmBusyException(); try { - // Bound the generation with a server-side deadline so a HUNG model call - // cannot hold this slot (and the request thread) forever. The linked - // source cancels on EITHER the caller's request-abort OR the timeout; - // on timeout the work throws OperationCanceledException carrying the - // timeout token (ct is NOT cancelled), so callers' request-cancellation - // guards (`when (ct.IsCancellationRequested)`) correctly treat it as an - // LLM failure (structure_failed / deterministic fallback), not a client - // disconnect. The finally releases the slot either way. - using var timeout = CancellationTokenSource.CreateLinkedTokenSource(ct); - timeout.CancelAfter(options.Value.LlmCallTimeoutMs); - return await work(timeout.Token); + return await RunHeldAsync(work, ct); } finally { @@ -44,5 +34,42 @@ public async Task RunAsync(Func> work, Cancella } } + /// Runs ONLY if a slot is immediately free + /// (non-blocking acquire, no wait). Returns false without running when the + /// gate is saturated — the caller decides what a busy gate means. Used by + /// /health, which must never add GPU load beyond the gate nor steal a slot + /// from a live request: a full gate already proves the model is loaded and + /// generating, so "busy" is itself a healthy answer. Exceptions from the + /// work (e.g. the health timeout) propagate; the slot is always released. + public async Task TryRunAsync(Func work, CancellationToken ct) + { + if (!await _slots.WaitAsync(0, ct)) + return false; + try + { + await RunHeldAsync(async innerCt => { await work(innerCt); return null; }, ct); + return true; + } + finally + { + _slots.Release(); + } + } + + // Bound the generation with a server-side deadline so a HUNG model call + // cannot hold this slot (and the request thread) forever. The linked source + // cancels on EITHER the caller's request-abort OR the timeout; on timeout + // the work throws OperationCanceledException carrying the timeout token (ct + // is NOT cancelled), so callers' request-cancellation guards + // (`when (ct.IsCancellationRequested)`) correctly treat it as an LLM failure + // (structure_failed / deterministic fallback), not a client disconnect. + // Assumes a slot is already held; the caller's finally releases it. + private async Task RunHeldAsync(Func> work, CancellationToken ct) + { + using var timeout = CancellationTokenSource.CreateLinkedTokenSource(ct); + timeout.CancelAfter(options.Value.LlmCallTimeoutMs); + return await work(timeout.Token); + } + public void Dispose() => _slots.Dispose(); } diff --git a/src/FeedbackIntelligence.Api/Program.cs b/src/FeedbackIntelligence.Api/Program.cs index f4651ed..1cc0cfd 100644 --- a/src/FeedbackIntelligence.Api/Program.cs +++ b/src/FeedbackIntelligence.Api/Program.cs @@ -414,15 +414,26 @@ await reports.ReadLatestSnapshotHtmlAsync(ct) is { } html : Results.NotFound()); // Health = a 1-token REAL completion (RAG-measured pattern): "server up" does -// not mean "model loaded and generating". -app.MapGet("/health", async (IServiceProvider services, IOptions options, CancellationToken ct) => +// not mean "model loaded and generating". GATED (ADR-0040): the probe takes a +// slot ONLY if one is immediately free, so it can never add GPU load beyond the +// 2-slot LlmGate nor steal a slot from a live request under load. A saturated +// gate already proves the model is loaded and generating, so TryRunAsync +// returning false ("busy") is itself a healthy answer — no probe needed. +app.MapGet("/health", async ( + IServiceProvider services, LlmGate gate, IOptions options, CancellationToken ct) => { var client = services.GetRequiredKeyedService(LlmServiceCollectionExtensions.StructuringKey); try { - using var timeout = CancellationTokenSource.CreateLinkedTokenSource(ct); - timeout.CancelAfter(TimeSpan.FromSeconds(options.Value.HealthTimeoutSeconds)); - _ = await client.GetResponseAsync("ping", new ChatOptions { MaxOutputTokens = 1 }, timeout.Token); + // false = gate full = model busy = up (no completion run). true = ran + // the 1-token probe successfully. A throw = the model is unreachable or + // too slow to answer within HealthTimeoutSeconds → caught below. + await gate.TryRunAsync(async slotCt => + { + using var timeout = CancellationTokenSource.CreateLinkedTokenSource(slotCt); + timeout.CancelAfter(TimeSpan.FromSeconds(options.Value.HealthTimeoutSeconds)); + _ = await client.GetResponseAsync("ping", new ChatOptions { MaxOutputTokens = 1 }, timeout.Token); + }, ct); return Results.Ok(new { status = "ok" }); } catch (Exception ex) when (ex is not OperationCanceledException || !ct.IsCancellationRequested) diff --git a/src/FeedbackIntelligence.Api/wwwroot/desk.html b/src/FeedbackIntelligence.Api/wwwroot/desk.html index 8a8cadc..0fc7a87 100644 --- a/src/FeedbackIntelligence.Api/wwwroot/desk.html +++ b/src/FeedbackIntelligence.Api/wwwroot/desk.html @@ -126,7 +126,7 @@

interpretHeading: "Tulkinta — tarkista ja korjaa tarvittaessa", theme: "Aihe", severity: "Vakavuus", type: "Tyyppi", cancel: "Peruuta", save: "Tallenna", writeFirst: "Kirjoita palaute ensin.", interpreting: "Tulkitaan…", - busy: "Ruuhkaa — yritä hetken päästä uudelleen.", interpretFailed: "Tulkinta epäonnistui.", + busy: "Ruuhkaa — yritä hetken päästä uudelleen.", rateLimited: "Liikaa pyyntöjä juuri nyt — hetki ja yritä uudelleen.", interpretFailed: "Tulkinta epäonnistui.", modelFailed: "Automaattinen tulkinta ei onnistunut — täytä kentät itse.", interpretDone: "Tulkinta valmis — tarkista alta.", connError: "Yhteysvirhe — palvelin ei vastaa.", serverDown: "Palvelin ei vastaa — yritetään uudelleen…", @@ -160,7 +160,7 @@

interpretHeading: "Interpretation — review and correct if needed", theme: "Theme", severity: "Severity", type: "Type", cancel: "Cancel", save: "Save", writeFirst: "Write the feedback first.", interpreting: "Interpreting…", - busy: "Busy — try again in a moment.", interpretFailed: "Interpretation failed.", + busy: "Busy — try again in a moment.", rateLimited: "Too many requests right now — wait a moment and retry.", interpretFailed: "Interpretation failed.", modelFailed: "Automatic interpretation failed — fill the fields yourself.", interpretDone: "Interpretation ready — review below.", connError: "Connection error — the server is not responding.", serverDown: "Server not responding — retrying…", @@ -270,12 +270,23 @@

el("interpret").disabled = true; setStatus("status1", "", L.interpreting); try { - const res = await fetch(`${API}/interpret`, { - method: "POST", - headers: { "Content-Type": "application/json" }, - body: JSON.stringify({ text }), - }); + // A 503 is the GPU gate shedding (2 slots busy, ADR-0040) — usually + // transient. Retry a FEW times with JITTERED backoff so a roomful of clerks + // clicking at once don't re-collide in lockstep; give up to the manual busy + // message after that. A 429 is the shared rate bucket (ADR-0025), NOT the + // GPU — never hammer it, just tell the user to wait. + let res; + for (let attempt = 0; ; attempt++) { + res = await fetch(`${API}/interpret`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ text }), + }); + if (res.status !== 503 || attempt >= 2) break; + await new Promise(r => setTimeout(r, 300 + attempt * 400 + Math.random() * 500)); + } if (res.status === 503) { setStatus("status1", "warn", L.busy); return; } + if (res.status === 429) { setStatus("status1", "warn", L.rateLimited); return; } if (!res.ok) { setStatus("status1", "err", L.interpretFailed); return; } const data = await res.json(); startEntry(text); diff --git a/tests/FeedbackIntelligence.Api.Tests/LlmGateTests.cs b/tests/FeedbackIntelligence.Api.Tests/LlmGateTests.cs new file mode 100644 index 0000000..cdb5141 --- /dev/null +++ b/tests/FeedbackIntelligence.Api.Tests/LlmGateTests.cs @@ -0,0 +1,92 @@ +using Microsoft.Extensions.Options; +using FeedbackIntelligence.Api; + +namespace FeedbackIntelligence.Api.Tests; + +public class LlmGateTests +{ + private static LlmGate Gate(int slots, int acquireMs = 500, int callMs = 120_000) => + new(Options.Create(new IngestOptions + { + LlmMaxConcurrency = slots, + LlmAcquireTimeoutMs = acquireMs, + LlmCallTimeoutMs = callMs, + })); + + [Fact] + public async Task TryRunAsync_RunsWork_WhenASlotIsFree() + { + var gate = Gate(slots: 2); + var ran = false; + + var acquired = await gate.TryRunAsync(_ => { ran = true; return Task.CompletedTask; }, CancellationToken.None); + + Assert.True(acquired); // a free slot means the work runs + Assert.True(ran); + } + + [Fact] + public async Task TryRunAsync_SkipsWork_WhenGateIsSaturated_WithoutBlocking() + { + // ADR-0040: /health must never add GPU load beyond the gate. Hold BOTH + // slots, then a non-blocking probe returns false immediately WITHOUT + // running its work and WITHOUT waiting the acquire timeout. + var gate = Gate(slots: 2, acquireMs: 10_000); // long acquire — the probe must not wait it + var release = new TaskCompletionSource(); + var occupied = new TaskCompletionSource(); + var held = 0; + + async Task Hold(CancellationToken _) + { + if (Interlocked.Increment(ref held) == 2) occupied.SetResult(); + await release.Task; + } + var h1 = gate.RunAsync(async ct => { await Hold(ct); return null; }, CancellationToken.None); + var h2 = gate.RunAsync(async ct => { await Hold(ct); return null; }, CancellationToken.None); + await occupied.Task; // both slots now held + + var probeRan = false; + var sw = System.Diagnostics.Stopwatch.StartNew(); + var acquired = await gate.TryRunAsync(_ => { probeRan = true; return Task.CompletedTask; }, CancellationToken.None); + sw.Stop(); + + Assert.False(acquired); // gate full — not acquired + Assert.False(probeRan); // ...so the probe work never ran + Assert.True(sw.ElapsedMilliseconds < 1_000); // and it did NOT wait the 10 s acquire timeout + + release.SetResult(); + await Task.WhenAll(h1, h2); + + // Slots freed — a probe now runs again (release really returned them). + Assert.True(await gate.TryRunAsync(_ => Task.CompletedTask, CancellationToken.None)); + } + + [Fact] + public async Task TryRunAsync_ReleasesSlot_EvenWhenWorkThrows() + { + var gate = Gate(slots: 1); + + await Assert.ThrowsAsync(() => + gate.TryRunAsync(_ => throw new InvalidOperationException("model down"), CancellationToken.None)); + + // The single slot must have been released despite the throw — the next + // acquire (RunAsync) succeeds rather than shedding. + var ran = await gate.RunAsync(_ => Task.FromResult(true), CancellationToken.None); + Assert.True(ran); + } + + [Fact] + public async Task RunAsync_Sheds_WhenNoSlotFreesWithinAcquireTimeout() + { + var gate = Gate(slots: 1, acquireMs: 50); + var release = new TaskCompletionSource(); + var holding = gate.RunAsync(async _ => { await release.Task; return null; }, CancellationToken.None); + await Task.Delay(20); // let the holder take the only slot + + await Assert.ThrowsAsync(() => + gate.RunAsync(_ => Task.FromResult(true), CancellationToken.None)); + + release.SetResult(); + await holding; + } +}