Skip to content

Commit 2035a4d

Browse files
committed
fix(worker): reap deleted backends and stop models that live on a worker
Three related backend-lifecycle defects, all reachable from the same production incident on a Jetson/Thor worker: a deleted backend's gRPC process survived ~40 minutes with its directory removed from disk, a later model load was routed to that orphan and failed with a certifi path pointing into the deleted directory, and the admin could not stop the model because the frontend reported it as not loaded. 1. backend.delete orphaned the process it claimed to delete ------------------------------------------------------------ s.processes is keyed by `modelID#replicaIndex` (buildProcessKey), so the backend name never appeared in a key and was recorded nowhere on the process. backend.delete resolved its target via isRunning/stopBackend, whose prefix path only matches a bare *modelID* - a delete keyed on a backend name resolved to zero keys, the stop silently no-op'd, and the files were removed out from under a live process. The install fast path then handed that orphan back out: it returns any live process for the (model, replica) slot without checking which backend started it, so a reinstalled variant inherited the deleted backend's port. - Record backendName on backendProcess, threaded installBackend -> startBackend. - Add resolveProcessKeysForBackend, matching the recorded name and resolving alias <-> concrete via ListSystemBackends *before* DeleteBackendFromSystem erases the metadata that carries the alias. Alias resolution failure degrades to name-only matching so a delete never fails on it. - backend.stop goes through resolveStopTargets, which accepts a backend name, a model name, or an exact modelID#replica key. Its payload field is named "backend" but is published with all three meanings: the admin UI sends a backend name, UnloadRemoteModel sends a model name, and the router's abandoned-load reap (#10948) sends an exact replica key. Narrowing it to backend names alone would strand the latter two. backend.delete stays strict - its identifier is unambiguously a backend. - Gate the install fast path on processMatchesBackend so a slot held by a different backend is restarted rather than reused. Processes with no recorded name (pre-upgrade) are accepted, so rollout does not restart every running backend. - stopBackendExact reports a real stop failure - the process still being alive afterwards, which is precisely what finishBackendStop already detects to keep the entry and its port reserved - and backend.delete no longer replies success when it knew about a process and could not kill it. "No process was running" stays a success but is logged, so the orphan case is visible rather than silent. 2. /backend/shutdown reported a running model as missing --------------------------------------------------------- ModelLoader.deleteProcess short-circuits on a miss in this replica's in-memory store. In distributed mode the authoritative record of "is this model loaded" is the shared node registry: a frontend replica that never served the model itself (load balancer picked a peer, or the replica restarted) has no local entry. The remote unload path that pkg/model documents ("when ShutdownModel is called for a model with no local process, UnloadRemoteModel is called") sat behind that short-circuit, unreachable in exactly the case it exists for. #10865 reworked this function but kept the short-circuit at the top, so the gap survived that refactor. - deleteProcess consults the remote unloader on a local-store miss, via a shared unloadRemote helper so this branch and the existing no-local-process branch both prefer #10865's RemoteModelContextUnloader, preserving force propagation across the distributed boundary. - UnloadRemoteModelContext reports ErrRemoteModelNotLoaded when no node has the model; it previously returned nil, making a no-op stop indistinguishable from a real one. The converse case (nodes have it, none could be stopped) already errors since #10865 joined the per-node failures, so that half of the original fix was dropped as redundant. - Only when the model is absent locally AND cluster-wide does the endpoint report not-found, now 404 naming both scopes rather than a bare 500. - modelNotFoundErr becomes the exported ErrModelNotFound so the HTTP layer can map it without string matching; watchdog's identity comparison becomes errors.Is. 3. Coverage for the bounded Free() that #10865 shipped untested ---------------------------------------------------------------- The original branch also bounded the pre-stop Free(), but #10865 landed that fix first (workerBackendFreeTimeout, applied in both stopBackendExact and handleModelUnload). That production change is therefore DROPPED here as superseded - master's version is strictly better, since it also releases the supervisor mutex across the call and keeps the port reserved until termination completes. What #10865 did not ship is a test, and the bound is load-bearing: the router-side reap in #10948 sends backend.stop for an abandoned load, and against a wedged backend an unbounded Free would swallow that stop before it reached the process. Nothing failed if the bound regressed. The spec stands up a real gRPC backend server whose Free handler never returns - what a Python backend looks like when its single worker thread (PYTHON_GRPC_MAX_WORKERS=1 on 37 backends) is occupied by a stuck LoadModel. A stub socket is not sufficient and was tried first: without a completed HTTP/2 handshake, gRPC's own ~20s connect timeout ends the call, so that version passed against the very bug it targets. With the connection READY, only the caller's deadline can end it, so the spec hangs to its 60s limit if the timeout is removed and passes with it. Its fixture process is deliberately never started. go-processmanager v0.1.1 writes Process.pid from readPID() without synchronization, so a live process races its own monitor goroutine under -race - reproducible with a bare Run()+Stop() and unrelated to this spec. Since scripts/model-lifecycle-conformance.sh runs this package with -race and is fail-closed, starting one would turn that gate red on an upstream defect. An unstarted process still proves the point: the stop is reached and the slot released, which is exactly what an unbounded Free prevents. Verified: make lint (new-from-merge-base origin/master) reports 0 issues; scripts/model-lifecycle-conformance.sh passes all three stages including the FizzBee liveness check (1458 states, IsLive: true). Assisted-by: Claude:claude-opus-4-8 golangci-lint Signed-off-by: Ettore Di Giacinto <mudler@localai.io>
1 parent fb4c61d commit 2035a4d

11 files changed

Lines changed: 738 additions & 48 deletions

File tree

core/http/endpoints/localai/backend_monitor.go

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,15 @@
11
package localai
22

33
import (
4+
"errors"
5+
"fmt"
6+
"net/http"
7+
48
"github.com/labstack/echo/v4"
59
"github.com/mudler/LocalAI/core/schema"
610
"github.com/mudler/LocalAI/core/services/monitoring"
11+
"github.com/mudler/LocalAI/pkg/model"
12+
"github.com/mudler/xlog"
713
)
814

915
// BackendMonitorEndpoint returns the status of the specified backend
@@ -48,7 +54,25 @@ func BackendShutdownEndpoint(bm *monitoring.BackendMonitorService) echo.HandlerF
4854
if err := c.Bind(input); err != nil {
4955
return err
5056
}
57+
if input.Model == "" {
58+
return echo.NewHTTPError(http.StatusBadRequest, "model is required")
59+
}
5160

52-
return bm.ShutdownModel(input.Model)
61+
if err := bm.ShutdownModel(input.Model); err != nil {
62+
// "Not loaded" is a client-side condition, not a server fault, so
63+
// it should not surface as a 500. In distributed mode this branch
64+
// used to be reached for models that were running on a worker —
65+
// only this replica's local store was empty. The loader now
66+
// consults the node registry first, so reaching it means the
67+
// model is loaded neither here nor anywhere in the cluster.
68+
if errors.Is(err, model.ErrModelNotFound) {
69+
xlog.Info("Shutdown requested for a model that is not loaded", "model", input.Model)
70+
return echo.NewHTTPError(http.StatusNotFound,
71+
fmt.Sprintf("model %q is not loaded on this instance or on any worker node", input.Model))
72+
}
73+
xlog.Error("Failed to shut down model", "model", input.Model, "error", err)
74+
return err
75+
}
76+
return c.JSON(http.StatusOK, map[string]string{"message": "model stopped"})
5377
}
5478
}

