Repository navigation
fix(desktop): never reattach a previous installation's volumes on a fresh install - #821
dustywusty wants to merge 3 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2de468bf7c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…resh install A desktop install set up again after its folder was deleted refused to reuse only the old database. Every other volume the previous install left was reattached silently: Compose project volumes (agent-workspace, agent-profiles, spire-*) and the supervisor's per-Bot volumes (<ns>-workspace-<bot>, <ns>-profile-<bot>). Default Bot ids are stable across installs, so a new Bot opened the old Bot's files and Chromium profile, including its website sign-ins. On Start, a verified-fresh installation (no usable key, no database URL, no saved model) now lists every leftover it can prove it owns: Compose volumes and containers by project label, supervisor volumes and containers by all three supervisor labels in this namespace. Start is refused with one confirmed reset that removes the stopped containers, then the volumes, bound to the exact set shown. A running owned container blocks the reset and nothing is removed. Installations with their saved key keep their data as before.
2de468b to
ebc8e04
Compare
|
Agree with the goal: a new install must never pick up the previous install's browser sign-ins or Bot files. The ownership detection (labels, tied to the exact confirmed set, no The concern is the experience. Someone deletes Suggestion: a verified-fresh install starts right away under new names (a new Compose project name and supervisor namespace, e.g. with a short suffix), so nothing old is reattached and nothing is deleted. The leftovers then show up as an optional, quiet cleanup ("A previous installation left 2.1 GB of data. Remove it?"). It's safe by default, nothing blocks, and nothing is lost if they meant to restore the old folder. The detection and reset code here would power that cleanup. If that's too big for this PR, the smaller fix is to present it as a choice rather than a failure: "Start fresh" as the main action, with "Keep previous data" explained underneath, instead of a refused Start. |
The reset command checked the confirmed containers as an empty list, so any offer that named a stopped container was always refused as no longer current. The removal now receives the matched offer itself, so it can only remove the set the check accepted.
A fresh install that found a previous installation's data refused Start and asked for a separate, two-step reset. For somebody who deleted the folder to start over, that read as an error during normal startup. The screen now offers one choice instead: Start fresh deletes exactly the data shown and carries on into Start. How to keep the data (restore the previous folder) is explained under it, and the volumes and containers it removes stay listed behind a disclosure. The failure alert is no longer shown for this case. Errors from the deletion itself are still shown as failures, and nothing starts. Starting under new names, so nothing is reused and nothing must be deleted, is tracked in FOR-345.
|
Took the smaller fix here and tracked the bigger one. In this PR (3e675dc): leftover data is no longer a refused Start. The screen offers one choice. Start fresh is the main action: it deletes exactly the data shown and then carries on into Start, with no second confirmation. Underneath, it explains how to keep the data: restore the previous installation's folder (and, for the database, its original encryption key) and choose it under Installation options. The volumes and containers it removes are listed behind a "What Start fresh deletes" disclosure, and the "That did not finish" alert is no longer shown for this case. If the deletion itself fails (for example, a container from the previous install is still running), that is still shown as a failure and nothing starts. I didn't add a "Keep previous data" button, because keeping the data where it is can't be offered safely. The database can't be opened without the lost key, and reattaching the profile volumes is exactly how the previous install's browser sign-ins leak into the new one. So keeping it means restoring the folder, and the screen explains that. Follow-up: FOR-345. This is your suggestion: a verified-fresh install creates its own suffix, saves it as |
What this changes
A desktop installation set up again after its folder was deleted silently reattaches what the previous installation left in the container engine. Only the Postgres volume was guarded. Observed on a real machine (Apple silicon, Docker Desktop): after deleting
~/OpenBotand reinstalling,openbot_agent-workspace,openbot_agent-profilesand the per-Botopenbot-workspace-picked-harness/openbot-profile-picked-harnessvolumes from the earlier install were reused by the new one, without a prompt.This PR:
KEY_ENCRYPTION_KEY, noDATABASE_URL, no saved model), lists every leftover the deployment can prove it owns:<project>_<name>, local, not external, nodriver_opts, labels matching this project and source);<ns>-profile-<bot>,<ns>-workspace-<bot>) carrying all three labelsopenbot.supervisor=true,openbot.bot-id=<bot>,openbot.namespace=<ns>, with the namespace resolved the waysupervisor/src/names.tsresolves it;<ns>-computer-<bot>with the supervisor labels, or this Compose project), which would otherwise keep those volumes attached.--force. A running owned container blocks it with a sentence saying so, and nothing is removed or started.Problemgainsleftover_reset/leftover_containers(skipped when empty);database_resetis unchanged.reset_leftover_databasetakesvolumesandcontainers.Root cause
6014b39, 21 Sep) protects only the database, because only the database is tied to the encryption key. The other volumes predate it: Compose'sagent-profiles/agent-workspace(40cef15) and the supervisor's per-Bot volumes (f238ce9,supervisor/src/names.ts).general-assistant,knowledge,picked-harness), so the new install derives the same volume names and Docker hands back the old volumes.agent-profilesand the per-Bot profile volumes are Chromium user-data directories ("so logins survive container restarts"), so a reused profile carries the previous installation's website sign-ins.openbot_postgres-datawas created at the new install's first Start (the database guard did its job), whileopenbot_agent-workspace/openbot_agent-profilesand bothpicked-harnessvolumes carried creation times from the earlier install; the new database's Bots aregeneral-assistant,knowledge,picked-harness.Where it runs
Boundary and audit
Changelog
CHANGELOG.mdunderUnreleased("A reinstall no longer picks up the previous installation's Bot files and sign-ins").Proof
leftover_volumes_are_only_volumes_this_project_or_namespace_provably_owns,leftover_volume_reset_removes_exactly_the_confirmed_owned_set,leftover_reset_removes_stopped_owned_containers_before_their_volumes,a_running_owned_container_blocks_the_reset_and_removes_nothing,a_fresh_root_refuses_to_reattach_any_leftover_volume,leftover_reset_is_bound_to_the_exact_offered_set,a_refused_start_offers_exactly_the_containers_and_volumes_it_named; plus an App test for the multi-volume confirmation. Fake engine only; no real volumes were touched.cargo fmt --check,cargo clippy --locked --all-targets -- -D warnings,cargo test --locked --lib --bins(542 lib, 152 bin),bun test desktop/src(150), desktop typecheck and build, Biome on changed files: pass on macOS arm64. Windows and Linux left to CI.