fix: stop a stale payload re-enabling a bridge the user turned off

Review of this branch found that `update_project` restored
`browser_view_enabled` from the store but took `auth_bridge_enabled` from
the IPC payload, on a comment claiming the Config tab edits it through that
save. The comment was wrong. `AuthBridgeRow` is the only writer, it calls
`set_auth_bridge_enabled` out of band precisely so the switch works while a
login is hanging, and it never writes the value back into frontend state --
so a payload's copy of that flag is always a stale snapshot.

The consequence was not cosmetic: turn the bridge off, then close a renamed
terminal tab, and `useTerminal` round-trips the stale `true` and the
reconcile block restarts a bridge whose own UI warns that a bridged port is
unauthenticated and reachable by any local process. Defaulting the flag to
true earlier in this branch made it worse, since the stale value is now
true for every pre-existing project.

Both flags are now restored from the store by `restore_store_owned_fields`,
and the reconcile block is gone rather than corrected: with the value
always restored it could only re-assert what was already true, and every
writer already owns its own side effect -- the setter starts and stops
synchronously, container start arms the bridge, launch reconcile re-arms
it, and the poller re-reads the flag each tick and self-terminates.
Re-adding a start path to the one function that no longer owns the flag is
what caused this.

Turning the browser view off also stopped tearing the session down when the
project record had vanished, because the persist used `?` and returned
early -- the supervisor's own `store.get()` check exists because records do
vanish mid-session. Teardown is now unconditional and the write error still
surfaces afterwards, since the stored flag saying "enabled" means the view
returns on next launch and that is worth reporting.

Finally, the opener no longer falls through to `gio` on any non-zero exit.
xdg-open's 1, 2 and 3 assert no handler ran; 4 also covers a handler that
was launched and then failed, which would have opened the link twice --
two authorize requests for one click in an OAuth flow. Reasoned from
documented exit codes rather than an observed double-open, and the cost is
stated: a genuine code-4 failure no longer reaches gio.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

app/src-tauri/src/browser_view/commands.rs

@@ -32,11 +32,28 @@ pub async fn set_browser_view_enabled(
         // Persist first, then tear down: the supervisor's own teardown emit
         // reads this flag back out of the store, and reading it mid-stop would
         // announce a view that is going away as still enabled.
-        state
+        //
+        // But the write's outcome is a *value*, not a branch. A `?` here meant
+        // that a store with no such project record returned early and
+        // `manager().stop()` never ran, leaving the supervisor, the proxy and
+        // the host port up for a project that, as far as the user is concerned,
+        // just had its view switched off. That state is not hypothetical while
+        // a session is live — the supervisor's own `store.get()` check in
+        // [`crate::browser_view`] exists because a record can go away
+        // underneath it — and before the flag was persisted at all, turning the
+        // view off always tore the session down.
+        let persisted = state
             .projects_store
-            .set_browser_view_enabled(&project_id, false)?;
+            .set_browser_view_enabled(&project_id, false);
         // Awaits the supervisor, so the host port is released before we return.
-        manager().stop(&project_id).await;
+        //
+        // A failed write is still reported rather than logged and swallowed.
+        // The resources are gone either way by this point, so surfacing it
+        // costs nothing that matters, and the failure it describes is one the
+        // user needs: the stored flag still says *enabled*, so the view comes
+        // back by itself on the next launch. Returning `Ok` would be a claim
+        // about persistence that isn't true.
+        tear_down_then_report(persisted, manager().stop(&project_id)).await?;
         return Ok(manager().status(&project_id, false).await);
     }
 
@@ -52,6 +69,20 @@ pub async fn set_browser_view_enabled(
         .await
 }
 
+/// Await `teardown`, then report `persisted`.
+///
+/// Trivial on purpose, and split out for one reason: it is the whole rule the
+/// disable path of [`set_browser_view_enabled`] has to obey — the teardown is
+/// unconditional, and a failed persist surfaces only after it has run — and as
+/// a free function that rule can be tested without a live `AppState`.
+async fn tear_down_then_report(
+    persisted: Result<(), String>,
+    teardown: impl std::future::Future<Output = ()>,
+) -> Result<(), String> {
+    teardown.await;
+    persisted
+}
+
 /// Current status. Cheap: the session map in this process plus the stored flag,
 /// never the container.
 ///
