Skip to content

perf(chat): bound paginated transcript reads - #8235

Open
Pearcekieser wants to merge 1 commit into
kirodotdev:mainfrom
Pearcekieser:perf/long-thread-loading
Open

perf(chat): bound paginated transcript reads#8235
Pearcekieser wants to merge 1 commit into
kirodotdev:mainfrom
Pearcekieser:perf/long-thread-loading

Conversation

@Pearcekieser

@Pearcekieser Pearcekieser commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Problem / Motivation

Chat history is paginated at the API boundary but not at the storage boundary. Each bounded slot-detail request currently parses the complete chained JSONL history before selecting the requested rows.

This makes gateway work scale with total conversation length and repeats complete-history parsing as users load older pages.

Why it matters

Long agent conversations commonly contain thousands of messages and large tool results. Repeated complete-history parsing increases page latency, gateway CPU, and peak memory despite the browser requesting only a bounded window.

For a 10,000-row transcript, the existing full-read baseline was approximately 97.7 ms and 10.7 MiB. The bounded restore-warmed reader completes in approximately 3.77 ms and 70 KiB.

What changed (motivation → approach → change)

This change keeps JSONL as the authoritative transcript while adding a gateway-owned sparse read projection:

  • Adds offset-preserving bounded JSONL framing.
  • Maintains trusted in-memory row checkpoints keyed by full file stamp and history generation.
  • Builds checkpoints during authoritative off-loop full reads instead of scanning restored transcripts twice.
  • Extends checkpoints across verified same-inode appends and rebuilds after rewrites.
  • Adds ConversationLog.read_messages_chained_page with existing total, before, next_before, and has_more semantics.
  • Pins all durable ranges composing one HTTP response to an exact ordered chain revision.
  • Retries complete page composition after concurrent mutation and falls back to the legacy full-reader oracle after repeated churn.
  • Reconciles bounded durable ranges with the existing event-loop-captured resident and unflushed tail.
  • Switches only explicitly paginated slot-detail requests to the bounded path.
  • Leaves unlimited reads and callers requiring complete history unchanged.
  • Keeps redaction at the existing response-preparation boundary.
  • Documents the index, invalidation, revision, fallback, and trust contracts.

The sparse index is deliberately not persisted. It contains only row counts and byte offsets and never trusts agent-writable derived state.

This change is independent of #7916: that PR handles archive reachability and browser scroll stability, while this PR bounds gateway storage work for the current chained transcript. If archive pagination lands later, its archive segments should be incorporated into this bounded projection rather than returning to complete-corpus reads.

Tests

Added and updated coverage for:

  • exact page equivalence across transcript chains;
  • boundary, before, next_before, has_more, and total semantics;
  • universal newline and byte-offset framing;
  • bounded warm reads over a 10,000-row transcript;
  • restore-time index warming without a second scan;
  • safe same-inode append extension;
  • rebuilds after rewrites and invalidation;
  • concurrent revision changes between response ranges;
  • complete-response retry and legacy fallback;
  • live and unflushed tail ordering;
  • loop-captured tail snapshots;
  • shared cached-list identity;
  • fork, rewind, cache, and locking compatibility.

Validation:

  • 838 affected tests passed
  • Targeted mypy passed
  • Targeted Flake8 passed
  • Black and isort checks passed
  • Documentation lint passed
  • Subprocess-encoding, agent-SDK boundary, sync-I/O, lockdown, brand, harness, lock, testpath, changelog, focus, vendor-manifest, scrub-lint, and CloudFormation gates passed
  • Frontend scoped tests, TypeScript, ESLint, i18n, phantom-class, duplication, production build, and bundle-size gates passed
  • Electron gate: 1,558 passed with zero assertion failures; one unrelated gateway-stop timing test remained pending on this host and Node cancelled it plus 25 descendants
  • Full backend run: 83,756 passed; remaining failures were unrelated host-isolation, inherited command-environment, ownership-topology, and AF_UNIX path limitations. No changed history, JSONL, pagination, fork, or dashboard-chat test failed.

Manual verification

Reader-level profiling:

