Repository navigation
bug(engine): the request gate does not protect against the scheduler loop — a concurrent out-of-band forward aborts the process #461
Description
Found and fixed on issue/451-embeddings while adding /v1/embeddings. Filing separately
because the bug is general, not embeddings-specific, and would be lost if that branch were
reworked.
The defect
The per-request model gate is not sufficient to make an out-of-band forward pass safe.
ContinuousBatchSchedulerService's run loop drives forwards on the same model outside that
gate, by design — and it is on by default for paged-KV CPU models.
So any second code path that runs a forward while the scheduler loop is live races it. With the
embeddings guard removed, a concurrent /v1/completions generation and /v1/embeddings
request aborts the process:
CountdownEvent ... below zero
from the shared ComputeThreadPool. That is a hard process abort, not a bad result.
Why it matters beyond #451
Any future endpoint or background task that performs a forward outside the scheduler — a
reranker, a classifier head, a warm-up probe, a diagnostic capture — hits the same race. The
existing request gate looks like it protects the model, so the trap is easy to walk into.
The fix applied
AcquireModelAsync() added to ContinuousBatchSchedulerService; the run loop takes the same
lock around each Step, so an out-of-band forward interleaves between scheduler steps rather
than during one. Cost measured as one uncontended semaphore per forward pass.
Follow-ups
- Consider whether the gate should be made impossible to bypass (e.g. forwards only reachable
through a single guarded entry point) rather than relying on each caller remembering. - Check the CUDA and Vulkan scheduler paths for the same shape — per CLAUDE.md's
cross-backend rule, a correctness bug duplicated across backends must be fixed everywhere,
and this one was only exercised on the CPU paged path. - The regression test is an HTTP concurrency test; it was demonstrated to fail (process
abort) with the lease dropped.
Activity
fix(server): take the scheduler lease inside ExecuteAsync, not per ca…
test(server): pin that ExecuteAsync holds the scheduler lease (#461)
merge: issue/461-gate-in-execute into dev
jamesburton commentedon Sep 21, 2026
Follow-up 1 done on
dev(0a12b288, merged34a4d574) — and it turned up a live instance of thebug on the main generation path.
The exposure is wider than this issue records
The issue frames the risk as future callers — "a reranker, a classifier head, a warm-up probe".
It is already the present tense: streaming chat and streaming completions always take the
direct-generator path, regardless of whether a scheduler is running. Every streaming request that
overlapped a batched one was racing the loop, on the default configuration, today.
It does not abort constantly only because an idle loop performs no forward — the race needs a
batched request in flight at the same moment. That is precisely why it surfaced first in a test
that deliberately overlapped two endpoints rather than in ordinary use. The same applies to any
/v1/chat/completionsusing LoRA orlogprobs, which also fall through to the generator.The fix
The lease now lives in
ServerState.ExecuteAsync, inside the request gate, in the samegate-then-lease order the embeddings path had already documented. Every direct forward in the
server routes through that method, so protection is structural rather than a convention —
which is what follow-up 1 asked for.
EmbeddingsEndpointdrops its own acquisition; keeping itwould now self-deadlock, since
SemaphoreSlimis not reentrant.Checked for the deadlock this could have introduced: every
EnqueueAsynccall site sits outsideExecuteAsync, so nothing waits on the loop while holding the lease that blocks it.Cost, stated rather than left to be found
The lease is held for the whole of the work, so a long streaming generation stalls in-flight
batched requests for its duration. That is the correct trade — they cannot run concurrently in any
case — but it is a real throughput change for a mixed workload, not a free fix. It is written into
the XML doc, with a note that a caller needing finer granularity should take the lease around the
forward only.
Verification
Three tests reusing the existing service fixture. The first fails against a mutant restoring the
pre-fix body (#417).
The negative assertion needed care: "the lease is not grantable" can only be shown by a timeout,
and a lease acquired and never released would satisfy it just as well as correct serialisation.
So the same test also asserts the lease is grantable immediately after the work completes, and a
second asserts it is released when the work throws — otherwise one failed request would leave a
server that never generates again. A third pins that the scheduler-less configuration is unchanged.
Non-GPU unit suite: 2960 passed, 0 failed.
Follow-up 2 — cross-backend
Not a backend-specific path:
ContinuousBatchSchedulerServiceis backend-agnostic (it drivesIModel), and there is one service, one run loop and now one lease point, so CUDA and Vulkan arecovered by the same change rather than needing their own. The CLAUDE.md cross-backend rule is
satisfied by construction here, not by three separate patches — but the reason is worth recording
in case a backend ever grows its own scheduler loop.