perf(shared): cached SQLite statements no longer hold large parameters #19305 · pingdotgg/t3code

MmacroscopeappBotreviewed · review dismissed24d ago

Approvability

Verdict: Approved at 332ddf6

Macroscope's review found this PR approvable — This is a focused SQLite memory-retention fix that bypasses statement caching only for large or unmeasurable bindings while preserving normal caching and query behavior. Writer and reader-worker paths are covered by targeted tests, with no schema, deployment, security, billing, default, or static-analysis changes.

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

Added 4 commits24d ago

perf(shared): reader worker no longer caches statements with large parameters36f5f64
perf(shared): cached SQLite statements no longer hold large parameters57c8429
test(shared): detached view test no longer depends on the Node version6f60b46
fix(shared): large-binding check no longer runs getters or misses raw buffers332ddf6

CcoderabbitaiBot24d ago

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.config.ts
Review profile: CHILL
Plan: Team
Run ID: 2f577f94-4291-4cce-9062-0c05493bac1a

📥 Commits

Reviewing files that changed from the base of the PR and between 173a97097659be5f77ab2c05cf5dfe84be3eb094 and 332ddf66ccd6194339d8de25563733c36096111f.

📒 Files selected for processing: 2

packages/shared/src/nodeSqliteClient.test.ts
packages/shared/src/nodeSqliteClient.ts

Included review availability: This review used your included allowance. 6 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 10 reviews per hour.

📝 Walkthrough

The SQLite client now bypasses statement caches for oversized or unmeasurable bindings. Value-array execution also applies the current safe-integer setting.

Changes: SQLite binding and execution

Layer / File(s)	Summary
Measure bindings and select statements `packages/shared/src/nodeSqliteClient.ts`, `packages/shared/src/nodeSqliteClient.test.ts`	Bindings over 64 KiB or with sizes that cannot be measured use uncached statements in the writer and reader paths. Tests cover large and small parameters, detached views, and parameter getters.
Apply value-array safe-integer settings `packages/shared/src/nodeSqliteClient.ts`, `packages/shared/src/nodeSqliteClient.test.ts`	Value-array execution sets safe-integer mode from the current fiber context. Tests check number and bigint results for small and large text parameters.

Priority: ⬇️ Low

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

Change: Bug fix

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to 332dd

The changed SQLite paths have no identified issue requiring a fix before merge.

Pre-merge checks: ^Passed 3 · ^Failed 1

^Failed Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Description check	Warning	The description explains the problem, fix, affected components, and verification results. It does not include the required Scope and approval section or provide explicit maintainer approval, triaged i…	Add a Scope and approval section. Link the triaged issue or approval discussion with the relevant maintainer approval comment, or explain why this focused fix qualifies for an exemption.

^Passed Passed checks: 3 passed

Check name	Status	Explanation
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 change: preventing cached SQLite statements from retaining large parameters.

Full details: Description check

Explanation

The description explains the problem, fix, affected components, and verification results. It does not include the required Scope and approval section or provide explicit maintainer approval, triaged issue details, or a valid exemption rationale.

Fix all pre-merge checks with AI

✨ Finishing Touches

📝 Generate docstrings

Commit to this branch
Create a new PR

🧪 Generate unit tests (beta)

Commit to this branch
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.}

MmacroscopeappBotapproved these changes24d ago

Ggithub-actionsBot24d ago

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

Provider	Metric	Main baseline	This PR	Impact	PR ceiling
Codex	Total thread wire	5.0 KiB	5.0 KiB	0 B (0.0%)	6.8 KiB	✅
Codex	Thread snapshot wire	3.8 KiB	3.8 KiB	0 B (0.0%)	4.9 KiB	✅
Codex	Live turn WebSocket wire	1.2 KiB	1.2 KiB	0 B (0.0%)	2.0 KiB	✅
Codex	Live turn WebSocket decoded	20.9 KiB	20.9 KiB	0 B (0.0%)	29.3 KiB	✅
Codex	Live turn messages	2	2	0 (0.0%)	8	✅
Claude	Total thread wire	5.0 KiB	5.0 KiB	0 B (0.0%)	6.8 KiB	✅
Claude	Thread snapshot wire	3.8 KiB	3.8 KiB	0 B (0.0%)	4.9 KiB	✅
Claude	Live turn WebSocket wire	1.2 KiB	1.2 KiB	0 B (0.0%)	2.0 KiB	✅
Claude	Live turn WebSocket decoded	21.2 KiB	21.2 KiB	0 B (0.0%)	29.3 KiB	✅
Claude	Live turn messages	2	2	0 (0.0%)	8	✅

Baseline: 173a970 · PR result: 332ddf6 · Source CI: success

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

Codex decoded thread snapshot: 108.5 KiB
Claude decoded thread snapshot: 108.8 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

perf(shared): cached SQLite statements no longer hold large parameters#19305

Problem

Fix

Proof

Checks

Approvability

Thread transfer impact