fix: race condition in saving history before switching accounts #6487

arafatkatze · 2024-12-29T10:48:29Z

Problem

When switching between Cody accounts, the last active chat would incorrectly appear in both accounts' chat histories. This happened because:

During account switching, disposeAllChats() was called which attempts to save the current chat state
However, by the time disposeAllChats() runs, the currentAuthAccount had already been updated to the new account
This caused the chat to be saved under the wrong account's history

Solution

Separated the logout and account switch logic:

For logout: Continue to call disposeAllChats() to properly save and clear chats
For account switch: Skip disposeAllChats() and directly dispose UI elements without saving
- This is safe because:
  - Active chats are already saved during normal operation via saveSession()
  - We're just switching views, not losing data
  - Each account's chat history remains properly preserved in storage

Test plan

Create chats in Account A
Switch to Account B and check history to see if the latest chat on Account A is NOT seen on account B
Make a new chat on account B
Switch back to Account A - verify only Account A's chats are visible
Switch to Account B - verify only Account B's chats are visible
Verify logout still properly saves chats before clearing

Changelog

arafatkatze · 2024-12-29T11:06:40Z

@abeatrix So I want to make a test for this but the test for the account switching is flaky? Since this test would be built on top that test so what do i dO?

abeatrix · 2024-12-29T15:47:23Z

@abeatrix So I want to make a test for this but the test for the account switching is flaky? Since this test would be built on top that test so what do i dO?

Oh is it still flaky? I forgot to remove the skip after I updated it since switch account now works.

Update: I marked it as flaky when switch account didn't work

arafatkatze · 2024-12-30T10:12:25Z

@abeatrix So I changed this to a test that technically works


// TODO: Fix flaky test
test.extend<ExpectedV2Events>({
    expectedV2Events: [
        'cody.extension:installed',
        'cody.auth.login:clicked',
        'cody.auth.login:firstEver',
        'cody.auth.login.token:clicked',
        'cody.auth:connected',
        'cody.userMenu:open',
        'cody.auth:disconnected',
        'cody.interactiveTutorial:attemptingStart',
        'cody.experiment.interactiveTutorial:enrolled',
        'cody.signInNotification:shown',
    ],
})('switch account via account dropdown menu in webview', async ({ page, sidebar }) => {
        await expect(sidebar!.getByText('Sign in to Sourcegraph')).toBeVisible()
        await sidebar!.getByRole('button', { name: 'Sourcegraph logo Continue' }).click()
        await sidebar!.getByText('Sourcegraph Instance URL').click()
        await sidebar!.getByPlaceholder('Example: https://instance.').click()
        await sidebar!.getByPlaceholder('Example: https://instance.').fill(SERVER_URL)

        await sidebar!.getByText('Access Token (Optional)').click()

        await sidebar!.getByPlaceholder('Access token...').click()
        await sidebar!.getByPlaceholder('Access token...').fill(VALID_TOKEN)
        await sidebar!.getByPlaceholder('Access token...').press('Enter')

        await expect(sidebar!.getByText('Invalid access token.')).not.toBeVisible()
        await expect(sidebar!.getByText('Sign in to Sourcegraph')).not.toBeVisible()
        await expect(sidebar!.getByLabel('Chat message')).toBeVisible()

        const sidebarChat = getChatSidebarPanel(page)
        const chatInput = getChatInputs(sidebarChat).first()
        await chatInput.fill('Hello world')
        await chatInput.press('Enter')
        await sidebar!.getByTestId('tab-history').click();
        await sidebar!.getByRole('button', { name: 'Hello world' }).first().click();

        // Open the User Dropdown menu
        await expect(sidebar!.getByRole('button', { name: 'New Chat' })).toBeVisible()
        await sidebar!.getByLabel('Account Menu Button').click({ delay: 2000 })
        await sidebar!.getByRole('option', { name: 'Switch Account' }).click()
        await expect(sidebar!.getByText('Active')).toBeVisible()
        await expect(sidebar!.getByText(SERVER_URL)).toBeVisible()

        await sidebar!.getByText('Add another account').click()
        await expect(sidebar!.getByText('Account Details')).toBeVisible()
        await expect(sidebar!.getByRole('button', { name: 'Add and Switch' })).toBeVisible()
        await sidebar!.getByLabel('Instance URL').click()
        await sidebar!.getByLabel('Instance URL').fill(SERVER_URL)
        await sidebar!.getByText('Access Token (Optional)').click()
        await sidebar!.getByPlaceholder('sgp_xxx_xxx').click()
        await sidebar!.getByPlaceholder('sgp_xxx_xxx').fill(VALID_TOKEN_PERSON2)
        await sidebar!.getByRole('button', { name: 'Add and Switch' }).click()

        // Makes sure the dropdown menu is closed.
        await expect(sidebar!.getByText('Account Details')).not.toBeVisible()
        await expect(sidebar!.getByRole('option', { name: 'Help' })).not.toBeVisible()

        await chatInput.fill('test message')
        await chatInput.press('Enter')
        await sidebar!.getByTestId('tab-history').click();
        expect(await sidebar!.getByRole('button', { name: 'Hello world' }).count()).toBe(0)
    })

