Skip to content

fix(distributed): reject wrong-model requests at the backend#10970

Merged
mudler merged 1 commit into
masterfrom
feat/predict-model-identity
Jul 20, 2026
Merged

fix(distributed): reject wrong-model requests at the backend#10970
mudler merged 1 commit into
masterfrom
feat/predict-model-identity

Conversation

@localai-bot

Copy link
Copy Markdown
Collaborator

Problem

In distributed mode the controller caches a NodeModel row naming a backend's host:port. A worker can recycle a stopped backend's gRPC port for a different model's backend. probeHealth verifies liveness, not identity, so the probe succeeds against whatever now occupies the port and the request is dispatched to the wrong backend. The caller gets a silent wrong-model answer.

Nothing in the request path could catch it:

  • PredictOptions had no model field, so identity crossed the wire only in ModelOptions.Model at LoadModel — and the cached-hit path in Route issues no LoadModel.
  • Every backend's "model not loaded" guard checks a nil handle (backend/python/vllm/backend.py:392-395, backend/cpp/llama-cpp/grpc-server.cpp:2183-2188, backend/go/parakeet-cpp/goparakeetcpp.go:295-297). A process holding a different model passes all of them, so IsModelNotLoaded never matched, reconcile never fired, and the stale row was never dropped.
  • Backend processes are single-model, so "wrong backend" means "a live process holding a different model" — and it answers.

Fixes #10952. PR #10968 (per-key port affinity) closes the common paths but cannot close this class: while liveness is the only check, a stale row over a re-bound port is indistinguishable from a valid one.

Approach

Add PredictOptions.ModelIdentity and enforce at the point of use, rather than in probeHealth — which short-circuits on a 30s probeCache keyed nodeID|addr and would skip verification during exactly the window that matters.

  • Controller: populated in gRPCPredictOpts from ModelConfig.Model.
  • Backends: pkg/grpc/server.go (27/27 Go backends), an interceptor in backend/python/common (36/36 Python backends, no per-backend change), and the llama-cpp / ik-llama-cpp / ds4 C++ servers.
  • Router: reconcile drops the stale replica row on a mismatch, so the next request reloads correctly.

Four RPCs take PredictOptions and all four are covered: Predict, PredictStream, Embedding, TokenizeString.

Why this cannot false-reject

ModelOptions(c, ...) passes model.WithModel(c.Model), which becomes ModelOptions.Model at LoadModel. gRPCPredictOpts(c, ...) receives the same config value in the same function a few lines later — verified at all four call sites (llm.go:136/177, embeddings.go:46/58, tokenize.go:32/39, face_embed.go:23/33). Equal by construction, not by convention.

stageModelFiles rewrites ModelFile, MMProj, LoraAdapter, DraftModel, CLIPModel, Tokenizer and AudioPath — but never Model. That matters given the managed-artifact work, which moves ModelFile to a snapshot directory.

Compatibility

Empty means skip, enforced on both sides:

  • New controller + old backend: field unknown, proto3 ignores it. Unprotected, as expected.
  • Old controller + new backend: field arrives empty, backend skips. Not optional — tests/e2e-backends/backend_test.go drives real backends with bare pb.PredictOptions{Prompt: ...} at 8+ sites.
  • A backend loaded by an old controller has no recorded identity, so it skips too.
  • The C++ server synthesizes PredictOptions internally on the ASR path — empty, so it skips.

A unit spec pins "empty identity is accepted", so a future tightening fails there before it breaks the e2e suite.

Deliberately not in scope

TTSRequest.model / SoundGenerationRequest.model are NOT validated, despite already carrying a model string. FileStagingClient.TTS/.TTSStream (file_staging_client.go:219,260) already rewrite in.Model into a worker-local absolute path, while the load-time value is untranslated — so in distributed mode those two strings already differ today. Comparing them would reject valid requests, in the exact configuration this fix targets, and only for TTS.

This also explains backend/go/piper/piper.go:29-31, which passes opts.Model straight to synthesis and stores nothing at load: it needs the translated path.

Extending identity to those RPCs requires a separate field carrying the untranslated value. Recorded in the proto comment so it is not rediscovered the hard way.

Known coverage gap (not a regression)

Rows are keyed by ModelID() but identity compares c.Model, so two configs sharing one Model with different ModelID() are a false accept. Sending ModelID() instead would false-reject universally, since it is not what LoadModel receives. c.Model is the only value with the structural guarantee above.

Why IsModelMismatch needs code AND sentinel

Unlike the neighbouring helpers, which match on code or message, this one requires NOT_FOUND and a sentinel substring.

backend/python/insightface/backend.py:127 returns NOT_FOUND "no face detected" from Embedding, which is a PredictOptions RPC. A code-only check would make reconcile drop a healthy replica row on every faceless image — turning this fix into a new bug. FailedPrecondition was unusable (claimed by IsModelNotLoaded), as was InvalidArgument (llama.cpp returns it ~15 times for ordinary bad parameters).

Documented in code, with a spec asserting IsModelMismatch is false for the insightface error, so a future "simplification" fails a test.

Scope correction during implementation

The long tail was originally scoped out as "not on the hot path". Checking it properly: privacy-filter implements no PredictOptions RPC at all, and kokoros answers all four with unimplemented. The tail was one backend — ds4, which has real Predict/PredictStream/TokenizeString — so it is included rather than left as the only exposed backend without the guard.

Testing

Every guard was verified red-first against the mistake it prevents, not merely written and observed green:

  • IsModelMismatch implemented code-only first, so the insightface spec failed on behavior: 3 Failed. Adding the sentinel turned it green.
  • The Python interceptor wired below get_auth_interceptors' early return first — the natural mistake, since it returns [] when LOCALAI_GRPC_AUTH_TOKEN is unset, which is the default. Red output: identity enforcement must be installed even with gRPC auth off (the default); got []. That got [] is the silent no-op across all 36 Python backends. Moving it above the token check: Ran 15 tests — OK.
  • Bare-PredictOptions acceptance passed from the start (correct shape — tightening it is what would break e2e); the red case was the mismatch being served when it should reject.
  • End-to-end: load identity A, predict identity B, assert mismatch and assert reconcile drops the row, for both unary and streamed calls.
ok  pkg/grpc              (race) 2.141s
ok  pkg/grpc/grpcerrors   (race) 1.022s
ok  core/backend                 0.544s
ok  core/services/nodes          175.373s
Python: Ran 15 tests — OK

make lint: 0 issues. Coverage 53.4% → 53.6%, baseline untouched.

C++ helpers could not be compiled in this environment; each was spliced out of the real source and compiled standalone against stubs, exercising all four rules (skip on empty request, skip on empty loaded, reject on mismatch, serve on match). Residual risk there is the call sites, not the logic.

Follow-ups

  • Transparent same-request retry on mismatch (deferred: it means re-running routing mid-request).
  • Model identity for the other modalities (GenerateImage, TranscriptRequest, RerankRequest, DetectOptions, VADRequest) — they share the exposure but carry no model field, so each needs its own.
  • Unrelated one-liner: root .gitignore:44 has a bare mock-backend pattern that matches the directory, so git will not descend into it even for tracked files (tests/e2e/mock-backend/main.go needed git add -f). Should be /mock-backend or mock-backend/mock-backend.

🤖 Generated with Claude Code

In distributed mode the controller caches a NodeModel row naming a backend's
host:port. A worker can recycle a stopped backend's gRPC port for a different
model's backend, and probeHealth verifies liveness rather than identity, so the
probe succeeds against whatever now occupies the port and the request is
dispatched to the wrong backend. The caller gets a silent wrong-model answer.

