diff --git a/desktop/src-tauri/src/lib.rs b/desktop/src-tauri/src/lib.rs index e4982045eb5..b8a6c63e9fe 100644 --- a/desktop/src-tauri/src/lib.rs +++ b/desktop/src-tauri/src/lib.rs @@ -137,10 +137,15 @@ fn hide_dashboard(app: tauri::AppHandle) { /// /// The page asks for this when it loads rather than relying only on the event stream: the first /// states finish in milliseconds and an event emitted before the listener exists is simply gone. +/// +/// It always answers with a state. Answering `None` put the one case the page cannot render — a +/// shell with no startup state — behind a value the page silently discards, which is a frozen +/// window with no diagnostic and no way to tell it from a slow start. #[tauri::command] -fn startup_snapshot(app: tauri::AppHandle) -> Option { +fn startup_snapshot(app: tauri::AppHandle) -> startup::Progress { app.try_state::() .map(|startup| startup.latest()) + .unwrap_or_else(startup::unavailable) } /// The named states the startup sequence moves through, in order. diff --git a/desktop/src-tauri/src/startup.rs b/desktop/src-tauri/src/startup.rs index bcc08ba2279..0fccc6fddfc 100644 --- a/desktop/src-tauri/src/startup.rs +++ b/desktop/src-tauri/src/startup.rs @@ -30,12 +30,12 @@ use serde::Serialize; use std::{ path::PathBuf, sync::{ - atomic::{AtomicBool, Ordering}, + atomic::{AtomicBool, AtomicU64, Ordering}, Mutex, MutexGuard, PoisonError, }, }; use tauri::{AppHandle, Emitter, Manager}; -use tokio::time::{sleep, Duration, Instant}; +use tokio::time::{sleep, sleep_until, Duration, Instant}; /// The event the bootstrap page listens on. pub const PHASE_EVENT: &str = "startup-phase"; @@ -51,6 +51,13 @@ pub const DEADLINE: Duration = Duration::from_secs(30); const POLL: Duration = Duration::from_millis(250); +/// How long the deadline guard waits past the ceiling before speaking for a run that has not. +/// +/// The run's own failure names the endpoint, the home and how the child ended; the guard's can +/// only name where it stalled. The grace lets the run lose its own race first, so the better +/// diagnostic is the one on screen. +const SETTLE_GRACE: Duration = Duration::from_secs(2); + /// Where the launch came from. #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub enum LaunchOrigin { @@ -91,6 +98,14 @@ pub fn shows_window(origin: LaunchOrigin, tray: TrayAvailability) -> bool { /// A named state of the startup sequence. #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub enum Phase { + /// Nothing has run yet. + /// + /// This is what the sequence's state says before its first report, and it is deliberately not + /// one of the [`PHASES`]: it is the absence of a run, not a step of one. Seeding the state + /// with `Registering` instead made "the sequence has not started" render exactly like "the + /// sequence is registering", so a shell that never began was indistinguishable from one that + /// had — on the one surface whose job is to tell those apart. + NotStarted, Registering, Resolving, Probing, @@ -103,6 +118,9 @@ pub enum Phase { /// Every phase, in the order they run. The bootstrap page derives its checklist from this rather /// than restating it, so a phase cannot exist in one place and be missing from the other. +/// +/// [`Phase::NotStarted`] is absent on purpose. It is the state of not having run, so a checklist +/// row for it would be a step that never completes. pub const PHASES: [Phase; 8] = [ Phase::Registering, Phase::Resolving, @@ -118,6 +136,7 @@ impl Phase { /// The stable identifier the bootstrap page keys on. pub fn id(self) -> &'static str { match self { + Self::NotStarted => "not-started", Self::Registering => "registering", Self::Resolving => "resolving", Self::Probing => "probing", @@ -131,6 +150,7 @@ impl Phase { pub fn label(self) -> &'static str { match self { + Self::NotStarted => "Waiting for the startup sequence to begin", Self::Registering => "Registering the tray and the login item", Self::Resolving => "Resolving the configuration home and port", Self::Probing => "Looking for a runtime that is already listening", @@ -145,6 +165,14 @@ impl Phase { pub fn is_terminal(self) -> bool { matches!(self, Self::Ready | Self::Failed) } + + /// The phase a published id came from, for a caller that only has the wire value. + /// + /// Derived from [`PHASES`] rather than restating the mapping, so a phase cannot be resolvable + /// here and missing from the checklist. + pub fn from_id(id: &str) -> Option { + PHASES.into_iter().find(|phase| phase.id() == id) + } } /// One phase, as the bootstrap page sees it. @@ -203,6 +231,27 @@ impl Progress { } } +/// What the page is told when the sequence's own state is not registered. +/// +/// The command used to answer `None` here, and the page dropped it: `apply` returns early on a +/// falsy progress, so the surface kept its initial markup, no event ever arrived, and nothing on +/// screen distinguished that from a run still in progress. A shell that cannot find its own +/// startup state is a defect, and a defect the user can read and copy beats a window that looks +/// like it is still working. +pub fn unavailable() -> Progress { + let reason = + "the shell's startup state is not registered, so it cannot report on its own startup"; + let mut progress = Progress::new(Phase::Failed, 0); + progress.diagnostic = Some(format!( + "OpenCodex desktop {} on {}\nstate: {}\nreason: {reason}", + env!("CARGO_PKG_VERSION"), + std::env::consts::OS, + Phase::NotStarted.id(), + )); + progress.detail = Some(reason.to_owned()); + progress +} + /// Where the sequence is pointed, once the CLI has said. #[derive(Clone)] struct Target { @@ -228,6 +277,11 @@ struct Live { pub struct Startup { live: Mutex, running: AtomicBool, + /// Which run the state belongs to. + /// + /// A run's deadline guard outlives the run it was started for, and a retry that begins before + /// the old guard fires would otherwise be failed by it. + generation: AtomicU64, /// The outcome of the one-time registration, once it has happened. registered: Mutex>, } @@ -236,10 +290,11 @@ impl Startup { pub fn new() -> Self { Self { live: Mutex::new(Live { - latest: Progress::new(Phase::Registering, 0), + latest: Progress::new(Phase::NotStarted, 0), reported: Vec::new(), }), running: AtomicBool::new(false), + generation: AtomicU64::new(0), registered: Mutex::new(None), } } @@ -270,7 +325,16 @@ impl Startup { fn restart(&self) { let mut live = self.live(); live.reported.clear(); - live.latest = Progress::new(Phase::Registering, 0); + live.latest = Progress::new(Phase::NotStarted, 0); + } + + /// Whether the run has already said how it ended. + /// + /// A terminal state is the page's only promise that the screen has stopped changing, so it is + /// also what tells a late guard there is nothing left to report. + fn settled(&self) -> bool { + let phase = self.live().latest.phase; + phase == Phase::Ready.id() || phase == Phase::Failed.id() } fn publish(&self, progress: &mut Progress, failed_in: Option) { @@ -311,22 +375,82 @@ pub fn begin(app: &AppHandle) { return; } startup.restart(); + let generation = startup.generation.fetch_add(1, Ordering::AcqRel) + 1; + let started = Instant::now(); let app = app.clone(); + + // The ceiling is a promise to the page, and something has to keep it when the run does not. + // Every `return` below that reports nothing, and every step that outlives the ceiling, used to + // leave the surface on whatever it was last told — or on its own initial markup when nothing + // had been published at all — for as long as the process lived. That screen is the one a user + // cannot tell from a hung application, which is the whole thing this surface exists to avoid. + let guard = app.clone(); tauri::async_runtime::spawn(async move { - run(&app).await; + sleep_until(started + DEADLINE + SETTLE_GRACE).await; + settle( + &guard, + started, + generation, + format!( + "the startup sequence did not finish within {} seconds", + DEADLINE.as_secs() + ), + ); + }); + + tauri::async_runtime::spawn(async move { + run(&app, started).await; + settle( + &app, + started, + generation, + "the startup sequence ended without reporting a result".to_owned(), + ); if let Some(startup) = app.try_state::() { startup.running.store(false, Ordering::Release); } }); } -async fn run(app: &AppHandle) { - let started = Instant::now(); +/// Report a terminal state for a run that did not report one itself. +/// +/// Idempotent and bound to the run it was started for: a run that already said Ready or Failed is +/// left alone, and a guard whose run has been superseded by a retry says nothing. +fn settle(app: &AppHandle, started: Instant, generation: u64, reason: String) { + let Some(startup) = app.try_state::() else { + return; + }; + if startup.generation.load(Ordering::Acquire) != generation || startup.settled() { + return; + } + let stalled_in = startup.latest().phase; + let elapsed_ms = elapsed(started); + let mut progress = Progress::new(Phase::Failed, elapsed_ms); + progress.diagnostic = Some( + [ + format!( + "OpenCodex desktop {} on {}", + env!("CARGO_PKG_VERSION"), + std::env::consts::OS + ), + format!("state: {stalled_in}"), + format!("reason: {reason}"), + format!("elapsed: {elapsed_ms}ms"), + ] + .join("\n"), + ); + progress.detail = Some(reason); + emit(app, progress, Phase::from_id(stalled_in)); +} + +async fn run(app: &AppHandle, started: Instant) { let deadline = started + DEADLINE; + // Publishing comes before any lookup that can fail. A sequence that returns before it has + // said anything leaves the page unable to tell "not started" from "still going". + report(app, started, Phase::Registering, None); let Some(watch) = app.try_state::().map(|state| state.watch.clone()) else { return; }; - report(app, started, Phase::Registering, None); let registration = register(app, deadline).await; report( app, @@ -800,10 +924,60 @@ fn elapsed(started: Instant) -> u64 { #[cfg(test)] mod tests { - use super::{shows_window, LaunchOrigin, Phase, AUTOSTART_FLAG, DEADLINE, PHASES, POLL}; + use super::{ + shows_window, unavailable, LaunchOrigin, Phase, Progress, Startup, AUTOSTART_FLAG, + DEADLINE, PHASES, POLL, + }; use crate::tray_availability::TrayAvailability; use tokio::time::Duration; + #[test] + fn not_having_started_is_not_a_step_of_the_run() { + // A checklist row for it would be a step that never completes, and resolving it out of a + // published id would name a phase the page has nowhere to draw. + assert!(!PHASES.contains(&Phase::NotStarted)); + assert_eq!(Phase::from_id(Phase::NotStarted.id()), None); + for phase in PHASES { + assert_eq!(Phase::from_id(phase.id()), Some(phase)); + } + } + + #[test] + fn a_sequence_that_has_not_run_says_so() { + // Seeding the state with Registering made "has not started" render exactly like "started, + // and registering" — on the one surface whose job is to tell those apart. + let startup = Startup::new(); + assert_eq!(startup.latest().phase, Phase::NotStarted.id()); + assert!(!startup.latest().can_retry); + assert!(!startup.settled()); + } + + #[test] + fn the_snapshot_never_answers_with_nothing() { + // The page returns early on a falsy progress, so answering None here was a window frozen + // on its own markup with no diagnostic in it and no event coming. + let progress = unavailable(); + assert_eq!(progress.phase, Phase::Failed.id()); + assert!(progress.can_retry); + assert!(progress.detail.is_some()); + assert!(progress + .diagnostic + .is_some_and(|text| text.contains("reason:"))); + } + + #[test] + fn only_a_terminal_state_settles_a_run() { + // This is what stops the deadline guard from overwriting a run that already reported, and + // what makes it speak for one that never did. + let startup = Startup::new(); + let mut running = Progress::new(Phase::Waiting, 1); + startup.publish(&mut running, None); + assert!(!startup.settled()); + let mut done = Progress::new(Phase::Ready, 2); + startup.publish(&mut done, None); + assert!(startup.settled()); + } + #[test] fn only_the_autostart_argument_marks_a_login_launch() { let user = ["/Applications/OpenCodex.app".to_owned()]; diff --git a/structure/desktop-shell.md b/structure/desktop-shell.md index 94cad45e7c5..86b32275c57 100644 --- a/structure/desktop-shell.md +++ b/structure/desktop-shell.md @@ -10,7 +10,11 @@ webview to the proxy's loopback dashboard (`/#/usage`) rather than bundling or s itself. The page renders what the shell tells it and probes nothing on its own; it asks `startup_phases` for the state list rather than restating it, takes the current state from `startup_snapshot` on load because the first states finish in milliseconds, and then follows the -`startup-phase` event. It uses no `alert`, `confirm` or `prompt`: the embedded webview implements +`startup-phase` event. `startup_snapshot` always answers with a state; it used to be able to +answer with nothing, and the page returns early on a falsy progress, so the one case it could not +render — a shell with no startup state — arrived as silence rather than as a diagnostic. A shell +that cannot find its own startup state now reports that as a failure the user can read and copy. +It uses no `alert`, `confirm` or `prompt`: the embedded webview implements none of the matching WKUIDelegate panel methods on macOS, so a platform dialog is declined without drawing anything. `withGlobalTauri` is on so that page can invoke without a bundler. Only the local app origin @@ -30,6 +34,20 @@ into that record instead of discarding it, which is what makes an immediate side distinguishable from a slow start. The page asks for the state list and the run's progress rather than reconstructing either, because the early states finish faster than a listener can attach. +The deadline is a promise that the screen stops changing, so something keeps it when the run does +not. The sequence publishes its first state before any lookup that can fail, and a guard bound to +that run reports a terminal state for it if the run returns without one or outlives the ceiling. +The guard is idempotent and generation-scoped: it will not overwrite a result the run reported, +and one left over from an earlier run will not fail the retry that replaced it. It waits a short +grace past the ceiling so the run's own failure, which names the endpoint, the home and how the +child ended, is the diagnostic on screen rather than the guard's thinner one. + +"Has not started" is a state of its own rather than the first phase. The sequence's state used to +be seeded with `registering`, so a shell that never began rendered exactly like one that had just +begun — on the surface whose whole job is to tell those apart. `not-started` is deliberately +absent from the phase list the page draws its checklist from: it is the absence of a run, so a row +for it would be a step that never completes. + The shell resolves nothing itself. Resolving runs the bundled `ocx resolve --json` and reads one `ocx-resolve/1` document: the configuration home, the effective port, and a liveness verdict with three answers rather than two. `live` means attach as a guest; `absent-proven` means every diff --git a/tests/clients/desktop-cli-contracts.test.ts b/tests/clients/desktop-cli-contracts.test.ts index d4ae062c866..9c84bd8da27 100644 --- a/tests/clients/desktop-cli-contracts.test.ts +++ b/tests/clients/desktop-cli-contracts.test.ts @@ -86,7 +86,13 @@ describe("desktop CLI contracts", () => { test("the startup sequence refuses to start on anything but a proven absence", () => { const startup = code(STARTUP); - const run = startup.slice(startup.indexOf("async fn run(app: &AppHandle)")); + // Anchor on the name, not the full signature: a parameter added to the sequence is not a + // change to the order this case is about, and `indexOf` returning -1 silently slices the + // last character instead of failing, so every index below reads -1 and the case passes + // vacuously. That is exactly what it did when `run` gained its start instant. + const at = startup.indexOf("async fn run(app: &AppHandle"); + expect(at).toBeGreaterThan(-1); + const run = startup.slice(at); const unknown = run.indexOf("let Some(answer) = resolution.resolved() else {"); const attach = run.indexOf("match resolve::live_verdict(&resolution) {"); const guard = run.indexOf("if !resolve::may_start(&resolution) {"); diff --git a/tests/clients/desktop-startup-surface.test.ts b/tests/clients/desktop-startup-surface.test.ts index 72ccc9005a7..ee7e5ac61f5 100644 --- a/tests/clients/desktop-startup-surface.test.ts +++ b/tests/clients/desktop-startup-surface.test.ts @@ -157,6 +157,51 @@ describe("desktop startup surface", () => { expect(page).toContain("progress.failedPhase"); }); + test("the snapshot answers with a state rather than with nothing", () => { + // The page returns early on a falsy progress, so an absent answer was not a neutral one: it + // was a window frozen on its own markup, with no diagnostic in it and no event coming. + expect(lib).toContain("fn startup_snapshot(app: tauri::AppHandle) -> startup::Progress"); + expect(lib).not.toContain("Option"); + expect(lib).toContain("unwrap_or_else(startup::unavailable)"); + expect(startup).toContain("pub fn unavailable() -> Progress"); + }); + + test("not having started is a state of its own, and not a checklist row", () => { + // Seeding the state with the first phase made "has not started" render exactly like "started, + // and registering". A row for it would instead be a step that never completes. + expect(startup).toContain('Self::NotStarted => "not-started"'); + const list = startup.indexOf("pub const PHASES"); + expect(list).toBeGreaterThan(-1); + expect(startup.slice(list, startup.indexOf("];", list))).not.toContain("NotStarted"); + expect(startup).not.toContain("Progress::new(Phase::Registering, 0)"); + }); + + test("the run publishes before anything it does can return", () => { + // The lookup below used to come first, so a run that returned there had said nothing at all + // and the page could not tell that from a run still going. + const at = startup.indexOf("async fn run(app: &AppHandle"); + expect(at).toBeGreaterThan(-1); + const body = startup.slice(at, startup.indexOf("async fn register(", at)); + const published = body.indexOf("report(app, started, Phase::Registering, None);"); + expect(published).toBeGreaterThan(-1); + expect(body.indexOf("try_state::()")).toBeGreaterThan(published); + }); + + test("a run that reports nothing is still a run that ends", () => { + // Every early return in the sequence, and every step that outlives the ceiling, used to leave + // the surface on its last state for as long as the process lived. + const begin = startup.slice(startup.indexOf("pub fn begin("), startup.indexOf("fn settle(")); + expect(begin).toContain("run(&app, started).await;"); + expect(begin.slice(begin.indexOf("run(&app, started).await;"))).toContain("settle("); + expect(begin).toContain("sleep_until(started + DEADLINE + SETTLE_GRACE)"); + // Idempotent, and bound to the run it was started for: it may not overwrite a real result, + // and a guard left over from an earlier run may not fail the retry that replaced it. + const settle = startup.slice(startup.indexOf("fn settle("), startup.indexOf("async fn run(")); + expect(settle).toContain("startup.settled()"); + expect(settle).toContain("generation.load(Ordering::Acquire) != generation"); + expect(settle).toContain("Progress::new(Phase::Failed, elapsed_ms)"); + }); + test("the retry, the snapshot and the phase list are reachable from the page", () => { const handler = lib.slice( lib.indexOf("generate_handler!["),