Problem/Motivation

Note: discovered while working on #3580666: Autologout kills the dynamic page cache for all authenticated users

When a user extends their session via the dialog, the module works correctly the first time. On the second (or later) extend, the user is unexpectedly logged out or the dialog appears at inconsistent times.

Root cause

Multiple places in autologout.js set t = setTimeout(...) or paddingTimer = setTimeout(...) without first calling clearTimeout() on the existing timer. This causes multiple concurrent timer callbacks to accumulate over time, resulting in erratic behavior.

Steps to reproduce

- Set a short timeout (e.g. 10s) and padding (e.g. 5s)

ddev drush config:set autologout.settings timeout 10 -y
ddev drush config:set autologout.settings padding 5 -y

- Log in
- Wait for the dialog to appear, click Yes
- Wait for the dialog to appear again, click Yes
- User is logged out despite clicking Yes

Proposed resolution

Introduce two small helper functions that encapsulate clearTimeout + setTimeout, making it structurally impossible to accidentally leave an orphaned timer.

Remaining tasks

do it

User interface changes

N/A

API changes

N/A

Data model changes

N/A

Issue fork autologout-3580664

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

herved created an issue. See original summary.

herved opened merge request !103

herved’s picture

herved commented
Issue summary: View changes
Status: Active » Needs review
ericgsmith’s picture

ericgsmith commented
Status: Needs review » Reviewed & tested by the community

Confirmed the bug as reported - code changes look good and resolves the issue.

the_g_bomb made their first commit to this issue’s fork.

the_g_bomb’s picture

the_g_bomb commented

Updated the fork; it would be good to get a test in place to protect this.
May need a rebase

ericgsmith’s picture

ericgsmith commented
Status: Reviewed & tested by the community » Needs work

Setting to needs work based on above need for test

ericgsmith’s picture

ericgsmith commented
Status: Needs work » Needs review

Added a test that passes - test only job fails - https://git.drupalcode.org/issue/autologout-3580664/-/jobs/9091331

This is not great test really as I found I needed to add up to a 30 second allowance to the wait for element as the activity reset timer is hard coded to 30 seconds - I wonder if we should add some logic to change this if the timeout is less than 30 seconds, use the timeout value? Haven't looked too close to what that activity reset is doing.

herved’s picture

herved commented

Sorry for the late reply.
I initially wrote a test - actually updating the existing one in AutologoutCookieTest - and noticed the same 30s delay.
I don't think waiting for that long in tests is a viable option. This arbitrary 30s delay also doesn't make sense to me. I think the whole activityResetTimer should be deleted and then in init we could just:

let activity = false;

function init() {
  const noDialog = settings.autologout.no_dialog;
  if (activity) {
    activity = false;
    refresh();
  }
  else {
    ...

So it will just follow the main timeout.
Also, why is it altering the settings var, from drupalSettings, and not using a local var?

ericgsmith’s picture

ericgsmith commented

@herved that sounds sensible - do you think that should be done here or in a follow up / separate issue?

My concern is that there is a lot of open issues touching the JS and it would be good to get this fixed before widening the scope - #3395581: Autologout can log out from other tabs/windows appears to remove the 30 seconds as well so not sure how much would be covered if that issue lands.

What if we just mark this test as skipped - @the_g_bomb - we have demonstrated the issue via the test but the downside (an additional 1 minute to run a single test) is not really worth it - can we mark it skipped for now pending a follow up to remove the 30s timer?

ericgsmith’s picture

ericgsmith commented

I've marked the test as skipped - not ideal introducing a test that isn't running, but I think getting this fix in is pretty important to be able to accurately test and fix other issues - and given other issues like #3395581 are (indirectly) tackling the activity reset timer, I don't think we should widen the scope of this issue to fix that here.

@the_g_bomb is there anything else needed here to get this in? I think my RTBC of the @herved’s changes is still valid although leaving needs review for the test / test conversation.

the_g_bomb’s picture

the_g_bomb commented

Sorry, I've been really busy at work, and I hope to get back to this asap. Happy to leave the tests until a follow-up ticket.

benstallings’s picture

benstallings commented
Status: Needs review » Reviewed & tested by the community

1. JS timer accumulation fix (4e99d0b) — Good fix. The core issue is that setTimeout was called repeatedly without clearing previous timers, causing multiple concurrent
timeouts that could trigger premature logout. The schedule() / schedulePadding() wrapper pattern is clean and correct:
- Each call to schedule() clears timeoutTimer before setting a new one
- Each call to schedulePadding() clears paddingTimer before setting a new one
- All 7 setTimeout call sites are covered

One minor note: Math.max(0, localSettings.timeout - difference) on line 283 is a good defensive addition — prevents negative delays if the clock difference exceeds timeout.

2. Test (baba1c9 + 53395e4) — The test is immediately markTestSkipped due to the 30-second hardcoded activity reset timer making it impractically slow. This means:
- The test documents intent but provides no CI coverage
- It won't catch regressions

the_g_bomb’s picture

the_g_bomb commented
Status: Reviewed & tested by the community » Fixed

Finally got around to testing this myself and merging.
Thank you all.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.