diff --git a/app/src-tauri/src/commands/project_commands.rs b/app/src-tauri/src/commands/project_commands.rs index 1ae7275..4768638 100644 --- a/app/src-tauri/src/commands/project_commands.rs +++ b/app/src-tauri/src/commands/project_commands.rs @@ -751,6 +751,20 @@ pub async fn remove_project( // `remove_volumes_by_name`'s fail-closed handling of the same // situation — the alternative silently drops the one resource most // likely to block everything else if it does exist. + // + // Exec sessions are closed for `project.container_id` unconditionally, + // before the lookup above and regardless of whether it succeeds — + // that is host-side state with no Docker dependency, so it must not + // wait on a daemon that might not answer. Resolving through + // `find_existing_container` instead of using it directly would leave + // these open in exactly the two cases this whole change exists to + // handle: Docker unreachable (no id resolved, no way to ever close + // them again once the project record is gone) and the stale-id race + // (sessions were opened against the container that actually exists, + // which is what gets resolved below, not the stored id). + if let Some(ref stored_id) = project.container_id { + state.exec_manager.close_sessions_for_container(stored_id).await; + } let container_ref = match docker::find_existing_container(project).await { Ok(found) => found, Err(e) => { @@ -763,7 +777,9 @@ pub async fn remove_project( } }; if let Some(ref container_id) = container_ref { - state.exec_manager.close_sessions_for_container(container_id).await; + if project.container_id.as_deref() != Some(container_id.as_str()) { + state.exec_manager.close_sessions_for_container(container_id).await; + } let _ = docker::stop_container(container_id).await; if let Err(e) = docker::remove_container(container_id).await { log::warn!( @@ -832,9 +848,24 @@ pub async fn remove_project( // record would tell startup housekeeping to delete its container and // volumes out from under it. Roll the record back rather than leaving // that mismatch for the retry to discover the hard way. + // + // This is a second, narrower line of defence, not the only one — a crash + // between the `save` above and the `remove` below leaves exactly the same + // mismatch with no error for either side to catch, which is why + // `retry_pending_cleanup_logged` also refuses to act on a record whose + // project is still listed in `projects.json`. Belt and suspenders: a + // caught failure here is handled immediately rather than waiting for the + // next launch to notice. if let Err(e) = state.projects_store.remove(&project_id) { if !report.is_clean() { - let _ = crate::storage::pending_cleanup::clear(&project_id); + if let Err(clear_err) = crate::storage::pending_cleanup::clear(&project_id) { + log::error!( + "Project {} was not removed ({}), and its pending-cleanup record could not \ + be rolled back either ({}) — it will name this still-live project until \ + startup housekeeping's own guard clears it", + project_id, e, clear_err + ); + } } return Err(e); } @@ -853,7 +884,21 @@ pub async fn remove_project( /// reasoning), so there is no IPC contract to keep. A record that still has /// leftovers after this is written back so the next run does not lose track /// of what changed; one that is now empty is deleted. -pub async fn retry_pending_cleanup_logged() { +/// +/// Takes the `ProjectsStore` so it can refuse to touch a project that is +/// still live: `remove_project` writes a pending-cleanup record durably +/// (fsync'd) *before* it asks the store to drop the project, and that +/// store write is a plain `fs::write` with no fsync of its own. A crash or +/// power loss in the gap between the two — or the store write failing +/// outright, on top of the round-2 fix that only rolls the record back when +/// that failure is caught in-process — can leave a record on disk pointing +/// at a project `projects.json` still lists. Without this check, the very +/// first retry after such a crash deletes that project's container, +/// snapshot image and *both volumes, including the one holding the OAuth +/// credential and every session transcript*, out from under a project the +/// user still sees in the sidebar. A record whose project still exists is +/// therefore always stale — cleared without touching Docker, not retried. +pub async fn retry_pending_cleanup_logged(projects_store: &crate::storage::projects_store::ProjectsStore) { let records = crate::storage::pending_cleanup::list(); if records.is_empty() { return; @@ -863,6 +908,23 @@ pub async fn retry_pending_cleanup_logged() { let mut still_pending = 0usize; for mut record in records { + if projects_store.get(&record.project_id).is_some() { + log::warn!( + "Pending cleanup record for project {} ({}) names a project that still exists — \ + clearing the record without touching Docker rather than risk deleting a live \ + project's resources", + record.project_id, record.project_name + ); + if let Err(e) = crate::storage::pending_cleanup::clear(&record.project_id) { + log::error!( + "Could not clear the stale pending-cleanup record for still-live project {} \ + ({}): {}", + record.project_id, record.project_name, e + ); + } + continue; + } + if let Some(container_id) = record.container_id.take() { match docker::remove_container(&container_id).await { Ok(()) => {} @@ -1472,9 +1534,17 @@ pub async fn rebuild_project_container( // handling of the same lookup failing, `?` here aborts Reset outright: // every step after this one needs Docker too, so there is no useful // partial progress to make without it. + // Closed for the stored id unconditionally, then again for the resolved + // one if it differs — see the matching comment in `remove_project` for + // why the stale-id race can leave sessions open under either identity. + if let Some(ref stored_id) = project.container_id { + state.exec_manager.close_sessions_for_container(stored_id).await; + } let container_ref = docker::find_existing_container(&project).await?; if let Some(ref container_id) = container_ref { - state.exec_manager.close_sessions_for_container(container_id).await; + if project.container_id.as_deref() != Some(container_id.as_str()) { + state.exec_manager.close_sessions_for_container(container_id).await; + } let _ = docker::stop_container(container_id).await; docker::remove_container(container_id).await?; state.projects_store.set_container_id(&project_id, None)?; diff --git a/app/src-tauri/src/lib.rs b/app/src-tauri/src/lib.rs index b9a92e6..2bbe8f1 100644 --- a/app/src-tauri/src/lib.rs +++ b/app/src-tauri/src/lib.rs @@ -250,6 +250,7 @@ pub fn run() { // an image open and the sweep will not force; pins are untagged // second so the images they were holding are dangling by the time // the sweep lists them; the sweep runs last and collects both. + let projects_store_for_cleanup = projects_store_setup.clone(); tauri::async_runtime::spawn(async move { crate::docker::reap_probe_containers().await; let reaped = crate::docker::reap_stale_migration_pins().await; @@ -259,8 +260,13 @@ pub fn run() { crate::docker::sweep_orphaned_snapshots_logged("startup").await; // A container/image/volume `remove_project` could not delete // is recorded rather than lost — see triple-c#31 — and this is - // the only place anything ever retries it. - crate::commands::project_commands::retry_pending_cleanup_logged().await; + // the only place anything ever retries it. Takes the store so + // it can refuse to touch a project that turns out to still be + // live — see the long comment on the function itself. + crate::commands::project_commands::retry_pending_cleanup_logged( + &projects_store_for_cleanup, + ) + .await; }); // Auto-start web terminal server if enabled in settings diff --git a/app/src-tauri/src/storage/pending_cleanup.rs b/app/src-tauri/src/storage/pending_cleanup.rs index c544884..85e89ee 100644 --- a/app/src-tauri/src/storage/pending_cleanup.rs +++ b/app/src-tauri/src/storage/pending_cleanup.rs @@ -295,6 +295,28 @@ mod tests { fs::remove_dir_all(&dir).ok(); } + /// A record that fails to parse is moved aside once, rather than left in + /// place to be re-warned about — and re-warned about — on every future + /// launch forever. + #[test] + fn an_unparseable_record_is_moved_aside_exactly_once() { + let dir = temp_dir("corrupt-aside"); + let bad = dir.join("bad.json"); + fs::write(&bad, "{ not json").unwrap(); + + list_in(&dir); + assert!(!bad.exists(), "the bad file should have been moved aside"); + let corrupt = dir.join("bad.json.corrupt"); + assert!(corrupt.exists(), "and the moved copy should be at .json.corrupt"); + + // A second pass must not warn about `bad.json` again — it is gone — + // and must not choke on `.json.corrupt` already being there. + assert!(list_in(&dir).is_empty()); + assert!(corrupt.exists(), "the aside copy is not itself deleted"); + + fs::remove_dir_all(&dir).ok(); + } + /// `list_in` must not pick up the `.json.tmp` staging file `save_in` /// leaves behind if a crash lands between the write and the rename — the /// whole point of the temp-then-rename dance is that only the renamed diff --git a/app/src/components/projects/home/ProjectHome.tsx b/app/src/components/projects/home/ProjectHome.tsx index 907b8a9..024446e 100644 --- a/app/src/components/projects/home/ProjectHome.tsx +++ b/app/src/components/projects/home/ProjectHome.tsx @@ -19,7 +19,7 @@ import ConfigTab from "./ConfigTab"; import FilesTab from "./FilesTab"; import BrowserTab from "./BrowserTab"; import { formatUptime } from "./format"; -import { describeLeftovers, leftoverVerb } from "./removalReport"; +import { describeLeftovers, leftoverPronoun, leftoverVerb } from "./removalReport"; const TABS = [ { id: "overview", label: "Overview" }, @@ -299,7 +299,7 @@ export default function ProjectHome({ projectId, active }: Props) { useAppState.getState().pushToast({ kind: "error", message: `“${project.name}” was removed, but Triple-C could not confirm its Docker resources were removed`, - detail: `Triple-C could not confirm ${describeLeftovers(report)} ${verb} removed, and could not record this for a retry. You may need to remove them manually (\`docker rm\` / \`docker rmi\` / \`docker volume rm\`).`, + detail: `Triple-C could not confirm ${describeLeftovers(report)} ${verb} removed, and could not record this for a retry. You may need to remove ${leftoverPronoun(report)} manually (\`docker rm\` / \`docker rmi\` / \`docker volume rm\`).`, }); } } diff --git a/app/src/components/projects/home/removalReport.ts b/app/src/components/projects/home/removalReport.ts index dc0367f..52f9aa4 100644 --- a/app/src/components/projects/home/removalReport.ts +++ b/app/src/components/projects/home/removalReport.ts @@ -18,9 +18,22 @@ export function describeLeftovers(report: ProjectRemovalReport): string { return parts.join(", "); } +/** How many distinct things `describeLeftovers` is describing — a container + * and an image each count as one, however many volumes are named. Shared by + * `leftoverVerb` and `leftoverPronoun` so the two can never disagree about + * singular vs. plural. */ +function leftoverCount(report: ProjectRemovalReport): number { + return (report.container ? 1 : 0) + (report.image ? 1 : 0) + report.volumes.length; +} + /** Verb agreement for `describeLeftovers`'s output — "its container" needs * "was", "its container, a volume" needs "were". */ export function leftoverVerb(report: ProjectRemovalReport): "was" | "were" { - const count = (report.container ? 1 : 0) + (report.image ? 1 : 0) + report.volumes.length; - return count === 1 ? "was" : "were"; + return leftoverCount(report) === 1 ? "was" : "were"; +} + +/** Pronoun agreement for referring back to `describeLeftovers`'s output — + * "remove it manually" for one thing, "remove them manually" for more. */ +export function leftoverPronoun(report: ProjectRemovalReport): "it" | "them" { + return leftoverCount(report) === 1 ? "it" : "them"; } diff --git a/app/src/hooks/useProjectActions.ts b/app/src/hooks/useProjectActions.ts index 9f81e8f..a8102d1 100644 --- a/app/src/hooks/useProjectActions.ts +++ b/app/src/hooks/useProjectActions.ts @@ -3,6 +3,7 @@ import { save } from "@tauri-apps/plugin-dialog"; import type { Project } from "../lib/types"; import * as commands from "../lib/tauri-commands"; import { formatBytes } from "../lib/formatBytes"; +import { describeResetLeftovers, resetLeftoverPronoun } from "../lib/resetOutcome"; import { useAppState } from "../store/appState"; import { useProjects } from "./useProjects"; import { useTerminal } from "./useTerminal"; @@ -58,12 +59,7 @@ export function useProjectActions(project: Project) { () => run("Reset", async () => { const outcome = await rebuild(project.id); - const { leftover_image, leftover_volumes } = outcome; - if (leftover_image || leftover_volumes.length > 0) { - const parts: string[] = []; - if (leftover_image) parts.push("its previous container image"); - if (leftover_volumes.length === 1) parts.push("a volume"); - else if (leftover_volumes.length > 1) parts.push(`${leftover_volumes.length} volumes`); + if (outcome.leftover_image || outcome.leftover_volumes.length > 0) { // Not "run `docker volume rm`" — by the time this renders, the new // container this same call just started already has the leftover // volume mounted, so that command would just hit the same 409 @@ -71,9 +67,9 @@ export function useProjectActions(project: Project) { pushToast({ kind: "error", message: `Reset for “${project.name}” did not fully clean up`, - detail: `Triple-C could not remove ${parts.join(" and ")} from before the reset, so \ + detail: `Triple-C could not remove ${describeResetLeftovers(outcome)} from before the reset, so \ the new container may still be built from, or contain, old data. Stop the project, then try \ -Reset again, or remove ${parts.length > 1 ? "them" : "it"} manually once stopped.`, +Reset again, or remove ${resetLeftoverPronoun(outcome)} manually once stopped.`, }); } return outcome; diff --git a/app/src/hooks/useProjects.test.ts b/app/src/hooks/useProjects.test.ts index 1130479..3a2cded 100644 --- a/app/src/hooks/useProjects.test.ts +++ b/app/src/hooks/useProjects.test.ts @@ -140,3 +140,27 @@ describe("useProjects puts the status back when a refused command never ran", () expect(statusOf()).toBe("stopped"); }); }); + +describe("useProjects.rebuild on success", () => { + it("puts the outcome's project, not the whole outcome, into the list", async () => { + const rebuilt = project("running"); + rebuildProjectContainer.mockResolvedValue({ + project: rebuilt, + leftover_image: null, + leftover_volumes: [], + }); + + const { result } = renderHook(() => useProjects()); + let outcome!: Awaited>; + await act(async () => { + outcome = await result.current.rebuild("p1"); + }); + + // A regression here would put the `{ project, leftover_image, + // leftover_volumes }` wrapper into the projects list instead of the + // `Project` it wraps — a shape mismatch `tsc` would not catch inside a + // callback typed to take `unknown` per Tauri's `invoke`. + expect(useAppState.getState().projects.find((p) => p.id === "p1")).toEqual(rebuilt); + expect(outcome.leftover_volumes).toEqual([]); + }); +}); diff --git a/app/src/lib/resetOutcome.test.ts b/app/src/lib/resetOutcome.test.ts new file mode 100644 index 0000000..1307f9b --- /dev/null +++ b/app/src/lib/resetOutcome.test.ts @@ -0,0 +1,39 @@ +import { describe, it, expect } from "vitest"; +import { describeResetLeftovers, resetLeftoverPronoun } from "./resetOutcome"; +import type { ProjectResetOutcome } from "./types"; + +function outcome(overrides: Partial = {}): ProjectResetOutcome { + return { + project: {} as ProjectResetOutcome["project"], + leftover_image: null, + leftover_volumes: [], + ...overrides, + }; +} + +describe("describeResetLeftovers", () => { + it("names the image first, then the volumes", () => { + expect(describeResetLeftovers(outcome({ leftover_image: "x" }))).toBe( + "its previous container image", + ); + expect(describeResetLeftovers(outcome({ leftover_volumes: ["v1"] }))).toBe("a volume"); + expect(describeResetLeftovers(outcome({ leftover_volumes: ["v1", "v2"] }))).toBe("2 volumes"); + expect( + describeResetLeftovers(outcome({ leftover_image: "x", leftover_volumes: ["v1", "v2"] })), + ).toBe("its previous container image and 2 volumes"); + }); +}); + +describe("resetLeftoverPronoun", () => { + it("is singular for exactly one leftover", () => { + expect(resetLeftoverPronoun(outcome({ leftover_image: "x" }))).toBe("it"); + expect(resetLeftoverPronoun(outcome({ leftover_volumes: ["v1"] }))).toBe("it"); + }); + + it("is plural once more than one thing survived", () => { + expect(resetLeftoverPronoun(outcome({ leftover_image: "x", leftover_volumes: ["v1"] }))).toBe( + "them", + ); + expect(resetLeftoverPronoun(outcome({ leftover_volumes: ["v1", "v2"] }))).toBe("them"); + }); +}); diff --git a/app/src/lib/resetOutcome.ts b/app/src/lib/resetOutcome.ts new file mode 100644 index 0000000..f2a2bf7 --- /dev/null +++ b/app/src/lib/resetOutcome.ts @@ -0,0 +1,32 @@ +import type { ProjectResetOutcome } from "./types"; + +/** + * Names what a `ProjectResetOutcome` says Reset could not clear, for + * `useProjectActions`'s Reset toast. + * + * The image is named first and phrased as "its previous container image" + * rather than folded in with the volumes — it is the more serious of the + * two: the new container is built from it whenever it exists, so a + * surviving image means Reset silently rebuilt the exact system layer it + * was asked to discard, while a surviving volume only means old data rides + * along. + */ +export function describeResetLeftovers(outcome: ProjectResetOutcome): string { + const parts: string[] = []; + if (outcome.leftover_image) parts.push("its previous container image"); + if (outcome.leftover_volumes.length === 1) parts.push("a volume"); + else if (outcome.leftover_volumes.length > 1) parts.push(`${outcome.leftover_volumes.length} volumes`); + return parts.join(" and "); +} + +/** How many distinct things `describeResetLeftovers` is describing — the + * image counts as one, however many volumes are named alongside it. */ +function resetLeftoverCount(outcome: ProjectResetOutcome): number { + return (outcome.leftover_image ? 1 : 0) + outcome.leftover_volumes.length; +} + +/** Pronoun agreement for referring back to `describeResetLeftovers`'s + * output — "remove it manually" for one thing, "remove them" for more. */ +export function resetLeftoverPronoun(outcome: ProjectResetOutcome): "it" | "them" { + return resetLeftoverCount(outcome) === 1 ? "it" : "them"; +}