Closed andrej-schaefer closed 1 year ago
Heya, thanks for reporting.
We'll need more info and/or code to debug this further. Can you please create a repository with the command below, commit the code that reproduces the issue as one separate commit on the main/master branch and share the repository here? Please make sure that you have the latest version of the Laravel installer in order to run this command. Please also make sure you have both Git & the GitHub CLI tool properly set up.
laravel new bug-report --github="--public"
Please do not amend and create a separate commit with your custom changes. After you've posted the repository, we'll try to reproduce the issue.
Thanks!
Is this reproducible when the serializer
and compression
options are disabled?
@tillkruss seems that works:
My first thought is Lua scripts. They are used by the Queue component, but will disregard any serializer
or compression
options that PhpRedis may use. So PhpRedis may set queue:job-123
to {foo: "bar"}
and then the Lua script would consider it gibberish and fail.
For this scenario the only solution is: Don't use data mutations for queue
Redis connections.
Alternatively, refactor everything to not use Lua (not a bad idea) and use atomic MULTI
transactions.
It may be good to actually unset the two Redis::OPT_*
options on the connection in there queue module.
@tillkruss nice finds. Thanks!
@andrej-schaefer are you willing to work on a PR?
@driesvints im sorry but im not Im not so deep in laravel that i can somehow find the error fast enough, and I have very limited time What i can offer is discussion and analysis on proposals :)
BTW: do i have to create the commit for the reproduction if it is already reproduced :), due to my limited time it will take a while. And keep in mind the second error regarding the DB constraint on unique jobs and multiple fails (If you want i can prepare a ticket for it separate) Or you say it is not a bug and i have misunderstood the documentation for unique jobs?
No worries. I'll keep this open then and see if anyone can help here.
Thank you for reporting this issue!
As Laravel is an open source project, we rely on the community to help us diagnose and fix issues as it is not possible to research and fix every issue reported to us via GitHub.
If possible, please make a pull request fixing the issue you have described, along with corresponding tests. All pull requests are promptly reviewed by the Laravel team.
Thank you!
And keep in mind the second error regarding the DB constraint on unique jobs and multiple fails (If you want i can prepare a ticket for it separate) Or you say it is not a bug and i have misunderstood the documentation for unique jobs?
Maybe that's something for the docs?
I mean, when creating unique jobs through ShouldBeUnique interface it means it is unique for the current execution, that multiple workers are not picking a job with the same jobId in parallel? If so then if the same job with the same jobId fails multiple times (f.e. first run fail, second run succeed, third run fail again) it will result in a unqie constraint error I think it is solvable by using the WithoutOverlapping middleware due to this not effect the jobID. As I said, maybe i misunderstand the ShouldBeUnique interface, but even then it is a bug (I think :) )
@tillkruss : Would this be something relay.so, being the successor of phpredis, would/could handle differently?
Besides, I'd guess there's quite a connection to https://github.com/laravel/framework/pull/40569 & https://github.com/laravel/framework/pull/41718 , which sadly wasn't tackled back than.
@Huggyduggy: Theoretically, yes. But I think it's much better to unset any compression/serializer on the queue
Redis connection, that will solve it and it's hard to do, just needs a PR?
Thanks @tillkruss - I've drafted a PR, fixing the same issue as reported by @andrej-schaefer. I'll submit it against the current 10.x branch tomorrow.
Sad that it's prohibiting using compression within queues, guess we'll stick to manually compressing our larger objects within our event listeners & jobs prior to dispatching 😌
Can somebody advise whether this issue would also affect Laravel Horizon. I can't reproduce the issue from a quick test.
Looks like Taylor noted he'd appreciate a note being added to the docs: https://github.com/laravel/framework/pull/48180#issuecomment-1699685697.
Laravel Version
10.13.1
PHP Version
8.2.6
Database Driver & Version
No response
Description
We encountered problems with job uniqueness. The jobs, which are configured to have a maximum of 1 attempt, failed multiple times and couldn't be written to the failed_jobs table due to the failed_jobs_uuid_unique constraint. It seems that the failed_jobs_uuid_unique constraint may be a separate bug for jobs that have a "ShouldBeUnique" implementation, and there is an issue with error handling when they fail multiple times. Upon further investigation through the stack trace, we found that the issue was caused by the MaxAttemptsExceededException.
After removing the unique constraint from the database (for easier debugging), we narrowed down the error to the following log line:
After diving deeper into debugging, we discovered that the jobs were not being removed from the "reserved" queue correctly, and the workers were executing them again, resulting in the MaxAttemptsExceededException. Further examination of the source code revealed a failed removal of entries in \Illuminate\Queue\RedisQueue::deleteReserved. The result of the execution was always 0, indicating that the entry was not being removed.
After spending more hours experimenting with different Redis clients and configurations, we found that setting either compression and/or serialization in the Redis database configuration resulted in a faulty zrem request to Redis. As a workaround, we disabled the two options for the default database, which is used by the worker, and did not investigate the problem further.
Our hypothesis is that the lack of compression or serialization in the Redis client causes a mismatch between the member in the zrem request, leading to the entry not being deleted. Workaround was to disable serializer and compression
Hope i haven't forgotten anything :) Thanks
Steps To Reproduce
Configure redis DB phpredis as client serializer option 2 (Redis::SERIALIZER_IGBINARY) compression option set to 2 (Redis::COMPRESSION_ZSTD)
Create a simple job with tries set to 1 (I have also added the ShouldBeUnique to provocate the DB constraint error which from my point of view is a separate error)
Dispatch the job and wait for second execution As described above this will result in multiple executions on the second worker run due to the job was not removed from the
reserved
queue