fix(client): schema install must not commit or destroy a caller's transaction - #399
Conversation
…nsaction Follow-up to #398 review (P1 + P3): - The Proc-supplied shared connection can arrive mid-transaction (perform_later inside an application `transaction do`); the unconditional BEGIN was a warning no-op there, so the matching COMMIT/ROLLBACK operated on the CALLER's transaction. The framing is now transaction_status-aware: idle -> owned BEGIN..COMMIT, in-transaction -> SAVEPOINT/RELEASE with ROLLBACK TO SAVEPOINT on failure. - The process-wide serialization spec's fixed `sleep 0.05` could false-pass on a saturated scheduler; it now waits deterministically for thread B to block or terminate before asserting zero connection traffic. Refs #398
|
Running ultrareview automatically — Schema-install transaction framing is now transaction-status-aware, switching between owned BEGIN/COMMIT and savepoint-based handling; a subtle bug here could commit or truncate a caller's open transaction, and the advisory-lock/savepoint interplay is concurrency-sensitive, so this warrants a.... I'll post findings when complete. |
There was a problem hiding this comment.
Ultrareview completed in 9m 27s
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
A savepoint-path ensure rides the caller's transaction, so the install is only durable once THAT commits — caching @schema_ensured there means an outer rollback leaves the schema missing while every future check is skipped. Only the owned-COMMIT path caches now; the savepoint path re-checks on the next ensure (one SELECT). Also: the serialization spec's settle-wait now fails explicitly if thread B never settles, instead of silently degrading back into a fixed-duration window. Refs #399 review
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Confidence score: 2/5
- In
lib/pgbus/client.rb,ensure_single_queuecan cache queue existence before the surrounding transaction commits, so a rollback can leave in-memory state saying the queue exists when its DDL was undone; subsequent calls may skip creation and fail at runtime when publishing/consuming. Move the cache update to post-commit (or clear it on rollback) so cache state cannot outlive rolled-back DDL.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/pgbus/client.rb">
<violation number="1" location="lib/pgbus/client.rb:989">
P1: When the first queue creation runs inside a caller transaction, this savepoint path leaves the queue DDL in that transaction, but `ensure_single_queue` caches it before commit. If the caller rolls back, later `ensure_queue` calls skip recreation and message operations fail; defer or avoid the queue cache update until the caller transaction commits.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
Queue DDL on the shared Proc-supplied connection joins the caller's open transaction; caching @queues_created there outlives a caller rollback, so later ensures skip recreation and message operations fail against a missing queue. When the DDL rides a caller's transaction the queue is created (idempotent CREATE IF NOT EXISTS) but not cached; dedicated-path DDL runs on pgmq-ruby's own pool connections and caches as before. Refs #399 review
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
…nstall The shared Proc-supplied PG::Connection is single-owner: every access must hold @pgmq_mutex. The new transaction-status probe ran outside it, and the #397/#398 rework had swapped ensure_pgmq_schema's synchronized for the class-level install mutex — silently dropping the connection-ownership guard the original code had. Both now hold synchronized (nested inside the class install mutex; safe order, nothing acquires them reversed). Also dedupes the create+tune DDL pair shared by queue and DLQ creation. Refs #399 review
Summary
Follow-up addressing the cubic review on #398 (both findings valid, merged before I read them):
BEGIN…COMMIT/ROLLBACKunconditionally. On the Proc-supplied shared-AR path the connection can arrive mid-transaction (perform_laterinside an applicationtransaction do), whereBEGINis a warning no-op and the matchingCOMMIT/ROLLBACKcommits half of — or destroys — the caller's transaction. Framing is nowtransaction_status-aware: idle → ownedBEGIN…COMMIT; in-transaction →SAVEPOINT pgbus_pgmq_install/RELEASE,ROLLBACK TO SAVEPOINTon failure. On the savepoint path the advisory lock joins the caller's transaction until it ends — over-holding delays a concurrent installer but never corrupts it.sleep 0.05could false-pass on a saturated scheduler (an unserialized B might simply not be scheduled inside the window). It now waits deterministically until thread B blocks or terminates before asserting zero connection traffic — an unserialized B terminates with traffic recorded and fails every time.Refs #398
Test plan
transaction_statusisINTRANS;ROLLBACK TO SAVEPOINT(never bareROLLBACK) on duplicate-install; owned BEGIN/COMMIT when status is idlebundle exec rspec spec/pgbus/ spec/generators/— 3857 examples, 0 failuresbundle exec rubocopclean on changed filesSummary by cubic
Prevents PGMQ schema install from committing or destroying a caller’s open transaction and restores single‑owner access to the shared connection. Also stops caching non‑durable results so callers don’t skip checks after a rollback.
pg_advisory_xact_lock; install if missing; ROLLBACK on failure with a duplicate‑install recheck.pgbus_pgmq_install; acquire the same lock; RELEASE on success; ROLLBACK TO SAVEPOINT on failure; never issue bare BEGIN/COMMIT/ROLLBACK; do not set@schema_ensuredhere. Side effect: the advisory lock is held until the outer transaction ends.@queues_created; the next ensure re‑checks. Dedicated connections still cache.Written for commit dfac1d3. Summary will update on new commits.