[app-server] make app server not throw error when login id is not found (#7831)
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.
This commit is contained in:
parent
c1367808fb
commit
7cabe54fc7
4 changed files with 36 additions and 65 deletions
|
|
@ -668,10 +668,21 @@ pub struct CancelLoginAccountParams {
|
|||
pub login_id: String,
|
||||
}
|
||||
|
||||
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)]
|
||||
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema, TS)]
|
||||
#[serde(rename_all = "camelCase")]
|
||||
#[ts(rename_all = "camelCase")]
|
||||
#[ts(export_to = "v2/")]
|
||||
pub enum CancelLoginAccountStatus {
|
||||
Canceled,
|
||||
NotFound,
|
||||
}
|
||||
|
||||
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema, TS)]
|
||||
#[serde(rename_all = "camelCase")]
|
||||
#[ts(export_to = "v2/")]
|
||||
pub struct CancelLoginAccountResponse {}
|
||||
pub struct CancelLoginAccountResponse {
|
||||
pub status: CancelLoginAccountStatus,
|
||||
}
|
||||
|
||||
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)]
|
||||
#[serde(rename_all = "camelCase")]
|
||||
|
|
|
|||
|
|
@ -19,6 +19,7 @@ use codex_app_server_protocol::AuthMode;
|
|||
use codex_app_server_protocol::AuthStatusChangeNotification;
|
||||
use codex_app_server_protocol::CancelLoginAccountParams;
|
||||
use codex_app_server_protocol::CancelLoginAccountResponse;
|
||||
use codex_app_server_protocol::CancelLoginAccountStatus;
|
||||
use codex_app_server_protocol::CancelLoginChatGptResponse;
|
||||
use codex_app_server_protocol::ClientRequest;
|
||||
use codex_app_server_protocol::CommandExecParams;
|
||||
|
|
@ -195,6 +196,11 @@ struct ActiveLogin {
|
|||
login_id: Uuid,
|
||||
}
|
||||
|
||||
#[derive(Clone, Copy, Debug)]
|
||||
enum CancelLoginError {
|
||||
NotFound(Uuid),
|
||||
}
|
||||
|
||||
impl Drop for ActiveLogin {
|
||||
fn drop(&mut self) {
|
||||
self.shutdown_handle.shutdown();
|
||||
|
|
@ -828,7 +834,7 @@ impl CodexMessageProcessor {
|
|||
async fn cancel_login_chatgpt_common(
|
||||
&mut self,
|
||||
login_id: Uuid,
|
||||
) -> std::result::Result<(), JSONRPCErrorError> {
|
||||
) -> std::result::Result<(), CancelLoginError> {
|
||||
let mut guard = self.active_login.lock().await;
|
||||
if guard.as_ref().map(|l| l.login_id) == Some(login_id) {
|
||||
if let Some(active) = guard.take() {
|
||||
|
|
@ -836,11 +842,7 @@ impl CodexMessageProcessor {
|
|||
}
|
||||
Ok(())
|
||||
} else {
|
||||
Err(JSONRPCErrorError {
|
||||
code: INVALID_REQUEST_ERROR_CODE,
|
||||
message: format!("login id not found: {login_id}"),
|
||||
data: None,
|
||||
})
|
||||
Err(CancelLoginError::NotFound(login_id))
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -851,7 +853,12 @@ impl CodexMessageProcessor {
|
|||
.send_response(request_id, CancelLoginChatGptResponse {})
|
||||
.await;
|
||||
}
|
||||
Err(error) => {
|
||||
Err(CancelLoginError::NotFound(missing_login_id)) => {
|
||||
let error = JSONRPCErrorError {
|
||||
code: INVALID_REQUEST_ERROR_CODE,
|
||||
message: format!("login id not found: {missing_login_id}"),
|
||||
data: None,
|
||||
};
|
||||
self.outgoing.send_error(request_id, error).await;
|
||||
}
|
||||
}
|
||||
|
|
@ -860,16 +867,14 @@ impl CodexMessageProcessor {
|
|||
async fn cancel_login_v2(&mut self, request_id: RequestId, params: CancelLoginAccountParams) {
|
||||
let login_id = params.login_id;
|
||||
match Uuid::parse_str(&login_id) {
|
||||
Ok(uuid) => match self.cancel_login_chatgpt_common(uuid).await {
|
||||
Ok(()) => {
|
||||
self.outgoing
|
||||
.send_response(request_id, CancelLoginAccountResponse {})
|
||||
.await;
|
||||
}
|
||||
Err(error) => {
|
||||
self.outgoing.send_error(request_id, error).await;
|
||||
}
|
||||
},
|
||||
Ok(uuid) => {
|
||||
let status = match self.cancel_login_chatgpt_common(uuid).await {
|
||||
Ok(()) => CancelLoginAccountStatus::Canceled,
|
||||
Err(CancelLoginError::NotFound(_)) => CancelLoginAccountStatus::NotFound,
|
||||
};
|
||||
let response = CancelLoginAccountResponse { status };
|
||||
self.outgoing.send_response(request_id, response).await;
|
||||
}
|
||||
Err(_) => {
|
||||
let error = JSONRPCErrorError {
|
||||
code: INVALID_REQUEST_ERROR_CODE,
|
||||
|
|
|
|||
|
|
@ -1,8 +1,6 @@
|
|||
use anyhow::Result;
|
||||
use app_test_support::McpProcess;
|
||||
use app_test_support::to_response;
|
||||
use codex_app_server_protocol::CancelLoginChatGptParams;
|
||||
use codex_app_server_protocol::CancelLoginChatGptResponse;
|
||||
use codex_app_server_protocol::GetAuthStatusParams;
|
||||
use codex_app_server_protocol::GetAuthStatusResponse;
|
||||
use codex_app_server_protocol::JSONRPCError;
|
||||
|
|
@ -14,7 +12,6 @@ use codex_core::auth::AuthCredentialsStoreMode;
|
|||
use codex_login::login_with_api_key;
|
||||
use serial_test::serial;
|
||||
use std::path::Path;
|
||||
use std::time::Duration;
|
||||
use tempfile::TempDir;
|
||||
use tokio::time::timeout;
|
||||
|
||||
|
|
@ -87,48 +84,6 @@ async fn logout_chatgpt_removes_auth() -> Result<()> {
|
|||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
// Serialize tests that launch the login server since it binds to a fixed port.
|
||||
#[serial(login_port)]
|
||||
async fn login_and_cancel_chatgpt() -> Result<()> {
|
||||
let codex_home = TempDir::new()?;
|
||||
create_config_toml(codex_home.path())?;
|
||||
|
||||
let mut mcp = McpProcess::new(codex_home.path()).await?;
|
||||
timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??;
|
||||
|
||||
let login_id = mcp.send_login_chat_gpt_request().await?;
|
||||
let login_resp: JSONRPCResponse = timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
mcp.read_stream_until_response_message(RequestId::Integer(login_id)),
|
||||
)
|
||||
.await??;
|
||||
let login: LoginChatGptResponse = to_response(login_resp)?;
|
||||
|
||||
let cancel_id = mcp
|
||||
.send_cancel_login_chat_gpt_request(CancelLoginChatGptParams {
|
||||
login_id: login.login_id,
|
||||
})
|
||||
.await?;
|
||||
let cancel_resp: JSONRPCResponse = timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
mcp.read_stream_until_response_message(RequestId::Integer(cancel_id)),
|
||||
)
|
||||
.await??;
|
||||
let _ok: CancelLoginChatGptResponse = to_response(cancel_resp)?;
|
||||
|
||||
// Optionally observe the completion notification; do not fail if it races.
|
||||
let maybe_note = timeout(
|
||||
Duration::from_secs(2),
|
||||
mcp.read_stream_until_notification_message("codex/event/login_chat_gpt_complete"),
|
||||
)
|
||||
.await;
|
||||
if maybe_note.is_err() {
|
||||
eprintln!("warning: did not observe login_chat_gpt_complete notification after cancel");
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
fn create_config_toml_forced_login(codex_home: &Path, forced_method: &str) -> std::io::Result<()> {
|
||||
let config_toml = codex_home.join("config.toml");
|
||||
let contents = format!(
|
||||
|
|
|
|||
|
|
@ -241,7 +241,7 @@ async fn login_account_chatgpt_rejected_when_forced_api() -> Result<()> {
|
|||
#[tokio::test]
|
||||
// Serialize tests that launch the login server since it binds to a fixed port.
|
||||
#[serial(login_port)]
|
||||
async fn login_account_chatgpt_start() -> Result<()> {
|
||||
async fn login_account_chatgpt_start_can_be_cancelled() -> Result<()> {
|
||||
let codex_home = TempDir::new()?;
|
||||
create_config_toml(codex_home.path(), CreateConfigTomlParams::default())?;
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue