Skip to content

fix: make database job reservation atomic and always close the transaction - #104

Open
gr8man wants to merge 2 commits into
codeigniter4:developfrom
gr8man:fix/database-model-get-from-queue-atomicity
Open

gr8man wants to merge 2 commits into
codeigniter4:developfrom
gr8man:fix/database-model-get-from-queue-atomicity

Conversation

@gr8man

@gr8man gr8man commented Nov 10, 2026

Copy link
Copy Markdown
Contributor

Description

QueueJobModel::popWithLock() reserves a job inside a transaction, but the transaction was not closed on every code path, and a successful reservation was never verified.

  1. Transaction leak. When the SELECT failed, the method returned null without calling transComplete(). The transaction depth stayed unbalanced, so a later transComplete() from an unrelated (nested) operation could commit or roll back work it did not own, and the connection remained inside an open transaction.
  2. Unconfirmed reservations. The result of the UPDATE that changes the job status to RESERVED was discarded. A job could therefore be returned to the worker (and processed) even though the status was never actually persisted, which allows two workers to process the same job.

Changes

  • src/Models/QueueJobModel.php
    • Wrapped the reservation in try { ... } finally { $this->db->transComplete(); } so the transaction is always closed, whichever path is taken.
    • The reservation is now confirmed with update() !== false && $this->db->affectedRows() === 1. When the reservation cannot be confirmed, the transaction is rolled back and null is returned, so the job stays PENDING and is never processed.
    • An empty result set (no pending job available) returns null before the update.

Testing

Added three cases to tests/Models/QueueJobModelTest.php:

  • testPopWithLockReturnsNullAndClosesTransactionWhenQueryFails() — after a failed query, transDepth is back to 0.
  • testPopWithLockRollsBackAndReturnsNullWhenUpdateFails() — when update() returns false, the method returns null, rolls back, and the row remains PENDING.
  • testPopWithLockRollsBackAndReturnsNullWhenAffectedRowsIsNotOne() — when the update affects no rows, the same rollback and PENDING state are asserted.

Verified locally on PHP 8.5 with PHPUnit 11.5:

phpunit ....... OK (233 tests, 655 assertions, 27 skipped)
phpstan ....... [OK] No errors
psalm ......... No errors found
php-cs-fixer .. 0 of 75 files would be fixed

This branch has not been deployed

No deployments
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