fix: derived-state determinism, error visibility, metrics naming, HTTP hardening

- rebuild: fold observations per fingerprint in a total order (hcl DESC, node_id DESC, id DESC) so two nodes with identical logs rebuild identical entries (was: arbitrary bare-column row, merge-order dependent) - threads: CreateThreadCluster uses INSERT OR IGNORE + existing-id fallback — concurrent auto-threaders converge instead of hitting UNIQUE - errors surfaced instead of swallowed: scanEntries returns rows.Err(), Stats() fails fast on query errors, AutoLinkThreadObservations / linkTemporalNeighbors / golden-thread linking propagate failures, AddThreadNote + LinkObservationToThread write under one tx, watch records session upsert failures - gossip client: push checks HTTP status and reports errors (a broken push direction no longer looks like a silent success); gossip diff gets a 10s timeout so a dead peer cannot hang the CLI - ingest: failed source sweeps (obsidian/browser/gitea) are logged, and -d's help text now states its file-only scope - watch --quiet: fatal errors go to stderr instead of io.Discard - main: cobra SilenceErrors/SilenceUsage (errors print once, usage is not dumped on runtime failures); knox mcp exits 0 on SIGINT/SIGTERM - metrics: drop _total suffix from gauges (knox_observations, knox_entries, knox_projects, knox_sessions, knox_peers, knox_threads); _total stays on counters per Prometheus convention - http: ReadHeaderTimeout + IdleTimeout on gossip, metrics, and web servers tests: concurrent cluster-create idempotency, HCL-order rebuild fold (both merge orders), push HTTP-error surfacing; full suite + -race pass, gofmt clean
2026-09-17 02:28:08 -07:00
parent bb852faa27
commit 52c4d1f1e6
12 changed files with 294 additions and 88 deletions
@@ -308,18 +308,21 @@ func (k *KnoxDB) RecordObservation(o ObservationRecord) (obsID int64, isNew bool
 				`INSERT OR IGNORE INTO thread_observations (thread_id, observation_id, relevance) VALUES (?, ?, 'auto')`,
 				goldenID, obsID,
 			)
-			linked := err == nil && res != nil
-			if linked {
-				if n, _ := res.RowsAffected(); n == 0 {
-					linked = false
-				}
+			if err != nil {
+				return 0, false, fmt.Errorf("auto-link golden thread: %w", err)
+			}
+			affected, _ := res.RowsAffected()
+			linked := affected > 0
+			if _, err := tx.Exec(`UPDATE threads SET updated_at=datetime('now') WHERE id=?`, goldenID); err != nil {
+				return 0, false, fmt.Errorf("bump golden thread updated_at: %w", err)
 			}
-			tx.Exec(`UPDATE threads SET updated_at=datetime('now') WHERE id=?`, goldenID)

 			// Temporal cross-reference: when a high-signal non-browser event links,
 			// nearby browser history (±1h) becomes thread context too.
 			if linked && o.SourceID != "browser-history" && o.CreatedAt != "" {
-				k.linkTemporalNeighbors(tx, goldenID, o.CreatedAt, keywords, bigrams)
+				if err := k.linkTemporalNeighbors(tx, goldenID, o.CreatedAt, keywords, bigrams); err != nil {
+					return 0, false, fmt.Errorf("link temporal neighbors: %w", err)
+				}
 			}
 		}
 	}
@@ -330,7 +333,7 @@ func (k *KnoxDB) RecordObservation(o ObservationRecord) (obsID int64, isNew bool
 // linkTemporalNeighbors links browser-history observations within ±1h of a
 // signal timestamp to the thread. A weak keyword check still applies so
 // unrelated browsing (lunch news) doesn't pollute the thread.
-func (k *KnoxDB) linkTemporalNeighbors(tx *sql.Tx, threadID int64, createdAt string, keywords []string, bigrams [][2]string) {
+func (k *KnoxDB) linkTemporalNeighbors(tx *sql.Tx, threadID int64, createdAt string, keywords []string, bigrams [][2]string) error {
 	rows, err := tx.Query(
 		`SELECT o.id, COALESCE(o.title,''), COALESCE(o.summary,'')
 		 FROM observations o
@@ -341,7 +344,7 @@ func (k *KnoxDB) linkTemporalNeighbors(tx *sql.Tx, threadID int64, createdAt str
 		createdAt,
 	)
 	if err != nil {
-		return
+		return err
 	}
 	defer rows.Close()

@@ -349,15 +352,18 @@ func (k *KnoxDB) linkTemporalNeighbors(tx *sql.Tx, threadID int64, createdAt str
 		var obsID int64
 		var title, summary string
 		if err := rows.Scan(&obsID, &title, &summary); err != nil {
-			continue
+			return err
 		}
 		if isRelevant(title, summary, "", keywords, bigrams) || weakRelevant(title, summary, keywords) {
-			tx.Exec(
+			if _, err := tx.Exec(
 				`INSERT OR IGNORE INTO thread_observations (thread_id, observation_id, relevance) VALUES (?, ?, 'temporal')`,
 				threadID, obsID,
-			)
+			); err != nil {
+				return err
+			}
 		}
 	}
+	return rows.Err()
 }

 // weakRelevant is a lower bar for temporal neighbors: any single keyword match.
@@ -447,23 +453,39 @@ func (k *KnoxDB) RebuildEntriesFromObservations() (before, after int, err error)
 	if _, err := tx.Exec("DELETE FROM entries"); err != nil {
 		return 0, 0, err
 	}
+	// Content fields come from the latest observation per fingerprint, folded in
+	// a total order (hcl DESC, node_id DESC, id DESC) — deterministic for any two
+	// nodes holding identical logs, per the gossip spec (bare GROUP BY columns
+	// would take an arbitrary row, merge-order dependent).
 	_, err = tx.Exec(
 		`INSERT INTO entries (fingerprint, source_id, source_path, project, content_type, title, summary, first_seen, last_seen, created_at, ref_count, last_confidence)
 		 SELECT
-		   o.fingerprint,
-		   o.source_id,
-		   o.source_path,
-		   o.project,
-		   o.content_type,
-		   o.title,
-		   o.summary,
-		   MIN(o.collected_at),
-		   MAX(o.collected_at),
-		   COALESCE(NULLIF(MAX(o.created_at),''), MIN(o.collected_at)),
-		   COUNT(*),
-		   MAX(o.confidence)
-		 FROM observations o
-		 GROUP BY o.fingerprint`,
+		   latest.fingerprint,
+		   latest.source_id,
+		   latest.source_path,
+		   latest.project,
+		   latest.content_type,
+		   latest.title,
+		   latest.summary,
+		   agg.first_seen,
+		   agg.last_seen,
+		   agg.created_at,
+		   agg.ref_count,
+		   agg.max_confidence
+		 FROM (
+		   SELECT *, ROW_NUMBER() OVER (PARTITION BY fingerprint ORDER BY hcl DESC, node_id DESC, id DESC) AS rn
+		   FROM observations
+		 ) latest
+		 JOIN (
+		   SELECT fingerprint,
+		          MIN(collected_at) AS first_seen,
+		          MAX(collected_at) AS last_seen,
+		          COALESCE(NULLIF(MAX(created_at),''), MIN(collected_at)) AS created_at,
+		          COUNT(*) AS ref_count,
+		          MAX(confidence) AS max_confidence
+		   FROM observations GROUP BY fingerprint
+		 ) agg ON agg.fingerprint = latest.fingerprint
+		 WHERE latest.rn = 1`,
 	)
 	if err != nil {
 		return 0, 0, fmt.Errorf("rebuild entries: %w", err)
@@ -604,6 +626,9 @@ func scanEntries(rows *sql.Rows) ([]Entry, error) {
 		}
 		entries = append(entries, e)
 	}
+	if err := rows.Err(); err != nil {
+		return nil, err
+	}
 	return entries, nil
 }

@@ -655,38 +680,50 @@ func (k *KnoxDB) MarkSessionIndexed(sessionID string) error {
 func (k *KnoxDB) Stats() (map[string]any, error) {
 	stats := make(map[string]any)

-	var v int
-	var s string
+	// A broken DB reports an error, not zeros: every aggregate is checked.
+	countInt := func(query string) (int, error) {
+		var v int
+		if err := k.db.QueryRow(query).Scan(&v); err != nil {
+			return 0, err
+		}
+		return v, nil
+	}
+	ints := []struct {
+		key   string
+		query string
+	}{
+		{"total_entries", "SELECT COUNT(*) FROM entries"},
+		{"entries_last_24h", "SELECT COUNT(*) FROM entries WHERE last_seen > datetime('now', '-1 day')"},
+		{"total_projects", "SELECT COUNT(DISTINCT project) FROM entries"},
+		{"total_observations", "SELECT COUNT(*) FROM observations"},
+		{"observations_last_24h", "SELECT COUNT(*) FROM observations WHERE collected_at > datetime('now', '-1 day')"},
+		{"total_sessions", "SELECT COUNT(*) FROM sessions"},
+		{"pending_reflections", "SELECT COUNT(*) FROM sessions WHERE indexed=0"},
+	}
+	for _, it := range ints {
+		v, err := countInt(it.query)
+		if err != nil {
+			return nil, fmt.Errorf("stats %s: %w", it.key, err)
+		}
+		stats[it.key] = v
+	}

-	k.db.QueryRow("SELECT COUNT(*) FROM entries").Scan(&v)
-	stats["total_entries"] = v
-	v = 0
-	k.db.QueryRow("SELECT COUNT(*) FROM entries WHERE last_seen > datetime('now', '-1 day')").Scan(&v)
-	stats["entries_last_24h"] = v
-	v = 0
-	k.db.QueryRow("SELECT COUNT(DISTINCT project) FROM entries").Scan(&v)
-	stats["total_projects"] = v
-	v = 0
-	k.db.QueryRow("SELECT COUNT(*) FROM observations").Scan(&v)
-	stats["total_observations"] = v
-	v = 0
-	k.db.QueryRow("SELECT COUNT(*) FROM observations WHERE collected_at > datetime('now', '-1 day')").Scan(&v)
-	stats["observations_last_24h"] = v
-	v = 0
-	k.db.QueryRow("SELECT COALESCE(MIN(collected_at),'') FROM observations").Scan(&s)
-	stats["earliest_observation"] = s
-	s = ""
-	k.db.QueryRow("SELECT COUNT(*) FROM sessions").Scan(&v)
-	stats["total_sessions"] = v
-	v = 0
-	k.db.QueryRow("SELECT COUNT(*) FROM sessions WHERE indexed=0").Scan(&v)
-	stats["pending_reflections"] = v
-	v = 0
+	var earliest string
+	if err := k.db.QueryRow("SELECT COALESCE(MIN(collected_at),'') FROM observations").Scan(&earliest); err != nil {
+		return nil, fmt.Errorf("stats earliest_observation: %w", err)
+	}
+	stats["earliest_observation"] = earliest

-	goldenID, _ := k.GoldenThreadID()
+	goldenID, err := k.GoldenThreadID()
+	if err != nil {
+		return nil, fmt.Errorf("stats golden_thread: %w", err)
+	}
 	if goldenID > 0 {
 		stats["golden_thread_id"] = goldenID
-		t, _ := k.GetThread(goldenID)
+		t, err := k.GetThread(goldenID)
+		if err != nil {
+			return nil, fmt.Errorf("stats golden_thread: %w", err)
+		}
 		if t != nil {
 			stats["golden_thread"] = t.Title
 		}
@@ -989,20 +1026,26 @@ func (k *KnoxDB) CreateThread(title, motivation, priority, tags, provenance stri
 }

 func (k *KnoxDB) CreateThreadCluster(title, motivation, priority, tags, provenance, clusterKey string) (int64, bool, error) {
-	if clusterKey != "" {
-		if existing := k.ThreadByClusterKey(clusterKey); existing != 0 {
-			return existing, false, nil
-		}
-	}
+	// INSERT OR IGNORE makes creation atomic under the partial unique index:
+	// concurrent creators (thread ticker + post-pull reconcile, or two gossip
+	// nodes converging on the same cluster) lose the race into a no-op instead
+	// of a UNIQUE error, then pick up the winner's id.
 	res, err := k.db.Exec(
-		`INSERT INTO threads (title, motivation, priority, tags, provenance, cluster_key) VALUES (?, ?, ?, ?, ?, ?)`,
+		`INSERT OR IGNORE INTO threads (title, motivation, priority, tags, provenance, cluster_key) VALUES (?, ?, ?, ?, ?, ?)`,
 		title, motivation, priority, tags, provenance, clusterKey,
 	)
 	if err != nil {
 		return 0, false, err
 	}
-	id, _ := res.LastInsertId()
-	return id, true, nil
+	if n, _ := res.RowsAffected(); n > 0 {
+		id, _ := res.LastInsertId()
+		return id, true, nil
+	}
+	// Ignored insert: a thread with this cluster key already exists.
+	if existing := k.ThreadByClusterKey(clusterKey); existing != 0 {
+		return existing, false, nil
+	}
+	return 0, false, fmt.Errorf("insert ignored but no thread with cluster key %q", clusterKey)
 }

 // ThreadByClusterKey returns the id of the thread auto-created for a given
@@ -1107,15 +1150,21 @@ func (k *KnoxDB) UpdateThread(id int64, title, motivation, priority, tags string
 }

 func (k *KnoxDB) LinkObservationToThread(threadID, observationID int64, relevance string) error {
-	_, err := k.db.Exec(
-		`INSERT OR IGNORE INTO thread_observations (thread_id, observation_id, relevance) VALUES (?, ?, ?)`,
-		threadID, observationID, relevance,
-	)
+	tx, err := k.db.Begin()
 	if err != nil {
 		return err
 	}
-	_, err = k.db.Exec(`UPDATE threads SET updated_at=datetime('now') WHERE id=?`, threadID)
-	return err
+	defer tx.Rollback()
+	if _, err := tx.Exec(
+		`INSERT OR IGNORE INTO thread_observations (thread_id, observation_id, relevance) VALUES (?, ?, ?)`,
+		threadID, observationID, relevance,
+	); err != nil {
+		return err
+	}
+	if _, err := tx.Exec(`UPDATE threads SET updated_at=datetime('now') WHERE id=?`, threadID); err != nil {
+		return err
+	}
+	return tx.Commit()
 }

 // AutoLinkThreadObservations resolves each entry fingerprint to its most recent
@@ -1130,6 +1179,7 @@ func (k *KnoxDB) AutoLinkThreadObservations(threadID int64, fingerprints []strin
 	defer tx.Rollback()

 	linked := 0
+	var firstErr error
 	for _, fp := range fingerprints {
 		if fp == "" {
 			continue
@@ -1138,6 +1188,11 @@ func (k *KnoxDB) AutoLinkThreadObservations(threadID int64, fingerprints []strin
 		if err := tx.QueryRow(
 			`SELECT id FROM observations WHERE fingerprint=? ORDER BY hcl DESC, id DESC LIMIT 1`, fp,
 		).Scan(&obsID); err != nil {
+			// A fingerprint without observations is expected (provenance links);
+			// a real query failure is not — remember the first one.
+			if err != sql.ErrNoRows && firstErr == nil {
+				firstErr = err
+			}
 			continue
 		}
 		res, err := tx.Exec(
@@ -1145,6 +1200,9 @@ func (k *KnoxDB) AutoLinkThreadObservations(threadID int64, fingerprints []strin
 			threadID, obsID,
 		)
 		if err != nil {
+			if firstErr == nil {
+				firstErr = err
+			}
 			continue
 		}
 		if n, _ := res.RowsAffected(); n > 0 {
@@ -1157,7 +1215,10 @@ func (k *KnoxDB) AutoLinkThreadObservations(threadID int64, fingerprints []strin
 			return linked, err
 		}
 	}
-	return linked, tx.Commit()
+	if err := tx.Commit(); err != nil {
+		return linked, err
+	}
+	return linked, firstErr
 }

 // ActiveThreadByKeyword returns the most recently updated active thread whose
@@ -1226,15 +1287,26 @@ func (k *KnoxDB) ThreadObservations(threadID int64) ([]Observation, error) {
 }

 func (k *KnoxDB) AddThreadNote(threadID int64, note string) (int64, error) {
-	res, err := k.db.Exec(
+	tx, err := k.db.Begin()
+	if err != nil {
+		return 0, err
+	}
+	defer tx.Rollback()
+	res, err := tx.Exec(
 		`INSERT INTO thread_notes (thread_id, note) VALUES (?, ?)`,
 		threadID, note,
 	)
 	if err != nil {
 		return 0, err
 	}
-	_, err = k.db.Exec(`UPDATE threads SET updated_at=datetime('now') WHERE id=?`, threadID)
-	return res.LastInsertId()
+	if _, err := tx.Exec(`UPDATE threads SET updated_at=datetime('now') WHERE id=?`, threadID); err != nil {
+		return 0, err
+	}
+	id, err := res.LastInsertId()
+	if err != nil {
+		return 0, err
+	}
+	return id, tx.Commit()
 }

 func (k *KnoxDB) ThreadNotes(threadID int64) ([]struct {
@@ -154,3 +154,89 @@ func TestOpenReopenHCLMonotonicAcrossRestart(t *testing.T) {
 		t.Errorf("unexpected row above max: %s", after[0].Fingerprint)
 	}
 }
+
+// TestCreateThreadClusterConcurrentIdempotent: two concurrent creators of the
+// same cluster key must converge on one thread (one created=true, one false)
+// with no UNIQUE constraint error — the INSERT OR IGNORE path.
+func TestCreateThreadClusterConcurrentIdempotent(t *testing.T) {
+	k := tmpDB(t)
+	const key = "cluster:race"
+	var wg sync.WaitGroup
+	ids := make([]int64, 2)
+	created := make([]bool, 2)
+	errs := make([]error, 2)
+	for i := 0; i < 2; i++ {
+		wg.Add(1)
+		go func(i int) {
+			defer wg.Done()
+			ids[i], created[i], errs[i] = k.CreateThreadCluster("Race thread", "m", "medium", "[]", `{}`, key)
+		}(i)
+	}
+	wg.Wait()
+	for i := range errs {
+		if errs[i] != nil {
+			t.Fatalf("creator %d: %v", i, errs[i])
+		}
+	}
+	if ids[0] != ids[1] {
+		t.Errorf("concurrent creators got different ids: %d vs %d", ids[0], ids[1])
+	}
+	if created[0] == created[1] {
+		t.Errorf("exactly one creator should report created=true, got %v %v", created[0], created[1])
+	}
+	threads, err := k.ListThreads("")
+	if err != nil {
+		t.Fatal(err)
+	}
+	n := 0
+	for _, th := range threads {
+		if th.Title == "Race thread" {
+			n++
+		}
+	}
+	if n != 1 {
+		t.Errorf("want exactly 1 race thread, got %d", n)
+	}
+}
+
+// TestRebuildEntriesDeterministicFold: two nodes holding the same observations
+// in different merge orders must rebuild identical derived state — content
+// fields come from the highest-HCL observation per fingerprint (spec 6.2),
+// never from an arbitrary (merge-order-dependent) row.
+func TestRebuildEntriesDeterministicFold(t *testing.T) {
+	const foreign = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
+	rows := []GossipObservation{
+		{NodeID: foreign, HCL: 10, Fingerprint: "fp:fold", SourceID: "test", Title: "old title", Summary: "old", CollectedAt: "2026-08-29T00:00:00Z", Confidence: 0.5},
+		{NodeID: foreign, HCL: 20, Fingerprint: "fp:fold", SourceID: "test", Title: "new title", Summary: "new", CollectedAt: "2026-08-29T01:00:00Z", Confidence: 0.9},
+	}
+	results := make([]Entry, 2)
+	for i, order := range [][]GossipObservation{{rows[0], rows[1]}, {rows[1], rows[0]}} {
+		k := tmpDB(t)
+		if _, err := k.PushObservations(order); err != nil {
+			t.Fatalf("push: %v", err)
+		}
+		if _, _, err := k.RebuildEntriesFromObservations(); err != nil {
+			t.Fatalf("rebuild: %v", err)
+		}
+		e, err := k.FindEntry("fp:fold")
+		if err != nil || e == nil {
+			t.Fatalf("find entry: %v", err)
+		}
+		results[i] = *e
+	}
+	if results[0].Title != "new title" {
+		t.Errorf("order [old,new]: title = %q, want %q", results[0].Title, "new title")
+	}
+	if results[1].Title != "new title" {
+		t.Errorf("order [new,old]: title = %q, want %q", results[1].Title, "new title")
+	}
+	if results[0] != results[1] {
+		t.Errorf("derived state diverged between merge orders:\n%+v\n%+v", results[0], results[1])
+	}
+	if results[0].RefCount != 2 {
+		t.Errorf("ref_count = %d, want 2", results[0].RefCount)
+	}
+	if results[0].FirstSeen != "2026-08-29T00:00:00Z" || results[0].LastSeen != "2026-08-29T01:00:00Z" {
+		t.Errorf("first/last seen = %q/%q", results[0].FirstSeen, results[0].LastSeen)
+	}
+}