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 — a volume, image or container that failed to delete became permanently unreachable the moment remove_project dropped the record.
remove_project_volumes / remove_snapshot_image / remove_container now accurately report what they could not remove, treating "already gone" (404) 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 (storage::pending_cleanup, modeled on the existing migration_store) naming whatever survived.
Startup housekeeping (lib.rs) now retries every pending-cleanup record on next launch, alongside the existing snapshot sweep, and clears a record once it is fully clean.
rebuild_project_container (Reset) got the same accurate leftover logging, since a volume that survives Reset is silently reused by the next container instead of starting clean.
cargo check / cargo clippy — clean, no new warnings
cargo test — 485 passed
npx tsc --noEmit — clean
npm run test — 602 passed
## Summary
- `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 — a volume, image or container that failed to delete became permanently unreachable the moment `remove_project` dropped the record.
- `remove_project_volumes` / `remove_snapshot_image` / `remove_container` now accurately report what they could not remove, treating "already gone" (404) 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 (`storage::pending_cleanup`, modeled on the existing `migration_store`) naming whatever survived.
- Startup housekeeping (`lib.rs`) now retries every pending-cleanup record on next launch, alongside the existing snapshot sweep, and clears a record once it is fully clean.
- `rebuild_project_container` (Reset) got the same accurate leftover logging, since a volume that survives Reset is silently reused by the next container instead of starting clean.
Closes #31
## Test plan
- [x] `cargo check` / `cargo clippy` — clean, no new warnings
- [x] `cargo test` — 485 passed
- [x] `npx tsc --noEmit` — clean
- [x] `npm run test` — 602 passed
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
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
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
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
jknapp
merged commit 06254db3d4 into main2026-08-27 16:59:57 +00:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Summary
remove_project_volumesalways returnedOk(())regardless of what actually happened, making theif let Err(e)guarding it at every call site dead code — a volume, image or container that failed to delete became permanently unreachable the momentremove_projectdropped the record.remove_project_volumes/remove_snapshot_image/remove_containernow accurately report what they could not remove, treating "already gone" (404) as success rather than a leftover.remove_projectsurfaces this to the user via a toast, and before dropping the project record it writes a pending-cleanup record (storage::pending_cleanup, modeled on the existingmigration_store) naming whatever survived.lib.rs) now retries every pending-cleanup record on next launch, alongside the existing snapshot sweep, and clears a record once it is fully clean.rebuild_project_container(Reset) got the same accurate leftover logging, since a volume that survives Reset is silently reused by the next container instead of starting clean.Closes #31
Test plan
cargo check/cargo clippy— clean, no new warningscargo test— 485 passednpx tsc --noEmit— cleannpm run test— 602 passedA 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