Skip to content

fix(distributed): configurable remote model-load timeout, and reap the load when it times out#10948

Merged
mudler merged 2 commits into
masterfrom
fix/reap-abandoned-load-on-timeout
Jul 19, 2026
Merged

fix(distributed): configurable remote model-load timeout, and reap the load when it times out#10948
mudler merged 2 commits into
masterfrom
fix/reap-abandoned-load-on-timeout

Conversation

@localai-bot

Copy link
Copy Markdown
Collaborator

Two changes to the distributed cold-load path. They ship together because the first makes large cold loads possible and the second stops a timeout from leaking a multi-GB process every time one still happens.

The incident

An ARM64 Thor worker loading meituan-longcat/LongCat-Video-Avatar-1.5 (~83 GB):

  • the cold load failed at exactly 302s with DeadlineExceeded, against a hardcoded 5m deadline
  • the abandoned backend kept running — PID 31954 at elapsed 05:17 / cpu 01:06 / rss 3.87 GB, still alive at elapsed 30:05 / cpu 05:32, having pulled ~57 GB from HuggingFace with nobody waiting on the result
  • it had to be reaped by hand via POST /api/nodes/:id/models/unload; we watched it happen twice in one session, each retry stacking another loader on the worker

Once the weights were cached locally the same model loaded in 108 seconds, comfortably inside the 5m budget. The default is not unreasonable for a warm load — the pathology is weight acquisition happening inside the load RPC, which is addressed separately.

1. LOCALAI_NATS_MODEL_LOAD_TIMEOUT / --model-load-timeout

Replaces the hardcoded context.WithTimeout(ctx, 5*time.Minute) in scheduleAndLoad. Default stays 5m, so unset clusters are unchanged. Mirrors the existing LOCALAI_NATS_BACKEND_INSTALL_TIMEOUT precedent, whose help text already says "Increase for slow links pulling multi-GB images" — the install path got this treatment; the load path never did.

The advisory-lock ceiling is now derived rather than constant: ModelLoadCeilingFor(install, load) = max(install + load + 5m, 25m). That matters — the old defaultModelLoadCeiling = 25 * time.Minute was explicitly documented as "install (15m) plus staging and the remote LoadModel (5m)", so raising the load budget past it would have been silently clipped and the fix would have appeared not to work. 15m + 5m + 5m = 25m at defaults, identical to before, with a 25m floor so shrinking either budget can never tighten it.

2. Reap the abandoned replica on timeout

A gRPC deadline only cancels the client side of the call. A backend blocked in a synchronous weight load never observes its cancelled handler context, and scheduleAndLoad previously just returned the error: no StopBackend, no unload. The worker kept loading forever.

On a deadline or cancellation the router now sends backend.stop for the exact modelID#replicaIndex process key it just abandoned. The exact key matters: a bare model ID stops every replica on that node, including healthy ones serving traffic.

Any other LoadModel failure does not reap. An unsupported model or bad option means the backend answered, its handler returned and the process is idle; stopping it would discard a warm process and its downloaded weights before the next attempt. Classification uses errors.Is for context errors plus errors.As on the GRPCStatus() interface (not status.Code, which does not unwrap in all grpc-go versions).

The reap is best-effort: a failed stop is logged via xlog.Warn and never replaces the load error the caller is waiting on, with a spec asserting the stop error cannot appear in the returned error.

The modelID#replicaIndex format was already hand-rolled in two places, so rather than add a third this exports model.BackendProcessKey from pkg/model — the lowest common dependency of the router and the worker, with no new import edges.

Merge-order dependency

This PR should merge after the worker-side fix that bounds client.Free(context.Background()) in stopBackendExact.

proc.Stop() itself is sound (go-processmanager defaults: SIGTERM to the process group, 15s grace, then SIGKILL) and will kill a Python loader wedged in torch.load. But stopBackendExact calls client.Free() with an unbounded context first. 37 Python backends default to PYTHON_GRPC_MAX_WORKERS=1 — including backend/python/longcat-video/backend.py:46, the exact backend from this incident — so a backend wedged in LoadModel has no thread to service Free, and the stop blocks before ever reaching the kill.

So the reap here dispatches the right stop for the right replica, but against a fully wedged single-worker Python backend it is inert until that worker-side timeout lands. This matches the field evidence: the manual unload worked, but only once the loader had gone idle enough to answer Free.

Testing

