Add benchmark gates for recurring sync and DB performance regressions

[mariusvniekerk]

Why

agentsview has repeatedly shipped the same performance regression shape: sync work that should scale with new data silently starts scaling with archive size, and nothing in CI notices until a machine spends minutes in a sync pass. Recent history: discovery recomputing root-derived project info per source (#912), the provider migration dropping pre-parse freshness skips, O(session-history) work on every streamed append (#954), bulk-ingest throughput (#411), and per-row json_extract in the usage scan (#309). Each fix landed without a gate, so any refactor could quietly reintroduce its class.

What this adds

Layer 1 — deterministic work-count invariants in the normal test suite (immune to runner noise): TestWarmFullSyncDoesNoBulkWriteWork asserts a second full sync over an unchanged Claude archive skips every session and runs zero bulk-write batches, via the existing SyncStats/PhaseStats counters. It complements the existing Vibe freshness, signal-scheduler debounce, and parser once-per-root seam tests; docs/internal/performance-gates.md maps each historical regression class to the gate that covers it.

Layer 2 — a per-PR benchmark gate (.github/workflows/bench.yml): hot-path benchmarks (warm no-op full sync, incremental append into a 1k-message session, cold-archive ingest through both the default and the resync bulk-write pipelines, streaming chunk-merge replace, batched insert, plus the existing usage and secret-scan benchmarks) run on the PR head and its merge base in the same job on the same runner, each side via make bench-gate — the Makefile is the single source of truth for the gated package list, sample count, and iteration count — and are compared by cmd/benchgate.

benchgate builds on golang.org/x/perf (benchfmt parsing, benchmath — the statistics behind benchstat) and adds only the policy benchstat doesn't provide: thresholds, floors, and a failing exit code. The policy is tuned for shared-runner noise:

• Gating is per benchmark; nothing is averaged across benchmarks.
• allocs/op (1.25x) and B/op (1.35x) are deterministic for fixed code and iteration count, so they gate the candidate's worst -count run against the baseline median — a single outlier run is a real intermittent allocation path and fails. Failure lines include the baseline's worst run so pre-existing instability is visible.
• Time gates medians at a loose 2.0x and requires Mann-Whitney significance, so one slow run can't flake a PR but algorithmic blowups fail. Fewer than 5 candidate samples is a loud configuration error; a baseline with fewer than 5 samples (a legitimately partial base run) is reported and not gated.
• Benchmarks present on only one side never gate, which makes the gate allowlist-free: every benchmark in the gated packages runs, and new ones auto-join once they exist on both sides. Because each side runs its own commit's make bench-gate, a PR that grows BENCH_GATE_PACKAGES cannot break the base run.
• A failing merge-base run degrades to whatever partial output it produced, plus a workflow warning — one broken package does not erase the baseline for the others.
• Captures with unparseable result lines (for example log output interleaved into a Benchmark line) fail loudly with exit 2 instead of silently dropping those benchmarks from both sides; the sync benchmarks silence the engine's logger for exactly this reason. A gated unit the baseline has but the candidate lost (e.g. -benchmem dropped) is also a configuration error; a unit missing from the baseline, and custom b.ReportMetric units, are reported as not gated.
• The gate runs a fixed -benchtime=20x so baseline and candidate measure identical workloads (two benchmarks deliberately grow their fixture per iteration); the count and benchtime are evaluated from the PR head's Makefile and passed into the merge-base run so changing them cannot skew the comparison. Benchmarks keep fixture construction and one-time leading-edge work out of the timed region so the gated ratios measure product cost, not test-helper cost.

Tradeoffs and limitations

• Subtle wall-clock regressions (< 2x or statistically ambiguous) are deliberately not time-gated; the allocation metrics act as the sensitive proxy for O(archive)-vs-O(delta) mistakes.
• On this PR itself, the merge base predates the bench-gate Make target, so the baseline step degrades to empty (with a visible warning) and every benchmark reports as new; the gate is fully live from the first PR after this merges.
• The resync sanitize-pass class (fixed on the unmerged heavy-angora branch) is not gated yet; a gate would fail against current main until that lands.
• golang.org/x/perf's module requirements force minor bumps of x/net, x/oauth2, x/crypto, x/exp, and x/text via MVS.
• The workflow runs only on PRs that touch Go files, go.mod/go.sum, the Makefile, or the workflow itself; docs- and frontend-only PRs skip it. Cross-backend query benchmarks stay in the opt-in internal/backendbench (Docker) and are not part of the gate.

Where to look

• cmd/benchgate/ — the comparison policy (worst-run gating, significance requirement, corrupted-capture and missing-unit reporting)
• .github/workflows/bench.yml — base-vs-head orchestration and partial-baseline degradation
• internal/sync/engine_bench_test.go, internal/db/messages_bench_test.go, internal/sync/perf_invariant_test.go
• docs/internal/performance-gates.md — regression history and how to add a gated benchmark

🤖 Generated with Claude Code

^{generated by a clanker}

[roborev-ci]

roborev: Combined Review (6e01b26)

No Medium, High, or Critical findings were reported.

Reviewers found no actionable findings at or above Medium severity.

---

Reviewers: 2 done | Synthesis: codex, 10s | Total: 10m17s

[wesm]

looking at this

[wesm]

I reviewed this branch and pushed 3109178 with fixes. Summary for reviewers, most severe first:

Bugs fixed

1. The three sync benchmarks were silently absent from the gate. The engine's log.Printf output interleaves into go test -bench result lines on stdout, so benchfmt could not parse BenchmarkSyncAllWarmNoop, BenchmarkSyncPathsIncrementalAppend, or BenchmarkSyncAllColdArchive. Because the corruption was identical on both sides, they did not even appear as new/removed — the gate's headline benchmarks measured nothing. The benchmarks now silence the global logger (silenceBenchLogs), and benchgate treats unparseable result lines as a corrupted capture: reported with positions, exit 2. Without that second half, the same failure mode could return invisibly.

2. Any base-side failure emptied the whole baseline, turning the gate into a vacuous pass. The worst trigger was the documented procedure for growing the gate: adding a package to BENCH_PACKAGES made the merge-base run exit 1 ("matched no packages"), truncating the baseline for every package on that very PR. The workflow now runs make bench-gate on both sides — the Makefile is the single source for the package list, sample count, and iteration count — and each side benchmarks its own commit's list, so growing the gate cannot break the base run. A failing base run keeps whatever partial output it produced (plus a ::warning) instead of truncating. To support partial baselines, the sample-count policy is now asymmetric: too few candidate samples is still a config error, but a short baseline is reported and not gated.

3. Silent gate gaps became report lines. A gated unit missing from one side (e.g. a baseline captured without -benchmem) now reports "missing from baseline, not gated" instead of a bare continue; custom b.ReportMetric units report "has no gate, not gated"; and detected regressions print even when a config issue forces exit 2, so a real violation is never hidden behind a configuration error.

4. Measurement quality of the append benchmark. The first append after a full sync triggers the leading-edge O(history) signal recompute inside the timed loop — roughly 58% of the measured allocs/op was that one-time cost, so a 1.25x steady-state per-append regression only moved the gated metric ~1.10x and passed. The benchmark now absorbs it before ResetTimer, stretches the debounce window so the 10s flush timer cannot fire nondeterministically inside a worst-run-gated loop, and pre-builds the appended lines. BenchmarkInsertMessagesBatch similarly builds its 200-message fixture once outside the timer (only the SessionID is rewritten per iteration, which allocates nothing).

Cleanups: testDB widened to testing.TB, which removes the bench-only openBenchDB copy and the pre-existing testDB(&testing.T{}) hack in BenchmarkGetDailyUsage (its require failures and cleanups were wired to a detached T that this workflow would have run 12x per PR); benchgate's compare()/main() split into evalGate/compareUnits/render/parseFlags; set -euo pipefail in both run steps; a Go-paths filter so docs/frontend-only PRs skip the job; docs updated to match.

Implications

• On this PR itself, the merge base predates the bench-gate Make target, so the baseline step degrades (with a visible warning) and every benchmark reports as "new, no baseline". The gate is fully live from the first PR after this merges.
• benchgate exiting 2 on corrupted captures means a future benchmark whose code logs to stdout fails its own PR loudly rather than silently un-gating itself. The docs now tell benchmark authors to silence loggers.

Verified end-to-end: the full gated suite parses all 8 benchmarks, a self-compare exits 0, an injected corrupted line exits 2, and the benchgate/db/sync suites pass.

[roborev-ci]

roborev: Combined Review (3109178)

Summary verdict: Medium issues remain; no Critical or High findings were reported.

Medium

• cmd/benchgate/main.go:311
  A gated metric present in the baseline but missing from the candidate is only reported as “not gated” and does not create a config issue, so render can still exit 0. If -benchmem is dropped or the candidate capture loses B/op / allocs/op, CI can silently disable deterministic gates despite the documented exit-2 policy.
  Fix: Treat missing candidate gated units as configuration issues so the gate exits 2, while keeping intentionally partial-baseline behavior explicit.

• internal/sync/engine_bench_test.go:296
BenchmarkSyncAllColdArchive claims to measure bulk-write ingest but calls SyncAll, which uses syncWriteDefault; the actual writeBatchBulk / DB.WriteSessionBatch path used by resync is not exercised, leaving the documented bulk-ingest regression class ungated.
  Fix: Add or change a benchmark to drive syncAllLocked(..., syncWriteBulk, ...), resync, or DB.WriteSessionBatch directly.

---

Reviewers: 2 done | Synthesis: codex, 9s | Total: 9m25s

[roborev-ci]

roborev: Combined Review (6342b2b)

No issues found.

---

Reviewers: 2 done | Synthesis: codex | Total: 7m41s