Skip to content

Fix failed SQLite payload commit cleanup; document remaining OOM recovery - #65

Merged
andinux merged 8 commits into
mainfrom
codex/sqlite-apply-transaction-cleanup
Sep 22, 2026
Merged

andinux merged 8 commits into
mainfrom
codex/sqlite-apply-transaction-cleanup

Conversation

@marcobambini

@marcobambini marcobambini commented Sep 19, 2026

Copy link
Copy Markdown
Member

Payload apply can return a deferred foreign-key or busy-commit error while leaving its internally opened SQLite transaction active. Uncommitted rows remain visible and a later BEGIN fails.

This change centralizes error cleanup, rolls back a failed transaction only when apply started it from autocommit mode, preserves the original error and committed prefixes, and excludes rolled-back rows from applied statistics. It also rejects a failed group-savepoint open and fixes a primary-key-name allocation leak found by fault injection. Caller-owned transactions remain under the caller's control.

Validation

  • 100 deferred-constraint failure/retry cycles, covering final and intermediate source-version commit boundaries.
  • 30 repeated reader-blocked commits, followed by successful retry.
  • Caller transaction and savepoint preservation, unchanged checkpoints, data/metadata rollback and connection reuse.
  • Core and audit suites pass under ASan/UBSan, including the SQLite amalgamation; zero outstanding SQLite memory.
  • Independent PostgreSQL 15.19 compatibility suite: 521 checks pass.
  • PostgreSQL does not need the cleanup: apply never owns the transaction there (database_in_transaction() is always true under SPI), so the new ROLLBACK is unreachable. test/postgresql/62_deferred_fk_caller_commit.sql pins this down: a deferred foreign key lets the apply succeed and fails the caller's COMMIT with 23503, rolling back rows, metadata, checkpoint and caller work together; autocommit keeps nothing; a write failing inside a caller savepoint (P0001) leaves the caller's transaction usable, and redelivery applies. Full suite with ON_ERROR_STOP=on: 528 checks pass on standalone PostgreSQL 17, 526 on Supabase (test 39's lock-contention part skips there by design).
  • Negative control: restoring the old apply implementation fails 400 assertions in the initial deferred-constraint tests.

Remaining limitation: allocation failures (#74)

The engine-level OOM case from the report is not fully fixed and is tracked in #74. When an allocation fails mid-apply, SQLite refuses the cleanup ROLLBACK from inside the running SQL function, so 126 of the 348 swept allocation points (0–347, ordinary build) still leave the apply's transaction open. There are zero leaks, and every retry succeeds once the host rolls back. The diagnostic test/stress/payload_oom.c deliberately exits nonzero for those cases and is not part of make unittest.

docs/internal/apply-transaction-cleanup.md documents the fix, reproduction command, results and remaining recovery requirement.

This is one of three independent follow-ups to #64, now based on main (rebased after #64 was squash-merged as 49ba88e). Apart from running the e2e suite against the shared CI test tenants, no live cloud endpoint was used or modified.

CHANGELOG.md records the fix under [Unreleased]: it will ship in a later release together with the other open follow-ups, so there is no version bump here.

Cloud integration reliability

The shared chunked-tenant job is serialized across branches with a queued concurrency group. This prevents a different PR from writing during the negative-cache test's required idle phase; all idle assertions remain intact, and other platforms still run in parallel.

Fresh receivers now wait for actual received rows and expected fixture data rather than requiring the first poll to contain rows. Polling is bounded and SQL/protocol failures abort immediately. A materialized result ensures one network call per attempt. The offline integration_bootstrap test runs as part of make unittest, covering 312 scenarios including delayed/partial delivery, timeout, missing data, protocol errors and malformed responses. Local ordinary and ASan/UBSan runs pass with zero outstanding SQLite memory.

The chunked rowset and paths e2e tests now fail on receive.error instead of polling until the timeout, and a timeout reports rows present/received, elapsed time and the last receive.lastFailure. With that, the intermittent chunked rowset e2e batch ... was not received failure was traced (reproduced locally on this branch and on main): a fresh receiver first downloads the whole shared tenant history (~113k rows), which the server took ~25 s to prepare, past the old ~22 s budget. Both polls now allow 120 attempts and still stop as soon as the rows arrive; the full e2e suite passes locally.

Latest CI verification

Workflow run 35661808765 completed successfully on 1d22099: 37 jobs passed, with only the release job skipped. All chunked e2e tests passed on Linux x86_64, including the rowset test with the new polling budget.

@andinux
andinux changed the base branch from pg-fixes11092026 to main September 21, 2026 18:15
@andinux
andinux force-pushed the codex/sqlite-apply-transaction-cleanup branch from a35c0eb to 063d323 Compare September 21, 2026 18:15
andinux and others added 4 commits September 21, 2026 13:04
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
PostgreSQL apply never owns the transaction, so the SQLite failed-commit
cleanup does not apply there. Cover that: a deferred foreign key fails the
caller's COMMIT with 23503 and rolls back rows, metadata, checkpoint and
caller work together; autocommit keeps nothing; a write failing inside a
caller savepoint leaves the caller's transaction usable for redelivery.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… tenant history

The rowset and paths tests ignored the receive JSON, so an apply error
reported in receive.error looked the same as a slow server. Fail on
receive.error, and on timeout report rows present/received, elapsed time
and the last receive.lastFailure.

A fresh receiver first downloads the whole shared tenant history (~113k
rows); the server took ~25s to prepare it, past the ~22s budget, on main
too. Raise both polls to 120 attempts; they still stop as soon as the
rows arrive.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@andinux
andinux marked this pull request as ready for review September 21, 2026 22:39
@andinux
andinux merged commit 4534c21 into main Sep 22, 2026
39 of 72 checks passed
@andinux
andinux deleted the codex/sqlite-apply-transaction-cleanup branch September 22, 2026 00:27
andinux added a commit that referenced this pull request Sep 22, 2026
Test 62 is now 62_deferred_fk_caller_commit.sql, merged with #65.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.

2 participants