Rows Full read + index Process-cold page Warm page
100 2.428 ms / 118 KiB 5.308 ms / 75 KiB 2.765 ms / 69 KiB
1,000 14.629 ms / 1.05 MiB 23.155 ms / 76 KiB 3.014 ms / 70 KiB
10,000 141.661 ms / 10.47 MiB 201.805 ms / 85 KiB 3.766 ms / 70 KiB

A first-ever process-cold page still performs O(total rows) index construction. This PR does not claim constant-time process-cold opening.

Related Issues

Fixes #8234.

Checklist

  • One commit with a Conventional Commits title
  • Existing affected tests pass and new tests cover the new behavior
  • Self-review completed; code follows project style guidelines
  • History-system documentation updated
  • No secrets, credentials, or internal references in the diff

Contribution License Agreement

The exact CLA wording will be supplied by OSPO before the first public PR.

@Pearcekieser
Pearcekieser requested a review from a team as a code owner September 3, 2026 18:22
@Pearcekieser
Pearcekieser requested a review from Zedmor September 3, 2026 18:22
@github-actions github-actions Bot added fork Pull request from a fork (external contributor) readiness: checking Automated validation is still running labels Sep 3, 2026
@Pearcekieser
Pearcekieser force-pushed the perf/long-thread-loading branch 3 times, most recently from b5f14c1 to 155c6e3 Compare September 4, 2026 00:07
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 4, 2026
@Pearcekieser
Pearcekieser force-pushed the perf/long-thread-loading branch from 155c6e3 to c9b40b5 Compare September 4, 2026 01:13
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 4, 2026
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5, fork) — 🟡 CONCERNS

Design-level review of d61f943aed3d483a6eb9b59cbab4b422cc2e0f65 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: CONCERNS

Sound, reversible in-memory projection with a correct fallback oracle, but the largest transcripts bypass it and every bounded-path failure degrades silently.

Watch

  • requires_full_reader = bool(rotated) sends any session with rotate-archived history back to the full-corpus reader, and rotation is size-triggered — so the transcripts most likely to hit the 10k-row pain this PR targets are the ones least likely to take the bounded path. Fine as staged work, but say so in history.md so the follow-up (perf(chat): reachable archived history and stable phone scrolling #7916 integration) is on record.
  • The handler's except Exception: continue around _bounded_slot_page plus the projection's own retry/oracle means any defect in the new machinery silently reverts to the legacy slow path at logger.debug. The feature can be dead in production with no signal; log at warning (or count) when all _FLUSH_SNAPSHOT_RETRIES attempts exhaust.

Suggestions

  • Gate the bounded path with a stat-only segment-existence probe (the chain_mid_rotation pattern) instead of read_rotated_messages_chained, which parses the archive to answer a boolean.
  • Make index warming an explicit parameter threaded from off-loop callers instead of the ambient asyncio.get_running_loop() probe inside _read_transcript — storage behavior keyed on caller thread context is hard to test and reason about.
  • Reimplement _frames as a thin wrapper over _frames_with_offsets so the subtle universal-newline/oversized framing policy has one implementation, not two that must stay byte-identical.

[DESIGN-REVIEWED] d61f943

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5, fork) — 🟡 CONCERNS

Premise-level review of d61f943aed3d483a6eb9b59cbab4b422cc2e0f65 via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All evidence gathered. Composing the review.

First-Principles-Verdict: CONCERNS

Every item traces to the measured 97.7 ms / 10.7 MiB harm, but the framing loop is copy-pasted and two TranscriptPage fields ship with zero consumers.

What this change ships

Intent: make loading a chat-history page cost the page, not the whole transcript — a FIX of a measured cost.

  1. Paginated slot-detail reads only the requested window (warm ~3.8 ms/70 KiB vs ~98 ms/10.7 MiB) — justified
  2. Sparse in-memory row index per transcript revision, warmed by off-loop full reads — justified
  3. New facade method read_messages_chained_page — justified, 1 real consumer (chat_handlers)
  4. Retry on concurrent mutation, fallback to the legacy full reader — justified, files mutate between composing reads
  5. Rotated-archive sessions and unlimited reads stay on the full reader — justified, declared
  6. Unflushed-tail reconciliation gains an offset form; wrapper keeps 2 existing callers — justified
  7. New JSONL sibling _frames_with_offsets — duplicate of _frames' loop (jsonl_util.py:216)
  8. TranscriptPage.next_before / has_more — zero consumers
  9. Full reader opens newline="" and counts offsets when off-loop — rides along (it is the warming mechanism), declared
  10. history.md section — justified, AGENTS.md same-commit rule

