Skip to content

Commit 878a0d0

Browse files
localai-botmudler
andauthored
fix(distributed): reaper reaps live backends, ghost model stubs, in_flight leak, sidecar staging runaway (#11142)
* fix(distributed): stop the probe reaper from orphaning busy backends The reconciler's liveness probe is a 1s gRPC HealthCheck, and a single failed probe deleted the model's node_models row. A backend that is merely busy cannot answer it: single-threaded Python backends (video and avatar generation) block for minutes inside one request, so the reaper was deleting registry rows for backends that were alive and mid-request. The model then vanished from the nodes page while it was still generating, and because the row was gone the in-flight decrement had nothing to decrement ("DecrementInFlight: no matching row or already zero"). Every subsequent request re-routed and re-staged the full model from scratch. Two guards: - Replicas with in-flight requests are excluded in SQL. A row that is actively serving is proof of life, and the running request is exactly what stops the backend from answering the probe. - Idle replicas must miss three CONSECUTIVE probes before removal, so a transient blip cannot orphan a live replica. A successful probe resets the streak. Signed-off-by: Ettore Di Giacinto <mudler@localai.io> Assisted-by: Claude:claude-opus-5 [Claude Code] * fix(distributed): drop the local model stub when its last replica goes In distributed mode every routed model leaves an in-process stub in the frontend's ModelLoader, and DistributedModelStore.Range reports local stubs UNION the registry rows. Every registry removal path deletes only the DB row, so the stub outlived the replica and the model was reported as loaded forever. That is the "loaded on the home page, absent from every node" ghost: /system reads the union and still sees the stub, while /api/nodes/models reads the registry and correctly sees nothing. It never self-healed, and both frontend replicas showed it independently. The replica-removed chokepoint could not fix this as it stood, because it held a SINGLE hook that the prefix cache already owned, and it was registered only when the prefix cache was enabled. Registering a second listener would have silently displaced the first. - Turn replicaRemovedHook into a list (AddReplicaRemovedHook), so independent subsystems can each register without displacing others. - Add NewLocalStubInvalidator, which drops the local stub once no healthy replica of the model remains anywhere in the cluster, and wire it unconditionally in startup. The stub is kept while another node still serves the model: the frontend is right to consider it loaded, and each request re-routes through SmartRouter to pick a live replica anyway. Signed-off-by: Ettore Di Giacinto <mudler@localai.io> Assisted-by: Claude:claude-opus-5 [Claude Code] * fix(distributed): stop staging checksum sidecars back to workers The file transfer server writes a "<file>.sha256" sidecar next to every file it accepts. The sender walked the model directory with no filter, so it staged those sidecars too, and the receiver duly wrote a sidecar for each sidecar. Every staging pass multiplied the tree: config.json -> config.json.sha256 -> config.json.sha256.sha256 -> ... One LongCat snapshot had grown to 498 files, 466 of them chained, up to 29 levels deep, and the staged file count climbed on every pass. This inflates each transfer and grows disk without bound on both ends. Skip hash sidecars in stageDirectory, and mirror the skip in countStageableFiles so the progress bar still reaches 100%. The check is "a sidecar sitting next to a real file" rather than a blanket suffix ban, so a model that genuinely ships a .sha256 payload with no corresponding base file is still transferred. Signed-off-by: Ettore Di Giacinto <mudler@localai.io> Assisted-by: Claude:claude-opus-5 [Claude Code] * fix(distributed): classify the liveness probe instead of gating on in_flight The previous commit excluded replicas with in-flight requests from the probe reaper. That was the wrong guard, and could invert the bug it fixed. in_flight has no decrement guarantee: track() balances its increment with a defer, but a frontend killed mid-request never runs it, and the load-time reservation is released only when the first inference completes. Nothing resets a leaked counter. Gating the reaper on it therefore meant a leaked counter would shield a genuinely dead replica from ever being reaped. Nor was patience alone a fix: three misses at the default interval is ~90s of silence, while the generation that triggered this blocks for 15+ minutes. The real conflation was in the probe itself. A gRPC HealthCheck against the backend's serving port measures "is it idle enough to answer", not "does the process exist", and probeLoadedModels discarded the error that tells them apart. Because the gRPC client is lazy, the status code is decisive: - DeadlineExceeded: transport fine, nothing serviced the RPC. Busy. - Unavailable: nothing is listening. Gone. ModelProber now returns a ProbeOutcome, and only ProbeUnreachable counts toward the reap threshold. ProbeBusy clears the streak: it is evidence of life. A blackholed network reads as busy too, deliberately, since whole-node failure is the health monitor's job. Signed-off-by: Ettore Di Giacinto <mudler@localai.io> Assisted-by: Claude:claude-opus-5 [Claude Code] * feat(distributed): reconcile replicas against worker-reported processes Probing a backend's own serving port cannot distinguish "busy" from "gone" without inferring it from an error code. The worker can answer directly: it spawned the process, holds the handle, and its reply is not blocked by whatever that backend is doing. Adds a models.running request-reply subject. The worker answers out of its in-memory process table, reporting each live process as (modelID, replicaIndex, address) — the supervisor's process keys are `modelID#replica`, which is isomorphic to a NodeModel row, so the reconciler can diff the two directly. reconcileNodeProcesses runs before the port probe and reaps rows for models the worker is not running. Models the worker vouches for get updated_at bumped, which takes them out of the port prober's stale set entirely: that is what keeps a backend deep in a long generation away from the probe in the first place, rather than relying on classifying its silence after the fact. A worker that does not answer is skipped, not assumed empty. A messaging failure says nothing about the processes, and assuming the worst would delete a node's rows on a transient NATS blip; the port probe stays as the fallback for those nodes. Rows younger than probeStaleAfter are ignored so a freshly created row is never judged against a process table that has not caught up. Signed-off-by: Ettore Di Giacinto <mudler@localai.io> Assisted-by: Claude:claude-opus-5 [Claude Code] * fix(distributed): stop in_flight leaking and pin replicas against eviction A leaked in_flight counter is not cosmetic. FindLRUModel, FindGlobalLRUModelWithZeroInFlight and the router's eviction query all require in_flight = 0, so a replica whose counter never came back is pinned and its VRAM is unreclaimable for the lifetime of the process. Two halves. The source: routing reserves in_flight = 1 at load time so a freshly loaded replica is not evicted out from under the request that caused the load. That reservation was released ONLY by the first inference completing, so a route torn down before any inference ran (client disconnect, handler error, failure between load and the backend call) stranded it. newRouteResult now wires the reservation to a sync.Once fired by whichever comes first, the first inference or route teardown, and replaces three copies of the old wiring. The backstop: a sweeper for counters leaked by paths that cannot run a defer at all, such as a frontend killed mid-request. Identifying a leak by elapsed time alone is unsafe. IncrementInFlight stamps last_used at request START and nothing moves it while the request runs, so a long generation is indistinguishable from a leak by age, and resetting there would expose a serving model to eviction. The probe supplies the missing bit: a backend that answers a health check promptly is not inside a request, because that is precisely what a busy one cannot do. Requiring the row to also be idle for 30 minutes covers backends that serve in parallel and can answer while working, since those keep last_used fresh through each new increment. Two existing tests asserted the old behaviour ("No decrement on Release"). That assertion was the leak, so both now pin the release instead. Signed-off-by: Ettore Di Giacinto <mudler@localai.io> Assisted-by: Claude:claude-opus-5 [Claude Code] --------- Signed-off-by: Ettore Di Giacinto <mudler@localai.io> Co-authored-by: Ettore Di Giacinto <mudler@localai.io>
1 parent e9c2754 commit 878a0d0

22 files changed

Lines changed: 1784 additions & 101 deletions

core/application/distributed.go

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -286,13 +286,14 @@ func initDistributed(cfg *config.ApplicationConfig, authDB *gorm.DB, configLoade
286286
prefixProvider = prefixSync
287287

288288
// Invalidate the prefix-cache index whenever a replica row is removed.
289-
// SetReplicaRemovedHook fires from the single chokepoint all removal paths
289+
// AddReplicaRemovedHook fires from the single chokepoint all removal paths
290290
// funnel through (RemoveNodeModel / RemoveAllNodeModelReplicas), so this
291291
// one hook covers every path: reconciler scale-down, probe reaper,
292292
// health-monitor reap, RemoteUnloaderAdapter, and the router. Registering
293293
// it only inside this enabled block keeps the disabled path a true no-op
294-
// (the registry stays hook-less).
295-
registry.SetReplicaRemovedHook(func(model, node string, replica int) {
294+
// for the prefix cache; other subsystems register their own hooks
295+
// independently and are unaffected either way.
296+
registry.AddReplicaRemovedHook(func(model, node string, replica int) {
296297
if replica < 0 {
297298
prefixSync.InvalidateNode(model, node)
298299
} else {

core/application/startup.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -283,6 +283,14 @@ func New(opts ...config.AppOption) (*Application, error) {
283283
distSvc.Registry,
284284
)
285285
application.modelLoader.SetModelStore(distStore)
286+
// Drop the local stub when a model's last replica leaves the registry.
287+
// The store reports local stubs UNION registry rows, and every removal
288+
// path deletes the row only, so without this the frontend keeps
289+
// reporting a model as loaded long after the replica is gone.
290+
// Registered unconditionally: this is independent of the prefix cache.
291+
distSvc.Registry.AddReplicaRemovedHook(
292+
nodes.NewLocalStubInvalidator(distSvc.Registry, distStore),
293+
)
286294
// Start health monitor
287295
distSvc.Health.Start(options.Context)
288296
// Start replica reconciler for auto-scaling model replicas

core/services/messaging/subjects.go

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -365,6 +365,36 @@ type ModelDeleteReply struct {
365365
Error string `json:"error,omitempty"`
366366
}
367367

368+
// SubjectNodeModelsRunning asks a worker node which model backend processes it
369+
// currently has running. Uses NATS request-reply.
370+
//
371+
// This is the authoritative answer to "is this replica still alive". The worker
372+
// owns the process table, so unlike a health probe against the backend's own
373+
// serving port, its reply does not depend on whether that backend happens to be
374+
// busy: a model mid-generation cannot answer a gRPC health check for minutes at
375+
// a time, but the worker answers immediately either way.
376+
func SubjectNodeModelsRunning(nodeID string) string {
377+
return subjectNodePrefix + sanitizeSubjectToken(nodeID) + ".models.running"
378+
}
379+
380+
// ModelsRunningRequest is the payload for a models.running NATS request.
381+
type ModelsRunningRequest struct{}
382+
383+
// ModelsRunningReply is the response from a models.running NATS request.
384+
type ModelsRunningReply struct {
385+
Models []RunningModelInfo `json:"models"`
386+
Error string `json:"error,omitempty"`
387+
}
388+
389+
// RunningModelInfo identifies one live backend process on a worker. The triple
390+
// is isomorphic to a controller NodeModel row's (model_name, replica_index,
391+
// address), which is what lets the reconciler diff the two directly.
392+
type RunningModelInfo struct {
393+
ModelID string `json:"model_id"`
394+
ReplicaIndex int `json:"replica_index"`
395+
Address string `json:"address,omitempty"`
396+
}
397+
368398
// SubjectNodeStop tells a serve-backend node to shut down entirely
369399
// (deregister + exit). The node will not restart the backend process.
370400
func SubjectNodeStop(nodeID string) string {
Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
package nodes
2+
3+
import (
4+
"context"
5+
6+
"github.com/mudler/LocalAI/pkg/model"
7+
"github.com/mudler/xlog"
8+
)
9+
10+
// NewLocalStubInvalidator returns a replica-removed hook that drops the
11+
// frontend's in-process stub for a model once no healthy replica of it remains
12+
// anywhere in the cluster.
13+
//
14+
// Why this is needed: in distributed mode every routed model leaves a stub in
15+
// the frontend's ModelLoader, and DistributedModelStore.Range reports local
16+
// stubs UNION the registry rows. Every registry removal path deletes only the
17+
// DB row, so without this hook the stub outlives the replica and the model is
18+
// reported as loaded forever. That is the "loaded on the home page, absent from
19+
// every node" ghost: /system reads the union and still sees the stub, while
20+
// /api/nodes/models reads the registry and correctly sees nothing.
21+
//
22+
// The stub is dropped only when the LAST replica is gone. While another node
23+
// still serves the model the frontend is right to consider it loaded, and each
24+
// request re-routes through SmartRouter to pick a live replica anyway.
25+
//
26+
// This deletes the store entry directly rather than going through
27+
// ShutdownModel: the replica is already gone from the registry, so there is
28+
// nothing left to unload, and ShutdownModel would emit a pointless remote
29+
// unload to a node that no longer hosts it.
30+
func NewLocalStubInvalidator(registry *NodeRegistry, store model.ModelStore) func(modelID, nodeID string, replicaIndex int) {
31+
return func(modelID, nodeID string, replicaIndex int) {
32+
if registry == nil || store == nil || modelID == "" {
33+
return
34+
}
35+
if _, ok := store.Get(modelID); !ok {
36+
return // no local stub for this model, nothing to invalidate
37+
}
38+
39+
// Registry rows are the source of truth for "is this model loaded
40+
// anywhere". Only healthy nodes with a loaded row are returned.
41+
remaining, err := registry.FindNodesWithModel(context.Background(), modelID)
42+
if err != nil {
43+
// Leave the stub alone rather than risk dropping a live model on a
44+
// transient DB error. A later removal re-runs this check, and the
45+
// reconciler keeps probing, so the ghost is not permanent.
46+
xlog.Warn("Local stub invalidation skipped: failed to count remaining replicas",
47+
"model", modelID, "node", nodeID, "replica", replicaIndex, "error", err)
48+
return
49+
}
50+
if len(remaining) > 0 {
51+
return
52+
}
53+
54+
store.Delete(modelID)
55+
xlog.Info("Dropped local model stub after its last replica was removed",
56+
"model", modelID, "node", nodeID, "replica", replicaIndex)
57+
}
58+
}
Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,109 @@
1+
package nodes
2+
3+
import (
4+
"context"
5+
"runtime"
6+
"time"
7+
8+
. "github.com/onsi/ginkgo/v2"
9+
. "github.com/onsi/gomega"
10+
"gorm.io/gorm"
11+
12+
"github.com/mudler/LocalAI/pkg/model"
13+
"github.com/mudler/LocalAI/core/services/testutil"
14+
)
15+
16+
// In distributed mode the frontend keeps an in-process stub for every model it
17+
// has routed, and DistributedModelStore.Range reports local stubs UNION the
18+
// registry rows. Every registry removal path deletes the DB row only, so
19+
// without an invalidator the stub outlives the replica and the model is
20+
// reported as loaded forever (visible on the home page, absent from the nodes
21+
// page). These tests pin the invalidator that closes that gap.
22+
var _ = Describe("LocalStubInvalidator", func() {
23+
var (
24+
db *gorm.DB
25+
registry *NodeRegistry
26+
local *model.InMemoryModelStore
27+
nodeA *BackendNode
28+
nodeB *BackendNode
29+
)
30+
31+
BeforeEach(func() {
32+
if runtime.GOOS == "darwin" {
33+
Skip("testcontainers requires Docker, not available on macOS CI")
34+
}
35+
db = testutil.SetupTestDB()
36+
var err error
37+
registry, err = NewNodeRegistry(db)
38+
Expect(err).ToNot(HaveOccurred())
39+
local = model.NewInMemoryModelStore()
40+
nodeA = &BackendNode{Name: "node-a", NodeType: NodeTypeBackend, Address: "10.0.0.1:50051"}
41+
nodeB = &BackendNode{Name: "node-b", NodeType: NodeTypeBackend, Address: "10.0.0.2:50051"}
42+
Expect(registry.Register(context.Background(), nodeA, true)).To(Succeed())
43+
Expect(registry.Register(context.Background(), nodeB, true)).To(Succeed())
44+
})
45+
46+
It("drops the local stub once the last replica of the model is gone", func() {
47+
store := NewDistributedModelStore(local, registry)
48+
Expect(registry.SetNodeModel(context.Background(), nodeA.ID, "ghost-model", 0, "loaded", "10.0.0.1:12345", 0)).To(Succeed())
49+
local.Set("ghost-model", model.NewModel("ghost-model", "10.0.0.1:12345", nil))
50+
51+
registry.AddReplicaRemovedHook(NewLocalStubInvalidator(registry, store))
52+
Expect(registry.RemoveNodeModel(context.Background(), nodeA.ID, "ghost-model", 0)).To(Succeed())
53+
54+
_, ok := local.Get("ghost-model")
55+
Expect(ok).To(BeFalse(), "the stub must not outlive the last replica of the model")
56+
57+
// And the model must disappear from the loaded listing that feeds /system.
58+
listed := []string{}
59+
store.Range(func(id string, _ *model.Model) bool {
60+
listed = append(listed, id)
61+
return true
62+
})
63+
Expect(listed).ToNot(ContainElement("ghost-model"))
64+
})
65+
66+
It("keeps the local stub while another replica still serves the model", func() {
67+
store := NewDistributedModelStore(local, registry)
68+
Expect(registry.SetNodeModel(context.Background(), nodeA.ID, "shared-model", 0, "loaded", "10.0.0.1:12345", 0)).To(Succeed())
69+
Expect(registry.SetNodeModel(context.Background(), nodeB.ID, "shared-model", 0, "loaded", "10.0.0.2:12345", 0)).To(Succeed())
70+
local.Set("shared-model", model.NewModel("shared-model", "10.0.0.1:12345", nil))
71+
72+
registry.AddReplicaRemovedHook(NewLocalStubInvalidator(registry, store))
73+
Expect(registry.RemoveNodeModel(context.Background(), nodeA.ID, "shared-model", 0)).To(Succeed())
74+
75+
_, ok := local.Get("shared-model")
76+
Expect(ok).To(BeTrue(), "a model still loaded on another node must stay in the local store")
77+
})
78+
79+
It("fires every registered replica-removed hook", func() {
80+
Expect(registry.SetNodeModel(context.Background(), nodeA.ID, "m", 0, "loaded", "10.0.0.1:12345", 0)).To(Succeed())
81+
82+
var first, second int
83+
registry.AddReplicaRemovedHook(func(_, _ string, _ int) { first++ })
84+
registry.AddReplicaRemovedHook(func(_, _ string, _ int) { second++ })
85+
86+
Expect(registry.RemoveNodeModel(context.Background(), nodeA.ID, "m", 0)).To(Succeed())
87+
88+
// Prefix-cache invalidation and local-stub invalidation are independent
89+
// subsystems; registering one must never silently displace the other.
90+
Expect(first).To(Equal(1))
91+
Expect(second).To(Equal(1))
92+
})
93+
94+
It("drops the local stub when a whole node's replicas are removed", func() {
95+
store := NewDistributedModelStore(local, registry)
96+
Expect(registry.SetNodeModel(context.Background(), nodeA.ID, "node-model", 0, "loaded", "10.0.0.1:12345", 0)).To(Succeed())
97+
local.Set("node-model", model.NewModel("node-model", "10.0.0.1:12345", nil))
98+
99+
registry.AddReplicaRemovedHook(NewLocalStubInvalidator(registry, store))
100+
// Negative replica index signals "all replicas of this model on the node",
101+
// the shape used by node deregistration and health reaping.
102+
Expect(registry.RemoveAllNodeModelReplicas(context.Background(), nodeA.ID, "node-model")).To(Succeed())
103+
104+
Eventually(func() bool {
105+
_, ok := local.Get("node-model")
106+
return ok
107+
}, time.Second, 10*time.Millisecond).Should(BeFalse())
108+
})
109+
})

core/services/nodes/model_router_test.go

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -197,19 +197,22 @@ var _ = Describe("ModelRouterAdapter", func() {
197197
adapter.mu.Unlock()
198198
Expect(hasRelease).To(BeTrue())
199199

200-
// The initial in-flight reservation is released via OnFirstComplete after
201-
// the first inference call, not during ReleaseModel. ReleaseModel only
202-
// closes the client.
200+
// The initial in-flight reservation is released by whichever comes
201+
// first: the first inference completing, or the route being released.
202+
// Nothing has happened yet, so it is still held.
203203
fakeReg.mu.Lock()
204204
countBeforeRelease := fakeReg.decrementCalled["node-1:test-model"]
205205
fakeReg.mu.Unlock()
206206
Expect(countBeforeRelease).To(Equal(0))
207207

208+
// Releasing the route without any inference must give it back, or the
209+
// counter leaks and pins the replica against every eviction query.
208210
adapter.ReleaseModel("test-model")
209-
fakeReg.mu.Lock()
210-
countAfterRelease := fakeReg.decrementCalled["node-1:test-model"]
211-
fakeReg.mu.Unlock()
212-
Expect(countAfterRelease).To(Equal(0))
211+
Eventually(func() int {
212+
fakeReg.mu.Lock()
213+
defer fakeReg.mu.Unlock()
214+
return fakeReg.decrementCalled["node-1:test-model"]
215+
}).Should(Equal(1))
213216
})
214217
})
215218
})

0 commit comments

Comments
 (0)