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

2026-09-17T09:28:32Z

david commented

2026-09-17 09:28:32 +00:00

Second batch from the codebase review — deterministic derived state, error visibility, metrics convention, and HTTP hardening.

Deterministic rebuild (gossip spec §6.2)

RebuildEntriesFromObservations now folds per fingerprint in a total order (hcl DESC, node_id DESC, id DESC) via a window function: two nodes holding identical observation logs now rebuild identical entries. Previously bare GROUP BY columns picked an arbitrary, merge-order-dependent row.

Concurrency

CreateThreadCluster is an atomic INSERT OR IGNORE + existing-id fallback under the partial unique index — concurrent auto-threaders (thread ticker + post-pull reconcile, or two converged gossip nodes) converge instead of one erroring on UNIQUE.

Error visibility (errors are surfaced, not swallowed)

scanEntries returns rows.Err() (silent truncation of search results no longer possible)
Stats() fails fast on query errors instead of reporting zeros on a broken DB
AutoLinkThreadObservations / linkTemporalNeighbors / golden-thread auto-linking propagate failures
AddThreadNote + LinkObservationToThread write both rows under one transaction
Daemon logs session-upsert failures; ingest logs failed source sweeps (obsidian/browser/gitea); watch --quiet fatal errors go to stderr instead of io.Discard

Gossip client

Push checks the HTTP status and reports errors — a broken push direction no longer looks like a silent success
knox gossip diff gets a 10s timeout; a dead peer can no longer hang the CLI indefinitely

Prometheus convention (breaking for dashboards)

Gauges renamed to drop the _total suffix (reserved for counters): knox_observations, knox_entries, knox_projects, knox_sessions, knox_peers, knox_threads. Counters keep _total. Update any dashboards/alerts accordingly.

HTTP hardening

ReadHeaderTimeout (10s) + IdleTimeout (60s) on the gossip, metrics, and web servers (slowloris posture)

CLI UX

cobra SilenceErrors/SilenceUsage: errors print once, runtime failures no longer dump usage text
knox mcp exits 0 on SIGINT/SIGTERM instead of "Error: context canceled" + exit 1
ingest -d help now states it scopes file-based sources only

Tests — concurrent cluster-create idempotency; HCL-order rebuild fold in both insertion orders (catches the old non-determinism); push HTTP-error surfacing. Full suite + -race pass; gofmt clean.

Second batch from the codebase review — deterministic derived state, error visibility, metrics convention, and HTTP hardening. **Deterministic rebuild (gossip spec §6.2)** - `RebuildEntriesFromObservations` now folds per fingerprint in a total order (`hcl DESC, node_id DESC, id DESC`) via a window function: two nodes holding identical observation logs now rebuild *identical* entries. Previously bare `GROUP BY` columns picked an arbitrary, merge-order-dependent row. **Concurrency** - `CreateThreadCluster` is an atomic `INSERT OR IGNORE` + existing-id fallback under the partial unique index — concurrent auto-threaders (thread ticker + post-pull reconcile, or two converged gossip nodes) converge instead of one erroring on UNIQUE. **Error visibility** (errors are surfaced, not swallowed) - `scanEntries` returns `rows.Err()` (silent truncation of search results no longer possible) - `Stats()` fails fast on query errors instead of reporting zeros on a broken DB - `AutoLinkThreadObservations` / `linkTemporalNeighbors` / golden-thread auto-linking propagate failures - `AddThreadNote` + `LinkObservationToThread` write both rows under one transaction - Daemon logs session-upsert failures; `ingest` logs failed source sweeps (obsidian/browser/gitea); `watch --quiet` fatal errors go to stderr instead of `io.Discard` **Gossip client** - `Push` checks the HTTP status and reports errors — a broken push direction no longer looks like a silent success - `knox gossip diff` gets a 10s timeout; a dead peer can no longer hang the CLI indefinitely **Prometheus convention (breaking for dashboards)** - Gauges renamed to drop the `_total` suffix (reserved for counters): `knox_observations`, `knox_entries`, `knox_projects`, `knox_sessions`, `knox_peers`, `knox_threads`. Counters keep `_total`. Update any dashboards/alerts accordingly. **HTTP hardening** - `ReadHeaderTimeout` (10s) + `IdleTimeout` (60s) on the gossip, metrics, and web servers (slowloris posture) **CLI UX** - cobra `SilenceErrors`/`SilenceUsage`: errors print once, runtime failures no longer dump usage text - `knox mcp` exits 0 on SIGINT/SIGTERM instead of "Error: context canceled" + exit 1 - `ingest -d` help now states it scopes file-based sources only **Tests** — concurrent cluster-create idempotency; HCL-order rebuild fold in *both* insertion orders (catches the old non-determinism); push HTTP-error surfacing. Full suite + `-race` pass; gofmt clean.

david added 1 commit 2026-09-17 09:28:33 +00:00

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

- 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

This pull request can be merged automatically.

You are not authorized to merge this pull request.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin fix/review-hardening-2:fix/review-hardening-2

git checkout fix/review-hardening-2

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: david/knox#5