Data Liberation: Add timeouts to browser calls so extraction cannot hang

[aagam-shah]

data-liberation <url> can hang forever on large sites. A 26-page Wix site hung on every run, always after the first page, and never finished. Small runs (--limit 2) always passed. The process showed no error — it just stopped making progress.

Related issues

• None filed. Found while testing large Wix imports.

How AI was used in this PR

Claude (Fable 5) found the cause and wrote the patch while I ran live tests against a real 26-page Wix site. I reviewed the diff and the test results.

Proposed Changes

The cause: Playwright only puts a default timeout on navigations and actions. page.evaluate, page.content(), and direct browser protocol (CDP) calls have no timeout at all and can wait forever. When the source site slows down or starts blocking our requests, the browser tab can stop responding. A call into that tab then never returns, and the whole extraction gets stuck with it.

The fix: a small withTimeout helper, time limits on all browser calls in the Wix adapter, and a 5-minute time limit for each page in the shared extraction loop, so every adapter is covered. A stuck page now fails with a logged timeout and the run continues to the next page.

dist/ is rebuilt from this change (this repo commits built files) — please review src/ only.

Testing Instructions

• npm test in packages/data-liberation-agent — 2914 tests pass.
• Live check: run node dist/cli.js <large wix site> --no-agent --non-interactive. Before this patch it hung after page 1. Now it completes; a page that freezes (for example an infinite-scroll category page) logs a timeout and the run continues.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Copilot reviewed 5 out of 10 changed files in this pull request and generated no new comments.

Suppressed comments (2)

packages/data-liberation-agent/src/adapters/wix/page.ts:334

• If newCDPSession() exceeds this deadline but resolves later, withTimeout does not cancel it and client remains null, so the finally block cannot detach the newly attached session. Repeated late resolutions can still accumulate CDP sessions. Keep the original session promise and attach a late-resolution cleanup that detaches when the timeout wins.

    client = await withTimeout(
      p.context().newCDPSession(page), RENDERER_CALL_TIMEOUT_MS, 'wix CDP session');

packages/data-liberation-agent/src/adapters/wix/page.ts:11

• addInitScript() at line 127 is still an unbounded Playwright call, even though this deadline is presented as covering the renderer-bound Wix path. Because the response listener is installed before that await and is not removed in a finally, a stuck call reaches the outer five-minute watchdog, which abandons this extraction while leaving its listener attached to the shared page. Subsequent URLs then accumulate handlers and parse each response repeatedly. Please bound addInitScript() and guarantee listener removal in a finally.

/** Hard deadline for renderer-bound Playwright calls (evaluate/content/CDP).
 *  Playwright gives them NO default timeout, so a frozen renderer would
 *  otherwise leave the await pending forever and block the extraction loop. */
const RENDERER_CALL_TIMEOUT_MS = 30_000;

[chubes4]

The normal path works, but the timeout path is not safe to merge yet.

I tested the exact head (c01de5fad):

• npm test --workspace=data-liberation: 2914 passed, 1 skipped, 1 todo.
• Package build passed.
• A real extraction of 10 URLs from a public 54-URL Wix site completed successfully with no extraction failures.

I then tested the behavior introduced by withTimeout against a real Playwright Page. After the old operation timed out, the next operation updated the shared page; the abandoned old operation later completed and overwrote that state:

{"afterNext":"next-operation","final":"old-operation"}

That confirms the concern at src/adapters/shared.ts: the five-minute watchdog reports a failed URL and continues, but it does not stop extractPage. Adapters such as Wix and Squarespace reuse mutable browser resources, so the timed-out extraction can race the next URL. The helper explicitly documents that the underlying operation is not cancelled.

I also reproduced the late CDP-session case around src/adapters/wix/page.ts:333: when newCDPSession() resolves after the deadline, client is never assigned and the current finally cannot detach it:

{"timeout":"wix CDP session timed out after 10ms","clientAssigned":false,"detachCalls":0}

addInitScript() is also still unbounded after the response listener is attached, while listener removal is not protected by finally.

Please make timeout completion own resource cleanup before the loop continues. For shared Playwright state, that could mean closing and recreating the page/context after timeout, or adding cancellation and awaiting cleanup. The regression test should prove that a timed-out operation cannot mutate resources used by the next URL and that late-created CDP sessions are detached.

AI assistance: OpenCode with openai/gpt-5.6-sol was used to inspect the diff, run the package/build/live-extraction checks, and construct the focused Playwright and CDP lifecycle reproductions.

[aagam-shah]

Thanks for the review, Chris — the reproductions made these easy to pin down. All points addressed in e4b959f. What changed:

• New createManagedBrowser in browser-kit. The browser is used through a lease: reset() and end() invalidate every older lease, so a timed-out extraction can never touch the next URL's session — and it checks lease.isValid() before writing to shared state, so a late completion cannot add products or mutate results after its URL was logged as failed. Launches are cached as a promise (no double launch), health-checked (isConnected, page.isClosed), and every close is bounded. Sessions that resolve after a deadline are disposed the moment they appear, including CDP sessions — your clientAssigned:false case.
• The loop runs the adapter's cleanup after a watchdog timeout, before any later URL. Wix, Shopify, and Squarespace reset their shared browser through it.
• Page extraction for those three adapters is capped at concurrency 1. The tuner could raise it to 2–3 on one shared page, which mixes content across URLs. This makes runs slower; per-URL tabs to get concurrency back safely would be a follow-up.
• addInitScript is bounded and the response listener is removed in a finally.
• Regression tests as requested: a timed-out operation cannot mutate resources the next URL uses, and late-created CDP sessions get detached. Full suite: 2939 passing.

Live check: a real 26-page Wix site extracted end to end in a sandbox — all 26 pages, no hangs, no timeouts, 28 minutes of extraction at the new serial concurrency.

Known gap I did not fix here: a permanently frozen renderer can keep each page under the 5-minute watchdog via the inner 30s fallbacks, so it degrades output without triggering a reset. I'd take that as a follow-up.

[Copilot]

Pull request overview

Copilot reviewed 17 out of 25 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

packages/data-liberation-agent/src/lib/browser-kit/browser-kit.ts:58

• The managed timeout cannot clean up a browser when newContext() or newPage() is the call that hangs. launchBrowser() has already obtained browser, but its promise never resolves to a BrowserSession, so disposeOnce never receives anything to close; each retry can therefore leave another Chromium process/tab behind. Bound these page-creation calls inside launchBrowser() and close browser on timeout before rejecting.

    page = await ctx.newPage();

[aagam-shah]

Hi Chris — quick summary of where this PR ended up, and what I'd push to follow-ups.

Since your review:

• Browser lifecycle handling now lives in one place: createManagedBrowser. Leases fence out abandoned extractions, health checks catch crashed browsers, and every close and launch has a time limit.
• Copilot caught four more gaps in later passes — all fixed: bounded launch, bounded wix discovery, the SVG rasterizer drops its stuck browser on timeout, and shopify buffers product rows until the lease check passes.
• I swept the package for the same bug classes: all nine adapters are now clear on shared-state writes inside extractPage. Five already commit products through the loop; wix and shopify are fenced for now.
• Full suite: 2946 passing. Live check: a 26-page Wix site, all pages, no hangs.

Follow-ups I'd file as separate issues — tell me which you want:

