Skip to content

fix(model): make backend shutdown model-scoped#10865

Open
richiejp wants to merge 1 commit into
mudler:masterfrom
richiejp:fix/watchdog-realtime-stuck-busy
Open

fix(model): make backend shutdown model-scoped#10865
richiejp wants to merge 1 commit into
mudler:masterfrom
richiejp:fix/watchdog-realtime-stuck-busy

Conversation

@richiejp

Copy link
Copy Markdown
Collaborator

Description

This should prevent deadlocking the model loader when a backend instance is refusing to stop or not responding. Also throw FizzBee at it because it's a distributed system sort of.

Notes for Reviewers

Avoid holding the global loader lock across backend lifecycle waits and propagate forced shutdown through distributed workers. Track parallel requests with in-flight counters and reserve worker ports until process termination.

Add focused race tests and an authoritative FizzBee lifecycle model with a fail-closed conformance target.

Assisted-by: Codex:GPT-5 [FizzBee] [Ginkgo]
Signed-off-by: Richard Palethorpe io@richiejp.com

Signed commits

  • Yes, I signed my commits.

Avoid holding the global loader lock across backend lifecycle waits and propagate forced shutdown through distributed workers. Track parallel requests with in-flight counters and reserve worker ports until process termination.

Add focused race tests and an authoritative FizzBee lifecycle model with a fail-closed conformance target.

Assisted-by: Codex:GPT-5 [FizzBee] [Ginkgo]
Signed-off-by: Richard Palethorpe <io@richiejp.com>
@localai-bot

Copy link
Copy Markdown
Collaborator

Did a deep review of this (including running the touched suites locally with -race: pkg/model, pkg/grpc, core/services/worker, core/services/nodes all pass). The locking redesign looks correct and well tested: the per-model refcounted semaphores are FIFO so exclusive shutdown cannot be starved, the loading-coalesce channel plus per-model health mutex plus store re-fetch inside deleteProcess prevent double-stop, and I found no lock-ordering cycles. The #10841/#10859 watchdog double-close fix is untouched. Wire compat for distributed workers is handled nicely (legacy nil-payload stop-all preserved, old workers ignore the new force field).

Two concrete issues in the worker supervisor before this merges, both small:

  1. Double port-free in the startBackend death path (core/services/worker/supervisor.go ~L143-150). If a backend.stop lands while startBackend is polling readiness, finishBackendStop deletes the entry and recycles the port; the readiness loop then sees !proc.IsAlive() and appends the same port to freePorts a second time, so two future backends can be handed the same port, which is the exact collision class this PR fixes. The timeout path at ~L164-168 already has the if cur, ok := s.processes[backend]; ok && cur == bp guard; the death path needs the same one.

  2. Success path can return an address being torn down (supervisor.go ~L128-133). The post-health-check validation checks !exists || currentBP != bp but not currentBP.stopping. Before this PR a concurrent stop deleted the map entry up front so !exists caught it; now the entry stays with stopping=true until termination completes, so startBackend (and the backend.install NATS reply) can report success with the address of a process that stopBackendExact is concurrently killing. One-line fix: also fail when currentBP.stopping.

Smaller notes, none blocking:

  • pkg/model/loader.go ~L478-485: the raw ml.store.Get(modelID) fast path can return a known-unreachable remote model, because deleteProcess now retains the store entry when a remote unload errors. Consider deleting the entry even on remote-unload error, or health-gating this path.
  • pkg/model/watchdog.go ~L246-258: with busyTime now anchored to the start of the continuous non-idle interval, a healthy parallel backend under sustained overlapping load (in-flight never reaching 0 for busyTimeout) gets force-killed mid-traffic. Fixing the old masking bug is right; a comment on this trade-off (or tracking the oldest in-flight request later) would help.
  • The new make test-model-lifecycle-conformance gate is not wired into any workflow or the pre-commit hook, so the FizzBee spec is never actually checked in CI despite the README calling it fail-closed. A workflow mirroring realtime-conformance.yml with a path filter on pkg/model/**, core/services/worker/** and the spec would close that.
  • ShutdownModel now returns ErrModelBusy after 30s instead of waiting forever, and LOCALAI_FORCE_BACKEND_SHUTDOWN changed meaning to "escalate to force after the graceful deadline". Existing callers treat it as best-effort so nothing breaks, but docs/content/ should mention the new behavior.

CI is green across the board. cc @mudler: core design is sound, recommend merging once the two supervisor guards above are in. @richiejp happy to re-review quickly after the update.

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