Skip to content

fix: handle detached frames during snapshots - #2606

Draft
shrey150 wants to merge 11 commits into
v4-spikefrom
shrey/stg-2781-snapshot-iframe-churn
Draft

fix: handle detached frames during snapshots#2606
shrey150 wants to merge 11 commits into
v4-spikefrom
shrey/stg-2781-snapshot-iframe-churn

Conversation

@shrey150

@shrey150 shrey150 commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Prevent a detached child frame from retrying an accessibility-tree request against the main CDP session and duplicating the root snapshot.
  • Preserve the unscoped fallback required by the main frame and live out-of-process iframe (OOPIF) session roots.
  • Add deterministic race coverage, a bounded real-browser churn test, and the regenerated Go extension asset.

There is no public API or protocol change.

Why this is a Stagehand bug

The failure first appeared in a public OdysseysBench task after the run reached Hulu, but the same behavior reproduces on a local synthetic page with no third-party site, model, or external network dependency.

The failure sequence is internal to the V4 snapshot implementation:

  1. captureHybridSnapshot() records the frame topology.
  2. A same-process iframe detaches before its per-frame accessibility capture.
  3. Session lookup for that stale frame ID falls back to the main CDP session.
  4. Accessibility.getFullAXTree({ frameId }) rejects the detached frame ID.
  5. The previous broad recovery path retries Accessibility.getFullAXTree() without a frame ID.
  6. That unscoped request captures the main document again and attributes it to the stale child.

Each stale child can therefore duplicate root-sized work and data. The current v4-spike base no longer imposes the former implicit 10-second page.snapshot() client deadline, but the underlying correctness and work-amplification bug remains.

Fix

  • Classify only known frame-scope CDP errors as eligible for recovery; unrelated CDP errors still propagate.
  • Check child-frame registry liveness before and after expensive per-frame work.
  • Permit unscoped accessibility fallback only for the main frame or a still-live OOPIF session root.
  • Omit a child that detaches during capture instead of merging partial or root-duplicated data.
  • Merge only captured frames whose parent path was resolved successfully.

The fix stays local to snapshot capture rather than changing the global unknown-frame session lookup contract. That keeps the blast radius narrow and preserves callers that may rely on the existing lookup behavior.

Resolved spike questions

Question Decision
Change unknown-frame lookup globally? No. The snapshot path now rejects the unsafe fallback at the point where frame/session context is known.
Preserve OOPIF behavior? Yes. A live OOPIF may still retry unscoped on its own child session; deterministic coverage locks this in.
Cover nested and detach/reattach races? Yes. Tests cover nested-frame omission and replacement frame IDs.
Add skipped-frame logging or metrics? No. Frame detach is normal browser behavior, so logging every race would be noisy.
Add cancellation or serialization? Not in this PR. Those are broader safeguards and are not required to remove the stale-frame amplification.
Keep the stress fixture? Yes, as a bounded on-demand integration test: churn starts only after initial frames load and stops automatically after two seconds.
Regenerate packaged extension assets? Yes. The Go embedded extension is regenerated and drift-checked.

Historical fit

This follows the repository's usual shape for lifecycle/CDP fixes: a narrow behavioral change, deterministic regression coverage, real-browser coverage where timing matters, and regenerated packaged assets. Comparable precedents include #2077, #1719, and the V4 snapshot packaging work in #2556.

Risk and limitations

  • A child that detaches during capture is omitted from that snapshot. This is intentional: it no longer belongs to the page topology that can be represented consistently.
  • The PR does not change general snapshot cancellation. A different future source of long-running work may still justify cancellation independently.
  • The production path remains sequential, so this does not introduce new concurrency or ordering behavior.

E2E Test Matrix

