Files
obscura/crates/obscura-cdp/tests/concurrent_navigations.rs
Timothy Olaleke 85739d3f6a fix(#19): serialize V8 work across pages with process-wide async lock (#36)
* fix(#19): serialize V8 work across pages with process-wide async lock

Concurrent CDP clients (≥5) trip V8's
`heap->isolate() == Isolate::TryGetCurrent()` invariant and abort the
process via V8_Fatal — no Rust panic, just abort(3). Each Page owns its
own JsRuntime/Isolate and they all run on a single OS thread via
LocalSet+spawn_local; when two pages' V8-touching futures interleave
across an .await, V8 trips the check.

This adds a process-wide tokio::sync::Mutex (obscura_js::v8_lock) that
is acquired:

  - At the top of obscura_cdp::dispatch::dispatch (covers every CDP
    handler that may call into a JsRuntime).
  - In server.rs's process_with_interception spawn_local that runs
    page.navigate_with_wait + execute_preload_script (this path
    bypasses dispatch).

The guard keeps each V8-touching block contiguous on the thread: V8
fully exits one Isolate before another page is allowed in. Trade-off:
non-V8 handlers (Browser.*, Storage.*, Emulation.*) also serialize,
but those are cheap and the safety win dominates.

The properly concurrent fix is to pin each JsRuntime to its own OS
thread and message-pass; tracked as the larger #19 follow-up.

Adds an opt-in smoke test (`--ignored`) that boots the CDP server and
runs 5 parallel clients through createTarget+navigate. Note: the
deterministic abort needs subresource-fetch yields (page.rs:285
join_all) which data: URLs skip — full repro requires JS-heavy real
traffic (issue author used scrapegraphai.com).

* fix(#19): suspend other Isolates and defer foreign CDP during interception nav

Address maintainer feedback on PR #36 (comment 4328238087): the V8 fatal
still reproduces with `Fetch.enable patterns:["*"]` + 5 concurrent
`Page.navigate`s. The `obscura_js::v8_lock` mutex was insufficient
because V8's actual invariant is "only one Isolate may be entered per
OS thread", and the mutex can't enforce that across `.await` points
inside V8 ops.

Two distinct paths violated the invariant:

1. Foreign CDP messages dispatched while a nav task was running.
   `process_with_interception`'s `select!` routed every non-Fetch
   message through `process_cdp_message → dispatch → page::handle::*`.
   `get_session_page_mut` then `suspend_js`'es other pages (drops
   their `JsRuntime` → `Isolate::Exit`) while the spawned nav task is
   parked at `op_fetch_url`'s `resolve_rx.await` with its Isolate
   still entered. Drop trips
   `heap->isolate() == Isolate::TryGetCurrent()`.

2. Subsequent navs through `process_with_interception` did not
   suspend prior pages. After nav-1 completes (page-1's JsRuntime
   stays alive in `ctx.pages`), nav-2's `init_js` constructs
   Isolate-2 while Isolate-1 is still current. The next V8 scope
   unwind aborts via `Context::Exit`.

Fix (3 changes in `crates/obscura-cdp/src/server.rs`):

- `cdp_processor`: add a `VecDeque<ServerMessage> deferred` drained
  before each new `rx.recv()`, so deferred messages are processed
  with no nav task in flight.
- `process_with_interception` `select!`: only handle `NewConnection`
  and `Fetch.{continue,fulfill,fail}Request` inline (safe — no V8
  enter on this side); push everything else into `deferred`.
- `process_with_interception` entry: explicitly `suspend_js()` every
  other page in `ctx.pages` before spawning the nav task, mirroring
  the invariant the regular dispatch path already enforces via
  `get_session_page_mut`.

Verification:

- New test `crates/obscura-cdp/tests/concurrent_navigations_with_fetch.rs`
  is the maintainer's exact repro (5 clients, `Fetch.enable patterns:["*"]`,
  concurrent `Page.navigate` to https://example.com, auto-reply
  `Fetch.continueRequest`). Aborts on PR #36 head, passes after fix
  (5/5 stable runs, ~1.2s each).
- Existing data-URL smoke test (`concurrent_navigations`) still passes.
- Full workspace test suite (90 tests) still passes.

Run with:

  cargo test -p obscura-cdp --test concurrent_navigations_with_fetch \
    -- --nocapture --ignored

Honest caveat. This is still the lock/serialize approach — it converts
the abort into latency. The properly concurrent fix (Option 2 in
issue #19: pin each `JsRuntime` to its own OS thread and message-pass)
remains the right long-term direction.

* fix(cdp): bound deferred message queue to prevent OOM during stalled navigation

PR #36 comment 4341743194: process_with_interception deferred foreign
CDP messages into an unbounded VecDeque while a navigation task was in
flight. A stalled navigation (e.g. waiting for Fetch interception
resolution) could accumulate arbitrary queued messages and OOM the
process.

Add MAX_DEFERRED_MESSAGES = 256. When the cap is reached, return an
explicit CDP error (-32000) to the client instead of silently dropping
or growing without bound.

All 90 workspace tests pass.
2026-05-09 18:26:38 +02:00

146 lines
5.6 KiB
Rust

//! Issue #19 smoke test: 5 parallel CDP clients each performing
//! `Target.createTarget` + `Page.navigate` must not abort the process.
//!
//! NOTE on coverage. The deterministic abort in #19 requires the navigations
//! to interleave on the shared `LocalSet` thread, which only happens once
//! `navigate_single` actually yields — the heaviest yields come from
//! subresource fetches in `futures::future::join_all` (page.rs:285) when the
//! page has scripts/images to pull. `data:` URLs skip every fetch, so this
//! test exercises the chokepoint shape (5 clients hitting `dispatch`
//! concurrently) without driving the original abort. Treat it as a smoke
//! check: it ensures the V8-lock plumbing compiles and isn't hitting an
//! obvious deadlock, not as a strict regression for the abort.
//!
//! For an end-to-end repro of the abort, the issue author used
//! `scrapegraphai.com` driven from Node CDP at concurrency 5. Reproducing
//! that here would require standing up a local server with JS subresources;
//! out of scope for this PR.
//!
//! Run with `cargo test -p obscura-cdp --test concurrent_navigations
//! -- --nocapture --ignored` (the `ignored` gate keeps it out of the default
//! suite — it boots a real CDP server, which is heavier than a unit test).
use std::time::Duration;
use futures_util::{SinkExt, StreamExt};
use serde_json::{json, Value};
use tokio::net::TcpListener;
use tokio_tungstenite::{connect_async, tungstenite::Message};
async fn pick_port() -> u16 {
let l = TcpListener::bind("127.0.0.1:0").await.unwrap();
let port = l.local_addr().unwrap().port();
drop(l);
port
}
async fn one_client(port: u16, id_base: u64) -> Result<(), String> {
let url = format!("ws://127.0.0.1:{}/devtools/browser", port);
let (mut ws, _) = connect_async(&url).await.map_err(|e| e.to_string())?;
// Target.createTarget — get a sessionId via Target.attachToTarget.
let create = json!({
"id": id_base,
"method": "Target.createTarget",
"params": {"url": "about:blank"},
});
ws.send(Message::Text(create.to_string().into()))
.await
.map_err(|e| e.to_string())?;
let mut session_id: Option<String> = None;
let mut target_id: Option<String> = None;
while session_id.is_none() {
let msg = tokio::time::timeout(Duration::from_secs(5), ws.next())
.await
.map_err(|_| "timeout waiting for createTarget".to_string())?
.ok_or("ws closed")?
.map_err(|e| e.to_string())?;
let text = match msg {
Message::Text(t) => t.to_string(),
_ => continue,
};
let v: Value = serde_json::from_str(&text).map_err(|e| e.to_string())?;
if let Some(t) = v.get("result").and_then(|r| r.get("targetId")).and_then(|s| s.as_str()) {
target_id = Some(t.to_string());
}
if let Some(s) = v
.get("params")
.and_then(|p| p.get("sessionId"))
.and_then(|s| s.as_str())
{
session_id = Some(s.to_string());
}
}
let sid = session_id.unwrap();
let _ = target_id; // kept for debugging — unused by the assertion path
// Page.navigate to a data URL — exercises init_js + execute_scripts
// without touching the network, which is what the V8 race needs.
let nav = json!({
"id": id_base + 1,
"method": "Page.navigate",
"sessionId": sid,
"params": {"url": "data:text/html,<html><body><h1>x</h1></body></html>"},
});
ws.send(Message::Text(nav.to_string().into()))
.await
.map_err(|e| e.to_string())?;
let deadline = tokio::time::Instant::now() + Duration::from_secs(10);
loop {
if tokio::time::Instant::now() >= deadline {
return Err("timeout waiting for navigate response".to_string());
}
let remaining = deadline - tokio::time::Instant::now();
let msg = tokio::time::timeout(remaining, ws.next())
.await
.map_err(|_| "timeout".to_string())?
.ok_or("ws closed mid-navigate")?
.map_err(|e| e.to_string())?;
let text = match msg {
Message::Text(t) => t.to_string(),
_ => continue,
};
let v: Value = serde_json::from_str(&text).map_err(|e| e.to_string())?;
if v.get("id").and_then(|x| x.as_u64()) == Some(id_base + 1) {
return Ok(());
}
}
}
#[tokio::test(flavor = "current_thread")]
#[ignore = "boots a real CDP server; opt-in with --ignored"]
async fn concurrency_5_does_not_abort_v8() {
let port = pick_port().await;
let local = tokio::task::LocalSet::new();
local
.run_until(async {
tokio::task::spawn_local(async move {
let _ = obscura_cdp::server::start(port).await;
});
// Give the listener a beat.
tokio::time::sleep(Duration::from_millis(150)).await;
let mut handles = Vec::new();
for i in 0..5u64 {
let id_base = (i + 1) * 1000;
handles.push(tokio::task::spawn_local(async move {
one_client(port, id_base).await
}));
}
let mut ok = 0usize;
for (i, h) in handles.into_iter().enumerate() {
match h.await {
Ok(Ok(())) => ok += 1,
Ok(Err(e)) => panic!("client {} failed: {}", i, e),
Err(e) => panic!("client {} join error: {}", i, e),
}
}
assert_eq!(ok, 5, "all 5 concurrent clients must complete navigate");
})
.await;
}