fix(media-use): a slow download no longer leaves an empty asset that undo brings back - #4989
Conversation
Edit accuracy: accurate 1556 (base branch 1556), smooth 1447 of thoseThe gate passes. Quarantined, measured but not gated (0) |
jrusso1020
left a comment
There was a problem hiding this comment.
Approving at e8c2e6c4.
The fix holds. The empty reservation file used to sit at the asset's final name. Now it is an empty marker named <asset>.hfXXXXXX.tmp. That matches core's TEMP_SUFFIX (/\.hf[0-9a-f]{6}\.tmp$/), which both projectSignature (affectsProjectSignature and isSkippedEntry) and the CLI fileWatcher skip, so history never commits the marker. nextFreeId matches ^<type>_(\d+) against every file in the type folder. The marker starts with the id, so it still holds that id while the download runs, and the new test confirms a second allocateId returns bgm_002.
Every caller. Every reservation goes through withReservedFile or withReservedFileSync. Those are the three cache-import sites in resolve.mjs, the two freezeUrl/freezeLocalFile sites, the parametric LUT, and both LUT paths in lut-preset-provider.mjs. So every path releases the marker. None of the writers relied on the placeholder already existing at the final path:
writeFrozenuseswriteFileSync;- the cache import uses
copyFileSyncwith no EXCL flag; - the LUT paths rename into place.
In reuse --sha, the process.exit runs only after withReservedFileSync has returned, so the marker is already gone by then. skills/media-use/scripts/lib/manifest.mjs is byte-identical to the CLI copy.
SKILL.md line. It matches references/setup-providers.md:9-12: heygen auth login --oauth gives the free allowance for TTS and avatar video, and an API key bills API credits. It states no amounts.
Verified locally:
- The CI skills job's exact file set (
skills+packages/cli/src/media-use*.test.mjs+ the two scripts, withtest-home.mjsandHYPERFRAMES_MEDIA_HOME_REQUIRED=1): 774 tests, 772 pass, 0 fail. The import-boundary check passes (2/2), and so does the new vitest file (2/2). - Mutations, each reverted after:
- naming the marker
.reservedfails the first new test; - dropping the partial-file cleanup on failure fails the second;
- putting the empty file back at the final name fails both the first new test and the MU-23 test in
manifest.test.mjs.
- naming the marker
Nonblocking. If the process dies mid-download, the .tmp marker is left behind and that id gets skipped from then on. That is still better than before: history ignores the marker, while the old leftover was a 0-byte asset history would commit.
— Rames
e8c2e6c to
4bd8497
Compare
jrusso1020
left a comment
There was a problem hiding this comment.
Re-approving at 4bd8497c, which rebases the e8c2e6c4 head I approved onto main after #4988.
- Range-diff against
e8c2e6c4: all four commits are unchanged except for theskills-manifest.jsonmedia-use hash, which each commit regenerates.manifest.mjsis byte-identical to the approved version, and the skills copy still matches it. - Hash:
bun packages/cli/scripts/gen-skills-manifest.ts --checkreports it in sync (21 skills). - Interaction with #4988: the only other media-use change between the two heads is #4988's
heygen.mjs.envfix, which came in from main. It doesn't touchmanifest.mjsor anything that uses the reservation. - Verified locally at this head: I ran the CI skills job's exact file set (775 tests, 773 pass, 0 fail, 2 skipped; one more test than before, from #4988). The new vitest file passes 2/2.
— Rames
4bd8497 to
0eb7ee9
Compare
jrusso1020
left a comment
There was a problem hiding this comment.
Re-approving at 0eb7ee9b, which rebases 4bd8497c (approved) onto main after #4995.
- Range-diff against
4bd8497c: all four commits are unchanged except the regeneratedskills-manifest.jsonmedia-use hash. Apart from that hash, the two heads differ only in #4995'sheygen.mjscredentials-folder fix, which came in from main.manifest.mjsis still byte-identical to the approved version and to its skills copy. - Hash:
gen-skills-manifest.ts --checkreports it in sync (21 skills). - Verified locally at this head: I ran the CI skills job's exact file set: 777 tests, 775 pass, 0 fail, 2 skipped. The two extra tests come from #4995. The new vitest file passes 2/2.
— Rames
What
media-use no longer puts an empty file at an asset's final name while it downloads. The id is still reserved on disk, but now by an empty marker under an atomic temp name (
<asset>.hfXXXXXX.tmp), which project history skips. The asset appears under its real name only when it is written, and the marker is removed when the reservation commits or rolls back.Also:
skills/media-use/SKILL.mdnow tells the agent to tell the person, before generating a voiceover or an avatar video, that signing in to the heygen CLI with OAuth (heygen auth login --oauth) gives a free allowance for TTS voiceover and avatar videos, while an API key bills API credits. This comes fromreferences/setup-providers.md; no allowance amounts are stated.Why
allocateIdreserved an id by creating an empty file at the final path (.media/<type>/<id><ext>, flagwx) before the download. Studio's project history does not skip.media, and it commits outside changes afterquietMs(2 s by default). When the download took longer, history committed the 0-byte file as an outside change; undoing past it later restored an empty asset.Related work
Same shape as #4982 (freeze-frame stills), which moved the still's reservation to a temp name that history skips.
How
allocateId(media-use/lib/manifest.mjs): under the existing lock, creates the marker<final path>.hf<6 hex>.tmpwithwx, the same suffix as core'satomicTempPath. Studio'sprojectSignatureand the CLI file watcher skip it viaisAtomicTempPath.nextFreeIdstill counts it (its name starts with the id), so a concurrent resolve cannot take the same id during the download.withReservedFile/withReservedFileSync: a committed reservation removes only the marker; a failed or empty one removes the marker and any partial asset at the final path.freezeUrlbuffers the whole body and writes once, cache imports copy a complete file, and the LUT paths already rename a validated file into place.skills/media-useand cannot import@hyperframes/core, so the suffix is written out here and pinned by a test against core'sisAtomicTempPath.skills/media-use/scripts/lib/manifest.mjsis the byte-identical copy the parity check requires.manifest.d.mtsdeclares the three exports the new TypeScript test uses, likeconfig-lock.d.mts.Test plan
src/media-use/manifestReservation.test.ts(vitest, outsidelib/so the build does not ship it, imports core's realisAtomicTempPath): while a reservation is being filled, the type folder holds nothing history would commit, and a secondallocateIdgets the next id; afterwards only the asset remains. Fails on main (expected [ 'bgm_001.wav' ] to deeply equal []). It also fails when the marker uses a name core does not treat as temp (bgm_001.wav.reserved). A second test writes a partial file and then fails the download: the folder is left empty. It fails if the rollback stops removing the partial file.manifest.test.mjs: the MU-23 test now checks that the marker holds the id and that nothing sits at the final name; the LUT allocation test checks the marker.node:testfiles plus the copy-parity check (the CI skills job's set): 772 pass, 0 fail, 3 runs in a row. The vitest media-use tests pass 3 times. Linux, Node 22.tsc --noEmit, oxlint, oxfmt and the pre-commit hooks (fallow, skills manifest) are clean.Not exercised: a Studio walk with a real slow download. The test uses the same
isAtomicTempPathcheck that history'sprojectSignatureuses.Size
One reservation change in
manifest.mjs(plus its skills copy), one declaration file, two new tests, two updated assertions, one SKILL.md line.