From d1e8799828a08533926f5206c9baacf9c8225393 Mon Sep 17 00:00:00 2001 From: M3gA-Mind Date: Mon, 20 Jul 2026 18:48:22 +0530 Subject: [PATCH 1/2] fix(harness-init): show runtime-setup overlay on first run only (GH-5047) The Python/Node `is_done` probes used `try_cached()`, whose memo is process-local and empty after every restart. The orchestrator therefore re-entered a visible `running` state on every launch and the full-screen HarnessInitOverlay flashed even when everything was already installed. - Add durable, non-downloading `probe_installed()` to PythonBootstrap and NodeBootstrap (system detect / on-disk managed scan; never hits network) and use them for the harness-init readiness probes. - Classify each step `provisioning` (download/install) vs routine startup; only publish a visible `Running` state when a provisioning step genuinely needs work, so relaunching the already-installed Python server stays silent. - Persist the "Run in background" dismissal per provisioning run (sessionStorage + module mirror) so a remount/reload cannot reopen the overlay. - Regression tests: durable probe recovers readiness from disk after a simulated restart (Rust, Python+Node); provisioning classification; overlay warm-start / dismissal-persistence / new-run reopen (Vitest). --- .../HarnessInitOverlay.test.tsx | 98 ++++++++++++ .../InitProgressScreen/HarnessInitOverlay.tsx | 60 +++++++- src/openhuman/harness_init/ops.rs | 46 +++++- src/openhuman/harness_init/registry.rs | 51 ++++++- src/openhuman/runtime_node/bootstrap.rs | 139 ++++++++++++++++++ src/openhuman/runtime_python/bootstrap.rs | 76 ++++++++++ .../runtime_python/bootstrap_tests.rs | 61 ++++++++ 7 files changed, 522 insertions(+), 9 deletions(-) create mode 100644 app/src/components/InitProgressScreen/HarnessInitOverlay.test.tsx diff --git a/app/src/components/InitProgressScreen/HarnessInitOverlay.test.tsx b/app/src/components/InitProgressScreen/HarnessInitOverlay.test.tsx new file mode 100644 index 0000000000..9e214e6e67 --- /dev/null +++ b/app/src/components/InitProgressScreen/HarnessInitOverlay.test.tsx @@ -0,0 +1,98 @@ +import { fireEvent, screen, waitFor } from '@testing-library/react'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import type { HarnessInitSnapshot } from '../../services/harnessInitService'; +import { renderWithProviders } from '../../test/test-utils'; +// Imported after the mock is registered. +import HarnessInitOverlay from './HarnessInitOverlay'; + +// The overlay polls the service; drive it with a controllable mock. +const fetchHarnessInitStatus = vi.fn<() => Promise>(); +const runHarnessInit = vi.fn<(force?: boolean) => Promise>(); + +vi.mock('../../services/harnessInitService', () => ({ + fetchHarnessInitStatus: () => fetchHarnessInitStatus(), + runHarnessInit: (force?: boolean) => runHarnessInit(force), +})); + +function snapshot(overrides: Partial = {}): HarnessInitSnapshot { + return { + overall: 'running', + startedAt: '2026-07-20T00:00:00Z', + finishedAt: null, + steps: [ + { + id: 'python_runtime', + label: 'Python runtime', + required: false, + state: 'running', + message: null, + percent: null, + updatedAt: null, + }, + ], + ...overrides, + }; +} + +beforeEach(() => { + fetchHarnessInitStatus.mockReset(); + runHarnessInit.mockReset(); + window.sessionStorage.clear(); +}); + +afterEach(() => { + vi.restoreAllMocks(); +}); + +describe('HarnessInitOverlay', () => { + it('renders nothing on a warm start (already done)', async () => { + fetchHarnessInitStatus.mockResolvedValue(snapshot({ overall: 'done', startedAt: 'warm-run' })); + + const { container } = renderWithProviders(); + + await waitFor(() => expect(fetchHarnessInitStatus).toHaveBeenCalled()); + expect(screen.queryByText('Run in background')).not.toBeInTheDocument(); + expect(container).toBeEmptyDOMElement(); + }); + + it('shows the blocking overlay while a provisioning run is in progress', async () => { + fetchHarnessInitStatus.mockResolvedValue(snapshot({ startedAt: 'cold-run' })); + + renderWithProviders(); + + expect(await screen.findByText('Run in background')).toBeInTheDocument(); + }); + + it('keeps the overlay dismissed across a remount for the same run (GH-5047)', async () => { + const run = snapshot({ startedAt: 'same-run' }); + fetchHarnessInitStatus.mockResolvedValue(run); + + const first = renderWithProviders(); + fireEvent.click(await screen.findByText('Run in background')); + await waitFor(() => expect(screen.queryByText('Run in background')).not.toBeInTheDocument()); + first.unmount(); + + // Remount while the same run is still in progress — it must not reopen. + const second = renderWithProviders(); + await waitFor(() => expect(fetchHarnessInitStatus).toHaveBeenCalled()); + // Give any pending poll a chance to (wrongly) re-render the overlay. + await Promise.resolve(); + expect(screen.queryByText('Run in background')).not.toBeInTheDocument(); + expect(second.container).toBeEmptyDOMElement(); + }); + + it('reopens for a genuinely new provisioning run after a prior dismissal', async () => { + // Dismiss the first run. + fetchHarnessInitStatus.mockResolvedValue(snapshot({ startedAt: 'run-1' })); + const first = renderWithProviders(); + fireEvent.click(await screen.findByText('Run in background')); + await waitFor(() => expect(screen.queryByText('Run in background')).not.toBeInTheDocument()); + first.unmount(); + + // A new run (fresh startedAt) is allowed to surface again. + fetchHarnessInitStatus.mockResolvedValue(snapshot({ startedAt: 'run-2' })); + renderWithProviders(); + expect(await screen.findByText('Run in background')).toBeInTheDocument(); + }); +}); diff --git a/app/src/components/InitProgressScreen/HarnessInitOverlay.tsx b/app/src/components/InitProgressScreen/HarnessInitOverlay.tsx index eeed28a190..69c4c47f8f 100644 --- a/app/src/components/InitProgressScreen/HarnessInitOverlay.tsx +++ b/app/src/components/InitProgressScreen/HarnessInitOverlay.tsx @@ -12,6 +12,47 @@ const log = debugFactory('harness-init'); const POLL_MS = 2000; +// Persist the "Run in background" dismissal for the *current* provisioning run +// so a remount or reload does not reopen the overlay (GH-5047). A run is keyed +// by its `startedAt` timestamp — a genuinely new provisioning run gets a fresh +// timestamp and is allowed to surface again. `sessionStorage` survives a +// renderer reload within the same window; a module-level mirror covers plain +// React remounts even if storage is unavailable. +const DISMISS_KEY = 'harness-init-dismissed-run'; +// Runs before `startedAt` is stamped (or when it is absent) still need a stable +// key so an early dismissal sticks. +const UNKEYED_RUN = 'pending'; + +let dismissedRunMirror: string | null = null; + +function runKey(snapshot: HarnessInitSnapshot | null): string { + return snapshot?.startedAt ?? UNKEYED_RUN; +} + +function readDismissedRun(): string | null { + if (dismissedRunMirror !== null) { + return dismissedRunMirror; + } + try { + return window.sessionStorage.getItem(DISMISS_KEY); + } catch { + return null; + } +} + +function writeDismissedRun(key: string): void { + dismissedRunMirror = key; + try { + window.sessionStorage.setItem(DISMISS_KEY, key); + } catch { + // Non-fatal: the module-level mirror still guards remounts this session. + } +} + +function isRunDismissed(snapshot: HarnessInitSnapshot | null): boolean { + return readDismissedRun() === runKey(snapshot); +} + /** * Blocking first-run initialization gate. * @@ -47,6 +88,14 @@ export default function HarnessInitOverlay() { } if (next) { setSnapshot(next); + // If this run was already dismissed to the background (possibly in a + // prior mount / before a reload), stay hidden and stop polling — + // don't let a remount reopen the overlay (GH-5047). + if (isRunDismissed(next)) { + dismissedRef.current = true; + setDismissed(true); + return; + } // Stop polling once the run is terminal; a `failed` snapshot stays // on screen (with Retry) but does not need further polling. if (next.overall === 'done' || next.overall === 'failed') { @@ -88,15 +137,22 @@ export default function HarnessInitOverlay() { const handleContinue = useCallback(() => { // Hide the overlay and stop polling; the core keeps running init as a - // background task regardless. + // background task regardless. Persist the dismissal for this run so a + // remount/reload does not reopen it (GH-5047). + writeDismissedRun(runKey(snapshot)); dismissedRef.current = true; setDismissed(true); - }, []); + }, [snapshot]); if (dismissed || !snapshot) { return null; } + // A run dismissed to the background stays hidden across remounts. + if (isRunDismissed(snapshot)) { + return null; + } + // Block only while a run is actively in progress, or hold a failed run on // screen until the user explicitly continues. `idle` (no run started yet) // and `done` never block. diff --git a/src/openhuman/harness_init/ops.rs b/src/openhuman/harness_init/ops.rs index a09b7270e3..bb2db7663e 100644 --- a/src/openhuman/harness_init/ops.rs +++ b/src/openhuman/harness_init/ops.rs @@ -19,12 +19,30 @@ use crate::openhuman::config::Config; /// *required* step fails. pub async fn run_harness_init_with(config: Config, force: bool) -> HarnessInitSnapshot { log::info!("[harness_init] starting one-time init run (force={force})"); - store::set_overall(OverallState::Running); + + let steps = registry::all_steps(); + + // Warm-start guard (GH-5047): decide visibility *before* publishing any + // `Running` state. The blocking overlay is justified only when a + // provisioning step (download/install/repair) genuinely needs work. Routine + // startup — e.g. re-launching an already-installed local Python server — is + // NOT provisioning and must run silently in the background on every launch. + // Probing on-disk readiness first (durably) keeps the overlay off warm + // restarts entirely instead of flashing it while the run settles. + let needs_provisioning = force || provisioning_required(&config, &steps).await; + if needs_provisioning { + log::info!("[harness_init] provisioning required — surfacing setup overlay"); + store::set_overall(OverallState::Running); + } else { + log::info!( + "[harness_init] warm start — all provisioning satisfied; running remaining startup silently" + ); + } let mut failed_required = false; - for step in registry::all_steps() { - run_one(&config, &step, force, &mut failed_required).await; + for step in &steps { + run_one(&config, step, force, &mut failed_required).await; } let overall = if failed_required { @@ -43,6 +61,28 @@ pub async fn run_harness_init(config: Config) { run_harness_init_with(config, false).await; } +/// Whether any *provisioning* step (download/install/repair) still needs work. +/// +/// Only provisioning steps gate the visible overlay; a non-provisioning step +/// (routine service startup) is ignored here so an already-installed host never +/// surfaces the blocking screen just to relaunch a background server. Each +/// probe is the step's durable, network-free `is_done` — see [`registry`]. +async fn provisioning_required(config: &Config, steps: &[HarnessInitStep]) -> bool { + for step in steps { + if !step.provisioning { + continue; + } + if !(step.is_done)(config).await { + log::info!( + "[harness_init] provisioning step {} not satisfied on disk — setup needed", + step.id + ); + return true; + } + } + false +} + async fn run_one(config: &Config, step: &HarnessInitStep, force: bool, failed_required: &mut bool) { if !force && (step.is_done)(config).await { log::debug!("[harness_init] step {} already satisfied", step.id); diff --git a/src/openhuman/harness_init/registry.rs b/src/openhuman/harness_init/registry.rs index b76c890565..80a793d00a 100644 --- a/src/openhuman/harness_init/registry.rs +++ b/src/openhuman/harness_init/registry.rs @@ -30,8 +30,20 @@ pub struct HarnessInitStep { pub label: &'static str, /// When true, a failure blocks the app. All steps are non-required today. pub required: bool, + /// When true this step may **download/install/repair** — the only kind of + /// work that justifies the blocking first-run overlay. When false the step + /// is routine startup (e.g. launching an already-provisioned local server) + /// that must run silently in the background on every launch and must NOT + /// surface the overlay. Drives the warm-start visibility decision in + /// [`super::ops`]. + pub provisioning: bool, /// Cheap, network-free probe: is this step already satisfied on this host? /// When true the orchestrator marks it `Done` without invoking `run`. + /// + /// **Must be durable across process restarts** for provisioning steps — a + /// process-local memo (e.g. `try_cached`) reads empty on every launch and + /// would wrongly re-enter a visible `running` state (GH-5047). Probe the + /// on-disk install instead. pub is_done: for<'a> fn(&'a Config) -> StepFuture<'a, bool>, /// Perform the work. `Ok(())` → done; `Err(msg)` → failed/skipped. pub run: for<'a> fn(&'a Config) -> StepFuture<'a, Result<(), String>>, @@ -55,6 +67,7 @@ fn python_runtime_step() -> HarnessInitStep { id: "python_runtime", label: "Python runtime", required: false, + provisioning: true, is_done: |config| Box::pin(python_is_done(config)), run: |config| Box::pin(python_run(config)), } @@ -65,11 +78,13 @@ async fn python_is_done(config: &Config) -> bool { // Disabled → nothing to provision; treat as satisfied. return true; } - // Memoised within this process. Across restarts this is None at boot and - // the (fast) `resolve()` probe in `run` settles it quickly. + // Durable on-disk probe: survives restarts (unlike the process-local + // `try_cached`), so an already-installed interpreter is detected without + // entering a user-visible provisioning run (GH-5047). Never downloads. use crate::openhuman::runtime_python::PythonBootstrap; PythonBootstrap::new(config.runtime_python.clone()) - .try_cached() + .probe_installed() + .await .is_some() } @@ -98,6 +113,10 @@ fn runtime_python_server_step() -> HarnessInitStep { id: "runtime_python_server", label: "Runtime Python server", required: false, + // Routine service startup, not an install: launching an already + // provisioned server must stay silent (its venv is provisioned by the + // `spacy` / `kompress` steps, which are the ones that surface progress). + provisioning: false, is_done: |config| Box::pin(runtime_python_server_is_done(config)), run: |config| Box::pin(runtime_python_server_run(config)), } @@ -128,6 +147,7 @@ fn spacy_step() -> HarnessInitStep { id: "spacy", label: "spaCy language model", required: false, + provisioning: true, is_done: |config| Box::pin(spacy_is_done(config)), run: |config| Box::pin(spacy_run(config)), } @@ -159,6 +179,7 @@ fn kompress_step() -> HarnessInitStep { id: "kompress", label: "TokenJuice ML compressor (torch)", required: false, + provisioning: true, is_done: |config| Box::pin(kompress_is_done(config)), run: |config| Box::pin(kompress_run(config)), } @@ -199,6 +220,7 @@ fn node_runtime_step() -> HarnessInitStep { id: "node_runtime", label: "Node.js runtime", required: false, + provisioning: true, is_done: |config| Box::pin(node_is_done(config)), run: |config| Box::pin(node_run(config)), } @@ -216,7 +238,13 @@ async fn node_is_done(config: &Config) -> bool { if !config.node.enabled { return true; } - build_node_bootstrap(config).try_cached().is_some() + // Durable on-disk probe: survives restarts (unlike the process-local + // `try_cached`), so an already-installed toolchain is detected without + // entering a user-visible provisioning run (GH-5047). Never downloads. + build_node_bootstrap(config) + .probe_installed() + .await + .is_some() } async fn node_run(config: &Config) -> Result<(), String> { @@ -258,6 +286,21 @@ mod tests { assert!(steps.iter().all(|s| !s.label.is_empty())); } + /// GH-5047: only genuine install/download steps may surface the blocking + /// overlay. `runtime_python_server` is routine service startup and must be + /// classified non-provisioning so a warm restart never re-shows setup. + #[test] + fn provisioning_classification_excludes_service_startup() { + for step in all_steps() { + let expected = step.id != "runtime_python_server"; + assert_eq!( + step.provisioning, expected, + "step {} provisioning flag mismatch", + step.id + ); + } + } + #[tokio::test] async fn disabled_runtimes_report_done_without_work() { let mut config = Config::default(); diff --git a/src/openhuman/runtime_node/bootstrap.rs b/src/openhuman/runtime_node/bootstrap.rs index d5256e7d1f..bf9685ed84 100644 --- a/src/openhuman/runtime_node/bootstrap.rs +++ b/src/openhuman/runtime_node/bootstrap.rs @@ -89,6 +89,57 @@ impl NodeBootstrap { self.cached.try_lock().ok().and_then(|g| g.clone()) } + /// Durable, **non-downloading** readiness probe. + /// + /// Unlike [`try_cached`], whose state is process-local and therefore empty + /// after every app restart, this inspects the host: a compatible system + /// `node` (when `prefer_system`) or an already-extracted managed + /// distribution under the cache root. The managed install directory is + /// derived deterministically from the configured version + host arch (no + /// network), so the check is purely filesystem `stat`s. A hit is memoised + /// into the same cache `resolve()` uses. + /// + /// Returns `Some(..)` when Node is already provisioned on disk, `None` when + /// a genuine download/install is still required (or the runtime is + /// disabled — callers treat that as "nothing to provision" separately). + pub async fn probe_installed(&self) -> Option { + if let Some(existing) = self.try_cached() { + return Some(existing); + } + if !self.config.enabled { + return None; + } + if self.config.prefer_system { + if let Some(system) = detect_system_node(&self.config.version) { + if let Ok(resolved) = resolve_from_system(system) { + tracing::debug!( + version = %resolved.version, + "[node_runtime::bootstrap] durable probe found system node" + ); + *self.cached.lock().await = Some(resolved.clone()); + return Some(resolved); + } + } + } + let dist = NodeDistribution::for_host(&self.config.version).ok()?; + let install_dir = self.install_dir(&dist); + let cache_root = self.cache_root(); + if let Some(resolved) = + probe_managed_install(&install_dir, &cache_root, &self.config.version) + { + tracing::debug!( + version = %resolved.version, + "[node_runtime::bootstrap] durable probe found managed node on disk" + ); + *self.cached.lock().await = Some(resolved.clone()); + return Some(resolved); + } + tracing::debug!( + "[node_runtime::bootstrap] durable probe found no installed node (provisioning required)" + ); + None + } + /// Resolve the Node.js toolchain, downloading + extracting a managed /// distribution if necessary. Idempotent: the first successful call /// memoises the result; later calls return it without further I/O. @@ -359,3 +410,91 @@ fn probe_managed_install( } Some(resolved) } + +#[cfg(test)] +mod probe_installed_tests { + use super::*; + + fn touch(path: &Path) { + if let Some(parent) = path.parent() { + std::fs::create_dir_all(parent).unwrap(); + } + std::fs::write(path, b"#!/bin/sh\n").unwrap(); + } + + fn managed_config(cache_root: &Path) -> NodeConfig { + NodeConfig { + enabled: true, + version: NodeConfig::default().version, + cache_dir: cache_root.to_string_lossy().to_string(), + // Force the managed path so the probe never depends on a host node. + prefer_system: false, + } + } + + /// GH-5047: a warm restart is a fresh process, so the in-memory + /// `try_cached` memo is empty. The durable probe must still recover + /// readiness from the on-disk managed install — otherwise `is_done` reports + /// "not ready" every launch and the harness-init overlay re-appears. + #[tokio::test] + async fn probe_installed_true_from_disk_after_simulated_restart() { + let tmp = tempfile::tempdir().expect("tempdir"); + let cache_root = tmp.path(); + + let config = managed_config(cache_root); + let dist = NodeDistribution::for_host(&config.version).expect("host arch supported"); + let bootstrap = NodeBootstrap::new(config, tmp.path().join("ws"), Client::new()); + + // Lay down a managed install exactly where the bootstrap expects it. + let bin_dir = managed_bin_dir(&bootstrap.install_dir(&dist)); + let (node_name, npm_name) = if cfg!(windows) { + ("node.exe", "npm.cmd") + } else { + ("node", "npm") + }; + touch(&bin_dir.join(node_name)); + touch(&bin_dir.join(npm_name)); + + // Simulated cold process: nothing memoised yet. + assert!( + bootstrap.try_cached().is_none(), + "precondition: process-local cache is empty right after a restart" + ); + + // The durable probe recovers readiness from disk (and never downloads). + assert!( + bootstrap.probe_installed().await.is_some(), + "durable probe should detect the on-disk managed node install" + ); + assert!( + bootstrap.try_cached().is_some(), + "a probe hit should memoise into the shared cache for the rest of the process" + ); + } + + /// A fresh machine (empty cache, no install) must report "not installed" so + /// a genuine first-run download still runs and the overlay still shows. + #[tokio::test] + async fn probe_installed_none_when_nothing_on_disk() { + let tmp = tempfile::tempdir().expect("tempdir"); + let bootstrap = NodeBootstrap::new( + managed_config(tmp.path()), + tmp.path().join("ws"), + Client::new(), + ); + assert!( + bootstrap.probe_installed().await.is_none(), + "no on-disk install → provisioning still required" + ); + } + + /// A disabled runtime is "nothing to provision", not "installed". + #[tokio::test] + async fn probe_installed_none_when_disabled() { + let tmp = tempfile::tempdir().expect("tempdir"); + let mut config = managed_config(tmp.path()); + config.enabled = false; + let bootstrap = NodeBootstrap::new(config, tmp.path().join("ws"), Client::new()); + assert!(bootstrap.probe_installed().await.is_none()); + } +} diff --git a/src/openhuman/runtime_python/bootstrap.rs b/src/openhuman/runtime_python/bootstrap.rs index 3bb729d077..7868ef540b 100644 --- a/src/openhuman/runtime_python/bootstrap.rs +++ b/src/openhuman/runtime_python/bootstrap.rs @@ -100,6 +100,54 @@ impl PythonBootstrap { Ok(managed) } + /// Durable, **non-downloading** readiness probe. + /// + /// Unlike [`try_cached`], whose state is process-local and therefore empty + /// after every app restart, this inspects the host itself: a compatible + /// system interpreter (when `prefer_system`) or an already-extracted managed + /// distribution under the cache root. It performs only cheap `stat`/version + /// checks and **never** touches the network, so it is safe to call as a + /// warm-start guard before deciding whether to surface a visible + /// provisioning run. A hit is memoised into the same cache `resolve()` uses. + /// + /// Returns `Some(..)` when Python is already provisioned on disk, `None` + /// when a genuine download/install is still required (or the runtime is + /// disabled — callers treat that as "nothing to provision" separately). + pub async fn probe_installed(&self) -> Option { + if let Some(existing) = self.try_cached() { + return Some(existing); + } + if !self.config.enabled { + return None; + } + if self.config.prefer_system { + if let Some(system) = detect_system_python( + &self.config.minimum_version, + empty_to_none(&self.config.preferred_command), + ) { + let resolved = resolve_from_system(system); + tracing::debug!( + version = %resolved.version, + "[runtime_python::bootstrap] durable probe found system python" + ); + *self.cached.lock().await = Some(resolved.clone()); + return Some(resolved); + } + } + if let Some(resolved) = probe_any_managed_install(&self.cache_root()) { + tracing::debug!( + version = %resolved.version, + "[runtime_python::bootstrap] durable probe found managed python on disk" + ); + *self.cached.lock().await = Some(resolved.clone()); + return Some(resolved); + } + tracing::debug!( + "[runtime_python::bootstrap] durable probe found no installed python (provisioning required)" + ); + None + } + /// Build a preconfigured child-process launcher for stdio-oriented Python /// workloads such as MCP servers. pub async fn spawn_stdio( @@ -210,6 +258,34 @@ fn empty_to_none(value: &str) -> Option<&str> { } } +/// Scan the managed cache root for an already-extracted CPython install, +/// probing each immediate subdirectory without any network access. Returns the +/// first usable interpreter found. Used by the durable readiness probe so a +/// warm restart detects a prior managed install without re-downloading — the +/// exact install-dir name is derived from network release metadata during +/// install, so on a cold cache we cannot reconstruct it offline and must scan. +fn probe_any_managed_install(cache_root: &Path) -> Option { + let entries = std::fs::read_dir(cache_root).ok()?; + for entry in entries.flatten() { + let path = entry.path(); + if !path.is_dir() { + continue; + } + // Skip transient staging dirs (`.stage--`) left mid-install. + if path + .file_name() + .and_then(|n| n.to_str()) + .is_some_and(|n| n.starts_with(".stage-")) + { + continue; + } + if let Some(resolved) = probe_managed_install(&path) { + return Some(resolved); + } + } + None +} + fn probe_managed_install(install_dir: &Path) -> Option { let python_bin = find_python_binary(install_dir)?; let version = super::resolver::probe_python_version_public(&python_bin)?; diff --git a/src/openhuman/runtime_python/bootstrap_tests.rs b/src/openhuman/runtime_python/bootstrap_tests.rs index 3146b2b29e..a1e4833edc 100644 --- a/src/openhuman/runtime_python/bootstrap_tests.rs +++ b/src/openhuman/runtime_python/bootstrap_tests.rs @@ -167,3 +167,64 @@ fn test_asset_name() -> &'static str { fn test_asset_name() -> &'static str { "cpython-3.12.13+20260510-aarch64-unknown-linux-gnu-install_only.tar.gz" } + +/// GH-5047: a warm restart is a fresh process, so `try_cached` is empty. The +/// durable probe must still recover readiness from a prior managed install on +/// disk — otherwise `is_done` reports "not ready" every launch and the +/// harness-init overlay re-appears. +#[cfg(unix)] +#[tokio::test] +async fn probe_installed_true_from_disk_after_simulated_restart() { + use std::os::unix::fs::PermissionsExt; + + let tmp = tempfile::tempdir().expect("tempdir"); + let cache_root = tmp.path().join("runtime-python"); + // A prior managed install, extracted under the cache root. + let bin_dir = cache_root.join("cpython-3.12.13-managed").join("bin"); + std::fs::create_dir_all(&bin_dir).expect("mkdir install"); + let python_path = bin_dir.join("python3.12"); + std::fs::write(&python_path, b"#!/bin/sh\necho 'Python 3.12.13'\n").expect("write python stub"); + std::fs::set_permissions(&python_path, std::fs::Permissions::from_mode(0o755)).expect("chmod"); + + let mut cfg = RuntimePythonConfig::default(); + cfg.enabled = true; + cfg.cache_dir = cache_root.display().to_string(); + cfg.prefer_system = false; // force the managed-scan path — no host dependency + + let bootstrap = PythonBootstrap::new(cfg); + assert!( + bootstrap.try_cached().is_none(), + "precondition: process-local cache is empty right after a restart" + ); + let resolved = bootstrap.probe_installed().await; + assert!( + resolved.is_some(), + "durable probe should recover the managed python install from disk" + ); + assert_eq!(resolved.unwrap().source, PythonSource::Managed); +} + +/// A fresh machine (empty cache, no install) must report "not installed" so a +/// genuine first-run download still runs and the overlay still shows. +#[tokio::test] +async fn probe_installed_none_when_nothing_on_disk() { + let tmp = tempfile::tempdir().expect("tempdir"); + let mut cfg = RuntimePythonConfig::default(); + cfg.enabled = true; + cfg.cache_dir = tmp.path().join("runtime-python").display().to_string(); + cfg.prefer_system = false; + let bootstrap = PythonBootstrap::new(cfg); + assert!( + bootstrap.probe_installed().await.is_none(), + "no on-disk install → provisioning still required" + ); +} + +/// A disabled runtime is "nothing to provision", not "installed". +#[tokio::test] +async fn probe_installed_none_when_disabled() { + let mut cfg = RuntimePythonConfig::default(); + cfg.enabled = false; + let bootstrap = PythonBootstrap::new(cfg); + assert!(bootstrap.probe_installed().await.is_none()); +} From 4e2c70c9742903efad8ff2dcfa2c7d99ac87fe24 Mon Sep 17 00:00:00 2001 From: M3gA-Mind Date: Mon, 20 Jul 2026 21:22:39 +0530 Subject: [PATCH 2/2] =?UTF-8?q?fix(harness-init):=20address=20review=20?= =?UTF-8?q?=E2=80=94=20symlink-safe=20probe,=20test=20split,=20dismissal?= =?UTF-8?q?=20logging?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address CodeRabbit review on #5066 (#5047): - security(runtime_python): reject symlinked cache entries in probe_any_managed_install before the is_dir() check. path.is_dir() follows symlinks, so a link under cache_root could point the durable probe at a dir outside the root and be trusted as managed Python; entry.file_type() reports the link itself and is skipped. (The Node bootstrap already gets equivalent protection from its canonicalize + starts_with(cache_root) guard.) - refactor(runtime_node): extract the inline probe_installed_tests module into runtime_node/bootstrap_tests.rs, wired via #[cfg(test)] #[path = "bootstrap_tests.rs"] mod tests; — matching the sibling runtime_python pattern and taking bootstrap.rs back under the ~500-line ceiling. - logging(HarnessInitOverlay): add namespaced harness-init debug diagnostics across the dismissal-persistence flow — read/write helpers (incl. sessionStorage-failure paths), the warm-poll stay-hidden branch, and the handleContinue dismissal action. Targeted: cargo fmt; cargo test probe_installed (6 passed, incl. the moved node module); prettier on the overlay. --- .../InitProgressScreen/HarnessInitOverlay.tsx | 8 ++ src/openhuman/runtime_node/bootstrap.rs | 88 +------------------ src/openhuman/runtime_node/bootstrap_tests.rs | 84 ++++++++++++++++++ src/openhuman/runtime_python/bootstrap.rs | 12 +++ 4 files changed, 106 insertions(+), 86 deletions(-) create mode 100644 src/openhuman/runtime_node/bootstrap_tests.rs diff --git a/app/src/components/InitProgressScreen/HarnessInitOverlay.tsx b/app/src/components/InitProgressScreen/HarnessInitOverlay.tsx index 69c4c47f8f..0b6b0836da 100644 --- a/app/src/components/InitProgressScreen/HarnessInitOverlay.tsx +++ b/app/src/components/InitProgressScreen/HarnessInitOverlay.tsx @@ -36,6 +36,7 @@ function readDismissedRun(): string | null { try { return window.sessionStorage.getItem(DISMISS_KEY); } catch { + log('sessionStorage read failed; treating run as not dismissed'); return null; } } @@ -44,8 +45,10 @@ function writeDismissedRun(key: string): void { dismissedRunMirror = key; try { window.sessionStorage.setItem(DISMISS_KEY, key); + log('dismissed run persisted to sessionStorage: %s', key); } catch { // Non-fatal: the module-level mirror still guards remounts this session. + log('sessionStorage unavailable; dismissed run %s held in module mirror only', key); } } @@ -92,6 +95,10 @@ export default function HarnessInitOverlay() { // prior mount / before a reload), stay hidden and stop polling — // don't let a remount reopen the overlay (GH-5047). if (isRunDismissed(next)) { + log( + 'warm poll: run %s already dismissed — staying hidden, stopping poll', + runKey(next) + ); dismissedRef.current = true; setDismissed(true); return; @@ -139,6 +146,7 @@ export default function HarnessInitOverlay() { // Hide the overlay and stop polling; the core keeps running init as a // background task regardless. Persist the dismissal for this run so a // remount/reload does not reopen it (GH-5047). + log('user dismissed overlay to background for run %s', runKey(snapshot)); writeDismissedRun(runKey(snapshot)); dismissedRef.current = true; setDismissed(true); diff --git a/src/openhuman/runtime_node/bootstrap.rs b/src/openhuman/runtime_node/bootstrap.rs index bf9685ed84..9edf048d69 100644 --- a/src/openhuman/runtime_node/bootstrap.rs +++ b/src/openhuman/runtime_node/bootstrap.rs @@ -412,89 +412,5 @@ fn probe_managed_install( } #[cfg(test)] -mod probe_installed_tests { - use super::*; - - fn touch(path: &Path) { - if let Some(parent) = path.parent() { - std::fs::create_dir_all(parent).unwrap(); - } - std::fs::write(path, b"#!/bin/sh\n").unwrap(); - } - - fn managed_config(cache_root: &Path) -> NodeConfig { - NodeConfig { - enabled: true, - version: NodeConfig::default().version, - cache_dir: cache_root.to_string_lossy().to_string(), - // Force the managed path so the probe never depends on a host node. - prefer_system: false, - } - } - - /// GH-5047: a warm restart is a fresh process, so the in-memory - /// `try_cached` memo is empty. The durable probe must still recover - /// readiness from the on-disk managed install — otherwise `is_done` reports - /// "not ready" every launch and the harness-init overlay re-appears. - #[tokio::test] - async fn probe_installed_true_from_disk_after_simulated_restart() { - let tmp = tempfile::tempdir().expect("tempdir"); - let cache_root = tmp.path(); - - let config = managed_config(cache_root); - let dist = NodeDistribution::for_host(&config.version).expect("host arch supported"); - let bootstrap = NodeBootstrap::new(config, tmp.path().join("ws"), Client::new()); - - // Lay down a managed install exactly where the bootstrap expects it. - let bin_dir = managed_bin_dir(&bootstrap.install_dir(&dist)); - let (node_name, npm_name) = if cfg!(windows) { - ("node.exe", "npm.cmd") - } else { - ("node", "npm") - }; - touch(&bin_dir.join(node_name)); - touch(&bin_dir.join(npm_name)); - - // Simulated cold process: nothing memoised yet. - assert!( - bootstrap.try_cached().is_none(), - "precondition: process-local cache is empty right after a restart" - ); - - // The durable probe recovers readiness from disk (and never downloads). - assert!( - bootstrap.probe_installed().await.is_some(), - "durable probe should detect the on-disk managed node install" - ); - assert!( - bootstrap.try_cached().is_some(), - "a probe hit should memoise into the shared cache for the rest of the process" - ); - } - - /// A fresh machine (empty cache, no install) must report "not installed" so - /// a genuine first-run download still runs and the overlay still shows. - #[tokio::test] - async fn probe_installed_none_when_nothing_on_disk() { - let tmp = tempfile::tempdir().expect("tempdir"); - let bootstrap = NodeBootstrap::new( - managed_config(tmp.path()), - tmp.path().join("ws"), - Client::new(), - ); - assert!( - bootstrap.probe_installed().await.is_none(), - "no on-disk install → provisioning still required" - ); - } - - /// A disabled runtime is "nothing to provision", not "installed". - #[tokio::test] - async fn probe_installed_none_when_disabled() { - let tmp = tempfile::tempdir().expect("tempdir"); - let mut config = managed_config(tmp.path()); - config.enabled = false; - let bootstrap = NodeBootstrap::new(config, tmp.path().join("ws"), Client::new()); - assert!(bootstrap.probe_installed().await.is_none()); - } -} +#[path = "bootstrap_tests.rs"] +mod tests; diff --git a/src/openhuman/runtime_node/bootstrap_tests.rs b/src/openhuman/runtime_node/bootstrap_tests.rs new file mode 100644 index 0000000000..a0728ecc1f --- /dev/null +++ b/src/openhuman/runtime_node/bootstrap_tests.rs @@ -0,0 +1,84 @@ +use super::*; + +fn touch(path: &Path) { + if let Some(parent) = path.parent() { + std::fs::create_dir_all(parent).unwrap(); + } + std::fs::write(path, b"#!/bin/sh\n").unwrap(); +} + +fn managed_config(cache_root: &Path) -> NodeConfig { + NodeConfig { + enabled: true, + version: NodeConfig::default().version, + cache_dir: cache_root.to_string_lossy().to_string(), + // Force the managed path so the probe never depends on a host node. + prefer_system: false, + } +} + +/// GH-5047: a warm restart is a fresh process, so the in-memory +/// `try_cached` memo is empty. The durable probe must still recover +/// readiness from the on-disk managed install — otherwise `is_done` reports +/// "not ready" every launch and the harness-init overlay re-appears. +#[tokio::test] +async fn probe_installed_true_from_disk_after_simulated_restart() { + let tmp = tempfile::tempdir().expect("tempdir"); + let cache_root = tmp.path(); + + let config = managed_config(cache_root); + let dist = NodeDistribution::for_host(&config.version).expect("host arch supported"); + let bootstrap = NodeBootstrap::new(config, tmp.path().join("ws"), Client::new()); + + // Lay down a managed install exactly where the bootstrap expects it. + let bin_dir = managed_bin_dir(&bootstrap.install_dir(&dist)); + let (node_name, npm_name) = if cfg!(windows) { + ("node.exe", "npm.cmd") + } else { + ("node", "npm") + }; + touch(&bin_dir.join(node_name)); + touch(&bin_dir.join(npm_name)); + + // Simulated cold process: nothing memoised yet. + assert!( + bootstrap.try_cached().is_none(), + "precondition: process-local cache is empty right after a restart" + ); + + // The durable probe recovers readiness from disk (and never downloads). + assert!( + bootstrap.probe_installed().await.is_some(), + "durable probe should detect the on-disk managed node install" + ); + assert!( + bootstrap.try_cached().is_some(), + "a probe hit should memoise into the shared cache for the rest of the process" + ); +} + +/// A fresh machine (empty cache, no install) must report "not installed" so +/// a genuine first-run download still runs and the overlay still shows. +#[tokio::test] +async fn probe_installed_none_when_nothing_on_disk() { + let tmp = tempfile::tempdir().expect("tempdir"); + let bootstrap = NodeBootstrap::new( + managed_config(tmp.path()), + tmp.path().join("ws"), + Client::new(), + ); + assert!( + bootstrap.probe_installed().await.is_none(), + "no on-disk install → provisioning still required" + ); +} + +/// A disabled runtime is "nothing to provision", not "installed". +#[tokio::test] +async fn probe_installed_none_when_disabled() { + let tmp = tempfile::tempdir().expect("tempdir"); + let mut config = managed_config(tmp.path()); + config.enabled = false; + let bootstrap = NodeBootstrap::new(config, tmp.path().join("ws"), Client::new()); + assert!(bootstrap.probe_installed().await.is_none()); +} diff --git a/src/openhuman/runtime_python/bootstrap.rs b/src/openhuman/runtime_python/bootstrap.rs index 7868ef540b..86a42709cb 100644 --- a/src/openhuman/runtime_python/bootstrap.rs +++ b/src/openhuman/runtime_python/bootstrap.rs @@ -268,6 +268,18 @@ fn probe_any_managed_install(cache_root: &Path) -> Option { let entries = std::fs::read_dir(cache_root).ok()?; for entry in entries.flatten() { let path = entry.path(); + // Reject symlinked cache entries before probing: `path.is_dir()` follows + // symlinks, so a link planted under `cache_root` could point the durable + // probe at a directory *outside* the cache root and have it reused as a + // trusted managed install. `entry.file_type()` reports the link itself + // (it does not follow), so skip anything symlinked; treat an unknown type + // as untrusted too. (The Node bootstrap gets equivalent protection from + // its `canonicalize` + `starts_with(cache_root)` guard.) + match entry.file_type() { + Ok(ft) if ft.is_symlink() => continue, + Ok(_) => {} + Err(_) => continue, + } if !path.is_dir() { continue; }