Fix file cache concurrency

[Hellysonrp]

Fixes #419

I was going to open the PR after my tests, but I decided to open it now to get feedback about the code.

Changes

Use p-queue to avoid the execution of evictOldestIfNecessary concurrently.
I also removed the recursion from the function.
I don't like recursion and prefer to avoid it, only using it if it's really necessary. I always think of the possibility of a stack overflow.

I removed the await when calling evictOldestIfNecessary.
I want some feedback about this change. See the commit message for more info if needed.

[codetheweb]

I made a number of changes, let me know what you think.

• I think we need to wait for the eviction process to finish in .cleanup(), so I changed it back to an async function that waits for the queue to be empty. Less sure about needing to wait inside of .on('close', ...)--if you have strong feelings about that we can change it back to what you had.
• What was the reasoning for originally making the queue a static property?
• I removed the empty queue checks inside of .evictOldestIfNecessary() as running .evictOldest() isn't expensive.
• Removed all recursion and just shoved it inside a loop.

Logging now looks like:

[0] 2021-12-03T16:06:10.363Z muse Evicting oldest files...
[0] 2021-12-03T16:06:10.366Z muse 45b92f0c6a33e3d23043ccbca8fd1b5895924b3d06327f2042cb341b0eea88100539d893776e1d22e16a2f4659f9074af760bdeacaf6d6216d360636fda7408e has been evicted
[0] 2021-12-03T16:06:10.367Z muse 1 files have been evicted

[codetheweb]

Also, do we need to be catching and silently continuing here? I don't think this would throw because of race conditions anymore.

[Hellysonrp]

What was the reasoning for originally making the queue a static property?

Each guild has a player.
Each player has a file cache.
If the queue isn't static, each file cache will have its own queue. Consequently, each guild will have its own queue.

The error might occur if two or more guilds are using the bot at the same time (yes, I had this problem when I was testing with the mutex).

[Hellysonrp]

if you have strong feelings about that we can change it back to what you had

I have a tendency of overthinking sometimes. What I thought was about a possible delay before returning the write stream, but as you said, evictOldest isn't expensive, so I was just overthinking again 😅

[codetheweb]

Ah, should have been thinking about multiple guilds. Makes sense.

[Hellysonrp]

I'll update my bot and test it today.

[Hellysonrp]

Today the bot died with the same error.
It doesn't appear to be related to concurrently trying to delete a file though.
Maybe between all the errors that happened before, some files exists in the database but not in the folder?

I don't think this will happen in clean installs, but it might be worth to expect this kind of error (file in the database but not in the folder) to avoid crashes.

[codetheweb]

You're probably right. .removeOrphans() currently treats the database as the source of truth, but should probably use the filesystem as the source of truth instead.

Would it make sense to update .removeOrphans() so it loops over table rows rather than files? Or should the existing code stay there and .removeOrphans() checks both directions?

[Hellysonrp]

I think it should check both directions, but I don't like the idea of checking some files twice. It might impact the startup time of bots with big caches.
Maybe we could do something to mark the file as 'already checked' and avoid checking it twice. One example would be something like:

• Add a column to the file cache table, that defaults to 0 and can be 1
• Set this column to 0 for all records in the beginning of .removeOrphans()
• Keep the current code, but update the database to set the column to 1 if the file is found
• Delete all other database records after finishing the current code

This is only an example, I don't know if it would be faster than looping through every file and looping through every database record, because of the update to the database.

I might be overthinking again lol 😄

[codetheweb]

Just pushed an update that checks both directions, let me know how that works.

I think adding a column is probably overkill and could lead to race conditions if you have multiple instances of Muse sharing the same database for some reason.

[Hellysonrp]

I think this can be merged.
My bot has been running for days without problems.

[codetheweb]

Looks like I'll have to re-evaluate my approach to the GitHub Workflows I just added since it seems Secrets aren't accessible when running from a forked repo.

[codetheweb]

Released in v0.1.1.