Repository navigation
Conversation
SbloodyS
requested changes
Oct 14, 2026
SbloodyS
left a comment
SbloodyS
left a comment
Member
There was a problem hiding this comment.
Please follow the pull reuqest notice.
…Absent Reuse the existing session when it is still valid and replace it when it has expired — the previous condition did the opposite, so a second successful login deleted the user's live session (killing their other tab/devices) while a timed-out session was refreshed and reused.
kittimzhe
force-pushed
the
session-expiry-inversion-18699
branch
from
October 14, 2026 03:59
bbe2b54 to
86a1226
Compare
kittimzhe
changed the title
Contributor
Author
|
Thanks for the gate check — fixed per the Pull Request Notice:
The issue triage, root-cause analysis, fix and tests are unchanged. |
SbloodyS
changed the title
SbloodyS
reviewed
Oct 14, 2026
| } | ||
| session = sessionList.get(0); | ||
| if (isSessionExpire(session)) { | ||
| // An unexpired session is reused (its last-login time refreshed); an |
Member
There was a problem hiding this comment.
Suggested change
| // An unexpired session is reused (its last-login time refreshed); an |
| session = sessionList.get(0); | ||
| if (isSessionExpire(session)) { | ||
| // An unexpired session is reused (its last-login time refreshed); an | ||
| // expired one is deleted so a new session is created below |
Member
There was a problem hiding this comment.
Suggested change
| // expired one is deleted so a new session is created below |
Comment on lines
+123
to
+126
| /** | ||
| * #18699: an unexpired session must be reused on re-login, not deleted and | ||
| * replaced (the expired/unexpired branches were inverted before the fix) | ||
| */ |
Member
There was a problem hiding this comment.
Suggested change
| /** | |
| * #18699: an unexpired session must be reused on re-login, not deleted and | |
| * replaced (the expired/unexpired branches were inverted before the fix) | |
| */ |
Comment on lines
+144
to
+148
| /** | ||
| * #18699: an expired session must be replaced by a new one on re-login, | ||
| * not refreshed and reused (the expired/unexpired branches were inverted | ||
| * before the fix) | ||
| */ |
Member
There was a problem hiding this comment.
Suggested change
| /** | |
| * #18699: an expired session must be replaced by a new one on re-login, | |
| * not refreshed and reused (the expired/unexpired branches were inverted | |
| * before the fix) | |
| */ |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Was this PR generated or assisted by AI?
YES — issue triage, root-cause analysis, fix and tests were developed with AI assistance (mechanism verified against the code and pinned by red→green tests below).
Purpose of the pull request
Fixes #18699.
SessionServiceImpl#createSessionIfAbsentreused an expired session (refreshing its last-login time) and deleted a still-valid one — the exact opposite of the method's contract. A second successful login invalidated the user's live session, while a timed-out session ID was kept alive.Brief change log
createSessionIfAbsent: reuse the existing session when!isSessionExpire(session), delete-and-recreate when it has expired.Regression origin (answering the question in the issue): commit 12f8138 (#15219, "Expire session when update user password") rewrote the old
createSessionintocreateSessionIfAbsentand flipped the polarity along the way — the pre-refactor code reused the session whennow - lastLoginTime <= SESSION_TIME_OUT(i.e. not expired). So this is a regression introduced in #15219, not an intentional login policy.Verify this pull request
This change added tests and can be verified as follows:
SessionServiceTest#testCreateSessionIfAbsentReusesUnexpiredSession— an unexpired session is returned as-is (updateByIdcalled; nodeleteById/insert). Fails on the pre-fix code (expected the old session id but got a freshly generated UUID).SessionServiceTest#testCreateSessionIfAbsentReplacesExpiredSession— a session 40 days past its last login is deleted and a new one inserted. Fails on the pre-fix code (the expired id was returned).SessionServiceTest: 6/6 green with the fix (JDK 8,mvn test -pl dolphinscheduler-api -Dtest=SessionServiceTest).