Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/core/socketio.rs
Original file line number Diff line number Diff line change
Expand Up @@ -363,6 +363,11 @@ struct ChatStartPayload {
#[derive(Debug, Deserialize)]
struct ChatCancelPayload {
thread_id: String,
/// The request this cancel targets. When the client passes the id of the
/// turn it started, the cancel is scoped to that turn so a late cancel for a
/// timed-out request can't kill the next turn on the thread (#4760).
#[serde(default)]
request_id: Option<String>,
}

#[derive(Debug, Deserialize)]
Expand Down Expand Up @@ -551,9 +556,10 @@ pub fn attach_socketio() -> (socketioxide::layer::SocketIoLayer, SocketIo) {
client_id,
payload.thread_id
);
let _ = crate::openhuman::channels::providers::web::cancel_chat(
let _ = crate::openhuman::channels::providers::web::cancel_chat_scoped(
&client_id,
&payload.thread_id,
payload.request_id.as_deref(),
)
.await;
},
Expand Down
2 changes: 1 addition & 1 deletion src/openhuman/agent/task_dispatcher/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ mod tests;
pub use dispatch::dispatch_card;
pub use poller::start_board_poller;
pub use prompt::build_task_prompt;
pub use registry::cancel_session;
pub use registry::{cancel_session, cancel_session_scoped};
pub use types::DispatchOutcome;

/// Run a one-off **system** agent turn on an existing chat thread, streaming the
Expand Down
25 changes: 25 additions & 0 deletions src/openhuman/agent/task_dispatcher/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,3 +72,28 @@ pub async fn cancel_session(thread_id: &str) -> bool {
);
true
}

/// Request-scoped variant of [`cancel_session`].
///
/// When `request_id` is `Some`, the active run is aborted only if its `run_id`
/// matches — a scoped cancel for a superseded or unrelated request is a no-op so
/// it can't tear down a newer autonomous run on the thread (#4760). When
/// `request_id` is `None`, this behaves exactly like [`cancel_session`] (stop
/// whatever run is on the thread — the Stop button / session-teardown path).
/// Returns `true` if a run was found and cancelled.
pub async fn cancel_session_scoped(thread_id: &str, request_id: Option<&str>) -> bool {
if let Some(rid) = request_id {
// Peek (don't remove) so a non-matching scoped cancel leaves the run
// intact for the request that actually owns it.
let matches = active_runs()
.lock()
.expect("active_runs mutex poisoned")
.get(thread_id)
.map(|run| run.run_id == rid)
.unwrap_or(false);
if !matches {
return false;
}
}
cancel_session(thread_id).await
}
87 changes: 86 additions & 1 deletion src/openhuman/agent/task_dispatcher/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use crate::openhuman::todos::ops::{self, BoardLocation, CardPatch};
use super::executor::{truncate_chars, write_back, EVIDENCE_MAX_CHARS};
use super::poller::{pick_next_todo, requires_plan_approval};
use super::prompt::{build_progress_instruction, build_task_prompt};
use super::registry::{register_active_run, take_active_run};
use super::registry::{cancel_session_scoped, register_active_run, take_active_run};
use super::types::ActiveRun;

#[tokio::test]
Expand Down Expand Up @@ -36,6 +36,91 @@ async fn active_run_registry_take_is_once() {
handle.abort();
}

#[tokio::test]
async fn cancel_session_scoped_returns_false_without_a_run() {
// No autonomous run on the thread — scoped and unscoped both no-op.
assert!(!cancel_session_scoped("no-such-thread-xyz", Some("r1")).await);
assert!(!cancel_session_scoped("no-such-thread-xyz", None).await);
}

#[tokio::test]
async fn cancel_session_scoped_ignores_a_mismatched_request() {
// #4760: a scoped cancel whose request_id does not match the in-flight run's
// run_id must NOT abort it — the run survives for the request that owns it,
// so a stale cancel for a superseded request can't kill a newer run.
let (tx, _rx) = tokio::sync::watch::channel(false);
let handle = tokio::spawn(async { std::future::pending::<()>().await });
let key = "task-cancel-scoped-mismatch-test";
register_active_run(
key.to_string(),
ActiveRun {
abort: handle.abort_handle(),
hb_cancel: tx,
location: BoardLocation::Scratch,
card_id: "c1".to_string(),
run_id: "r1".to_string(),
},
);
assert!(
!cancel_session_scoped(key, Some("some-other-request")).await,
"a scoped cancel naming a different request must be a no-op"
);
assert!(
take_active_run(key).is_some(),
"the mismatched scoped cancel must leave the run in flight"
);
handle.abort();
}

