Skip to content

perf(server): stop decoding unrelated events during startup - #12846

Merged
juliusmarminge merged 2 commits into
pingdotgg:t3code/codex-turn-mappingfrom
extoci:perf/narrow-attachment-cleanup-replay
Sep 21, 2026
Merged

juliusmarminge merged 2 commits into
pingdotgg:t3code/codex-turn-mappingfrom
extoci:perf/narrow-attachment-cleanup-replay

Conversation

@extoci

@extoci extoci commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor

Attachment cleanup replay read and decoded every event after its retry cursor during bootstrap, even though it only needs thread revert/delete metadata and the newest sequence. On a 1.25 GB snapshot of a real T3 database, that path decoded 4,954 events and 23.8 MB of JSON after an ordinary restart.

Query those cleanup rows directly, plus the newest event needed to advance the cursor. Cleanup ordering, deduplication, and retry behavior stay the same without making restart cost grow with unrelated event payloads.

Across three fresh copies of the same snapshot, the database-dependent phase from migrations complete through projection bootstrap fell from a 971 ms median to 512 ms (47.3%). The full launch remained noisy because provider startup runs in the same window, so this measurement isolates the repeated database work changed here.

Projection bootstrap Before After Change
Run 1 1,018 ms 512 ms 49.7% faster
Run 2 780 ms 535 ms 31.4% faster
Run 3 971 ms 411 ms 57.7% faster
Median 971 ms 512 ms 47.3% faster

Validation: all 34 legacy projection pipeline tests passed before retargeting. On the Orchestrator v2 base, server typecheck passes and targeted lint and formatting pass.

Model: GPT-5.6 Sol. Harness: Codex in T3 Code.

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:L 100-499 changed lines (additions + deletions). labels Sep 21, 2026
@macroscopeapp

macroscopeapp Bot commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Not approved

Macroscope's review found this PR not approvable — The PR substantially reduces startup event decoding and adds focused coverage, but it also changes the production attachment-cleanup replay and retry-cursor path. A nontrivial SQL predicate now controls filesystem cleanup work, so the behavioral risk merits human review.

You can add or adjust custom eligibility rules. Learn more.

macroscopeapp[bot]
macroscopeapp Bot previously approved these changes Sep 21, 2026
@coderabbitai

coderabbitai Bot commented Sep 21, 2026 •

Copy link
Copy Markdown

Review Change StackReview Change Stack

Understand this PR’s impact

Explore downstream dependencies and potential security impact with Blast Radius.

View blast radius →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: pingdotgg/t3code/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 8a6827ad-e505-4a26-b7f2-04309cd93c6a

📥 Commits

Reviewing files that changed from the base of the PR and between 61b25b8 and b142e7d.

📒 Files selected for processing (1)
  • apps/server/src/orchestration/Layers/ProjectionPipeline.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

The projection pipeline now loads bootstrap attachment cleanup state through a typed SQL replay query. It derives pending cleanup and the last event from the returned rows, maps query failures, and applies attachment side effects using only event sequence and type.

Changes

Attachment cleanup replay

Layer / File(s) Summary
Typed cleanup replay query
apps/server/src/orchestration/Layers/ProjectionPipeline.ts
Adds AttachmentCleanupReplayRow and a SQL query that returns the latest event after the cleanup cursor plus thread.reverted and thread.deleted events.
Bootstrap cleanup application
apps/server/src/orchestration/Layers/ProjectionPipeline.ts
Builds pending cleanup from replay rows, maps decode and SQL failures, and invokes applyAttachmentSideEffects with reduced event data.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Refactor

Suggested reviewers: t3dotgg

Sequence Diagram(s)

sequenceDiagram
  participant ProjectionPipeline
  participant listAttachmentCleanupReplayRows
  participant SQLDatabase
  participant applyAttachmentSideEffects
  ProjectionPipeline->>listAttachmentCleanupReplayRows: load rows after cleanup cursor
  listAttachmentCleanupReplayRows->>SQLDatabase: execute cleanup replay query
  SQLDatabase-->>listAttachmentCleanupReplayRows: return decoded rows
  listAttachmentCleanupReplayRows-->>ProjectionPipeline: return replay rows
  ProjectionPipeline->>applyAttachmentSideEffects: apply cleanup side effects
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly and concisely describes the main performance change: avoiding decoding unrelated events during server startup.
Description check ✅ Passed The description clearly explains what changed, why it changed, the performance impact, scope, and validation results. It does not use the template headings or include the checklist, but the required i…
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

Comment @coderabbitai help to get the list of available commands.

@juliusmarminge juliusmarminge left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Retarget against v2 pls

