fix(amazonq): Profile needing to be selected on server restart #7316
Conversation
Signed-off-by: nkomonen-amazon <[email protected]>
Signed-off-by: nkomonen-amazon <[email protected]>
Problem: Profiles and Customizations were pushed to the LSP before Auth was initialized (and its bearer token pushed to the LPS), this caused errors since it does not make sense to push profile/customization if a bearer token does not exist. Solution: Ensure the bearer token is pushed first, then follow up with pushing the profile/customization after. Signed-off-by: nkomonen-amazon <[email protected]>
|
Problem: THe old way we got the startUrl did not return a value since the underlying activeConnection in the Auth class was empty. The activeConnection we actually want was in AuthUtil. Solution: Use the connection we get from AuthUtil as that is the most accurate one. Signed-off-by: nkomonen-amazon <[email protected]>
packages/amazonq/src/lsp/auth.ts
Outdated
| @@ -114,11 +115,14 @@ export class AmazonQLspAuth { | |||
| .setProtectedHeader({ alg: 'dir', enc: 'A256GCM' }) | |||
| .encrypt(encryptionKey) | |||
|
|
|||
| const conn = AuthUtil.instance.conn | |||
| const startUrl = conn?.type === 'sso' ? conn.startUrl : undefined | |||
There was a problem hiding this comment.
can we just use AuthUtil.instance.startUrl here? I think previously we were using AuthUtil.instance.auth.startUrl which might have been causing the problem?
There was a problem hiding this comment.
Yeah that was the root, of it. I haven't updated the description but AuthUtil.instance.auth.startUrl is actually using Auth and not the one from AuthUtil. It looks like we never set an activeConnection in Auth so startUrl from there returns nothing.
Idk why this only broke now though
Signed-off-by: nkomonen-amazon <[email protected]>
+1 |
Is there any way we can enforce this with a test? Since this behavior is so critical its likely worth finding a way to ensure this doesn't accidentally get broken in the future. Or is this not worth it since this will be deleted by Flare migration soon?
If these make it through the Flare Migration, we should invest time to rename this or find a way to make this more obvious, because as a consumer I would have no idea. |
packages/amazonq/src/lsp/client.ts
Outdated
| await activate(client, encryptionKey, resourcePaths.ui) | ||
| } | ||
| async function initializeAuth(client: LanguageClient): Promise<AmazonQLspAuth> { | ||
| AuthUtil.instance.regionProfileManager.onDidChangeRegionProfile(async () => { |
There was a problem hiding this comment.
technically this listener could trigger before we push the bearer token below in await auth.refreshConnection(true) right? Wouldn't that be problematic?
There was a problem hiding this comment.
From testing I haven't found a code path where this happens. This flow will be simplified in flare and I will revisit
|
potentially out of scope here, but the error message is deceiving. |
|
Did we always have this race condition or a regression that got introduced recently? |
- Moves the profile/customization code up a level out of the LSP chat activation, since it doesn't make sense there - Remove the previous onDidChangeProfile handler from the auth setup since it is redundant due to us having it already in onLanguageServerReady() - Renamed function to onLanguageServerReady() to be more aligned with flare naming Signed-off-by: nkomonen-amazon <[email protected]>
This is still unknown. I tried previous versions of the extension and language server this morning and still saw the same error so I suspect this isn't recent. |
See individual commits for isolated changes
Problem
We were seeing the following errors from the Q Language Server on startup:
Amazon Q Profile is not selected for IDC connection typeAmazon Q service is not signed inSolution
We needed to do 2 solutions, each is a separate commit (see their message). There were also some minor refactors.
In short:
UpdateBearerTokenit showedssodid not contain the startUrlfeature/xbranches will not be squash-merged at release time.