Skip to content

fix(history): stop shipping cumulative request history in session detail; page-scope list payload lookups - #803

Open
scottwofford wants to merge 1 commit into
mainfrom
fix/activity-monitor-slowness
Open

fix(history): stop shipping cumulative request history in session detail; page-scope list payload lookups#803
scottwofford wants to merge 1 commit into
mainfrom
fix/activity-monitor-slowness

Conversation

@scottwofford

Copy link
Copy Markdown
Member

Summary

Fixes the admin-dashboard slowness Sami reported (May 12-15, #trajectory-luthien): "Loading the history takes many, many seconds. Trying to load a transcript, I think, can take like a minute." Addresses Trello card Fix UI slowness on the activity monitor (Trajectory unblocker).

Per Paolo's May 15 diagnosis, the root problem is admin-client response payload bloat under realistic agent-session volumes. This PR is the minimal, semantics-preserving unblock for both slow paths:

1. Session detail (GET /api/history/sessions/{id}) — the "transcript takes a minute" path.
Agent clients (Claude Code) re-send the full conversation history on every request, so each stored transaction.request_recorded payload is cumulative. The endpoint returned every turn's full parsed history, growing the response O(turns²). The dedup the frontend already performed client-side (presentTurns slicing by the previous turn's message count) now runs server-side (_dedup_cumulative_request_messages): unmodified turns carry only their new messages; policy-modified turns keep full arrays (so original-vs-final diff panels still line up index-by-index) with a new request_delta_start field marking where the new messages begin. Rendered output is unchanged. Markdown/JSONL exports apply the same boundary, so they stop repeating prior turns' history too.

Measured on a 200-turn synthetic session (~1KB tool results): detail JSON 27.2 MB → 0.56 MB (49x); markdown export 23.3 MB → 0.44 MB. The ratio grows linearly with session length — at the 1,771-turn scale reported in #795 this is the difference between ~69 MB and well under 1 MB per load.

Bonus fix: a turn whose request shrinks (context compaction) previously rendered as empty in the viewer (client slice past the end); it now displays its full compacted request and resets the dedup baseline.

