feat(postgres): keep a tab's transaction alive across runs - #801
Conversation
An editor batch already runs its statements on one pooled connection, so BEGIN ... COMMIT inside a single script works. The workflow the transaction exists for did not: run BEGIN, inspect, run the changes, verify, then COMMIT, each as its own run. Between runs the connection went back to the pool, so the next run could land on a different one - the verify step saw pre-transaction data and COMMIT reported no transaction in progress, while the real one stayed open elsewhere. Worse, deadpool recycles without resetting (no RecyclingMethod is configured), so the stranded connection went back to the pool still inside the transaction, holding its locks, and the next borrower ran inside it. - common/query: transaction_effect() classifies a statement's effect on the surrounding transaction from its leading keywords. - postgres/session: registry of connections pinned per editor tab, with a ROLLBACK before any pinned connection returns to the pool and an idle sweep so an abandoned tab cannot hold locks indefinitely. - driver_trait: execute_batch_in_session() and release_session(), both defaulting to today's behaviour so only Postgres changes. - commands: execute_query_batch takes session_id and emits session-transaction-state; new release_query_session command.
Plumb the editor tab id through as a session id, forward it to plugin
drivers over RPC, release the connection when the tab closes, and show
on the tab that it is holding one.
- plugins/driver: session_id in the execute_query_batch RPC plus a
release_session call, both tolerating method-not-found so existing
plugins keep working. parse_batch_response accepts the old bare array
and the new { results, in_transaction } object, so core and plugin can
be updated in either order.
- Editor: send sessionId with every batch, track the tabs a
session-transaction-state event reports as in a transaction, show a TX
badge on them, and release the connection once a close proceeds -
after the unsaved-file prompt, so cancelling cannot discard a live
transaction.
- Tests for transaction_effect and parse_batch_response.
Running statements one at a time is exactly how a transaction is driven -
BEGIN, the changes, a verifying SELECT, COMMIT - and that path bypassed
the session entirely. Editor.tsx routes a one-statement run to runQuery,
which calls execute_query, not execute_query_batch, so each run took a
fresh pooled connection: the UPDATE auto-committed on its own connection
and the later COMMIT/ROLLBACK hit a connection with no transaction, where
PostgreSQL only warns and reports success. The TX badge stayed on from an
earlier multi-statement run with nothing to clear it, which made it look
like pinning was working.
- driver_trait: execute_query_in_session(), defaulting to execute_query.
- postgres: execute_query is now a one-statement execute_batch_in_session,
so there is a single session code path.
- plugins/driver: session_id on the execute_query RPC, and
parse_query_response accepting both the bare QueryResult and the new
{ result, in_transaction } wrapper. The wrapper is recognised by
in_transaction being present, so a query selecting a column named
'result' is not mistaken for one.
- commands: execute_query takes session_id and emits
session-transaction-state like the batch does.
- Editor: sessionId on all three execute_query call sites. Paging within
a result and copy-all-rows need it too, or they read around the tab's
own open transaction and show pre-transaction data.
|
Cross-linking from the companion PR review over in tabularis-postgresql-plugin#114: a live-DB pass over there against the plugin's The bug
if outcome.is_ok() {
match crate::drivers::common::transaction_effect(q) {
crate::drivers::common::TransactionEffect::Opens => in_transaction = true,
crate::drivers::common::TransactionEffect::Closes => in_transaction = false,
crate::drivers::common::TransactionEffect::None => {}
}
}That's correct for an ordinary statement failing mid-transaction (a bad Confirmed against real Postgres 16, with a test built around a Also worth noting since it surprised me: a The fix is likely the same in both places: a Why it's more visible here than in the plugin aloneThis PR wires |
PostgreSQL rolls the transaction back when COMMIT fails on its own (e.g. a deferred constraint), so keeping the tab pinned left the TX badge on and a connection held with no transaction behind it. Also classify ABORT, PREPARE TRANSACTION, COMMIT/ROLLBACK PREPARED and ... AND CHAIN, the last of which previously returned a connection to the pool with a fresh transaction still open.
The idle check only ran inside take(), so a tab abandoned mid-transaction kept its locks until another tab happened to run a query. A sweeper task started on the first pin now releases idle sessions every minute.
|
Same two fixes applied here to keep it in step with the plugin (31d8f71, 2fda2dc):
The host Rust still can't be built on my machine, so CI is the first compile. |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
This pass reviewed the incremental diff Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (6 files changed in this pass)
Verification limitsNo Rust compilation, no Fix these issues in Kilo Cloud Previous Review Summaries (5 snapshots, latest commit a181e67)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit a181e67)Status: 2 Issues Found | Recommendation: Address before merge Overview
This pass reviewed the incremental diff Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (4 files changed in this pass)
Verified but unchanged, findings carried above: Verification limitsThe Rust side was not compiled or exercised against a live PostgreSQL instance. Fix these issues in Kilo Cloud Previous review (commit 47a420c)Status: 6 Issues Found | Recommendation: Address before merge Overview
This pass reviewed the incremental diff Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (1 file changed in this pass)
Verified but unchanged, findings carried above: Verification limitsThe Rust side was not compiled or exercised against a live PostgreSQL instance. The two new findings are derived from the code: Fix these issues in Kilo Cloud Previous review (commit a3e9473)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (9 changed in this pass)
Verification limitsThe Rust side was not compiled or exercised against a live PostgreSQL instance in this environment. The export cancellation and savepoint findings are derived from the code, from Fix these issues in Kilo Cloud Previous review (commit e4b4f37)Status: 8 Issues Found | Recommendation: Address before merge Overview
This pass covers the 7 new commits since the last review ( The remaining findings all concern the newly added per-session locking and the new read-in-transaction path used by export and count. Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (11 changed in this pass)
Verification limitsThe Rust side was not compiled or exercised against a live PostgreSQL instance in this environment. The export paging/snapshot and savepoint-semantics findings are derived from the code and documented PostgreSQL behaviour, not from a test run. The new Fix these issues in Kilo Cloud Previous review (commit 2fda2dc)Status: 11 Issues Found | Recommendation: Address before merge Overview
The core design is sound and well tested: Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Also worth noting (not commentable inline — outside the diff)
Files Reviewed (22 files)
Verification limits: the Rust side was not compiled or exercised against a live PostgreSQL instance in this environment, so the PostgreSQL-behaviour findings above are derived from the code and documented server semantics, not from a test run. Reviewed by free · Input: 0 · Output: 0 · Cached: 0 |
Two overlapping runs from one tab each took a fresh connection, and the later store() rolled back the other's transaction. Each session now has its own lock held for the whole run, and release waits for a run still in flight. Pressing Stop aborted the run after its connection had left the session map, returning it to the pool inside the open transaction. A run's connection is now closed if the run is dropped, so the server rolls back.
Only the success path emitted session-transaction-state, so a COMMIT that failed on a deferred constraint left the TX badge on although the transaction had ended. The failure and cancel arms now ask the driver through a new session_in_transaction() and emit what it reports.
release_all was documented as running on shutdown but nothing called it.
…omment parse_query_response now recognises the wrapper by the in_transaction key alone, like parse_batch_response. The is_method_not_found doc had been spliced onto parse_query_response.
Only tabs shown as pinned were released, but that state is lost when the Editor remounts and is not yet set while a tab's first run is in flight. The backend now waits for an in-flight run before releasing, so every closing tab is released. The TX badge gets role=status so it is announced.
Row count and export ran on a pooled connection, so a tab mid-transaction counted and exported pre-transaction data, while its grid showed its own uncommitted rows. For a badged tab both now read on the pinned connection, wrapped in a savepoint so a failing read cannot abort the transaction.
|
Replied on each inline thread. Two things not covered there:
The host Rust still can't be built on my machine (no GTK), so CI is the first compile of these commits. I type-checked the new |
| .await?; | ||
| let result = match session_id { | ||
| Some(id) => { | ||
| crate::commands::read_in_open_transaction( |
There was a problem hiding this comment.
WARNING: Cancelling an in-session export closes the pinned connection and silently rolls back the tab's transaction
Every page enters postgres::execute_batch_in_session, which takes the client out of the slot for the whole batch (mod.rs:1210), and cancel_export aborts the spawned task. If the abort lands mid-page, InFlight::drop closes the connection instead of re-pinning it, so the server rolls the tab's transaction back without a ROLLBACK and the user only sees "Export cancelled". The run is therefore cancellable at exactly the point where cancelling it destroys uncommitted work. Pinning once for the whole export, rather than re-taking the slot per page, would remove the window.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed in 1d036bb. The export now borrows the pinned connection through Slot::client() while holding the session lock, instead of taking it out for a batch. A cancel only drops the row stream: the connection stays pinned and the transaction stays open. At worst it leaves the export's savepoint behind, which is harmless.
| // the timeout keeps a run still holding a session from blocking exit. | ||
| tauri::async_runtime::block_on(async { | ||
| let release = crate::drivers::postgres::session::release_all(); | ||
| let _ = tokio::time::timeout(std::time::Duration::from_secs(3), release).await; |
There was a problem hiding this comment.
SUGGESTION: The 3s budget is shared across release_all's serial slot locks, and the timeout result is discarded without a log
release_all awaits slot.lock().await one slot at a time (session.rs:155-157), so a single run still in flight consumes the whole 3 seconds and every later session is skipped — precisely the case the comment above says the timeout exists for. sweep_idle handles the same situation with try_lock and skipping busy slots. Separately, let _ = ... drops the timeout result, so a skipped release leaves no trace; the sibling run_exit_backup in this same handler logs its timeout, and matching that pattern would keep the exit path diagnosable. Also worth a comment: this calls the PostgreSQL module directly rather than going through DatabaseDriver, so no other pinning driver gets an exit sweep.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed in 7380b4f. release_all now uses try_lock and skips a session a run still holds, as sweep_idle does. That connection closes with the process anyway, so one busy tab can no longer use up the budget for the others. A timeout is now logged with log::warn!. The comment also says that only the built-in PostgreSQL driver pins, and that plugin processes release in their own shutdown handler. The plugin's release_all got the same change.
| // This tab is holding a pooled connection open, and its | ||
| // uncommitted changes are invisible to every other tab. | ||
| <span | ||
| role="status" |
There was a problem hiding this comment.
SUGGESTION: A live region inserted together with its content is not announced
The span is mounted by the transactionTabIds.has(tab.id) && conditional above, so the role="status" region does not exist before its text appears inside it. Per the live-region model (and observed NVDA/JAWS behaviour), content added along with the region is not announced, which defeats the purpose of the role. If the state change should be announced, keep the region mounted and toggle its contents; otherwise the existing title/aria-label pair already covers the static case.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed in a3e9473. The badge is back to a plain span with its title and aria-label. A single always-mounted, visually hidden role="status" region in the tab bar now holds the active tab's state, so its text changes inside an existing live region and is announced.
A PostgreSQL tab inside a transaction was exported through the generic paged path, re-running the query per page with LIMIT/OFFSET. Each page took a new snapshot, so concurrent commits could duplicate or drop rows, and a cancel mid-page closed the connection and rolled the transaction back. The export now runs as one cursor on the pinned connection, which stays pinned, inside a savepoint.
The idle sweep, which also forgets the empty slot every run leaves, only started on the first pin, so an app that never opened a transaction grew the map for its lifetime. It now starts on the first lock. On exit, release_all skips a session a run still holds instead of waiting on it, so one busy tab cannot use up the timeout, and a timeout is logged.
Deleting a notebook and closing a crashed tab call closeTab directly and skipped the release in requestTabClosure. EditorProvider now releases any tab that leaves its list, on the tab's own connection, so every path is covered, including while the Editor itself has crashed.
The badge's role=status region was mounted together with its text, which screen readers do not announce. One always-mounted, visually hidden region in the tab bar now carries the active tab's state; the badge keeps its title and aria-label.
Disconnecting closed the pool but left pinned sessions holding their transactions and locks until the idle sweep. The host now records which sessions each connection reported as inside a transaction and, on disconnect, releases them through the driver before closing the pool, so plugin drivers are covered with no protocol change. Their TX badges clear.
|
One more gap, not raised in review: disconnecting a connection closed its pool but left the tabs' pinned sessions holding their transactions and locks until the idle sweep. Fixed in the latest commit on the branch. The host now records which sessions each connection has reported as inside a transaction ( |
The theme token check rejects raw Tailwind palette classes.
…ting Disconnect released each open session inline, so it waited on any tab still running a query, and a failed health check closed the pool without releasing them at all. Both paths now share release_connection_sessions, which clears the bookkeeping, clears the badges and runs the releases in the background.
…ORK/TRANSACTION ROLLBACK WORK/TRANSACTION TO SAVEPOINT was classified as closing, and only leading comments were skipped, so COMMIT -- and chain read as chaining. Same change as the PostgreSQL plugin.
…ped pin release_all drained every slot before skipping busy ones, so a run still in flight pinned into an untracked slot and its connection went back to the pool without a ROLLBACK. Busy slots now stay in the map, and a slot dropped while holding a connection closes it.
A session-aware plugin now answers a failed statement with its error and in_transaction. The host reads that, remembers each session's last reported state, and answers session_in_transaction from it, so a plugin tab's TX badge clears after a failed COMMIT.
| match res { | ||
| Ok(value) => { | ||
| let (results, in_transaction) = parse_batch_response(value)?; | ||
| remember_session_state(session_id, in_transaction); |
There was a problem hiding this comment.
SUGGESTION: The batch path never sees the session-error shape execute_query_in_session just gained
parse_session_error is applied only to the single-statement RPC. execute_batch_in_session hands the value straight to parse_batch_response, which looks for a results key and otherwise deserialises the whole thing as Vec<BatchStatementResult>. A session-aware plugin that reports a failed batch as {error: ..., in_transaction: false} - the shape parse_session_error documents, and close to BatchStatementResult's own {result, error, execution_time_ms} - therefore surfaces as a serde message such as invalid type: map, expected a sequence instead of the plugin's error text, and the ? on the previous line short-circuits before remember_session_state, so session_in_transaction keeps the value from the previous run. That is exactly the stale TX badge this commit set out to fix, and it means the state of a batch that ends in a failed COMMIT is never learned. Hoisting the same two-line check as execute_query_in_session above parse_batch_response would cover both RPCs.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
Hey @egertaia, @aesslinger, thanks a lot to both of you for this one. The back and forth with the live-DB pass on the plugin side really paid off. I did a local review on top of the latest commit ( To me this looks mergeable. Only a few nits came up, nothing that needs to hold this PR:
@aesslinger, since the same logic lives in the plugin (#114), these could be picked up there as follow-ups, if you're both ok with that. If you agree, I'll go ahead with the merge. |
If you want to create a new issue in the plugin repo for these follow-ups we could track it that way. Defer to @debba for release/merge confirmation on this side. |
3 participants
Problem
A batch already runs its statements on one pooled connection, so
BEGIN … COMMITinside a single script works. The workflow the transaction exists for does not:Between runs the connection goes back to the pool (
execute_batchacquires at the top and drops at the end), so run 2 can land on a different connection and show pre-transaction data, and run 3 reportsthere is no transaction in progresswhile the real transaction stays open somewhere else.It is also a correctness hazard for unrelated queries. No
RecyclingMethodis configured for the Postgres pool, so deadpool's defaultFastapplies and resets nothing: the stranded connection returns to the pool still inside the transaction, holding its locks, and the next borrower runs inside it.Fix
An editor tab is a session. When a batch leaves an explicit transaction open, that tab's connection is pinned to it instead of returning to the pool, so the next run from the same tab continues the same transaction. Committing, rolling back, closing the tab, or leaving it idle for 30 minutes releases it. A pinned connection is never handed back to the pool without a
ROLLBACKfirst, so an open transaction can no longer leak into an unrelated query.Pinning is lazy: a tab that never opens a transaction behaves exactly as before and holds nothing, so N idle tabs still cost no connections.
Core
common/query:transaction_effect()classifies a statement's effect on the surrounding transaction from its leading keywords only (BEGIN,START TRANSACTION,COMMIT,END,ROLLBACK,ABORT,PREPARE TRANSACTION, and... AND CHAIN, which ends the transaction and opens a new one), so aBEGINin a string literal or a PL/pgSQL block body cannot be mistaken for transaction control.ROLLBACK TO SAVEPOINTleaves the transaction open and is classified as no-op.driver_trait:execute_query_in_session(),execute_batch_in_session()andrelease_session(), all defaulting to today's behaviour, so every driver that does not implement them is untouched.commands:execute_queryandexecute_query_batchtakesession_idand emitsession-transaction-state; newrelease_query_sessioncommand.plugins/driver:session_idgoes out in theexecute_queryandexecute_query_batchRPCs and arelease_sessioncall is added, tolerating method-not-found.parse_query_response/parse_batch_responseaccept the existing bare shapes and the new{ result | results, in_transaction }objects, so this and the PostgreSQL plugin can be updated in either order. The wrapper is recognised byin_transactionbeing present, so a query selecting a column namedresultis not mistaken for one.Postgres (built-in driver)
postgres/session: the registry of pinned connections, theROLLBACK-before-release rule and the idle sweep, which runs every minute from a task started on the first pin.postgres/mod:execute_batch_in_sessionreuses the tab's pinned connection and re-pins it if a transaction is still open. A failed ordinary statement leaves the transaction state alone (it is aborted but still open, so the user canROLLBACKfrom the same tab). A failedCOMMITis different: PostgreSQL has already rolled the transaction back, so the session is released.execute_queryis now a one-statementexecute_batch_in_session, so there is a single session code path.Editor
sessionId(the tab id) on every run, batch and single statement. The single-statement path matters most:Editor.tsxroutes a one-statement run torunQuery/execute_query, which is exactly how a transaction gets driven, so without itBEGIN, the changes andCOMMITeach landed on a different pooled connection andCOMMIT/ROLLBACKhit a connection with no transaction, where PostgreSQL only warns and reports success.TXbadge on tabs reported as in a transaction, since their uncommitted changes are invisible to every other tab.requestTabClosure, so it is hooked there once.Behaviour change worth flagging
A batch that leaves a transaction open with no session to pin it to (a non-editor caller) is now rolled back rather than handed back to the pool. That is the leak described above; nothing depended on it except by accident.
Testing
pnpm vitest run tests/utils tests/components: 3680 pass. Two intermediate runs failed on jsdom timeouts inNewConnectionModaland other component tests; I reproduced that class of failure on a cleanmaincheckout under load too, so it is pre-existing flakiness, not a regression.tsc -p tsconfig.app.json --noEmitandeslint src/pages/Editor.tsxclean.transaction_effect(opens / closes / savepoint / leading comments / PL/pgSQL body / empty),parse_batch_response(bare array, object form, missing flag) andparse_query_response(bare result, wrapper, aresultcolumn not mistaken for a wrapper).cargo checkfails ingdk-sysbefore reaching any of this code. CI is the first real build, and I have not exercised it against a live PostgreSQL instance.Follow-up
The same pinning is needed in the PostgreSQL plugin, whose
execute_query_batchhandler has the identical acquire-and-release shape. Separate PR there; this one only defines the contract and forwards the session id.