bullmq icon indicating copy to clipboard operation
bullmq copied to clipboard

ci(upstash): test updates for Upstash

Open sancar opened this issue 1 year ago • 4 comments

Most of the changes are related to the close taking long due to fact that Upstash does not return from Blocking commands when the connection is closed on the client side. Adding disconnectTimeout: 0, to ioredis is the workaround.

We have applied an optimization on XTRIM for near exact trimming with ~ parameter. This made some tests to be finish much faster but some tests relying on the implementation details of trim fails now. Related codes are commented with: "Upstash fix. Trim near is not guaranteed to trim all in redis spec"

One test was failing time time "process stalled jobs when starting a queue" Test is changed to make it more stable.

And there were a couple of tests where job is expected to be nil/not nil but the behavior seems to be not stable against Upstash. We are assuming that it is not guaranteed but root cause is not identified. Related parts are commented out and added a comment "Upstash fix .... The reason is not clear yet!". We can further work on this after getting feedback from Bullmq team.

Note that there were server side fixes which will be available with Upstash Redis version 1.10.0. Version can be checked via redis INFO command

cli> INFO server
# Server
upstash_version:1.9.1. <<<<=====
redis_version:6.2.6
redis_git_sha1:03ba486
redis_build_id:20231011
redis_mode:standalone

sancar avatar Nov 30 '23 08:11 sancar

Great job! I wonder about the disconnectTimeout option. I have tried to find some documentation about it in ioredis repo without success, it would be useful to get a better understanding on what it does, as we would need to recommend BullMQ users to use this setting in production to avoid potential graceful shutdowns to hang forever.

manast avatar Nov 30 '23 10:11 manast

disconnectTimeout is by default 2 seconds as far as I see. So almost each test is taking 2 seconds longer. It is not a big deal for users but some tests were timing out because of this. And yes, I could not find a documentation as well. I found it in the code after investigation the root cause of the problem. Here it is the link to the option. No doc on the code as well.

sancar avatar Nov 30 '23 10:11 sancar

Hello. I made several fixes and improvements based on the discussion here. In theory all tests should pass. Locally in my computer they don't as the latency seems to be too large. Now, the problem I found is that due to a security policy (read more here if interested https://github.com/orgs/community/discussions/55940) it is not possible to actually use a secret with the UPSTASH_HOST in a PR (as it would be fairly easy to discover the secret creating a PR that just prints the secret). This is a bit problematic actually, not sure what is the proper way to do this, with Redis and Dragonfly we just run a docker image in the runner, but I guess this is not a viable solution for Upstash. Let me know what you think.

manast avatar Dec 13 '23 21:12 manast

Hi @manast, unfortunately we don't have a local solution that we can provide. To be able to use the upstash credentials without giving them way, the only option looks like running the tests on merge push rather than pull_request in github actions. In there, you can use the secrets. Not really the best option but it is what we are doing for our sdk's as well for community pr's.

sancar avatar Dec 19 '23 12:12 sancar