laravel / horizon

Dashboard and code-driven configuration for Laravel queues.
https://laravel.com/docs/horizon
MIT License
3.87k stars 658 forks source link

Updated purge() and updateMetrics() LUA-scripts to be more deterministic #1501

Closed Jespercal closed 1 month ago

Jespercal commented 2 months ago

After being unable to use the 'horizon:clear' without getting the Exception: "ERR Error running script (call to f_281c23c60c642d42fdedcc1acc8adc7317dd75a5): @user_script:18: @user_script: 18: Write commands not allowed after non deterministic commands. Call redis.replicate_commands() at the start of your script in order to switch to single commands replication mode."

I figured I would try to resolve it, so I updated the scripts to be more deterministic.

In purge(), the cursor is set to 0 and the batch-size is 100, but this could be changed.

taylorotwell commented 1 month ago

Do we have tests for this?

Jespercal commented 1 month ago

test_it_removes_recent_jobs_when_queue_is_purged() in Feature/RedisJobRepositoryTest tests the command. It doesn't throw any errors here on Github, but if I run it without the code changes locally, it throws the same exception as above. After the code change, the test passes.

Jespercal commented 1 month ago

I did some digging, and it all comes down to the fact that I'm using Redis 3.2.9 locally through MAMP PRO. In that version, script replication is verbatim by default, so I'm getting the exception. As of version 5.2, effect is the default replication mode, so the exception is never thrown, because it doesn't matter for the effect mode.

All in all, the script doesn't really need to be deterministic for Redis versions above 5.2, but it doesn't hurt that it is. And it also adds backward compatibility with older versions of Redis, like the one I'm using.

If the code changes seem a little extreme, an alternative could be adding "redis.replicate_commands()" at the beginning. It changes the mode from verbatim to effect, where Redis is verbatim as default, like in <5.2 versions of Redis. In newer versions of Redis, it does nothing and just returns true.

taylorotwell commented 1 month ago

If it's only for older versions will just table it for now.