fix(server): reconcile alias_to_name on reload (#150)#153
Draft
marksverdhei wants to merge 1 commit into
Draft
Conversation
load_models() reload rebuilds each non-running model's meta.aliases but never updated the global alias_to_name map (the pre-loop cleanup only drops entries whose target model is gone). get_meta() consults alias_to_name before the authoritative meta.aliases scan, so an alias removed from — or moved off — a still-existing model kept resolving to the old model (has_model, which skips the map, disagreed). After re-parsing a model's aliases, reconcile the map: erase that model's own entries, then register its current aliases. Scoped to the (non-running) model being re-parsed, so a running instance's entries — which #135 deliberately keeps when its meta.aliases are shadowed/lost — are left untouched. Correct for add/remove/move regardless of iteration order.
14 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #150.
Problem
load_models()reload rebuilds each non-running model'smeta.aliasesbut never reconciled the globalalias_to_namemap (the pre-loop cleanup atserver-models.cpp:595only drops entries whose target model is gone). Sinceget_meta()consultsalias_to_namebefore the authoritativemeta.aliasesscan, an alias removed from or moved off a still-existing model kept resolving to the old model — whilehas_model()(which skips the map) disagreed. Editing a preset INI to rename/move an alias +POST /models/reloadreproduces it.Fix
After the re-parse loop finalizes a model's
meta.aliases, reconcile the map: erase that model's own entries, then register its current aliases. Correct for add/remove/move regardless of the order models are iterated.Scoping (why not clear-and-rebuild the whole map): #135 (
ecf2de428) addedalias_to_namespecifically so aliases still resolve when a loaded instance'smeta.aliasesare shadowed/lost, and reload only re-parses non-running models. So the reconcile is scoped to the model being re-parsed — a running instance's entries are left untouched, preserving #135's behavior.Verification
cmake --build --target llama-server(GGML_CUDA=OFF) —server-models.cppcompiles clean, links. CI covers the matrix.unload_lru/load/update_status; this touchesload_modelsreload) — different regions, no conflict, mergeable in either order.Testing note
The bug lives in
get_meta()'salias_to_namefast-path, which is hard to isolate through the black-box HTTP API (the correctmeta.aliasesscan masks it, and no endpoint echoesget_meta's resolved name). Did not add a non-isolating test rather than a flaky one. A targeted C++ unit test onserver_models(move an alias between two presets across a reload, assertget_metafollows) would be the right home if a test seam is added.Known limitation (pre-existing, not introduced here)
Alias-conflict detection during reload (
:624) usesmeta.aliases, which can be shadowed for a running model — so a re-parsed model could still claim a running model's alias in that compound edge case. That's the pre-existing weakness #135 works around; this PR doesn't widen or fix it.