Skip to content

Raise on a COMMIT that the server downgrades to ROLLBACK - #131

Open
Taure wants to merge 1 commit into
erleans:mainfrom
Taure:fix/surface-aborted-commit
Open

Raise on a COMMIT that the server downgrades to ROLLBACK#131
Taure wants to merge 1 commit into
erleans:mainfrom
Taure:fix/surface-aborted-commit

Conversation

@Taure

@Taure Taure commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Problem

pgo:transaction/2,3 reports success even when the transaction was not committed.

When a statement inside the transaction errors and its error is not surfaced as an exception (e.g. a caller ignores the {error, _} return of pgo:query/1,2), PostgreSQL puts the transaction block into the aborted state. A subsequent COMMIT is then downgraded by the server to a ROLLBACK, and PostgreSQL returns the ROLLBACK command tag.

Today both command tags are treated identically:

case pgo_handler:extended_query(Conn, "COMMIT", [], ...) of
    #{command := commit}   -> Result;
    #{command := rollback} -> Result   %% aborted txn: rolled back, but returns Result anyway
end

So the caller receives the function's return value as if everything committed, when in fact the whole transaction was rolled back. That is a silent data-integrity footgun: metrics/audit/side-effects downstream record work that never persisted.

Fix

Raise transaction_rolled_back when the COMMIT comes back as a ROLLBACK. This is consistent with the existing behaviour of pgo:transaction, which already raises (re-raises) when the fun itself throws, so both "the transaction did not commit" paths now behave the same. The rollback outcome is raised after the after block (checkin/erase), so the connection is still returned to the pool cleanly and no spurious extra ROLLBACK is issued.

Tests

Added to pgo_basic_SUITE (clear group):

  • transaction_returns_value - the happy path still returns the fun's value and commits inner queries.
  • transaction_aborted_raises - an ignored in-transaction error causes pgo:transaction to raise transaction_rolled_back, and the pool/connection remains usable afterward.

Validation note

I could not run the full CT suite locally: a transitive test dependency (chatterbox, via opentelemetry) fails to compile on OTP 28/29 (deprecated prefix-catch), unrelated to this change. The library itself compiles cleanly, and I verified the behaviour end-to-end against a live PostgreSQL - a normal transaction returns its value, an aborted transaction now raises transaction_rolled_back, and the pool is healthy afterward.

Note on the API choice

I went with raising because it matches the existing fun-exception path and surfaces the failure loudly. An alternative that stays within the current any() | {error, any()} return type would be to return {error, transaction_rolled_back} instead. Happy to switch to the non-raising variant if you would prefer to preserve the current non-raising contract for aborted commits.

When a statement inside a transaction errors and the error is not surfaced
as an exception, PostgreSQL aborts the transaction block and downgrades a
subsequent COMMIT to a ROLLBACK, returning the ROLLBACK command tag. pgo
treated both tags identically and returned the fun's value, reporting
success for a transaction that did not commit.

Raise transaction_rolled_back in that case, consistent with the existing
fun-exception path which already re-raises. The rollback is raised after
the after-block so the connection is still checked in cleanly.

Adds transaction_returns_value and transaction_aborted_raises to
pgo_basic_SUITE.
@Taure
Taure force-pushed the fix/surface-aborted-commit branch from ab46dbd to 1a28c2b Compare July 5, 2026 06:46
Taure added a commit to Taure/shigoto that referenced this pull request Jul 5, 2026
* fix: harden transactional enqueue (close telemetry hole, honour pool option)

Close the deferred-telemetry hole: when a statement inside a shigoto:transaction
aborts the block and the caller ignores the error, PostgreSQL downgrades the
COMMIT to a ROLLBACK. pgo now raises transaction_rolled_back in that case
(erleans/pgo#131), so shigoto's existing catch clause discards the deferred
job_inserted events and re-raises. No job is enqueued and no telemetry fires.

Honour the transaction/2 :pool option: insert/1,2 and insert_all/1,2 now run on
the transaction's pool (threaded via the process-dictionary accumulator) instead
of always resolving shigoto_config:pool/0, so a non-default pool no longer trips
pgo's {in_other_pool_transaction, _}. Nested transactions inherit the outer pool.

Temporarily pin pgo to the Taure/pgo fork branch (v0.20.0 + the fix) until
erleans/pgo#131 merges and a hex release is cut; then revert to a hex version.

Tests: commit-rollback emits no telemetry, nested-transaction single flush,
explicit-pool transaction. Doc nits: ~"" sigil in the example, drop redundant
comments.

* fix: address review — full pool threading + discriminating pool test

- Thread txn_pool/0 through new_batch/1, get_batch/1, get_job/1 and
  report_progress/2 so batch creation and reads participate in a custom-pool
  transaction (previously they hit the default pool and would trip pgo's
  in_other_pool_transaction guard mid-transaction).
- Nested transaction/2 now inherits the outer pool from the accumulator
  instead of recomputing a pool from its own opts (which pgo ignores).
- Strengthen test_transaction_pool_option with a rollback assertion that
  actually pins pool threading, and factor out start_pool2/0.

Revert-pin tracked in #35.
Taure added a commit to Taure/shigoto that referenced this pull request Aug 3, 2026
Replaces the moving {branch, "fix/surface-aborted-commit"} pin with a
real annotated tag. The branch pin was the last place in the fleet that
tracked a moving ref, so every build here was reproducible only by
accident.

The tag is a strict superset of the branch it replaces: same aborted
COMMIT handling (erleans/pgo#131), plus erleans/pgo#116, #119 channel-name
escaping, tcp_closed handling, #124 query_timeout, prepared statements
and the #127 type exports. It deliberately excludes upstream
erleans/pgo#121, #122, #123 and #125.
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