Connection refused does not return an error

[jahands]

I noticed that when SRH is unable to connect to redis, it does not return an error, making the @upstash/redis client return nothing, but not throw an error.

In the SRH logs, I see this:

17:16:16.075 [error] #PID<0.3067.0> running Srh.Http.BaseRouter (connection #PID<0.2318.0>, stream id 298) terminated
Server: redis.example.com:80 (http)
Request: POST /
** (exit) an exception was raised:
    ** (KeyError) key :message not found in: %Redix.ConnectionError{reason: :closed}
        (srh 0.1.0) lib/srh/http/command_handler.ex:160: Srh.Http.CommandHandler.dispatch_command/2
        (srh 0.1.0) lib/srh/http/base_router.ex:35: Srh.Http.BaseRouter.do_command_request/2
        (srh 0.1.0) lib/plug/router.ex:246: anonymous fn/4 in Srh.Http.BaseRouter.dispatch/2
        (telemetry 1.1.0) /app/deps/telemetry/src/telemetry.erl:320: :telemetry.span/3
        (srh 0.1.0) lib/plug/router.ex:242: Srh.Http.BaseRouter.dispatch/2
        (srh 0.1.0) lib/srh/http/base_router.ex:1: Srh.Http.BaseRouter.plug_builder_call/2
        (plug_cowboy 2.5.2) lib/plug/cowboy/handler.ex:12: Plug.Cowboy.Handler.init/2
        (cowboy 2.9.0) /app/deps/cowboy/src/cowboy_handler.erl:37: :cowboy_handler.execute/2

This can be repro'd by specifying the wrong redis port

[hiett]

Thanks for reporting this, I can work on getting a fix together. How do you think is approriate to respond? Since this is a situation that upstash sdk isn't expecting, do you think just throwing back a 500 w/ a desc as body? What status is it responding with right now?

[jahands]

Not sure what status it's returning currently - the client just doesn't return data. I think returning 500 with a description would work - just something to get the client to throw an error

[hiett]

Running into one issue that Upstash's SDK doesn't have error 500 handling (as it doesn't expect it), so when I return 500 with a message, it's actually hitting a JSON parse error. Invalid token is short enough that that the error message doesn't get truncated, but with "Unable to connect to Redis", it becomes SyntaxError: Unexpected token 'U', "Unable to "... is not valid JSON

This does throw an error however, and if I also add a simple log message to SRH (I think it's valid to have it in SRH logs because it is a misconfiguration of SRH in most cases) those two pieces of the puzzle should be enough to figure out whats going wrong

[hiett]

SRH now logs WARNING: SRH was unable to connect to the Redis server. Please make sure it is running, and the connection information is correct. SRH ID: some_unique_identifier

[jahands]

This sounds good! That’s odd that the sdk doesn’t actually care about status codes. I wonder how Upstash communicates errors

[hiett]

I'm not sure either. I'm going to dig into the code later and find out - if I can't find anything that I can use there, I'm just going to push out this new code as-is.

I don't want it to report back as a Redis error (e.g command, missing error, etc), because that can be handled in code as a valid response (so I don't think it makes any DX improvement over what is now), instead doing it this way and using a 5xx status code

I want it to be as close as if Upstash was down than there being a syntax / command logic error

[hiett]

Upstash doesn't directly look at response codes, instead just uses fetch's res.ok

https://github.com/upstash/upstash-redis/blob/9b75ea104a3ebdcee8ce0360302782200938999c/pkg/http.ts#L214-L217

It also does res.json() before doing the check, so the error being thrown in regards to JSON parsing is caused directly here. If non 2xx, it throws an UpstashError with the error field from the parsed json.

The question then is: is it reasonable for an UpstashError to be thrown, when this exact field is used for normal Redis errors? Or is "hacking around" and causing the JSON error more reasonable (so it's clear its not a Redis command error)

[jahands]

I think triggering UpstashError is probably fine - I'd just prefix the error with SRH or something so it's clear that it's not an actual Redis error. I presume that Upstash probably does something similar if you try to connect to a server that doesn't exist/etc.

Only danger is that someone is parsing out these errors and might get a false flag based on what we put there - if we're really worried about that, we could make it something structured like {"error": "srh_unable_to_connect"} with a reference to these codes in the README

[hiett]

Yeah you're right, it'll just be fine as an Upstash error - I've prefixed with SRH and now being thrown is UpstashError: SRH: Unable to connect to the Redis server. See SRH logs for more information.

Going to get this all cleaned up and will put out a release