Fix/engine hardening (#249)

* fix(dom): read insert_before prev_sibling after detach to avoid a sibling cycle

insert_before captured the reference node's prev_sibling BEFORE detaching the
node being inserted. When page JS inserts a node before its own immediate
previous sibling (a no-op reorder frameworks do constantly), detach moves that
pointer, so the stale prev_id spliced new.next_sibling = new. The resulting
sibling self-cycle made descendants() inner next_sibling walk loop forever in
native Rust, uninterruptible by tokio or the V8 watchdog. This hung ebay.com,
found by a 1500-page crash/hang sweep.

Read prev_sibling after detach so it reflects the post-detach tree. Cap the inner
sibling-collection loops in descendants() and log when a descendants cap fires,
as defense in depth so a sibling cycle from any path cannot hang the engine. Add
test_insert_before_previous_sibling_no_cycle.

ebay.com now renders (192K chars) instead of hanging. Verified: 33/33 obscura-dom
tests, obstacle course 33/33.

* fix(cli): settle async --eval work before reading the page (#248)

--eval ran after the selector check and returned its value immediately, so an
expression that only kicks off async work (e.g. fetch().then(t => DOM write))
returned the still-pending Promise as {} and any --selector/--dump were ignored.

When --eval is combined with --selector and/or --dump, run the eval to kick off
its async work, settle the event loop again so it completes, then read the page
(selector wait + dump) instead of returning the Promise. A bare --eval still
returns its value unchanged, so synchronous expressions are not affected, and the
eval stays bounded by evaluate_with_timeout.

dump becomes Option<DumpFormat> so an explicit --dump is distinguishable from the
default. Verified: the repro now returns the fetched content, a plain
JSON.stringify eval is unchanged, 37 + 16 cli tests pass.

crates/obscura-cli/src/main.rs

@@ -89,8 +89,12 @@ enum Command {
     Fetch {
         url: String,
 
-        #[arg(long, default_value = "html")]
-        dump: DumpFormat,
+        // Default is html. Kept as Option so we can tell whether --dump was
+        // explicitly passed: a bare --eval returns its own value, while --eval
+        // combined with --dump (or --selector) runs the eval, lets its async
+        // work settle, then reads the page (issue #248).
+        #[arg(long)]
+        dump: Option<DumpFormat>,
 
         #[arg(long)]
         selector: Option<String>,
@@ -463,7 +467,7 @@ async fn run_multi_worker_serve(
 
 async fn run_fetch(
     url_str: &str,
-    dump: DumpFormat,
+    dump: Option<DumpFormat>,
     selector: Option<String>,
     wait_secs: u64,
     timeout_secs: u64,
@@ -477,6 +481,12 @@ async fn run_fetch(
     storage_dir: Option<std::path::PathBuf>,
     allow_private_network: bool,
 ) -> anyhow::Result<()> {
+    // Whether the user explicitly passed --dump. With --eval also present this
+    // decides whether we return the eval value or read the page after the
+    // eval's async work settles (issue #248).
+    let dump_specified = dump.is_some();
+    let dump = dump.unwrap_or(DumpFormat::Html);
+
     // --dump original short-circuits the browser stack entirely: fetch the raw
     // response body via HTTP and stream the bytes verbatim. Useful for binary
     // payloads (images, fonts, …) and any non-HTML resource where parsing the
@@ -548,6 +558,33 @@ async fn run_fetch(
     // pages stay fast.
     page.settle(wait_secs.saturating_mul(1000)).await;
 
+    if let Some(ref expr) = eval {
+        // Bound the eval by the same budget as navigation so a runaway
+        // expression (infinite loop, never-settling sync work) cannot hang.
+        let result = page.evaluate_with_timeout(expr, Duration::from_secs(timeout_secs));
+
+        // A bare --eval (no --selector, no --dump) returns the eval value
+        // directly, so synchronous expressions (JSON.stringify, ...) are
+        // unchanged.
+        if !dump_specified && selector.is_none() {
+            let rendered = match result {
+                serde_json::Value::String(s) => s,
+                serde_json::Value::Null => "null".to_string(),
+                other => other.to_string(),
+            };
+            write_or_print(rendered, output.as_ref()).await?;
+            context.save_cookies();
+            return Ok(());
+        }
+
+        // --eval combined with --selector and/or --dump: the eval typically
+        // kicks off async work (a fetch promise, a timer) that writes the DOM.
+        // Drive the event loop again so that work completes, then fall through
+        // to the selector wait and the dump path instead of returning the
+        // still-pending eval value (issue #248).
+        page.settle(wait_secs.saturating_mul(1000)).await;
+    }
+
     if let Some(ref sel) = selector {
         let found = wait_for_selector(&mut page, sel, wait_secs).await;
         if !found {
@@ -555,20 +592,6 @@ async fn run_fetch(
         }
     }
 
-    if let Some(ref expr) = eval {
-        // Bound the eval by the same budget as navigation so a runaway
-        // expression (infinite loop, never-settling sync work) cannot hang.
-        let result = page.evaluate_with_timeout(expr, Duration::from_secs(timeout_secs));
-        let rendered = match result {
-            serde_json::Value::String(s) => s,
-            serde_json::Value::Null => "null".to_string(),
-            other => other.to_string(),
-        };
-        write_or_print(rendered, output.as_ref()).await?;
-        context.save_cookies();
-        return Ok(());
-    }
-
     let rendered = match dump {
         DumpFormat::Html => dump_html(&page),
         DumpFormat::Text => dump_text(&mut page),
@@ -1143,7 +1166,7 @@ mod tests {
         .expect("clap should accept --dump original");
         match args.command {
             Some(Command::Fetch { dump, .. }) => {
-                assert_eq!(dump, DumpFormat::Original);
+                assert_eq!(dump, Some(DumpFormat::Original));
             }
             _ => panic!("expected Fetch command"),
         }
@@ -1406,7 +1429,7 @@ mod tests {
     fn matcher_still_uses_fetch_variant() {
         let cmd = Some(Command::Fetch {
             url: "https://x".to_string(),
-            dump: super::DumpFormat::Html,
+            dump: Some(super::DumpFormat::Html),
             selector: None,
             wait: 5,
             timeout: 30,
@@ -1495,7 +1518,7 @@ mod tests {
         .expect("clap should accept --dump assets");
         match args.command {
             Some(Command::Fetch { dump, .. }) => {
-                assert_eq!(dump, DumpFormat::Assets);
+                assert_eq!(dump, Some(DumpFormat::Assets));
             }
             _ => panic!("expected Fetch command"),
         }

crates/obscura-dom/src/tree.rs

@@ -306,14 +306,10 @@ impl DomTree {
         if existing_id == new_sibling_id {
             return;
         }
-        let (parent_id, prev_id) = {
+        let parent_id = {
             let inner = self.inner.borrow();
-            let node = match inner.nodes.get(existing_id.index()).and_then(|n| n.as_ref()) {
-                Some(n) => n,
-                None => return,
-            };
-            match node.parent {
-                Some(p) => (p, node.prev_sibling),
+            match inner.nodes.get(existing_id.index()).and_then(|n| n.as_ref()).and_then(|n| n.parent) {
+                Some(p) => p,
                 None => return,
             }
         };
@@ -349,6 +345,17 @@ impl DomTree {
 
         self.detach(new_sibling_id);
 
+        // Read existing's prev AFTER detaching new. If new was existing's
+        // immediate previous sibling, detach moved that pointer; using the
+        // pre-detach value would splice new.next_sibling = new (a self-cycle)
+        // and hang every later sibling walk. This is what hung ebay.com.
+        let prev_id = {
+            let inner = self.inner.borrow();
+            inner.nodes.get(existing_id.index())
+                .and_then(|n| n.as_ref())
+                .and_then(|n| n.prev_sibling)
+        };
+
         let mut inner = self.inner.borrow_mut();
 
         if let Some(Some(node)) = inner.nodes.get_mut(new_sibling_id.index()) {
@@ -496,6 +503,10 @@ impl DomTree {
         let mut children_to_push = Vec::new();
         while let Some(child_id) = first {
             children_to_push.push(child_id);
+            if children_to_push.len() > inner.nodes.len() {
+                eprintln!("obscura: sibling-chain cap hit at node {} - cycle", node_id.index());
+                break;
+            }
             first = inner.nodes.get(child_id.index())
                 .and_then(|n| n.as_ref())
                 .and_then(|n| n.next_sibling);
@@ -512,6 +523,11 @@ impl DomTree {
             // than grow the stack and result forever and wedge the engine. On a
             // valid tree this bound is never reached, so the hot path is unchanged.
             if result.len() > inner.nodes.len() {
+                eprintln!(
+                    "obscura: descendants() cap hit at node {} ({} nodes) - tree has a cycle",
+                    node_id.index(),
+                    inner.nodes.len()
+                );
                 break;
             }
 
@@ -521,6 +537,10 @@ impl DomTree {
             let mut children_to_push = Vec::new();
             while let Some(child_id) = child {
                 children_to_push.push(child_id);
+                if children_to_push.len() > inner.nodes.len() {
+                    eprintln!("obscura: sibling-chain cap hit at node {} - cycle", current.index());
+                    break;
+                }
                 child = inner.nodes.get(child_id.index())
                     .and_then(|n| n.as_ref())
                     .and_then(|n| n.next_sibling);
@@ -853,6 +873,37 @@ mod tests {
         assert_eq!(tree.descendants(doc).len(), before);
     }
 
+    #[test]
+    fn test_insert_before_previous_sibling_no_cycle() {
+        // Inserting a node before its own immediate previous sibling is a no-op
+        // reorder that frameworks do constantly. It used to splice
+        // next_sibling = self via a prev_id captured before detach, hanging every
+        // later sibling walk (this hung ebay.com). The result must stay a
+        // well-formed [a, b] with no cycle.
+        let tree = DomTree::new();
+        let doc = tree.document();
+        let mk = |n: &str| {
+            tree.new_node(NodeData::Element {
+                name: QualName::new(None, ns!(html), LocalName::from(n)),
+                attrs: vec![],
+                template_contents: None,
+                mathml_annotation_xml_integration_point: false,
+            })
+        };
+        let parent = mk("div");
+        let a = mk("a");
+        let b = mk("b");
+        tree.append_child(doc, parent);
+        tree.append_child(parent, a);
+        tree.append_child(parent, b); // parent -> [a, b]
+
+        // a is already b's previous sibling; this reorder must not create a cycle.
+        tree.insert_before(b, a);
+
+        let kids = tree.descendants(parent);
+        assert_eq!(kids, vec![a, b], "order preserved, no cycle");
+    }
+
     #[test]
     fn test_append_text_merges() {
         let tree = DomTree::new();