fix: cancel yt-dlp audio imports reliably when no child process is running - #6092
Merged
Conversation
… null job.process (#6013) cancelYoutubeImport/cancelReferenceAudioImport keyed off job.process, which is null both during job setup and again during post-processing (runYtDlp clears it on child exit). A cancel in either window returned false, emitted no canceled frame, and let the import commit a track/upload anyway. Both jobs now carry a canceled flag that the kickoff re-checks at each phase boundary, and both modules export __testing so the cancel path is actually unit-tested.
…ed (#6013) Review follow-up: the job lingers in the map until closeJobAfterDelay evicts it, so guarding cancel on the canceled flag alone made a cancel for a completed import answer ok:true. Mark the job done in the kickoff's finally and require a running status.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
cancelYoutubeImport/cancelReferenceAudioImportdecided whether a cancel was possible by looking atjob.process. That field is transient — it isnullwhile the job is being set up and again during post-processing, becauserunYtDlpclears it the moment the child exits. A cancel landing in either window returnedfalse, emitted no{ type: 'canceled' }frame, and let the import go on to land the audio file and create the Track / uploads record anyway.canceledflag;cancel*sets it and only signalskillWithEscalationwhen a child is actually running.{ type: 'canceled' }and cleaning temp files instead of committing the import.canceledoutcome and the flag are handled by one shared abort path rather than two branches.false: the job lingers in the map untilcloseJobAfterDelayevicts it, so the kickoff marks itdoneandcancel*requires a running status.__testing = { importJobs }, matchingvideoDownload.js, so the cancel path is unit-testable at all.Test plan
server/services/trackYoutubeImport.test.jsandserver/services/roundReferenceAudioImport.test.jsgain cancellation coverage: unknown job id, cancel with no child spawned yet, cancel signalling a running child plus a rejected second cancel, cancel of a finished-but-not-yet-evicted job, and a kickoff cancelled after the download finished (assertscanceledis broadcast andimportUploadedTrack/createTrack/importFileToUploadsare never called).cd server && npx vitest run services/trackYoutubeImport.test.js services/roundReferenceAudioImport.test.js services/ytdlpAudioImport.test.js services/videoDownload.test.js routes/tracks.test.js routes/rounds.test.js— all green.Closes #6013