Skip to content

fix: make transaction savepoints portable - #3346

Merged
chubes4 merged 4 commits into
mainfrom
fix-3340-sqlite-savepoints
Aug 23, 2026
Merged

fix: make transaction savepoints portable#3346
chubes4 merged 4 commits into
mainfrom
fix-3340-sqlite-savepoints

Conversation

@chubes4

@chubes4 chubes4 commented Aug 23, 2026

Copy link
Copy Markdown
Member

Summary

  • use the same bare, internally generated datamachine_transaction_<integer> identifier for SAVEPOINT, RELEASE SAVEPOINT, and ROLLBACK TO SAVEPOINT
  • close scopes only after successful commit or rollback boundary closure, preserving retries after rollback/release failures
  • add executable SQLite persistence/rollback ownership coverage plus deterministic MySQL-shape and rollback failure-path coverage

Root cause and portability

wpdb::prepare( 'RELEASE SAVEPOINT %i', $name ) and the matching rollback query changed the SQL spelling from the bare identifier used by SAVEPOINT to a backtick-quoted identifier. The WordPress SQLite adapter does not preserve the expected boundary under that mixed spelling.

These names are not caller-controlled: TransactionScope constructs them exclusively from the fixed datamachine_transaction_ prefix plus an incrementing integer. Keeping that closed token bare is valid in MySQL and SQLite, avoids adapter-specific identifier quoting, and does not accept raw external identifiers.

Rollback ownership remains fail-closed: RELEASE SAVEPOINT runs only after successful ROLLBACK TO SAVEPOINT; failed rollback, failed release, and failed top-level rollback keep the scope active for retry. A fully closed scope rejects stale commits without issuing SQL.

E2E evidence

Before this change, retained fixture artifact runtime-mt5c9sh5-gcoe1j showed the pending-to-processing update followed by the job remaining pending and the Action Scheduler action completing.

The exact pinned #2578 bundle (b2669b1e59681e58997f2bf6bacfd18eb8bf230d) was rerun against this branch with deterministic OpenAI/GitHub doubles. Artifact runtime-mt5utmim-kzuc6i confirms the next independent SQLite blocker: the ordered SQL is SAVEPOINT datamachine_transaction_1, pending-to-processing UPDATE with empty last_error, then immediate ROLLBACK TO SAVEPOINT datamachine_transaction_1 and RELEASE SAVEPOINT datamachine_transaction_1. There is no release attempt before the rollback, proving this run stops because SQLite's wpdb->update() affected-row result is not exactly 1, not because the portable release spelling fails. The job therefore does not yet advance beyond pending in the full fixture; this evidence is retained rather than hidden.

Verification

  • php tests/transaction-scope-smoke.php: 0 failures, including rollback failure/retry, release failure/retry, top-level rollback retry, stale-owner rejection, and MySQL-compatible bare SQL shape
  • focused SQLite TransactionScopeTest: 3 passed, 0 failed (homeboy run 899d51da-0b1a-4947-9411-d1ef7ce57777)
  • broad SQLite gate: 1,499 total, 1,471 passed, 28 skipped, 0 failed (homeboy run 25ea0549-14b7-44db-b49f-61734c367510)
  • PHPCS on changed implementation/tests: passed
  • PHP syntax and git diff --check: passed
  • first PR MySQL shard run 32643153590: all four candidate test shard execution steps succeeded; shard 2 failed only afterward at Validate candidate Test shard terminal provenance, an infrastructure/provenance failure rather than a test failure
  • pre-PR local MySQL-equivalent run 9ffc13d3-ff81-49fe-a34b-7374de040afe: PHPUNIT_ZERO_TESTS cause=recipe_run_payload_unparseable

Fixes #3340
Refs #2578
Parent #3113

@chubes4

chubes4 commented Aug 23, 2026

Copy link
Copy Markdown
Member Author

Blocking transaction-safety finding before merge:

TransactionScope::rollback() marks the scope inactive before any rollback SQL succeeds, ignores a failed ROLLBACK TO SAVEPOINT, and then unconditionally issues RELEASE SAVEPOINT. If rollback fails but release succeeds, changes intended for rollback can survive while the scope loses its only retry path. The top-level ROLLBACK path has the same premature inactive transition.

Please keep the scope active unless the rollback boundary is successfully closed, issue RELEASE SAVEPOINT only after successful ROLLBACK TO SAVEPOINT, and add failure-path coverage for rollback failure/retry and release failure. Preserve the safe bare internally generated identifier change.

@chubes4

chubes4 commented Aug 23, 2026

Copy link
Copy Markdown
Member Author

Addressed the blocking rollback-safety finding in 0bc5964. Failed savepoint rollback now skips release and remains retryable; failed release and top-level rollback also preserve retry paths. Deterministic failure-path smoke coverage and focused SQLite integration are green.

@chubes4
chubes4 merged commit a2844a5 into main Aug 23, 2026
30 checks passed
@chubes4
chubes4 deleted the fix-3340-sqlite-savepoints branch August 23, 2026 20:36
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.

SQLite savepoint release rolls back job status transitions

1 participant