phraser: gate every llm.Client call on the swap drain
The drain counted only the phrasing paths in internal/phraser. The router, the replier, the mail extractor and the memory evaluator reach llama-server through llm.Client, so quiesce could report zero requests in flight while the router was mid-generation, and the old server was killed under it. The turn then finished on the new model, which is the split turn the swap exists to prevent. llm.Client now enters an optional Gate before every completion and LLMPhraser implements it, so one counter covers every holder of the base URL. A total failure also reported itself as a rollback. Swap set RolledBack on the path where the rollback failed too, so the page rendered "rolled back to — she is still answering, with the old model" over an empty model name and a daemon with no model at all. The total failure has its own flag now, LiveModel stops naming a gguf that is not loaded, and the log says another attempt can recover without a restart, which is true. The swap also ran on the connection every other page shares. ipc.Client holds its mutex for a whole roundtrip with no read deadline on either side, so a load froze /dash, /history and /notifications for minutes. mavweb dials a second connection for /models alone. POST /models joins the route table, and the load settings no longer come off a form that renders no input for them. Found in review of #68.
This commit is contained in:
kami
@@ -370,11 +370,17 @@ type SwapModelReq struct {
|
||||
// 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.
|
||||
//
|
||||
// NoBackend is the other failure and it is not a milder one: the rollback failed
|
||||
// too, no model is loaded, and every phrasing path is on its template fallback
|
||||
// with routing on the classifier. It is a separate field from RolledBack because
|
||||
// the two need opposite words on the page.
|
||||
type SwapModelResp struct {
|
||||
Model string `json:"model"`
|
||||
ModelPath string `json:"model_path"`
|
||||
BaseURL string `json:"base_url"`
|
||||
RolledBack bool `json:"rolled_back,omitempty"`
|
||||
NoBackend bool `json:"no_backend,omitempty"`
|
||||
TookMs int64 `json:"took_ms"`
|
||||
}
|
||||
|
||||
|
||||
+50
-5
@@ -14,17 +14,57 @@ import (
|
||||
"time"
|
||||
)
|
||||
|
||||
// Gate — admission control for a completion. Enter blocks or refuses while the
|
||||
// resident model is being swapped, and the returned release says the request is
|
||||
// done. The phraser implements it: a swap kills the running llama-server, so
|
||||
// every holder of a base URL has to be counted before the kill, not just the
|
||||
// phrasing paths.
|
||||
//
|
||||
// Without this the drain saw only the phraser's own calls. The LLM router, the
|
||||
// replier, the mail extractor and the memory evaluator all reach llama-server
|
||||
// through this client, so a swap could report zero requests in flight and kill
|
||||
// the server out from under a routing decision. The turn then finished on the
|
||||
// new model, which is the "half of one model and half of another" the swap is
|
||||
// supposed to make impossible.
|
||||
type Gate interface {
|
||||
Enter() (release func(), err error)
|
||||
}
|
||||
|
||||
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 guards base and gate. The base URL can change when the daemon swaps the
|
||||
// resident model (Vikunja #250) and every holder of this client — the LLM
|
||||
// router, the replier, the mail extractor — must follow without being
|
||||
// rebuilt. A swap re-points the client, it does not replace it.
|
||||
//
|
||||
// On the deploy shape the new server binds the same fixed port the killed
|
||||
// one released (startLlamaProc passes the port out of phraser.listen), so
|
||||
// SetBaseURL is normally a no-op and the gate is the part doing the work.
|
||||
// The re-pointing stays because nothing guarantees the port: a phraser
|
||||
// listening on :0, or a future swap that moves the server, changes the base.
|
||||
mu sync.RWMutex
|
||||
base string
|
||||
gate Gate
|
||||
http *http.Client
|
||||
}
|
||||
|
||||
// SetGate installs the admission gate. Nil (the default, and what the eval
|
||||
// harness and the tests use) means no gating.
|
||||
func (c *Client) SetGate(g Gate) {
|
||||
c.mu.Lock()
|
||||
c.gate = g
|
||||
c.mu.Unlock()
|
||||
}
|
||||
|
||||
func (c *Client) enter() (func(), error) {
|
||||
c.mu.RLock()
|
||||
g := c.gate
|
||||
c.mu.RUnlock()
|
||||
if g == nil {
|
||||
return func() {}, nil
|
||||
}
|
||||
return g.Enter()
|
||||
}
|
||||
|
||||
func New(baseURL string, timeout time.Duration) *Client {
|
||||
return &Client{base: baseURL, http: &http.Client{Timeout: timeout}}
|
||||
}
|
||||
@@ -81,6 +121,11 @@ type resp struct {
|
||||
}
|
||||
|
||||
func (c *Client) Complete(ctx context.Context, r Req) (string, error) {
|
||||
release, err := c.enter()
|
||||
if err != nil {
|
||||
return "", err
|
||||
}
|
||||
defer release()
|
||||
b, _ := json.Marshal(body{
|
||||
Messages: []msg{{Role: "system", Content: r.System}, {Role: "user", Content: r.User}},
|
||||
MaxTokens: r.MaxTokens,
|
||||
|
||||
@@ -28,6 +28,10 @@ import (
|
||||
// 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.
|
||||
// "Every path" means every path: the router, the replier, the mail
|
||||
// extractor and the memory evaluator do not call acquire, they call
|
||||
// llm.Client.Complete, so LLMPhraser implements llm.Gate and the client
|
||||
// enters through the same counter.
|
||||
//
|
||||
// 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
|
||||
@@ -68,11 +72,16 @@ type SwapSpec struct {
|
||||
// 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.
|
||||
// RolledBack is true only when a model is serving again. NoBackend is the other
|
||||
// failure, and it is the worse one: the rollback failed too and nothing is
|
||||
// loaded. They are separate flags because the operator surface reads them, and
|
||||
// "rolled back" spelled over a dead daemon reads as reassurance.
|
||||
type SwapResult struct {
|
||||
Model string
|
||||
BaseURL string
|
||||
ModelPath string
|
||||
RolledBack bool
|
||||
NoBackend bool
|
||||
Took time.Duration
|
||||
}
|
||||
|
||||
@@ -140,6 +149,19 @@ func (p *LLMPhraser) acquire() (string, func(), error) {
|
||||
}, nil
|
||||
}
|
||||
|
||||
// Enter implements llm.Gate so every holder of an *llm.Client is drained by a
|
||||
// swap, not only the phrasing paths in this package.
|
||||
//
|
||||
// The router, the replier, the mail extractor and the memory evaluator do not
|
||||
// call acquire; they call llm.Client.Complete. Before this existed quiesce could
|
||||
// see zero requests in flight while the router was mid-generation and kill the
|
||||
// server under it. A refusal here is the same ErrSwapping the phrasing paths
|
||||
// get, and every caller of Complete already falls back.
|
||||
func (p *LLMPhraser) Enter() (func(), error) {
|
||||
_, release, err := p.acquire()
|
||||
return release, err
|
||||
}
|
||||
|
||||
// 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.
|
||||
@@ -195,8 +217,13 @@ func (p *LLMPhraser) Swap(ctx context.Context, spec SwapSpec) (SwapResult, error
|
||||
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)},
|
||||
// Nothing is loaded, so LiveModel must stop naming a gguf: the page
|
||||
// would show a file next to an unknown model and read as half-working.
|
||||
p.mu.Lock()
|
||||
p.live = liveModel{}
|
||||
p.mu.Unlock()
|
||||
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. Swap is still wired, so another attempt can recover without restarting the daemon", oldLive.ModelPath, rbErr)
|
||||
return SwapResult{NoBackend: true, Took: time.Since(started)},
|
||||
fmt.Errorf("phraser: swap failed (%w) and rollback failed too: %v", err, rbErr)
|
||||
}
|
||||
p.publish(rb, oldLive)
|
||||
|
||||
@@ -0,0 +1,127 @@
|
||||
package phraser
|
||||
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"github.com/kami/maven/internal/llm"
|
||||
)
|
||||
|
||||
// The drain has to cover every holder of the base URL, not only the phrasing
|
||||
// paths in this package. The LLM router, the replier, the mail extractor and the
|
||||
// memory evaluator all reach llama-server through llm.Client, and a swap that
|
||||
// does not count them kills the server mid-turn.
|
||||
|
||||
// blockingLLM — a completion endpoint that does not answer until the test says
|
||||
// so. It stands in for a router call that is generating when the swap arrives.
|
||||
func blockingLLM(t *testing.T, release <-chan struct{}) *httptest.Server {
|
||||
t.Helper()
|
||||
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
<-release
|
||||
w.Write([]byte(`{"choices":[{"message":{"content":"ok"}}]}`))
|
||||
}))
|
||||
t.Cleanup(srv.Close)
|
||||
return srv
|
||||
}
|
||||
|
||||
func (p *LLMPhraser) inflightCount() int {
|
||||
p.mu.Lock()
|
||||
defer p.mu.Unlock()
|
||||
return p.inflight
|
||||
}
|
||||
|
||||
func TestSwap_WaitsForARouterCallThatWentThroughLLMClient(t *testing.T) {
|
||||
fl := &fakeFleet{models: map[string]string{"/m/old.gguf": "old", "/m/new.gguf": "new"}}
|
||||
p := newSwapPhraser(t, fl, "/m/old.gguf")
|
||||
|
||||
release := make(chan struct{})
|
||||
c := llm.New(blockingLLM(t, release).URL, 5*time.Second)
|
||||
c.SetGate(p)
|
||||
|
||||
completed := make(chan error, 1)
|
||||
go func() {
|
||||
_, err := c.Complete(context.Background(), llm.Req{System: "s", User: "u"})
|
||||
completed <- err
|
||||
}()
|
||||
deadline := time.Now().Add(2 * time.Second)
|
||||
for p.inflightCount() == 0 {
|
||||
if time.Now().After(deadline) {
|
||||
t.Fatal("the llm.Client request never registered with the phraser gate")
|
||||
}
|
||||
time.Sleep(5 * time.Millisecond)
|
||||
}
|
||||
|
||||
swapped := make(chan error, 1)
|
||||
go func() { _, e := p.Swap(context.Background(), SwapSpec{ModelPath: "/m/new.gguf"}); swapped <- e }()
|
||||
|
||||
select {
|
||||
case e := <-swapped:
|
||||
t.Fatalf("the swap finished while a router call was still generating (%v); the old server was killed under it", e)
|
||||
case <-time.After(200 * time.Millisecond):
|
||||
}
|
||||
|
||||
close(release)
|
||||
if e := <-completed; e != nil {
|
||||
t.Fatalf("the in-flight call did not finish on the old model: %v", e)
|
||||
}
|
||||
if e := <-swapped; e != nil {
|
||||
t.Fatalf("Swap after the drain: %v", e)
|
||||
}
|
||||
}
|
||||
|
||||
func TestSwap_RefusesARouterCallThatArrivesMidSwap(t *testing.T) {
|
||||
fl := &fakeFleet{models: map[string]string{"/m/old.gguf": "old", "/m/new.gguf": "new"}}
|
||||
p := newSwapPhraser(t, fl, "/m/old.gguf")
|
||||
|
||||
// An open server: the refusal has to come from the gate, not from a stall.
|
||||
open := make(chan struct{})
|
||||
close(open)
|
||||
c := llm.New(blockingLLM(t, open).URL, 5*time.Second)
|
||||
c.SetGate(p)
|
||||
|
||||
// Hold the door shut the way quiesce does.
|
||||
_, held, err := p.acquire()
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
defer held()
|
||||
go p.Swap(context.Background(), SwapSpec{ModelPath: "/m/new.gguf"})
|
||||
|
||||
deadline := time.Now().Add(2 * time.Second)
|
||||
for {
|
||||
_, err := c.Complete(context.Background(), llm.Req{User: "u"})
|
||||
if errors.Is(err, ErrSwapping) {
|
||||
return
|
||||
}
|
||||
if time.Now().After(deadline) {
|
||||
t.Fatalf("a router call during a swap was not refused (last error: %v)", err)
|
||||
}
|
||||
time.Sleep(10 * time.Millisecond)
|
||||
}
|
||||
}
|
||||
|
||||
func TestSwap_TotalFailureIsNotReportedAsARollback(t *testing.T) {
|
||||
fl := &fakeFleet{models: map[string]string{"/m/old.gguf": "old"}}
|
||||
p := newSwapPhraser(t, fl, "/m/old.gguf")
|
||||
fl.mu.Lock()
|
||||
delete(fl.models, "/m/old.gguf")
|
||||
fl.mu.Unlock()
|
||||
|
||||
res, 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")
|
||||
}
|
||||
if res.RolledBack {
|
||||
t.Error("a total failure set RolledBack; the page then says she is still answering with the old model, and she is not answering at all")
|
||||
}
|
||||
if !res.NoBackend {
|
||||
t.Error("a total failure did not set NoBackend, so nothing distinguishes it from a rolled-back swap")
|
||||
}
|
||||
if path, _, _ := p.LiveModel(); path != "" {
|
||||
t.Errorf("LiveModel = %q after a total failure; nothing is loaded, and naming a gguf makes the page read as half-working", path)
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user