Allow in progress recording downloads (#21)

### Description

We also want to be able to show recordings of the session so far. The way we've hooked up `ffmpeg` allows this already! The work in this pr is to:

1. Relax constraints around downloading in progress recordings
2. Instead, we'll check the file size and return a `202` if the recording doesn't have a reasonable amount of data. For the moment we've picked 100 bytes, which is substantially smaller than what `ffmpeg` will flush to disk

Also lurked in some minor config validation + cleaned up the `Start` logic as recommended by my cursor agents

### Testing

Via the local docker set up:

1. Build and run the container with the recording server
2. Start recording
3. Immediately hit the download -> confirm a `202` response
4. (wait for a bit)
5. Hit the download again -> confirm valid video output
6. Stop recording
7. Download -> confirm valid video output

Author: Sayan-

server/cmd/api/api/api.go

@@ -27,11 +27,6 @@ func New(recordManager recorder.RecordManager, factory recorder.FFmpegRecorderFa
 func (s *ApiService) StartRecording(ctx context.Context, req oapi.StartRecordingRequestObject) (oapi.StartRecordingResponseObject, error) {
 	log := logger.FromContext(ctx)
 
-	if rec, exists := s.recordManager.GetRecorder(s.mainRecorderID); exists && rec.IsRecording(ctx) {
-		log.Error("attempted to start recording while one is already active")
-		return oapi.StartRecording409JSONResponse{ConflictErrorJSONResponse: oapi.ConflictErrorJSONResponse{Message: "recording already in progress"}}, nil
-	}
-
 	var params recorder.FFmpegRecordingParams
 	if req.Body != nil {
 		params.FrameRate = req.Body.Framerate
@@ -45,6 +40,10 @@ func (s *ApiService) StartRecording(ctx context.Context, req oapi.StartRecording
 		return oapi.StartRecording500JSONResponse{InternalErrorJSONResponse: oapi.InternalErrorJSONResponse{Message: "failed to create recording"}}, nil
 	}
 	if err := s.recordManager.RegisterRecorder(ctx, rec); err != nil {
+		if rec, exists := s.recordManager.GetRecorder(s.mainRecorderID); exists && rec.IsRecording(ctx) {
+			log.Error("attempted to start recording while one is already active")
+			return oapi.StartRecording409JSONResponse{ConflictErrorJSONResponse: oapi.ConflictErrorJSONResponse{Message: "recording already in progress"}}, nil
+		}
 		log.Error("failed to register recorder", "err", err)
 		return oapi.StartRecording500JSONResponse{InternalErrorJSONResponse: oapi.InternalErrorJSONResponse{Message: "failed to register recording"}}, nil
 	}
@@ -90,6 +89,10 @@ func (s *ApiService) StopRecording(ctx context.Context, req oapi.StopRecordingRe
 	return oapi.StopRecording200Response{}, nil
 }
 
+const (
+	minRecordingSizeInBytes = 100
+)
+
 func (s *ApiService) DownloadRecording(ctx context.Context, req oapi.DownloadRecordingRequestObject) (oapi.DownloadRecordingResponseObject, error) {
 	log := logger.FromContext(ctx)
 
@@ -100,17 +103,21 @@ func (s *ApiService) DownloadRecording(ctx context.Context, req oapi.DownloadRec
 		return oapi.DownloadRecording404JSONResponse{NotFoundErrorJSONResponse: oapi.NotFoundErrorJSONResponse{Message: "no recording found"}}, nil
 	}
 
-	if rec.IsRecording(ctx) {
-		log.Warn("attempted to download recording while is still in progress")
-		return oapi.DownloadRecording400JSONResponse{BadRequestErrorJSONResponse: oapi.BadRequestErrorJSONResponse{Message: "recording still in progress, please stop first"}}, nil
-	}
-
 	out, meta, err := rec.Recording(ctx)
 	if err != nil {
 		log.Error("failed to get recording", "err", err)
 		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
+	if rec.IsRecording(ctx) && meta.Size <= minRecordingSizeInBytes {
+		return oapi.DownloadRecording202Response{
+			Headers: oapi.DownloadRecording202ResponseHeaders{
+				RetryAfter: 300,
+			},
+		}, nil
+	}
+
 	log.Info("serving recording file for download", "size", meta.Size)
 	return oapi.DownloadRecording200Videomp4Response{
 		Body:          out,

server/cmd/api/api/api_test.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"context"
 	"io"
+	"math/rand"
 	"testing"
 
 	oapi "github.com/onkernel/kernel-images/server/lib/oapi"
@@ -92,15 +93,40 @@ func TestApiService_DownloadRecording(t *testing.T) {
 		require.IsType(t, oapi.DownloadRecording404JSONResponse{}, resp)
 	})
 
+	randomBytes := func(n int) []byte {
+		data := make([]byte, n)
+		for i := range data {
+			data[i] = byte(rand.Intn(256))
+		}
+		return data
+	}
+
 	t.Run("still recording", func(t *testing.T) {
 		mgr := recorder.NewFFmpegManager()
-		rec := &mockRecorder{id: "main", isRecordingFlag: true}
+		rec := &mockRecorder{id: "main", isRecordingFlag: true, recordingData: randomBytes(minRecordingSizeInBytes - 1)}
 		require.NoError(t, mgr.RegisterRecorder(ctx, rec), "failed to register recorder")
 
 		svc := New(mgr, newMockFactory())
+		// will return a 202 when the recording is too small
 		resp, err := svc.DownloadRecording(ctx, oapi.DownloadRecordingRequestObject{})
 		require.NoError(t, err)
-		require.IsType(t, oapi.DownloadRecording400JSONResponse{}, resp)
+		require.IsType(t, oapi.DownloadRecording202Response{}, resp)
+
+		// mimic writing more data to the recording
+		data := randomBytes(minRecordingSizeInBytes * 2)
+		rec.recordingData = data
+
+		// now that the recording is larger than the minimum size, it should return a 200
+		resp, err = svc.DownloadRecording(ctx, oapi.DownloadRecordingRequestObject{})
+		require.NoError(t, err)
+		require.IsType(t, oapi.DownloadRecording200Videomp4Response{}, resp)
+		r, ok := resp.(oapi.DownloadRecording200Videomp4Response)
+		require.True(t, ok, "expected 200 mp4 response, got %T", resp)
+		buf := new(bytes.Buffer)
+		_, copyErr := io.Copy(buf, r.Body)
+		require.NoError(t, copyErr)
+		require.Equal(t, data, buf.Bytes(), "response body mismatch")
+		require.Equal(t, int64(len(data)), r.ContentLength, "content length mismatch")
 	})
 
 	t.Run("success", func(t *testing.T) {

server/cmd/config/config.go

@@ -41,11 +41,11 @@ func validate(config *Config) error {
 	if config.DisplayNum < 0 {
 		return fmt.Errorf("DISPLAY_NUM must be greater than 0")
 	}
-	if config.FrameRate < 0 {
-		return fmt.Errorf("FRAME_RATE must be greater than 0")
+	if config.FrameRate < 0 || config.FrameRate > 120 {
+		return fmt.Errorf("FRAME_RATE must be greater than 0 and less than 120")
 	}
-	if config.MaxSizeInMB < 0 {
-		return fmt.Errorf("MAX_SIZE_MB must be greater than 0")
+	if config.MaxSizeInMB < 0 || config.MaxSizeInMB > 1000 {
+		return fmt.Errorf("MAX_SIZE_MB must be greater than 0 and less than 1000")
 	}
 	if config.PathToFFmpeg == "" {
 		return fmt.Errorf("FFMPEG_PATH is required")

server/cmd/config/config_test.go

@@ -0,0 +1,102 @@
+package config
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestLoad(t *testing.T) {
+	testCases := []struct {
+		name    string
+		env     map[string]string
+		wantErr bool
+		wantCfg *Config
+	}{
+		{
+			name: "defaults (no env set)",
+			env:  map[string]string{},
+			wantCfg: &Config{
+				Port:         10001,
+				FrameRate:    10,
+				DisplayNum:   1,
+				MaxSizeInMB:  500,
+				OutputDir:    ".",
+				PathToFFmpeg: "ffmpeg",
+			},
+		},
+		{
+			name: "custom valid env",
+			env: map[string]string{
+				"PORT":        "12345",
+				"FRAME_RATE":  "24",
+				"DISPLAY_NUM": "2",
+				"MAX_SIZE_MB": "250",
+				"OUTPUT_DIR":  "/tmp",
+				"FFMPEG_PATH": "/usr/local/bin/ffmpeg",
+			},
+			wantCfg: &Config{
+				Port:         12345,
+				FrameRate:    24,
+				DisplayNum:   2,
+				MaxSizeInMB:  250,
+				OutputDir:    "/tmp",
+				PathToFFmpeg: "/usr/local/bin/ffmpeg",
+			},
+		},
+		{
+			name: "negative display num",
+			env: map[string]string{
+				"DISPLAY_NUM": "-1",
+			},
+			wantErr: true,
+		},
+		{
+			name: "frame rate too high",
+			env: map[string]string{
+				"FRAME_RATE": "1201",
+			},
+			wantErr: true,
+		},
+		{
+			name: "max size too big",
+			env: map[string]string{
+				"MAX_SIZE_MB": "10001",
+			},
+			wantErr: true,
+		},
+		{
+			name: "missing ffmpeg path (set to empty)",
+			env: map[string]string{
+				"FFMPEG_PATH": "",
+			},
+			wantErr: true,
+		},
+		{
+			name: "missing output dir (set to empty)",
+			env: map[string]string{
+				"OUTPUT_DIR": "",
+			},
+			wantErr: true,
+		},
+	}
+
+	for idx := range testCases {
+		tc := testCases[idx]
+		t.Run(tc.name, func(t *testing.T) {
+			for k, v := range tc.env {
+				t.Setenv(k, v)
+			}
+
+			cfg, err := Load()
+
+			if tc.wantErr {
+				require.Error(t, err)
+			} else {
+				require.NoError(t, err)
+				require.NotNil(t, cfg)
+				require.Equal(t, tc.wantCfg, cfg)
+			}
+		})
+	}
+}

server/lib/oapi/oapi.go

@@ -188,10 +188,6 @@ func (fr *FFmpegRecorder) IsRecording(ctx context.Context) bool {
 
 // Recording returns the recording file as an io.ReadCloser.
 func (fr *FFmpegRecorder) Recording(ctx context.Context) (io.ReadCloser, *RecordingMetadata, error) {
-	if fr.IsRecording(ctx) {
-		return nil, nil, fmt.Errorf("recording still in progress, please call stop first")
-	}
-
 	file, err := os.Open(fr.outputPath)
 	if err != nil {
 		return nil, nil, fmt.Errorf("failed to open recording file: %w", err)

server/lib/recorder/ffmpeg.go

@@ -50,6 +50,14 @@ paths:
               schema:
                 type: string
                 format: binary
+        "202":
+          description: Recording is still in progress, please try again later
+          headers:
+            Retry-After:
+              description: Suggested wait time in seconds before retrying
+              schema:
+                type: integer
+                minimum: 1
         "400":
           $ref: "#/components/responses/BadRequestError"
         "404":