mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-09-12 23:02:41 +00:00
fix(server): distinguish terminal run control conflicts
This commit is contained in:
parent
23c7e6fb2b
commit
185c474a5c
5 changed files with 199 additions and 71 deletions
|
|
@ -9,12 +9,12 @@ This plan is **not** a template for adding automated test coverage — it exists
|
|||
Live list of bugs and notable observations surfaced during the sweep. Each entry links back to the scenario where it was found.
|
||||
|
||||
### Bugs / mismatches
|
||||
- **I7 / I9 — Misleading "Run not found." on terminal runs**: `message` or `cancel` against a terminal run returns `Run not found.` (which means "not in managed-runs map"), not a status-appropriate error like "run is already terminal". Confusing to operators.
|
||||
- **I10 — Archived runs not filtered from default search**: `fabro_run_search` with no `archived` filter returns archived runs alongside active ones. Most systems hide archived by default; consider flipping the default.
|
||||
|
||||
### Rechecked / no longer open
|
||||
- **C4 — `inputs` schema/runtime mismatch**: fixed by narrowing MCP input values to scalar JSON (`string`, `boolean`, `integer`, `number`) and rejecting arrays/objects locally with scalar-only errors. Re-tested on 2026-05-11 against `127.0.0.1:32276`; `tools/list` now advertises scalar-only `inputs.additionalProperties`.
|
||||
- **C5 — Misleading null-input error message**: fixed. Re-tested on 2026-05-11; null now returns ``input `maybe` cannot be null; use a string, boolean, or number``.
|
||||
- **I7 / I9 — Misleading "Run not found." on terminal runs**: fixed on 2026-05-11 in the server API layer. `message`/steer against a durable terminal run that no longer has a live managed engine now returns `409` with `run_not_steerable`; `cancel` returns `409` with `Run is already terminal and cannot be cancelled.` True missing runs still return `404`.
|
||||
- **I15 / I16 — yes/no answer flow**: re-tested on 2026-05-11 against `fabro server` `0.230.0-nightly.0` at `127.0.0.1:32276`. `answer=true` and `answer=false` both submit successfully for the bundled `interview` workflow's first `yes_no` question. `true` advanced the run to the next `confirmation` question.
|
||||
- **I22 — numeric answer local validation**: re-tested on 2026-05-11 against the same server. `answer=42` now returns `unsupported answer value: 42; expected boolean, string, or object` from the MCP layer before reaching the API.
|
||||
- **X6 — Cursor/filter ordering**: simplified on 2026-05-11 by applying search filters before sorting and applying the `after` cursor. This prevents unrelated runs outside the filtered result set from trimming the page. Pagination is explicitly not snapshot-isolated; a new matching run inserted before the cursor during traversal appears when the client starts a new search.
|
||||
|
|
@ -184,11 +184,11 @@ Source: `run_tools/interact.rs:201`
|
|||
- [ ] **I4** Steer a running LLM agent — **DEFERRED** (requires an active LLM agent stage; would burn LLM tokens; can be exercised manually once the answer bug below is resolved).
|
||||
- [ ] **I5** `interrupt=true` — **DEFERRED** along with I4.
|
||||
- [x] **I6** Missing `message` → `message is required for action message`. — **PASS**.
|
||||
- [x] **I7** Message a terminal run → `Run not found.` — **FINDING / BUG**: should distinguish "run is terminal / no managed engine" from "run doesn't exist".
|
||||
- [x] **I7** Message a terminal run → initially returned `Run not found.`. — **FIXED**: durable terminal runs without a live managed engine now return `409 run_not_steerable`; true missing runs remain `404`.
|
||||
|
||||
#### `cancel`
|
||||
- [x] **I8** Cancel a `gh-list` run during `starting`. Returns summary at request time (status=`starting`). Subsequent `gather` returned terminal `failed` within 5s; `get` projection shows `status: {kind: "failed", reason: "cancelled"}` and `conclusion.failure_reason: "Pipeline cancelled"`. — **PASS** + **observation**: `cancel`'s returned summary is a snapshot at request time, not the eventual terminal status.
|
||||
- [x] **I9** Cancel an already-terminal run → `Run not found.` — **FINDING / BUG**: same misleading error as I7. Should say "run is terminal" instead.
|
||||
- [x] **I9** Cancel an already-terminal run → initially returned `Run not found.`. — **FIXED**: durable terminal runs without a live managed engine now return `409` with `Run is already terminal and cannot be cancelled.`; true missing runs remain `404`.
|
||||
|
||||
#### `archive` / `unarchive`
|
||||
- [x] **I10** Archive terminal run → `archived=true` in summary; visible via `search archived=true`. — **PASS**. **BUT FINDING**: archived runs are **not** filtered out of default search (no archive filter applied unless explicitly requested).
|
||||
|
|
|
|||
|
|
@ -2322,6 +2322,15 @@ async fn load_pending_control(
|
|||
.and_then(|summary| summary.lifecycle.pending_control))
|
||||
}
|
||||
|
||||
async fn durable_run_status(state: &AppState, run_id: RunId) -> anyhow::Result<Option<RunStatus>> {
|
||||
Ok(state
|
||||
.store
|
||||
.runs()
|
||||
.find(&run_id)
|
||||
.await?
|
||||
.map(|summary| summary.lifecycle.status))
|
||||
}
|
||||
|
||||
fn fail_managed_run(state: &Arc<AppState>, run_id: RunId, reason: FailureReason, message: String) {
|
||||
let mut runs = state.runs.lock().expect("runs lock poisoned");
|
||||
if let Some(managed_run) = runs.get_mut(&run_id) {
|
||||
|
|
|
|||
|
|
@ -5,7 +5,7 @@ use super::super::{
|
|||
Principal, RequiredUser, Response, RewindRequest, RewindResponse, Router, RunAnswerTransport,
|
||||
RunControlAction, RunExecutionMode, RunId, RunStatus, StartRunRequest, State, StatusCode,
|
||||
Storage, TimelineEntryResponse, WORKER_CANCEL_GRACE, WorkflowError, append_control_request,
|
||||
get, load_pending_control, managed_run, operations, parse_run_id_path,
|
||||
durable_run_status, get, load_pending_control, managed_run, operations, parse_run_id_path,
|
||||
persist_cancelled_run_status, post, reject_if_archived, sleep, update_live_run_from_event,
|
||||
workflow_event,
|
||||
};
|
||||
|
|
@ -171,7 +171,7 @@ async fn cancel_run(
|
|||
.into_response();
|
||||
}
|
||||
};
|
||||
let (persist_cancelled_status, answer_transport, cancel_token, cancel_tx, worker_pid) = {
|
||||
let cancel_target = {
|
||||
let mut runs = state.runs.lock().expect("runs lock poisoned");
|
||||
match runs.get_mut(&id) {
|
||||
Some(managed_run) => match managed_run.status {
|
||||
|
|
@ -192,7 +192,7 @@ async fn cancel_run(
|
|||
reason: FailureReason::Cancelled,
|
||||
};
|
||||
}
|
||||
(
|
||||
Some((
|
||||
persist_cancelled_status,
|
||||
managed_run.answer_transport.clone(),
|
||||
managed_run.cancel_token.clone(),
|
||||
|
|
@ -200,16 +200,21 @@ async fn cancel_run(
|
|||
.then(|| managed_run.cancel_tx.take())
|
||||
.flatten(),
|
||||
managed_run.worker_pid,
|
||||
)
|
||||
))
|
||||
}
|
||||
_ => {
|
||||
return ApiError::new(StatusCode::CONFLICT, "Run is not cancellable.")
|
||||
.into_response();
|
||||
}
|
||||
},
|
||||
None => return ApiError::not_found("Run not found.").into_response(),
|
||||
None => None,
|
||||
}
|
||||
};
|
||||
let Some((persist_cancelled_status, answer_transport, cancel_token, cancel_tx, worker_pid)) =
|
||||
cancel_target
|
||||
else {
|
||||
return unmanaged_cancel_response(state.as_ref(), id).await;
|
||||
};
|
||||
|
||||
if pending_control != Some(RunControlAction::Cancel) {
|
||||
if let Err(err) = append_control_request(
|
||||
|
|
@ -256,6 +261,23 @@ async fn cancel_run(
|
|||
run_response(state.as_ref(), id, StatusCode::OK).await
|
||||
}
|
||||
|
||||
async fn unmanaged_cancel_response(state: &AppState, id: RunId) -> Response {
|
||||
match durable_run_status(state, id).await {
|
||||
Ok(Some(status)) if status.is_terminal() => ApiError::new(
|
||||
StatusCode::CONFLICT,
|
||||
"Run is already terminal and cannot be cancelled.",
|
||||
)
|
||||
.into_response(),
|
||||
Ok(Some(_)) => {
|
||||
ApiError::new(StatusCode::CONFLICT, "Run is not cancellable.").into_response()
|
||||
}
|
||||
Ok(None) => ApiError::not_found("Run not found.").into_response(),
|
||||
Err(err) => {
|
||||
ApiError::new(StatusCode::INTERNAL_SERVER_ERROR, err.to_string()).into_response()
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// How `pause_run` should enact the transition, chosen from the current run
|
||||
/// status.
|
||||
enum PauseMode {
|
||||
|
|
|
|||
|
|
@ -9,7 +9,9 @@ use fabro_api::types::SteerRunRequest;
|
|||
use fabro_types::Principal;
|
||||
use fabro_workflow::run_status::RunStatus;
|
||||
|
||||
use super::super::{AnswerTransportError, AppState, parse_run_id_path, reject_if_archived};
|
||||
use super::super::{
|
||||
AnswerTransportError, AppState, durable_run_status, parse_run_id_path, reject_if_archived,
|
||||
};
|
||||
use crate::error::ApiError;
|
||||
use crate::principal_middleware::RequiredUser;
|
||||
|
||||
|
|
@ -77,73 +79,72 @@ async fn control_run(
|
|||
|
||||
// Status + steerability gate. Take the answer_transport snapshot under
|
||||
// the same lock so we can hand it off without further state races.
|
||||
let answer_transport = {
|
||||
let managed_answer_transport = {
|
||||
let runs = state.runs.lock().expect("runs lock poisoned");
|
||||
let Some(managed_run) = runs.get(&id) else {
|
||||
return ApiError::not_found("Run not found.").into_response();
|
||||
};
|
||||
match managed_run.status {
|
||||
RunStatus::Blocked { .. } => {
|
||||
return ApiError::with_code(
|
||||
StatusCode::CONFLICT,
|
||||
"Run is blocked on a question; use the interview-answer endpoint instead.",
|
||||
"use_answer_endpoint",
|
||||
)
|
||||
.into_response();
|
||||
match runs.get(&id) {
|
||||
Some(managed_run) => {
|
||||
match managed_run.status {
|
||||
RunStatus::Blocked { .. } => {
|
||||
return ApiError::with_code(
|
||||
StatusCode::CONFLICT,
|
||||
"Run is blocked on a question; use the interview-answer endpoint \
|
||||
instead.",
|
||||
"use_answer_endpoint",
|
||||
)
|
||||
.into_response();
|
||||
}
|
||||
RunStatus::Submitted
|
||||
| RunStatus::Queued
|
||||
| RunStatus::Starting
|
||||
| RunStatus::Paused { .. } => {
|
||||
return ApiError::with_code(
|
||||
StatusCode::CONFLICT,
|
||||
"Run is not currently running.",
|
||||
"run_not_steerable",
|
||||
)
|
||||
.into_response();
|
||||
}
|
||||
RunStatus::Failed { .. }
|
||||
| RunStatus::Succeeded { .. }
|
||||
| RunStatus::Removing
|
||||
| RunStatus::Dead => {
|
||||
return terminal_control_response(&control);
|
||||
}
|
||||
RunStatus::Running => {}
|
||||
}
|
||||
// Steerability predicate. Best-effort, target-oriented:
|
||||
// - If at least one API-mode session is active → forward.
|
||||
// - Else if no agent stages are active at all → forward (worker hub buffers
|
||||
// for the next session).
|
||||
// - Else (active agents exist but all are CLI-mode) → 409.
|
||||
if managed_run.active_api_stages.is_empty()
|
||||
&& !managed_run.active_cli_stages.is_empty()
|
||||
{
|
||||
return ApiError::with_code(
|
||||
StatusCode::CONFLICT,
|
||||
"All currently running agent stages are CLI-mode and cannot be steered.",
|
||||
"cli_agent_not_steerable",
|
||||
)
|
||||
.into_response();
|
||||
}
|
||||
if managed_run.active_api_stages.is_empty() && control.requires_active_api_session()
|
||||
{
|
||||
return ApiError::with_code(
|
||||
StatusCode::CONFLICT,
|
||||
"Run has no active API-mode agent session.",
|
||||
"no_active_api_session",
|
||||
)
|
||||
.into_response();
|
||||
}
|
||||
Some(managed_run.answer_transport.clone())
|
||||
}
|
||||
RunStatus::Submitted
|
||||
| RunStatus::Queued
|
||||
| RunStatus::Starting
|
||||
| RunStatus::Paused { .. } => {
|
||||
return ApiError::with_code(
|
||||
StatusCode::CONFLICT,
|
||||
"Run is not currently running.",
|
||||
"run_not_steerable",
|
||||
)
|
||||
.into_response();
|
||||
}
|
||||
RunStatus::Failed { .. }
|
||||
| RunStatus::Succeeded { .. }
|
||||
| RunStatus::Removing
|
||||
| RunStatus::Dead => {
|
||||
let code = if matches!(&control, RunControlRequest::Interrupt) {
|
||||
"run_not_interruptible"
|
||||
} else {
|
||||
"run_not_steerable"
|
||||
};
|
||||
return ApiError::with_code(
|
||||
StatusCode::CONFLICT,
|
||||
"Run is no longer steerable.",
|
||||
code,
|
||||
)
|
||||
.into_response();
|
||||
}
|
||||
RunStatus::Running => {}
|
||||
None => None,
|
||||
}
|
||||
// Steerability predicate. Best-effort, target-oriented:
|
||||
// - If at least one API-mode session is active → forward.
|
||||
// - Else if no agent stages are active at all → forward (worker hub buffers
|
||||
// for the next session).
|
||||
// - Else (active agents exist but all are CLI-mode) → 409.
|
||||
if managed_run.active_api_stages.is_empty() && !managed_run.active_cli_stages.is_empty() {
|
||||
return ApiError::with_code(
|
||||
StatusCode::CONFLICT,
|
||||
"All currently running agent stages are CLI-mode and cannot be steered.",
|
||||
"cli_agent_not_steerable",
|
||||
)
|
||||
.into_response();
|
||||
}
|
||||
if managed_run.active_api_stages.is_empty() && control.requires_active_api_session() {
|
||||
return ApiError::with_code(
|
||||
StatusCode::CONFLICT,
|
||||
"Run has no active API-mode agent session.",
|
||||
"no_active_api_session",
|
||||
)
|
||||
.into_response();
|
||||
}
|
||||
managed_run.answer_transport.clone()
|
||||
};
|
||||
|
||||
let Some(answer_transport) = managed_answer_transport else {
|
||||
return unmanaged_control_response(state.as_ref(), id, &control).await;
|
||||
};
|
||||
let Some(answer_transport) = answer_transport else {
|
||||
return ApiError::with_code(
|
||||
StatusCode::SERVICE_UNAVAILABLE,
|
||||
|
|
@ -178,3 +179,32 @@ async fn control_run(
|
|||
.into_response(),
|
||||
}
|
||||
}
|
||||
|
||||
fn terminal_control_response(control: &RunControlRequest) -> Response {
|
||||
let code = if matches!(control, RunControlRequest::Interrupt) {
|
||||
"run_not_interruptible"
|
||||
} else {
|
||||
"run_not_steerable"
|
||||
};
|
||||
ApiError::with_code(StatusCode::CONFLICT, "Run is no longer steerable.", code).into_response()
|
||||
}
|
||||
|
||||
async fn unmanaged_control_response(
|
||||
state: &AppState,
|
||||
id: fabro_types::RunId,
|
||||
control: &RunControlRequest,
|
||||
) -> Response {
|
||||
match durable_run_status(state, id).await {
|
||||
Ok(Some(status)) if status.is_terminal() => terminal_control_response(control),
|
||||
Ok(Some(_)) => ApiError::with_code(
|
||||
StatusCode::SERVICE_UNAVAILABLE,
|
||||
"Run has no live worker control channel.",
|
||||
"worker_control_unavailable",
|
||||
)
|
||||
.into_response(),
|
||||
Ok(None) => ApiError::not_found("Run not found.").into_response(),
|
||||
Err(err) => {
|
||||
ApiError::new(StatusCode::INTERNAL_SERVER_ERROR, err.to_string()).into_response()
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -6877,6 +6877,40 @@ async fn cancel_nonexistent_run_returns_not_found() {
|
|||
assert_status!(response, StatusCode::NOT_FOUND).await;
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn cancel_terminal_durable_run_returns_conflict() {
|
||||
let state = test_app_state();
|
||||
let app = crate::test_support::build_test_router(Arc::clone(&state));
|
||||
let run_id = fixtures::RUN_1;
|
||||
create_durable_run_with_events(&state, run_id, &[
|
||||
workflow_event::Event::WorkflowRunCompleted {
|
||||
duration_ms: 1000,
|
||||
artifact_count: 0,
|
||||
status: "succeeded".to_string(),
|
||||
reason: SuccessReason::Completed,
|
||||
total_usd_micros: None,
|
||||
final_git_commit_sha: None,
|
||||
final_patch: None,
|
||||
diff_summary: None,
|
||||
billing: None,
|
||||
},
|
||||
])
|
||||
.await;
|
||||
|
||||
let req = Request::builder()
|
||||
.method("POST")
|
||||
.uri(api(&format!("/runs/{run_id}/cancel")))
|
||||
.body(Body::empty())
|
||||
.unwrap();
|
||||
|
||||
let response = app.oneshot(req).await.unwrap();
|
||||
let body = response_json!(response, StatusCode::CONFLICT).await;
|
||||
assert_eq!(
|
||||
body["errors"][0]["detail"],
|
||||
"Run is already terminal and cannot be cancelled."
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn steer_nonexistent_run_returns_not_found() {
|
||||
let app = test_app_with();
|
||||
|
|
@ -6893,6 +6927,39 @@ async fn steer_nonexistent_run_returns_not_found() {
|
|||
assert_status!(response, StatusCode::NOT_FOUND).await;
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn steer_terminal_durable_run_returns_run_not_steerable() {
|
||||
let state = test_app_state();
|
||||
let app = crate::test_support::build_test_router(Arc::clone(&state));
|
||||
let run_id = fixtures::RUN_1;
|
||||
create_durable_run_with_events(&state, run_id, &[
|
||||
workflow_event::Event::WorkflowRunCompleted {
|
||||
duration_ms: 1000,
|
||||
artifact_count: 0,
|
||||
status: "succeeded".to_string(),
|
||||
reason: SuccessReason::Completed,
|
||||
total_usd_micros: None,
|
||||
final_git_commit_sha: None,
|
||||
final_patch: None,
|
||||
diff_summary: None,
|
||||
billing: None,
|
||||
},
|
||||
])
|
||||
.await;
|
||||
|
||||
let req = Request::builder()
|
||||
.method("POST")
|
||||
.uri(api(&format!("/runs/{run_id}/steer")))
|
||||
.header("content-type", "application/json")
|
||||
.body(Body::from(r#"{"text":"try again"}"#))
|
||||
.unwrap();
|
||||
|
||||
let response = app.oneshot(req).await.unwrap();
|
||||
let body = response_json!(response, StatusCode::CONFLICT).await;
|
||||
assert_eq!(body["errors"][0]["code"], "run_not_steerable");
|
||||
assert_eq!(body["errors"][0]["detail"], "Run is no longer steerable.");
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn steer_empty_text_returns_bad_request() {
|
||||
let state = test_app_state();
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue