Repository navigation
fix(engine): a composition whose script throws is skipped with a warning, not refused #6156
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
884a410
fa1476d
e74596a
813e979
bf3614e
98e37ec
141f08f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| import { spawnSync } from "node:child_process"; | ||
| import { afterAll, beforeAll, describe, expect, it } from "vitest"; | ||
| import type { Browser, Page } from "puppeteer-core"; | ||
| import { resolveHeadlessShellPath } from "./browserManager.js"; | ||
| import { recordSubTimelineWarning, waitForSubCompositionTimelines } from "./frameCapture.js"; | ||
| import type { CaptureSession } from "./frameCapture.js"; | ||
| import { closeTestChrome, launchTestChrome } from "./__fixtures__/loopback.js"; | ||
|
|
||
| const chromePath = resolveHeadlessShellPath(); | ||
| const hasBrowser = chromePath !== undefined && spawnSync(chromePath, ["--version"]).status === 0; | ||
|
|
||
| // A stand-in for gsap: the isolation only needs timeline() and set() on what it returns. | ||
| const FAKE_GSAP = `window.gsap = { timeline: function () { var tl = { total: 0, set: function (a, b, at) { tl.total = at; return tl; } }; return tl; } };`; | ||
|
|
||
| function sessionFor(page: Page, errors: string[]): CaptureSession { | ||
| return { | ||
| page, | ||
| pageErrors: errors, | ||
| scriptLoadFailures: [], | ||
| warnings: [], | ||
| subTimelineWaitMemo: {}, | ||
| } as unknown as CaptureSession; | ||
| } | ||
|
|
||
| describe.skipIf(!hasBrowser)("a composition script that throws", () => { | ||
| let browser: Browser; | ||
| beforeAll(async () => { | ||
| browser = await launchTestChrome(chromePath); | ||
| }); | ||
| afterAll(async () => { | ||
| await closeTestChrome(browser); | ||
| }); | ||
|
|
||
| it("is skipped with a named warning instead of waiting out the timeout and refusing", async () => { | ||
| const page = await browser.newPage(); | ||
| await page.setContent( | ||
| `<script>${FAKE_GSAP}</script> | ||
| <div id="chart" data-composition-id="root" data-duration="4"></div> | ||
| <script>window.addEventListener("error", function (e) { window.__errors = (window.__errors || []).concat(String(e.message)); });</script> | ||
| <script>document.getElementById("missing").appendChild(document.createElement("i"));</script>`, | ||
| ); | ||
| const session = sessionFor(page, [ | ||
| "TypeError: Cannot read properties of null (reading 'appendChild')", | ||
| ]); | ||
| const started = Date.now(); | ||
| await waitForSubCompositionTimelines(session, page, 60_000, { scriptErrorGraceMs: 100 }); | ||
| expect(Date.now() - started).toBeLessThan(10_000); | ||
|
|
||
| expect(session.isolatedCompositions).toEqual([{ compositionId: "root", elementId: "chart" }]); | ||
| const registered = await page.evaluate( | ||
| `(function () { var t = window.__timelines && window.__timelines.root; return t ? t.total : null; })()`, | ||
| ); | ||
| expect(registered).toBe(4); | ||
|
|
||
| recordSubTimelineWarning(session, 60_000); | ||
| expect(session.warnings.map((w) => w.code)).toEqual(["script_error_isolated"]); | ||
| expect(session.warnings[0]?.details?.isolatedElements).toEqual([ | ||
| { compositionId: "root", elementId: "chart" }, | ||
| ]); | ||
| await page.close(); | ||
| }, 30_000); | ||
|
|
||
| it("leaves a composition whose timeline registered alone", async () => { | ||
| const page = await browser.newPage(); | ||
| await page.setContent( | ||
| `<script>${FAKE_GSAP}</script> | ||
| <div data-composition-id="root" data-duration="2"></div> | ||
| <script>window.__timelines = { root: {} };</script>`, | ||
| ); | ||
| const session = sessionFor(page, []); | ||
| await waitForSubCompositionTimelines(session, page, 5_000); | ||
| expect(session.isolatedCompositions).toBeUndefined(); | ||
| await page.close(); | ||
| }, 30_000); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -207,6 +207,7 @@ export interface CaptureSession { | |
| * our console output. | ||
| */ | ||
| pendingTimelineIds?: string[]; | ||
| isolatedCompositions?: IsolatedComposition[]; | ||
| /** The render's shared memo when the caller passed one, else this session's own. */ | ||
| subTimelineWaitMemo: SubTimelineWaitMemo; | ||
| /** Structured readiness warnings surfaced to the producer's render policy. */ | ||
|
|
@@ -1820,20 +1821,52 @@ async function listUnregisteredTimelineIds(page: Page): Promise<string[]> { | |
| return Array.isArray(evaluated) ? evaluated.map((id) => String(id)) : []; | ||
| } | ||
|
|
||
| function isScriptFailure(session: CaptureSession): boolean { | ||
| const outcome = session.subTimelineWaitOutcome; | ||
| return outcome === "script_failure" || (outcome === "timeout" && session.pageErrors.length > 0); | ||
| } | ||
|
|
||
| /** A composition whose script failed gets an empty paused timeline of its authored length, so the render goes on. */ | ||
| async function isolateUnregisteredCompositions(page: Page): Promise<IsolatedComposition[]> { | ||
| const isolated = await page.evaluate(`(function() { | ||
| var timelines = (window.__timelines = window.__timelines || {}); | ||
| var out = []; | ||
| if (!window.gsap) return out; | ||
| document.querySelectorAll("[data-composition-id]").forEach(function(host) { | ||
| var id = host.getAttribute("data-composition-id"); | ||
| if (!id || timelines[id] || host.hasAttribute("data-no-timeline")) return; | ||
| var tl = window.gsap.timeline({ paused: true }); | ||
| var duration = parseFloat(host.getAttribute("data-duration")); | ||
| if (duration > 0) tl.set({}, {}, duration); | ||
| timelines[id] = tl; | ||
| out.push({ compositionId: id, elementId: host.id || host.getAttribute("data-hf-id") || null }); | ||
| }); | ||
| if (out.length && typeof window.__hfForceTimelineRebind === "function") window.__hfForceTimelineRebind(); | ||
| return out; | ||
| })()`); | ||
| return Array.isArray(isolated) ? (isolated as IsolatedComposition[]) : []; | ||
| } | ||
|
|
||
| export async function waitForSubCompositionTimelines( | ||
| session: CaptureSession, | ||
| page: Page, | ||
| timeoutMs: number, | ||
| options: { scriptErrorGraceMs?: number } = {}, | ||
| ): Promise<void> { | ||
| const memo = session.subTimelineWaitMemo; | ||
| session.subTimelineWaitOutcome = await pollSubCompositionTimelines(page, timeoutMs, { | ||
| getScriptLoadFailures: () => session.scriptLoadFailures, | ||
| getPageErrors: () => session.pageErrors, | ||
| scriptErrorGraceMs: options.scriptErrorGraceMs, | ||
| onPending: (ids) => { | ||
| session.pendingTimelineIds = [...ids]; | ||
| }, | ||
| shouldStop: () => session.vfxFailure !== undefined, | ||
| knownUnregisteredIds: memo.unregisteredIds, | ||
| }); | ||
| if (isScriptFailure(session)) { | ||
| session.isolatedCompositions = await isolateUnregisteredCompositions(page); | ||
| } | ||
| // Only a clean full-length timeout is replayed: a VFX stop ends early, and page or script | ||
| // errors must reach each session so it can still classify the wait as a script failure. | ||
| const cleanTimeout = | ||
|
|
@@ -1846,12 +1879,20 @@ export async function waitForSubCompositionTimelines( | |
| } | ||
| } | ||
|
|
||
| export interface IsolatedComposition { | ||
| compositionId: string; | ||
| elementId: string | null; | ||
| } | ||
|
|
||
| export interface SubTimelinePollOptions { | ||
| intervalMs?: number; | ||
| // Fail-fast: a script resource that failed to load can never register its timeline, | ||
| // so once failures exist the poll ends `scriptFailureGraceMs` after it started. | ||
| getScriptLoadFailures?: () => readonly string[]; | ||
| scriptFailureGraceMs?: number; | ||
| // A script that threw can never register its timeline either; waits this long once a page error exists. | ||
| getPageErrors?: () => readonly string[]; | ||
| scriptErrorGraceMs?: number; | ||
| // Receives the ids still unregistered at bail time, for the structured warning. | ||
| onPending?: (ids: readonly string[]) => void; | ||
| // Ends the wait at once; the caller then fails the render for its own reason. | ||
|
|
@@ -1867,6 +1908,8 @@ export async function pollSubCompositionTimelines( | |
| intervalMs = 150, | ||
| getScriptLoadFailures, | ||
| scriptFailureGraceMs = 2_000, | ||
| getPageErrors, | ||
| scriptErrorGraceMs = 5_000, | ||
| onPending, | ||
| shouldStop, | ||
| knownUnregisteredIds = [], | ||
|
|
@@ -1899,13 +1942,16 @@ export async function pollSubCompositionTimelines( | |
| const now = Date.now(); | ||
| if (now >= deadline) break; | ||
| const failures = getScriptLoadFailures?.() ?? []; | ||
| if (failures.length > 0 && now - start >= scriptFailureGraceMs) { | ||
| const errors = getPageErrors?.() ?? []; | ||
| const failed = failures.length > 0 ? failures : errors; | ||
| const graceMs = failures.length > 0 ? scriptFailureGraceMs : scriptErrorGraceMs; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. important (new): With a root that registers late, this 5 s grace turns a render that used to succeed into a refused one. Take a root that builds its timeline after an async |
||
| if (failed.length > 0 && now - start >= graceMs) { | ||
| scriptFailureBail = true; | ||
| console.warn( | ||
| `[FrameCapture] Sub-composition timeline wait cut short after ${now - start}ms: ` + | ||
| `script resource(s) failed to load (${failures.join(", ")}), so ` + | ||
| `script(s) failed (${failed.join(", ")}), so ` + | ||
| `the timeline registration they carry can never arrive. ` + | ||
| `Fix the script reference; the render proceeds without those animations.`, | ||
| `Fix the script; the render proceeds without those animations.`, | ||
| ); | ||
| break; | ||
| } | ||
|
|
@@ -2138,10 +2184,28 @@ export function recordSubTimelineWarning(session: CaptureSession, timeoutMs: num | |
| const outcome = session.subTimelineWaitOutcome; | ||
| if (outcome === "ready" || !outcome) return; | ||
| const threwThenTimedOut = outcome === "timeout" && session.pageErrors.length > 0; | ||
| const scriptFailure = outcome === "script_failure" || threwThenTimedOut; | ||
| const scriptFailure = isScriptFailure(session); | ||
| const sources = [...session.scriptLoadFailures, ...session.pageErrors]; | ||
| const pending = session.pendingTimelineIds ?? []; | ||
| const pendingSuffix = pending.length > 0 ? ` (still unregistered: ${pending.join(", ")})` : ""; | ||
| const isolated = session.isolatedCompositions ?? []; | ||
| if (isolated.length > 0) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. important: No test runs the real producer path with a gsap-bearing composition whose script throws. The engine browser test calls the two helpers directly with a fake gsap, so nothing checks end to end that |
||
| recordCaptureWarnings(session, [ | ||
| { | ||
| code: "script_error_isolated", | ||
| message: | ||
| `A composition script failed, so its animation was skipped and the render went on: ` + | ||
| `${isolated.map((c) => c.elementId ?? c.compositionId).join(", ")} (${sources.join(", ")}).`, | ||
| details: { | ||
| sources, | ||
| pendingCompositionIds: [...pending], | ||
| isolatedElements: isolated, | ||
| failureOwner: "user", | ||
| }, | ||
| }, | ||
| ]); | ||
| return; | ||
| } | ||
| recordCaptureWarnings(session, [ | ||
| { | ||
| code: scriptFailure ? "sub_timeline_script_failure" : "sub_timeline_readiness_timeout", | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
blocker: Isolation turns every
script_failureinto the non-blockingscript_error_isolated. Two cases now ship a broken video with exit 0:script_failure(the load-failure path), so it gets isolated too. The PR body only describes scripts that throw.root, gets isolated and the render goes ahead.The #3352 regression tests in
scriptFailureAttribution.test.tsstill pass only because their fixtures never load gsap, soisolateUnregisteredCompositionsreturns[]and the old blocking code still fires.Suggested fix: limit isolation to page errors (leave load failures blocking). Also keep
sub_timeline_script_failurewhen the root composition is isolated, or when no composition with a real timeline is left. If dropping the #3352 policy is intended, the PR should say so and update the orchestrator comment and its test.