diff --git a/app/src-tauri/src/browser_view/commands.rs b/app/src-tauri/src/browser_view/commands.rs index 15a0639..1b4a5af 100644 --- a/app/src-tauri/src/browser_view/commands.rs +++ b/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, +) -> 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()); + } +} diff --git a/app/src-tauri/src/commands/project_commands.rs b/app/src-tauri/src/commands/project_commands.rs index 25912c4..e82d011 100644 --- a/app/src-tauri/src/commands/project_commands.rs +++ b/app/src-tauri/src/commands/project_commands.rs @@ -1036,7 +1036,6 @@ fn pending_cleanup_is_stale(recorded_at: &str, now: chrono::DateTime, ) -> Result { // 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); + } } diff --git a/app/src-tauri/src/url_open.rs b/app/src-tauri/src/url_open.rs index a07fa94..7372386 100644 --- a/app/src-tauri/src/url_open.rs +++ b/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) -> 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)); + } +}