From a788ca39156d6a8e606f9812cd818a3a994d928d Mon Sep 17 00:00:00 2001 From: kami Date: Fri, 31 Jul 2026 14:25:28 +0400 Subject: [PATCH 1/3] Label eval runs with the model the server actually loaded (#379) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The phrasing eval printed "llm (0.8B, ...)" no matter which gguf llama-server had loaded, so two runs of two different models came out named the same and were easy to mix up when comparing. It now asks llama-server over /v1/models, same as the router eval already did. The helper moved to internal/llm so both share it, and it now errors instead of returning a blank name when the id field is missing — an unreachable server gets labelled "unknown-model", never a plausible-looking guess. Both eval paths stay opt-in behind MAVEN_LLM_URL; no server needed for go test. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ --- internal/{router/eval => llm}/modelid.go | 27 ++++++++-- internal/llm/modelid_test.go | 64 ++++++++++++++++++++++++ internal/phraser/eval/llmphraser_test.go | 16 +++++- internal/router/eval/llmrouter_test.go | 6 +-- 4 files changed, 105 insertions(+), 8 deletions(-) rename internal/{router/eval => llm}/modelid.go (55%) create mode 100644 internal/llm/modelid_test.go diff --git a/internal/router/eval/modelid.go b/internal/llm/modelid.go similarity index 55% rename from internal/router/eval/modelid.go rename to internal/llm/modelid.go index 5a9102d..36b90da 100644 --- a/internal/router/eval/modelid.go +++ b/internal/llm/modelid.go @@ -1,4 +1,4 @@ -package eval +package llm import ( "context" @@ -6,8 +6,20 @@ import ( "fmt" "net/http" "strings" + "time" ) +// UnknownModel is the label to print when the server would not say what it has +// loaded. Deliberately ugly: an honest "unknown" is fine, a plausible-looking +// but wrong model name is the bug this whole file exists to prevent. +const UnknownModel = "unknown-model" + +// llama-server is local, so never send this through a proxy: this box's +// http_proxy answers 503 for loopback, which would look like "server won't say +// which model it has" when the server is right there and fine. +// A Transport with no Proxy set bypasses http_proxy entirely. +var modelHTTP = &http.Client{Timeout: 10 * time.Second, Transport: &http.Transport{}} + // ModelID asks llama-server which model it has loaded, so a scoring run can // label itself. Without this a bake-off between two models produces two tables // that look identical, and the operator has to remember which server was up. @@ -19,7 +31,7 @@ func ModelID(ctx context.Context, base string) (string, error) { if err != nil { return "", err } - resp, err := http.DefaultClient.Do(req) + resp, err := modelHTTP.Do(req) if err != nil { return "", err } @@ -38,14 +50,21 @@ func ModelID(ctx context.Context, base string) (string, error) { if len(out.Data) == 0 { return "", fmt.Errorf("models: empty list") } - return shortModelID(out.Data[0].ID), nil + short := shortModelID(out.Data[0].ID) + if short == "" { + // Server answered but the id field was missing or blank. Say so + // instead of handing back an empty label that reads as a real name. + return "", fmt.Errorf("models: no id in response") + } + return short, nil } // shortModelID trims the path and the .gguf suffix — llama-server reports the // file name it was started with, which is too long for a table header. func shortModelID(id string) string { + id = strings.TrimSpace(id) if i := strings.LastIndexAny(id, "/\\"); i >= 0 { id = id[i+1:] } - return strings.TrimSuffix(id, ".gguf") + return strings.TrimSpace(strings.TrimSuffix(id, ".gguf")) } diff --git a/internal/llm/modelid_test.go b/internal/llm/modelid_test.go new file mode 100644 index 0000000..c5ce1df --- /dev/null +++ b/internal/llm/modelid_test.go @@ -0,0 +1,64 @@ +package llm + +import ( + "context" + "net/http" + "net/http/httptest" + "testing" +) + +// The point of these tests: a wrong-but-plausible model label is the bug, so +// every path that cannot learn the real name must return an error instead of a +// guess. No llama-server needed — a stub server stands in. +func TestModelID(t *testing.T) { + cases := []struct { + name string + body string + code int + want string // "" ⇒ expect an error + }{ + {"full path", `{"data":[{"id":"/mnt/hdd1/llms/qwen3.5/Qwen3.5-0.8B.Q4_K_M.gguf"}]}`, 200, "Qwen3.5-0.8B.Q4_K_M"}, + {"bare name", `{"data":[{"id":"LFM2.5-1.2B"}]}`, 200, "LFM2.5-1.2B"}, + {"empty list", `{"data":[]}`, 200, ""}, + {"id missing", `{"data":[{}]}`, 200, ""}, + {"id blank", `{"data":[{"id":" "}]}`, 200, ""}, + {"server error", `nope`, 500, ""}, + {"not json", ``, 200, ""}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/v1/models" { + t.Errorf("asked for %s, want /v1/models", r.URL.Path) + } + w.WriteHeader(c.code) + _, _ = w.Write([]byte(c.body)) + })) + defer srv.Close() + + got, err := ModelID(context.Background(), srv.URL+"/") + if c.want == "" { + if err == nil { + t.Fatalf("want an error, got label %q", got) + } + return + } + if err != nil { + t.Fatalf("ModelID: %v", err) + } + if got != c.want { + t.Errorf("got %q, want %q", got, c.want) + } + }) + } +} + +func TestModelIDUnreachable(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(http.ResponseWriter, *http.Request) {})) + url := srv.URL + srv.Close() // nothing listening now + + if got, err := ModelID(context.Background(), url); err == nil { + t.Fatalf("want an error from a dead server, got label %q", got) + } +} diff --git a/internal/phraser/eval/llmphraser_test.go b/internal/phraser/eval/llmphraser_test.go index 12b108a..316eaa2 100644 --- a/internal/phraser/eval/llmphraser_test.go +++ b/internal/phraser/eval/llmphraser_test.go @@ -7,6 +7,7 @@ import ( "testing" "time" + "github.com/kami/maven/internal/llm" "github.com/kami/maven/internal/phraser" ) @@ -32,6 +33,7 @@ func TestLLMPhrasingBaseline(t *testing.T) { // case as a phrasing error and read as "the model cannot phrase". noProxyLoopback(t) + ctx := context.Background() f, err := Load() if err != nil { t.Fatalf("Load: %v", err) @@ -44,7 +46,19 @@ func TestLLMPhrasingBaseline(t *testing.T) { p := phraser.NewLLMPhraserAt(base, cfg) defer p.Close() - rep, err := Score(context.Background(), "llm (0.8B, built-in persona)", p, f) + // Label the run with whatever gguf the server actually has loaded. It used + // to say "0.8B" no matter what, so two runs of two different models came + // out named the same and were easy to mix up when comparing. + model, err := llm.ModelID(ctx, base) + if err != nil { + // An unlabelled score is still a score, but say so loudly — a made-up + // name in a bake-off table is worse than no name. + t.Logf("could not read model id from %s: %v — report will say %q", base, err, llm.UnknownModel) + model = llm.UnknownModel + } + t.Logf("scoring model %s at %s", model, base) + + rep, err := Score(ctx, "llm ("+model+", built-in persona)", p, f) if err != nil { t.Fatalf("Score: %v", err) } diff --git a/internal/router/eval/llmrouter_test.go b/internal/router/eval/llmrouter_test.go index 940eea9..12a8e76 100644 --- a/internal/router/eval/llmrouter_test.go +++ b/internal/router/eval/llmrouter_test.go @@ -61,12 +61,12 @@ func TestLLMRouterBaseline(t *testing.T) { } ctx := context.Background() - model, err := ModelID(ctx, base) + model, err := llm.ModelID(ctx, base) if err != nil { // Not fatal: an unlabelled score is still a score. But say so loudly, // because an unlabelled row in a bake-off table is worthless. - t.Logf("could not read model id from %s: %v — reports will say %q", base, err, "unknown-model") - model = "unknown-model" + t.Logf("could not read model id from %s: %v — reports will say %q", base, err, llm.UnknownModel) + model = llm.UnknownModel } t.Logf("scoring model %s at %s", model, base) lr := router.NewLLMRouter(client) From 02e878669551f3d090221e86fa7998072fdb8c33 Mon Sep 17 00:00:00 2001 From: kami Date: Fri, 31 Jul 2026 14:30:51 +0400 Subject: [PATCH 2/3] Stop the containers instead of claiming success (#380) docker-compose.yml has no 'pid: host', so each container has its own PID namespace and pkill on the host matches nothing inside them. The script then printed "All services gracefully stopped" while mavend, its llama-server and the rest were still running. Now it checks for running compose containers first and stops them with docker compose. If it cannot ask docker and finds nothing to kill on the host, or anything survives the kill, it says so and exits non-zero instead of claiming success. The bare-metal path is unchanged apart from verifying the SIGKILL actually worked, and no longer risks killing the shell it was launched from. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ --- kill-maven.sh | 79 ++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 75 insertions(+), 4 deletions(-) diff --git a/kill-maven.sh b/kill-maven.sh index f0754bd..725a3fa 100755 --- a/kill-maven.sh +++ b/kill-maven.sh @@ -1,8 +1,10 @@ #!/usr/bin/env bash # Unified script to stop all Maven services. # Usage: ./kill-maven.sh -# - Graceful SIGTERM is attempted first. -# - If any process lingers, force with SIGKILL. +# - Docker deploy: `docker compose stop` (see why below). +# - Bare-metal / dev run: graceful SIGTERM first, SIGKILL if anything lingers. +# Exits non-zero if it cannot confirm everything is stopped. It must never say +# "stopped" unless it checked. set -euo pipefail @@ -24,6 +26,69 @@ else LLM='llama-server.*\.gguf' fi +COMPOSE_FILE="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/docker-compose.yml" + +# --- containerised deploy ------------------------------------------------ +# docker-compose.yml does not set `pid: host`, so each container has its own +# PID namespace: pkill on the host sees nothing inside them. This script used +# to print "all stopped" while every daemon was still happily running. Stop the +# containers through compose instead — that actually reaches them. +# +# running_containers prints the ids of the project's running containers, or +# nothing. Empty output plus a non-zero return means "could not ask docker", +# which is different from "nothing is running" and is handled below. +running_containers() { + docker compose -f "$COMPOSE_FILE" ps -q --status running 2>/dev/null +} + +DOCKER_OK=0 +CONTAINERS="" +if command -v docker >/dev/null 2>&1 && [ -f "$COMPOSE_FILE" ]; then + if CONTAINERS="$(running_containers)"; then + DOCKER_OK=1 + fi +fi + +if [ "$DOCKER_OK" = 1 ] && [ -n "$CONTAINERS" ]; then + echo "--- Maven is running in containers: stopping via docker compose ---" + if ! docker compose -f "$COMPOSE_FILE" stop; then + echo "ERROR: 'docker compose stop' failed. Containers may still be running." >&2 + exit 1 + fi + echo "--- Verifying containers are gone ---" + LEFT="$(running_containers || true)" + if [ -n "$LEFT" ]; then + echo "ERROR: containers still running after stop:" >&2 + docker compose -f "$COMPOSE_FILE" ps >&2 || true + exit 1 + fi + echo "All containers stopped." + exit 0 +fi + +# --- bare-metal / dev run ----------------------------------------------- +# pgrep -f matches whole command lines, so a shell that merely mentions +# "mavend" (this script's own parent, for one) shows up. Drop ourselves and our +# parent, otherwise the SIGKILL sweep can take out the terminal you ran this in. +host_pids() { + pgrep -f "$PAT|$LLM" | grep -v -e "^$$\$" -e "^$PPID\$" | paste -sd, - || true +} +HOST_PIDS=$(host_pids) + +if [ -z "$HOST_PIDS" ]; then + # Nothing to kill on the host and no running containers. Either Maven is + # already down, or it is somewhere this script cannot see (another PID + # namespace, another user, docker unreachable). We cannot tell the + # difference, so refuse to claim success. + echo "ERROR: found no Maven processes on this host and no running containers." >&2 + if [ "$DOCKER_OK" != 1 ]; then + echo " Could not ask docker either — if this is the container deploy," >&2 + echo " run: docker compose -f $COMPOSE_FILE stop" >&2 + fi + echo " Nothing was stopped. Check by hand before assuming Maven is down." >&2 + exit 1 +fi + echo "--- Sending graceful SIGTERM to Maven services ---" pkill -TERM -f "$PAT" || true # mavend's Pdeathsig SIGKILLs its llama-server on exit, but sweep strays too @@ -32,12 +97,18 @@ pkill -TERM -f "$LLM" || true echo "--- Verifying processes are gone ---" sleep 1 -PIDS=$(pgrep -d ',' -f "$PAT|$LLM") || PIDS="" +PIDS=$(host_pids) if [ -n "$PIDS" ]; then echo "Warning: some processes still alive. PIDs: $PIDS" echo "--- Force killing with SIGKILL ---" echo "$PIDS" | tr ',' '\n' | xargs -r kill -9 + sleep 1 + LEFT=$(host_pids) + if [ -n "$LEFT" ]; then + echo "ERROR: still alive after SIGKILL. PIDs: $LEFT" >&2 + exit 1 + fi echo "Done (SIGKILL)." else echo "All services gracefully stopped." -fi \ No newline at end of file +fi From 80f73222943434ad80058e17733a3c1b192aebe8 Mon Sep 17 00:00:00 2001 From: kami Date: Fri, 31 Jul 2026 14:32:45 +0400 Subject: [PATCH 3/3] Don't fail when docker confirms nothing is running "Nothing on the host" meant two different things and the script treated them the same. If docker answers and names no running containers, Maven really is down and the script should say so and exit 0. Only when docker cannot be asked is the answer unknown, and that is the case that must fail loudly. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ --- kill-maven.sh | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/kill-maven.sh b/kill-maven.sh index 725a3fa..df3ec80 100755 --- a/kill-maven.sh +++ b/kill-maven.sh @@ -76,15 +76,19 @@ host_pids() { HOST_PIDS=$(host_pids) if [ -z "$HOST_PIDS" ]; then - # Nothing to kill on the host and no running containers. Either Maven is - # already down, or it is somewhere this script cannot see (another PID - # namespace, another user, docker unreachable). We cannot tell the - # difference, so refuse to claim success. - echo "ERROR: found no Maven processes on this host and no running containers." >&2 - if [ "$DOCKER_OK" != 1 ]; then - echo " Could not ask docker either — if this is the container deploy," >&2 - echo " run: docker compose -f $COMPOSE_FILE stop" >&2 + # Nothing on the host. Whether that means "already down" depends on whether + # we managed to ask docker, and the two must not read the same. + if [ "$DOCKER_OK" = 1 ]; then + # Docker answered and named no running containers, and there is nothing + # on the host either. That is a real answer: Maven is already stopped. + echo "Nothing to stop: no Maven processes and no running containers." + exit 0 fi + # We could not ask docker, so Maven may be alive in a container we cannot + # see. Saying "stopped" here is the exact false success this script had. + echo "ERROR: no Maven processes on this host, and docker could not be asked." >&2 + echo " If this is the container deploy it may still be running:" >&2 + echo " docker compose -f $COMPOSE_FILE stop" >&2 echo " Nothing was stopped. Check by hand before assuming Maven is down." >&2 exit 1 fi