Fix HTTP session timestamp units

[codingkiddo]

Summary

Fix AwsHttpSession timestamps to use epoch milliseconds as required by the Jakarta Servlet HttpSession contract.

Previously, creationTime and lastAccessedTime were stored using Instant#getEpochSecond(), while HttpSession#getCreationTime() and getLastAccessedTime() are defined in milliseconds since the Unix epoch.

Changes

• Use Instant#toEpochMilli() for session creation time.
• Use Instant#toEpochMilli() for last accessed time.
• Convert maxInactiveInterval from seconds to milliseconds when comparing it with session timestamps.
• Update the existing creation-time test to validate millisecond timestamps.

Testing

Ran:

mvn -pl aws-serverless-java-container-core \
    -Dtest=AwsHttpSessionTest \
    test

and:

mvn -pl aws-serverless-java-container-core clean test

Both pass successfully.

[aws-sam-tooling-bot]

Code Review Results

Reviewed: 75822fe..1ebaab2
Files: 2
Comments: 2

[aws-sam-tooling-bot]

[BUG] Switching touch() to millisecond resolution regresses isNew(), which is implemented as an equality check on the two timestamps (line 121):

public boolean isNew() {
   return lastAccessedTime == creationTime;
}

With second granularity, any getAttribute/setAttribute/removeAttribute call occurring in the same wall-clock second as construction left lastAccessedTime equal to creationTime, so isNew() stayed true for the duration of a typical sub-second Lambda invocation. With toEpochMilli(), the first attribute access almost always advances the timestamp, so isNew() starts returning false within the very same request that created the session.