@@ -383,3 +414,43 @@ async fn running_container(
     }
     Ok(container_id)
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use std::sync::atomic::{AtomicBool, Ordering};
+
+    /// The regression: turning the view off must not leave the supervisor, the
+    /// proxy and the host port running just because the project record could
+    /// not be written — which is exactly what a missing record did.
+    #[tokio::test]
+    async fn a_failed_persist_does_not_skip_the_teardown() {
+        let torn_down = AtomicBool::new(false);
+        let result = tear_down_then_report(Err("Project x not found".to_string()), async {
+            torn_down.store(true, Ordering::SeqCst);
+        })
+        .await;
+
+        assert!(
+            torn_down.load(Ordering::SeqCst),
+            "the session must be torn down even when the store write failed"
+        );
+        assert_eq!(
+            result.err().as_deref(),
+            Some("Project x not found"),
+            "and the write failure must still reach the caller, not be swallowed"
+        );
+    }
+
+    #[tokio::test]
+    async fn a_successful_persist_reports_success_after_the_teardown() {
+        let torn_down = AtomicBool::new(false);
+        let result = tear_down_then_report(Ok(()), async {
+            torn_down.store(true, Ordering::SeqCst);
+        })
+        .await;
+
+        assert!(torn_down.load(Ordering::SeqCst));
+        assert!(result.is_ok());
+    }
+}

app/src-tauri/src/commands/project_commands.rs

@@ -1036,7 +1036,6 @@ fn pending_cleanup_is_stale(recorded_at: &str, now: chrono::DateTime<chrono::Utc
 #[tauri::command]
 pub async fn update_project(
     project: serde_json::Value,
-    app_handle: tauri::AppHandle,
     state: State<'_, AppState>,
 ) -> Result<Project, String> {
     // Taken as raw JSON, then deserialised, for one reason: a secret field that
@@ -1098,46 +1097,57 @@ pub async fn update_project(
     // [`crate::models::validate_env_vars_update`].
     crate::models::validate_env_vars_update(&stored.custom_env_vars, &project.custom_env_vars)?;
 
-    project.container_id = stored.container_id;
-    project.status = stored.status;
-    // `browser_view_enabled` is owned by `set_browser_view_enabled` and is
-    // restored here rather than taken from the payload, exactly like
-    // `container_id` and `status` above. The Config tab has no control for it
-    // — the Browser tab's toggle is the only way it ever changes — so the
-    // project object the frontend round-trips carries whatever it was told at
-    // load time and would silently undo a toggle made since. `auth_bridge_enabled`
-    // is different and does arrive through this save: the Config tab edits it,
-    // which is why the reconcile below follows whatever was just persisted.
-    project.browser_view_enabled = stored.browser_view_enabled;
-    project.created_at = stored.created_at;
+    restore_store_owned_fields(&mut project, &stored);
     project.updated_at = chrono::Utc::now().to_rfc3339();
 
     store_secrets_for_project(&project, &explicitly_cleared)?;
-    let updated = state.projects_store.update(project)?;
 
-    // `auth_bridge_enabled` can arrive through this generic save as well as
-    // through `set_auth_bridge_enabled`, so reconcile the running bridge with
-    // whatever was just persisted. `start` is idempotent and `stop` is a no-op
-    // when nothing is running, so this is safe on every project save.
-    if updated.auth_bridge_enabled {
-        if let Some(ref container_id) = updated.container_id {
-            if docker::is_container_running(container_id).await.unwrap_or(false) {
-                state
-                    .auth_bridge
-                    .start(
-                        updated.id.clone(),
-                        container_id.clone(),
-                        app_handle,
-                        state.projects_store.clone(),
-                    )
-                    .await;
-            }
-        }
-    } else {
-        state.auth_bridge.stop(&updated.id).await;
-    }
+    // Nothing reconciles the *running* auth bridge here any more, and there is
+    // nothing left for such a step to do. This command can no longer change
+    // `auth_bridge_enabled` at all (see [`restore_store_owned_fields`]), so a
+    // reconcile could only ever re-assert what was already true. The paths that
+    // do change it each own their own side effect: `set_auth_bridge_enabled`
+    // starts or stops the bridge itself, [`start_project_container`] arms it
+    // when the container comes up, and `reconcile_project_statuses` re-arms it
+    // for every already-running container at launch. The version of this that
+    // re-asserted on every save is what turned a stale flag in a payload into a
+    // restarted bridge.
+    state.projects_store.update(project)
+}
 
-    Ok(updated)
+/// Restore onto `project` the fields whose value belongs to the store rather
+/// than to whoever is saving the project. See the comment above `stored` in
+/// [`update_project`] for `container_id`, `status` and `created_at`.
+///
+/// **Both feature flags are in here, for one reason that covers them equally:
+/// neither ever arrives through this command as an edit.** Each has a
+/// dedicated setter — [`crate::browser_view::commands::set_browser_view_enabled`]
+/// and [`crate::commands::auth_bridge_commands::set_auth_bridge_enabled`] —
+/// and that setter is the only control the UI offers for it. Neither is wired
+/// into the Config tab's `save`: the browser view's toggle lives in the Browser
+/// tab, and `AuthBridgeRow`'s switch calls `set_auth_bridge_enabled` directly
+/// even though it is rendered *in* the Config tab, because that tab's editors
+/// are disabled while the container runs and the bridge is precisely the thing
+/// a user needs to flip while a login is hanging.
+///
+/// So the flags in an incoming payload are never a choice — they are whatever
+/// the frontend was told when it loaded the project, and the setters do not
+/// write their new value back into frontend app state. Every unrelated save
+/// (a renamed session, an env var, a mount name) carries that snapshot back.
+/// Taking it would silently undo a toggle made since.
+///
+/// This restored only `browser_view_enabled` before, on the stated belief that
+/// the Config tab edited `auth_bridge_enabled` through this save. It does not.
+/// The consequence was specific: a user turns the bridge off — having been told
+/// a bridged port is unauthenticated and reachable by any local process — then
+/// closes a renamed terminal tab, and the stale `true` in that save re-persisted
+/// and restarted the bridge.
+fn restore_store_owned_fields(project: &mut Project, stored: &Project) {
+    project.container_id = stored.container_id.clone();
+    project.status = stored.status.clone();
+    project.browser_view_enabled = stored.browser_view_enabled;
+    project.auth_bridge_enabled = stored.auth_bridge_enabled;
+    project.created_at = stored.created_at.clone();
 }
 
 #[tauri::command]
@@ -2195,4 +2205,89 @@ mod tests {
         // Changing it to a different root is a change, and refused.
         assert!(validate_mounted_host_path("x", Some("/"), Some("C:\\")).is_err());
     }
+    // ── Fields a generic save does not get to write ───────────────────────
+
+    /// A project as the store holds it, plus the copy the frontend is about to
+    /// save back: same record, one unrelated edit, and the flags as they were
+    /// when the frontend last loaded it.
+    fn stored_and_stale_payload() -> (Project, Project) {
+        let mut stored = Project::new("demo".to_string(), Vec::new());
+        stored.container_id = Some("abc123".to_string());
+        stored.status = ProjectStatus::Running;
+
+        let mut payload = stored.clone();
+        payload.container_id = None;
+        payload.status = ProjectStatus::Stopped;
+        payload
+            .renamed_session_names
+            .insert("s1".to_string(), "build".to_string());
+
+        (stored, payload)
+    }
+
+    /// The regression. The user turns the auth bridge off — the switch calls
+    /// `set_auth_bridge_enabled`, which persists `false` and stops the bridge,
+    /// and writes nothing back into the frontend's copy of the project. Every
+    /// holder of that copy still has `auth_bridge_enabled: true`, and the next
+    /// unrelated save (closing a renamed terminal tab) posts it back. That save
+    /// must not re-enable the bridge.
+    #[test]
+    fn a_stale_auth_bridge_flag_in_a_save_cannot_re_enable_a_disabled_bridge() {
+        let (mut stored, mut payload) = stored_and_stale_payload();
+        stored.auth_bridge_enabled = false;
+        payload.auth_bridge_enabled = true;
+
+        restore_store_owned_fields(&mut payload, &stored);
+
+        assert!(
+            !payload.auth_bridge_enabled,
+            "a save must not be able to turn the bridge back on: the stored value is the user's"
+        );
+        // The edit the save was actually for still goes through.
+        assert_eq!(
+            payload.renamed_session_names.get("s1").map(String::as_str),
+            Some("build")
+        );
+    }
+
+    /// The mirror image, and the reason the serde default going to `true`
+    /// made this worse: a pre-existing record with no `auth_bridge_enabled`
+    /// key reads as enabled, so the stale payload is `true` for every project
+    /// that predates the field. A user who has *not* turned the bridge off is
+    /// equally entitled to have the store's answer win.
+    #[test]
+    fn an_enabled_bridge_is_left_enabled_by_the_same_rule() {
+        let (mut stored, mut payload) = stored_and_stale_payload();
+        stored.auth_bridge_enabled = true;
+        payload.auth_bridge_enabled = false;
+
+        restore_store_owned_fields(&mut payload, &stored);
+
+        assert!(payload.auth_bridge_enabled);
+    }
+
+    /// The flag that was already restored, kept under test beside the one that
+    /// was not — the two are owned by their setters for the same reason and
+    /// must not drift apart again.
+    #[test]
+    fn a_stale_browser_view_flag_cannot_undo_the_panes_toggle_either() {
+        let (mut stored, mut payload) = stored_and_stale_payload();
+        stored.browser_view_enabled = true;
+        payload.browser_view_enabled = false;
+
+        restore_store_owned_fields(&mut payload, &stored);
+
+        assert!(payload.browser_view_enabled);
+    }
+
+    #[test]
+    fn the_container_handle_status_and_creation_time_still_come_from_the_store() {
+        let (stored, mut payload) = stored_and_stale_payload();
+
+        restore_store_owned_fields(&mut payload, &stored);
+
+        assert_eq!(payload.container_id.as_deref(), Some("abc123"));
+        assert_eq!(payload.status, ProjectStatus::Running);
+        assert_eq!(payload.created_at, stored.created_at);
+    }
 }

app/src-tauri/src/url_open.rs

@@ -346,11 +346,47 @@ const OPENERS: &[(&str, &[&str])] = &[("xdg-open", &[]), ("gio", &["open"])];
 /// `xdg-open` usually returns immediately (it hands the URL to a running
 /// browser and exits), but in its generic fallback mode it *is* the browser's
 /// parent and stays alive for the session. So "still running" cannot be read
-/// as failure, and "exited non-zero quickly" is the only reliable signal
-/// there is.
+/// as failure, and "exited non-zero quickly" is the only negative signal there
+/// is — though not, on its own, a trustworthy one. See
+/// [`exit_code_means_nothing_was_launched`].
 #[cfg(target_os = "linux")]
 const OPENER_GRACE: std::time::Duration = std::time::Duration::from_millis(400);
 
+/// Whether a non-zero exit says the opener certainly launched nothing, and so
+/// that the next candidate can be tried without risking a second tab.
+///
+/// The loop used to treat every quick non-zero exit as "it did nothing" and
+/// fall through. That is safe for most of `xdg-open`'s documented codes — 1
+/// (syntax), 2 (file not found) and 3 (a required tool could not be found) are
+/// all statements that it never got as far as launching a handler, and 3 is the
+/// missing-association case `gio open` is in [`OPENERS`] for. 127 is the same
+/// statement made by a shell, which is how a `$BROWSER` or `x-www-browser`
+/// wrapper naming a program that does not exist comes back.
+///
+/// Code 4 is the one that cannot be read that way, and it is the catch-all:
+/// "the action failed" also covers a handler that *was* launched and then
+/// returned non-zero. A browser that takes the URL, opens the tab in an already
+/// running instance and exits non-zero for its own reasons ends up here, as
+/// does a wrapper script that does its job and then returns the exit status of
+/// something else. Falling through on that hands the same URL to a second
+/// opener: two tabs for one click, and for an OAuth link two authorize
+/// requests.
+///
+/// So anything not recognised below — 4, an unfamiliar code, or a death by
+/// signal (`code()` is `None`) — ends the loop rather than continuing it. The
+/// caller is told the opener failed, which is the honest report of an
+/// ambiguous outcome, and no second request is made on the user's behalf. Note
+/// what this costs: an opener that genuinely failed with code 4 no longer falls
+/// through to `gio`, so a user whose `xdg-open` fails that way sees an error
+/// where they previously might have got a tab.
+///
+/// This is reasoning from `xdg-open`'s documented exit codes, not from an
+/// observed double-open in this app.
+#[cfg(target_os = "linux")]
+fn exit_code_means_nothing_was_launched(code: Option<i32>) -> bool {
+    matches!(code, Some(1 | 2 | 3 | 127))
+}
+
 /// Spawn `url` with an opener, under a sanitized environment.
 #[cfg(target_os = "linux")]
 fn spawn_with_clean_env(url: &str) -> Result<(), String> {
@@ -383,6 +419,10 @@ fn spawn_with_clean_env(url: &str) -> Result<(), String> {
             .stdout(std::process::Stdio::null())
             .stderr(std::process::Stdio::null());
 
+        // A spawn failure — `ErrorKind::NotFound` for an opener that is not
+        // installed, `PermissionDenied` for one that cannot be executed — is
+        // the unambiguous case: nothing ran, so nothing was opened, and the
+        // next candidate is free to try.
         let mut child = match command.spawn() {
             Ok(child) => child,
             Err(err) => {
@@ -395,6 +435,14 @@ fn spawn_with_clean_env(url: &str) -> Result<(), String> {
         match child.try_wait() {
             Ok(Some(status)) if !status.success() => {
                 failures.push(format!("{program} exited with {status}"));
+                // A program that *ran* is not a program that did nothing.
+                if !exit_code_means_nothing_was_launched(status.code()) {
+                    return Err(format!(
+                        "Could not confirm the link opened. Tried: {}. It may have opened anyway \
+                         — check your browser before trying again.",
+                        failures.join("; ")
+                    ));
+                }
                 continue;
             }
             Ok(_) => {}
@@ -699,3 +747,45 @@ mod tests {
         assert_eq!(changes, vec![("GTK_PATH".to_string(), None)]);
     }
 }
+
+#[cfg(all(test, target_os = "linux"))]
+mod opener_fallback_tests {
+    use super::*;
+
+    /// The codes `xdg-open` documents as "nothing was launched". Falling
+    /// through to the next opener on these is what keeps `gio open` reachable
+    /// for the case it was added for: no usable `x-scheme-handler/https`
+    /// association.
+    #[test]
+    fn the_codes_that_mean_no_handler_ran_fall_through() {
+        for code in [1, 2, 3, 127] {
+            assert!(
+                exit_code_means_nothing_was_launched(Some(code)),
+                "exit {code} means the opener never launched anything"
+            );
+        }
+    }
+
+    /// The regression this guards: `xdg-open` returns 4 both when it could not
+    /// act and when the handler it launched returned non-zero — including a
+    /// browser that had already opened the tab. Trying `gio open` next would
+    /// open it a second time, which for an OAuth URL is a second authorize
+    /// request.
+    #[test]
+    fn an_exit_that_may_follow_a_successful_open_does_not_fall_through() {
+        assert!(!exit_code_means_nothing_was_launched(Some(4)));
+        for code in [5, 7, 126, 255] {
+            assert!(
+                !exit_code_means_nothing_was_launched(Some(code)),
+                "exit {code} is not a documented 'did nothing', so it must not be assumed to be one"
+            );
+        }
+    }
+
+    /// Killed by a signal: `code()` is `None` and the outcome is unknowable,
+    /// so it is treated like any other unrecognised exit.
+    #[test]
+    fn a_death_by_signal_does_not_fall_through() {
+        assert!(!exit_code_means_nothing_was_launched(None));
+    }
+}