core/services/nodes/unloader.go

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import (
1212

1313
"github.com/mudler/LocalAI/core/services/galleryop"
1414
"github.com/mudler/LocalAI/core/services/messaging"
15+
"github.com/mudler/LocalAI/pkg/model"
1516
"github.com/mudler/xlog"
1617
)
1718

@@ -91,8 +92,12 @@ func (a *RemoteUnloaderAdapter) UnloadRemoteModelContext(ctx context.Context, mo
9192
return fmt.Errorf("finding nodes with model %q: %w", modelName, err)
9293
}
9394
if len(nodes) == 0 {
95+
// Distinguish "no node has it" from "stopped successfully" so the
96+
// caller can report the truth to an operator. Returning nil here made
97+
// a no-op stop indistinguishable from a real one, and left the loader
98+
// unable to tell a cluster-wide miss from a completed unload.
9499
xlog.Debug("No remote nodes found with model", "model", modelName)
95-
return nil
100+
return model.ErrRemoteModelNotLoaded
96101
}
97102

98103
var unloadErr error

core/services/nodes/unloader_test.go

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import (
1414

1515
"github.com/mudler/LocalAI/core/services/galleryop"
1616
"github.com/mudler/LocalAI/core/services/messaging"
17+
"github.com/mudler/LocalAI/pkg/model"
1718
)
1819

1920
// --- Fakes ---
@@ -127,9 +128,13 @@ var _ = Describe("RemoteUnloaderAdapter", func() {
127128
})
128129

129130
Describe("UnloadRemoteModel", func() {
130-
It("with no nodes returns nil", func() {
131+
It("with no nodes reports the model is not loaded", func() {
132+
// Previously returned nil, making "stopped it" and "there was
133+
// nothing to stop" indistinguishable. ShutdownModel relies on
134+
// this distinction to choose between success and a genuine 404,
135+
// so a cluster-wide miss must be reported, not swallowed.
131136
locator.nodes = nil
132-
Expect(adapter.UnloadRemoteModel("my-model")).To(Succeed())
137+
Expect(adapter.UnloadRemoteModel("my-model")).To(MatchError(model.ErrRemoteModelNotLoaded))
133138
Expect(mc.published).To(BeEmpty())
134139
})
135140

Lines changed: 208 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,208 @@
1+
package worker
2+
3+
import (
4+
"github.com/mudler/LocalAI/core/gallery"
5+
6+
. "github.com/onsi/ginkgo/v2"
7+
. "github.com/onsi/gomega"
8+
)
9+
10+
// Production incident (Jetson/Thor worker): deleting a backend returned HTTP
11+
// 200 but left its gRPC process alive for ~40 minutes with its directory
12+
// removed from disk. A later model load was routed to that orphan and failed
13+
// with a certifi path pointing into the deleted directory.
14+
//
15+
// Root cause: s.processes is keyed by `modelID#replicaIndex`
16+
// (buildProcessKey), so a delete keyed on the *backend* name resolved to no
17+
// keys at all and the stop silently no-op'd. These specs pin the two lookups
18+
// that must work by backend name.
19+
var _ = Describe("Backend deletion reaps the backend's processes", func() {
20+
const (
21+
concrete = "cuda13-nvidia-l4t-arm64-longcat-video"
22+
development = "cuda13-nvidia-l4t-arm64-longcat-video-development"
23+
alias = "longcat-video"
24+
)
25+
26+
Describe("resolveProcessKeysForBackend", func() {
27+
It("finds processes started for a backend even though they are keyed by modelID", func() {
28+
s := &backendSupervisor{
29+
processes: map[string]*backendProcess{
30+
// Started by a model load: key is modelID#replica, and the
31+
// backend name survives only in backendName.
32+
"LongCat-Video#0": {addr: "127.0.0.1:30232", backendName: concrete},
33+
"LongCat-Video#1": {addr: "127.0.0.1:30234", backendName: concrete},
34+
"other-model#0": {addr: "127.0.0.1:30235", backendName: "llama-cpp"},
35+
},
36+
}
37+
38+
Expect(s.resolveProcessKeysForBackend(setOf(concrete))).To(
39+
ConsistOf("LongCat-Video#0", "LongCat-Video#1"),
40+
"a delete keyed on the backend name must reach every process started for it")
41+
})
42+
43+
It("matches through the alias the model config used at install time", func() {
44+
// The model config referenced `backend: longcat-video` (the alias),
45+
// so the process recorded the alias; the delete request carries the
46+
// concrete directory name. Without alias resolution the orphan
47+
// survives exactly as it did in production.
48+
s := &backendSupervisor{
49+
processes: map[string]*backendProcess{
50+
"LongCat-Video#0": {addr: "127.0.0.1:30232", backendName: alias},
51+
},
52+
}
53+
54+
Expect(s.resolveProcessKeysForBackend(setOf(concrete, alias))).To(
55+
ConsistOf("LongCat-Video#0"),
56+
"alias and concrete name must resolve to the same process")
57+
})
58+
59+
It("does not reap processes belonging to a different backend", func() {
60+
s := &backendSupervisor{
61+
processes: map[string]*backendProcess{
62+
"LongCat-Video#0": {addr: "127.0.0.1:30233", backendName: development},
63+
},
64+
}
65+
66+
Expect(s.resolveProcessKeysForBackend(setOf(concrete))).To(BeEmpty(),
67+
"deleting the non-development backend must not kill the -development process")
68+
})
69+
70+
It("still resolves legacy entries that predate backendName via the modelID prefix", func() {
71+
// Installs with an empty modelID key the map by the backend name
72+
// itself (buildProcessKey falls back). Those must keep working.
73+
s := &backendSupervisor{
74+
processes: map[string]*backendProcess{
75+
concrete + "#0": {addr: "127.0.0.1:30232"},
76+
},
77+
}
78+
79+
Expect(s.resolveProcessKeysForBackend(setOf(concrete))).To(ConsistOf(concrete + "#0"))
80+
})
81+
})
82+
83+
Describe("resolveStopTargets", func() {
84+
// backend.stop is ambiguous by design: the admin backends UI publishes
85+
// a BACKEND name, UnloadRemoteModel publishes a MODEL name, and the
86+
// router's abandoned-load reap (#10948) publishes an exact
87+
// modelID#replica key. Resolving only one of the three silently
88+
// strands the others.
89+
newSupervisor := func() *backendSupervisor {
90+
return &backendSupervisor{
91+
processes: map[string]*backendProcess{
92+
"LongCat-Video#0": {addr: "127.0.0.1:30232", backendName: concrete},
93+
},
94+
}
95+
}
96+
97+
It("stops a process addressed by model ID", func() {
98+
Expect(newSupervisor().resolveStopTargets("LongCat-Video")).To(ConsistOf("LongCat-Video#0"),
99+
"model-name stop (UnloadRemoteModel) must still reach the process")
100+
})
101+
102+
It("stops a process addressed by backend name", func() {
103+
Expect(newSupervisor().resolveStopTargets(concrete)).To(ConsistOf("LongCat-Video#0"),
104+
"backend-name stop must reach the process too")
105+
})
106+
107+
It("stops a process addressed by an exact replica key", func() {
108+
Expect(newSupervisor().resolveStopTargets("LongCat-Video#0")).To(ConsistOf("LongCat-Video#0"),
109+
"the router reaps an abandoned load by exact modelID#replica key")
110+
})
111+
112+
It("does not stop unrelated processes", func() {
113+
Expect(newSupervisor().resolveStopTargets("some-other-model")).To(BeEmpty())
114+
})
115+
})
116+
117+
Describe("processMatchesBackend", func() {
118+
It("rejects reusing a process that was started for a different backend", func() {
119+
// The install fast path returns the address of any live process
120+
// under this processKey. After the concrete backend was deleted and
121+
// the -development variant installed, the same model+replica got
122+
// the deleted backend's port handed back to it.
123+
s := &backendSupervisor{
124+
processes: map[string]*backendProcess{
125+
"LongCat-Video#0": {addr: "127.0.0.1:30232", backendName: concrete},
126+
},
127+
}
128+
129+
Expect(s.processMatchesBackend("LongCat-Video#0", development)).To(BeFalse(),
130+
"a process started for the deleted backend must not be reused for another backend")
131+
})
132+
133+
It("accepts reusing a process started for the same backend", func() {
134+
s := &backendSupervisor{
135+
processes: map[string]*backendProcess{
136+
"LongCat-Video#0": {addr: "127.0.0.1:30232", backendName: concrete},
137+
},
138+
}
139+
140+
Expect(s.processMatchesBackend("LongCat-Video#0", concrete)).To(BeTrue())
141+
})
142+
143+
It("accepts a process recorded under the alias of the requested backend", func() {
144+
s := &backendSupervisor{
145+
processes: map[string]*backendProcess{
146+
"LongCat-Video#0": {addr: "127.0.0.1:30232", backendName: alias},
147+
},
148+
}
149+
150+
Expect(s.processMatchesBackend("LongCat-Video#0", alias)).To(BeTrue())
151+
})
152+
153+
It("accepts legacy entries with no recorded backend name", func() {
154+
// Pre-upgrade processes carry no backendName. Treating them as a
155+
// mismatch would restart every running backend once on rollout.
156+
s := &backendSupervisor{
157+
processes: map[string]*backendProcess{
158+
"LongCat-Video#0": {addr: "127.0.0.1:30232"},
159+
},
160+
}
161+
162+
Expect(s.processMatchesBackend("LongCat-Video#0", concrete)).To(BeTrue())
163+
})
164+
})
165+
166+
Describe("backendIdentitySet", func() {
167+
It("maps an alias to its concrete backend and back", func() {
168+
backends := gallery.SystemBackends{
169+
concrete: {Name: concrete, Metadata: &gallery.BackendMetadata{Name: concrete, Alias: alias}},
170+
alias: {Name: alias, Metadata: &gallery.BackendMetadata{Name: concrete, Alias: alias}},
171+
}
172+
173+
Expect(backendIdentitySet(backends, concrete)).To(HaveKey(alias))
174+
Expect(backendIdentitySet(backends, alias)).To(HaveKey(concrete))
175+
})
176+
177+
It("falls back to the bare name when the backend is unknown", func() {
178+
// A delete must never fail (or over-reach) because the gallery
179+
// listing is unavailable or the entry is already gone from disk.
180+
Expect(backendIdentitySet(gallery.SystemBackends{}, concrete)).To(
181+
Equal(map[string]struct{}{concrete: {}}))
182+
})
183+
184+
It("does not conflate two concrete backends sharing an alias", func() {
185+
// Both variants declare alias "longcat-video"; ListSystemBackends
186+
// picks one for the alias row. Deleting the non-chosen concrete
187+
// must not pull in the chosen one's identity.
188+
backends := gallery.SystemBackends{
189+
development: {Name: development, Metadata: &gallery.BackendMetadata{Name: development, Alias: alias}},
190+
alias: {Name: alias, Metadata: &gallery.BackendMetadata{Name: development, Alias: alias}},
191+
}
192+
193+
set := backendIdentitySet(backends, development)
194+
Expect(set).To(HaveKey(development))
195+
Expect(set).ToNot(HaveKey(concrete))
196+
})
197+
})
198+
})
199+
200+
// setOf builds the identity set the resolver consumes, keeping the specs
201+
// readable without repeating the map literal.
202+
func setOf(names ...string) map[string]struct{} {
203+
set := make(map[string]struct{}, len(names))
204+
for _, n := range names {
205+
set[n] = struct{}{}
206+
}
207+
return set
208+
}

0 commit comments

Comments
 (0)