Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix double agent messages with rate limit errors #2535

Closed
wants to merge 3 commits into from

Conversation

pkukielka
Copy link
Contributor

@pkukielka pkukielka commented Jan 2, 2024

Fixes sourcegraph/jetbrains#233

Test plan

Automatic tests to cover the issue were added.

Details

Error Error: Cannot add a bot message after a bot message was popping out when message was sent to agent with rate limit exhausted. It was caused by SimpleChatPanelProvider closing typewriter, which in turn was calling close callback from sendLLMRequest in SimpleChatPanelProvider.
Close callback was then dumping remaining content, resulting in adding agent message twice.

    at SimpleChatPanelProvider.addBotMessageWithGuardrails (cody/vscode/src/chat/chat-view/SimpleChatPanelProvider.ts:855:21)
    at Object.close (cody/vscode/src/chat/chat-view/SimpleChatPanelProvider.ts:690:26)
    at Object.close (cody/vscode/src/chat/chat-view/SimpleChatPanelProvider.ts:729:27)
    at Typewriter.stop (cody/lib/shared/src/chat/typewriter.ts:116:27)
    at Object.onError (cody/vscode/src/chat/chat-view/SimpleChatPanelProvider.ts:748:32)
    at onErrorOnce (cody/lib/shared/src/sourcegraph-api/completions/nodeClient.ts:29:20)
    at handleError (cody/lib/shared/src/sourcegraph-api/completions/nodeClient.ts:82:25)
    at IncomingMessage.<anonymous> (cody/lib/shared/src/sourcegraph-api/completions/nodeClient.ts:107:41)
    at IncomingMessage.emit (node:events:529:35)
    at IncomingMessage.emit (node:domain:489:12)

@pkukielka pkukielka changed the title Fix double agent messages with rate limit errors WIP Fix double agent messages with rate limit errors Jan 2, 2024
@pkukielka pkukielka changed the title WIP Fix double agent messages with rate limit errors [WIP] Fix double agent messages with rate limit errors Jan 2, 2024
@pkukielka pkukielka force-pushed the pkukielka/fix-bot-message-after-bot-message branch 3 times, most recently from d0601ca to e85ec03 Compare January 3, 2024 16:57
@pkukielka pkukielka changed the title [WIP] Fix double agent messages with rate limit errors Fix double agent messages with rate limit errors Jan 3, 2024
@pkukielka pkukielka force-pushed the pkukielka/fix-bot-message-after-bot-message branch from e85ec03 to 8107b84 Compare January 3, 2024 17:04
Copy link
Contributor

@abeatrix abeatrix left a comment

Choose a reason for hiding this comment

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

lgtm!

Copy link
Member

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏻 Nice job tracking this down and good to have a test case to reproduce it.

@olafurpg
Copy link
Member

olafurpg commented Jan 4, 2024

window.showErrorMessage: Failed to download bfg from URL https://github.com/sourcegraph/bfg/releases/download/v5.2.12792/bfg-linux-x64.zip: PollyError: [Polly] [adapter:node-http] Recording for the following request is not found and recordIfMissing is false.

I'm surprised to see this error since we configure Polly to passthrough requests to github.com

https://sourcegraph.com/github.com/sourcegraph/cody@olafurpg/evaluate-more/-/blob/agent/src/cli/jsonrpc.ts?L162

The fact this only fails in Node v16 indicates something else might be going on...

this.consumer.close()
} else {
console.error('Typewriter stopped', error)
if (error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (error) {
if (error) {
console.error('Typewriter stopped', error)
if (this.consumer.error) {
this.consumer.error(error)
return
}
}
this.consumer.close()

would this do the same thing?

abeatrix added a commit that referenced this pull request Jan 5, 2024
…th new fixes (#2583)

Close #2588

- Reverts #2575 with updated fixes from
#2580 that I've confirmed with
@Gedochao on call.

- Merged main and updated the recording to confirm there are no errors
caused by the changes in this branch in all covered tests:


![image](https://github.com/sourcegraph/cody/assets/68532117/e54d893a-0798-48af-a838-061056fbc889)

- Also confirmed this change also passed the new rate limit test
introduced in #2535

- Fixed upresponsive stop button when error is presented


https://github.com/sourcegraph/cody/assets/68532117/6d9263b0-3e9c-419e-939d-20a2ea011824

## Test Plan

- run `pnpm run test:unit` to confirm all the tests are passing
Copy link
Contributor

@abeatrix abeatrix left a comment

Choose a reason for hiding this comment

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

Confirmed the rate limit test is passing when merged with the latest changes in main:

image

The large chat test will need to be updated accordingly though. Thanks for all your help on this @pkukielka !

@olafurpg
Copy link
Member

olafurpg commented Jan 9, 2024

@pkukielka close this PR? My understanding is that this has been merged through other PRs

@olafurpg olafurpg closed this Jan 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error: Cannot add a bot message after a bot message on an error from chat
3 participants