Skip to content

fix(plugin-sqlite): report a query that stops early instead of returning the rows it got - #2355

Merged
datlechin merged 1 commit into
mainfrom
fix/sqlite-first-table-load
Aug 21, 2026
Merged

fix(plugin-sqlite): report a query that stops early instead of returning the rows it got#2355
datlechin merged 1 commit into
mainfrom
fix/sqlite-first-table-load

Conversation

@datlechin

Copy link
Copy Markdown
Member

Found while investigating #2342. This does not close that issue: the reported hang did not reproduce here, and what it fixes is a different, measured defect on the same path. See the note at the end for where #2342 stands.

The defect

All three sqlite3_step loops in the SQLite driver were written as while sqlite3_step(stmt) == SQLITE_ROW, so the code that ends the loop was never looked at. SQLITE_ROW is one of six things a step can return, and every other one exits the loop the same way a clean finish does.

The result is that a query that failed halfway, or never started, is reported as a successful result containing whatever rows were read before it stopped:

  • A database locked by another process returns zero rows and no error, so the grid renders an empty table.
  • An I/O error partway through a scan on a network volume returns the rows read so far as the complete table.
  • A corrupt page does the same.

Measured against the system SQLite (3.54.0, which is what project.yml:526 links), with a second handle holding BEGIN EXCLUSIVE while a prepared statement steps:

PROBE rows=0 terminatingCode=5 isDone=false
PROBE oldBehaviour: returns a successful result with 0 rows and no error
PROBE newBehaviour: throws 'database is locked'

The prepare call was already checked, which is why this only shows up when the lock is taken after the statement is prepared, or when the failure happens mid-scan.

The fix

Each of the three loops keeps its terminating code and reports anything that is not SQLITE_DONE as the error SQLite gives for it. The buffered paths throw; the streaming path finishes its continuation with the error rather than completing normally, after yielding what it had. Hitting the row cap is still a clean truncation, not an error.

Verification

  • xcodebuild -scheme TablePro build: passes. The SQLite driver is a bundled plugin, so this compiles it.
  • The probe above, before and after.
  • New UI test SQLiteFirstTableLoadUITests: opens the sample SQLite database, clicks a table, and asserts the rows arrive and the "Executing…" indicator clears. It passes on main, which is part of why SQLite: Clicking a table immediately after opening DB hangs on "Executing…" #2342 is still open; it is here so the flow the issue describes has a test at all.

No unit test: the SQLite driver has no test target, its sources are not in TableProTests, and the actor that owns these loops is private to the plugin. Making it testable means putting the plugin's sources in the test target, which is a change worth making on its own rather than inside this one.

Where #2342 stands

Not reproduced, so not fixed. What was ruled out, all by measurement:

  • The driver's connect path. sqlite3_open is lazy and took 0.1ms; the file is untouched until the first statement.
  • A second connection blocking the first. TablePro opens a pooled metadata connection alongside the browse query, so this was the leading theory. An uncommitted writer does not block a second handle's reads in rollback-journal mode, and WAL never blocks them.
  • Row count. Nothing in the driver treats an empty table differently, which matches the reporter saying an empty table hangs the same way.

What is still open, and what would settle it: whether the stall is in the app's execution-ownership state (retireQueryTask clears the toolbar only for the claim that installed it) or in a wait that never returns. Both would need the reporter's own database, or the TableLoadTracer output from a session where it happens.

Two things worth fixing separately, both confirmed while investigating:

  1. sqlite3_interrupt does not break a busy wait, measured. Stop therefore cannot end a lock wait, only the app's own state, so the driver keeps waiting out the busy timeout with nothing to stop it. A sqlite3_busy_handler that checks a cancel flag would make Stop reach it.
  2. Every non-parameterized DML statement reports "0 rows affected": executeUserQuery returns a hardcoded rowsAffected: 0 for the streaming path, and sqlite3_changes is only called on the two buffered ones. MySQL and PostgreSQL report the true count, so SQLite is the only driver that tells a user their DELETE did nothing.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@datlechin
datlechin merged commit d4f39cd into main Aug 21, 2026
8 checks passed
@datlechin
datlechin deleted the fix/sqlite-first-table-load branch August 21, 2026 15:59
@datlechin

Copy link
Copy Markdown
Member Author

One more finding, which invalidates the coverage this area already had.

SampleDatabaseLauncher.open finishes by routing .openTable(table: "Track"), so opening the sample database opens a table for you. Every UI test built on launchWithSampleDatabase therefore clicks its table against a connection that has already connected, fetched metadata and run a query: it tests the second query, never the first. MainContentCoordinator.swift:1249-1292 only spawns the concurrent schema fetch when the table's metadata is uncached, which is exactly the first-open state #2342 describes and the one that route skips.

I could not get a cold session in the harness. A relaunch restores the last session rather than showing the welcome window, and after File > Close Connection the welcome window's connection row would not resolve by value, label, identifier, or a descendants-of-any predicate. Rather than leave a red test in this PR, the suite keeps the case that passes and the finding is written into its doc comment, so the next person does not spend the same three runs discovering that the launch route cannot reach the reported state.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant