fix(core): conflict between dialog sessions contexts on update #4493
Conversation
laurentlp
changed the title
|
If this is an urgent issue to be fixed, then we might not want to go and fix the underlying issue as this might require some further testing, |
Kind of yes. Since it can affect any users with multiple bots using the same channel (e.g. Converse API, Teams, Messenger, ...) I also think that we should work on the underlying issue on another PR. |
|
I took a look and I think we never added the botId in the session ID before because it wasn't "necessary" a couple of years ago. Let's go ahead by adding the botId (in Session duration is 30 min per default, and I guess that when the update is made, users are probably not using the bot and sessions would be cleared anyway. If we want to make this transparent, a migration could simply fix the session ID in the table with the botId since they are already stored there... I'd make that change in this PR so we can avoid a modification of the SDK twice (one to add the botId and the other to remove it) |
…pdate_session_conflicts
| try { | ||
| if (client === 'sqlite3') { | ||
| await db.transaction(async trx => { | ||
| await trx.raw(`UPDATE ${TABLE_NAME} SET id = botId || '::' || id;`) |
There was a problem hiding this comment.
I'm not sure what you're trying to do here ... you set the id to being the botId ? what's the logic behind that. Or perhaps I don't understand that statement.
I think what we're trying to do is (pseudo) : id = id + botId
There was a problem hiding this comment.
I choose to prefix the session id with the botId. So, in the migration, I first update the table ids with the botId like so id = botId || '::' || id which is the equivalent of id = botId + separator + id
As for testing, I just made sure that the ids were prefixed with the botId, that the botId column was dropped and that the id column was the primary key (for both SQLite and PostgreSQL)
There was a problem hiding this comment.
Wow, totally sorry I'm used to use the concat function and had no idea of the || operator .. that is totally my bad I thought you were making botID or '::" which makes no sens
|
Just to make sure I'll test on PG, give me a few mins |
|
Ok I just tried this 5 times and migration on PG does not work.. I still doubt my manual testing skills though |
What error do you get ? It works fine for me, postgres and sqlite. Started a conversation, ran the migration and it I could continue it without issue |
We found what was the issue. It should be fixed with this PR: #4511 |
This PR fixes (yet another) issue around conflicting sessions. In this case, the logic surrounding the change to a composite primary key (see #4394) was missing for the update process 🤦.
Since the session's update function was not taking the botId into account, all the sessions with a given id were overridden with one of the sessions data. Meaning that sessions, for a given user using channels without threadId, could be unintentionally shared between bots. This could result in conflicts between session's contexts which would possibly raise a node not found error.
To reproduce the bug, you had to have more than one bot on a single channel without threadId (e.g. Converse API) and one of the bots had to wait for the user's input.