Skip to content

fix(supabase, postgres): bind what was asked, survive an ended session, and never report a rolled-back commit - #70

Merged
jddunn merged 6 commits into
masterfrom
fix/review-l2-supabase
Oct 18, 2026
Merged

jddunn merged 6 commits into
masterfrom
fix/review-l2-supabase

Conversation

@jddunn

@jddunn jddunn commented Oct 18, 2026 •

Copy link
Copy Markdown
Member

Summary

Fixes for the defects a review of 0.7.16 found, red first (bf2497a: 11 of the new cases failed, 1501 passed, and vitest caught 3 uncaught Connection terminated errors), released as 0.7.17 with the mutation sync merged in #65.

  • transaction() on PostgresAdapter, on a connection withConnection() holds, and on SupabaseAdapter rejects when PostgreSQL answers its COMMIT with ROLLBACK. PostgreSQL ends the COMMIT of an aborted transaction that way, with no error (utility.c), so a callback that caught a failed statement's error and went on resolved while nothing was written.
  • SupabaseAdapter.batchWrite runs each operation under a savepoint, as PostgresAdapter.batch does, so one failed operation no longer aborts the rest and rolls the whole batch back at COMMIT.
  • SupabaseAdapter listens for connection errors on the clients it checks out (transaction() and the cache's read) and on its pool. A session the server ended emitted an error event nothing heard, which ends a Node process. The statement now rejects with its own error and code (57P01), the client is closed, and an idle client's failure is logged.
  • A transaction or cached read whose ROLLBACK could not run, as when pg's query_timeout drops it, closes its client instead of pooling it inside the transaction.
  • With the cache on, a SELECT the cache may keep is sent the values pg's own prepareValue writes when the call is made, and keyed by them. It used to bind the caller's values only once it had a client, key named parameters by the object's own enumerable fields, and key a Date by its time. On pg before 8.15.0, whose main export has no utils, a SELECT with parameters runs each time.
  • A -- comment ends at \r as well as \n, as PostgreSQL reads it.
  • Found on the way: run, get and all, and the transaction handle's get and all, answer a string of several statements as PostgresAdapter does (get null, all an empty list). pg answers such a string with a list of results, so get and run threw a TypeError after every statement had run, and all answered undefined.

package.json is 0.7.17. #68 also writes 0.7.17: whichever merges second takes 0.7.18.

Checklist

  • Tests added/updated
  • Docs updated (README or typedoc)
  • Conventional commit title/body

Related

None.

Summary by Sourcery

Harden PostgreSQL and Supabase transaction, connection, batch-write, result, and query-cache behavior so failures cannot be reported as successful or leave unsafe pooled sessions behind.

Bug Fixes:

  • Prevent transactions from reporting success when PostgreSQL rolls back an aborted transaction at COMMIT.
  • Make Supabase batch writes isolate failed operations while committing successful operations.
  • Handle terminated or unusable PostgreSQL connections safely during transactions, cached reads, and pooling.
  • Correct Supabase result handling for multi-statement queries and align cache parameter binding with PostgreSQL serialization semantics.
  • Recognize carriage returns as comment terminators when classifying cacheable SQL.

Enhancements:

  • Improve Supabase query caching so parameters are bound and keyed at call time, including named, inherited, non-enumerable, date, and PostgreSQL-specific values.

Documentation:

  • Document aborted-transaction COMMIT behavior and savepoint guidance for PostgreSQL transactions.
  • Record the 0.7.17 release and its PostgreSQL and Supabase fixes in the changelog.

Tests:

  • Add coverage for rolled-back commits, connection termination, rollback failures, resilient batch writes, multi-statement results, and parameter-aware query caching.

Chores:

  • Bump the package version to 0.7.17.

…ommits the review of 0.7.16 found wrong

With its cache on, SupabaseAdapter binds a read's parameters after it
has checked out a client, so values the caller changed after the call
were bound and the answer kept under the key of the old ones; named
parameters were keyed by their own enumerable fields, not the values
bound; a Date by its time, not the text pg binds; and a -- comment
ended only at \n.

A session the server ends while the cache's read or a transaction holds
it, or while it sits idle, emits an error nothing hears, which ends the
process. A read or transaction whose ROLLBACK could not run, as when
pg's query_timeout drops it, went back to the pool inside its
transaction. batchWrite ran its operations without savepoints, so one
failure aborted the rest and its COMMIT rolled all of them back. And a
transaction whose callback caught a failed statement resolved, in both
adapters, although PostgreSQL answered its COMMIT with ROLLBACK.
@coderabbitai

coderabbitai Bot commented Oct 18, 2026 •

Copy link
Copy Markdown

Warning

Review limit reached

  • Run on-demand review

This review includes 7 billable files and costs up to $1.75.

  • Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Or wait 40 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available. Your 91 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Repository: framerslab/sql-storage-adapter/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Team
  • Run ID: 7a8389fa-5130-42fb-af04-2d24c30bbf26
📥 Commits

Reviewing files that changed from the base of the PR and between 80b75c9 and 9195218.

📒 Files selected for processing: 7
  • CHANGELOG.md
  • README.md
  • package.json
  • src/adapters/postgresAdapter.ts
  • src/adapters/supabase.ts
  • tests/postgres.transaction-errors.spec.ts
  • tests/supabaseAdapter.spec.ts
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@sourcery-ai

sourcery-ai Bot commented Oct 18, 2026

Copy link
Copy Markdown

Reviewer's Guide

This test-focused PR adds integration regressions for SupabaseAdapter and PostgresAdapter around aborted-transaction COMMITs, batch-write isolation, terminated or timed-out sessions, and cache parameter/comment handling. The suite uses dedicated adapters, administrative backend termination, query timeouts, and post-failure usability checks to pin the defects described in the PR.

File-Level Changes

Change Details Files
Adds regression coverage for transaction outcomes when PostgreSQL implicitly rolls back an aborted transaction at COMMIT.
  • Exercises both direct transactions and transactions started from a held connection.
  • Verifies caught statement errors still cause the transaction to reject and leave no writes persisted.
  • Confirms the adapter remains usable afterward.
tests/postgres.transaction-errors.spec.ts
Adds integration tests for SupabaseAdapter resilience when sessions fail during reads, transactions, or pool idleness.
  • Tests server-terminated active sessions and ensures the original PostgreSQL error is preserved.
  • Tests idle-session termination and replacement before the next query.
  • Tests timed-out statements whose cleanup ROLLBACK cannot execute, ensuring the tainted session is discarded rather than reused inside a transaction.
  • Adds setup and cleanup helpers for dedicated adapters, administrative session termination, and query-timeout URLs.
tests/supabaseAdapter.spec.ts
Adds regression coverage for batch writes and transaction error reporting.
  • Verifies a failed batch operation is isolated while successful operations before and after it commit.
  • Verifies a callback that swallows a failed statement cannot falsely resolve as committed when PostgreSQL returns ROLLBACK at commit.
tests/supabaseAdapter.spec.ts
Adds cache correctness tests for parameter binding and SQL classification.
  • Ensures cached reads snapshot positional, named, and document parameters before asynchronous client checkout.
  • Covers non-enumerable and inherited named parameters and Date serialization under PostgreSQL driver settings.
  • Verifies carriage returns terminate PostgreSQL line comments so statements are not incorrectly classified as reads.
tests/supabaseAdapter.spec.ts

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

…n, and never report a rolled-back commit (0.7.17)

transaction() on the PostgreSQL adapter, on a held connection and on
SupabaseAdapter rejects when PostgreSQL answers its COMMIT with
ROLLBACK, as it ends an aborted transaction: a callback that caught a
failed statement's error and went on resolved with nothing written.
SupabaseAdapter.batchWrite runs each operation under a savepoint, so a
failed one no longer aborts the rest and rolls the batch back at COMMIT.

SupabaseAdapter listens for connection errors on the clients it checks
out and on its pool, so the server ending a session no longer ends the
process, and closes a client whose ROLLBACK could not run, as when pg's
query_timeout drops it, instead of pooling it inside the transaction.

With its cache on, a SELECT it may keep is sent the values pg's own
prepareValue writes when the call is made, and keyed by them: the
values bound, named ones included, and a Date as the text pg binds. A
-- comment ends at a carriage return too. run, get and all answer a
string of statements as the PostgreSQL adapter does.
@codecov

codecov Bot commented Oct 18, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.85714% with 9 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/adapters/supabase.ts 94.67% 9 Missing ⚠️

📢 Thoughts on this report? Let us know!

@jddunn
jddunn marked this pull request as ready for review October 18, 2026 04:09
@sourcery-ai

sourcery-ai Bot commented Oct 18, 2026

Copy link
Copy Markdown

Sorry @jddunn, you've used your own review budget of 250,000 diff characters for the last 7 days.

You can request another review in 14 hours and 52 minutes by commenting @sourcery-ai review. Upgrade to get a review now.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 18, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-18T04:13:02.462453Z 3fcf603 Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@qodo-free-for-open-source-projects

Copy link
Copy Markdown

PR Summary by Qodo

Fix PostgreSQL commit reporting and Supabase session, batch, and cache handling

🐞 Bug fix 🧪 Tests 📝 Documentation ⚙️ Configuration changes 🕐 40+ Minutes

Grey Divider

AI Description

• Reject transactions that PostgreSQL rolled back at COMMIT, rather than reporting success.
• Keep Supabase batches recoverable, close unsafe clients, and bind cached reads to call-time
 values.
• Cover session failures and multi-statement results with PostgreSQL tests; release as 0.7.17.
Diagram

graph TD
  Caller["Adapter caller"] --> Supabase["Supabase adapter"] --> Client["Pooled client"] --> Database[(PostgreSQL)]
  Caller --> Postgres["Postgres adapter"] --> Client
  Supabase --> Cache["Read cache"]
  Supabase --> Savepoints["Batch savepoints"] --> Client
  Database -->|COMMIT command| Supabase
  Database -->|COMMIT command| Postgres
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Migrate Supabase users to PostgresAdapter
  • ➕ Concentrates transaction and connection handling in the recommended adapter.
  • ➖ Requires a migration for existing users and does not preserve SupabaseAdapter's cache and extension behavior automatically.

Recommendation: Keep the targeted fixes for the existing SupabaseAdapter while recommending PostgresAdapter for new integrations. A migration is a reasonable longer-term simplification, but it would not address current callers without compatibility work.

Files changed (7) +572 / -183

Bug fix (2) +272 / -179
postgresAdapter.tsReject transactions rolled back at COMMIT +22/-1

Reject transactions rolled back at COMMIT

• Checks PostgreSQL's COMMIT command in the shared transaction runner, covering both pooled and held connections. A ROLLBACK response now rejects instead of returning the callback's result.

src/adapters/postgresAdapter.ts

supabase.tsCorrect Supabase transaction, client, batch, and cache behavior +250/-178

Correct Supabase transaction, client, batch, and cache behavior

• Detects rolled-back commits, isolates batch operations with savepoints, and closes clients after connection or rollback failures. Cached reads use pg-prepared values captured at call time; multi-statement results and carriage-return comments also receive correct handling.

src/adapters/supabase.ts

Tests (2) +284 / -2
postgres.transaction-errors.spec.tsTest rolled-back commits on pooled and held connections +25/-0

Test rolled-back commits on pooled and held connections

• Verifies that transactions reject when a callback catches a statement failure, and that subsequent transactions still work.

tests/postgres.transaction-errors.spec.ts

supabaseAdapter.spec.tsTest Supabase failure recovery and read semantics +259/-2

Test Supabase failure recovery and read semantics

• Adds PostgreSQL-backed cases for savepoint batches, aborted commits, ended or timed-out sessions, and multi-statement answers. Cache cases cover mutated parameters, named properties, Date formatting, and carriage-return comments.

tests/supabaseAdapter.spec.ts

Documentation (2) +15 / -1
CHANGELOG.mdDocument the 0.7.17 release +14/-0

Document the 0.7.17 release

• Records the adapter fixes and the mutation-sync functionality merged for this release.

CHANGELOG.md

README.mdExplain aborted PostgreSQL transaction outcomes +1/-1

Explain aborted PostgreSQL transaction outcomes

• Clarifies that a caught statement error can leave a transaction aborted, causing COMMIT to roll back, and recommends savepoints for statements that may fail.

README.md

Other (1) +1 / -1
package.jsonSet the package version to 0.7.17 +1/-1

Set the package version to 0.7.17

• Bumps the release version from 0.7.16.

package.json

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Oct 18, 2026 •

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. One malformed batch item rolls back all ✓ Resolved
Description
batchWrite calls batchStatementOf before entering the per-operation try, so an error while
reading or enumerating an operation’s values escapes instead of becoming a failed item. If an
operation has a throwing value getter or proxy, the outer transaction rolls back earlier successful
operations and never attempts later ones.
Code

src/adapters/supabase.ts[658]

+        const step = batchStatementOf(operations[index]);
Evidence
The new helper reads op.values and enumerates it with Object.keys and Object.values; those
operations can throw for an otherwise supplied operation. The changed loop invokes that helper
before its catch, whereas the previous SQL-building switch was inside the catch. batchWrite runs
the loop inside transaction, so an escaping error rejects the callback and rolls back the batch.

src/adapters/supabase.ts[131-159]
src/adapters/supabase.ts[651-674]
src/adapters/supabase.ts[597-611]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`batchStatementOf` now runs outside the per-operation catch. A throwing values getter or proxy aborts the entire batch rather than recording that operation as failed and continuing.
## Fix Focus Areas
- src/adapters/supabase.ts[657-670]
## Recommended Fix
Move statement construction into the per-operation error boundary, preserving the skip for unknown operation types. Ensure a failure during construction records that item's index and allows subsequent operations to run, while any savepoint opened for it is released or rolled back appropriately.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Tip of the day
💡 Did you know, you can move or hide resolved findings to focus on what still needs action

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread src/adapters/supabase.ts Outdated
… failed

batchWrite read each operation's values outside the operation's own
error boundary, so a getter or proxy that threw ended the whole batch
and rolled back the operations before it. The statement is built inside
it now: such an operation fails at its index and the batch goes on.
@jddunn
jddunn merged commit eb058f1 into master Oct 18, 2026
6 checks passed
@jddunn
jddunn deleted the fix/review-l2-supabase branch October 18, 2026 04:37
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