Repository navigation
Expired sessions should close the session #1049
Description
I am realizing the behavior for handling an expired session is different when replaying and normal state machine progressions.
if SystemTime::now() > self.context.expiry {
return Err(InternalSessionError::Expired(self.context.expiry).into());
}In most cases we return an error instead of closing the session and pushing an error event.
Originally posted by @arminsabouri in #1036 (comment)
Activity
DanGould commentedon Sep 8, 2025
don't we want to throw an error and then have the error handler produce an event? Or is that separated where you think it should be unified?
arminsabouri commentedon Sep 8, 2025
yes this is essentially what I am thinking. Typestate should return a fatal state transition variant which closes the session and saves a session expired event.
DanGould commentedon Sep 8, 2025
I'm a bit unclear, seems like a "yes" answer to an either-or question. You think separated or unified error production + event production?
arminsabouri commentedon Sep 8, 2025
I dont think we should be returning an
Err. I want to be clear about that. Instead doing what I suggested above.Wdym by seperate or unified? Replaying will still check for expired session after a replaying all events. So in that sense its seperate
arminsabouri commentedon Sep 8, 2025
Related comment: #1036 (comment)
benalleng commentedon Sep 17, 2025
Is this something we would like to use the
SessionEvent::Closedfrom #1078 with expiration as the follow up reason?arminsabouri commentedon Sep 18, 2025
yes. I think a new
Expiredvariant onSessionOutcomewould make the most sense.nothingmuch commentedon Sep 18, 2025
if we adhere to the principle that only primary information becomes an event, then knowing when the session's expiry (part of its context) is sufficient in principle, and no new information would imply no outcome event.
however, this assumes the system clock is monotone... if the system clock rewinds after a session is considered expired, then it might be considered active again afterwards. i don't think that is a situation worth worrying about, especially since the completed_at column would be set when the the expiration condition is detected
on the other hand if we do add an expired outcome event, then that's one more thing to keep in sync with the completed_at column, if it's ever not in sync than the session data would be in an inconsistent state (easy to repair such an inconsistency but that doesn't strongly adhere to the make-invallid-state-unrepresentable maxim)
so i think my vote is that we don't need an Expired outcome, it's enough to just set completed at and reserve the SessionEvent::Closed event variants to record information that is otherwise missing (#1078 (comment)) but i'm not strongly against it if it makes the code more readable for example
arminsabouri commentedon Sep 22, 2025
Follow up on the above: State machine should generate the expired outcome (state transition object), the persister will close the session when processing this outcome.
arminsabouri commentedon Sep 22, 2025
This issue is blocked by #1060