SessionCacheTest fails intermittently when the clock crosses a second boundary mid-test #1224

Symptom

build-maven (ubuntu-latest, 11) failed on the master push of 8527b9b (run 42177273626, job 127758691227):

SessionCacheTest.shouldResetIdleTimeResetSetToTrue:86 WrongTypeOfReturnValue
SessionState cannot be returned by getProperties()
getProperties() should return Hashtable
Tests run: 2327, Failures: 1, Errors: 0, Skipped: 0

The pushed commits (d61edbb, 8527b9b) touch nothing in the session code. Their only openam-core change adds RealmTestHelper.mockMalformedRealm. The other eight build-maven legs of the run passed openam-core (2327 tests, 0 failures). ubuntu-latest, 17 failed later for an unrelated reason: Java heap space in openam-doc-source.

Cause

The tests run with a maximum caching time of 0, so the session counts as stale as soon as the clock crosses a second boundary. Two places in each test act on that.

setup() spies an anonymous Session whose refresh(true) re-stubs sessionInfo and calls session.update(sessionInfo). SessionInfo.getMaxCaching() is not stubbed, so update() sets maxCachingTime to 0. Near its end, update() also sets latestRefreshTime = currentTimeMillis() / 1000 (Session.java:806). Session.maxCachingTimeReached() (Session.java:535) computes currentTimeMillis() / 1000 - latestRefreshTime > maxCachingTime * 60, so it turns true as soon as the second changes after update(). The test's getLatestRefreshTime() override does not prevent this, because maxCachingTimeReached() reads the field directly.

Race 1: stubbing the spy (the failure seen in CI)

1. Line 85 calls session.update(sessionInfo).
2. Line 86 stubs the spy with when(session.getState(true)).thenReturn(SessionState.VALID), which first runs the real Session.getState(true) (Session.java:423).
3. If the second has changed, getState calls refresh(true). Cache-based polling is off by default. The overridden refresh(true) runs update(sessionInfo), whose last mock call is sessionInfo.getProperties().
4. thenReturn(SessionState.VALID) is applied to getProperties(), and Mockito throws WrongTypeOfReturnValue.

shouldIgnoreResetIdleTimeWhenInvaliodSessionIsAllowed stubs getState(true) the same way (line 116). shouldNotResetIdleTimeWhenResetSetToFalse stubs getState(false). There the overridden refresh(false) does nothing, so that test cannot fail this way.

Race 2: the code under test

SessionCache.getSession checks the same condition itself (SessionCache.java:252). When maxCachingTimeReached() is true, it calls refresh(false) instead of refresh(true). If the second changes between line 85 and line 89, shouldResetIdleTimeResetSetToTrue fails at line 93 because refresh(true) was never invoked. This window is wider than the one in race 1. Replacing when(...) with doReturn(...) would close race 1 only.

The getState stubs do not affect the outcome: getSession, getIdleTime and the refresh override never call getState.

Proposed change

Test code only, in openam-core/src/test/java/org/forgerock/openam/session/SessionCacheTest.java:

1. In setup(), stub given(sessionInfo.getMaxCaching()).willReturn(1L);. maxCachingTimeReached() then needs more than 60 seconds since the last update(), so neither race can fire.
2. Delete the three getState stubs (lines 86, 101 and 116). They have no effect on the result, and the one at line 86 is where the CI failure comes from.

How to check the fix

Not reproduced in the real test yet. Each race should be reproducible deterministically on the current code. Set latestRefreshTime (declared on Session) to currentTimeMillis() / 1000 - 1 on the spy through reflection:

• right after line 85: the WrongTypeOfReturnValue seen in CI;
• after line 86, before line 89: the line-93 failure.

With the fix, both injections should leave all three tests green.