fix: Ensure code suggestions state is updated after init (!1624) · Merge requests · GitLab.org / gitlab-vscode-extension

Tristan Read requested to merge tristan.read/fix-cs-state-check into main May 30, 2024

Description

There is an issue where the code suggestion state can become incorrect, based on a race condition within the state policy initialization.

This fix forces a status check once all policies have been initialized.

Thanks @jrushford for pairing with me on this, it was a great debugging session 🎉

More detail about the issue

I applied this patch to add extensive logging: patch.diff

Click to expand

diff --git a/src/common/code_suggestions/code_suggestions_state_manager.ts b/src/common/code_suggestions/code_suggestions_state_manager.ts
index cffd3a07..37a2d412 100644
--- a/src/common/code_suggestions/code_suggestions_state_manager.ts
+++ b/src/common/code_suggestions/code_suggestions_state_manager.ts
@@ -127,6 +127,8 @@ export class CodeSuggestionsStateManager {
       log.error('Code suggestions status bar item failed to initialize due to an error: ', e);
     }
 
+    log.error(`this.#subscriptions.push`);
+
     this.#subscriptions.push(
       ...this.#policies.map(p => p.onEngagedChange(() => this.#fireChange())),
     );
diff --git a/src/common/code_suggestions/state_policy/license_status_policy.ts b/src/common/code_suggestions/state_policy/license_status_policy.ts
index 5754457e..9fd4fcc8 100644
--- a/src/common/code_suggestions/state_policy/license_status_policy.ts
+++ b/src/common/code_suggestions/state_policy/license_status_policy.ts
@@ -10,6 +10,8 @@ import { log } from '../../log';
 const isLicenseAvailable = async (platform: GitLabPlatform) => {
   try {
     const availabilityResponse = await platform.fetchFromApi(getSuggestionsAvailability());
+    log.error(`${JSON.stringify(availabilityResponse)}`);
+    // TODO: look into rechecking if the user changes?
     return availabilityResponse.currentUser.duoCodeSuggestionsAvailable;
   } catch (e) {
     log.error(
@@ -34,18 +36,24 @@ export class LicenseStatusPolicy implements StatePolicy {
   #dependency: StatePolicy;
 
   constructor(manager: GitLabPlatformManagerForCodeSuggestions, dependencies: StatePolicy[]) {
+    log.error('HELLO')
     this.#manager = manager;
     this.#dependency = new CombinedPolicy(...dependencies);
     this.#manager.onPlatformChange(this.#updateEngaged);
     this.#subscriptions.push(
-      this.#dependency.onEngagedChange(async () => {
+      this.#dependency.onEngagedChange(async (e) => {
+        log.error(`this.#dependency.onEngagedChange. dependency.engaged: ${e}`)
         const platform = await this.#manager.getGitLabPlatform();
         await this.#updateEngaged(platform);
       }),
     );
+    this.onEngagedChange((engaged) => {
+      log.error(`onEngagedChange. engaged = ${engaged}`)
+    })
   }
 
   async init() {
+    log.error('init');
     const platform = await this.#manager.getGitLabPlatform();
     await this.#updateEngaged(platform);
   }
@@ -55,15 +63,20 @@ export class LicenseStatusPolicy implements StatePolicy {
   }
 
   #updateEngaged = async (platform: GitLabPlatform | undefined) => {
+    log.error(`updateEngaged`);
     if (this.#dependency.engaged) {
+      log.error('dependency was engaged');
       return;
     }
     if (!platform) {
+      log.error(`no platform`);
       this.#isLicenseAvailable = false;
       this.#eventEmitter.fire(this.engaged);
       return;
     }
+    log.error(`performing license check`);
     this.#isLicenseAvailable = await isLicenseAvailable(platform);
+    log.error(`license check result: ${this.#isLicenseAvailable}`);
     this.#eventEmitter.fire(this.engaged);
   };

After running the app, the following logs are produced:

2024-05-30T23:48:21:597 [error]: init
2024-05-30T23:48:21:597 [error]: updateEngaged
2024-05-30T23:48:21:597 [error]: performing license check
2024-05-30T23:48:21:880 [error]: {"currentUser":{"duoCodeSuggestionsAvailable":true}}
2024-05-30T23:48:21:880 [error]: license check result: true
2024-05-30T23:48:21:880 [error]: onEngagedChange. engaged = false
2024-05-30T23:48:22:809 [error]: this.#dependency.onEngagedChange. dependency.engaged: false
2024-05-30T23:48:22:809 [error]: updateEngaged
2024-05-30T23:48:22:809 [error]: performing license check
2024-05-30T23:48:22:810 [error]: this.#subscriptions.push
2024-05-30T23:48:23:094 [error]: {"currentUser":{"duoCodeSuggestionsAvailable":true}}
2024-05-30T23:48:23:094 [error]: license check result: true

Note in particular these two lines:

onEngagedChange - triggered when the event emitter for the policy is fired.
this.#subscriptions.push - triggered when the state manager subscribes to all the policies.

The logs show that the event emitter is fired during init, before state manager subscribes to policy change emitters. The state change has now been swallowed. Once init completes, the status is stuck on the license check error, even though the license check passed.

The real-life behavior is dependent on the init of each policy, and whether any subsequent state changes occur. This is why it behaves differently in the Web IDE - there are different policies in play.

The QA test failed because it was waiting on the status to update: https://gitlab.com/gitlab-org/gitlab/-/blob/7240c44c6f8532cd04fb86b7f06e82e54eda558d/qa/qa/page/project/web_ide/vscode.rb#L349.

Related Issues

Issue: #1386 (closed)

How has this been tested?

Set up a local gitlab-web-ide + gitlab-vscode-extension
- Instructions here: https://gitlab.com/gitlab-org/gitlab-web-ide/-/blob/main/docs/dev/development_environment_setup.md?ref_type=heads#setup-integrating-with-gitlab-workflow-vs-code-extension
Run the web ide yarn start:example
Run the example app, ensuring that:
- you have a valid token that has Duo access
- code suggestions are switched on
- you select a project where Duo is not disabled
Observe the Code Suggestions status icon at the bottom of the editor.

Screenshots (if appropriate)

before	after
cs-status-error	cs-status-fixed

What CHANGELOG entry will this MR create?

fix: Bug fix fixes - a user-facing issue in production - included in changelog
feature: New feature - a user-facing change which adds functionality - included in changelog
BREAKING CHANGE: (fix or feature that would cause existing functionality to change) - should bump major version, mentioned in the changelog
None - other non-user-facing changes

Edited May 30, 2024 by Tristan Read

fix: Ensure code suggestions state is updated after init