fix(ingest): act on the transfer signal when it arrives, not a cycle later

[KillerX]

9/n of a stack. Base: fix/generate-short-bounded-polling (#447).

The live ingest watched for the file-transferred signal from a background coroutine that set a bool, and the copy loop only read that bool after the current rsync returned and the one minute sleep elapsed. A transfer that finished thirty seconds into a sleep kept the ingest rsyncing a file nobody was writing to for another cycle — and the delay is charged to a live event.

The wait now selects on the signal and the retry timer together, so it returns the moment the right signal lands, including immediately when the signal arrived while the copy activity was running and is already queued on the channel. Signals for other files are still consumed and ignored. Cancelling the timer on the way out keeps an abandoned timer out of the history.

This introduces workflow.GetVersion, which the repository has not used before, and I think it earns its keep here. Cancelling a timer early is a command that executions started under the old code do not have in their history, so replaying them against this fails the workflow task — and Temporal retries a failed workflow task forever. A live ingest running at deploy time would hang rather than fail. Old executions keep the coroutine and the full sleep; the branch can be deleted once none can still be running, which is bounded by maxCopyAttempts minutes.

One correction to the finding. The loop is not unbounded: maxCopyAttempts caps it at 1000 iterations, roughly 5,000 events and 16 hours. That is past the 10,000-event warning but well inside the hard limit. ContinueAsNew across a workflow carrying signal state and a rolling sample window is a bigger change than this one and is not attempted here.

🤖 Generated with Claude Code

[KillerX]

Added in 8e90add: the preview is now transcoded once more after the transfer completes.

You are right that it was missing, and the gap is worse than a lag. stopPreviewFunc() cancels the growing preview the instant the copy loop ends, so the lowres shape the asset keeps covers only as much as ffmpeg had reached by then — the tail of the recording, which is the part most likely to be checked right after the event, is not in it. And nothing later replaces it: this workflow never runs TranscodePreviewVX.

Once the file is complete and closed in Vidispine, it now transcodes a preview of the whole file and imports that over the same shape tag with Replace: true. The growing import deliberately leaves Replace false because it is the first file to claim the tag; this one is the second and is meant to supersede it.

Placement: started right after CloseFile, awaited at the very end, so a full-file transcode does not hold up the reaper audio imports, the transcription or the duration fix. A failure is logged and sent to Telegram rather than returned — by that point the audio and video are already in place, and an incomplete preview is not worth failing an ingest over.

One incidental change to make it possible: previewPath held the aux output folder and was then reassigned to the file inside it, so nothing pointed at the directory any more. The folder is previewOutputDir now.

Note the branches above this one in the stack (#449–#454) were rebased onto the new commit and force-pushed. Their bases and contents are unchanged; make test is green on the tip.

[KillerX]

You are right to be suspicious of that line, and the re-transcode was the wrong answer — it was papering over a truncation instead of removing its cause. Reverted in be0b3cc, replaced by 91576d5.

There were two causes, and one of them is a regression I introduced in this PR.

The missing final copy. The loop copies at the top and then waits for the signal, so the last copy always ran before the signal arrived — anything written to the source in between was never fetched. The old code did not have this problem, and not by design: it read the flag after the next copy, so breaking out already implied one more copy. Making the wait return as soon as the signal lands removed that copy without anyone noticing. The new path now does it explicitly, and the version gate leaves the old path exactly as it was.

The preview being cancelled while ffmpeg is behind. Reading the activity: tail -c +1 -f feeds ffmpeg's stdin, and cancelling SIGKILLs tail. The shutdown is careful — ffmpeg sees EOF, finalises the playlist, and there is a last remux — but that can only finish what ffmpeg has already received. rsync delivers roughly a minute of video at a time, so at the moment the ingest ends the transcode is typically a segment or more behind, and everything past ffmpeg's read position dies with tail. Nothing about the shutdown recovers it, which is why the preview kept coming out short.

So the workflow now waits for the preview to reach the source duration before cancelling, which is what you asked for. The observable is the preview file itself: the activity remuxes its segments into it every minute, so a preview that has reached the source duration is one where ffmpeg has consumed everything. It gives up after ten minutes and cancels anyway — a late preview beats an ingest that never finishes.

Worth knowing about the shape of the wait: it costs one AnalyzeFile per minute while it waits, and it can only see progress at the activity's one-minute remux cadence, so in the normal case it adds a minute or two to the ingest. If you would rather have a fixed grace period, or have the activity signal completeness itself instead of the workflow inferring it from durations, say so — this is the cheapest thing that uses only what is already there.

[KillerX]

Follow-up: the full re-transcode had come back as e0f1a44 on #449 through a rebase error of mine — I used e6b5f21 as the upstream when rebasing the stack, and 8e90add is inside that range, so git replayed the reverted commit on top of its own revert. The stack was doing both the catch-up wait and a second full-file transcode. That commit is gone.

This PR is also now two commits rather than four: the add-and-revert pair is dropped, so the history is the selector change plus the copy-and-wait fix, with nothing arguing with itself. The comment that claimed the preview is cancelled immediately went with the reverted commit, so the code and the comments agree again.

I also trimmed the comments here to the same standard as #449 — the mechanism worth stating is that cancelling costs whatever tail has not yet written, and that the old path already copies after the signal.

[KillerX]

Valid, and fixed in 8d378d3 — with three corrections to the finding.

The preview wait was ungated, and that is the real bug. It emits one AnalyzeFile and then up to ten Sleep+AnalyzeFile pairs where an older history has ListReaperFiles. It now sits behind its own change ID:

if workflow.GetVersion(ctx, versionPreviewCatchUp, workflow.DefaultVersion, 1) != workflow.DefaultVersion {
    waitForPreviewToCatchUp(ctx, rawPath, previewPath)
}

Its own ID rather than versionSignalSelector: sharing would be sound only while the two changes always deploy together — true today because they are one PR, but nothing enforces it, and the cost of it not holding is a hung ingest.

The final copy was already gated — if watchSignalInline && signalReceived, line 272 — as is the NewTimer in waitForTransferSignal, which is only reachable on the new path. Of the commands this PR adds after the copy loop, the preview wait was the only ungated one.

:314 is pre-existing. That line is ExecuteChildWorkflow(ImportAudioFileFromReaper) from master. The #449 change to this file was e0f1a44, the accidentally reintroduced transcode, which was removed before this review — #449 no longer touches incremental_ingest.go, so only one new gate was needed rather than two.

On "prove no executions are open during each deployment": not provable, and the deployment model argues the other way. publish.yml tags on every merge to master and workers self-update within five minutes, while Incremental runs up to maxCopyAttempts minutes under a fixed ID during live events. A deploy landing mid-ingest is the normal case.

Test: incremental_ingest_version_test.go drives the whole workflow with OnGetVersion(...).Return(DefaultVersion) and asserts AnalyzeFile is never reached. Checked that it discriminates — with the gate removed it reports two calls.

Two adjacent cases I checked while here, so they are not left to assumption:

• GenerateShort (fix(export): bound GenerateShort's wait for the short service #447), no gate needed. It replaced Sleep(5s) with a 5s/30s interval. isCommandMatchEvent matches COMMAND_TYPE_START_TIMER on timer ID only (internal/internal_task_handlers.go:1675-1686); the duration is not compared and the timer ID is an unchanged sequence number. The activity-per-iteration sequence is unchanged, and the new non-retryable timeout is only reachable where no older history has anything.
• MoveFilesWorkerFlow (fix: the remaining Unbounded workflow histories sub-items #449), gated. GetContinueAsNewSuggested replays its historical value, so a run already past the server's threshold would emit CONTINUE_AS_NEW where its history has a signal receive. Now behind versionContinueAsNew. Not testable — the test environment cannot make that flag true, so a test would pass either way, and the commit says so.