That contradicts the HttpSession contract: isNew() must return true until the client has joined the session (i.e. until the client sends back the session id on a subsequent request). Since a session here is created per request and never returned by the client, it should remain new for its whole lifetime. Frameworks layered on top (for example Spring Security's session-fixation and "session created" handling) branch on isNew(), so the flip is externally observable.

Decoupling isNew() from the timestamps keeps the fix to units only:

private boolean isNew = true;

@Override
public boolean isNew() {
   return isNew;
}

Note that the existing test validSession_expectCorrectValidationOrInvalidation asserts assertFalse(sess.isNew()) after a Thread.sleep(1000), so it passes either way and does not cover this behavior change.

[aws-sam-tooling-bot]

[BUG] The unit conversion here is correct, but it cements a comparison that measures the wrong quantity: lastAccessedTime - creationTime is the session's total age, not its idle time. maxInactiveInterval is defined as the time the container will allow between client requests before invalidating the session, so a continuously used session is invalidated once it is older than the interval, even though it was never idle. Conversely, a session that has been idle for hours is still reported valid as long as it was created recently and never touched again.

The check should compare current time against the last access:

boolean isValid() {
   if (Instant.now().toEpochMilli() - lastAccessedTime < maxInactiveInterval  1000L) {
       return valid;
   } else {
       return false;
   }
}

While touching this line, also consider the spec rule that a zero or negative maxInactiveInterval (settable via setMaxInactiveInterval) means the session never expires. Today a negative value makes the comparison immediately false, so isValid() returns false for a brand-new session — the exact opposite of the intended "never time out" behavior.

[aws-sam-tooling-bot]

Code Review Results

Reviewed: 75822fe..20aff40
Files: 2
Comments: 1

[aws-sam-tooling-bot]

[BUG] The new isNew field is initialized to true at line 43 and is never assigned false anywhere in the class, so isNew() is now effectively a hardcoded return true. This silently changes observable behavior of a public HttpSession API method: previously any getAttribute/setAttribute/removeAttribute/getAttributeNames call caused isNew() to report false via touch(); now no code path can ever produce false. The modified assertion in AwsHttpSessionTest.validSession_expectCorrectValidationOrInvalidation (assertFalse(sess.isNew()) → assertTrue(sess.isNew())) locks in the new behavior, so the test no longer exercises any state transition — it would still pass if isNew() were replaced by a literal.

The PR description scopes this change to timestamp units only, so this looks unintended. Two coherent resolutions:

If the prior semantics should be preserved, clear the flag when the session is accessed:

private void touch() {
   lastAccessedTime = Instant.now().toEpochMilli();
   isNew = false;
}

If always-new is the intended contract for this container (each request builds a fresh session, getRequestedSessionId() returns null and isRequestedSessionIdValid() returns false, so no client ever rejoins an existing session), then drop the dead field and make the invariant explicit so future readers do not mistake it for mutable state:

@Override
public boolean isNew() {
   // Sessions are created per request and are never restored from the client,
   // so a session is always new.
   return true;
}

Either way, the test should assert the intended contract deliberately rather than being edited to match the implementation.

[aws-sam-tooling-bot]

Code Review Results

Reviewed: 75822fe..8243f8e
Files: 2
Comments: 1

[aws-sam-tooling-bot]

[BUG] isNew() is now an unconditional return true, so a public HttpSession API method no longer reflects any session state. This is outside the stated scope of the PR (timestamp units) and regresses observable behavior in two ways:

1. After invalidate(), isNew() still reports true. Per the Jakarta Servlet contract, isNew() must throw IllegalStateException on an invalidated session — returning true is the least correct of the options.
2. Callers that use isNew() to decide whether a session was already touched/persisted (a common pattern in Spring Security's session-fixation and session-creation logic) now always take the "brand new" branch.

Note that the millisecond switch actually fixes the original equality check rather than breaking it: with second granularity, lastAccessedTime == creationTime stayed true for any attribute access within the same wall-clock second, which was the real bug. If the intent is to preserve "new until first attribute access", track it explicitly instead of relying on timestamp equality:

private boolean isNew = true;

@Override
public boolean isNew() {
   if (!valid) {
       throw new IllegalStateException("Session is invalidated");
   }
   return isNew;
}

private void touch() {
   lastAccessedTime = Instant.now().toEpochMilli();
   isNew = false;
}

If instead the intent is "every Lambda invocation gets a fresh session, so it is always new", that is a deliberate semantic change and should be stated in the PR description and a code comment — not left as a bare literal.

Related: AwsHttpSessionTest.java:80 was changed from assertFalse(sess.isNew()) to assertTrue(sess.isNew()), so the test now asserts the hardcoded constant and can no longer detect a regression here. The preceding Thread.sleep(1000) in that test is also dead weight once the assertion no longer depends on elapsed time.

[aws-sam-tooling-bot]

Code Review Results

Reviewed: 75822fe..c68876d
Files: 2
Comments: 3

[aws-sam-tooling-bot]

[BUG] Setting isNew = false inside touch() keeps the newness flag coupled to attribute access, which contradicts the HttpSession contract on both counts:

• isNew() is defined as "returns true if the client does not yet know about the session or if the client chooses not to join the session" — it reflects whether the session id has round-tripped with the client, not whether the application read or wrote attributes.
• getLastAccessedTime() is explicitly documented as unaffected by application actions: "Actions that your application takes, such as getting or setting a value associated with the session, do not affect the access time."

touch() is called from getAttribute, getAttributeNames, setAttribute and removeAttribute, so a purely read-only lookup now permanently flips the session out of "new" state.

This is also an observable behavior change relative to the pre-PR code. Previously isNew() compared second-granularity timestamps, so within a single Lambda invocation (sub-second in the overwhelming majority of cases) it stayed true for the whole request. Now the first framework-internal attribute read — e.g. a security filter probing for its context attribute — makes isNew() return false for the rest of the request. Given that AwsHttpServletRequest.getRequestedSessionId() returns null and isRequestedSessionIdValid() returns false, the client never joins the session, so true is the correct answer for the object's whole lifetime.

Since a new AwsHttpSession is created per request, the straightforward fix is to leave newness independent of attribute access:

private void touch() {
    lastAccessedTime = Instant.now().toEpochMilli();
}

and keep isNew true unless/until the container has a real notion of the client joining the session. If you instead intend to keep the current semantics, it is worth a comment explaining the deliberate divergence from the spec, because the field name suggests spec behavior.

[aws-sam-tooling-bot]

[BUG] Throwing IllegalStateException from isNew() matches the spec in isolation, but it is the only accessor on this class that does so, and the request object keeps handing out invalidated sessions. AwsHttpServletRequest.getSession(boolean) caches the instance and never clears it on invalidation:

public HttpSession getSession(boolean b) {
    if (b && null == this.session) {
        AwsProxyRequestContext requestContext = (AwsProxyRequestContext) getAttribute(RequestReader.API_GATEWAY_CONTEXT_PROPERTY);
        this.session = new AwsHttpSession(requestContext.getRequestId());
    }
    return this.session;
}

So after any framework code calls session.invalidate() (a logout handler, for example), a later request.getSession(false) still returns the dead session, and calling isNew() on it now throws an unchecked exception where it previously returned a boolean. Meanwhile getAttribute, getCreationTime and getLastAccessedTime continue to answer normally on the same invalidated object, so a caller has no non-throwing way to detect the invalid state (isValid() is package-private).

Two consistent options:

• Apply the invalidation check across the accessors that the spec says must throw, and make getSession(false) return null (or create a fresh session for getSession(true)) once the cached session has been invalidated.
• Or drop the new throw and keep isNew() total, leaving the invalidation contract out of this PR's scope.

Note also that isNew() throws only on explicit invalidate(); a session that has exceeded maxInactiveInterval has valid == true and so does not throw, even though isValid() reports false.

[aws-sam-tooling-bot]

[GENERAL] Replacing assertNull(sess.getAttribute("test")) with the isNew assertion removes the only coverage of invalidate() clearing the attribute map — no other test in this class (or elsewhere in the module) exercises attribute state after invalidation, so a regression in attributes.clear() would now go unnoticed. Keep both assertions:

sess.invalidate();

assertFalse(sess.isValid());
assertNull(sess.getAttribute("test"));
assertThrows(IllegalStateException.class, sess::isNew);