Nothing in the request could catch this: PredictOptions had no model field, so
model identity crossed the wire only in ModelOptions.Model at LoadModel time,
and the cached-hit path issues no LoadModel. Every backend's "model not loaded"
guard checks a nil handle, which a process holding a different model passes, so
the stale row was never dropped either.

Add PredictOptions.ModelIdentity and enforce it at the point of use:

  - The controller populates it in gRPCPredictOpts from ModelConfig.Model, the
    same expression ModelOptions feeds to model.WithModel and therefore the
    same value the backend received as ModelOptions.Model. Both are read from
    one config value in one function, so they are equal by construction and the
    comparison cannot false-reject.
  - Backends compare it against what they loaded and return NOT_FOUND with a
    fixed sentinel. Enforced in pkg/grpc/server.go (27 Go backends), an
    interceptor in backend/python/common (all 36 Python backends, no
    per-backend change), and the llama-cpp / ik-llama-cpp / ds4 C++ servers.
    That is every backend with real exposure: kokoros answers all four RPCs
    with unimplemented and privacy-filter implements none of them.
  - The router's reconcile drops the stale replica row on a mismatch, so the
    next request reloads somewhere correct.

Empty means "skip the check" on both sides: a controller that predates the
field sends nothing, a backend loaded by such a controller has nothing to
compare, and the C++ server synthesizes PredictOptions internally for ASR. That
keeps upgrades working in both directions.

Scoped to the four PredictOptions RPCs. TTSRequest.model and
SoundGenerationRequest.model are deliberately NOT validated: FileStagingClient
already rewrites them to worker-local absolute paths, so in distributed mode
they already differ from the load-time value and comparing them would reject
valid requests.

IsModelMismatch requires both the NOT_FOUND code and the sentinel, unlike the
neighbouring helpers which accept either. insightface's Embedding returns
NOT_FOUND "no face detected" on a PredictOptions RPC, and a code-only check
would drop a healthy replica row on every faceless image.

Signed-off-by: Ettore Di Giacinto <mudler@localai.io>
Assisted-by: Claude Code:claude-opus-4-8[1m] [Read] [Edit] [Bash]
@mudler
mudler merged commit 465d488 into master Jul 20, 2026
103 checks passed
@mudler
mudler deleted the feat/predict-model-identity branch July 20, 2026 11:05
mudler added a commit that referenced this pull request Jul 20, 2026
… coverage-ratchet fixes (#10989)

* fix(http): make /readyz reflect startup readiness instead of always 200

/readyz was registered as a static handler returning 200 unconditionally,
so it carried no information: it was green whenever it could be reached at
all. Readiness could not distinguish "serving" from "still starting", and
any future change that started the HTTP listener earlier would silently
turn the probe into a lie.

Track startup completion on the Application (atomic flag, flipped at the
very end of New() on the success path only) and have the readiness handler
consult it per request, returning 503 with a small JSON body while startup
is in progress. A nil readiness source fails open so embedders keep the
historical behaviour.

/healthz is deliberately left readiness-independent. Liveness and readiness
answer different questions, and failing liveness during a long preload makes
an orchestrator restart the pod mid-download so the preload never finishes.

This matters because since #10949 the startup preload materializes
HuggingFace artifacts for managed backends: tens of GB for a large model
(31 GB observed on a live cluster). Both probes stay in quietPaths and stay
exempt from auth.

Note the listener is still started only after New() returns, so today the
not-ready state is not observable over HTTP. Moving the listener earlier is
a separate, deliberate decision and is not made here.

Signed-off-by: Ettore Di Giacinto <mudler@localai.io>
Assisted-by: Claude:claude-opus-4-8 [Claude Code]

* chore(gitignore): anchor the mock-backend pattern so its source dir is traversable

The bare `mock-backend` pattern matched the *directory*
tests/e2e/mock-backend/, not just the binary built into it. Git will not
descend into an ignored directory even for tracked files, so
`git add tests/e2e/mock-backend/main.go` required -f. This was hit while
working on #10970.

Anchor it to the artifact's full path. The built binary stays ignored (it is
also covered by tests/e2e/mock-backend/.gitignore) while the source directory
becomes traversable again.