Command / flow Observed output Confidence / sufficiency
pnpm exec vitest run packages/extension/understudy/a11y/snapshot/frame-races.test.ts --reporter=verbose 1 file passed; 7 race tests passed. Directly covers detached same-process frames, live and detached OOPIFs, detachment during merge-prefix resolution, main-frame fallback, unrelated CDP errors, nested omission, and detach/reattach replacement.
Five consecutive runs of pnpm run test:integration -- packages/sdk-ts/tests/integration/snapshotIframeChurn.test.ts All 5 runs passed. During churn the root appeared exactly once; after churn each of 16 frame labels appeared exactly 24 times and the root still appeared once. Exercises the built extension through the public TypeScript object model and a real local Chrome process while checking liveness during detachment and exact snapshot completeness/uniqueness after frames settle.
pnpm check && pnpm build && pnpm test All repository tasks passed. Extension tests: 295 passed, 10 todo; SDK tests: 155 passed; eval tests: 409 passed. Covers formatting, lint, types, builds, package tests, and browser-runtime tests across the TypeScript workspace.
pnpm exec vitest run 148 files passed and 1 skipped; 1,305 tests passed, 3 skipped, 10 todo. Reproduces CI's root-wide Vitest collection and confirms the integration fixture no longer contaminates unrelated suites.
uv --directory packages/sdk-python run --locked pytest followed by installed-wheel scripts/smoke.py 411 passed, 1 skipped; the built wheel installed in isolation, contained the extension, launched Chrome, navigated, and closed cleanly. Covers Python compatibility and the exact packaged extension consumers install.
Node 24.18.0 + pnpm 11.10.0: pnpm --filter ./packages/extension build; go -C packages/sdk-go run ./internal/extensionpack --check The clean local build and CI's Linux build artifact produced the same SHA-256; the embedded-extension check passed against that artifact. Confirms the committed Go/Python extension asset is reproducible with CI's release toolchain, rather than merely matching a different local Node version.
Go generator/package checks and tests Generator drift check, vet, example checks, package build, and tests all passed. Confirms the generated Go SDK remains current and the embedded extension is usable from the Go package.

@changeset-bot

changeset-bot Bot commented Aug 5, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 206f811

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@shrey150 shrey150 changed the title fix: avoid snapshot timeouts during iframe churn fix: handle detached frames during snapshots Aug 6, 2026
@shrey150
shrey150 marked this pull request as ready for review August 6, 2026 08:58

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cubic analysis

All reported issues were addressed across 10 files

Linked issue analysis

Linked issue: STG-2781: Harden snapshots against detached iframe races

Status Acceptance criteria Notes
Prevent a detached same-process child frame from retrying an accessibility-tree request against the main CDP session (avoid duplicating the root snapshot). The code classifies frame-scope errors and refuses unscoped fallback for stale child frames; a11yForFrame now checks allowUnscopedFrameFallback before retrying. Unit test covers the scenario.
Permit unscoped Accessibility.getFullAXTree fallback only for the main frame and for live OOPIF session roots. The allowUnscopedFrameFallback predicate is wired where per-frame AX is requested so only main frame or live OOPIF session roots allow unscoped fallback.
Re-check child liveness before and after expensive CDP work and omit frames that detach during capture instead of merging partial/root-duplicated data. collectPerFrameMaps repeatedly checks page.hasFrame and guards around doc-root resolution and after AX capture; detached frames are skipped rather than merged. Unit tests exercise detachment during capture.
Merge only captured frames whose parent path was resolved successfully (avoid including frames without a valid parent prefix). computeFramePrefixes and mergeFramesIntoSnapshot are updated to compute/limit mergeable frame ids and to filter per-frame outlines/maps by the included set before combining.
Preserve live OOPIF behavior and propagate unrelated CDP failures (do not swallow non-frame-scope errors). isFrameScopeError distinguishes frame-scope errors; unrelated CDP errors are rethrown so they propagate. Tests assert unrelated failure propagation.
Add deterministic frame-race unit tests covering detached SPIFs, live/detached OOPIFs, main-frame fallback, unrelated errors, nested omission, and replacement frames. A new comprehensive unit test file targets the listed race scenarios.
Add a bounded real-browser iframe-churn integration test and include it in local integration groups. A snapshot-iframe-churn integration test was added and the integration test groups were updated to include it; the PR describes multiple local runs and stability repetitions.

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread packages/extension/understudy/a11y/snapshot/capture.ts
Comment thread packages/sdk-ts/tests/integration/snapshotIframeChurn.test.ts
Comment thread packages/extension/understudy/a11y/snapshot/a11yTree.ts Outdated

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 7 files (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread packages/sdk-ts/tests/integration/snapshotIframeChurn.test.ts
Comment thread .changeset/tidy-frames-snapshot.md Outdated
@shrey150
shrey150 marked this pull request as draft August 7, 2026 19:23
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.

2 participants