feat(perf): performance test harness, Server-Timing middleware, and baseline evidence

[PaoloC68]

Summary

Adds the performance testing infrastructure for the admin UI:

• Perf test tier (tests/luthien_proxy/perf_tests/): Playwright harness with per-page timing, throttled-network, transcript-open, and SSE memory scenarios
• Server-Timing middleware (src/luthien_proxy/perf/timing_middleware.py): Instruments db, serialize, render phases on history/debug endpoints
• Deterministic seeding (src/luthien_proxy/perf/seeding.py): 100/1k/10k tier seeding with isolated perf DB
• EXPLAIN capture (scripts/perf_explain.py): SQLite EXPLAIN QUERY PLAN output at 10k scale
• Baseline evidence (.sisyphus/evidence/perf-report-baseline.md): SQLite baseline captured
• SSE regression test: Activity stream regression test in sqlite_e2e tier
• API contract snapshots: JSON shape snapshots for 4 endpoints

Changelog: changelog.d/perf-baseline.md

Depends on: nothing (standalone infrastructure)
Followed by: perf-baseline PR (cursor pagination + UI rewrites)

[claude]

Review: feat(perf): performance test harness, Server-Timing middleware, baseline evidence

Overall a high-quality piece of infrastructure work. Carefully scoped path filter, ContextVar isolation, explicit isolation guards on the perf DB, and reasonable test coverage for new modules. The notes below are roughly in priority order.

Higher-impact

1. result.model_dump() called twice on every request just to time serialization.
   In history/routes.py:91-93,110-112 and debug/routes.py:61-64,127-129 the handler explicitly calls result.model_dump() inside time_phase("serialize") purely to capture a measurement — but FastAPI will then serialize the same model again on its way out. For the largest endpoint (fetch_session_detail on the 442-msg fixture) this doubles serialization cost on every request, undermining what the Server-Timing middleware is trying to measure. Two cleaner options:

   • Wrap a custom JSONResponse so serialization happens once inside time_phase.
   • Drop the explicit model_dump() and rely on db-only timing; serialize time can be derived later if needed.

2. perf/seeding.py recreates indexes from a hardcoded list.
   _seed_sqlite drops/recreates a fixed set of indexes (lines 150–234). If a future migration adds, renames, or changes a WHERE clause on a conversation_* index, the perf DB will diverge from production schema silently — and EXPLAIN QUERY PLAN output captured for "baseline" comparisons will be misleading. Two safer options:

   • Reapply migrations after seeding instead of hand-listing indexes.
   • Add a unit test that diffs the post-seed schema against a freshly migrated DB.

3. Seeding module lives under src/luthien_proxy/perf/ but is test-only.
   timing_middleware.py is legitimately production code, but db.py and seeding.py are test infrastructure: they import sqlite3 directly, use random fixtures, reach into _apply_sqlite_migrations (private). Shipping them inside the wheel adds surface that operators can call (and accidentally point at production data if ensure_perf_isolation is ever bypassed). Consider splitting:

   • src/luthien_proxy/perf/timing_middleware.py (production)
   • tests/luthien_proxy/perf_tests/_seeding.py (test-only, not shipped)

4. scripts/perf_explain.py always writes to the same baseline-query-plans.md path (line 29). The PR also includes after-query-plans-sqlite.md and perf-report-after-sqlite.md, but the script has no --label/--output flag — capturing an "after" run requires manual renaming, which is fragile. Add a --label {baseline,after} or --output PATH argument.

