From 08b6d341f24cae03507ffc14355673a542041fa1 Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Sun, 15 Nov 2026 10:30:56 -0800 Subject: [PATCH] fix(aws): stop guard review round 2 A failed StopTask removes the stop marker it recorded, so a later stop pushes as configured. The unchecked guard's Stop anyway sends no choice and records nothing, so the workspace's own shutdown push still runs as the dialog says. The idle watcher keeps a pushed/discard marker instead of overwriting it; the remove-resources dialog sends discard only when there may be saved files. Co-Authored-By: Claude Opus 5.5 --- .../src/commands/project_commands.rs | 6 ++- app/src-tauri/src/commands/stop_guard.rs | 54 +++++++++++++++++++ app/src-tauri/src/docker/aws_mode_tests.rs | 19 +++++++ .../projects/StopGuardModal.test.tsx | 3 ++ .../components/projects/StopGuardModal.tsx | 9 ++-- .../projects/home/config/HostSection.tsx | 2 +- app/src/hooks/useProjectActions.test.ts | 12 +++++ app/src/hooks/useProjectActions.ts | 6 ++- app/src/lib/awsWorkspace.ts | 2 +- container/triple-c-idle-watch | 7 ++- 10 files changed, 110 insertions(+), 10 deletions(-) diff --git a/app/src-tauri/src/commands/project_commands.rs b/app/src-tauri/src/commands/project_commands.rs index 971d5b0..dfcaaea 100644 --- a/app/src-tauri/src/commands/project_commands.rs +++ b/app/src-tauri/src/commands/project_commands.rs @@ -1519,16 +1519,16 @@ pub async fn stop_project_container( // The stop guard's choice, before anything else is torn down: a push that // fails aborts the stop with the project as it was (auth bridge included). let mut note = None; + let guard_target = if project.is_aws() { ContainerTarget::for_project(&project) } else { None }; if project.is_aws() { if wip == Some(WipChoice::Push) { emit_progress(&app_handle, &project_id, "Pushing unsaved work..."); } - let target = ContainerTarget::for_project(&project); note = aws_guard_before_stop( &state.projects_store, &project_id, project.status.clone(), - target.as_ref(), + guard_target.as_ref(), wip, super::stop_guard::PUSH_TIMEOUT, ) @@ -1549,6 +1549,8 @@ pub async fn stop_project_container( let settings = state.settings_store.get(); if let Err(e) = ProjectHost::for_project(&project).stop_project(&project, &settings).await { log::warn!("Stop failed for project {}: {}", project_id, e); + // Still running: a later stop must push as configured again. + super::stop_guard::forget_choice(guard_target.as_ref(), wip).await; let _ = state.projects_store.update_status(&project_id, project.status.clone()); return Err(format!("Could not stop the task: {e}")); } diff --git a/app/src-tauri/src/commands/stop_guard.rs b/app/src-tauri/src/commands/stop_guard.rs index 6d22c1e..05c003d 100644 --- a/app/src-tauri/src/commands/stop_guard.rs +++ b/app/src-tauri/src/commands/stop_guard.rs @@ -34,6 +34,8 @@ pub trait WipRunner: Sync { fn run(&self, args: Vec) -> impl Future> + Send; /// Record `marker` in [`STOP_REASON_FILE`] (as root: the directory is root's). fn write_stop_reason(&self, marker: &str) -> impl Future> + Send; + /// Remove [`STOP_REASON_FILE`] (as root), so the hook pushes as configured again. + fn clear_stop_reason(&self) -> impl Future> + Send; } impl WipRunner for ContainerTarget { @@ -53,6 +55,40 @@ impl WipRunner for ContainerTarget { Err(format!("exit {code}: {}", cap(&out))) } } + + async fn clear_stop_reason(&self) -> Result<(), String> { + let (out, code) = self + .exec_oneshot_as("root", clear_stop_reason_command(), Vec::new()) + .await?; + if code == 0 { + Ok(()) + } else { + Err(format!("exit {code}: {}", cap(&out))) + } + } +} + +/// The root command that forgets a recorded marker (a fixed argv). +pub fn clear_stop_reason_command() -> Vec { + vec!["rm".into(), "-f".into(), STOP_REASON_FILE.into()] +} + +/// After StopTask failed: the workspace keeps running, so a marker this stop +/// recorded must not suppress the push of a later stop (Spot, ECS, idle) the +/// user never chose for. Best effort; logged. +pub async fn forget_choice(runner: Option<&R>, choice: Option) { + if choice.is_none() { + return; + } + let Some(r) = runner else { + log::warn!("Stop failed and the workspace is unreachable; its stop marker may remain"); + return; + }; + match tokio::time::timeout(DRY_RUN_TIMEOUT, r.clear_stop_reason()).await { + Ok(Ok(())) => {} + Ok(Err(e)) => log::warn!("Could not remove the stop marker after a failed stop: {e}"), + Err(_) => log::warn!("Removing the stop marker after a failed stop timed out"), + } } /// The root command that records `marker`; the value travels as `$1`, never @@ -230,6 +266,10 @@ pub(crate) mod tests { self.markers.lock().unwrap().push(marker.to_string()); Ok(()) } + async fn clear_stop_reason(&self) -> Result<(), String> { + self.markers.lock().unwrap().push("".to_string()); + Ok(()) + } } const T: Duration = Duration::from_secs(5); @@ -355,4 +395,18 @@ pub(crate) mod tests { let ep = include_str!("../../../../container/entrypoint.sh"); assert!(ep.contains("pushed|discard)") && ep.contains(STOP_REASON_FILE)); } + + #[tokio::test] + async fn a_failed_stop_forgets_the_recorded_choice() { + let fake = Fake::new(Ok((String::new(), 0))); + prepare_stop(Some(&fake), Some(WipChoice::Discard), T).await.unwrap(); + forget_choice(Some(&fake), Some(WipChoice::Discard)).await; + assert_eq!(fake.markers(), vec![MARKER_DISCARD, ""]); + // An unguarded stop recorded nothing, so there is nothing to forget. + let fake = Fake::new(Ok((String::new(), 0))); + forget_choice(Some(&fake), None).await; + assert!(fake.markers().is_empty()); + forget_choice::(None, Some(WipChoice::Push)).await; + assert_eq!(clear_stop_reason_command(), vec!["rm", "-f", "/run/triple-c/stop-reason"]); + } } diff --git a/app/src-tauri/src/docker/aws_mode_tests.rs b/app/src-tauri/src/docker/aws_mode_tests.rs index de6238e..1801cc2 100644 --- a/app/src-tauri/src/docker/aws_mode_tests.rs +++ b/app/src-tauri/src/docker/aws_mode_tests.rs @@ -842,6 +842,25 @@ fn idle_watch_signals_the_entrypoint_with_reason_idle() { assert_eq!(fs::read_to_string(w.tmp.path().join("reason")).unwrap().trim(), "idle"); } +#[test] +fn idle_watch_keeps_a_stop_guard_marker_already_recorded() { + for marker in ["pushed", "discard"] { + let w = Watch::new(); + fs::write(w.tmp.path().join("reason"), format!("{marker}\n")).unwrap(); + let (url, mut agent) = fake_agent(w.tmp.path(), r#"{"last_activity_unix":1,"active_execs":0,"active_tunnels":0}"#); + let mut target = Command::new("bash") + .arg("-c") + .arg("trap 'exit 0' TERM; while :; do sleep 0.1; done") + .spawn() + .unwrap(); + let out = w.cmd(&url, target.id()).output().unwrap(); + target.wait().unwrap(); + agent.kill().ok(); + assert!(out.status.success(), "{}", text(&out)); + assert_eq!(fs::read_to_string(w.tmp.path().join("reason")).unwrap().trim(), marker); + } +} + // ------------------------------------------------------- stop (SIGTERM) path #[test] diff --git a/app/src/components/projects/StopGuardModal.test.tsx b/app/src/components/projects/StopGuardModal.test.tsx index 1a638df..48bb677 100644 --- a/app/src/components/projects/StopGuardModal.test.tsx +++ b/app/src/components/projects/StopGuardModal.test.tsx @@ -49,5 +49,8 @@ describe("StopGuardModal", () => { expect(screen.getByText(/could not check/)).toBeInTheDocument(); const stop = screen.getByRole("button", { name: "Stop anyway" }); await waitFor(() => expect(stop).toHaveFocus()); + // Nothing is recorded: the shutdown push the copy promises still runs. + fireEvent.click(stop); + expect(onChoose).toHaveBeenCalledWith("unguarded"); }); }); diff --git a/app/src/components/projects/StopGuardModal.tsx b/app/src/components/projects/StopGuardModal.tsx index bcb3826..390d32c 100644 --- a/app/src/components/projects/StopGuardModal.tsx +++ b/app/src/components/projects/StopGuardModal.tsx @@ -3,13 +3,16 @@ import type { UnsavedWork, WipChoice } from "../../lib/types"; import Modal from "../ui/Modal"; import Button from "../ui/Button"; +export type StopGuardChoice = WipChoice | "unguarded" | null; + interface Props { projectName: string; work: UnsavedWork; /** The project's "push work in progress when the workspace stops" setting. */ wipPushOnStop?: boolean; - /** `null` = Cancel. */ - onChoose: (choice: WipChoice | null) => void; + /** `null` = Cancel. `"unguarded"` = stop without recording any choice + * (the unchecked case: the workspace's own shutdown push still runs). */ + onChoose: (choice: StopGuardChoice) => void; } function describeRepo(dirty: boolean, unpushed: number): string { @@ -45,7 +48,7 @@ export default function StopGuardModal({ projectName, work, wipPushOnStop = fals ref={stopRef} size="md" variant={work.checked ? "danger" : "primary"} - onClick={() => onChoose("discard")} + onClick={() => onChoose(work.checked ? "discard" : "unguarded")} > {work.checked ? "Stop without pushing" : "Stop anyway"} diff --git a/app/src/components/projects/home/config/HostSection.tsx b/app/src/components/projects/home/config/HostSection.tsx index 6092d17..b5b8a4f 100644 --- a/app/src/components/projects/home/config/HostSection.tsx +++ b/app/src/components/projects/home/config/HostSection.tsx @@ -367,7 +367,7 @@ function RemoveAwsResourcesModal({ project, onClose }: { project: Project; onClo setRunning(true); setError(null); try { - const updated = await commands.removeAwsResources(project.id, discardData); + const updated = await commands.removeAwsResources(project.id, hasSavedFiles(project) && discardData); useAppState.getState().updateProjectInList(updated); onClose(); } catch (e) { diff --git a/app/src/hooks/useProjectActions.test.ts b/app/src/hooks/useProjectActions.test.ts index f1f5fc8..457906b 100644 --- a/app/src/hooks/useProjectActions.test.ts +++ b/app/src/hooks/useProjectActions.test.ts @@ -76,6 +76,18 @@ describe("useProjectActions stop guard", () => { expect(stop).not.toHaveBeenCalled(); }); + it("stops an unchecked workspace without sending a choice", async () => { + checkUnsavedWork.mockResolvedValue({ checked: false, repos: [], error: "dry run timed out" }); + const { result } = renderHook(() => useProjectActions(awsProject({ status: "running" }))); + await act(async () => { + await result.current.handleStop(); + }); + await act(async () => { + await result.current.resolveStopGuard("unguarded"); + }); + expect(stop).toHaveBeenCalledWith("p1", undefined); + }); + it("toasts a failed push or StopTask and leaves the project as it was", async () => { checkUnsavedWork.mockResolvedValue(dirty); stop.mockRejectedValue("Pushing unsaved work failed (exit 1), so the workspace was not stopped:\npush denied"); diff --git a/app/src/hooks/useProjectActions.ts b/app/src/hooks/useProjectActions.ts index 6e43507..0642f3c 100644 --- a/app/src/hooks/useProjectActions.ts +++ b/app/src/hooks/useProjectActions.ts @@ -105,11 +105,13 @@ export function useProjectActions(project: Project) { }, [aws, project.status, project.id, run, stop]); const resolveStopGuard = useCallback( - (choice: WipChoice | null) => { + (choice: WipChoice | "unguarded" | null) => { setStopGuard(null); if (choice === null) return undefined; return run("Stop", async () => { - const note = await stop(project.id, choice); + // "unguarded": no choice is sent, so nothing is recorded in the + // workspace and its own shutdown push runs as configured. + const note = await stop(project.id, choice === "unguarded" ? undefined : choice); if (note) pushToast({ kind: "info", message: `“${project.name}” stopped`, detail: note }); return note; }); diff --git a/app/src/lib/awsWorkspace.ts b/app/src/lib/awsWorkspace.ts index a1b3faf..7998f47 100644 --- a/app/src/lib/awsWorkspace.ts +++ b/app/src/lib/awsWorkspace.ts @@ -107,7 +107,7 @@ export function cleanRepos(rows: RepoSource[]): RepoSource[] { })); } -/** Whether Stop must ask first (mirrors Rust `needs_guard`). */ +/** Whether Stop must ask first (the one place this rule lives). */ export function stopGuardNeeded(work: UnsavedWork): boolean { return !work.checked || work.repos.some((r) => r.dirty || r.unpushed > 0); } diff --git a/container/triple-c-idle-watch b/container/triple-c-idle-watch index c50c5df..17f934d 100755 --- a/container/triple-c-idle-watch +++ b/container/triple-c-idle-watch @@ -132,7 +132,12 @@ while :; do if check; then echo "triple-c-idle-watch: idle for ${minutes} minutes — stopping the workspace" mkdir -p "$(dirname "$reason_file")" 2>/dev/null - printf 'idle\n' > "$reason_file" + # A pushed/discard marker means the app's stop guard has decided and + # its stop is under way: keep that decision rather than overwrite it. + case "$(cat "$reason_file" 2>/dev/null)" in + pushed|discard) ;; + *) printf 'idle\n' > "$reason_file" ;; + esac kill -TERM "$signal_pid" exit 0 fi