New Ginkgo specs in core/services/nodes/router_reap_load_test.go drive RoutescheduleAndLoad with a stub grpc.Backend and assert: the stop is dispatched for the exact modelID#replicaIndex (pinned to replica 2, so a hardcoded #0 cannot pass), the original DeadlineExceeded still propagates when the stop itself fails, a wrapped context.DeadlineExceeded reaps too, and a clean InvalidArgument does not.

ok  core/services/nodes             193.025s
ok  core/services/nodes/prefixcache   0.009s
ok  core/config                       0.425s
ok  core/cli                          0.042s
ok  pkg/model                         1.343s

make lint → 0 issues. Coverage rose to 53.0% (baseline 48.5%).


🤖 Generated with Claude Code

mudler added 2 commits July 18, 2026 23:28
The router hardcoded a 5 minute gRPC deadline for the remote LoadModel
call. Staging finishes before the timer starts, so those five minutes
cover only the worker backend's own checkpoint load and pipeline init.
A cold load of meituan-longcat/LongCat-Video-Avatar-1.5 (~83 GB) on an
ARM64 Thor worker fails at exactly 302s with DeadlineExceeded while the
backend process is still making progress (CPU time accumulating, RSS
moving as weights are mapped), so the load was cut short rather than
wedged.

Add LOCALAI_NATS_MODEL_LOAD_TIMEOUT / --model-load-timeout mirroring the
existing backend-install timeout knob, defaulting to 5m so unset
clusters keep today's behaviour.

The cold-load hold ceiling (which bounds how long one load may hold the
per-model advisory lock) was derived from the install timeout alone, so
raising the load deadline past it would have been silently clipped.
Derive it from both budgets via ModelLoadCeilingFor:

    max(install + load + 5m staging margin, 25m)

With the defaults that is 15m + 5m + 5m = 25m, identical to the previous
constant, and the 25m floor means shrinking either budget can never
tighten the ceiling below what clusters relied on before.

Assisted-by: Claude:claude-opus-4-8 golangci-lint
Signed-off-by: Ettore Di Giacinto <mudler@localai.io>
… out

The gRPC deadline on the remote LoadModel call only cancels the client
side. A backend blocked in a synchronous weight load never observes its
cancelled handler context, so when scheduleAndLoad gave up it left the
worker loading with nobody waiting for the result.

Observed on an ARM64 Thor worker loading LongCat-Video-Avatar-1.5: the
client returned DeadlineExceeded at 302s, and the backend process was
still alive 30 minutes later having pulled ~57GB from HuggingFace. Every
retry stacked another multi-GB loader on the worker; they had to be
reaped by hand via POST /api/nodes/:id/models/unload.

Send backend.stop for the exact `modelID#replicaIndex` process key we
just abandoned. The exact key matters: a bare model ID stops every
replica on that node, including healthy ones serving traffic.

Only a deadline or cancellation triggers the reap. Any other LoadModel
failure is the backend answering, which means its handler returned and
the process is idle - stopping it there would discard a warm process and
its downloaded weights. The reap is best-effort and never replaces the
load error the caller is waiting on.

The `modelID#replicaIndex` format was already hand-rolled in two places
(the worker's buildProcessKey and pkg/model's log store). Rather than add
a third, export model.BackendProcessKey from pkg/model, the lowest common
dependency of both sides.

Signed-off-by: Ettore Di Giacinto <mudler@localai.io>
Assisted-by: Claude:claude-opus-4-8 golangci-lint
@mudler
mudler merged commit fb4c61d into master Jul 19, 2026
66 checks passed
@mudler
mudler deleted the fix/reap-abandoned-load-on-timeout branch July 19, 2026 10:01
@localai-bot

Copy link
Copy Markdown
Collaborator Author

Correction to the "Merge-order dependency" section of the description above: it was wrong, and this PR had no unmet dependency.

That section claimed stopBackendExact still called client.Free(context.Background()) unbounded, and that the reap added here would therefore be inert against a backend wedged in LoadModel with PYTHON_GRPC_MAX_WORKERS=1.

That was already fixed on master before this merged, by #10865, which added:

const workerBackendFreeTimeout = 5 * time.Second

and applied it at both relevant call sites — stopBackendExact (core/services/worker/supervisor.go:243) and handleModelUnload (core/services/worker/lifecycle.go:261). Master's implementation is also better than the one this PR's description anticipated: it releases the supervisor mutex across the Free and keeps the port reserved until termination.

So the reap in this PR is effective as merged. No follow-up is required to make it work.

The error came from checking a worktree branched off an older master rather than master itself. Leaving this as a comment rather than editing the description, so the original claim and its correction both stay visible.

mudler added a commit that referenced this pull request Jul 19, 2026
…ker (#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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants