venture icon indicating copy to clipboard operation
venture copied to clipboard

Issue deleting Eloquent models in non Venture jobs on sync driver

Open simple-hacker opened this issue 1 year ago • 2 comments

This is a follow on from a previously closed/stale issue #65 as I am also experiencing the issue.

I have created a minimal repository on a fresh Laravel application with one test, and one job that simply deletes the user. This job does not extend WorkflowableJob or interacts with Laravel Venture.

As discussed #65 it just seems to be when delete/forceDelete on Eloquent models that are passed in to jobs, and using QUEUE_CONNECTION=sync

I think the issue only occurs when forceDeleting as this is what's happening in my application. All other jobs that do soft deleting inside jobs seem to be fine. This minimal repository, the User model does not implement SoftDeletes and so it's failing when just using ->delete()

Here is the minimal repository

laravel/laravel: v10.45.1 sassnowski/venture: v5.2.0 PHP 8.1.27

After forking you will need to run the test suit php artisan test and you should expect this simple job to fail.

image

Normal Laravel job (not Workflowable job)

image

Test fails

image

As described in #65 seems to be an issue with the Base64WorkflowSerializer.php when using QUEUE_CONNECTION=sync, but this Base64WorkflowSerializer` is interfering with non Venture jobs :/

image

simple-hacker avatar Feb 21 '24 15:02 simple-hacker

Can confirm it's just when the model is force deleted.

The results of soft deleting means the job is ran without issue, as the job is performed. Test is failing but that's because it found a soft deleted record.

image

simple-hacker avatar Feb 21 '24 16:02 simple-hacker

@simple-hacker Can confirm this is indeed an issue, as the WorkflowEventSubscriber doesn't differentiate between non-Venture workflow steps and regular queued jobs, which means it will always unserialize all queued jobs payload's regardless of it being a workflow step or not:

https://github.com/ksassnowski/venture/blob/73cdd0f05b32f161104d30f4e61c103b16845074/src/WorkflowEventSubscriber.php#L82-L91

https://github.com/ksassnowski/venture/blob/73cdd0f05b32f161104d30f4e61c103b16845074/src/WorkflowEventSubscriber.php#L96-L105

Only thing I can think of atm to prevent this would be to prevent unserializing the job if the job doesn't implement the WorkflowableJob interface:

/**
 * @param Closure(WorkflowableJob): void $callback
 */
private function withWorkflowJob(
    JobProcessing|JobProcessed|JobFailed $event,
    Closure $callback,
): void {
+   $jobName = $event->job->payload()['data']['commandName'];

+   if (! isset(class_implements($jobName)[WorkflowableJob::class])) {
+       return;
+   }

    $jobInstance = $this->jobExtractor->extractWorkflowJob($event->job);

    if (null !== $jobInstance) {
        $callback($jobInstance);
    }
}

Though this is indeed a weird edge-case, since the SerializesModels trait is sort of the culprit here, as it listens for when the job is unserialized and attempts a findOrFail on the deleted user. Unfortunately the Ignore Missing Models feature doesn't resolve this issue either (I've tested locally):

class DeleteUser ...
{
    /**
     * Delete the job if its models no longer exist.
     *
     * @var bool
     */
    public $deleteWhenMissingModels = true;

stevebauman avatar Feb 21 '24 22:02 stevebauman