Settings sync stuck at "Synchronizing User Configuration..."

[eamodio]

Issue Type: Bug

There was apparently something wrong with my token/user when I first signed into sync (I used my GitHub account). Since then, vs code just gets stuck:

Often, I can just turn off sync (via the menu), but sometimes (I think when I vscode start with sync on) the menu choice doesn't exist at all.

There doesn't seem to be a way for me to reset the sync and pick a new user account to get out of the state.

VS Code version: Code - Insiders 1.42.0-insider (5e758c1, 2020-01-13T09:52:21.788Z)
OS version: Windows_NT x64 10.0.19041

System Info

Item	Value
CPUs	AMD Ryzen 9 3900X 12-Core Processor (24 x 3800)
GPU Status	2d_canvas: enabled flash_3d: enabled flash_stage3d: enabled flash_stage3d_baseline: enabled gpu_compositing: enabled multiple_raster_threads: enabled_on oop_rasterization: disabled_off protected_video_decode: unavailable_off rasterization: enabled skia_renderer: disabled_off_ok video_decode: enabled viz_display_compositor: enabled_on viz_hit_test_surface_layer: disabled_off_ok webgl: enabled webgl2: enabled
Load (avg)	undefined
Memory (System)	31.95GB (16.94GB free)
Process Argv
Screen Reader	no
VM	0%

[Tyriar]

The server would have been sending a 401 unauthorized for this.

[sandy081]

@RMacfarlane I also saw this today and my actions in gear icon are not shown. Sync is not happening any more. But I can see Sync: Turn Off action in command pallette. I could run this command and after that I do not see any sync actions. Looks like the auth status is stuck at initializing.

[Tyriar]

@sandy081 to be clear, this was happening because of a bug in the server where @eamodio's user specifically was being rejected with a 401 despite having a valid token. So this issue is about handling when the server rejects a token. In today's build for me sync just doesn't seem to work at all so that's probably a different issue.

[sandy081]

I see. But I think both are auth related. I used to refresh the token when there is a 401 rejected status so that we can get a new and valid token. Looks like this was changed and removed when we moved auth to extension by @RMacfarlane.

@RMacfarlane what is the recommended way to handle this? How can a client can say that the auth is not valid?

[Tyriar]

@sandy081 there's always a chance that the refreshed token will be rejected by the server, this issue is about handling that case gracefully. Maybe #88317 is the underlying problem?

[RMacfarlane]

With moving the auth to the extension, the extension should now refresh the token in the background. If refresh fails, it will delete the refresh token stored in the keychain and send an update event, which the core will see as a sign out event. So the UI would update to show the indicator on the gear icon again, and the event would also make it to the shared process and stop syncing. Right now I'm not showing any additional notification for this case, but that might be useful to have.

With this strategy, the shared process ideally would never see any 401s from the token because the extension should always be sending it something valid. I think if it does see a 401, it should stop syncing.

[sandy081]

Backing off requests with errors will be handled here - #85211

So closing this in favour of that issue where errors will be handled.

[sandy081]

@RMacfarlane As per your suggestion, I stopped syncing if there are errors (401s) consecutively for 5 times. This actually caused this #89334

In this case I should see a sign in required notification which is missing here. Also, this information should be needed in shared process auth service so that the sync can be suspended until sign in is done. I think this information and bridging is missing now.

Reopening it to consider this.

[sandy081]

I am removing the code that disables sync on successive failures on auth until we find the right solution.

[Tyriar]

@sandy081 this is how I would expect it to work:

1. Auth service gives sync service a (seemingly) valid token
2. Sync service makes a request to server
3. Server returns 401
4. Sync service informs auth provider that the token didn't work
5. Sync service is disabled until auth provider resolves the problem

[sandy081]

This is how it is used to work when auth is in the core. And as I said in my comment, point 4 is missing in the new auth flow. So, it is not possible for the core to disable the sync now.

[RMacfarlane]

Right, communication from a renderer to the shared process is easy, but the other direction is complicated because there can be many renderers/extension hosts to the one shared process. The sync server never directly communicates to the extension right now, it is simply pushed tokens.

[sandy081]

I think shared process can send a signal that it has invalid auth token and the renderer(s) can listen to this and can inform extension to handle this by changing the auth state to logout or refreshing the token?

[RMacfarlane]

Can we be more specific about what an "invalid" auth token is? @Tyriar Do we have logging about why a request returns 401?

From my understanding, the token needs to be a valid AAD token and needs to be not expired. I think the issue we potentially run into is with token expiry. If the auth extension isn't pushing valid AAD tokens, then it is broken. Are there other scenarios where a token would be "invalid" besides being expired?

I think it doesn't make sense for the shared process to inform the auth extension that a token has expired. The auth extension already knows how long access tokens live for. Currently, it attempts to do a refresh 10 seconds before a token expires. It could start doing a refresh earlier so that there is more buffer time it.

[sandy081]

Not sure whether a token is expired or invalid or not, all I get from the server is 401 Unauthorized response in which case I would expect to signal to the auth extension that the token is expired or invalid.

[Tyriar]

Updated server to send the reason in the body, or just "Unauthorized" if some other exception happened during auth. The reason should equal "Failed to verify user: jwt expired" when the token is expired.

[RMacfarlane]

Awesome, thanks! I'm also going to change the auth extension to start the refresh sooner

[RMacfarlane]

The original issue no longer reproduces, and any 401 should now trigger a token refresh: #89629

[roblourens]

Anything that needs to be verified?

[RMacfarlane]

This should be covered by #91653, so will mark as a duplicate