Repository navigation
fix(engine): a script the page cancelled is not a failed load - #7093
Conversation
There was a problem hiding this comment.
Requesting changes at d937f66a. The only blocker is the PR description.
Blocker: the body has no fails without the fix: line, so it doesn't name a test that fails without this change. That line is required for fix PRs. Please add one, for example:
fails without the fix: packages/engine/src/services/frameCapture-subTimelinePoll.test.ts › "a script request the page cancelled" › "is not a load failure: …"
Before you add it, confirm that the test actually fails when the ERR_ABORTED guard is reverted. With the guard gone, the scriptLoadFailures toEqual [] assertion fails, and so does the poll that should end "ready" after the grace period.
Code (no blocker):
- Moving the logic into
recordFailedRequestkeeps the same behavior. The only change is skippingnet::ERR_ABORTEDfor scripts, and the browser diagnostic is still written for those. - The engine doesn't intercept requests itself. The CLI interception path (
captureCompositionFrame.ts:913) aborts with"failed", which shows up asERR_FAILED, so a script the network refused is still counted as a failure. The second test covers this case. - HTTP error statuses never fire
requestfailed, so this change can't affect how they're counted.
d937f66 to
e8d98bc
Compare
There was a problem hiding this comment.
Approving at e8d98bc5. The one blocker from my last review is fixed, and I re-checked the change against #7088, which has now merged.
fails without the fix:line: present, and I confirmed it locally. With theERR_ABORTEDguard removed, the named test fails (× is not a load failure…, 1 failed / 22 passed). With the guard in place, all 23 tests pass.- On top of #7088:
recordFailedRequestpassesclassifyScriptLoadCause(...)through, so failures that still count keep their recorded cause. A cancelled script records no URL and no cause, and its browser diagnostic is still written. - Other tests: the
frameCapture-subTimelinePoll,frameCaptureandframeCapture-subTimelineWarningtests all pass (94 total), andtsc --noEmiton the engine package is clean. - Other failures still count: the engine doesn't intercept requests, and the CLI's own blocking aborts with
"failed", so those scripts are still recorded as failures. HTTP error statuses never firerequestfailed.
No blockers.
Adopted with tests that fail on the old code (except the link change): - offload: rewrite media inside <template>, keep same-named files from other folders (heygen-com#6842, offload part only) - autoUpdate: no background upgrade across a 0.x minor; Homebrew installs only notify (heygen-com#7013, autoUpdate part only) - engine: script load failures carry origin/code; aborted scripts are not load failures (heygen-com#7088, heygen-com#7093) - community Discord invite links (heygen-com#7097) - studio: write media duration to the store once per file (heygen-com#6745) - check: restart the idle watchdog on each driver call during analysis (heygen-com#6919, adapted to this line's fixed 120s idle window) - check: default wait raised from 10s to 45s, matching snapshot/render (heygen-com#6947) - image cancellations no longer ignored by extension (heygen-com#6928) - capture: system sleep no longer counts as a stall (heygen-com#7096, hand-ported; the sequential branch also uses the shared stall clock) Commit watermark advances to 183614b (28 consecutive commits with file:line evidence); PR and issue watermarks are unchanged. The rest of the review is in docs/UPSTREAM_REVIEW_2026-11-04_CONTINUED.md. Co-authored-by: Miguel Ángel <miguel.sierra@heygen.com> Co-authored-by: WaterrrForever <miao.yang@heygen.com> Co-authored-by: Wenbo Zhu <wenbo_zhu@berkeley.edu> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
What
A script request that the page cancels itself (Chromium's
net::ERR_ABORTED, e.g. a frame that is removed or a load the page stops) no longer counts as a failed script load. Before, anyrequestfailedon a script was recorded as a load failure, which starts the sub-composition wait's 2 s fail-fast grace, so a render whose timeline registered a little later failed with "A sub-composition timeline script failed to load" on every attempt. Every other network error and every HTTP error status still counts, and the cancelled request's browser diagnostic is still written.Why
A cancelled request was never refused by any server; treating it as a refusal fails renders that would have completed.
Related work
Refs #7088 (the failure's origin and code). This sits on top of it: the cause it records is kept for every failure that still counts.
How
The
requestfailedhandler moves into an exportedrecordFailedRequest(session, request), which records a script load failure (with its cause) unless the error is the page's own cancel, then writes the diagnostic as before.Test plan
frameCapture-subTimelinePoll.test.ts› "a script request the page cancelled": a cancelled script records no failure and a timeline that registers after the grace ends the waitready; a script the network refused (net::ERR_CONNECTION_RESET) is still recorded.fails without the fix: packages/engine/src/services/frameCapture-subTimelinePoll.test.ts › "a script request the page cancelled" › "is not a load failure: a timeline that registers after the grace still ends the wait ready" (with the cancel check removed: 1 failed, 22 passed).
frameCapture-subTimelinePoll,frameCaptureandframeCapture-subTimelineWarningtests: 94 passed, 3 runs in a row.oxlintandoxfmtclean on the changed files.A repro outside the test (a script cancelled by the page while its timeline registers after the grace) failed every attempt before this change.
Unit tests added/updated
Manual testing performed
Documentation updated (if applicable)
Comments follow CONTRIBUTING.md "Comments": they say why, not what, and a bug fix says what the code must do and how to reproduce the bug
Size
68 changed lines: a lone root fix with its test.