fix: Corrected algorithm that releases stale sessions in the pool

[dazuma]

This seems like a bad bug that releases stale sessions incorrectly: instead of releasing stale sessions down to the min level (as I assume we're supposed to), it releases more sessions if the number is below the min, but never releases sessions if the number is above the min. Fixed and added tests.

[viacheslav-rostovtsev]

Yes, this looks like a nasty one. Good catch!

[viacheslav-rostovtsev]

LGTM

[viacheslav-rostovtsev]

One thing that will happen with the fixed code is that if a lot of sessions go inactive they will all get sent for release in one go, wrapped in individual futures. These will all be concurrent server requests to delete_session, causing a spike. The futures as far as I can tell will also swallow all potential errors.

Monitoring the futures is probably a backlog candidate for volume of work reasons, but do we perhaps want to stagger the releases?

[dazuma]

Monitoring the futures is probably a backlog candidate for volume of work reasons, but do we perhaps want to stagger the releases?

That is an interesting thought. There are things we could do. For example, we can limit the number of sessions to release at a time, to a certain max. Then that many sessions would be released, and the next batch would be done the next time the session keepalive timer ticks (every 5 minutes currently), and so forth. However, before we take a step like that, I'd want some evidence that a "spike" of session releases is an actual problem (and also, some quantitative idea of what limits we should be placing). These are questions that go well beyond this fix.