diff --git a/cmd/mavend/clarify_test.go b/cmd/mavend/clarify_test.go index e07298a..8b377e1 100644 --- a/cmd/mavend/clarify_test.go +++ b/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) + } +} diff --git a/cmd/mavgpud/runner.go b/cmd/mavgpud/runner.go index 39ce341..83a68b5 100644 --- a/cmd/mavgpud/runner.go +++ b/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 diff --git a/cmd/mavgpud/runner_test.go b/cmd/mavgpud/runner_test.go new file mode 100644 index 0000000..e9cae0d --- /dev/null +++ b/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") + } +} diff --git a/deploy/mavgpud.service b/deploy/mavgpud.service index b200baa..dc0d6f2 100644 --- a/deploy/mavgpud.service +++ b/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 diff --git a/internal/router/llmrouter.go b/internal/router/llmrouter.go index 5b88593..341193b 100644 --- a/internal/router/llmrouter.go +++ b/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) diff --git a/internal/router/llmrouter_test.go b/internal/router/llmrouter_test.go index 49279c1..f82e016 100644 --- a/internal/router/llmrouter_test.go +++ b/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) + } +} diff --git a/internal/router/router.go b/internal/router/router.go index bf9b430..1df278c 100644 --- a/internal/router/router.go +++ b/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 }