Skip to content

fix(server): bootstrap attachment cleanup without decoding the event log - #11183

Closed
ThomasCrund wants to merge 2 commits into
pingdotgg:mainfrom
ThomasCrund:fix/attachment-cleanup-filtered-replay
Closed

ThomasCrund wants to merge 2 commits into
pingdotgg:mainfrom
ThomasCrund:fix/attachment-cleanup-filtered-replay

Conversation

@ThomasCrund

@ThomasCrund ThomasCrund commented Sep 11, 2026 •

Copy link
Copy Markdown

What Changed

Attachment cleanup at startup no longer replays the whole event log. A new event store query, readEventsOfTypes, returns only thread.deleted and thread.reverted rows, filtered in SQL. getHead captures the newest sequence before replay. Bootstrap streams those rows into the same dedupe map and advances the cleanup cursor to the captured head after a successful pass, even when nothing matched. Ordering, the recreate check, and retry on a failed cleanup are unchanged.

Tests cover the typed reader and the head query, plus a pipeline case where a caught-up history holds an undecodable row of another type. The old bootstrap fails it with PersistenceDecodeError. The new one succeeds and lands the cursor on the head.

Why

Fixes #11182. Follow-up to #10777.

#10777 released consumed pages, but one 500-row page can still hold over a gigabyte of JSON. On the affected profile, page 42001 to 42500 holds 22 thread.session-set events that each carry a 73 MB lastError. Decoding that page exhausts the heap before the cursor moves, so nightlies 1400 through 1507 crash-loop at startup. Only 17 events in that history matter to cleanup.

A/B on one fresh copy of the profile through dev:desktop with an isolated home: the parent commit, which includes #10777, hit JavaScript heap out of memory at 3.8 GB about 4 s after spawn, three times in a row. This branch reached backend ready on the same copy. Cursor went from 0 to 225560, thread count unchanged.

The three touched server suites pass (71 tests). Typecheck and lint are clean.

Done with Claude Fable 5.1 in Claude Code.

Checklist

  • This PR is small and focused
  • I explained what changed and why
  • I included before/after screenshots for any UI changes (no UI changes)
  • I included a video for animation/interaction changes (not applicable)

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Improved projection startup reliability by processing only relevant event types during attachment cleanup.
    • Attachment cleanup now consistently reaches the latest available event, including when no matching events are found.
    • Prevented unrelated malformed event data from blocking cleanup of deleted thread attachments.
  • Tests
    • Added coverage for filtered event reading, empty event stores, malformed data handling, and repeated projection startup.

The attachment-cleanup bootstrap added in pingdotgg#9871 replayed every event after
its cursor and filtered for thread.deleted and thread.reverted in JS. Even
after pingdotgg#10777 released consumed pages, a single 500-row page can hold over a
gigabyte of serialized payload (historical session records carrying a 73 MB
lastError), so the backend still exhausts its heap before the cursor moves
and the desktop app crash-loops at startup on affected profiles.

Cleanup now reads only deleted and reverted events through a new
type-filtered event store query, so unrelated rows are never decoded. The
head sequence is captured before replay and the cleanup cursor advances to
it after a successful pass, including when no cleanup events exist. Ordering,
the last-wins dedupe per thread, the recreate check, and the retry-on-failure
cursor behavior are unchanged.

Regression tests append a caught-up history containing an undecodable row of
another type and assert bootstrap succeeds, removes the deleted thread's
attachment, and lands the cursor on the head; the event store tests cover the
typed reader's range and type filtering and the head query.

Done with Claude Fable 5.1 in Claude Code.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:L 100-499 changed lines (additions + deletions). labels Sep 11, 2026
@macroscopeapp

macroscopeapp Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at f93ac23

Macroscope's review found this PR approvable — This is a focused startup cleanup bug fix that filters unrelated event payloads in SQL while preserving existing projector replay, attachment cleanup, deduplication, and retry behavior. It adds targeted tests and does not change product defaults, schemas, deployment, or static-analysis configuration.

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

@coderabbitai

coderabbitai Bot commented Sep 11, 2026 •

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 2ec9a192-f5a8-44c8-8422-2a323f4fd58c

📥 Commits

