Skip to content

fix(sqlite): retry failed statement preparations - #10584

Merged
shivamhwp merged 2 commits into
pingdotgg:mainfrom
yashranaway:fix/retry-sqlite-prepare-failures
Sep 25, 2026
Merged

shivamhwp merged 2 commits into
pingdotgg:mainfrom
yashranaway:fix/retry-sqlite-prepare-failures

Conversation

@yashranaway

@yashranaway yashranaway commented Sep 7, 2026 •

Copy link
Copy Markdown
Contributor

What Changed

SQLite caches successfully prepared statements for the configured lifetime, but expires failed preparations immediately. A later query can retry after the database becomes available without recreating the client.

Why

The prepare cache retained failures for ten minutes by default. A query that initially failed because a table was missing or another connection held an exclusive lock continued returning its cached error after the condition was resolved.

Testing

  • The missing-table regression fails on upstream even after creating and populating the table.
  • A real temporary SQLite database verifies positional and object queries recover immediately after an exclusive lock is released.
  • All 16 focused shared-client and server persistence tests pass across three suites.
  • Shared-package typecheck, scoped lint, and formatting pass.
  • No live database access, frontend changes, or browser verification.

Model: GPT-6 Astra
Harness: T3 code

Note

Fix retry of failed statement preparations in 'nodeSqliteClient'

  • Failed statement preparations were previously cached for the configured TTL in nodeSqliteClient.ts, preventing retries after the underlying issue resolved.
  • The makeWithDatabase client factory now passes the prepare function through Cache.makeWith. Successful preparations keep the configured TTL, while failed results use a zero TTL to allow retrying.
  • Added tests covering query reuse after a missing table is created and after an exclusive database lock is released.
  • Risk: Behavioral Change: failed prepare results in nodeSqliteClient no longer remain cached, so later query executions will re-attempt the prepare step rather than returning the cached error.

Macroscope summarized 0c63948.

Summary by CodeRabbit

  • Bug Fixes
    • SQLite queries can be retried successfully after a temporarily missing table is created.
    • Queries affected by an exclusive database lock can recover once the lock is released.
    • Failed query preparations are no longer cached, preventing repeat failures after database conditions change.
    • Successfully prepared queries continue to be cached for the configured duration, preserving the performance benefit for subsequent executions.

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

macroscopeapp Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 0c63948

Macroscope's review found this PR approvable — This is a focused SQLite cache bug fix that evicts failed statement preparations immediately while preserving the existing successful-query cache policy. The accompanying tests cover recovery from missing schemas and released database locks, with no schema, API, security, or deployment changes.

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

@macroscopeapp macroscopeapp Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All clear

Posted via Macroscope — Effect Service Conventions

@coderabbitai

coderabbitai Bot commented Sep 7, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their 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: efa3b170-6deb-4ec1-bfbb-716551ec38a8

📥 Commits

Reviewing files that changed from the base of the PR and between 0c63948 and 004f2e8.

📒 Files selected for processing (2)
  • packages/shared/src/nodeSqliteClient.test.ts
  • packages/shared/src/nodeSqliteClient.ts

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


📝 Walkthrough

Walkthrough

The SQLite client now expires failed prepared statements immediately and retains successful preparations for the configured TTL. Integration tests cover recovery after table creation and lock release.

Changes

SQLite preparation recovery

Layer / File(s) Summary
Preparation cache behavior
packages/shared/src/nodeSqliteClient.ts
The prepared-statement cache uses Cache.makeWith. Successful preparations use the configured or default ten-minute TTL. Failed preparations receive a zero TTL.
Preparation recovery tests
packages/shared/src/nodeSqliteClient.test.ts
Integration tests verify typed preparation errors and successful re-execution after a missing table is created or an exclusive database lock is released.

Priority: ⬇️ Low

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

Change: Bug fix

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to 004f2

No identified issue prevents merging after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 004f2

The change lets queries retry after a temporary SQLite preparation failure without adding an external entrypoint or changing database permissions. Behavior under simultaneous or sustained failures remains less certain.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The changed cache remains connection-local; the examined change does not introduce a cross-connection cache or a production entrypoint. Downstream caller exposure was not established.

Resilience and Maintainability Implications

  • inferred — Zero-TTL failures permit later retry rather than retaining a transient error. The available evidence does not establish how concurrent lookups or interruption interact with eviction, so it does not establish a security-relevant failure path under those conditions.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: retrying failed SQLite statement preparations.
Description check ✅ Passed The description clearly explains what changed, why it changed, and how it was tested. The UI Changes section is not needed because the PR has no UI changes. The repository checklist is omitted, but th…
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 2…
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.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

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

@shivamhwp
shivamhwp merged commit d06f0ff into pingdotgg:main Sep 25, 2026
21 checks passed
github-actions Bot added a commit to omarcresp/t3code-flake that referenced this pull request Sep 25, 2026
## What's Changed
* feat(usage): read cursor, opencode, and antigravity history by @maria-rcks in pingdotgg/t3code#10409
* fix(sqlite): retry failed statement preparations by @yashranaway in pingdotgg/t3code#10584
* fix(mobile): scale Android controls with appearance text size by @none23 in pingdotgg/t3code#13356
* fix(web): return focus to the composer after saving a citation note by @mackinleysmith in pingdotgg/t3code#13450
* feat(observability): honor the standard OTLP endpoint, headers, and protocol variables by @yordis in pingdotgg/t3code#13492
* fix(terminal): settling a thread closes its idle shells by @t3dotgg in pingdotgg/t3code#13673
* fix(server): load Cursor keyring with createRequire by @Yash-Singh1 in pingdotgg/t3code#13678
* perf(server): avoid rereading unchanged files in review previews by @shivamhwp in pingdotgg/t3code#13395