This test checks for the condition that we want to test and when I run this with the debugger it runs slowly and gives me the right response


  ✓  4 auth.test.ts:136:3 › switch account via account dropdown menu in webview (37.6s)
Cody output channel: /Users/arafatkhan/Desktop/cody/playwright/switch_account_via_account_dropdown_menu_in_webview/temp-log/logger.log

  Slow test file: auth.test.ts (37.6s)
  Consider splitting slow test files to speed up parallel execution
  3 skipped
  1 passed (38.6s)
➜  cody git:(save-hist-fix) ✗

But the part where it switches the accounts happens so fast without the debugger that the frames get detched etc so that's why this is flaky. So we have 3 options

Adding fake timeouts
Doing test.slow() which is what I did now so this raises the timeout but makes the test still work.
Keep the tests but skip them

For now my recommendation is to go with Option 2 and if this ever bothers us again we skip these tests

abeatrix

Left some commands inline that are not merge blocker.

abeatrix · 2025-01-03T20:38:15Z

vscode/src/chat/chat-view/ChatsController.ts

+                        this.activeEditor = undefined
+                        const oldEditors = this.editors
+                        this.editors = []
+                        for (const editor of oldEditors) {
+                            if (editor.webviewPanelOrView) {
+                                disposeWebviewViewOrPanel(editor.webviewPanelOrView)
+                            }
+                            editor.dispose()
+                        }


Suggested change

this.activeEditor = undefined

const oldEditors = this.editors

this.editors = []

for (const editor of oldEditors) {

if (editor.webviewPanelOrView) {

disposeWebviewViewOrPanel(editor.webviewPanelOrView)

}

editor.dispose()

}

this.disposeAllChats()

This looks like it does the same thing as disposeAllChats method? On account switch, the new ChatBuilder(undefined) will be recreated, so it should be safe to saveSession? We should also look into not invoking saveSession when the chat is empty (unrelated to this PR) 👀

abeatrix · 2025-01-03T20:40:23Z

vscode/src/chat/chat-view/ChatsController.ts

@@ -81,11 +81,31 @@ export class ChatsController implements vscode.Disposable {
                    const hasSwitchedAccount =
                        this.currentAuthAccount &&
                        this.currentAuthAccount.endpoint !== authStatus.endpoint
-                    if (hasLoggedOut || hasSwitchedAccount) {
+                    if (hasLoggedOut) {


Suggested change

if (hasLoggedOut) {

if (hasLoggedOut) {

this.disposeAllChats()

return

Can we return early since the code below is doing the same thing without saving the session?

abeatrix · 2025-01-03T20:41:40Z

vscode/src/chat/chat-view/ChatController.ts

@@ -1912,10 +1912,11 @@ export class ChatController implements vscode.Disposable, vscode.WebviewViewProv
        })
    }

-    public async clearAndRestartSession(chatMessages?: ChatMessage[]): Promise<void> {
+    public async clearAndRestartSession(chatMessages?: ChatMessage[], shouldSave = true): Promise<void> {


Q: Should we separate the secession step from this method if this has been causing issues?

arafatkatze added 2 commits December 29, 2024 14:46

fix: race condition in saving history before switching accounts

5468c52

fix: race condition in saving history before switching accounts

37e791d

arafatkatze requested a review from abeatrix December 29, 2024 11:04

arafatkatze marked this pull request as ready for review December 29, 2024 11:05

Adding tests

c529738

abeatrix approved these changes Jan 6, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: race condition in saving history before switching accounts #6487

fix: race condition in saving history before switching accounts #6487

arafatkatze commented Dec 29, 2024 •

edited

Loading

arafatkatze commented Dec 29, 2024

abeatrix commented Dec 29, 2024 •

edited

Loading

arafatkatze commented Dec 30, 2024

abeatrix left a comment

abeatrix Jan 3, 2025

abeatrix Jan 3, 2025

abeatrix Jan 3, 2025

fix: race condition in saving history before switching accounts #6487

Are you sure you want to change the base?

fix: race condition in saving history before switching accounts #6487

Conversation

arafatkatze commented Dec 29, 2024 • edited Loading

Problem

Solution

Test plan

Changelog

arafatkatze commented Dec 29, 2024

abeatrix commented Dec 29, 2024 • edited Loading

arafatkatze commented Dec 30, 2024

abeatrix left a comment

Choose a reason for hiding this comment

abeatrix Jan 3, 2025

Choose a reason for hiding this comment

abeatrix Jan 3, 2025

Choose a reason for hiding this comment

abeatrix Jan 3, 2025

Choose a reason for hiding this comment

arafatkatze commented Dec 29, 2024 •

edited

Loading

abeatrix commented Dec 29, 2024 •

edited

Loading