Skip to content

fix: route browser tools through tool_fn so they honor RATE_LIMIT - #169

Open
karaposu wants to merge 1 commit into
brightdata:mainfrom
karaposu:fix/browser-tools-tool-fn
Open

fix: route browser tools through tool_fn so they honor RATE_LIMIT#169
karaposu wants to merge 1 commit into
brightdata:mainfrom
karaposu:fix/browser-tools-tool-fn

Conversation

@karaposu

Copy link
Copy Markdown

Problem

tool_fn is the wrapper every tool is supposed to run through — rate limiting, entry/duration logging, session_stats counting, error translation. It is applied by hand at each definition site, and the 14 scraping_browser_* tools in browser_tools.js were never wrapped (importing tool_fn back into that file would be a circular dependency). Consequences for the whole browser family:

  • RATE_LIMIT does not apply at all — the one control built to stop a runaway agent is absent from exactly the tools that hold paid browser sessions and have unbounded navigate → snapshot → click fan-out;
  • no log line that a browser tool ran, failed, or how long it took;
  • session_stats silently under-reports the most expensive tool family.

Fix

Wrap at the single registration chokepoint instead of at 60 definition sites:

  • tool_fn is now idempotent — wrapping an already-wrapped function returns it unchanged (__tool_fn_wrapped marker);
  • addTool wraps every tool's execute as it registers it, so tools defined without a manual wrapper — notably the browser tools — get rate limiting, logging, and counting uniformly.

The 11 existing manual tool_fn(...) calls become harmless no-ops (no double count, no second rate-limit check); removing them is a separate cleanup.

Behaviour change (intended)

Browser tools now count against RATE_LIMIT. A browser automation is a sequence of tool calls, so under a low limit it can now throttle mid-workflow — the exemption was the bug. Browser error surfaces are unchanged: their errors are UserErrors / plain Errors, which tool_fn re-throws as-is.

Tests

test/browser-rate-limit.test.js (spawn-based, network-free): with RATE_LIMIT=1/1h, session_stats succeeds and consumes the slot, then scraping_browser_go_back is rejected with Rate limit exceeded before any connection attempt. On unfixed code the browser call bypasses the limiter (different failure), so the test discriminates fixed from unfixed.

Notes

Independent of #163 (no shared commits). Touches addTool, so it may need a trivial rebase against sibling PRs depending on merge order.

The 14 scraping_browser_* tools were registered without the tool_fn
wrapper every other tool gets, so they bypassed rate-limiting, per-call
logging, and session_stats counting. Move the wrapper to the single
registration chokepoint (addTool), so every tool -- including the ones
defined in browser_tools.js -- is wrapped uniformly, and make tool_fn
idempotent so tools already wrapped at their definition site are not
double-wrapped (no double count, no second rate-limit check).

Behaviour change: browser tools now count against RATE_LIMIT. A browser
automation is a sequence of calls, so under a low limit it can now
throttle mid-workflow; this is intended -- the exemption was the bug.

The 11 existing manual tool_fn(...) calls are now redundant but harmless
(idempotency makes them no-ops); removing them is a separate cleanup.

Adds test/browser-rate-limit.test.js: with RATE_LIMIT=1/1h, session_stats
consumes the slot and a following browser tool call is rejected with
"Rate limit exceeded", proving browser tools now run through tool_fn.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant