diff --git a/apps/fabro-web/app/install-app.tsx b/apps/fabro-web/app/install-app.tsx index 3d660ea05..27836b4cb 100644 --- a/apps/fabro-web/app/install-app.tsx +++ b/apps/fabro-web/app/install-app.tsx @@ -92,7 +92,7 @@ export default function InstallApp() { return; } } - setSaveError(null); + setSaveError((current) => (current === null ? current : null)); }, [location.pathname]); useEffect(() => { @@ -162,9 +162,13 @@ export default function InstallApp() { setTimedOut(true); }, 30_000); - const interval = window.setInterval(async () => { + const controller = new AbortController(); + let inFlight = false; + const poll = async () => { + if (inFlight || controller.signal.aborted) return; + inFlight = true; try { - const response = await fetch("/health"); + const response = await fetch("/health", { signal: controller.signal }); const body = response.ok ? ((await response.json()) as { mode?: string }) : undefined; @@ -178,13 +182,18 @@ export default function InstallApp() { window.location.href = finishState.restart_url; } } catch { + if (controller.signal.aborted) return; if (shouldRedirectAfterHealthPoll({ kind: "error" })) { window.location.href = finishState.restart_url; } + } finally { + inFlight = false; } - }, 1_000); + }; + const interval = window.setInterval(poll, 2_000); return () => { + controller.abort(); window.clearTimeout(deadline); window.clearInterval(interval); }; @@ -277,9 +286,9 @@ export default function InstallApp() { setSubmitting(true); setSaveError(null); try { - for (const provider of providers) { - await testInstallLlm(installToken, provider); - } + await Promise.all( + providers.map((provider) => testInstallLlm(installToken, provider)), + ); await putInstallLlm(installToken, providers); const nextSession = await getInstallSession(installToken); setSession(nextSession); @@ -829,7 +838,7 @@ function ProviderFields({ onChange: (nextValue: ProviderSelection) => void; }) { return ( -
+
{INSTALL_PROVIDERS.map((provider) => { const current = value[provider.id] ?? { apiKey: "" }; return ( diff --git a/lib/crates/fabro-cli/tests/it/cmd/server_start.rs b/lib/crates/fabro-cli/tests/it/cmd/server_start.rs index 2b6b677f7..40e0196ca 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/server_start.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/server_start.rs @@ -112,7 +112,7 @@ fn start_already_running_exits_with_error() { clippy::disallowed_methods, reason = "This integration test needs the real foreground process to verify install-mode startup behavior." )] -fn start_without_settings_enters_install_mode_in_foreground() { +fn start_without_default_settings_enters_install_mode_in_foreground() { let home_dir = tempfile::tempdir_in("/tmp").unwrap(); let storage_root = isolated_storage_dir(); let storage_dir = storage_root.path().join("storage"); @@ -197,6 +197,101 @@ fn start_without_settings_ignores_no_web_during_install() { ); } +#[test] +fn start_with_missing_explicit_flag_config_errors_without_entering_install_mode() { + let context = test_context!(); + let missing_config = tempfile::tempdir_in("/tmp") + .unwrap() + .path() + .join("missing-settings.toml"); + + let mut filters = context.filters(); + filters.push(( + regex::escape(missing_config.to_str().unwrap()), + "[MISSING_CONFIG]".to_string(), + )); + + let mut explicit_cmd = context.command(); + explicit_cmd.args([ + "server", + "start", + "--config", + missing_config.to_str().unwrap(), + ]); + fabro_snapshot!(filters.clone(), explicit_cmd, @" + success: false + exit_code: 1 + ----- stdout ----- + ----- stderr ----- + error: reading config file [MISSING_CONFIG]: No such file or directory (os error 2) + > No such file or directory (os error 2) + "); +} + +#[test] +fn start_with_missing_env_config_errors_without_entering_install_mode() { + let context = test_context!(); + let missing_config = tempfile::tempdir_in("/tmp") + .unwrap() + .path() + .join("missing-settings.toml"); + + let mut filters = context.filters(); + filters.push(( + regex::escape(missing_config.to_str().unwrap()), + "[MISSING_CONFIG]".to_string(), + )); + + let mut env_cmd = context.command(); + env_cmd + .env("FABRO_CONFIG", &missing_config) + .args(["server", "start"]); + fabro_snapshot!(filters, env_cmd, @" + success: false + exit_code: 1 + ----- stdout ----- + ----- stderr ----- + error: reading config file [MISSING_CONFIG]: No such file or directory (os error 2) + > No such file or directory (os error 2) + "); +} + +#[test] +fn start_with_malformed_default_settings_errors_without_entering_install_mode() { + let context = test_context!(); + let settings_dir = context.home_dir.join(".fabro"); + std::fs::create_dir_all(&settings_dir).unwrap(); + let settings_path = settings_dir.join("settings.toml"); + std::fs::write(&settings_path, "[server.listen\n").unwrap(); + + let mut filters = context.filters(); + filters.push(( + regex::escape(settings_path.to_str().unwrap()), + "[SETTINGS_PATH]".to_string(), + )); + + let mut cmd = context.command(); + cmd.args(["server", "start"]); + fabro_snapshot!(filters, cmd, @" + success: false + exit_code: 1 + ----- stdout ----- + ----- stderr ----- + error: Failed to parse settings file at [HOME_DIR]/.fabro/settings.toml: settings file is not valid TOML: TOML parse error at line 1, column 15 + | + 1 | [server.listen + | ^ + invalid table header + expected `.`, `]` + > settings file is not valid TOML: TOML parse error at line 1, column 15 + > | + > 1 | [server.listen + > | ^ + > invalid table header + > expected `.`, `]` + "); +} + #[test] fn start_without_bind_uses_home_socket_instead_of_storage_socket() { let context = test_context!(); diff --git a/lib/crates/fabro-server/src/install.rs b/lib/crates/fabro-server/src/install.rs index 56fa15f59..5a5e6259c 100644 --- a/lib/crates/fabro-server/src/install.rs +++ b/lib/crates/fabro-server/src/install.rs @@ -40,6 +40,7 @@ pub struct InstallAppState { pending_install: Arc>, storage_dir: Arc, config_path: Arc, + home: Option, install_listen: Arc>, first_operator: Arc>>, finish_in_progress: Arc, @@ -73,6 +74,7 @@ impl InstallAppState { pending_install: Arc::new(Mutex::new(PendingInstall::default())), storage_dir: Arc::new(storage_dir.to_path_buf()), config_path: Arc::new(config_path.to_path_buf()), + home: None, install_listen: Arc::new(Mutex::new(InstallListenConfig::Tcp( DEFAULT_INSTALL_TCP_LISTEN_ADDRESS.to_string(), ))), @@ -96,6 +98,7 @@ impl InstallAppState { pending_install: Arc::new(Mutex::new(PendingInstall::default())), storage_dir: Arc::new(storage_dir.to_path_buf()), config_path: Arc::new(config_path.to_path_buf()), + home: None, install_listen: Arc::new(Mutex::new(InstallListenConfig::Tcp( DEFAULT_INSTALL_TCP_LISTEN_ADDRESS.to_string(), ))), @@ -106,13 +109,20 @@ impl InstallAppState { } } - fn with_finish_callback(self, on_finish: Arc) -> Self { + #[must_use] + pub fn with_finish_callback(self, on_finish: Arc) -> Self { Self { on_finish: Some(on_finish), ..self } } + #[must_use] + pub fn with_home(mut self, home: Home) -> Self { + self.home = Some(home); + self + } + #[must_use] pub fn with_provider_base_url( mut self, @@ -353,10 +363,10 @@ impl Drop for InstallFinishGuard { pub async fn serve_install_command( bind_request: BindRequest, state: InstallAppState, - mut on_ready: F, + on_ready: F, ) -> anyhow::Result<()> where - F: FnMut(&Bind) -> anyhow::Result<()>, + F: FnOnce(&Bind) -> anyhow::Result<()>, { let (shutdown_tx, shutdown_rx) = watch::channel(false); let finish_callback: Arc = Arc::new(move || { @@ -838,7 +848,7 @@ async fn post_install_finish( return install_error_response(StatusCode::INTERNAL_SERVER_ERROR, err.to_string()); } }; - let home = Home::from_env(); + let home = state.home.clone().unwrap_or_else(Home::from_env); let dev_token = match dev_token::load_or_create_dev_token(&home.dev_token_path()) { Ok(value) => value, Err(err) => { @@ -1150,22 +1160,10 @@ async fn validate_llm_provider( state: &InstallAppState, input: &InstallLlmTestInput, ) -> Result<(), String> { - let base_url = provider_base_url(state, input.provider); - let client = install_http_client_for_url(&base_url)?; - let request = match input.provider { - Provider::Anthropic => client - .get(format!("{base_url}/models")) - .header("x-api-key", &input.api_key) - .header("anthropic-version", "2023-06-01") - .header("User-Agent", "fabro-server"), - Provider::OpenAi => client - .get(format!("{base_url}/models")) - .header("Authorization", format!("Bearer {}", input.api_key)) - .header("User-Agent", "fabro-server"), - Provider::Gemini => client - .get(format!("{base_url}/models")) - .header("x-goog-api-key", &input.api_key) - .header("User-Agent", "fabro-server"), + let (auth_header, auth_value) = match input.provider { + Provider::Anthropic => ("x-api-key", input.api_key.clone()), + Provider::OpenAi => ("Authorization", format!("Bearer {}", input.api_key)), + Provider::Gemini => ("x-goog-api-key", input.api_key.clone()), Provider::Kimi | Provider::Zai | Provider::Minimax @@ -1178,6 +1176,16 @@ async fn validate_llm_provider( } }; + let base_url = provider_base_url(state, input.provider); + let client = install_http_client_for_url(&base_url)?; + let mut request = client + .get(format!("{base_url}/models")) + .header(auth_header, auth_value) + .header("User-Agent", "fabro-server"); + if matches!(input.provider, Provider::Anthropic) { + request = request.header("anthropic-version", "2023-06-01"); + } + let response = request .timeout(Duration::from_secs(10)) .send() diff --git a/lib/crates/fabro-server/src/static_files.rs b/lib/crates/fabro-server/src/static_files.rs index d9c0f0c46..cae56e925 100644 --- a/lib/crates/fabro-server/src/static_files.rs +++ b/lib/crates/fabro-server/src/static_files.rs @@ -1,4 +1,5 @@ use std::path::{Path, PathBuf}; +use std::sync::OnceLock; use axum::body::Body; use axum::http::{HeaderMap, HeaderValue, StatusCode, header}; @@ -15,15 +16,29 @@ pub fn serve_install(path: &str, headers: &HeaderMap) -> Response { } pub(crate) fn assert_install_mode_shell_ready() { - let index = load_asset("index.html").expect("install-mode SPA shell asset missing"); - let injected = inject_install_mode(index); - let html = String::from_utf8(injected).expect("install-mode SPA shell must be valid UTF-8"); + let shell = cached_install_mode_shell().clone().unwrap_or_else(|| { + load_injected_install_shell().expect("install-mode SPA shell asset missing") + }); + let html = String::from_utf8(shell).expect("install-mode SPA shell must be valid UTF-8"); assert!( html.contains(INSTALL_MODE_MARKER), "install-mode SPA shell marker missing after injection" ); } +fn cached_install_mode_shell() -> Option> { + if cfg!(debug_assertions) { + // In debug builds the SPA is reloaded from disk on every request. + return load_injected_install_shell(); + } + static SHELL: OnceLock>> = OnceLock::new(); + SHELL.get_or_init(load_injected_install_shell).clone() +} + +fn load_injected_install_shell() -> Option> { + Some(inject_install_mode(load_asset("index.html")?)) +} + #[derive(Clone, Copy, Debug, Eq, PartialEq)] enum SpaMode { Normal, @@ -88,16 +103,16 @@ fn load_asset(path: &str) -> Option> { } fn load_asset_for_mode(path: &str, mode: SpaMode) -> Option> { - let asset = load_asset(path)?; if mode == SpaMode::Install && path == "index.html" { - return Some(inject_install_mode(asset)); + return cached_install_mode_shell(); } - Some(asset) + load_asset(path) } fn inject_install_mode(bytes: Vec) -> Vec { - let Ok(html) = String::from_utf8(bytes.clone()) else { - return bytes; + let html = match String::from_utf8(bytes) { + Ok(html) => html, + Err(err) => return err.into_bytes(), }; if html.contains(INSTALL_MODE_MARKER) { return html.into_bytes(); diff --git a/lib/crates/fabro-server/tests/it/api/install.rs b/lib/crates/fabro-server/tests/it/api/install.rs index d61f71676..c00d39560 100644 --- a/lib/crates/fabro-server/tests/it/api/install.rs +++ b/lib/crates/fabro-server/tests/it/api/install.rs @@ -1,14 +1,73 @@ +use std::sync::Arc; +use std::sync::atomic::{AtomicBool, Ordering}; +use std::time::Duration; + use axum::body::Body; use axum::http::{Request, StatusCode}; use fabro_config::{Storage, parse_settings_layer, resolve_server_from_file}; use fabro_model::Provider; use fabro_server::install::{InstallAppState, build_install_router}; +use fabro_util::{Home, dev_token}; use fabro_vault::Vault; use httpmock::MockServer; +use tokio::time::sleep; use tower::ServiceExt; use crate::helpers::body_json; +async fn configure_token_install(app: &axum::Router, token: &str) { + let llm_response = app + .clone() + .oneshot( + Request::builder() + .method("PUT") + .uri("/install/llm") + .header("authorization", format!("Bearer {token}")) + .header("content-type", "application/json") + .body(Body::from( + r#"{"providers":[{"provider":"anthropic","api_key":"anthropic-test-key"}]}"#, + )) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(llm_response.status(), StatusCode::NO_CONTENT); + + let server_response = app + .clone() + .oneshot( + Request::builder() + .method("PUT") + .uri("/install/server") + .header("authorization", format!("Bearer {token}")) + .header("content-type", "application/json") + .body(Body::from( + r#"{"canonical_url":"https://fabro.example.com"}"#, + )) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(server_response.status(), StatusCode::NO_CONTENT); + + let github_response = app + .clone() + .oneshot( + Request::builder() + .method("PUT") + .uri("/install/github/token") + .header("authorization", format!("Bearer {token}")) + .header("content-type", "application/json") + .body(Body::from( + r#"{"token":"ghp_test_token","username":"brynary"}"#, + )) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(github_response.status(), StatusCode::NO_CONTENT); +} + #[tokio::test] async fn install_router_isolated_from_normal_api_surface() { let app = build_install_router(InstallAppState::for_test("test-install-token")); @@ -320,6 +379,41 @@ async fn token_install_finish_persists_settings_env_and_vault() { assert_eq!(vault.get("GITHUB_TOKEN"), Some("ghp_test_token")); } +#[tokio::test] +async fn token_install_finish_invokes_shutdown_callback_after_accepting() { + let temp_dir = tempfile::tempdir().unwrap(); + let home_root = tempfile::tempdir().unwrap(); + let config_path = temp_dir.path().join("settings.toml"); + let callback_invoked = Arc::new(AtomicBool::new(false)); + let callback_flag = Arc::clone(&callback_invoked); + let app = build_install_router( + InstallAppState::for_test_with_paths("test-install-token", temp_dir.path(), &config_path) + .with_home(Home::new(home_root.path().join(".fabro"))) + .with_finish_callback(Arc::new(move || { + callback_flag.store(true, Ordering::Release); + })), + ); + + configure_token_install(&app, "test-install-token").await; + + let finish_response = app + .oneshot( + Request::builder() + .method("POST") + .uri("/install/finish") + .header("authorization", "Bearer test-install-token") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(finish_response.status(), StatusCode::ACCEPTED); + assert!(!callback_invoked.load(Ordering::Acquire)); + + sleep(Duration::from_millis(650)).await; + assert!(callback_invoked.load(Ordering::Acquire)); +} + #[tokio::test] async fn install_validation_endpoints_validate_credentials_and_github_token() { let llm_mock = MockServer::start_async().await; @@ -902,6 +996,7 @@ async fn install_server_rejects_trailing_slash_canonical_urls() { #[tokio::test] async fn install_finish_failure_restores_settings_and_vault_but_leaves_env_keys() { let temp_dir = tempfile::tempdir().unwrap(); + let home_root = tempfile::tempdir().unwrap(); let config_path = temp_dir.path().join("settings.toml"); std::fs::write(&config_path, "_version = 1\n[project]\nname = \"keep\"\n").unwrap(); @@ -909,63 +1004,18 @@ async fn install_finish_failure_restores_settings_and_vault_but_leaves_env_keys( let vault_path = storage.secrets_path(); std::fs::create_dir_all(vault_path.parent().unwrap()).unwrap(); std::fs::write(&vault_path, "{ not valid json").unwrap(); + let callback_invoked = Arc::new(AtomicBool::new(false)); + let callback_flag = Arc::clone(&callback_invoked); - let app = build_install_router(InstallAppState::for_test_with_paths( - "test-install-token", - temp_dir.path(), - &config_path, - )); + let app = build_install_router( + InstallAppState::for_test_with_paths("test-install-token", temp_dir.path(), &config_path) + .with_home(Home::new(home_root.path().join(".fabro"))) + .with_finish_callback(Arc::new(move || { + callback_flag.store(true, Ordering::Release); + })), + ); - let llm_response = app - .clone() - .oneshot( - Request::builder() - .method("PUT") - .uri("/install/llm") - .header("authorization", "Bearer test-install-token") - .header("content-type", "application/json") - .body(Body::from( - r#"{"providers":[{"provider":"anthropic","api_key":"anthropic-test-key"}]}"#, - )) - .unwrap(), - ) - .await - .unwrap(); - assert_eq!(llm_response.status(), StatusCode::NO_CONTENT); - - let server_response = app - .clone() - .oneshot( - Request::builder() - .method("PUT") - .uri("/install/server") - .header("authorization", "Bearer test-install-token") - .header("content-type", "application/json") - .body(Body::from( - r#"{"canonical_url":"https://fabro.example.com"}"#, - )) - .unwrap(), - ) - .await - .unwrap(); - assert_eq!(server_response.status(), StatusCode::NO_CONTENT); - - let github_response = app - .clone() - .oneshot( - Request::builder() - .method("PUT") - .uri("/install/github/token") - .header("authorization", "Bearer test-install-token") - .header("content-type", "application/json") - .body(Body::from( - r#"{"token":"ghp_test_token","username":"brynary"}"#, - )) - .unwrap(), - ) - .await - .unwrap(); - assert_eq!(github_response.status(), StatusCode::NO_CONTENT); + configure_token_install(&app, "test-install-token").await; let finish_response = app .clone() @@ -1012,6 +1062,7 @@ async fn install_finish_failure_restores_settings_and_vault_but_leaves_env_keys( let server_env = std::fs::read_to_string(storage.server_state().env_path()).unwrap(); assert!(server_env.contains("SESSION_SECRET=")); assert!(server_env.contains("FABRO_DEV_TOKEN=")); + assert!(!callback_invoked.load(Ordering::Acquire)); let session_response = app .oneshot( @@ -1034,3 +1085,47 @@ async fn install_finish_failure_restores_settings_and_vault_but_leaves_env_keys( .any(|value| value == "github") ); } + +#[tokio::test] +async fn install_finish_failure_leaves_home_dev_token_mirror_written() { + let temp_dir = tempfile::tempdir().unwrap(); + let home_root = tempfile::tempdir().unwrap(); + let home = Home::new(home_root.path().join(".fabro")); + let config_path = temp_dir.path().join("settings.toml"); + std::fs::write(&config_path, "_version = 1\n[project]\nname = \"keep\"\n").unwrap(); + + let storage = Storage::new(temp_dir.path()); + let vault_path = storage.secrets_path(); + std::fs::create_dir_all(vault_path.parent().unwrap()).unwrap(); + std::fs::write(&vault_path, "{ not valid json").unwrap(); + + let app = build_install_router( + InstallAppState::for_test_with_paths("test-install-token", temp_dir.path(), &config_path) + .with_home(home.clone()), + ); + + configure_token_install(&app, "test-install-token").await; + + let finish_response = app + .oneshot( + Request::builder() + .method("POST") + .uri("/install/finish") + .header("authorization", "Bearer test-install-token") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(finish_response.status(), StatusCode::INTERNAL_SERVER_ERROR); + + let home_dev_token = dev_token::read_dev_token_file(&home.dev_token_path()) + .expect("home dev token should exist"); + let storage_dev_token = + dev_token::read_dev_token_file(&storage.server_state().dev_token_path()) + .expect("storage dev token should exist"); + assert_eq!(home_dev_token, storage_dev_token); + + let server_env = std::fs::read_to_string(storage.server_state().env_path()).unwrap(); + assert!(server_env.contains(&format!("FABRO_DEV_TOKEN={home_dev_token}"))); +}