Reviewing files that changed from the base of the PR and between 0a37240 and f93ac23.

📒 Files selected for processing (6)
  • apps/server/src/orchestration/Layers/OrchestrationEngine.test.ts
  • apps/server/src/orchestration/Layers/ProjectionPipeline.test.ts
  • apps/server/src/orchestration/Layers/ProjectionPipeline.ts
  • apps/server/src/persistence/Layers/OrchestrationEventStore.test.ts
  • apps/server/src/persistence/Layers/OrchestrationEventStore.ts
  • apps/server/src/persistence/Services/OrchestrationEventStore.ts

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


📝 Walkthrough

Walkthrough

The event store now supports typed range reads and head lookup without decoding unrelated payloads. Attachment cleanup uses these operations during bootstrap. Tests cover malformed history, cursor advancement, repeated bootstrap, empty stores, and updated fixtures.

Changes

Orchestration cleanup replay

Layer / File(s) Summary
Event store typed reads and head lookup
apps/server/src/persistence/Services/OrchestrationEventStore.ts, apps/server/src/persistence/Layers/OrchestrationEventStore.ts
Adds the readEventsOfTypes and getHead contracts, SQL queries, pagination, error mapping, and store wiring.
Attachment cleanup bootstrap integration
apps/server/src/orchestration/Layers/ProjectionPipeline.ts
Captures the event head, replays only thread.reverted and thread.deleted events through that head, and advances the cleanup cursor to the captured head.
Store and bootstrap validation
apps/server/src/persistence/Layers/OrchestrationEventStore.test.ts, apps/server/src/orchestration/Layers/ProjectionPipeline.test.ts, apps/server/src/orchestration/Layers/OrchestrationEngine.test.ts
Tests filtered reads, head lookup, malformed unrelated payloads, cleanup behavior, repeated bootstrap, and updated event-store fixtures.

Priority: ⬆️ High

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

Change: Bug fix · Severity of issue fixed: High

Sequence Diagram(s)

sequenceDiagram
  participant ProjectionPipeline
  participant OrchestrationEventStore
  participant Database
  participant AttachmentCleanupProjector
  ProjectionPipeline->>OrchestrationEventStore: getHead()
  OrchestrationEventStore->>Database: query latest event metadata
  Database-->>OrchestrationEventStore: head sequence and occurredAt
  ProjectionPipeline->>OrchestrationEventStore: readEventsOfTypes()
  OrchestrationEventStore->>Database: query deletion and revert events
  Database-->>OrchestrationEventStore: filtered event rows
  OrchestrationEventStore-->>AttachmentCleanupProjector: typed cleanup events
  AttachmentCleanupProjector-->>ProjectionPipeline: cleanup completed
  ProjectionPipeline->>ProjectionPipeline: advance cleanup cursor
Loading

Suggested reviewers: t3dotgg

Merge Risk: ⚪ Minimal · up to b0568

Attachment cleanup now avoids decoding unrelated event payloads while retaining bounded replay and successful cursor advancement behavior. No current merge-blocking risk was identified.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes satisfy the coding requirements in [#11182]. ProjectionPipeline.ts captures the event-store head before replay, reads only thread.reverted and thread.deleted events through `readEven…
Out of Scope Changes check ✅ Passed The pull request stays within [#11182]. The event-store interface, SQL implementation, projection bootstrap changes, and test updates directly support filtered attachment-cleanup replay and cursor sem…
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 6…
Title check ✅ Passed The title clearly and concisely describes the main change: preventing attachment-cleanup bootstrap from decoding the full event log.
Description check ✅ Passed The description includes complete What Changed and Why sections, explains the issue and solution, documents test coverage and validation, and completes the checklist. It also identifies that UI change…
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@cursor

cursor Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

@juliusmarminge

Copy link
Copy Markdown
Member

Thanks for the PR. We're not taking changes to the orchestration and provider layers right now: that part of the server is being rewritten for V2, and merging into the current code would either conflict with or be thrown away by that work.

Closing for now. If this is still an issue once V2 lands, please reopen (or open a fresh PR against the new code) and we'll take a proper look.

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:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Startup OOM with oversized replay batches remains unaddressed by #10777

2 participants