speaker: do not ship three methods that cannot work

The package comment, the embedder log and the startup line all said
enrolment was live and only recognition was blocked. Enroll embeds every
sample before it stores anything, so with no model on the box it fails
on the first sample with ErrDisabled and nothing is ever stored. List
then returns an empty list forever and Forget has nothing to delete. The
shipped state was three methods, all no-ops, announced as a working
half.

SpeakerConfig.Recognizes was written as the gate for this and never
called, so a block with enabled and no model_path wired everything and
skipped the one warning the operator needed. It is the gate now, and
that config shape logs why it stayed off.

Three smaller repairs. ErrDisabled had no case in speakerErr and reached
the surface as an opaque core failure, when it means the same thing
ErrUnknownMethod does. Forget read the row first and answered ErrNotFound
on a second call, so the layer documented as the one that must always
work reintroduced a failure for a voiceprint that was already gone.
And a row with unparsable metadata listed as a plausible profile named
after its own id with 0 samples, which is what a real minimal enrolment
looks like; it is reported as damaged now.

Found in review of #74.

cmd/mavend/speaker.go

@@ -3,14 +3,17 @@
 //
 // # What is actually wired here, and what is not
 //
-// The enrolment plumbing is real: profiles are stored, listed and deleted, and
-// the wire methods exist as soon as a speaker block is configured. The
-// recognising half is NOT, and cannot be on this box, because there is no
-// speaker-embedding model on disk — no ECAPA, no x-vector, no titanet, no
-// wespeaker, nothing in /mnt/hdd1/llms but text ggufs. Until one is downloaded,
-// newSpeakerEmbedder returns nil, internal/speaker falls back to
-// speaker.Disabled, and every Identify answers ErrDisabled. The daemon logs
-// which half is off at startup rather than pretending.
+// Nothing is, on this box. There is no speaker-embedding model on disk — no
+// ECAPA, no x-vector, no titanet, no wespeaker, nothing in /mnt/hdd1/llms but
+// text ggufs. Until one is downloaded, newSpeakerEmbedder returns nil.
+//
+// Without an embedder the capability has no runnable half. This comment used to
+// say enrolment was real and only recognition was blocked, and the startup log
+// said the same. Both were wrong: Recognizer.Enroll embeds every sample before
+// it stores anything, so with no model it fails on the first sample and nothing
+// is ever stored, which leaves List empty forever and Forget with nothing to
+// delete. So the gate is cfg.Speaker.Recognizes() — enabled AND a model path —
+// and a box without one gets no speaker methods, not three no-ops.
 //
 // This is deliberately not papered over with a hand-rolled MFCC floor. A
 // biometric that is confidently wrong writes false claims about named people
