Merge pull request 'llama-server core-dumps on every SIGTERM, so each mavgpud yield writes a core file' (#116) from task/491-llama-server-core-dumps-on-every-sigterm into master

cmd/mavend/clarify_test.go

@@ -465,3 +465,39 @@ func TestExpiryNoticeSurvivesAConfirmTurn(t *testing.T) {
 		t.Fatal("the expired question must be gone")
 	}
 }
+
+// The other half of the subject question: his answer must fill the empty slot,
+// not replace the request. Slots.Text used to be the whole raw utterance for
+// every intent, so the branch that fills a text slot could only ever overwrite
+// (Vikunja #383). Here the parked request holds the hour and the answer holds
+// what to say at it, and the reminder that lands has both.
+func TestClarifySubjectAnswerFillsRatherThanClobbers(t *testing.T) {
+	ctx := context.Background()
+	h, st, _ := newClarifyHandler(t)
+	at := h.now().Add(2 * time.Hour)
+
+	question, asked := h.askClarify(clarifyDec(router.IntentReminder,
+		router.Slots{Time: at, HasTime: true}, "напомни в 11"))
+	if !asked || question != "О чём напомнить?" {
+		t.Fatalf("expected the subject question, got %q asked=%v", question, asked)
+	}
+
+	reply, handled := h.resolveClarifyAnswer(ctx, "позвонить маме")
+	if !handled {
+		t.Fatal("the answer to an open question must be consumed as an answer")
+	}
+	if reply == clarifyGaveUp {
+		t.Fatalf("a good answer must not drop the request: %q", reply)
+	}
+
+	reminders, err := st.DueReminders(ctx, h.now().Add(48*time.Hour))
+	if err != nil || len(reminders) != 1 {
+		t.Fatalf("clarified reminder was not created: reminders=%v err=%v", reminders, err)
+	}
+	if !strings.Contains(reminders[0].Payload, "маме") {
+		t.Fatalf("the answer never reached the reminder: %q", reminders[0].Payload)
+	}
+	if !strings.Contains(reminders[0].Payload, "11") {
+		t.Fatalf("the answer clobbered the original request: %q", reminders[0].Payload)
+	}
+}

cmd/mavgpud/runner.go