Verified with `git check-ignore -v`: a new source file under
tests/e2e/mock-backend/ is no longer ignored, and the binary produced by
`make build-mock-backend` still is.

Signed-off-by: Ettore Di Giacinto <mudler@localai.io>
Assisted-by: Claude:claude-opus-4-8 [Claude Code]

* chore(coverage): raise the coverage ratchet from 48.5% to 54.2%

The committed baseline had drifted well below reality: it still read 48.5%
while a full instrumented run measures 54.2%. A stale-low baseline makes the
gate meaningless — coverage could regress by more than 5 percentage points
and still pass.

Raising a ratchet is a deliberate act, not something to fold into an
unrelated fix, so it gets its own commit. The headroom was earned by tests
landed in #10946, #10947, #10948, #10949, #10956, #10967, #10968, #10970 and
#10975.

Measured with `make test-coverage` on this branch (the same instrumented run
`make test-coverage-baseline` uses: ginkgo over ./pkg and ./core plus the
in-process tests/e2e suite, --covermode=atomic, --coverpkg over core/... and
pkg/..., generated protobuf excluded). The run completed with exit 0 and zero
spec failures; the total was then written with the exact command the
test-coverage-baseline target uses:

  go tool cover -func=coverage/coverage.out \
    | awk '/^total:/{gsub(/%/,"",$NF); print $NF}' > coverage-baseline.txt

Verified afterwards with scripts/coverage-check.sh, which reports OK.

Note the measured figure includes the readiness specs added earlier on this
branch, so it is a demonstrated floor rather than an estimate.

Signed-off-by: Ettore Di Giacinto <mudler@localai.io>
Assisted-by: Claude:claude-opus-4-8 [Claude Code]

---------

Signed-off-by: Ettore Di Giacinto <mudler@localai.io>
Co-authored-by: Ettore Di Giacinto <mudler@localai.io>
mudler added a commit that referenced this pull request Jul 20, 2026
…ties (#10990)

#10970 gave the four PredictOptions RPCs a model-identity check so a
backend reached through a stale distributed route rejects the request
instead of answering from whatever model it holds (#10952). Every other
modality shares that exposure: the route is cached by host:port, a worker
can recycle a stopped backend's port for another model's backend, and a
liveness-only probe cannot tell a stale row from a valid one.

Extends the same mechanism to the 21 remaining request messages that reach
a backend through the router, using the pattern #10970 established rather
than a parallel one:

- proto: ModelIdentity on each modality request message.
- controller: populated from ModelConfig.Model at the call site that also
  builds ModelOptions, so load-time and request-time values are equal by
  construction.
- backends: one generic guard in pkg/grpc/server.go (27 Go backends), the
  method set in backend/python/common (36 Python backends), llama-cpp
  (AudioTranscription/Stream, Rerank, Score) and privacy-filter
  (TokenClassify).
- reconcile already drops the stale row on IsModelMismatch; no change.

TTSRequest and SoundGenerationRequest get a SEPARATE ModelIdentity field
rather than reusing their existing `model`: FileStagingClient rewrites
`model` to a worker-local path, so comparing it would reject valid
requests in exactly the configuration this guards.

AudioEncode/AudioDecode are deliberately left unguarded: the opus codec
backend is loaded from a literal rather than a ModelConfig, so no value
carries the equality guarantee the comparison depends on. The four
bidirectional stream RPCs are out of scope; they bypass reconcile.

Empty means skip on both sides, so an old controller, an old backend, and
the bare request structs in tests/e2e-backends all keep working.


Assisted-by: Claude Code:claude-opus-4-8 [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.

distributed: worker recycles a stopped backend's gRPC port, allowing a silent misroute

2 participants