You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
ModelSlot::get_or_load in crates/vera-serve/src/provider_cache.rs:70-92 holds the slot lock
across the model load and awaits the loader inline. When a client disconnects during a cold load,
axum drops the handler future, the guard drops, and the partially-built model is discarded without
reaching the slot. The next request finds the slot empty and rebuilds from scratch.
The build itself is not cancelled with the future — it runs under spawn_blocking
(crates/vera-core/src/retrieval/local_reranker.rs:232-233, crates/vera-core/src/embedding/local_provider.rs:606,609), and spawn_blocking tasks run to
completion regardless of whether anyone is still awaiting the JoinHandle. So the work continues,
its result is dropped, and a second full load is paid.
Raised by cubic on #134
(#134 (comment)). Filing separately rather than
folding it into #134: that PR's change is per-slot caching, and this needs get_or_load's
contract to change, which deserves its own review round.
Half of the finding as originally stated did not reproduce. cubic predicted duplicate sessions
and a peak-memory spike. The duplicated work is real and measured below. The memory spike is not:
a burst of cancelled clients peaks lower than a burst that completes. Numbers for both are
below, because the distinction decides what the fix needs to be.
Reproduction
Debug build at e5ca97c (#134's head), default backend, --idle-timeout 300, machine load
average 2.4. RSS sampled every 20 ms. The reranker is used because #134 seeds the embedding model
at startup, so the reranker slot is the one that is still cold when the server is up.
Control — one uninterrupted cold load:
baseline RSS (reranker cold): 1024.86 MB
client B http=200 time=1.296584s
peak RSS: 1708.38 MB delta over baseline: 683.52 MB
One reranker session costs 683 MB and takes 1.30 s to build.
Client A disconnects 1.1 s into that 1.30 s load, then client B asks for the same model:
client A exit=28 (timed out and disconnected)
client B http=200 time=1.427886s
peak RSS: 1681.5 MB delta over baseline: 666.75 MB
time=1.427886s is the defect. B paid a full rebuild, so A's nearly-finished build never
reached the slot. Had A been allowed to finish, B would have hit the cache — which is exactly what
happens when the disconnect lands after the build completes:
client A exit=28, disconnecting after 1.25s
client B http=200 time=0.051060s <- cache hit
So the window is "client disconnects before the build completes", and inside it the next request
redoes the entire load.
What did not reproduce: the memory spike
Six clients disconnecting at staggered points during a cold load, against six clients making the
same requests and being allowed to finish. Interleaved, two runs each:
burst
baseline
peak
delta
cancelled
1015 MB
1493 MB
477 MB
completing
1017 MB
1678 MB
661 MB
cancelled
1018 MB
1528 MB
510 MB
completing
1018 MB
1678 MB
660 MB
A cancelled burst peaks below a completing burst, and below the 683 MB single-session control.
The reason is that the per-slot mutex is held across the load, so loads serialise: only one build
is ever inside the lock, and an abandoned build's session is dropped rather than accumulated. The
cost of this bug is wasted work and latency, not resident memory.
(One earlier single run showed 2041 MB, which is why the controlled A/B above exists. It did not
reproduce across four interleaved runs and I am treating it as contaminated.)
The consequence is one extra model load (~1.3 s here, longer on a release build with a real
model), not a leak. Nothing accumulates across occurrences.
What a fix has to change
Not a one-line change, because it alters get_or_load's contract rather than adding to it:
The loader is F: FnOnce() -> Fut with Fut owned by the caller, so the load lives and dies
with whoever initiated it. Making it survive its initiator means the slot owns the in-flight
load — a shared future, or a broadcast of the result — and later callers subscribe instead of
waiting on a mutex.
That makes the error type shared, so E must become Clone or be reduced to a shared
representation. Today acquire_embedding propagates the loader's Err unchanged, and fix(serve): cache each model per slot instead of rebuilding it per request #134 added a_failed_load_is_not_cached_and_a_later_load_succeeds to pin exactly that.
It needs a decision about what happens when every subscriber disconnects: finish the build and
cache it, or abandon it. Both are defensible and they differ in whether a disconnecting client
can make the server do work nobody asked for.
The test that matters is a cancellation test, and cancellation tests are easy to write
vacuously. It would need reinjection against the un-fixed get_or_load to be worth anything.
Environment
e5ca97c on citron07r:fix/serve-provider-cache (#134), macOS 15.6, debug build, default
local models.
Summary
ModelSlot::get_or_loadincrates/vera-serve/src/provider_cache.rs:70-92holds the slot lockacross the model load and awaits the loader inline. When a client disconnects during a cold load,
axum drops the handler future, the guard drops, and the partially-built model is discarded without
reaching the slot. The next request finds the slot empty and rebuilds from scratch.
The build itself is not cancelled with the future — it runs under
spawn_blocking(
crates/vera-core/src/retrieval/local_reranker.rs:232-233,crates/vera-core/src/embedding/local_provider.rs:606,609), andspawn_blockingtasks run tocompletion regardless of whether anyone is still awaiting the
JoinHandle. So the work continues,its result is dropped, and a second full load is paid.
Raised by cubic on #134
(#134 (comment)). Filing separately rather than
folding it into #134: that PR's change is per-slot caching, and this needs
get_or_load'scontract to change, which deserves its own review round.
Half of the finding as originally stated did not reproduce. cubic predicted duplicate sessions
and a peak-memory spike. The duplicated work is real and measured below. The memory spike is not:
a burst of cancelled clients peaks lower than a burst that completes. Numbers for both are
below, because the distinction decides what the fix needs to be.
Reproduction
Debug build at
e5ca97c(#134's head), default backend,--idle-timeout 300, machine loadaverage 2.4. RSS sampled every 20 ms. The reranker is used because #134 seeds the embedding model
at startup, so the reranker slot is the one that is still cold when the server is up.
Control — one uninterrupted cold load:
One reranker session costs 683 MB and takes 1.30 s to build.
Client A disconnects 1.1 s into that 1.30 s load, then client B asks for the same model:
time=1.427886sis the defect. B paid a full rebuild, so A's nearly-finished build neverreached the slot. Had A been allowed to finish, B would have hit the cache — which is exactly what
happens when the disconnect lands after the build completes:
So the window is "client disconnects before the build completes", and inside it the next request
redoes the entire load.
What did not reproduce: the memory spike
Six clients disconnecting at staggered points during a cold load, against six clients making the
same requests and being allowed to finish. Interleaved, two runs each:
A cancelled burst peaks below a completing burst, and below the 683 MB single-session control.
The reason is that the per-slot mutex is held across the load, so loads serialise: only one build
is ever inside the lock, and an abandoned build's session is dropped rather than accumulated. The
cost of this bug is wasted work and latency, not resident memory.
(One earlier single run showed 2041 MB, which is why the controlled A/B above exists. It did not
reproduce across four interleaved runs and I am treating it as contaminated.)
Impact
Bounded, which is why this is not urgent:
per slot per server lifetime, not one per request.
eviction.
model), not a leak. Nothing accumulates across occurrences.
What a fix has to change
Not a one-line change, because it alters
get_or_load's contract rather than adding to it:F: FnOnce() -> FutwithFutowned by the caller, so the load lives and dieswith whoever initiated it. Making it survive its initiator means the slot owns the in-flight
load — a shared future, or a broadcast of the result — and later callers subscribe instead of
waiting on a mutex.
Emust becomeCloneor be reduced to a sharedrepresentation. Today
acquire_embeddingpropagates the loader'sErrunchanged, andfix(serve): cache each model per slot instead of rebuilding it per request #134 added
a_failed_load_is_not_cached_and_a_later_load_succeedsto pin exactly that.cache it, or abandon it. Both are defensible and they differ in whether a disconnecting client
can make the server do work nobody asked for.
vacuously. It would need reinjection against the un-fixed
get_or_loadto be worth anything.Environment
e5ca97concitron07r:fix/serve-provider-cache(#134), macOS 15.6, debug build, defaultlocal models.