fix(pruning): Avoid accidentally full-table scans when pruning session tokens

[rfk]

This adjusts the session-pruning logic to address two observations from #294.

First, it adds an explicit check for whether @pruneUntil is NULL, and short-circuits if so. This indicates that no expired tokens are available to prune.

Second, it changes the way that @pruneUntil is calculated, by dropping the filter on "does this session have a decide record?". This is to help us deal better with a database that's already been partly pruned, and hence has a session token timeline like:

|--- millions of device sessions ---|--- a mix of device- and non-device sessions ---|

The previous version would calculate @pruneUntil to be after the millions of unexpirable tokens in the early part of the timeline, forcing the subsequent DELETE statement to wastefully examine them all. The new version should incrementally work its way through these millions of tokens, not deleting any of them, but just slowly catching back up to the point where we find expirable tokens.

Fixes #294; @philbooth r?

I couldn't think of any terribly useful tests to add here sorry; if you can suggest some I'm happy to add them.

[rfk]

@jrgm, re: your question in IRC about how to fix up the prod db - I believe it would be safe to include a change like this in the next release, re-enable the pruning task, and just let it recover in the background without manually adjusting "sessionTokensPrunedUntil" in the database.

Alternately, we could try to calculate the timestamp of the oldest expirable token and manually reset "sessionTokensPrunedUntil" to that value. But I'd worry about the being an expensive query to do in production.

[philbooth]

Whoops, a loop without a sane termination condition. Schoolboy error on my part! 🤦‍♂️😄

Anyway, the fixes look great to me. Just on the tests quickly:

I couldn't think of any terribly useful tests to add here sorry; if you can suggest some I'm happy to add them.

Would it make sense to assert that sessionTokensPrunedUntil is not the empty string towards the end of test/local/prune_tokens.js, possibly after adding an extra iteration of the prune loop for good measure? This assumes that the tests will be run on a small-ish db, but that's pretty much always the case isn't it? (and it would still be no harm when they're run against a large-ish db of course, the assertion passes but is just less useful)

[rfk]

Would it make sense to assert that sessionTokensPrunedUntil is not the empty string
towards the end of test/local/prune_tokens.js

Good call. I added this and confirmed that it fails with the previous implementation prune_6. If it successfully passes with the new implementation prune_7 I'll go ahead and merge.