From 61bdbc4a5bef4ebce9806aac8443c9e98ca1c766 Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Thu, 27 Aug 2026 09:47:45 -0700 Subject: [PATCH] Close the crash-window gap and exec-session leak a third review found MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A third Opus review pass confirmed round 2's fixes hold up, then found: - The pending-cleanup record `remove_project` writes is fully durable (fsync'd); the projects_store.remove() that follows it is a plain fs::write with no fsync. A crash or power loss in that window — or that store write failing outright, beyond what the previous round's in-process rollback catches — leaves a record on disk naming a project projects.json still lists as present. The very next startup retry would then delete 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. retry_pending_cleanup_logged now takes the ProjectsStore and refuses to touch — clearing instead — any record whose project id still exists. Also stopped swallowing the round-2 rollback's own failure. - Resolving the container through find_existing_container instead of project.container_id (round 2's stale-id fix) changed what drove close_sessions_for_container in remove_project and rebuild_project_ container: sessions are now leaked when Docker is unreachable (nothing resolves, so nothing closes, and the project record is gone a moment later) and in the stale-id race itself (sessions were opened against the container that actually exists, not the id find_existing_container bypasses). Both functions now close sessions for the stored id unconditionally, and again for the resolved id if it differs. - A pronoun-agreement bug in the no-retry removal toast ("remove them manually" for a single leftover) that was fixed one line above for verb agreement but not for the pronoun. Also closed the test gaps the review named: the pending-cleanup corrupt-record aside-move had no test, the Reset toast's leftover copy was inline and untested (extracted to lib/resetOutcome.ts, mirroring components/projects/home/removalReport.ts, with unit tests), and nothing asserted rebuild()'s success path maps outcome.project into the list rather than the whole outcome. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01FGjXq6fqtAFHdbhk4f3PfZ --- .../src/commands/project_commands.rs | 78 ++++++++++++++++++- app/src-tauri/src/lib.rs | 10 ++- app/src-tauri/src/storage/pending_cleanup.rs | 22 ++++++ .../components/projects/home/ProjectHome.tsx | 4 +- .../components/projects/home/removalReport.ts | 17 +++- app/src/hooks/useProjectActions.ts | 12 +-- app/src/hooks/useProjects.test.ts | 24 ++++++ app/src/lib/resetOutcome.test.ts | 39 ++++++++++ app/src/lib/resetOutcome.ts | 32 ++++++++ 9 files changed, 220 insertions(+), 18 deletions(-) create mode 100644 app/src/lib/resetOutcome.test.ts create mode 100644 app/src/lib/resetOutcome.ts 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"; +}