fix(browser): enforce limits and add cleanup

[skulidropek]

Summary

• enforce DOCKER_GIT_BROWSER_CPU_LIMIT and DOCKER_GIT_BROWSER_RAM_LIMIT in Rust-owned browser container startup
• add start CLI overrides for browser CPU/RAM limits
• add a Rust stop command so docker-git generated entrypoints can clean up dg-*-browser containers on shutdown

Validation

• cargo fmt --check
• cargo check --locked --bins
• cargo test --locked
• cargo clippy --locked --all-targets -- -D warnings

[coderabbitai]

📝 Walkthrough

Summary by CodeRabbit

• New Features

  • Browser start now accepts --cpu-limit and --ram-limit (env defaults supported) and enforces those limits.
  • Added a stop command that reliably stops and cleans up the browser container and reports removal status.

• Tests

  • Added CLI integration tests verifying the new resource-limit options and stop command help output.

Walkthrough

This PR adds configurable CPU and RAM resource limits for browser container startup and introduces a stop command for container cleanup. Resource limits are loaded from environment variables, accept CLI overrides, and are applied as Docker flags during container startup. A new stop operation removes containers and reports removal status.

Changes

Browser container resource limits and lifecycle

Layer / File(s)	Summary
Resource limits data model and environment loading src/lib.rs	Introduces BrowserResourceLimits struct with optional CPU/RAM fields, normalization helpers to trim/coerce empty strings to None, and public constructors for building limits from environment variables or explicit values with CLI override support. Adds BrowserStopInfo return type. Includes unit tests for limit normalization and override precedence.
Docker resource argument generation src/browser.rs	Introduces browser_resource_limit_args() helper to convert limits into Docker --cpus and --memory flags (empty when no limits specified). Updates run_browser_container to accept and append limit flags to the Docker run command. Tests verify correct flag rendering and omission for empty limits.
Container start and stop operations src/browser.rs, src/lib.rs	Updates ensure_browser_container signature to accept limits and pass them to run_browser_container. Adds start_browser_with_limits to load environment limits with CLI overrides and delegate to the lower layer. Implements stop_browser_container to remove containers and return removal status; exposes stop_browser on BrowserConnection to derive container spec and map results into BrowserStopInfo.
CLI command setup and integration tests src/main.rs, tests/cli_status.rs, changelog.d/...	Imports browser_resource_limits_from_env and adds optional --cpu-limit and --ram-limit flags to Start command. Updates Start handler to compute limits from environment with CLI overrides and call start_browser_with_limits. Adds help output tests to verify resource-limit flags appear in start help and stop command appears in root help. Includes changelog entry documenting the resource limit enforcement and stop command.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• ProverCoderAI/rust-browser-connection#2: The resource-limit and stop changes directly extend the same browser-container orchestration code—particularly DockerBrowserShell::ensure_browser_container and run_browser_container in src/browser.rs and the BrowserConnection start flow in src/lib.rs.
• ProverCoderAI/rust-browser-connection#7: Both PRs modify DockerBrowserShell::ensure_browser_container signature and behavior around container startup, so the main PR's resource-limit plumbing intersects with #7's container/network/URL runtime provisioning.

Poem

🐰 I tuned the rails for CPU and RAM,
Docker sings light, no longer a sham,
Start with limits, stop with care,
Containers vanish, tidy and fair,
A rabbit's patch—efficient and calm.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 36.36% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the two main changes: enforcing resource limits and adding cleanup functionality via a stop command.
Description check	✅ Passed	The description is directly related to the changeset, clearly explaining the three key objectives and validation steps performed.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/browser-limits-cleanup-347

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/browser.rs`:
- Around line 140-148: The stop_browser_container function checks
inspect_container_state(&spec.container_name) then calls docker(["rm", "-f",
&spec.container_name], ...) which can race if another process removes the
container; make stop idempotent by treating "container not found" as success:
wrap the docker rm call in error-handling that converts a not-found /
already-removed error into Ok(true) instead of propagating an Err, keeping other
errors failing normally. Update the logic inside stop_browser_container
(referencing inspect_container_state, docker, BrowserSpec::container_name and
the Result<BrowserStopRuntime> return) so if docker rm returns an error
indicating the container no longer exists the function returns as if removed.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 1fb84152-87b0-4c0a-9872-83411bb2629a

📥 Commits

Reviewing files that changed from the base of the PR and between 3745470 and b69fa09.

📒 Files selected for processing (5)

• changelog.d/20260525_000000_browser_limits_cleanup.md
• src/browser.rs
• src/lib.rs
• src/main.rs
• tests/cli_status.rs

[skulidropek]

AI Session Backup

Commit: acd76d1
Status: success
Files: 19 (4.31 MB)
Links: README | Manifest

git status

On branch fix/browser-limits-cleanup-347
Your branch is up to date with 'origin/fix/browser-limits-cleanup-347'.

nothing to commit, working tree clean

[coderabbitai]

♻️ Duplicate comments (1)

src/browser.rs (1)

144-151: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Don’t suppress docker rm failures via ambiguous inspect fallback.

On Line 147, inspect_container_state() returns Ok(None) for any failed inspect, not only “container missing”. If docker rm fails (e.g., daemon/permission issue) and inspect also fails, this branch reports success instead of surfacing the error.

Suggested fix

-                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
+                match docker(["rm", "-f", &spec.container_name], "docker rm browser") {
+                    Ok(_) => true,
+                    Err(error) => {
+                        let message = error.to_string();
+                        if message.contains("No such container")
+                            || message.contains("No such object")
+                        {
+                            false
+                        } else {
+                            return Err(error);
+                        }
+                    }
+                }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/browser.rs` around lines 144 - 151, The current error-handling around
docker(["rm", ...]) suppresses real failures by treating any non-Ok inspect as
non-existent; change the logic in the docker rm error branch to only swallow the
original error when inspect_container_state(&spec.container_name) returns
Ok(Some(_)) (meaning the container still exists) or, better, to explicitly
handle the "container not found" case by matching Err variants from inspect and
propagating the docker rm error for other failures; update the match using the
inspect_container_state function and the spec.container_name identifier so that
any inspect errors (Ok(None) or Err(_)) do not convert a docker rm failure into
success and instead return Err(error).

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Duplicate comments:
In `@src/browser.rs`:
- Around line 144-151: The current error-handling around docker(["rm", ...])
suppresses real failures by treating any non-Ok inspect as non-existent; change
the logic in the docker rm error branch to only swallow the original error when
inspect_container_state(&spec.container_name) returns Ok(Some(_)) (meaning the
container still exists) or, better, to explicitly handle the "container not
found" case by matching Err variants from inspect and propagating the docker rm
error for other failures; update the match using the inspect_container_state
function and the spec.container_name identifier so that any inspect errors
(Ok(None) or Err(_)) do not convert a docker rm failure into success and instead
return Err(error).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: e2030b8a-4fad-41b4-a2fb-448a199a954a

📥 Commits

Reviewing files that changed from the base of the PR and between b69fa09 and acd76d1.

📒 Files selected for processing (1)

• src/browser.rs