#[tokio::test]
async fn cancel_session_scoped_aborts_the_run_when_the_request_matches() {
// The matching-request path: a scoped cancel that names the running turn
// tears it down and lands the card in a terminal (blocked) state.
let dir = tempfile::tempdir().unwrap();
let loc = board_loc(dir.path());
let id = ops::add(&loc, "run me", CardPatch::default())
.unwrap()
.cards[0]
.id
.clone();
ops::update_status(&loc, &id, TaskCardStatus::InProgress).unwrap();

let (tx, _rx) = tokio::sync::watch::channel(false);
let handle = tokio::spawn(async { std::future::pending::<()>().await });
let key = "task-cancel-scoped-match-test";
register_active_run(
key.to_string(),
ActiveRun {
abort: handle.abort_handle(),
hb_cancel: tx,
location: loc.clone(),
card_id: id.clone(),
run_id: "r1".to_string(),
},
);

assert!(
cancel_session_scoped(key, Some("r1")).await,
"a scoped cancel naming the running request must abort it"
);
assert!(
take_active_run(key).is_none(),
"the cancelled run is removed from the registry"
);
let card = ops::list(&loc)
.unwrap()
.cards
.into_iter()
.find(|c| c.id == id)
.unwrap();
assert_eq!(
card.status,
TaskCardStatus::Blocked,
"cancellation lands the card in a terminal state, not stale in_progress"
);
handle.abort();
}

