Skip to content

fix(watchdog): keep tracking a model that an eviction could not stop - #12859

Open
hcharbonnier wants to merge 3 commits into
mudler:masterfrom
hcharbonnier:mr/watchdog-keep-tracking-failed-stop
Open

hcharbonnier wants to merge 3 commits into
mudler:masterfrom
hcharbonnier:mr/watchdog-keep-tracking-failed-stop

Conversation

@hcharbonnier

@hcharbonnier hcharbonnier commented Oct 23, 2026 •

Copy link
Copy Markdown

Description

An eviction untracks the backend before it stops it. When the stop failed, the error was only logged: the backend kept running and kept its VRAM, but the watchdog had already forgotten it. Group and LRU enforcement then saw no conflict, and the next load started next to it and ran out of memory.

Stopping a model takes its exclusive lifecycle lock, which a model that is still loading holds for the whole load. The stop then times out ("waiting to shut down model: context deadline exceeded"), and the watchdog no longer knows the model is there.

The tracking state is now captured before the backend is untracked, and put back when the stop fails. This applies to every eviction path that untracks first: group, LRU, idle timeout and busy timeout. A model that is already gone (ErrModelNotFound) still counts as stopped, and a replacement registered on the same address in the meantime is left alone. EvictedCount only counts backends that really stopped, and NeedMore is set when one did not, so the loader keeps retrying instead of treating the room as made. The idle and busy checks retry the stop on their next tick.

The state is only put back while the loader still holds the model at that address (ModelLoader.HasModelAtAddress). deleteProcess drops a remote-only model, or the local half of a local+remote placement, before it reports a failed remote unload; tracking that model again would leave a phantom entry counted as loaded and as a group conflict. The memory reclaimer applies the same rule and untracks a model the loader already dropped.

Assisted-by: Claude:claude-sonnet-5.5

Notes for Reviewers

Signed commits

  • Yes, I signed my commits.
  • Documentation updated (docs/content/) for user-facing changes, or not applicable

An eviction untracks the backend before it stops it. When the stop failed,
the error was only logged: the backend kept running and kept its VRAM, but
the watchdog had already forgotten it. Group and LRU enforcement then saw no
conflict, and the next load started next to it and ran out of memory.

