diff --git a/changelog.d/20260525_000000_browser_limits_cleanup.md b/changelog.d/20260525_000000_browser_limits_cleanup.md new file mode 100644 index 0000000..a4961e5 --- /dev/null +++ b/changelog.d/20260525_000000_browser_limits_cleanup.md @@ -0,0 +1,6 @@ +--- +bump: patch +--- + +### Fixed +- Enforce docker-git's browser CPU/RAM limits in the Rust browser container startup and add a Rust stop command for shutdown cleanup. diff --git a/src/browser.rs b/src/browser.rs index 1270d06..7b45aa8 100644 --- a/src/browser.rs +++ b/src/browser.rs @@ -21,7 +21,7 @@ use std::path::{Path, PathBuf}; use std::process::{Command, Stdio}; use std::time::{Duration, SystemTime, UNIX_EPOCH}; -use crate::BrowserSpec; +use crate::{BrowserResourceLimits, BrowserSpec}; pub(crate) struct BrowserRuntime { pub container_name: String, @@ -29,6 +29,11 @@ pub(crate) struct BrowserRuntime { pub cdp_url: String, } +pub(crate) struct BrowserStopRuntime { + pub container_name: String, + pub removed: bool, +} + const BROWSER_DOCKERFILE: &str = r#"FROM kechangdev/browser-vnc:latest # bash/procps keep upstream startup scripts compatible; socat exposes a stable CDP port. @@ -89,7 +94,11 @@ impl DockerBrowserShell { Self } - pub fn ensure_browser_container(&self, spec: &BrowserSpec) -> Result { + pub fn ensure_browser_container( + &self, + spec: &BrowserSpec, + limits: &BrowserResourceLimits, + ) -> Result { ensure_docker_available()?; ensure_browser_image(spec)?; @@ -119,7 +128,7 @@ impl DockerBrowserShell { ); ensure_volume(&runtime_spec.volume_name)?; - run_browser_container(&runtime_spec)?; + run_browser_container(&runtime_spec, limits)?; let (cdp_url, novnc_url) = runtime_urls(&runtime_spec)?; Ok(BrowserRuntime { container_name: runtime_spec.container_name, @@ -127,6 +136,29 @@ impl DockerBrowserShell { cdp_url, }) } + + pub fn stop_browser_container(&self, spec: &BrowserSpec) -> Result { + ensure_docker_available()?; + let removed = match inspect_container_state(&spec.container_name)? { + Some(_) => { + match docker(["rm", "-f", &spec.container_name], "docker rm browser") { + Ok(_) => {} + Err(error) => { + if inspect_container_state(&spec.container_name)?.is_some() { + return Err(error); + } + } + } + true + } + None => false, + }; + + Ok(BrowserStopRuntime { + container_name: spec.container_name.clone(), + removed, + }) + } } fn docker(args: [&str; N], label: &str) -> Result { @@ -346,7 +378,21 @@ fn ensure_volume(volume_name: &str) -> Result<()> { docker(["volume", "create", volume_name], "docker volume create").map(|_| ()) } -fn run_browser_container(spec: &BrowserSpec) -> Result<()> { +fn browser_resource_limit_args(limits: &BrowserResourceLimits) -> Vec { + let mut args = Vec::new(); + + if let Some(cpu_limit) = &limits.cpu_limit { + args.extend(["--cpus".to_string(), cpu_limit.clone()]); + } + + if let Some(ram_limit) = &limits.ram_limit { + args.extend(["--memory".to_string(), ram_limit.clone()]); + } + + args +} + +fn run_browser_container(spec: &BrowserSpec, limits: &BrowserResourceLimits) -> Result<()> { let mut args = vec![ "run".to_string(), "-d".to_string(), @@ -362,6 +408,8 @@ fn run_browser_container(spec: &BrowserSpec) -> Result<()> { "2g".to_string(), ]; + args.extend(browser_resource_limit_args(limits)); + if should_publish_ports(&spec.network_mode) { args.extend([ "-p".to_string(), @@ -639,6 +687,27 @@ mod tests { assert_eq!(effective_network_mode("bridge", None), "bridge"); } + #[test] + fn browser_resource_limit_args_are_omitted_when_limits_are_empty() { + assert!(browser_resource_limit_args(&BrowserResourceLimits::none()).is_empty()); + } + + #[test] + fn browser_resource_limit_args_render_docker_run_limits() { + assert_eq!( + browser_resource_limit_args(&BrowserResourceLimits::from_values( + Some("0.5"), + Some("1g") + )), + vec![ + "--cpus".to_string(), + "0.5".to_string(), + "--memory".to_string(), + "1g".to_string() + ] + ); + } + #[test] fn docker_host_autodetect_keeps_explicit_env_and_project_env_precedence() { assert_eq!( diff --git a/src/lib.rs b/src/lib.rs index fb3c1a2..cabde1e 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -70,6 +70,32 @@ pub struct BrowserSpec { pub ports: BrowserPorts, } +#[derive(Debug, Clone, Default, PartialEq, Eq, Serialize)] +pub struct BrowserResourceLimits { + pub cpu_limit: Option, + pub ram_limit: Option, +} + +impl BrowserResourceLimits { + pub fn none() -> Self { + Self::default() + } + + pub fn from_values(cpu_limit: Option<&str>, ram_limit: Option<&str>) -> Self { + Self { + cpu_limit: normalize_optional_limit(cpu_limit), + ram_limit: normalize_optional_limit(ram_limit), + } + } + + pub fn with_cli_overrides(&self, cpu_limit: Option<&str>, ram_limit: Option<&str>) -> Self { + Self { + cpu_limit: normalize_optional_limit(cpu_limit).or_else(|| self.cpu_limit.clone()), + ram_limit: normalize_optional_limit(ram_limit).or_else(|| self.ram_limit.clone()), + } + } +} + #[derive(Debug, Clone, Serialize)] pub struct BrowserInfo { pub project_id: String, @@ -78,6 +104,13 @@ pub struct BrowserInfo { pub cdp_url: String, } +#[derive(Debug, Clone, Serialize)] +pub struct BrowserStopInfo { + pub project_id: String, + pub container_name: String, + pub removed: bool, +} + fn resolved_or(resolve: &F, name: &str, fallback: String) -> String where F: Fn(&str) -> Option, @@ -88,6 +121,13 @@ where .unwrap_or(fallback) } +fn normalize_optional_limit(value: Option<&str>) -> Option { + value + .map(str::trim) + .filter(|value| !value.is_empty()) + .map(str::to_string) +} + fn env_value(name: &str) -> Option { env::var(name).ok() } @@ -190,6 +230,29 @@ pub fn browser_spec_from_env(project_id: &str, network: Option<&str>) -> Browser browser_spec_from_resolver(project_id, network, env_value) } +// CHANGE: read browser container resource ceilings from docker-git's generated environment. +// WHY: docker-git emits DOCKER_GIT_BROWSER_*_LIMIT for the Rust-owned browser container. +// QUOTE(ТЗ): "When users set --playwright-cpu/--playwright-ram or rely on the defaults" +// REF: issue-347-review +// SOURCE: n/a +// FORMAT THEOREM: env.limit != empty -> docker_run_args contains the corresponding Docker limit flag. +// PURITY: CORE (except environment boundary read isolated in browser_resource_limits_from_env) +// INVARIANT: empty env values do not create malformed Docker flags. +// COMPLEXITY: O(n)/O(n), n = total limit string length. +fn browser_resource_limits_from_resolver(resolve: F) -> BrowserResourceLimits +where + F: Fn(&str) -> Option, +{ + BrowserResourceLimits::from_values( + resolve("DOCKER_GIT_BROWSER_CPU_LIMIT").as_deref(), + resolve("DOCKER_GIT_BROWSER_RAM_LIMIT").as_deref(), + ) +} + +pub fn browser_resource_limits_from_env() -> BrowserResourceLimits { + browser_resource_limits_from_resolver(env_value) +} + pub fn render_novnc_url() -> String { format!( "http://127.0.0.1:{}/vnc.html?autoconnect=true&resize=remote&path=websockify", @@ -228,8 +291,18 @@ impl BrowserConnection { } pub fn start_browser(&self, project_id: &str, network: Option<&str>) -> Result { + let limits = browser_resource_limits_from_env(); + self.start_browser_with_limits(project_id, network, &limits) + } + + pub fn start_browser_with_limits( + &self, + project_id: &str, + network: Option<&str>, + limits: &BrowserResourceLimits, + ) -> Result { let spec = browser_spec_from_env(project_id, network); - let runtime = self.shell.ensure_browser_container(&spec)?; + let runtime = self.shell.ensure_browser_container(&spec, limits)?; Ok(BrowserInfo { project_id: spec.project_id, @@ -239,6 +312,17 @@ impl BrowserConnection { }) } + pub fn stop_browser(&self, project_id: &str) -> Result { + let spec = browser_spec_from_env(project_id, None); + let runtime = self.shell.stop_browser_container(&spec)?; + + Ok(BrowserStopInfo { + project_id: spec.project_id, + container_name: runtime.container_name, + removed: runtime.removed, + }) + } + pub fn get_novnc_url(&self, project_id: &str) -> String { render_novnc_url_for_ports(compute_browser_ports(project_id)) } @@ -282,4 +366,25 @@ mod tests { assert_eq!(spec.network_mode, "container:dg-docker-git-issue-347"); assert_eq!(spec.ports, compute_browser_ports("dg-docker-git-issue-347")); } + + #[test] + fn browser_resource_limits_from_resolver_trims_empty_values() { + let limits = browser_resource_limits_from_resolver(|name| match name { + "DOCKER_GIT_BROWSER_CPU_LIMIT" => Some(" 0.5 ".to_string()), + "DOCKER_GIT_BROWSER_RAM_LIMIT" => Some(" ".to_string()), + _ => None, + }); + + assert_eq!(limits.cpu_limit, Some("0.5".to_string())); + assert_eq!(limits.ram_limit, None); + } + + #[test] + fn browser_resource_limits_cli_overrides_env_values() { + let env_limits = BrowserResourceLimits::from_values(Some("1.5"), Some("2g")); + let limits = env_limits.with_cli_overrides(Some("0.5"), None); + + assert_eq!(limits.cpu_limit, Some("0.5".to_string())); + assert_eq!(limits.ram_limit, Some("2g".to_string())); + } } diff --git a/src/main.rs b/src/main.rs index 776c521..7ca4769 100644 --- a/src/main.rs +++ b/src/main.rs @@ -7,7 +7,9 @@ //! to guarantee a single unified browser (noVNC + CDP). use clap::{Parser, Subcommand}; -use docker_git_browser_connection::{BrowserConnection, BrowserInfo}; +use docker_git_browser_connection::{ + browser_resource_limits_from_env, BrowserConnection, BrowserInfo, +}; #[derive(Parser)] #[command( @@ -29,6 +31,17 @@ enum Commands { /// Docker network mode for the browser container. Defaults to container:. #[arg(long)] network: Option, + /// Docker --cpus limit for the browser container. Defaults to DOCKER_GIT_BROWSER_CPU_LIMIT. + #[arg(long)] + cpu_limit: Option, + /// Docker --memory limit for the browser container. Defaults to DOCKER_GIT_BROWSER_RAM_LIMIT. + #[arg(long)] + ram_limit: Option, + }, + /// Stop and remove the single browser container for a project + Stop { + #[arg(long)] + project: String, }, /// Show status / URLs for the project's browser Status { @@ -44,15 +57,31 @@ fn main() -> anyhow::Result<()> { let conn = BrowserConnection::new()?; match cli.command { - Commands::Start { project, network } => { + Commands::Start { + project, + network, + cpu_limit, + ram_limit, + } => { let net_ref = network.as_deref(); - let info: BrowserInfo = conn.start_browser(&project, net_ref)?; + let limits = browser_resource_limits_from_env() + .with_cli_overrides(cpu_limit.as_deref(), ram_limit.as_deref()); + let info: BrowserInfo = conn.start_browser_with_limits(&project, net_ref, &limits)?; println!("Browser started for project: {}", info.project_id); println!("Container: {}", info.container_name); println!("noVNC: {}", info.novnc_url); println!("CDP (for MCP Playwright / Hermes): {}", info.cdp_url); println!("Use the CDP URL in your MCP Playwright config to get automatic noVNC."); } + Commands::Stop { project } => { + let info = conn.stop_browser(&project)?; + if info.removed { + println!("Browser stopped for project: {}", info.project_id); + } else { + println!("Browser already absent for project: {}", info.project_id); + } + println!("Container: {}", info.container_name); + } Commands::Status { project } => { let novnc = conn.get_novnc_url(&project); let cdp = conn.get_cdp_url(&project); diff --git a/tests/cli_status.rs b/tests/cli_status.rs index 7dd58f4..2cf6232 100644 --- a/tests/cli_status.rs +++ b/tests/cli_status.rs @@ -18,3 +18,28 @@ fn status_command_prints_single_browser_urls_without_docker() { assert!(stdout.contains(&format!("CDP: {}", render_cdp_url_for_ports(ports)))); assert!(stdout.contains("Invariant check: true")); } + +#[test] +fn start_help_exposes_resource_limit_flags_without_docker() { + let output = Command::new(env!("CARGO_BIN_EXE_docker-git-browser-connection")) + .args(["start", "--help"]) + .output() + .expect("Failed to execute binary"); + + assert!(output.status.success()); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!(stdout.contains("--cpu-limit")); + assert!(stdout.contains("--ram-limit")); +} + +#[test] +fn root_help_exposes_stop_command_without_docker() { + let output = Command::new(env!("CARGO_BIN_EXE_docker-git-browser-connection")) + .args(["--help"]) + .output() + .expect("Failed to execute binary"); + + assert!(output.status.success()); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!(stdout.contains("stop")); +}