Skip to content

Commit f735cb2

Browse files
localai-botmudler
andauthored
fix(worker): reap deleted backends and stop models that live on a worker (#10956)
* 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> * fix(distributed): keep remote unload idempotent, ask presence separately 2035a4d made UnloadRemoteModel return ErrRemoteModelNotLoaded when no node holds the model, so ShutdownModel could answer 404 instead of a misleading 500. That narrowed a shared adapter contract to serve one caller and broke the documented idempotent-unload guarantee, which CI caught on PR #10956: [FAIL] Node Backend Lifecycle (NATS-driven) > NATS backend.stop events should be no-op for models not on any node [Distributed] Expected success, but got: model not loaded on any node The spec name states the contract outright. The matching unit assertion was updated in that commit; this e2e one was missed because it lives under tests/e2e/ with no build tags and does not run in package-scoped test runs. Caller audit - who breaks when an idempotent unload becomes an error: - pkg/model/watchdog.go:902 (LRU memory reclaimer) is the serious one. It untracks a model ONLY when shutdown returns nil or ErrModelNotFound. A new error type means the model is never untracked, so the reclaimer keeps re-selecting the same entry and never reclaims - a live wedge whenever a local store entry outlives the remote model. - core/services/galleryop/managers_local.go:43 (DeleteModel) would warn on every deletion of an already-unloaded model. - core/services/modeladmin/{state,config,remote_sync}.go stop instances best-effort against models that are frequently not loaded. - deleteProcess itself: the no-local-process branch returns the unload result directly, so a stale local entry for a model no longer on any node turned a previously-successful cleanup into a failure. Only ShutdownModel wants the distinction, and only on the local-store-miss path. So the distinction moves to the caller instead of the contract: - UnloadRemoteModel/UnloadRemoteModelContext return nil again when no node has the model, and ErrRemoteModelNotLoaded is removed. - New optional RemoteModelPresenceChecker (HasRemoteModel) answers the question directly. deleteProcess consults it BEFORE unloading, because an idempotent unload cannot report afterwards whether anything was stopped. Absent locally AND cluster-wide is the only case that reports 404. - A failed registry lookup is surfaced rather than reported as absence: an unreachable registry is not evidence a model is gone, and answering a confident 404 off a failed lookup is how an operator gets told a running model does not exist. - Unloaders that predate the extension keep working - deleteProcess attempts the unload rather than refusing it - and compile-time assertions in the nodes package now pin all three optional interfaces, since both are consumed by runtime type assertion where drift degrades behavior silently instead of failing the build. The contract is now pinned at both levels that disagreed, each spec pointing at the other: "with no nodes returns nil" in unloader_test.go and "should be no-op for models not on any node" in node_lifecycle_test.go. Verified: full distributed e2e suite 233 passed / 0 failed (the suite that failed 232/1 in CI); pkg/model and core/services/nodes green; make lint new-from-merge-base reports 0 issues. Assisted-by: Claude:claude-opus-4-8 golangci-lint Signed-off-by: Ettore Di Giacinto <mudler@localai.io> * fix(distributed): drop replica rows when a worker stops a backend A worker returns a stopped backend's gRPC port to its allocator as soon as the process is confirmed dead, and hands it to the next backend that starts. The controller's NodeModel row for the old address survives, and both SmartRouter.probeHealth and the HealthMonitor per-model probe verify liveness, not identity, so once an unrelated backend binds the recycled port the stale row passes every check and the request is served by the wrong backend instead of failing. backend.delete is newly able to trigger this: before #10956 a delete never actually stopped a process, so it never recycled a port. backend.upgrade has the identical gap and always did — upgradeBackend force-stops every process using the binary and starts none back up, while DistributedBackendManager.UpgradeBackend never removes rows. model.unload is the one path that gets this right today: it calls RemoveAllNodeModelReplicas straight after StopBackend. Report the process keys the worker terminated on the delete and upgrade replies, and drop the matching rows in RemoteUnloaderAdapter, which already holds a ModelLocator with RemoveNodeModel. All three call sites funnel through that adapter, so no new interface, DB migration, or proto change is needed. A key is reported only once its process is confirmed gone, so the list stays trustworthy on the partial-failure replies too. Old workers never populate the new fields. ReportsStoppedProcesses tells "stopped nothing" apart from "does not report", so an old worker's silence falls back to the pre-existing probe-based staleness recovery instead of being mistaken for a completed cleanup. Quarantine released ports for a short window as an interlock covering the NATS round-trip between the worker freeing the port and the controller dropping the row. It is deliberately not derived from HealthCheckInterval: that cadence is operator-tunable and the per-model reaper can be disabled outright, so coupling a worker-local constant to it would be silently wrong on some clusters. Eager row removal is the fix; the delay only closes the handoff gap. Identity verification in probeHealth was considered and rejected: Health and Status carry no backend identity, so it needs a proto change plus an implementation in 36 Python and 4 C++ Health servicers, it is fail-open for any backend not yet rebuilt, and the probeCache short-circuit means it would not even execute during the 30s window where the misroute happens. Fixes #10952 Refs #10954, #10956 Assisted-by: Claude:claude-opus-4-8 golangci-lint Signed-off-by: Ettore Di Giacinto <mudler@localai.io> * chore(deps): bump go-processmanager, assert real backend termination go-processmanager wrote Process.PID from readPID() with no synchronization while its own monitor goroutine cleared the same field on exit, so a bare Run()+Stop() tripped the race detector without any concurrent access from the caller. LocalAI hit this on every backend stop. Upstream fixed it in a94e2b7 by guarding PID with a mutex and adding CurrentPID() as a race-safe accessor. The exported field was kept to avoid a breaking change but is now deprecated: a direct read still races the monitor. No tag carries the fix yet, so pin the pseudo-version. GetGRPCPID reads through CurrentPID() instead of the field. The accessor returns the same string under an RLock, so the empty-PID and strconv error paths are unchanged; it is the only direct field read in the tree. With the race gone, the Free-timeout spec no longer has to leave its fixture process unstarted. It now runs a real child and asserts the child genuinely exits, which is exactly what the earlier workaround gave up: the spec could show the stop was reached and the slot released, but not that SIGTERM ever landed. Termination is observed through Done(), which closes only once the library has waited on the child. The pidfile-based liveness helpers cannot serve here, because Stop() deletes the pidfile while releasing the handle and so reports "not alive" even if no signal was sent. Signed-off-by: Ettore Di Giacinto <mudler@localai.io> Assisted-by: Claude Code:claude-opus-4-8[1m] [Read] [Edit] [Bash] --------- Signed-off-by: Ettore Di Giacinto <mudler@localai.io> Co-authored-by: Ettore Di Giacinto <mudler@localai.io>
1 parent 5c607c0 commit f735cb2

21 files changed

Lines changed: 1515 additions & 83 deletions

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/messaging/subjects.go

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -226,6 +226,14 @@ type BackendUpgradeRequest struct {
226226
type BackendUpgradeReply struct {
227227
Success bool `json:"success"`
228228
Error string `json:"error,omitempty"`
229+
230+
// StoppedProcessKeys / ReportsStoppedProcesses carry the same
231+
// stale-row-invalidation contract as on BackendDeleteReply; an upgrade
232+
// force-stops every process using the binary and starts none back up, so it
233+
// recycles ports exactly the way a delete does. See that type for why the
234+
// boolean is not redundant with an empty list.
235+
StoppedProcessKeys []string `json:"stopped_process_keys,omitempty"`
236+
ReportsStoppedProcesses bool `json:"reports_stopped_processes,omitempty"`
229237
}
230238

231239
// SubjectNodeBackendList queries a worker node for its installed backends.
@@ -290,6 +298,23 @@ type BackendDeleteRequest struct {
290298
type BackendDeleteReply struct {
291299
Success bool `json:"success"`
292300
Error string `json:"error,omitempty"`
301+
302+
// StoppedProcessKeys names every `modelID#replica` process the worker
303+
// terminated while serving this delete. Stopping a process returns its gRPC
304+
// port to the worker's allocator, so any NodeModel row still pointing at
305+
// that address becomes a live misroute the moment an unrelated backend
306+
// binds the recycled port: probeHealth verifies liveness, not identity, so
307+
// the request is served by the wrong backend rather than failing. The
308+
// controller uses these keys to drop the rows eagerly.
309+
StoppedProcessKeys []string `json:"stopped_process_keys,omitempty"`
310+
311+
// ReportsStoppedProcesses distinguishes "this worker enumerates what it
312+
// stopped and stopped nothing" from "this worker predates the field". Both
313+
// send an empty list, and only the first is authoritative. Without this
314+
// flag a controller cannot tell them apart and would eventually be tempted
315+
// to read silence as a completed cleanup, which is precisely the wrong
316+
// conclusion against an older worker.
317+
ReportsStoppedProcesses bool `json:"reports_stopped_processes,omitempty"`
293318
}
294319

295320
// SubjectNodeModelUnload tells a worker node to unload a model (gRPC Free) without killing the backend.

core/services/nodes/unloader.go

Lines changed: 84 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

@@ -71,6 +72,17 @@ func (a *RemoteUnloaderAdapter) InstallTimeout() time.Duration {
7172
return a.installTimeout
7273
}
7374

75+
// Compile-time proof that the adapter still satisfies the loader's optional
76+
// extensions. Both are consumed via runtime type assertion in deleteProcess, so
77+
// a signature drift here would silently downgrade behavior — losing force
78+
// propagation, or making ShutdownModel unable to tell a cluster-wide miss from
79+
// a completed unload — rather than failing the build.
80+
var (
81+
_ model.RemoteModelUnloader = (*RemoteUnloaderAdapter)(nil)
82+
_ model.RemoteModelContextUnloader = (*RemoteUnloaderAdapter)(nil)
83+
_ model.RemoteModelPresenceChecker = (*RemoteUnloaderAdapter)(nil)
84+
)
85+
7486
// UnloadRemoteModel finds the node(s) hosting the given model and tells them
7587
// to stop their backend process via NATS backend.stop event.
7688
// The worker process handles a bounded Free() followed by process termination;
@@ -80,6 +92,22 @@ func (a *RemoteUnloaderAdapter) UnloadRemoteModel(modelName string) error {
8092
return a.UnloadRemoteModelContext(context.Background(), modelName, false)
8193
}
8294

95+
// HasRemoteModel reports whether any node currently holds the model. It exists
96+
// because UnloadRemoteModel is idempotent and so cannot signal "there was
97+
// nothing to stop"; ShutdownModel consults this first so it can answer 404 for
98+
// a model loaded neither locally nor anywhere in the cluster, instead of the
99+
// misleading 500 "model not found" that a local-store miss used to produce.
100+
func (a *RemoteUnloaderAdapter) HasRemoteModel(ctx context.Context, modelName string) (bool, error) {
101+
if ctx == nil {
102+
ctx = context.Background()
103+
}
104+
nodes, err := a.registry.FindNodesWithModel(ctx, modelName)
105+
if err != nil {
106+
return false, fmt.Errorf("finding nodes with model %q: %w", modelName, err)
107+
}
108+
return len(nodes) > 0, nil
109+
}
110+
83111
// UnloadRemoteModelContext is the cancellation-aware extension used by the
84112
// model loader to preserve forced shutdown across the distributed boundary.
85113
func (a *RemoteUnloaderAdapter) UnloadRemoteModelContext(ctx context.Context, modelName string, force bool) error {
@@ -91,6 +119,10 @@ func (a *RemoteUnloaderAdapter) UnloadRemoteModelContext(ctx context.Context, mo
91119
return fmt.Errorf("finding nodes with model %q: %w", modelName, err)
92120
}
93121
if len(nodes) == 0 {
122+
// Unloading is idempotent by contract: cleanup paths (model deletion,
123+
// config edits, watchdog eviction) legitimately run against an
124+
// already-unloaded model and must not fail. Callers that need to tell
125+
// this case apart use HasRemoteModel before unloading.
94126
xlog.Debug("No remote nodes found with model", "model", modelName)
95127
return nil
96128
}
@@ -238,6 +270,9 @@ func (a *RemoteUnloaderAdapter) UpgradeBackend(nodeID, backendType, galleriesJSO
238270
return nil, fmt.Errorf("%w (subject=%s nodeID=%s backend=%s): %v",
239271
galleryop.ErrWorkerStillInstalling, subject, nodeID, backendType, err)
240272
}
273+
if err == nil {
274+
a.dropStoppedReplicaRows(nodeID, "backend.upgrade", backendType, reply.StoppedProcessKeys, reply.ReportsStoppedProcesses)
275+
}
241276
return reply, err
242277
}
243278

@@ -305,7 +340,55 @@ func (a *RemoteUnloaderAdapter) DeleteBackend(nodeID, backendName string) (*mess
305340
subject := messaging.SubjectNodeBackendDelete(nodeID)
306341
xlog.Info("Sending NATS backend.delete", "nodeID", nodeID, "backend", backendName)
307342

308-
return messaging.RequestJSON[messaging.BackendDeleteRequest, messaging.BackendDeleteReply](a.nats, subject, messaging.BackendDeleteRequest{Backend: backendName}, 2*time.Minute)
343+
reply, err := messaging.RequestJSON[messaging.BackendDeleteRequest, messaging.BackendDeleteReply](a.nats, subject, messaging.BackendDeleteRequest{Backend: backendName}, 2*time.Minute)
344+
if err != nil {
345+
return reply, err
346+
}
347+
a.dropStoppedReplicaRows(nodeID, "backend.delete", backendName, reply.StoppedProcessKeys, reply.ReportsStoppedProcesses)
348+
return reply, nil
349+
}
350+
351+
// dropStoppedReplicaRows removes the NodeModel rows addressing processes a
352+
// worker just terminated.
353+
//
354+
// Why eagerly, rather than leaving it to the existing health checks: stopping a
355+
// process returns its gRPC port to the worker's allocator, and the next backend
356+
// started there can be handed that same port. Until the row is gone it names a
357+
// live address, so both SmartRouter.probeHealth and the HealthMonitor per-model
358+
// probe — which verify liveness, not identity — pass against whatever now
359+
// occupies the port, and the request is served by the wrong backend rather than
360+
// failing. Nothing else on the delete/upgrade path tells the controller the
361+
// address just became invalid, unlike model.unload which drops its rows itself.
362+
//
363+
// reported=false means the worker predates this reply field. Its empty list is
364+
// then indistinguishable from "stopped nothing", so it must NOT be read as a
365+
// completed cleanup: leave the rows alone and fall back to the probe-based
366+
// staleness recovery that was the only mechanism before this change.
367+
func (a *RemoteUnloaderAdapter) dropStoppedReplicaRows(nodeID, op, backendName string, processKeys []string, reported bool) {
368+
if !reported {
369+
xlog.Debug("Worker did not report stopped processes; relying on probe-based staleness recovery",
370+
"nodeID", nodeID, "op", op, "backend", backendName)
371+
return
372+
}
373+
ctx := context.Background()
374+
for _, key := range processKeys {
375+
modelName, replicaIndex, ok := model.ParseBackendProcessKey(key)
376+
if !ok {
377+
// Acting on a guess could evict the row of a healthy sibling replica.
378+
xlog.Warn("Ignoring unparseable process key reported by worker",
379+
"nodeID", nodeID, "op", op, "backend", backendName, "processKey", key)
380+
continue
381+
}
382+
xlog.Info("Dropping replica row for a process the worker stopped",
383+
"nodeID", nodeID, "op", op, "backend", backendName, "model", modelName, "replica", replicaIndex)
384+
if err := a.registry.RemoveNodeModel(ctx, nodeID, modelName, replicaIndex); err != nil {
385+
// Best-effort: probe-based recovery remains the backstop, and failing
386+
// the operator's delete over a bookkeeping error would be worse than
387+
// the stale row this prevents.
388+
xlog.Warn("Failed to drop replica row for a stopped process",
389+
"nodeID", nodeID, "op", op, "model", modelName, "replica", replicaIndex, "error", err)
390+
}
391+
}
309392
}
310393

311394
// UnloadModelOnNode sends a model.unload request to a specific node.
Lines changed: 186 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,186 @@
1+
package nodes
2+
3+
import (
4+
"encoding/json"
5+
"time"
6+
7+
. "github.com/onsi/ginkgo/v2"
8+
. "github.com/onsi/gomega"
9+
10+
"github.com/mudler/LocalAI/core/services/messaging"
11+
)
12+
13+
// Replies are handed to the adapter as raw JSON rather than as marshalled
14+
// structs on purpose: these specs pin the behavior a controller must exhibit
15+
// when a worker on a *different* build answers it. Marshalling the current
16+
// struct would only ever produce the current field set and could not express
17+
// an old worker's reply at all.
18+
var _ = Describe("RemoteUnloaderAdapter stale replica rows", func() {
19+
var (
20+
locator *fakeModelLocator
21+
mc *fakeMessagingClient
22+
adapter *RemoteUnloaderAdapter
23+
)
24+
25+
BeforeEach(func() {
26+
locator = &fakeModelLocator{}
27+
mc = &fakeMessagingClient{}
28+
adapter = NewRemoteUnloaderAdapter(locator, mc, 3*time.Minute, 15*time.Minute)
29+
})
30+
31+
Describe("DeleteBackend", func() {
32+
It("drops the replica rows for every process the worker stopped", func() {
33+
// The worker has just terminated these processes and returned their
34+
// gRPC ports to its allocator. Any row still pointing at those
35+
// addresses will pass probeHealth as soon as an unrelated backend
36+
// binds the recycled port, and the request is then silently served
37+
// by the wrong backend.
38+
mc.requestReply = []byte(`{
39+
"success": true,
40+
"reports_stopped_processes": true,
41+
"stopped_process_keys": ["qwen3-0.6b#0", "qwen3-0.6b#2"]
42+
}`)
43+
44+
reply, err := adapter.DeleteBackend("node-1", "llama-cpp")
45+
Expect(err).NotTo(HaveOccurred())
46+
Expect(reply.Success).To(BeTrue())
47+
48+
Expect(locator.removedReplicas).To(ConsistOf(
49+
modelReplicaRef{"node-1", "qwen3-0.6b", 0},
50+
modelReplicaRef{"node-1", "qwen3-0.6b", 2},
51+
))
52+
})
53+
54+
It("keeps model IDs that contain the replica separator intact", func() {
55+
// Process keys are `modelID#replica` and model IDs are free to
56+
// contain '#' themselves, so the split must be anchored at the last
57+
// separator. Splitting at the first one addresses a row that does
58+
// not exist and leaves the real stale row in place.
59+
mc.requestReply = []byte(`{
60+
"success": true,
61+
"reports_stopped_processes": true,
62+
"stopped_process_keys": ["weird#name#3"]
63+
}`)
64+
65+
_, err := adapter.DeleteBackend("node-1", "llama-cpp")
66+
Expect(err).NotTo(HaveOccurred())
67+
Expect(locator.removedReplicas).To(ConsistOf(modelReplicaRef{"node-1", "weird#name", 3}))
68+
})
69+
70+
It("removes nothing when a worker that reports stopped processes stopped none", func() {
71+
// Deleting a backend that was never loaded is routine. The reply is
72+
// authoritative here, so "no keys" genuinely means "no rows".
73+
mc.requestReply = []byte(`{"success": true, "reports_stopped_processes": true}`)
74+
75+
_, err := adapter.DeleteBackend("node-1", "llama-cpp")
76+
Expect(err).NotTo(HaveOccurred())
77+
Expect(locator.removedReplicas).To(BeEmpty())
78+
})
79+
80+
It("does not read an old worker's silence as a completed cleanup", func() {
81+
// A worker predating this change never sets either field. Its empty
82+
// list is indistinguishable from "stopped nothing", so the
83+
// controller must not conclude the node is clean, and equally must
84+
// not guess at rows to delete. It falls back to the probe-based
85+
// self-heal in SmartRouter.probeHealth, which is exactly the
86+
// pre-change behavior.
87+
mc.requestReply = []byte(`{"success": true}`)
88+
89+
reply, err := adapter.DeleteBackend("node-1", "llama-cpp")
90+
Expect(err).NotTo(HaveOccurred())
91+
Expect(reply.Success).To(BeTrue())
92+
Expect(reply.ReportsStoppedProcesses).To(BeFalse())
93+
Expect(locator.removedReplicas).To(BeEmpty())
94+
Expect(locator.removedPairs).To(BeEmpty())
95+
})
96+
97+
It("leaves rows alone when the worker could not stop the process", func() {
98+
// The worker aborts the delete without listing the key it failed to
99+
// kill: that process survived, so its address is still correct and
100+
// dropping the row would force a needless reload of a live replica.
101+
mc.requestReply = []byte(`{
102+
"success": false,
103+
"error": "could not stop running process qwen3-0.6b#0",
104+
"reports_stopped_processes": true
105+
}`)
106+
107+
reply, err := adapter.DeleteBackend("node-1", "llama-cpp")
108+
Expect(err).NotTo(HaveOccurred())
109+
Expect(reply.Success).To(BeFalse())
110+
Expect(locator.removedReplicas).To(BeEmpty())
111+
})
112+
113+
It("drops rows for processes that died before a later step failed", func() {
114+
// The worker reports a key only once that process is confirmed gone,
115+
// so a failure further along (removing files, re-registering) does
116+
// not make the already-recycled ports any less dangerous. Gating
117+
// removal on overall success would strand exactly those rows.
118+
mc.requestReply = []byte(`{
119+
"success": false,
120+
"error": "failed to delete backend files",
121+
"reports_stopped_processes": true,
122+
"stopped_process_keys": ["qwen3-0.6b#0"]
123+
}`)
124+
125+
_, err := adapter.DeleteBackend("node-1", "llama-cpp")
126+
Expect(err).NotTo(HaveOccurred())
127+
Expect(locator.removedReplicas).To(ConsistOf(modelReplicaRef{"node-1", "qwen3-0.6b", 0}))
128+
})
129+
130+
It("ignores malformed process keys instead of removing a wrong row", func() {
131+
mc.requestReply = []byte(`{
132+
"success": true,
133+
"reports_stopped_processes": true,
134+
"stopped_process_keys": ["no-replica-suffix", "qwen3-0.6b#notanumber", "good#1"]
135+
}`)
136+
137+
_, err := adapter.DeleteBackend("node-1", "llama-cpp")
138+
Expect(err).NotTo(HaveOccurred())
139+
Expect(locator.removedReplicas).To(ConsistOf(modelReplicaRef{"node-1", "good", 1}))
140+
})
141+
})
142+
143+
Describe("UpgradeBackend", func() {
144+
It("drops the replica rows for every process the worker stopped", func() {
145+
// An upgrade force-stops every process using the binary and starts
146+
// none of them back up, so it recycles ports exactly as delete does
147+
// while leaving the same rows behind.
148+
mc.requestReply = []byte(`{
149+
"success": true,
150+
"reports_stopped_processes": true,
151+
"stopped_process_keys": ["whisper#0", "whisper#1"]
152+
}`)
153+
154+
reply, err := adapter.UpgradeBackend("node-1", "whisper", "", "", "", "", 0, "", nil)
155+
Expect(err).NotTo(HaveOccurred())
156+
Expect(reply.Success).To(BeTrue())
157+
158+
Expect(locator.removedReplicas).To(ConsistOf(
159+
modelReplicaRef{"node-1", "whisper", 0},
160+
modelReplicaRef{"node-1", "whisper", 1},
161+
))
162+
})
163+
164+
It("does not read an old worker's silence as a completed cleanup", func() {
165+
mc.requestReply = []byte(`{"success": true}`)
166+
167+
reply, err := adapter.UpgradeBackend("node-1", "whisper", "", "", "", "", 0, "", nil)
168+
Expect(err).NotTo(HaveOccurred())
169+
Expect(reply.ReportsStoppedProcesses).To(BeFalse())
170+
Expect(locator.removedReplicas).To(BeEmpty())
171+
})
172+
})
173+
174+
Describe("reply wire compatibility", func() {
175+
It("decodes an old worker's delete reply without the new fields", func() {
176+
// Guards the rolling-upgrade direction that matters: a new
177+
// controller must keep working against every worker already
178+
// deployed, not just ones rebuilt from this commit.
179+
var reply messaging.BackendDeleteReply
180+
Expect(json.Unmarshal([]byte(`{"success": true}`), &reply)).To(Succeed())
181+
Expect(reply.Success).To(BeTrue())
182+
Expect(reply.ReportsStoppedProcesses).To(BeFalse())
183+
Expect(reply.StoppedProcessKeys).To(BeEmpty())
184+
})
185+
})
186+
})

0 commit comments

Comments
 (0)