Do explicit session garbage collection on cron

Yeah, the active session thing is a bit awkward, but the documentation for session_gc() says that you should just call session_start() then, doing that with a session is started check and confirmed that this calls down into \Drupal\Core\Session\SessionHandler::gc().

> I switched the session backend to redis and had a rude shock when I discovered that the default session.gc_maxlifetime is 24 minutes if you don't override it in services.yml. Yet my databases in environments that haven't done the switch yet have sessions in them that are 19 days old.

The default setting is "gc_maxlifetime: 200000", that's ~2.3 days. And it confused my a lot initially as well, it's combined with the cookie expiration, which has an extra 0 and is ~23 days. The trick here is that by default, your session stays active up to 23 days as long as you visit the site at least once within those 2.3 days, then you extend your session expiration by another 2.3 days until eventually your cookie vanishes after 23 days even if you access it daily.

There is no official redis session module yet, are you using the patch. Maybe there's a bug there, maybe it gets second/millisecond calculation wrong or something like that. I'm assuming that the redis logic uses built-in redis expiration and therefore doesn't need to do any garbage collection, redis handles that automatically.

The last submitted patch, 19: session-gc-3033791-19.patch, failed testing. View results →

@Berdir, yes I'm using the patch.

The default setting is "gc_maxlifetime: 200000", that's ~2.3 days. And it confused my a lot initially as well, it's combined with the cookie expiration, which has an extra 0 and is ~23 days. The trick here is that by default, your session stays active up to 23 days as long as you visit the site at least once within those 2.3 days, then you extend your session expiration by another 2.3 days until eventually your cookie vanishes after 23 days even if you access it daily.

Thanks for the explanation, that makes sense. It also makes sense that the redis patch might be failing to convert seconds to milliseconds.

+1 to this. I think this will improve security of sites. One consideration though is that now you can still have expired sessions lasting longer than they should. It all depends on how often cron runs on your site. But that's already the case with this 1% probability cleaner.

The trick here is that by default, your session stays active up to 23 days as long as you visit the site at least once within those 2.3 days

This is not true, at least with Drupal's default Drupal\Core\Session\SessionHandler:

  /**
   * {@inheritdoc}
   */
  #[\ReturnTypeWillChange]
  public function gc($lifetime) {
    // Be sure to adjust 'php_value session.gc_maxlifetime' to a large enough
    // value. For example, if you want user sessions to stay in your database
    // for three weeks before deleting them, you need to set gc_maxlifetime
    // to '1814400'. At that value, only after a user doesn't log in after
    // three weeks (1814400 seconds) will their session be removed.
    $this->connection->delete('sessions')
      ->condition('timestamp', REQUEST_TIME - $lifetime, '<')
      ->execute();
    return TRUE;
  }

This examines the "timestamp" column in the sessions table to determine the "age" of a session. The problem is that the timestamp is only updated when session data changes, causing a session write. An authenticated just browsing the site will not cause the timestamp to be incremented.

I think we also need to update the comment in default.services.yml to be accurate. It currently says this (emphasis mine):

# Set session lifetime (in seconds), i.e. the grace period for session
# data. Sessions are deleted by the session garbage collector after one
# session lifetime has elapsed since the user's last visit. When a session
# is deleted, authenticated users are logged out, and the contents of the
# user's session is discarded.
# @default 200000
gc_maxlifetime: 200000

I think at a time, this may have been true, maybe we always use to write the session at the end of every request? But we don't do that with our lazy sessions, which are only opened/written when there's something to write.

Converted the patch to a MR and fixed an issue. The patch was running session_start(), but we ALSO need to run ->start() on the session object so that SessionManager can initialize the session configuration. Otherwise the values set in services.yml for the session lifetime aren't used.

Leaving to Needs work since this will need a test