fn card(objective: Option<&str>) -> TaskBoardCard {
TaskBoardCard {
id: "task-1".into(),
Expand Down
5 changes: 3 additions & 2 deletions src/openhuman/channels/providers/web/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,9 @@ pub use event_bus::fresh_approval_surface_subscription;
#[cfg(any(test, debug_assertions))]
pub use ops::parallel_in_flight_entries_for_test;
pub use ops::{
cancel_chat, channel_web_cancel, channel_web_chat, channel_web_queue_clear,
channel_web_queue_status, in_flight_entries_for_test, invalidate_thread_sessions, start_chat,
cancel_chat, cancel_chat_scoped, cancel_should_target, channel_web_cancel, channel_web_chat,
channel_web_queue_clear, channel_web_queue_status, in_flight_entries_for_test,
invalidate_thread_sessions, start_chat,
};
pub use types::ChatRequestMetadata;

Expand Down
131 changes: 121 additions & 10 deletions src/openhuman/channels/providers/web/ops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -932,7 +932,78 @@ pub async fn parallel_in_flight_entries_for_test() -> Vec<(String, String)> {
.collect()
}

/// Whether a cancel request should tear down the turn currently in flight for a
/// thread.
///
/// `requested` is the `request_id` the caller is cancelling; `None` means an
/// unscoped stop ("cancel whatever is running", e.g. a Stop button or a session
/// teardown). `in_flight` is the `request_id` currently registered for the
/// thread.
///
/// A *scoped* cancel matches only its own request. This is the fix for #4760: a
/// client that times out on request A and then sends request B — which
/// supersedes A on the same thread — must not have A's late-arriving cancel tear
/// down B. Scoping the cancel to A makes it a no-op once B is in flight, so the
/// newer turn survives instead of being killed at t=0.
pub fn cancel_should_target(requested: Option<&str>, in_flight: &str) -> bool {
match requested {
Some(rid) => rid == in_flight,
None => true,
}
}

/// Cancel a single parallel (forked) turn identified by `request_id`, but only
/// when it belongs to `thread_id`. Returns the cancelled id (as a one-element
/// vec, mirroring [`cancel_parallel_turns_for_thread`]) or empty when no such
/// parallel turn exists. Request-scoped cancel path (#4760).
async fn cancel_parallel_turn_by_request_id(thread_id: &str, request_id: &str) -> Vec<String> {
let mut parallel = PARALLEL_IN_FLIGHT.lock().await;
let matches = parallel
.get(request_id)
.map(|entry| entry.thread_id == thread_id)
.unwrap_or(false);
if !matches {
return Vec::new();
}
if let Some(entry) = parallel.remove(request_id) {
entry.cancel_token.cancel();
let mut handle = entry.handle;
tokio::spawn(async move {
tokio::select! {
_ = &mut handle => {}
_ = tokio::time::sleep(Duration::from_secs(5)) => {
handle.abort();
}
}
});
return vec![request_id.to_string()];
}
Vec::new()
}

/// Cancel whatever turn is currently running on a thread (unscoped stop).
///
/// Back-compat entry point (Stop button / session teardown). For a cancel that
/// must only affect a specific turn — so a stale cancel can't kill a newer turn
/// on the same thread — use [`cancel_chat_scoped`] with the target `request_id`
/// (#4760).
pub async fn cancel_chat(client_id: &str, thread_id: &str) -> Result<Option<String>, String> {
cancel_chat_scoped(client_id, thread_id, None).await
}

/// Cancel the in-flight turn(s) for a thread.
///
/// When `request_id` is `Some`, the cancel is **scoped**: it only tears down the
/// primary turn if that exact request is still running (and only the matching
/// parallel turn), so a stale cancel for a superseded request can't kill the
/// newer turn that replaced it (#4760). When `request_id` is `None`, it stops
/// whatever is running on the thread (primary + every parallel) — the "stop
/// everything" behaviour used by session teardown / a Stop button.
pub async fn cancel_chat_scoped(
client_id: &str,
thread_id: &str,
request_id: Option<&str>,
) -> Result<Option<String>, String> {
let client_id = client_id.trim();
let thread_id = thread_id.trim();

Expand All @@ -948,18 +1019,47 @@ pub async fn cancel_chat(client_id: &str, thread_id: &str) -> Result<Option<Stri

{
let mut in_flight = IN_FLIGHT.lock().await;
if let Some(existing) = in_flight.remove(&map_key) {
removed_request_id = Some(cancel_in_flight_gracefully(existing));
// #4760: only tear down the primary turn when the cancel is unscoped OR
// targets exactly the request that is running. A stale cancel for an
// already-superseded request must be a no-op so the newer turn lives.
let should_cancel_primary = in_flight
.get(&map_key)
.map(|entry| cancel_should_target(request_id, &entry.request_id))
.unwrap_or(false);
if should_cancel_primary {
if let Some(existing) = in_flight.remove(&map_key) {
removed_request_id = Some(cancel_in_flight_gracefully(existing));
}
} else if let Some(rid) = request_id {
log::info!(
"[web-channel] ignoring stale cancel request_id={} for thread_id={} — current in-flight is {:?}; newer turn preserved",
rid,
thread_id,
in_flight.get(&map_key).map(|e| e.request_id.as_str())
);
}
}

// Also tear down any concurrent parallel (forked) turns on the thread so a
// cancel/stop covers every in-flight turn, not just the primary one.
let cancelled_parallel = cancel_parallel_turns_for_thread(thread_id).await;
// Also tear down concurrent parallel (forked) turns. A scoped cancel targets
// only the named parallel turn (if it is one); an unscoped cancel/stop
// covers every parallel turn on the thread, not just the primary one.
let cancelled_parallel = match request_id {
Some(rid) => cancel_parallel_turn_by_request_id(thread_id, rid).await,
None => cancel_parallel_turns_for_thread(thread_id).await,
};

// #4760: a scoped cancel that matched only a parallel (forked) turn — not the
// primary — still genuinely tore a turn down and emitted its cancelled event.
// Surface that id so `channel_web_cancel` reports `cancelled: true` with the
// right request_id instead of misreporting a no-op just because the primary
// turn wasn't the one cancelled.
let cancelled_any = removed_request_id
.clone()
.or_else(|| cancelled_parallel.first().cloned());

// Emit a cancelled chat_error for each cancelled turn (primary + parallels)
// so every interleaved branch's UI is resolved.
for request_id in removed_request_id.iter().cloned().chain(cancelled_parallel) {
for request_id in removed_request_id.into_iter().chain(cancelled_parallel) {
publish_web_channel_event(WebChannelEvent {
event: "chat_error".to_string(),
client_id: client_id.to_string(),
Expand All @@ -971,7 +1071,7 @@ pub async fn cancel_chat(client_id: &str, thread_id: &str) -> Result<Option<Stri
});
}

Ok(removed_request_id)
Ok(cancelled_any)
}

pub async fn channel_web_chat(
Expand Down Expand Up @@ -1078,13 +1178,24 @@ pub async fn channel_web_queue_clear(thread_id: &str) -> Result<RpcOutcome<Value
pub async fn channel_web_cancel(
client_id: &str,
thread_id: &str,
request_id: Option<&str>,
) -> Result<RpcOutcome<Value>, String> {
let cancelled_request_id = cancel_chat(client_id, thread_id).await?;

let cancelled_request_id = cancel_chat_scoped(client_id, thread_id, request_id).await?;

// No web-channel turn matched. Fall through to the task-dispatcher registry,
// which holds autonomous runs that are NOT web-channel turns (so they never
// appear in IN_FLIGHT and can only be reached here). The fallback is itself
// request-scoped: a scoped cancel aborts the run only when its run_id
// matches, so a stale cancel for a superseded request can't tear down a newer
// run on the thread (#4760); an unscoped stop aborts whatever run is running.
let cancelled = if cancelled_request_id.is_some() {
true
} else {
crate::openhuman::agent::task_dispatcher::cancel_session(thread_id.trim()).await
crate::openhuman::agent::task_dispatcher::cancel_session_scoped(
thread_id.trim(),
request_id,
)
.await
};

Ok(RpcOutcome::single_log(
Expand Down
6 changes: 5 additions & 1 deletion src/openhuman/channels/providers/web/schemas.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,10 @@ pub fn schemas(function: &str) -> ControllerSchema {
inputs: vec![
required_string("client_id", "Client stream identifier."),
required_string("thread_id", "Thread identifier."),
optional_string(
"request_id",
"Request id to cancel. When set, only that turn is cancelled (a stale cancel for a superseded request is ignored so the newer turn survives). Omit to stop whatever is running on the thread.",
),
],
outputs: vec![json_output("ack", "Cancellation payload.")],
},
Expand Down Expand Up @@ -149,7 +153,7 @@ fn handle_queue_clear(params: Map<String, Value>) -> ControllerFuture {
fn handle_cancel(params: Map<String, Value>) -> ControllerFuture {
Box::pin(async move {
let p = deserialize_params::<WebCancelParams>(params)?;
to_json(channel_web_cancel(&p.client_id, &p.thread_id).await?)
to_json(channel_web_cancel(&p.client_id, &p.thread_id, p.request_id.as_deref()).await?)
Comment thread
M3gA-Mind marked this conversation as resolved.
})
}

Expand Down
5 changes: 5 additions & 0 deletions src/openhuman/channels/providers/web/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -148,4 +148,9 @@ pub(super) struct WebQueueParams {
pub(super) struct WebCancelParams {
pub(super) client_id: String,
pub(super) thread_id: String,
/// The `request_id` this cancel targets. When present, the cancel is scoped
/// to that exact turn, so a stale cancel for a superseded request can't kill
/// the newer turn on the thread (#4760). Absent = stop whatever is running.
#[serde(default)]
pub(super) request_id: Option<String>,
}
4 changes: 2 additions & 2 deletions tests/channels_web_startup_raw_coverage_e2e.rs
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ async fn web_controllers_validate_inputs_and_emit_structured_forced_errors() {
.expect_err("blank messages are rejected");
assert!(err.contains("message is required"));

let cancel = channel_web_cancel("client", "missing-thread")
let cancel = channel_web_cancel("client", "missing-thread", None)
.await
.expect("cancel without in-flight request is ok")
.into_cli_compatible_json()
Expand Down Expand Up @@ -201,7 +201,7 @@ async fn web_chat_cancel_aborts_in_flight_thread_without_real_provider() {
.await
.expect("start chat");

let cancel = channel_web_cancel("cancel-client", "cancel-thread")
let cancel = channel_web_cancel("cancel-client", "cancel-thread", None)
.await
.expect("cancel")
.into_cli_compatible_json()
Expand Down
Loading
Loading