diff --git a/cmd/mavend/speaker.go b/cmd/mavend/speaker.go index 4585908..f717400 100644 --- a/cmd/mavend/speaker.go +++ b/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), diff --git a/cmd/mavend/speaker_wiring_test.go b/cmd/mavend/speaker_wiring_test.go new file mode 100644 index 0000000..96fd8bd --- /dev/null +++ b/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) + } +} diff --git a/internal/ipc/api.go b/internal/ipc/api.go index d3faccb..13f4381 100644 --- a/internal/ipc/api.go +++ b/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. diff --git a/internal/speaker/recognizer.go b/internal/speaker/recognizer.go index 73b3776..d881de5 100644 --- a/internal/speaker/recognizer.go +++ b/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 -} diff --git a/internal/speaker/speaker.go b/internal/speaker/speaker.go index 25f880b..2fc9b28 100644 --- a/internal/speaker/speaker.go +++ b/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:"-"` diff --git a/internal/speaker/speaker_test.go b/internal/speaker/speaker_test.go index 6c9ed77..5af12b1 100644 --- a/internal/speaker/speaker_test.go +++ b/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) } }