From c59ca8013f90ce85db02044a11ce41e3a6ee58c4 Mon Sep 17 00:00:00 2001 From: James Pine Date: Fri, 6 Mar 2026 18:11:11 -0800 Subject: [PATCH 1/5] fix: browser tabs always destroyed when workers finish When persist_session is true, close_policy still defaulted to CloseBrowser, so the LLM's close call (prompted by the worker system prompt) killed the entire Chrome process. The shared BrowserState handle survived the worker drop correctly but was left holding wiped state. Two fixes: - Default close_policy to Detach when persist_session is enabled and no explicit close_policy is configured (config/load.rs) - Conditionalize the worker prompt so persistent sessions say 'Detach from the browser' instead of 'Shut down the browser' --- prompts/en/worker.md.j2 | 4 ++++ src/agent/channel_dispatch.rs | 6 ++++-- src/agent/cortex.rs | 2 ++ src/config/load.rs | 25 ++++++++++++++++++++++--- src/prompts/engine.rs | 2 ++ 5 files changed, 34 insertions(+), 5 deletions(-) diff --git a/prompts/en/worker.md.j2 b/prompts/en/worker.md.j2 index d0b92d4f7..111f0b18f 100644 --- a/prompts/en/worker.md.j2 +++ b/prompts/en/worker.md.j2 @@ -100,7 +100,11 @@ Automate a headless Chrome browser. Use this for web scraping, testing web inter 3. `snapshot` — Get the page's accessibility tree with element refs (e1, e2, e3...) 4. `act` — Interact with elements by ref: `click`, `type`, `press_key`, `hover`, `scroll_into_view`, `focus` 5. `screenshot` — Capture the page or a specific element +{%- if browser_persist_session %} +6. `close` — Detach from the browser when done (tabs and session are preserved for the next worker) +{%- else %} 6. `close` — Shut down the browser when done +{%- endif %} **Multi-tab support:** Use `open` to create new tabs, `tabs` to list them, `focus` to switch between them, `close_tab` to close one. diff --git a/src/agent/channel_dispatch.rs b/src/agent/channel_dispatch.rs index 216557a9d..cfd682aec 100644 --- a/src/agent/channel_dispatch.rs +++ b/src/agent/channel_dispatch.rs @@ -352,6 +352,7 @@ pub async fn spawn_worker_from_state( None => Vec::new(), }; + let browser_config = (**rc.browser_config.load()).clone(); let worker_system_prompt = prompt_engine .render_worker_prompt( &rc.instance_dir.display().to_string(), @@ -361,10 +362,10 @@ pub async fn spawn_worker_from_state( sandbox_read_allowlist, sandbox_write_allowlist, &tool_secret_names, + browser_config.persist_session, ) .map_err(|e| AgentError::Other(anyhow::anyhow!("{e}")))?; let skills = rc.skills.load(); - let browser_config = (**rc.browser_config.load()).clone(); let brave_search_key = (**rc.brave_search_key.load()).clone(); // Append skills listing to worker system prompt. Suggested skills are @@ -887,6 +888,7 @@ pub async fn resume_idle_worker_into_state( Some(store) => store.tool_secret_names(), None => Vec::new(), }; + let browser_config = (**rc.browser_config.load()).clone(); let system_prompt = prompt_engine .render_worker_prompt( &rc.instance_dir.display().to_string(), @@ -896,9 +898,9 @@ pub async fn resume_idle_worker_into_state( sandbox_read_allowlist, sandbox_write_allowlist, &tool_secret_names, + browser_config.persist_session, ) .map_err(|error| format!("failed to render worker prompt: {error}"))?; - let browser_config = (**rc.browser_config.load()).clone(); let brave_search_key = (**rc.brave_search_key.load()).clone(); let (worker, input_tx) = Worker::resume_interactive( diff --git a/src/agent/cortex.rs b/src/agent/cortex.rs index 8534f3f33..49b23a68e 100644 --- a/src/agent/cortex.rs +++ b/src/agent/cortex.rs @@ -2398,6 +2398,7 @@ async fn pickup_one_ready_task(deps: &AgentDeps, logger: &CortexLogger) -> anyho None => Vec::new(), }; + let browser_persist_session = deps.runtime_config.browser_config.load().persist_session; let worker_system_prompt = prompt_engine .render_worker_prompt( &deps.runtime_config.instance_dir.display().to_string(), @@ -2407,6 +2408,7 @@ async fn pickup_one_ready_task(deps: &AgentDeps, logger: &CortexLogger) -> anyho sandbox_read_allowlist, sandbox_write_allowlist, &tool_secret_names, + browser_persist_session, ) .map_err(|error| anyhow::anyhow!("failed to render worker prompt: {error}"))?; diff --git a/src/config/load.rs b/src/config/load.rs index f78657ecb..014fc04c9 100644 --- a/src/config/load.rs +++ b/src/config/load.rs @@ -1424,7 +1424,15 @@ impl Config { .or_else(|| base.screenshot_dir.clone()), persist_session: b.persist_session.unwrap_or(base.persist_session), close_policy: parse_close_policy(b.close_policy.as_deref()) - .unwrap_or(base.close_policy), + .unwrap_or_else(|| { + // When persist_session is enabled and no explicit close_policy + // is set, default to Detach so tabs survive across workers. + if b.persist_session.unwrap_or(base.persist_session) { + ClosePolicy::Detach + } else { + base.close_policy + } + }), chrome_cache_dir: chrome_cache_dir.clone(), } }) @@ -1600,8 +1608,19 @@ impl Config { persist_session: b .persist_session .unwrap_or(defaults.browser.persist_session), - close_policy: parse_close_policy(b.close_policy.as_deref()) - .unwrap_or(defaults.browser.close_policy), + close_policy: parse_close_policy(b.close_policy.as_deref()).unwrap_or_else( + || { + // When persist_session is enabled and no explicit close_policy + // is set, default to Detach so tabs survive across workers. + if b.persist_session + .unwrap_or(defaults.browser.persist_session) + { + ClosePolicy::Detach + } else { + defaults.browser.close_policy + } + }, + ), chrome_cache_dir: defaults.browser.chrome_cache_dir.clone(), }), channel: a.channel.and_then(|channel_config| { diff --git a/src/prompts/engine.rs b/src/prompts/engine.rs index 80640271a..c43bf7fd1 100644 --- a/src/prompts/engine.rs +++ b/src/prompts/engine.rs @@ -253,6 +253,7 @@ impl PromptEngine { sandbox_read_allowlist: Vec, sandbox_write_allowlist: Vec, tool_secret_names: &[String], + browser_persist_session: bool, ) -> Result { self.render( "worker", @@ -264,6 +265,7 @@ impl PromptEngine { sandbox_read_allowlist => sandbox_read_allowlist, sandbox_write_allowlist => sandbox_write_allowlist, tool_secret_names => tool_secret_names, + browser_persist_session => browser_persist_session, }, ) } From 1caa405e85fd025ea1a5dca700ce6757320d5012 Mon Sep 17 00:00:00 2001 From: James Pine Date: Fri, 6 Mar 2026 19:39:18 -0800 Subject: [PATCH 2/5] fix: browser cookies lost across agent restarts with persist_session The user_data_dir was always a random UUID temp path, even with persist_session=true. On agent restart the old BrowserState dropped, deleting the temp dir (and Chrome's cookie database) via Drop, then the new launch created a fresh UUID dir with no cookies. Fix: when persist_session is true, use a stable path under chrome_cache_dir/profile instead of a random temp dir. Mark it as a persistent profile so Drop, CloseBrowser, and the concurrent- launch cleanup path all skip deleting it. --- src/tools/browser.rs | 79 ++++++++++++++++++++++++++++++-------------- 1 file changed, 54 insertions(+), 25 deletions(-) diff --git a/src/tools/browser.rs b/src/tools/browser.rs index 60fffd9a9..0a804c022 100644 --- a/src/tools/browser.rs +++ b/src/tools/browser.rs @@ -161,9 +161,14 @@ pub struct BrowserState { element_refs: HashMap, /// Counter for generating element refs. next_ref: usize, - /// Per-launch temp directory for Chrome's user data. Cleaned up on drop to - /// prevent stale singleton locks from blocking subsequent launches. + /// Chrome's user data directory. For ephemeral sessions this is a random + /// temp dir cleaned up on drop. For persistent sessions this is a stable + /// path under the instance dir that survives agent restarts. user_data_dir: Option, + /// When true, `user_data_dir` is a stable path that should NOT be deleted + /// on drop — it holds cookies, localStorage, and login sessions that must + /// survive across agent restarts. + persistent_profile: bool, } impl BrowserState { @@ -176,6 +181,7 @@ impl BrowserState { element_refs: HashMap::new(), next_ref: 0, user_data_dir: None, + persistent_profile: false, } } } @@ -184,6 +190,13 @@ impl Drop for BrowserState { fn drop(&mut self) { // Browser and handler task are dropped automatically — // chromiumoxide's Child has kill_on_drop(true). + + // Persistent profiles store cookies, localStorage, and login sessions + // that must survive across agent restarts — never delete them. + if self.persistent_profile { + return; + } + if let Some(dir) = self.user_data_dir.take() { // Offload sync fs cleanup to a blocking thread so we don't stall // the tokio worker that's dropping this state. @@ -212,6 +225,7 @@ impl std::fmt::Debug for BrowserState { .field("pages", &self.pages.len()) .field("active_target", &self.active_target) .field("element_refs", &self.element_refs.len()) + .field("persistent_profile", &self.persistent_profile) .finish() } } @@ -554,10 +568,16 @@ impl BrowserTool { // 4. Auto-download via BrowserFetcher (cached in chrome_cache_dir) let executable = resolve_chrome_executable(&self.config).await?; - // Use a unique temp dir per launch to avoid singleton lock collisions - // when multiple workers launch browsers or a previous session crashed. - let user_data_dir = - std::env::temp_dir().join(format!("spacebot-browser-{}", uuid::Uuid::new_v4())); + // Persistent sessions use a stable profile dir under chrome_cache_dir so + // cookies, localStorage, and login sessions survive across agent restarts. + // Ephemeral sessions use a random temp dir to avoid singleton lock collisions. + let (user_data_dir, persistent_profile) = if self.config.persist_session { + (self.config.chrome_cache_dir.join("profile"), true) + } else { + let dir = + std::env::temp_dir().join(format!("spacebot-browser-{}", uuid::Uuid::new_v4())); + (dir, false) + }; let mut builder = ChromeConfig::builder() .no_sandbox() @@ -594,12 +614,15 @@ impl BrowserTool { // the browser we just created and return success. drop(browser); handler_task.abort(); - // Clean up the temp user data dir asynchronously so we don't block - // while holding the shared mutex. - let dir = user_data_dir; - tokio::spawn(async move { - let _ = tokio::fs::remove_dir_all(&dir).await; - }); + // Clean up the temp user data dir — but only for ephemeral sessions. + // Persistent profiles use a shared stable path that the winner is + // actively using. + if !persistent_profile { + let dir = user_data_dir; + tokio::spawn(async move { + let _ = tokio::fs::remove_dir_all(&dir).await; + }); + } if self.config.persist_session { return self.reconnect_existing_tabs(&mut state).await; @@ -610,6 +633,7 @@ impl BrowserTool { state.browser = Some(browser); state._handler_task = Some(handler_task); state.user_data_dir = Some(user_data_dir); + state.persistent_profile = persistent_profile; tracing::info!("browser launched"); Ok(BrowserOutput::success("Browser launched successfully")) @@ -1196,16 +1220,17 @@ impl BrowserTool { ClosePolicy::CloseBrowser => { // Take everything out of state under the lock, then do the // actual teardown outside it. - let (browser, handler_task, user_data_dir) = { + let (browser, handler_task, user_data_dir, persistent_profile) = { let mut state = self.state.lock().await; let browser = state.browser.take(); let handler_task = state._handler_task.take(); let user_data_dir = state.user_data_dir.take(); + let persistent_profile = state.persistent_profile; state.pages.clear(); state.active_target = None; state.element_refs.clear(); state.next_ref = 0; - (browser, handler_task, user_data_dir) + (browser, handler_task, user_data_dir, persistent_profile) }; if let Some(task) = handler_task { @@ -1220,17 +1245,21 @@ impl BrowserTool { return Err(BrowserError::new(message)); } - // Clean up the per-launch user data dir to free disk space. - if let Some(dir) = user_data_dir { - tokio::spawn(async move { - if let Err(error) = tokio::fs::remove_dir_all(&dir).await { - tracing::debug!( - path = %dir.display(), - %error, - "failed to clean up browser user data dir" - ); - } - }); + // Clean up the user data dir — but only for ephemeral sessions. + // Persistent profiles hold cookies and login sessions that must + // survive browser restarts. + if !persistent_profile { + if let Some(dir) = user_data_dir { + tokio::spawn(async move { + if let Err(error) = tokio::fs::remove_dir_all(&dir).await { + tracing::debug!( + path = %dir.display(), + %error, + "failed to clean up browser user data dir" + ); + } + }); + } } tracing::info!(policy = "close_browser", "browser closed"); From 9128ee2d42aa942b3285b5b53241cc5e5183e28c Mon Sep 17 00:00:00 2001 From: James Pine Date: Fri, 6 Mar 2026 19:43:52 -0800 Subject: [PATCH 3/5] fix: add missing browser_persist_session arg in context_dump test --- tests/context_dump.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/context_dump.rs b/tests/context_dump.rs index 64e5b21fb..290ddf20e 100644 --- a/tests/context_dump.rs +++ b/tests/context_dump.rs @@ -356,6 +356,7 @@ async fn dump_worker_context() { let prompt_engine = rc.prompts.load(); let instance_dir = rc.instance_dir.to_string_lossy(); let workspace_dir = rc.workspace_dir.to_string_lossy(); + let browser_config = (**rc.browser_config.load()).clone(); let worker_prompt = prompt_engine .render_worker_prompt( &instance_dir, @@ -365,13 +366,11 @@ async fn dump_worker_context() { Vec::new(), Vec::new(), &[], + browser_config.persist_session, ) .expect("failed to render worker prompt"); print_section("WORKER SYSTEM PROMPT", &worker_prompt); print_stats("System prompt", &worker_prompt); - - // Build the actual worker tool server - let browser_config = (**rc.browser_config.load()).clone(); let brave_search_key = (**rc.brave_search_key.load()).clone(); let worker_id = uuid::Uuid::new_v4(); @@ -530,6 +529,7 @@ async fn dump_all_contexts() { println!("--- TOTAL BRANCH: ~{} tokens ---", branch_total / 4); // ── Worker ── + let browser_config = (**rc.browser_config.load()).clone(); let worker_prompt = prompt_engine .render_worker_prompt( &instance_dir, @@ -539,9 +539,9 @@ async fn dump_all_contexts() { Vec::new(), Vec::new(), &[], + browser_config.persist_session, ) .expect("failed to render worker prompt"); - let browser_config = (**rc.browser_config.load()).clone(); let brave_search_key = (**rc.brave_search_key.load()).clone(); let worker_tool_server = spacebot::tools::create_worker_tool_server( deps.agent_id.clone(), From 5e47b3c30633563a2a86a1ce1b1075aceeccaea4 Mon Sep 17 00:00:00 2001 From: James Pine Date: Fri, 6 Mar 2026 19:48:07 -0800 Subject: [PATCH 4/5] fix: collapse nested if to satisfy clippy collapsible_if --- src/tools/browser.rs | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/src/tools/browser.rs b/src/tools/browser.rs index 0a804c022..072984d22 100644 --- a/src/tools/browser.rs +++ b/src/tools/browser.rs @@ -1248,18 +1248,16 @@ impl BrowserTool { // Clean up the user data dir — but only for ephemeral sessions. // Persistent profiles hold cookies and login sessions that must // survive browser restarts. - if !persistent_profile { - if let Some(dir) = user_data_dir { - tokio::spawn(async move { - if let Err(error) = tokio::fs::remove_dir_all(&dir).await { - tracing::debug!( - path = %dir.display(), - %error, - "failed to clean up browser user data dir" - ); - } - }); - } + if !persistent_profile && let Some(dir) = user_data_dir { + tokio::spawn(async move { + if let Err(error) = tokio::fs::remove_dir_all(&dir).await { + tracing::debug!( + path = %dir.display(), + %error, + "failed to clean up browser user data dir" + ); + } + }); } tracing::info!(policy = "close_browser", "browser closed"); From ace5ddbff1e7d78af0709f267d334c149e519a17 Mon Sep 17 00:00:00 2001 From: James Pine Date: Fri, 6 Mar 2026 20:31:36 -0800 Subject: [PATCH 5/5] fix: address PR review feedback - Log errors instead of `let _ =` on race-path and Drop cleanup - Extract resolve_close_policy helper to deduplicate defaulting logic - Single browser_config load in cortex task pickup (no TOCTOU) --- src/agent/cortex.rs | 5 ++--- src/config/load.rs | 47 +++++++++++++++++++++++--------------------- src/tools/browser.rs | 15 ++++++++++++-- 3 files changed, 40 insertions(+), 27 deletions(-) diff --git a/src/agent/cortex.rs b/src/agent/cortex.rs index 49b23a68e..6d7342361 100644 --- a/src/agent/cortex.rs +++ b/src/agent/cortex.rs @@ -2398,7 +2398,7 @@ async fn pickup_one_ready_task(deps: &AgentDeps, logger: &CortexLogger) -> anyho None => Vec::new(), }; - let browser_persist_session = deps.runtime_config.browser_config.load().persist_session; + let browser_config = (**deps.runtime_config.browser_config.load()).clone(); let worker_system_prompt = prompt_engine .render_worker_prompt( &deps.runtime_config.instance_dir.display().to_string(), @@ -2408,7 +2408,7 @@ async fn pickup_one_ready_task(deps: &AgentDeps, logger: &CortexLogger) -> anyho sandbox_read_allowlist, sandbox_write_allowlist, &tool_secret_names, - browser_persist_session, + browser_config.persist_session, ) .map_err(|error| anyhow::anyhow!("failed to render worker prompt: {error}"))?; @@ -2442,7 +2442,6 @@ async fn pickup_one_ready_task(deps: &AgentDeps, logger: &CortexLogger) -> anyho tracing::warn!(%error, path = %logs_dir.display(), "failed to create logs directory"); } - let browser_config = (**deps.runtime_config.browser_config.load()).clone(); let brave_search_key = (**deps.runtime_config.brave_search_key.load()).clone(); let worker = Worker::new( None, diff --git a/src/config/load.rs b/src/config/load.rs index 014fc04c9..d919396ed 100644 --- a/src/config/load.rs +++ b/src/config/load.rs @@ -126,6 +126,21 @@ fn parse_close_policy(value: Option<&str>) -> Option { } } +/// Resolve the effective close policy. When `persist_session` is enabled and no +/// explicit `close_policy` was provided, default to `Detach` so browser tabs and +/// cookies survive across workers. +fn resolve_close_policy( + explicit: Option<&str>, + persist_session: bool, + fallback: ClosePolicy, +) -> ClosePolicy { + parse_close_policy(explicit).unwrap_or(if persist_session { + ClosePolicy::Detach + } else { + fallback + }) +} + impl CortexConfig { fn resolve(overrides: TomlCortexConfig, defaults: CortexConfig) -> CortexConfig { CortexConfig { @@ -1423,16 +1438,11 @@ impl Config { .map(PathBuf::from) .or_else(|| base.screenshot_dir.clone()), persist_session: b.persist_session.unwrap_or(base.persist_session), - close_policy: parse_close_policy(b.close_policy.as_deref()) - .unwrap_or_else(|| { - // When persist_session is enabled and no explicit close_policy - // is set, default to Detach so tabs survive across workers. - if b.persist_session.unwrap_or(base.persist_session) { - ClosePolicy::Detach - } else { - base.close_policy - } - }), + close_policy: resolve_close_policy( + b.close_policy.as_deref(), + b.persist_session.unwrap_or(base.persist_session), + base.close_policy, + ), chrome_cache_dir: chrome_cache_dir.clone(), } }) @@ -1608,18 +1618,11 @@ impl Config { persist_session: b .persist_session .unwrap_or(defaults.browser.persist_session), - close_policy: parse_close_policy(b.close_policy.as_deref()).unwrap_or_else( - || { - // When persist_session is enabled and no explicit close_policy - // is set, default to Detach so tabs survive across workers. - if b.persist_session - .unwrap_or(defaults.browser.persist_session) - { - ClosePolicy::Detach - } else { - defaults.browser.close_policy - } - }, + close_policy: resolve_close_policy( + b.close_policy.as_deref(), + b.persist_session + .unwrap_or(defaults.browser.persist_session), + defaults.browser.close_policy, ), chrome_cache_dir: defaults.browser.chrome_cache_dir.clone(), }), diff --git a/src/tools/browser.rs b/src/tools/browser.rs index 072984d22..90186b262 100644 --- a/src/tools/browser.rs +++ b/src/tools/browser.rs @@ -212,7 +212,12 @@ impl Drop for BrowserState { }); } else { // Dropped outside a tokio runtime (unlikely) — clean up inline. - let _ = std::fs::remove_dir_all(&dir); + if let Err(error) = std::fs::remove_dir_all(&dir) { + eprintln!( + "failed to clean up browser user data dir {}: {error}", + dir.display() + ); + } } } } @@ -620,7 +625,13 @@ impl BrowserTool { if !persistent_profile { let dir = user_data_dir; tokio::spawn(async move { - let _ = tokio::fs::remove_dir_all(&dir).await; + if let Err(error) = tokio::fs::remove_dir_all(&dir).await { + tracing::debug!( + path = %dir.display(), + %error, + "failed to clean up browser user data dir (concurrent launch race)" + ); + } }); }