diff --git a/hive-agent/src/web_ui/actions.rs b/hive-agent/src/web_ui/actions.rs index 8df1a9686..8aa5daef5 100644 --- a/hive-agent/src/web_ui/actions.rs +++ b/hive-agent/src/web_ui/actions.rs @@ -9,7 +9,7 @@ use axum::{ use serde::{Deserialize, Serialize}; use std::future::Future; -use super::{AppState, error_response}; +use super::{AppState, SOCKET_FETCH_TIMEOUT, error_response}; pub(super) async fn post_cancel_turn(State(state): State) -> Response { super::cancel_turn(&state.bus, &state.interrupted, "operator: /cancel"); @@ -52,6 +52,9 @@ pub(super) async fn post_new_session(State(state): State) -> Response (axum::http::StatusCode::OK, "ok").into_response() } +/// The most ids one mark-done request may carry. +const MAX_MARK_DONE_IDS: usize = 200; + #[derive(Deserialize)] pub(super) struct MarkTodosDoneForm { /// Comma-separated todo ids. Same "one field, JS joins the checked @@ -71,7 +74,10 @@ pub(super) struct MarkTodosDoneForm { /// single-id path); a request with no ids or where every id fails to parse /// is rejected as a client error rather than silently acking nothing. An id /// whose store call errors or gets no answer fails the request with a 500 -/// naming how many failed. +/// naming how many failed. More than [`MAX_MARK_DONE_IDS`] ids is a client +/// error, and the calls share one [`SOCKET_FETCH_TIMEOUT`]: ids the store has +/// not answered by then count as unanswered, so a stalled store gets its 500 +/// after about 3 seconds like the other handlers. pub(super) async fn post_mark_todos_done(Form(form): Form) -> Response { let ids: Vec = form .ids @@ -81,7 +87,13 @@ pub(super) async fn post_mark_todos_done(Form(form): Form) -> if ids.is_empty() { return error_response(StatusCode::BAD_REQUEST, "mark-done: no todo ids selected"); } - mark_ids_done(ids, |id| async move { + if ids.len() > MAX_MARK_DONE_IDS { + return error_response( + StatusCode::BAD_REQUEST, + &format!("mark-done: at most {MAX_MARK_DONE_IDS} todo ids per request"), + ); + } + mark_ids_done(ids, SOCKET_FETCH_TIMEOUT, |id| async move { crate::todo_server::dial(&hive_agent_sock::Request::MarkTodoDone { id }).await }) .await @@ -90,14 +102,20 @@ pub(super) async fn post_mark_todos_done(Form(form): Form) -> /// The handler body behind `dial`, which answers one id's `MarkTodoDone` /// (`None` when the store doesn't answer). Tests inject it so they never /// reach the live in-agent socket. -async fn mark_ids_done(ids: Vec, dial: F) -> Response +async fn mark_ids_done(ids: Vec, within: std::time::Duration, dial: F) -> Response where F: Fn(i64) -> Fut, Fut: Future>, { + let deadline = tokio::time::Instant::now() + within; let mut answers = Vec::with_capacity(ids.len()); for id in ids { - answers.push(dial(id).await); + // Past the deadline every remaining id is an unanswered one. + answers.push( + tokio::time::timeout_at(deadline, dial(id)) + .await + .unwrap_or(None), + ); } match settle_mark_done(answers) { Ok(acked) => axum::Json(MarkTodosDoneBody { acked }).into_response(), @@ -156,7 +174,10 @@ mod tests { #[tokio::test] async fn mark_done_handler_answers_500_with_the_message_when_the_store_does_not_answer() { - let response = mark_ids_done(vec![1, 2], |_| async { None }).await; + let response = mark_ids_done(vec![1, 2], std::time::Duration::from_secs(3), |_| async { + None + }) + .await; assert_eq!( response.status(), axum::http::StatusCode::INTERNAL_SERVER_ERROR @@ -168,7 +189,7 @@ mod tests { #[tokio::test] async fn mark_done_handler_answers_500_with_the_store_error() { - let response = mark_ids_done(vec![7], |_| async { + let response = mark_ids_done(vec![7], std::time::Duration::from_secs(3), |_| async { Some(hive_agent_sock::Response::Err { message: "database is locked".to_owned(), }) @@ -183,7 +204,7 @@ mod tests { #[tokio::test] async fn mark_done_handler_answers_200_with_the_acked_total() { - let response = mark_ids_done(vec![1, 2], |_| async { + let response = mark_ids_done(vec![1, 2], std::time::Duration::from_secs(3), |_| async { Some(hive_agent_sock::Response::Acked { count: 1 }) }) .await; @@ -191,6 +212,38 @@ mod tests { assert_eq!(body_text(response).await, r#"{"acked":2}"#); } + #[tokio::test(start_paused = true)] + async fn a_stalled_store_gets_one_deadline_for_all_ids_not_one_per_id() { + let started = tokio::time::Instant::now(); + let response = mark_ids_done( + (1..=50).collect(), + std::time::Duration::from_secs(3), + |_| async { + tokio::time::sleep(std::time::Duration::from_hours(1)).await; + None + }, + ) + .await; + assert_eq!( + response.status(), + axum::http::StatusCode::INTERNAL_SERVER_ERROR + ); + assert!(started.elapsed() < std::time::Duration::from_secs(4)); + assert!(body_text(response).await.contains("50 of 50 failed")); + } + + #[tokio::test] + async fn more_ids_than_the_cap_are_a_client_error() { + let ids = (0..=super::MAX_MARK_DONE_IDS) + .map(|id| id.to_string()) + .collect::>() + .join(","); + let response = + super::post_mark_todos_done(axum::Form(super::MarkTodosDoneForm { ids })).await; + assert_eq!(response.status(), axum::http::StatusCode::BAD_REQUEST); + assert!(body_text(response).await.contains("at most 200")); + } + #[test] fn mark_done_with_no_answer_is_an_error() { let err = settle_mark_done(vec![None]).unwrap_err();