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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0117tgnmbgZpHVV3XSNw8Qua
This commit is contained in:
2026-08-06 19:20:24 +04:00
parent 7852aad60f
commit e1f84a3474
6 changed files with 39 additions and 11 deletions
+23
View File
@@ -16,6 +16,7 @@ import (
"encoding/json" "encoding/json"
"log" "log"
"strings" "strings"
"time"
"github.com/kami/maven/internal/decision" "github.com/kami/maven/internal/decision"
"github.com/kami/maven/internal/store" "github.com/kami/maven/internal/store"
@@ -37,6 +38,21 @@ func traceSink(s *store.Store) traceWriter {
return s 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 // 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. // 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) == "" { if h.traces == nil || rec == nil || strings.TrimSpace(rec.Utterance) == "" {
return 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) claims, err := json.Marshal(rec.Claims)
if err != nil { if err != nil {
log.Printf("routing trace: marshal claims: %v", err) log.Printf("routing trace: marshal claims: %v", err)
+3
View File
@@ -149,6 +149,9 @@ func wireVoice(cfg *config.Config, coreAPI ipc.CoreAPI, phr phraser.Phraser, mem
w.embedder = emb w.embedder = emb
repairFactVectors(dataStore, emb) repairFactVectors(dataStore, emb)
checkStoredEmbedder(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) ----- // ----- tool executor (the enabled act allowlist, store-backed) -----
// Config tools are the declarative bootstrap: seed them into the store as // Config tools are the declarative bootstrap: seed them into the store as
@@ -2,9 +2,9 @@
**Owner's call, 06-08-2026. Vikunja #629, umbrella #628.** **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 **Verdict: the per-turn decision record now persists.** That reverses a written decision,
written decision, which is the point of this file. It is not an incidental telemetry which is the point of this file. It is not an incidental telemetry
feature and must not be read as one. feature. Do not read it as one.
Last verified: 06-08-2026 @ 799cf55 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 ## 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 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 count. The useful question is what she did this week, and a busy Tuesday must not push last
Friday out. 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 - **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. 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 - **Retention is enforced on write and again at start.** `WriteRoutingTrace` prunes every
hours at human rate. 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 - **Deletion already exists.** `Store.Wipe` drops every table the database reports, so
`mavend -wipe -confirm-wipe` covers this one with no list to edit. `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 - **The ring did not move.** It is still what `/trace` reads and still what a test with no
+1 -1
View File
@@ -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 // 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 // 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 // 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 // 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 // corrected pair is promoted out of here into a seed-shaped row and kept, so
+3 -3
View File
@@ -7,7 +7,7 @@ import (
"time" "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, // 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 // 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", // 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 // 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 // indefinitely, because a label is not a transcript. Keeping the transcript that
// carried it would defeat the point of the bound. // 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 // 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 // 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 // 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. // paying for a delete on every one of them buys nothing. 64 turns is hours.
if id%64 == 0 { 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 return id, err
} }
} }
+1 -1
View File
@@ -62,7 +62,7 @@ func TestPruneRoutingTracesByAge(t *testing.T) {
t.Fatal(err) 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) t.Fatal(err)
} }
got, err := s.RecentRoutingTraces(ctx, 10) got, err := s.RecentRoutingTraces(ctx, 10)