Garbage collector do not collect things as it should

[lekoala]

I believe the current unit tests are broken for the garbage collector

proof: set DBDatetime::set_mock_now('2022-08-15 12:00:00'); and still, the session id3 is not collected when it clearly should from what i understand (unless persistent sessions without hash are meant to stay permanently)

actually, the db queries from the garbage collector DO NOT USE the mock time which already proves that setting the mock time will have no impact

when there is no hash, the query 'LoginHash.ExpiryDate:LessThan' => date('Y-m-d H:i:s') never do anything which is not great if the hash is deleted but the session is not this means that we end up with persistent sessions that will never be collected when there are no associated hashes (i doubt this is by design?)

EDITED : actually i got it wrong the first time reading the code, it seems that it's by design that some persistent login session don't get a hash, updated the issue accordingly

PR

• https://github.com/silverstripe/silverstripe-session-manager/pull/155

[lekoala]

Also, it uses

https://github.com/silverstripe/silverstripe-session-manager/blob/c5ca5eb7458f10c941d0aa52321e760955dfbc18/src/Services/GarbageCollectionService.php#L28

and I believe it should be using LoginSession::getSessionLifetime() instead ? not a big deal since it's just a matter of it being collected later, but i'm not sure it's intentional or at least it deserve a comment to clarify

[lekoala]

@GuySartorelli @emteknetnz do you know if the current behavior is the expected one?

currently, there are three cases:

• temporary sessions that are collected based on LastAccessed
• permanent sessions with a remember hash that are collected based on ExpiryDate
• permanent sessions without a remember hash, that are never collected

I'm not sure how meaningful is the last case, because from what i can see, permanent session are always expected to have a matching remember hash. So, I would say that without one, it should be collected like any temporary session. This can be seen in my PR (on top of checking ExpiryDate:LessThan, that only matches the LoginSession with a matching remember hash, I also check ExpiryDate = null in another request)

Depending on that, I will update my PR to remove that last bit I added to the collector and at least the unit tests will prove something (at the moment, they only prove that the current setup pass for any date in the far future)

[GuySartorelli]

I'm not sure, to be honest, and I don't have time to dive into the past to see if there are PRs or issues with additional context. It looks like @bergice and @kinglozzer were involved in the initial implementation of this module so one of them may have the context we're after.

[GuySartorelli]

Thanks for that! I've merged the PR. It'll be released in 2.1.0