fix(server): cache absent legacy transcripts for V2 threads #24707 · pingdotgg/t3code

Added 1 commit8d ago

fix(server): cache absent legacy transcripts for V2 threads6cee913

MmacroscopeappBotapproved these changes8d ago

Approvability

Verdict: Approved at 6cee913

Macroscope's review found this PR approvable — This focused server bug fix caches the absence of legacy transcript rows for native V2 threads, eliminating repeated SQLite lookups and locks without changing transcript contents or schemas. It includes targeted regression coverage and does not alter product defaults or static-analysis settings.

^{You can add or adjust custom eligibility rules. Learn more.}

CcoderabbitaiBot8d ago

Review in Change Stack →

No actionable comments were generated in the recent review.

External image Recent review info

External image Run configuration

Source mode: Auto
Repository: pingdotgg/t3code
Branch: main
File: .coderabbit.config.ts
Commit: 90c455f
Layers: Repository TS config
Review profile: CHILL
Plan: Advanced
Run ID: 0fe1f727-1704-4b33-90a0-337280f4fb78

External image Commits

Reviewing files that changed from the base of the PR and between 90c455ff8fceef426bafc64c0b755ef864a353ab and 6cee9137c36a408031a4fa96f7d72d84409bad9f.

External image Files selected for processing: 2

apps/server/src/orchestration-v2/legacy/LegacyV1ThreadImporter.test.ts
apps/server/src/orchestration-v2/legacy/LegacyV1ThreadImporter.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

The importer now caches thread IDs when a legacy import row is absent or the transcript is already imported. A new test checks that repeated ensureTranscript calls for a native V2 thread do not repeat the legacy-import lookup or change the thread projection.

Changes: Legacy transcript cache

Layer / File(s)	Summary
Cache absent legacy imports `apps/server/src/orchestration-v2/legacy/LegacyV1ThreadImporter.ts`, `apps/server/src/orchestration-v2/legacy/LegacyV1ThreadImporter.test.ts`	The importer caches thread IDs when the import row is absent or the transcript is already imported. The test calls `ensureTranscript` three times for a native V2 thread and checks that the lookup occurs once, no threads or messages are imported, and the projection remains unchanged.

Priority: External image Low

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

Change: Bug fix

Possibly related issues

Issue 24660 describes repeated ensureTranscript lookups for native V2 threads causing reader contention. This change caches absent legacy transcripts to prevent repeated lookups.

Suggested reviewers: maria-rcks, juliusmarminge

Merge Risk: External image Minimal · up to 6cee9

The cache change has no identified merge-blocking issue; it is ready for normal checks.

Pre-merge checks: ^Passed 4

Check name	Status	Explanation
Title check	Passed	The title names the main change: ensureTranscript now caches absent legacy transcripts (missing legacy-import rows) for V2 threads, which matches the code summary and PR objectives. It uses the conven…
Description check	Passed	The description follows the template. It has Problem, Change, Scope and approval, and Verification sections. The Problem and Change sections clearly explain the repeated legacy-import lookup and the f…
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.

Full details: Title check

Explanation

The title names the main change: ensureTranscript now caches absent legacy transcripts (missing legacy-import rows) for V2 threads, which matches the code summary and PR objectives. It uses the conventional commit format with a clear scope. The title is specific and accurate.

Full details: Description check

Explanation

The description follows the template. It has Problem, Change, Scope and approval, and Verification sections. The Problem and Change sections clearly explain the repeated legacy-import lookup and the fix. The Scope section links Refs #24660 and explains why the fix qualifies as an obvious bug fix without a prior approval discussion. The Verification section gives concrete test and trace results, the failing assertion, lint and format status, and the typecheck result. The description does not state that the test-only regression was reproduced on main beyond the assertion text, which is adequate. The model and harness line is present as the template requests.

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.}

Mmaria2d ago

Note

Written by gpt-6.1-sol on behalf of Maria

Verified in a real dev environment

verified unchanged head 6cee9137c36a408031a4fa96f7d72d84409bad9f against github/main at 6575ce3250 on this linux boat vm. installed both revisions with vp i and ran their dev servers sequentially. used repository playwright with /usr/bin/google-chrome.

created the cache-verification project through the web app, pointing to a real git repository. selected codex gpt-6.1-sol and submitted the same read-only README prompt on both revisions. both real provider turns completed and displayed CACHE_CHECK_ONE.

reproduced the original bug on main, then verified the fix through authenticated requests to the running app's /api/orchestration/threads/<id>/bounded endpoint with x-t3-orchestration-protocol: 2. an external node preload observed actual sqlite reader requests without changing source, queries, or responses. read-only database inspection confirmed neither native v2 thread had a legacy-import row.

text

1	main: 10 sequential snapshot reads -> 10 HTTP 200, identical response bodies
2	main: 10 thread-specific legacy-import lookups
3	pr: 1 lookup since thread creation, 0 additional across 10 snapshot reads
4	pr: 10 HTTP 200, identical response bodies
5	pr after process restart: history restored; 10 HTTP 200, identical bodies
6	pr after process restart: 1 lookup total for that thread

nearby behavior passed: a follow-up codex turn returned CACHE_CHECK_TWO; a second independent codex thread returned CACHE_OTHER_THREAD and got its own single cache lookup. switching back and reloading preserved the first thread's two replies. restarting the pr server against its isolated home also preserved those replies and rebuilt the process-local cache.

ran vp test run apps/server/src/orchestration-v2/legacy/LegacyV1ThreadImporter.test.ts through blacksmith-check: all 5 tests passed, including legacy hydration/idempotency and the new absent-row regression. the existing head has green ci (21 passed, 7 skipped, 0 failing, 0 pending). no source changes or pushes were needed.

before, main:

main real codex reply before the cache fix

after, pr head with the same project, prompt, model, and reply:

pr head real codex reply with the absent-row cache

follow-up interaction, 15 seconds, exported as an animated capture for github compatibility. the source recording was also checked for playback and seeking in t3's player:

real codex follow-up preserves the existing transcript

Not verified

native ios/android clients, electron shell, macos/windows, relay/tunnel connections, and other providers were not exercised on this linux web/codex pass. no real v1 database migration was exercised; existing importer tests cover that neighboring path. no large-database latency or stall claim is made: the measured result is the eliminated sqlite lookup, not a timing benchmark.

fix(server): cache absent legacy transcripts for V2 threads#24707

Problem

Change

Scope and approval

Verification

Checks

Approvability

Verified in a real dev environment

Not verified