**Full Changelog**: pingdotgg/t3code@v0.0.43-nightly.20260925.2251...v0.0.43-nightly.20260925.2269

Upstream release: https://github.com/pingdotgg/t3code/releases/tag/v0.0.43-nightly.20260925.2269
aorwall added a commit to aorwall/t3code that referenced this pull request Sep 27, 2026
Merges `pingdotgg/t3code` up to `eeea71a88` (55 commits after base
`ebdcda135`). This PR is based on `main`; no other merge PR is open.

## Resolution
- **Files that landed:** 271, against 272 in the upstream range. The one
missing is `apps/server/src/cli/pair.ts`, which stays deleted under
`deletedUpstreamPaths`. The fork delta is 756 files, the same as the
last merge.
- **Conflicts (4):**
  - `apps/server/src/cli/pair.ts` (modify/delete): kept deleted.
- `ProviderSettingsPanel.tsx`, `SettingsSidebarNav.tsx`,
`useAvailableSettingsSearchItems.ts` (converged): kept the fork's gates,
search filters and feature-flag read. Took upstream's `scopeSearch`
argument and `cursorKeychainUsageEnabled` prop.
- **New fork gate:** the `cursor-keychain-usage` search item from pingdotgg#13714
is now `providerConfigurationOnly`, because its row sits inside
`UsageProviderSettings`, which the fork hides. Upstream's test now
asserts against `FEATURES.providerConfiguration` and gained the fork's
`forgejoEnabled` field. That field was the only typecheck failure.
- **Lockfile:** re-derived with `install.mjs`. The install left it
unchanged, and the fork's edges are present.
- **Sweep:** new upstream files matched none of the owned-concern
keywords, and upstream added no workflows.
- **Unsupported methods:** nothing to add or drop.

## Verification
The full `verify.mjs` run passes all 10 checks: duplicate-adds,
tripwires, resolution-check, unsupported-methods, lockfile, fmt, lint,
typecheck, build and test.

## Usable as-is
- Chat width setting (a client setting) (pingdotgg#11594)
- The "agents working" banner links to the Agents panel (pingdotgg#13572)
- Composer fixes: paste lands in the composer after clicking away
(pingdotgg#13553), the collapsed composer bar keeps its labels while scrolling
(pingdotgg#13555), focus returns after saving a citation note (pingdotgg#13450)
- Nested task states stay out of parent bullets (pingdotgg#11477). Compact
provider instance badges are back (pingdotgg#13700), and the OpenAI logo is
updated (pingdotgg#13611)
- Client runtime: sync status no longer flickers (pingdotgg#13551), slow servers
finish loading the thread list once instead of twice (pingdotgg#13683), hidden
terminal drawers release thread history (pingdotgg#13686)
- Mobile fixes: Android control sizing, project icons, Home row
performance, and running threads open at the latest message
- The worktree setup label fix (pingdotgg#13590). It applies to UI that
`FEATURES.worktreeSelection` gates.

## Unsupported in Moatless / needs implementation
- **Cursor, OpenCode and Antigravity usage history** (pingdotgg#10409), and the
Cursor keychain usage toggle `cursorKeychainUsageEnabled` (pingdotgg#13714). They
read usage in `apps/server/src/usage/*UsageReader.ts`, and the toggle is
written through `server.updateSettings`, which the backend does not
dispatch. The toggle stays hidden under
`FEATURES.providerConfiguration`.
- **Android foldable controls in the Device panel** (pingdotgg#13534, pingdotgg#13574).
They sit under `FEATURES.deviceHub`, which is off.
- **Desktop and server only:** `RunningThreadKeepAlive` (pingdotgg#13554), the
desktop compile cache (pingdotgg#13501), the Linux .deb auto-updater (pingdotgg#13575),
OTLP environment variables (pingdotgg#13492, pingdotgg#13641), the heap snapshot on
SIGUSR2 (pingdotgg#13694), event-loop stall tracing (pingdotgg#13697), and the `t3 trace`
CLI (pingdotgg#13698). None of these applies to the Moatless web deployment.

## Backend behavior to consider reproducing in Moatless
Added to `docs/fork/gaps.md`, under *Runtime fixes upstream made to its
own server*:
- Settling a thread closes its idle shells: `terminal/Manager.ts`
(pingdotgg#13673)
- Usage reads Cursor, OpenCode and Antigravity history (pingdotgg#10409)
- Newer Codex models get the runtime instructions again:
`CodexDriver.ts`, `RuntimeInstructions.ts` (pingdotgg#13547)
- Background work no longer scales with every thread or project:
  - no per-minute git reruns (pingdotgg#13689)
- no thread-list rebuilds for per-thread settlement or PR checks
(pingdotgg#13691, pingdotgg#13720, pingdotgg#13693)
  - PR sync reads only threads with a linked PR (pingdotgg#13704)
  - the SQLite WAL shrinks after large writes (pingdotgg#13684)
  - shutdown no longer rewrites every stopped session row (pingdotgg#13688)
- Also: the OpenCode v2 serve ready line (pingdotgg#13651), and retrying failed
SQLite statement preparations (pingdotgg#10584)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---
Moatless task:
https://moatless.soaplabstest.com/tasks/0af5f959-42c9-4219-b6ff-2f43e9e72a5d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XS 0-9 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