Skip to content

Commit 36c7f00

Browse files
authored
fix: add post-processing step to fix MP4 duration metadata (#100)
## Summary Fixes two issues: 1. MP4 files recorded by the recording endpoints don't display the full video length when downloaded (duration extends as videos are played) 2. Concurrent `Stop()` calls could corrupt recordings due to multiple SIGINTs being sent to ffmpeg ## Problem ### Duration Metadata The recording uses **fragmented MP4** format (`-movflags +frag_keyframe+empty_moov`) for data safety during recording. This format: - Writes an empty `moov` atom at the start (without duration information) - Streams fragments incrementally as it records - Is great for crash safety, but causes players to calculate duration on playback ### Concurrent Stop Race Condition When multiple goroutines called `Stop()` concurrently (e.g., from duplicate HTTP requests), each would send SIGINT to ffmpeg. Multiple SIGINTs cause ffmpeg to terminate immediately without flushing buffers, resulting in corrupted/empty MP4 files. ## Solution ### Post-Processing (Duration Fix) Add a **post-processing step** that remuxes the fragmented MP4 into a standard MP4 with proper duration metadata after recording stops: - `finalizeRecording()` method - Remuxes using `ffmpeg -c copy -movflags +faststart` - No re-encoding (fast operation) - Moves the `moov` atom to the start with correct duration - Uses temp file to safely replace the original ### Concurrent-Safe Stop (Race Condition Fix) Use `sync.Once` to ensure shutdown and finalization only run once: - `stopOnce` - Ensures shutdown signals are only sent once (prevents duplicate SIGINTs) - `finalizeOnce` - Ensures finalization only runs once - Wait for process exit before proceeding to finalization ### API Changes - `DownloadRecording`: Returns `202 Accepted` with `Retry-After: 5` if recording is being finalized - `DeleteRecording`: Returns `400` if recording is being finalized; now synchronous instead of async ## Changes ### Recorder (`server/lib/recorder/ffmpeg.go`) - Add `finalizeRecording()` to remux fragmented MP4 to standard MP4 - Introduce `ErrRecordingFinalizing` and `finalizing` state - Gate `Recording()`/`Delete()` to return error while finalizing - Make shutdown/finalization idempotent via `sync.Once` (`stopOnce`, `finalizeOnce`) - Wait for process exit before finalizing - `Stop()` always attempts finalization after graceful shutdown - `ForceStop()` attempts finalization with warning-only on failure ### API (`server/cmd/api/api/api.go`) - `DownloadRecording`: Return `202` with `Retry-After: 5` if finalizing - `DeleteRecording`: Synchronous delete; return `400` if finalizing ### Tests (`server/lib/recorder/ffmeg_test.go`) - Set explicit `outputPath` and adjust expectations for new finalization behavior ## Testing Tested with concurrent stop script: go run ./scripts/concurrent_stop_test/main.go -url http://localhost:444 -concurrency 5 -iterations 10Results: **10/10 passed** with 5 concurrent stop calls per iteration. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > Adds FFmpeg post-processing to finalize MP4 duration metadata and singleflight-guards Stop/finalize; API now waits for finalization on download and returns 409 on delete during finalization. > > - **Recorder (FFmpeg)**: > - Add `finalizeRecording()` remux step (faststart) and finalize automatically on natural exits; expose `WaitForFinalization`. > - Introduce `ErrRecordingFinalizing`; gate `Recording()`/`Delete()` until finalization completes. > - Use `singleflight` to dedupe concurrent `Stop()` and finalization; `Stop()` always attempts finalize; `ForceStop()` tries finalize but warns on failure. > - Update `waitForCommand()` to trigger finalization; `FFmpegManager.StopAll()` now calls `Stop()` for all. > - **API**: > - `StopRecording`: always invoke `Stop()` even if not recording. > - `DownloadRecording`: if finalizing, wait and then serve; still returns `202` when in-progress and tiny file. > - `DeleteRecording`: make synchronous; return `409 Conflict` when finalizing; log improvements. > - **OpenAPI/Client**: > - Add `409 Conflict` response to `DELETE /recording/delete`; regenerate `oapi` client/server stubs and embedded swagger. > - **Tests**: > - Update recorder tests to set explicit `outputPath` and assert new finalization/force-stop behaviors. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit d3e043f. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
1 parent 1349e74 commit 36c7f00

File tree

5 files changed

+356
-148
lines changed

5 files changed

+356
-148
lines changed

server/cmd/api/api/api.go

Lines changed: 36 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -129,10 +129,10 @@ func (s *ApiService) StopRecording(ctx context.Context, req oapi.StopRecordingRe
129129
if !exists {
130130
log.Error("attempted to stop recording when none is active", "recorder_id", recorderID)
131131
return oapi.StopRecording400JSONResponse{BadRequestErrorJSONResponse: oapi.BadRequestErrorJSONResponse{Message: "no active recording to stop"}}, nil
132-
} else if !rec.IsRecording(ctx) {
133-
log.Warn("recording already stopped", "recorder_id", recorderID)
134-
return oapi.StopRecording200Response{}, nil
135132
}
133+
// Always call Stop() even if IsRecording() is false. Recordings that exit naturally
134+
// (max duration, max file size, etc.) finalize automatically, but Stop() is still
135+
// needed to update scale-to-zero state and ensure clean shutdown.
136136

137137
// Check if force stop is requested
138138
forceStop := false
@@ -182,12 +182,34 @@ func (s *ApiService) DownloadRecording(ctx context.Context, req oapi.DownloadRec
182182

183183
out, meta, err := rec.Recording(ctx)
184184
if err != nil {
185-
log.Error("failed to get recording", "err", err, "recorder_id", recorderID)
186-
return oapi.DownloadRecording500JSONResponse{InternalErrorJSONResponse: oapi.InternalErrorJSONResponse{Message: "failed to get recording"}}, nil
185+
if errors.Is(err, recorder.ErrRecordingFinalizing) {
186+
// Wait for finalization to complete instead of asking client to retry
187+
log.Info("waiting for recording finalization", "recorder_id", recorderID)
188+
ffmpegRec, ok := rec.(*recorder.FFmpegRecorder)
189+
if !ok {
190+
log.Error("failed to cast recorder to FFmpegRecorder", "recorder_id", recorderID)
191+
return oapi.DownloadRecording500JSONResponse{InternalErrorJSONResponse: oapi.InternalErrorJSONResponse{Message: "internal error"}}, nil
192+
}
193+
// WaitForFinalization blocks until finalization completes and returns the result
194+
if finalizeErr := ffmpegRec.WaitForFinalization(ctx); finalizeErr != nil {
195+
log.Error("finalization failed", "err", finalizeErr, "recorder_id", recorderID)
196+
return oapi.DownloadRecording500JSONResponse{InternalErrorJSONResponse: oapi.InternalErrorJSONResponse{Message: "failed to finalize recording"}}, nil
197+
}
198+
// Finalization complete, retry getting the recording
199+
out, meta, err = rec.Recording(ctx)
200+
if err != nil {
201+
log.Error("failed to get recording after finalization", "err", err, "recorder_id", recorderID)
202+
return oapi.DownloadRecording500JSONResponse{InternalErrorJSONResponse: oapi.InternalErrorJSONResponse{Message: "failed to get recording"}}, nil
203+
}
204+
} else {
205+
log.Error("failed to get recording", "err", err, "recorder_id", recorderID)
206+
return oapi.DownloadRecording500JSONResponse{InternalErrorJSONResponse: oapi.InternalErrorJSONResponse{Message: "failed to get recording"}}, nil
207+
}
187208
}
188209

189210
// short-circuit if the recording is still in progress and the file is arbitrary small
190211
if rec.IsRecording(ctx) && meta.Size <= minRecordingSizeInBytes {
212+
out.Close() // Close the file handle to prevent descriptor leak
191213
return oapi.DownloadRecording202Response{
192214
Headers: oapi.DownloadRecording202ResponseHeaders{
193215
RetryAfter: 300,
@@ -224,15 +246,18 @@ func (s *ApiService) DeleteRecording(ctx context.Context, req oapi.DeleteRecordi
224246
return oapi.DeleteRecording400JSONResponse{BadRequestErrorJSONResponse: oapi.BadRequestErrorJSONResponse{Message: "recording must be stopped first"}}, nil
225247
}
226248

227-
// fine to do this async
228-
go func() {
229-
if err := rec.Delete(context.Background()); err != nil && !errors.Is(err, os.ErrNotExist) {
249+
if err := rec.Delete(ctx); err != nil {
250+
if errors.Is(err, recorder.ErrRecordingFinalizing) {
251+
log.Info("recording is being finalized, client should retry", "recorder_id", recorderID)
252+
return oapi.DeleteRecording409JSONResponse{ConflictErrorJSONResponse: oapi.ConflictErrorJSONResponse{Message: "recording is being finalized, please retry in a few seconds"}}, nil
253+
}
254+
if !errors.Is(err, os.ErrNotExist) {
230255
log.Error("failed to delete recording", "err", err, "recorder_id", recorderID)
231-
} else {
232-
log.Info("recording deleted", "recorder_id", recorderID)
256+
return oapi.DeleteRecording500JSONResponse{InternalErrorJSONResponse: oapi.InternalErrorJSONResponse{Message: "failed to delete recording"}}, nil
233257
}
234-
}()
258+
}
235259

260+
log.Info("recording deleted", "recorder_id", recorderID)
236261
return oapi.DeleteRecording200Response{}, nil
237262
}
238263

0 commit comments

Comments
 (0)