5. Repository bloat from evidence files. The PR adds ~17k lines of CI-style logs (.sisyphus/evidence/*.log, *-devchecks.txt). These look more like ephemeral artifacts than source. Consider gitignoring .sisyphus/evidence/*.log and .sisyphus/evidence/task-*.txt and only committing the curated perf-report-*.md summaries.

Smaller correctness notes

6. perf_gateway_url fixture lifecycle (tests/.../perf_tests/conftest.py:218-289). The fixture mutates ANTHROPIC_BASE_URL/ANTHROPIC_API_KEY for the duration of the session — but errors raised before reaching the yield (e.g. RuntimeError("Perf gateway did not start within 10 s")) restore env in the failure branch, while exceptions inside create_app would leak the mutated env vars to subsequent test modules. Wrap the entire setup in a try/except that restores env on any failure path.

7. _extract_shape only inspects the first list element (test_api_contract.py:58-61). Heterogeneous lists or schema drift on the 2nd+ element will pass silently. Acceptable for sanity, but worth documenting in the test docstring, or extracting {frozenset(_extract_shape(x) for x in obj)} as a comparison key for full coverage.

8. add_init_script accumulates per-call (conftest.py:112-132). measure_page_load adds the same init script every time it's invoked; the window.__perfObserverInstalled guard prevents observer duplication but the script body still re-runs on each navigation. Trivial perf cost; worth a one-line comment so a future reader doesn't assume it's idempotent registration.

9. Server-Timing header may emit duplicate phase names. If the same time_phase("db") block runs twice in one request (e.g. paginated query + post-lookup, which already happens in _fetch_session_list_sqlite), the header value will have two db;dur=… entries. That's valid per the spec and browsers handle it, but the format_phases docstring/example shows only unique names — a quick note clarifying repeated-name semantics would help.

10. time_phase discards phases when called outside the middleware context (timing_middleware.py:70-72). This is intentional and well-documented, but currently silent — a debug-level log when the ContextVar is missing would catch mis-wiring during development.

Test coverage / nits

• Strong unit coverage for the new perf/ modules, including the concurrent-isolation test for _phases_var. Nice.
• Integration test only asserts the absence of Server-Timing on /v1/messages and /health. Worth adding a positive integration assertion (present on /api/history/sessions or /api/debug/calls) that exercises the full middleware stack, not just the in-test _make_app factory.
• test_seeding_refuses_dev_db is great — exactly the right kind of guardrail test for the isolation invariant.

Documentation

• dev/context/migration_concurrent.md is well-written but doesn't appear to be referenced from this PR's code changes. If it's meant as persistent context for the follow-up perf-fix PR (which the changelog hints at), drop a pointer in MEMORY.md or dev/context/codebase_learnings.md so future agents can find it.

---

Generated by Claude (claude-opus-4-7) on PR #753.

[claude]

Claude review — perf-infra (PR #753)

Solid infrastructure PR — Server-Timing, deterministic seeding, cursor pagination, fragment endpoints, perf-test tier. Test coverage is good (cursor tampering, concurrent context isolation, path filtering). A few things worth addressing before flipping out of draft.

Security

1. Cursor HMAC key default is a hardcoded dev secret — src/luthien_proxy/config_fields.py:190

ConfigFieldMeta(
    "cursor_hmac_key", "CURSOR_HMAC_KEY", str, "luthien-perf-cursor-key-dev",
    ...
)

The default "luthien-perf-cursor-key-dev" is checked into source. If a production deployer doesn't override CURSOR_HMAC_KEY, anyone can forge pagination cursors. Cursors today only encode (last_ts, session_id), so the blast radius is limited (you can skip to or repeat arbitrary pages), but the moment cursors carry any access-control bits this becomes a real bypass. Either drop the default and raise ValueError at startup if unset, or generate a random per-process key when no env value is provided and log a warning.

2. Likely XSS in templates/fragments/sessions.html

<div class="session-card"
     onclick="window.location='/conversation/live/{{ session.session_id }}'">

Jinja autoescape converts ' → ', which is correct for an HTML attribute. But after HTML attribute parsing the value is decoded back to a raw ' before the JS engine evaluates it — so a session_id containing ';alert(1);// round-trips through autoescape and lands inside the JS string as a break-out. Session IDs come from client-controlled headers in some auth modes, so this is reachable. Fix: use a data-session-id attribute and bind via @click / addEventListener (you're already using Alpine), or | tojson | safe the value.

3. Inconsistent search semantics across backends — history/service.py:_fetch_sessions_page
SQLite branch uses session_id LIKE ? (case-sensitive); Postgres uses ILIKE. Worth picking one (probably case-insensitive everywhere — SQLite supports PRAGMA case_sensitive_like = OFF, or just LOWER(session_id) LIKE LOWER(?)).

User-supplied q is parameterized — no SQLi — but % / _ in input aren't escaped, so a user typing _ matches any character. Minor UX wart, not a bug.

Correctness / bugs

4. Double serialization in /api/history/sessions, /api/debug/calls, etc.

result = await fetch_session_list(...)
with time_phase("serialize"):
    result.model_dump()        # discarded
return result                  # FastAPI serializes again

You're measuring the right thing but doing the work twice, which inflates serialize time and doubles real CPU on the hot path. Either return JSONResponse(result.model_dump()) from inside the time_phase block, or accept that "serialize" can't be measured cleanly from inside the handler and move it to middleware.

5. Misleading middleware-ordering comment — main.py:443

# Add ServerTimingMiddleware as the last (innermost) middleware
# so it captures actual handler latency
app.add_middleware(ServerTimingMiddleware)

app.add_middleware adds to the outside of the stack — the last call is outermost, so this middleware sees the full pipeline duration, not just handler latency. The effect is probably what you want (you get a number that's closer to what the client measures), but the comment claims the opposite.

6. UUID cast in _fetch_session_turns_page — history/service.py

cursor_id_param: str | _UUID = _UUID(cursor_event_id) if not db_pool.is_sqlite else cursor_event_id

This will ValueError at runtime if a Postgres-backed event id ever isn't a valid UUID (e.g. seeded fixtures, future event_id schema). Guard the conversion or branch on the event row type.

7. Unindexed full-text scan in the "Claude Code" quick filter

AND session_id IN (SELECT DISTINCT session_id FROM conversation_events WHERE payload::text ILIKE '%claude-code%')

At the 10k-session scale you're benchmarking this is going to be the slowest query in the entire UI. If the goal is "show sessions that used Claude Code", filter on payload->>'final_model' LIKE 'claude%' (indexable, no full-payload scan).

8. Alpine state hack in history_list.html

const newSessions = Array.from(container.querySelectorAll('.session-card')).length - this.sessions.length;
for(let i=0; i<newSessions; i++) this.sessions.push({});

The comment already calls it a hack. The bigger issue is that the canonical session data now lives in DOM rather than reactive state — meaning Alpine reactivity around sessions (empty-state guard, future filter/sort) is detached from reality. Either drop the empty-state guard and rely on server-rendered empty fragments, or have the server return JSON and render Alpine-side. Don't half-and-half.

9. Feature regression vs. previous list page
The Alpine rewrite removes: Today / This-week / Last-week / Last-30-days quick filters, sort dropdown (newest/oldest/longest/shortest), Modified-by-policy / Unmodified filters, and Today/Yesterday date grouping. If intentional (server-side pagination makes some impossible to compute cheaply, fair), call it out in the changelog. If accidental, this is a noticeable UX regression for current users.

Minor

• __all__ in history/service.py exports _fetch_session_turns_page and _fetch_sessions_page — pick one: drop the underscore prefix or drop them from __all__.
• _fetch_sessions_page(filter: str | None = None) shadows the builtin filter. Rename to quick_filter or tag.
• 400 Invalid cursor: {e} echoes the inner exception. Messages are generic today, but the pattern invites accidentally leaking key/state info later — consider returning a fixed "Invalid cursor" and logging the detail server-side.
• seeding.py uses synchronous=OFF — fine for benchmark setup, but worth a comment saying "intentionally unsafe; perf DB is disposable".
• Truncated 8-byte HMAC (hmac.new(...).digest()[:8]) is borderline for a signed token. Forgery is 1 in 2^64, which is fine for opaque pagination, but bumping to 16 bytes costs nothing and is the industry default.
• perf/cursor.py is a one-line re-export shim. If nothing imports it directly, just delete it; if something does, the comment should say what.

Test coverage

Good: cursor roundtrip / tampering / short-token, concurrent ContextVar isolation, Server-Timing path filter (positive + negative), fragment pagination + filter combinations.

Gaps worth filling:

• No test that the cursor-page-overlap invariant holds at a tied last_ts (the whole reason for the composite cursor).
• No test for the XSS case above — a session_id containing ' / < / > would be a good regression fixture for the fragment template.
• No test for the "30days" / "claude" quick filters across both backends.
• test_db.py:test_ensure_perf_isolation_accepts_perf_db uses a hardcoded /Users/test/... path — harmless but won't make sense on the CI Linux runner; consider tmp_path for symmetry.

Happy to dig deeper on any of these if useful.

[claude]

Code Review — PR #753

Solid foundation overall: isolated perf DB, deterministic seeding, contextvar‑based timing middleware with good concurrency tests, and broad coverage across page-load / transcript / throttled / SSE / API-contract / EXPLAIN scenarios. The middleware design and isolation gates are particularly well thought out. Below are the issues worth addressing — grouped by severity.

Blocking / High-priority

1. changelog.d/perf-fix.md describes work that isn't in this PR. The PR body explicitly says optimization work is "Followed by: perf-baseline PR (cursor pagination + UI rewrites)", and the file list confirms no HTML/JS/fragment-route changes. Yet changelog.d/perf-fix.md claims:

   • "Cursor-paginated infinite scroll on /history"
   • "Lazy-loaded turns on /conversation/live"
   • "New fragment endpoints: /ui/fragments/sessions, /ui/fragments/sessions/{id}/turns"
   • "Debounced server-side filter"

   None of those are in the diff. Either remove perf-fix.md from this PR (it belongs to the follow-up) or this PR is bundling two concerns — CLAUDE.md's "One PR = One Concern" rule applies.

2. Seeding payloads are 10–70× smaller than the docstrings claim. src/luthien_proxy/perf/seeding.py:83-92:

   • _req_payload docstring says "~5 KB JSON string"; actual content is _REQ_HEAD + content + _REQ_MID + content + _REQ_TAIL, which works out to roughly 500–700 bytes.
   • _resp_payload docstring says "~20 KB"; _RESP_PAD = "B" * 100 produces a payload of roughly 300 bytes.

   This isn't just a comment bug — the entire perf baseline is being measured against a payload distribution that's a tiny fraction of production size. p95 numbers and the "after" comparison in .sisyphus/evidence/perf-report-after.md will not be representative of real Sami-like sessions. Either pad the payloads up to the documented size, or update the docstrings and the baseline-report context so consumers don't draw the wrong conclusions from this data.

3. Drop-and-recreate of indexes in seeding silently breaks on future migrations. src/luthien_proxy/perf/seeding.py:150-234 drops a hardcoded list of indexes, bulk-inserts, then recreates the same hardcoded list. If migrations ever add a new index (e.g. user_id, model_name, etc.), the perf DB will end up missing it after every reseed — making EXPLAIN plans and query timings diverge from production. Options:

   • Read the index list from sqlite_master before dropping, recreate by name; OR
   • Skip the drop/recreate dance entirely (5k‑row batched inserts are fast enough); OR
   • At minimum, add a test that asserts the seeded DB has the same indexes as a freshly‑migrated DB.

Notable issues

4. _discover_html_routes() runs at module import time and leaks a DB pool. tests/luthien_proxy/perf_tests/test_page_load.py:41-76 constructs a DatabasePool and calls create_app(...) on every import — including during plain pytest --collect-only. The pool is never closed. Move this into a session-scoped fixture (or use FastAPI's app object created by the existing perf_gateway_url fixture) so collection is side-effect free.

5. conftest.py cleanup_loop is the wrong loop. tests/luthien_proxy/perf_tests/conftest.py:225-289 creates cleanup_loop = asyncio.new_event_loop() and uses it to await db_pool.close(). But the pool was opened indirectly from the uvicorn thread that runs asyncio.run(...) internally. Closing async resources on a different loop than they were created on works inconsistently across drivers. Today this is "fine" because aiosqlite is thread‑local, but it will bite once a Postgres backend lands. Prefer asyncio.run(db_pool.close()) here, or open the pool on the cleanup loop up front.

6. perf_gateway_url clobbers global env vars for the session. Same fixture, lines 236-247: sets ANTHROPIC_BASE_URL and ANTHROPIC_API_KEY in os.environ (session-scoped, restored only at teardown). If any other test marker is collected into the same pytest session — including the new sqlite_e2e regression test in this PR — it'll see these values. The perf marker is excluded by default so this is latent, but worth a comment or a monkeypatch-based scoping.

7. postgres path of get_perf_db_url is not exercised. src/luthien_proxy/perf/db.py:30-34 builds ?options=-csearch_path=perf_test. That happens to be the correct libpq syntax (-c search_path=... with spaces removed), but no test validates that a Postgres connection actually lands in the perf_test schema. Since the rest of the Postgres path is NotImplementedError, please add a # TODO or strip the unused Postgres code from drop_perf_db/get_perf_db_url until _seed_sqlite's counterpart exists. Otherwise the first person to flip the switch will discover a bug.

8. drop_perf_db calls asyncio.run unconditionally for the Postgres path. src/luthien_proxy/perf/db.py:86. Fine from scripts, but if anything ever calls this from inside a running loop (e.g. an async test fixture cleanup) it raises RuntimeError: asyncio.run() cannot be called from a running event loop. Either document "scripts/sync contexts only", or accept an optional loop.

9. _apply_sqlite_migrations is imported as a private symbol. src/luthien_proxy/perf/db.py:115 imports from luthien_proxy.utils.migration_check import _apply_sqlite_migrations. Reaching into another module's underscore-prefixed function is fragile — promote it (apply_sqlite_migrations) and use the public name. Otherwise a maintainer renaming the private will silently break perf migrations.

10. API-contract snapshots embed shapes that are brittle to seed data. tests/luthien_proxy/perf_tests/snapshots/sessions_list.json has "user_ids": "list[unknown]" because seeded sessions don't populate user_id. The day a user_id column gets backfilled by seeding (or by seed_sami_like becoming more realistic), the shape will move to ["str"] and the contract test fails for a no-op reason. Consider normalizing empty-list shapes to a sentinel and adding a comment that explains why, or always seed a representative user_id so the shape settles.

11. _extract_shape only inspects list[0] in test_api_contract.py:58-61. A heterogeneous list (e.g. union types, optional fields) won't be detected. Acceptable for now but worth a note in the docstring.

12. test_path_filter_includes_debug matches /api/debug/events, which isn't a real route — it's just a path that satisfies the /api/debug/ prefix. Test still proves what it claims; minor nit: pick a path that maps to a real handler (/api/debug/calls) so the test doubles as a smoke check.

13. time_phase records phases when the block raises. timing_middleware.py:65-72 puts the append in finally. That's reasonable behavior, but worth a one-line note in the docstring so a future reader doesn't assume phases are skipped on exception (they aren't).

Performance considerations

• Server-Timing middleware is correctly excluded from /v1/messages — good. The contextvar isolation test (test_concurrent_isolation) is a nice catch.
• Session-list SQLite path now does 3 queries on the cold path even when there are no rows — the early-return at line 625 takes care of that; nice.
• The new time_phase("db") and time_phase("serialize") wrappers add nanosecond overhead on the hot history paths — negligible and worth it for observability.

Security

• No new auth gaps. All new endpoints stay behind verify_admin_token. The seeded JSON payloads are pure ASCII and don't risk SQL/JSON injection.
• The Postgres URL composition in get_perf_db_url interpolates DATABASE_URL verbatim and appends an option string — fine because the value comes from a trusted env var, but if a user copy‑pastes a malformed URL they'll get a confusing error rather than an explicit validation message.
• Throttled-network tests run real Playwright with no isolation from local network — assumed acceptable given perf tests are opt-in.

Test coverage

• Strong: middleware concurrency, isolation enforcement, idempotent seeding, snapshot contracts, in‑process gateway harness.
• Weak: no unit test for the index drop/recreate behavior (see datetime.datetime.utcnow() is deprecated #3), no test asserting payload size matches docstring claims (Remove PolicyEngine and inline Redis setup #2), no Postgres path exercised at all.
• The unit-test layer is well‑mirrored under tests/luthien_proxy/unit_tests/perf/.

Small nits

• scripts/perf_explain.py:22 mutates sys.path instead of relying on uv run's package install; the comment explains why but uv run python -m scripts.perf_explain would be cleaner.
• scripts/run_perf.sh example in help shows sqlite://... (two slashes) instead of sqlite:///... (three). Real users will hit this.
• _get_event_summary docstring says "every current emitter writes a non-empty summary" — that invariant lives in code far from this file. A # linked invariant: see <file> pointer would help future maintainers.
• dev/context/migration_concurrent.md is excellent context — thanks for capturing the runner analysis. (It belongs to a different concern than this PR; consider promoting it or moving it to its own PR.)

---

Overall this is a high‑quality scaffold. The seeding payload‑size discrepancy (#2) and the bundled perf-fix changelog (#1) are the two that should be resolved before merging — the rest are cleanups that could land here or in a follow-up.

[claude]

Review: perf-infra (PR #753)

Reviewed core code changes (skipping evidence/log artifacts). Overall this is a well-structured infrastructure PR: isolation enforcement, deterministic seeding, and clear separation between baseline-only measurement and SLO assertion. Comments below in rough order of importance.

Bugs / behavioral concerns

1. Relative EVIDENCE_DIR = Path(\".sisyphus/evidence\") in all four perf-test modules (test_page_load.py:30, test_throttled_network.py:28, test_sse_memory.py:31, test_transcript_open.py:28). Pytest can be invoked from any cwd, including subdirectories; writing relative paths silently lands evidence outside the repo. Resolve against Path(__file__).resolve().parents[3] (or the existing _REPO_ROOT pattern used in scripts/perf_report.py).

2. drop_perf_db(\"postgres\") calls asyncio.run() (src/luthien_proxy/perf/db.py:87). If ever invoked from within an event loop (which any pytest-asyncio fixture / FastAPI handler would be), this raises RuntimeError. The comment says "untested," so flagging now: prefer await from an async wrapper and have callers manage the loop.

3. Debug routes lost response_model (src/luthien_proxy/debug/routes.py — both /api/debug/calls and /api/debug/calls/{call_id}). The OpenAPI schema for these endpoints no longer documents the response shape — that's a quiet regression for any client that introspects /docs or generates clients from OpenAPI. The with time_phase(\"serialize\") block is what motivated the switch to JSONResponse. Consider keeping response_model= and moving the timing earlier (around the dependency-resolved object construction), or only wrap with JSONResponse when timing is actually needed. The history routes have the same pattern but they were already returning JSONResponse, so no regression there.

4. _apply_sqlite_migrations → apply_sqlite_migrations rename (src/luthien_proxy/utils/migration_check.py:56). Removing the leading underscore makes this part of the public utils API solely to satisfy a perf-test consumer. Prefer either: (a) keep the private name and add a thin public migrate_for_perf wrapper in perf/db.py that imports the private symbol, or (b) leave the rename but add a docstring note that this is intentionally public. Right now there's no signal that this was a deliberate API surface change.

5. test_first_turn_painted_500_turns in test_transcript_open.py:202 is parameter-fixed to the 442-message session and the docstring acknowledges this is "closest available". Misleading name will eventually drift further; rename to test_first_turn_painted_largest_sami_session or include the actual count.

6. Changelog metadata mismatch: changelog.d/perf-baseline.md declares pr: 752 but this PR is feat(perf): performance test harness, Server-Timing middleware, and baseline evidence #753.

Performance / correctness on the gateway-side changes

7. ServerTimingMiddleware extends BaseHTTPMiddleware (timing_middleware.py:94). Starlette's BaseHTTPMiddleware is documented to have meaningful overhead vs. a pure ASGI middleware, particularly around streaming and exception edge cases (issue: streamed responses get fully materialized in some paths). Since this is added as the outermost middleware and explicitly intends to measure the full pipeline, that risk applies. A pure-ASGI implementation (a callable app: ASGIApp that wraps send) is ~30 lines and avoids these foot-guns. Worth doing before this expands beyond the three timed prefixes.

8. _phases_var.set without a prior default: time_phase() reads via _phases_var.get(None) and silently discards if outside a request — that's a deliberate design choice and it's tested in test_concurrent_isolation. Good.

9. PRAGMA synchronous=OFF in _seed_sqlite (seeding.py:145) is correctly commented as intentionally unsafe for the disposable perf DB. Note: the function also doesn't wrap the whole job in BEGIN EXCLUSIVE — Python's sqlite3 module auto-opens an implicit transaction per DML, so the multiple executemany calls plus the final commit() should coalesce into one transaction, but that depends on isolation_level. If you ever switch to isolation_level=None for parallelism this would silently break. Worth explicit BEGIN; ... COMMIT;.

10. Hardcoded index list in _seed_sqlite (seeding.py:152-160 and re-create at 218-236). This is a maintenance hazard: a new migration adding an index on conversation_events will not be dropped during bulk insert (write amplification) and the seeded DB will lack it. The test test_seeded_db_has_same_indexes_as_migrated_db catches the missing case at seeding time, which is great. But the test won't catch a partial mismatch where a new index gets created by both paths but with different definitions. Consider deriving the index list from sqlite_master at runtime: SELECT sql FROM sqlite_master WHERE type='index' AND tbl_name IN ('conversation_events', 'conversation_calls') AND name NOT LIKE 'sqlite_%', then drop+recreate by name+sql.

Security / safety

11. ensure_perf_isolation substring check on \"local.db\" (db.py:50) is a good defense, but a user with a project at /home/me/local.db.backup/perf.db would be rejected. Not actually exploitable — the failure mode is conservative — but the error message would be confusing. Minor.

12. History query SQL in _fetch_session_list_pg and _fetch_session_list_sqlite: the user_id placeholder slot is computed dynamically (f\"${len(session_ids) + 1}\") across three separate query bodies. There's a comment noting these can't be folded, but the dynamic offset is brittle. The existing test test_fetch_session_list_user_filter_sql_injection covers the security invariant; consider adding a tighter unit test that exercises the multi-user-filter code path on SQLite specifically (the diff doesn't show new coverage for the SQLite branch when user_id is set).

Tests

13. Test coverage for the new perf/ package is strong: db, seeding, timing-middleware, harness helpers all have unit tests with appropriate isolation patterns.

14. _extract_shape in test_api_contract.py uses only the first element's type for lists. For heterogeneous lists or Optional[X] arrays this hides schema drift. Acceptable as a starting point; flag this in a comment so future maintainers know the limit.

15. N_RUNS = 3 in test_throttled_network.py is a tight sample for median+p95; the file claims "median over ≥5 runs" elsewhere. Consider raising or documenting why 3 is fine for the CDP-throttle path.

Nits

• scripts/perf_report.py:113 hardcodes \"sqlite\" in the hardware section. With --backend postgres planned, this should be parameterized.
• tests/luthien_proxy/perf_tests/CLAUDE.md is a symlink per the repo convention — good.
• test_seeded_db_has_same_indexes_as_migrated_db is a great regression guard.

Summary

Solid baseline infrastructure. The biggest concerns are the relative EVIDENCE_DIR paths (will silently misbehave) and the BaseHTTPMiddleware choice for ServerTimingMiddleware (technical debt that will hurt once it's used more broadly). The debug-route response_model removal is a small but real OpenAPI regression and should be addressed before the follow-up perf-baseline PR builds on this surface.

[claude]

Review: perf-baseline

Overall this is a well-scoped infrastructure PR with strong isolation guards (dev-DB refusal), good determinism (deterministic seeding, snapshot tests), and thorough unit coverage. A few items worth attention before merge — one of them is a potential hot-path regression.

Significant concerns

1. ServerTimingMiddleware is a BaseHTTPMiddleware added as the outermost layer — affects /v1/messages and SSE streams even when the path filter excludes them.

In src/luthien_proxy/main.py:447, ServerTimingMiddleware is registered outermost. The early-return path filter

if not should_time:
    return await call_next(request)

does not bypass BaseHTTPMiddleware's pipe-buffering — call_next already runs the inner app through Starlette's internal memory-channel wrapper, which is the long-known footgun with BaseHTTPMiddleware and streaming responses (it can serialize chunks through a pipe, break ContextVar propagation in some Starlette versions, and add latency).

/v1/messages returns FastAPIStreamingResponse (pipeline/anthropic_processor.py) and /ui/routes.py:45 returns SSE. Both now traverse another BaseHTTPMiddleware. The PR description claims "All other paths (including /v1/messages) are untouched" / "zero overhead beyond a single str.startswith check" — this isn't quite accurate. The middleware's dispatch does run, and call_next is the lossy wrapper regardless of the branch.

Recommendation: implement as a pure ASGI middleware (a callable async def __call__(scope, receive, send)) that wraps send and conditionally adds the header, OR mount it only on the timed sub-routers. Either avoids the BaseHTTPMiddleware cost on the hot path. At minimum: add an integration test that captures TTFB / first-byte latency on /v1/messages SSE before vs after enabling the middleware, so any regression is visible.

2. History/debug routes now bypass FastAPI's response_model output filtering.

In history/routes.py and debug/routes.py, return types changed from SessionListResponse / SessionDetail / CallEventsResponse / CallListResponse to JSONResponse(content=result.model_dump(mode="json")) so the dump can be wrapped in time_phase("serialize"). Side effects:

• response_model=... on the decorator is now decorative — FastAPI does not validate the response when the handler returns a Response instance. A backend bug producing an extra/missing field will no longer be caught here, only in the snapshot tests (and only for the four endpoints those cover).
• Likely slower than before for the warm path: model_dump(mode="json") produces a Python dict, then JSONResponse re-serializes via json.dumps. Pydantic's model_dump_json() (which FastAPI uses under the hood with orjson when available) avoids the intermediate dict. So you're measuring a serialization path that is slower than what production was actually doing pre-PR. Worth confirming the baseline numbers reflect this, or switching to model_dump_json() and wrapping a Response(content=..., media_type="application/json").

3. _extract_shape in test_api_contract.py only inspects obj[0] (test_api_contract.py:65). Lists with heterogeneous items (discriminated unions, mixed event types) will silently miss shape regressions in items 2..N. Consider walking all items and unioning shape, or at least asserting list homogeneity.

Smaller issues

• ensure_perf_isolation Postgres check is substring-based ("perf_test" not in url). A DB or user named perf_test_dev, or a connection string with a stray comment containing perf_test, would pass. Consider parsing the URL and requiring options=-csearch_path=perf_test exactly.
• apply_sqlite_migrations rename (utils/migration_check.py): made public for perf/db.py and tests. Fine, but dev/context/migration_concurrent.md and any callsites elsewhere should be updated to match. Confirmed no other private-name callers via grep.
• tests/luthien_proxy/perf_tests/test_page_load.py::_discover_html_routes calls create_app() at module import time. This means test collection alone instantiates the app (DatabasePool, settings load, etc.). If anything in create_app has side effects (envs read, threads, etc.) this leaks into the collection phase and can affect parallel/other test runs. Move route discovery into a session-scoped fixture.
• perf_gateway_url mutates os.environ at session scope (ANTHROPIC_BASE_URL, ANTHROPIC_API_KEY). Under pytest-xdist or parallel test sessions this race-conditions. Document that perf tests must run in a dedicated pytest session (the run_perf.sh script does this; just worth a comment).
• drop_perf_db postgres branch is unreachable from tests and marked TODO (perf/db.py:71). asyncio.run inside a function that can be called from async contexts will blow up. Either gate with a clear NotImplementedError or implement properly; the current state invites a foot-gun if anyone calls it from inside an event loop.
• migrate_perf_db likewise wraps asyncio.run — fine for current sync call sites, but worth a docstring note that it cannot be called from async contexts.
• Tier-10000 disk footprint: ~250k events × ~25 KB resp + ~5 KB req ≈ 7-8 GB on disk. The _seed_sqlite PRAGMAs (synchronous=OFF, in-memory temp store) are appropriate, but run_perf.sh --tier 10000 should probably warn about disk requirements.
• SSE memory test (test_sse_memory.py) records only unless PERF_ASSERT_MEMORY=1. That's fine for a baseline PR, but the 60-s hold runs in every perf invocation regardless. Consider gating the test on an env var or marker so CI doesn't unnecessarily spend 90 s per run measuring something with no assertion.
• _REQ_PAD / _RESP_PAD magic numbers (2368, 20202) — fragile if _REQ_HEAD / _REQ_MID / _REQ_TAIL ever change. The size-range test (test_payload_sizes) catches this, so OK, but a comment near the constants pointing at the test that asserts ~5 KB / ~20 KB would help.
• Path filter _TIMED_PREFIXES is hard-coded. As more admin endpoints are added (/api/admin/*, /api/credentials/*, etc.), they will silently not be timed. Consider making the prefix list a config field, or default to all /api/ minus an explicit _v1 deny-list.

Nits

• ServerTimingMiddleware.dispatch(self, request, call_next) — call_next is untyped (# noqa: D102). Add RequestResponseEndpoint type for clarity.
• perf_explain.py::ensure_no_dev_db_in_env relies on the substring "isolation" in the error message of a sibling function — brittle coupling. Catch the exception type and short-circuit instead.
• seed_sami_like builds other_plan with the deterministic seed 0xABCDEF, but the docstring says the spread is 1-187 — pinning the exact distribution in a docstring or asserting it in a test would catch unintentional drift if random.Random's output ever changes across Python versions.
• The 4 contract snapshots commit the shape only, not field ordering — fine, but mention in the test docstring that _extract_shape is dict-order preserving in Python 3.7+, so reorder-only changes will be caught.

Test coverage

Coverage looks good for the new code (perf/db.py, perf/seeding.py, perf/timing_middleware.py all have unit tests; integration tests exercise the path filter and concurrency). One gap: no test for the time_phase recording when called outside a middleware request (the silent-discard branch in timing_middleware.py:73). Trivial to add: call with time_phase("x"): pass outside the middleware and assert it doesn't raise.

What I like

• Isolation guards everywhere (URL refusal, fixture-level checks, script-level checks) — the dev-DB story is well protected.
• Determinism: seed_sessions with a fixed seed + drop/re-seed equivalence test is exactly right.
• Cold-vs-warm separation in n_runs is the right primitive.
• test_seeded_db_has_same_indexes_as_migrated_db is a thoughtful invariant — protects against drop-index-and-recreate drift from the canonical migration definitions.
• Contract snapshots over manual schema assertions: low maintenance, high signal.

Happy to dig deeper on the middleware concern if useful — I think that one warrants either a measurement or a switch to a pure-ASGI implementation before this lands, given /v1/messages is the hottest path in the proxy.

[claude]

Review (Claude)

Solid foundation for the perf workstream — good test coverage, ContextVar-based isolation in the middleware, and strong perf-DB isolation guards. Below are concrete things to address or consider.

Inconsistency: get_call_diff was skipped

src/luthien_proxy/debug/routes.py:73-101 — get_call_events and list_recent_calls were both converted to manual Response + time_phase("serialize"), but get_call_diff was left returning CallDiffResponse directly. Either it's an oversight (this endpoint is missed for both timing and the manual-serialization perf win) or the choice is intentional but worth a one-line comment in the file explaining why.

response_model=... + return Response(...) silently disables output validation

The new pattern in history/routes.py and debug/routes.py:

@router.get("/calls/{call_id}", response_model=CallEventsResponse)
async def get_call_events(...) -> Response:
    result = await fetch_call_events(call_id, db_pool)
    with time_phase("serialize"):
        body = result.model_dump_json()
    return Response(content=body, media_type="application/json")

FastAPI's behavior: when you return a pre-built Response, it does not coerce/validate against response_model. OpenAPI docs still advertise CallEventsResponse, but the wire output is whatever result.model_dump_json() produces. If fetch_call_events ever returns a slightly different shape (extra debug fields, a missing Optional, etc.), the docs lie and there's no runtime check. Worth a comment on each route explaining the tradeoff (skip pydantic double-pass for perf) so the next person doesn't accidentally drift them apart. The new contract snapshot tests cover four endpoints, which helps, but not all of them.

Postgres options= query-string concatenation

src/luthien_proxy/perf/db.py:30-34:

separator = "&" if "?" in base_url else "?"
return f"{base_url}{separator}options=-csearch_path=perf_test"

If DATABASE_URL already contains ?options=... (e.g. ?options=-cstatement_timeout=5000), this appends a second options= parameter — libpq's behavior with duplicate keys is brittle, and an existing options=-csearch_path=public would be silently overridden. Parsing with urlsplit and merging the options would be more robust. Low priority — not blocking — but worth noting since the helper is the central isolation entry point.

PRAGMA cache_size=-131072 (128 MB) in seeding

src/luthien_proxy/perf/seeding.py:146 — 128 MB cache per seeding connection. Fine on a dev laptop, but if seeding ever runs in CI in parallel or on memory-constrained runners that's significant. The disposable-DB tradeoffs (synchronous=OFF, drop+recreate indexes) are correctly called out — consider noting the memory footprint in run_perf.sh next to the tier-10000 disk warning.

migrate_perf_db uses asyncio.run()

Documented, but it makes the helper fragile if seed_sessions or any of its callers is ever refactored to be async — the inner asyncio.run() will fail with "asyncio.run() cannot be called from a running event loop." Consider exposing both sync and async variants, or moving the asyncio.run() boundary up so it's only at the script entry point.

tests/luthien_proxy/perf_tests/conftest.py — concurrent fixture seeding

Several session-scoped fixtures (seeded_perf_db_all, seeded_transcript_fixtures, seeded_sami_sse, seeded_sami, seeded_perf_db) all do SELECT COUNT(*) ... WHERE session_id LIKE 'prefix-%'-then-seed. With session-scope + serial pytest this is fine, but the comment "perf tests must run in a dedicated pytest session" relies on run_perf.sh enforcing it. If anyone ever flips on pytest-xdist, the os.environ mutation in perf_gateway_url AND the seeding race will both break. A pytest_configure check that refuses to run perf tests under xdist would make this guard rail explicit.

Smaller notes

• tests/luthien_proxy/perf_tests/conftest.py:30-37 — pytest_addoption works because the perf-tests directory becomes pytest's rootdir when running pytest tests/luthien_proxy/perf_tests/. Running pytest --update-snapshots tests/ from the repo root would error with "unrecognized arguments." Minor; the orchestration script handles this correctly.
• src/luthien_proxy/perf/timing_middleware.py:144-149 — headers = list(message.get("headers", [])) is built unconditionally inside the http.response.start branch, but it's only mutated when phases is non-empty. Cosmetic — could skip the copy when there's nothing to append.
• ensure_perf_isolation does a substring check for local.db — that catches the dev path and would also catch e.g. ~/.luthien/my_local.db_backup. Conservative (false-positive-prone) is the right direction for an isolation gate, but might surprise someone someday.
• _apply_sqlite_migrations → apply_sqlite_migrations rename is a small drive-by alongside the perf work. The coupling (perf seeding needs to invoke the same migration apply path) is defensible, but "one PR = one concern" would split it. Not blocking.

What's good

• ServerTimingMiddleware is implemented as pure ASGI (not BaseHTTPMiddleware) — correctly avoids the streaming-response buffering pitfall, with a clear docstring explaining why. test_concurrent_isolation actually exercises the ContextVar isolation under interleaved requests, which is the only test that would catch the regression that motivated the pure-ASGI choice.
• Path filter is a single str.startswith against a tuple — /v1/messages truly pays zero overhead.
• ensure_perf_isolation is called from every perf-DB entry point (get_perf_db_url, migrate_perf_db, seed_*, perf_explain.py), run_perf.sh has a separate shell-level guard, and tests verify both halves. Layered defense done right.
• Test coverage for the perf module itself (db isolation, seeding, timing middleware, migration symbol export) is thorough and lives at the unit-test tier so it runs in default CI.
• changelog.d/perf-baseline.md lands a properly scoped fragment.

Overall: ready to merge after the get_call_diff consistency call (oversight vs. intentional) and ideally a one-line comment on each response_model + Response route explaining the perf tradeoff.

[claude]

Review: perf-infra (PR #753)

Overall this is solid foundational work — the isolation, ContextVar-based timing, and harness design are well thought out. A few comments below, mostly minor.

Strengths

• Pure-ASGI middleware instead of BaseHTTPMiddleware — the in-file comment correctly explains why (Starlette's pipe-buffering breaks streaming + ContextVar propagation). Good call.
• time_phase is orphan-safe (_phases_var.get(None) returns None outside a middleware-managed request and the phase is silently discarded). Tested directly in test_time_phase_outside_request_context_does_not_raise.
• ContextVar isolation under concurrency has a real test (test_phase_isolation_under_concurrency) that asserts cross-request leakage doesn't happen — exactly the property that's easy to get wrong.
• ensure_perf_isolation runs before every seed/migrate, with the magic word "isolation" baked into the message for grep-ability and tests. Defence in depth on a destructive operation.
• pytest_configure refuses xdist explicitly with a useful message — turns a silent race into a loud failure.
• n_runs separates cold from warm — first-run cold-cache measurements excluded from median/p95. Correct methodology for caching benchmarks.
• Seeding drops then recreates indexes around the bulk insert — standard but easily missed optimization. The test_seeded_db_has_same_indexes_as_migrated_db test guards against drift between migration-applied and seeding-applied index sets, which is a nice invariant to lock down.

Issues / suggestions

1. ensure_perf_isolation uses naive substring match. "local.db" in url would accept the dev DB even if someone constructs a path like /var/data/local.dbsetup/perf.db, and conversely could falsely reject a legitimate path that happens to contain that substring. Consider parsing the URL and comparing against Path(...).name == "local.db" or against the resolved dev-DB path. Low risk in practice, but the function is the only safety gate.

2. drop_perf_db("postgres") raises NotImplementedError but scripts/run_perf.sh documents --clean [--backend postgres] as a supported invocation, and the help text claims "executes DROP SCHEMA perf_test CASCADE". That path will blow up at runtime. Either implement it (a few lines) or gate the script so the unsupported combo fails early with a clear message rather than crashing in Python.

3. time_phase(\"db\") wraps more than DB calls in history/service.py. In _fetch_session_list_pg/_fetch_session_list_sqlite the block now wraps the entire async with db_pool.connection() body — including the Python work between queries (dict-building loops, list comprehensions, placeholder string construction). For 10k-tier sessions this Python is non-trivial. The label is then mildly misleading when reading Server-Timing headers and the report. Either narrow the scope to actual conn.fetch(...) calls or relabel as \"db+postprocess\".

4. Response(content=model.model_dump_json(), ...) with response_model= kept on the decorator. Documented intent: skip Pydantic's response-validation pass (the 2× serialization). This works, but the OpenAPI schema is then a claim that's no longer enforced — if a service function silently returns a field name not in the model, OpenAPI lies and only the snapshot tests catch it. The snapshot tests in test_api_contract.py are good coverage, but they run under the perf opt-in tier. Consider adding the contract tests (or at least one per endpoint) to a non-opt-in tier so drift gets caught on every PR, not only when perf is run.

5. asyncio.run(...) for fixture setup + teardown in test_server_timing.py. db_pool = asyncio.run(_setup()) creates the pool in one loop, then asyncio.run(db_pool.close()) closes it in a different loop. For SQLite this happens to work, but the pattern is brittle (asyncpg pools tied to a loop, etc.). Prefer pytest_asyncio async fixtures, or anyio.from_thread, so the pool lives in a single loop.

6. _extract_shape only inspects obj[0] for lists. Heterogeneous lists (e.g. turns with messages of different shapes, or events with different event_type payload structures) won't have shape divergence past index 0 detected by the snapshot. Probably OK for an API contract test, but worth a comment in the helper so future readers don't expect deeper coverage.

7. PRAGMA synchronous=OFF is annotated as intentional in the perf-seed path, which is fine for a disposable benchmark DB. The risk is small — a crash mid-seed just means a re-run — but since the perf DB and the dev DB share a parent directory, a future change that accidentally points seeding at local.db would be quietly destructive. The ensure_perf_isolation check covers this, so OK.

8. Massive evidence blobs (.sisyphus/evidence/*.log, ~14k lines) committed to the repo. These are valuable for the baseline-vs-after diff, but they'll bloat clones forever. Consider whether they belong in a separate artifact store or under dev/archive/ with rotation, vs. permanently in the tree.

Nits

• scripts/perf_report.py:_ram_info shells out to sysctl -n hw.memsize then falls back to /proc/meminfo. The macOS branch swallows all exceptions (except Exception: pass) before trying Linux — a Linux box where sysctl exists but returns garbage would silently skip both paths. Minor.
• seeding._call_count mixes rng_seed * 1_000_003 + session_idx for distinct seeds across tiers. Cute, but a random.Random((tier, session_idx)) would express intent more clearly.
• Response(content=..., media_type=\"application/json\") could share a small helper (_json_response(model)) to dedupe the with time_phase(\"serialize\") pattern across the four handlers and ensure they stay in lockstep.

Test coverage

Good — new unit tests cover the middleware (including concurrency), seeding (row counts, idempotency, payload sizes, isolation refusal), the perf DB (isolation + migration), and the report generator (deterministic output). The SSE regression test in the sqlite_e2e tier is a nice addition. Integration test for Server-Timing absence on /v1/messages is the right negative assertion; consider adding the positive assertion (header present on /api/history/sessions) in the integration tier too — currently only the perf tier exercises that path end-to-end.

[claude]

Review: PR #753 — perf test harness, Server-Timing middleware, baseline evidence

Strong, well-scoped PR. Architecture is clean, tests are comprehensive, and isolation enforcement is taken seriously. Below: a few real issues and several smaller nits.

Strengths

• Pure-ASGI middleware (timing_middleware.py:100) correctly sidesteps Starlette's BaseHTTPMiddleware streaming issues. ContextVar-based phase tracking is exercised by an explicit concurrent-isolation test (test_timing_middleware.py:90). Nice.
• Isolation discipline: ensure_perf_isolation + pyproject.toml excluding the perf marker from default runs + the explicit pytest_configure xdist refusal (conftest.py:40-46) make it hard to footgun yourself into the dev DB or run perf under parallel workers.
• Determinism: fixed RNG seeds + fixed _BASE_TS + the test_seed_idempotent test (test_seeding.py:55) and test_seeded_db_has_same_indexes_as_migrated_db (test_seeding.py:120) keep drift detectable.
• Scope discipline: Postgres backend stubbed with NotImplementedError rather than half-baked. Matches PR description.

Issues

1. AGENTS.md describes fixtures that don't exist

tests/luthien_proxy/perf_tests/AGENTS.md:55-60 lists browser, page, perf_admin_api_key, measure_time(), perf_db_path. Actual fixtures in conftest.py are playwright_browser, playwright_page, admin_headers, measure_page_load/measure_scroll_fps, perf_db_url. A dev following these docs will hit a fixture 'browser' not found error within their first 5 minutes.

2. measure_scroll_fps is unused dead code

conftest.py:174-224 defines a 50-line FPS helper that no test calls. AGENTS.md advertises a "Scroll Performance SLO (<33ms p95 frame time)" but no test enforces it. Either delete the helper or wire up a scroll test — CLAUDE.md explicitly calls out half-finished implementations.

3. time_phase("db+python") is misleading

In history/service.py, the with time_phase("db+python"): block in _fetch_session_list_pg and _fetch_session_list_sqlite wraps the await conn.fetch(...) calls but exits before the row hydration (preview extraction, model dedup, parse_db_ts) runs in the list comprehension. So the phase is "db + connection acquire", not "db + python". Compare to debug/service.py which uses plain "db". Either:

• rename to db for cross-handler consistency, or
• extend the block to include hydration so the name is accurate.

4. ensure_perf_isolation is a blocklist, not an allowlist

perf/db.py:50 only rejects URLs containing local.db. A URL like sqlite:///$HOME/.luthien/scratch.db passes silently and would get seeded over. Consider an allowlist (the URL must end in perf.db or specify the perf_test Postgres schema) — the current check protects only the one known dev-DB name.

5. Response return type skips response_model validation — only 4 of 6 affected endpoints have snapshots

history/routes.py and debug/routes.py now return pre-built Response objects, which makes FastAPI skip response_model validation. The double-serialization optimization is good, but the snapshot tests (test_api_contract.py) only cover 4 endpoints. /api/debug/calls/{call_id}/events and /api/debug/calls/{call_id}/diff also lose validation and have no contract test. Either add snapshots for them or accept the gap — but the gap should be noted.

6. apply_sqlite_migrations rename smells like a workaround

The rename + docstring justification ("intentionally exported without a leading underscore so that perf/db.py and test infrastructure can call it directly") reads as the perf module reaching into another module's internals. Consider whether migrate_perf_db should call check_migrations (the existing public function) instead.

7. Direct seed_sessions("sqlite", tier=10000) lacks the disk-space guard

run_perf.sh warns about 7–8 GB disk + 128 MB SQLite cache when tier=10000 is requested via CLI, but seed_sessions("sqlite", tier=10000) is called directly from test_page_load.py:102 with no equivalent check. Consider promoting the warning to a Python-side guard at the seed_sessions entry point so the safety net works regardless of caller.

Minor nits

• timing_middleware.py:156: body: str | bytes = model.model_dump_json() — Pydantic v2's model_dump_json returns str, not str | bytes. The widening + # type: ignore[attr-defined] could both go away if model were typed as BaseModel.
• scripts/run_perf.sh:12-13: ABOUTME: appears twice — likely template artifact.
• test_api_contract.py:24: from pathlib import Path re-imported inside the function (already at module level).
• test_payload_sizes (test_seeding.py:111) imports private _req_payload/_resp_payload — fine for an invariant test, just couples to implementation.
• seed_sami_like's rng.randint(1, 187) for the 77 non-outlier sessions doesn't produce the same long-tail distribution as _call_count. Not a bug — just means tier-100 is not a scaled-down sami-like fixture, even though the AGENTS.md framing implies the relationship.

Security & performance

No security concerns — synthetic ASCII payloads, isolation gates in place, the apply_sqlite_migrations rename doesn't expand attack surface. Performance optimizations (index drop/recreate around bulk insert, WAL + synchronous=OFF for the disposable perf DB, single-connection seed run, pre-serialized Response) all look measured and reasonable. The PRAGMA synchronous=OFF is explicitly justified ("intentionally unsafe — perf DB is disposable") which is the right call to document.

Test coverage

Excellent breadth — concurrent isolation, idempotency, payload-size invariants, index parity, isolation refusal, ASGI middleware path filtering. The known gap (SLOs not asserted by default, opt-in via PERF_ASSERT_SLO=1) is the stated scope of this PR.

[claude]

Code Review — perf infrastructure + Server-Timing

Substantial, well-documented PR. The isolation story is strong, the middleware design avoids the classic BaseHTTPMiddleware streaming pitfalls, and the security comments around user_id filtering in history/service.py are exemplary. A few concrete bugs worth fixing before merge.

🔴 Bugs

1. scripts/perf_report.py cannot parse actual measurement output.
_find_result(results, type_) looks for r["type"] == "page_timings" etc. (perf_report.py:98, 237), but none of the perf tests write a type field — test_page_load.py writes {"fixture": ..., "scenarios": [...]}, test_throttled_network.py writes {"fixture": ..., "route": ...}, etc. As a result, every measurement section in the generated baseline renders NO DATA YET — confirmed in the committed .sisyphus/evidence/perf-report-baseline.md (7 occurrences). The committed reports only contain the hardware/query-plan sections, so the "baseline" isn't actually a measurement baseline yet. Either the tests need to emit type consistently, or the reader needs to recognize the current file shape.

2. --assert-slo flag in run_perf.sh is wired to nothing.
The flag exports PERF_ASSERT_SLO=1 (run_perf.sh:241), but grep -rn PERF_ASSERT_SLO tests/ finds zero readers. test_page_load.py enforces SLOs unconditionally via the hardcoded _SLO_FIXTURES × _SLO_PAGES matrix; test_throttled_network.py reads a different env var (PERF_THROTTLE_BASELINE) which run_perf.sh never sets; test_sse_memory.py reads PERF_ASSERT_MEMORY, also unset. Net: --assert-slo and --throttled are no-ops for ~half the suite. Pick one convention and wire it end-to-end.

3. Disk-warning math in seeding.py is off by ~30×.
f"~{tier * 25 // 1000} GB" (seeding.py:277) yields 250 GB for tier=10000, while the run_perf.sh warning correctly estimates 7–8 GB and actual seeded size is ~6–7 GB (avg ~27 calls/session × 25 KB/call × 10 000 sessions). The warning will scare users into thinking they need 250 GB free.

🟡 Smaller issues

4. _extract_shape inconsistency for empty lists (test_api_contract.py:75)
Returns "list[unknown]" (a string) for empty lists, but [type] (a list) for non-empty. A snapshot captured when the field happens to be empty will mismatch the moment any element arrives. Use [] or a sentinel that has the same outer container shape.

5. JSON built via string concatenation in _req_payload / _resp_payload (seeding.py:85-94)
Works today because session_ids are constrained, but interpolating arbitrary input into a JSON string template is fragile. json.dumps is fast enough at seeding rates and removes the foot-gun.

6. _make_send_with_timing blindly appends server-timing (timing_middleware.py:167-171)
No check whether the header already exists — would produce duplicate headers if a future inner app or middleware also emits Server-Timing. RFC permits this, but it'll surprise someone. Either replace-or-merge, or assert single-source.

7. _TIMED_PREFIXES omits SSE paths.
/api/activity/stream (and other SSE endpoints) aren't included. Probably intentional given the long-lived nature, but worth a comment in _TIMED_PREFIXES explaining the exclusion.

8. Duplicate seeding pattern across test files.
test_api_contract.py:33-44, test_sse_memory.py:39-51, test_page_load.py:93-111, test_transcript_open.py:73-90, and test_throttled_network.py:40-52 each open ~/.luthien/perf.db via raw sqlite3.connect to check row counts. Extract into a single conftest.py helper to reduce drift.

9. _BATCH_SIZE = 5000 × ~25 KB payloads ≈ 125 MB in-memory per batch. Fine for tier-100/1000, but combined with the 128 MB SQLite cache and the in-memory events_batch accumulating during the outer loop, tier-10000 can spike well above what casual users expect. Either shrink the batch or document the peak RSS alongside the disk estimate.

10. _BASE_TS = datetime(2025, 1, 1) — current date is 2026-05-17, so seeded sessions appear ~17 months old. Any UI feature that filters by recency (e.g., "last 30 days") will show empty results against the perf DB. Consider basing it on datetime.now() - timedelta(days=N).

✅ Things to keep

• ensure_perf_isolation() is a solid safety gate; the comment trail through run_perf.sh → get_perf_db_url → ensure_perf_isolation is easy to audit.
• Pure-ASGI ServerTimingMiddleware (not BaseHTTPMiddleware) — the docstring at timing_middleware.py:100-117 is exactly the right kind of "why" comment.
• test_concurrent_isolation in unit_tests/perf/test_timing_middleware.py validates the ContextVar story under genuine concurrency, not just smoke-tests it.
• pytest_configure rejecting xdist (perf_tests/conftest.py:40-46) with a clear error pointing at run_perf.sh — nice failure mode.
• SQL parameter binding with explicit SECURITY INVARIANT comments in _fetch_session_list_pg/_sqlite makes future reviewers' job easier.

Test coverage

Good coverage of the new code: unit tests for db.py, seeding.py, timing_middleware.py, perf_report.py, plus integration tests for the middleware on the real app. The contract snapshots are a nice forcing function for shape drift, modulo issue #4 above.

🤖 Generated with Claude Code

[claude]

Review — perf-infra

Solid foundation overall: clean isolation gates, deterministic seeding, pure-ASGI middleware, and good unit coverage on the new perf/ module. A handful of issues worth addressing before merge.

Bugs / correctness

1. ensure_perf_isolation substring checks are too loose — src/luthien_proxy/perf/db.py:50,56

• "local.db" in url and "perf_test" not in url match arbitrary substrings. A Postgres URL with local.db in a hostname or username would falsely trip the dev-DB guard; a Postgres URL where perf_test appears in a hostname/username (but no search_path override) would falsely pass. Tighten to check the actual options=-csearch_path=perf_test substring (and only reject local.db for sqlite:// URLs).

2. time_phase(\"render\") is in the PR description but never recorded — verified via grep -rn 'time_phase(\"render\"' src/ (no matches). Only db and serialize are emitted. Either drop the mention from the PR body / timing_middleware.py docstring, or wire it up.

3. Route handlers switched from typed Pydantic returns to pre-built Response — src/luthien_proxy/debug/routes.py:67,99
This eliminates FastAPI's response-model validation. test_api_contract.py:14-17 self-documents that /api/debug/calls/{id}/events and /api/debug/calls/{id}/diff are not covered by snapshots, so a future drift in fetch_call_events / fetch_call_diff would ship broken JSON with no test failure. Add snapshot tests for these two endpoints (or assert the bytes round-trip through CallEventsResponse.model_validate_json).

4. _seed_sqlite lacks explicit transaction boundaries — src/luthien_proxy/perf/seeding.py:143-237
The intermixed DROP INDEX / inserts / CREATE INDEX rely on sqlite3's implicit-commit behavior. A mid-run crash leaves the perf DB index-less. Wrap the inserts in an explicit BEGIN/COMMIT and recreate indexes after. (Low blast radius since the perf DB is disposable, but cheap to fix.)

5. _BASE_TS captured at import time — seeding.py:26
"Deterministic" claim only holds within one process. Two seed runs produce different created_at values, which will break any snapshot test that compares timestamps. Use a fixed epoch like datetime(2024, 1, 1, tzinfo=timezone.utc).

Performance / production-path concerns

6. ServerTimingMiddleware interacts with BaseHTTPMiddleware — src/luthien_proxy/main.py:442-447, src/luthien_proxy/perf/timing_middleware.py:105-109
The middleware's own docstring calls out that BaseHTTPMiddleware breaks ContextVar propagation for streaming responses, but the stack still includes StaticCacheMiddleware (a BaseHTTPMiddleware) between the route and ServerTimingMiddleware. Any time_phase call from inside a StreamingResponse may silently miss spans. Either convert StaticCacheMiddleware to pure ASGI or document the constraint in timing_middleware.py with a concrete "don't use time_phase from streaming handlers" warning.

7. Phases recorded after http.response.start are dropped silently — timing_middleware.py:170
The header is finalized at response start, so any time_phase(...) block that runs during body streaming never appears in Server-Timing. A footgun for StreamingResponse handlers. Worth a docstring note.

8. time_phase(\"db\") over-scopes — history/service.py:398,566, debug/service.py:220,276,343
The block spans the entire connection lifetime including pure-Python list comprehensions, placeholder construction, and the session_ids_on_page = [str(row[\"session_id\"]) for row in rows] step. Either narrow the spans around the actual conn.fetch(...) calls or rename the phase to something like \"db_block\" so the metric matches its name.

Minor

• migrate_perf_db uses asyncio.run and will hard-fail from any async fixture/handler — consider exposing both sync and async variants (db.py:122).
• total_rows = n_calls_total + 2 * n_calls_total → just 3 * n_calls_total (seeding.py:243).
• Tier disk estimate at seeding.py:276 (tier * 25 * 25 // 1_000_000) under-reports — for tier=10_000 it reports ~6 GB but each session ≈ 24 calls × ~45 KB ≈ ~10 GB actual.
• _seed_sqlite hardcodes the list of indexes to drop/recreate — will silently diverge from migrations/sqlite/*.sql. Either query sqlite_master dynamically or add a unit test asserting parity.
• drop_perf_db and migrate_perf_db raise differently-worded NotImplementedErrors for Postgres — align them.

What's good

• ensure_perf_isolation design (fail-closed safety gate) is the right shape, just needs tighter matching.
• Pure-ASGI middleware with documented ContextVar caveats — much better than wrapping BaseHTTPMiddleware.
• Unit tests in tests/luthien_proxy/unit_tests/perf/ cover the new module thoroughly.
• The history/service.py refactor (258/-256) preserves SQL queries byte-for-byte; the diff is genuinely just time_phase wrapping. Nice non-invasive instrumentation.

[claude]

Code Review — PR #753

TL;DR: Well-engineered PR with strong isolation guards, deterministic seeding, and a thoughtful pure-ASGI timing middleware. The eye-catching history/service.py diff (258 add / 256 del) is essentially indentation-only around new with time_phase(...) blocks — zero semantic changes to the data layer, which is the right way to do this. Three substantive issues are flagged below.

---

Major

1. Middleware ordering contradicts the middleware's own documented hazard. src/luthien_proxy/main.py:442-447

app.add_middleware(StaticCacheMiddleware)   # BaseHTTPMiddleware
# … add_middleware stacks outside-in, so ServerTimingMiddleware ends up outermost:
app.add_middleware(ServerTimingMiddleware)

The docstring on ServerTimingMiddleware (timing_middleware.py:121-125) explicitly warns: "if any BaseHTTPMiddleware sits between this middleware and the route handler … ContextVar propagation may silently break for streaming responses." By making ServerTimingMiddleware the outer middleware, StaticCacheMiddleware (a BaseHTTPMiddleware) now sits between it and every route. For buffered JSON responses this happens to work (the body completes before the inner Tasks pop the ContextVar token), but any StreamingResponse on a path matched by the middleware (notably /ui/fragments/*) will silently lose its time_phase records.

Fix: swap the order so ServerTimingMiddleware is added first (innermost), OR convert StaticCacheMiddleware to a pure-ASGI middleware, OR drop /ui/fragments/ from _INSTRUMENTED_PATHS and document that fragments are not instrumented.

---

2. run_perf.sh --clean --backend postgres bypasses ensure_perf_isolation(). scripts/run_perf.sh:175-203

The shell-level guard is a substring check for "local.db" in DATABASE_URL. With --backend postgres that check is trivially true (postgres URLs never contain local.db), and the script then runs:

conn = psycopg2.connect(url)
cur.execute("DROP SCHEMA IF EXISTS perf_test CASCADE")

…on whatever DATABASE_URL the operator has set, without calling perf.db.ensure_perf_isolation(). The schema name is hard-coded so the blast radius is bounded ("only" a perf_test schema gets dropped), but the Python module already has a stronger gate (it requires options=-csearch_path=perf_test in the URL) and it would cost nothing to call it here.

Suggested fix:

from luthien_proxy.perf.db import ensure_perf_isolation, drop_perf_schema_postgres
ensure_perf_isolation(os.environ["DATABASE_URL"])
drop_perf_schema_postgres()

---

3. time_phase("db") vs time_phase("db_block") will silently underreport DB time.

• history/service.py:398, 566 use "db_block"
• history/service.py:757 and debug/service.py:220, 276, 343 use "db"
• scripts/perf_report.py:248 aggregates only for phase in ("db", "serialize"):

So every db_block row gets dropped from the perf report — exactly the listings (list_sessions, list_calls) whose DB time you most care about reporting. Pick one name (recommend "db" everywhere — a block of queries is still "db time") or extend the report aggregator to fold db_block into db.

---

Minor

4. ensure_perf_isolation postgres check is a literal substring match. perf/db.py:60-65 — looks for the exact string options=-csearch_path=perf_test. URL-encoded options (%20), reordered query params, or libpq-style connection strings will be falsely rejected. Fine today since get_perf_db_url() is the only producer, but worth a comment that any future hand-built URL must match this exact form.

5. _seed_sqlite rollback path leaves the DB index-less. perf/seeding.py:142-244 — the try block drops indexes before the bulk insert (good for speed) and recreates them after, but the except → ROLLBACK path doesn't recreate them. A failed seed leaves the perf DB usable for queries but slow and structurally different from a successful seed. Either recreate indexes in a finally, or delete the entire perf DB on seed failure.

6. time_phase() is a public API with no header-injection sanitization. perf/timing_middleware.py — format_phases interpolates name directly into a Server-Timing header. All current call sites use literal strings, but the module is exported for use by other modules per the docstring. A future caller passing user-controlled name would inject \r\n and forge headers. Either document that name MUST be a literal/hard-coded identifier, or add if "\r" in name or "\n" in name: raise ValueError(...) at the start of time_phase.

7. migrate_perf_db() uses asyncio.run() internally and is called from seed_sessions (sync). perf/seeding.py — fine from CLI scripts and sync pytest fixtures, but will raise RuntimeError: asyncio.run() cannot be called from a running event loop if any async fixture ever calls seed_sessions(...) directly. The docstring on migrate_perf_db notes this; please propagate the same warning to seed_sessions's docstring so callers don't have to chase it.

8. utils/migration_check.py — _apply_sqlite_migrations → apply_sqlite_migrations. The promotion to public is fine, but the multi-line "Public API: intentionally exported without a leading underscore so that perf/db.py and test infrastructure can call it directly" comment is the kind of explanatory comment CLAUDE.md discourages — the rename + a one-line docstring suffice.

---

Nit

9. Magic seed value. perf/seeding.py:319 — random.Random(0xABCDEF) would read better as a module-level _DETERMINISTIC_RNG_SEED = 0xABCDEF.

10. Duplicate isolation check. scripts/perf_explain.py:117 and :208 both call the same guard; the second is redundant.

11. Cost estimate magic formula. perf/seeding.py:277-285 — tier * 25 * 45 // 1_000_000 could use a one-line "events/session × KB/event" comment.

---

Test coverage

Test coverage is strong: unit tests for db.py, seeding.py, report.py, timing_middleware.py, plus an SSE regression test, an API-contract snapshot suite, and a Playwright harness with smoke + page-load + throttled-network + transcript-open + SSE-memory tiers. Snapshot files are checked in. Nice work.

One small gap: I didn't find a test that exercises the middleware ordering end-to-end on a streaming response. Given finding (1), adding a tiny /ui/fragments/__test__ route that returns StreamingResponse and asserts the Server-Timing header carries phase data would have caught the issue.

---

Style / conventions

• All public functions/classes have docstrings, mostly Google-style ✓
• f-strings used throughout ✓
• Type annotations on public APIs ✓
• The new perf/ module follows the existing package layout convention ✓
• A few comment blocks explain what rather than why (CLAUDE.md prefers WHY-only comments) — items gh actions to check correctness #4, Agent Tweaks #8 above are examples; not blocking.

Overall this is a careful, well-tested infra PR. Address findings #1, #2, and #3 before merge; the rest are polish.

[claude]

Review — feat(perf): performance test harness, Server-Timing, baseline evidence

Solid foundation. The perf-DB isolation gate, ContextVar-based timing, and snapshot-based contract tests are the right shapes. Most comments below are nits and one regression risk in the StaticCacheMiddleware refactor.

Bugs / regressions

1. StaticCacheMiddleware now appends Cache-Control instead of replacing it — src/luthien_proxy/main.py:444-461. The previous BaseHTTPMiddleware used response.headers[\"Cache-Control\"] = ... (overwrite via MutableHeaders); the new ASGI version does headers.append((b\"cache-control\", ...)) with no filter for an existing header. If a handler (or downstream middleware) ever sets Cache-Control, you'll emit two of them. The peer ServerTimingMiddleware correctly filters existing server-timing headers before appending (perf/timing_middleware.py:183) — please make StaticCacheMiddleware symmetric. Probably benign today since none of the current handlers/StaticFiles set Cache-Control, but it's an unguarded foot-gun added by a non-functional refactor.

2. Activity stream regression test has a timing dependence — tests/luthien_proxy/e2e_tests/sqlite/test_activity_stream_regression.py:89 waits a fixed await asyncio.sleep(0.3) for the SSE connection to establish before firing synthetic requests. On a slow CI runner this can race and drop early events. Prefer signaling readiness from collect_sse via an asyncio.Event after the response headers arrive, then awaiting it in send_synthetic_requests.

3. _seed_sqlite finally-block runs a redundant COMMIT — src/luthien_proxy/perf/seeding.py:237-242. After a rollback the connection is in autocommit mode (isolation_level=None), so conn.execute(\"COMMIT\") raises and is silently swallowed by except Exception: pass. CREATE INDEX IF NOT EXISTS in autocommit already persists; the trailing COMMIT is dead code that depends on its own failure being ignored. Drop it.

Code quality

4. time_phase(\"db\") blocks span more than the DB call — e.g. history/service.py:_fetch_session_list_pg wraps both the await conn.fetch(...) and the Python aggregation of user_ids_by_session under the same \"db\" phase. The \"db\" name will then include row-iteration / dict-building time. Either rename the phase or split the aggregation out so phase names match their meaning.

5. ensure_perf_isolation is a no-op for unrecognized URL schemes — src/luthien_proxy/perf/db.py:50-65. Only sqlite://, sqlite+aiosqlite://, postgresql://, postgres:// are checked; anything else silently passes. Internally URLs come from get_perf_db_url so it's bounded, but the function is exported and the docstring promises a safety gate. A final else: raise RuntimeError(\"Perf DB isolation: unrecognized URL scheme\") would close the loophole cheaply.

6. GB estimate in seed_sessions is rough — seeding.py:284: tier * 25 * 45 // 1_000_000 gives ~11 GB for tier=10_000, while actual disk is ~6–7 GB. Direction is fine for a warning, but the comment claims it's a calibrated formula. Either tighten the constants or soften the comment.

7. (nit) timed_json_response uses model.model_dump_json() with # type: ignore[attr-defined]. Since this is the contract the function exists to satisfy, a Protocol with model_dump_json(self) -> str or a pydantic.BaseModel parameter type would let pyright keep the check.

Performance

• ServerTimingMiddleware cost on /v1/messages: a single str.startswith on the ASGI scope before delegating — negligible. ✓
• The pre-built Response from timed_json_response correctly bypasses FastAPI's response_model revalidation (Pydantic→dict→json twice). Snapshot tests in perf_tests/test_api_contract.py cover the regression risk introduced by skipping that validation. ✓
• _phases_var.get(None) correctly avoids LookupError for time_phase calls outside a request context. ✓

Security

• time_phase rejects \\r/\\n in phase names — header injection guard. ✓
• Returning a pre-built Response does not bypass Depends(verify_admin_token); auth still runs in the route. ✓
• Perf-DB isolation gates writes against ~/.luthien/local.db; tests exercise the refusal path. ✓
• seeding.py uses parameterized executemany — no SQL injection risk even with deterministic synthetic IDs. ✓

Test coverage

Strong. Notable additions: ContextVar concurrent-isolation test, time_phase-outside-context safety test, isolation-refusal test for the seeder, and shape-snapshot contract tests for the six endpoints affected by the timed_json_response switch.

Gaps worth a follow-up (not blocking):

• No test that StaticCacheMiddleware + ServerTimingMiddleware compose correctly on a timed path that also gets Cache-Control (e.g. /api/history/sessions) — directly relevant to comment 1.
• No test that time_phase records the elapsed time when the wrapped block raises (the finally guarantees it, but a 3-line test would lock it in).

Smaller stuff

• apply_sqlite_migrations was promoted from _apply_sqlite_migrations for perf/db.py use — fine, but it's now reachable from any caller. Worth a one-line note in the docstring that it expects to be the sole writer (it manages _migrations state).
• The Note: Must be called from a synchronous context... warning on migrate_perf_db is good — keep that pattern if you add the Postgres backend.
• Server-Timing header uses dur=%.1f (100 µs resolution). Fine for current uses; if you ever need sub-ms timing for a hot path, bump precision.

Nice work overall — the .sisyphus/evidence/ artifacts make the baseline reproducible, and splitting this from the UI rewrites (per the PR description) keeps the diff reviewable despite its size.

[claude]

Code Review

Thanks for the thorough infrastructure — the isolation enforcement, evidence capture, and snapshot coverage are well thought through. Posting feedback grouped by severity.

Important

1. seed_sessions does not drop existing perf data — "deterministic" claim is conditional
src/luthien_proxy/perf/seeding.py:260-301. Calling seed_sessions(sqlite, tier=100) then seed_sessions(sqlite, tier=1000) leaves the tier-100 rows in place because each tier uses a distinct perf-seed-{tier}- prefix and there is no explicit truncate/drop. The docstring on line 268 claims "drop + re-seed produces identical data" but the function doesn't drop. Either call drop_perf_db(backend) at the top of seed_sessions, or document that callers MUST drop first and add an assertion that no perf-seed-{tier}- rows already exist.

2. _seed_sqlite swallows index-recreation failures silently
src/luthien_proxy/perf/seeding.py:228-242. After ROLLBACK, the finally block re-runs CREATE INDEX IF NOT EXISTS … with a bare except Exception: pass. If recreation legitimately fails (disk full, schema corruption), the perf DB ends up index-less and benchmarks silently report inflated numbers. Log the exception at minimum, ideally re-raise on the rollback path so the caller knows the DB is in a degraded state.

3. migrate_perf_db() uses asyncio.run() and cannot be called from async contexts
src/luthien_proxy/perf/db.py:118-133. The docstring (line 96-98) acknowledges this, but seed_sessions (line 294) calls it unconditionally, propagating the constraint. Anyone who wires perf seeding into an async test fixture or FastAPI startup hook will get a confusing RuntimeError: asyncio.run() cannot be called from a running event loop. Recommend exposing async def migrate_perf_db_async() and making the sync version a thin wrapper, so async callers have an opt-in path.

4. apply_sqlite_migrations is now public but has no concurrency guard
src/luthien_proxy/utils/migration_check.py:56,70-72. The docstring warns that concurrent calls are unsafe, but the function was just renamed from _apply_sqlite_migrations and is now reachable from perf/db.py against ~/.luthien/perf.db. If a developer runs scripts/perf_explain.py while a perf test is mid-seed, both processes race on _migrations. Either add a file lock (fcntl.flock on a sidecar) or a BEGIN IMMEDIATE guard in the runner. Failure mode is loud but recoverable; worth a note.

5. format_phases only validates \r/\n — Server-Timing grammar is stricter
src/luthien_proxy/perf/timing_middleware.py:76-77, 103. A phase name containing ,, ;, or = produces a malformed header (e.g. "db;dur=0,evil" → db;dur=0,evil;dur=12.3). Not exploitable today (all callers pass literals), but the validation is half-done. Tighten the check to the RFC 8941 token grammar ([A-Za-z0-9_-]+), or document that name is caller-trusted.

Worth Noting

6. history/service.py "rewrite" is indentation only — behavior preserved
The 258/256 line swap is misleading: the actual change is wrapping three async with db_pool.connection() as conn: bodies in with time_phase("db"): and re-indenting every nested line. SQL strings, parameter binding (including the user_id security filter on lines 411, 589, 622, 678), and post-query processing are byte-identical. Considered using a helper like await timed_fetch(conn, ...) to keep future diffs smaller, but acceptable as-is.

7. Middleware integration is correct
ServerTimingMiddleware is pure-ASGI (not BaseHTTPMiddleware), per-request ContextVar isolation is correct (_phases_var.set(phases) with reset(token) in finally), and SSE paths are excluded from _TIMED_PREFIXES. The docstring's caveat about phases-during-streaming being lost is accurate. Verified no StreamingResponse callers use time_phase on /api/history/* or /api/debug/*.

8. Perf isolation gate is solid
ensure_perf_isolation is called at every public entry point. SQLite branch hardcodes ~/.luthien/perf.db (never reads DATABASE_URL), so accidental prod targeting is structurally impossible. scripts/run_perf.sh:155-171 adds a redundant shell-level check. Postgres branch requires search_path=perf_test in the URL.

Nits

• src/luthien_proxy/perf/seeding.py:79-89, 329 — SeedingReport.tier is typed int | str because seed_sami_like uses "sami" while seed_sessions uses an int. Consider splitting into separate tier: int | None + fixture: str | None fields, or an enum.
• scripts/perf_explain.py:217-218 — exits 0 when Postgres is unavailable; should be non-zero so CI matrices catch the gap.
• src/luthien_proxy/perf/seeding.py:155 — PRAGMA synchronous=OFF on WAL is documented as "intentionally unsafe", but a kill -9 mid-seed leaves an unrecoverable DB. One-line README note would help.
• src/luthien_proxy/perf/seeding.py:53-54 — _REQ_PAD = "A" * 2368 is a magic constant; test_payload_sizes covers the result but a brief comment on the target byte budget would aid future tuning.

Test Coverage Gaps for New Production Code

• ServerTimingMiddleware: missing concurrent-request isolation test (two simultaneous requests must not share phase lists) — important because that's the central correctness claim of the design.
• _seed_sqlite: no test for the ROLLBACK + index-recreation path, which is exactly where issue Remove PolicyEngine and inline Redis setup #2 hides.
• seed_sessions: no test that re-seeding without drop_perf_db accumulates rows (would catch issue chore(logging): replace prints with structured logging #1).
• StaticCacheMiddleware was converted from BaseHTTPMiddleware to pure-ASGI in main.py but I don't see a direct test for the new form — header overwrite vs. append behavior should be exercised.
• ensure_perf_isolation is missing a test for unrecognized URL schemes (mysql://, etc.) — currently raises, but no regression coverage.

[claude]

PR Review — perf-infra

Solid foundation for a perf test tier. Isolation discipline (ensure_perf_isolation, refusing local.db, dedicated ~/.luthien/perf.db) is the right shape, and the ServerTimingMiddleware is well-engineered (pure-ASGI, ContextVar-isolated, concurrent-isolation test included). Below are the things I'd push back on or want to see addressed before merge.

High priority

1. ~21k lines of dev-process artifacts committed to git (.sisyphus/evidence/*.log, *-devchecks.txt, task-P*-*.txt). These are agent run logs and tier baselines — useful while the work was in flight, but they'll bloat the repo permanently and noise up future blame/grep. .sisyphus/ is not in .gitignore and has no prior history. Suggest one of:

   • Move ongoing run outputs to dev/scratch/ (already gitignored), or
   • Add .sisyphus/ to .gitignore and check in only the canonical perf-report-baseline*.md and baseline-query-plans.md, or
   • Keep all of it but gitignore future runs.
     The 7437-line after-run-sqlite.log / 6915-line baseline-run-sqlite.log especially feel like CI-artifact material, not source-controlled docs.

2. Bug-fix-ish change bundled with feature work. Renaming _apply_sqlite_migrations → apply_sqlite_migrations (utils/migration_check.py:56) plus matching test updates is a public-API shift driven by perf/db.py needing it. Per CLAUDE.md ("One PR = One Concern"), this would normally be a tiny precursor PR; not blocking here since it's mechanical, but worth a note.

3. Hot-path overhead in time_phase (src/luthien_proxy/perf/timing_middleware.py:76-79):

   import re as _re  # noqa: PLC0415
   if not _re.fullmatch(r"[A-Za-z0-9_-]+", name):
       raise ValueError(...)

   Every time_phase() call re-imports re (cached but still a dict lookup) and recompiles a regex. Phase names are static string literals at call sites — validate once at module import via a precompiled re.compile(...), or skip validation entirely and trust callers (it's an internal API). On a transcript page that wraps many DB hops, this adds up.

Medium priority

4. _seed_sqlite index re-creation in finally block is mostly dead code (seeding.py:237-246). On ROLLBACK, the in-transaction DROP INDEXes are also rolled back, so the indexes still exist; the finally then runs CREATE INDEX IF NOT EXISTS against the (already-indexed) schema. On COMMIT, the indexes are created inside the transaction, so the same IF-NOT-EXISTS runs as a no-op. Defensive, but consider whether the try/except: rollback / raise branch is buying you anything beyond what with sqlite3.connect(...) would.

5. ServerTimingMiddleware lives in luthien_proxy.perf but is mounted unconditionally in production (main.py:462). Conceptually it's an observability/telemetry middleware, not a perf-test utility — perf/timing_middleware.py makes main.py import from a module whose docstring says "Isolated from the main application — writes only to the perf database." Consider moving to observability/timing_middleware.py (or similar) and leaving perf/ strictly for test-only utilities. Pure naming/placement nit, not a correctness issue.

6. response_model= decorator + pre-serialized Response is a footgun. The new pattern (debug/routes.py, history/routes.py) keeps response_model=... on the decorator "for OpenAPI only" and bypasses validation by returning a pre-built Response. The doc comment is clear, but anyone copying this pattern will not realize FastAPI's validation has been disabled. The contract snapshot tests cover the current endpoints, but if a future contributor adds a route from this template, validation will be silently off. Worth a # WARNING: comment at each handler return, or a tiny helper that asserts the pre-serialized model matches the declared response_model in DEBUG mode.

7. measure_page_load uses wait_until="networkidle" on pages that open SSE streams (/conversation/live/{id}). SSE connections never go idle, so navigation will block until Playwright's default 30s timeout for those routes. May be why test_throttled_conversation_live has no explicit timeout protection. Recommend domcontentloaded + an explicit wait_for_selector for the first turn.

8. get_perf_db_url("postgres") appends options=-csearch_path=perf_test via a naive ?/& check (perf/db.py:33-34). If the caller's DATABASE_URL already contains options= (e.g. a custom statement_timeout), this produces two options= params and Postgres will silently use only the last one. Low likelihood, but worth either parsing the URL properly or asserting options= is absent.

9. Postgres perf migration / drop is NotImplementedError but get_perf_db_url("postgres") and ensure_perf_isolation for Postgres URLs are fully wired and tested. The "Postgres support" surface is half-complete; would be cleaner to either gate the URL helper behind a flag or leave a TODO(PR#XXX) pointer so the next person knows where to pick up.

Low priority / nits

10. time_phase docstring claims "Phases are recorded even when the block raises" but the test test_time_phase_outside_request_context_does_not_raise is the only related test — no explicit test asserting the phase IS recorded across an exception. Easy add given the docstring promise.

11. **tests/luthien_proxy/perf_tests/conftest.py:181 — db_pool = DatabasePool(perf_db_url)** is created at fixture setup but, on the start-failure path (line 222-228), is closed via asyncio.run(db_pool.close())`. If the gateway thread already opened the pool, that's fine; if not, you're closing an unopened pool. Probably harmless but worth a quick check.

12. _call_count seeding distribution uses different RNG seeds for bulk (tier) vs sami-like (_DETERMINISTIC_RNG_SEED = 0xABCDEF). Documented elsewhere, but reads inconsistent — consider always using a named constant per fixture name for clarity.

13. changelog.d/perf-baseline.md is categorized as "Chores & Docs" but this PR adds test infrastructure, middleware, and a CLI script — feels closer to "Internal" or a new "Performance" category. Minor.

14. scripts/perf_explain.py:86-87 seeds the perf DB with tier=100 if empty as a side-effect of the EXPLAIN script. Surprising behavior for an "EXPLAIN-only" helper; consider making seeding explicit (--seed-if-empty flag) so the script does one thing.

Things I liked

• The isolation gates (ensure_perf_isolation, the bash-level local.db check in run_perf.sh, the env var refusal) are belt-and-braces, exactly right for a destructive perf rig.
• test_concurrent_isolation in test_timing_middleware.py actually exercises the ContextVar invariant under concurrent requests — that's the test most people skip.
• The pytest_configure xdist guard with a useful error message is a nice touch.
• Pre-existing test files were systematically updated to match the new Response return type — no half-migrated state.
• dev/context/migration_concurrent.md is a good record of the CONCURRENTLY investigation; future work on Postgres seeding will benefit.

Summary

Nothing here is a hard blocker on functional correctness, but the committed evidence files (#1) and the hot-path regex (#3) are worth addressing before merge. The remaining items are polish or "consider this for follow-up."

[claude]

Review: feat(perf) — performance test harness, Server-Timing middleware, and baseline evidence

Solid, well-documented infrastructure PR. The ContextVar-based middleware is a thoughtful design (the BaseHTTPMiddleware pitfalls are explicitly avoided and called out in the docstring), the isolation guards around the perf DB are layered and convincing, and test coverage is comprehensive — unit/integration/e2e regression/snapshot contract tests all touched. Below are concrete observations, ordered by impact.

Bugs / correctness

1. seed_sami_like skips the duplicate-seed guard. seed_sessions calls _assert_no_existing_rows(...) before inserting (seeding.py:313), but seed_sami_like does not (seeding.py:319-345). Re-running seed_sami_like without drop_perf_db first will hit a sqlite3.IntegrityError on the deterministic PKs — a surprising failure mode given the symmetric API. Either call _assert_no_existing_rows(_sqlite_path(url), prefix) in seed_sami_like for a clean RuntimeError, or have the fixtures consistently drop_perf_db first.

2. assert isinstance(conn, SqliteConnection) in migration_check.py:147 is silently removed under python -O. Since apply_sqlite_migrations is now a documented public entry point (called by perf/db.py and tests), promote it to a real if not isinstance(...): raise TypeError(...) so the safety check isn't optimized away in a production build.

3. Postgres URL option-parameter collision (perf/db.py:34-37, already TODO'd): when DATABASE_URL already has ?options=..., appending another options=-csearch_path=perf_test produces a duplicate; only the last wins. Worth at least raising a clear error today rather than silently relying on the Postgres "last value wins" rule.

4. run_perf.sh isolation match is a substring check (scripts/run_perf.sh:164): [[ "\$_db_url" == *"local.db"* ]] would also trip on a perf DB named e.g. mylocal.db.perf. Consider matching the basename precisely.

Performance considerations

5. Postgres _fetch_session_list_pg outer GROUP BY ... f.request_payload (history/service.py:487-489) groups by a JSONB blob. Because session_first_message already returns one row per session via DISTINCT ON, this is functionally equivalent to grouping by s.session_id, but JSONB equality in GROUP BY is expensive. Consider GROUP BY s.session_id and using MIN(f.request_payload).

6. SQLite preview/model bulk queries fetch every matching event for the page (history/service.py:646-675). With sami-like sessions of 442 calls, a 50-session page can fetch tens of thousands of rows just to pick the first preview per session in Python. SQLite supports ROW_NUMBER() / MIN() aggregates — fine to defer, but worth a TODO since this is exactly the hotspot the baseline is trying to characterize.

7. test_page_load.py parametrizes 4 fixtures × ~11 routes = ~44 cases, each running 5 page loads + final measurement + HTTP fetch + Playwright trace capture. Cumulative wall time will be large; consider defaulting the discovery loop to tier-100/sami-like and gating the heavier fixtures behind an env flag.

Security

• time_phase validates names against [A-Za-z0-9_-]+ — defends against header-injection through phase names. Good.
• build_tempo_url correctly URL-encodes the TraceQL query.
• Export-route filename sanitization (history/routes.py:142) handles path traversal correctly.
• The user_id filter SQL comments explicitly call out the parameter-binding invariant, and the symmetric "leak existence of other users via shared session_id" reasoning is captured in code. Nice work — clearly thought through.

Test coverage

• test_concurrent_isolation is exactly the right test for the ContextVar approach — explicitly verifies phases don't leak across concurrent requests sharing the event loop.
• API contract snapshot tests cover the response-shape regression risk of the model_dump_json() + bare Response pattern. The big WARNING comment on both routes files is appreciated.
• One gap: time_phase accepts an empty phase list (no blocks ran). The middleware correctly skips header injection (timing_middleware.py:186), but there's no integration test covering "filtered path with zero phases recorded". Worth a one-liner.

Style / minor

• _extract_shape (test_api_contract.py:44-77) reads heterogeneous lists by inspecting only index 0. Already noted in the docstring; fine for shape testing but the events array (a discriminated union) is exactly the case where shape drift might be missed.
• perf_explain.py silently seeds the DB if empty (scripts/perf_explain.py:142-152). The comment acknowledges the side effect; I'd still prefer an explicit --seed-if-empty flag.
• seeding.py:_call_count's magic prime offset (rng_seed * 1_000_003 + session_idx) is correct for decorrelation but cryptic; a one-line comment would help the next reader.

Verdict

Ship-worthy infrastructure. The two items I'd address before merge are (1) the seed_sami_like guard inconsistency and (2) replacing the assert isinstance with a real type check now that apply_sqlite_migrations is a public API. Everything else is incremental or follow-up territory.

Co-Authored-By: Claude Opus 4.7 noreply@anthropic.com

[claude]

Review — PR #753 (perf-infra)

Comprehensive infrastructure PR — perf test harness, Server-Timing middleware, deterministic seeding, EXPLAIN capture, and contract snapshots. Overall the design choices are sound and the safety story is much better than I expected for "perf tooling." Comments below.

Strengths

• Isolation safety is well-designed. ensure_perf_isolation() (perf/db.py:46) rejects local.db and Postgres URLs without search_path=perf_test. Both paths (Python + run_perf.sh) gate on it. The options= duplicate-detection in get_perf_db_url() (perf/db.py:35) is a thoughtful catch — last-value-wins semantics would have silently broken isolation.
• ServerTimingMiddleware is correctly pure-ASGI. The docstring (timing_middleware.py:109-136) explains exactly why BaseHTTPMiddleware was avoided (ContextVar propagation through Starlette's pipe-buffering wrapper). The _phases_var ContextVar is set per-request and reset in finally. Concurrent isolation is verified by test_concurrent_isolation (test_timing_middleware.py:90).
• Header injection at http.response.start is the right hook — it doesn't touch streaming bodies. Existing server-timing headers are stripped before append (timing_middleware.py:187), so a downstream re-wrap won't duplicate the header.
• time_phase() validates the phase name against RFC 8861 token grammar ([A-Za-z0-9_-]+) — prevents malformed headers if a phase name leaks an attacker-controlled string.
• Deterministic seeding. _DETERMINISTIC_RNG_SEED + _assert_no_existing_rows prevents silent accumulation across re-runs; test_seed_idempotent confirms it. The bulk-insert pattern (drop indexes → executemany → recreate) plus WAL+synchronous=OFF is a sane perf-DB shape.
• xdist refusal in pytest_configure (perf_tests/conftest.py:40) is a good guardrail given the session-scoped os.environ mutation in perf_gateway_url.
• timed_json_response + contract snapshots. The route-handler comment in history/routes.py:9-16 explicitly warns about bypassing FastAPI response_model validation, and test_api_contract.py provides regression coverage for the shape — exactly the right safety net for that pattern.

Issues & Suggestions

1. Doc/test name drift — history/service.py:391
The comment references test_fetch_session_list_user_filter_sql_injection, but the actual test is test_user_id_filter_sql_injection_safe (tests/luthien_proxy/unit_tests/history/test_service_sqlite.py:914). Update the comment so the cross-reference doesn't rot further. The invariant itself looks correctly preserved (user_id is bound as $N, never interpolated).

2. Inconsistent disk-size warning for tier-10000
run_perf.sh:215 says ~7–8 GB on disk; seeding.py:298 computes max(1, tier * 25 * 45 // 1_000_000) → 11 GB. Pick one estimate (the Python one is closer to reality given the call-count distribution: ~27 avg calls × 2 events × ~25 KB ≈ 13 GB for tier=10000). Worth aligning so operators don't underestimate disk needs.

3. synchronous=OFF and seeding interruption
The PRAGMA is intentional ("perf DB is disposable") and acceptable for tier-100/1000, but at tier-10000 a power-loss / SIGKILL mid-seed can leave the file corrupt. Consider either:

• A try/except that calls drop_perf_db("sqlite") on any seed failure (so re-run starts fresh rather than failing in _assert_no_existing_rows against a partially-corrupt DB), or
• A note in the tier-10000 warning that crashes can require a manual --clean.

Not blocking, but the next person to ctrl-C a 10k seed will appreciate it.

4. fetch_recent_calls returns total=len(calls) (debug/service.py:371)
This reports "rows in this page," not "rows in the table" — semantically wrong if any consumer treats it as a true total for pagination. Pre-existing, not introduced here, but the perf work is the right moment to fix it (or rename to count/page_size). Same for the CallListResponse model.

5. _safe_parse_json discards valid non-dict JSON
history/service.py:151 returns None for valid JSON arrays/scalars (result if isinstance(result, dict) else None). That's fine for _extract_preview_message where the payload must be a dict, but the helper is also used at service.py:127 for OpenAI tool-call arguments strings. If a model ever emits a JSON-list arguments (rare but legal), the input gets silently lost. Worth either narrowing the helper's name (_safe_parse_json_dict) or letting both paths through.

6. Hardcoded line-number callouts in perf_report.py
_section_top_hotspots (scripts/perf_report.py:283-291) embeds line numbers like conversation_live.js:164-172 and history_list.html:514. These will rot. Since this is a baseline doc that gets re-rendered, consider:

• regenerating these from a stable anchor (function name / data-test attribute), or
• demoting to "see commit <sha> for current line numbers" so the rot is visible.

7. _discover_html_routes creates a full FastAPI app at collection time
test_page_load.py:43 instantiates create_app(...) inside pytest_generate_tests purely to enumerate routes. Works, but pulls in the whole import graph (litellm, redis, etc.) for parametrization. If startup time becomes a concern, a lightweight route inventory (constant list, or app.routes from a cached app instance) would be cheaper. Low priority.

8. Server-Timing duplicate phase names
format_phases (timing_middleware.py:91) emits db;dur=X, db;dur=Y when time_phase("db") is called twice in one request. RFC 8941 allows duplicates and browsers render them, so it's not wrong — just worth flagging if you ever want to aggregate dashboard graphs by phase name. Could either dedupe-and-sum at format time or pick distinct names per call site.

Test Coverage

Coverage looks solid:

• Middleware: format, path filter (include/exclude), concurrent isolation, exception-safe recording, header replace-not-append.
• Perf DB: isolation rejection for local.db / non-perf Postgres / unknown schemes; idempotent drop; migration table creation.
• Seeding: row counts, idempotency, prefix discipline, sami-like 442-msg invariant, payload size invariant, index parity after seed, rollback path recreates indexes, refusal on dev DB.
• Report: required sections, deterministic output, NO-DATA-YET fallback.

One gap worth considering: there's no test that ServerTimingMiddleware plays nicely with StaticCacheMiddleware when both are active and a phase is recorded — the cross-middleware regression test (test_static_cache_middleware_replaces_not_appends) doesn't actually exercise time_phase. Given the existing docstring warning about BaseHTTPMiddleware breaking ContextVar propagation, a test that records a phase from inside a /api/history/* handler with StaticCacheMiddleware in the stack would lock in the "this is safe because both are pure ASGI" claim.

Nits

• _seed_sqlite uses list[tuple] for batches — list[tuple[str, ...]] would type-check more strictly.
• _REQ_PAD = "A" * 2368 magic numbers are explained inline; consider naming the target size (e.g. _REQ_TARGET_BYTES = 5 * 1024).
• EVIDENCE_DIR.mkdir(parents=True, exist_ok=True) appears 4+ times across perf tests — easy candidate to push into a shared helper in conftest.py.

Nothing here is blocking. Approving the design; the items above are mostly cleanup or follow-ups.

🤖 Generated with Claude Code

[scottwofford]

Claude-generated merge-queue triage of all open Luthien PRs, requested by Scott (Jul 7, 2026). Advisory only; Scott has not yet acted on these recommendations.

Recommendation: close.

The slowness this harness was built to measure is being fixed directly (see #795 and the related history work), and the code it instruments has been heavily rewritten since May, which is why the branch now conflicts and sits roughly 600 commits behind. Landing it would mean re-validating 5,400 lines of measurement infrastructure against a codebase that moved past the captured baseline. A performance-test tier can be reintroduced later; the isolation and middleware design here got consistently positive review and remain a good reference.