Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue deleting Eloquent models in non Venture jobs on sync driver #90

Open
simple-hacker opened this issue Feb 21, 2024 · 2 comments · May be fixed by #94
Open

Issue deleting Eloquent models in non Venture jobs on sync driver #90

simple-hacker opened this issue Feb 21, 2024 · 2 comments · May be fixed by #94

Comments

@simple-hacker
Copy link

simple-hacker commented Feb 21, 2024

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
Copy link
Author

simple-hacker commented Feb 21, 2024

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

@stevebauman
Copy link
Collaborator

stevebauman commented Feb 21, 2024

@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:

public function onJobProcessing(JobProcessing $event): void
{
$this->withWorkflowJob($event, function (WorkflowableJob $jobInstance) use ($event): void {
if ($jobInstance->workflow()?->isCancelled()) {
$event->job->delete();
} else {
event(new Events\JobProcessing($jobInstance));
}
});
}

private function withWorkflowJob(
JobProcessing|JobProcessed|JobFailed $event,
Closure $callback,
): void {
$jobInstance = $this->jobExtractor->extractWorkflowJob($event->job);
if (null !== $jobInstance) {
$callback($jobInstance);
}
}

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;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants