feat: persistent browser sessions with configurable close policy#309
Merged
feat: persistent browser sessions with configurable close policy#309
Conversation
Add persist_session and close_policy to BrowserConfig, allowing browser instances to survive across worker lifetimes. When persist_session is enabled, workers share a single browser handle via Arc<Mutex<BrowserState>> and reconnect to existing tabs on launch instead of starting fresh. ClosePolicy controls what happens when a worker finishes: - close_browser (default): full teardown, same as before - close_tabs: close all pages but keep browser process alive - detach: leave browser and tabs running for the next worker Backend: SharedBrowserHandle type, BrowserTool::new_shared() constructor, reconnect_existing_tabs() discovery, close policy dispatch in handle_close(), RuntimeConfig.shared_browser field, API types and TOML support. Frontend: persist session toggle and close policy dropdown in AgentConfig. Docs: config.mdx reference table and browser.mdx persistent sessions guide.
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR adds two new browser configuration options— Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/api/config.rs (1)
200-206:⚠️ Potential issue | 🟠 MajorValidate
close_policybefore writing toconfig.toml.At Line 205 and Lines 684-685,
close_policyis a rawStringand is persisted without validation. Invalid values can be stored and break later reload/startup behavior.Suggested fix (typed input + strict serialization)
+#[derive(Deserialize, Debug, Clone, Copy)] +#[serde(rename_all = "snake_case")] +pub(super) enum BrowserClosePolicyUpdate { + CloseBrowser, + CloseTabs, + Detach, +} + #[derive(Deserialize, Debug)] pub(super) struct BrowserUpdate { enabled: Option<bool>, headless: Option<bool>, evaluate_enabled: Option<bool>, persist_session: Option<bool>, - close_policy: Option<String>, + close_policy: Option<BrowserClosePolicyUpdate>, } @@ - if let Some(ref v) = browser.close_policy { - table["close_policy"] = toml_edit::value(v.as_str()); + if let Some(value) = browser.close_policy { + let close_policy = match value { + BrowserClosePolicyUpdate::CloseBrowser => "close_browser", + BrowserClosePolicyUpdate::CloseTabs => "close_tabs", + BrowserClosePolicyUpdate::Detach => "detach", + }; + table["close_policy"] = toml_edit::value(close_policy); }As per coding guidelines: "Don't silently discard errors; use
let _ =only on channel sends where the receiver may be dropped; handle, log, or propagate all other errors".Also applies to: 681-686
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/config.rs` around lines 200 - 206, The BrowserUpdate.close_policy field is stored as a raw String and written to config.toml without validation, allowing invalid values to be persisted and later break startup; change BrowserUpdate.close_policy to use a typed enum (e.g., ClosePolicy) with serde strict deserialization/serialization (deny unknown variants) and validate/convert any incoming raw strings before writing, returning/logging an error on invalid values instead of persisting them, and ensure any write errors are handled (not ignored) when saving the config so failures are propagated or logged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/content/docs/`(features)/browser.mdx:
- Around line 166-173: The docs describe shared browser state via
persist_session = true, SharedBrowserHandle, RuntimeConfig and close_policy =
"detach" but later still claims "single browser per worker"; update the later
limitation text to be conditional: when persist_session = false maintain the
"single browser per worker" wording, and otherwise describe the shared-browser
behavior (workers reconnect via launch to the same process, discover tabs and
share state). Locate references to persist_session, SharedBrowserHandle,
RuntimeConfig, launch and close_policy and revise the sentence(s) to mention
both modes (persist_session = true vs false) so the limitation only applies when
persist_session is false.
In `@src/tools/browser.rs`:
- Around line 631-644: The reconnection logic currently only inserts missing
entries into state.pages and leaves stale entries and possibly a stale
state.active_target; change the handler that processes the incoming pages list
(the loop that calls page_target_id(&page) and manipulates state.pages) to
replace the tab map instead of only inserting missing keys: clear or build a new
HashMap from the provided pages (using page_target_id as the key) and assign it
to state.pages so stale entries are removed, and after rebuilding ensure
state.active_target is set to Some(first_existing_id) only if the existing
active_target is missing from the new map (i.e., if
state.active_target.as_ref().map(|id|
!state.pages.contains_key(id)).unwrap_or(true) then set it to the first key);
apply the same replacement logic for the other reconnection block that mirrors
this behavior so active_target never points at a dead page.
- Around line 1145-1147: handle_close is currently swallowing failures from
page.close(), context.close(), and browser.close() by only logging them and
still returning success; change handle_close to propagate these shutdown errors
as a structured Result instead. Locate the calls to page.close().await,
context.close().await, and browser.close().await inside handle_close and replace
the debug-only handling with propagation (use ? or map_err to convert the
underlying error into the function's error type, e.g.,
BrowserError/ToolError/anyhow::Error) so the function returns Err(...) when any
close call fails; retain logging if desired but do not return Ok(()) when a
close failed, and aggregate or wrap multiple errors if more than one close can
fail before returning. Ensure the function signature reflects a Result return
type and update callers to handle the propagated error.
---
Outside diff comments:
In `@src/api/config.rs`:
- Around line 200-206: The BrowserUpdate.close_policy field is stored as a raw
String and written to config.toml without validation, allowing invalid values to
be persisted and later break startup; change BrowserUpdate.close_policy to use a
typed enum (e.g., ClosePolicy) with serde strict deserialization/serialization
(deny unknown variants) and validate/convert any incoming raw strings before
writing, returning/logging an error on invalid values instead of persisting
them, and ensure any write errors are handled (not ignored) when saving the
config so failures are propagated or logged.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a3cc5e6b-3cd9-4567-97d8-5d29b34ac07d
📒 Files selected for processing (13)
docs/content/docs/(configuration)/config.mdxdocs/content/docs/(features)/browser.mdxinterface/src/api/client.tsinterface/src/routes/AgentConfig.tsxsrc/api/agents.rssrc/api/config.rssrc/config/load.rssrc/config/runtime.rssrc/config/toml_schema.rssrc/config/types.rssrc/main.rssrc/tools.rssrc/tools/browser.rs
- P1: Use typed ClosePolicy enum in BrowserUpdate API layer instead of raw String, so invalid values get a 400 error on deserialization rather than being silently persisted to config.toml. - P2: Rebuild tab map in reconnect_existing_tabs from browser.pages() instead of append-only, pruning stale entries. Validate active_target points to a live page after refresh. - P2: Reduce mutex hold time in handle_close — CloseTabs drains pages under the lock then closes them outside it; CloseBrowser takes state out under the lock then tears down outside it. - P3: Preserve active_target on Detach so next worker picks up exactly where the previous one left off (element_refs still cleared). - P3: Gate shared browser mode on both persist_session config flag AND shared_browser handle presence, not just handle existence. - P3: Fix doc inconsistency — 'single browser per worker' limitation now conditional on persist_session mode. Tighten ClosePolicy doc comment to only mention explicit close action. - P4: Use async tokio::fs::remove_dir_all in launch race path and CloseBrowser teardown to avoid blocking while holding the mutex. - Propagate close errors as BrowserError instead of swallowing them, so the LLM sees failures and can recover.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
persist_sessionandclose_policyconfig options to[browser], allowing browser instances to survive across worker lifetimes with tabs, cookies, and login sessions intact.SharedBrowserHandle(Arc<Mutex<BrowserState>>) held inRuntimeConfigwhenpersist_session = true, reconnecting to existing tabs onlaunchinstead of starting a fresh process.ClosePolicycontrols what happens on close:close_browser(default, full teardown),close_tabs(close pages, keep Chrome), ordetach(disconnect, leave everything running).Details
Backend (Rust)
New types:
ClosePolicyenum with serde support,as_str(),Display(src/config/types.rs)SharedBrowserHandletype alias (Arc<Mutex<BrowserState>>) andnew_shared_browser_handle()factory (src/tools/browser.rs)Config pipeline:
TomlBrowserConfiggainspersist_session/close_policyfields (src/config/toml_schema.rs)parse_close_policy()helper; both defaults-resolution and per-agent override paths wired (src/config/load.rs)RuntimeConfig.shared_browserconditionally created at construction; hot-reload logs a warning that restart is required (src/config/runtime.rs)Browser tool:
BrowserState::new()constructor,BrowserTool::new_shared()for shared-handle modereconnect_existing_tabs()discovers open tabs viabrowser.pages(), populatesstate.pages, returns tab info to the LLMhandle_launch()reconnects instead of erroring when persistent browser is already runninghandle_close()dispatches onClosePolicy:Detachclears element refs only,CloseTabscloses pages but keeps browser alive,CloseBrowserdoes full teardownWiring:
create_worker_tool_serverandcreate_cortex_chat_tool_serveruseBrowserTool::new_shared()when shared handle is available (src/tools.rs,src/main.rs,src/api/agents.rs)BrowserSection/BrowserUpdateinclude the new fields; TOML writer updated (src/api/config.rs)Frontend
interface/src/routes/AgentConfig.tsx)interface/src/api/client.ts)Docs
config.mdx— browser config example and reference table updatedbrowser.mdx— new "Persistent Sessions" section, close policy table, updated architecture diagramConfig example
Gate results
cargo fmt— cleancargo check— compilescargo clippy— no warningscargo test --lib— 321/321 passedcargo test --tests --no-run— integration tests compilebun run build(frontend) — builds successfullyNotes
persist_sessionrequires an agent restart (logged as warning on hot-reload).connect_url/ external browser via CDP) is a natural complement — when it lands,connect_url+persist_session+close_policy = "detach"gives the full external persistent browser experience.