Sign up
View stargazersSign up

perf(server): streamed writes no longer re-parse their payload#25162

closed1
main
GitHub fork: perf/streamed-projection-small-fixes
opened6d agobyEEmanuelMedinaAraujo
2 files changed
View on GitHubTwo-way sync enabledPR last imported 1d ago

Problem

Every streamed update writes message.updated and turn-item.updated through ProjectionStoreV2.apply. Both cases encode the payload to JSON, then JSON.parse the whole thing again, full streamed reply included, only to read createdAt/updatedAt back out. The extra parse runs on the event loop inside the write transaction, and its cost grows with reply length on every chunk.

Fixes #25157

Change

Encode the two timestamps directly from event.payload with Schema.DateTimeUtcFromString, the same schema the payload uses, so the indexed columns get identical strings without the parse. Only these two cases change; other projection cases still parse their small payloads.

Scope and approval

This is a small, focused fix with no behavior change: the stored columns and payload stay byte-identical. The bug is described in #25157.

Verification

  • New test indexes streamed messages and turn items by the timestamps in their payloads checks that created_at/updated_at columns equal the timestamps inside payload_json for a ~3 KB streamed message and turn item, including millisecond precision.
  • vp test run apps/server/src/orchestration-v2/ProjectionStore.test.ts: 47 passed. vp fmt --check, vp lint and the server typecheck are clean.
  • Benchmark on file-backed SQLite (SqlitePersistence.layerFromPath), Linux, Node 26.12.1. Each figure is the median of 200 message.updated + turn-item.updated writes of one reply, from two separate runs:
Reply sizemainthis PR
10 KB0.92 / 0.92 ms0.38 / 0.44 ms
100 KB1.16 / 1.15 ms0.96 / 0.99 ms
500 KB4.43 / 4.38 ms3.40 / 3.43 ms

I did not measure this through a live provider stream.

Written by Opus 5.5 (high) in T3 Code (Claude Code harness) on behalf of Emanuel.

🤖 Generated with Claude Code

Checks

6 successful · 7 skipped
All checks passed
c1dfb3f

Added 1 commit

  • perf(server): streamed writes no longer re-parse their payloadc1dfb3f
MmacroscopeappBot6d ago
Synced from GitHub

Approvability

Verdict: Approved at 36b2517

Macroscope's review found this PR approvable — This is a narrowly scoped server performance fix: timestamp columns are encoded without reparsing large streamed payloads, and live websocket events avoid a redundant wire projection already performed upstream. Added tests verify timestamp consistency and single-projection behavior, with no product-default or static-analysis configuration changes.

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

CcoderabbitaiBot6d ago
Synced from GitHub

Review in Change Stack →

No actionable comments were generated in the recent review.

External image Recent review info
External image Run configuration
External image Commits

Reviewing files that changed from the base of the PR and between 2eccc57f1e551ce455efc29ed1b2526f951ee2f2 and c1dfb3f91a712d286f2941ee5a3b7490a718b50a.

External image Files selected for processing: 2
  • apps/server/src/orchestration-v2/ProjectionStore.test.ts
  • apps/server/src/orchestration-v2/ProjectionStore.ts

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

Walkthrough

ProjectionStore now encodes message and turn-item timestamps directly from event payload fields instead of parsing encoded payload JSON. A SQL test checks that the indexed timestamps match the payload timestamps.

Changes: Timestamp indexing

Layer / File(s)Summary
Encode payload timestamps for SQL writes
apps/server/src/orchestration-v2/ProjectionStore.ts, apps/server/src/orchestration-v2/ProjectionStore.test.ts		Message and turn-item writes encode timestamps from payload fields. The SQL test checks stored timestamp columns against the payload values.

Priority: External image Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Refactor

Possibly related PRs

  • pingdotgg/t3code#24145: Both PRs change the message.updated and turn-item.updated SQL handlers in ProjectionStore; this PR avoids parsing payload JSON, while that PR skips updates when indexed fields are unchanged.

Suggested reviewers: t3dotgg

Merge Risk: External image Minimal · up to c1dfb

No actionable merge-blocking risk remains.

Pre-merge checks: Passed 4
Check nameStatusExplanation
Title checkPassedThe title is concise, specific, and accurately summarizes the main performance change: streamed writes no longer re-parse their payload.
Description checkPassedThe description includes all required sections. It explains the problem, implementation, scope, approval basis, focused verification, benchmark results, and unmeasured live-stream coverage.
Linked Issues checkPassedFor #25157, ProjectionStore.ts now encodes event.payload.createdAt and event.payload.updatedAt with Schema.DateTimeUtcFromString in the message.updated case. It encodes `event.payload.update…
Out of Scope Changes checkPassedThe changes stay within #25157. The production diff changes only timestamp encoding in the two specified projection cases. The added test directly verifies the required indexed values for streamed mes…
Full details: Linked Issues check

Explanation

For #25157, ProjectionStore.ts now encodes event.payload.createdAt and event.payload.updatedAt with Schema.DateTimeUtcFromString in the message.updated case. It encodes event.payload.updatedAt the same way in the turn-item.updated case. These cases no longer call parseEncodedPayload to read timestamps. The new test verifies indexed timestamp values against payload_json, including millisecond precision and streamed text. The separate ws.ts concern is not required by #25157.

Full details: Out of Scope Changes check

Explanation

The changes stay within #25157. The production diff changes only timestamp encoding in the two specified projection cases. The added test directly verifies the required indexed values for streamed messages and turn items. No unrelated production behavior is changed.

Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

JJulius Marminge2d ago

Thank you, @EmanuelMedinaAraujo, for finding and fixing this first. #25252 makes the same change (encode createdAt/updatedAt straight from the typed payload instead of re-parsing the JSON) in the same two files, and adds a regression test that fails on main plus a live-server run, so we're taking that one forward and closing this as a duplicate. Your write-up of where the parse sits inside the write transaction, and the file-backed SQLite benchmark, were the clearest description of the cost.