[Fix Logouts] Remove buggy hackfix

[gaearon]

Depends on bluesky-social/atproto#4238

Once bluesky-social/atproto#4238 is merged, this removes the hackfix working around it. That resolves one of the potential issues that @mackuba identified in #8695 (comment), namely the lastSession variable getting stale.

I don't know if there's an actual repro case for that problem, but removing clearly buggy code in favor of upstream fix seems like a good idea anyway.

Test Plan

Apply this PR first. We should see bad behavior on network errors, same as before the hackfix was added in #4911.

Then apply bluesky-social/atproto#4238. The test plan from #4911 should pass although the hackfix is no longer present.

I have not done this myself yet.

[gaearon]

I'm actually not even sure how to reproduce the original issue because I'm not sure how to forcefully get a network-error. I'm sure there was some way (maybe captive wifi? broken wifi?) but turning off wifi doesn't do the job.

I think this fix is correct but this will need someone from the team to shepherd as I'm unlikely to test this in depth.

[ThisIsMissEm]

I'm actually not even sure how to reproduce the original issue because I'm not sure how to forcefully get a network-error. I'm sure there was some way (maybe captive wifi? broken wifi?) but turning off wifi doesn't do the job.

I think this fix is correct but this will need someone from the team to shepherd as I'm unlikely to test this in depth.

Yeah, probably using the developer tools and setting the network mode to something lossy would do the trick?

[mackuba]

You could try this thing: https://www.avanderlee.com/debugging/network-link-conditioner-utility/ (just don't forget to turn it off…)

[estrattonbailey]

You could try this thing: https://www.avanderlee.com/debugging/network-link-conditioner-utility/ (just don't forget to turn it off…)

Yeah this is what I've used in the past

[mackuba]

Tbh, I haven't been able to manually trigger this condition where it would restore an expired session in that spot because of a network error, even with the Link Conditioner… so I can't confirm it. But also I remember being at the Ahoy conference in Hamburg in the spring on a bad hotel WiFi, and being logged out then all the time…

[estrattonbailey]

I'm struggling as well. Maybe I haven't used Network Link Conditioner for this in the past 🤔

As noted here, this handling was originally added for unknown and server errors. Gonna spend a little more time trying to repro using the local dev-env package, but in any case, the logic here looks sound to me and I believe it should accomplish the same goals as the previous implementation.

[estrattonbailey]

Ok partial repro here bluesky-social/atproto#4242

Appears that BskyAgent.prepare races and may not be called in time, so on a cold load we actually end up hitting the debugger before the persistSessionHandler is attached. That means there's a case where this handling wasn't even used. Even prior to Dan's upstream patch the session shouldn't have been unset.

I gotta pause on this, would be curious to know if anyone can confirm this finding.

[gaearon]

Yeah I wouldn’t expect this to work reliably for the initial load, just for some cases of losing network midway.

[estrattonbailey]

Yeah I wouldn’t expect this to work reliably for the initial load, just for some cases of losing network midway.

Oh then I should point out that network-error is only emitted during a resumeSession call. Other network requests internally call refreshSession, which will emit expired events and rug the session if the token is legit expired, but otherwise won't emit anything.

[estrattonbailey]

With the fix upstream we can merge this. I'm not positive of the impact, but it's a nice cleanup regardless, and should match existing handling. Thanks again for everyone's help on this 🙏