mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-09-11 22:53:00 +00:00
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 <noreply@anthropic.com>
This commit is contained in:
parent
84c5468722
commit
9adf24348b
5 changed files with 37 additions and 36 deletions
|
|
@ -36,8 +36,7 @@ export function RowActionsMenu({ run }: { run: RunWithStatus }) {
|
|||
const { mutate } = useSWRConfig();
|
||||
const { push } = useToast();
|
||||
const [pendingAction, setPendingAction] = useState<LifecycleAction | "delete" | null>(null);
|
||||
const [optimisticCancellationRunId, setOptimisticCancellationRunId] =
|
||||
useState<string | null>(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:
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -381,14 +381,7 @@ fn schedule_worker_cancel_escalation(state: Arc<AppState>, 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<AppState>, 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<AppState>, 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<AppState>, 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);
|
||||
}
|
||||
});
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue