perf(studio-server): coordinate cancelable thumbnail generation - #2720
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
3146b27 to
e0f7082
Compare
b29a6d5 to
a73e142
Compare
e0f7082 to
780c625
Compare
a73e142 to
1d0490e
Compare
780c625 to
48b314a
Compare
1d0490e to
94c7b9b
Compare
0d4470c to
42b929a
Compare
94c7b9b to
e5af1ad
Compare
42b929a to
c360755
Compare
e5af1ad to
8fdacf4
Compare
c360755 to
f175d42
Compare
f175d42 to
e2d73cd
Compare
e2d73cd to
4ffcc20
Compare
121042e to
958be61
Compare
4ffcc20 to
25688be
Compare
958be61 to
425417c
Compare
25688be to
4076082
Compare
4076082 to
57d0a4b
Compare
57d0a4b to
686daf0
Compare
jrusso1020
left a comment
There was a problem hiding this comment.
Approving at 686daf0222. All 8 required contexts green at this exact head, enumerated from the branch ruleset with cancelled filtered before taking the latest per context; only the non-required Graphite / mergeability_check is outstanding.
Verified the consolidation claim before reviewing the delta. 8e4e16ac6b (#2719's head) is an ancestor of this head — compare reports ahead: 1, behind: 0. So this PR really does contain all of Family G, and the plan to merge only #2720 lands the whole family rather than dropping anything. I reviewed the 17-file server-side delta as the new surface.
The generation coordinator
The tricky paths hold up. I checked the three that usually break in a dedupe-plus-abort design:
- Replacement entries under the same key.
release()guards withthis.entries.get(entry.key) === entrybefore deleting, so a dying entry can't evict a successor that took the same key. The earlier!this.entries.has(...)bail is the fast path and the identity check is the correct one behind it. - The aborted-but-still-running window.
protectedKeys()unionsentrieswithactiveEntries, which is exactly the state an entry occupies afterrelease()removed it from the map but beforerun()'sfinallylands. Without that union the cache pruner could delete a file a live generation is about to write. - Abort producing the right status. The lease's
onAbortrejects before the adapter'snullreturn propagates, so a disconnected client gets499rather than the500"Chrome browser may not be available". Worth noting because the adapter returnsnullon abort rather than throwing, so the correct status depends on that ordering rather than on the adapter's own signal handling.
writeThumbnailAtomically is right: wx then rename, and the finally rmSync is a no-op once the rename succeeded. adapter.generateThumbnail! is guarded by the early return at the top of the handler; the assertion is only needed because narrowing is lost inside the async closure.
In the Puppeteer adapter, closePage is registered before page is assigned, so an abort landing during newPage() no-ops on the listener and the finally still closes the page once it resolves. No orphaned page on that race.
One contract change worth stating plainly
JPEG requests that don't pass output now default to the bounded preview rather than source density. That's the intent, and the cache key carries both outputMode and the output dimensions so nothing mixes across the change. The audit that matters is the full-fidelity consumers, and both are handled: frameCapture and studioSelectionSnapshot opt into output=source explicitly. They were already on the PNG path that defaults to source, so being explicit there is belt-and-braces rather than a fix, which is the right call if the PNG default ever moves.
Observation rather than a request: the 240x135 cap sits against THUMBNAIL_CLIP_HEIGHT = 66, so it covers a 2x display almost exactly (132 of 135) and upscales roughly 1.5x on a 3x one. That reads as the deliberate tradeoff the PR is making, I just wanted the number written down somewhere before it becomes a "why did timeline thumbnails get soft" question later.
Merge-order note
Since every PR in the family independently targets main and this one contains the rest, merging it alone is coherent. One operational consequence: this repo allows squash merges, and a squash lands a new commit rather than the family's own commits, so #2716 through #2719 would not auto-close on merge and would need closing by hand. A merge commit or rebase would close them naturally. Nothing blocking, just worth knowing before you pick the button.
— Rames Jusso
vanceingalls
left a comment
There was a problem hiding this comment.
R1 review at 686daf0 — tip commit perf(studio-server): coordinate cancelable thumbnail generation on top of #2719s head. Consolidated Family G tip; merge target per Miguel.
Tip-commit scope (isolated diff from 8e4e16ac)
thumbnailGenerationCoordinator.ts(new, +129) — same-key dedup, per-lease AbortSignal, queue with concurrency=1, single owner for cancellation on last-lease dropthumbnailOutput.ts(new, +20) —thumbnailDeviceScaleFactorshared helper exported throughstudio-server/index.tsthumbnail.tsroute +115/-16 — coordinator wired in;writeThumbnailAtomically(temp+rename+rmSyncfinally withforce:true);pruneThumbnailCacherespectscoordinator.protectedKeys()so in-flight cachePath is never evictedtypes.ts— adapter contract gainsoutputWidth,outputHeight,signal: AbortSignalvite.browser.ts+180/-137 — removes in-process_thumbnailInflight(dedup delegated to coordinator), removes local content-hash cacheKey (centralized in the route), wiressignal.addEventListener("abort", closePage, { once: true })with pairedremoveEventListenerinfinally, widens Chrome resolution (Chromium/Brave/Edge on darwin; more Linux paths; Puppeteer cache-dir search;PUPPETEER_EXECUTABLE_PATH/CHROME_PATH/CHROME_BINoverrides)packages/cli/src/server/studioServer.ts+18/-5 — mirror of the abort/close pattern in the CLI adapter; adoptsthumbnailDeviceScaleFactorvite.thumbnail.ts— minorThumbnailPreviewPagetype tightening viaPick<Page, "evaluate">
Lens sweep (non-React server-side surface — lenses 2-5, 9-10, 11 largely N/A)
- Silent-catch (lens 1): every catch has a signal-aware
!signal.abortedguard aroundconsole.warn, or is an idempotent cleanup (page?.close().catch(() => {}),rmSync({force: true}),pruneThumbnailCacheunlink race). All justified. - Retainer-leak / listener-pair (adapted DOM/SPA lens 4):
signal.addEventListener("abort", closePage, { once: true })paired withsignal.removeEventListener("abort", closePage)infinally— in bothvite.browser.tsand CLIstudioServer.ts. Coordinator does the same in its ownlease()(onAbortremoved inrelease()). No dangling listeners. - Race audit on coordinator (adversarial):
signal.abortedearly return before any entry mutation ✓- Last-lease abort:
release()decrements, aborts controller, deletes entries, and — ifstate === "queued"— splices from queue and rejects the shared promise; theentries.get(key) === entryguard prevents cross-deleting a same-key re-inserted entry ✓ run()finally uses the same identity guard beforeentries.delete; new same-key acquire betweenentry.resolveand finally correctly piggybacks on the resolved promise (tested) ✓pump()skips zero-lease entries as belt-and-suspenders even thoughrelease()splices them out- Test
does not attach a new lease to work already aborted by its final leasecovers the trickiest edge ✓
- Atomic-write correctness:
writeFileSync({flag:"wx"})+renameSync+rmSync({force:true})finally. Randomized temp path (.${pid}.${randomUUID()}.tmp) so concurrent writers on the same cachePath dont collide, and the coordinator gates same-key work anyway.renameSyncis atomic on same-FS POSIX/Windows. Testdeduplicates concurrent generation and writes one complete cache entryasserts no.tmpleftover. - Cache invalidation (lens 7 / regression fix):
sourceKey = sha1(html)is now ALWAYS computed even when explicitw/hwere supplied — the prior code skipped the HTML content-hash on Studio-issued requests (which always pass dimensions) and served stale thumbnails after edits. Theregenerates when the composition HTML changes even with explicit w/htest pins this. Nice pre-emptive follow-up on the previous stale-thumb path. - Device-scale semantic delta (lens 12): the JPEG scale factor moves from
format === "png" ? 1 : 0.5tomin(1, outputWidth/width, outputHeight/height)— for standard 240×135 previews of 1920×1080 comps, that is 0.125 instead of 0.5. Smaller memory footprint, potentially slightly softer thumbs. PR body covers this as "expose the shared device-scale contract", and PNG-in-source-mode is preserved (outputWidth=width, outputHeight=height ⇒ scale=1). Callable out but not a blocker; the direction matches the perf thesis. - PR body vs diff (lens 12): every body claim maps to code; the Chrome-path widening in
vite.browser.tsis not called out in the body but is orthogonal enhancement, not a silent behavior delta on the coordinator path.
Gates
- Required CI at
686daf0: all 8 canonical checks green (Build,Render on windows-latest,Semantic PR title,Test,Test: runtime contract,Tests on windows-latest,Typecheck,regression) - 0 unresolved review threads
- Parent chain sits directly on
8e4e16ac(#2719 head)
Ship-ready as the consolidated Family G tip.
— Via

What
Family G — Adaptive thumbnails.
Coordinate cancelable thumbnail generation in Studio Server and the browser bridge.
Why
Client-side virtualization is insufficient if duplicate or obsolete thumbnail requests continue consuming browser and server resources.
How
Deduplicate generation by request identity, own cancellation and cleanup in one server coordinator, make output writes atomic, expose the shared device-scale contract, and propagate aborts through the CLI/Vite bridge.
Test plan
Validated with 78 changed-surface Studio tests, 19 focused server tests, 3,026 full Studio tests, 406 full Studio Server tests, both typechecks, format, lint, file-size gate, and full workspace build.