Watch

  • _frames_with_offsets re-states _frames' entire oversized/pending-CR framing policy (~35 lines). The in-diff justification, "avoids weakening _frames' stable policy surface", does not hold: a thin adapter preserves that surface while making divergence impossible. Grepped _frames(: 2 existing callers, both signature-compatible with an adapter.
  • The description advertises the method's "next_before, and has_more semantics", but _bounded_slot_page consumes only .messages/.total/.revision; the two fields exist for a hoped-for archive-pagination follow-up (perf(chat): reachable archived history and stable phone scrolling #7916), i.e. inherited "so we can later".

Subtractions

  • Delete _frames' loop body; reimplement it as (frame for _s, _e, frame in _frames_with_offsets(handle, cap)) so one framing policy exists (jsonl_util.py:216).
  • Drop next_before and has_more from TranscriptPage (0 non-test consumers; both are one-expression derivations of start the caller already has).

[FIRST-PRINCIPLES-REVIEWED] d61f943

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review (fork) — 🔴 changes requested (blocking)

Reviewed d61f943aed3d483a6eb9b59cbab4b422cc2e0f65 via the fork AI-review pipeline; updated in place on each push.

1 of 1 blocking finding(s) are security-class and were withheld from adjudication, so the blocking verdict stands.

BLOCKING -- src/kiro_crew/history_projection.py:687 -- Invalid UTF-8 silently truncates paginated history
except (ValueError, UnicodeDecodeError):
Invalid transcript bytes -> paginated slot read -> row omitted with HTTP 200 and shifted cursors.
Anchor: residual/crash-data-loss-corruption
Fix: Propagate decode failures so the handler falls back to the fail-closed reader.

FINDING -- src/kiro_crew/history_cache.py:401 -- "_page_index_cache.pop" runs after every append, so the next page rescans from byte zero instead of providing the claimed same-inode append extension -> Fix: retain the prior index and extend it only after verifying an append-only revision. (origin: validation)

[GPT-REVIEWED] d61f943
[BLOCK-MERGE] d61f943

Adjudication (Opus 4.8) — is blocking on each finding proportionate?

I have the complete evidence record. Let me state it.

F1 — Divergence is real but only for the invalid-UTF-8 subclass. The new bounded reader _message_record catches (ValueError, UnicodeDecodeError) and returns None, silently omitting the row and shifting the row-count/cursors with HTTP 200 (history_projection.py new _message_record, patch lines 522–527). The base authoritative full reader _read_messages_locked reads via open(path, encoding="utf-8").read() which raises UnicodeDecodeError on invalid bytes before parsing, propagating to the handler's except Exception → history_corpus_unreadable() — i.e. it fails closed/loud (history_projection.py:780,787,800-812). So the PR converts a loud 503 into a silent truncation for that one subclass. (For invalid-JSON-but-valid-UTF-8, the base full reader also skips with except json.JSONDecodeError: continue at line 807 — no divergence.)

Conditions: a transcript message row must contain byte-level-invalid UTF-8 on disk. The app's own writer serializes with json.dumps (always valid UTF-8) through atomic_write (no torn writes), so this row cannot be produced by the writer — it requires external/disk corruption. Recovery: none on the decode path (the full-reader fallback fires only on revision instability, not decode failure), and even the proposed fix only converts silent→loud; the corrupt byte is unrecoverable either way. The "loss" is the signal that history is unreadable, over data that is already corrupt — not fresh data destruction.

This meets the FLAG bar: the sole divergent condition is an input the system's own writer provably cannot generate, and a human would plausibly accept that a byte-corrupted transcript renders a truncated page rather than a 503.

