From e1f84a34747e968a204c2781edc56f31b63cc4be Mon Sep 17 00:00:00 2001 From: claude Date: Thu, 6 Aug 2026 19:20:24 +0400 Subject: [PATCH] review: a cancelled turn keeps its trace, and a quiet box still expires (V-629) Two defects found reviewing the PR. The insert ran on the turn's own context, so a caller that hung up or timed out cancelled it. That is exactly the turn worth having. It now runs detached, with a one-second bound of its own, because a write must not hold the reply. Retention was enforced on write alone, so a box that goes quiet for a month kept every row until the next sixty-fourth turn. pruneTracesOnStart closes that, and RoutingTraceRetention is exported so the daemon reads the same number the store enforces. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_0117tgnmbgZpHVV3XSNw8Qua --- cmd/mavend/routingtrace.go | 23 +++++++++++++++++++ cmd/mavend/voicewire.go | 3 +++ docs/plans/21-persisting-the-routing-trace.md | 14 ++++++----- internal/store/migrations.go | 2 +- internal/store/routingtraces.go | 6 ++--- internal/store/routingtraces_test.go | 2 +- 6 files changed, 39 insertions(+), 11 deletions(-) diff --git a/cmd/mavend/routingtrace.go b/cmd/mavend/routingtrace.go index 0bcfcb3..02e531b 100644 --- a/cmd/mavend/routingtrace.go +++ b/cmd/mavend/routingtrace.go @@ -16,6 +16,7 @@ import ( "encoding/json" "log" "strings" + "time" "github.com/kami/maven/internal/decision" "github.com/kami/maven/internal/store" @@ -37,6 +38,21 @@ func traceSink(s *store.Store) traceWriter { return s } +// pruneTracesOnStart enforces retention once at wiring time. Pruning on write +// alone is not enough: a box that goes quiet for a month keeps every row until +// the next sixty-fourth turn, and "kept for fourteen days" would then be true +// only of a box in daily use. Called for its effect and never blocks a start. +func pruneTracesOnStart(s *store.Store, now time.Time) { + if s == nil { + return + } + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + if err := s.PruneRoutingTraces(ctx, now.Add(-store.RoutingTraceRetention)); err != nil { + log.Printf("routing trace: prune on start: %v", err) + } +} + // persistDecision writes one finished record. It takes the same *decision.Record // the ring takes, so the two sinks cannot disagree about what the turn did. // @@ -46,6 +62,13 @@ func (h *reactiveHandler) persistDecision(ctx context.Context, rec *decision.Rec if h.traces == nil || rec == nil || strings.TrimSpace(rec.Utterance) == "" { return } + // Detached from the turn's context, and bounded on its own. Two reasons, and + // the first is the one that matters: the turn is over by the time this runs, + // so a caller that hung up or timed out would cancel the insert, and the turn + // he abandoned halfway is exactly the one worth having. The second is that a + // write must not hold the reply, so it gets a second and no more. + ctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), time.Second) + defer cancel() claims, err := json.Marshal(rec.Claims) if err != nil { log.Printf("routing trace: marshal claims: %v", err) diff --git a/cmd/mavend/voicewire.go b/cmd/mavend/voicewire.go index 9ede6ee..95ad503 100644 --- a/cmd/mavend/voicewire.go +++ b/cmd/mavend/voicewire.go @@ -149,6 +149,9 @@ func wireVoice(cfg *config.Config, coreAPI ipc.CoreAPI, phr phraser.Phraser, mem w.embedder = emb repairFactVectors(dataStore, emb) checkStoredEmbedder(dataStore, emb) + // Retention is enforced on write, which is not enough on its own: a box that + // goes quiet keeps every trace until the next sixty-fourth turn (V-629). + pruneTracesOnStart(dataStore, time.Now()) // ----- tool executor (the enabled act allowlist, store-backed) ----- // Config tools are the declarative bootstrap: seed them into the store as diff --git a/docs/plans/21-persisting-the-routing-trace.md b/docs/plans/21-persisting-the-routing-trace.md index 9af6d03..62a5f0d 100644 --- a/docs/plans/21-persisting-the-routing-trace.md +++ b/docs/plans/21-persisting-the-routing-trace.md @@ -2,9 +2,9 @@ **Owner's call, 06-08-2026. Vikunja #629, umbrella #628.** -**Verdict: the per-turn decision record is now written to the database.** That reverses a -written decision, which is the point of this file. It is not an incidental telemetry -feature and must not be read as one. +**Verdict: the per-turn decision record now persists.** That reverses a written decision, +which is the point of this file. It is not an incidental telemetry +feature. Do not read it as one. Last verified: 06-08-2026 @ 799cf55 @@ -27,7 +27,7 @@ So the choice was between no routing heads and a persisted trace. The owner chos ## Retention, and why it is two answers -**Raw trace: 14 days.** `routingTraceRetention` in `internal/store/routingtraces.go`. That +**Raw trace: 14 days.** `store.RoutingTraceRetention` in `internal/store/routingtraces.go`. That is the life of a diagnosis with room for a weekend. The bound is an age and not a row count. The useful question is what she did this week, and a busy Tuesday must not push last Friday out. @@ -45,8 +45,10 @@ support, and making it would be worse than staying silent. - **Nothing here leaves the box.** The rule that the owner's notes and facts are never search input covers this table too. No query source reads it, and no upstream engine can. -- **Retention is enforced on write.** `WriteRoutingTrace` prunes every 64th row, which is - hours at human rate. +- **Retention is enforced on write and again at start.** `WriteRoutingTrace` prunes every + 64th row, which is hours at human rate. `pruneTracesOnStart` covers the case write alone + cannot. A box that goes quiet keeps every row until the next sixty-fourth turn. Without + the start-time prune, the bound would hold only for a box in daily use. - **Deletion already exists.** `Store.Wipe` drops every table the database reports, so `mavend -wipe -confirm-wipe` covers this one with no list to edit. - **The ring did not move.** It is still what `/trace` reads and still what a test with no diff --git a/internal/store/migrations.go b/internal/store/migrations.go index 5081a46..720b822 100644 --- a/internal/store/migrations.go +++ b/internal/store/migrations.go @@ -311,7 +311,7 @@ ALTER TABLE reminders ADD COLUMN next_fire_ts INTEGER;`, // #2 // sentence is substantially recoverable, so storing vectors instead would be a // privacy claim we cannot support. What makes it safe is the same thing that // makes the fact store safe: it never leaves the box, retention is bounded at - // routingTraceRetention, and Wipe drops it with everything else. + // store.RoutingTraceRetention, and Wipe drops it with everything else. // // correction is empty until the owner corrects a turn on /chat (V-630). A // corrected pair is promoted out of here into a seed-shaped row and kept, so diff --git a/internal/store/routingtraces.go b/internal/store/routingtraces.go index ac7f922..bdf83a9 100644 --- a/internal/store/routingtraces.go +++ b/internal/store/routingtraces.go @@ -7,7 +7,7 @@ import ( "time" ) -// routingTraceRetention is how long a raw trace lives (owner's call, +// RoutingTraceRetention is how long a raw trace lives (owner's call, // 06-08-2026). A trace is read within a day or two of the turn that produced it, // or never, so two weeks is diagnosis with room for a weekend. It is deliberately // an age and not a row count: the useful question is "what did she do this week", @@ -17,7 +17,7 @@ import ( // turn, the pair is promoted out of the trace into a seed-shaped row and kept // indefinitely, because a label is not a transcript. Keeping the transcript that // carried it would defeat the point of the bound. -const routingTraceRetention = 14 * 24 * time.Hour +const RoutingTraceRetention = 14 * 24 * time.Hour // RoutingTrace is one turn's arbitration, persisted. It is internal/decision's // Record plus the four things the ring never had to carry: which reach the @@ -70,7 +70,7 @@ func (s *Store) WriteRoutingTrace(ctx context.Context, tr RoutingTrace) (int64, // Prune rarely. Turns arrive at human rate, so the bound is a ceiling and // paying for a delete on every one of them buys nothing. 64 turns is hours. if id%64 == 0 { - if err := s.PruneRoutingTraces(ctx, tr.Ts.Add(-routingTraceRetention)); err != nil { + if err := s.PruneRoutingTraces(ctx, tr.Ts.Add(-RoutingTraceRetention)); err != nil { return id, err } } diff --git a/internal/store/routingtraces_test.go b/internal/store/routingtraces_test.go index 34a058b..3db2161 100644 --- a/internal/store/routingtraces_test.go +++ b/internal/store/routingtraces_test.go @@ -62,7 +62,7 @@ func TestPruneRoutingTracesByAge(t *testing.T) { t.Fatal(err) } } - if err := s.PruneRoutingTraces(ctx, now.Add(-routingTraceRetention)); err != nil { + if err := s.PruneRoutingTraces(ctx, now.Add(-RoutingTraceRetention)); err != nil { t.Fatal(err) } got, err := s.RecentRoutingTraces(ctx, 10)