@extoci
extoci force-pushed the perf/narrow-attachment-cleanup-replay branch from b142e7d to 94dce0e Compare September 21, 2026 10:11
@extoci
extoci changed the base branch from main to t3code/codex-turn-mapping September 21, 2026 10:12
@macroscopeapp
macroscopeapp Bot dismissed their stale review September 21, 2026 10:12

Dismissing prior approval to re-evaluate 94dce0e

@github-actions github-actions Bot added size:XXL 1,000+ changed lines (additions + deletions). and removed size:L 100-499 changed lines (additions + deletions). labels Sep 21, 2026
@macroscopeapp

macroscopeapp Bot commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor

Macroscope has since reviewed this pull request. An earlier review was skipped by a cost limit; a review has now completed, so that notice no longer applies.

@extoci
extoci force-pushed the perf/narrow-attachment-cleanup-replay branch from 94dce0e to 12a5406 Compare September 21, 2026 10:17
@github-actions github-actions Bot added size:L 100-499 changed lines (additions + deletions). and removed size:XXL 1,000+ changed lines (additions + deletions). labels Sep 21, 2026
extoci and others added 2 commits September 21, 2026 13:30
Guards the narrowed replay query: the cleanup cursor lands on the latest
event even when it is not a revert or delete, repeated reverts for one
thread collapse into one prune, and a failed cleanup leaves the cursor
behind so the next bootstrap retries it.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@juliusmarminge
juliusmarminge force-pushed the perf/narrow-attachment-cleanup-replay branch from 12a5406 to b027642 Compare September 21, 2026 20:35
@juliusmarminge

Copy link
Copy Markdown
Member

Rebased onto t3code/codex-turn-mapping at 9d6feae1ff. The commit applied cleanly with no conflicts.

One follow-up commit on top (b027642515): added apps/server/src/orchestration/Layers/ProjectionPipeline.test.ts (v2 has no pipeline test file yet; it uses the same OrchestrationProjectionPipelineLive + in-memory SQLite layer shape as main's) with a single focused case for the narrowed replay query:

  • the cleanup cursor advances to MAX(sequence) when the latest event is a plain thread.archived, not a revert or delete;
  • two thread.reverted events for one thread collapse into one prune;
  • a thread.deleted whose cleanup cannot remove its files leaves the cursor where it was, and the next bootstrap after the files are fixable moves it forward.

The test also passes against the v2 baseline, so it pins behavior rather than the implementation.

Verified in apps/server: vp test run src/orchestration/Layers/ProjectionPipeline.test.ts (1 passed) and vpr typecheck (clean).

Nothing else changed; the production diff is as you wrote it.

Rebased and touched up by a maintainer's agent; a human will re-review.

@juliusmarminge
juliusmarminge merged commit df6e170 into pingdotgg:t3code/codex-turn-mapping Sep 21, 2026
24 checks passed
juliusmarminge added a commit that referenced this pull request Sep 23, 2026
Co-authored-by: Julius Marminge <51714798+juliusmarminge@users.noreply.github.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
juliusmarminge added a commit that referenced this pull request Sep 23, 2026
Co-authored-by: Julius Marminge <51714798+juliusmarminge@users.noreply.github.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
juliusmarminge added a commit that referenced this pull request Sep 23, 2026
Co-authored-by: Julius Marminge <51714798+juliusmarminge@users.noreply.github.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
juliusmarminge added a commit that referenced this pull request Sep 23, 2026
Co-authored-by: Julius Marminge <51714798+juliusmarminge@users.noreply.github.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
juliusmarminge added a commit that referenced this pull request Sep 23, 2026
Co-authored-by: Julius Marminge <51714798+juliusmarminge@users.noreply.github.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
juliusmarminge added a commit that referenced this pull request Sep 23, 2026
Co-authored-by: Julius Marminge <51714798+juliusmarminge@users.noreply.github.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
juliusmarminge added a commit that referenced this pull request Sep 24, 2026
Co-authored-by: Julius Marminge <51714798+juliusmarminge@users.noreply.github.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
juliusmarminge added a commit that referenced this pull request Sep 24, 2026
Co-authored-by: Julius Marminge <51714798+juliusmarminge@users.noreply.github.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
juliusmarminge added a commit that referenced this pull request Sep 24, 2026
Co-authored-by: Julius Marminge <51714798+juliusmarminge@users.noreply.github.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
juliusmarminge added a commit that referenced this pull request Sep 25, 2026
Co-authored-by: Julius Marminge <51714798+juliusmarminge@users.noreply.github.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L 100-499 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants