Repository navigation
fix(check): the analysis restarts the idle window with every call it makes - #6919
Conversation
There was a problem hiding this comment.
Verdict: APPROVE · 0 blocker · 1 important · 0 nits · head 16d930f4
- important: the new tests pin
reportingProgressand theanalysislabel on their own, but nothing checks thatrunAuditGridactually uses the wrapper. If the analysis section went back to the baredriver, the timeout from the report would return and every test would still pass.check.test.ts:2269–2358already runsrunAuditGrid(..., deadline!.enter)with fake timers. One case there whose motion/craft analysis makes several slow driver calls (together longer than the idle window, each shorter) would pin the fix end to end. A follow-up is fine.
Scope
Read the full diff, CheckDeadline.enter/rearm, runAuditGrid and craftEnabled. The proxy calls the method on the real driver object (member.apply(target)), so private state and calls the driver makes on itself behave as before, and typeof analysing.collectCraftFrame === "function" still holds. The only raw-driver call left after analysis is matchComponents, a single call that the window covers. The fails without the fix: line is present. No prior reviews. Required checks were still running at review time; none had failed.
Edit accuracy: accurate 267 (base branch 267), smooth 105 of thoseThe gate passes. Quarantined, measured but not gated (8)
Unstable (1)
|
|
Added at 8d5201b, in check.test.ts's runAuditGrid fake-timer block: 'slow analysis calls that each finish keep the check alive past 120 s'. After the grid it makes hasNoTimelineDeclaration and getCanvas each take 100 s (slow only once report('analyze') fires, so the sample phase is unaffected); together they outlast the 120 s window, so the check survives only if every analysis call restarts it. Proved: 157 tests green x3 with the wrapper; with 'const analysing = reportingProgress(driver, enter)' replaced by 'const analysing = driver', exactly that test fails (check_runtime_failure). fmt and tsc clean. |
There was a problem hiding this comment.
Verdict: APPROVE — 1 resolved, 0 open, 0 new · 16d930f4 → 8d5201ba. ✅ The wiring gap is closed. I checked locally: check.test.ts and checkPipeline.reportingProgress.test.ts pass (160/160), and with runAuditGrid switched back to the bare driver, "slow analysis calls that each finish keep the check alive past 120 s" fails.
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
After the samples are collected,
checknow marks ananalysisphase ("analyzing the samples") and every call the analysis makes to the page (re-seeks, frame captures, craft sampling, contrast and clock reads) restarts the idle window, as the sample loop already does. A stalled analysis ends with... while analyzing the samples (<call>)instead of... while checking samples.Why
A
check --json --at 0,1,2,3on static native 3D poses finished every sampled frame and contrast pass and then ended withno progress for 120 s after Chrome was ready, while checking samples. The window is restarted only when a phase is entered or an operation settles: the sample loop enterssamplesper sample, but the analysis that follows (withFrameMotion/markFrameChanges,framesMoved, the craft audit and itsseekGeometry(..., { draws: true })re-seeks) runs many seeks and captures with no restart, all inside the lastsamplesentry. On a 3D scene each draw is slow, so the sum passes 120 s although no single step does, and the error names the wrong phase.How
reportingProgress(driver, enter)wraps the audit driver in aProxythat callsenter("analysis", <method>)before each call;runAuditGriduses it for the whole analysis section.analysisis added to the deadline's phase labels. A step that makes no call for the whole window still ends the check, now under the right name.Not done: I could not produce a >120 s analysis on my machines, so the timing is covered with fake timers rather than a real scene, and a single call that itself runs longer than the window (one very slow draw) is still ended by it. The other report in this tick (
The turn check stopped: the shell left) is not an OSS message.Test plan
fails without the fix:
checkPipeline.reportingProgress.test.ts"restarts the idle window on every call the analysis makes" (reportingProgress is not a functionat main).checkDeadline,checkBrowserandcheckProgresstests: 83 passed, 3 runs in a row (devbox); both behavioural new tests fail at main. Fake timers, no real waits.Size
About 75 changed lines: one wrapper, one phase label and a test file.