Repository navigation
fix(media-use): clean up temporary TTS text directories - #5477
Conversation
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed at 7c5e306a. The change is correct and I found no blocker. I'm commenting instead of approving only because CI hasn't run: all 8 workflows are waiting on maintainer approval (action_required). I'll approve once they're green.
The fix. withTmpText creates the hf-txt-* folder, writes line.txt, awaits run(p), and removes the folder in a finally. The ElevenLabs and Kokoro paths both go through it. Because the spawn is awaited inside it, the folder can't be removed while the subprocess is still reading the file. Failures behave as before:
- An error from
mkdtempSyncorwriteFileSync, or a thrownspawn, still rejectssynthesizeOnewith the same error. - A nonzero exit, missing binary or missing output still resolves through
synthResult. - An
rmSyncerror is swallowed, so it can't replace the result or the original error.
writeTmpText had no other callers.
The test is collected. Test: skills runs every skills/**/*.test.mjs (find skills … -name "*.test.mjs"), and its path filter includes skills/**.
The monkeypatching takes effect. The child patches the node:fs and node:child_process module objects, calls syncBuiltinESMExports(), and only then imports tts.mjs. That updates the named bindings tts.mjs imports. The tests show it:
allocation,writeandspawn-throwreject with the exact injected error object.cleanupleaves exactly one folder behind.
Neither could happen if the patches didn't reach tts.mjs.
Mutations, locally:
- With
main'stts.mjs, 13/17 fail. The 4 that pass are the twoallocationand twocleanupcases, which can't see a leak. - Letting the cleanup error propagate (dropping the inner
catch) fails bothcleanupcases.
Flakiness.
- 17/17 passed in 5 back-to-back runs, about 1.1 s each.
- Each case runs in its own child with its own
TMPDIR,HOMEand aPATHcontaining only the stubs. It uses absoluteexecPathshebangs, which are short on CI runners. - The concurrency case waits on a
.startedfile with a 5 s deadline, then lets the slow child finish through a release file, also in afinally. Nothing depends on timing beyond 5–10 ms polls. - Skipping the tests on Windows is fine.
gen-skills-manifest --check is in sync. check-media-use-copy-parity.test.mjs fails locally with the same "both media-use copies must exist" list on main (the build-step copies aren't generated here). It doesn't involve tts.mjs.
— Rames
What
Clean up the per-call
hf-txt-*directory used by the ElevenLabs and Kokoro paths insynthesizeOne. Add isolated regression tests and refresh the media-use skills manifest.Why
Each synthesized line creates a temporary text directory. The text must remain available until its subprocess finishes, then only that call's directory should be removed, including when writing or synthesis fails.
Related work
Fixes #5466.
#5469 separately changes ElevenLabs Python text decoding to UTF-8. This PR does not include or replace that change.
How
withTmpText, with cleanup infinally.Test plan
Validated in an isolated Linux cloud environment against base
43be84d82ce9d576d369dd4db4255924240cca0fplus this exact patch:node --test --test-reporter=tap skills/media-use/audio/scripts/lib/tts.cleanup.test.mjsproduced 13 failures and 4 passing controls.HYPERFRAMES_MEDIA_HOME_REQUIRED=1 node --import ./packages/cli/scripts/test-home.mjs --test --test-reporter=tap skills/media-use/audio/scripts/lib/tts*.test.mjs: 40 passed on Node 22 and Node 24.node --import ./packages/cli/scripts/test-home.mjs --test --test-reporter=tap 'skills/**/*.test.mjs' 'packages/cli/src/media-use/**/*.test.mjs' scripts/check-media-use-copy-parity.test.mjs scripts/creator-editing-capabilities.test.mjs: 895 passed, 2 Windows-only skips on Node 22.bun install --frozen-lockfile,bun run build,bun run --filter '*' typecheck,bun run typecheck:scripts,bun run lint,bun run format:check, andbun packages/cli/scripts/gen-skills-manifest.ts --checkpassed.Tests use isolated HOME/TMPDIR and synthetic provider executables. Native Windows/macOS and live providers were not tested; the new POSIX subprocess tests skip Windows. These are local validation results, not hosted CI results. Upstream main has advanced since the tested base; the current-main integration has not been locally rerun.