2. Session list (GET /api/history/sessions) — the "history takes many seconds" path.
The Postgres list query probed ce.payload for every transaction.request_recorded row in the table (the final_model extraction and the max_tokens probe-gate in the session_models / session_first_message CTEs), detoasting every stored cumulative payload on every page load. The SQLite path shipped every request payload for the page's sessions across into Python. Both backends now use the same 5-query shape: a metadata-only page aggregate, then models / previews / user_ids lookups keyed on the page's session_ids. The PG preview lookup uses LATERAL ... LIMIT 1 (walks each page session's events in created_at order, stops at the first non-probe payload); the SQLite preview keeps one row per session inside SQLite via ROW_NUMBER. Response fields and filter semantics are unchanged, including the user-scoped preview/models isolation.

API note

ConversationTurn gains request_delta_start: int = 0. request_messages now contains only the turn's new messages for unmodified turns (full arrays, with the boundary marked, for modified turns). The bundled viewer (conversation_live.js) is updated in this PR; external consumers of the detail endpoint that relied on cumulative per-turn arrays will see deltas instead. JSONL/markdown exports likewise no longer repeat prior turns per turn.

Relationship to open PRs

Root cause (RCA/COE)

  1. Root cause: Cumulative request payloads (an inherent property of stateless LLM APIs driven by agent scaffolds) were treated as independent per-turn data by the history read paths. Every surface that touched them — detail JSON, exports, list preview/model extraction — silently inherited O(turns²) growth in response size or DB payload reads. The frontend even knew about the redundancy (it deduplicated client-side for display) but the payload had already crossed DB → server → network by then.
  2. Why it wasn't caught: All history tests (and dev usage) run on toy sessions of 1-3 short turns, where quadratic growth is invisible. There was no test asserting any size/shape bound on the detail response, and no perf regression harness in CI (feat(perf): performance test harness, Server-Timing middleware, and baseline evidence #753 adds one; it was opened 2026-05-17 and is still unreviewed). The list query's table-wide payload probing was also invisible on SQLite unit fixtures because tiny in-memory payloads make full scans free.
  3. Why it won't recur: test_payload_size_grows_linearly_not_quadratically locks the O(total messages) contract for the detail response into the default unit pass, and the new dedup tests pin the modified-turn / preflight / compaction edge cases. The dedup rule now lives in one server-side place with the invariant documented, instead of implicitly in a frontend comment. For the list path, both backends now share the page-scoped lookup shape, and the query comments state the "payload reads must be proportional to the page" invariant explicitly. Landing feat(perf): performance test harness, Server-Timing middleware, and baseline evidence #753's harness would add end-to-end regression detection at realistic volumes; recommended as the structural follow-up.

Verification

  • ./scripts/dev_checks.sh green (ruff, pyright, full unit suite: 2848 passed).
  • ./scripts/run_e2e.sh sqlite (47 passed) and mock tier green.
  • Real Postgres 16 (throwaway Docker container, migrations 001-021 applied): unfiltered list, user-filtered, model-filtered, pagination, and detail dedup all verified against seeded cumulative sessions (probe-first session included).
  • Live browser check (dockerless gateway + Playwright): transcript renders 3/3 turns with each message displayed exactly once (matching pre-change rendering), history list previews intact, no new console errors (the nav.js $cleanup error is pre-existing on main).

Remaining known costs (out of scope, tracked by open PRs)

🤖 Generated with Claude Code

…ail; page-scope list payload lookups

Session detail responses repeated the full conversation prefix inside every
turn (agent clients re-send history each request), growing O(turns^2) with
session length. The dedup the activity-monitor frontend already performed
client-side (slice by previous turn's count) now runs server-side; policy-
modified turns keep full arrays for diff alignment with request_delta_start
marking the boundary. Exports apply the same boundary.

Session list queries probed request payloads table-wide (PG: final_model +
max_tokens gates detoast every stored payload; SQLite: every request payload
for page sessions crossed into Python). Both backends now fetch models,
previews, and user_ids via page-scoped lookups; the PG preview uses a
LATERAL LIMIT 1 so only the first qualifying payload per page session is
read; the SQLite preview keeps one row per session inside SQLite via
ROW_NUMBER.

Measured: 200-turn synthetic session detail JSON 27.2MB -> 0.56MB (49x);
markdown export 23.3MB -> 0.44MB. Verified against real Postgres 16
(migrations 001-021, list/user/model filters, pagination, dedup).

Addresses https://trello.com/c/vhlQyEO0 (Trajectory unblocker).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@claude

claude Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Review

Solid, focused fix. The problem statement (cumulative-history-as-per-turn payload), the semantics-preserving contract (request_delta_start for modified turns, delta-in-place for unmodified), and the measured 49× reduction all line up. Test coverage on the dedup rules (unmodified / modified / preflight / title / shrink) and the O(n) invariant lock-in are the right shape for keeping this from silently regressing.

A few things worth addressing before merge, ordered by impact.

1. JSONL export leaks the fix for modified turns (bug)

In export_session_jsonl at service.py:1313 / service.py:1321:

"request_messages": [m.model_dump(mode="json") for m in turn.request_messages[turn.request_delta_start :]],
...
if turn.original_request_messages is not None:
    record["original_request_messages"] = [m.model_dump(mode="json") for m in turn.original_request_messages]

For modified turns, request_messages is sliced by request_delta_start (delta only) but original_request_messages is emitted whole (full cumulative). This is asymmetric and reintroduces O(turns²) growth in the original_request_messages field for any session with many modified turns. It's also confusing to a downstream consumer (final = new messages, original = all messages).

Suggested fix (assuming both arrays have the same delta boundary, which is the design invariant):

record["original_request_messages"] = [
    m.model_dump(mode="json") for m in turn.original_request_messages[turn.request_delta_start :]
]

Add a JSONL test analogous to test_jsonl_export_respects_delta_start but with original_request_messages present.

2. _dedup_cumulative_request_messages skips original_request_messages (footprint)

Same shape as (1) but on the wire, not in exports. _dedup_cumulative_request_messages mutates turn.request_messages for unmodified turns and updates request_delta_start for modified turns, but never touches turn.original_request_messages. So for modified turns the detail response still carries the full cumulative original_request_messages array — the frontend diff view (renderDiffPanels at conversation_live.js:742) walks it with Math.max(originalMsgs.length, finalMsgs.length), so consumer expectations are for full arrays there today.

Two options:

  • Keep full arrays and document it — modification is expected to be rare and the O(turns²) impact is bounded by count-of-modified-turns × session length. Fine, but the invariant is worth stating explicitly in the model / service docstring.
  • Strip both consistently — server strips both request_messages and original_request_messages by prev_count for modified turns, JS renderDiffPanels becomes slice(deltaStart)-on-both. Cleaner semantically and matches the "delta everywhere" story.

Either is defensible; the PR should pick one and land it. Right now the code does neither cleanly.

3. presentTurns lost its invariant warning (observability)

The removed code included:

if (displayMessages.length === 0 && messages.length > 0) {
    console.warn('Dedup produced empty messages for turn', turn.call_id,
        '— cumulative array invariant may be violated');
}

That was a useful client-side signal when the server's cumulative-count assumption breaks. The server now trusts the same invariant (see the "shrinking history resets baseline" branch), and if a modified turn ever ends up with delta_start > messages.length, the display is silently empty. Cheap to restore:

if (deltaStart > 0 && messages.length <= deltaStart) {
    console.warn('Empty display slice for turn', turn.call_id, '— delta_start may be wrong');
}

4. PG LATERAL preview query and index coverage (perf; worth confirming, not blocking)

The new preview lookup is:

JOIN LATERAL (
    SELECT ce.payload FROM conversation_events ce
    WHERE ce.session_id = s.sid
      AND ce.event_type = 'transaction.request_recorded'
      AND COALESCE((ce.payload->'final_request'->>'max_tokens')::int, 2) > 1
    ORDER BY ce.created_at ASC
    LIMIT 1
) fm ON true

The existing indexes on conversation_events are idx_conversation_events_session (session_id, WHERE session_id IS NOT NULL), idx_conversation_events_call_created (call_id, created_at), and idx_conversation_events_type (event_type). There is no (session_id, created_at) composite. On sessions with many events, PG will still have to sort inside the LATERAL. Given the payload-detoast fix is the dominant win, this is probably fine — but if EXPLAIN ANALYZE on realistic data shows the sort dominating on hot sessions, a partial index (session_id, created_at) WHERE event_type = 'transaction.request_recorded' would nail it.

5. Duplicated preflight classification (maintenance)

_is_preflight_turn (service.py:1164) and classifyPreflight (conversation_live.js:296) implement the same rules independently. Both use 256 as the title-gen max_tokens ceiling with json_schema output. Small divergence risk if only one is edited later (e.g. Python's isinstance(max_tokens, int) vs JS's params.max_tokens != null differ subtly for max_tokens: 0 or non-int types). At minimum, cross-reference each side in a comment; ideally, ship the classification in the API response (e.g. is_preflight: bool on the turn) so the frontend doesn't reimplement it.

6. Preview-lookup consistency isn't transactional (minor)

The metadata query and the three follow-up queries (models, previews, user_ids) run sequentially on the same connection but not inside an explicit transaction. Between them, new events could land — the follow-ups may see rows the aggregate didn't. Harmless for the list UI (fields still line up via session_id), but if consistency matters for downstream, an explicit BEGIN ... COMMIT (or a repeatable-read snapshot) would eliminate the window.


Positives worth calling out

  • The docstring on _dedup_cumulative_request_messages states the exact rules (preflight / unmodified / modified / shrink) and the invariant being trusted. Future readers won't have to reverse-engineer it.
  • test_payload_size_grows_linearly_not_quadratically locks the O(n) shape into the default unit pass — the right regression to have.
  • test_shrinking_history_resets_baseline catches the context-compaction case that previously rendered as empty. Nice edge-case find.
  • The SQL security comment in _fetch_session_list_pg about placeholder allocation makes the parameterization contract explicit rather than implicit.
  • Not attempting to also solve the O(turns²) read cost (still parses every stored cumulative payload) — deliberately scoped, deferred to feat(history): bound memory + O(turns) detail with turn pagination #795, called out in the PR body. Good discipline.

Suggested actions

  1. Fix (1) — asymmetric slicing in JSONL export. Small, clear bug.
  2. Pick a direction on (2) and either apply it or document the tradeoff on original_request_messages.
  3. Restore or replace the invariant console.warn in (3).
  4. Consider (4) after verifying with real PG data; not blocking.
  5. Cross-reference (5) between the two implementations at minimum.

Nothing above blocks merge on functional grounds — the core fix is correct and the tests pin the important invariants. (1) is the only thing I'd want in-PR before landing.

@scottwofford

Copy link
Copy Markdown
Member Author

Post-review note (adversarial verification pass, Jul 7): the headline 49x / 27.2MB→0.56MB figure was a one-off local measurement, not a committed reproducible benchmark; the committed regression test (test_payload_size_grows_linearly_not_quadratically) guards message-count linearity at 10 turns, not bytes. The dedup mechanism itself is real and verified; treat the specific multiplier as indicative. Also note CI runs unit/lint only (no e2e job); the e2e and Postgres results in the PR body were run locally.

@scottwofford

Copy link
Copy Markdown
Member Author

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: hold; close once #795 is merged and verified.

This revises the Jul 7 run-report suggestion to merge this PR first. Reasoning:

Not closing yet on purpose: if #795 review stalls for multiple weeks, this PR is the fallback interim unblock (fix the export asymmetry first, then merge, and ask for a rebase of #795).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant