Repository navigation
fix: close the 25P02 cascade for query_typed/execute_typed runtime errors inside an open transaction (#181) - #184
Merged
Conversation
…rors inside an open transaction (#181) Follow-up to #170. #170 closed the 25P02 transaction-abort cascade for the prepare-failure path: a prepare Parse-error inside an open transaction aborts it, and #170's short-circuit surfaces the real error + ROLLBACKs + signals `txn_aborted=true` so the caller forces `in_transaction=false` and releases the connection instead of re-pinning it. But a *runtime* error at execute time (division by zero, a constraint violation at fetch, a 42P01 on a relation dropped between prepare and fetch) — or a failed DDL/write on the `execute_typed` path — also aborts the transaction, and #170 deliberately left those two error arms returning `txn_aborted: false` (documented as a scope boundary by the `QueryOutcome` struct doc). The result: the aborted connection is re-pinned, and the next batch's first statement 25P02s until an explicit ROLLBACK — the same cascade from a different source. Three independent reviewers on PR #180's pre-handoff ensemble flagged it. Fix: * New pure helper `error_aborted_open_txn` (with inner `error_aborts_open_txn_ inner` for unit-testability — a `DbError` can't be constructed without a real server response, the same constraint that drove `pooler_error_lead`'s extraction). A server `DbError` (any ERROR-class response — WARNING/NOTICE never arrive as a query `Error`, they're delivered out-of-band as `AsyncMessage::Notice`) always aborts an open transaction. Non-`DbError` errors (transport/timeout) are conservatively assumed to have aborted it; the rare `FromSql` over-fire (the txn is untouched, but `Kind` is private in tokio-postgres so it's indistinguishable) is the accepted cost of not poisoning the pool on the more common Timeout/Closed/Io cases. Returns `false` when not `in_transaction`. * The `execute_typed` early-return and the `query_typed` error arm classify on the raw `tokio_postgres::Error` (captured before `map_err`), ROLLBACK the now-dead transaction, append the "rolled back" notice (matching #170's wording), and signal `txn_aborted=true` so the caller forces `in_transaction=false`. * Gate on `transaction_effect(query) != Closes`: a failed `COMMIT`/`ROLLBACK`/ `END` already ends the transaction server-side and is handled by the existing `Closes` path in `in_transaction_after_outcome` — without the gate, #181 would issue a spurious no-op ROLLBACK and a misleading "rolled back" notice for a transaction COMMIT already rolled back. The gate preserves `commit_that_fails_on_a_deferred_constraint_releases_the_session` exactly. * Keep `should_short_circuit_on_prepare_err` (#170) and `error_aborted_open_txn` (#181) as separate helpers — they answer different questions (short-circuit-before vs classify-after) and take different inputs; cross-referenced in their doc comments as the two faces of the 25P02 cascade. The `#126` cancel contract and the recently-shipped pooler work (#168/#172/#174) are untouched — the fix changes only the runtime-error path; the prepare-success path is byte-for-byte unchanged (the `rows` restructure is type-level only). Tests: * Unit: `error_aborts_open_txn_inner_truth_table` (the 4-case truth table) and `error_aborted_open_txn_timeout_error_conservative` (exercises the real outer fn on a constructible non-DbError via `__private_api_timeout`). * Live (`#[ignore]`, `PG_PLUGIN_TEST_HOST`): the headline `query_typed_runtime_error_in_open_transaction_does_not_cascade` (BEGIN → SELECT 1/0 → SELECT 1; written RED first — confirmed it failed with a 25P02 cascade on the follow-up against current main, then green), `execute_typed_failure_in_open_transaction_does_not_cascade` (BEGIN → INSERT into nonexistent → SELECT 1), and a direct-connection variant. Verified: lib+bins 445, query_tests 47/47 (45 prior + 2 new), client_live_tests 19/19 (17 prior + 2 new), live_db 39/39 against the pgvector DB, the #126 cancel contract, the failed-COMMIT `Closes`-gate guard, worker_starvation 2/2, shutdown 3/3, clippy, fmt — all green.
aesslinger
added
the
prerelease:rc
Version suggestionBased on this PR's title (
This is informational only — no tag or release is created automatically yet. |
Six findings from the 12-agent pre-handoff ensemble, all fixed: 1. (HIGH, validation-coverage-gaps + docs-vs-code-drift) The `debug_assert!` in `run_pinned_body` panicked in debug builds on a #181 runtime error. The assert was written for #170 and reasoned only about the prepare-failure short-circuit, but #181's `query_typed` runtime-error classification sets `txn_aborted=true` on the same call (`SELECT 1/0` on a transaction pooler: `run_pinned_body`'s own `prepare` succeeds, `query_typed` 22012s at execute → `txn_aborted=true` → `debug_assert!(!outcome.txn_aborted)` panics). Removed the stale assert and updated the comment to explain #181's `txn_aborted=true` is intentionally ignored on the one-shot path — `pin_pooled_txn` owns the transaction lifecycle and ROLLBACKs on `Err` regardless. 2. (LOW, 6 corroboring sources — correctness-logic, backward-compat, docs x2, code-review, boundary) The `transaction_effect(query) != Closes` gate missed `Chains` (`COMMIT AND CHAIN`/`ROLLBACK AND CHAIN`/`ABORT AND CHAIN`). A failed `Chains` verb ends the transaction server-side just like a failed `Closes` (per `in_transaction_after`'s `(Chains, false) => false` arm), yet passed the gate and got a spurious no-op ROLLBACK + a misleading "rolled back" notice. Widened both gates to `!matches!(transaction_effect(query), Closes | Chains)`. 3. (docs-vs-code-drift) The `QueryOutcome` struct doc was stale after #181 — said `txn_aborted` is `true` "only" for the #170 prepare-failure short-circuit and listed "a `query_typed`/extraction error" as a path where it's `false`. After #181 both are false. Rewrote the doc to enumerate the two `true` cases (#170 prepare + #181 runtime) and the gate, plus a note on the one-shot path's intentional ignore. 4. (docs-vs-code-drift) `exec_query_on_client`'s `in_transaction` param doc mentioned only #170; updated to mention both #170 and #181. 5. (docs-vs-code-drift) `in_transaction_after_outcome`'s attribution updated from "(#170)" to "(#170 prepare-failure + #181 runtime-error)". 6. (docs-vs-code-drift) Stale line-number refs in two test comments replaced with symbolic references (the `else` branch of `if prepare_err.is_some()`; the `if !returns_result_set` early-return) so they don't rot on future shifts. No behavior change beyond widening the gate to exclude `Chains` (the spurious ROLLBACK + misleading notice on a failed `COMMIT AND CHAIN` is now correctly suppressed). The release-mode behavior of finding 1 was already functionally correct (the assert is a no-op in release); removing it only fixes the debug crash. Verified: lib+bins 445, query_tests 47/47, client_live_tests 19/19 (debug build — exercises the formerly-panicking `run_pinned_body` path), live_db 39/39 against the pgvector DB, the #126 cancel contract, the failed-COMMIT `Closes`-gate guard, worker_starvation 2/2, shutdown 3/3, clippy, fmt — all green.
aesslinger
marked this pull request as ready for review
October 29, 2026 17:58
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (1 file in incremental diff)
Fix these issues in Kilo Cloud Previous Review Summaries (2 snapshots, latest commit c411641)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit c411641)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (3 files)
Fix these issues in Kilo Cloud Previous review (commit 719ab5f)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (3 files)
Reviewed by claude-sonnet-5.5 · Input: 258 · Output: 1.3K · Cached: 137.4K |
…#181) Closes the test-coverage gap the pre-handoff ensemble's test-coverage-of-new- branches agent flagged: the Closes/Chains gate's *negative* behavior (a failed COMMIT/ROLLBACK/END/COMMIT AND CHAIN issued via the batch path while in_transaction=true must NOT receive the spurious ROLLBACK + "rolled back" notice) was uncovered by any test. The existing `commit_that_fails_on_a_deferred_constraint_releases_the_session` test uses the single-statement `execute_query` RPC, which passes `in_transaction=false` — so the gate is never evaluated and its assertion (`commit["error"].is_string()` only) would still pass if #181 wrongly appended "rolled back" to a failed COMMIT. The new `failed_commit_in_batch_does_not_get_spurious_rolled_back_notice` test exercises the gate via the batch path: `["BEGIN", "INSERT (violating)", "COMMIT"]` as a session-pinned batch on a transaction pooler, so the COMMIT fails with a deferred-FK violation while `in_transaction=true`. It asserts: - the COMMIT's error carries the real FK-violation SQLSTATE, not a 25P02 cascade; - the COMMIT's error does NOT contain "rolled back" (the load-bearing negative assertion — fails if the gate's `Closes | Chains` conjunct is deleted); - the follow-up `SELECT 1` succeeds (the Closes path released the connection, no re-pin of an aborted connection). If someone deletes the `&& !matches!(transaction_effect(query), Closes | Chains)` conjunct from either #181 error arm, this test fails — catching the regression. Verified: client_live_tests 20/20 (19 prior + this), query_tests 47/47, lib+bins 445, clippy, fmt — all green.
…r || true` tautology (#181) Two kilo-code-bot findings on PR #184: 1. (real bug) The `query_typed`-arm gate comment was garbled — my edit to widen the gate from `!= Closes` to `!matches!(…, Closes | Chains)` left the old `!= Closes` wording's trailing lines in place, producing a self-contradicting comment that repeated "batch's first statement). Gate on `transaction_effect(query)`" and stated both `!= Closes` and `neither Closes nor Chains`. Removed the stale fragment; the comment now reads cleanly with only the `Closes | Chains` wording. 2. (cosmetic, no behavior change) `error_aborts_open_txn_inner`'s `is_db_error || true` collapses to `true`, making `is_db_error` a no-op at the call site. The bot flagged it reads as a typo for `&&`/`!`. It is deliberate — the param is kept so the `error_aborts_open_txn_inner_truth_ table` unit test can assert the intent for all 4 `(is_db_error, in_transaction)` cases (both error classes abort when `in_transaction`, not just one). Expanded the comment to state the tautology is intentional, not a typo, and that the outer `error_aborted_open_txn` still threads `as_db_error().is_some()` so the discriminator is exercised end-to-end. No code change for this finding — the tautology is the cost of the testable truth table. Verified: client_live_tests 20/20, query_tests 47/47, clippy, fmt — all green.
aesslinger
added a commit
that referenced
this pull request
Oct 29, 2026
Bumps version to 1.0.0-rc.13 in Cargo.toml, Cargo.lock, .tabularium, and CHANGELOG.md. ### Included since rc.12 **Fixed:** - A `prepare` failure inside an open transaction no longer cascades to a confusing 25P02 on every subsequent statement in the same session (#170, #180). The fix gates the `.ok()` prepare-swallow on `in_transaction`: inside an open transaction it short-circuits (ROLLBACK + surface the prepare's real SQLSTATE + signal `txn_aborted=true`); outside a transaction the recoverable swallow is preserved (a 42P05 on a real PgBouncer bare batch). The error message tells the user the transaction was rolled back. - A `query_typed`/`execute_typed` runtime error inside an open transaction no longer re-pins the aborted connection and 25P02-cascades the next statement (#181, #184). The sibling of #170 from a different source: classifies on the error's `as_db_error()` (a server `DbError` always aborts an open transaction; non-`DbError` conservatively assumed aborted). Both error arms ROLLBACK + surface the real error + signal `txn_aborted=true`, gated on `transaction_effect(query)` being neither `Closes` nor `Chains` so a failed `COMMIT`/`COMMIT AND CHAIN` doesn't get a spurious ROLLBACK + notice. Closes the 25P02 cascade end-to-end (#170 prepare-failure + #181 runtime-error = the two faces). **Dependencies:** - `uuid` 1.28.0 → 1.29.0 (#182). Additive only (versioned `NonNilUuid` constructors); the existing `Uuid` API is untouched. - `tokio` 1.53.3 → 1.54.1 (#183). The 1.54.1 patch reverts the only `time`-subsystem change from 1.54.0, so `tokio::time::timeout` behavior is back to 1.53.3-equivalent — the bound the #170/#181 fixes rely on. MSRV bumped to 1.85. **Docs:** - Centered the PostgreSQL banner, converted to PNG; fixed a stale Tabularis logo URL (#179).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #181 — the sibling of #170 (merged). #170 closed the 25P02 transaction-abort cascade for the prepare-failure path. This PR closes it for the runtime-error path: a
query_typed/execute_typederror at execute time inside an open transaction also aborts the transaction, but #170 deliberately left those two error arms returningtxn_aborted: false(documented as a scope boundary by theQueryOutcomestruct doc). The result: the aborted connection was re-pinned, and the next batch's first statement 25P02-cascaded until an explicitROLLBACK— the same cascade from a different source. Three independent reviewers on PR #180's pre-handoff ensemble flagged it.The fix
error_aborted_open_txn(with innererror_aborts_open_txn_innerfor unit-testability). A serverDbError(any ERROR-class response — WARNING/NOTICE never arrive as a queryError, they're delivered out-of-band asAsyncMessage::Notice) always aborts an open transaction. Non-DbErrorerrors (transport/timeout) are conservatively assumed to have aborted it; the rareFromSqlover-fire (the txn is untouched, butKindis private in tokio-postgres so it's indistinguishable) is the accepted cost of not poisoning the pool on the more commonTimeout/Closed/Iocases. Returnsfalsewhen notin_transaction.execute_typedearly-return and thequery_typederror arm classify on the rawtokio_postgres::Error(captured beforemap_err), ROLLBACK the now-dead transaction, append the "rolled back" notice (matching Direct-path pre-resolve .ok() swallow can reintroduce the hang; no plugin-level timeout (asymmetry with pooler path) #170's wording), and signaltxn_aborted=trueso the caller forcesin_transaction=falseand releases the connection.transaction_effect(query) != Closes— a failedCOMMIT/ROLLBACK/ENDalready ends the transaction server-side and is handled by the existingClosespath inin_transaction_after_outcome. Without the gate, Runtime query_typed/execute_typed errors inside an open transaction still re-pin the aborted connection (the #170 cascade class, from a different source) #181 would issue a spurious no-op ROLLBACK and a misleading "rolled back" notice for a transaction COMMIT already rolled back. The gate preservescommit_that_fails_on_a_deferred_constraint_releases_the_sessionexactly.should_short_circuit_on_prepare_err(Direct-path pre-resolve .ok() swallow can reintroduce the hang; no plugin-level timeout (asymmetry with pooler path) #170) anderror_aborted_open_txn(Runtime query_typed/execute_typed errors inside an open transaction still re-pin the aborted connection (the #170 cascade class, from a different source) #181) as separate helpers — they answer different questions (short-circuit-before vs classify-after) and take different inputs; cross-referenced in their doc comments as the two faces of the 25P02 cascade.The
#126cancel contract and the recently-shipped pooler work (#168/#172/#174) are untouched — the fix changes only the runtime-error path; the prepare-success path is byte-for-byte unchanged (therowsrestructure is type-level only).Tests
src/handlers/query_tests.rs):error_aborts_open_txn_inner_truth_table(the 4-case truth table),error_aborted_open_txn_timeout_error_conservative(exercises the real outer fn on a constructible non-DbError via__private_api_timeout).src/client_live_tests.rs,#[ignore],PG_PLUGIN_TEST_HOST): the headlinequery_typed_runtime_error_in_open_transaction_does_not_cascade(BEGIN →SELECT 1/0→SELECT 1; written RED first — confirmed it failed with a 25P02 cascade on the follow-up against currentmain, then green),execute_typed_failure_in_open_transaction_does_not_cascade(BEGIN →INSERT INTO nonexistent→SELECT 1), and a direct-connection variant.Verification (all local —
client_live_testsisn't in CI)query_tests47/47 (45 prior + 2 new),client_live_tests19/19 (17 prior + 2 new)live_db39/39 against the pgvector DB, the#126cancel contract, the failed-COMMITCloses-gate guard,worker_starvation2/2,shutdown3/3, clippy, fmt — all greenNotes
client_live_testssuite is not in CI (it needsPG_PLUGIN_TEST_HOST), so this PR's regression guarantee is local-verification until CI runs the unit tests + non-ignoredlive_db.