fix(queue): restore BackgroundQueue later(), fix after-commit handling#378
Merged
binaryfire merged 14 commits into0.4from May 1, 2026
Merged
fix(queue): restore BackgroundQueue later(), fix after-commit handling#378binaryfire merged 14 commits into0.4from
binaryfire merged 14 commits into0.4from
Conversation
… dispatchAfterCommit type Pull the ShouldBeUnique rollback registration out of enqueueUsing() into a protected addUniqueJobRollbackCallback() helper so SyncQueue, DeferredQueue, and BackgroundQueue can share it. Tighten the dispatchAfterCommit property from ?bool to bool and remove the now- redundant ?? false at the read site.
…::push Replace the inline ShouldBeUnique rollback block with the helper extracted in Queue.php. Drop the now-unused UniqueLock, Cache, and ShouldBeUnique imports. Tighten dispatchAfterCommit constructor type from ?bool to bool to match the parent property.
…dling and worker-exit safety PR #361 added Timer-based later() to the 0.3 ancestor (CoroutineQueue) but was never forward-ported to 0.4. Restore it on BackgroundQueue with the corrections discovered during review: - After-commit branch wraps the timer scheduling itself, so the delay clock starts at commit time and ShouldBeUnique rollback is honored. - Timer callback skips execution when the worker is closing ($isClosing = true) instead of running pending delayed jobs against shutdown cleanup. - Drop the push() override; inherit SyncQueue::push() polymorphically so after-commit + ShouldBeUnique rollback are handled in one place. The executeJob() override (which spawns a coroutine) is what makes push() background-execute under the hood. - Non-nullable Timer property assigned in constructor body from a nullable parameter so phpstan doesn't see ambiguity at every call site while tests can still inject a mock.
…rop push() override, standardize coroutine wrapper PR #377 added later() to DeferredQueue but bypassed the after-commit branch that push() honors, and duplicated the push() body when it could inherit. This commit: - Adds shouldDispatchAfterCommit + addUniqueJobRollbackCallback handling to later() so delayed jobs respect transaction commits and unique locks roll back correctly. - Schedules the timer inside the commit callback so the delay clock starts at commit time, matching enqueueUsing semantics on the real distributed queues. - Clamps the timer delay with max(0.0, ...) so a negative integer delay (or a past DateTimeInterface) executes immediately instead of triggering Timer::after's "wait until worker exit" branch. - Skips execution when the timer fires with $isClosing = true. - Drops the push() override; inherits SyncQueue::push() polymorphically so after-commit + ShouldBeUnique rollback handling come for free. The executeJob() override now wraps Coroutine::defer around parent::executeJob(), absorbing the old deferJob() into the override. - Switches the Coroutine import from Hypervel\Engine\Coroutine to the high-level Hypervel\Coroutine\Coroutine wrapper used by BackgroundQueue, for consistency. - Non-nullable Timer property pattern (same as BackgroundQueue).
…move dead constructor assignments Five constructor-body assignments were dead code — the four other parameters are already promoted (PHP auto-assigns), and dispatchAfterCommit was being re-promoted as ?bool which redeclared the abstract Queue's property. Un-promote dispatchAfterCommit (drop `protected`), tighten type to bool, and assign it explicitly to the inherited property. Queue is abstract with no constructor, so we cannot call parent::__construct().
…ad constructor assignments Same pattern as BeanstalkdQueue — five constructor-body assignments were dead code (other parameters auto-assign via promotion), and dispatchAfterCommit was being re-promoted as ?bool. Un-promote it, tighten to bool, and assign explicitly to the inherited Queue property.
…nector defaults, fix NullQueue return value Small cleanups grouped into one commit: - DatabaseQueue, RedisQueue: tighten dispatchAfterCommit constructor parameter from ?bool to bool to match the parent Queue property. - SqsConnector, Horizon Connectors\RedisConnector: change after_commit default from null to false. Both connectors were silently passing null into a constructor that never meant to accept it; the type tightening on the queue constructors makes this a hard error otherwise. - NullQueue::creationTimeOfOldestPendingJob(): return null instead of 0. The method's contract is "timestamp of oldest pending job, or null if none" — Laravel and our own SyncQueue both return null. The 0 was a drift; 0 is a real Unix timestamp (1970-01-01) and would fool null-checks into thinking a job exists.
…g coverage to QueueDeferredQueueTest Lock in the DeferredQueue behavior changes: - testItAddsATransactionCallbackForAfterCommitUniqueJobs and the ShouldQueueAfterCommit interface variant — covers the new ShouldBeUnique rollback handling on push() inherited from SyncQueue. - testLaterAddsTransactionCallbackForAfterCommitJobs and three variants — covers the after-commit branch added to later(), with unique-rollback × afterCommit-property × interface combinations. - testLaterClampsNegativeIntegerDelay, testLaterClampsPastDateTimeInterface — confirm Timer::after is called with 0.0 instead of negative values. - testLaterFailedJobGetsHandledWhenAnExceptionIsThrown — confirms exceptions thrown from inside later()'s deferred execution flow through to setExceptionCallback. - testLaterDoesNotExecuteJobWhenWorkerIsClosing — regression test for the worker-exit safety: when Timer::after fires the callback with $isClosing = true, the job does not execute. Also collapses the three near-identical handler classes (DeferredQueueLaterTestHandler, *IntervalTestHandler, *DateTimeTestHandler) into one, since they only differed in the $_SERVER key.
…roundQueueTest After dropping BackgroundQueue::push() in favor of inheriting SyncQueue::push() polymorphically, and after restoring later() with after-commit handling and worker-exit safety, the test file needs parity with QueueDeferredQueueTest. Adds 13 new tests: - testItAddsATransactionCallbackForAfterCommitUniqueJobs and interface variant — covers the inherited ShouldBeUnique rollback handling now reaching BackgroundQueue. - testLaterSchedulesJobWithDelay, testLaterWithDateInterval, testLaterWithDateTime — basic delay-type coverage for later(). - testLaterAddsTransactionCallbackForAfterCommitJobs plus three variants — after-commit handling in later() with unique-rollback × afterCommit-property × interface combinations. - testLaterClampsNegativeIntegerDelay, testLaterClampsPastDateTimeInterface — clamping regression tests. - testLaterFailedJobGetsHandledWhenAnExceptionIsThrown — exception flow through setExceptionCallback in later()'s coroutine. - testLaterDoesNotExecuteJobWhenWorkerIsClosing — worker-exit safety regression test.
Per porting.md, queue tests must extend Hypervel\Tests\TestCase (not raw PHPUnit\Framework\TestCase) so AfterEachTestSubscriber's global state cleanup runs and the coroutine harness is in place. QueueDelayTest was the only file in tests/Queue still on the wrong base class.
…ingJob Locks in the NullQueue::creationTimeOfOldestPendingJob() return-value fix (0 → null). Two assertions: with default queue and with a custom queue name. Prevents future regression toward the 0 sentinel.
…ommit config Smoke-tests SqsConnector::connect() with a config that omits after_commit, asserting an SqsQueue is returned without TypeError. Locks in the connector default fix (?? null → ?? false) — without that fix and with the bool-tightened SqsQueue constructor, this would throw at construction time.
…mmit config Mirrors QueueSqsConnectorTest for Horizon's RedisConnector. Mocks the Redis factory and asserts connect(['queue' => 'default']) returns a Hypervel\Horizon\RedisQueue without TypeError. Locks in the Arr::get(..., null) → Arr::get(..., false) default fix.
…ueSyncQueueTest setUp Hypervel's queue payload references Illuminate\Queue\CallQueuedHandler for cross-framework interop (see Queue::createObjectPayload). The alias is registered in QueueServiceProvider::boot, but unit tests that instantiate SyncQueue directly without booting the framework miss it, causing testFailedJobHasAccessToJobInstance and testCreatesPayloadObject to fail with "Target class does not exist" when run via plain `vendor/bin/phpunit` (paratest's bootstrap masks this through different load order). Register the alias in setUp() if it isn't already present, mirroring the same conditional class_alias pattern from the service provider. Now both the focused phpunit invocation and composer test:parallel agree.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #377 for the renamed 0.4 queue classes. 0.4 renamed
CoroutineQueue/DeferQueueto Laravel-compatibleBackgroundQueue/DeferredQueue; this finishes the delayed-dispatch work for those names and fixes a few related queue package issues I found while reviewing it.Main changes
BackgroundQueue::later()
BackgroundQueuenow has a reallater()method. Before this, it inheritedSyncQueue::later(), which just callspush()and ignores the delay.The new implementation:
Timer::after()executeJob()directly from the timer callback instead of going back throughpush()0, solater(-5, $job)runs immediatelyDeferredQueue::later()
DeferredQueue::later()now respects after-commit jobs. If the job should run after commit, the timer is scheduled inside the transaction callback, so the delay starts after commit instead of at dispatch time.Worker shutdown
Timer::after()callbacks can also run when the worker is closing.BackgroundQueueandDeferredQueuenow check$isClosingand skip delayed jobs during shutdown.These queues are still in-memory/best-effort. Use Redis, database, or SQS for durable jobs.
push() cleanup
BackgroundQueueandDeferredQueueno longer overridepush(). They inheritSyncQueue::push()and only overrideexecuteJob():BackgroundQueueusesCoroutine::create()DeferredQueueusesCoroutine::defer()This keeps after-commit and unique-job rollback handling in one place. It also fixes the case where a job using both
ShouldBeUniqueand after-commit could leave its unique lock behind on transaction rollback.Unique rollback helper
Moved the repeated
ShouldBeUniquerollback callback code intoQueue::addUniqueJobRollbackCallback(), then reused it from:Queue::enqueueUsing()SyncQueue::push()BackgroundQueue::later()DeferredQueue::later()Smaller fixes
?bool $dispatchAfterCommittoboolacross the queue package.NullQueue::creationTimeOfOldestPendingJob()to returnnullinstead of0.BeanstalkdQueueandSqsQueue.DeferredQueuefromHypervel\Engine\CoroutinetoHypervel\Coroutine\Coroutine.after_commitdefaults fromnulltofalse.QueueDelayTestto extendHypervel\Tests\TestCase.QueueSyncQueueTestself-contained by registering theIlluminate\Queue\CallQueuedHandleralias insetUp().Not changed
DatabaseQueue::bulk()still uses one multi-row insert. It still bypasses per-job after-commit, unique rollback, and queued events.That matches Laravel and keeps batch dispatch fast.
Bus\Batch::add()is the main caller here, and changing this to loop overpush()/later()would make large batches much slower.Tests
New/updated coverage:
QueueDeferredQueueTest: after-commit, unique rollback, negative delays, exception handling, worker shutdown, and handler cleanup.QueueBackgroundQueueTest: same coverage as deferred queue, plus the basiclater()delay cases.NullQueue,SqsConnector, and Horizon's Redis connector.