From 9adf24348babe4eca83fe68270cc35af44501809 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Thu, 23 Jul 2026 21:15:35 -0400 Subject: [PATCH] refactor: simplify cancellation lifecycle code from review - Extract the quadruplicated watchdog check-and-clear logic in schedule_worker_cancel_escalation into ManagedRun methods (escalation_still_current, clear_escalation_for) - Derive strum::IntoStaticStr for WorkerRef instead of a hand-written variant-to-string match in kind() - Use the generated AgentControlState constant instead of the raw "waiting_for_steer" literal in run-detail.tsx - Replace optimisticCancellationRunId state with a boolean; the component is keyed by run id, so the stored id could only ever be this run's own Co-Authored-By: Claude Fable 5 --- .../components/runs-list/row-actions-menu.tsx | 7 ++-- apps/fabro-web/app/routes/run-detail.tsx | 3 +- lib/apps/fabro-server/src/server.rs | 22 +++++++++++++ .../src/server/handler/lifecycle.rs | 32 ++++--------------- lib/apps/fabro-server/src/worker_runtime.rs | 9 +++--- 5 files changed, 37 insertions(+), 36 deletions(-) diff --git a/apps/fabro-web/app/components/runs-list/row-actions-menu.tsx b/apps/fabro-web/app/components/runs-list/row-actions-menu.tsx index 49bd39ace..9a7787869 100644 --- a/apps/fabro-web/app/components/runs-list/row-actions-menu.tsx +++ b/apps/fabro-web/app/components/runs-list/row-actions-menu.tsx @@ -36,8 +36,7 @@ export function RowActionsMenu({ run }: { run: RunWithStatus }) { const { mutate } = useSWRConfig(); const { push } = useToast(); const [pendingAction, setPendingAction] = useState(null); - const [optimisticCancellationRunId, setOptimisticCancellationRunId] = - useState(null); + const [optimisticallyCancelled, setOptimisticallyCancelled] = useState(false); const [deleteDialogOpen, setDeleteDialogOpen] = useState(false); const [idCopied, setIdCopied] = useState(false); @@ -52,7 +51,7 @@ export function RowActionsMenu({ run }: { run: RunWithStatus }) { const cancellationPending = isCancellationPendingState( status, run.pendingControl, - pendingAction === "cancel" || optimisticCancellationRunId === run.id, + pendingAction === "cancel" || optimisticallyCancelled, ); const pending = pendingAction !== null || cancellationPending; @@ -69,7 +68,7 @@ export function RowActionsMenu({ run }: { run: RunWithStatus }) { try { const result = await action(); if (label === "cancel") { - setOptimisticCancellationRunId(run.id); + setOptimisticallyCancelled(true); } push({ message: diff --git a/apps/fabro-web/app/routes/run-detail.tsx b/apps/fabro-web/app/routes/run-detail.tsx index a5613dae9..97ce00702 100644 --- a/apps/fabro-web/app/routes/run-detail.tsx +++ b/apps/fabro-web/app/routes/run-detail.tsx @@ -9,6 +9,7 @@ import { useMatches, useNavigate, } from "react-router"; +import { AgentControlState } from "@qltysh/fabro-api-client"; import { type SteerBarHandle } from "../components/steer-bar"; import { ErrorState } from "../components/state"; @@ -107,7 +108,7 @@ export default function RunDetail({ params }: { params: { id: string } }) { const childrenCount = runQuery.data?.children_count ?? null; const hasSandbox = runHasSandbox(runStateQuery.data); const waitingForSteer = Object.values(runStateQuery.data?.stages ?? {}).some( - (stage) => stage.agent_control === "waiting_for_steer", + (stage) => stage.agent_control === AgentControlState.WAITING_FOR_STEER, ); const tabs = buildRunDetailTabs({ hasSandbox, diff --git a/lib/apps/fabro-server/src/server.rs b/lib/apps/fabro-server/src/server.rs index 1dd5548ed..fac2dd51a 100644 --- a/lib/apps/fabro-server/src/server.rs +++ b/lib/apps/fabro-server/src/server.rs @@ -288,6 +288,28 @@ struct ManagedRun { execution_mode: RunExecutionMode, } +impl ManagedRun { + /// True if cancellation should still escalate to `worker_ref`; clears a + /// stale escalation marker as a side effect. + fn escalation_still_current(&mut self, worker_ref: &WorkerRef) -> bool { + let matches_watchdog = self.cancel_escalation_worker.as_ref() == Some(worker_ref); + let still_current = matches_watchdog + && !self.status.is_terminal() + && self.worker_ref.as_ref() == Some(worker_ref); + if matches_watchdog && !still_current { + self.cancel_escalation_worker = None; + } + still_current + } + + /// Clears the escalation marker if it is still owned by `worker_ref`. + fn clear_escalation_for(&mut self, worker_ref: &WorkerRef) { + if self.cancel_escalation_worker.as_ref() == Some(worker_ref) { + self.cancel_escalation_worker = None; + } + } +} + #[derive(Clone, Copy)] enum RunExecutionMode { Start, diff --git a/lib/apps/fabro-server/src/server/handler/lifecycle.rs b/lib/apps/fabro-server/src/server/handler/lifecycle.rs index f19e35d3b..fe4020aba 100644 --- a/lib/apps/fabro-server/src/server/handler/lifecycle.rs +++ b/lib/apps/fabro-server/src/server/handler/lifecycle.rs @@ -381,14 +381,7 @@ fn schedule_worker_cancel_escalation(state: Arc, run_id: RunId, worker let Some(run) = runs.get_mut(&run_id) else { return; }; - let matches_watchdog = run.cancel_escalation_worker.as_ref() == Some(&worker_ref); - let should_escalate = matches_watchdog - && !run.status.is_terminal() - && run.worker_ref.as_ref() == Some(&worker_ref); - if matches_watchdog && !should_escalate { - run.cancel_escalation_worker = None; - } - should_escalate + run.escalation_still_current(&worker_ref) }; if !should_escalate { tracing::debug!( @@ -401,11 +394,8 @@ fn schedule_worker_cancel_escalation(state: Arc, run_id: RunId, worker } if !state.worker_runtime.is_alive(&worker_ref).await { let mut runs = state.runs.lock().expect("runs lock poisoned"); - if let Some(run) = runs - .get_mut(&run_id) - .filter(|run| run.cancel_escalation_worker.as_ref() == Some(&worker_ref)) - { - run.cancel_escalation_worker = None; + if let Some(run) = runs.get_mut(&run_id) { + run.clear_escalation_for(&worker_ref); } tracing::debug!( run_id = %run_id, @@ -420,14 +410,7 @@ fn schedule_worker_cancel_escalation(state: Arc, run_id: RunId, worker let Some(run) = runs.get_mut(&run_id) else { return; }; - let matches_watchdog = run.cancel_escalation_worker.as_ref() == Some(&worker_ref); - let still_current = matches_watchdog - && !run.status.is_terminal() - && run.worker_ref.as_ref() == Some(&worker_ref); - if matches_watchdog && !still_current { - run.cancel_escalation_worker = None; - } - still_current + run.escalation_still_current(&worker_ref) }; if !still_current { tracing::debug!( @@ -449,11 +432,8 @@ fn schedule_worker_cancel_escalation(state: Arc, run_id: RunId, worker ); state.worker_runtime.force_stop(&worker_ref).await; let mut runs = state.runs.lock().expect("runs lock poisoned"); - if let Some(run) = runs - .get_mut(&run_id) - .filter(|run| run.cancel_escalation_worker.as_ref() == Some(&worker_ref)) - { - run.cancel_escalation_worker = None; + if let Some(run) = runs.get_mut(&run_id) { + run.clear_escalation_for(&worker_ref); } }); } diff --git a/lib/apps/fabro-server/src/worker_runtime.rs b/lib/apps/fabro-server/src/worker_runtime.rs index 0c059ccf7..f2779f055 100644 --- a/lib/apps/fabro-server/src/worker_runtime.rs +++ b/lib/apps/fabro-server/src/worker_runtime.rs @@ -21,7 +21,8 @@ pub(crate) trait WorkerRuntime: Send + Sync { async fn is_alive(&self, worker_ref: &WorkerRef) -> bool; } -#[derive(Clone, Debug, PartialEq, Eq)] +#[derive(Clone, Debug, PartialEq, Eq, strum::IntoStaticStr)] +#[strum(serialize_all = "snake_case")] pub(crate) enum WorkerRef { /// A worker running as a local subprocess. `pre_exec_setpgid` ensures the /// child is the leader of its own process group with `pgid == pid`, so a @@ -30,10 +31,8 @@ pub(crate) enum WorkerRef { } impl WorkerRef { - pub(crate) const fn kind(&self) -> &'static str { - match self { - Self::Local { .. } => "local", - } + pub(crate) fn kind(&self) -> &'static str { + self.into() } }