Skip to content

fix(studio-server): a render that ran over 5 minutes can still be read after it finishes - #7087

Merged
miguel-heygen merged 2 commits into
mainfrom
drenderq/render-job-ttl-from-finish
Nov 3, 2026
Merged

miguel-heygen merged 2 commits into
mainfrom
drenderq/render-job-ttl-from-finish

Conversation

@miguel-heygen

@miguel-heygen miguel-heygen commented Nov 3, 2026 •

Copy link
Copy Markdown
Collaborator

What

A finished render or background-removal job now stays readable for 5 minutes after it finishes. Before, the sweep counted those 5 minutes from when the job started, so any render that ran longer than 5 minutes was forgotten within a minute of finishing: GET /render/:jobId/progress answered 404 although the video was on disk, and a client waiting on it reported the export as missing.

Why

The job store's TTL read now - job.createdAt, and createdAt is stamped when the job is created. A long render (a 6-minute export is ordinary) is past the TTL the moment it ends, and the next 60 s sweep deletes it. The same pattern was in the media-processing job store.

Related work

None.

How

  • One helper, helpers/finishedJobs.ts forgetJobsFinishedLongerThan, owns the rule for both stores. The adapter finishes a job by changing it in place (no event reaches the server), so the sweep notes, per job object, the first time it sees it finished, and forgets it once that is older than the TTL. A job is therefore kept at least the TTL after it finishes, and at most one sweep interval longer.
  • routes/render.ts and routes/media.ts use it; createdAt is removed from both job maps (nothing else read it; the renders listing keeps its own createdAt field, from the file's mtime).

Test plan

  • routes/finishedJobs.test.ts (new): a render that runs 6 minutes, finishes, and is read 61 s later answers 200, then 404 once it has been finished for longer than the TTL; the same for a background-removal job.

  • Fails without the fix: with render.ts and media.ts at main and the new test kept, both tests fail (expected 404 to be 200).

  • routes/renderDeletion.test.ts: its render is complete from the start, so it is first seen finished at the 1-minute sweep and now forgotten at 7 minutes instead of 6; the test advances 7 minutes.

  • vitest run of finishedJobs, render, media, render.streaming and renderDeletion tests: 5 files, 122 tests passed, 3 runs in a row. tsc --noEmit in studio-server exit 0. oxlint and oxfmt --check on the changed files: clean.

  • 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

@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

Edit accuracy: accurate 267 (base branch 267), smooth 107 of those

The gate passes.
Smoothness is reported in the artifact, not gated. A case fails only if it fails 2 of 3 runs.

Quarantined, measured but not gated (8)

@heygengenesis heygengenesis Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved at fb588727. No blockers found.

  • The fix is right: the TTL now counts from the first sweep that sees a job finished, not from createdAt. The marker lives in a WeakMap keyed by the job object, which suits an adapter that mutates jobs in place and doesn't leak. A job is kept at least the TTL after it finishes and at most one sweep longer. Both stores share the helper. createdAt is gone from the job maps, and the renders listing still reports its own mtime createdAt.
  • Fails without the fix: confirmed locally. With render.ts and media.ts at main (47ddded), both finishedJobs.test.ts cases fail with expected 404 to be 200. At this head, the finishedJobs, render, media, render.streaming and renderDeletion tests give 5 files and 122 tests passed. tsc --noEmit in studio-server exits 0.
  • Change since 9daa47d4: a rename only (forgetJobsFinishedLongerThan, firstSeenFinishedAt, ttlMs moved first). Both call sites use the new argument order. Behaviour is the same.
  • renderDeletion.test.ts: the move from 6 to 7 minutes follows from the new rule, since the job is first seen finished at the 1-minute sweep.
  • Side effect (fine): on-disk renders registered by the listing used to be dropped at the next sweep because their mtime was old. Now they stay for about 5 minutes, which is in keeping with the comment that they "remain downloadable".

CI: both CI runs at this head were cancelled because the Build job hit its 10-minute limit inside bun run verify:packed-manifests. That was not a test failure. The same timeout hit the earlier head and an unrelated branch (miao/templates-unpack-stdin), so it looks like runner slowness, not this change. I re-ran the failed jobs on run 41635493264. Merge should wait for that rerun to go green.

@miguel-heygen
miguel-heygen added this pull request to the merge queue Nov 3, 2026
Merged via the queue into main with commit fc006f7 Nov 3, 2026
138 of 144 checks passed
@miguel-heygen
miguel-heygen deleted the drenderq/render-job-ttl-from-finish branch November 3, 2026 14:40
@WaterrrForever WaterrrForever mentioned this pull request Nov 3, 2026
1 task done
SanHsien added a commit to SanHsien/hyperframes that referenced this pull request Nov 4, 2026
…r long runs

Ports upstream heygen-com#7087 (fc006f7): the finished-job TTL now counts from
when a sweep first sees the job finished, not from createdAt, so a render
that ran longer than the TTL is still readable after it completes.
Adds the shared helper and its regression tests, which failed on the old
code with 404.

Also records the fourth 2026-11-04 upstream review pass: watermark
advanced to 3f42dc1 / PR heygen-com#7077 with file:line evidence; the remaining
range stays in ROADMAP.

Co-authored-by: Miguel Ángel <[email protected]>
Co-Authored-By: Claude Opus 5.5 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant