[suchintan]

https://github.com/Skyvern-AI/rustwright-cloud/pull/239

[claude]

Summary

Large, well-engineered change that bounds page-owned memory (navigation-response ring buffer + fulfilled-route bodies) on the Python side and adds a matching Rust-side CdpRetentionGate that safely tears down CDP event/traffic/frame state on page/browser close without racing in-flight dispatchers or lease commits. The new Rust tests specifically target the races this kind of change usually gets wrong (late dispatch after release, in-flight OOPIF attach vs. release, lease commit vs. release), and the CHANGELOG/benchmark-tooling changes are honestly scoped (no overclaiming). Nothing rises to blocking; one behavioral change deserves a second look.

🔴 Critical Issues (0)

None found.

🟡 Suggestions (3)

• _fulfilled_route_bodies changed from non-destructive get() to destructive pop(). In Response._cache_body() (python/rustwright/sync_api.py) and the new _transfer_fulfilled_route_body_locked() called from _record_response, the fulfilled body is now popped out of page._fulfilled_route_bodies on first access and attached to whichever Response object happens to be the "matching" one in _navigation_responses/_response_log. If a second, distinct Response Python object for the same request_id is ever recorded/read after the first consumes it (e.g. one instance from a page.on("response") listener, another from route.fulfill()'s own bookkeeping, or from wait_for_response), that second object will find nothing in the dict and fall back to a live Page.getResponseBody CDP call — which likely fails or returns the wrong bytes for a synthetically fulfilled route. The new tests cover ring-buffer eviction/byte-cap survival but I didn't see one exercising two independent Response handles for the same fulfilled route. Worth double-checking (or adding a test) that route-fulfilled bodies stay readable from every handle, not just the first one recorded.
• New user-visible failure mode on console capture / worker registration. enable_console_capture and RustwrightPage.targets/getTargets-adjacent workers path now raise RwError::Message("CDP event log contains unreplayable console history"/"... worker history") when the retained CDP event log has tombstoned an oversized/overflowed entry before the page finished replaying its iframe/worker attachment history. This is a legitimate fail-closed choice (matches the "explicit _overflow, never silently wrong" pattern used elsewhere), but it's a new, Rustwright-specific error class with no Playwright analog. Consider documenting when a user could hit it (very chatty OOPIF/worker attachment history exceeding the 8 MiB / 64 KiB-per-entry retained-event budget) so it doesn't read as a mysterious internal error.
• classify_process_role() (benchmarks/process_tree_memory.py) is a hardcoded, version-specific mapping of Chromium --type=/--utility-sub-type= argv values to roles. It fails safe (unknown types bucket into "other", so PSS totals still reconcile), but if Chromium renames a process type or sub-type, role attribution will silently drift into "other" without any signal that the taxonomy is stale. A comment noting it's best-effort against current Chromium switches (and/or a benchmark assertion that "other" stays near-zero on the CI Chromium version) would make regressions visible.

📝 Minor / Style (2)

• The defensive if not hasattr(self, "_x"): self._x = ... guards repeated across _drop_page_owned_response_locked, _prune_navigation_responses_locked, etc. exist to support Page.__new__()-based unit-test doubles (see _memory_test_page() in tests/test_rustwright_sync_api.py). They work, but the same six attributes are re-initialized in multiple methods; a single _ensure_network_history_state() helper called once would avoid drift as more retained collections get added later.
• Nice touch on the benchmark-matrix honesty side: positive_repetitions validation, complete_repetition_group/canonical_aggregate_complete, and splitting partial-repetition results into diagnostic_aggregate (explicitly excluded from canonical metrics and called out in the markdown table) meaningfully reduces the risk of comparing implementations on unequal sample sizes. No action needed, just flagging as a good pattern to keep.