1. Converge wix and shopify onto the loop's product-commit path and delete the lease checks around CSV writes. runExtractionLoop already commits products for the simple adapters via the extractProduct hook — wix and shopify write from inside extractPage instead, which is the only reason they need fencing. Moving them over retires this bug class for good.
2. Bound the ~25 browser calls outside the extraction path (screenshot module, other adapters' discovery, one MCP handler). I'll put the full list in the issue.
3. A permanently frozen renderer never triggers a browser reset: each call inside extractWixPage fails at its own 30s limit and falls back, so the page "succeeds" with served-HTML content in ~4 minutes — under the 5-minute watchdog. Every later page then does the same. Degraded output, no error, no recreation.
4. Per-URL tabs, to bring back page concurrency. Extraction is serial now because concurrent extractions shared one page; giving each URL its own tab would make concurrency safe again.
5. Media downloads sometimes produce a duplicate URL with a stray 2F prefix (.../media/2F<id>... — looks like a leftover %2F decode). The original URL downloads fine; the 2F copy gets a 403. Found on live Wix runs.

Ready for another look when you have time.

[chubes4]

The original timeout-path lifecycle issues are substantively resolved: stale leases fence late work, adapter cleanup runs before the next batch, late CDP sessions are detached, and Wix/Shopify shared-output writes are guarded. The added tests cover those contracts well. Two blockers remain on the current head.

1. Browser context/page creation is still unbounded. launchBrowser() awaits browser.newContext() and ctx.newPage() directly. If Chromium keeps its transport open but never answers either protocol command, launchBrowser() never produces a BrowserSession. The 60-second wrapper in createManagedBrowser() therefore has no resolved or late-resolved session to dispose, leaving both the await and the already-launched browser outstanding. Wix discovery also calls launchBrowser() directly, outside the managed deadline. Please bound context/page creation while retaining ownership of the browser so timeout cleanup can close it deterministically, and add a regression where newContext() or newPage() never settles.

2. Serializing Wix, Shopify, and Squarespace with maxPageConcurrency: 1 is not an acceptable final performance tradeoff for this extraction path. It prevents shared-page races, but removes the adaptive 2-3 URL concurrency and the reported 26-page Wix run now takes 28 minutes. The desired isolation boundary is per URL: share the Chromium process, give concurrent extractions independent pages or contexts, close only the timed-out URL resource, and commit output centrally after successful completion. Full-browser reset should remain the browser-health fallback rather than the normal page-timeout mechanism. Please restore safe bounded concurrency, with a regression proving concurrent URLs cannot observe or mutate each other's page state.

The timeout and lease machinery is otherwise a meaningful improvement, but merging with an unbounded creation path would leave the “cannot hang” contract incomplete, while merging the serial caps would lock in a material throughput regression that the extraction architecture should avoid.

AI assistance: OpenCode with openai/gpt-5.6-sol was used to inspect the current PR head, trace timeout/resource ownership, compare the follow-up implementation against the prior review, and assess the concurrency tradeoff. Chris Huber reviewed and remains responsible for this review.

[aagam-shah]

Problem 1 is fixed in 93307a0. newContext() and newPage() now have time limits inside launchBrowser() itself, and on timeout we close the browser we are already holding — so it either returns a working session or rejects with nothing left running. Added the regression tests for a newContext()/newPage() that never settles. (While verifying, we also bounded the connect step and the default adapter's own context creation, and made a late-arriving CDP page close itself instead of orphaning a tab — same class, small additions.)

On problem 2 — the shared-page concurrency is already broken on trunk today. I tried to reproduce it: two extractions on the same page at the same time, and in 10 out of 10 tries the pages got mixed — one extraction returned the other page's content and title under its own URL, and it was recorded as a success. The tuner reaches concurrency 2 by the third batch of a healthy run, so this is the normal case, not a rare one. Happy to share the repro script.

So the serial cap is not removing working speed — it is removing speed that was producing wrong pages. I'd prefer to keep this PR at "safe and linear," and build the per-URL pages design you described as a separate PR — folding it in here would make this PR quite complex. Does that split work for you?

[chubes4]

The split works for me. Your 10/10 reproduction establishes that trunk's shared-page concurrency is corrupting page identity, so keeping this PR safe and serial is the right baseline. Per-URL pages can restore real concurrency separately without holding this reliability fix.

93307a053 resolves the context/page creation blocker and the new regression coverage looks good. I found one final small cleanup gap before approval: launchBrowser() bounds connectBrowser() but does not pass an onLateResolve disposer. If Chromium launch/CDP connection exceeds 60 seconds and later resolves, the command has already moved on and that browser can be orphaned. The context/page paths already handle this ownership correctly.

I'm happy to push the focused late-connect close plus regression test to your branch if that works for you; otherwise it should be a small follow-up commit on your side. After that, I think this is ready to land.

AI assistance: OpenCode with openai/gpt-5.6-sol was used to inspect the latest commit, verify the timeout ownership paths and test coverage, and compare this PR with the separate #3952 capture workflow. Chris Huber reviewed and remains responsible for this feedback.

[wpmobilebot]

📊 Performance Test Results

Comparing 69dfbe5 vs trunk

app-size

Metric	trunk	69dfbe5	Diff	Change
App Size (Mac)	1421.98 MB	1421.98 MB	+0.00 MB	⚪ 0.0%

site-editor

Metric	trunk	69dfbe5	Diff	Change
load	1185 ms	1186 ms	+1 ms	⚪ 0.0%

site-startup

Metric	trunk	69dfbe5	Diff	Change
siteCreation	7541 ms	7538 ms	3 ms	⚪ 0.0%
siteStartup	3389 ms	3369 ms	20 ms	⚪ 0.0%

---

Results are median values from multiple test runs.

Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change (<50ms diff)

[chubes4]

The requested timeout lifecycle fixes are now in place and the full CI suite is green. Approving for merge.

AI assistance: OpenCode with openai/gpt-5.6-sol was used to verify the current review state and CI results before submitting this approval. Chris Huber reviewed and remains responsible for this approval.