contribsys / faktory

Language-agnostic persistent background job server
https://contribsys.com/faktory/
Other
5.78k stars 230 forks source link

Improve error validation on Redis persistence #451

Closed pupocw closed 1 year ago

pupocw commented 1 year ago

Is there any chance of not being able to save a task in Redis due to lack of error validation here?

I did some tests simulating an error in Redis and the client was not informed that the task was not saved. By changing the implementation to return the error, the client is notified about the problem and can take some action to avoid losing the task.

Does it make sense to change to something like this?

func (q *redisQueue) Push(ctx context.Context, payload []byte) error {
    return q.store.rclient.LPush(ctx, q.name, payload).Err()
}

We are using the enterprise version and I don't know if we have the same behavior in it.

Thank you!

mperham commented 1 year ago

Great point. Because Faktory runs Redis as a child process, there's less risk than calling Redis over the network but it could still run out of memory and fail, for instance.

pupocw commented 1 year ago

Yes, my test was based on out of memory error. Will something similar need to be applied to the enterprise version?

mperham commented 1 year ago

Good question. The issue is that the Redis command API doesn't return an error directly so the errcheck linter doesn't detect the lack of error handling. I'll audit and fix any missing error handling.