From 0e45a6ee7f75ce5e53a72e39eca8d44c9d30e6ee Mon Sep 17 00:00:00 2001 From: Sayan Samanta Date: Wed, 2 Jul 2025 14:44:20 -0700 Subject: [PATCH 1/6] switch order to prevent check then act --- server/cmd/api/api/api.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/server/cmd/api/api/api.go b/server/cmd/api/api/api.go index f0106b25..550ceb43 100644 --- a/server/cmd/api/api/api.go +++ b/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 } From 9fc9d69d96b5cee00331837900718ecc1ab6b7b1 Mon Sep 17 00:00:00 2001 From: Sayan Samanta Date: Wed, 2 Jul 2025 14:54:39 -0700 Subject: [PATCH 2/6] minor validation + test config --- server/cmd/config/config.go | 8 +-- server/cmd/config/config_test.go | 102 +++++++++++++++++++++++++++++++ 2 files changed, 106 insertions(+), 4 deletions(-) create mode 100644 server/cmd/config/config_test.go diff --git a/server/cmd/config/config.go b/server/cmd/config/config.go index bff6d2b7..60b9fa1e 100644 --- a/server/cmd/config/config.go +++ b/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") diff --git a/server/cmd/config/config_test.go b/server/cmd/config/config_test.go new file mode 100644 index 00000000..984d5e8a --- /dev/null +++ b/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) + } + }) + } +} From 255e23c420b858a4615fb3dd799de18a2e9d4244 Mon Sep 17 00:00:00 2001 From: Sayan Samanta Date: Wed, 2 Jul 2025 15:26:39 -0700 Subject: [PATCH 3/6] 202 for in progress recordings before data is available --- server/openapi.yaml | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/server/openapi.yaml b/server/openapi.yaml index 8d4979c9..fc1ebebb 100644 --- a/server/openapi.yaml +++ b/server/openapi.yaml @@ -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": From ae38fe62b76b6c1bb6701397815c74ba7e1cac2c Mon Sep 17 00:00:00 2001 From: Sayan Samanta Date: Wed, 2 Jul 2025 15:26:46 -0700 Subject: [PATCH 4/6] make oapi-generate --- server/lib/oapi/oapi.go | 41 ++++++++++++++++++++++++++++------------- 1 file changed, 28 insertions(+), 13 deletions(-) diff --git a/server/lib/oapi/oapi.go b/server/lib/oapi/oapi.go index 51856a62..8bce6db3 100644 --- a/server/lib/oapi/oapi.go +++ b/server/lib/oapi/oapi.go @@ -835,6 +835,20 @@ func (response DownloadRecording200Videomp4Response) VisitDownloadRecordingRespo return err } +type DownloadRecording202ResponseHeaders struct { + RetryAfter int +} + +type DownloadRecording202Response struct { + Headers DownloadRecording202ResponseHeaders +} + +func (response DownloadRecording202Response) VisitDownloadRecordingResponse(w http.ResponseWriter) error { + w.Header().Set("Retry-After", fmt.Sprint(response.Headers.RetryAfter)) + w.WriteHeader(202) + return nil +} + type DownloadRecording400JSONResponse struct{ BadRequestErrorJSONResponse } func (response DownloadRecording400JSONResponse) VisitDownloadRecordingResponse(w http.ResponseWriter) error { @@ -1061,19 +1075,20 @@ func (sh *strictHandler) StopRecording(w http.ResponseWriter, r *http.Request) { // Base64 encoded, gzipped, json marshaled Swagger object var swaggerSpec = []string{ - "H4sIAAAAAAAC/7xVUW/bNhD+KwduDxsgWMqaDZjelm0FjCFd0ext2AMjnmwWJI89nty6hf77QCqWo9hr", - "gy3Lm0we7+77vrvPn1RHPlLAIEm1nxRjihQSlh9X2rzBdwMm+ZWZOB91FASD5E8do7OdFkuhfpso5LPU", - "bdHr/PU1Y69a9VV9zF9Pt6meso3jWCmDqWMbcxLV5oJwV1GNlfqZQu9s91zVD+Vy6XUQ5KDdM5U+lIMb", - "5B0y3AVW6hXJSxqCeaY+XpFAqafy3V14zjbXj0wRWew0IR5T0hvMn7KPqFqVhG3YlOeM7wbLaFT75xz4", - "V3UIpNu3OHF9I5rlDXbExobNQf8M0BibG9Pu9b2qvXYJqweN9Kw9spbSyhLTnBnmILAB+pjgG9ohszWY", - "IE3EG+z14ORbVSmvP1g/eNX+0FTK2zD9uJgB2CC4waKS1x9eWoc39iOuw/XVaQ/XUy7orUNI9mPp4Prq", - "kQ1cNE2z6KE5bWI8SyzF/8orcYc5z4Sp9DaHPphh79FYLej2kIQivLeypUFgw7rDfnCQtoMYeh9W8MfW", - "JvB6D4xpcJLZ0NAR8xAFDeysQSpkrdSM65bIoQ7noOYjG3oqc2jF5bvfkAM6WHu9wQQ/vV6rSu2Q09Rs", - "s7pYNZkjihh0tKpVL1bN6oWqVNSyLdhrPnBX564daZOPN1hIzCyV3Vsb1apf7gJmulW19NLvmubB+haQ", - "tY+Xy73tib2WjNcGzfsj/nmzTrb23oRbhxnV5VTtnAfMXdUP7b28u/zyu6UnjZX6/jHVlo5a7GXwPiM8", - "sgeyRfCUBBg7DOLygGRsi5koj++Jk7J/FG+idEaapb2oyZcwyRWZ/ZM56nkPG8fJBxdzcPE5iypY0Exa", - "/PhlVpd/kU+hRUECGlLHiAFmmlfwe3B7oIDHM+h0gFsE3YndIej8TqzH1alEk4P8k0L3fOp/E+iMF57V", - "p/m8PhTjQZ9/t2NPoBDFsindwIxBjnoUQH8HAAD///UXgf/TCQAA", + "H4sIAAAAAAAC/7xVX2/bNhD/KgduDxug2UqaDZjfmm0FjCFdkext2AMjnmQWJI89npy6hb/7QCqWI9tL", + "gy3Nm8Q/d/f7c8fPqiEfKWCQpBafFWOKFBKWn0ttrvFDj0l+YybOSw0FwSD5U8fobKPFUpi/TxTyWmpW", + "6HX++paxVQv1zXwffz7spvkQbbvdVspgatjGHEQtckK4z6i2lfqFQuts81LZd+ly6mUQ5KDdC6XepYMb", + "5DUy3B+s1FuSN9QH80J1vCWBkk/lvfvjOdqYPzJFZLGDQzympDvMn7KJqBYqCdvQleuMH3rLaNTir/Hg", + "39XuIN2+x4HrG9Es19gQGxu6nf4ZoDE2F6bduwdZW+0SVgeFtKw9spZSyhTTGBnGQ2ADtDHBd7RGZmsw", + "QRqIN9jq3sn3qlJef7S+92rxU10pb8PwczYCsEGww6KS1x/fWIc39hMuw9XlcQ1XQyxorUNI9lOp4Ory", + "iQWc1XU9qaE+LmJ7kliK/5dX4gZznAFTqW08euBh79FYLeg2kIQi3FlZUS/QsW6w7R2kVS+G7sIM/lzZ", + "BF5vgDH1TjIbGhpi7qOggbU1SIWsmRpx3RI51OEU1LxkQ0vFh1Zc3vsdOaCDpdcdJnj9bqkqtUZOQ7H1", + "7GxWZ44oYtDRqoV6Natnr1SlopZVwT7nHXfzXLUjbfJyh4XEzFLpvaVRC/Xr/YGRblVNZ+l5XR+0bwE5", + "9/Fi2rctsdeS8dqgebPHP3bWUdc+cLh1mFGd1+ePNYJNkMQ6l3mPTB1jShVEhzohCG9Ad9oGcFqQVaVW", + "qA1ygXGNwpsfXrd54yjBTd91mLKAd9oKiPXF5wkbCibBLbbECJxDDAztUT/WXwXxxcDfqak28jw/fLDK", + "vYsv35tO2W2lfnxKtukbUQZm733WbO8HkBWCpyTA2GAQly2fZZi4vFx+YLeUJ2KZtpROmG06MNUwaTHJ", + "JZnNs70Rp6fyIMaBs88e81rBgmbQ4ucvszp99J9Di4IENKSGEQOMNM/gj+A2QAH3a9DoALcIuhG7RtD5", + "Xvbx7FiiYSb+m0IPJu9XE+jEdD+pT/24PhTjTp//1mPPoBDF0ilNz4xB9noUQP8EAAD//2F+XUqlCgAA", } // GetSwagger returns the content of the embedded swagger specification file From a749a1c5850f66515fc0fa295d44a1deb559197d Mon Sep 17 00:00:00 2001 From: Sayan Samanta Date: Wed, 2 Jul 2025 15:27:02 -0700 Subject: [PATCH 5/6] allow downloading in progress recordings --- server/cmd/api/api/api.go | 14 +++++++++----- server/lib/recorder/ffmpeg.go | 4 ---- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/server/cmd/api/api/api.go b/server/cmd/api/api/api.go index 550ceb43..c4d7fe1e 100644 --- a/server/cmd/api/api/api.go +++ b/server/cmd/api/api/api.go @@ -99,17 +99,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 <= 100 { + return oapi.DownloadRecording202Response{ + Headers: oapi.DownloadRecording202ResponseHeaders{ + RetryAfter: 300, + }, + }, nil + } + log.Info("serving recording file for download", "size", meta.Size) return oapi.DownloadRecording200Videomp4Response{ Body: out, diff --git a/server/lib/recorder/ffmpeg.go b/server/lib/recorder/ffmpeg.go index 05fbc952..b3dbbe3a 100644 --- a/server/lib/recorder/ffmpeg.go +++ b/server/lib/recorder/ffmpeg.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) From 090da5265fe5b8f5f1ff1b3a02b8a180ae09aff8 Mon Sep 17 00:00:00 2001 From: Sayan Samanta Date: Wed, 2 Jul 2025 15:42:10 -0700 Subject: [PATCH 6/6] update tests --- server/cmd/api/api/api.go | 6 +++++- server/cmd/api/api/api_test.go | 30 ++++++++++++++++++++++++++++-- 2 files changed, 33 insertions(+), 3 deletions(-) diff --git a/server/cmd/api/api/api.go b/server/cmd/api/api/api.go index c4d7fe1e..a8140992 100644 --- a/server/cmd/api/api/api.go +++ b/server/cmd/api/api/api.go @@ -89,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) @@ -106,7 +110,7 @@ func (s *ApiService) DownloadRecording(ctx context.Context, req oapi.DownloadRec } // short-circuit if the recording is still in progress and the file is arbitrary small - if rec.IsRecording(ctx) && meta.Size <= 100 { + if rec.IsRecording(ctx) && meta.Size <= minRecordingSizeInBytes { return oapi.DownloadRecording202Response{ Headers: oapi.DownloadRecording202ResponseHeaders{ RetryAfter: 300, diff --git a/server/cmd/api/api/api_test.go b/server/cmd/api/api/api_test.go index 01d0dc75..7366256d 100644 --- a/server/cmd/api/api/api_test.go +++ b/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) {