Skip to content

perf: rewriting a transcript no longer re-reads the whole history (#99) - #128

Merged
Carl723000 merged 8 commits into
ClaudeCodeUsage:mainfrom
rsyuzyov:perf/rewrite-without-rebuild
Oct 17, 2026
Merged

Carl723000 merged 8 commits into
ClaudeCodeUsage:mainfrom
rsyuzyov:perf/rewrite-without-rebuild

Conversation

@rsyuzyov

@rsyuzyov rsyuzyov commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

Problem

Any mid-file mutation of one transcript — a /rewind truncation, session repair, an mtime-only touch that fails append verification — produced a 'source' rebuild plan with a prior, and unsafeSourceMutation escalated it to replaceWithFullAnalysisPlans(): every surviving file was re-read from disk. Live logs from one workstation show ~31 such full rebuilds in four days, 20–40 s each — the dominant remaining refresh cost after #119–#123. (Same class as the deletion rebuild in #127, through a different clause.)

Change

The sourcePlans clause is removed from unsafeSourceMutation; only deletions (until #127 lands) and moves still escalate pre-parse. Everything the bounded path needs already existed:

  • the rewritten file is re-read in full — its body genuinely changed;
  • its previously owned UUIDs already feed the ownership-restoration pass (a surviving twin carrying them gets an 'ownership' plan and is re-read, nothing else);
  • the append fast path's plan filter already excludes rebuild-with-prior plans, so the materialized analysis is rebuilt in memory from retained per-file contributions — exact, no extra I/O.

Two post-parse probes guard what a rewrite can actually break, reusing the existing append-probe fallback:

  1. Order shift — the file's first event moved (contribution.firstTimestampMs !== prior.firstTimestampMs), so its place in the canonical order is unproven.
  2. Ownership preemption — it now touches a UUID owned by a later file, which the full loader resolves by scan order.

Either one retries once as the full ordered rebuild.

Measured

976-file, 1.58 GB corpus, one mid-history transcript truncated:

CPU body reads bytes aggregate mutations
before 19.3 s 976 1.59 GB 148 675
after 0.8 s 1 228 KB 11

Tests

Five new tests, each comparing records and contentAnalysis against the full loader:

  • a truncated transcript re-reads only that file — bodyReads === 1 (fails on main: full rebuild).
  • a mid-file rewrite re-reads only the rewritten file — same.
  • a rewrite that drops an owned uuid re-reads the twin that also carries it — the rewritten file plus the surviving twin, nothing else.
  • a rewrite that moves the first event falls back to a full ordered rebuild.
  • a rewrite claiming a uuid owned by a later file falls back to a full rebuild.

🤖 Generated with Claude Code

@github-actions

github-actions Bot commented Oct 9, 2026

Copy link
Copy Markdown

🤖 Automated first-pass review

TL;DR
The bounded-rewrite path is well reasoned and the CHANGELOG entry is present and concrete. Two things need attention before merge: an AGENTS.md invariant that this change deliberately relaxes, and the two probe tests asserting exact bodyReads counts that will move when #127 lands.

Analysis

  • Policy tension (correctness/readiness). AGENTS.md states runtime refreshes "must not fall back to a full-corpus body read or full-record aggregation," but the post-parse probes intentionally can fall back to one ordered full rebuild on order shift or ownership preemption. The diff's own comments justify this as a per-occurrence, guarded exception, and the measured behaviour is a large net win — but the PR does not reconcile the invariant text. Either add a note to the PR description acknowledging this is a documented, probe-guarded exception, or update the invariant text in the same PR (with the AGENTS.zh-CN.md companion). This is the main merge-readiness question.

  • Tests are coupled to the perf: deleting a transcript no longer re-reads the whole history (#99) #127 state. a rewrite that moves the first event… and a rewrite claiming a uuid owned by a later file… assert bodyReads === 3, which is "1 bounded read + fallback re-reads both files." The PR description says deletions still escalate pre-parse "until perf: deleting a transcript no longer re-reads the whole history (#99) #127 lands." Once/if perf: deleting a transcript no longer re-reads the whole history (#99) #127 changes the deletion/rebuild escalation, the fallback read count here will shift and these assertions break. Consider asserting the fallback happened via a more robust signal (e.g. changed.rebuild on the affected files, or that the final records/contentAnalysis match assertMatchesFull) rather than a hard-coded total bodyReads, or add a comment tying the 3 to the current full-rebuild definition. The companion bodyReads === 2 twin test is fine as-is.

  • Probe ordering. laterOwnerCollision is seeded with orderShiftedRebuild, and sourceRebasedRebuilds is gated on previousAnalysisRuntime && parsedPlans.length > 0. Confirm the orderShiftedRebuild short-circuit cannot mask a different file's genuine later-owner collision that the loop would otherwise detect — the loop breaks on first laterOwnerCollision, so seeding it true skips the whole scan. If a full rebuild is taken anyway, correctness holds, but the diagnostic reason reported may mislead. Low risk, worth a one-line comment.

  • CHANGELOG. Entry is present under ### Fixed, references Extension causes high cpu load #99, and includes the measured numbers. Good. It mixes two corpora (976-file/1.58 GB in the PR body and 650-file/1.4 GB in the unsafeSourceMutation comment) — the comment's numbers are stale relative to the new measurement; align them so a future reader isn't confused.

  • i18n / README. No user-facing strings or dashboard copy change, so no i18n.ts or nine-README update is required. If the maintainer decides to document the new bounded-rewrite guarantee as user-facing behaviour, the READMEs would need updating — but that's a judgment call, not a defect.

Suggested next step(s)

  1. Resolve the AGENTS.md invariant wording — either document the probe-guarded fallback as an explicit exception in the PR, or amend the invariant (plus AGENTS.zh-CN.md) in this PR.
  2. Make the two fallback tests resilient to the expected perf: deleting a transcript no longer re-reads the whole history (#99) #127 read-count change (assert the fallback outcome rather than bodyReads === 3).
  3. Add a one-line comment on the orderShiftedRebuild seed explaining the short-circuit, and align the stale 650-file/1.4 GB numbers in the unsafeSourceMutation comment with the 976-file/1.58 GB measurement.

🤖 Generated by DeepSeek as an automated first pass — not a maintainer decision.

@rsyuzyov

Copy link
Copy Markdown
Contributor Author

Addressing the first-pass review, all four points in one push:

  1. AGENTS.md invariant — amended (with the AGENTS.zh-CN.md companion) to name the single permitted exception: a probe-guarded retry when a changed file's ownership order cannot be proven safe from retained contributions. Note the sibling invariant at line ~105 already requires truncate work to be limited to affected files, and the pre-existing append probe has had this fallback shape since it was introduced — this PR narrows real fallbacks from 31 in four days to the genuinely unprovable cases.
  2. Fallback tests — now assert changed.rebuild === 2 (every surviving file escalated to a rebuild plan) instead of bodyReads === 3, decoupling them from the probe's read-count details and from perf: deleting a transcript no longer re-reads the whole history (#99) #127.
  3. Seed short-circuit — one-line comment added: seeding laterOwnerCollision with orderShiftedRebuild only skips the scan; both conditions select the same full-rebuild fallback, so no distinct outcome is masked.
  4. Stale figures — the comment now carries the 976-file / 1.58 GB / 19.3 s → 0.8 s measurement.

@Carl723000 Carl723000 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @rsyuzyov for the order/ownership probes and the updates to the earlier feedback. A fresh review against released main d6d69aa found a mixed rewrite/append case those probes do not cover.

Rewritten owner + appended surviving claimant: A owns UUID X, and B's existing prefix contains X's suppressed duplicate. Rewrite A to drop X while keeping its first timestamp unchanged, and append unrelated content to B. The bounded-rewrite guard change exposes the ownership-restoration pass's plannedPaths.has(claimant) skip (head line 2731). B gets only a tail read. X disappears from content analysis and stays missing on the next unchanged refresh.

Order is unchanged, and X is absent from B's parsed tail, so neither probe restores the missing contribution. The released baseline matches the full loader. The case was independently rerun with synthetic files; token totals and prior snapshots remained correct. Combining #127 and #128 still reproduces the content-analysis loss.

Please promote append plans that require ownership replay and add this mixed-event regression with full-loader equivalence, an unchanged follow-up refresh and snapshot immutability. Existing index tests pass, so those tests alone do not cover the missing path. Please also reconcile this original branch with current main: GitHub reports policy-document merge conflicts. Preserve the current canonical/bilingual policy and move the new release note under Unreleased, not released 2.5.0.

Reviewed head: a998b040b1cc596b5b71fe46f370419de06c8e18. This remains a valuable optimization to refine through your original PR; it has not been included in 2.5.0.


🤖 Generated by OpenAI Codex as an automated first pass — not a maintainer decision.

rsyuzyov and others added 4 commits October 15, 2026 22:04
…audeCodeUsage#99)

A mid-file rewrite or truncation produced a 'source' rebuild plan with a
prior, and unsafeSourceMutation escalated it to replaceWithFullAnalysisPlans:
every surviving file was re-read. Live logs show ~31 such full rebuilds in
four days on one workstation, 20–40 s each.

The bounded path was already in place: the rewritten file is re-read in full,
its previously owned UUIDs feed the ownership restoration pass, and the
materialized analysis is rebuilt in memory from retained contributions. Two
post-parse probes guard what a rewrite can actually break — a moved first
event (canonical order) and a UUID claimed ahead of its later owner
(first-owner dedup); either retries as the full ordered rebuild, reusing the
existing append-probe fallback.

Measured on a 976-file, 1.58 GB corpus, one mid-history transcript truncated:
19.3 s CPU, 976 body reads, 149k aggregate mutations before; 0.8 s CPU, one
body read, 11 mutations after.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ClaudeCodeUsage#99)

First-pass review follow-ups: the AGENTS.md invariant (and the zh-CN
companion) now names the one permitted full-corpus retry — probe-guarded,
when a changed file's ownership order is unproven; the two fallback tests
assert the escalation through changed.rebuild instead of an exact bodyReads
total; the orderShiftedRebuild seed documents its short-circuit; the stale
650-file figures in the unsafeSourceMutation comment match the 976-file bench.

Co-Authored-By: Claude Code <noreply@anthropic.com>
…CodeUsage#99)

The ownership-restoration pass skipped a claimant already planned as a
tail append, so a prefix duplicate vanished and stayed missing. That
append is promoted to a full replay. Rebased onto the published 2.5.0
policy wording; the release note stays under Unreleased.

Co-Authored-By: Claude Code <noreply@anthropic.com>
@rsyuzyov
rsyuzyov force-pushed the perf/rewrite-without-rebuild branch from a998b04 to 05d6816 Compare October 15, 2026 17:06
@rsyuzyov

Copy link
Copy Markdown
Contributor Author

Addressing the review of a998b04, rebased onto current main:

  1. Rewritten owner + appended claimant. Same gap as perf: deleting a transcript no longer re-reads the whole history (#99) #127: the restoration pass skipped a claimant already planned as a tail append, and neither order probe sees a prefix duplicate when the rewritten file keeps its first timestamp. That append is promoted to a full replay. Test: rewriting an owner while appending its claimant replays that claimant — bodyReads === 2 (rewritten owner and claimant; a bystander is not read), full-loader content-analysis equivalence, an unchanged follow-up with zero body reads, and the prior snapshot frozen.

  2. Policy docs. AGENTS.md and AGENTS.zh-CN.md keep the published 2.5.0 wording. The probe-guarded exception is an additional sentence, not a replacement of the unchanged-refresh invariant.

  3. Changelog. The note is under Unreleased, not 2.5.0.

@Carl723000 Carl723000 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The appended-claimant restoration fix is verified, but a separate ordering regression remains at 05d6816cee7c16d8b719d0afb620e8bc3288be75.

Blocking: ownership promotion can bypass the first-timestamp/order-change guard

The promotion around src/claudeIncrementalIndex.ts:2897–2903 changes an append plan into an ownership rebuild. The order-shift check around :3027–3029 only considers source rebuilds, so it misses an order change in that promoted file.

Synthetic reproduction (no real logs required):

  1. A and B have no dated events. A owns UUID X. B's existing prefix contains a suppressed duplicate X and UUID Y owned by B.
  2. C has its first event at 09:00 and contains a duplicate Y with different content.
  3. In one refresh, rewrite A to remove X (still undated), and append B's first dated event at 11:00.
  4. Restoring X promotes B's append to an ownership rebuild. B now sorts after C, but the source-only guard does not replay canonical content ownership.

Expected: Y's content-analysis owner is C, matching a full load. Actual: the incremental snapshot keeps Y from B; an unchanged follow-up with zero body reads retains the mismatch. The baseline and #127 individually pass this case; #128 fails it. Token totals and previously returned snapshots stayed correct in this fixture, so this finding is about content-analysis ownership, not demonstrated token-total corruption.

Please retain order-change detection for promoted rebuilds regardless of their overwritten reason (or an equivalent correctness-preserving guard), with a regression comparing the initial, changed and unchanged follow-up snapshots against the full loader, and checking prior-snapshot immutability. Please also exercise the deletion variant when integrating with #127. Holding this PR until that is addressed; no need to replace the original contribution.


🤖 Generated by OpenAI Codex as an automated first pass — not a maintainer decision.

Retain the contributor's bounded transcript rewrite/truncation path alongside
the existing deletion fast path and concurrent manifest discovery. Validate first
timestamp changes for all prior-file rebuild reasons so ownership restoration
cannot leave stale source-order deduplication.

Add rewrite/truncation/deletion regressions with canonical-loader equivalence,
unchanged-refresh zero body reads and immutable prior-snapshot checks.

Co-authored-by: OpenAI Codex <215057067+openai-codex[bot]@users.noreply.github.com>

@Carl723000 Carl723000 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correctness finding

The current a864bfe head can retain stale content-analysis UUID ownership when discovery ties reorder existing files while a new file is added. canonicalOrderChanged requires the projected old-file count to equal the complete current manifest count, so the new file suppresses that check.

Synthetic case: B/C share a UUID at the same first timestamp; rewrite an unrelated A without changing its timestamp, add unique D, and change the manifest from [A,B,C] to [A,C,B,D]. The canonical loader assigns the shared UUID to C; the incremental snapshot retains B. The content-analysis estimate is 16 rather than 106 and remains stale on an unchanged refresh. This is not an actual usage/billing discrepancy in the fixture.

Compare the relative canonical order of surviving existing files even when the manifest also has insertions/deletions, and retain a regression against the canonical loader before merging this head.

中文:新增文件不应屏蔽存活旧文件之间的相对归属顺序检查。当前组合场景仍会产生持久的内容分析归属偏差,需补齐守卫和回归后再合入。


🤖 Generated by OpenAI Codex as an automated first pass — not a maintainer decision.

Carl723000 and others added 3 commits October 17, 2026 22:30
Compare the relative canonical order of surviving old files even when a refresh
also inserts or deletes files. Detect discovery-rank changes on changed bodies,
without treating deletions or distinct timestamp order as unsafe by themselves.

Add mixed rewrite/insertion/deletion/append regressions and safe controls with
canonical-loader equality, immutable snapshots and zero-read unchanged followups.

Co-authored-by: OpenAI Codex <215057067+openai-codex[bot]@users.noreply.github.com>
Co-authored-by: OpenAI Codex <215057067+openai-codex[bot]@users.noreply.github.com>

@Carl723000 Carl723000 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed final integration commit 5dd3da06f9b1604f5647b5b348617400e5f14b2a against current main fcdb858f1eb4b8d9dcd7ae5a8988435cddfd510a.

The ownership-restoration and surviving-file order findings are addressed. Regression cases cover rewrite/insertion/deletion combinations, tied versus distinct timestamps, concurrent appends and deterministic reused identities. Synthetic comparisons agree with the canonical full loader, preserve prior snapshots, and read zero bodies on an unchanged follow-up.

Local integrated Node suite: 2699 passed, 2 existing skips, no failures. The latest hosted workflow is still running; merge remains conditional on all required checks succeeding at this exact head. No release is part of this change.

中文:前两轮归属问题已修复;新增文件不再屏蔽旧文件顺序变化,独立文件与身份复用均有确定性用例。与最新 main 联合后的本地全量测试通过,仍须等待本提交完整云端门禁全绿。


🤖 Generated by OpenAI Codex as an automated first pass — not a maintainer decision.

@Carl723000
Carl723000 merged commit 7c82792 into ClaudeCodeUsage:main Oct 17, 2026
10 checks passed
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.

2 participants