fix: relaunch browser when launch options change (#996)
* fix: relaunch browser when launch options change (#993) When the daemon already held a running browser, handle_launch only checked connection type and liveness to decide reuse. Config changes like adding extensions to config.json were silently ignored. Store a hash of the relaunch-relevant LaunchOptions fields and compare on each launch command. If the hash differs the browser is closed and relaunched with the new options. * fmt * fix * fix * fmt --------- Co-authored-by: hyunjinee <leehj0110@kakao.com> Co-authored-by: ctate <366502+ctate@users.noreply.github.com>
This commit is contained in:
+127
-97
@@ -166,6 +166,30 @@ struct DrainedEvents {
|
|||||||
detached_iframe_sessions: Vec<String>,
|
detached_iframe_sessions: Vec<String>,
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Compute a hash of the [`LaunchOptions`] fields that require a browser
|
||||||
|
/// relaunch when changed (baked into the Chrome process at startup).
|
||||||
|
///
|
||||||
|
/// Fields NOT hashed (adjustable at runtime via CDP without relaunch):
|
||||||
|
/// ignore_https_errors, color_scheme, download_path, storage_state
|
||||||
|
fn launch_hash(opts: &LaunchOptions) -> u64 {
|
||||||
|
use std::collections::hash_map::DefaultHasher;
|
||||||
|
use std::hash::{Hash, Hasher};
|
||||||
|
|
||||||
|
let mut h = DefaultHasher::new();
|
||||||
|
opts.headless.hash(&mut h);
|
||||||
|
opts.extensions.hash(&mut h);
|
||||||
|
opts.profile.hash(&mut h);
|
||||||
|
opts.executable_path.hash(&mut h);
|
||||||
|
opts.args.hash(&mut h);
|
||||||
|
opts.proxy.hash(&mut h);
|
||||||
|
opts.proxy_bypass.hash(&mut h);
|
||||||
|
opts.proxy_username.hash(&mut h);
|
||||||
|
opts.proxy_password.hash(&mut h);
|
||||||
|
opts.user_agent.hash(&mut h);
|
||||||
|
opts.allow_file_access.hash(&mut h);
|
||||||
|
h.finish()
|
||||||
|
}
|
||||||
|
|
||||||
pub struct DaemonState {
|
pub struct DaemonState {
|
||||||
pub browser: Option<BrowserManager>,
|
pub browser: Option<BrowserManager>,
|
||||||
pub appium: Option<AppiumManager>,
|
pub appium: Option<AppiumManager>,
|
||||||
@@ -218,6 +242,8 @@ pub struct DaemonState {
|
|||||||
pub stream_client: Option<Arc<RwLock<Option<Arc<CdpClient>>>>>,
|
pub stream_client: Option<Arc<RwLock<Option<Arc<CdpClient>>>>>,
|
||||||
/// Stream server instance kept alive so the broadcast channel remains open.
|
/// Stream server instance kept alive so the broadcast channel remains open.
|
||||||
pub stream_server: Option<Arc<StreamServer>>,
|
pub stream_server: Option<Arc<StreamServer>>,
|
||||||
|
/// Hash of launch options used for the current browser, for relaunch detection.
|
||||||
|
launch_hash: Option<u64>,
|
||||||
/// Browser engine name (e.g. "chrome", "lightpanda") for observability.
|
/// Browser engine name (e.g. "chrome", "lightpanda") for observability.
|
||||||
pub engine: String,
|
pub engine: String,
|
||||||
}
|
}
|
||||||
@@ -267,6 +293,7 @@ impl DaemonState {
|
|||||||
),
|
),
|
||||||
stream_client: None,
|
stream_client: None,
|
||||||
stream_server: None,
|
stream_server: None,
|
||||||
|
launch_hash: None,
|
||||||
engine: env::var("AGENT_BROWSER_ENGINE").unwrap_or_else(|_| "chrome".to_string()),
|
engine: env::var("AGENT_BROWSER_ENGINE").unwrap_or_else(|_| "chrome".to_string()),
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -1523,9 +1550,12 @@ async fn auto_launch(state: &mut DaemonState) -> Result<(), String> {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
let hash = launch_hash(&options);
|
||||||
let mgr = BrowserManager::launch(options, engine.as_deref()).await?;
|
let mgr = BrowserManager::launch(options, engine.as_deref()).await?;
|
||||||
state.reset_input_state();
|
state.reset_input_state();
|
||||||
state.browser = Some(mgr);
|
state.browser = Some(mgr);
|
||||||
|
state.launch_hash = Some(hash);
|
||||||
state.subscribe_to_browser_events();
|
state.subscribe_to_browser_events();
|
||||||
state.start_fetch_handler();
|
state.start_fetch_handler();
|
||||||
state.start_dialog_handler();
|
state.start_dialog_handler();
|
||||||
@@ -1616,12 +1646,95 @@ async fn handle_launch(cmd: &Value, state: &mut DaemonState) -> Result<Value, St
|
|||||||
.and_then(|v| v.as_bool())
|
.and_then(|v| v.as_bool())
|
||||||
.unwrap_or(false);
|
.unwrap_or(false);
|
||||||
|
|
||||||
// Relaunch logic: check if we can reuse the existing connection.
|
let extensions: Option<Vec<String>> =
|
||||||
// Fast process-exit check first to avoid expensive CDP timeout.
|
cmd.get("extensions").and_then(|v| v.as_array()).map(|arr| {
|
||||||
|
arr.iter()
|
||||||
|
.filter_map(|v| v.as_str().map(String::from))
|
||||||
|
.collect()
|
||||||
|
});
|
||||||
|
let storage_state = cmd.get("storageState").and_then(|v| v.as_str());
|
||||||
|
|
||||||
|
let launch_options = LaunchOptions {
|
||||||
|
headless,
|
||||||
|
executable_path: cmd
|
||||||
|
.get("executablePath")
|
||||||
|
.and_then(|v| v.as_str())
|
||||||
|
.map(|s| s.to_string())
|
||||||
|
.or_else(|| env::var("AGENT_BROWSER_EXECUTABLE_PATH").ok()),
|
||||||
|
proxy: cmd.get("proxy").and_then(|v| {
|
||||||
|
v.as_str().map(|s| s.to_string()).or_else(|| {
|
||||||
|
v.get("server")
|
||||||
|
.and_then(|s| s.as_str())
|
||||||
|
.map(|s| s.to_string())
|
||||||
|
})
|
||||||
|
}),
|
||||||
|
proxy_bypass: cmd
|
||||||
|
.get("proxy")
|
||||||
|
.and_then(|v| v.get("bypass"))
|
||||||
|
.and_then(|v| v.as_str())
|
||||||
|
.map(String::from),
|
||||||
|
proxy_username: cmd
|
||||||
|
.get("proxy")
|
||||||
|
.and_then(|v| v.get("username"))
|
||||||
|
.and_then(|v| v.as_str())
|
||||||
|
.map(String::from)
|
||||||
|
.or_else(|| env::var("AGENT_BROWSER_PROXY_USERNAME").ok()),
|
||||||
|
proxy_password: cmd
|
||||||
|
.get("proxy")
|
||||||
|
.and_then(|v| v.get("password"))
|
||||||
|
.and_then(|v| v.as_str())
|
||||||
|
.map(String::from)
|
||||||
|
.or_else(|| env::var("AGENT_BROWSER_PROXY_PASSWORD").ok()),
|
||||||
|
profile: cmd
|
||||||
|
.get("profile")
|
||||||
|
.and_then(|v| v.as_str())
|
||||||
|
.map(|s| s.to_string()),
|
||||||
|
allow_file_access: cmd
|
||||||
|
.get("allowFileAccess")
|
||||||
|
.and_then(|v| v.as_bool())
|
||||||
|
.unwrap_or(false),
|
||||||
|
args: cmd
|
||||||
|
.get("args")
|
||||||
|
.and_then(|v| v.as_array())
|
||||||
|
.map(|arr| {
|
||||||
|
arr.iter()
|
||||||
|
.filter_map(|v| v.as_str().map(|s| s.to_string()))
|
||||||
|
.collect()
|
||||||
|
})
|
||||||
|
.unwrap_or_default(),
|
||||||
|
extensions,
|
||||||
|
storage_state: storage_state.map(String::from),
|
||||||
|
user_agent: cmd
|
||||||
|
.get("userAgent")
|
||||||
|
.and_then(|v| v.as_str())
|
||||||
|
.map(String::from),
|
||||||
|
ignore_https_errors: cmd
|
||||||
|
.get("ignoreHTTPSErrors")
|
||||||
|
.and_then(|v| v.as_bool())
|
||||||
|
.unwrap_or(false),
|
||||||
|
color_scheme: cmd
|
||||||
|
.get("colorScheme")
|
||||||
|
.and_then(|v| v.as_str())
|
||||||
|
.map(String::from),
|
||||||
|
download_path: cmd
|
||||||
|
.get("downloadPath")
|
||||||
|
.and_then(|v| v.as_str())
|
||||||
|
.map(String::from),
|
||||||
|
};
|
||||||
|
|
||||||
|
let new_hash = launch_hash(&launch_options);
|
||||||
|
|
||||||
|
// Hash comparison and fast process-exit check are evaluated before the
|
||||||
|
// async is_connection_alive to skip the expensive CDP liveness probe
|
||||||
|
// when a relaunch is already certain.
|
||||||
let needs_relaunch = if let Some(ref mut mgr) = state.browser {
|
let needs_relaunch = if let Some(ref mut mgr) = state.browser {
|
||||||
let is_external = cdp_url.is_some() || cdp_port.is_some() || auto_connect;
|
let is_external = cdp_url.is_some() || cdp_port.is_some() || auto_connect;
|
||||||
let was_external = mgr.is_cdp_connection();
|
let was_external = mgr.is_cdp_connection();
|
||||||
is_external != was_external || mgr.has_process_exited() || !mgr.is_connection_alive().await
|
let hash_changed = !is_external && state.launch_hash != Some(new_hash);
|
||||||
|
is_external != was_external
|
||||||
|
|| hash_changed
|
||||||
|
|| mgr.has_process_exited()
|
||||||
|
|| !mgr.is_connection_alive().await
|
||||||
} else {
|
} else {
|
||||||
true
|
true
|
||||||
};
|
};
|
||||||
@@ -1630,6 +1743,7 @@ async fn handle_launch(cmd: &Value, state: &mut DaemonState) -> Result<Value, St
|
|||||||
if let Some(ref mut b) = state.browser {
|
if let Some(ref mut b) = state.browser {
|
||||||
b.close().await?;
|
b.close().await?;
|
||||||
state.browser = None;
|
state.browser = None;
|
||||||
|
state.launch_hash = None;
|
||||||
state.screencasting = false;
|
state.screencasting = false;
|
||||||
state.reset_input_state();
|
state.reset_input_state();
|
||||||
state.update_stream_client().await;
|
state.update_stream_client().await;
|
||||||
@@ -1638,33 +1752,15 @@ async fn handle_launch(cmd: &Value, state: &mut DaemonState) -> Result<Value, St
|
|||||||
return Ok(json!({ "launched": true, "reused": true }));
|
return Ok(json!({ "launched": true, "reused": true }));
|
||||||
}
|
}
|
||||||
state.ref_map.clear();
|
state.ref_map.clear();
|
||||||
let extensions: Option<Vec<String>> =
|
|
||||||
cmd.get("extensions").and_then(|v| v.as_array()).map(|arr| {
|
|
||||||
arr.iter()
|
|
||||||
.filter_map(|v| v.as_str().map(String::from))
|
|
||||||
.collect()
|
|
||||||
});
|
|
||||||
|
|
||||||
let profile = cmd.get("profile").and_then(|v| v.as_str());
|
|
||||||
let storage_state = cmd.get("storageState").and_then(|v| v.as_str());
|
|
||||||
let allow_file_access = cmd
|
|
||||||
.get("allowFileAccess")
|
|
||||||
.and_then(|v| v.as_bool())
|
|
||||||
.unwrap_or(false);
|
|
||||||
let executable_path: Option<String> = cmd
|
|
||||||
.get("executablePath")
|
|
||||||
.and_then(|v| v.as_str())
|
|
||||||
.map(String::from)
|
|
||||||
.or_else(|| std::env::var("AGENT_BROWSER_EXECUTABLE_PATH").ok());
|
|
||||||
|
|
||||||
let has_cdp = cdp_url.is_some() || cdp_port.is_some();
|
let has_cdp = cdp_url.is_some() || cdp_port.is_some();
|
||||||
super::browser::validate_launch_options(
|
super::browser::validate_launch_options(
|
||||||
extensions.as_deref(),
|
launch_options.extensions.as_deref(),
|
||||||
has_cdp,
|
has_cdp,
|
||||||
profile,
|
launch_options.profile.as_deref(),
|
||||||
storage_state,
|
storage_state,
|
||||||
allow_file_access,
|
launch_options.allow_file_access,
|
||||||
executable_path.as_deref(),
|
launch_options.executable_path.as_deref(),
|
||||||
)?;
|
)?;
|
||||||
|
|
||||||
if let Some(url) = cdp_url {
|
if let Some(url) = cdp_url {
|
||||||
@@ -1759,81 +1855,13 @@ async fn handle_launch(cmd: &Value, state: &mut DaemonState) -> Result<Value, St
|
|||||||
.map(String::from)
|
.map(String::from)
|
||||||
.or_else(|| env::var("AGENT_BROWSER_ENGINE").ok());
|
.or_else(|| env::var("AGENT_BROWSER_ENGINE").ok());
|
||||||
|
|
||||||
let options = LaunchOptions {
|
|
||||||
headless,
|
|
||||||
executable_path: cmd
|
|
||||||
.get("executablePath")
|
|
||||||
.and_then(|v| v.as_str())
|
|
||||||
.map(|s| s.to_string())
|
|
||||||
.or_else(|| env::var("AGENT_BROWSER_EXECUTABLE_PATH").ok()),
|
|
||||||
proxy: cmd.get("proxy").and_then(|v| {
|
|
||||||
v.as_str().map(|s| s.to_string()).or_else(|| {
|
|
||||||
v.get("server")
|
|
||||||
.and_then(|s| s.as_str())
|
|
||||||
.map(|s| s.to_string())
|
|
||||||
})
|
|
||||||
}),
|
|
||||||
profile: cmd
|
|
||||||
.get("profile")
|
|
||||||
.and_then(|v| v.as_str())
|
|
||||||
.map(|s| s.to_string()),
|
|
||||||
allow_file_access: cmd
|
|
||||||
.get("allowFileAccess")
|
|
||||||
.and_then(|v| v.as_bool())
|
|
||||||
.unwrap_or(false),
|
|
||||||
args: cmd
|
|
||||||
.get("args")
|
|
||||||
.and_then(|v| v.as_array())
|
|
||||||
.map(|arr| {
|
|
||||||
arr.iter()
|
|
||||||
.filter_map(|v| v.as_str().map(|s| s.to_string()))
|
|
||||||
.collect()
|
|
||||||
})
|
|
||||||
.unwrap_or_default(),
|
|
||||||
extensions,
|
|
||||||
storage_state: storage_state.map(String::from),
|
|
||||||
proxy_bypass: cmd
|
|
||||||
.get("proxy")
|
|
||||||
.and_then(|v| v.get("bypass"))
|
|
||||||
.and_then(|v| v.as_str())
|
|
||||||
.map(String::from),
|
|
||||||
proxy_username: cmd
|
|
||||||
.get("proxy")
|
|
||||||
.and_then(|v| v.get("username"))
|
|
||||||
.and_then(|v| v.as_str())
|
|
||||||
.map(String::from)
|
|
||||||
.or_else(|| env::var("AGENT_BROWSER_PROXY_USERNAME").ok()),
|
|
||||||
proxy_password: cmd
|
|
||||||
.get("proxy")
|
|
||||||
.and_then(|v| v.get("password"))
|
|
||||||
.and_then(|v| v.as_str())
|
|
||||||
.map(String::from)
|
|
||||||
.or_else(|| env::var("AGENT_BROWSER_PROXY_PASSWORD").ok()),
|
|
||||||
user_agent: cmd
|
|
||||||
.get("userAgent")
|
|
||||||
.and_then(|v| v.as_str())
|
|
||||||
.map(String::from),
|
|
||||||
ignore_https_errors: cmd
|
|
||||||
.get("ignoreHTTPSErrors")
|
|
||||||
.and_then(|v| v.as_bool())
|
|
||||||
.unwrap_or(false),
|
|
||||||
color_scheme: cmd
|
|
||||||
.get("colorScheme")
|
|
||||||
.and_then(|v| v.as_str())
|
|
||||||
.map(String::from),
|
|
||||||
download_path: cmd
|
|
||||||
.get("downloadPath")
|
|
||||||
.and_then(|v| v.as_str())
|
|
||||||
.map(String::from),
|
|
||||||
};
|
|
||||||
|
|
||||||
// Store proxy credentials for Fetch.authRequired handling
|
// Store proxy credentials for Fetch.authRequired handling
|
||||||
let has_proxy_auth = options.proxy_username.is_some();
|
let has_proxy_auth = launch_options.proxy_username.is_some();
|
||||||
if has_proxy_auth {
|
if has_proxy_auth {
|
||||||
let mut creds = state.proxy_credentials.write().await;
|
let mut creds = state.proxy_credentials.write().await;
|
||||||
*creds = Some((
|
*creds = Some((
|
||||||
options.proxy_username.clone().unwrap_or_default(),
|
launch_options.proxy_username.clone().unwrap_or_default(),
|
||||||
options.proxy_password.clone().unwrap_or_default(),
|
launch_options.proxy_password.clone().unwrap_or_default(),
|
||||||
));
|
));
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -1850,7 +1878,8 @@ async fn handle_launch(cmd: &Value, state: &mut DaemonState) -> Result<Value, St
|
|||||||
write_engine_file(&state.session_id, &state.engine);
|
write_engine_file(&state.session_id, &state.engine);
|
||||||
write_extensions_file(&state.session_id);
|
write_extensions_file(&state.session_id);
|
||||||
state.reset_input_state();
|
state.reset_input_state();
|
||||||
state.browser = Some(BrowserManager::launch(options, engine.as_deref()).await?);
|
state.browser = Some(BrowserManager::launch(launch_options, engine.as_deref()).await?);
|
||||||
|
state.launch_hash = Some(new_hash);
|
||||||
state.subscribe_to_browser_events();
|
state.subscribe_to_browser_events();
|
||||||
state.start_fetch_handler();
|
state.start_fetch_handler();
|
||||||
state.start_dialog_handler();
|
state.start_dialog_handler();
|
||||||
@@ -2188,6 +2217,7 @@ async fn handle_close(state: &mut DaemonState) -> Result<Value, String> {
|
|||||||
mgr.close().await?;
|
mgr.close().await?;
|
||||||
}
|
}
|
||||||
state.browser = None;
|
state.browser = None;
|
||||||
|
state.launch_hash = None;
|
||||||
state.screencasting = false;
|
state.screencasting = false;
|
||||||
state.reset_input_state();
|
state.reset_input_state();
|
||||||
state.update_stream_client().await;
|
state.update_stream_client().await;
|
||||||
|
|||||||
@@ -3822,3 +3822,65 @@ async fn e2e_externally_opened_tab_detected() {
|
|||||||
let resp = execute_command(&json!({ "id": "99", "action": "close" }), &mut state).await;
|
let resp = execute_command(&json!({ "id": "99", "action": "close" }), &mut state).await;
|
||||||
assert_success(&resp);
|
assert_success(&resp);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// ---------------------------------------------------------------------------
|
||||||
|
// Regression: issue #993 — launch options change must trigger relaunch
|
||||||
|
// ---------------------------------------------------------------------------
|
||||||
|
|
||||||
|
/// When the browser is already running and a second launch command arrives with
|
||||||
|
/// different options (e.g., extensions added), the daemon must relaunch the
|
||||||
|
/// browser instead of silently reusing the old one.
|
||||||
|
///
|
||||||
|
/// Before the fix, `handle_launch` only checked connection type and liveness,
|
||||||
|
/// so changed options like extensions were ignored and the old browser was reused.
|
||||||
|
#[tokio::test]
|
||||||
|
#[ignore]
|
||||||
|
async fn e2e_relaunch_on_options_change() {
|
||||||
|
let mut state = DaemonState::new();
|
||||||
|
|
||||||
|
// First launch — headless, no extensions.
|
||||||
|
let resp = execute_command(
|
||||||
|
&json!({ "id": "1", "action": "launch", "headless": true }),
|
||||||
|
&mut state,
|
||||||
|
)
|
||||||
|
.await;
|
||||||
|
assert_success(&resp);
|
||||||
|
assert_eq!(get_data(&resp)["launched"], true);
|
||||||
|
assert!(
|
||||||
|
get_data(&resp).get("reused").is_none(),
|
||||||
|
"first launch must not be a reuse"
|
||||||
|
);
|
||||||
|
|
||||||
|
// Second launch — same options → should reuse.
|
||||||
|
let resp = execute_command(
|
||||||
|
&json!({ "id": "2", "action": "launch", "headless": true }),
|
||||||
|
&mut state,
|
||||||
|
)
|
||||||
|
.await;
|
||||||
|
assert_success(&resp);
|
||||||
|
assert_eq!(
|
||||||
|
get_data(&resp)["reused"],
|
||||||
|
true,
|
||||||
|
"identical options must reuse the browser"
|
||||||
|
);
|
||||||
|
|
||||||
|
// Third launch — different options (extensions added) → must relaunch, not reuse.
|
||||||
|
let resp = execute_command(
|
||||||
|
&json!({
|
||||||
|
"id": "3",
|
||||||
|
"action": "launch",
|
||||||
|
"headless": false,
|
||||||
|
"extensions": ["/tmp/fake-extension"]
|
||||||
|
}),
|
||||||
|
&mut state,
|
||||||
|
)
|
||||||
|
.await;
|
||||||
|
assert_success(&resp);
|
||||||
|
assert!(
|
||||||
|
get_data(&resp).get("reused").is_none(),
|
||||||
|
"changed options must trigger a relaunch, not reuse (issue #993)"
|
||||||
|
);
|
||||||
|
|
||||||
|
let resp = execute_command(&json!({ "id": "99", "action": "close" }), &mut state).await;
|
||||||
|
assert_success(&resp);
|
||||||
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user