-
Notifications
You must be signed in to change notification settings - Fork 6.1k
[app-server] Add cancellation registry and $/cancelRequest #6064
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
a9debc6 to
fe3e787
Compare
49e3a84 to
37b6a71
Compare
37b6a71 to
65586a3
Compare
65586a3 to
2b1b3ea
Compare
2b1b3ea to
18b3fd8
Compare
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pub(crate) fn insert<F>(&self, id: RequestId, f: F) | ||
| where | ||
| F: Fn() + Send + 'static, | ||
| { | ||
| self.inner | ||
| .lock() | ||
| .unwrap_or_else(PoisonError::into_inner) | ||
| .insert(id, Box::new(f)); | ||
| } |
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.
Allow FnOnce cancellation callbacks
The new registry only accepts F: Fn() + Send + 'static, which means every cancellation action must implement Fn. That rules out the most common patterns for cancelling background work, e.g. signalling a tokio::sync::oneshot::Sender or taking ownership of an abort handle, because closures such as move || tx.send(()) are only FnOnce. Trying to register one of those today will not compile, so long-running requests that rely on a one-shot cancel signal still cannot integrate with $/cancelRequest, defeating the purpose of this feature. Please change the registry to handle FnOnce callbacks (e.g. by storing them behind an Option/Mutex so they can be invoked exactly once).
Useful? React with 👍 / 👎.
| #[serde(rename = "$/cancelRequest")] | ||
| #[ts(rename = "$/cancelRequest")] | ||
| #[strum(serialize = "$/cancelRequest")] | ||
| CancelRequest(CancelRequestParams), |
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.
ah very interesting that this is done as a notification. makes sense after reading https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#cancelRequest
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.
ah, looks like this might not be applicable to login. the use case for a $/cancelRequest call seems to be:
- client makes a request to server
- server is processing, no response sent yet
- client sends a $/cancelRequest notification
- server responds to the original request with a response with ErrorCodes.RequestCancelled
so this seems generally useful to interrupt long-running operations.
whereas with chatgpt login, we do:
- client makes an account/login request
- server immediately responds with {"login_id": uuid, "auth_url": }, where the url needs to be opened in a webview on the client
- user authenticates via the webview
- server sends a notification to the client indicating the login was successful
So the codex process starts up an HTTP server on port 1455 to listen for the redirect after login is successful, and then update its auth storage. Asking codex, looks like it'll time out in 10 mins if user doesn't complete login flow and there's no cancellation. We expose the CancelChatGptLogin request as a way to immedately cancel the login, mostly for UX I think (i.e. they can render a "cancel login" button in their UI).
tldr my 2c: based on our convo with pavel, I think we actually want to name it account/login/start and pair it with account/login/cancel since the login request returns immediately with a URL
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.
sorry for the back and forth on this!
|
closed in favor of |
Add cancellation registry and
$/cancelRequest, so that long-running Json-RPC endpoints can orchestrate their own cancellation logic. This is in line with the LSP protocol (doc). Registered cancellation forlogin_chatgpt, so$/cancelRequestperforms the same as the legacycancelLoginGptendpoint.