Refactor FFmpegRecorder

Author: hiroTamada

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)

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)