redash: Redis decode_responses rq incompatibility

imo waiting for 60s for a test isn’t that big of a deal, I’d be fine with that. Messing with the internals of Redis sounds more error-prone and in my opinion doesn’t give enough lift that it’s worth doing instead.

I’ve had a chance to look into this. I found one place where the wrong connection was being passed to an RQ function leading to the UnicodeDecodeError. https://github.com/getredash/redash/pull/6539

While looking for the problem, I noticed that the enqueue_query method is creating a Redis pipeline to watch a value, but the pipeline isn’t being reset. I don’t think this is related to the issue at hand, but seems like something that should be fixed. I’ve created a PR https://github.com/getredash/redash/pull/6540 that calls reset inside a finally block. The other option is to change line 39 - with redis_connection.pipeline() as pipe:, but that does mean another layer of nesting and the code is already quite nested in that function.

I believe the actual source of the problem is the HardLimitingWorker.

This is performing a heartbeat on the Worker, but not on the Job. Once a query is picked off the queue for execution, RQ is giving the job a default timeout of 60s. Normally this would be updated every time the job heartbeat is called. Nothing happens immediately after this timeout expires. The next time the RQ StartedJobRegistry.cleanup method is called, the job status will be set to failed. The job will continue running, and will still list the job (execute_query) on the Redash Admin RQ status page. However the query will not appear on the query tab of the Admin RQ status page. It will also look like it has stopped executing to a user who has the query open, possibly leading them to click refresh and start another copy of the query.

The StartedJobRegistry.cleanup method is called a result of calling get_job_ids, and somewhat ironically it looks like this happens in two places in Redash -

• The remove_ghost_locks maintenance task, which is probably not actually progressing to this step because of the UnicodeDecodeError
• The rq_status API call, used by the Admin RQ status page where you would go to see the list of running queries

Rather than adding a call to the job heartbeat from within the HardLimitingWorker, I believe the best fix in this case is to remove the HardLimitingWorker entirely. It looks like this was cloned from an early version of RQ, and its purpose is to hard-kill a job that runs for too long without respecting its time limit. This functionality has since been baked directly into RQ. It was added in version 1.2.1, and has undergone a number of iterations of bugfixes.

I have managed to reproduce this locally for ad-hoc queries. I’m not certain if this is also the cause of the scheduled queries running multiple times yet, but will give the fixes a go and get back to you with the results.

can only concatenate str (not “bytes”) to str

This does indeed probably stem from Redis, but I agree that it’s probably unrelated to the original error. AFAIK Redis by default returns bytes, and you have to decode with utf-8 to get str type, so that’s what the error in the tests is complaining about.