Fix merging bug in 6.1.0 that resulted in sessions being deleted.

[zkrising]

This PR fixes the bug in 6.1.0 without reverting the commit that added the functionality.

---

Rundown

0fbca57#diff-759b3934f9097285185078895a48275b1a0b0221852ad264a4a2e2dc67b24430R207-R222

This function here does not merge dates properly. Instead, it replaces any merging instances of a date with an empty object. This appears as a race condition because mergeDeep is only called sometimes based on timestamps -- which is why the bug triggers more reliably when a page loads and a lot of ajax calls are made.

Of course, when expires gets set to an empty object instead of a date, whenever math is performed on it, it turns into NaN, and NaN > 0 is false, so all sessions get immediately thrown away.

This is emphatically not a bug in express-session, but I did get sent on a wild goosechase by its logic 😅.

The correct fix here is to replace mergeDeep with a function that properly merges dates. I'm using deepmerge here, because it's pretty battle tested.

[wavded]

@zkldi thanks for looking deeply into this. Can you fix up the failed checks please?

@apydo not sure if you'd be willing/able, but since you could replicate the issue before, I wonder if you'd be able to test your workflow and see if this PR fixes the issue you were having with v6.1.0?

[zkrising]

Pushed a fix for the formatting!

[apydo]

@wavded, I'll be able to make some tests next monday, not before. Sorry.

[zkrising]

Not sure why the tests are failing, not entirely sure what they're doing either...

[apydo]

@wavded , I made several quick tests in the same way I did it last time and I cannot see any error while authenticating in page and launching several quick ajax calls within a second.

[wavded]

Thx @apydo for taking the time to check this out. @zkldi you can pull the test changes from the original PR introducing this feature. That "should" fix them -> https://github.com/tj/connect-redis/pull/333/files

[zkrising]

Backported those changes -- hopefully I got all of that right? Not the best git user 😅

[fillon]

any update on this PR?

[wavded]

Thx @fillon, this had slipped my mind, it has been released now!

[SamDuvall]

After upgrading from 6.1.1 to 6.1.2 this morning, I've noticed tests in my automated test suite failing intermittently. I'll run the entire test suite once and 1% of the tests will fail and I'll run the entire test suite again and a different 1% of the tests will fail.

I'm using connect-redis with express-session for session/user management. I don't know for sure, but it looks like there might be some timing issue from when the session is written to when the session is read from redis, where the session doesn't come back on the read.

I'm still investigating the issue, but I'm fairly confident I've isolated the issue to 6.1.2 of connect-redis. Any ideas on why this might be happening?

[zkrising]

That's not good, the 6.1.2 patch was intended to fix this bug in (deprecated) 6.1.0. I'll crack a look at it now. I'm pretty confident this should work though, since I tested it for a while.

[zkrising]

Cursory glance shows that there's another timing issue in the original 6.1.0 code, since git diff v6.1.0 master lib/connect-redis.js shows fairly benign changes.

Maybe we'll have to revert back to pre-6.1.0 again until this can be properly investigated. I also can't reproduce the issue yet, but since this is a patch fix this potentially broken code is currently propogating across the ecosystem. ouch.

[craigjbass]

Is this related #354 ? https://github.com/uktrade/data-hub-frontend/blob/a87c30333a63533fe148f4c84d3a7b6c5e13e756/src/middleware/session-store.js#L7 this is how we're using it in production.

[wavded]

This has been reverted.

[zkrising]

yeah, ugh. Sorry about this. I think there might be some other unrelated bugs in the 6.1.0 update, but my testing didn't catch any of this.

[wavded]

No worries. Appreciate the time you took to look into it. I think we may need to give this feature a rest until we can get something rock solid. Want to keep releases stable.

[craigjbass]

Just my $0.02 - I’m not convinced this feature will work as designed, unless there is a way to provide users the ability to atomically lock the session when they modify its contents.

It’s ok if a request just reads the data, but if there is any read, change of contents followed by a write back to redis this whole block needs to be protected by a lock to prevent concurrency issues. The way I’ve implemented this with redis in the past is by creating an empty key alongside the session suffixed with :lock with a TTL of the max request time. If this key exists then it’s locked. This ensures no lock is left hanging around if a process dies longer that the longest running request, but protects these session edits.

Ideally, the session is only ever written once. WORM - write-once-read-many, and then you can avoid locking entirely. Potentially for session expiry another key could be created suffixed with :expired, that signals even if the session is present - it should be treated as expired.

[craigjbass]

In terms of the session getting huge in our production environment. We were seeing our permissions array, which just contains a list of strings of permissions the user has, get the permissions repeated thousands of times. Maybe the deepMerge just duplicates them?

[zkrising]

In terms of the session getting huge in our production environment. We were seeing our permissions array, which just contains a list of strings of permissions the user has, get the permissions repeated thousands of times. Maybe the deepMerge just duplicates them?

Oh, this is a default feature of the deepmerge library and can trivially be changed.

[HughePaul]

I'm not sure merging conflicting sessions a good idea. Aside from deepmerge concatenating arrays exponentially, it has no way to know how merging two deep session objects might affect functionality.

Either:

• conflicting sessions should continue to be overwritten with the latest and this should be solved by the app,
• there should be some sort of locking or queuing in place, (in a major version update)
• or the app should be presented with the conflicting session objects to decide how to actively merge them, or choose between them.

If they can be locked, perhaps it should be possible to also request a session in read only mode that could be a long running (eg file upload) request but that is not saved at the end of the session.