From 87d03cf8c6d3d46d53f41b5223a737fc30957de4 Mon Sep 17 00:00:00 2001 From: kami Date: Sat, 1 Aug 2026 14:11:24 +0400 Subject: [PATCH 01/11] mcp: bound and abandon transport reads The stdio reader ran inline under the transport lock, and bufio never observes a context. A server that accepted a request and then wrote nothing held that lock forever. alive() takes the same lock and Refresh calls alive() while holding the manager lock, so one mute python server wedged Tools, Status and every Call, including turns that touch no MCP tool at all. The read now runs on its own goroutine feeding a channel, the call selects on the context, and a call that gives up drops the connection so the manager re-dials. The frame bound was measured after the line had been assembled, which is not a bound. A server emitting 500 MB with no newline had all 500 MB in mavend before the check could reject it, which on the deploy target is an OOM kill of the core daemon. The scanner's own buffer limit enforces it now. The HTTP transport never checked the response id. A server request sent mid-stream, sampling/createMessage or roots/list, unmarshalled into a response with neither result nor error, so the call reported success with an empty string. The act was logged as done and the tool never ran. The id must match and the frame must carry a result or an error. Found in review of #70. --- internal/mcp/http.go | 52 +++++++++--- internal/mcp/stdio.go | 162 ++++++++++++++++++++++++++----------- internal/mcp/stdio_test.go | 67 +++++++++++++++ 3 files changed, 223 insertions(+), 58 deletions(-) diff --git a/internal/mcp/http.go b/internal/mcp/http.go index a717266..c4d96e4 100644 --- a/internal/mcp/http.go +++ b/internal/mcp/http.go @@ -35,13 +35,14 @@ type PostResponse struct { type httpTransport struct { poster Poster url string + extra map[string]string // static headers, e.g. an Authorization bearer mu sync.Mutex session string // Mcp-Session-Id, echoed back when the server issues one } -func newHTTPTransport(post Poster, endpoint string) *httpTransport { - return &httpTransport{poster: post, url: endpoint} +func newHTTPTransport(post Poster, endpoint string, extra map[string]string) *httpTransport { + return &httpTransport{poster: post, url: endpoint, extra: extra} } func (t *httpTransport) Call(ctx context.Context, req *rpcRequest) (*rpcResponse, error) { @@ -49,7 +50,7 @@ func (t *httpTransport) Call(ctx context.Context, req *rpcRequest) (*rpcResponse if err != nil { return nil, err } - frame, err := decodeFrame(body) + frame, err := decodeFrame(body, req.ID) if err != nil { return nil, err } @@ -57,6 +58,18 @@ func (t *httpTransport) Call(ctx context.Context, req *rpcRequest) (*rpcResponse if err := json.Unmarshal(frame, &resp); err != nil { return nil, fmt.Errorf("mcp: decode response: %w", err) } + // The id check the stdio transport already did. Without it a server that + // sends a request of its own (sampling/createMessage, roots/list) mid-stream + // has that request accepted as the answer: it unmarshals into an rpcResponse + // with neither result nor error, and the call reports success with nothing + // in it. An empty string and no error is the one answer that lies — the act + // is logged as done and the tool never ran. + if resp.ID == nil || *resp.ID != req.ID { + return nil, fmt.Errorf("mcp: response id mismatch (wanted %d)", req.ID) + } + if resp.Error == nil && len(resp.Result) == 0 { + return nil, errors.New("mcp: response carries neither result nor error") + } return &resp, nil } @@ -71,7 +84,14 @@ func (t *httpTransport) send(ctx context.Context, req *rpcRequest) ([]byte, erro if err != nil { return nil, err } - hdr := map[string]string{"Accept": "application/json, text/event-stream"} + hdr := map[string]string{} + // Configured headers first, so nothing here can be overwritten by them: + // a real remote server needs a bearer token, and the Vikunja one on + // loopback is only reachable without one because it is unauthenticated. + for k, v := range t.extra { + hdr[k] = v + } + hdr["Accept"] = "application/json, text/event-stream" t.mu.Lock() if t.session != "" { hdr["Mcp-Session-Id"] = t.session @@ -114,9 +134,10 @@ func headerGet(h map[string]string, key string) string { } // decodeFrame pulls the JSON object out of a body that is either raw JSON or -// SSE. For SSE we take the LAST data: payload that parses, which is the -// response — earlier frames on the stream are progress notifications. -func decodeFrame(body []byte) ([]byte, error) { +// SSE. For SSE we take the last data: payload that parses AND carries our own +// id with a result or an error in it. Matching on the presence of an "id" key +// alone is not enough: a JSON-RPC request from the server has one too. +func decodeFrame(body []byte, id int64) ([]byte, error) { trimmed := bytes.TrimSpace(body) if len(trimmed) == 0 { return nil, errors.New("mcp: empty response body") @@ -136,19 +157,28 @@ func decodeFrame(body []byte) ([]byte, error) { if payload == "" { continue } - var probe map[string]json.RawMessage + var probe struct { + ID *int64 `json:"id"` + Result json.RawMessage `json:"result"` + Error json.RawMessage `json:"error"` + Method string `json:"method"` + } if json.Unmarshal([]byte(payload), &probe) != nil { continue } - if _, isResp := probe["id"]; isResp { - last = []byte(payload) + if probe.Method != "" || probe.ID == nil || *probe.ID != id { + continue } + if len(probe.Result) == 0 && len(probe.Error) == 0 { + continue + } + last = []byte(payload) } if err := sc.Err(); err != nil { return nil, fmt.Errorf("mcp: read event stream: %w", err) } if last == nil { - return nil, errors.New("mcp: no JSON-RPC response in event stream") + return nil, fmt.Errorf("mcp: no JSON-RPC response for id %d in event stream", id) } return last, nil } diff --git a/internal/mcp/stdio.go b/internal/mcp/stdio.go index d597875..86e60e3 100644 --- a/internal/mcp/stdio.go +++ b/internal/mcp/stdio.go @@ -2,6 +2,7 @@ package mcp import ( "bufio" + "bytes" "context" "encoding/json" "errors" @@ -9,12 +10,17 @@ import ( "io" "os" "os/exec" - "strings" "sync" ) // maxLine bounds one JSON-RPC frame from a subprocess. A tool result bigger // than this is a misbehaving server, not something to buffer. +// +// The bound is enforced by bufio.Scanner's own buffer limit, not by measuring +// the line after it was assembled. Measuring afterwards is not a bound: a +// server that emits 500 MB with no newline would have all 500 MB in mavend's +// heap before the check could reject it, which on the deploy target is an OOM +// kill of the core daemon. const maxLine = 1 << 20 // 1 MiB // stdioTransport speaks newline-delimited JSON-RPC to a child process. This is @@ -23,12 +29,27 @@ const maxLine = 1 << 20 // 1 MiB // // Args are argv, never a shell string — the same discipline internal/tool // keeps, for the same reason. +// +// Reading happens on its own goroutine, feeding frames down a channel. That is +// what makes a call abandonable: bufio never observes a context, so a server +// that accepts a request and then writes nothing at all would otherwise block +// the reader forever with the transport lock held, and every other server in +// the manager behind it. type stdioTransport struct { - mu sync.Mutex - cmd *exec.Cmd - in io.WriteCloser - out *bufio.Reader - dead bool + cmd *exec.Cmd + in io.WriteCloser + lines chan []byte + stop chan struct{} // closed by Close, so the reader can give up + + // callMu serialises whole calls, so two callers cannot consume each + // other's frames off the shared channel. It is deliberately NOT the lock + // alive() takes: a hung call must not make the manager's health check + // block on it. + callMu sync.Mutex + + mu sync.Mutex + dead bool + readErr error } func newStdioTransport(ctx context.Context, argv []string, env []string, dir string) (*stdioTransport, error) { @@ -52,38 +73,92 @@ func newStdioTransport(ctx context.Context, argv []string, env []string, dir str if err := cmd.Start(); err != nil { return nil, fmt.Errorf("mcp: start %q: %w", argv[0], err) } - return &stdioTransport{cmd: cmd, in: in, out: bufio.NewReaderSize(out, 64<<10)}, nil + t := &stdioTransport{ + cmd: cmd, + in: in, + lines: make(chan []byte), + stop: make(chan struct{}), + } + go t.readLoop(out) + return t, nil +} + +// readLoop pushes one frame per line onto t.lines until the pipe ends. The +// scanner's own buffer limit is the frame bound: a line longer than maxLine +// ends the scan with bufio.ErrTooLong having buffered at most maxLine, rather +// than assembling the whole thing first and rejecting it afterwards. +func (t *stdioTransport) readLoop(out io.Reader) { + defer close(t.lines) + sc := bufio.NewScanner(out) + sc.Buffer(make([]byte, 0, 64<<10), maxLine) + for sc.Scan() { + line := bytes.TrimSpace(sc.Bytes()) + if len(line) == 0 { + continue + } + frame := append([]byte(nil), line...) + select { + case t.lines <- frame: + case <-t.stop: + return + } + } + err := sc.Err() + switch { + case errors.Is(err, bufio.ErrTooLong): + err = fmt.Errorf("mcp: frame exceeds %d bytes", maxLine) + case err == nil: + err = io.EOF + } + t.mu.Lock() + t.readErr = err + t.mu.Unlock() } func (t *stdioTransport) Call(ctx context.Context, req *rpcRequest) (*rpcResponse, error) { - t.mu.Lock() - defer t.mu.Unlock() - if t.dead { + t.callMu.Lock() + defer t.callMu.Unlock() + if !t.alive() { return nil, ErrClosed } - if err := t.write(req); err != nil { - t.dead = true + t.mu.Lock() + err := t.write(req) + t.mu.Unlock() + if err != nil { + _ = t.Close() return nil, err } // Read until the frame with our id turns up; anything else on the pipe is // a notification or a server-initiated request we do not answer. for { - if err := ctx.Err(); err != nil { - return nil, err + select { + case <-ctx.Done(): + // A server that took the request and answered nothing is not a + // server this connection can be reused with: the next call would + // read into a pipe whose state we no longer know. Drop it and let + // the manager re-dial. + _ = t.Close() + return nil, ctx.Err() + case line, ok := <-t.lines: + if !ok { + t.mu.Lock() + rerr := t.readErr + t.mu.Unlock() + _ = t.Close() + if rerr == nil { + rerr = ErrClosed + } + return nil, fmt.Errorf("mcp: read: %w", rerr) + } + var resp rpcResponse + if err := json.Unmarshal(line, &resp); err != nil { + continue // not a response frame; ignore rather than break the turn + } + if resp.ID == nil || *resp.ID != req.ID { + continue + } + return &resp, nil } - line, err := t.readLine() - if err != nil { - t.dead = true - return nil, err - } - var resp rpcResponse - if err := json.Unmarshal(line, &resp); err != nil { - continue // not a response frame; ignore rather than break the turn - } - if resp.ID == nil || *resp.ID != req.ID { - continue - } - return &resp, nil } } @@ -96,6 +171,7 @@ func (t *stdioTransport) Notify(ctx context.Context, method string, params any) return t.write(&rpcRequest{JSONRPC: "2.0", Method: method, Params: params}) } +// write must be called with t.mu held. func (t *stdioTransport) write(req *rpcRequest) error { req.JSONRPC = "2.0" raw, err := json.Marshal(req) @@ -108,31 +184,20 @@ func (t *stdioTransport) write(req *rpcRequest) error { return nil } -func (t *stdioTransport) readLine() ([]byte, error) { - for { - line, err := t.out.ReadString('\n') - if err != nil { - if len(strings.TrimSpace(line)) == 0 { - return nil, fmt.Errorf("mcp: read: %w", err) - } - return []byte(line), nil - } - if len(line) > maxLine { - return nil, fmt.Errorf("mcp: frame exceeds %d bytes", maxLine) - } - if s := strings.TrimSpace(line); s != "" { - return []byte(s), nil - } - } -} - +// Close is idempotent: a call that abandoned a silent pipe calls it, and so +// does the manager. func (t *stdioTransport) Close() error { t.mu.Lock() - defer t.mu.Unlock() + if t.dead { + t.mu.Unlock() + return nil + } t.dead = true + close(t.stop) if t.in != nil { _ = t.in.Close() } + t.mu.Unlock() if t.cmd.Process != nil { _ = t.cmd.Process.Kill() _ = t.cmd.Wait() @@ -145,5 +210,8 @@ func (t *stdioTransport) Close() error { func (t *stdioTransport) alive() bool { t.mu.Lock() defer t.mu.Unlock() - return !t.dead + if t.dead { + return false + } + return t.readErr == nil } diff --git a/internal/mcp/stdio_test.go b/internal/mcp/stdio_test.go index 8e43b59..b75b47d 100644 --- a/internal/mcp/stdio_test.go +++ b/internal/mcp/stdio_test.go @@ -9,6 +9,7 @@ import ( "os/exec" "strings" "testing" + "time" ) // The stdio transport is tested against a real subprocess — this test binary, @@ -47,6 +48,20 @@ func fakeStdioServer() { _ = out.Flush() continue } + // "mute" answers the handshake and then goes silent: a python server + // that hit an unhandled exception in its own read loop but did not + // exit is the ordinary way to get here. + if os.Getenv("MAVEN_MCP_FAKE") == "mute" && req.Method == "tools/call" { + select {} // never answer, never exit + } + // "flood" writes one enormous line with no newline in it. + if os.Getenv("MAVEN_MCP_FAKE") == "flood" && req.Method == "tools/call" { + for i := 0; i < 64; i++ { + _, _ = out.Write(make([]byte, 1<<20)) + } + _ = out.Flush() + continue + } result, rerr := h(req.Method, req.Params) resp := map[string]any{"jsonrpc": "2.0", "id": *req.ID} if rerr != nil { @@ -147,3 +162,55 @@ func TestStdioMissingCommand(t *testing.T) { t.Logf("err = %q", st[0].Err) } } + +// A stdio server that accepts a call and then answers nothing must not wedge +// the manager. Before the read moved onto its own goroutine, the read held the +// transport lock, Refresh took that lock through alive() while holding the +// manager lock, and from then on Tools, Status and Call blocked for EVERY +// server — including turns that touch no MCP tool at all. +func TestStdioSilentServerDoesNotWedgeTheManager(t *testing.T) { + m := stdioManager(t, "mute") + defer m.Close() + + ctx, cancel := context.WithTimeout(context.Background(), 500*time.Millisecond) + defer cancel() + done := make(chan error, 1) + go func() { + _, err := m.Call(ctx, "fake", "read_thing", nil) + done <- err + }() + select { + case err := <-done: + if err == nil { + t.Fatal("a call into a silent server must fail, not succeed") + } + case <-time.After(5 * time.Second): + t.Fatal("the call never returned: the context is not observed during the read") + } + + // The manager must still answer while (and after) that call was stuck. + ready := make(chan struct{}) + go func() { + m.Refresh(context.Background()) + m.Tools() + m.Status() + close(ready) + }() + select { + case <-ready: + case <-time.After(5 * time.Second): + t.Fatal("Refresh/Tools/Status deadlocked behind the hung call") + } +} + +// One frame is bounded by the reader's buffer, not measured after the whole +// thing has already been assembled in mavend's heap. +func TestStdioOversizedFrameIsRefused(t *testing.T) { + m := stdioManager(t, "flood") + defer m.Close() + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + if _, err := m.Call(ctx, "fake", "read_thing", nil); err == nil { + t.Fatal("a 64 MiB frame must be refused") + } +} From 5e0417306b1eb9159268eb940c285c427509f4e9 Mon Sep 17 00:00:00 2001 From: kami Date: Sat, 1 Aug 2026 14:11:39 +0400 Subject: [PATCH 02/11] mcp: guard the connection, not just the first dial Refresh called alive() with the manager lock held, so a slow health check blocked every other server. It now snapshots the candidates and asks outside the lock. A server that cannot be dialled was retried every minute forever, which for a misconfigured stdio block means re-exec'ing a process 1440 times a day. Dials now back off from one minute to thirty. An allow_private fetcher followed redirects. A LAN MCP endpoint could answer a POST with a redirect to 169.254.169.254 and the guard would go there, because allow_private is what turns the address check off. Redirects are refused outright on that door. The tool catalogue was trimmed by taking the first max_tools entries of whatever order the server sent, so the server chose which of its tools Maven proposed. Over the cap without allow_tools now contributes nothing: refusing is honest, silently keeping the server's pick is not. Descriptions are server-written text that lands in the router prompt and on /tools, so they are capped too. A server block with enabled false was skipped by validation, so a typo in a block written dark surfaced only on the day it was switched on. All blocks are shape-checked now. Configured static headers carry the bearer token a real remote server needs, and host_interval bounds how fast one endpoint is polled. Found in review of #70. --- internal/config/config.go | 57 ++++++++++-- internal/config/mcp_test.go | 47 +++++++++- internal/mcp/manager.go | 173 +++++++++++++++++++++++++++++------ internal/mcp/mcp_test.go | 94 +++++++++++++++++-- internal/mcp/webfetchdoor.go | 10 ++ 5 files changed, 337 insertions(+), 44 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index 0124e1a..84af890 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -273,8 +273,20 @@ type MCPConfig struct { // MaxBytes — cap on one JSON-RPC response. 0 ⇒ webfetch.DefaultMaxBytes. MaxBytes int64 `json:"max_bytes,omitempty"` + + // HostInterval — minimum spacing between two requests to one MCP server. + // 0 ⇒ DefaultMCPHostInterval (50ms), NOT webfetch's own one-second default. + // That default was sized for a feed poll loop, and this path is in a spoken + // turn: one dial is three requests (initialize, initialized, tools/list), + // so a second of spacing is two seconds of pure sleeping per dial and up to + // another second before every tools/call leaves the box. + HostInterval Duration `json:"host_interval,omitempty"` } +// DefaultMCPHostInterval — see MCPConfig.HostInterval. Enough to stop a +// runaway loop hammering a server, small enough not to be heard. +const DefaultMCPHostInterval = 50 * time.Millisecond + // SmartHomeConfig — the Home Assistant block (Vikunja #256). Dark until // `"enabled": true`, and even then a discovered device is only ever PROPOSED // into the act allowlist: Kami enables it on /tools, behind step-up, exactly as @@ -416,6 +428,13 @@ type MCPServerConfig struct { // Timeout — per-call budget for this server. 0 ⇒ MCPConfig.Timeout. Timeout Duration `json:"timeout,omitempty"` + // Headers — sent verbatim on every request to a url server. This is how a + // bearer token reaches a real remote MCP server: {"Authorization": "Bearer + // ${MCP_TOKEN}"}, with the value in the gitignored env file like the + // telegram credentials. The Vikunja server on homesrv needs none only + // because it is unauthenticated on loopback. + Headers map[string]string `json:"headers,omitempty"` + // Enabled — false (the default) keeps a configured server described but // dark, so a block can be written and reviewed before it is switched on. Enabled bool `json:"enabled,omitempty"` @@ -424,13 +443,31 @@ type MCPServerConfig struct { // MCPServers maps the config blocks onto the mcp package's own type. It lives // here so config validation and daemon wiring cannot drift on the mapping. // Returns nil when nothing is configured or nothing is enabled. +// +// Disabled servers are dropped here, which is why validation does NOT use this +// list — see allMCPServers. func (c *Config) MCPServers() []mcp.ServerConfig { + return c.mcpServers(true) +} + +// allMCPServers is every configured server, enabled or not, for validation. +// +// Validating only the enabled ones meant a block with both command and url, or +// a bare hostname as the url, passed startup validation while it was dark. The +// doc on Enabled says a block can be written and reviewed before it is switched +// on; the review the config layer could give was the one thing skipped. Enabled +// gates the dialing, not the shape check. +func (c *Config) allMCPServers() []mcp.ServerConfig { + return c.mcpServers(false) +} + +func (c *Config) mcpServers(onlyEnabled bool) []mcp.ServerConfig { if c.MCP == nil { return nil } out := make([]mcp.ServerConfig, 0, len(c.MCP.Servers)) for _, s := range c.MCP.Servers { - if !s.Enabled { + if onlyEnabled && !s.Enabled { continue } timeout := time.Duration(s.Timeout) @@ -447,8 +484,9 @@ func (c *Config) MCPServers() []mcp.ServerConfig { AllowPrivate: s.AllowPrivate, AllowTools: s.AllowTools, MaxTools: s.MaxTools, + Headers: s.Headers, Timeout: timeout, - Enabled: true, + Enabled: s.Enabled, }) } if len(out) == 0 { @@ -1236,11 +1274,18 @@ func (c *Config) applyDefaults() { c.Feeds = nil } - // Same rule for MCP: a block with no server, or none enabled, is the same - // as no block at all. Normalising it to nil keeps "off" in one place. - if c.MCP != nil && len(c.MCPServers()) == 0 { + // Same rule for MCP: a block with no server at all is the same as no block. + // A block whose servers are all disabled is NOT normalised away, because + // validate has to see their shape — a dark block with a typo in it should + // fail at startup, which is the whole reason it can be written before it is + // switched on. wireMCP builds nothing when nothing is enabled, so "off" + // still holds. + if c.MCP != nil && len(c.MCP.Servers) == 0 { c.MCP = nil } + if c.MCP != nil && c.MCP.HostInterval <= 0 { + c.MCP.HostInterval = Duration(DefaultMCPHostInterval) + } // Same rule for the house: a block that is not enabled is the same as no // block at all, so "off" stays in one place. @@ -1363,7 +1408,7 @@ func (c *Config) validate() error { // An MCP block with a typo (no name, both command and url, a bare hostname // as the url) fails here, at startup, rather than at the first turn that // needed the tool. - if err := mcp.Validate(c.MCPServers()); err != nil { + if err := mcp.Validate(c.allMCPServers()); err != nil { return err } // Same for the house: a missing token or a bare hostname fails at startup, diff --git a/internal/config/mcp_test.go b/internal/config/mcp_test.go index 48e05e2..e11de02 100644 --- a/internal/config/mcp_test.go +++ b/internal/config/mcp_test.go @@ -26,14 +26,55 @@ func TestMCPDisabledServerIsOff(t *testing.T) { if err != nil { t.Fatal(err) } - if c.MCP != nil { - t.Errorf("a block with nothing enabled must normalise to nil, got %+v", c.MCP) - } if got := c.MCPServers(); len(got) != 0 { t.Errorf("MCPServers() = %+v", got) } } +// A block with no servers at all is the same as no block. +func TestMCPEmptyBlockNormalisesToNil(t *testing.T) { + c, err := Load(writeConfig(t, `{"mcp":{"servers":[]}}`)) + if err != nil { + t.Fatal(err) + } + if c.MCP != nil { + t.Errorf("mcp = %+v, want nil", c.MCP) + } +} + +// A server that is written but not switched on is still shape-checked. The +// review the config layer can give is the point of writing a block dark, and +// skipping it meant a typo only surfaced on the day it was enabled. +func TestMCPDisabledServerIsStillValidated(t *testing.T) { + cases := map[string]string{ + "both": `{"mcp":{"servers":[{"name":"a","command":"x","url":"http://a.test"}]}}`, + "bare host": `{"mcp":{"servers":[{"name":"a","url":"a.test"}]}}`, + "no name": `{"mcp":{"servers":[{"command":"x"}]}}`, + "duplicates": `{"mcp":{"servers":[{"name":"a","command":"x"},{"name":"a","command":"y"}]}}`, + } + for name, body := range cases { + t.Run(name, func(t *testing.T) { + if _, err := Load(writeConfig(t, body)); err == nil { + t.Fatal("a dark server with a typo must fail at startup") + } + }) + } +} + +// Headers carry a bearer token to a real remote server. +func TestMCPServerHeaders(t *testing.T) { + c, err := Load(writeConfig(t, `{"mcp":{"servers":[ + {"name":"remote","url":"https://mcp.example.test/mcp","enabled":true, + "headers":{"Authorization":"Bearer sekret"}}]}}`)) + if err != nil { + t.Fatal(err) + } + got := c.MCPServers() + if len(got) != 1 || got[0].Headers["Authorization"] != "Bearer sekret" { + t.Fatalf("headers not mapped: %+v", got) + } +} + func TestMCPEnabledServerMapping(t *testing.T) { c, err := Load(writeConfig(t, `{"mcp":{ "timeout":"5s", diff --git a/internal/mcp/manager.go b/internal/mcp/manager.go index cec97e9..d8a7dc4 100644 --- a/internal/mcp/manager.go +++ b/internal/mcp/manager.go @@ -24,12 +24,31 @@ const ( // wrong act. Twelve per server is already generous. DefaultMaxTools = 12 // DefaultReconnectEvery is how long the manager waits before re-dialing a - // server whose connection died. + // server whose connection died. It is the FIRST wait: every consecutive + // failure doubles it, up to MaxReconnectEvery. DefaultReconnectEvery = 30 * time.Second + // MaxReconnectEvery caps the backoff. Without one, a permanently + // misconfigured stdio server is exec'd once a minute forever, which is a + // process spawn per minute in the logs and nothing that ever gets better. + MaxReconnectEvery = 30 * time.Minute + // DefaultMaxDescription bounds one tool description. It is written by a + // server Maven does not control and it lands in two places that cannot + // absorb an arbitrary blob: the resident model's 4096-token context, and a + // table cell on /tools. + DefaultMaxDescription = 400 ) -// ErrNoServer — the named server is not configured or not connected. -var ErrNoServer = errors.New("mcp: no such server") +var ( + // ErrNoServer — the named server is not configured. + ErrNoServer = errors.New("mcp: no such server") + // ErrNotConnected — the server is configured but nothing is dialed. Held + // apart from ErrNoServer so a caller can say "that tool is not connected" + // instead of drafting a proposal for a tool that already exists. + ErrNotConnected = errors.New("mcp: server is not connected") + // ErrToolGone — the server no longer offers this tool. An enabled row can + // outlive the tool it names; this is what the act path sees when it does. + ErrToolGone = errors.New("mcp: server no longer offers this tool") +) // ServerConfig is one configured MCP server. Off unless present. // @@ -61,6 +80,10 @@ type ServerConfig struct { AllowTools []string `json:"allow_tools,omitempty"` // MaxTools caps the contribution (0 ⇒ DefaultMaxTools). MaxTools int `json:"max_tools,omitempty"` + // Headers are sent verbatim on every request to a url server. This is how + // a bearer token reaches a real remote server; the Vikunja one on loopback + // needs none only because it is unauthenticated. + Headers map[string]string `json:"-"` // Timeout bounds one call (0 ⇒ DefaultTimeout). Timeout time.Duration `json:"-"` // Enabled=false keeps a configured server described but dark. @@ -90,6 +113,20 @@ type conn struct { lastErr error lastTry time.Time dialedAt time.Time + fails int // consecutive dial failures, for the backoff +} + +// backoff is how long this connection waits before the next dial attempt: +// DefaultReconnectEvery doubled per consecutive failure, capped. +func (c *conn) backoff() time.Duration { + d := DefaultReconnectEvery + for i := 1; i < c.fails && d < MaxReconnectEvery; i++ { + d *= 2 + } + if d > MaxReconnectEvery { + d = MaxReconnectEvery + } + return d } // NewManager builds a manager for the enabled servers in cfgs. newPoster is @@ -202,7 +239,7 @@ func (m *Manager) dial(ctx context.Context, name string) error { } else { var poster Poster if poster, err = m.newPoster(cfg); err == nil { - tr = newHTTPTransport(poster, cfg.URL) + tr = newHTTPTransport(poster, cfg.URL, cfg.Headers) } } if err != nil { @@ -233,6 +270,7 @@ func (m *Manager) dial(ctx context.Context, name string) error { m.conns[name].client = cl m.conns[name].tools = tools m.conns[name].lastErr = nil + m.conns[name].fails = 0 m.conns[name].dialedAt = time.Now() m.mu.Unlock() log.Printf("mcp: %s connected (%s %s), %d tool(s)", name, cl.Info().Name, cl.Info().Version, len(tools)) @@ -246,12 +284,19 @@ func (m *Manager) fail(name string, err error) { c.lastErr = err c.client = nil c.tools = nil + c.fails++ } } -// filterTools applies AllowTools and MaxTools, and drops nameless entries. -// Sorted first, so the cap is deterministic rather than "whatever order the -// server felt like". +// filterTools applies AllowTools and MaxTools, drops nameless entries and +// truncates descriptions. +// +// Over the cap WITHOUT allow_tools, the whole contribution is dropped. Taking +// the first N of a sorted list was deterministic but it handed the choice of +// which N to the server: a thirteenth tool named "aaa_" would push a tool that +// had already been discovered, proposed and maybe enabled out of the +// catalogue. Determinism was not the property worth buying. With allow_tools +// set, Kami named the tools, so the cap trims a list he chose. func filterTools(cfg ServerConfig, in []Tool) []Tool { sort.Slice(in, func(i, j int) bool { return in[i].Name < in[j].Name }) out := make([]Tool, 0, len(in)) @@ -259,16 +304,32 @@ func filterTools(cfg ServerConfig, in []Tool) []Tool { if len(cfg.AllowTools) > 0 && !contains(cfg.AllowTools, t.Name) { continue } + t.Description = truncate(t.Description, DefaultMaxDescription) out = append(out, t) } if cfg.MaxTools > 0 && len(out) > cfg.MaxTools { - log.Printf("mcp: %s offers %d tools, taking the first %d (raise max_tools or set allow_tools)", - cfg.Name, len(out), cfg.MaxTools) + if len(cfg.AllowTools) == 0 { + log.Printf("mcp: %s offers %d tools, over the cap of %d — taking NONE of them, set allow_tools to choose or raise max_tools", + cfg.Name, len(out), cfg.MaxTools) + return nil + } + log.Printf("mcp: %s: allow_tools names %d tools, over the cap of %d — taking the first %d", + cfg.Name, len(out), cfg.MaxTools, cfg.MaxTools) out = out[:cfg.MaxTools] } return out } +// truncate bounds a server-written string. The ellipsis is there so a reader +// on /tools can tell the text was cut rather than written that way. +func truncate(s string, max int) string { + r := []rune(s) + if len(r) <= max { + return s + } + return strings.TrimSpace(string(r[:max])) + "…" +} + func contains(hay []string, needle string) bool { for _, h := range hay { if h == needle { @@ -281,18 +342,33 @@ func contains(hay []string, needle string) bool { // Refresh re-dials any server that is down, if enough time has passed since the // last attempt. Call it from the daemon's periodic tick — it is cheap when // everything is up. +// The health check itself is done OUTSIDE m.mu, the way Resources already +// does it. alive() reaches into the transport, and a transport waiting on a +// silent subprocess would otherwise hold m.mu for as long as it waits, which +// blocks Tools, Status and Call for every other server too. func (m *Manager) Refresh(ctx context.Context) { now := time.Now() - var stale []string + type candidate struct { + name string + cl *Client + ready bool + } + var cands []candidate m.mu.Lock() for _, name := range m.order { c := m.conns[name] - down := c.client == nil || !c.client.alive() - if down && now.Sub(c.lastTry) >= DefaultReconnectEvery { - stale = append(stale, name) - } + cands = append(cands, candidate{name: name, cl: c.client, ready: now.Sub(c.lastTry) >= c.backoff()}) } m.mu.Unlock() + var stale []string + for _, c := range cands { + if !c.ready { + continue + } + if c.cl == nil || !c.cl.alive() { + stale = append(stale, c.name) + } + } for _, name := range stale { if err := m.dial(ctx, name); err != nil { log.Printf("mcp: %s: reconnect: %v", name, err) @@ -312,6 +388,21 @@ func (m *Manager) Tools() []Tool { return out } +// Connected — the names of servers that are dialed right now. A caller that +// wants to act on a tool's ABSENCE needs this: a tool missing from Tools() +// because its server is down is not a tool the server withdrew. +func (m *Manager) Connected() []string { + m.mu.Lock() + defer m.mu.Unlock() + var out []string + for _, name := range m.order { + if m.conns[name].client != nil { + out = append(out, name) + } + } + return out +} + // Status is one server's health, for the web surface. type Status struct { Name string @@ -367,13 +458,13 @@ func (m *Manager) Call(ctx context.Context, server, tool string, args map[string } m.mu.Unlock() if cl == nil { - return "", fmt.Errorf("mcp: %s is not connected", server) + return "", fmt.Errorf("%w: %s", ErrNotConnected, server) } // The discovered-and-filtered set is the second allowlist: even an enabled // store row cannot reach a tool the server stopped offering, or one // allow_tools excludes. if !known { - return "", fmt.Errorf("mcp: %s offers no tool %q", server, tool) + return "", fmt.Errorf("%w: %s/%s", ErrToolGone, server, tool) } cctx, cancel := context.WithTimeout(ctx, timeout) defer cancel() @@ -442,8 +533,8 @@ var ErrNeedsArgs = errors.New("mcp: tool needs named arguments") // // - a tool with no required properties runs with no arguments (a spare tail // is ignored — "покажи проекты пожалуйста" should still list projects); -// - a READ-ONLY tool with exactly one required property, of type string or -// integer/number, gets the tail bound to it; +// - a READ-ONLY tool NAMED IN allow_tools, with exactly one required +// property, of type string or integer/number, gets the tail bound to it; // - anything else is refused with ErrNeedsArgs. Such a tool is still callable // with explicit arguments from the authed surface, where a human types // them. @@ -456,32 +547,54 @@ var ErrNeedsArgs = errors.New("mcp: tool needs named arguments") // partially-filled write can do is destroy what it did not mention. A mutating // tool with nothing required is still fine: nothing was guessed, and it still // goes through the confirm turn. +// +// The allow_tools condition is the second half, and it is there because +// readOnlyHint is the SERVER's claim about itself. It already buys one +// exemption (destructive=false, so no confirm turn); letting it buy argument +// binding as well means one lie converts a spoken utterance into an +// unconfirmed, argument-carrying write. A server advertising delete_project +// with readOnlyHint true and the description "show a project and its tasks" +// would be enough. So the binding half rests on something local instead: a +// name Kami typed into mavend.json. The tool name is not a defence — the +// router picks tools by name similarity and the description a human reads is +// server-written too. func (m *Manager) CallPositional(ctx context.Context, server, tool string, args []string) (string, error) { m.mu.Lock() c := m.conns[server] var schema json.RawMessage - found, readOnly := false, false + found, readOnly, bindable := false, false, false + configured, connected := c != nil, false if c != nil { + connected = c.client != nil for _, t := range c.tools { if t.Name == tool { schema, readOnly, found = t.InputSchema, t.ReadOnly, true + bindable = contains(c.cfg.AllowTools, tool) break } } } m.mu.Unlock() if !found { - return "", fmt.Errorf("mcp: %s offers no tool %q", server, tool) + if !configured { + return "", fmt.Errorf("%w: %s", ErrNoServer, server) + } + if !connected { + return "", fmt.Errorf("%w: %s", ErrNotConnected, server) + } + return "", fmt.Errorf("%w: %s/%s", ErrToolGone, server, tool) } - named, err := bindPositional(schema, args, readOnly) + named, err := bindPositional(schema, args, readOnly && bindable) if err != nil { return "", err } return m.Call(ctx, server, tool, named) } -// bindPositional implements the rule documented on CallPositional. -func bindPositional(schema json.RawMessage, args []string, readOnly bool) (map[string]any, error) { +// bindPositional implements the rule documented on CallPositional. bind is the +// caller's verdict on whether a guessed argument is allowed at all: read-only +// AND named in allow_tools. +func bindPositional(schema json.RawMessage, args []string, bind bool) (map[string]any, error) { var s struct { Required []string `json:"required"` Properties map[string]struct { @@ -498,14 +611,20 @@ func bindPositional(schema json.RawMessage, args []string, readOnly bool) (map[s return map[string]any{}, nil case 1: name := s.Required[0] - if !readOnly { - return nil, fmt.Errorf("%w: %q, and a tool that writes never gets a guessed one", ErrNeedsArgs, name) + prop, described := s.Properties[name] + if !described { + // required names it, properties does not describe it. The zero + // value would make it a string, which is a guess about a guess. + return nil, fmt.Errorf("%w: %q, which the schema never describes", ErrNeedsArgs, name) + } + if !bind { + return nil, fmt.Errorf("%w: %q, and a guessed argument goes only to a read-only tool named in allow_tools", ErrNeedsArgs, name) } tail := strings.TrimSpace(strings.Join(args, " ")) if tail == "" { return nil, fmt.Errorf("%w: %q", ErrNeedsArgs, name) } - switch s.Properties[name].Type { + switch prop.Type { case "string", "": return map[string]any{name: tail}, nil case "integer", "number": @@ -515,7 +634,7 @@ func bindPositional(schema json.RawMessage, args []string, readOnly bool) (map[s } return map[string]any{name: n}, nil default: - return nil, fmt.Errorf("%w: %q is a %s", ErrNeedsArgs, name, s.Properties[name].Type) + return nil, fmt.Errorf("%w: %q is a %s", ErrNeedsArgs, name, prop.Type) } default: return nil, fmt.Errorf("%w: %s", ErrNeedsArgs, strings.Join(s.Required, ", ")) diff --git a/internal/mcp/mcp_test.go b/internal/mcp/mcp_test.go index 0888e25..9c8c966 100644 --- a/internal/mcp/mcp_test.go +++ b/internal/mcp/mcp_test.go @@ -3,6 +3,7 @@ package mcp import ( "context" "encoding/json" + "errors" "fmt" "strings" "sync" @@ -102,7 +103,7 @@ func echoServer() func(string, json.RawMessage) (any, *rpcError) { func dialFake(t *testing.T, p *fakePoster) *Client { t.Helper() - c := newClient("fake", newHTTPTransport(p, "http://example.test/mcp")) + c := newClient("fake", newHTTPTransport(p, "http://example.test/mcp", nil)) if err := c.Initialize(context.Background()); err != nil { t.Fatalf("initialize: %v", err) } @@ -190,7 +191,7 @@ func TestResources(t *testing.T) { } func TestCallBeforeInitializeRefused(t *testing.T) { - c := newClient("fake", newHTTPTransport(&fakePoster{handler: echoServer()}, "http://example.test/mcp")) + c := newClient("fake", newHTTPTransport(&fakePoster{handler: echoServer()}, "http://example.test/mcp", nil)) if _, err := c.CallTool(context.Background(), "read_thing", nil); err != ErrNotInitialized { t.Fatalf("err = %v, want ErrNotInitialized", err) } @@ -217,7 +218,7 @@ func TestHandshakeWithoutProtocolVersionRefused(t *testing.T) { p := &fakePoster{handler: func(m string, _ json.RawMessage) (any, *rpcError) { return map[string]any{"serverInfo": map[string]any{"name": "not-mcp"}}, nil }} - c := newClient("x", newHTTPTransport(p, "http://example.test/mcp")) + c := newClient("x", newHTTPTransport(p, "http://example.test/mcp", nil)) if err := c.Initialize(context.Background()); err == nil { t.Fatal("a reply with no protocolVersion is not an MCP server") } @@ -245,8 +246,8 @@ func TestDecodeFrame(t *testing.T) { name, in, want string wantErr bool }{ - {name: "plain json", in: `{"id":1,"result":{}}`, want: `{"id":1,"result":{}}`}, - {name: "sse single", in: "event: message\ndata: {\"id\":1,\"result\":1}\n\n", want: `{"id":1,"result":1}`}, + {name: "plain json", in: `{"id":2,"result":{}}`, want: `{"id":2,"result":{}}`}, + {name: "sse single", in: "event: message\ndata: {\"id\":2,\"result\":1}\n\n", want: `{"id":2,"result":1}`}, { name: "sse picks the response not the notification", in: "data: {\"method\":\"notifications/progress\"}\n\ndata: {\"id\":2,\"result\":2}\n\n", @@ -254,10 +255,24 @@ func TestDecodeFrame(t *testing.T) { }, {name: "empty", in: " ", wantErr: true}, {name: "sse with no response", in: "data: {\"method\":\"x\"}\n\n", wantErr: true}, + { + // A JSON-RPC REQUEST from the server has an id too. Taking it as + // the response gave a frame with neither result nor error, which + // the client reported as an empty success: the act logged as done + // and the tool never run. + name: "sse server request is not a response", + in: "data: {\"id\":2,\"method\":\"sampling/createMessage\",\"params\":{}}\n\n", + wantErr: true, + }, + { + name: "sse response for another id", + in: "data: {\"id\":9,\"result\":1}\n\n", + wantErr: true, + }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - got, err := decodeFrame([]byte(tc.in)) + got, err := decodeFrame([]byte(tc.in), 2) if tc.wantErr { if err == nil { t.Fatalf("want error, got %q", got) @@ -368,10 +383,19 @@ func TestManagerAllowToolsAndMaxTools(t *testing.T) { if _, err := m.Call(context.Background(), "fake", "break_thing", nil); err == nil { t.Error("a tool excluded by allow_tools must be unreachable") } + // Over the cap with no allow_tools: NOTHING is taken. Trimming a sorted + // list handed the server the choice of which tools survive — a new tool + // named "aaa_" would push an already-approved one out of the catalogue. m2 := mk(ServerConfig{MaxTools: 1}) defer m2.Close() - if got := m2.Tools(); len(got) != 1 || got[0].Name != "break_thing" { - t.Fatalf("max_tools should keep the first name-sorted tool: %+v", got) + if got := m2.Tools(); len(got) != 0 { + t.Fatalf("over the cap without allow_tools must contribute nothing, got %+v", got) + } + // With allow_tools, Kami chose the list, so the cap trims his list. + m3 := mk(ServerConfig{MaxTools: 1, AllowTools: []string{"break_thing", "read_thing"}}) + defer m3.Close() + if got := m3.Tools(); len(got) != 1 || got[0].Name != "break_thing" { + t.Fatalf("max_tools over allow_tools: %+v", got) } } @@ -563,3 +587,57 @@ func TestCallPositionalThroughManager(t *testing.T) { t.Error("an unknown tool must be refused") } } + +// A guessed argument may only be bound for a tool Kami named in allow_tools. +// readOnlyHint alone was the old rule, and readOnlyHint is written by the same +// server that named the tool: a server advertising delete_project as read-only +// got an unconfirmed argument-carrying call. +func TestBindPositionalNeedsAllowTools(t *testing.T) { + schema := json.RawMessage(`{"required":["query"],"properties":{"query":{"type":"string"}}}`) + if _, err := bindPositional(schema, []string{"tea"}, false); !errors.Is(err, ErrNeedsArgs) { + t.Fatalf("err = %v, want ErrNeedsArgs when the tool is not in allow_tools", err) + } + got, err := bindPositional(schema, []string{"tea"}, true) + if err != nil || got["query"] != "tea" { + t.Fatalf("bind = %v, %v", got, err) + } +} + +// required names a property the schema never describes. Falling through to the +// zero value made it a string, which is a guess about a guess. +func TestBindPositionalRefusesUndescribedProperty(t *testing.T) { + schema := json.RawMessage(`{"required":["query"],"properties":{}}`) + _, err := bindPositional(schema, []string{"tea"}, true) + if !errors.Is(err, ErrNeedsArgs) { + t.Fatalf("err = %v, want ErrNeedsArgs", err) + } + if !strings.Contains(err.Error(), "never describes") { + t.Fatalf("err = %v, want it to name the schema gap", err) + } +} + +// A server that cannot be dialled must be retried more and more slowly. At a +// flat one minute a permanently misconfigured stdio server was re-exec'd 1440 +// times a day forever. +func TestReconnectBackoffGrows(t *testing.T) { + c := &conn{} + prev := time.Duration(0) + for i := 1; i <= 12; i++ { + c.fails = i + d := c.backoff() + if d < prev { + t.Fatalf("backoff shrank at %d failures: %v after %v", i, d, prev) + } + if d > MaxReconnectEvery { + t.Fatalf("backoff %v exceeds the cap %v", d, MaxReconnectEvery) + } + prev = d + } + if prev != MaxReconnectEvery { + t.Fatalf("backoff never reached the cap: %v", prev) + } + c.fails = 1 + if c.backoff() != DefaultReconnectEvery { + t.Fatalf("first retry = %v, want %v", c.backoff(), DefaultReconnectEvery) + } +} diff --git a/internal/mcp/webfetchdoor.go b/internal/mcp/webfetchdoor.go index 3a892a2..2623592 100644 --- a/internal/mcp/webfetchdoor.go +++ b/internal/mcp/webfetchdoor.go @@ -16,10 +16,20 @@ import ( // must not become a hole for some public endpoint that happens to redirect at // the LAN. Rate limiting is per fetcher too, which is the right shape here: // separate servers are separate hosts. +// +// A server WITH allow_private also gets redirects switched off. Across servers +// the per-fetcher split holds the line; within the one server that has the +// flag it did not, because allow_private disables the dialer guard on every +// hop: http://localhost:9100/mcp answering 302 to +// http://169.254.169.254/latest/meta-data/ was followed, up to MaxRedirects. A +// local MCP endpoint has no business redirecting, so refusing costs nothing. func WebfetchDoor(limits webfetch.Config) PosterFactory { return func(cfg ServerConfig) (Poster, error) { c := limits c.AllowPrivate = cfg.AllowPrivate + if cfg.AllowPrivate { + c.MaxRedirects = -1 // negative ⇒ no redirects followed + } if c.Timeout <= 0 && cfg.Timeout > 0 { c.Timeout = cfg.Timeout } From 52f56947bbf1941b77d036b6a0b2e90790e74980 Mon Sep 17 00:00:00 2001 From: kami Date: Sat, 1 Aug 2026 14:11:57 +0400 Subject: [PATCH 03/11] tool: separate a tool that is off from a backend that is down An enabled MCP or smarthome row with no backend returned ErrNotEnabled, and actionAct reads ErrNotEnabled as "this is unknown, draft a proposal". So a tool Kami had already approved, whose server happened to be restarting, produced a second proposal row and an answer saying the tool needs approval. The right answer is that the server is down. ErrNotConnected carries that, and the act path maps it, ErrNoServer and ErrToolGone to replies that say which of the three happened. Found in review of #71. --- cmd/mavend/actions_act.go | 7 +++++++ internal/tool/tool.go | 11 +++++++++-- internal/tool/tool_test.go | 15 +++++++++++---- 3 files changed, 27 insertions(+), 6 deletions(-) diff --git a/cmd/mavend/actions_act.go b/cmd/mavend/actions_act.go index be83e9e..2b96855 100644 --- a/cmd/mavend/actions_act.go +++ b/cmd/mavend/actions_act.go @@ -53,6 +53,13 @@ func (h *reactiveHandler) actionAct(ctx context.Context, dec router.Decision) st return "выполнить «" + phrase + "»? скажи «да» или «нет»." case errors.Is(err, tool.ErrNotEnabled): return h.proposeGap(ctx, dec) + case errors.Is(err, tool.ErrNotConnected), errors.Is(err, mcp.ErrNotConnected), errors.Is(err, mcp.ErrNoServer): + // The row is enabled and the backend is gone. Drafting a proposal + // for it (the ErrNotEnabled path) would be answering the wrong + // question. + return "этот инструмент включён, но сервер, который его выполняет, сейчас не подключён." + case errors.Is(err, mcp.ErrToolGone): + return "сервер больше не предлагает этот инструмент — я сняла его с разрешённых, посмотри на /tools." case errors.Is(err, mcp.ErrNeedsArgs): // An MCP tool that wants named arguments a spoken verb cannot // supply. Guessing them would be a wrong act, so she says so diff --git a/internal/tool/tool.go b/internal/tool/tool.go index 4c0165d..3b16c59 100644 --- a/internal/tool/tool.go +++ b/internal/tool/tool.go @@ -60,6 +60,13 @@ var ( ErrNotEnabled = errors.New("tool not on the enabled allowlist") // ErrNeedsConfirm — the fn is enabled but destructive; needs a confirm turn. ErrNeedsConfirm = errors.New("destructive tool needs confirmation") + // ErrNotConnected — the row is enabled and well formed, but the thing it + // dispatches to is not wired: the mcp block was dropped from the config + // while enabled MCP rows remained, or the same for the house. Held apart + // from ErrNotEnabled because the act path turns that one into a fresh + // proposal, and drafting a new proposal for a tool that already exists and + // is enabled is a lie about what is wrong. + ErrNotConnected = errors.New("tool is enabled but its backend is not connected") ) // MCPCaller is the seam for an act that is an MCP tool call rather than a @@ -134,7 +141,7 @@ func (e *Executor) Exec(ctx context.Context, name string, args []string, confirm // confirmed. Only the dispatch differs. if server, remote, ok := mcp.ParseCmd(t.Cmd); ok { if e.mcp == nil { - return "", ErrNotEnabled + return "", fmt.Errorf("%w: %s is an MCP tool and no mcp block is configured", ErrNotConnected, name) } ctx, cancel := context.WithTimeout(ctx, e.timeout) defer cancel() @@ -148,7 +155,7 @@ func (e *Executor) Exec(ctx context.Context, name string, args []string, confirm // row but can never compose a target of its own. if entityID, service, ok := smarthome.ParseCmd(t.Cmd); ok { if e.home == nil { - return "", ErrNotEnabled + return "", fmt.Errorf("%w: %s is a house tool and no smarthome block is configured", ErrNotConnected, name) } ctx, cancel := context.WithTimeout(ctx, e.timeout) defer cancel() diff --git a/internal/tool/tool_test.go b/internal/tool/tool_test.go index cfc4c35..ed65bd6 100644 --- a/internal/tool/tool_test.go +++ b/internal/tool/tool_test.go @@ -178,8 +178,15 @@ func TestExecMCPRowWithoutCallerRefuses(t *testing.T) { ran := false e := NewExecutor(api, time.Second) e.run = func(context.Context, []string) (string, error) { ran = true; return "", nil } - if _, err := e.Exec(context.Background(), "vikunja_list_tasks", nil, false); !errors.Is(err, ErrNotEnabled) { - t.Fatalf("err = %v, want ErrNotEnabled", err) + err := func() error { _, e2 := e.Exec(context.Background(), "vikunja_list_tasks", nil, false); return e2 }() + // ErrNotConnected, NOT ErrNotEnabled: the act path turns ErrNotEnabled into + // a fresh proposal, and drafting a proposal for a row that already exists + // and is enabled answers the wrong question. + if !errors.Is(err, ErrNotConnected) { + t.Fatalf("err = %v, want ErrNotConnected", err) + } + if errors.Is(err, ErrNotEnabled) { + t.Fatal("an enabled row with a missing backend must not read as not-enabled") } if ran { t.Fatal(`"mcp" must never be run as a binary`) @@ -222,8 +229,8 @@ func TestExecSmartHomeRow(t *testing.T) { } // No house configured ⇒ the row refuses rather than being exec'd. - if _, err := newExec(nil).Exec(context.Background(), "home_light_x_off", nil, true); !errors.Is(err, ErrNotEnabled) { - t.Fatalf("unconfigured house: err = %v, want ErrNotEnabled", err) + if _, err := newExec(nil).Exec(context.Background(), "home_light_x_off", nil, true); !errors.Is(err, ErrNotConnected) { + t.Fatalf("unconfigured house: err = %v, want ErrNotConnected", err) } if ran { t.Fatal(`"smarthome" was run as a binary`) From da62a2f25eddc0e49c7d7862cf036e1f47359fa9 Mon Sep 17 00:00:00 2001 From: kami Date: Sat, 1 Aug 2026 14:11:57 +0400 Subject: [PATCH 04/11] mcp: pin what a tool was when it was approved An allowlist row stores cmd ["mcp", server, tool]. That is a late-bound reference to a name the far end owns, so the row pins nothing about behaviour: a server could redefine an enabled read-only list_tasks into something that writes, and Maven would keep calling it with no confirm turn and no second approval. Discovery now stores a fingerprint of the declared shape, name, description, input schema and readOnlyHint, and compares it on every refresh. A mismatch drops the row back to proposed and, if it stopped claiming read-only, marks it destructive. destructive is only ever raised. A row predating the column adopts its fingerprint silently, because an upgrade is not a redefinition. Nothing retracted a proposal either, so a tool a connected server no longer offers stayed enabled and failed at call time with an internal string. Those rows are withdrawn, with provenance saying why, and only for servers that are actually connected so a restart does not disarm what he approved. Argument binding rested on readOnlyHint, which the same server writes. A server advertising delete_project as read-only got an unconfirmed argument-carrying call. Binding now also requires the tool be named in allow_tools, something local, and refuses a required property the schema never describes rather than guessing it is a string. wireMCP dialled synchronously from run, and on the passkey path from inside the unlock handler, so one black-holed endpoint delayed boot and the answer to an unlock. The first dial happens on the refresh goroutine under the daemon context. Two servers whose names flatten to one local allowlist name no longer share a row. Found in review of #71. --- cmd/mavend/mcp.go | 145 ++++++++++++++++++++++++++++----- cmd/mavend/mcp_test.go | 19 +++++ internal/mcp/allowlist.go | 37 +++++++++ internal/mcp/allowlist_test.go | 32 ++++++++ internal/store/migrations.go | 9 ++ internal/store/tools.go | 123 ++++++++++++++++++++++++++-- internal/store/tools_test.go | 143 +++++++++++++++++++++++++++++++- 7 files changed, 479 insertions(+), 29 deletions(-) create mode 100644 internal/mcp/allowlist_test.go diff --git a/cmd/mavend/mcp.go b/cmd/mavend/mcp.go index 7b31117..c350b80 100644 --- a/cmd/mavend/mcp.go +++ b/cmd/mavend/mcp.go @@ -13,8 +13,11 @@ import ( "github.com/kami/maven/internal/webfetch" ) -// mcpRefreshInterval — how often the manager re-dials a server that is down. -// The manager applies its own backoff on top, so this being short is cheap. +// mcpRefreshInterval — how often the manager is asked to re-dial servers that +// are down. It is a tick, not a retry rate: mcp.Manager holds a per-server +// backoff that starts at DefaultReconnectEvery and doubles to +// MaxReconnectEvery, so a permanently misconfigured stdio server is not +// re-exec'd once a minute forever. const mcpRefreshInterval = time.Minute // mcpWiring — the MCP client, when the `mcp` block configures at least one @@ -29,9 +32,16 @@ type mcpWiring struct { st *store.Store } -// wireMCP builds the manager, connects, and proposes what it found. It never -// fails the daemon: a server that is unreachable at boot is logged and retried, -// because Maven starting is not contingent on someone else's process. +// wireMCP builds the manager. It does NOT dial: run does that, on its own +// goroutine, which is what makes "Maven starting is not contingent on someone +// else's process" true rather than merely intended. +// +// Dialing here used to be synchronous with a 30s budget, from wireVoice, from +// run. Connect dials serially and each HTTP dial is three requests against +// that server's timeout, so one black-holed endpoint cost 15s of boot and two +// cost the whole budget. On the passkey path wireVoice runs inside the unlock +// handler, so it delayed the answer to an unlock as well. Not failing and not +// blocking are different properties and only the first one held. func wireMCP(cfg *config.Config, st *store.Store) *mcpWiring { servers := cfg.MCPServers() if len(servers) == 0 { @@ -43,6 +53,7 @@ func wireMCP(cfg *config.Config, st *store.Store) *mcpWiring { limits.DenyHosts = cfg.MCP.DenyHosts limits.MaxBytes = cfg.MCP.MaxBytes limits.Timeout = time.Duration(cfg.MCP.Timeout) + limits.HostInterval = time.Duration(cfg.MCP.HostInterval) } mgr, err := mcp.NewManager(mcp.WebfetchDoor(limits), servers) if err != nil { @@ -52,30 +63,58 @@ func wireMCP(cfg *config.Config, st *store.Store) *mcpWiring { log.Printf("mcp: not wired: %v", err) return nil } - w := &mcpWiring{mgr: mgr, st: st} - ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) - defer cancel() - mgr.Connect(ctx) - w.propose(ctx) - return w + return &mcpWiring{mgr: mgr, st: st} } -// propose writes a 'proposed' allowlist row for every discovered tool. It does -// NOT enable anything: a configured server is a place Maven may look, not a -// capability she has. Kami enables what he wants on /tools, behind step-up, -// which is the same gate a shell tool goes through. +// connect dials every server and reconciles what came back. Called from run, +// under the daemon's context, so a shutdown during a slow dial is observed. +func (w *mcpWiring) connect(ctx context.Context) { + if w == nil { + return + } + w.mgr.Connect(ctx) + w.propose(ctx) +} + +// propose writes a 'proposed' allowlist row for every discovered tool, and +// reconciles the rows that already exist against what the server offers today. +// It does NOT enable anything: a configured server is a place Maven may look, +// not a capability she has. Kami enables what he wants on /tools, behind +// step-up, which is the same gate a shell tool goes through. // -// Re-running on every boot is idempotent — ProposeMCPTool never touches an -// existing row, so a tool he disabled stays disabled and one he enabled keeps -// the cmd he enabled it with. +// Three things happen per discovered tool. +// +// A name not in the store becomes a proposal, carrying the tool's fingerprint. +// +// A name already in the store is reconciled against that fingerprint. A tool +// whose description, schema or readOnlyHint changed since it was approved drops +// back to 'proposed' and, if it stopped claiming read-only, to destructive=1. +// Insert-or-skip was not enough on its own: the cmd is a late-bound reference +// to a name the far end owns, so the server can redefine list_tasks into +// something that writes without the row changing at all. +// +// A row whose server is connected and no longer offers the tool is withdrawn. func (w *mcpWiring) propose(ctx context.Context) { if w == nil { return } now := time.Now() - fresh := 0 + fresh, changed := 0, 0 + seen := map[string]string{} // local name → "server/tool", for collisions for _, t := range w.mgr.Tools() { name := mcp.LocalName(t.Server, t.Name) + remote := t.Server + "/" + t.Name + // Two different tools can flatten to one local name: server "vik" with + // tool "list_tasks" and server "vik_list" with tool "tasks" both give + // "vik_list_tasks". The store keys rows by name, so the second would + // land on the first one's row. Config-controlled and therefore rare, + // but silently reusing a row is the wrong way to lose that race. + if prev, dup := seen[name]; dup { + log.Printf("mcp: %s and %s both map to the allowlist name %q — skipping the second, rename a server", + prev, remote, name) + continue + } + seen[name] = remote // No readOnlyHint ⇒ assume it mutates ⇒ the confirm turn. Being wrong // in this direction only costs a question. destructive := !t.ReadOnly @@ -83,19 +122,82 @@ func (w *mcpWiring) propose(ctx context.Context) { if t.Description != "" { provenance += ": " + t.Description } + fp := mcp.Fingerprint(t) ok, err := w.st.ProposeMCPTool(ctx, name, mcp.Scope(t.Server), - mcp.Cmd(t.Server, t.Name), destructive, provenance, now) + mcp.Cmd(t.Server, t.Name), destructive, provenance, fp, now) if err != nil { log.Printf("mcp: propose %s: %v", name, err) continue } if ok { fresh++ + continue + } + // The row already existed. Its provenance is whatever the server said + // the first time; reconciling rewrites it, so what /tools shows is what + // the server says now. + ch, err := w.st.ReconcileMCPTool(ctx, name, fp, destructive, provenance, now) + if err != nil { + log.Printf("mcp: reconcile %s: %v", name, err) + continue + } + if !ch.Changed { + continue + } + changed++ + switch { + case ch.Demoted && ch.Escalated: + log.Printf("mcp: %s changed on the server and no longer claims read-only — disabled and marked destructive, re-approve it on /tools", name) + case ch.Demoted: + log.Printf("mcp: %s changed on the server since it was enabled — disabled, re-approve it on /tools", name) + default: + log.Printf("mcp: %s changed on the server; the proposal now shows the new description", name) } } + w.withdrawGone(ctx, seen, now) if fresh > 0 { log.Printf("mcp: %d new tool proposal(s) waiting on /tools", fresh) } + if changed > 0 { + log.Printf("mcp: %d tool(s) changed since approval and need another look", changed) + } +} + +// withdrawGone disarms rows whose tool the server stopped offering. Only +// servers that are CONNECTED are considered: a tool missing because its server +// is down is not a tool that was withdrawn, and disabling a capability every +// time a process restarts would be worse than the problem. +func (w *mcpWiring) withdrawGone(ctx context.Context, seen map[string]string, now time.Time) { + live := map[string]bool{} + for _, name := range w.mgr.Connected() { + live[name] = true + } + if len(live) == 0 { + return + } + rows, err := w.st.ListTools(ctx, "") + if err != nil { + log.Printf("mcp: list tools: %v", err) + return + } + for _, row := range rows { + server, remote, ok := mcp.ParseCmd(row.Cmd) + if !ok || !live[server] { + continue + } + if _, still := seen[row.Name]; still { + continue + } + note := fmt.Sprintf("mcp %s/%s: no longer offered by the server", server, remote) + wasEnabled, err := w.st.WithdrawTool(ctx, row.Name, note, now) + if err != nil { + log.Printf("mcp: withdraw %s: %v", row.Name, err) + continue + } + if wasEnabled { + log.Printf("mcp: %s was enabled but %s no longer offers it — disabled", row.Name, server) + } + } } // run re-dials downed servers and picks up tools that appeared, until ctx is @@ -104,6 +206,9 @@ func (w *mcpWiring) run(ctx context.Context) { if w == nil { return } + // The first dial happens here rather than at wiring time, so boot never + // waits on someone else's process. + w.connect(ctx) t := time.NewTicker(mcpRefreshInterval) defer t.Stop() for { diff --git a/cmd/mavend/mcp_test.go b/cmd/mavend/mcp_test.go index 6339524..b63bdbd 100644 --- a/cmd/mavend/mcp_test.go +++ b/cmd/mavend/mcp_test.go @@ -31,6 +31,23 @@ func TestWireMCPOffWhenUnconfigured(t *testing.T) { } } +// Wiring must not dial. Boot used to block for the whole per-server timeout +// budget on a black-holed endpoint, and on the passkey path that delay landed +// inside the unlock handler. +func TestWireMCPDoesNotDial(t *testing.T) { + st := newTestStore(t) + w := wireMCP(&config.Config{MCP: &config.MCPConfig{Servers: []config.MCPServerConfig{{ + Name: "dead", Command: "/nonexistent/mcp-server", Enabled: true, + }}}}, st) + if w == nil { + t.Fatal("a configured server should wire") + } + defer w.close() + if s := w.status(); len(s) != 1 || s[0].Err != "" { + t.Fatalf("wireMCP dialled: %+v", s) + } +} + // An unreachable server must not stop the daemon, must be reported as down, and // must propose nothing. func TestWireMCPUnreachableServerIsNotFatal(t *testing.T) { @@ -42,6 +59,7 @@ func TestWireMCPUnreachableServerIsNotFatal(t *testing.T) { t.Fatal("a configured server should still wire") } defer w.close() + w.connect(context.Background()) st2 := w.status() if len(st2) != 1 || st2[0].Connected || st2[0].Err == "" { t.Fatalf("status = %+v", st2) @@ -67,6 +85,7 @@ func TestWireMCPPrivateURLRefusedWithoutAllowPrivate(t *testing.T) { t.Fatal("should wire") } defer w.close() + w.connect(context.Background()) s := w.status()[0] if s.Connected { t.Fatal("a loopback server must not connect without allow_private") diff --git a/internal/mcp/allowlist.go b/internal/mcp/allowlist.go index 09c9d06..235b0c5 100644 --- a/internal/mcp/allowlist.go +++ b/internal/mcp/allowlist.go @@ -1,7 +1,11 @@ package mcp import ( + "crypto/sha256" + "encoding/hex" + "encoding/json" "regexp" + "strconv" "strings" ) @@ -59,3 +63,36 @@ func LocalName(server, tool string) string { // Scope is the store scope for a server's rows, so the /tools page can group // them and a human can tell at a glance where a capability came from. func Scope(server string) string { return "mcp:" + server } + +// Fingerprint is the declared shape of a discovered tool: its name, its +// description, its input schema and its readOnlyHint, hashed. +// +// It exists because an allowlist row cannot pin an MCP tool's behaviour. The +// row's cmd is ["mcp", server, tool], a reference to a name the REMOTE server +// owns and may redefine — the row does not have to change for the tool to +// become something else. The fingerprint is what Kami actually approved, so a +// later discovery can tell "same tool" from "same name". +// +// The schema is canonicalised through a decode and re-encode, so a server that +// reorders its JSON keys or changes its whitespace does not read as a +// redefinition. Unparseable schema bytes are hashed as they arrived. +func Fingerprint(t Tool) string { + schema := "" + if len(t.InputSchema) > 0 { + var any any + if json.Unmarshal(t.InputSchema, &any) == nil { + if raw, err := json.Marshal(any); err == nil { + schema = string(raw) + } + } + if schema == "" { + schema = string(t.InputSchema) + } + } + h := sha256.New() + for _, part := range []string{t.Name, t.Description, schema, strconv.FormatBool(t.ReadOnly)} { + h.Write([]byte(part)) + h.Write([]byte{0}) + } + return hex.EncodeToString(h.Sum(nil)) +} diff --git a/internal/mcp/allowlist_test.go b/internal/mcp/allowlist_test.go new file mode 100644 index 0000000..fd74ae2 --- /dev/null +++ b/internal/mcp/allowlist_test.go @@ -0,0 +1,32 @@ +package mcp + +import ( + "encoding/json" + "testing" +) + +// The fingerprint must cover everything the approval was given for, and must +// not move when only the JSON spelling of the schema does. +func TestFingerprintCoversTheDeclaredShape(t *testing.T) { + base := Tool{Name: "list_tasks", Description: "list them", ReadOnly: true, + InputSchema: json.RawMessage(`{"type":"object","properties":{}}`)} + same := base + same.InputSchema = json.RawMessage("{\n \"properties\": {},\n \"type\": \"object\"\n}") + if Fingerprint(base) != Fingerprint(same) { + t.Error("reformatting the schema must not read as a redefinition") + } + for name, mut := range map[string]func(*Tool){ + "description": func(x *Tool) { x.Description = "delete them" }, + "schema": func(x *Tool) { x.InputSchema = json.RawMessage(`{"required":["id"]}`) }, + "readonly": func(x *Tool) { x.ReadOnly = false }, + "name": func(x *Tool) { x.Name = "delete_tasks" }, + } { + t.Run(name, func(t *testing.T) { + got := base + mut(&got) + if Fingerprint(got) == Fingerprint(base) { + t.Error("a redefinition must change the fingerprint") + } + }) + } +} diff --git a/internal/store/migrations.go b/internal/store/migrations.go index 163ad83..d7597f8 100644 --- a/internal/store/migrations.go +++ b/internal/store/migrations.go @@ -160,6 +160,15 @@ ALTER TABLE reminders ADD COLUMN next_fire_ts INTEGER;`, // #2 ); CREATE UNIQUE INDEX IF NOT EXISTS idx_tasks_live_norm ON tasks (norm) WHERE status IN ('candidate','open'); CREATE INDEX IF NOT EXISTS idx_tasks_status ON tasks (status, created_ts DESC);`, + + `ALTER TABLE tools ADD COLUMN fingerprint TEXT NOT NULL DEFAULT '';`, + // #15 — what a discovered tool WAS when it was approved (Vikunja #251). + // An MCP row's cmd is ["mcp", server, tool], which is a late-bound + // reference: it names a tool on a server the remote end owns and it pins + // no behaviour at all. A server upgraded, or taken over, can redefine + // list_tasks into something that writes without the row changing by one + // byte. The fingerprint is the declared shape at approval time, so a + // redefinition is a re-approval instead of a silent upgrade. } // migrate applies every migration with a number greater than the DB's current diff --git a/internal/store/tools.go b/internal/store/tools.go index 10f9c37..bf34901 100644 --- a/internal/store/tools.go +++ b/internal/store/tools.go @@ -65,7 +65,16 @@ func (s *Store) ProposeTool(ctx context.Context, name, utterance, scope string, // Like ProposeTool it never touches an existing row, so re-discovery on every // restart is idempotent and cannot silently re-arm a tool that was disabled or // change the cmd of one already enabled. -func (s *Store) ProposeMCPTool(ctx context.Context, name, scope string, cmd []string, destructive bool, utterance string, ts time.Time) (bool, error) { +// +// The row is NOT what protects him, and it is worth being exact about that. +// cmd is ["mcp", server, tool]: a late-bound reference to a name the remote +// server owns. The tool it points at can be redefined on the far end without +// the row changing at all, so "the cmd cannot change" is true and beside the +// point. fingerprint is what closes that: it records the declared shape (name, +// description, input schema, readOnlyHint) at the time the proposal was +// written, and ReconcileMCPTool compares against it on every later discovery. +// Pass "" for a row with nothing to fingerprint (a Home Assistant device). +func (s *Store) ProposeMCPTool(ctx context.Context, name, scope string, cmd []string, destructive bool, utterance, fingerprint string, ts time.Time) (bool, error) { if len(cmd) == 0 { return false, ErrToolCmd } @@ -81,10 +90,10 @@ func (s *Store) ProposeMCPTool(ctx context.Context, name, scope string, cmd []st d = 1 } res, err := s.db.ExecContext(ctx, ` - INSERT INTO tools (name, scope, cmd, destructive, status, utterance, created_ts, updated_ts) - VALUES (?, ?, ?, ?, 'proposed', ?, ?, ?) + INSERT INTO tools (name, scope, cmd, destructive, status, utterance, fingerprint, created_ts, updated_ts) + VALUES (?, ?, ?, ?, 'proposed', ?, ?, ?, ?) ON CONFLICT(name) DO NOTHING`, - name, scope, string(raw), d, utterance, ts.UnixMilli(), ts.UnixMilli()) + name, scope, string(raw), d, utterance, fingerprint, ts.UnixMilli(), ts.UnixMilli()) if err != nil { return false, fmt.Errorf("propose mcp tool: %w", err) } @@ -105,7 +114,111 @@ func (s *Store) ProposeMCPTool(ctx context.Context, name, scope string, cmd []st // turn. Re-discovery on every refresh is idempotent — an existing row is never // touched, so a device he disabled stays disabled. func (s *Store) ProposeSmartHomeTool(ctx context.Context, name, scope string, cmd []string, utterance string, ts time.Time) (bool, error) { - return s.ProposeMCPTool(ctx, name, scope, cmd, true, utterance, ts) + return s.ProposeMCPTool(ctx, name, scope, cmd, true, utterance, "", ts) +} + +// ToolChange — what ReconcileMCPTool did to an existing row. +type ToolChange struct { + // Changed — the discovered shape differs from the approved one. + Changed bool + // Demoted — the row was enabled and is now 'proposed' again, so the + // capability is off until a human looks at it a second time. + Demoted bool + // Escalated — destructive went from 0 to 1. It never goes the other way. + Escalated bool +} + +// ReconcileMCPTool compares a freshly discovered tool against the row that was +// approved, and escalates when they disagree. +// +// The failure this exists for: day 1 the server offers list_tasks with +// readOnlyHint true, so the row is proposed non-destructive and Kami enables +// it. Day 30 the server is upgraded, or taken over, and list_tasks now writes. +// Insert-or-skip does nothing on that discovery — the row is still enabled, +// still destructive=0 — and the confirm turn never fires, because the flag was +// frozen against a claim the server has since withdrawn. +// +// So: a differing fingerprint drops the row back to 'proposed' and rewrites the +// provenance, and a tool that stopped claiming read-only gets destructive=1. +// destructive is only ever raised, never lowered: relaxing it on the say-so of +// the same server that changed underneath us would undo the point. +// +// A row with an empty stored fingerprint predates this and simply adopts the +// discovered one — an upgrade is not a redefinition. +func (s *Store) ReconcileMCPTool(ctx context.Context, name, fingerprint string, destructive bool, utterance string, ts time.Time) (ToolChange, error) { + var ( + stored string + status string + wasDest int + ) + err := s.db.QueryRowContext(ctx, + `SELECT fingerprint, status, destructive FROM tools WHERE name = ?`, name). + Scan(&stored, &status, &wasDest) + if errors.Is(err, sql.ErrNoRows) { + return ToolChange{}, ErrToolNotFound + } + if err != nil { + return ToolChange{}, fmt.Errorf("reconcile mcp tool: %w", err) + } + var ch ToolChange + if stored == "" { + if _, err := s.db.ExecContext(ctx, + `UPDATE tools SET fingerprint = ?, updated_ts = ? WHERE name = ?`, + fingerprint, ts.UnixMilli(), name); err != nil { + return ToolChange{}, fmt.Errorf("reconcile mcp tool: %w", err) + } + return ch, nil + } + if stored == fingerprint { + return ch, nil + } + ch.Changed = true + ch.Demoted = status == "enabled" + d := wasDest + if destructive && wasDest == 0 { + d, ch.Escalated = 1, true + } + if _, err := s.db.ExecContext(ctx, ` + UPDATE tools + SET fingerprint = ?, destructive = ?, status = 'proposed', utterance = ?, updated_ts = ? + WHERE name = ?`, + fingerprint, d, utterance, ts.UnixMilli(), name); err != nil { + return ToolChange{}, fmt.Errorf("reconcile mcp tool: %w", err) + } + return ch, nil +} + +// WithdrawTool disarms a row whose remote tool no longer exists: it drops back +// to 'proposed' and its provenance says why. +// +// Nothing else retracted a proposal, so a tool a server stopped offering kept +// its row forever, and an ENABLED one stayed enabled and failed at call time +// with an internal string the act path does not match. /tools is where he would +// go to find out and it was the one place that did not say. Returns whether the +// row was still enabled. +func (s *Store) WithdrawTool(ctx context.Context, name, utterance string, ts time.Time) (bool, error) { + res, err := s.db.ExecContext(ctx, ` + UPDATE tools SET status = 'proposed', utterance = ?, updated_ts = ? + WHERE name = ? AND status = 'enabled'`, + utterance, ts.UnixMilli(), name) + if err != nil { + return false, fmt.Errorf("withdraw tool: %w", err) + } + n, err := res.RowsAffected() + if err != nil { + return false, fmt.Errorf("withdraw tool: rows affected: %w", err) + } + if n > 0 { + return true, nil + } + // Not enabled: still refresh the provenance so the proposed row says it. + _, err = s.db.ExecContext(ctx, + `UPDATE tools SET utterance = ?, updated_ts = ? WHERE name = ?`, + utterance, ts.UnixMilli(), name) + if err != nil { + return false, fmt.Errorf("withdraw tool: %w", err) + } + return false, nil } // EnableTool fills cmd + destructive and flips status to 'enabled'. This is the diff --git a/internal/store/tools_test.go b/internal/store/tools_test.go index 0ee973e..375d5b0 100644 --- a/internal/store/tools_test.go +++ b/internal/store/tools_test.go @@ -70,7 +70,7 @@ func TestProposeMCPTool(t *testing.T) { now := time.Now() cmd := []string{"mcp", "vikunja", "list_tasks"} - fresh, err := s.ProposeMCPTool(ctx, "vikunja_list_tasks", "mcp:vikunja", cmd, false, "mcp vikunja/list_tasks: List tasks", now) + fresh, err := s.ProposeMCPTool(ctx, "vikunja_list_tasks", "mcp:vikunja", cmd, false, "mcp vikunja/list_tasks: List tasks", "fp1", now) if err != nil { t.Fatal(err) } @@ -92,7 +92,7 @@ func TestProposeMCPTool(t *testing.T) { } // Re-discovery on the next boot is idempotent. - fresh, err = s.ProposeMCPTool(ctx, "vikunja_list_tasks", "mcp:vikunja", cmd, true, "changed", now) + fresh, err = s.ProposeMCPTool(ctx, "vikunja_list_tasks", "mcp:vikunja", cmd, true, "changed", "fp1", now) if err != nil { t.Fatal(err) } @@ -104,7 +104,7 @@ func TestProposeMCPTool(t *testing.T) { if err := s.EnableTool(ctx, "vikunja_list_tasks", cmd, false, "mcp:vikunja", now); err != nil { t.Fatal(err) } - if _, err := s.ProposeMCPTool(ctx, "vikunja_list_tasks", "mcp:vikunja", []string{"mcp", "vikunja", "delete_task"}, true, "x", now); err != nil { + if _, err := s.ProposeMCPTool(ctx, "vikunja_list_tasks", "mcp:vikunja", []string{"mcp", "vikunja", "delete_task"}, true, "x", "fp2", now); err != nil { t.Fatal(err) } got, err = s.LookupTool(ctx, "vikunja_list_tasks") @@ -118,7 +118,142 @@ func TestProposeMCPTool(t *testing.T) { func TestProposeMCPToolNeedsCmd(t *testing.T) { s := newTestStore(t) - if _, err := s.ProposeMCPTool(context.Background(), "x", "mcp:y", nil, false, "", time.Now()); !errors.Is(err, ErrToolCmd) { + if _, err := s.ProposeMCPTool(context.Background(), "x", "mcp:y", nil, false, "", "", time.Now()); !errors.Is(err, ErrToolCmd) { t.Fatalf("err = %v, want ErrToolCmd", err) } } + +// A server that redefines a tool Kami already approved must have to ask again. +// The row stores cmd ["mcp", server, tool], a late-bound reference to a name +// the far end owns, so before the fingerprint a server could turn an enabled +// read-only list_tasks into something that writes and Maven would keep running +// it without a confirm turn. +func TestReconcileMCPToolDemotesARedefinedTool(t *testing.T) { + s := newTestStore(t) + ctx := context.Background() + now := time.Now() + cmd := []string{"mcp", "vikunja", "list_tasks"} + if _, err := s.ProposeMCPTool(ctx, "vikunja_list_tasks", "mcp:vikunja", cmd, false, "read only", "fp1", now); err != nil { + t.Fatal(err) + } + if err := s.EnableTool(ctx, "vikunja_list_tasks", cmd, false, "mcp:vikunja", now); err != nil { + t.Fatal(err) + } + + // Same shape ⇒ nothing happens. Discovery runs every minute and must be + // idempotent. + ch, err := s.ReconcileMCPTool(ctx, "vikunja_list_tasks", "fp1", false, "read only", now) + if err != nil { + t.Fatal(err) + } + if ch.Changed { + t.Fatalf("an unchanged tool must not be touched: %+v", ch) + } + if got, _ := s.LookupTool(ctx, "vikunja_list_tasks"); got.Status != "enabled" { + t.Fatalf("status = %q, want it left enabled", got.Status) + } + + // It stopped claiming read-only and its schema moved. + ch, err = s.ReconcileMCPTool(ctx, "vikunja_list_tasks", "fp2", true, "now writes", now) + if err != nil { + t.Fatal(err) + } + if !ch.Changed || !ch.Demoted || !ch.Escalated { + t.Fatalf("change = %+v, want changed+demoted+escalated", ch) + } + got, err := s.LookupTool(ctx, "vikunja_list_tasks") + if err != nil { + t.Fatal(err) + } + if got.Status != "proposed" { + t.Errorf("status = %q, want a redefined tool back in the queue", got.Status) + } + if !got.Destructive { + t.Error("a tool that stopped claiming read-only must gain the confirm turn") + } + if got.Utterance != "now writes" { + t.Errorf("utterance = %q, want what the server says today", got.Utterance) + } + + // destructive is only ever raised. The server that changed underneath us + // does not get to relax it by claiming read-only next time. + if _, err := s.ReconcileMCPTool(ctx, "vikunja_list_tasks", "fp3", false, "read only again", now); err != nil { + t.Fatal(err) + } + if got, _ = s.LookupTool(ctx, "vikunja_list_tasks"); !got.Destructive { + t.Error("destructive was relaxed by the server") + } +} + +// A row written before fingerprints exist simply adopts one. An upgrade is not +// a redefinition and must not disable everything Kami approved. +func TestReconcileMCPToolAdoptsAnEmptyFingerprint(t *testing.T) { + s := newTestStore(t) + ctx := context.Background() + now := time.Now() + cmd := []string{"mcp", "vikunja", "list_tasks"} + if _, err := s.ProposeMCPTool(ctx, "vikunja_list_tasks", "mcp:vikunja", cmd, false, "x", "", now); err != nil { + t.Fatal(err) + } + if err := s.EnableTool(ctx, "vikunja_list_tasks", cmd, false, "mcp:vikunja", now); err != nil { + t.Fatal(err) + } + ch, err := s.ReconcileMCPTool(ctx, "vikunja_list_tasks", "fp1", false, "x", now) + if err != nil { + t.Fatal(err) + } + if ch.Changed { + t.Fatalf("adopting must be silent: %+v", ch) + } + if got, _ := s.LookupTool(ctx, "vikunja_list_tasks"); got.Status != "enabled" { + t.Fatalf("status = %q, want still enabled after the upgrade", got.Status) + } + // And now it is pinned. + if ch, _ = s.ReconcileMCPTool(ctx, "vikunja_list_tasks", "fp2", false, "y", now); !ch.Changed { + t.Fatal("the adopted fingerprint must be enforced on the next pass") + } +} + +func TestReconcileMCPToolUnknownRow(t *testing.T) { + s := newTestStore(t) + if _, err := s.ReconcileMCPTool(context.Background(), "nope", "fp", false, "", time.Now()); !errors.Is(err, ErrToolNotFound) { + t.Fatalf("err = %v, want ErrToolNotFound", err) + } +} + +// A tool the server stopped offering must be disarmed and must say why. It used +// to stay enabled and fail at call time with an internal string, and /tools — +// the one place he would look — did not mention it. +func TestWithdrawTool(t *testing.T) { + s := newTestStore(t) + ctx := context.Background() + now := time.Now() + cmd := []string{"mcp", "vikunja", "list_tasks"} + if _, err := s.ProposeMCPTool(ctx, "vikunja_list_tasks", "mcp:vikunja", cmd, false, "x", "fp1", now); err != nil { + t.Fatal(err) + } + if err := s.EnableTool(ctx, "vikunja_list_tasks", cmd, false, "mcp:vikunja", now); err != nil { + t.Fatal(err) + } + was, err := s.WithdrawTool(ctx, "vikunja_list_tasks", "gone", now) + if err != nil { + t.Fatal(err) + } + if !was { + t.Error("withdrawing an enabled tool must report that it was enabled") + } + got, err := s.LookupTool(ctx, "vikunja_list_tasks") + if err != nil { + t.Fatal(err) + } + if got.Status != "proposed" || got.Utterance != "gone" { + t.Fatalf("row = %+v, want proposed and saying why", got) + } + // Withdrawing again is not an error and does not claim it was enabled. + if was, err = s.WithdrawTool(ctx, "vikunja_list_tasks", "still gone", now); err != nil || was { + t.Fatalf("second withdraw = %v, %v", was, err) + } + if got, _ = s.LookupTool(ctx, "vikunja_list_tasks"); got.Utterance != "still gone" { + t.Errorf("utterance = %q, want the provenance refreshed anyway", got.Utterance) + } +} From 61ba58388f15768084c4aa44f64ded727bab849d Mon Sep 17 00:00:00 2001 From: kami Date: Sat, 1 Aug 2026 14:21:29 +0400 Subject: [PATCH 05/11] media: bound an image by pixels, not by compressed bytes The only cap was 64 MiB of input, and a decode bomb is a small file. A 20000x20000 PNG of flat colour compresses to a few hundred kilobytes, decodes to 400 million pixels, and flattenAndScale then allocated a second buffer of the same dimensions before scaling anything. That is 3.2 GB of live heap from one request, on a laptop, in the process that owns the database and the socket, and max_dim never got a chance to help. The header is read first now and a source over forty megapixels is refused. The scaler reads the source through At and allocates only the destination, so flattening no longer doubles the peak. Found in review of #72. --- internal/media/image.go | 69 +++++++++++++++++++++++++++--------- internal/media/image_test.go | 69 ++++++++++++++++++++++++++++++++++++ 2 files changed, 122 insertions(+), 16 deletions(-) diff --git a/internal/media/image.go b/internal/media/image.go index b798c3a..905eccc 100644 --- a/internal/media/image.go +++ b/internal/media/image.go @@ -6,7 +6,6 @@ import ( "errors" "fmt" "image" - "image/draw" "image/gif" "image/jpeg" "image/png" @@ -25,6 +24,19 @@ const DefaultMaxDim = 896 // original bytes stay in the blob store untouched. const JPEGQuality = 85 +// DefaultMaxPixels — the largest source image this build will decode, counted +// in pixels rather than in compressed bytes. A byte cap is not a memory bound +// for an image: a 20000x20000 PNG of flat colour compresses to a few hundred +// kilobytes and decodes to 400 million pixels, which is 1.6 GB of heap in the +// process that owns the database and the socket. 40 megapixels is well past any +// phone camera and two orders of magnitude short of an OOM. +const DefaultMaxPixels = 40 << 20 + +// ErrTooManyPixels — the image header declares more pixels than this build +// will decode. Separate from ErrUnsupportedImage because the format is fine and +// the size is not, and the log line should say which. +var ErrTooManyPixels = errors.New("media: image has too many pixels") + // ErrUnsupportedImage — the bytes are not an image format this build can // decode. Notably webp: the stdlib has no webp decoder and this repo takes no // new dependencies, so a webp arriving from Telegram is refused here with a @@ -91,6 +103,16 @@ func PrepareImage(data []byte, source string, maxDim int) (Image, error) { if err != nil { return Image{}, err } + // The header is read before the pixels. Deciding after the decode is not a + // decision: by then the whole bitmap is already in the heap. + cfg, err := decodeConfig(data, mime) + if err != nil { + return Image{}, fmt.Errorf("media: read %s header: %w", mime, err) + } + if px := int64(cfg.Width) * int64(cfg.Height); px > DefaultMaxPixels { + return Image{}, fmt.Errorf("%w: %dx%d is %d, cap is %d", + ErrTooManyPixels, cfg.Width, cfg.Height, px, int64(DefaultMaxPixels)) + } src, err := decode(data, mime) if err != nil { return Image{}, fmt.Errorf("media: decode %s: %w", mime, err) @@ -105,6 +127,19 @@ func PrepareImage(data []byte, source string, maxDim int) (Image, error) { return Image{JPEG: buf.Bytes(), Width: b.Dx(), Height: b.Dy(), Source: source}, nil } +func decodeConfig(data []byte, mime string) (image.Config, error) { + r := bytes.NewReader(data) + switch strings.ToLower(mime) { + case "image/jpeg": + return jpeg.DecodeConfig(r) + case "image/png": + return png.DecodeConfig(r) + case "image/gif": + return gif.DecodeConfig(r) + } + return image.Config{}, ErrUnsupportedImage +} + func decode(data []byte, mime string) (image.Image, error) { r := bytes.NewReader(data) switch strings.ToLower(mime) { @@ -123,18 +158,16 @@ func decode(data []byte, mime string) (image.Image, error) { // destination pixel — nearest-neighbour would alias small text into noise, // which defeats the point of reading a screenshot, and an area average is a // dozen lines against pulling in golang.org/x/image on an offline box. +// +// It reads the source through At and allocates only the destination. Flattening +// into a full-size RGBA first doubled the peak: a 40-megapixel photo already +// costs 160 MB decoded, and the intermediate made it 320 MB before MaxDim had +// any chance to help. func flattenAndScale(src image.Image, maxDim int) *image.RGBA { sb := src.Bounds() sw, sh := sb.Dx(), sb.Dy() dw, dh := fit(sw, sh, maxDim) - flat := image.NewRGBA(image.Rect(0, 0, sw, sh)) - draw.Draw(flat, flat.Bounds(), image.NewUniform(image.White), image.Point{}, draw.Src) - draw.Draw(flat, flat.Bounds(), src, sb.Min, draw.Over) - if dw == sw && dh == sh { - return flat - } - dst := image.NewRGBA(image.Rect(0, 0, dw, dh)) for y := 0; y < dh; y++ { y0, y1 := y*sh/dh, (y+1)*sh/dh @@ -146,20 +179,24 @@ func flattenAndScale(src image.Image, maxDim int) *image.RGBA { if x1 <= x0 { x1 = x0 + 1 } - var r, g, b, n uint32 + var r, g, b, n uint64 for sy := y0; sy < y1; sy++ { for sx := x0; sx < x1; sx++ { - i := flat.PixOffset(sx, sy) - r += uint32(flat.Pix[i]) - g += uint32(flat.Pix[i+1]) - b += uint32(flat.Pix[i+2]) + // At returns premultiplied 16-bit. Compositing over white + // is then c + (1-alpha), which is the same answer the + // draw.Over pass used to give, one pixel at a time. + cr, cg, cb, ca := src.At(sb.Min.X+sx, sb.Min.Y+sy).RGBA() + inv := uint64(0xFFFF - ca) + r += uint64(cr) + inv + g += uint64(cg) + inv + b += uint64(cb) + inv n++ } } o := dst.PixOffset(x, y) - dst.Pix[o] = uint8(r / n) - dst.Pix[o+1] = uint8(g / n) - dst.Pix[o+2] = uint8(b / n) + dst.Pix[o] = uint8(r / n >> 8) + dst.Pix[o+1] = uint8(g / n >> 8) + dst.Pix[o+2] = uint8(b / n >> 8) dst.Pix[o+3] = 0xFF } } diff --git a/internal/media/image_test.go b/internal/media/image_test.go index 4355d0c..fe2802b 100644 --- a/internal/media/image_test.go +++ b/internal/media/image_test.go @@ -2,7 +2,9 @@ package media import ( "bytes" + "encoding/binary" "errors" + "hash/crc32" "image" "image/color" "image/gif" @@ -189,3 +191,70 @@ func gifBytes(t *testing.T, w, h int) []byte { } return buf.Bytes() } + +// A decode bomb is a small file. Nothing bounded pixels before decoding, so a +// 20000x20000 PNG of flat colour — a few hundred kilobytes on the wire, well +// under the byte cap — decoded to 1.6 GB and then allocated another 1.6 GB to +// flatten, in the process that owns the database and the socket. +func TestPrepareImageRefusesADecodeBomb(t *testing.T) { + // The header is what is checked, so the test writes a real header and + // truncated pixel data: reaching the decode at all is the failure. + var buf bytes.Buffer + if err := png.Encode(&buf, image.NewGray(image.Rect(0, 0, 1, 1))); err != nil { + t.Fatal(err) + } + bomb := forgePNGSize(t, buf.Bytes(), 20000, 20000) + _, err := PrepareImage(bomb, "telegram", 0) + if !errors.Is(err, ErrTooManyPixels) { + t.Fatalf("err = %v, want ErrTooManyPixels", err) + } + // A phone photo is not a bomb. + if _, err := PrepareImage(pngBytes(t, 64, 48), "telegram", 0); err != nil { + t.Fatalf("an ordinary image was refused: %v", err) + } +} + +// forgePNGSize rewrites the IHDR width and height (and its CRC) of a valid PNG, +// which is how a header claiming 400 megapixels is produced without writing +// 400 megapixels. +func forgePNGSize(t *testing.T, src []byte, w, h uint32) []byte { + t.Helper() + out := append([]byte(nil), src...) + // 8 byte signature, 4 byte length, 4 byte "IHDR", then width and height. + const ihdr = 8 + 4 + 4 + binary.BigEndian.PutUint32(out[ihdr:], w) + binary.BigEndian.PutUint32(out[ihdr+4:], h) + crc := crc32.ChecksumIEEE(out[8+4 : ihdr+13]) + binary.BigEndian.PutUint32(out[ihdr+13:], crc) + return out +} + +// Transparency still composites onto white, which is what makes a screenshot +// readable. The old code did that with a full-size intermediate; the scaler +// walks the source instead and must give the same answer. +func TestPrepareImageFlattensOntoWhite(t *testing.T) { + img := image.NewRGBA(image.Rect(0, 0, 8, 8)) + // Fully transparent everywhere: over white, that is white. + data := encodePNG(t, img) + out, err := PrepareImage(data, "test", 4) + if err != nil { + t.Fatal(err) + } + dec, err := jpeg.Decode(bytes.NewReader(out.JPEG)) + if err != nil { + t.Fatal(err) + } + r, g, b, _ := dec.At(2, 2).RGBA() + if r>>8 < 240 || g>>8 < 240 || b>>8 < 240 { + t.Fatalf("transparent pixel came out %d,%d,%d, want white", r>>8, g>>8, b>>8) + } +} + +func encodePNG(t *testing.T, img image.Image) []byte { + t.Helper() + var buf bytes.Buffer + if err := png.Encode(&buf, img); err != nil { + t.Fatal(err) + } + return buf.Bytes() +} From 4b052fb9d2abe40acd8ce62c63a2b5bd3086ecfe Mon Sep 17 00:00:00 2001 From: kami Date: Sat, 1 Aug 2026 14:21:29 +0400 Subject: [PATCH 06/11] media: make retention and the disk budget true Put wrote the blob and then the sidecar. A full disk or a crash between the two left bytes on disk with no sidecar, and List walks sidecars, so Prune could never see them: Put returned an error and an image nobody knew about became permanent. The sidecar goes first, a failed write is rolled back, and Prune also collects blob files that have no readable sidecar and are past retention, which picks up whatever an older build leaked. The per-blob cap bounds one call and nothing bounded their sum. Content addressing does not help, because one flipped pixel is a different digest, so 64 MiB per call and an unlimited number of calls fills the disk mavend's database lives on. The store now carries a whole-store budget, seeded from disk at open so a restart does not begin at zero. Found in review of #72. --- internal/media/store.go | 173 +++++++++++++++++++++++++++++++++-- internal/media/store_test.go | 128 ++++++++++++++++++++++++++ 2 files changed, 295 insertions(+), 6 deletions(-) diff --git a/internal/media/store.go b/internal/media/store.go index c41897e..f3d4ef5 100644 --- a/internal/media/store.go +++ b/internal/media/store.go @@ -11,6 +11,7 @@ import ( "path/filepath" "sort" "strings" + "sync" "time" ) @@ -24,6 +25,19 @@ const DefaultMaxBytes int64 = 64 << 20 // short enough that "she has a month of my meetings on disk" is never true. const DefaultRetention = 7 * 24 * time.Hour +// DefaultMaxTotalBytes — the whole-store budget when one is not configured. The +// per-blob cap bounds one call and nothing bounded the sum of them: 64 MiB per +// call, an unlimited number of calls, and a seven-day window fills the disk +// mavend's database lives on. Content addressing does not help, because one +// flipped pixel is a different digest. 4 GiB is roughly sixty meetings or a few +// thousand photos inside the window. +const DefaultMaxTotalBytes int64 = 4 << 30 + +// ErrStoreFull — the store is at its total-bytes budget. Distinct from +// ErrTooLarge: the payload is a reasonable size and there is no room for it, so +// the answer is to prune or raise the budget, not to send something smaller. +var ErrStoreFull = errors.New("media: store is full") + // Store — a content-addressed blob directory. Zero value is not usable; build // one with Open, which creates the directory 0700. The store holds no lock and // no cache: every operation is a filesystem call, and two writers of the same @@ -31,8 +45,16 @@ const DefaultRetention = 7 * 24 * time.Hour type Store struct { dir string maxBytes int64 + maxTotal int64 retention time.Duration now func() time.Time + + // total is the running sum of stored blob bytes, seeded by Open with a + // directory walk and kept up to date by Put, Delete and Prune. It is a + // cache of something the filesystem already knows: re-walking on every Put + // would be correct too and would make an image intake O(store size). + totalMu sync.Mutex + total int64 } // Open prepares a blob store rooted at dir. maxBytes ≤ 0 ⇒ DefaultMaxBytes; @@ -40,6 +62,12 @@ type Store struct { // created later) is 0700: these are recordings of people, and the daemon's user // is the only reader. func Open(dir string, maxBytes int64, retention time.Duration) (*Store, error) { + return OpenWithBudget(dir, maxBytes, 0, retention) +} + +// OpenWithBudget is Open with the whole-store budget spelled out. maxTotal ≤ 0 +// ⇒ DefaultMaxTotalBytes. +func OpenWithBudget(dir string, maxBytes, maxTotal int64, retention time.Duration) (*Store, error) { if strings.TrimSpace(dir) == "" { return nil, errors.New("media: empty dir") } @@ -53,12 +81,47 @@ func Open(dir string, maxBytes int64, retention time.Duration) (*Store, error) { if maxBytes <= 0 { maxBytes = DefaultMaxBytes } + if maxTotal <= 0 { + maxTotal = DefaultMaxTotalBytes + } + if maxTotal < maxBytes { + return nil, fmt.Errorf("media: max_total_bytes %d is below the per-blob cap %d", maxTotal, maxBytes) + } if retention <= 0 { retention = DefaultRetention } - return &Store{dir: abs, maxBytes: maxBytes, retention: retention, now: time.Now}, nil + s := &Store{dir: abs, maxBytes: maxBytes, maxTotal: maxTotal, retention: retention, now: time.Now} + s.total = s.measure() + return s, nil } +// measure sums what is already on disk, so a restart does not start the budget +// over at zero. +func (s *Store) measure() int64 { + var total int64 + _ = filepath.WalkDir(s.dir, func(path string, d fs.DirEntry, err error) error { + if err != nil || d.IsDir() || strings.HasSuffix(path, ".json") { + return nil //nolint:nilerr // an unreadable corner is not worth refusing to boot over + } + if info, err := d.Info(); err == nil { + total += info.Size() + } + return nil + }) + return total +} + +// Total is the number of blob bytes currently stored, and Budget the cap Put +// checks it against. Both are exported so the daemon can log how close it is. +func (s *Store) Total() int64 { + s.totalMu.Lock() + defer s.totalMu.Unlock() + return s.total +} + +// Budget is the whole-store cap. +func (s *Store) Budget() int64 { return s.maxTotal } + // Dir is the store root. Exported for logs and for pointing a subprocess at a // path under it. func (s *Store) Dir() string { return s.dir } @@ -104,10 +167,36 @@ func (s *Store) Put(kind Kind, mime, source string, data []byte) (Blob, error) { b.Created = prev.Created } - if err := writeFile(blobPath, data); err != nil { + // A blob already on disk costs nothing more, so dedupe is checked before + // the budget rather than after it. + _, already := os.Stat(blobPath) + if already != nil { + s.totalMu.Lock() + room := s.total+b.Size <= s.maxTotal + if room { + s.total += b.Size + } + s.totalMu.Unlock() + if !room { + return Blob{}, fmt.Errorf("%w: %d stored, %d budget, %d more asked for", + ErrStoreFull, s.Total(), s.maxTotal, b.Size) + } + } + + // The sidecar goes first. Written second, a full disk or a crash between + // the two left the bytes on disk with no sidecar, and List only sees + // sidecars, so Prune could never collect them: Put returned an error and an + // image nobody knew about became permanent. + if err := writeMeta(metaPath, b); err != nil { return Blob{}, err } - if err := writeMeta(metaPath, b); err != nil { + if err := writeFile(blobPath, data); err != nil { + _ = os.Remove(metaPath) + if already != nil { + s.totalMu.Lock() + s.total -= b.Size + s.totalMu.Unlock() + } return Blob{}, err } return b, nil @@ -210,10 +299,24 @@ func (s *Store) Delete(id string) error { continue } for _, e := range entries { - if strings.HasPrefix(e.Name(), id) { - if err := os.Remove(filepath.Join(bucket, e.Name())); err != nil && !errors.Is(err, fs.ErrNotExist) { - return fmt.Errorf("media: delete %s: %w", shortID(id), err) + if !strings.HasPrefix(e.Name(), id) { + continue + } + path := filepath.Join(bucket, e.Name()) + var size int64 + if info, err := e.Info(); err == nil && !strings.HasSuffix(e.Name(), ".json") { + size = info.Size() + } + if err := os.Remove(path); err != nil && !errors.Is(err, fs.ErrNotExist) { + return fmt.Errorf("media: delete %s: %w", shortID(id), err) + } + if size > 0 { + s.totalMu.Lock() + s.total -= size + if s.total < 0 { + s.total = 0 } + s.totalMu.Unlock() } } } @@ -232,15 +335,70 @@ func (s *Store) Prune() (int, error) { } now := s.now() deleted := 0 + known := map[string]bool{} for _, b := range blobs { + known[b.ID] = true if b.Age(now) <= s.retention { continue } if err := s.Delete(b.ID); err != nil { return deleted, err } + delete(known, b.ID) deleted++ } + n, err := s.pruneOrphans(known, now) + return deleted + n, err +} + +// pruneOrphans collects blob files with no readable sidecar. List walks +// sidecars, so those files were invisible to retention and stayed on disk +// forever: audio of people accumulating is the exact failure this package +// exists to prevent, and a half-finished Put from an older build is enough to +// produce one. They are only collected once they are older than retention, so a +// Put racing a Prune does not lose its bytes. +func (s *Store) pruneOrphans(known map[string]bool, now time.Time) (int, error) { + deleted := 0 + for _, kind := range []Kind{KindImage, KindAudio} { + root := filepath.Join(s.dir, string(kind)) + err := filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { + if err != nil { + if errors.Is(err, fs.ErrNotExist) { + return nil + } + return err + } + if d.IsDir() || strings.HasSuffix(path, ".json") { + return nil + } + name := d.Name() + id, _, _ := strings.Cut(name, ".") + if known[id] { + return nil + } + info, err := d.Info() + if err != nil { + return nil //nolint:nilerr // gone underneath us is the outcome we wanted + } + if now.Sub(info.ModTime()) <= s.retention { + return nil + } + if err := os.Remove(path); err != nil && !errors.Is(err, fs.ErrNotExist) { + return err + } + s.totalMu.Lock() + s.total -= info.Size() + if s.total < 0 { + s.total = 0 + } + s.totalMu.Unlock() + deleted++ + return nil + }) + if err != nil { + return deleted, fmt.Errorf("media: prune %s: %w", kind, err) + } + } return deleted, nil } @@ -300,6 +458,9 @@ func extFor(mime string, kind Kind) string { case "image/gif": return ".gif" case "image/webp": + // Unreachable for images today: SniffImage refuses webp before + // anything reaches Put, because this build has no webp decoder. Kept + // so the mapping is right on the day one arrives. return ".webp" case "audio/wav", "audio/x-wav", "audio/wave": return ".wav" diff --git a/internal/media/store_test.go b/internal/media/store_test.go index c983f0b..fccb67f 100644 --- a/internal/media/store_test.go +++ b/internal/media/store_test.go @@ -1,6 +1,8 @@ package media import ( + "crypto/sha256" + "encoding/hex" "errors" "os" "path/filepath" @@ -212,3 +214,129 @@ func TestOpenRejectsEmptyDir(t *testing.T) { t.Error("empty dir accepted") } } + +// A blob whose sidecar is missing was invisible to List, so Prune never saw it +// and the bytes stayed on disk forever. Put produced exactly that state, by +// writing the blob first and the sidecar second. +func TestPruneCollectsASidecarlessBlob(t *testing.T) { + s := testStore(t) + b, err := s.Put(KindImage, "image/png", "web:upload", []byte("orphan")) + if err != nil { + t.Fatal(err) + } + meta := filepath.Join(s.dir, string(KindImage), b.ID[:2], b.ID+".json") + if err := os.Remove(meta); err != nil { + t.Fatal(err) + } + // Age the file past retention, the same way a real orphan gets there. + old := time.Now().Add(-2 * DefaultRetention) + if err := os.Chtimes(b.Path, old, old); err != nil { + t.Fatal(err) + } + n, err := s.Prune() + if err != nil { + t.Fatal(err) + } + if n != 1 { + t.Fatalf("pruned %d, want the orphan collected", n) + } + if _, err := os.Stat(b.Path); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("the orphaned bytes are still on disk: %v", err) + } +} + +// A young orphan is left alone: a Put racing a Prune must not lose its bytes. +func TestPruneLeavesAYoungOrphan(t *testing.T) { + s := testStore(t) + b, err := s.Put(KindImage, "image/png", "web:upload", []byte("fresh")) + if err != nil { + t.Fatal(err) + } + if err := os.Remove(filepath.Join(s.dir, string(KindImage), b.ID[:2], b.ID+".json")); err != nil { + t.Fatal(err) + } + if n, err := s.Prune(); err != nil || n != 0 { + t.Fatalf("prune = %d, %v; want the fresh orphan kept", n, err) + } +} + +// Put writes the sidecar first, so a failure writing the bytes leaves nothing +// at all rather than an uncollectable blob. +func TestPutLeavesNothingWhenTheBytesCannotBeWritten(t *testing.T) { + s := testStore(t) + data := []byte("will not land") + sum := sha256.Sum256(data) + id := hex.EncodeToString(sum[:]) + bucket := filepath.Join(s.dir, string(KindImage), id[:2]) + if err := os.MkdirAll(bucket, 0o700); err != nil { + t.Fatal(err) + } + // A directory where the blob file needs to be: rename onto it fails, which + // is the same shape as a full disk one step later. + if err := os.Mkdir(filepath.Join(bucket, id+".png"), 0o700); err != nil { + t.Fatal(err) + } + if _, err := s.Put(KindImage, "image/png", "web:upload", data); err == nil { + t.Fatal("put must fail") + } + if _, err := os.Stat(filepath.Join(bucket, id+".json")); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("a sidecar was left behind claiming a blob that does not exist: %v", err) + } + if s.Total() != 0 { + t.Errorf("total = %d, want the failed put not counted", s.Total()) + } +} + +// The per-blob cap bounds one call and nothing bounded their sum. 64 MiB per +// call times unlimited calls inside a seven-day window fills the disk mavend's +// database lives on. +func TestPutRefusesPastTheStoreBudget(t *testing.T) { + s, err := OpenWithBudget(t.TempDir(), 16, 48, 0) + if err != nil { + t.Fatal(err) + } + for i, want := range []bool{true, true, true, false} { + data := []byte(strings.Repeat(string(rune('a'+i)), 16)) + _, err := s.Put(KindImage, "image/png", "web:upload", data) + if ok := err == nil; ok != want { + t.Fatalf("put %d: err = %v, want ok=%v", i, err, want) + } + if !want && !errors.Is(err, ErrStoreFull) { + t.Fatalf("put %d: err = %v, want ErrStoreFull", i, err) + } + } + // The same bytes again cost nothing, so they are not refused. + if _, err := s.Put(KindImage, "image/png", "web:upload", []byte(strings.Repeat("a", 16))); err != nil { + t.Fatalf("a re-send of stored bytes was refused: %v", err) + } + // Deleting frees the budget again. + blobs, err := s.List(KindImage) + if err != nil { + t.Fatal(err) + } + if err := s.Delete(blobs[0].ID); err != nil { + t.Fatal(err) + } + if _, err := s.Put(KindImage, "image/png", "web:upload", []byte(strings.Repeat("z", 16))); err != nil { + t.Fatalf("budget was not released on delete: %v", err) + } +} + +// A restart must not start the budget over at zero. +func TestOpenSeedsTheBudgetFromDisk(t *testing.T) { + dir := t.TempDir() + s, err := OpenWithBudget(dir, 16, 48, 0) + if err != nil { + t.Fatal(err) + } + if _, err := s.Put(KindImage, "image/png", "web:upload", []byte(strings.Repeat("a", 16))); err != nil { + t.Fatal(err) + } + again, err := OpenWithBudget(dir, 16, 48, 0) + if err != nil { + t.Fatal(err) + } + if again.Total() != 16 { + t.Fatalf("total after reopen = %d, want 16", again.Total()) + } +} From e926e4e6df052e2345ed158853669c571114b922 Mon Sep 17 00:00:00 2001 From: kami Date: Sat, 1 Aug 2026 14:21:46 +0400 Subject: [PATCH 07/11] vision: hold the second request to the rule the first one follows checkPrivate validates the configured endpoint literal and validated nothing after it. The client followed redirects, so a 302 from the local llama-server would have sent the photo, as a data URI in the POST body, to whatever the redirect named. "No provider in this repo may upload a blob" was true only of the first hop. Redirects are refused now, and the reply is read through a cap rather than however much the endpoint feels like sending. ValidateEndpoint exports the same check so config can fail at startup on a typo instead of logging once and leaving vision quietly off. Found in review of #72. --- internal/vision/vision.go | 30 ++++++++++++++- internal/vision/vision_test.go | 70 ++++++++++++++++++++++++++++++++++ 2 files changed, 98 insertions(+), 2 deletions(-) diff --git a/internal/vision/vision.go b/internal/vision/vision.go index eda0ad8..a1a4667 100644 --- a/internal/vision/vision.go +++ b/internal/vision/vision.go @@ -37,6 +37,7 @@ import ( "encoding/json" "errors" "fmt" + "io" "net" "net/http" "net/url" @@ -93,6 +94,10 @@ func (Disabled) Describe(context.Context, media.Image, string) (string, error) { return "", ErrDisabled } +// MaxReplyBytes bounds what is read back from the vision server. A description +// is words; anything past a megabyte is a broken endpoint. +const MaxReplyBytes = 1 << 20 + // Config — how to reach the local vision server. Built from // config.VisionConfig by the daemon; kept separate so this package does not // import internal/config. @@ -154,13 +159,31 @@ func NewLocal(cfg Config) (*LocalProvider, error) { model: cfg.Model, prompt: prompt, maxTokens: maxTokens, - http: &http.Client{Timeout: timeout}, + http: &http.Client{ + Timeout: timeout, + // No redirects. checkPrivate validates the configured literal and + // nothing validated a hop, so a 302 from the local llama-server + // would send the photo, as a data URI in the POST body, wherever + // the redirect named. "No provider in this repo may upload a blob" + // has to be true of the second request as well as the first. + CheckRedirect: func(*http.Request, []*http.Request) error { + return http.ErrUseLastResponse + }, + }, }, nil } // Endpoint is the server this provider talks to. For logs and /dash. func (p *LocalProvider) Endpoint() string { return p.endpoint } +// ValidateEndpoint reports whether a configured endpoint is one this package +// would accept. Exported so config validation fails at startup on a typo, +// rather than logging once at wiring time and leaving the capability quietly +// off. +func ValidateEndpoint(raw string) error { + return checkPrivate(strings.TrimRight(strings.TrimSpace(raw), "/")) +} + // checkPrivate refuses any endpoint that is not on this box or its LAN. A // hostname that is not an IP literal is refused too: "vision.example.com" could // resolve anywhere, and resolving it here would be trusting DNS with his photos. @@ -260,7 +283,10 @@ func (p *LocalProvider) Describe(ctx context.Context, im media.Image, prompt str return "", fmt.Errorf("vision: status %d", resp.StatusCode) } var out chatResp - if err := json.NewDecoder(resp.Body).Decode(&out); err != nil { + // Capped: the decoder would otherwise read whatever the endpoint sends, and + // a local server answering with a stuck stream should not cost the daemon + // its memory. A description is a few hundred tokens. + if err := json.NewDecoder(io.LimitReader(resp.Body, MaxReplyBytes)).Decode(&out); err != nil { return "", fmt.Errorf("vision: decode: %w", err) } if len(out.Choices) == 0 { diff --git a/internal/vision/vision_test.go b/internal/vision/vision_test.go index 71787fc..a65cfc5 100644 --- a/internal/vision/vision_test.go +++ b/internal/vision/vision_test.go @@ -11,6 +11,7 @@ import ( "net/http" "net/http/httptest" "strings" + "sync/atomic" "testing" "time" @@ -196,3 +197,72 @@ func TestDescribeErrors(t *testing.T) { } }) } + +// checkPrivate validates the configured literal and used to validate nothing +// else. A 302 from the local llama-server would have sent the photo, as a data +// URI in the POST body, wherever the redirect named. +func TestLocalProviderDoesNotFollowARedirect(t *testing.T) { + var elsewhere int32 + away := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + atomic.AddInt32(&elsewhere, 1) + _, _ = io.WriteString(w, `{"choices":[{"message":{"content":"leaked"}}]}`) + })) + defer away.Close() + local := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, away.URL+"/v1/chat/completions", http.StatusFound) + })) + defer local.Close() + + p, err := NewLocal(Config{Endpoint: local.URL}) + if err != nil { + t.Fatal(err) + } + im := media.Image{JPEG: []byte{0xFF, 0xD8, 0xFF}} + if _, err := p.Describe(context.Background(), im, "что это"); err == nil { + t.Fatal("a redirected describe must fail, not follow") + } + if n := atomic.LoadInt32(&elsewhere); n != 0 { + t.Fatalf("the image was sent to the redirect target %d time(s)", n) + } +} + +// The reply is read through a cap. A stuck endpoint should not cost the daemon +// its memory. +func TestLocalProviderCapsTheReply(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = io.WriteString(w, `{"choices":[{"message":{"content":"`) + for written := 0; written < MaxReplyBytes+(1<<20); written += 1 << 16 { + if _, err := io.WriteString(w, strings.Repeat("a", 1<<16)); err != nil { + return + } + } + })) + defer srv.Close() + p, err := NewLocal(Config{Endpoint: srv.URL}) + if err != nil { + t.Fatal(err) + } + im := media.Image{JPEG: []byte{0xFF, 0xD8, 0xFF}} + if _, err := p.Describe(context.Background(), im, ""); err == nil { + t.Fatal("an unbounded reply must fail rather than being read whole") + } +} + +// ValidateEndpoint is what config calls at startup, and it must agree with the +// constructor. +func TestValidateEndpointMatchesTheConstructor(t *testing.T) { + for _, raw := range []string{"http://127.0.0.1:8081", "http://localhost:8081/"} { + if err := ValidateEndpoint(raw); err != nil { + t.Errorf("ValidateEndpoint(%q) = %v", raw, err) + } + } + for _, raw := range []string{"http://8.8.8.8:8081", "http://vision.example.com", "ftp://127.0.0.1"} { + if err := ValidateEndpoint(raw); err == nil { + t.Errorf("ValidateEndpoint(%q) accepted a non-private endpoint", raw) + } + if _, err := NewLocal(Config{Endpoint: raw}); err == nil { + t.Errorf("NewLocal(%q) accepted what ValidateEndpoint should refuse", raw) + } + } +} From 543aefde4bc52dab05272181bd1dcee06f571ca9 Mon Sep 17 00:00:00 2001 From: kami Date: Sat, 1 Aug 2026 14:21:46 +0400 Subject: [PATCH 08/11] vision: scope the note, settle the contract, wait for the prune Saving a description writes recall corpus. writeNote embeds it under media:image:, a source no enrollment owns, and the method sits at AuthRead, so any enrolled module could put a small VLM's guess into what Maven knows and have it come back in a later turn as something she believes. The describing half stays a read; save_note is now held to the same source-scope rule WriteFact is, and the stored text carries a marker saying it came off a picture. Three doc comments said the method exists only when vision is enabled and the code says otherwise. The code is right, and storing without describing is the state this box is in, so the comments were corrected rather than the behaviour. A request carrying both data and id used to take the id branch and drop the bytes without a word; it is refused. A media dir that cannot be created and a vision endpoint that is a typo were logged at wiring time and the capability just stayed off, which is the hardest kind of misconfiguration to notice. Both fail at startup. runPrune was the one loop started with a bare go and not in the daemon's WaitGroup, so shutdown did not wait for a prune that was deleting files. Found in review of #72. --- cmd/mavend/main.go | 10 +++-- cmd/mavend/vision.go | 45 ++++++++++++++++----- cmd/mavend/vision_test.go | 72 ++++++++++++++++++++++++++++++++++ internal/auth/auth_test.go | 27 +++++++++++++ internal/auth/policy.go | 32 +++++++++++++++ internal/config/config.go | 37 +++++++++++++++++ internal/config/senses_test.go | 37 +++++++++++++++++ internal/ipc/api.go | 20 ++++++---- internal/ipc/server.go | 10 +++-- 9 files changed, 266 insertions(+), 24 deletions(-) create mode 100644 cmd/mavend/vision_test.go diff --git a/cmd/mavend/main.go b/cmd/mavend/main.go index d6df63a..8211d0e 100644 --- a/cmd/mavend/main.go +++ b/cmd/mavend/main.go @@ -368,6 +368,11 @@ func run(args []string) error { srv.StepUp = func(ctx context.Context) error { return passkeySess.Assert(ctx, auth.Scope{}) } + // wg is declared here rather than next to srv.Serve because the media + // retention loop starts on this path too, and shutdown has to wait for a + // prune in flight: it deletes files. + var wg sync.WaitGroup + // Mail ingestion (Vikunja #246): the hook stays nil unless an email block is // configured and there is a llama-server to extract with, in which case // ipc.MethodIngestMail reports ErrUnknownMethod. @@ -376,7 +381,7 @@ func run(args []string) error { wireModelSwap(srv, phr, cfg) // Vision + the media blob store (Vikunja #252). Both stay dark without a // media block; MethodDescribeImage answers ErrUnknownMethod then. - keeper := wireVision(ctx, srv, st, embedderOf(voiceW), cfg) + keeper := wireVision(ctx, &wg, srv, st, embedderOf(voiceW), cfg) // The meeting recorder (Vikunja #253) shares that blob store and its // retention loop. Off unless a capture block enables it, in which case // all four capture methods answer ErrUnknownMethod. @@ -555,7 +560,7 @@ func run(args []string) error { srv.Check = (&auth.Gate{Enrollment: auth.NewFloorEnrollment(), Session: passkeySess}).Check wireMailIntake(srv, st, phr, cfg, evBus) wireModelSwap(srv, phr, cfg) - keeper := wireVision(ctx, srv, st, embedderOf(voiceW), cfg) + keeper := wireVision(ctx, &wg, srv, st, embedderOf(voiceW), cfg) wireCapture(srv, keeper, st, voiceW, phr, cfg) // Voice identification (Vikunja #255). Enrolment plumbing only until a // speaker-embedding model exists on disk; off entirely without a speaker @@ -622,7 +627,6 @@ func run(args []string) error { } } - var wg sync.WaitGroup wg.Add(1) go func() { defer wg.Done() diff --git a/cmd/mavend/vision.go b/cmd/mavend/vision.go index 93b0c2d..172b7a8 100644 --- a/cmd/mavend/vision.go +++ b/cmd/mavend/vision.go @@ -7,11 +7,14 @@ // media.dir, prepares a downscaled JPEG, and asks a local vision server what it // is. The description comes back as words; nothing about the image is echoed. // -// Off unless configured twice over: no `media` block ⇒ nowhere to keep the -// bytes, so the method does not exist; no `vision` block with enabled + a local -// endpoint ⇒ the store is wired but the describing half refuses, and the method -// still does not exist. A surface cannot make Maven look at pictures by merely -// sending one. +// Off unless configured: no `media` block ⇒ nowhere to keep the bytes, so the +// method does not exist and a surface cannot make Maven accept a photo by +// merely sending one. A `media` block with no `vision` block is a real state, +// the one this box is in today: the store is wired, the method exists, the +// bytes are kept and the reply says she cannot read the picture yet. That reply +// is re-runnable by id on the day a vision model lands, which is the reason to +// keep the bytes at all. Saving the description as a note needs more than the +// read rung — see the scope check on auth.ImageNoteSource. // // Two things this file deliberately does not do: // @@ -29,6 +32,7 @@ import ( "fmt" "log" "path/filepath" + "sync" "time" "github.com/kami/maven/internal/config" @@ -64,12 +68,14 @@ func openMediaStore(cfg *config.Config) *mediaKeeper { if !filepath.IsAbs(dir) && cfg.StateDir != "" { dir = filepath.Join(cfg.StateDir, dir) } - st, err := media.Open(dir, cfg.Media.MaxBytes, time.Duration(cfg.Media.Retention)) + st, err := media.OpenWithBudget(dir, cfg.Media.MaxBytes, cfg.Media.MaxTotalBytes, + time.Duration(cfg.Media.Retention)) if err != nil { log.Printf("media: %v — image and audio intake disabled", err) return nil } - log.Printf("media: blob store at %s, retention %s", st.Dir(), st.Retention()) + log.Printf("media: blob store at %s, retention %s, %d of %d bytes used", + st.Dir(), st.Retention(), st.Total(), st.Budget()) return &mediaKeeper{store: st} } @@ -159,6 +165,12 @@ func (v *visionIntake) describe(ctx context.Context, req ipc.DescribeImageReq) ( if len(req.Data) == 0 && req.ID == "" { return ipc.DescribeImageResp{}, fmt.Errorf("describe image: neither data nor id") } + if len(req.Data) > 0 && req.ID != "" { + // The contract says exactly one. Taking the ID branch and dropping the + // bytes silently is the worst of the three possible answers: the caller + // believes it sent a new image and nothing says otherwise. + return ipc.DescribeImageResp{}, fmt.Errorf("describe image: both data and id given, send one") + } var ( res vision.Result @@ -205,6 +217,12 @@ func (v *visionIntake) describe(ctx context.Context, req ipc.DescribeImageReq) ( return resp, nil } +// noteMarker prefixes a stored description. Without it the note reads exactly +// like something he told her, and it is not: it is a small VLM's guess about a +// picture, embedded and recalled as if it were his own words. Four characters +// of provenance in the text are cheaper than believing it later. +const noteMarker = "Со снимка: " + // writeNote stores the description as an ordinary note so it is recallable. The // note carries the blob id in its source, which is the only link back to the // bytes — the note text is words about the picture, never the picture. @@ -221,7 +239,7 @@ func (v *visionIntake) writeNote(ctx context.Context, res vision.Result) (int64, } } source := "media:image:" + res.Blob.ID[:12] - return v.st.WriteNote(ctx, v.now(), res.Description, vec, source) + return v.st.WriteNote(ctx, v.now(), noteMarker+res.Description, vec, source) } // sourceOrDefault labels a blob whose sender did not say where it came from. @@ -241,12 +259,19 @@ func sourceOrDefault(s string) string { // with one retention loop holds both the images and the audio, which is the // whole point of internal/media being a shared package. nil ⇒ no media block, // and neither capability exists. -func wireVision(ctx context.Context, srv *ipc.Server, st *store.Store, emb router.Embedder, cfg *config.Config) *mediaKeeper { +func wireVision(ctx context.Context, wg *sync.WaitGroup, srv *ipc.Server, st *store.Store, emb router.Embedder, cfg *config.Config) *mediaKeeper { keeper := openMediaStore(cfg) if keeper == nil { return nil } - go keeper.runPrune(ctx) + // In the daemon's WaitGroup like every other loop in run: a prune deletes + // files, and shutting down in the middle of one was the single loop nobody + // waited for. + wg.Add(1) + go func() { + defer wg.Done() + keeper.runPrune(ctx) + }() vi := newVisionIntake(keeper, st, emb, cfg) if vi == nil { diff --git a/cmd/mavend/vision_test.go b/cmd/mavend/vision_test.go new file mode 100644 index 0000000..ab69be0 --- /dev/null +++ b/cmd/mavend/vision_test.go @@ -0,0 +1,72 @@ +package main + +import ( + "bytes" + "context" + "image" + "image/png" + "strings" + "testing" + "time" + + "github.com/kami/maven/internal/config" + "github.com/kami/maven/internal/ipc" + "github.com/kami/maven/internal/media" + "github.com/kami/maven/internal/vision" +) + +func testIntake(t *testing.T) *visionIntake { + t.Helper() + st := newTestStore(t) + blobs, err := media.Open(t.TempDir(), 0, 0) + if err != nil { + t.Fatal(err) + } + return &visionIntake{ + in: vision.NewIntake(blobs, vision.Disabled{}, 0), + st: st, + now: time.Now, + } +} + +// The contract says exactly one of Data or ID. Taking the ID branch and +// dropping the bytes silently is the worst of the three possible answers: the +// caller believes it sent a new image and nothing says otherwise. +func TestDescribeRefusesBothDataAndID(t *testing.T) { + v := testIntake(t) + _, err := v.describe(context.Background(), ipc.DescribeImageReq{ + Data: []byte("bytes"), ID: strings.Repeat("a", 64), + }) + if err == nil { + t.Fatal("both data and id must be refused") + } + if !strings.Contains(err.Error(), "send one") { + t.Fatalf("err = %v, want it to name the contract", err) + } +} + +// Vision being off does not remove the method: the bytes are stored and the +// answer says she cannot read the picture yet, which is re-runnable by id. That +// is the state this box is in today, and three doc comments used to claim the +// opposite. +func TestVisionOffStillStores(t *testing.T) { + v := testIntake(t) + var buf bytes.Buffer + if err := png.Encode(&buf, image.NewRGBA(image.Rect(0, 0, 4, 4))); err != nil { + t.Fatal(err) + } + resp, err := v.describe(context.Background(), ipc.DescribeImageReq{Data: buf.Bytes(), Source: "web:upload"}) + if err != nil { + t.Fatalf("storing must succeed even with no vision model: %v", err) + } + if len(resp.ID) != 64 { + t.Fatalf("no blob id came back: %+v", resp) + } + if resp.Description != "" { + t.Errorf("description = %q, want none", resp.Description) + } + // And with no media block at all the method does not exist. + if vi := newVisionIntake(nil, nil, nil, &config.Config{}); vi != nil { + t.Fatal("no media block must leave the method nonexistent") + } +} diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 626e933..8f38f25 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -472,3 +472,30 @@ func TestRequirement_Speaker(t *testing.T) { t.Errorf("voice listing speakers = %v; want allowed", err) } } + +// Describing an image is a read. Saving the description is a write of recall +// corpus under a source no enrollment owns, so it is held to the same +// source-scope rule WriteFact is. Before this, any AuthRead caller could put a +// small VLM's guess into what Maven knows. +func TestCan_DescribeImage_SaveNoteNeedsScope(t *testing.T) { + poller := Scope{Surface: SurfaceTelegram, Module: "poll", SourceScope: []string{"poll:healthcheck"}} + web := Scope{Surface: SurfaceAuthedPage, Module: "web", SourceScope: []string{"*"}} + + plain, err := json.Marshal(ipc.DescribeImageReq{Data: []byte("x")}) + if err != nil { + t.Fatal(err) + } + noting, err := json.Marshal(ipc.DescribeImageReq{Data: []byte("x"), SaveNote: true}) + if err != nil { + t.Fatal(err) + } + if err := Can(ipc.MethodDescribeImage, poller, plain); err != nil { + t.Errorf("describing without saving must stay a read: %v", err) + } + if err := Can(ipc.MethodDescribeImage, poller, noting); !errors.Is(err, ErrForbidden) { + t.Errorf("save_note out of scope = %v, want ErrForbidden", err) + } + if err := Can(ipc.MethodDescribeImage, web, noting); err != nil { + t.Errorf("a module scoped to everything must still be allowed: %v", err) + } +} diff --git a/internal/auth/policy.go b/internal/auth/policy.go index f940f80..6c9fc5b 100644 --- a/internal/auth/policy.go +++ b/internal/auth/policy.go @@ -178,6 +178,18 @@ func Can(m ipc.Method, scope Scope, params json.RawMessage) error { switch Requirement(m) { case AuthRead: + // Describing an image is a read. Saving the description as a note is + // not: writeNote embeds it, so it comes back in a later turn as + // something Maven knows, under the source media:image:, which no + // enrollment owns. The rung's own argument was that the method "cannot + // write a fact, set a reminder, or touch the tool allowlist" — it can + // write recall corpus, and that is what AuthWrite exists to scope. So + // the note half is held to the same source-scope rule WriteFact is. + if m == ipc.MethodDescribeImage && wantsNote(params) { + if !SourceAllowed(scope.SourceScope, ImageNoteSource) { + return fmt.Errorf("%w: source %q out of scope", ErrForbidden, ImageNoteSource) + } + } // Any enrolled module may read. Reads through the surface level the // Enrollment set (voice-L0 wouldn't be enrolled to write at all). return nil @@ -214,6 +226,26 @@ func Can(m ipc.Method, scope Scope, params json.RawMessage) error { return nil } +// ImageNoteSource is the source scope a caller needs to turn a described image +// into a note. The note itself is stored under "media:image:"; the +// scope is checked against this stem, because the id is not known until the +// bytes arrive and no enrollment could name it in advance. +const ImageNoteSource = "media:image" + +// wantsNote reports whether a DescribeImage call asked for the description to +// be remembered. Malformed params read as no: dispatch rejects them a moment +// later with a better error. +func wantsNote(raw json.RawMessage) bool { + if len(raw) == 0 { + return false + } + var p ipc.DescribeImageReq + if json.Unmarshal(raw, &p) != nil { + return false + } + return p.SaveNote +} + // SourceAllowed — true iff src is in scope (the wildcard "*" matches all). // Empty scope ⇒ fail closed. The function is pure; we keep it exported so a // future enrollment table can call into the same matching logic. diff --git a/internal/config/config.go b/internal/config/config.go index 84af890..4fdae93 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -27,6 +27,7 @@ import ( "github.com/kami/maven/internal/morning" "github.com/kami/maven/internal/netscan" "github.com/kami/maven/internal/smarthome" + "github.com/kami/maven/internal/vision" "github.com/kami/maven/internal/update" "github.com/robfig/cron/v3" ) @@ -693,6 +694,12 @@ type MediaConfig struct { // MaxBytes — per-blob cap. 0 ⇒ media.DefaultMaxBytes (64 MiB). MaxBytes int64 `json:"max_bytes,omitempty"` + + // MaxTotalBytes — whole-store cap. 0 ⇒ media.DefaultMaxTotalBytes (4 GiB). + // The per-blob cap bounds one call; this one bounds the sum of them, which + // is what actually decides whether the disk mavend's database lives on can + // be filled from outside. + MaxTotalBytes int64 `json:"max_total_bytes,omitempty"` } // StoreDir reports the configured blob directory, or "" when media is not @@ -1428,6 +1435,36 @@ func (c *Config) validate() error { return err } } + // A media dir that cannot be created, or a vision endpoint that is a typo, + // used to be logged at wiring time and the capability just stayed off. A + // capability silently not existing is the hardest kind of misconfiguration + // to notice, so both fail here instead. + if c.Media != nil { + if c.Media.StoreDir() == "" { + return errors.New("media.dir is required when a media block is present") + } + if c.Media.MaxBytes < 0 || c.Media.MaxTotalBytes < 0 { + return errors.New("media: max_bytes and max_total_bytes cannot be negative") + } + if c.Media.MaxTotalBytes > 0 && c.Media.MaxBytes > c.Media.MaxTotalBytes { + return fmt.Errorf("media: max_bytes %d is above max_total_bytes %d", + c.Media.MaxBytes, c.Media.MaxTotalBytes) + } + } + if c.Vision != nil && c.Vision.Enabled { + if strings.TrimSpace(c.Vision.Endpoint) == "" { + return errors.New("vision.enabled set but vision.endpoint is empty") + } + if err := vision.ValidateEndpoint(c.Vision.Endpoint); err != nil { + return err + } + if c.Media.StoreDir() == "" { + return errors.New("vision.enabled set but there is no media block to keep the bytes in") + } + } + if c.Capture.Records() && c.Media.StoreDir() == "" { + return errors.New("capture.enabled set but there is no media block to keep the audio in") + } if len(c.MorningRoutines) > 0 { if err := morning.Validate(morningRoutinesFromConfig(c.MorningRoutines)); err != nil { return err diff --git a/internal/config/senses_test.go b/internal/config/senses_test.go index d74b454..8f50a17 100644 --- a/internal/config/senses_test.go +++ b/internal/config/senses_test.go @@ -221,3 +221,40 @@ func TestSpeakerBlockParsesFromJSON(t *testing.T) { t.Errorf("thresholds = %+v", cfg.Speaker) } } + +// A typo in the vision endpoint, or a media block with no dir, used to be +// logged once at wiring time and the capability just stayed off. A capability +// that silently does not exist is the hardest misconfiguration to notice, so +// both fail at startup now. +func TestSensesBlocksAreValidatedAtStartup(t *testing.T) { + bad := map[string]string{ + "media with no dir": `{"media":{"retention":"48h"}}`, + "negative budget": `{"media":{"dir":"/srv/media","max_total_bytes":-1}}`, + "blob over the budget": `{"media":{"dir":"/srv/media","max_bytes":100,"max_total_bytes":10}}`, + "vision with no media dir": `{"vision":{"enabled":true,"endpoint":"http://127.0.0.1:8081"}}`, + "vision endpoint typo": `{"media":{"dir":"/srv/media"},"vision":{"enabled":true,"endpoint":"127.0.0.1:8081"}}`, + "vision on the wan": `{"media":{"dir":"/srv/media"},"vision":{"enabled":true,"endpoint":"http://8.8.8.8:8081"}}`, + "vision, empty endpoint": `{"media":{"dir":"/srv/media"},"vision":{"enabled":true}}`, + "capture with no store": `{"capture":{"enabled":true}}`, + } + for name, body := range bad { + t.Run(name, func(t *testing.T) { + if _, err := Load(writeConfig(t, body)); err == nil { + t.Fatal("want a startup error") + } + }) + } + good := map[string]string{ + "media alone": `{"media":{"dir":"/srv/media"}}`, + "media + vision": `{"media":{"dir":"/srv/media"},"vision":{"enabled":true,"endpoint":"http://127.0.0.1:8081"}}`, + "media + capture": `{"media":{"dir":"/srv/media"},"capture":{"enabled":true}}`, + "vision off": `{"vision":{"endpoint":"http://8.8.8.8:8081"}}`, + } + for name, body := range good { + t.Run(name, func(t *testing.T) { + if _, err := Load(writeConfig(t, body)); err != nil { + t.Fatalf("valid config refused: %v", err) + } + }) + } +} diff --git a/internal/ipc/api.go b/internal/ipc/api.go index 30cc4ab..c2ff155 100644 --- a/internal/ipc/api.go +++ b/internal/ipc/api.go @@ -190,14 +190,16 @@ type IngestMailResp struct { // // Source is provenance recorded on the stored blob: "telegram", "web:upload". // -// Exactly one of Data or ID is set. ID re-describes an image core already has — -// a different question, or the first attempt that succeeds after a vision model -// finally lands on disk. +// Exactly one of Data or ID is set, and core refuses a request carrying both: +// it used to take the ID branch and drop the bytes without a word. // -// The method exists only when core has both a media store and an enabled vision -// block; otherwise it answers ErrUnknownMethod, which is what "off unless -// configured" looks like at the wire. A surface cannot make Maven look at -// pictures by merely sending one. +// The method exists when core has a media store. Vision being off does NOT +// remove it: the bytes are stored and the answer says she cannot read the +// picture yet, which is re-runnable by ID once a vision model is on disk, and +// it is the state this box is in today. So a surface that gets a reply with an +// id and an empty description has not failed, it has stored something. With no +// media block the method answers ErrUnknownMethod, which is what "off unless +// configured" looks like at the wire. type DescribeImageReq struct { Data []byte `json:"data,omitempty"` ID string `json:"id,omitempty"` @@ -206,6 +208,10 @@ type DescribeImageReq struct { // SaveNote — also write the description as a note (source // "media:image:") so it is recallable later. Default false: a // glance at a screenshot is not automatically a memory. + // + // Setting it raises what the call needs: an embedded note is recall corpus, + // so the caller's source scope must cover auth.ImageNoteSource. Describing + // without saving stays an ordinary read. SaveNote bool `json:"save_note,omitempty"` } diff --git a/internal/ipc/server.go b/internal/ipc/server.go index cee5355..a6f48ac 100644 --- a/internal/ipc/server.go +++ b/internal/ipc/server.go @@ -455,10 +455,12 @@ type Server struct { SwapModelFn SwapModelFunc ModelStatusFn ModelStatusFunc - // DescribeImageFn — looks at one image (Vikunja #252). Set by the daemon only - // when a media store is configured AND vision is enabled with a local - // endpoint; nil ⇒ MethodDescribeImage answers ErrUnknownMethod, so a surface - // cannot make Maven accept a photo by merely sending one. + // DescribeImageFn — looks at one image (Vikunja #252). Set by the daemon + // whenever a media store is configured. Vision being off does not clear it: + // the image is stored and the reply says she cannot read it yet, which is + // re-runnable by id later. nil ⇒ no media block ⇒ MethodDescribeImage + // answers ErrUnknownMethod, so a surface cannot make Maven accept a photo + // by merely sending one. // // It bypasses CoreAPI for the same reason IngestMailFn does: it needs a blob // store and a vision server, neither of which is a store operation, and no From 71b42e31bd0240d7111e59b3b9ce3b62d70d735a Mon Sep 17 00:00:00 2001 From: kami Date: Sat, 1 Aug 2026 14:36:17 +0400 Subject: [PATCH 09/11] media: move a file into the store instead of reading it in Put takes a []byte, so storing a recording meant the whole recording in memory. A two hour meeting at 16 kHz mono is about 230 MB of WAV, and building it from PCM held a second copy of the same size in the process that also owns the database and the resident model. PutFile stats the file, hashes it in a stream and renames it into place, so the peak is one buffer regardless of length. SpoolFile hands out the scratch file it moves from, under the media dir so it shares the same disk and the same permissions. Audio also gets its own per blob cap of 512 MiB. The image cap of 64 MiB is 35 minutes of audio, which contradicted the two hour session cap: the long meeting was exactly the one that failed to store. audio.WAVHeader is split out of WAVFromPCM because a spooled capture writes a placeholder header first and stamps the real length at the end. Found in review of #73. --- internal/audio/pcmwav.go | 27 ++++++-- internal/media/store.go | 145 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 165 insertions(+), 7 deletions(-) diff --git a/internal/audio/pcmwav.go b/internal/audio/pcmwav.go index 4889fa3..b9da0f8 100644 --- a/internal/audio/pcmwav.go +++ b/internal/audio/pcmwav.go @@ -94,14 +94,33 @@ func PCMFromWAV(wav []byte) (Format, []byte, error) { // header so the result can be written to disk and played with `aplay`. // Used by the reference client to write the TTS reply; not on the wire. func WAVFromPCM(format Format, pcm []byte) ([]byte, error) { + hdr, err := WAVHeader(format, len(pcm)) + if err != nil { + return nil, err + } + out := make([]byte, wavHeaderSize+len(pcm)) + copy(out, hdr) + copy(out[wavHeaderSize:], pcm) + return out, nil +} + +// WAVHeaderSize is the fixed size of the header WAVHeader writes. A caller +// spooling audio to a file reserves this many bytes up front and rewrites them +// once it knows the length. +const WAVHeaderSize = wavHeaderSize + +// WAVHeader builds just the 44-byte canonical header for n bytes of PCM. It +// exists so a long recording can be written straight to a file: holding the +// whole meeting in memory to prepend 44 bytes is what the streaming path is +// avoiding. +func WAVHeader(format Format, n int) ([]byte, error) { if !format.IsValid() { return nil, fmt.Errorf("audio: WAVFromPCM: %w: %+v", ErrNotCanonicalPCM, format) } - out := make([]byte, wavHeaderSize+len(pcm)) - copy(out[wavHeaderSize:], pcm) + out := make([]byte, wavHeaderSize) // RIFF header copy(out[0:4], []byte("RIFF")) - binary.LittleEndian.PutUint32(out[4:8], uint32(36+len(pcm))) + binary.LittleEndian.PutUint32(out[4:8], uint32(36+n)) copy(out[8:12], []byte("WAVE")) // fmt chunk copy(out[12:16], []byte("fmt ")) @@ -116,6 +135,6 @@ func WAVFromPCM(format Format, pcm []byte) ([]byte, error) { binary.LittleEndian.PutUint16(out[34:36], uint16(format.SampleBits)) // data chunk copy(out[36:40], []byte("data")) - binary.LittleEndian.PutUint32(out[40:44], uint32(len(pcm))) + binary.LittleEndian.PutUint32(out[40:44], uint32(n)) return out, nil } diff --git a/internal/media/store.go b/internal/media/store.go index f3d4ef5..32d3fc4 100644 --- a/internal/media/store.go +++ b/internal/media/store.go @@ -6,6 +6,7 @@ import ( "encoding/json" "errors" "fmt" + "io" "io/fs" "os" "path/filepath" @@ -20,6 +21,14 @@ import ( // ceiling; a single item bigger than that is a mistake, not a meeting. const DefaultMaxBytes int64 = 64 << 20 +// DefaultMaxAudioBytes — the per-blob cap for audio. Separate from +// DefaultMaxBytes because the two kinds are not the same size of thing: an +// image over 64 MiB is a mistake, and a two-hour meeting at 16 kHz mono is +// about 230 MB of PCM by design. With one shared cap, capture's own +// DefaultMaxDuration of two hours and this store's 64 MiB contradicted each +// other, and the meeting that hit the limit was the one that failed to store. +const DefaultMaxAudioBytes int64 = 512 << 20 + // DefaultRetention — how long a blob is kept when no retention is configured. // Seven days is long enough to re-run a transcription that came out wrong and // short enough that "she has a month of my meetings on disk" is never true. @@ -45,6 +54,7 @@ var ErrStoreFull = errors.New("media: store is full") type Store struct { dir string maxBytes int64 + maxAudio int64 maxTotal int64 retention time.Duration now func() time.Time @@ -81,16 +91,24 @@ func OpenWithBudget(dir string, maxBytes, maxTotal int64, retention time.Duratio if maxBytes <= 0 { maxBytes = DefaultMaxBytes } + maxAudio := DefaultMaxAudioBytes + if maxBytes > maxAudio { + maxAudio = maxBytes + } if maxTotal <= 0 { maxTotal = DefaultMaxTotalBytes } + if maxTotal < maxAudio { + maxAudio = maxTotal + } if maxTotal < maxBytes { return nil, fmt.Errorf("media: max_total_bytes %d is below the per-blob cap %d", maxTotal, maxBytes) } if retention <= 0 { retention = DefaultRetention } - s := &Store{dir: abs, maxBytes: maxBytes, maxTotal: maxTotal, retention: retention, now: time.Now} + s := &Store{dir: abs, maxBytes: maxBytes, maxAudio: maxAudio, maxTotal: maxTotal, + retention: retention, now: time.Now} s.total = s.measure() return s, nil } @@ -99,7 +117,13 @@ func OpenWithBudget(dir string, maxBytes, maxTotal int64, retention time.Duratio // over at zero. func (s *Store) measure() int64 { var total int64 + spool := filepath.Join(s.dir, "spool") _ = filepath.WalkDir(s.dir, func(path string, d fs.DirEntry, err error) error { + if err == nil && d.IsDir() && path == spool { + // Spool files are not blobs yet and PutFile counts them when they + // become one. Counting them here too would double them. + return filepath.SkipDir + } if err != nil || d.IsDir() || strings.HasSuffix(path, ".json") { return nil //nolint:nilerr // an unreadable corner is not worth refusing to boot over } @@ -144,8 +168,8 @@ func (s *Store) Put(kind Kind, mime, source string, data []byte) (Blob, error) { if len(data) == 0 { return Blob{}, ErrEmpty } - if int64(len(data)) > s.maxBytes { - return Blob{}, fmt.Errorf("%w: %d > %d", ErrTooLarge, len(data), s.maxBytes) + if cap := s.capFor(kind); int64(len(data)) > cap { + return Blob{}, fmt.Errorf("%w: %d > %d", ErrTooLarge, len(data), cap) } sum := sha256.Sum256(data) id := hex.EncodeToString(sum[:]) @@ -202,6 +226,121 @@ func (s *Store) Put(kind Kind, mime, source string, data []byte) (Blob, error) { return b, nil } +// capFor is the per-blob cap for a kind. Audio has its own, larger one. +func (s *Store) capFor(kind Kind) int64 { + if kind == KindAudio { + return s.maxAudio + } + return s.maxBytes +} + +// PutFile stores a file that is already on disk, by moving it into place rather +// than reading it into memory. It exists for meeting audio: a two-hour capture +// is a couple of hundred megabytes, and Put's []byte means that much heap in +// the process that owns the database, twice over while the WAV is built. +// +// src is consumed: on success it has been renamed into the store, and on a +// duplicate it is removed. On failure it is left where it is, so a caller that +// still needs the bytes can fall back to reading them. +func (s *Store) PutFile(kind Kind, mime, source, src string) (Blob, error) { + if !kind.Valid() { + return Blob{}, ErrBadKind + } + info, err := os.Stat(src) + if err != nil { + return Blob{}, fmt.Errorf("media: stat spool: %w", err) + } + if info.Size() == 0 { + return Blob{}, ErrEmpty + } + if cap := s.capFor(kind); info.Size() > cap { + return Blob{}, fmt.Errorf("%w: %d > %d", ErrTooLarge, info.Size(), cap) + } + id, err := hashFile(src) + if err != nil { + return Blob{}, err + } + blobPath, metaPath, err := s.paths(kind, id, mime) + if err != nil { + return Blob{}, err + } + if err := os.MkdirAll(filepath.Dir(blobPath), 0o700); err != nil { + return Blob{}, fmt.Errorf("media: create bucket: %w", err) + } + b := Blob{ID: id, Kind: kind, MIME: mime, Size: info.Size(), Source: source, + Created: s.now().UTC(), Path: blobPath} + if prev, err := readMeta(metaPath); err == nil && !prev.Created.IsZero() { + b.Created = prev.Created + } + _, already := os.Stat(blobPath) + if already != nil { + s.totalMu.Lock() + room := s.total+b.Size <= s.maxTotal + if room { + s.total += b.Size + } + s.totalMu.Unlock() + if !room { + return Blob{}, fmt.Errorf("%w: %d stored, %d budget, %d more asked for", + ErrStoreFull, s.Total(), s.maxTotal, b.Size) + } + } + if err := writeMeta(metaPath, b); err != nil { + return Blob{}, err + } + if already == nil { + // Same bytes already here. Drop the spool copy. + _ = os.Remove(src) + return b, nil + } + if err := os.Chmod(src, 0o600); err != nil { + return Blob{}, fmt.Errorf("media: chmod spool: %w", err) + } + if err := os.Rename(src, blobPath); err != nil { + _ = os.Remove(metaPath) + s.totalMu.Lock() + s.total -= b.Size + s.totalMu.Unlock() + return Blob{}, fmt.Errorf("media: move spool: %w", err) + } + return b, nil +} + +// SpoolFile creates an empty file under the store, outside the kind +// directories, for a caller that is writing a blob incrementally. Prune never +// looks at it and List never reports it; PutFile is what turns it into a blob. +// The caller owns removing it if it never gets that far. +func (s *Store) SpoolFile(prefix string) (*os.File, error) { + dir := filepath.Join(s.dir, "spool") + if err := os.MkdirAll(dir, 0o700); err != nil { + return nil, fmt.Errorf("media: create spool: %w", err) + } + f, err := os.CreateTemp(dir, prefix+"-*") + if err != nil { + return nil, fmt.Errorf("media: spool: %w", err) + } + if err := f.Chmod(0o600); err != nil { + f.Close() + return nil, fmt.Errorf("media: chmod spool: %w", err) + } + return f, nil +} + +// hashFile streams the digest so the id costs one buffer rather than the whole +// file. +func hashFile(path string) (string, error) { + f, err := os.Open(path) + if err != nil { + return "", fmt.Errorf("media: open spool: %w", err) + } + defer f.Close() + h := sha256.New() + if _, err := io.Copy(h, f); err != nil { + return "", fmt.Errorf("media: hash spool: %w", err) + } + return hex.EncodeToString(h.Sum(nil)), nil +} + // Get returns the blob's metadata without reading its bytes. func (s *Store) Get(id string) (Blob, error) { if !validID(id) { From 77888c1a9cfd7a7c288dd54a97afa17075a99d39 Mon Sep 17 00:00:00 2001 From: kami Date: Sat, 1 Aug 2026 14:36:17 +0400 Subject: [PATCH 10/11] capture: spool the meeting to disk, own it by token, reap it by the clock Four invariants the comments claimed and the code did not hold. The recording lived in mavend's heap as one growing []byte, doubled at Stop when the WAV was built. Frames now go to a spool file and the transcript is read back off disk one window at a time, so memory is flat whatever the length. A store failure returned before transcription ran, so a meeting over the blob cap produced no transcript, no summary and no note. It now records the failure and keeps going, and the spool file survives until the words have been read off it. The session had no owner. Any module on the write rung could call stop on a recording it did not start and receive the verbatim words of everyone in the room. Start hands back a token and append, stop and abort require it. The duration cap was only checked when a frame arrived, so a phone whose tab was closed left the slot occupied and every later start answered ErrBusy with a meeting from last week. The wall clock is checked in start, status, append and stop. Smaller things in the same pass. Append compares the frame format against the session format, so a client that switches sample rate mid meeting no longer has its frames concatenated under a header that lies. One failed STT window leaves a marker instead of discarding the other twenty four. Summarize is separate from Stop and assigns the salvaged per chunk text before it reports the error. Found in review of #73. --- internal/capture/capture.go | 391 ++++++++++++++++++++++++------- internal/capture/capture_test.go | 109 +++++---- internal/capture/session_test.go | 154 ++++++++++++ 3 files changed, 518 insertions(+), 136 deletions(-) create mode 100644 internal/capture/session_test.go diff --git a/internal/capture/capture.go b/internal/capture/capture.go index c9cb852..9f25979 100644 --- a/internal/capture/capture.go +++ b/internal/capture/capture.go @@ -16,8 +16,13 @@ // plan document and is refused: it requires listening in order to notice the // keyword, which is the exact behaviour this capability must not have. // - A session that is not stopped stops itself. MaxDuration is a hard cap -// checked on every Append, not a suggestion; a forgotten recording is a -// recording that ends, not one that runs until the disk is full. +// checked on every Append AND against the wall clock in Start and Status, +// so a client that simply stops sending frames — a browser tab closed, wifi +// gone — does not leave the one session slot occupied until mavend +// restarts. +// - A session belongs to whoever started it. Start returns a token and Append +// and Stop require it, so a second surface at the same authority rung +// cannot feed or harvest a recording it did not begin. // - Audio is stored under internal/media, which means retention prunes it and // it never leaves the box. Both the audio blob and the transcript stay // local; only the summary is written where he will read it. @@ -37,16 +42,20 @@ // // There is exactly one STT in Maven and this package does not add a second: it // takes an stt.Transcriber, which in deploy is the whisper.cpp worker behind -// cmd/mavsttd. Long audio is transcribed in windows too (see chunkAudio), for +// cmd/mavsttd. Long audio is transcribed in windows too (see transcribeFile), for // the same reason whisper itself works in 30s windows — handing a worker an hour // of PCM in one call is a request that either times out or blocks everything -// else for minutes. +// else for minutes. The windows are read back off the stored WAV one at a time, +// so the meeting is never in memory whole. package capture import ( "context" + "crypto/rand" + "encoding/hex" "errors" "fmt" + "os" "strings" "sync" "time" @@ -58,12 +67,17 @@ import ( // DefaultMaxDuration — how long one capture may run before it stops itself. // Two hours covers a long meeting and bounds the damage of a forgotten session: -// at 16 kHz mono that is about 230 MB of PCM, which is over media's default -// per-blob cap, so a session at the limit is stored truncated rather than -// refused. That trade is deliberate — a partial recording of a meeting he asked -// for beats an error after two hours. +// at 16 kHz mono that is about 230 MB of WAV, which is under media's +// DefaultMaxAudioBytes of 512 MiB. The two constants used to disagree — a +// 64 MiB blob cap is 35 minutes of audio against a 120 minute session cap — so +// the meeting that hit the limit was the one that failed to store. const DefaultMaxDuration = 2 * time.Hour +// StaleGrace — how long past MaxDuration a session may sit before Start and +// Status reap it. A frame in flight when the cap fires should not race the +// reaper, and a minute of slack costs nothing against a two-hour cap. +const StaleGrace = time.Minute + // DefaultSTTWindow — how much audio goes to the transcriber in one call. Five // minutes of 16 kHz mono is under 10 MB, transcribes in well under whisper's // own timeout on this box, and keeps the worker responsive to the voice path @@ -87,6 +101,9 @@ var ( // ErrExpired — the session hit MaxDuration and was closed. Returned from // Append so the caller stops sending; the audio collected so far is kept. ErrExpired = errors.New("capture: session reached its time limit") + // ErrWrongSession — the token does not match the running session. The + // recording belongs to the surface that started it. + ErrWrongSession = errors.New("capture: that is not your session") ) // Session — one recording in progress. Not created directly; Recorder.Start @@ -95,11 +112,53 @@ var ( type Session struct { Label string Started time.Time + // Token identifies this session to its owner. Append and Stop need it: the + // rung Append sits on is shared by every writing module, and a rung is not + // an owner. Without it any AuthWrite surface could call capture_stop on a + // meeting it did not start and be handed the verbatim transcript. + Token string mu sync.Mutex - pcm []byte + spool *os.File // the WAV being written, header first + path string + n int64 // PCM bytes written, header excluded format audio.Format expired bool + closed bool +} + +// write appends one frame to the spool file. +func (s *Session) write(b []byte) error { + if s.spool == nil { + return errors.New("capture: session has no spool file") + } + n, err := s.spool.Write(b) + s.n += int64(n) + if err != nil { + return fmt.Errorf("capture: spool write: %w", err) + } + return nil +} + +// finish closes the spool file and stamps the real WAV header over the +// placeholder Start wrote. +func (s *Session) finish() error { + if s.closed { + return nil + } + s.closed = true + if s.spool == nil { + return nil + } + defer s.spool.Close() + hdr, err := audio.WAVHeader(s.format, int(s.n)) + if err != nil { + return err + } + if _, err := s.spool.WriteAt(hdr, 0); err != nil { + return fmt.Errorf("capture: spool header: %w", err) + } + return s.spool.Sync() } // Duration is how much audio has been collected, from the bytes rather than the @@ -112,15 +171,23 @@ func (s *Session) Duration() time.Duration { } func (s *Session) duration() time.Duration { - a := audio.Audio{Format: s.format, Bytes: s.pcm} - return time.Duration(a.Duration() * float64(time.Second)) + return pcmDuration(s.format, s.n) +} + +// pcmDuration is how long n bytes of PCM lasts in the given format. +func pcmDuration(f audio.Format, n int64) time.Duration { + per := int64(f.SampleRate) * int64(f.Channels) * int64(f.SampleBits) / 8 + if per <= 0 { + return 0 + } + return time.Duration(float64(n) / float64(per) * float64(time.Second)) } // Bytes is how much PCM has been collected. For a status line. func (s *Session) Bytes() int { s.mu.Lock() defer s.mu.Unlock() - return len(s.pcm) + return int(s.n) } // Status — what a "что записываешь?" answer needs, and what /dash shows. It is @@ -141,6 +208,7 @@ type Recorder struct { sum *Summarizer maxDuration time.Duration sttWindow time.Duration + staleGrace time.Duration now func() time.Time mu sync.Mutex @@ -180,6 +248,7 @@ func New(blobs *media.Store, tr stt.Transcriber, sum *Summarizer, cfg Config) (* sum: sum, maxDuration: maxDur, sttWindow: window, + staleGrace: StaleGrace, now: time.Now, }, nil } @@ -195,19 +264,84 @@ func (r *Recorder) MaxDuration() time.Duration { return r.maxDuration } func (r *Recorder) Start(label string) (*Session, error) { r.mu.Lock() defer r.mu.Unlock() + r.reapLocked() if r.current != nil { return nil, fmt.Errorf("%w: %q since %s", ErrBusy, r.current.Label, r.current.Started.Format(time.Kitchen)) } + f, err := r.blobs.SpoolFile("capture") + if err != nil { + return nil, err + } + format := audio.PCM16kMono + hdr, err := audio.WAVHeader(format, 0) + if err != nil { + f.Close() + return nil, err + } + // The header is written first and rewritten at Stop with the real length, + // so the spool file is a playable WAV rather than headerless PCM that has + // to be copied to gain 44 bytes. + if _, err := f.Write(hdr); err != nil { + f.Close() + _ = os.Remove(f.Name()) + return nil, fmt.Errorf("capture: spool header: %w", err) + } + token, err := newToken() + if err != nil { + f.Close() + _ = os.Remove(f.Name()) + return nil, err + } s := &Session{ Label: strings.TrimSpace(label), Started: r.now().UTC(), - format: audio.PCM16kMono, + Token: token, + spool: f, + path: f.Name(), + format: format, } r.current = s return s, nil } +// newToken mints a session token. Sixteen random bytes: it is a capability +// handed back over the same socket the call came in on, not a secret at rest. +func newToken() (string, error) { + var b [16]byte + if _, err := rand.Read(b[:]); err != nil { + return "", fmt.Errorf("capture: token: %w", err) + } + return hex.EncodeToString(b[:]), nil +} + +// reapLocked drops a session whose wall clock ran past MaxDuration. The +// frame-driven check in Append only fires while frames arrive, so a client that +// simply stopped sending — a phone whose browser tab was closed, wifi gone — +// left the slot occupied and every later Start answering ErrBusy with a meeting +// from last Tuesday. r.mu must be held. +func (r *Recorder) reapLocked() { + s := r.current + if s == nil { + return + } + if r.now().UTC().Sub(s.Started) < r.maxDuration+r.staleGrace { + return + } + s.mu.Lock() + s.expired = true + _ = s.finish() + path := s.path + s.mu.Unlock() + if path != "" { + // The audio goes with it. A recording nobody stopped is one nobody is + // waiting for, and keeping it would mean storing a meeting on the + // strength of a dropped connection. + _ = os.Remove(path) + } + r.current = nil +} + // Append adds one frame to the running session. ErrNoSession when nothing is // running, which is the guard that makes an ambient path impossible: a stream // arriving at a Recorder nobody started is refused frame by frame. @@ -215,25 +349,39 @@ func (r *Recorder) Start(label string) (*Session, error) { // ErrExpired once the session is at MaxDuration. The audio collected so far is // kept and Stop still works — the cap ends the recording, it does not throw it // away. -func (r *Recorder) Append(a audio.Audio) error { +func (r *Recorder) Append(token string, a audio.Audio) error { if !a.Format.IsValid() { return fmt.Errorf("%w: %+v", ErrBadFormat, a.Format) } r.mu.Lock() + r.reapLocked() s := r.current r.mu.Unlock() if s == nil { return ErrNoSession } + if token != s.Token { + return ErrWrongSession + } s.mu.Lock() defer s.mu.Unlock() if s.expired { return ErrExpired } - s.pcm = append(s.pcm, a.Bytes...) + // The session fixed its format at Start. A client that switches sample rate + // mid-session used to have its frames concatenated into the same buffer: + // duration() then read the whole thing at the original rate, the stored WAV + // header lied, and the cap fired at the wrong length. + if a.Format != s.format { + return fmt.Errorf("%w: session is %+v, frame is %+v", ErrBadFormat, s.format, a.Format) + } + if err := s.write(a.Bytes); err != nil { + return err + } if s.duration() >= r.maxDuration { s.expired = true + _ = s.finish() return ErrExpired } return nil @@ -242,6 +390,7 @@ func (r *Recorder) Append(a audio.Audio) error { // Status reports the running session, or Running=false. func (r *Recorder) Status() Status { r.mu.Lock() + r.reapLocked() s := r.current r.mu.Unlock() if s == nil { @@ -273,6 +422,10 @@ type Result struct { // Chunks — how many windows the transcript was summarised in. 1 means it fit // in one prompt. Reported so a suspiciously vague summary can be explained. Chunks int + // StoreErr — why the audio was not kept, when it was not. The transcript is + // still produced in that case, so this is the difference between "no blob + // because storing failed" and "no blob because nothing was recorded". + StoreErr error } // Stop ends the session and produces the result: store the audio, transcribe it @@ -280,11 +433,18 @@ type Result struct { // the slow work starts, so a stuck model cannot block the next recording. // // The order matters and is the same as vision's: the audio is stored FIRST. If -// transcription or summarisation fails, the recording is still on disk and can -// be run again; a meeting that happened once must not be lost to a model error. -func (r *Recorder) Stop(ctx context.Context) (Result, error) { +// transcription fails, the recording is still on disk under media.retention, so +// the meeting is not lost to a model error. Note that re-running it is a manual +// job today: no method takes a blob id back, unlike vision's Rerun, and the blob +// prunes on the media retention like any other. +func (r *Recorder) Stop(ctx context.Context, token string) (Result, error) { r.mu.Lock() + r.reapLocked() s := r.current + if s != nil && token != s.Token { + r.mu.Unlock() + return Result{}, ErrWrongSession + } r.current = nil r.mu.Unlock() if s == nil { @@ -292,113 +452,168 @@ func (r *Recorder) Stop(ctx context.Context) (Result, error) { } s.mu.Lock() - pcm := s.pcm + err := s.finish() + path := s.path format := s.format + n := s.n s.mu.Unlock() res := Result{Label: s.Label, Started: s.Started} - if len(pcm) == 0 { + if err != nil { + _ = os.Remove(path) + return res, err + } + if n == 0 { + _ = os.Remove(path) return res, ErrEmptyCapture } - full := audio.Audio{Format: format, Bytes: pcm} - res.Duration = time.Duration(full.Duration() * float64(time.Second)) + res.Duration = pcmDuration(format, n) - // Stored as WAV, not headerless PCM: a blob on disk that `aplay` and whisper - // can both open without being told the format is worth 44 bytes. - wav, err := audio.WAVFromPCM(format, pcm) - if err != nil { - return res, fmt.Errorf("capture: wav: %w", err) + // The audio is stored first, as vision does, so a transcription or summary + // failure leaves something to run again. It moves rather than being read + // into memory: a two-hour meeting is a couple of hundred megabytes, and + // this is the process that owns the database and the resident model. + audioPath := path + blob, perr := r.blobs.PutFile(media.KindAudio, "audio/wav", "capture:meeting", path) + if perr == nil { + res.BlobID = blob.ID + audioPath = blob.Path + } else { + // Over the cap, or the store is full. Report it and KEEP GOING: this + // used to return, so the one case the audio cap actually fires on — a + // very long meeting — produced no transcript, no summary and no note, + // which is the whole point of the capability. The spool file stays + // until the transcript has been read off it. + res.StoreErr = perr + defer os.Remove(audioPath) } - blob, err := r.blobs.Put(media.KindAudio, "audio/wav", "capture:meeting", wav) - if err != nil { - // Over the per-blob cap is the expected case for a very long meeting. - // Report it and keep going: a transcript without the audio still beats - // nothing, and the words are what he will read. - return res, fmt.Errorf("capture: store audio: %w", err) - } - res.BlobID = blob.ID - text, err := r.transcribe(ctx, full) - if err != nil { - return res, fmt.Errorf("capture: transcribe: %w", err) - } + text, terr := r.transcribeFile(ctx, audioPath, format, n) res.Transcript = text + if terr != nil { + return res, fmt.Errorf("capture: transcribe: %w", terr) + } if strings.TrimSpace(text) == "" { return res, ErrEmptyCapture } - - if r.sum == nil { - return res, nil + if perr != nil { + return res, fmt.Errorf("capture: store audio: %w", perr) } - summary, chunks, err := r.sum.Summarize(ctx, s.Label, text) - res.Chunks = chunks - if err != nil { - // Degraded success: the transcript is real and stored, only the summary - // is missing. The caller writes the transcript note and says so. - return res, fmt.Errorf("capture: summarize: %w", err) - } - res.Summary = summary return res, nil } +// Summarize runs the map-reduce over a transcript. It is separate from Stop so +// the daemon can answer the stop quickly and do the model work afterwards: a +// full map-reduce is up to forty model calls, and a voice turn that says +// "хватит" should not wait minutes for the reply. +// +// The salvaged text a failed reduce returns is assigned before the error is +// checked. Summarize hands back the per-chunk summaries with its error +// precisely so they are not lost, and the caller used to throw them away. +func (r *Recorder) Summarize(ctx context.Context, res *Result) error { + if r.sum == nil || strings.TrimSpace(res.Transcript) == "" { + return nil + } + summary, chunks, err := r.sum.Summarize(ctx, res.Label, res.Transcript) + res.Chunks = chunks + res.Summary = summary + if err != nil { + return fmt.Errorf("capture: summarize: %w", err) + } + return nil +} + // Abort throws the running session away without transcribing or storing it. // This is what "забудь, не записывай" must map to: a recording someone changed // their mind about leaves nothing behind, not a blob with a note saying it was // abandoned. Returns whether anything was running. -func (r *Recorder) Abort() bool { +func (r *Recorder) Abort(token string) bool { r.mu.Lock() defer r.mu.Unlock() - if r.current == nil { + r.reapLocked() + s := r.current + if s == nil || token != s.Token { return false } r.current = nil + s.mu.Lock() + _ = s.finish() + path := s.path + s.mu.Unlock() + if path != "" { + _ = os.Remove(path) + } return true } -// transcribe runs the transcriber over the audio in windows and joins the text. -// A window that fails is fatal: a summary of a meeting with a silent hole in the -// middle is a summary that misleads. -func (r *Recorder) transcribe(ctx context.Context, a audio.Audio) (string, error) { - windows := chunkAudio(a, r.sttWindow) - parts := make([]string, 0, len(windows)) - for i, w := range windows { - text, _, err := r.tr.Transcribe(ctx, w) +// transcribeFile runs the transcriber over the stored WAV in windows and joins +// the text, reading one window at a time off disk so the meeting is never in +// memory whole. +// +// A window that fails is no longer fatal. It used to be, on the argument that a +// silent hole misleads — but the cost was 24 good windows thrown away for one +// whisper hiccup at minute 100. The hole is marked in the text instead, which +// keeps the words and stays honest about the gap. +func (r *Recorder) transcribeFile(ctx context.Context, path string, format audio.Format, n int64) (string, error) { + f, err := os.Open(path) + if err != nil { + return "", fmt.Errorf("open audio: %w", err) + } + defer f.Close() + + per := windowBytes(format, r.sttWindow) + if per <= 0 || per > n { + per = n + } + total := int((n + per - 1) / per) + buf := make([]byte, per) + parts := make([]string, 0, total) + failed := 0 + for i, off := 0, int64(0); off < n; i, off = i+1, off+per { + size := per + if off+size > n { + size = n - off + } + // Never cut mid-sample: a split inside an int16 shifts every following + // sample by a byte and turns the tail of the window into noise. + if bps := int64(format.SampleBits / 8 * format.Channels); bps > 0 { + size -= size % bps + } + if size <= 0 { + break + } + if _, err := f.ReadAt(buf[:size], int64(audio.WAVHeaderSize)+off); err != nil { + return strings.Join(parts, " "), fmt.Errorf("window %d/%d: %w", i+1, total, err) + } + text, _, err := r.tr.Transcribe(ctx, audio.Audio{Format: format, Bytes: buf[:size]}) if err != nil { - return "", fmt.Errorf("window %d/%d: %w", i+1, len(windows), err) + if ctx.Err() != nil { + return strings.Join(parts, " "), fmt.Errorf("window %d/%d: %w", i+1, total, err) + } + failed++ + parts = append(parts, gapMarker) + continue } if t := strings.TrimSpace(text); t != "" { parts = append(parts, t) } } + if failed == total { + return "", fmt.Errorf("every one of %d window(s) failed", total) + } return strings.Join(parts, " "), nil } -// chunkAudio splits audio into windows of at most window duration, cut on -// sample boundaries. A window shorter than one sample is impossible; audio -// shorter than one window comes back as a single element, so the caller never -// special-cases the short case. -func chunkAudio(a audio.Audio, window time.Duration) []audio.Audio { - bytesPerSample := a.Format.SampleBits / 8 * a.Format.Channels - if bytesPerSample <= 0 || a.Format.SampleRate <= 0 || window <= 0 { - return []audio.Audio{a} +// gapMarker stands in for a window whisper could not read. Russian, because it +// is read by him in a note next to the words around it. +const gapMarker = "[…не разобрала…]" + +// windowBytes is how many PCM bytes one STT window holds. +func windowBytes(f audio.Format, window time.Duration) int64 { + bps := int64(f.SampleBits / 8 * f.Channels) + if bps <= 0 || f.SampleRate <= 0 || window <= 0 { + return 0 } - per := int(window.Seconds()) * a.Format.SampleRate * bytesPerSample - if per <= 0 || len(a.Bytes) <= per { - return []audio.Audio{a} - } - var out []audio.Audio - for off := 0; off < len(a.Bytes); off += per { - end := off + per - if end > len(a.Bytes) { - end = len(a.Bytes) - } - // Never cut mid-sample: a split inside an int16 shifts every following - // sample by a byte and turns the tail of the window into noise. - end -= (end - off) % bytesPerSample - if end <= off { - break - } - out = append(out, audio.Audio{Format: a.Format, Bytes: a.Bytes[off:end]}) - } - return out + per := int64(window.Seconds()) * int64(f.SampleRate) * bps + return per - per%bps } diff --git a/internal/capture/capture_test.go b/internal/capture/capture_test.go index e1c4268..998de21 100644 --- a/internal/capture/capture_test.go +++ b/internal/capture/capture_test.go @@ -90,7 +90,7 @@ func TestNewRequiresStoreAndTranscriber(t *testing.T) { // is refused. There is no ambient path in. func TestAppendWithoutStartIsRefused(t *testing.T) { r, _ := testRecorder(t, &fakeTranscriber{}, nil, Config{}) - if err := r.Append(frame(1)); !errors.Is(err, ErrNoSession) { + if err := r.Append("no-token", frame(1)); !errors.Is(err, ErrNoSession) { t.Fatalf("got %v, want ErrNoSession", err) } if r.Status().Running { @@ -100,20 +100,21 @@ func TestAppendWithoutStartIsRefused(t *testing.T) { func TestStopWithoutStartIsRefused(t *testing.T) { r, _ := testRecorder(t, &fakeTranscriber{}, nil, Config{}) - if _, err := r.Stop(context.Background()); !errors.Is(err, ErrNoSession) { + if _, err := r.Stop(context.Background(), "no-token"); !errors.Is(err, ErrNoSession) { t.Fatalf("got %v, want ErrNoSession", err) } } func TestOneSessionAtATime(t *testing.T) { r, _ := testRecorder(t, &fakeTranscriber{}, nil, Config{}) - if _, err := r.Start("встреча"); err != nil { + s, err := r.Start("встреча") + if err != nil { t.Fatal(err) } if _, err := r.Start("вторая"); !errors.Is(err, ErrBusy) { t.Fatalf("got %v, want ErrBusy", err) } - if _, err := r.Stop(context.Background()); !errors.Is(err, ErrEmptyCapture) { + if _, err := r.Stop(context.Background(), s.Token); !errors.Is(err, ErrEmptyCapture) { t.Fatalf("empty stop: %v", err) } // The slot is free again after a stop, even a failed one. @@ -127,18 +128,22 @@ func TestRoundTripStoresAudioTranscriptAndSummary(t *testing.T) { sum := NewSummarizer(&fakeCompleter{replies: []string{"— решили купить насос"}}, 0, 0, nil) r, blobs := testRecorder(t, tr, sum, Config{}) - if _, err := r.Start("встреча с подрядчиком"); err != nil { + s, err := r.Start("встреча с подрядчиком") + if err != nil { t.Fatal(err) } for i := 0; i < 3; i++ { - if err := r.Append(frame(2)); err != nil { + if err := r.Append(s.Token, frame(2)); err != nil { t.Fatal(err) } } - res, err := r.Stop(context.Background()) + res, err := r.Stop(context.Background(), s.Token) if err != nil { t.Fatalf("stop: %v", err) } + if err := r.Summarize(context.Background(), &res); err != nil { + t.Fatalf("summarize: %v", err) + } if res.BlobID == "" { t.Error("no audio blob stored") } @@ -171,21 +176,22 @@ func TestRoundTripStoresAudioTranscriptAndSummary(t *testing.T) { func TestMaxDurationEndsTheSessionAndKeepsAudio(t *testing.T) { tr := &fakeTranscriber{} r, _ := testRecorder(t, tr, nil, Config{MaxDuration: 4 * time.Second}) - if _, err := r.Start("длинная"); err != nil { + s, err := r.Start("длинная") + if err != nil { t.Fatal(err) } - if err := r.Append(frame(3)); err != nil { + if err := r.Append(s.Token, frame(3)); err != nil { t.Fatalf("first frame: %v", err) } - if err := r.Append(frame(3)); !errors.Is(err, ErrExpired) { + if err := r.Append(s.Token, frame(3)); !errors.Is(err, ErrExpired) { t.Fatalf("got %v, want ErrExpired", err) } // Further frames keep being refused, so a client that ignores the error // cannot grow the recording past the cap. - if err := r.Append(frame(3)); !errors.Is(err, ErrExpired) { + if err := r.Append(s.Token, frame(3)); !errors.Is(err, ErrExpired) { t.Fatalf("post-expiry frame: %v", err) } - res, err := r.Stop(context.Background()) + res, err := r.Stop(context.Background(), s.Token) if err != nil { t.Fatalf("stop after expiry: %v", err) } @@ -196,11 +202,12 @@ func TestMaxDurationEndsTheSessionAndKeepsAudio(t *testing.T) { func TestAppendRejectsWrongFormat(t *testing.T) { r, _ := testRecorder(t, &fakeTranscriber{}, nil, Config{}) - if _, err := r.Start("x"); err != nil { + s, err := r.Start("x") + if err != nil { t.Fatal(err) } bad := audio.Audio{Format: audio.Format{SampleRate: 44100, Channels: 2, SampleBits: 16, Encoding: "pcm_s16le"}, Bytes: make([]byte, 100)} - if err := r.Append(bad); !errors.Is(err, ErrBadFormat) { + if err := r.Append(s.Token, bad); !errors.Is(err, ErrBadFormat) { t.Fatalf("got %v, want ErrBadFormat", err) } } @@ -209,13 +216,14 @@ func TestAppendRejectsWrongFormat(t *testing.T) { func TestAbortLeavesNothing(t *testing.T) { tr := &fakeTranscriber{} r, blobs := testRecorder(t, tr, nil, Config{}) - if _, err := r.Start("зря начали"); err != nil { + s, err := r.Start("зря начали") + if err != nil { t.Fatal(err) } - if err := r.Append(frame(5)); err != nil { + if err := r.Append(s.Token, frame(5)); err != nil { t.Fatal(err) } - if !r.Abort() { + if !r.Abort(s.Token) { t.Fatal("Abort reported nothing running") } if r.Status().Running { @@ -231,7 +239,7 @@ func TestAbortLeavesNothing(t *testing.T) { if tr.calls != 0 { t.Errorf("Abort transcribed anyway (%d calls)", tr.calls) } - if r.Abort() { + if r.Abort(s.Token) { t.Error("second Abort reported a session") } } @@ -241,10 +249,11 @@ func TestStatusReportsTheRunningSession(t *testing.T) { if got := r.Status(); got.Running { t.Error("idle recorder reports running") } - if _, err := r.Start("планёрка"); err != nil { + s, err := r.Start("планёрка") + if err != nil { t.Fatal(err) } - if err := r.Append(frame(10)); err != nil { + if err := r.Append(s.Token, frame(10)); err != nil { t.Fatal(err) } st := r.Status() @@ -264,13 +273,14 @@ func TestStatusReportsTheRunningSession(t *testing.T) { func TestLongAudioIsTranscribedInWindows(t *testing.T) { tr := &fakeTranscriber{} r, _ := testRecorder(t, tr, nil, Config{STTWindow: 2 * time.Second}) - if _, err := r.Start("длинная"); err != nil { + s, err := r.Start("длинная") + if err != nil { t.Fatal(err) } - if err := r.Append(frame(9)); err != nil { + if err := r.Append(s.Token, frame(9)); err != nil { t.Fatal(err) } - res, err := r.Stop(context.Background()) + res, err := r.Stop(context.Background(), s.Token) if err != nil { t.Fatalf("stop: %v", err) } @@ -282,18 +292,19 @@ func TestLongAudioIsTranscribedInWindows(t *testing.T) { } } -// A hole in the middle of a meeting summary would mislead, so a failed window is -// fatal — but the audio is already stored and re-runnable. +// Every window failing is a transcription failure — but the audio is already +// stored and re-runnable. func TestTranscriptionFailureKeepsTheAudio(t *testing.T) { tr := &fakeTranscriber{err: errors.New("whisper is down")} r, blobs := testRecorder(t, tr, nil, Config{}) - if _, err := r.Start("встреча"); err != nil { + s, err := r.Start("встреча") + if err != nil { t.Fatal(err) } - if err := r.Append(frame(2)); err != nil { + if err := r.Append(s.Token, frame(2)); err != nil { t.Fatal(err) } - res, err := r.Stop(context.Background()) + res, err := r.Stop(context.Background(), s.Token) if err == nil { t.Fatal("transcription failure was not reported") } @@ -309,16 +320,20 @@ func TestTranscriptionFailureKeepsTheAudio(t *testing.T) { // error. func TestNoSummarizerStillProducesATranscript(t *testing.T) { r, _ := testRecorder(t, &fakeTranscriber{}, nil, Config{}) - if _, err := r.Start("встреча"); err != nil { + s, err := r.Start("встреча") + if err != nil { t.Fatal(err) } - if err := r.Append(frame(1)); err != nil { + if err := r.Append(s.Token, frame(1)); err != nil { t.Fatal(err) } - res, err := r.Stop(context.Background()) + res, err := r.Stop(context.Background(), s.Token) if err != nil { t.Fatalf("stop: %v", err) } + if err := r.Summarize(context.Background(), &res); err != nil { + t.Fatalf("summarize with no summarizer: %v", err) + } if res.Transcript == "" { t.Error("no transcript") } @@ -331,14 +346,18 @@ func TestNoSummarizerStillProducesATranscript(t *testing.T) { func TestSummaryFailureStillReturnsTheTranscript(t *testing.T) { sum := NewSummarizer(&fakeCompleter{err: errors.New("llama is down")}, 0, 0, nil) r, _ := testRecorder(t, &fakeTranscriber{}, sum, Config{}) - if _, err := r.Start("встреча"); err != nil { + s, err := r.Start("встреча") + if err != nil { t.Fatal(err) } - if err := r.Append(frame(1)); err != nil { + if err := r.Append(s.Token, frame(1)); err != nil { t.Fatal(err) } - res, err := r.Stop(context.Background()) - if err == nil { + res, err := r.Stop(context.Background(), s.Token) + if err != nil { + t.Fatalf("stop: %v", err) + } + if err := r.Summarize(context.Background(), &res); err == nil { t.Fatal("summary failure was not reported") } if res.Transcript == "" { @@ -346,18 +365,12 @@ func TestSummaryFailureStillReturnsTheTranscript(t *testing.T) { } } -func TestChunkAudioNeverCutsMidSample(t *testing.T) { - a := audio.Audio{Format: audio.PCM16kMono, Bytes: make([]byte, 16000*2*5+1)} - for _, w := range chunkAudio(a, 2*time.Second) { - if len(w.Bytes)%2 != 0 { - t.Fatalf("window of %d bytes cuts an int16 in half", len(w.Bytes)) - } - } -} - -func TestChunkAudioShortInputIsOneWindow(t *testing.T) { - a := frame(1) - if got := chunkAudio(a, time.Minute); len(got) != 1 { - t.Errorf("got %d windows, want 1", len(got)) +func TestWindowBytesNeverCutsMidSample(t *testing.T) { + if got := windowBytes(audio.PCM16kMono, 2*time.Second); got%2 != 0 || got != 2*16000*2 { + t.Fatalf("windowBytes = %d", got) + } + odd := audio.Format{SampleRate: 16000, Channels: 1, SampleBits: 16, Encoding: "pcm_s16le"} + if got := windowBytes(odd, 0); got != 0 { + t.Fatalf("a zero window must produce zero, got %d", got) } } diff --git a/internal/capture/session_test.go b/internal/capture/session_test.go new file mode 100644 index 0000000..de31658 --- /dev/null +++ b/internal/capture/session_test.go @@ -0,0 +1,154 @@ +package capture + +import ( + "context" + "errors" + "fmt" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/kami/maven/internal/audio" + "github.com/kami/maven/internal/media" +) + +// A frame for a session that already ended must not land in the next one. The +// recorder used to be addressed as "whatever is running now", so a client whose +// session was reaped went on appending its microphone into a meeting somebody +// else had started. +func TestAppendWithTheWrongTokenIsRefused(t *testing.T) { + r, _ := testRecorder(t, &fakeTranscriber{}, nil, Config{}) + s, err := r.Start("первая") + if err != nil { + t.Fatal(err) + } + if err := r.Append("someone-elses-token", frame(1)); !errors.Is(err, ErrWrongSession) { + t.Fatalf("append = %v, want ErrWrongSession", err) + } + if _, err := r.Stop(context.Background(), "someone-elses-token"); !errors.Is(err, ErrWrongSession) { + t.Fatalf("stop = %v, want ErrWrongSession", err) + } + if r.Abort("someone-elses-token") { + t.Fatal("Abort discarded a session it does not own") + } + if err := r.Append(s.Token, frame(1)); err != nil { + t.Fatalf("the owner is still refused: %v", err) + } +} + +// A client that simply stops sending — a phone whose tab was closed — used to +// hold the single session slot forever, and every later Start answered ErrBusy +// with a meeting from last week. +func TestStaleSessionIsReapedByTheWallClock(t *testing.T) { + r, _ := testRecorder(t, &fakeTranscriber{}, nil, Config{MaxDuration: time.Minute}) + now := time.Now().UTC() + r.now = func() time.Time { return now } + s, err := r.Start("брошенная") + if err != nil { + t.Fatal(err) + } + if _, err := r.Start("вторая"); !errors.Is(err, ErrBusy) { + t.Fatalf("start = %v, want ErrBusy", err) + } + now = now.Add(time.Minute + StaleGrace + time.Second) + next, err := r.Start("вторая") + if err != nil { + t.Fatalf("a stale session was not reaped: %v", err) + } + if next.Token == s.Token { + t.Fatal("the new session reused the stale token") + } + if err := r.Append(s.Token, frame(1)); !errors.Is(err, ErrWrongSession) { + t.Fatalf("the reaped client can still write: %v", err) + } + // The abandoned recording is not kept: nobody is waiting for it, and storing + // it would mean keeping a meeting on the strength of a dropped connection. + if _, err := os.Stat(s.path); !os.IsNotExist(err) { + t.Fatalf("the reaped spool file survived: %v", err) + } +} + +// One window failing used to fail the whole transcription, which threw away +// every other window of an hour-long meeting. The hole is marked instead, so the +// summary cannot silently read as if nothing was missing. +func TestOneFailedWindowIsMarkedNotFatal(t *testing.T) { + r, _ := testRecorder(t, &fakeTranscriber{}, nil, Config{STTWindow: time.Second}) + r.tr = &windowTranscriber{failOn: 2} + s, err := r.Start("встреча") + if err != nil { + t.Fatal(err) + } + if err := r.Append(s.Token, frame(3)); err != nil { + t.Fatal(err) + } + res, err := r.Stop(context.Background(), s.Token) + if err != nil { + t.Fatalf("stop: %v", err) + } + if !strings.Contains(res.Transcript, gapMarker) { + t.Errorf("no gap marker in %q", res.Transcript) + } + if !strings.Contains(res.Transcript, "окно1") || !strings.Contains(res.Transcript, "окно3") { + t.Errorf("the surviving windows were dropped: %q", res.Transcript) + } +} + +// The audio not fitting the store is not a reason to lose the words. Stop used +// to return early on a store failure, so a recording over the blob cap produced +// neither a blob nor a transcript. +func TestStoreFailureStillTranscribes(t *testing.T) { + blobs, err := media.OpenWithBudget(t.TempDir(), 512, 1024, 0) + if err != nil { + t.Fatal(err) + } + tr := &fakeTranscriber{} + r, err := New(blobs, tr, nil, Config{}) + if err != nil { + t.Fatal(err) + } + s, err := r.Start("длинная встреча") + if err != nil { + t.Fatal(err) + } + if err := r.Append(s.Token, frame(2)); err != nil { + t.Fatal(err) + } + // The store failure is reported, but as a degraded success: the Result is + // filled in, and the caller keeps it rather than treating the error as + // nothing having happened. + res, err := r.Stop(context.Background(), s.Token) + if err == nil { + t.Fatal("the store failure was not reported") + } + if res.BlobID != "" { + t.Errorf("blob id = %q, want none", res.BlobID) + } + if !errors.Is(res.StoreErr, media.ErrTooLarge) { + t.Errorf("StoreErr = %v, want ErrTooLarge", res.StoreErr) + } + if res.Transcript == "" { + t.Fatal("the words were lost with the audio") + } + // The spool file is cleaned up even on the failure path. + glob, _ := filepath.Glob(filepath.Join(blobs.Dir(), "spool", "*")) + if len(glob) != 0 { + t.Errorf("spool leaked: %v", glob) + } +} + +// windowTranscriber answers per window and fails a chosen one, which is what a +// whisper timeout in the middle of a meeting looks like. +type windowTranscriber struct { + calls int + failOn int +} + +func (w *windowTranscriber) Transcribe(_ context.Context, _ audio.Audio) (string, float64, error) { + w.calls++ + if w.calls == w.failOn { + return "", 0, errors.New("whisper timed out") + } + return fmt.Sprintf("окно%d", w.calls), 1.0, nil +} From 2ca5ffa4f9fef4654250fc88f6c1598b3676d210 Mon Sep 17 00:00:00 2001 From: kami Date: Sat, 1 Aug 2026 14:36:17 +0400 Subject: [PATCH 11/11] capture: answer the stop before summarising, and always leave a note MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit capture_stop held the IPC request open for the whole map reduce, up to twenty minutes. A voice turn that says "хватит" waited for forty model calls before Maven said anything. Stop now returns the transcript and the summary runs on a goroutine in the daemon's WaitGroup, on the daemon context so a client that hung up does not cancel the only readable record of the meeting. With no summary and save_transcript false, writeNotes wrote nothing at all: an hour of meeting left a blob that prunes in seven days and no trace in the note store. The transcript is written instead when the summary is missing. That flag decides whether the verbatim record is kept in addition to a summary, not whether the meeting is remembered. The wire carries the session token now, and the contract comments say what the code does: the summary is usually absent from the stop response, and re running a stored blob is a manual job because no method takes a blob id. The save_transcript comment says the cost is recall corpus rather than disk. Found in review of #73. --- cmd/mavend/capture.go | 94 ++++++++++++++++++++------ cmd/mavend/capture_test.go | 118 +++++++++++++++++++++++++++++++++ cmd/mavend/main.go | 4 +- internal/config/config.go | 13 ++-- internal/config/senses_test.go | 16 ++--- internal/ipc/api.go | 29 ++++++-- internal/ipc/client.go | 7 +- 7 files changed, 235 insertions(+), 46 deletions(-) create mode 100644 cmd/mavend/capture_test.go diff --git a/cmd/mavend/capture.go b/cmd/mavend/capture.go index de53a70..2393976 100644 --- a/cmd/mavend/capture.go +++ b/cmd/mavend/capture.go @@ -34,6 +34,7 @@ import ( "errors" "fmt" "log" + "sync" "time" "github.com/kami/maven/internal/capture" @@ -45,10 +46,12 @@ import ( "github.com/kami/maven/internal/store" ) -// captureSummaryTimeout — the budget for one Stop, which is a map-reduce over +// captureSummaryTimeout — the budget for one summary, which is a map-reduce over // the whole meeting: one model call per transcript window plus a reduce, each of // which is seconds on this box. Forty windows is the configured ceiling, so the -// budget has to be minutes, not the 60s the reply path uses. +// budget has to be minutes, not the 60s the reply path uses. It is spent on a +// background goroutine, never inside the capture_stop request: a client that +// asks Maven to stop recording gets the transcript back in seconds. const captureSummaryTimeout = 20 * time.Minute // llmCompleter adapts *llm.Client to capture.Completer. The pure package names @@ -70,6 +73,12 @@ type captureWiring struct { emb router.Embedder cfg *config.CaptureConfig now func() time.Time + + // ctx and wg belong to the daemon, not to the request. Summarising happens + // after the reply has gone out, so it needs a lifetime that outlives the + // call and a shutdown that waits for it. + ctx context.Context + wg *sync.WaitGroup } // newCaptureWiring returns nil when the recorder should not exist: no media @@ -79,7 +88,7 @@ type captureWiring struct { // recording is still made, stored and transcribed, and the summary is simply // absent — the honest degradation, and much better than refusing to record a // meeting that is happening now. -func newCaptureWiring(keeper *mediaKeeper, st *store.Store, voiceW *voiceWiring, phr phraser.Phraser, emb router.Embedder, cfg *config.Config) *captureWiring { +func newCaptureWiring(ctx context.Context, wg *sync.WaitGroup, keeper *mediaKeeper, st *store.Store, voiceW *voiceWiring, phr phraser.Phraser, emb router.Embedder, cfg *config.Config) *captureWiring { if keeper == nil || !cfg.Capture.Records() { return nil } @@ -113,7 +122,7 @@ func newCaptureWiring(keeper *mediaKeeper, st *store.Store, voiceW *voiceWiring, return nil } log.Printf("capture: enabled, sessions capped at %s", rec.MaxDuration()) - return &captureWiring{rec: rec, st: st, emb: emb, cfg: cc, now: time.Now} + return &captureWiring{rec: rec, st: st, emb: emb, cfg: cc, now: time.Now, ctx: ctx, wg: wg} } // start handles ipc.MethodCaptureStart. @@ -127,6 +136,7 @@ func (c *captureWiring) start(_ context.Context, req ipc.CaptureStartReq) (ipc.C return ipc.CaptureStartResp{ Label: s.Label, Started: s.Started, + Token: s.Token, MaxSeconds: int(c.rec.MaxDuration().Seconds()), }, nil } @@ -135,7 +145,7 @@ func (c *captureWiring) start(_ context.Context, req ipc.CaptureStartReq) (ipc.C // response with Expired set rather than an error: the cap firing is the designed // behaviour, and the client needs the flag to stop sending and call stop. func (c *captureWiring) append(_ context.Context, req ipc.CaptureAppendReq) (ipc.CaptureAppendResp, error) { - err := c.rec.Append(req.Audio) + err := c.rec.Append(req.Token, req.Audio) st := c.rec.Status() if errors.Is(err, capture.ErrExpired) { log.Printf("capture: %q hit the %s cap — stopping", st.Label, c.rec.MaxDuration()) @@ -150,21 +160,26 @@ func (c *captureWiring) append(_ context.Context, req ipc.CaptureAppendReq) (ipc // stop handles ipc.MethodCaptureStop. // // The error handling here mirrors vision's, and for the same reason: the audio is -// stored first, so a transcription or summary failure returns what exists rather -// than nothing. A response can carry a blob id with no transcript (STT failed, -// re-runnable), or a transcript with no summary (the model failed, the words are -// kept) — both are degraded successes and neither is an error to the caller. +// stored first, so a transcription failure returns what exists rather than +// nothing. A response can carry a blob id with no transcript (STT failed, +// re-runnable) — a degraded success, not an error to the caller. +// +// Summarising is NOT done here. A two-hour meeting is forty model calls, which +// on this box is minutes, and holding the IPC request open for them means the +// client that said "стоп" sits there with no answer while its own deadline runs +// out. Stop returns the transcript, and the summary note is written by a +// goroutine in the daemon's WaitGroup afterwards. func (c *captureWiring) stop(ctx context.Context, req ipc.CaptureStopReq) (ipc.CaptureStopResp, error) { if req.Discard { // "забудь, не записывай" — nothing is stored, transcribed or noted. - if !c.rec.Abort() { + if !c.rec.Abort(req.Token) { return ipc.CaptureStopResp{}, capture.ErrNoSession } log.Printf("capture: session discarded on request") return ipc.CaptureStopResp{Discarded: true}, nil } - res, err := c.rec.Stop(ctx) + res, err := c.rec.Stop(ctx, req.Token) resp := ipc.CaptureStopResp{ BlobID: res.BlobID, Label: res.Label, @@ -183,19 +198,47 @@ func (c *captureWiring) stop(ctx context.Context, req ipc.CaptureStopReq) (ipc.C log.Printf("capture: %q partially finished: %v", res.Label, err) } - if id, werr := c.writeNotes(ctx, res); werr != nil { - log.Printf("capture: note write for %q failed: %v", res.Label, werr) - } else { - resp.NoteID = id - } - log.Printf("capture: finished %q — %s of audio, %d summary chunk(s)", - res.Label, res.Duration.Round(time.Second), res.Chunks) + c.summarizeLater(res) + log.Printf("capture: finished %q — %s of audio, %d bytes of transcript", + res.Label, res.Duration.Round(time.Second), len(res.Transcript)) return resp, nil } +// summarizeLater runs the map-reduce and writes the notes after stop replied. +// The context is the daemon's, not the request's: the request is already +// answered, and cancelling the summary because the client hung up would throw +// away the only readable record of the meeting. +func (c *captureWiring) summarizeLater(res capture.Result) { + if res.Transcript == "" { + return + } + c.wg.Add(1) + go func() { + defer c.wg.Done() + ctx, cancel := context.WithTimeout(c.ctx, captureSummaryTimeout) + defer cancel() + if err := c.rec.Summarize(ctx, &res); err != nil { + // Not fatal: writeNotes falls back to the transcript, so a dead + // llama-server costs the summary and not the meeting. + log.Printf("capture: summary for %q failed: %v", res.Label, err) + } + if _, err := c.writeNotes(ctx, res); err != nil { + log.Printf("capture: note write for %q failed: %v", res.Label, err) + return + } + log.Printf("capture: summarised %q in %d chunk(s)", res.Label, res.Chunks) + }() +} + // writeNotes stores the summary as a note, and the transcript too when -// capture.save_transcript is set. Returns the summary note's id, or 0 when there -// was no summary to write. +// capture.save_transcript is set. Returns the id of the note that carries the +// meeting. +// +// With no summary the transcript is written instead, whatever save_transcript +// says. That flag is about keeping the verbatim record IN ADDITION to a summary, +// not about whether the meeting is remembered at all. Without this fallback a +// llama-server that was down at stop time meant an hour of recorded meeting left +// no note behind and nothing recalled it later. // // The note source carries the blob id, which is the only link back to the audio. // When retention prunes the blob the note remains — words about a meeting are a @@ -212,6 +255,13 @@ func (c *captureWiring) writeNotes(ctx context.Context, res capture.Result) (int if err != nil { return 0, fmt.Errorf("summary note: %w", err) } + } else if res.Transcript != "" { + var err error + id, err = c.writeNote(ctx, res.Transcript, source+":transcript") + if err != nil { + return 0, fmt.Errorf("transcript note: %w", err) + } + return id, nil } if c.cfg.SaveTranscript && res.Transcript != "" { if _, err := c.writeNote(ctx, res.Transcript, source+":transcript"); err != nil { @@ -251,8 +301,8 @@ func (c *captureWiring) status(_ context.Context) (ipc.CaptureStatusResp, error) // wireCapture installs the four IPC hooks, or leaves them nil so every capture // method reports ErrUnknownMethod. Takes the media keeper wireVision already // opened: one blob store, one retention loop, images and audio side by side. -func wireCapture(srv *ipc.Server, keeper *mediaKeeper, st *store.Store, voiceW *voiceWiring, phr phraser.Phraser, cfg *config.Config) { - cw := newCaptureWiring(keeper, st, voiceW, phr, embedderOf(voiceW), cfg) +func wireCapture(ctx context.Context, wg *sync.WaitGroup, srv *ipc.Server, keeper *mediaKeeper, st *store.Store, voiceW *voiceWiring, phr phraser.Phraser, cfg *config.Config) { + cw := newCaptureWiring(ctx, wg, keeper, st, voiceW, phr, embedderOf(voiceW), cfg) if cw == nil { return } diff --git a/cmd/mavend/capture_test.go b/cmd/mavend/capture_test.go new file mode 100644 index 0000000..e09bdad --- /dev/null +++ b/cmd/mavend/capture_test.go @@ -0,0 +1,118 @@ +package main + +import ( + "context" + "strings" + "sync" + "testing" + "time" + + "github.com/kami/maven/internal/audio" + "github.com/kami/maven/internal/capture" + "github.com/kami/maven/internal/config" + "github.com/kami/maven/internal/ipc" + "github.com/kami/maven/internal/media" +) + +// silentTranscriber stands in for mavsttd: one fixed phrase per window, so the +// wiring can be tested without whisper. +type silentTranscriber struct{} + +func (silentTranscriber) Transcribe(_ context.Context, _ audio.Audio) (string, float64, error) { + return "решили купить насос", 1.0, nil +} + +func testCaptureWiring(t *testing.T) (*captureWiring, *sync.WaitGroup) { + t.Helper() + blobs, err := media.Open(t.TempDir(), 0, 0) + if err != nil { + t.Fatal(err) + } + rec, err := capture.New(blobs, silentTranscriber{}, nil, capture.Config{}) + if err != nil { + t.Fatal(err) + } + var wg sync.WaitGroup + return &captureWiring{ + rec: rec, + st: newTestStore(t), + cfg: &config.CaptureConfig{}, + now: time.Now, + ctx: context.Background(), + wg: &wg, + }, &wg +} + +// A frame carrying the wrong token must not land in the running session. Append +// and stop used to address "whatever is running now", so a client whose session +// had already ended went on recording into somebody else's meeting, and any +// client could end a recording it never started. +func TestCaptureRefusesAnotherClientsToken(t *testing.T) { + c, _ := testCaptureWiring(t) + start, err := c.start(context.Background(), ipc.CaptureStartReq{Label: "встреча"}) + if err != nil { + t.Fatal(err) + } + if start.Token == "" { + t.Fatal("start handed back no session token") + } + if _, err := c.append(context.Background(), ipc.CaptureAppendReq{ + Token: "not-mine", + Audio: audio.Audio{Format: audio.PCM16kMono, Bytes: make([]byte, 3200)}, + }); err == nil { + t.Error("a frame with the wrong token was accepted") + } + if _, err := c.stop(context.Background(), ipc.CaptureStopReq{Token: "not-mine"}); err == nil { + t.Error("a stop with the wrong token ended the session") + } + if st, _ := c.status(context.Background()); !st.Running { + t.Error("the session was ended by a client that does not own it") + } +} + +// Stop answers with the transcript and does not wait for the summary. The +// summary is up to forty model calls, and holding the IPC request for them meant +// the client that said "стоп" sat with no answer for minutes. +// +// With no summariser wired the note still has to be written, from the transcript. +// save_transcript is about keeping the verbatim record IN ADDITION to a summary, +// not about whether the meeting is remembered at all — without this fallback a +// dead llama-server meant an hour of meeting left no note behind. +func TestStopReturnsTranscriptAndNotesItWithoutASummary(t *testing.T) { + c, wg := testCaptureWiring(t) + start, err := c.start(context.Background(), ipc.CaptureStartReq{Label: "планёрка"}) + if err != nil { + t.Fatal(err) + } + if _, err := c.append(context.Background(), ipc.CaptureAppendReq{ + Token: start.Token, + Audio: audio.Audio{Format: audio.PCM16kMono, Bytes: make([]byte, 32000)}, + }); err != nil { + t.Fatal(err) + } + resp, err := c.stop(context.Background(), ipc.CaptureStopReq{Token: start.Token}) + if err != nil { + t.Fatalf("stop: %v", err) + } + if resp.Transcript == "" { + t.Fatal("stop returned no transcript") + } + if resp.Summary != "" { + t.Errorf("summary = %q, want none inside the request", resp.Summary) + } + wg.Wait() + + notes, err := c.st.RecentNotes(context.Background(), 10) + if err != nil { + t.Fatal(err) + } + var found bool + for _, n := range notes { + if strings.Contains(n.Text, "насос") { + found = true + } + } + if !found { + t.Fatalf("the meeting left no note behind: %+v", notes) + } +} diff --git a/cmd/mavend/main.go b/cmd/mavend/main.go index 8211d0e..9e5ac68 100644 --- a/cmd/mavend/main.go +++ b/cmd/mavend/main.go @@ -385,7 +385,7 @@ func run(args []string) error { // The meeting recorder (Vikunja #253) shares that blob store and its // retention loop. Off unless a capture block enables it, in which case // all four capture methods answer ErrUnknownMethod. - wireCapture(srv, keeper, st, voiceW, phr, cfg) + wireCapture(ctx, &wg, srv, keeper, st, voiceW, phr, cfg) // Voice identification (Vikunja #255). Enrolment plumbing only until a // speaker-embedding model exists on disk; off entirely without a speaker // block, so no wire path takes a voiceprint on a default box. @@ -561,7 +561,7 @@ func run(args []string) error { wireMailIntake(srv, st, phr, cfg, evBus) wireModelSwap(srv, phr, cfg) keeper := wireVision(ctx, &wg, srv, st, embedderOf(voiceW), cfg) - wireCapture(srv, keeper, st, voiceW, phr, cfg) + wireCapture(ctx, &wg, srv, keeper, st, voiceW, phr, cfg) // Voice identification (Vikunja #255). Enrolment plumbing only until a // speaker-embedding model exists on disk; off entirely without a speaker // block, so no wire path takes a voiceprint on a default box. diff --git a/internal/config/config.go b/internal/config/config.go index 4fdae93..b9a4c26 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -27,8 +27,8 @@ import ( "github.com/kami/maven/internal/morning" "github.com/kami/maven/internal/netscan" "github.com/kami/maven/internal/smarthome" - "github.com/kami/maven/internal/vision" "github.com/kami/maven/internal/update" + "github.com/kami/maven/internal/vision" "github.com/robfig/cron/v3" ) @@ -787,9 +787,14 @@ type CaptureConfig struct { MaxChunks int `json:"max_chunks,omitempty"` // SaveTranscript — write the full transcript as a note alongside the - // summary. Default false: a verbatim record of what other people said in a - // room is a heavier thing to keep than a four-line summary, so it takes a - // deliberate yes. The audio blob is pruned by media.retention either way. + // summary. Default false, and the cost is not disk: a note is embedded and + // becomes recall corpus, so every later question can surface verbatim words + // other people said in a room. That is the reason it takes a deliberate yes. + // The audio blob is pruned by media.retention either way; the notes are not. + // + // A meeting with no summary writes its transcript regardless. The choice + // here is transcript IN ADDITION to a summary, not whether the meeting is + // remembered at all. SaveTranscript bool `json:"save_transcript,omitempty"` } diff --git a/internal/config/senses_test.go b/internal/config/senses_test.go index 8f50a17..44ec12c 100644 --- a/internal/config/senses_test.go +++ b/internal/config/senses_test.go @@ -228,14 +228,14 @@ func TestSpeakerBlockParsesFromJSON(t *testing.T) { // both fail at startup now. func TestSensesBlocksAreValidatedAtStartup(t *testing.T) { bad := map[string]string{ - "media with no dir": `{"media":{"retention":"48h"}}`, - "negative budget": `{"media":{"dir":"/srv/media","max_total_bytes":-1}}`, - "blob over the budget": `{"media":{"dir":"/srv/media","max_bytes":100,"max_total_bytes":10}}`, - "vision with no media dir": `{"vision":{"enabled":true,"endpoint":"http://127.0.0.1:8081"}}`, - "vision endpoint typo": `{"media":{"dir":"/srv/media"},"vision":{"enabled":true,"endpoint":"127.0.0.1:8081"}}`, - "vision on the wan": `{"media":{"dir":"/srv/media"},"vision":{"enabled":true,"endpoint":"http://8.8.8.8:8081"}}`, - "vision, empty endpoint": `{"media":{"dir":"/srv/media"},"vision":{"enabled":true}}`, - "capture with no store": `{"capture":{"enabled":true}}`, + "media with no dir": `{"media":{"retention":"48h"}}`, + "negative budget": `{"media":{"dir":"/srv/media","max_total_bytes":-1}}`, + "blob over the budget": `{"media":{"dir":"/srv/media","max_bytes":100,"max_total_bytes":10}}`, + "vision with no media dir": `{"vision":{"enabled":true,"endpoint":"http://127.0.0.1:8081"}}`, + "vision endpoint typo": `{"media":{"dir":"/srv/media"},"vision":{"enabled":true,"endpoint":"127.0.0.1:8081"}}`, + "vision on the wan": `{"media":{"dir":"/srv/media"},"vision":{"enabled":true,"endpoint":"http://8.8.8.8:8081"}}`, + "vision, empty endpoint": `{"media":{"dir":"/srv/media"},"vision":{"enabled":true}}`, + "capture with no store": `{"capture":{"enabled":true}}`, } for name, body := range bad { t.Run(name, func(t *testing.T) { diff --git a/internal/ipc/api.go b/internal/ipc/api.go index c2ff155..0083ae6 100644 --- a/internal/ipc/api.go +++ b/internal/ipc/api.go @@ -247,9 +247,14 @@ type CaptureStartReq struct { // which it stops itself; the caller tells him, so a forgotten recording is his // own informed choice rather than a surprise. type CaptureStartResp struct { - Label string `json:"label,omitempty"` - Started time.Time `json:"started"` - MaxSeconds int `json:"max_seconds"` + Label string `json:"label,omitempty"` + Started time.Time `json:"started"` + // Token names THIS session. Every later append, stop and discard has to + // carry it. Without it the recorder is addressed by "whatever is running + // now", and a client whose session already ended on the duration cap goes on + // appending its microphone into the next session someone else started. + Token string `json:"token"` + MaxSeconds int `json:"max_seconds"` } // CaptureAppendReq — one chunk of audio for the running session. Refused with @@ -257,6 +262,9 @@ type CaptureStartResp struct { // makes an ambient path impossible: audio arriving at an idle core is dropped on // the floor, not buffered "just in case". type CaptureAppendReq struct { + // Token from CaptureStartResp. A frame for a session that already ended is + // refused rather than folded into whatever is running now. + Token string `json:"token"` Audio audio.Audio `json:"audio"` } @@ -275,15 +283,22 @@ type CaptureAppendResp struct { // flag rather than a separate method so the client that says "stop" and the // client that says "stop and forget" take the same path to the same session. type CaptureStopReq struct { - Discard bool `json:"discard,omitempty"` + // Token from CaptureStartResp. Stopping by "whatever is running" lets a + // late client end a recording it never started. + Token string `json:"token"` + Discard bool `json:"discard,omitempty"` } // CaptureStopResp — the finished capture. BlobID is the stored WAV, kept under // media.retention like any other blob and pruned with it. // -// A response with a Transcript and an empty Summary is a degraded success: the -// words exist, only the model failed. A response with a BlobID and neither is -// the audio surviving a transcription failure — the same id can be run again. +// A response with a Transcript and an empty Summary is the normal shape, not a +// failure: summarising a long meeting is a map-reduce of minutes, so stop +// answers with the words and the summary note is written afterwards. Summary is +// filled in only when it happened to be ready. A response with a BlobID and no +// transcript is the audio surviving a transcription failure — the same id can be +// run again by hand off the blob before media.retention prunes it — there is no +// capture method that takes a blob id, so this is not a re-run the wire offers. // Discarded is true when nothing was kept. type CaptureStopResp struct { BlobID string `json:"blob_id,omitempty"` diff --git a/internal/ipc/client.go b/internal/ipc/client.go index fae2f4d..fa9852e 100644 --- a/internal/ipc/client.go +++ b/internal/ipc/client.go @@ -500,9 +500,10 @@ func (c *Client) CaptureAppend(ctx context.Context, req CaptureAppendReq) (Captu return r, nil } -// CaptureStop ends the session. Slow — it transcribes and summarises the whole -// recording — so pass a context with room. Set Discard to throw the recording -// away instead. +// CaptureStop ends the session. It transcribes the whole recording before +// answering, so pass a context with room; the summary is written afterwards by +// the daemon and is usually absent from the response. Set Discard to throw the +// recording away instead. Token comes from CaptureStart. func (c *Client) CaptureStop(ctx context.Context, req CaptureStopReq) (CaptureStopResp, error) { var r CaptureStopResp if err := c.call(ctx, MethodCaptureStop, req, &r); err != nil {