-
Notifications
You must be signed in to change notification settings - Fork 6.8k
[app-server] make app server not throw error when login id is not found #7831
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
Conversation
2cdb895 to
993bdc3
Compare
993bdc3 to
d87ad5b
Compare
d25dc0f to
734d5b4
Compare
734d5b4 to
b723907
Compare
bolinfest
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hopefully two qqs
| timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; | ||
|
|
||
| let request_id = mcp | ||
| .send_cancel_login_account_request(CancelLoginAccountParams { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just so I'm clear: we're sending a cancel request, but we never send send_login_account_chatgpt_request()? Is that a realistic flow or is this just the best we can do for this test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removing this test as it causes more confusion than needed haha. it's not a realistic workflow but we don't error out if there's a race condition for login finishes before canceling
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also deleting login_and_cancel_chatgpt v1 test which is flaky. should be safe since all clients have migrated off of this.
f54f840 to
d75ab1b
Compare
d75ab1b to
6ba0501
Compare
Our previous design of cancellation endpoint is not idempotent, which caused a bunch of flaky tests. Make app server just returned a not_found status instead of throwing an error if the login id is not found. Keep V1 endpoint behavior the same.