fix(plugin-sqlite): report a query that stops early instead of returning the rows it got - #2355
Conversation
…ing the rows it got
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
One more finding, which invalidates the coverage this area already had.
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. |
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_steploops in the SQLite driver were written aswhile sqlite3_step(stmt) == SQLITE_ROW, so the code that ends the loop was never looked at.SQLITE_ROWis 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:
Measured against the system SQLite (3.54.0, which is what
project.yml:526links), with a second handle holdingBEGIN EXCLUSIVEwhile a prepared statement steps:The
preparecall 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_DONEas 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.SQLiteFirstTableLoadUITests: opens the sample SQLite database, clicks a table, and asserts the rows arrive and the "Executing…" indicator clears. It passes onmain, 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:
sqlite3_openis lazy and took 0.1ms; the file is untouched until the first statement.What is still open, and what would settle it: whether the stall is in the app's execution-ownership state (
retireQueryTaskclears 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 theTableLoadTraceroutput from a session where it happens.Two things worth fixing separately, both confirmed while investigating:
sqlite3_interruptdoes 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. Asqlite3_busy_handlerthat checks a cancel flag would make Stop reach it.executeUserQueryreturns a hardcodedrowsAffected: 0for the streaming path, andsqlite3_changesis only called on the two buffered ones. MySQL and PostgreSQL report the true count, so SQLite is the only driver that tells a user theirDELETEdid nothing.