sessions: delete session after registration failure #1174
Conversation
bitromortac
force-pushed
the
2511-delete-session-after-fail
branch
from
d131076 to
2df8605
Compare
session_rpcserver.go
Outdated
| // If we tried to link to a previous session, we delete the newly | ||
| // created session in the case of errors to avoid having non-revoked | ||
| // sessions lying around. | ||
| fail := func(err error) error { |
There was a problem hiding this comment.
Nit: Not sure we need a separate lambda function for this when it's only being called once?
There was a problem hiding this comment.
right, I wasn't sure if we also want to delete the session on an error later on, so I left it for now
There was a problem hiding this comment.
I removed that and added it directly
There was a problem hiding this comment.
Not sure if you did 😄? But this is non-blocking. So I'm ok to Merge as is.
session_rpcserver.go
Outdated
| if err != nil { | ||
| return nil, fmt.Errorf("error registering session with "+ | ||
| "autopilot server: %v", err) | ||
| return nil, fail(fmt.Errorf("error registering session with "+ |
There was a problem hiding this comment.
Given that we're not 100% sure here that this errors due to the session not having been stored on the autopilot server (as this could simplify fail for loss of the connection mid execution for example), I can see the motivation still persisting this on the litd node, to aim to not have sessions stored on the autopilot server, which do not exist on the corresponding litd node.
But since that can already occur for various other reasons as well, I think it's fine to just delete it.
ellemouton
left a comment
ellemouton
left a comment
There was a problem hiding this comment.
Once suggestion - non blocking.
LGTM! 🔥
| DELETE FROM sessions | ||
| WHERE state = $1; | ||
|
|
||
| -- name: DeleteSession :exec |
There was a problem hiding this comment.
could maybe in future make things more defensive (both here and above by restricting this to only allowing it for "where status=reserved" - but i guess it's ok for now since your store interface does this protection
There was a problem hiding this comment.
right, I see your point enforcing that on the db layer would be safer, one downside of this is that we'd need to hard code an integer here for the type (i.e. 4), so we'd lose some type safety, right? for this reason I kept the current approach, but I'll definitely consider that approach in future work 🙏
| if session.State != StateReserved { | ||
| return fmt.Errorf("session not in reserved state, is "+ | ||
| "%v", session.State) | ||
| } | ||
|
|
||
| return deleteSession(sessionBucket, session) |
There was a problem hiding this comment.
as mentioned above, could just make the db query enforce this constraint
bitromortac
force-pushed
the
2511-delete-session-after-fail
branch
from
2df8605 to
da3ae1a
Compare
bitromortac
force-pushed
the
2511-delete-session-after-fail
branch
from
da3ae1a to
a20df7c
Compare
bitromortac
added
the
no-changelog
bitromortac
force-pushed
the
2511-delete-session-after-fail
branch
from
a20df7c to
0a659df
Compare
bitromortac
force-pushed
the
2511-delete-session-after-fail
branch
from
0a659df to
1b10afd
Compare
3 participants
Due to recent changes we'll always create a session first with status
reserved. After the session is created and if the autopilot rejects session registration we are left with a session that can't be used. This is problematic for session linking, as the reserved session would need to be revoked first in order to link another session, but we don't return a reference to the new session for revocation. The session isn't usable anyhow so I suggest to delete it after a failure. I only did that for the registration failure, but we could extend the case to other errors later in the session creation.An alternative to this would be to move the session into a terminal
failedstate, which we don't have so far and we'd end up storing non-useful sessions, but it could be useful for debugging.