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 <noreply@anthropic.com>
This commit is contained in:
shadowdaoandClaude Opus 5.5 committed 2026-11-15 10:30:56 -08:00
1 parent 8441a096c8
commit 08b6d341f2
10 files changed
+110 -10

No files matched your search

@@ -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}"));
}
+54
View File
@@ -34,6 +34,8 @@ pub trait WipRunner: Sync {
fn run(&self, args: Vec<String>) -> impl Future<Output = Result<(String, i64), String>> + Send;
/// Record `marker` in [`STOP_REASON_FILE`] (as root: the directory is root's).
fn write_stop_reason(&self, marker: &str) -> impl Future<Output = Result<(), String>> + Send;
/// Remove [`STOP_REASON_FILE`] (as root), so the hook pushes as configured again.
fn clear_stop_reason(&self) -> impl Future<Output = Result<(), String>> + 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<String> {
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<R: WipRunner>(runner: Option<&R>, choice: Option<WipChoice>) {
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("<cleared>".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, "<cleared>"]);
// 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::<Fake>(None, Some(WipChoice::Push)).await;
assert_eq!(clear_stop_reason_command(), vec!["rm", "-f", "/run/triple-c/stop-reason"]);
}
}
@@ -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]
@@ -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");
});
});
@@ -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"}
</Button>
@@ -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) {
+12
View File
@@ -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");
+4 -2
View File
@@ -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;
});
+1 -1
View File
@@ -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);
}
+6 -1
View File
@@ -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