@@ -53,17 +56,25 @@ type speakerWiring struct {
 // starts working with no change to the store, the protocol, the auth table or
 // the handlers. See the plan document for what to download.
 func newSpeakerEmbedder(cfg *config.SpeakerConfig) speaker.Embedder {
-	if cfg == nil || cfg.ModelPath == "" {
-		return nil
-	}
-	log.Printf("speaker: model_path %q is configured but no embedding backend is built yet; "+
-		"enrolment and deletion work, recognition does not (Vikunja #255)", cfg.ModelPath)
+	_ = cfg
 	return nil
 }
 
 // newSpeakerWiring builds the recognizer, or nil when the capability is off.
 func newSpeakerWiring(st *store.Store, cfg *config.Config) *speakerWiring {
-	if cfg == nil || cfg.Speaker == nil || !cfg.Speaker.Enabled {
+	if cfg == nil || cfg.Speaker == nil {
+		return nil
+	}
+	if !cfg.Speaker.Recognizes() {
+		// Recognizes() was written as the gate and documented as one, and then
+		// never called. "enabled": true with no model_path used to wire all
+		// three methods and log "enrolment on", which is the one config shape
+		// where the operator most needs to be told otherwise.
+		if cfg.Speaker.Enabled {
+			log.Print("speaker: enabled but no model_path, so there is nothing to embed with; " +
+				"enrol, list and forget would all be no-ops, staying off " +
+				"(see docs/plans/10-speaker-recognition.md)")
+		}
 		return nil
 	}
 	if st == nil {
@@ -81,8 +92,8 @@ func newSpeakerWiring(st *store.Store, cfg *config.Config) *speakerWiring {
 	if rec.Enabled() {
 		log.Printf("speaker: recognition on, threshold %.2f", rec.Threshold())
 	} else {
-		log.Print("speaker: enrolment on, recognition BLOCKED — no speaker-embedding model " +
-			"on this box (see docs/plans/10-speaker-recognition.md)")
+		log.Printf("speaker: model_path %q is configured but no embedding backend is built yet, "+
+			"so enrol, list and forget are all no-ops (Vikunja #255)", cfg.Speaker.ModelPath)
 	}
 	return &speakerWiring{rec: rec}
 }
@@ -114,7 +125,7 @@ func (w *speakerWiring) forget(ctx context.Context, req ipc.ForgetSpeakerReq) er
 // toWireSpeaker drops the voiceprint. A listing says who is enrolled; it does
 // not hand the biometric back out over the socket.
 func toWireSpeaker(p speaker.Profile) ipc.Speaker {
-	return ipc.Speaker{ID: p.ID, Name: p.Name, Enrolled: p.Enrolled, Samples: p.Samples}
+	return ipc.Speaker{ID: p.ID, Name: p.Name, Enrolled: p.Enrolled, Samples: p.Samples, Damaged: p.Damaged}
 }
 
 // speakerErr maps the package sentinels onto the wire vocabulary so a surface
@@ -123,6 +134,11 @@ func speakerErr(err error) error {
 	switch {
 	case err == nil:
 		return nil
+	case errors.Is(err, speaker.ErrDisabled):
+		// Not a core failure. The capability is present on the wire but has no
+		// embedding model behind it, which is the same thing an unconfigured
+		// method says, so say it the same way.
+		return ipc.ErrUnknownMethod
 	case errors.Is(err, speaker.ErrNotFound):
 		return ipc.ErrNoFact
 	case errors.Is(err, speaker.ErrBadID),

cmd/mavend/speaker_wiring_test.go

@@ -0,0 +1,50 @@
+package main
+
+import (
+	"errors"
+	"testing"
+
+	"github.com/kami/maven/internal/config"
+	"github.com/kami/maven/internal/ipc"
+	"github.com/kami/maven/internal/speaker"
+)
+
+// "enabled": true with no model_path used to wire all three methods and log
+// "enrolment on". Nothing behind them works without an embedder, so the
+// capability stays off and the socket answers "no such method".
+func TestSpeakerStaysOffWithoutAModelPath(t *testing.T) {
+	srv := &ipc.Server{}
+	cfg := &config.Config{Speaker: &config.SpeakerConfig{Enabled: true}}
+
+	wireSpeaker(srv, nil, cfg)
+
+	if srv.EnrollSpeakerFn != nil || srv.ListSpeakersFn != nil || srv.ForgetSpeakerFn != nil {
+		t.Error("speaker methods were wired with nothing to embed with")
+	}
+}
+
+// The gate is Recognizes(), so a disabled block with a model path is off too.
+func TestSpeakerStaysOffWhenDisabled(t *testing.T) {
+	srv := &ipc.Server{}
+	cfg := &config.Config{Speaker: &config.SpeakerConfig{ModelPath: "/nope/ecapa.onnx"}}
+
+	wireSpeaker(srv, nil, cfg)
+
+	if srv.EnrollSpeakerFn != nil {
+		t.Error("speaker methods were wired for a disabled block")
+	}
+}
+
+// ErrDisabled is "this capability is off", not "core broke". It used to fall
+// through speakerErr's default and reach the surface as an opaque failure.
+func TestSpeakerErrMapsDisabledToUnknownMethod(t *testing.T) {
+	if got := speakerErr(speaker.ErrDisabled); !errors.Is(got, ipc.ErrUnknownMethod) {
+		t.Errorf("speakerErr(ErrDisabled) = %v, want ErrUnknownMethod", got)
+	}
+	if got := speakerErr(speaker.ErrNotFound); !errors.Is(got, ipc.ErrNoFact) {
+		t.Errorf("speakerErr(ErrNotFound) = %v, want ErrNoFact", got)
+	}
+	if got := speakerErr(nil); got != nil {
+		t.Errorf("speakerErr(nil) = %v", got)
+	}
+}

internal/ipc/api.go

@@ -325,6 +325,12 @@ type Speaker struct {
 	Name     string    `json:"name"`
 	Enrolled time.Time `json:"enrolled"`
 	Samples  int       `json:"samples"`
+	// Damaged — the stored row's metadata did not read back cleanly. The
+	// voiceprint is still there; the name, sample count or enrolment time is
+	// not trustworthy. A surface should say so rather than render a corrupt
+	// row as a profile enrolled from zero samples, which is what a real
+	// minimal enrolment looks like.
+	Damaged bool `json:"damaged,omitempty"`
 }
 
 // EnrollSpeakerResp — the profile that was written.

internal/speaker/recognizer.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 	"sort"
+	"strconv"
 	"time"
 
 	"github.com/kami/maven/internal/audio"
@@ -140,14 +141,18 @@ func (r *Recognizer) Get(ctx context.Context, id string) (Profile, error) {
 // Forget deletes a profile. This is the one operation that must always work:
 // a voiceprint is data about a person, and "перестань узнавать её" has to
 // actually remove it, not mark it inactive.
+//
+// Forgetting a profile that is not there is not an error, matching
+// memory.Catalog.Delete. It used to read the row first and answer ErrNotFound,
+// which meant the layer documented as the one that must always work was the
+// layer reintroducing a failure: a surface retrying a forget after a partial
+// failure got an error on the second try, for a voiceprint that was already
+// gone. The caller asked for it to be gone and it is gone.
 func (r *Recognizer) Forget(ctx context.Context, id string) error {
 	id = NormalizeID(id)
 	if !ValidID(id) {
 		return fmt.Errorf("%w: %q", ErrBadID, id)
 	}
-	if _, err := r.Get(ctx, id); err != nil {
-		return err
-	}
 	if err := r.cat.Delete(ctx, Prefix+id); err != nil {
 		return fmt.Errorf("speaker: forget %q: %w", id, err)
 	}
@@ -170,6 +175,12 @@ func (r *Recognizer) embed(ctx context.Context, a audio.Audio) ([]float32, error
 // profileFromRecord reads a stored row back into a Profile. A row with
 // unreadable metadata still yields a usable voiceprint — the vector is the part
 // that matters, and losing a name should not lose the enrolment.
+//
+// It also says when the metadata did not read cleanly. Without that, a row
+// whose samples count is "12x" and whose name is missing came back as a
+// plausible profile called by its own id with 0 samples, which is exactly what
+// a real minimal enrolment looks like. Damaged is the difference between "he
+// enrolled badly" and "this row is broken".
 func profileFromRecord(rec memory.Record) Profile {
 	p := Profile{
 		ID:   trimPrefix(rec.ID),
@@ -178,15 +189,24 @@ func profileFromRecord(rec memory.Record) Profile {
 		Name: rec.Meta["name"],
 	}
 	if s := rec.Meta["samples"]; s != "" {
-		p.Samples = atoi(s)
+		n, err := strconv.Atoi(s)
+		if err != nil || n < 0 {
+			p.Damaged = true
+		} else {
+			p.Samples = n
+		}
 	}
 	if ts := rec.Meta["enrolled"]; ts != "" {
-		if t, err := time.Parse(time.RFC3339, ts); err == nil {
+		t, err := time.Parse(time.RFC3339, ts)
+		if err != nil {
+			p.Damaged = true
+		} else {
 			p.Enrolled = t
 		}
 	}
 	if p.Name == "" {
 		p.Name = p.ID
+		p.Damaged = true
 	}
 	return p
 }
@@ -197,16 +217,3 @@ func trimPrefix(id string) string {
 	}
 	return id
 }
-
-// atoi is a tolerant small-integer parse: metadata that is not a number reads
-// as 0 rather than failing the whole listing.
-func atoi(s string) int {
-	n := 0
-	for _, r := range s {
-		if r < '0' || r > '9' {
-			return 0
-		}
-		n = n*10 + int(r-'0')
-	}
-	return n
-}

internal/speaker/speaker.go

@@ -31,6 +31,14 @@
 // There are also no enrolment samples. So Recognizer runs against Disabled and
 // every Identify answers ErrDisabled until a model lands.
 //
+// "Recognition is blocked but enrolment works" is not true and this package
+// used to imply it. Enroll embeds every sample before it stores anything
+// (enroll.go, "Embed first, store second"), so with no model it fails on the
+// first sample with ErrDisabled and nothing is ever stored. List then returns
+// an empty list forever and Forget has nothing to delete. Without an embedder
+// all three operations are no-ops, and mavend does not wire the wire methods
+// at all in that state.
+//
 // The MFCC + GMM "simplest floor" in the plan document is refused rather than
 // deferred. A hand-rolled spectral distance would identify people confidently
 // and wrongly, and its output would be written into facts as "Ками said this".
@@ -46,11 +54,15 @@ import (
 	"time"
 
 	"github.com/kami/maven/internal/audio"
+	"github.com/kami/maven/internal/memory"
 )
 
 // Prefix — the id prefix speaker profiles carry in the shared vector table.
-// It is what ByPrefix enumerates and what keeps voiceprints out of note recall.
-const Prefix = "speaker:"
+// It is what ByPrefix enumerates, and what every Store implementation filters
+// out of Search so note recall cannot rank a voiceprint. The constant lives in
+// internal/memory because the store layer has to know it and cannot import this
+// package.
+const Prefix = memory.NonRecallPrefix
 
 // DefaultThreshold — cosine similarity a match must beat to be a match.
 //
@@ -133,6 +145,12 @@ type Profile struct {
 	Samples  int       `json:"samples"`
 	Dim      int       `json:"dim"`
 
+	// Damaged marks a row whose stored metadata did not read back cleanly —
+	// an unparsable sample count or timestamp, or a missing name. The
+	// voiceprint is still usable, but the row should be listed as damaged
+	// rather than as a plausible profile enrolled from nothing.
+	Damaged bool `json:"damaged,omitempty"`
+
 	// Vec is the voiceprint. Not serialised to any surface: a listing tells him
 	// who is enrolled, it does not hand out the biometric itself.
 	Vec []float32 `json:"-"`

internal/speaker/speaker_test.go

@@ -269,8 +269,58 @@ func TestForgetRemovesTheVoiceprint(t *testing.T) {
 	if _, err := r.Get(ctx, "guest"); !errors.Is(err, ErrNotFound) {
 		t.Errorf("Get after Forget = %v, want ErrNotFound", err)
 	}
-	if err := r.Forget(ctx, "guest"); !errors.Is(err, ErrNotFound) {
-		t.Errorf("second Forget = %v, want ErrNotFound", err)
+	// Forgetting twice is not an error. A surface retrying after a partial
+	// failure must not be told the voice it asked to remove is missing.
+	if err := r.Forget(ctx, "guest"); err != nil {
+		t.Errorf("second Forget = %v, want nil", err)
+	}
+}
+
+// A row whose metadata is corrupt lists as damaged, not as a plausible profile
+// enrolled from zero samples. Those two used to look identical.
+func TestCorruptMetadataListsAsDamaged(t *testing.T) {
+	r, cat := newRec(t, &fakeEmbedder{vec: []float32{1, 0}})
+	ctx := context.Background()
+	if err := cat.Insert(ctx, Prefix+"kami", []float32{1, 0}, map[string]string{
+		"kind":     "speaker",
+		"samples":  "12x",
+		"enrolled": "yesterday",
+	}); err != nil {
+		t.Fatal(err)
+	}
+	ps, err := r.List(ctx)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(ps) != 1 {
+		t.Fatalf("List returned %d profiles, want 1", len(ps))
+	}
+	p := ps[0]
+	if !p.Damaged {
+		t.Errorf("profile %+v is not marked damaged", p)
+	}
+	if p.Samples != 0 || !p.Enrolled.IsZero() {
+		t.Errorf("unreadable metadata was parsed anyway: samples %d, enrolled %v", p.Samples, p.Enrolled)
+	}
+	if len(p.Vec) != 2 {
+		t.Errorf("the voiceprint was dropped: %v", p.Vec)
+	}
+}
+
+// A clean row is not damaged. Without this the flag could be set always and
+// the test above would still pass.
+func TestCleanProfileIsNotDamaged(t *testing.T) {
+	r, _ := newRec(t, &fakeEmbedder{vec: []float32{1, 0}})
+	ctx := context.Background()
+	if _, err := r.Enroll(ctx, "kami", "Ками", enrolSamples(3, 4)); err != nil {
+		t.Fatal(err)
+	}
+	ps, err := r.List(ctx)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(ps) != 1 || ps[0].Damaged {
+		t.Fatalf("List = %+v, want one undamaged profile", ps)
 	}
 }