mirror of
https://github.com/tiennm99/goclaw.git
synced 2026-09-05 12:16:58 +00:00
feat(browser): add timeout, idle auto-close, and max-pages safety mechanisms
Prevent resource leaks and hanging actions in the browser tool: - Per-action context timeout (default 30s, configurable via timeoutMs param or config) - Idle page reaper goroutine closes pages unused for 10min (configurable) - Max pages per tenant (default 5) with LRU eviction - RefStore cleanup on page close/evict/reap to prevent memory leaks
This commit is contained in:
@@ -6,6 +6,7 @@ import (
|
||||
"log/slog"
|
||||
"os"
|
||||
"path/filepath"
|
||||
"time"
|
||||
|
||||
"github.com/google/uuid"
|
||||
|
||||
@@ -96,6 +97,18 @@ func setupToolRegistry(
|
||||
opts = append(opts, browser.WithHeadless(cfg.Tools.Browser.Headless))
|
||||
slog.Info("browser tool enabled", "headless", cfg.Tools.Browser.Headless)
|
||||
}
|
||||
if cfg.Tools.Browser.ActionTimeoutMs > 0 {
|
||||
opts = append(opts, browser.WithActionTimeout(time.Duration(cfg.Tools.Browser.ActionTimeoutMs)*time.Millisecond))
|
||||
}
|
||||
if cfg.Tools.Browser.IdleTimeoutMs > 0 {
|
||||
opts = append(opts, browser.WithIdleTimeout(time.Duration(cfg.Tools.Browser.IdleTimeoutMs)*time.Millisecond))
|
||||
} else if cfg.Tools.Browser.IdleTimeoutMs < 0 {
|
||||
// Explicitly disable idle reaper with negative value
|
||||
opts = append(opts, browser.WithIdleTimeout(0))
|
||||
}
|
||||
if cfg.Tools.Browser.MaxPages > 0 {
|
||||
opts = append(opts, browser.WithMaxPages(cfg.Tools.Browser.MaxPages))
|
||||
}
|
||||
browserMgr = browser.New(opts...)
|
||||
toolsReg.Register(browser.NewBrowserTool(browserMgr))
|
||||
}
|
||||
|
||||
@@ -394,9 +394,12 @@ type WebFetchPolicyConfig struct {
|
||||
|
||||
// BrowserToolConfig controls the browser automation tool.
|
||||
type BrowserToolConfig struct {
|
||||
Enabled bool `json:"enabled"` // enable the browser tool (default false)
|
||||
Headless bool `json:"headless,omitempty"` // run Chrome in headless mode (ignored when RemoteURL is set)
|
||||
RemoteURL string `json:"remote_url,omitempty"` // CDP endpoint for remote Chrome sidecar, e.g. "ws://chrome:9222"
|
||||
Enabled bool `json:"enabled"` // enable the browser tool (default false)
|
||||
Headless bool `json:"headless,omitempty"` // run Chrome in headless mode (ignored when RemoteURL is set)
|
||||
RemoteURL string `json:"remote_url,omitempty"` // CDP endpoint for remote Chrome sidecar, e.g. "ws://chrome:9222"
|
||||
ActionTimeoutMs int `json:"action_timeout_ms,omitempty"` // per-action timeout in ms (default 30000)
|
||||
IdleTimeoutMs int `json:"idle_timeout_ms,omitempty"` // idle page auto-close in ms (default 600000, 0=disabled)
|
||||
MaxPages int `json:"max_pages,omitempty"` // max open pages per tenant (default 5)
|
||||
}
|
||||
|
||||
// ToolPolicySpec defines a tool policy at any level (global, per-agent, per-provider).
|
||||
|
||||
+62
-9
@@ -5,6 +5,7 @@ import (
|
||||
"fmt"
|
||||
"log/slog"
|
||||
"sync"
|
||||
"time"
|
||||
|
||||
"github.com/go-rod/rod"
|
||||
"github.com/go-rod/rod/lib/launcher"
|
||||
@@ -19,9 +20,14 @@ type Manager struct {
|
||||
console map[string][]ConsoleMessage // targetID → console messages
|
||||
tenantCtxs map[string]*rod.Browser // tenantID → incognito browser context
|
||||
pageTenants map[string]string // targetID → tenantID (for filtering)
|
||||
headless bool
|
||||
remoteURL string // CDP endpoint for remote Chrome (sidecar); skips local launcher
|
||||
logger *slog.Logger
|
||||
pageLastUsed map[string]time.Time // targetID → last access time
|
||||
headless bool
|
||||
remoteURL string // CDP endpoint for remote Chrome (sidecar); skips local launcher
|
||||
actionTimeout time.Duration // per-action context timeout (default 30s)
|
||||
idleTimeout time.Duration // auto-close pages idle longer than this (default 10m, 0=disabled)
|
||||
maxPages int // max open pages per tenant (default 5)
|
||||
stopReaper chan struct{} // signal to stop the reaper goroutine
|
||||
logger *slog.Logger
|
||||
}
|
||||
|
||||
// Option configures a Manager.
|
||||
@@ -43,15 +49,34 @@ func WithLogger(l *slog.Logger) Option {
|
||||
return func(m *Manager) { m.logger = l }
|
||||
}
|
||||
|
||||
// WithActionTimeout sets the per-action context timeout.
|
||||
func WithActionTimeout(d time.Duration) Option {
|
||||
return func(m *Manager) { m.actionTimeout = d }
|
||||
}
|
||||
|
||||
// WithIdleTimeout sets the idle page auto-close timeout. 0 disables the reaper.
|
||||
func WithIdleTimeout(d time.Duration) Option {
|
||||
return func(m *Manager) { m.idleTimeout = d }
|
||||
}
|
||||
|
||||
// WithMaxPages sets the max open pages per tenant.
|
||||
func WithMaxPages(n int) Option {
|
||||
return func(m *Manager) { m.maxPages = n }
|
||||
}
|
||||
|
||||
// New creates a Manager with options.
|
||||
func New(opts ...Option) *Manager {
|
||||
m := &Manager{
|
||||
refs: NewRefStore(),
|
||||
pages: make(map[string]*rod.Page),
|
||||
console: make(map[string][]ConsoleMessage),
|
||||
tenantCtxs: make(map[string]*rod.Browser),
|
||||
pageTenants: make(map[string]string),
|
||||
logger: slog.Default(),
|
||||
refs: NewRefStore(),
|
||||
pages: make(map[string]*rod.Page),
|
||||
console: make(map[string][]ConsoleMessage),
|
||||
tenantCtxs: make(map[string]*rod.Browser),
|
||||
pageTenants: make(map[string]string),
|
||||
pageLastUsed: make(map[string]time.Time),
|
||||
actionTimeout: 30 * time.Second,
|
||||
idleTimeout: 10 * time.Minute,
|
||||
maxPages: 5,
|
||||
logger: slog.Default(),
|
||||
}
|
||||
for _, o := range opts {
|
||||
o(m)
|
||||
@@ -59,6 +84,16 @@ func New(opts ...Option) *Manager {
|
||||
return m
|
||||
}
|
||||
|
||||
// ActionTimeout returns the configured per-action timeout.
|
||||
func (m *Manager) ActionTimeout() time.Duration {
|
||||
return m.actionTimeout
|
||||
}
|
||||
|
||||
// touchPageLocked updates the last-used timestamp for a page. Must be called with mu held.
|
||||
func (m *Manager) touchPageLocked(targetID string) {
|
||||
m.pageLastUsed[targetID] = time.Now()
|
||||
}
|
||||
|
||||
// Start launches a local Chrome browser or connects to a remote one.
|
||||
// If already connected but the connection is dead, it reconnects automatically.
|
||||
func (m *Manager) Start(ctx context.Context) error {
|
||||
@@ -112,11 +147,28 @@ func (m *Manager) Start(ctx context.Context) error {
|
||||
}
|
||||
|
||||
m.browser = b
|
||||
|
||||
// Start idle-page reaper if configured
|
||||
if m.idleTimeout > 0 && m.stopReaper == nil {
|
||||
m.stopReaper = make(chan struct{})
|
||||
go m.runReaper()
|
||||
}
|
||||
|
||||
return nil
|
||||
}
|
||||
|
||||
// Stop closes the Chrome browser (local) or disconnects (remote sidecar).
|
||||
func (m *Manager) Stop(ctx context.Context) error {
|
||||
// Grab and nil-out stopReaper under the lock, then close outside to avoid
|
||||
// deadlock (reaper goroutine also acquires mu).
|
||||
m.mu.Lock()
|
||||
ch := m.stopReaper
|
||||
m.stopReaper = nil
|
||||
m.mu.Unlock()
|
||||
if ch != nil {
|
||||
close(ch)
|
||||
}
|
||||
|
||||
m.mu.Lock()
|
||||
defer m.mu.Unlock()
|
||||
|
||||
@@ -137,6 +189,7 @@ func (m *Manager) Stop(ctx context.Context) error {
|
||||
m.pages = make(map[string]*rod.Page)
|
||||
m.console = make(map[string][]ConsoleMessage)
|
||||
m.pageTenants = make(map[string]string)
|
||||
m.pageLastUsed = make(map[string]time.Time)
|
||||
return err
|
||||
}
|
||||
|
||||
|
||||
@@ -0,0 +1,54 @@
|
||||
package browser
|
||||
|
||||
import "time"
|
||||
|
||||
// runReaper periodically closes pages that have been idle longer than idleTimeout.
|
||||
// Runs as a goroutine; exits when stopReaper is closed.
|
||||
func (m *Manager) runReaper() {
|
||||
ticker := time.NewTicker(60 * time.Second)
|
||||
defer ticker.Stop()
|
||||
|
||||
for {
|
||||
select {
|
||||
case <-m.stopReaper:
|
||||
return
|
||||
case <-ticker.C:
|
||||
m.reapIdlePages()
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// reapIdlePages closes pages idle longer than idleTimeout.
|
||||
func (m *Manager) reapIdlePages() {
|
||||
m.mu.Lock()
|
||||
defer m.mu.Unlock()
|
||||
|
||||
if m.browser == nil {
|
||||
return
|
||||
}
|
||||
|
||||
now := time.Now()
|
||||
for targetID, lastUsed := range m.pageLastUsed {
|
||||
if now.Sub(lastUsed) <= m.idleTimeout {
|
||||
continue
|
||||
}
|
||||
|
||||
page, ok := m.pages[targetID]
|
||||
if !ok {
|
||||
delete(m.pageLastUsed, targetID)
|
||||
continue
|
||||
}
|
||||
|
||||
if err := page.Close(); err != nil {
|
||||
m.logger.Warn("reaper: failed to close idle page", "targetId", targetID, "error", err)
|
||||
continue
|
||||
}
|
||||
|
||||
delete(m.pages, targetID)
|
||||
delete(m.console, targetID)
|
||||
delete(m.pageTenants, targetID)
|
||||
delete(m.pageLastUsed, targetID)
|
||||
m.refs.Remove(targetID)
|
||||
m.logger.Info("reaper: closed idle page", "targetId", targetID, "idle", now.Sub(lastUsed).Round(time.Second))
|
||||
}
|
||||
}
|
||||
@@ -22,6 +22,7 @@ func (m *Manager) reconnectLocked() error {
|
||||
m.pages = make(map[string]*rod.Page)
|
||||
m.console = make(map[string][]ConsoleMessage)
|
||||
m.pageTenants = make(map[string]string)
|
||||
m.pageLastUsed = make(map[string]time.Time)
|
||||
m.refs = NewRefStore()
|
||||
|
||||
controlURL, err := resolveRemoteCDP(m.remoteURL)
|
||||
@@ -100,6 +101,11 @@ func (m *Manager) getPageForTenant(targetID, tenantID string) (*rod.Page, error)
|
||||
}
|
||||
// If no tenant context or master tenant, allow access to all pages
|
||||
if tenantID == "" || tenantID == MasterTenantID {
|
||||
resolvedTID := targetID
|
||||
if targetID == "" {
|
||||
resolvedTID = string(page.TargetID)
|
||||
}
|
||||
m.touchPageLocked(resolvedTID)
|
||||
return page, nil
|
||||
}
|
||||
// Check ownership: page must belong to this tenant
|
||||
@@ -110,6 +116,7 @@ func (m *Manager) getPageForTenant(targetID, tenantID string) (*rod.Page, error)
|
||||
if owner, ok := m.pageTenants[resolvedTID]; ok && owner != tenantID {
|
||||
return nil, fmt.Errorf("tab not found: %s", targetID)
|
||||
}
|
||||
m.touchPageLocked(resolvedTID)
|
||||
return page, nil
|
||||
}
|
||||
|
||||
|
||||
@@ -68,11 +68,18 @@ func (m *Manager) ListTabs(ctx context.Context) ([]TabInfo, error) {
|
||||
|
||||
// OpenTab opens a new tab with the given URL.
|
||||
// Pages are created within the tenant's incognito browser context for isolation.
|
||||
// If the tenant already has maxPages open, the oldest idle page is closed first.
|
||||
func (m *Manager) OpenTab(ctx context.Context, url string) (*TabInfo, error) {
|
||||
m.mu.Lock()
|
||||
defer m.mu.Unlock()
|
||||
|
||||
tenantID := tenantIDFromCtx(ctx)
|
||||
|
||||
// Enforce max pages per tenant
|
||||
if m.maxPages > 0 {
|
||||
m.evictOldestIfOverLimitLocked(tenantID)
|
||||
}
|
||||
|
||||
b, err := m.tenantBrowserLocked(tenantID)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
@@ -89,6 +96,7 @@ func (m *Manager) OpenTab(ctx context.Context, url string) (*TabInfo, error) {
|
||||
info, _ := page.Info()
|
||||
tid := string(page.TargetID)
|
||||
m.pages[tid] = page
|
||||
m.touchPageLocked(tid)
|
||||
if tenantID != "" {
|
||||
m.pageTenants[tid] = tenantID
|
||||
}
|
||||
@@ -104,6 +112,60 @@ func (m *Manager) OpenTab(ctx context.Context, url string) (*TabInfo, error) {
|
||||
return tab, nil
|
||||
}
|
||||
|
||||
// evictOldestIfOverLimitLocked closes the oldest idle page for a tenant if at or over maxPages.
|
||||
// Must be called with mu held.
|
||||
func (m *Manager) evictOldestIfOverLimitLocked(tenantID string) {
|
||||
isMaster := tenantID == "" || tenantID == MasterTenantID
|
||||
|
||||
// Collect targetIDs belonging to this tenant
|
||||
var owned []string
|
||||
for tid := range m.pages {
|
||||
if isMaster {
|
||||
// Master tenant owns pages not in pageTenants
|
||||
if _, hasOwner := m.pageTenants[tid]; !hasOwner {
|
||||
owned = append(owned, tid)
|
||||
}
|
||||
} else {
|
||||
if m.pageTenants[tid] == tenantID {
|
||||
owned = append(owned, tid)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
if len(owned) < m.maxPages {
|
||||
return
|
||||
}
|
||||
|
||||
// Find the oldest page by lastUsed
|
||||
var oldestID string
|
||||
var oldestTime time.Time
|
||||
for _, tid := range owned {
|
||||
lu, ok := m.pageLastUsed[tid]
|
||||
if !ok {
|
||||
oldestID = tid
|
||||
break
|
||||
}
|
||||
if oldestID == "" || lu.Before(oldestTime) {
|
||||
oldestID = tid
|
||||
oldestTime = lu
|
||||
}
|
||||
}
|
||||
|
||||
if oldestID == "" {
|
||||
return
|
||||
}
|
||||
|
||||
if page, ok := m.pages[oldestID]; ok {
|
||||
_ = page.Close()
|
||||
}
|
||||
delete(m.pages, oldestID)
|
||||
delete(m.console, oldestID)
|
||||
delete(m.pageTenants, oldestID)
|
||||
delete(m.pageLastUsed, oldestID)
|
||||
m.refs.Remove(oldestID)
|
||||
m.logger.Info("evicted oldest page (max pages reached)", "targetId", oldestID, "tenant", tenantID)
|
||||
}
|
||||
|
||||
// FocusTab activates a tab.
|
||||
func (m *Manager) FocusTab(ctx context.Context, targetID string) error {
|
||||
tenantID := tenantIDFromCtx(ctx)
|
||||
@@ -133,6 +195,8 @@ func (m *Manager) CloseTab(ctx context.Context, targetID string) error {
|
||||
delete(m.pages, targetID)
|
||||
delete(m.console, targetID)
|
||||
delete(m.pageTenants, targetID)
|
||||
delete(m.pageLastUsed, targetID)
|
||||
m.refs.Remove(targetID)
|
||||
return page.Close()
|
||||
}
|
||||
|
||||
|
||||
@@ -81,6 +81,14 @@ func NormalizeRef(raw string) string {
|
||||
return s
|
||||
}
|
||||
|
||||
// Remove deletes all refs for a target.
|
||||
func (rs *RefStore) Remove(targetID string) {
|
||||
rs.mu.Lock()
|
||||
defer rs.mu.Unlock()
|
||||
rs.removeFromOrder(targetID)
|
||||
delete(rs.entries, targetID)
|
||||
}
|
||||
|
||||
func (rs *RefStore) removeFromOrder(targetID string) {
|
||||
for i, id := range rs.order {
|
||||
if id == targetID {
|
||||
|
||||
@@ -151,6 +151,18 @@ func (t *BrowserTool) Execute(ctx context.Context, args map[string]any) *tools.R
|
||||
}
|
||||
}
|
||||
|
||||
// Apply per-action timeout for heavy operations
|
||||
switch action {
|
||||
case "open", "navigate", "snapshot", "screenshot", "act":
|
||||
timeout := t.manager.ActionTimeout()
|
||||
if ms, ok := args["timeoutMs"].(float64); ok && ms > 0 {
|
||||
timeout = time.Duration(ms) * time.Millisecond
|
||||
}
|
||||
var cancel context.CancelFunc
|
||||
ctx, cancel = context.WithTimeout(ctx, timeout)
|
||||
defer cancel()
|
||||
}
|
||||
|
||||
switch action {
|
||||
case "status":
|
||||
return t.handleStatus()
|
||||
|
||||
Reference in New Issue
Block a user