From 13d380233c2c07ff245c024598585afe310048e2 Mon Sep 17 00:00:00 2001 From: Hiro Tamada Date: Tue, 9 Dec 2025 13:26:00 -0500 Subject: [PATCH] Refactor FFmpegRecorder --- server/cmd/api/api/api.go | 37 ++++------------------- server/lib/recorder/ffmpeg.go | 56 ++++++++++++++++++----------------- 2 files changed, 34 insertions(+), 59 deletions(-) diff --git a/server/cmd/api/api/api.go b/server/cmd/api/api/api.go index 160120a7..1ba481f6 100644 --- a/server/cmd/api/api/api.go +++ b/server/cmd/api/api/api.go @@ -182,29 +182,8 @@ func (s *ApiService) DownloadRecording(ctx context.Context, req oapi.DownloadRec out, meta, err := rec.Recording(ctx) if err != nil { - if errors.Is(err, recorder.ErrRecordingFinalizing) { - // Wait for finalization to complete instead of asking client to retry - log.Info("waiting for recording finalization", "recorder_id", recorderID) - ffmpegRec, ok := rec.(*recorder.FFmpegRecorder) - if !ok { - log.Error("failed to cast recorder to FFmpegRecorder", "recorder_id", recorderID) - return oapi.DownloadRecording500JSONResponse{InternalErrorJSONResponse: oapi.InternalErrorJSONResponse{Message: "internal error"}}, nil - } - // WaitForFinalization blocks until finalization completes and returns the result - if finalizeErr := ffmpegRec.WaitForFinalization(ctx); finalizeErr != nil { - log.Error("finalization failed", "err", finalizeErr, "recorder_id", recorderID) - return oapi.DownloadRecording500JSONResponse{InternalErrorJSONResponse: oapi.InternalErrorJSONResponse{Message: "failed to finalize recording"}}, nil - } - // Finalization complete, retry getting the recording - out, meta, err = rec.Recording(ctx) - if err != nil { - log.Error("failed to get recording after finalization", "err", err, "recorder_id", recorderID) - return oapi.DownloadRecording500JSONResponse{InternalErrorJSONResponse: oapi.InternalErrorJSONResponse{Message: "failed to get recording"}}, nil - } - } else { - log.Error("failed to get recording", "err", err, "recorder_id", recorderID) - return oapi.DownloadRecording500JSONResponse{InternalErrorJSONResponse: oapi.InternalErrorJSONResponse{Message: "failed to get recording"}}, nil - } + log.Error("failed to get recording", "err", err, "recorder_id", recorderID) + return oapi.DownloadRecording500JSONResponse{InternalErrorJSONResponse: oapi.InternalErrorJSONResponse{Message: "failed to get recording"}}, nil } // short-circuit if the recording is still in progress and the file is arbitrary small @@ -246,15 +225,9 @@ func (s *ApiService) DeleteRecording(ctx context.Context, req oapi.DeleteRecordi return oapi.DeleteRecording400JSONResponse{BadRequestErrorJSONResponse: oapi.BadRequestErrorJSONResponse{Message: "recording must be stopped first"}}, nil } - if err := rec.Delete(ctx); err != nil { - if errors.Is(err, recorder.ErrRecordingFinalizing) { - log.Info("recording is being finalized, client should retry", "recorder_id", recorderID) - return oapi.DeleteRecording409JSONResponse{ConflictErrorJSONResponse: oapi.ConflictErrorJSONResponse{Message: "recording is being finalized, please retry in a few seconds"}}, nil - } - if !errors.Is(err, os.ErrNotExist) { - log.Error("failed to delete recording", "err", err, "recorder_id", recorderID) - return oapi.DeleteRecording500JSONResponse{InternalErrorJSONResponse: oapi.InternalErrorJSONResponse{Message: "failed to delete recording"}}, nil - } + if err := rec.Delete(ctx); err != nil && !errors.Is(err, os.ErrNotExist) { + log.Error("failed to delete recording", "err", err, "recorder_id", recorderID) + return oapi.DeleteRecording500JSONResponse{InternalErrorJSONResponse: oapi.InternalErrorJSONResponse{Message: "failed to delete recording"}}, nil } log.Info("recording deleted", "recorder_id", recorderID) diff --git a/server/lib/recorder/ffmpeg.go b/server/lib/recorder/ffmpeg.go index 52e6b054..57ef27db 100644 --- a/server/lib/recorder/ffmpeg.go +++ b/server/lib/recorder/ffmpeg.go @@ -32,10 +32,6 @@ const ( exitCodeProcessDoneMinValue = -1 ) -// ErrRecordingFinalizing is returned when attempting to access a recording that is -// currently being finalized (remuxed to add duration metadata). -var ErrRecordingFinalizing = errors.New("recording is being finalized") - // FFmpegRecorder encapsulates an FFmpeg recording session with platform-specific screen capture. // It manages the lifecycle of a single FFmpeg process and provides thread-safe operations. type FFmpegRecorder struct { @@ -237,14 +233,6 @@ func (fr *FFmpegRecorder) Stop(ctx context.Context) error { return fr.finalizeRecording(ctx) } -// WaitForFinalization blocks until finalization completes and returns the result. -// If finalization hasn't started, it will be triggered. If already complete, returns -// the cached result immediately. This is useful for callers like the download handler -// that need to wait for finalization before accessing the recording. -func (fr *FFmpegRecorder) WaitForFinalization(ctx context.Context) error { - return fr.finalizeRecording(ctx) -} - // ForceStop immediately terminates the recording process. func (fr *FFmpegRecorder) ForceStop(ctx context.Context) error { log := logger.FromContext(ctx) @@ -378,22 +366,25 @@ func (fr *FFmpegRecorder) Metadata() *RecordingMetadata { } // Recording returns the recording file as an io.ReadCloser. -// Returns ErrRecordingFinalizing if the recording is currently being finalized. +// If the recording has exited but finalization is pending, this method blocks +// until finalization completes. func (fr *FFmpegRecorder) Recording(ctx context.Context) (io.ReadCloser, *RecordingMetadata, error) { fr.mu.Lock() if fr.deleted { fr.mu.Unlock() return nil, nil, fmt.Errorf("recording deleted: %w", os.ErrNotExist) } - // Block access while finalization is pending or in progress. - // This covers both the race window (after process exit, before finalization starts) - // and during active finalization. - if fr.exitCode >= exitCodeProcessDoneMinValue && !fr.finalizeComplete { - fr.mu.Unlock() - return nil, nil, ErrRecordingFinalizing - } + // If recording has exited but finalization is pending or in progress, wait for it. + // This hides finalization as an implementation detail from callers. + needsFinalization := fr.exitCode >= exitCodeProcessDoneMinValue && !fr.finalizeComplete fr.mu.Unlock() + if needsFinalization { + if err := fr.finalizeRecording(ctx); err != nil { + return nil, nil, fmt.Errorf("failed to finalize recording: %w", err) + } + } + file, err := os.Open(fr.outputPath) if err != nil { return nil, nil, fmt.Errorf("failed to open recording file: %w", err) @@ -416,18 +407,29 @@ func (fr *FFmpegRecorder) Recording(ctx context.Context) (io.ReadCloser, *Record } // Delete removes the recording file from disk. -// Returns ErrRecordingFinalizing if the recording is currently being finalized. +// If the recording has exited but finalization is pending, this method blocks +// until finalization completes before deleting. func (fr *FFmpegRecorder) Delete(ctx context.Context) error { fr.mu.Lock() - defer fr.mu.Unlock() if fr.deleted { + fr.mu.Unlock() return nil // already deleted } - // Block deletion while finalization is pending or in progress. - // This covers both the race window (after process exit, before finalization starts) - // and during active finalization. - if fr.exitCode >= exitCodeProcessDoneMinValue && !fr.finalizeComplete { - return ErrRecordingFinalizing + // If recording has exited but finalization is pending or in progress, wait for it. + // We need to wait because deleting during finalization would corrupt the remux operation. + needsFinalization := fr.exitCode >= exitCodeProcessDoneMinValue && !fr.finalizeComplete + fr.mu.Unlock() + + if needsFinalization { + // Wait for finalization to complete. We don't fail the delete if finalization + // fails - the caller wants the file gone regardless. + _ = fr.finalizeRecording(ctx) + } + + fr.mu.Lock() + defer fr.mu.Unlock() + if fr.deleted { + return nil // deleted while we were waiting } if err := os.Remove(fr.outputPath); err != nil && !os.IsNotExist(err) { return fmt.Errorf("failed to delete recording file: %w", err)