[ADJUDICATION] d61f943aed3d483a6eb9b59cbab4b422cc2e0f65 total=0 uphold=0 downgrade=0
[GPT-ADJUDICATED] d61f943aed3d483a6eb9b59cbab4b422cc2e0f65
[ADJUDICATION-FENCED] d61f943aed3d483a6eb9b59cbab4b422cc2e0f65 fenced=1 flagged=1
FLAG F1 src/kiro_crew/history_projection.py:687 -- Silent truncation diverges from the fail-closed full reader only for byte-invalid-UTF-8 rows, which the app's own JSON+atomic writer cannot produce; the trigger requires pre-existing on-disk corruption and the "loss" is the loud-vs-silent signal over already-corrupt data, unrecoverable by any fix, so the residual risk is plausibly acceptable.
[GPT-ADJUDICATED-FENCED] d61f943aed3d483a6eb9b59cbab4b422cc2e0f65

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed d61f943aed3d483a6eb9b59cbab4b422cc2e0f65 via the fork AI-review pipeline; updated in place on each push.

Review details

The working tree is at base commit a8205e5, not the PR HEAD, so the patch file is authoritative for the changed lines. I've confirmed the diff facts against it.

Candidate 1 (doc splice): Confirmed from the diff. The added ### Bounded transcript pages section is inserted between the context line ...post-construction facade and the context line rebinds are read through narrow call-time lookups, severing the pre-existing sentence "The few module bindings with demonstrated post-construction facade rebinds are read through narrow call-time lookups". Real, non-blocking documentation defect.

Candidate 2 (splitlines vs binary framing divergence): Falsified. The divergence requires a raw U+2028/U+2029/U+0085 (or ASCII \v/\x1c-\x1e) byte inside a transcript record. Every in-tree transcript writer serializes with default json.dumps (ensure_ascii=True): the main append (history.py:2012), the rewrite path (history_rewrite.py:136-137, comment: "default ensure_ascii behavior"). ensure_ascii=True escapes all those codepoints to ASCII \uXXXX, and json.dumps escapes all control chars below 0x20 regardless. So no literal separator byte ever lands in a transcript, str.splitlines and _boundary_end frame identically, and the candidate's concrete input (a) does not occur in practice. The candidate self-rated LOW and could not name a write path. Dropped.

No additional groundable defects found under Step 2.

No blocking findings — one advisory: a new spec section spliced into the middle of an existing sentence.

FINDING — docs/system-specs/modules/history.md:30 — the added ### Bounded transcript pages H3 is inserted between ...post-construction facade and rebinds are read through narrow call-time lookups, splitting an existing sentence (subject "The few module bindings" severed from verb "are read") → Fix: move the entire ### Bounded transcript pages section to a paragraph boundary (after "...injected into every component." or before ## ConversationLog), leaving the "module bindings … rebinds are read through …" sentence intact.

[OPUS-REVIEWED] d61f943

@Pearcekieser
Pearcekieser force-pushed the perf/long-thread-loading branch from c9b40b5 to da2c5af Compare September 4, 2026 02:36
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 4, 2026
@Pearcekieser

Copy link
Copy Markdown
Contributor Author
  • fixed span=8b3eaa01b062 — Fixed in da2c5af. The full-reader fallback now witnesses chain membership and each indexed file revision before reading, verifies the same chain and revisions afterward, retries boundedly, and raises on continued churn. A regression inserts a production-eligible chain member after the first full read and proves the returned messages, total, and revision come from the retried snapshot.

@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 4, 2026
@Pearcekieser
Pearcekieser force-pushed the perf/long-thread-loading branch from da2c5af to 21029b9 Compare September 4, 2026 03:39
@Pearcekieser

Copy link
Copy Markdown
Contributor Author
  • fixed span=78d6e248f252 — Fixed in 21029b9. The loop now captures the durable-prefix counter alongside the raw window snapshot, includes it in the post-worker stability witness, and refuses bounded composition before any indexed read when the counters use different units. The endpoint then uses the established complete-reader reconciliation fallback. A regression trims a transient plus durable prefix and proves no bounded read occurs and the response has the exact durable display rows and total.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 4, 2026
