Report and retry Docker resources remove_project could not delete #36

Merged

jknapp merged 4 commits from fix/remove-project-cleanup-reporting into main 2026-08-27 16:59:57 +00:00

Conversation 0 Commits 4 Files Changed 16

+1181 -48

4 Commits

Author	SHA1	Message	Date
shadow-testandClaude Sonnet 5	61bdbc4a5b	Close the crash-window gap and exec-session leak a third review found ... 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FGjXq6fqtAFHdbhk4f3PfZ	2026-08-27 09:47:45 -07:00
shadow-testandClaude Sonnet 5	439ef16f07	Fix two new bugs a second review found: stale container id, orphaned record ... A second Opus review of commit 2 found it had introduced real problems of its own rather than just polish gaps: - remove_project's "None or stale" container-id fallback only handled None. A stale id (the documented start-failure race in start_project_container_locked, where the old container is removed and the new one's id isn't persisted until after start_container succeeds) still 404'd on removal — now treated as success by commit 1's own fix — while the real container survived to block every volume removal with a 409 forever, with nothing in the pending-cleanup record ever naming it. Both remove_project and rebuild_project_container now resolve the container via find_existing_container() unconditionally, matching every other container-destroying path in the codebase, and remove_project fails closed (records a leftover rather than silently skipping) if Docker itself can't be reached to check. - remove_project could leave a pending-cleanup record for a project still live in projects.json: if the store's own save failed after the record was written, startup housekeeping would delete that project's container and volumes out from under it on the next launch. The record is now rolled back when the store write fails. - rebuild_project_container (Reset) only surfaced a leftover volume, not a leftover snapshot image — the more serious failure, since the next container is built from that image whenever it exists, silently reviving the exact system layer Reset was asked to discard. ProjectResetOutcome now carries leftover_image too, and the toast's "run docker volume rm" advice is corrected: the new container has already remounted the volume by the time the toast renders, so that command would just hit the same conflict Reset did. Also from the same pass: reworded a couple of log/toast lines that still asserted resources were "still present" when the daemon-unreachable case covered by the same code path can't actually confirm that; fixed a singular/verb mismatch in the leftover toast text; moved an unparseable pending-cleanup record aside instead of re-warning about it forever; and added a debug log when a record's recorded_at can't be parsed, so aging never silently no-ops. Pulled describeLeftovers/leftoverVerb out of ProjectHome.tsx into their own module with unit tests, and added tests for the recorded_at staleness check — the previous commit's equivalent logic had none. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FGjXq6fqtAFHdbhk4f3PfZ	2026-08-27 09:09:46 -07:00
shadow-testandClaude Sonnet 5	d8bb5ab262	Address review findings: durability, stale container ids, honest toasts ... An Opus review of the previous commit found several real gaps: - pending_cleanup::save used plain write-temp-then-rename, unlike migration_store's fsync'd write it claimed to mirror — a crash in that window left a truncated record that list() would skip forever, silently reproducing the exact bug this module exists to fix. Now matches migration_store's File::create/write_all/sync_all/rename/sync_dir shape, and the tests exercise the real save/list/clear functions against a temp dir instead of re-implementing their bodies inline. - remove_project and rebuild_project_container only ever looked at project.container_id, unlike every other container-destroying path in the codebase, which falls back to find_existing_container for exactly this race (a crash between creating a container and persisting its id). A miss here left a container that then blocked every subsequent volume removal with a 409, forever. Both now resolve the same way the rest of the codebase does, and record the container by its deterministic name rather than its id so a retry still has something that resolves. - remove_project's toast promised an automatic retry unconditionally, even when writing the pending-cleanup record itself failed (the one case where nothing will actually retry). ProjectRemovalReport now carries retry_scheduled, and the UI is honest about which case it's in. - remove_volumes_by_name now retries once after a short delay on a 409, since Docker releasing a volume's mount reference right after its container is removed is not always instantaneous, and this is exactly the sequence remove_project runs. - rebuild_project_container (Reset) returns ProjectResetOutcome so the UI can warn when Reset could not fully clear a project's volumes, instead of only logging it — the new container silently reuses old data otherwise, which is what Reset promises not to do. - retry_pending_cleanup_logged escalates a record's log level after it has failed for a week, since recorded_at was otherwise write-only. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FGjXq6fqtAFHdbhk4f3PfZ	2026-08-27 08:36:27 -07:00
shadow-testandClaude Sonnet 5	4827170715	Report and retry Docker resources remove_project could not delete ... remove_project_volumes always returned Ok(()) regardless of what actually happened, making the `if let Err(e)` guarding it at every call site dead code. remove_project then dropped the project record unconditionally, so a volume, image or container that failed to delete became permanently unreachable — confirmed against a real orphaned volume pair found in the wild (fixes #31). remove_project_volumes/remove_snapshot_image/remove_container now report what they could not remove (treating "already gone" as success rather than a leftover), remove_project surfaces this to the user via a toast, and before dropping the project record it writes a pending-cleanup record that startup housekeeping retries automatically on the next launch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FGjXq6fqtAFHdbhk4f3PfZ	2026-08-27 08:18:41 -07:00