Stopping a model takes its exclusive lifecycle lock, which a model that is
still loading holds for the whole load. The stop then times out ("waiting to
shut down model: context deadline exceeded"), and the watchdog no longer
knows the model is there.

The tracking state is now captured before the backend is untracked, and put
back when the stop fails. This applies to every eviction path that untracks
first: group, LRU, idle timeout and busy timeout. A model that is already
gone (ErrModelNotFound) still counts as stopped, and a replacement
registered on the same address in the meantime is left alone. EvictedCount
only counts backends that really stopped, and NeedMore is set when one did
not, so the loader keeps retrying instead of treating the room as made. The
idle and busy checks retry the stop on their next tick.

The state is only put back while the loader still holds the model at that
address (ModelLoader.HasModelAtAddress). deleteProcess drops a remote-only
model, or the local half of a local+remote placement, before it reports a
failed remote unload; tracking that model again would leave a phantom entry
counted as loaded and as a group conflict. The memory reclaimer applies the
same rule and untracks a model the loader already dropped.

The untrack-before-stop ordering dates from mudler#7535, so this is not a
regression of 4.12.0.

Assisted-by: Claude:claude-sonnet-5.5 go vet
Signed-off-by: Hugues CHARBONNIER <hugues.charbonnier@gmail.com>
@localai-org-maint-bot

Copy link
Copy Markdown
Collaborator

@mudler review of head 3d2248d8 against master 3934ee8e3: needs changes.

P1: restoring the snapshot loses request lifecycle events during shutdown (pkg/model/watchdog.go:543–547). The watchdog untracks before calling shutdown, then restores the old inFlight and requestStarts values on failure:

  • If a request completes inside the shutdown interval, its completion callback finds no tracked request and returns. Restore then resurrects that completed request, leaving the backend marked busy.
  • If a new request starts during that interval, restore overwrites its tracking entry. Completing the older request then marks the backend idle while the new request is still running, allowing unsafe eviction.

I reproduced both deterministically with a fake process manager that invokes the completion callback or starts a request inside ShutdownModelForce, then returns a stop error. With force eviction enabled and LRU limit 1, both request-state assertions fail on this PR and pass on master. Please preserve/reconcile request lifecycle changes across a pending eviction and add regression coverage for both sequences; replaying the snapshot is insufficient.

Validation used the unmodified watchdog source and options with isolated Go/Ginkgo tests. The PR merges cleanly and passes whitespace checks. Full package tests remain unverified because generated protobuf bindings are absent and fetching protoc returned HTTP 403. This requires a concurrency-state change beyond a safe minor patch, so I have not pushed a speculative fix.

Restoring a snapshot after a failed stop lost the request lifecycle events
that happened while the stop was in progress. A request completing during
the stop found no tracked request, and the restore then resurrected it,
leaving the backend marked busy. A request starting during the stop was
overwritten by the restore, so completing an older request marked the
backend idle while the new one still ran, allowing an unsafe eviction.

An eviction now detaches only the backend identity (model, process, size
estimate) from the registry and marks the address as pending. Request
state stays live, so callbacks keep updating it during the stop. Idle and
busy checks skip pending addresses so they neither re-evict nor wipe that
state. When the stop settles, a backend that still runs is registered
again; otherwise its leftover request state is cleared. A replacement
registered on the address meanwhile is left alone.

Regression tests cover a request completing and a request starting inside
a failing force stop. The idle and busy failure tests now prove re-tracking
through a second stop attempt instead of sampling the registry, which
raced with the pending window.

Assisted-by: Claude:claude-opus-5.5 go vet golangci-lint
Signed-off-by: Hugues CHARBONNIER <hugues.charbonnier@gmail.com>
@hcharbonnier

Copy link
Copy Markdown
Author

Thanks for the reproduction, you are right: replaying the snapshot is unsound.

Fixed in c69e4fb0d (follow-up commit, no force-push).

Change
An eviction no longer snapshots and restores request state. It now detaches only the backend identity (model, process, size estimate) from the registry and marks the address as pending (pendingEvictions). Request lifecycle state (requestStarts, inFlight, busyTime, legacyRequests, idleTime, lastUsed) stays live during the stop, so TrackRequest completions and new requests update it normally.

  • checkIdle / checkBusy skip pending addresses, so they neither re-evict nor wipe that live state while the stop is in progress.
  • When the stop settles (finishEviction):
    • failed and the loader still holds the model → the identity is registered again, request state is untouched;
    • stopped, or failed after the loader already dropped it → leftover request state is cleared;
    • a replacement registered on the address meanwhile → nothing is touched.
  • The snapshot/restore code (snapshotLocked, restoreTracked, trackedState) is removed.

Regression coverage (pkg/model/watchdog_test.go, "request lifecycle during a stop that fails")
Both sequences you described, with a fake process manager that runs a hook inside ShutdownModelForce and then returns a stop error, force eviction enabled and LRU limit 1:

  1. a request completing during the stop is not resurrected: no InFlight / BusyTime entry afterwards;
  2. a request starting during the stop is kept: after the older request completes, InFlight is 1 and the backend stays busy until the new request completes.

I also made the idle and busy failure tests prove re-tracking through a second stop attempt instead of sampling the registry, which could race with the pending window.

Validation

  • go test ./pkg/model/: 202/202 pass.
  • Watchdog specs under -race: 8 consecutive clean runs.
  • go vet, golangci-lint run ./pkg/model/..., gofmt: clean.
  • With -race on the whole package, the race detector flags a pre-existing race in pkg/model/process_exit_test.go:38 (a bytes.Buffer read while the slog handler writes to it). That file is unchanged from master and unrelated to this PR.

@localai-org-maint-bot

Copy link
Copy Markdown
Collaborator

@mudler source-review sign-off on revised head 9a35647ec: the previous P1 finding is resolved. Good to merge once required CI checks and human DCO certification are satisfied.

The revision keeps request lifecycle state live while eviction is pending, so completed requests are not resurrected and newly started requests are not overwritten. Both original reproduction cases now pass. No new blocking source findings.

Validation: 72 isolated watchdog specs pass, including the two original regressions; focused go vet and whitespace checks pass, and the head integrates cleanly with master 3934ee8e3. These checks use the actual revised watchdog sources. Full model-package integration and race testing remain unverified here because generated protobuf bindings and a CGO compiler are unavailable. DCO currently reports ACTION_REQUIRED.

This branch has not been deployed

No deployments
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