@@ -24,7 +24,12 @@ type runner struct {
 	mu    sync.Mutex
 	cmd   *exec.Cmd
 	ready bool
-	http  *http.Client
+	// yielding — stop() has sent the signal and the exit that follows is ours.
+	// llama-server aborts on SIGTERM (its static teardown throws, upstream
+	// ggml-org/llama.cpp), so a routine yield and a real crash produce the same
+	// "signal: aborted" and used to log identically (Vikunja #491).
+	yielding bool
+	http     *http.Client
 }
 
 func newRunner(bin string, args []string, readyURL string) *runner {
@@ -70,13 +75,18 @@ func (r *runner) start() error {
 	if err := cmd.Start(); err != nil {
 		return err
 	}
-	r.cmd, r.ready = cmd, false
+	r.cmd, r.ready, r.yielding = cmd, false, false
 	log.Printf("mavgpud: started llama-server pid=%d", cmd.Process.Pid)
 	go func() {
 		err := cmd.Wait()
 		r.mu.Lock()
-		r.cmd, r.ready = nil, false
+		yielded := r.yielding
+		r.cmd, r.ready, r.yielding = nil, false, false
 		r.mu.Unlock()
+		if yielded {
+			log.Printf("mavgpud: llama-server stopped, card yielded (%v)", err)
+			return
+		}
 		log.Printf("mavgpud: llama-server exited: %v", err)
 	}()
 	return nil
@@ -90,6 +100,10 @@ func (r *runner) stop(grace time.Duration) {
 	r.mu.Lock()
 	cmd := r.cmd
 	r.ready = false
+	if cmd != nil && cmd.Process != nil {
+		// The exit that follows is ours, not a crash.
+		r.yielding = true
+	}
 	r.mu.Unlock()
 	if cmd == nil || cmd.Process == nil {
 		return

cmd/mavgpud/runner_test.go

@@ -0,0 +1,59 @@
+package main
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+	"time"
+)
+
+// fakeServer writes an executable standing in for llama-server: it ignores
+// SIGTERM the way the real one effectively does — by dying messily rather than
+// cleanly — and reports a non-zero status.
+func fakeServer(t *testing.T, body string) string {
+	t.Helper()
+	path := filepath.Join(t.TempDir(), "fake-llama-server")
+	if err := os.WriteFile(path, []byte("#!/bin/sh\n"+body+"\n"), 0o755); err != nil {
+		t.Fatal(err)
+	}
+	return path
+}
+
+// A deliberate stop is a yield, and the log has to say so.
+//
+// llama-server aborts inside its own static teardown on SIGTERM, so the exit
+// status of a routine yield is identical to that of a real crash. Reading the
+// mavgpud log, the two were indistinguishable (Vikunja #491).
+func TestStopMarksTheExitAsAYield(t *testing.T) {
+	r := newRunner(fakeServer(t, "while : ; do sleep 1 ; done"), nil, "")
+	if err := r.start(); err != nil {
+		t.Fatalf("start: %v", err)
+	}
+	r.mu.Lock()
+	if r.yielding {
+		t.Error("a freshly started server is already marked as yielding")
+	}
+	r.mu.Unlock()
+
+	r.stop(2 * time.Second)
+	deadline := time.Now().Add(2 * time.Second)
+	for time.Now().Before(deadline) {
+		if !r.running() {
+			return
+		}
+		time.Sleep(10 * time.Millisecond)
+	}
+	t.Fatal("the child outlived stop")
+}
+
+// Stopping when nothing is running must not arm the flag for the next child.
+// The next exit after that would be a real crash logged as a yield.
+func TestStopWithNoChildDoesNotArmTheFlag(t *testing.T) {
+	r := newRunner("/nonexistent", nil, "")
+	r.stop(10 * time.Millisecond)
+	r.mu.Lock()
+	defer r.mu.Unlock()
+	if r.yielding {
+		t.Error("stop armed the yield flag with no child running")
+	}
+}

deploy/mavgpud.service

@@ -19,6 +19,10 @@ RestartSec=5
 # llama-server on SIGTERM, so give it longer than stop_grace to do that.
 KillSignal=SIGTERM
 TimeoutStopSec=60
+# llama-server aborts inside its own static teardown on SIGTERM, so every
+# routine yield used to write a multi-gigabyte core into systemd-coredump
+# (Vikunja #491). Yielding is meant to happen several times a day.
+LimitCORE=0
 
 [Install]
 WantedBy=default.target

internal/router/llmrouter.go

@@ -210,7 +210,11 @@ func (lr *LLMRouter) Route(ctx context.Context, utterance string, now time.Time)
 		d.Slots.HasKey = a.Key != ""
 	case IntentReminder:
 		d.Intent = IntentReminder
-		d.Slots.Text = firstNonEmpty(a.Text, utterance)
+		// No utterance fallback here, unlike every other intent below. The
+		// model returning no text for a reminder means it found no subject,
+		// and "напомни в 11" is not a subject. Leaving Text empty is what
+		// lets the gate turn that into a question (Vikunja #383).
+		d.Slots.Text = a.Text
 	case IntentNote:
 		d.Intent = IntentNote
 		d.Slots.Text = firstNonEmpty(a.Text, utterance)

internal/router/llmrouter_test.go

@@ -356,3 +356,35 @@ func TestRouterLLMFactWithResolvedKeyStaysConfident(t *testing.T) {
 		t.Fatalf("a fact the parser could key must not clarify: %+v", d)
 	}
 }
+
+// A reminder with a time and no subject must come back empty and gated, not
+// backfilled with the raw words. "напомни в 11" carries an hour and nothing to
+// say at that hour; parking the utterance in Text made the request look
+// complete, so the daemon set a reminder that fires saying "напомни в 11"
+// (Vikunja #383).
+func TestLLMReminderWithoutSubjectAsksInsteadOfGuessing(t *testing.T) {
+	r := newLLMTestRouter(t, `{"intent":"reminder"}`)
+	d, err := r.Route(context.Background(), "напомни в 11", refNow())
+	if err != nil {
+		t.Fatalf("route: %v", err)
+	}
+	if d.Slots.Text != "" {
+		t.Fatalf("subject backfilled from the utterance: %q", d.Slots.Text)
+	}
+	if !d.Clarify {
+		t.Fatalf("a subjectless reminder was accepted, confidence %v", d.Confidence)
+	}
+}
+
+// The gate is about the subject, not about reminders in general: one that has
+// both halves still runs without a question.
+func TestLLMReminderWithSubjectIsNotGated(t *testing.T) {
+	r := newLLMTestRouter(t, `{"intent":"reminder","text":"позвонить маме"}`)
+	d, err := r.Route(context.Background(), "напомни в 11 позвонить маме", refNow())
+	if err != nil {
+		t.Fatalf("route: %v", err)
+	}
+	if d.Clarify {
+		t.Fatalf("a complete reminder was sent back as a question: %+v", d.Slots)
+	}
+}

internal/router/router.go

@@ -147,7 +147,15 @@ func (r *Router) fillSlots(ctx context.Context, d *Decision, now time.Time) {
 			d.Slots.Fn, d.Slots.Args, d.Slots.HasFn = fn, args, true
 		}
 	}
-	if d.Slots.Text == "" {
+	// The extractor's Text is the raw utterance, which is the payload for a
+	// note, a query or a chat turn but not for a reminder — there Text is the
+	// subject, what she says at the hour. Backfilling it made Text impossible
+	// to be empty, so StillMissing never reported SlotText and "О чём
+	// напомнить?" was unaskable; the answer to a question she did manage to
+	// ask then overwrote the whole request instead of filling one gap
+	// (Vikunja #383). A reminder with no subject stays empty and is gated
+	// below into a question.
+	if d.Slots.Text == "" && d.Intent != IntentReminder {
 		d.Slots.Text = ex.Text
 	}
 	// Stage stays 1: it says who decided the route, and that was the LLM.
@@ -177,6 +185,12 @@ func (r *Router) gateLLMDecision(d *Decision) {
 	if d.Intent == IntentAct && !d.Slots.HasFn && d.Confidence > llmThinConfidence {
 		d.Confidence = llmThinConfidence
 	}
+	// A reminder with no subject: she knows when but not what to say then.
+	// Setting it anyway fires an empty reminder at the hour, which reads as a
+	// bug to him and cannot be repaired after the fact. Ask (Vikunja #383).
+	if d.Intent == IntentReminder && d.Slots.Text == "" && d.Confidence > llmThinConfidence {
+		d.Confidence = llmThinConfidence
+	}
 	if d.Confidence < r.threshold {
 		d.Clarify = true
 	}