Updates CredentialModel to have an additional stateUnknown during startup while credentials are validated in the background asynchronously (15s timeout).

While loading credentials on startup, the tray app shows a loading message.

While updating credentials, we now check that the server version falls in our expected range.

The sign in page now uses a WinUI 3 Dialog to show errors.

Closes#42

deansheather added2 commits

March 6, 2025 16:48

chore: check server version on sign-in and launch

cb3a049

Updates CredentialModel to have an additional state "Unknown" duringstartup while credentials are validated in the background asynchronously(15s timeout).While loading credentials on startup, the tray app shows a loading message.While updating credentials, we now check that the server version fallsin our expected range.

Also use ServerVersionUtilities in Manager

da0ff72

deansheather marked this pull request as ready for review

March 6, 2025 05:53

deansheather requested review fromspikecurtis andethanndickson

March 6, 2025 05:53

spikecurtis reviewed

Mar 6, 2025

View reviewed changes

App/Models/CredentialModel.cs OutdatedShow resolvedHide resolved

App/Services/CredentialManager.cs OutdatedShow resolvedHide resolved

App/ViewModels/SignInViewModel.csShow resolvedHide resolved

Vpn/Utilities/ServerVersionUtilities.cs OutdatedShow resolvedHide resolved

deansheather added3 commits

March 8, 2025 14:31

PR comments

90e85fb

Rework CredentialManager

1089677

fixup! Rework CredentialManager

df23617

Copy link

MemberAuthor

deansheather commentedMar 9, 2025

@spikecurtis I pretty heavily reworked CredentialManager:

DuplicateLoadCredentials calls now get merged (the same Task will be returned from both, so they will receive identical exceptions/results)
ASetCredentials call during an ongoingLoadCredentials call will cancel it
Added tests
- To add tests I needed to addICoderApiClientFactory andICoderApiClient for mocking purposes
- Pulled theNativeApi components ofCredentialManager into a new typeWindowsCredentialBackend (and added interfaceICredentialBackend for mocking)

deansheather requested a review fromspikecurtis

March 9, 2025 13:05

Copy link

MemberAuthor

deansheather commentedMar 9, 2025

To protect things from concurrent access:

SetCredentials holds_opLock for the entire operation
LoadCredentials will hold_opLock while it creates the "inner" task then releases it
- The "inner" task will grab the lock again when it's ready to write the loaded value

deansheather added4 commits

March 10, 2025 15:15

Add timeouts to tests

27acd32

Add more timeouts to test

77916ed

Capture test hang dumps

9bedf2b

Also upload test binaries on test failure

585418c

spikecurtis reviewed

Mar 10, 2025

View reviewed changes

Copy link

Collaborator

spikecurtis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I like the new CredentialManager overall, but still a bit concerned about invoking callbacks while holding the lock. A couple other small nits FYI.

App/Services/CredentialManager.cs OutdatedShow resolvedHide resolved

App/App.xaml.cs

		// with no cancellation, the method itself will impose a timeout on the
		// HTTP portion.
		var credentialManager = _services.GetRequiredService<ICredentialManager>();
		_ = credentialManager.LoadCredentials(CancellationToken.None);

Copy link

Collaborator

spikecurtisMar 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Just to check my understanding here, because we are notawaiting the task, it doesn't matter if the task throws some exception, like being canceled. And, we don't block for it to complete, right?

I ask because we want the rest of this function to still run and handle closing of the tray window.

Copy link

MemberAuthor

deansheatherMar 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Yep, we run it in the background and ignore all errors. If the credentials fail to load, the state will transition to "invalid" which will show the "please sign in" screen.