@Pearcekieser

Copy link
Copy Markdown
Contributor Author
  • fixed span=8b3eaa01b062 — Fixed in 21029b9. Sparse-index scan OSError now propagates instead of becoming a stable zero-row entry, so endpoint retries and the complete-reader fallback remain authoritative. A regression injects a transient index scan failure, verifies it raises, and verifies no empty index is cached.

@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 4, 2026
@github-actions github-actions Bot added the readiness: action required A blocking check or review needs attention label Sep 4, 2026
@Pearcekieser
Pearcekieser force-pushed the perf/long-thread-loading branch 3 times, most recently from a6892ee to 40d8e53 Compare September 4, 2026 05:17
@Pearcekieser

Copy link
Copy Markdown
Contributor Author
  • fixed span=8b3eaa01b062 — Fixed structurally in 40d8e53. Stat-inferred append reuse and the extendable state were removed. Every file-stamp or generation change now invalidates the sparse index and rebuilds from byte zero; only an exact revision reuses checkpoints. A regression warms an index, performs a larger same-inode truncate/rewrite with different rows, and proves the complete index rebuild plus exact page contents.

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: passed Eligible automated validation passed for the current revision and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 4, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator

Open PR relationship audit

This is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion.

Relationship findings

  • PR #7916 is OVERLAPPING relative to this PR. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7916: REBASE. Same handler branch and same projection class, opposite pressures (add a corpus vs bound how much of a corpus is materialized). They are complementary in intent but cannot both land unreconciled: whichever merges second must extend the other rather than replace it, and the ordering decision is a design call, not a rebase. Files: src/kiro_crew/dashboard/chat_handlers.py, src/kiro_crew/history_projection.py.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.

@github-actions github-actions Bot added the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Sep 4, 2026
@Pearcekieser
Pearcekieser force-pushed the perf/long-thread-loading branch from 40d8e53 to d61f943 Compare September 6, 2026 00:49
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: passed Eligible automated validation passed for the current revision readiness: action required A blocking check or review needs attention merge conflict Branch has merge conflicts with its base — author must resolve before merge readiness: checking Automated validation is still running labels Sep 6, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator

@Pearcekieser this comes from a repo-wide audit of relationships between open PRs. Four other open PRs touch what this one changes, and one of them needs reconciling before either merges.

#8862 (@CrysisDeu), blocking overlap. Both rewrite the same legacy-pagination else-branch of api_chat_slot_detail in src/kiro_crew/dashboard/chat_handlers.py. #8862 rebuilds the whole corpus before the cut so that total, has_more and next_before are computed in the merged index space; this PR deletes the full-corpus read on that branch and composes those fields from an indexed durable prefix plus a resident suffix. The two requirements are opposed, so whichever lands second silently breaks the other's contract. Suggestion: land this PR first (8 files against 49) and have #8862 rebase onto it with its replay path setting requires_full_reader.

#8979 (@Premshay), coordinate. Both edit the same _read_transcript lines in src/kiro_crew/history_projection.py, and both add sections to docs/system-specs/modules/history.md. #8979 rekeys the existing transcript memos to _cache_identity (mtime_ns, ctime_ns, ino); this PR adds a new _page_index_cache with its own _file_stamp tuple. Please have the new index reuse _cache_identity instead of introducing a second stamp shape, and expect a textual conflict in the doc either way.

#9156 (@buluoray), no action. It shares chat_handlers.py and test/test_dashboard_chat.py but touches different functions: this PR bounds the storage read, #9156 shrinks the wire payload.

#9130 (@buluoray), no action. Website-only, zero file overlap, and it consumes the same total/before/next_before/has_more semantics this PR preserves.

A rebase is needed regardless: the branch is 248 commits behind main.

Posted from the 2026-09-08 open-PR relationship audit (read-only, one auditor per PR); reply here if any of this is wrong.

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

Labels

fork Pull request from a fork (external contributor) readiness: action required A blocking check or review needs attention

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bound backend transcript work for paginated chat history

2 participants