Repository navigation
fix(studio-server): the preview's proxy copy jumps queued thumbnails, and abandoned copies drop - #5340
Conversation
… and abandoned copies drop
Edit accuracy: accurate 2059 (base branch 2059), smooth 1549 of thoseThe gate passes. Quarantined, measured but not gated (0) |
7df575e to
30fa54a
Compare
terencecho
left a comment
terencecho
left a comment
There was a problem hiding this comment.
Request changes at 30fa54ad20dbbc5075e11bea4e082642f6c77020 — one P2 thumbnail-cache regression in the effective two-file diff against 0576b24c75a604deb9c909407b8328b3df17b0c1.
P2 — Capacity refusals must still invalidate thumbnail captures
The new early ProxyCapacityError at packages/studio-server/src/helpers/proxyTranscoder.ts:588 bypasses the activity-epoch update that previously ran on a refused job. This has a consumer beyond the completion statistic: thumbnail.ts:231,247,253–265 persists generated pixels when proxyActivityMark(project.dir) is unchanged, and its disk-cache read at thumbnail.ts:217–221 does not recheck that mark.
With the global queue occupied by another project, the target project's activity mark is non-null. Its proxy request can receive 503 without changing that mark. The preview runtime retries 503 (proxyWait.ts:12–16), but its seek hold is capped (mediaProxy.ts:350); the CLI thumbnail adapter can then screenshot the still-unavailable video (studioServer.ts:733–758). The unchanged marks permit those fallback pixels to be written to the long-lived disk thumbnail cache. Freeing capacity or subsequently finishing a proxy does not change the thumbnail cache key (.transcode-cache is excluded from the project signature), so the next request serves the fallback file rather than regenerating the decoded video frame. It survives recovery until an unrelated input/signature change or cache eviction; pruning uses a 14-day age/size budget, not proxy readiness.
A Chromium differential exercises the actual Studio API, preview runtime, Vite thumbnail generator, target metadata/transcode, signatures, and disk cache, using a real red ProRes fixture. With supported concurrency=1 / queue=0 overrides, only the other-project blocker encode is mocked/held to make saturation deterministic:
- Head: the first capture receives 503 proxy responses; marks stay
0 → 0; the background PNG is persisted. After releasing the blocker and successfully serving the target's real proxy (206), the same thumbnail URL still returns a byte-identical background PNG, RGB(28,32,40). A forced-fresh version returns the correctly decoded red frame, RGB(255,24,0). - Exact base: first-capture marks change
0 → 2; no fallback file is persisted; the same-URL retry after proxy recovery returns the red frame.
The browser regression assertion fails at head and passes at base. A separate real-route differential with modeled screenshot contents has the same result, and I independently reran both of those arms.
The same situation at the defaults requires saturating the other project's two active and eight queued jobs. Same-project in-flight work already returns a null mark and is not the failing case. This is a supported in-repo scenario: Studio's Vite adapter lists and resolves multiple projects (packages/studio/vite.adapter.ts:192–270), while the limiter/accounting module is process-global.
Please preserve a capture-invalidating activity change for a capacity refusal (without negative-caching that refusal), or explicitly prevent thumbnail persistence after unresolved/capacity-refused proxy requests. It is reasonable not to count refusals as finished encodes, but the thumbnail invalidation signal cannot silently lose that event. Add a regression covering cross-project global saturation and the next request after capacity recovers.
Other verified behavior
- Priority ordering/promotion, shared caller leases, pre-abort/immediate-abort, started-job detachment, listener cleanup, and dropped-job/replacement identity passed eight independent exact-source probes. Removing identity-aware deletion produced two concurrent ffmpeg starts for the same key; leaving the dropped entry registered rejected the immediate replacement. These probes use real temp files and mocked external boundaries, not a real encode.
- Studio's preview route intentionally remains on the normal queue because it also serves thumbnails; this is an optional exported API change, not an already-wired visible-preview prioritization feature.
Validation limits
Frozen Bun install and the full workspace build passed. Under NODE_ENV=test, 287 existing tests pass across 10 files: 213 tests in proxyTranscoder, mediaProxyPreview, mediaCodecMap, proxyCache, preview, and thumbnail; plus 74 CLI tests in checkBrowser, publishProxyBake, staticProjectServer, and play. The added real-route differential fails at this head and passes at the exact base; I independently reran both arms and verified that the baseline helper differs only in relocated import paths.
The 11 currently reported required checks pass. The complete CI rollup is not yet fully settled: Studio: timeline viewport gate is running again; the other reported checks are 65 successes and 13 skips. Its earlier attempt failed at this head (job). I have not established the cause of that earlier CI failure or attributed it to this diff.
The browser witness uses a small real ProRes fixture and a deterministically held other-project blocker, not the author's 12-clip timing benchmark. Its target proxy encode, runtime, Vite capture, and PNG decoding are real; only the blocker encode is mocked. The earlier route-only differential models screenshot contents. I did not independently replay the performance benchmark.
— tai
| return promise; | ||
| let entry = inFlight.get(cachePath); | ||
| if (!entry) { | ||
| if (!options.priority && queueIsFull()) throw new ProxyCapacityError(); |
There was a problem hiding this comment.
[P2] Preserve thumbnail invalidation on capacity refusal
This early throw no longer advances the activity mark used by thumbnail.ts to decide whether generated pixels are safe to persist. When another project fills the global queue, the target receives 503 while its mark remains unchanged, so the thumbnail can cache the still-unavailable video/background. Its disk-cache hit runs before the activity check and proxy completion is excluded from the project signature; freeing capacity does not repair that cached image.
A real Chromium/ProRes/Vite-generator differential confirms that this head serves a byte-identical background PNG even after the target proxy serves 206, while the exact base regenerates the red frame on the same URL. Only the other-project blocker encode is mocked. Please retain a capture-invalidating event for capacity refusal (without negative-caching it), or prevent persistence when the proxy is unresolved; add a cross-project saturation/recovery regression. Full evidence and validation limits are in the review body.
There was a problem hiding this comment.
Fixed in b58ad79. You're right: the early refusal skipped the mark bump that the rejected promise's .finally gave it on main. A refused ask now moves the activity mark before throwing (proxyTranscoder.ts:589), and it is still not remembered as a failure. New test "moves another project's activity mark when its ask is refused, and copies it once there is room" (1 slot, queue 0, another project holds the slot): the mark moves on the refusal, and once the slot frees the next ask for that clip starts a copy. Removing the bump turns exactly that test red. I also checked every other path where main moves the mark (cache hit, remembered failure, join, settle, dropped copy, priority over the cap); each matches main or moves it later on settle.
terencecho
left a comment
terencecho
left a comment
There was a problem hiding this comment.
Approve at b58ad79caab4fb0588a75eb532fad27c9d146dde. My previous P2 in review 5473437591 is fixed; no remaining scoped defect found.
The early capacity-refusal branch now advances proxyActivity before throwing (packages/studio-server/src/helpers/proxyTranscoder.ts:588–590). Admitted jobs still advance it on settlement. A target project with no in-flight job therefore sees a changed mark when another project's occupied slot refuses its proxy request, preventing the thumbnail route from persisting that incomplete capture. The refusal is not remembered as a transcode failure, so a later request can start the copy.
Replayed the same real Chromium/ProRes thumbnail-cache witness across four versions:
| Version | Result |
|---|---|
| This head | Pass: mark 0→2, incomplete image not cached; same URL regenerates red after actual target proxy HTTP 206 |
Exact effective base 0576b24c75a604deb9c909407b8328b3df17b0c1 |
Same passing behavior |
Previous head 30fa54ad20dbbc5075e11bea4e082642f6c77020 |
Fails: mark 0→0; cached background remains after proxy recovery |
| This head with only the refusal increment removed | Same failure as the previous head |
The new cross-project test at proxyTranscoder.test.ts:566–584 also fails when only that increment is removed. Focused Studio-server/CLI tests passed 288/288, three additional activity/settlement/retry tests passed, and the full local build passed. Test worktrees are clean and browser/server/fixture cleanup is complete.
Witness limits: a minimal supported-equivalent two-project adapter is used, and only the other project's blocking encode is mocked. Target metadata, FFmpeg transcode, runtime, thumbnail generation, Chromium and pixels are real. This is not a broad stress/load test.
CI snapshot: ten reported required checks pass, including replacement Test/Build/Typecheck/regression and Windows tests/render. Edit-accuracy's older gate row is cancelled while the replacement run's 20 shards are still running; the rollup also retains earlier failed/cancelled run rows. This is a code-merit approval, not a claim that all CI has completed green.
— tai
What changes
resolveProxy(the./proxy-transcoderexport of@hyperframes/studio-server) takes a new optional argument,{ priority?: boolean; signal?: AbortSignal }:priorityputs the copy ahead of every normal queued copy, and a priority ask is never refused because the queue is full. If the same clip is already queued as a normal copy, a priority ask moves that queued copy forward instead of starting a second one.signallets a caller stop waiting. The copy is dropped only when it has not started yet and no caller is left waiting for it. A copy that has started is never stopped for a caller that leaves, and a caller without a signal always keeps its copy.Callers that pass neither get the same results and errors as before, and a refused ask still moves the activity mark Studio's thumbnails use to decide whether a capture is safe to keep. That includes Studio's own preview route: it serves both the visible preview and thumbnail capture pages, and it cannot tell them apart, so giving it priority would let capture copies crowd the lane and push every other ask out. Priority is for a caller that knows which copy is on screen, such as an app embedding the preview.
Why
The transcoder runs 2 copies at a time with 8 queued, first come first served. A screen that asks for a copy of every clip for thumbnails fills that queue, so the copy the timeline preview needs waits behind all of them, or is refused with
ProxyCapacityErrorwhen the queue is full. A thumbnail that has scrolled out of view still holds its queued place. Repro: 12 clips of 20 s 1080p ProRes 422, 11 thumbnail asks, then the preview's ask; the preview's copy was refused at about 20 ms while the thumbnails took about 50 s.How
One wait queue of jobs replaces the queue of callbacks. Each job is the one shared copy for a cache key and counts the callers still waiting for it; a caller without a signal never leaves, so its copy is always made. Priority jobs are inserted before the first normal job. When the last caller leaves a job that is still queued, the job leaves the queue and the in-flight map at once, so an ask for the same clip in the same moment starts a fresh copy, and its promise rejects with an internal error that is not remembered as a failure. A full queue now refuses a normal ask before anything is registered, so a priority ask for the same clip in the same moment is still accepted. The same shape (one shared entry per key, a lease per caller, drop on the last lease) already exists for thumbnails in
routes/thumbnailGenerationCoordinator.ts; that one also stops started work, which a proxy copy must not. The module's existing rule stands: a started ffmpeg child is never killed on a caller's behalf.Priority asks can push the queue past its cap. That is deliberate: they are the copies a viewer is waiting on, and there are few of them.
Verification
packages/studio-server/src/helpers/proxyTranscoder.test.ts, 37 tests, 3 runs in a row, all green, no timers (fake ffmpeg spawn, real temp files). New cases, each shown to fail when its behaviour is removed: