use redis expire event to delete stored data immediately

[ehuggett]

fixes #521 which is now closed, but money saving patches are welcome! Hopefully this will half the amount of storage in use*... I believe its ready for criticism?

The redis server configuration needs to be altered for this to work setting the following appears to be suitable (/the bare minimum?)
notify-keyspace-events "xE"

• I was not able to test the s3 specific part of this PR
• No attempt has been made to verify the redis server is configured correctly, it would be possible to query the server config to check if this is required.
• the subscription is pointless if notify-keyspace-events is not set correctly, but it does not seem to be harmful? (no errors, but obviously does not delete the file data).
• the event is not guaranteed to fire on time or even at all, the rules for automatic deletion on the s3 bucket are still required

edit: *at some points in time

[dannycoates]

This looks really nice @ehuggett, thank you! I think we should put this feature behind a configuration variable in server/config.js before we merge it. A simple boolean like ENABLE_REDIS_NOTIFICATIONS maybe. We run several instances in production and probably only want it enabled on one at a time.

[ehuggett]

something like that? (I decided to use the redis_ prefix only to associate it with redis_host, but can change it to whatever)

I'm not sure if i should do anything about the following?

1. I'm not sure what the scope on forceDelete will be but I don't think I can use let or const (block scope?) so perhaps these errors are expected/ok? (I felt it was better only have one checks for local or s3 storage, but perhaps they way i have done this is not idea?)

2. npm test also fails (redis-mock doesn't implement duplicate() ) and I haven't tried (locally) to use my real redis server with it (my dev enviroment actually has it on localhost, using 127.0.0.1 for redis_host as a workaround for now)

related: I deliberately deleted a file (with rm) and got the following error with localForceDelete

(node:1061) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

looks like trying to delete a non-existent file will eventually become a fatal error? (ie as you implied multiple hosts reacting to the same redis event would trigger multiple attempts to delete the same file)

[ehuggett]

Just FYI, If someone is still downloading the file when it expires

• with local/filesystem storage, the file will be unlinked but I expect the download would continue without interruption (as node has the file open) but I have not tested this
• with aws s3, I have no idea what will happen (if the server has not finished buffering the file before it is removed from s3)

Even if downloading does not currently "brake" on expiry when using s3, i would suggest its not something this PR should attempt to address

• Perhaps this is actually desirable behaviour, there has to be a cutoff somewhere.It should be expected, as we told the uploader we would delete in in 24 hours!
• separately from this PR, Some low-risk leeway could be added if the record could be locked to a "session" (the browser that first clicks download), if this updates the redis record (with the session data) and extends the previously set expiry time. This still needs a sane limit (ie 1 hour or the expiry, whichever is later?) to prevent any potential shenanigans (downloading very slowly intentionally?)

[ehuggett]

When rebasing this I chose to disable deletion upon expiry by default in the config (as you only want it running once, hoping this also makes it easier to merge).

[dannycoates]

hey, better late than never, eh @ehuggett? 😉