From ad074cea3131e8a0a1c76fa50f2c813c5820fc8b Mon Sep 17 00:00:00 2001 From: kami Date: Sat, 1 Aug 2026 03:59:08 +0400 Subject: [PATCH] Swap the resident model without restarting mavend (#250) Loading a different gguf was a one-line edit to phraser.model_path plus a restart. It is now an owner-triggered IPC call, off unless configured. internal/phraser/swap.go holds the safety properties as code: - Never two models resident. The old llama-server is killed and reaped before the new one is launched. One 1.7B fits the Vega iGPU; a blue/green overlap would OOM the box, so it is not offered. - Atomic from a turn's point of view. Swap drains the in-flight turns (they finish on the old model), then refuses arrivals with ErrSwapping until the new server has answered /v1/models. No turn ever sees half a swap; refused turns fall back to the classifier cascade. - A failed load rolls back. If the new model does not start or does not probe, the previous one is reloaded and the call returns RolledBack with the error. If the rollback also fails the daemon says so and degrades to the classifier rather than pretending to serve. Holders of the completion client are re-pointed, not rebuilt: llm.Client guards its base URL and LLMPhraser.OnSwap re-points it, so the router, the replier, the mail extractor and the memory evaluator follow the new port without knowing a swap happened. Reach is deliberately narrow. phraser.swap_models is an exact-match allowlist of absolute paths a human wrote, rejected at startup otherwise, so "swap the model" can never mean "load any file on my disk"; the running model is always swappable back to. MethodSwapModel is AuthStepUp, the same rung as mutating the tool allowlist, and /models gates POST through the same stepUpOK the tools page uses. Nothing calls Swap on a timer and no act, intent or utterance reaches it. Vikunja #250 --- cmd/mavend/mail.go | 3 +- cmd/mavend/main.go | 2 + cmd/mavend/memoryeval.go | 3 +- cmd/mavend/modelswap.go | 113 +++++++++++ cmd/mavend/voicewire.go | 4 +- cmd/mavweb/main.go | 11 ++ cmd/mavweb/models.go | 145 +++++++++++++++ cmd/mavweb/models_test.go | 150 +++++++++++++++ internal/auth/auth_test.go | 23 +++ internal/auth/policy.go | 12 +- internal/config/config.go | 23 +++ internal/config/config_test.go | 32 ++++ internal/ipc/api.go | 44 +++++ internal/ipc/client.go | 22 +++ internal/ipc/ipc_test.go | 42 +++++ internal/ipc/server.go | 48 ++++- internal/ipc/wire.go | 2 + internal/llm/client.go | 28 ++- internal/llm/client_test.go | 34 ++++ internal/phraser/llmphraser.go | 184 ++++++++++++++---- internal/phraser/swap.go | 330 +++++++++++++++++++++++++++++++++ internal/phraser/swap_test.go | 311 +++++++++++++++++++++++++++++++ 22 files changed, 1523 insertions(+), 43 deletions(-) create mode 100644 cmd/mavend/modelswap.go create mode 100644 cmd/mavweb/models.go create mode 100644 cmd/mavweb/models_test.go create mode 100644 internal/phraser/swap.go create mode 100644 internal/phraser/swap_test.go diff --git a/cmd/mavend/mail.go b/cmd/mavend/mail.go index 2e6e796..ebde48a 100644 --- a/cmd/mavend/mail.go +++ b/cmd/mavend/mail.go @@ -27,7 +27,6 @@ import ( "github.com/kami/maven/internal/config" "github.com/kami/maven/internal/email" "github.com/kami/maven/internal/ipc" - "github.com/kami/maven/internal/llm" "github.com/kami/maven/internal/phraser" "github.com/kami/maven/internal/store" ) @@ -66,7 +65,7 @@ func newMailIntake(st *store.Store, phr phraser.Phraser, cfg *config.Config) *ma if timeout <= 0 { timeout = config.DefaultEmailTimeout } - ex := email.NewExtractor(llm.New(lp.BaseURL(), timeout), cfg.Email.MaxTasks, contextBlockFn(cfg, time.Now)) + ex := email.NewExtractor(llmClientFor(lp, timeout), cfg.Email.MaxTasks, contextBlockFn(cfg, time.Now)) log.Printf("mail intake: enabled (max %d candidates per message, timeout %s)", cfg.Email.MaxTasks, timeout) return &mailIntake{st: st, ex: ex, timeout: timeout, now: time.Now} } diff --git a/cmd/mavend/main.go b/cmd/mavend/main.go index 0c10d80..e4fffa2 100644 --- a/cmd/mavend/main.go +++ b/cmd/mavend/main.go @@ -329,6 +329,7 @@ func run(args []string) error { // ipc.MethodIngestMail reports ErrUnknownMethod. if !locked { wireMailIntake(srv, st, phr, cfg) + wireModelSwap(srv, phr, cfg) } // WrapKeyFn — wraps the env key with a passkey credential public key and @@ -471,6 +472,7 @@ func run(args []string) error { srv.SetAPI(newAPI) srv.Check = (&auth.Gate{Enrollment: auth.NewFloorEnrollment(), Session: passkeySess}).Check wireMailIntake(srv, st, phr, cfg) + wireModelSwap(srv, phr, cfg) // Start voice server. if voiceW != nil { diff --git a/cmd/mavend/memoryeval.go b/cmd/mavend/memoryeval.go index 9f0c641..2faea11 100644 --- a/cmd/mavend/memoryeval.go +++ b/cmd/mavend/memoryeval.go @@ -16,7 +16,6 @@ import ( "time" "github.com/kami/maven/internal/config" - "github.com/kami/maven/internal/llm" "github.com/kami/maven/internal/memeval" "github.com/kami/maven/internal/phraser" "github.com/kami/maven/internal/store" @@ -50,7 +49,7 @@ func newMemoryEvalWorker(st *store.Store, phr phraser.Phraser, cfg *config.Confi } // A generous per-request timeout: this is a long prompt to a Thinking model // and nobody is waiting on the answer. - client := llm.New(lp.BaseURL(), 5*time.Minute) + client := llmClientFor(lp, 5*time.Minute) ev := memeval.NewEvaluator(st, st, client, memeval.Config{ MaxItems: cfg.MemoryEval.MaxItems, MinConfidence: cfg.MemoryEval.MinConfidence, diff --git a/cmd/mavend/modelswap.go b/cmd/mavend/modelswap.go new file mode 100644 index 0000000..fae8cb7 --- /dev/null +++ b/cmd/mavend/modelswap.go @@ -0,0 +1,113 @@ +package main + +import ( + "context" + "fmt" + "log" + "path/filepath" + "time" + + "github.com/kami/maven/internal/config" + "github.com/kami/maven/internal/ipc" + "github.com/kami/maven/internal/llm" + "github.com/kami/maven/internal/phraser" +) + +// Swapping the resident model while the daemon runs (Vikunja #250). +// +// Off unless configured: with no phraser.swap_models allowlist the two IPC +// methods are never wired, so they answer ErrUnknownMethod. When it is wired the +// swap method is AuthStepUp (internal/auth), which means an authed human surface +// only — there is no act, no intent and no timer that reaches it. The daemon +// never decides to change its own brain. +// +// The allowlist is exact-match against paths a human wrote in mavend.json. The +// request carries a path and llama-server is started with it as `-m`, so +// anything looser would turn "swap the model" into "load any file on my disk". +func wireModelSwap(srv *ipc.Server, phr phraser.Phraser, cfg *config.Config) { + if cfg.Phraser == nil || len(cfg.Phraser.SwapModels) == 0 { + return + } + lp, ok := phr.(*phraser.LLMPhraser) + if !ok { + log.Printf("model swap: phraser.swap_models is set but there is no llama-server phraser — swap disabled") + return + } + allowed := map[string]bool{} + for _, m := range cfg.Phraser.SwapModels { + allowed[filepath.Clean(m)] = true + } + // The configured model is always swappable back to, listed or not: the way + // out of a bad swap must not depend on remembering to allowlist the model + // you are already running. + allowed[filepath.Clean(cfg.Phraser.ModelPath)] = true + + srv.SwapModelFn = func(ctx context.Context, req ipc.SwapModelReq) (ipc.SwapModelResp, error) { + path := filepath.Clean(req.ModelPath) + if !allowed[path] { + log.Printf("model swap: REFUSED %q — not in phraser.swap_models", req.ModelPath) + return ipc.SwapModelResp{}, fmt.Errorf("%w: %q is not in phraser.swap_models", ipc.ErrForbidden, req.ModelPath) + } + res, err := lp.Swap(ctx, phraser.SwapSpec{ + ModelPath: path, + NGpuLayers: req.NGpuLayers, + NCtx: req.NCtx, + }) + resp := ipc.SwapModelResp{ + Model: res.Model, + ModelPath: res.ModelPath, + BaseURL: res.BaseURL, + RolledBack: res.RolledBack, + TookMs: res.Took.Milliseconds(), + } + if err != nil { + // A rolled-back swap is a failure that left a working daemon behind. + // Both halves matter to the caller, so the response is filled in even + // though the error is returned. + log.Printf("model swap: %v", err) + return resp, err + } + return resp, nil + } + + srv.ModelStatusFn = func(ctx context.Context) (ipc.ModelStatusResp, error) { + path, ngl, nctx := lp.LiveModel() + base := lp.BaseURL() + resp := ipc.ModelStatusResp{ + ModelPath: path, + BaseURL: base, + NGpuLayers: ngl, + NCtx: nctx, + Swappable: cfg.Phraser.SwapModels, + } + if base == "" { + resp.Model = llm.UnknownModel + return resp, nil + } + id, err := llm.ModelID(ctx, base) + if err != nil { + // Report the honest "I could not confirm it" rather than echoing the + // configured filename as if the server had said it. + resp.Model = llm.UnknownModel + return resp, nil + } + resp.Model = id + return resp, nil + } + + log.Printf("model swap: enabled, %d allowlisted model(s) — step-up required", len(cfg.Phraser.SwapModels)) +} + +// llmClientFor builds a completion client on the phraser's llama-server and +// keeps it pointed at the right one across a model swap. +// +// Without the OnSwap registration every holder of a base URL — the LLM router, +// the replier, the mail extractor, the memory evaluator — would keep talking to +// the port of a server that no longer exists, and the daemon would degrade to +// the classifier permanently after the first swap. The client is re-pointed, not +// rebuilt, so nothing that holds it has to know a swap happened. +func llmClientFor(lp *phraser.LLMPhraser, timeout time.Duration) *llm.Client { + c := llm.New(lp.BaseURL(), timeout) + lp.OnSwap(func(base string) { c.SetBaseURL(base) }) + return c +} diff --git a/cmd/mavend/voicewire.go b/cmd/mavend/voicewire.go index 7167511..06fa81a 100644 --- a/cmd/mavend/voicewire.go +++ b/cmd/mavend/voicewire.go @@ -148,7 +148,9 @@ func wireVoice(cfg *config.Config, coreAPI ipc.CoreAPI, phr phraser.Phraser, mem // The replier uses the same llama-server as the phraser. var llmClient *llm.Client if lp, ok := phr.(*phraser.LLMPhraser); ok { - llmClient = llm.New(lp.BaseURL(), 60*time.Second) + // llmClientFor, not llm.New: this client must follow the phraser onto + // the new llama-server when the resident model is swapped (Vikunja #250). + llmClient = llmClientFor(lp, 60*time.Second) } // ----- router (the cascade; floor examples seed the classifier) ----- // The act matcher's allowlist is exactly the enabled tool names — the diff --git a/cmd/mavweb/main.go b/cmd/mavweb/main.go index b88bb06..4491084 100644 --- a/cmd/mavweb/main.go +++ b/cmd/mavweb/main.go @@ -124,6 +124,7 @@ var sidebarSections = []struct { Label: "Settings", Pages: []struct{ Label, URL, Key string }{ {Label: "Tools", URL: "/tools", Key: "tools"}, + {Label: "Model", URL: "/models", Key: "models"}, {Label: "Passkey", URL: "/auth/passkey", Key: "passkey"}, }, }, @@ -191,6 +192,8 @@ func pageIcon(key string) string { return `` case "tools": return `` + case "models": + return `` case "passkey": return `` default: @@ -225,6 +228,8 @@ func pageTitle(key string) string { return "Ecosystem" case "tools": return "Tools" + case "models": + return "Resident Model" case "passkey": return "Passkey" default: @@ -479,6 +484,12 @@ func main() { mux.HandleFunc("/routines", func(w http.ResponseWriter, r *http.Request) { handleRoutines(w, r, core, stepUpSession, *requireStepUp) }) + // /models — the resident-model surface (Vikunja #250). Same step-up gate as + // /tools, and for a comparable reason: which model is loaded decides how every + // utterance is routed and how every reply is worded. GET is read-only. + mux.HandleFunc("/models", func(w http.ResponseWriter, r *http.Request) { + handleModels(w, r, core, stepUpSession, *requireStepUp) + }) // State-changing routes on this server, and their gate (Vikunja #317): // diff --git a/cmd/mavweb/models.go b/cmd/mavweb/models.go new file mode 100644 index 0000000..11c4d41 --- /dev/null +++ b/cmd/mavweb/models.go @@ -0,0 +1,145 @@ +package main + +import ( + "context" + "errors" + "html/template" + "log" + "net/http" + "strconv" + "strings" + + "github.com/kami/maven/internal/ipc" + "github.com/kami/maven/internal/webauthn" +) + +// The resident-model surface (Vikunja #250). +// +// GET shows which model llama-server actually has loaded and which files the +// daemon is configured to allow. POST swaps to one of them, behind the same +// step-up gate as POST /tools: the loaded model decides how every utterance is +// routed and how every reply is worded, so it is an owner action. +// +// There is nothing on this page Maven can press. The swap is an IPC method rated +// AuthStepUp in internal/auth, unreachable from an act, an intent or a timer. + +// modelController — the two non-CoreAPI methods this page needs. *ipc.Client +// satisfies it; a core without a swap allowlist answers ErrUnknownMethod, which +// the page renders as "not configured" rather than an error. +type modelController interface { + ModelStatus(ctx context.Context) (ipc.ModelStatusResp, error) + SwapModel(ctx context.Context, req ipc.SwapModelReq) (ipc.SwapModelResp, error) +} + +var modelsTmpl = template.Must(template.New("models").Funcs(shellFuncs()).Parse(shellTopHTML + modelsHTML + shellBottomHTML)) + +const modelsHTML = `{{template "shellTop" "models"}} +

Resident model

+

swapping requires step-up — assert a passkey first. The old model is unloaded before the new one is loaded (one model fits the iGPU at a time), so turns during the load are refused and fall back to the classifier.

+{{if .Msg}}
{{.Msg}}
{{end}} +{{if .Err}}
{{.Err}}
{{end}} +{{if .Off}} +
+

swap not configured

+

this core has no phraser.swap_models allowlist, so there is nothing to swap to. Add the gguf paths you allow to deploy/mavend.json and restart once.

+
+{{else}} +
+

loaded now

+
+ + + + + +
model{{.Status.Model}}
file{{.Status.ModelPath}}
server{{.Status.BaseURL}}
n_ctx{{.Status.NCtx}}
n_gpu_layers{{.Status.NGpuLayers}}
+

the model name is what llama-server reports for itself, not what the config says it should be.

+
+
+

allowed models {{len .Status.Swappable}}

+{{if .Status.Swappable}}
+{{range .Status.Swappable}} +{{end}} +
file
{{.}}
+ +
+{{else}}
no models allowlisted
{{end}} +
+{{end}} +{{template "shellBottom"}}` + +type modelsPage struct { + Msg string + Err string + Off bool + Status ipc.ModelStatusResp +} + +// handleModels renders the model surface (GET) and applies a swap (POST). +// +// A failed swap is reported as a failure with the model that is still serving +// named, because that is the state the operator needs: the daemon rolled back +// and is answering turns, it just is not answering them with what he asked for. +func handleModels(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI, session *webauthn.PasskeySession, requireStepUp bool) { + if core == nil { + http.Error(w, "models disabled (no -core)", http.StatusServiceUnavailable) + return + } + mc, ok := core.(modelController) + if !ok { + http.Error(w, "models unavailable: core connection does not support model swap", http.StatusServiceUnavailable) + return + } + ctx := r.Context() + page := modelsPage{} + + if r.Method == http.MethodPost { + if !stepUpOK(session, requireStepUp) { + http.Error(w, "step-up required: assert a passkey first", http.StatusForbidden) + return + } + path := strings.TrimSpace(r.FormValue("model_path")) + if path == "" { + http.Error(w, "model_path required", http.StatusBadRequest) + return + } + req := ipc.SwapModelReq{ModelPath: path} + if v, err := strconv.Atoi(r.FormValue("n_ctx")); err == nil { + req.NCtx = v + } + res, err := mc.SwapModel(ctx, req) + switch { + case err == nil: + page.Msg = "loaded " + res.Model + " (" + strconv.FormatInt(res.TookMs, 10) + "ms)" + log.Printf("models: swapped to %s (%s) in %dms", res.ModelPath, res.Model, res.TookMs) + case errors.Is(err, ipc.ErrForbidden): + http.Error(w, "refused: that model is not in phraser.swap_models, or step-up was not asserted", http.StatusForbidden) + return + case errors.Is(err, ipc.ErrUnknownMethod): + http.Error(w, "swap not configured on this core", http.StatusServiceUnavailable) + return + case res.RolledBack: + page.Err = "swap failed, rolled back to " + res.Model + " — she is still answering, with the old model" + log.Printf("models: swap to %s failed, rolled back: %v", path, err) + default: + page.Err = "swap failed: " + err.Error() + log.Printf("models: swap to %s failed: %v", path, err) + } + } + + st, err := mc.ModelStatus(ctx) + if err != nil { + if errors.Is(err, ipc.ErrUnknownMethod) { + page.Off = true + } else { + log.Printf("models: status: %v", err) + http.Error(w, "core read failed", http.StatusBadGateway) + return + } + } + page.Status = st + w.Header().Set("Content-Type", "text/html; charset=utf-8") + if err := modelsTmpl.Execute(w, page); err != nil { + log.Printf("models render: %v", err) + } +} diff --git a/cmd/mavweb/models_test.go b/cmd/mavweb/models_test.go new file mode 100644 index 0000000..1115e9e --- /dev/null +++ b/cmd/mavweb/models_test.go @@ -0,0 +1,150 @@ +package main + +import ( + "context" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/kami/maven/internal/ipc" + "github.com/kami/maven/internal/webauthn" +) + +// fakeModelCore is a core that supports the two model methods. It records what +// the page asked for, so the tests can assert the gate rather than the HTML. +type fakeModelCore struct { + ipc.UnimplementedCoreAPI + + status ipc.ModelStatusResp + statusErr error + + swapResp ipc.SwapModelResp + swapErr error + swapped []ipc.SwapModelReq +} + +func (f *fakeModelCore) ModelStatus(ctx context.Context) (ipc.ModelStatusResp, error) { + return f.status, f.statusErr +} + +func (f *fakeModelCore) SwapModel(ctx context.Context, req ipc.SwapModelReq) (ipc.SwapModelResp, error) { + f.swapped = append(f.swapped, req) + return f.swapResp, f.swapErr +} + +func modelsGET(t *testing.T, core ipc.CoreAPI) *httptest.ResponseRecorder { + t.Helper() + w := httptest.NewRecorder() + handleModels(w, httptest.NewRequest(http.MethodGet, "/models", nil), core, nil, false) + return w +} + +func modelsPOST(t *testing.T, core ipc.CoreAPI, session *webauthn.PasskeySession, requireStepUp bool, path string) *httptest.ResponseRecorder { + t.Helper() + r := httptest.NewRequest(http.MethodPost, "/models", strings.NewReader("model_path="+path)) + r.Header.Set("Content-Type", "application/x-www-form-urlencoded") + w := httptest.NewRecorder() + handleModels(w, r, core, session, requireStepUp) + return w +} + +func TestModels_GETShowsTheLoadedModelAndTheAllowlist(t *testing.T) { + core := &fakeModelCore{status: ipc.ModelStatusResp{ + Model: "Qwen3-1.7B-UD-Q4_K_XL", + ModelPath: "/opt/maven/models/llm/qwen3.gguf", + BaseURL: "http://127.0.0.1:18099", + NCtx: 4096, + Swappable: []string{"/opt/maven/models/llm/qwen3.gguf", "/opt/maven/models/llm/qwen3-cpt.gguf"}, + }} + w := modelsGET(t, core) + if w.Code != http.StatusOK { + t.Fatalf("GET /models = %d; want 200", w.Code) + } + body := w.Body.String() + for _, want := range []string{"Qwen3-1.7B-UD-Q4_K_XL", "qwen3-cpt.gguf", "4096"} { + if !strings.Contains(body, want) { + t.Errorf("page does not mention %q", want) + } + } + if len(core.swapped) != 0 { + t.Errorf("a GET swapped the model: %v", core.swapped) + } +} + +func TestModels_POSTRequiresStepUpWhenFailingClosed(t *testing.T) { + // No WebAuthn configured (nil session) + -require-stepup ⇒ deny, exactly + // like POST /tools. Nothing reaches core. + core := &fakeModelCore{} + w := modelsPOST(t, core, nil, true, "/opt/maven/models/llm/qwen3.gguf") + if w.Code != http.StatusForbidden { + t.Fatalf("POST /models without assertable step-up = %d; want 403", w.Code) + } + if len(core.swapped) != 0 { + t.Fatalf("a denied POST still called SwapModel: %v", core.swapped) + } +} + +func TestModels_POSTSwapsAndReportsTheModelThatAnswered(t *testing.T) { + core := &fakeModelCore{ + swapResp: ipc.SwapModelResp{Model: "qwen3-cpt", ModelPath: "/m/cpt.gguf", TookMs: 4200}, + status: ipc.ModelStatusResp{Model: "qwen3-cpt", ModelPath: "/m/cpt.gguf"}, + } + w := modelsPOST(t, core, nil, false, "/m/cpt.gguf") + if w.Code != http.StatusOK { + t.Fatalf("POST /models = %d; want 200", w.Code) + } + if len(core.swapped) != 1 || core.swapped[0].ModelPath != "/m/cpt.gguf" { + t.Fatalf("SwapModel calls = %v; want one for /m/cpt.gguf", core.swapped) + } + if !strings.Contains(w.Body.String(), "loaded qwen3-cpt") { + t.Errorf("page does not report which model was loaded:\n%s", w.Body.String()) + } +} + +func TestModels_RolledBackSwapSaysSheIsStillAnswering(t *testing.T) { + core := &fakeModelCore{ + swapResp: ipc.SwapModelResp{Model: "qwen3", ModelPath: "/m/old.gguf", RolledBack: true}, + swapErr: errBrokenModel{}, + status: ipc.ModelStatusResp{Model: "qwen3", ModelPath: "/m/old.gguf"}, + } + w := modelsPOST(t, core, nil, false, "/m/cpt.gguf") + if w.Code != http.StatusOK { + t.Fatalf("POST /models after a rollback = %d; want 200 with the failure rendered", w.Code) + } + body := w.Body.String() + if !strings.Contains(body, "rolled back to qwen3") { + t.Errorf("page does not say it rolled back:\n%s", body) + } +} + +func TestModels_RefusedPathIs403(t *testing.T) { + core := &fakeModelCore{swapErr: ipc.ErrForbidden} + w := modelsPOST(t, core, nil, false, "/etc/passwd") + if w.Code != http.StatusForbidden { + t.Fatalf("POST /models with a non-allowlisted path = %d; want 403", w.Code) + } +} + +func TestModels_UnconfiguredCoreRendersOff(t *testing.T) { + core := &fakeModelCore{statusErr: ipc.ErrUnknownMethod} + w := modelsGET(t, core) + if w.Code != http.StatusOK { + t.Fatalf("GET /models against a core without the swap = %d; want 200", w.Code) + } + if !strings.Contains(w.Body.String(), "swap not configured") { + t.Errorf("page does not say the capability is off:\n%s", w.Body.String()) + } +} + +func TestModels_CoreWithoutTheMethodsIs503(t *testing.T) { + // An in-process CoreAPI (no swap methods) must not 500 the page. + w := modelsGET(t, ipc.UnimplementedCoreAPI{}) + if w.Code != http.StatusServiceUnavailable { + t.Fatalf("GET /models on a core without the methods = %d; want 503", w.Code) + } +} + +type errBrokenModel struct{} + +func (errBrokenModel) Error() string { return "llm: server did not start" } diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index c7c1872..67907af 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -393,3 +393,26 @@ func mustWriteFactParams(source string) []byte { } return b } + +// TestRequirement_SwapModel — loading a different resident model is an owner +// action at the same rung as mutating the tool allowlist: it decides how every +// utterance is routed and how every reply is worded. The read side is not. +func TestRequirement_SwapModel(t *testing.T) { + if got := Requirement(ipc.MethodSwapModel); got != AuthStepUp { + t.Errorf("SwapModel authority = %v; want AuthStepUp", got) + } + if got := Requirement(ipc.MethodModelStatus); got != AuthRead { + t.Errorf("ModelStatus authority = %v; want AuthRead", got) + } + // A surface that cannot carry a passkey gesture cannot swap the model, no + // matter what it is enrolled as — this is the "never through voice" property. + voice := Scope{Surface: SurfaceVoice, Module: "voice", SourceScope: []string{"*"}} + if err := Can(ipc.MethodSwapModel, voice, nil); !errors.Is(err, ErrForbidden) { + t.Errorf("voice swapping the model = %v; want ErrForbidden", err) + } + // And with no step-up session asserted, the gate refuses even a capable surface. + noSession := &Gate{Enrollment: NewFloorEnrollment()} + if err := noSession.Check(context.Background(), ipc.MethodSwapModel, nil); !errors.Is(err, ipc.ErrForbidden) { + t.Errorf("SwapModel with no asserted step-up = %v; want ErrForbidden", err) + } +} diff --git a/internal/auth/policy.go b/internal/auth/policy.go index 429edc3..ae6e9d0 100644 --- a/internal/auth/policy.go +++ b/internal/auth/policy.go @@ -53,6 +53,13 @@ func Requirement(m ipc.Method) Authority { // asserted — never a module or the voice/chat path. maven can propose // (MethodProposeTool, no step-up: she has no passkey) but never en/disable. return AuthStepUp + case ipc.MethodSwapModel: + // Swapping the resident model changes what routes every utterance and + // what words every reply. It is the owner's call, from a surface that can + // carry a passkey gesture — the same rung as mutating the tool allowlist, + // and for the same reason: nothing Maven says or does may reach it. + // MethodModelStatus is only the read side, so it stays at AuthRead. + return AuthStepUp case ipc.MethodWriteFact: return AuthWrite case ipc.MethodAssertStepUp: @@ -79,7 +86,10 @@ func Requirement(m ipc.Method) Authority { // produce: candidate tasks and nothing else. It cannot write a fact, set a // reminder, or touch the tool allowlist, so a compromised mail reader can // at worst put junk on a review page he clears in one click. - ipc.MethodIngestMail: + ipc.MethodIngestMail, + // The read side of the model swap: which model is resident, which ones are + // allowlisted. It loads nothing and changes nothing. + ipc.MethodModelStatus: return AuthRead } // Unknown method ⇒ AuthRead, but ipc.dispatch returns ErrUnknownMethod diff --git a/internal/config/config.go b/internal/config/config.go index c3fe4b6..579dfcc 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -579,6 +579,21 @@ type PhraserConfig struct { // persona and invented units). Chat, query and reminder phrasing always go // through the model regardless. See phraser.Config.LLMNudges. LLMNudges bool `json:"llm_nudges,omitempty"` + + // SwapModels — the gguf files the running daemon is allowed to swap to + // without a restart (Vikunja #250). Empty (the default) means the swap + // capability does not exist: ipc.MethodSwapModel answers ErrUnknownMethod, + // exactly like an unconfigured weather or telegram block. + // + // It is an allowlist and not a directory on purpose. The request carries a + // path, and llama-server is started with it as `-m`; anything short of an + // exact match against a list a human wrote in this file would make "swap the + // model" mean "load a file of your choosing off my disk". ModelPath is + // always swappable back to whether or not it is listed. + // + // Paths must be absolute — the daemon's working directory is not the + // operator's, and a relative path here would resolve somewhere surprising. + SwapModels []string `json:"swap_models,omitempty"` } // EmbedderConfig — paths for the ONNX multilingual embedder. The daemon @@ -820,6 +835,14 @@ func (c *Config) validate() error { if c.Phraser.ModelPath == "" { return errors.New("phraser.model_path is required") } + // A relative entry in the swap allowlist would resolve against the + // daemon's working directory, so the path a human reads in this file + // would not be the path llama-server is handed. Fail at startup. + for _, m := range c.Phraser.SwapModels { + if !filepath.IsAbs(m) { + return fmt.Errorf("phraser.swap_models: %q must be an absolute path", m) + } + } } if c.Voice != nil && c.Voice.Enabled { if c.Voice.Bind == "" { diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 9f25ed1..4786e58 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -293,3 +293,35 @@ func TestPatternProposalNotifyDefaultsOff(t *testing.T) { t.Errorf("cooldown = %v, want 6h", c.PatternProposals.Cooldown) } } + +// TestSwapModelsAbsentMeansOff — the swap capability does not exist unless the +// operator lists the models he allows (Vikunja #250). +func TestSwapModelsAbsentMeansOff(t *testing.T) { + c, err := Load(writeConfig(t, `{"phraser": {"model_path": "/m/qwen.gguf"}}`)) + if err != nil { + t.Fatalf("Load: %v", err) + } + if len(c.Phraser.SwapModels) != 0 { + t.Errorf("swap_models = %v; want empty when unconfigured", c.Phraser.SwapModels) + } +} + +func TestSwapModelsParsedAndMustBeAbsolute(t *testing.T) { + c, err := Load(writeConfig(t, `{"phraser": { + "model_path": "/m/qwen.gguf", + "swap_models": ["/m/qwen.gguf", "/m/qwen-cpt.gguf"] + }}`)) + if err != nil { + t.Fatalf("Load: %v", err) + } + if len(c.Phraser.SwapModels) != 2 { + t.Fatalf("swap_models = %v; want 2 entries", c.Phraser.SwapModels) + } + // A relative entry would resolve against the daemon's cwd, not the operator's. + if _, err := Load(writeConfig(t, `{"phraser": { + "model_path": "/m/qwen.gguf", + "swap_models": ["models/llm/qwen.gguf"] + }}`)); err == nil { + t.Error("Load accepted a relative swap_models entry; want a startup failure") + } +} diff --git a/internal/ipc/api.go b/internal/ipc/api.go index 28d7284..a9a0be7 100644 --- a/internal/ipc/api.go +++ b/internal/ipc/api.go @@ -176,6 +176,50 @@ type IngestMailResp struct { Skipped bool `json:"skipped,omitempty"` } +// SwapModelReq — load another resident model without restarting the daemon +// (Vikunja #250). ModelPath must be one of the paths in phraser.swap_models; +// anything else is ErrForbidden, and an unconfigured allowlist makes the whole +// method ErrUnknownMethod. +// +// NGpuLayers and NCtx are zero for "keep what is loaded now", which is the +// normal case — the same laptop iGPU, a different gguf. +// +// This is an owner action. It is AuthStepUp in the authority table, it is not on +// CoreAPI, and no act, intent or timer can reach it: swapping the model is not +// something Maven does to herself. +type SwapModelReq struct { + ModelPath string `json:"model_path"` + NGpuLayers int `json:"n_gpu_layers,omitempty"` + NCtx int `json:"n_ctx,omitempty"` +} + +// SwapModelResp — what the daemon ended up serving. Model is the identity the +// new llama-server reported for itself, not an echo of the request: if the file +// was not the model the operator thought it was, this is where it shows. +// +// RolledBack is true when the requested model failed to load or would not answer +// and the previous one was put back. In that case the call also returns an error +// — the swap did not happen — and Model names the model still serving. +type SwapModelResp struct { + Model string `json:"model"` + ModelPath string `json:"model_path"` + BaseURL string `json:"base_url"` + RolledBack bool `json:"rolled_back,omitempty"` + TookMs int64 `json:"took_ms"` +} + +// ModelStatusResp — which model is resident and which ones may be swapped in. +// Read-only; the authed page renders it. Swappable is the configured allowlist, +// so an empty list means the capability is off. +type ModelStatusResp struct { + Model string `json:"model"` + ModelPath string `json:"model_path"` + BaseURL string `json:"base_url"` + NGpuLayers int `json:"n_gpu_layers"` + NCtx int `json:"n_ctx"` + Swappable []string `json:"swappable,omitempty"` +} + type listTasksReq struct { Status string `json:"status"` // "" all | "live" | candidate|open|done|dropped } diff --git a/internal/ipc/client.go b/internal/ipc/client.go index 3602bbb..898e1c5 100644 --- a/internal/ipc/client.go +++ b/internal/ipc/client.go @@ -459,6 +459,28 @@ func (c *Client) IngestMail(ctx context.Context, req IngestMailReq) (IngestMailR return r, nil } +// SwapModel asks core to load another resident model (Vikunja #250). +// ErrUnknownMethod means core has no phraser.swap_models allowlist configured; +// ErrForbidden means the path is not on it, or step-up was not asserted. A +// non-nil error with RolledBack set means nothing changed — the old model is +// still serving. +func (c *Client) SwapModel(ctx context.Context, req SwapModelReq) (SwapModelResp, error) { + var r SwapModelResp + if err := c.call(ctx, MethodSwapModel, req, &r); err != nil { + return SwapModelResp{}, err + } + return r, nil +} + +// ModelStatus reports the resident model and the swap allowlist. Read-only. +func (c *Client) ModelStatus(ctx context.Context) (ModelStatusResp, error) { + var r ModelStatusResp + if err := c.call(ctx, MethodModelStatus, nil, &r); err != nil { + return ModelStatusResp{}, err + } + return r, nil +} + func (c *Client) DismissProposedRoutine(ctx context.Context, id int64) error { return c.call(ctx, MethodDismissProposedRoutine, dismissProposedRoutineReq{ID: id}, nil) } diff --git a/internal/ipc/ipc_test.go b/internal/ipc/ipc_test.go index 86f6a9d..885d7bd 100644 --- a/internal/ipc/ipc_test.go +++ b/internal/ipc/ipc_test.go @@ -6,6 +6,7 @@ import ( "encoding/binary" "encoding/json" "errors" + "fmt" "io" "net" "os" @@ -598,3 +599,44 @@ func TestIngestMail_Hook(t *testing.T) { t.Errorf("req across the wire = %+v", got) } } + +// TestSwapModel_OffUnlessConfigured — no allowlist in the config means the +// daemon never sets the hook, and the method does not exist. That is what "off +// unless configured" looks like at the wire for the model swap (Vikunja #250). +func TestSwapModel_OffUnlessConfigured(t *testing.T) { + _, _, cli, _ := newServerWithStore(t) + if _, err := cli.SwapModel(context.Background(), SwapModelReq{ModelPath: "/m/x.gguf"}); !errors.Is(err, ErrUnknownMethod) { + t.Fatalf("SwapModel error = %v, want ErrUnknownMethod", err) + } + if _, err := cli.ModelStatus(context.Background()); !errors.Is(err, ErrUnknownMethod) { + t.Fatalf("ModelStatus error = %v, want ErrUnknownMethod", err) + } +} + +// TestSwapModel_Hook — the request crosses the boundary intact and the reported +// identity comes back. A refusal from the daemon's allowlist arrives as +// ErrForbidden, which is what a caller keys its error message off. +func TestSwapModel_Hook(t *testing.T) { + _, srv, cli, _ := newServerWithStore(t) + var got SwapModelReq + srv.SwapModelFn = func(_ context.Context, req SwapModelReq) (SwapModelResp, error) { + got = req + if req.ModelPath != "/m/allowed.gguf" { + return SwapModelResp{}, fmt.Errorf("%w: not allowlisted", ErrForbidden) + } + return SwapModelResp{Model: "allowed", ModelPath: req.ModelPath, BaseURL: "http://127.0.0.1:9", TookMs: 12}, nil + } + resp, err := cli.SwapModel(context.Background(), SwapModelReq{ModelPath: "/m/allowed.gguf", NCtx: 4096}) + if err != nil { + t.Fatalf("SwapModel: %v", err) + } + if resp.Model != "allowed" || resp.TookMs != 12 { + t.Errorf("resp = %+v", resp) + } + if got.NCtx != 4096 { + t.Errorf("req across the wire = %+v", got) + } + if _, err := cli.SwapModel(context.Background(), SwapModelReq{ModelPath: "/etc/shadow"}); !errors.Is(err, ErrForbidden) { + t.Fatalf("swap to a non-allowlisted path = %v; want ErrForbidden", err) + } +} diff --git a/internal/ipc/server.go b/internal/ipc/server.go index 1137153..de072fe 100644 --- a/internal/ipc/server.go +++ b/internal/ipc/server.go @@ -432,6 +432,19 @@ type Server struct { // every CoreAPI implementation has to carry. IngestMailFn IngestMailFunc + // SwapModelFn / ModelStatusFn — the on-the-fly resident model swap (Vikunja + // #250) and its read side. Set by the daemon only when phraser.swap_models + // lists at least one model AND the phraser owns a llama-server; nil ⇒ both + // methods answer ErrUnknownMethod, which is what "off unless configured" + // looks like at the wire. + // + // They bypass CoreAPI for the same reason IngestMailFn does: this is not a + // store operation, it needs the daemon's llama-server, and no other CoreAPI + // implementation should have to carry it. MethodSwapModel is AuthStepUp in + // internal/auth — owner-triggered, never an act and never a timer. + SwapModelFn SwapModelFunc + ModelStatusFn ModelStatusFunc + // UnlockFn — unwraps the store encryption key from the wrapped blob using // the passkey credential public key, opens the encrypted store, and wires // the rest of the daemon (voice, loop, delivery). Set by the daemon when @@ -450,6 +463,12 @@ type WrapKeyFunc func(ctx context.Context, publicKey []byte) error // public key and completes daemon initialization. type UnlockFunc func(ctx context.Context, publicKey []byte) error +// SwapModelFunc — loads another resident model in place of the live one. +type SwapModelFunc func(ctx context.Context, req SwapModelReq) (SwapModelResp, error) + +// ModelStatusFunc — reports the resident model and the swap allowlist. +type ModelStatusFunc func(ctx context.Context) (ModelStatusResp, error) + // IngestMailFunc — core-side mail extraction. Returns what was captured. type IngestMailFunc func(ctx context.Context, req IngestMailReq) (IngestMailResp, error) @@ -620,8 +639,9 @@ func withoutParams[R any](fn func(ctx context.Context, api CoreAPI) (R, error)) // existed) as an argument — so SetAPI's runtime swap (the unlock transition) // is still honored on the very next request with no extra plumbing here. // -// MethodAssertStepUp, MethodStoreEncryptionKey, MethodUnlock and -// MethodIngestMail are NOT in this table: they bypass CoreAPI entirely +// MethodAssertStepUp, MethodStoreEncryptionKey, MethodUnlock, +// MethodIngestMail, MethodSwapModel and MethodModelStatus are NOT in this +// table: they bypass CoreAPI entirely // (s.StepUp / s.WrapKeyFn / s.UnlockFn / s.IngestMailFn), so dispatch // special-cases them before consulting the table. var methodTable = map[Method]handlerFunc{ @@ -874,6 +894,30 @@ func (s *Server) dispatch(ctx context.Context, req Request) (json.RawMessage, er return marshalResult(resp), nil } return nil, fmt.Errorf("%w: %s", ErrUnknownMethod, req.Method) + + case MethodSwapModel: + if s.SwapModelFn != nil { + var p SwapModelReq + if err := unmarshalParams(req.Params, &p); err != nil { + return nil, err + } + resp, err := s.SwapModelFn(ctx, p) + if err != nil { + return nil, err + } + return marshalResult(resp), nil + } + return nil, fmt.Errorf("%w: %s", ErrUnknownMethod, req.Method) + + case MethodModelStatus: + if s.ModelStatusFn != nil { + resp, err := s.ModelStatusFn(ctx) + if err != nil { + return nil, err + } + return marshalResult(resp), nil + } + return nil, fmt.Errorf("%w: %s", ErrUnknownMethod, req.Method) } h, ok := methodTable[req.Method] diff --git a/internal/ipc/wire.go b/internal/ipc/wire.go index 3d1ea08..151eebf 100644 --- a/internal/ipc/wire.go +++ b/internal/ipc/wire.go @@ -51,6 +51,8 @@ const ( MethodListTasks Method = "list_tasks" MethodSetTaskStatus Method = "set_task_status" MethodIngestMail Method = "ingest_mail" + MethodSwapModel Method = "swap_model" + MethodModelStatus Method = "model_status" ) // Request — one frame from module to core. Params is the JSON-encoded argument diff --git a/internal/llm/client.go b/internal/llm/client.go index 7760296..a43748f 100644 --- a/internal/llm/client.go +++ b/internal/llm/client.go @@ -10,10 +10,17 @@ import ( "encoding/json" "fmt" "net/http" + "sync" "time" ) type Client struct { + // mu guards base only. The base URL changes when the daemon swaps the + // resident model (Vikunja #250): llama-server is relaunched on a fresh + // port, and every holder of this client — the LLM router, the replier, the + // mail extractor — must follow without being rebuilt. One mutexed field is + // the whole mechanism; a swap re-points the client, it does not replace it. + mu sync.RWMutex base string http *http.Client } @@ -22,6 +29,25 @@ func New(baseURL string, timeout time.Duration) *Client { return &Client{base: baseURL, http: &http.Client{Timeout: timeout}} } +// SetBaseURL re-points the client at another llama-server. Safe to call while +// requests are in flight: a request that already read the old base finishes +// against the old base (or fails, and every caller of Complete has a fallback), +// and the next one uses the new base. It is deliberately NOT a queue-and-retry — +// the phraser quiesces around a swap, so the window is small and a lost turn +// degrades to the classifier rather than hanging. +func (c *Client) SetBaseURL(base string) { + c.mu.Lock() + c.base = base + c.mu.Unlock() +} + +// BaseURL is the server this client currently talks to. +func (c *Client) BaseURL() string { + c.mu.RLock() + defer c.mu.RUnlock() + return c.base +} + type Req struct { System string User string @@ -63,7 +89,7 @@ func (c *Client) Complete(ctx context.Context, r Req) (string, error) { RepeatPenalty: r.RepeatPenalty, Stop: r.Stop, }) - req, err := http.NewRequestWithContext(ctx, "POST", c.base+"/v1/chat/completions", bytes.NewReader(b)) + req, err := http.NewRequestWithContext(ctx, "POST", c.BaseURL()+"/v1/chat/completions", bytes.NewReader(b)) if err != nil { return "", err } diff --git a/internal/llm/client_test.go b/internal/llm/client_test.go index 68f174d..9526c72 100644 --- a/internal/llm/client_test.go +++ b/internal/llm/client_test.go @@ -57,3 +57,37 @@ func TestComplete(t *testing.T) { t.Errorf("got %q, want %q", got, "ok") } } + +// TestSetBaseURL — a model swap re-points every holder of the client rather than +// rebuilding the router, the replier and the extractors (Vikunja #250). +func TestSetBaseURL(t *testing.T) { + var hit string + srvA := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + hit = "A" + w.Write([]byte(`{"choices":[{"message":{"content":"a"}}]}`)) + })) + defer srvA.Close() + srvB := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + hit = "B" + w.Write([]byte(`{"choices":[{"message":{"content":"b"}}]}`)) + })) + defer srvB.Close() + + c := New(srvA.URL, 5*time.Second) + if _, err := c.Complete(context.Background(), Req{User: "x"}); err != nil { + t.Fatalf("Complete against A: %v", err) + } + if hit != "A" { + t.Fatalf("first request went to %q; want A", hit) + } + c.SetBaseURL(srvB.URL) + if got := c.BaseURL(); got != srvB.URL { + t.Errorf("BaseURL = %q; want %q", got, srvB.URL) + } + if _, err := c.Complete(context.Background(), Req{User: "x"}); err != nil { + t.Fatalf("Complete against B: %v", err) + } + if hit != "B" { + t.Errorf("request after the swap went to %q; want B", hit) + } +} diff --git a/internal/phraser/llmphraser.go b/internal/phraser/llmphraser.go index 558bec2..4006009 100644 --- a/internal/phraser/llmphraser.go +++ b/internal/phraser/llmphraser.go @@ -27,14 +27,46 @@ var listenRE = regexp.MustCompile(`listening on (https?://\S+)`) type LLMPhraser struct { cfg Config client *http.Client - port string - cmd *exec.Cmd - cancel context.CancelFunc - wg sync.WaitGroup // tmpl — the hand-written Russian nudges. Default path for nudges; see // Config.LLMNudges. nil only if the template file failed to load. tmpl *NudgeTemplates + + // spawnCtx — the parent of every llama-server this phraser starts, i.e. the + // daemon's own context. Deliberately NOT the per-request context of the call + // that asked for a model swap: that one is cancelled the moment the request + // returns, which would kill the model it had just loaded. + spawnCtx context.Context + cancel context.CancelFunc + + // launch / probe — the two side effects of a swap, injectable so the swap + // logic is testable without a real llama-server and a real model file. + // launch is nil when this phraser does not own its server (NewLLMPhraserAt), + // which is also what makes Swap refuse there. + launch func(ctx context.Context, cfg Config) (backend, error) + probe func(ctx context.Context, base string) (string, error) + + // swapMu — single-flight around Swap. Held for the whole swap, including the + // model load, so two concurrent swap requests can never both be loading. + swapMu sync.Mutex + + // mu guards everything below: the live backend, the swap gate and the + // in-flight request count. See acquire/quiesce in swap.go. + mu sync.Mutex + be backend + live liveModel + swapping bool + inflight int + observers []func(baseURL string) +} + +// liveModel — what is actually loaded right now. Distinct from Config, which +// stays immutable after construction: a swap changes these three fields and +// nothing else, so no reader of cfg (prompts, grammar, timeouts) races a swap. +type liveModel struct { + ModelPath string + NGpuLayers int + NCtx int } type Config struct { @@ -85,15 +117,21 @@ func DefaultConfig(modelPath string) Config { func NewLLMPhraser(ctx context.Context, cfg Config) (*LLMPhraser, error) { ctx, cancel := context.WithCancel(ctx) p := &LLMPhraser{ - cfg: cfg, - client: &http.Client{Timeout: cfg.Timeout}, - cancel: cancel, - tmpl: loadNudgeTemplates(), + cfg: cfg, + client: &http.Client{Timeout: cfg.Timeout}, + tmpl: loadNudgeTemplates(), + spawnCtx: ctx, + cancel: cancel, + launch: spawnLlamaServer, + probe: defaultProbe, + live: liveModel{ModelPath: cfg.ModelPath, NGpuLayers: cfg.NGpuLayers, NCtx: cfg.NCtx}, } - if err := p.start(ctx); err != nil { + be, err := p.launch(ctx, cfg) + if err != nil { cancel() return nil, err } + p.be = be return p, nil } @@ -106,11 +144,17 @@ func NewLLMPhraser(ctx context.Context, cfg Config) (*LLMPhraser, error) { // still uses NewLLMPhraser and still owns its own child process. func NewLLMPhraserAt(baseURL string, cfg Config) *LLMPhraser { return &LLMPhraser{ - cfg: cfg, - client: &http.Client{Timeout: cfg.Timeout}, - port: strings.TrimSuffix(baseURL, "/"), - cancel: func() {}, - tmpl: loadNudgeTemplates(), + cfg: cfg, + client: &http.Client{Timeout: cfg.Timeout}, + tmpl: loadNudgeTemplates(), + spawnCtx: context.Background(), + cancel: func() {}, + probe: defaultProbe, + // launch stays nil: we did not start this server, so we must not stop it. + // Swap therefore refuses here (ErrSwapNotOwned) instead of killing a + // server another process depends on. + be: borrowedBackend(strings.TrimSuffix(baseURL, "/")), + live: liveModel{ModelPath: cfg.ModelPath, NGpuLayers: cfg.NGpuLayers, NCtx: cfg.NCtx}, } } @@ -126,16 +170,66 @@ func loadNudgeTemplates() *NudgeTemplates { return nt } -func (p *LLMPhraser) start(ctx context.Context) error { +// backend — one llama-server this phraser talks to. Two implementations: a +// llamaProc we spawned and must reap, and a borrowedBackend someone else owns. +type backend interface { + BaseURL() string + Close() error +} + +// borrowedBackend — a server started and owned by someone else (the phrasing +// scorer's shared llama-server). Closing it is a no-op by construction. +type borrowedBackend string + +func (b borrowedBackend) BaseURL() string { return string(b) } +func (b borrowedBackend) Close() error { return nil } + +// llamaProc — a llama-server child process plus the goroutine reading its +// stderr. Close kills and reaps it; see the Pdeathsig note in spawnLlamaServer. +type llamaProc struct { + base string + cmd *exec.Cmd + cancel context.CancelFunc + wg sync.WaitGroup +} + +func (l *llamaProc) BaseURL() string { return l.base } + +func (l *llamaProc) Close() error { + l.cancel() + if l.cmd != nil && l.cmd.Process != nil { + _ = l.cmd.Process.Kill() + _ = l.cmd.Wait() // reap the process — without Wait, the child becomes a zombie + } + l.wg.Wait() + return nil +} + +// spawnLlamaServer starts one llama-server for cfg and waits until it says which +// address it is listening on. ctx owns the process lifetime, so it must be the +// daemon's context, not a request's. +func spawnLlamaServer(ctx context.Context, cfg Config) (backend, error) { + ctx, cancel := context.WithCancel(ctx) + p, err := startLlamaProc(ctx, cfg) + if err != nil { + cancel() + return nil, err + } + p.cancel = cancel + return p, nil +} + +func startLlamaProc(ctx context.Context, cfg Config) (*llamaProc, error) { + p := &llamaProc{} args := []string{ - "-m", p.cfg.ModelPath, + "-m", cfg.ModelPath, "--host", "127.0.0.1", - "--port", extractPort(p.cfg.Listen), - "-c", fmt.Sprintf("%d", p.cfg.NCtx), - "-ngl", fmt.Sprintf("%d", p.cfg.NGpuLayers), + "--port", extractPort(cfg.Listen), + "-c", fmt.Sprintf("%d", cfg.NCtx), + "-ngl", fmt.Sprintf("%d", cfg.NGpuLayers), "--no-webui", } - cmd := exec.CommandContext(ctx, p.cfg.BinPath, args...) + cmd := exec.CommandContext(ctx, cfg.BinPath, args...) // Pdeathsig: the kernel SIGKILLs llama-server the moment mavend dies — by // ANY means, including SIGKILL/OOM/panic where our Close() never runs. Without // it a hard-killed mavend orphans its llama-server (reparented to init, keeps @@ -148,12 +242,12 @@ func (p *LLMPhraser) start(ctx context.Context) error { stderr, err := cmd.StderrPipe() if err != nil { - return fmt.Errorf("llm: stderr pipe: %w", err) + return nil, fmt.Errorf("llm: stderr pipe: %w", err) } if err := cmd.Start(); err != nil { stderr.Close() - return fmt.Errorf("llm: start: %w", err) + return nil, fmt.Errorf("llm: start: %w", err) } portCh := make(chan string, 1) @@ -186,32 +280,44 @@ func (p *LLMPhraser) start(ctx context.Context) error { select { case addr := <-portCh: - p.port = addr - return nil + p.base = addr + return p, nil case err := <-errCh: _ = cmd.Process.Kill() _ = cmd.Wait() - return fmt.Errorf("llm: server output: %w", err) + return nil, fmt.Errorf("llm: server output: %w", err) case <-ctx.Done(): _ = cmd.Process.Kill() _ = cmd.Wait() - return ctx.Err() + return nil, ctx.Err() case <-time.After(60 * time.Second): _ = cmd.Process.Kill() _ = cmd.Wait() - return fmt.Errorf("llm: server did not start within 60s") + return nil, fmt.Errorf("llm: server did not start within 60s") } } -func (p *LLMPhraser) BaseURL() string { return p.port } +// BaseURL is the llama-server this phraser talks to right now. It changes when +// the model is swapped, so callers that cache it must register an observer +// (OnSwap) rather than keeping the string forever. +func (p *LLMPhraser) BaseURL() string { + p.mu.Lock() + defer p.mu.Unlock() + if p.be == nil { + return "" + } + return p.be.BaseURL() +} func (p *LLMPhraser) Close() error { p.cancel() - if p.cmd != nil && p.cmd.Process != nil { - _ = p.cmd.Process.Kill() - _ = p.cmd.Wait() // reap the process — without Wait, the child becomes a zombie + p.mu.Lock() + be := p.be + p.be = nil + p.mu.Unlock() + if be != nil { + return be.Close() } - p.wg.Wait() return nil } @@ -350,6 +456,11 @@ func chatSystemPrompt(block func() string) string { // the LLM completion endpoint. Like chatWithSystem but for an arbitrary message // slice — the caller owns the system prompt placement. func (p *LLMPhraser) chatWithMessages(ctx context.Context, msgs []chatMsg, maxTokens int) (string, error) { + base, release, err := p.acquire() + if err != nil { + return "", err + } + defer release() req := chatReq{ Messages: msgs, Temperature: 0.7, @@ -360,7 +471,7 @@ func (p *LLMPhraser) chatWithMessages(ctx context.Context, msgs []chatMsg, maxTo if err != nil { return "", fmt.Errorf("llm: marshal: %w", err) } - httpReq, err := http.NewRequestWithContext(ctx, "POST", p.port+"/v1/chat/completions", bytes.NewReader(body)) + httpReq, err := http.NewRequestWithContext(ctx, "POST", base+"/v1/chat/completions", bytes.NewReader(body)) if err != nil { return "", fmt.Errorf("llm: request: %w", err) } @@ -493,6 +604,11 @@ func (p *LLMPhraser) chat(ctx context.Context, userPrompt string) (string, error } func (p *LLMPhraser) chatWithSystem(ctx context.Context, system, user string, maxTokens int) (string, error) { + base, release, err := p.acquire() + if err != nil { + return "", err + } + defer release() req := chatReq{ Messages: []chatMsg{ {Role: "system", Content: system}, @@ -507,7 +623,7 @@ func (p *LLMPhraser) chatWithSystem(ctx context.Context, system, user string, ma return "", fmt.Errorf("llm: marshal: %w", err) } - httpReq, err := http.NewRequestWithContext(ctx, "POST", p.port+"/v1/chat/completions", bytes.NewReader(body)) + httpReq, err := http.NewRequestWithContext(ctx, "POST", base+"/v1/chat/completions", bytes.NewReader(body)) if err != nil { return "", fmt.Errorf("llm: request: %w", err) } diff --git a/internal/phraser/swap.go b/internal/phraser/swap.go new file mode 100644 index 0000000..22d9aeb --- /dev/null +++ b/internal/phraser/swap.go @@ -0,0 +1,330 @@ +package phraser + +import ( + "context" + "errors" + "fmt" + "log" + "time" + + "github.com/kami/maven/internal/llm" +) + +// Swapping the resident model without restarting the daemon (Vikunja #250). +// +// Three properties this file exists to hold, in order of importance: +// +// 1. NEVER two models resident at once. The deploy target is a laptop iGPU +// with the whole 1.7B offloaded to it (`n_gpu_layers: 99`); loading a second +// model beside the first is how you OOM the box, and a blue/green swap that +// "keeps the old one warm until the new one answers" does exactly that. So +// the old server is killed FIRST and the new one loaded after. The cost of +// that ordering is a window with no model at all, which is why: +// +// 2. A swap is atomic from a turn's point of view. An in-flight turn finishes +// on the old model — Swap waits for the last one to return before killing +// anything. A turn that arrives during the swap is REFUSED immediately with +// ErrSwapping rather than blocked: every phrasing path already has a +// fallback (templates, "вот что я нашла", the classifier for routing), so a +// fast refusal degrades one turn instead of hanging it for the length of a +// model load. No turn ever gets half of one model and half of another. +// +// 3. A failed load rolls back to the model that was working. The new server is +// probed (it must say which model it loaded) before it is published; if the +// launch or the probe fails, the previous config is relaunched and the +// phraser goes back to serving. Only if the rollback ALSO fails is the +// phraser left without a backend, and then it says so loudly and every turn +// degrades rather than breaks. +// +// Not here, deliberately: nothing calls Swap on a timer, and no act or intent can +// reach it. It is an IPC method behind the step-up gate, i.e. owner-triggered. + +var ( + // ErrSwapping — a turn arrived while the model was being swapped. Callers + // treat it like any other LLM error and use their fallback. + ErrSwapping = errors.New("phraser: model swap in progress") + + // ErrSwapNotOwned — this phraser did not start its llama-server, so it must + // not stop one (NewLLMPhraserAt: the eval harness shares a server). + ErrSwapNotOwned = errors.New("phraser: llama-server is not ours to swap") + + // ErrNoBackend — no model is loaded at all. Only reachable after a failed + // swap whose rollback also failed. + ErrNoBackend = errors.New("phraser: no llama-server loaded") + + // ErrSwapBusy — a turn was still running when the drain deadline expired, so + // the swap was abandoned. Nothing was killed; ask again. + ErrSwapBusy = errors.New("phraser: turns still in flight, swap abandoned") +) + +// SwapSpec — what to load. Zero NGpuLayers/NCtx keep whatever is live, so the +// common case ("same settings, different gguf") is one field. +type SwapSpec struct { + ModelPath string + NGpuLayers int + NCtx int +} + +// SwapResult — what happened. Model is the identity the NEW server reported, so +// it is evidence rather than an echo of the request: if the file at ModelPath is +// not what the operator thought it was, this is where that shows up. +type SwapResult struct { + Model string + BaseURL string + ModelPath string + RolledBack bool + Took time.Duration +} + +// drainTimeout — how long Swap waits for in-flight turns before giving up. A +// turn is at most Config.Timeout (30s in deploy) plus the model's own latency; +// 90s covers a slow Thinking generation without wedging the caller forever. +const drainTimeout = 90 * time.Second + +// probeTimeout — how long the new server gets to answer "which model do you +// have". The load itself is bounded by spawnLlamaServer's own 60s wait. +const probeTimeout = 30 * time.Second + +// defaultProbe asks the server which model it has loaded. This is the health +// check: a server that answers /v1/models has finished loading weights and is +// serving, and its answer is the identity we report back. +func defaultProbe(ctx context.Context, base string) (string, error) { + return llm.ModelID(ctx, base) +} + +// OnSwap registers a callback fired with the new base URL every time the live +// backend changes, including after a rollback. Holders of an *llm.Client (the +// LLM router, the replier, the mail extractor) register SetBaseURL here so a +// swap re-points them without rebuilding the router or the handler. +// +// Callbacks run with no lock held, in registration order. +func (p *LLMPhraser) OnSwap(fn func(baseURL string)) { + if fn == nil { + return + } + p.mu.Lock() + p.observers = append(p.observers, fn) + p.mu.Unlock() +} + +// LiveModel is the model file currently loaded (and its load settings). Empty +// ModelPath means no model is loaded. +func (p *LLMPhraser) LiveModel() (path string, nGpuLayers, nCtx int) { + p.mu.Lock() + defer p.mu.Unlock() + return p.live.ModelPath, p.live.NGpuLayers, p.live.NCtx +} + +// acquire reserves a slot for one request and returns the base URL to use. +// Every request path must call it and must call the returned release exactly +// once — that count is what Swap drains. +func (p *LLMPhraser) acquire() (string, func(), error) { + p.mu.Lock() + defer p.mu.Unlock() + if p.swapping { + return "", nil, ErrSwapping + } + if p.be == nil { + return "", nil, ErrNoBackend + } + p.inflight++ + base := p.be.BaseURL() + var once bool + return base, func() { + p.mu.Lock() + if !once { + once = true + p.inflight-- + } + p.mu.Unlock() + }, nil +} + +// Swap loads another model in place of the live one. See the file comment for +// the properties it guarantees. Returns the new model's reported identity, or +// an error plus RolledBack=true when the old model was put back. +// +// ctx bounds the drain and the probe. It does NOT own the new server's lifetime +// — that is the daemon's context, captured at construction — so a swap survives +// the request that asked for it. +func (p *LLMPhraser) Swap(ctx context.Context, spec SwapSpec) (SwapResult, error) { + if spec.ModelPath == "" { + return SwapResult{}, fmt.Errorf("phraser: swap needs a model path") + } + p.swapMu.Lock() + defer p.swapMu.Unlock() + + if p.launch == nil { + return SwapResult{}, ErrSwapNotOwned + } + + started := time.Now() + oldLive := p.liveSnapshot() + newLive := liveModel{ + ModelPath: spec.ModelPath, + NGpuLayers: pickInt(spec.NGpuLayers, oldLive.NGpuLayers), + NCtx: pickInt(spec.NCtx, oldLive.NCtx), + } + if newLive == oldLive && p.BaseURL() != "" { + // Already serving exactly this. Report the live identity rather than + // pointlessly unloading and reloading the same weights. + base := p.BaseURL() + id, err := p.probeWith(ctx, base) + if err != nil { + return SwapResult{}, err + } + return SwapResult{Model: id, BaseURL: base, ModelPath: oldLive.ModelPath, Took: time.Since(started)}, nil + } + + if err := p.quiesce(ctx); err != nil { + return SwapResult{}, err + } + defer p.resume() + + // Property 1: the old model leaves the GPU before the new one arrives. + p.mu.Lock() + old := p.be + p.be = nil + p.mu.Unlock() + if old != nil { + _ = old.Close() + } + + be, err := p.loadAndProbe(ctx, newLive) + if err != nil { + log.Printf("phraser: swap to %s FAILED (%v) — rolling back to %s", newLive.ModelPath, err, oldLive.ModelPath) + rb, rbErr := p.loadAndProbe(ctx, oldLive) + if rbErr != nil { + log.Printf("phraser: ROLLBACK to %s ALSO FAILED (%v) — no model is loaded, every phrasing path is on its fallback and routing is on the classifier until the daemon is restarted", oldLive.ModelPath, rbErr) + return SwapResult{RolledBack: true, Took: time.Since(started)}, + fmt.Errorf("phraser: swap failed (%w) and rollback failed too: %v", err, rbErr) + } + p.publish(rb, oldLive) + return SwapResult{ + Model: rb.id, BaseURL: rb.be.BaseURL(), ModelPath: oldLive.ModelPath, + RolledBack: true, Took: time.Since(started), + }, + fmt.Errorf("phraser: swap to %s failed, rolled back to %s: %w", newLive.ModelPath, oldLive.ModelPath, err) + } + p.publish(be, newLive) + log.Printf("phraser: model swapped to %s (%s) at %s in %s", newLive.ModelPath, be.id, be.be.BaseURL(), time.Since(started).Round(time.Millisecond)) + return SwapResult{ + Model: be.id, BaseURL: be.be.BaseURL(), ModelPath: newLive.ModelPath, + Took: time.Since(started), + }, nil +} + +// loaded — a started server plus the identity it reported. +type loaded struct { + be backend + id string +} + +// loadAndProbe starts a server for lm and verifies it answers. A server that +// starts but will not say what it loaded is treated as a failed load and is +// killed here — publishing it would hand every turn to a backend we could not +// confirm. +func (p *LLMPhraser) loadAndProbe(ctx context.Context, lm liveModel) (loaded, error) { + cfg := p.cfg + cfg.ModelPath = lm.ModelPath + cfg.NGpuLayers = lm.NGpuLayers + cfg.NCtx = lm.NCtx + // p.spawnCtx, not ctx: the process must outlive the request asking for it. + be, err := p.launch(p.spawnCtx, cfg) + if err != nil { + return loaded{}, err + } + id, err := p.probeWith(ctx, be.BaseURL()) + if err != nil { + _ = be.Close() + return loaded{}, fmt.Errorf("phraser: %s started but would not answer: %w", lm.ModelPath, err) + } + return loaded{be: be, id: id}, nil +} + +func (p *LLMPhraser) probeWith(ctx context.Context, base string) (string, error) { + probe := p.probe + if probe == nil { + probe = defaultProbe + } + pctx, cancel := context.WithTimeout(ctx, probeTimeout) + defer cancel() + return probe(pctx, base) +} + +// quiesce closes the door on new turns and waits for the ones already running. +// Polling rather than a sync.Cond: the wait happens once per swap, a 25ms poll +// is invisible next to a model load, and a poll cannot deadlock on a release +// path that panicked. +func (p *LLMPhraser) quiesce(ctx context.Context) error { + p.mu.Lock() + if p.swapping { + p.mu.Unlock() + return ErrSwapping + } + p.swapping = true + inflight := p.inflight + p.mu.Unlock() + if inflight == 0 { + return nil + } + + deadline := time.Now().Add(drainTimeout) + for { + select { + case <-ctx.Done(): + p.resume() + return ctx.Err() + case <-time.After(25 * time.Millisecond): + } + p.mu.Lock() + inflight = p.inflight + p.mu.Unlock() + if inflight == 0 { + return nil + } + if time.Now().After(deadline) { + // Nothing has been killed yet, so abandoning is free: reopen the door + // and let the operator try again rather than cutting a live turn off + // mid-generation. + p.resume() + return fmt.Errorf("%w (%d still running after %s)", ErrSwapBusy, inflight, drainTimeout) + } + } +} + +func (p *LLMPhraser) resume() { + p.mu.Lock() + p.swapping = false + p.mu.Unlock() +} + +// publish makes l the live backend and tells everyone holding a base URL. +func (p *LLMPhraser) publish(l loaded, lm liveModel) { + p.mu.Lock() + p.be = l.be + p.live = lm + obs := make([]func(string), len(p.observers)) + copy(obs, p.observers) + p.mu.Unlock() + base := l.be.BaseURL() + for _, fn := range obs { + fn(base) + } +} + +func (p *LLMPhraser) liveSnapshot() liveModel { + p.mu.Lock() + defer p.mu.Unlock() + return p.live +} + +// pickInt returns v when the caller set it, and fallback otherwise. 0 is the +// "unset" value: -1 already means "offload every layer" and deploy uses 99, so +// nothing legitimate asks for exactly zero GPU layers through this path. +func pickInt(v, fallback int) int { + if v == 0 { + return fallback + } + return v +} diff --git a/internal/phraser/swap_test.go b/internal/phraser/swap_test.go new file mode 100644 index 0000000..aea6f39 --- /dev/null +++ b/internal/phraser/swap_test.go @@ -0,0 +1,311 @@ +package phraser + +import ( + "context" + "errors" + "net/http" + "net/http/httptest" + "sync" + "sync/atomic" + "testing" + "time" +) + +// fakeModel — a stand-in llama-server. It answers /v1/models with its own name +// and /v1/chat/completions with a phrasing-contract reply that names itself, so +// a test can tell WHICH model answered a turn — the property the swap is about. +type fakeModel struct { + srv *httptest.Server + name string + closed atomic.Bool +} + +func newFakeModel(t *testing.T, name string) *fakeModel { + t.Helper() + f := &fakeModel{name: name} + mux := http.NewServeMux() + mux.HandleFunc("/v1/models", func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte(`{"data":[{"id":"/models/` + name + `.gguf"}]}`)) + }) + mux.HandleFunc("/v1/chat/completions", func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte(`{"choices":[{"message":{"content":"{\"response\":\"` + name + `\",\"mood\":\"neutral\"}"}}]}`)) + }) + f.srv = httptest.NewServer(mux) + t.Cleanup(f.srv.Close) + return f +} + +func (f *fakeModel) BaseURL() string { return f.srv.URL } +func (f *fakeModel) Close() error { f.closed.Store(true); return nil } + +// fakeFleet is the injected launcher: it hands out a prepared fakeModel per +// model path, and refuses paths the test did not prepare (that is what a bad +// gguf looks like from here). It also asserts the invariant that matters on a +// laptop iGPU: never two servers alive at the same time. +type fakeFleet struct { + mu sync.Mutex + models map[string]string // model path → fake name + live int + maxLive int + launch int +} + +func (fl *fakeFleet) launcher(t *testing.T) func(context.Context, Config) (backend, error) { + return func(ctx context.Context, cfg Config) (backend, error) { + fl.mu.Lock() + name, ok := fl.models[cfg.ModelPath] + fl.launch++ + if !ok { + fl.mu.Unlock() + return nil, errors.New("no such model file: " + cfg.ModelPath) + } + fl.live++ + if fl.live > fl.maxLive { + fl.maxLive = fl.live + } + fl.mu.Unlock() + f := newFakeModel(t, name) + return &fleetBackend{fleet: fl, model: f}, nil + } +} + +type fleetBackend struct { + fleet *fakeFleet + model *fakeModel + once sync.Once +} + +func (b *fleetBackend) BaseURL() string { return b.model.BaseURL() } +func (b *fleetBackend) Close() error { + b.once.Do(func() { + b.fleet.mu.Lock() + b.fleet.live-- + b.fleet.mu.Unlock() + }) + return b.model.Close() +} + +// newSwapPhraser builds an LLMPhraser with an injected launcher, so the swap +// path is exercised without a gguf or a GPU. +func newSwapPhraser(t *testing.T, fl *fakeFleet, modelPath string) *LLMPhraser { + t.Helper() + cfg := DefaultConfig(modelPath) + cfg.Timeout = 5 * time.Second + p := &LLMPhraser{ + cfg: cfg, + client: &http.Client{Timeout: cfg.Timeout}, + spawnCtx: context.Background(), + cancel: func() {}, + launch: fl.launcher(t), + probe: defaultProbe, + live: liveModel{ModelPath: modelPath, NGpuLayers: cfg.NGpuLayers, NCtx: cfg.NCtx}, + } + be, err := p.launch(p.spawnCtx, cfg) + if err != nil { + t.Fatalf("initial launch: %v", err) + } + p.be = be + t.Cleanup(func() { p.Close() }) + return p +} + +func TestSwap_LoadsNewModelAndRepointsHolders(t *testing.T) { + fl := &fakeFleet{models: map[string]string{"/m/old.gguf": "old", "/m/new.gguf": "new"}} + p := newSwapPhraser(t, fl, "/m/old.gguf") + + // A holder of the base URL (the LLM router's client, in the daemon). + var seen []string + p.OnSwap(func(base string) { seen = append(seen, base) }) + + before, err := p.PhraseChat(context.Background(), "привет", nil) + if err != nil || before != "old" { + t.Fatalf("before swap: %q, %v; want the old model to answer", before, err) + } + + res, err := p.Swap(context.Background(), SwapSpec{ModelPath: "/m/new.gguf"}) + if err != nil { + t.Fatalf("Swap: %v", err) + } + if res.Model != "new" { + t.Errorf("res.Model = %q; want the identity the NEW server reported (%q)", res.Model, "new") + } + if res.RolledBack { + t.Errorf("res.RolledBack = true on a successful swap") + } + after, err := p.PhraseChat(context.Background(), "привет", nil) + if err != nil || after != "new" { + t.Fatalf("after swap: %q, %v; want the new model to answer", after, err) + } + if path, _, _ := p.LiveModel(); path != "/m/new.gguf" { + t.Errorf("LiveModel = %q; want /m/new.gguf", path) + } + if len(seen) != 1 || seen[0] != p.BaseURL() { + t.Errorf("observers saw %v; want exactly one call with the new base %q", seen, p.BaseURL()) + } + if fl.maxLive > 1 { + t.Errorf("%d servers were alive at once; the iGPU only fits one model", fl.maxLive) + } +} + +func TestSwap_FailedLoadRollsBackToTheWorkingModel(t *testing.T) { + fl := &fakeFleet{models: map[string]string{"/m/old.gguf": "old"}} + p := newSwapPhraser(t, fl, "/m/old.gguf") + + res, err := p.Swap(context.Background(), SwapSpec{ModelPath: "/m/broken.gguf"}) + if err == nil { + t.Fatal("Swap to a model that will not load returned nil error") + } + if !res.RolledBack { + t.Errorf("res.RolledBack = false; a failed swap must say it rolled back") + } + if res.Model != "old" { + t.Errorf("res.Model = %q; want the old model back", res.Model) + } + // The point of the rollback: turns keep working. + got, err := p.PhraseChat(context.Background(), "привет", nil) + if err != nil || got != "old" { + t.Fatalf("after rollback: %q, %v; want the old model serving again", got, err) + } + if path, _, _ := p.LiveModel(); path != "/m/old.gguf" { + t.Errorf("LiveModel = %q; want the old model", path) + } + if fl.maxLive > 1 { + t.Errorf("%d servers alive at once during a rollback", fl.maxLive) + } +} + +func TestSwap_ProbeFailureIsTreatedAsAFailedLoad(t *testing.T) { + // A server that starts but will not say what it loaded must never be + // published — we would be serving turns from a backend we cannot confirm. + fl := &fakeFleet{models: map[string]string{"/m/old.gguf": "old", "/m/mute.gguf": "mute"}} + p := newSwapPhraser(t, fl, "/m/old.gguf") + // Fail the probe once — for the newly launched server — and let the + // rollback's probe through. + calls := 0 + p.probe = func(ctx context.Context, base string) (string, error) { + calls++ + if calls == 1 { + return "", errors.New("no answer from the new server") + } + return defaultProbe(ctx, base) + } + + _, err := p.Swap(context.Background(), SwapSpec{ModelPath: "/m/mute.gguf"}) + if err == nil { + t.Fatal("Swap published a server that failed its probe") + } + if path, _, _ := p.LiveModel(); path != "/m/old.gguf" { + t.Errorf("LiveModel = %q; want the old model after a failed probe", path) + } +} + +func TestSwap_RollbackFailureLeavesNoBackendAndDegrades(t *testing.T) { + fl := &fakeFleet{models: map[string]string{"/m/old.gguf": "old"}} + p := newSwapPhraser(t, fl, "/m/old.gguf") + // Make the rollback fail too: the old file "disappears" mid-swap. + fl.mu.Lock() + delete(fl.models, "/m/old.gguf") + fl.mu.Unlock() + + _, err := p.Swap(context.Background(), SwapSpec{ModelPath: "/m/broken.gguf"}) + if err == nil { + t.Fatal("Swap returned nil when both the load and the rollback failed") + } + // Nothing is loaded, and the request path says so rather than panicking. + if _, _, aerr := p.acquire(); !errors.Is(aerr, ErrNoBackend) { + t.Errorf("acquire error = %v; want ErrNoBackend", aerr) + } + // Phrasing degrades to its fallback instead of failing the turn. + got, err := p.PhraseChat(context.Background(), "привет", nil) + if err != nil { + t.Fatalf("PhraseChat after a total failure returned an error: %v", err) + } + if got == "" { + t.Error("PhraseChat returned empty; the fallback must still say something") + } +} + +func TestSwap_WaitsForInFlightTurnAndRefusesNewOnes(t *testing.T) { + fl := &fakeFleet{models: map[string]string{"/m/old.gguf": "old", "/m/new.gguf": "new"}} + p := newSwapPhraser(t, fl, "/m/old.gguf") + + // Hold one turn open by taking a slot directly — the same slot every + // request path takes. + base, release, err := p.acquire() + if err != nil { + t.Fatalf("acquire: %v", err) + } + if base == "" { + t.Fatal("acquire returned an empty base URL") + } + + swapped := make(chan error, 1) + go func() { _, e := p.Swap(context.Background(), SwapSpec{ModelPath: "/m/new.gguf"}); swapped <- e }() + + // While the swap waits to drain, a NEW turn is refused immediately rather + // than blocked for the length of a model load. + deadline := time.Now().Add(2 * time.Second) + for { + _, rel, aerr := p.acquire() + if rel != nil { + rel() + } + if errors.Is(aerr, ErrSwapping) { + break + } + if time.Now().After(deadline) { + t.Fatalf("new turns were never refused during a swap (last error: %v)", aerr) + } + time.Sleep(10 * time.Millisecond) + } + + // The swap cannot have completed while our turn was still in flight. + select { + case e := <-swapped: + t.Fatalf("Swap finished before the in-flight turn released: %v", e) + case <-time.After(50 * time.Millisecond): + } + + release() + if e := <-swapped; e != nil { + t.Fatalf("Swap after drain: %v", e) + } + got, err := p.PhraseChat(context.Background(), "привет", nil) + if err != nil || got != "new" { + t.Fatalf("after swap: %q, %v; want the new model", got, err) + } +} + +func TestSwap_RefusedWhenWeDoNotOwnTheServer(t *testing.T) { + // NewLLMPhraserAt points at a shared server the eval harness owns. Swapping + // there would kill a server another process depends on. + p := NewLLMPhraserAt("http://127.0.0.1:1/", DefaultConfig("/m/old.gguf")) + if _, err := p.Swap(context.Background(), SwapSpec{ModelPath: "/m/new.gguf"}); !errors.Is(err, ErrSwapNotOwned) { + t.Fatalf("Swap on a borrowed server = %v; want ErrSwapNotOwned", err) + } +} + +func TestSwap_SameModelIsANoOp(t *testing.T) { + fl := &fakeFleet{models: map[string]string{"/m/old.gguf": "old"}} + p := newSwapPhraser(t, fl, "/m/old.gguf") + launchesBefore := fl.launch + + res, err := p.Swap(context.Background(), SwapSpec{ModelPath: "/m/old.gguf"}) + if err != nil { + t.Fatalf("Swap to the live model: %v", err) + } + if res.Model != "old" { + t.Errorf("res.Model = %q; want old", res.Model) + } + if fl.launch != launchesBefore { + t.Errorf("%d extra launches; swapping to the live model must not reload weights", fl.launch-launchesBefore) + } +} + +func TestSwap_EmptyModelPathRefused(t *testing.T) { + fl := &fakeFleet{models: map[string]string{"/m/old.gguf": "old"}} + p := newSwapPhraser(t, fl, "/m/old.gguf") + if _, err := p.Swap(context.Background(), SwapSpec{}); err == nil { + t.Fatal("Swap with no model path returned nil error") + } +}