From 1a8e693b3f76b00f7d0e713711b118f6fb451f38 Mon Sep 17 00:00:00 2001 From: Hiro Tamada Date: Mon, 23 Feb 2026 15:27:53 -0500 Subject: [PATCH 1/7] feat: gracefully stop and restart recordings during display resize Instead of refusing display resize requests (409) when recordings are active, PatchDisplay now stops active recordings, performs the resize, then restarts them with the same ID and params. The pre-resize segment is preserved by renaming it (e.g. `-before-resize-.mp4`). Live view sessions still block the resize as before. Co-authored-by: Cursor --- server/cmd/api/api/display.go | 128 ++++++++++++++- server/cmd/api/api/display_test.go | 241 +++++++++++++++++++++++++++++ server/lib/recorder/ffmeg_test.go | 32 ++++ server/lib/recorder/ffmpeg.go | 14 ++ 4 files changed, 407 insertions(+), 8 deletions(-) create mode 100644 server/cmd/api/api/display_test.go diff --git a/server/cmd/api/api/display.go b/server/cmd/api/api/display.go index 534f2a01..63b91573 100644 --- a/server/cmd/api/api/display.go +++ b/server/cmd/api/api/display.go @@ -8,10 +8,12 @@ import ( "os/exec" "strconv" "strings" + "time" nekooapi "github.com/m1k1o/neko/server/lib/oapi" "github.com/onkernel/kernel-images/server/lib/logger" oapi "github.com/onkernel/kernel-images/server/lib/oapi" + "github.com/onkernel/kernel-images/server/lib/recorder" ) // PatchDisplay updates the display configuration. When require_idle @@ -62,18 +64,29 @@ func (s *ApiService) PatchDisplay(ctx context.Context, req oapi.PatchDisplayRequ requireIdle = *req.Body.RequireIdle } - // Check if resize is safe (no active sessions or recordings) + // Check if resize is safe (no active live view sessions) if requireIdle { live := s.getActiveNekoSessions(ctx) - isRecording := s.anyRecordingActive(ctx) - resizableNow := (live == 0) && !isRecording - - log.Info("checking if resize is safe", "live_sessions", live, "is_recording", isRecording, "resizable", resizableNow) - - if !resizableNow { + if live > 0 { + log.Info("resize refused: live view or replay active", "live_sessions", live) return oapi.PatchDisplay409JSONResponse{ ConflictErrorJSONResponse: oapi.ConflictErrorJSONResponse{ - Message: "resize refused: live view or recording/replay active", + Message: "resize refused: live view or replay active", + }, + }, nil + } + + // Gracefully stop active recordings so the resize can proceed. + // They will be restarted (with new segment files) after the resize completes. + stopped, stopErr := s.stopActiveRecordings(ctx) + if len(stopped) > 0 { + defer s.restartRecordings(context.WithoutCancel(ctx), stopped) + } + if stopErr != nil { + log.Error("failed to stop recordings for resize", "error", stopErr) + return oapi.PatchDisplay500JSONResponse{ + InternalErrorJSONResponse: oapi.InternalErrorJSONResponse{ + Message: fmt.Sprintf("failed to stop recordings for resize: %s", stopErr.Error()), }, }, nil } @@ -361,6 +374,105 @@ func (s *ApiService) getCurrentResolution(ctx context.Context) (int, int, int, e return width, height, refreshRate, nil } +// stoppedRecordingInfo holds state captured from a recording that was stopped +// so it can be restarted after a display resize. +type stoppedRecordingInfo struct { + id string + params recorder.FFmpegRecordingParams + outputPath string +} + +// stopActiveRecordings gracefully stops every recording that is currently in +// progress and deregisters them from the manager. It returns info needed to +// restart each recording later. Recordings that were successfully stopped are +// always included in the returned slice, even when a later recording fails to +// stop (so the caller can restart whatever was stopped). +func (s *ApiService) stopActiveRecordings(ctx context.Context) ([]stoppedRecordingInfo, error) { + log := logger.FromContext(ctx) + var stopped []stoppedRecordingInfo + + for _, rec := range s.recordManager.ListActiveRecorders(ctx) { + if !rec.IsRecording(ctx) { + continue + } + + id := rec.ID() + + ffmpegRec, ok := rec.(*recorder.FFmpegRecorder) + if !ok { + log.Warn("cannot capture params from non-FFmpeg recorder, skipping", "id", id) + continue + } + + params := ffmpegRec.Params() + outputPath := ffmpegRec.OutputPath() + + log.Info("stopping recording for resize", "id", id) + if err := rec.Stop(ctx); err != nil { + // Stop() returns finalization errors even when the process was + // successfully terminated. Only treat it as a hard failure if + // the process is still running. + if rec.IsRecording(ctx) { + log.Error("failed to stop recording for resize", "id", id, "error", err) + return stopped, fmt.Errorf("failed to stop recording %s: %w", id, err) + } + log.Warn("recording stopped with finalization warning", "id", id, "error", err) + } + + if err := s.recordManager.DeregisterRecorder(ctx, rec); err != nil { + log.Error("failed to deregister recorder", "id", id, "error", err) + } + + stopped = append(stopped, stoppedRecordingInfo{ + id: id, + params: params, + outputPath: outputPath, + }) + log.Info("recording stopped and deregistered for resize", "id", id) + } + + return stopped, nil +} + +// restartRecordings re-creates and starts recordings that were previously +// stopped for a display resize. The old (finalized) recording file is renamed +// to preserve it before the new recording begins at the same output path. +func (s *ApiService) restartRecordings(ctx context.Context, stopped []stoppedRecordingInfo) { + log := logger.FromContext(ctx) + + for _, info := range stopped { + // Preserve the pre-resize segment by renaming the finalized file. + if _, err := os.Stat(info.outputPath); err == nil { + preservedPath := strings.TrimSuffix(info.outputPath, ".mp4") + + fmt.Sprintf("-before-resize-%d.mp4", time.Now().UnixMilli()) + if err := os.Rename(info.outputPath, preservedPath); err != nil { + log.Error("failed to rename pre-resize recording", "id", info.id, "error", err) + continue + } + log.Info("preserved pre-resize recording segment", "id", info.id, "path", preservedPath) + } + + rec, err := s.factory(info.id, info.params) + if err != nil { + log.Error("failed to create recorder for restart", "id", info.id, "error", err) + continue + } + + if err := s.recordManager.RegisterRecorder(ctx, rec); err != nil { + log.Error("failed to register restarted recorder", "id", info.id, "error", err) + continue + } + + if err := rec.Start(ctx); err != nil { + log.Error("failed to start restarted recording", "id", info.id, "error", err) + _ = s.recordManager.DeregisterRecorder(ctx, rec) + continue + } + + log.Info("recording restarted after resize", "id", info.id) + } +} + // isNekoEnabled checks if Neko service is enabled func (s *ApiService) isNekoEnabled() bool { return os.Getenv("ENABLE_WEBRTC") == "true" diff --git a/server/cmd/api/api/display_test.go b/server/cmd/api/api/display_test.go new file mode 100644 index 00000000..05ce1e2c --- /dev/null +++ b/server/cmd/api/api/display_test.go @@ -0,0 +1,241 @@ +package api + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/onkernel/kernel-images/server/lib/recorder" + "github.com/onkernel/kernel-images/server/lib/scaletozero" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +var testMockFFmpegBin = filepath.Join("..", "..", "..", "lib", "recorder", "testdata", "mock_ffmpeg.sh") + +func testFFmpegFactory(t *testing.T, tempDir string) recorder.FFmpegRecorderFactory { + t.Helper() + fr := 5 + disp := 0 + size := 1 + config := recorder.FFmpegRecordingParams{ + FrameRate: &fr, + DisplayNum: &disp, + MaxSizeInMB: &size, + OutputDir: &tempDir, + } + return recorder.NewFFmpegRecorderFactory(testMockFFmpegBin, config, scaletozero.NewNoopController()) +} + +func newTestServiceWithFactory(t *testing.T, mgr recorder.RecordManager, factory recorder.FFmpegRecorderFactory) *ApiService { + t.Helper() + svc, err := New(mgr, factory, newTestUpstreamManager(), scaletozero.NewNoopController(), newMockNekoClient(t)) + require.NoError(t, err) + return svc +} + +func TestStopActiveRecordings(t *testing.T) { + t.Run("stops and deregisters active recording", func(t *testing.T) { + ctx := context.Background() + tempDir := t.TempDir() + factory := testFFmpegFactory(t, tempDir) + mgr := recorder.NewFFmpegManager() + svc := newTestServiceWithFactory(t, mgr, factory) + + rec, err := factory("test-rec", recorder.FFmpegRecordingParams{}) + require.NoError(t, err) + require.NoError(t, mgr.RegisterRecorder(ctx, rec)) + require.NoError(t, rec.Start(ctx)) + time.Sleep(50 * time.Millisecond) + require.True(t, rec.IsRecording(ctx)) + + stopped, err := svc.stopActiveRecordings(ctx) + require.NoError(t, err) + require.Len(t, stopped, 1) + assert.Equal(t, "test-rec", stopped[0].id) + assert.NotNil(t, stopped[0].params.FrameRate) + assert.Equal(t, filepath.Join(tempDir, "test-rec.mp4"), stopped[0].outputPath) + + _, exists := mgr.GetRecorder("test-rec") + assert.False(t, exists, "recorder should be deregistered after stop") + }) + + t.Run("stops multiple active recordings", func(t *testing.T) { + ctx := context.Background() + tempDir := t.TempDir() + factory := testFFmpegFactory(t, tempDir) + mgr := recorder.NewFFmpegManager() + svc := newTestServiceWithFactory(t, mgr, factory) + + ids := []string{"rec-a", "rec-b"} + for _, id := range ids { + rec, err := factory(id, recorder.FFmpegRecordingParams{}) + require.NoError(t, err) + require.NoError(t, mgr.RegisterRecorder(ctx, rec)) + require.NoError(t, rec.Start(ctx)) + } + time.Sleep(50 * time.Millisecond) + + stopped, err := svc.stopActiveRecordings(ctx) + require.NoError(t, err) + assert.Len(t, stopped, 2) + + stoppedIDs := map[string]bool{} + for _, s := range stopped { + stoppedIDs[s.id] = true + } + for _, id := range ids { + assert.True(t, stoppedIDs[id], "recording %s should have been stopped", id) + _, exists := mgr.GetRecorder(id) + assert.False(t, exists, "recorder %s should be deregistered", id) + } + }) + + t.Run("skips non-recording recorders", func(t *testing.T) { + ctx := context.Background() + tempDir := t.TempDir() + factory := testFFmpegFactory(t, tempDir) + mgr := recorder.NewFFmpegManager() + svc := newTestServiceWithFactory(t, mgr, factory) + + mock := &mockRecorder{id: "idle-rec", isRecordingFlag: false} + require.NoError(t, mgr.RegisterRecorder(ctx, mock)) + + stopped, err := svc.stopActiveRecordings(ctx) + require.NoError(t, err) + assert.Empty(t, stopped) + + _, exists := mgr.GetRecorder("idle-rec") + assert.True(t, exists, "non-recording recorder should remain registered") + }) + + t.Run("returns empty when no recorders exist", func(t *testing.T) { + ctx := context.Background() + tempDir := t.TempDir() + factory := testFFmpegFactory(t, tempDir) + mgr := recorder.NewFFmpegManager() + svc := newTestServiceWithFactory(t, mgr, factory) + + stopped, err := svc.stopActiveRecordings(ctx) + require.NoError(t, err) + assert.Empty(t, stopped) + }) +} + +func TestRestartRecordings(t *testing.T) { + t.Run("renames old file and starts new recording", func(t *testing.T) { + ctx := context.Background() + tempDir := t.TempDir() + factory := testFFmpegFactory(t, tempDir) + mgr := recorder.NewFFmpegManager() + svc := newTestServiceWithFactory(t, mgr, factory) + + outputPath := filepath.Join(tempDir, "test-rec.mp4") + require.NoError(t, os.WriteFile(outputPath, []byte("fake video data"), 0644)) + + fr := 5 + disp := 0 + size := 1 + info := stoppedRecordingInfo{ + id: "test-rec", + params: recorder.FFmpegRecordingParams{ + FrameRate: &fr, + DisplayNum: &disp, + MaxSizeInMB: &size, + OutputDir: &tempDir, + }, + outputPath: outputPath, + } + + svc.restartRecordings(ctx, []stoppedRecordingInfo{info}) + + entries, err := os.ReadDir(tempDir) + require.NoError(t, err) + + foundRenamed := false + for _, e := range entries { + if strings.Contains(e.Name(), "before-resize") { + foundRenamed = true + data, readErr := os.ReadFile(filepath.Join(tempDir, e.Name())) + require.NoError(t, readErr) + assert.Equal(t, []byte("fake video data"), data) + } + } + assert.True(t, foundRenamed, "pre-resize recording should be preserved with renamed file") + + rec, exists := mgr.GetRecorder("test-rec") + require.True(t, exists, "restarted recorder should be registered") + assert.True(t, rec.IsRecording(ctx), "restarted recorder should be recording") + + _ = rec.Stop(ctx) + }) + + t.Run("starts recording even when no old file exists", func(t *testing.T) { + ctx := context.Background() + tempDir := t.TempDir() + factory := testFFmpegFactory(t, tempDir) + mgr := recorder.NewFFmpegManager() + svc := newTestServiceWithFactory(t, mgr, factory) + + fr := 5 + disp := 0 + size := 1 + info := stoppedRecordingInfo{ + id: "fresh-rec", + params: recorder.FFmpegRecordingParams{ + FrameRate: &fr, + DisplayNum: &disp, + MaxSizeInMB: &size, + OutputDir: &tempDir, + }, + outputPath: filepath.Join(tempDir, "fresh-rec.mp4"), + } + + svc.restartRecordings(ctx, []stoppedRecordingInfo{info}) + + rec, exists := mgr.GetRecorder("fresh-rec") + require.True(t, exists, "recorder should be registered") + assert.True(t, rec.IsRecording(ctx)) + + _ = rec.Stop(ctx) + }) +} + +func TestStopAndRestartRecordings_RoundTrip(t *testing.T) { + ctx := context.Background() + tempDir := t.TempDir() + factory := testFFmpegFactory(t, tempDir) + mgr := recorder.NewFFmpegManager() + svc := newTestServiceWithFactory(t, mgr, factory) + + // Start a recording + rec, err := factory("round-trip", recorder.FFmpegRecordingParams{}) + require.NoError(t, err) + require.NoError(t, mgr.RegisterRecorder(ctx, rec)) + require.NoError(t, rec.Start(ctx)) + time.Sleep(50 * time.Millisecond) + require.True(t, rec.IsRecording(ctx)) + + // Stop all active recordings + stopped, err := svc.stopActiveRecordings(ctx) + require.NoError(t, err) + require.Len(t, stopped, 1) + assert.Equal(t, "round-trip", stopped[0].id) + + // Verify the recorder was deregistered + _, exists := mgr.GetRecorder("round-trip") + require.False(t, exists) + + // Restart recordings + svc.restartRecordings(ctx, stopped) + + // Verify the recording resumed with the same ID + newRec, exists := mgr.GetRecorder("round-trip") + require.True(t, exists, "recorder should be re-registered after restart") + assert.True(t, newRec.IsRecording(ctx), "recorder should be actively recording") + + _ = newRec.Stop(ctx) +} diff --git a/server/lib/recorder/ffmeg_test.go b/server/lib/recorder/ffmeg_test.go index 22aea379..09c1bbe7 100644 --- a/server/lib/recorder/ffmeg_test.go +++ b/server/lib/recorder/ffmeg_test.go @@ -50,6 +50,38 @@ func TestFFmpegRecorder_StartAndStop(t *testing.T) { require.False(t, rec.IsRecording(t.Context())) } +func TestFFmpegRecorder_Params(t *testing.T) { + tempDir := t.TempDir() + params := defaultParams(tempDir) + rec := &FFmpegRecorder{ + id: "params-test", + binaryPath: mockBin, + params: params, + outputPath: filepath.Join(tempDir, "params-test.mp4"), + stz: scaletozero.NewOncer(scaletozero.NewNoopController()), + } + + got := rec.Params() + assert.Equal(t, *params.FrameRate, *got.FrameRate) + assert.Equal(t, *params.DisplayNum, *got.DisplayNum) + assert.Equal(t, *params.MaxSizeInMB, *got.MaxSizeInMB) + assert.Equal(t, *params.OutputDir, *got.OutputDir) +} + +func TestFFmpegRecorder_OutputPath(t *testing.T) { + tempDir := t.TempDir() + expected := filepath.Join(tempDir, "path-test.mp4") + rec := &FFmpegRecorder{ + id: "path-test", + binaryPath: mockBin, + params: defaultParams(tempDir), + outputPath: expected, + stz: scaletozero.NewOncer(scaletozero.NewNoopController()), + } + + assert.Equal(t, expected, rec.OutputPath()) +} + func TestFFmpegRecorder_ForceStop(t *testing.T) { tempDir := t.TempDir() rec := &FFmpegRecorder{ diff --git a/server/lib/recorder/ffmpeg.go b/server/lib/recorder/ffmpeg.go index 52e6b054..0c6e9953 100644 --- a/server/lib/recorder/ffmpeg.go +++ b/server/lib/recorder/ffmpeg.go @@ -141,6 +141,20 @@ func (fr *FFmpegRecorder) ID() string { return fr.id } +// Params returns a copy of the merged recording parameters. +func (fr *FFmpegRecorder) Params() FFmpegRecordingParams { + fr.mu.Lock() + defer fr.mu.Unlock() + return fr.params +} + +// OutputPath returns the filesystem path where the recording is stored. +func (fr *FFmpegRecorder) OutputPath() string { + fr.mu.Lock() + defer fr.mu.Unlock() + return fr.outputPath +} + // Start begins the recording process by launching ffmpeg with the configured parameters. func (fr *FFmpegRecorder) Start(ctx context.Context) error { log := logger.FromContext(ctx) From 3d62c2abb9ea7589af80914c8c1fe05c49b5c214 Mon Sep 17 00:00:00 2001 From: Hiro Tamada Date: Mon, 23 Feb 2026 15:41:08 -0500 Subject: [PATCH 2/7] fix: address PR review feedback for recording resize - Don't block recording restart when rename of old file fails (rename is best-effort to preserve the pre-resize segment) - Deep copy pointer fields in FFmpegRecordingParams.clone() so Params() callers cannot mutate recorder internals Co-authored-by: Cursor --- server/cmd/api/api/display.go | 9 +++++---- server/lib/recorder/ffmpeg.go | 29 +++++++++++++++++++++++++++-- 2 files changed, 32 insertions(+), 6 deletions(-) diff --git a/server/cmd/api/api/display.go b/server/cmd/api/api/display.go index 63b91573..a31e6206 100644 --- a/server/cmd/api/api/display.go +++ b/server/cmd/api/api/display.go @@ -441,15 +441,16 @@ func (s *ApiService) restartRecordings(ctx context.Context, stopped []stoppedRec log := logger.FromContext(ctx) for _, info := range stopped { - // Preserve the pre-resize segment by renaming the finalized file. + // Best-effort: preserve the pre-resize segment by renaming the finalized file. + // If this fails the old file may be overwritten, but we still restart recording. if _, err := os.Stat(info.outputPath); err == nil { preservedPath := strings.TrimSuffix(info.outputPath, ".mp4") + fmt.Sprintf("-before-resize-%d.mp4", time.Now().UnixMilli()) if err := os.Rename(info.outputPath, preservedPath); err != nil { - log.Error("failed to rename pre-resize recording", "id", info.id, "error", err) - continue + log.Error("failed to rename pre-resize recording, old file may be overwritten", "id", info.id, "error", err) + } else { + log.Info("preserved pre-resize recording segment", "id", info.id, "path", preservedPath) } - log.Info("preserved pre-resize recording segment", "id", info.id, "path", preservedPath) } rec, err := s.factory(info.id, info.params) diff --git a/server/lib/recorder/ffmpeg.go b/server/lib/recorder/ffmpeg.go index 0c6e9953..20f19a70 100644 --- a/server/lib/recorder/ffmpeg.go +++ b/server/lib/recorder/ffmpeg.go @@ -141,11 +141,36 @@ func (fr *FFmpegRecorder) ID() string { return fr.id } -// Params returns a copy of the merged recording parameters. +// Params returns a deep copy of the merged recording parameters. func (fr *FFmpegRecorder) Params() FFmpegRecordingParams { fr.mu.Lock() defer fr.mu.Unlock() - return fr.params + return fr.params.clone() +} + +func (p FFmpegRecordingParams) clone() FFmpegRecordingParams { + c := p + if p.FrameRate != nil { + v := *p.FrameRate + c.FrameRate = &v + } + if p.DisplayNum != nil { + v := *p.DisplayNum + c.DisplayNum = &v + } + if p.MaxSizeInMB != nil { + v := *p.MaxSizeInMB + c.MaxSizeInMB = &v + } + if p.MaxDurationInSeconds != nil { + v := *p.MaxDurationInSeconds + c.MaxDurationInSeconds = &v + } + if p.OutputDir != nil { + v := *p.OutputDir + c.OutputDir = &v + } + return c } // OutputPath returns the filesystem path where the recording is stored. From 0820c4ae1f20479db53821bcf7860190f95d1347 Mon Sep 17 00:00:00 2001 From: Hiro Tamada Date: Mon, 23 Feb 2026 16:13:00 -0500 Subject: [PATCH 3/7] fix: skip restart when deregister fails to avoid ID conflict If DeregisterRecorder fails, the old recorder remains registered. Appending to the stopped list would cause RegisterRecorder to fail with a duplicate ID during restart. Co-authored-by: Cursor --- server/cmd/api/api/display.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/server/cmd/api/api/display.go b/server/cmd/api/api/display.go index a31e6206..88943284 100644 --- a/server/cmd/api/api/display.go +++ b/server/cmd/api/api/display.go @@ -420,7 +420,8 @@ func (s *ApiService) stopActiveRecordings(ctx context.Context) ([]stoppedRecordi } if err := s.recordManager.DeregisterRecorder(ctx, rec); err != nil { - log.Error("failed to deregister recorder", "id", id, "error", err) + log.Error("failed to deregister recorder, skipping restart to avoid ID conflict", "id", id, "error", err) + continue } stopped = append(stopped, stoppedRecordingInfo{ From c4fd4c7716a07d5ccc90d37d49c63b241d1eb74e Mon Sep 17 00:00:00 2001 From: Hiro Tamada Date: Tue, 3 Mar 2026 11:54:29 -0500 Subject: [PATCH 4/7] refactor: use segment-based recording restart on display resize Instead of deregistering old recorders and renaming files, stopped recorders now remain registered (keeping their finalized files discoverable via the API) and new segments are started with unique suffixed IDs. This removes file-rename complexity and keeps all recording segments accessible through existing list/download endpoints. Also moves the graceful stop/restart logic outside the requireIdle block so it applies unconditionally when recordings are active. Made-with: Cursor --- server/cmd/api/api/display.go | 96 ++++++++++-------------- server/cmd/api/api/display_test.go | 116 +++++++++++++++-------------- server/lib/recorder/ffmeg_test.go | 14 ---- server/lib/recorder/ffmpeg.go | 7 -- 4 files changed, 102 insertions(+), 131 deletions(-) diff --git a/server/cmd/api/api/display.go b/server/cmd/api/api/display.go index 88943284..c9cef61e 100644 --- a/server/cmd/api/api/display.go +++ b/server/cmd/api/api/display.go @@ -64,32 +64,36 @@ func (s *ApiService) PatchDisplay(ctx context.Context, req oapi.PatchDisplayRequ requireIdle = *req.Body.RequireIdle } - // Check if resize is safe (no active live view sessions) + // Check if resize is safe (no active sessions or recordings) if requireIdle { live := s.getActiveNekoSessions(ctx) - if live > 0 { - log.Info("resize refused: live view or replay active", "live_sessions", live) + isRecording := s.anyRecordingActive(ctx) + resizableNow := (live == 0) && !isRecording + + log.Info("checking if resize is safe", "live_sessions", live, "is_recording", isRecording, "resizable", resizableNow) + + if !resizableNow { return oapi.PatchDisplay409JSONResponse{ ConflictErrorJSONResponse: oapi.ConflictErrorJSONResponse{ - Message: "resize refused: live view or replay active", + Message: "resize refused: live view or recording/replay active", }, }, nil } + } - // Gracefully stop active recordings so the resize can proceed. - // They will be restarted (with new segment files) after the resize completes. - stopped, stopErr := s.stopActiveRecordings(ctx) - if len(stopped) > 0 { - defer s.restartRecordings(context.WithoutCancel(ctx), stopped) - } - if stopErr != nil { - log.Error("failed to stop recordings for resize", "error", stopErr) - return oapi.PatchDisplay500JSONResponse{ - InternalErrorJSONResponse: oapi.InternalErrorJSONResponse{ - Message: fmt.Sprintf("failed to stop recordings for resize: %s", stopErr.Error()), - }, - }, nil - } + // Gracefully stop active recordings so the resize can proceed. + // New recording segments (with unique IDs) will be started after the resize. + stopped, stopErr := s.stopActiveRecordings(ctx) + if len(stopped) > 0 { + defer s.startNewRecordingSegments(context.WithoutCancel(ctx), stopped) + } + if stopErr != nil { + log.Error("failed to stop recordings for resize", "error", stopErr) + return oapi.PatchDisplay500JSONResponse{ + InternalErrorJSONResponse: oapi.InternalErrorJSONResponse{ + Message: fmt.Sprintf("failed to stop recordings for resize: %s", stopErr), + }, + }, nil } // Detect display mode (xorg or xvfb) @@ -377,16 +381,14 @@ func (s *ApiService) getCurrentResolution(ctx context.Context) (int, int, int, e // stoppedRecordingInfo holds state captured from a recording that was stopped // so it can be restarted after a display resize. type stoppedRecordingInfo struct { - id string - params recorder.FFmpegRecordingParams - outputPath string + id string + params recorder.FFmpegRecordingParams } // stopActiveRecordings gracefully stops every recording that is currently in -// progress and deregisters them from the manager. It returns info needed to -// restart each recording later. Recordings that were successfully stopped are -// always included in the returned slice, even when a later recording fails to -// stop (so the caller can restart whatever was stopped). +// progress. The old recorders remain registered in the manager so their +// finalized files stay discoverable and downloadable. It returns info needed +// to start a new recording segment for each stopped recorder. func (s *ApiService) stopActiveRecordings(ctx context.Context) ([]stoppedRecordingInfo, error) { log := logger.FromContext(ctx) var stopped []stoppedRecordingInfo @@ -405,7 +407,6 @@ func (s *ApiService) stopActiveRecordings(ctx context.Context) ([]stoppedRecordi } params := ffmpegRec.Params() - outputPath := ffmpegRec.OutputPath() log.Info("stopping recording for resize", "id", id) if err := rec.Stop(ctx); err != nil { @@ -419,59 +420,44 @@ func (s *ApiService) stopActiveRecordings(ctx context.Context) ([]stoppedRecordi log.Warn("recording stopped with finalization warning", "id", id, "error", err) } - if err := s.recordManager.DeregisterRecorder(ctx, rec); err != nil { - log.Error("failed to deregister recorder, skipping restart to avoid ID conflict", "id", id, "error", err) - continue - } - stopped = append(stopped, stoppedRecordingInfo{ - id: id, - params: params, - outputPath: outputPath, + id: id, + params: params, }) - log.Info("recording stopped and deregistered for resize", "id", id) + log.Info("recording stopped for resize, old segment preserved", "id", id) } return stopped, nil } -// restartRecordings re-creates and starts recordings that were previously -// stopped for a display resize. The old (finalized) recording file is renamed -// to preserve it before the new recording begins at the same output path. -func (s *ApiService) restartRecordings(ctx context.Context, stopped []stoppedRecordingInfo) { +// startNewRecordingSegments creates and starts a new recording segment for +// each previously-stopped recorder. Each new segment gets a unique suffixed +// ID so the old (stopped) recorder and its finalized file remain accessible +// in the manager. +func (s *ApiService) startNewRecordingSegments(ctx context.Context, stopped []stoppedRecordingInfo) { log := logger.FromContext(ctx) for _, info := range stopped { - // Best-effort: preserve the pre-resize segment by renaming the finalized file. - // If this fails the old file may be overwritten, but we still restart recording. - if _, err := os.Stat(info.outputPath); err == nil { - preservedPath := strings.TrimSuffix(info.outputPath, ".mp4") + - fmt.Sprintf("-before-resize-%d.mp4", time.Now().UnixMilli()) - if err := os.Rename(info.outputPath, preservedPath); err != nil { - log.Error("failed to rename pre-resize recording, old file may be overwritten", "id", info.id, "error", err) - } else { - log.Info("preserved pre-resize recording segment", "id", info.id, "path", preservedPath) - } - } + newID := fmt.Sprintf("%s-%d", info.id, time.Now().UnixMilli()) - rec, err := s.factory(info.id, info.params) + rec, err := s.factory(newID, info.params) if err != nil { - log.Error("failed to create recorder for restart", "id", info.id, "error", err) + log.Error("failed to create recorder for new segment", "old_id", info.id, "new_id", newID, "error", err) continue } if err := s.recordManager.RegisterRecorder(ctx, rec); err != nil { - log.Error("failed to register restarted recorder", "id", info.id, "error", err) + log.Error("failed to register new segment recorder", "old_id", info.id, "new_id", newID, "error", err) continue } if err := rec.Start(ctx); err != nil { - log.Error("failed to start restarted recording", "id", info.id, "error", err) + log.Error("failed to start new segment recording", "old_id", info.id, "new_id", newID, "error", err) _ = s.recordManager.DeregisterRecorder(ctx, rec) continue } - log.Info("recording restarted after resize", "id", info.id) + log.Info("new recording segment started after resize", "old_id", info.id, "new_id", newID) } } diff --git a/server/cmd/api/api/display_test.go b/server/cmd/api/api/display_test.go index 05ce1e2c..72780ea8 100644 --- a/server/cmd/api/api/display_test.go +++ b/server/cmd/api/api/display_test.go @@ -2,9 +2,7 @@ package api import ( "context" - "os" "path/filepath" - "strings" "testing" "time" @@ -38,7 +36,7 @@ func newTestServiceWithFactory(t *testing.T, mgr recorder.RecordManager, factory } func TestStopActiveRecordings(t *testing.T) { - t.Run("stops and deregisters active recording", func(t *testing.T) { + t.Run("stops recording but keeps it registered", func(t *testing.T) { ctx := context.Background() tempDir := t.TempDir() factory := testFFmpegFactory(t, tempDir) @@ -57,10 +55,10 @@ func TestStopActiveRecordings(t *testing.T) { require.Len(t, stopped, 1) assert.Equal(t, "test-rec", stopped[0].id) assert.NotNil(t, stopped[0].params.FrameRate) - assert.Equal(t, filepath.Join(tempDir, "test-rec.mp4"), stopped[0].outputPath) - _, exists := mgr.GetRecorder("test-rec") - assert.False(t, exists, "recorder should be deregistered after stop") + oldRec, exists := mgr.GetRecorder("test-rec") + assert.True(t, exists, "old recorder should remain registered") + assert.False(t, oldRec.IsRecording(ctx), "old recorder should be stopped") }) t.Run("stops multiple active recordings", func(t *testing.T) { @@ -83,14 +81,10 @@ func TestStopActiveRecordings(t *testing.T) { require.NoError(t, err) assert.Len(t, stopped, 2) - stoppedIDs := map[string]bool{} - for _, s := range stopped { - stoppedIDs[s.id] = true - } for _, id := range ids { - assert.True(t, stoppedIDs[id], "recording %s should have been stopped", id) - _, exists := mgr.GetRecorder(id) - assert.False(t, exists, "recorder %s should be deregistered", id) + oldRec, exists := mgr.GetRecorder(id) + assert.True(t, exists, "recorder %s should remain registered", id) + assert.False(t, oldRec.IsRecording(ctx), "recorder %s should be stopped", id) } }) @@ -125,17 +119,14 @@ func TestStopActiveRecordings(t *testing.T) { }) } -func TestRestartRecordings(t *testing.T) { - t.Run("renames old file and starts new recording", func(t *testing.T) { +func TestStartNewRecordingSegments(t *testing.T) { + t.Run("creates new segment with suffixed ID", func(t *testing.T) { ctx := context.Background() tempDir := t.TempDir() factory := testFFmpegFactory(t, tempDir) mgr := recorder.NewFFmpegManager() svc := newTestServiceWithFactory(t, mgr, factory) - outputPath := filepath.Join(tempDir, "test-rec.mp4") - require.NoError(t, os.WriteFile(outputPath, []byte("fake video data"), 0644)) - fr := 5 disp := 0 size := 1 @@ -147,33 +138,30 @@ func TestRestartRecordings(t *testing.T) { MaxSizeInMB: &size, OutputDir: &tempDir, }, - outputPath: outputPath, } - svc.restartRecordings(ctx, []stoppedRecordingInfo{info}) + svc.startNewRecordingSegments(ctx, []stoppedRecordingInfo{info}) - entries, err := os.ReadDir(tempDir) - require.NoError(t, err) + // The new recorder should have a suffixed ID, not the original + _, existsOld := mgr.GetRecorder("test-rec") + assert.False(t, existsOld, "original ID should not be re-registered") - foundRenamed := false - for _, e := range entries { - if strings.Contains(e.Name(), "before-resize") { - foundRenamed = true - data, readErr := os.ReadFile(filepath.Join(tempDir, e.Name())) - require.NoError(t, readErr) - assert.Equal(t, []byte("fake video data"), data) + // Find the new segment by iterating active recorders + var newRec recorder.Recorder + for _, r := range mgr.ListActiveRecorders(ctx) { + if r.IsRecording(ctx) { + newRec = r + break } } - assert.True(t, foundRenamed, "pre-resize recording should be preserved with renamed file") + require.NotNil(t, newRec, "a new recording segment should be active") + assert.Contains(t, newRec.ID(), "test-rec-", "new ID should be prefixed with the original ID") + assert.True(t, newRec.IsRecording(ctx)) - rec, exists := mgr.GetRecorder("test-rec") - require.True(t, exists, "restarted recorder should be registered") - assert.True(t, rec.IsRecording(ctx), "restarted recorder should be recording") - - _ = rec.Stop(ctx) + _ = newRec.Stop(ctx) }) - t.Run("starts recording even when no old file exists", func(t *testing.T) { + t.Run("starts segment even when no old recorder exists in manager", func(t *testing.T) { ctx := context.Background() tempDir := t.TempDir() factory := testFFmpegFactory(t, tempDir) @@ -191,20 +179,25 @@ func TestRestartRecordings(t *testing.T) { MaxSizeInMB: &size, OutputDir: &tempDir, }, - outputPath: filepath.Join(tempDir, "fresh-rec.mp4"), } - svc.restartRecordings(ctx, []stoppedRecordingInfo{info}) + svc.startNewRecordingSegments(ctx, []stoppedRecordingInfo{info}) - rec, exists := mgr.GetRecorder("fresh-rec") - require.True(t, exists, "recorder should be registered") - assert.True(t, rec.IsRecording(ctx)) + var newRec recorder.Recorder + for _, r := range mgr.ListActiveRecorders(ctx) { + if r.IsRecording(ctx) { + newRec = r + break + } + } + require.NotNil(t, newRec, "new segment should be active") + assert.Contains(t, newRec.ID(), "fresh-rec-") - _ = rec.Stop(ctx) + _ = newRec.Stop(ctx) }) } -func TestStopAndRestartRecordings_RoundTrip(t *testing.T) { +func TestStopAndStartNewSegment_RoundTrip(t *testing.T) { ctx := context.Background() tempDir := t.TempDir() factory := testFFmpegFactory(t, tempDir) @@ -219,23 +212,36 @@ func TestStopAndRestartRecordings_RoundTrip(t *testing.T) { time.Sleep(50 * time.Millisecond) require.True(t, rec.IsRecording(ctx)) - // Stop all active recordings + // Stop active recordings (simulating resize) stopped, err := svc.stopActiveRecordings(ctx) require.NoError(t, err) require.Len(t, stopped, 1) assert.Equal(t, "round-trip", stopped[0].id) - // Verify the recorder was deregistered - _, exists := mgr.GetRecorder("round-trip") - require.False(t, exists) - - // Restart recordings - svc.restartRecordings(ctx, stopped) - - // Verify the recording resumed with the same ID - newRec, exists := mgr.GetRecorder("round-trip") - require.True(t, exists, "recorder should be re-registered after restart") - assert.True(t, newRec.IsRecording(ctx), "recorder should be actively recording") + // Old recorder should still be registered but stopped + oldRec, exists := mgr.GetRecorder("round-trip") + require.True(t, exists, "old recorder should remain registered") + assert.False(t, oldRec.IsRecording(ctx), "old recorder should be stopped") + + // Start new segments + svc.startNewRecordingSegments(ctx, stopped) + + // Old recorder should still be there + oldRec2, exists := mgr.GetRecorder("round-trip") + require.True(t, exists, "old recorder should still be registered after new segment starts") + assert.False(t, oldRec2.IsRecording(ctx)) + + // New recorder should be active with a different ID + var newRec recorder.Recorder + for _, r := range mgr.ListActiveRecorders(ctx) { + if r.ID() != "round-trip" && r.IsRecording(ctx) { + newRec = r + break + } + } + require.NotNil(t, newRec, "new segment recorder should exist") + assert.Contains(t, newRec.ID(), "round-trip-", "new ID should be suffixed") + assert.True(t, newRec.IsRecording(ctx)) _ = newRec.Stop(ctx) } diff --git a/server/lib/recorder/ffmeg_test.go b/server/lib/recorder/ffmeg_test.go index 09c1bbe7..bb739bb1 100644 --- a/server/lib/recorder/ffmeg_test.go +++ b/server/lib/recorder/ffmeg_test.go @@ -68,20 +68,6 @@ func TestFFmpegRecorder_Params(t *testing.T) { assert.Equal(t, *params.OutputDir, *got.OutputDir) } -func TestFFmpegRecorder_OutputPath(t *testing.T) { - tempDir := t.TempDir() - expected := filepath.Join(tempDir, "path-test.mp4") - rec := &FFmpegRecorder{ - id: "path-test", - binaryPath: mockBin, - params: defaultParams(tempDir), - outputPath: expected, - stz: scaletozero.NewOncer(scaletozero.NewNoopController()), - } - - assert.Equal(t, expected, rec.OutputPath()) -} - func TestFFmpegRecorder_ForceStop(t *testing.T) { tempDir := t.TempDir() rec := &FFmpegRecorder{ diff --git a/server/lib/recorder/ffmpeg.go b/server/lib/recorder/ffmpeg.go index 20f19a70..d3d2dc80 100644 --- a/server/lib/recorder/ffmpeg.go +++ b/server/lib/recorder/ffmpeg.go @@ -173,13 +173,6 @@ func (p FFmpegRecordingParams) clone() FFmpegRecordingParams { return c } -// OutputPath returns the filesystem path where the recording is stored. -func (fr *FFmpegRecorder) OutputPath() string { - fr.mu.Lock() - defer fr.mu.Unlock() - return fr.outputPath -} - // Start begins the recording process by launching ffmpeg with the configured parameters. func (fr *FFmpegRecorder) Start(ctx context.Context) error { log := logger.FromContext(ctx) From d2fc5a4582c0a2a03cad6f553020c283332fc0ea Mon Sep 17 00:00:00 2001 From: Hiro Tamada Date: Wed, 4 Mar 2026 10:46:59 -0500 Subject: [PATCH 5/7] fix: make recording duration and size limits cumulative across segments When a display resize splits a recording into segments, the new segment's MaxDurationInSeconds and MaxSizeInMB are now reduced by what the prior segment already consumed, keeping cumulative usage within the originally requested limits. Made-with: Cursor --- server/cmd/api/api/display.go | 52 +++++++++-- server/cmd/api/api/display_test.go | 140 +++++++++++++++++++++++++++++ 2 files changed, 187 insertions(+), 5 deletions(-) diff --git a/server/cmd/api/api/display.go b/server/cmd/api/api/display.go index c9cef61e..0296adb8 100644 --- a/server/cmd/api/api/display.go +++ b/server/cmd/api/api/display.go @@ -4,8 +4,10 @@ import ( "context" "encoding/base64" "fmt" + "log/slog" "os" "os/exec" + "path/filepath" "strconv" "strings" "time" @@ -381,8 +383,9 @@ func (s *ApiService) getCurrentResolution(ctx context.Context) (int, int, int, e // stoppedRecordingInfo holds state captured from a recording that was stopped // so it can be restarted after a display resize. type stoppedRecordingInfo struct { - id string - params recorder.FFmpegRecordingParams + id string + params recorder.FFmpegRecordingParams + metadata *recorder.RecordingMetadata } // stopActiveRecordings gracefully stops every recording that is currently in @@ -421,8 +424,9 @@ func (s *ApiService) stopActiveRecordings(ctx context.Context) ([]stoppedRecordi } stopped = append(stopped, stoppedRecordingInfo{ - id: id, - params: params, + id: id, + params: params, + metadata: rec.Metadata(), }) log.Info("recording stopped for resize, old segment preserved", "id", id) } @@ -430,17 +434,55 @@ func (s *ApiService) stopActiveRecordings(ctx context.Context) ([]stoppedRecordi return stopped, nil } +// adjustParamsForRemainingBudget reduces MaxDurationInSeconds and MaxSizeInMB +// in the cloned params to reflect what the previous segment already consumed. +// This keeps cumulative duration and disk usage within the originally requested limits. +func adjustParamsForRemainingBudget(log *slog.Logger, info stoppedRecordingInfo) recorder.FFmpegRecordingParams { + params := info.params + + if params.MaxDurationInSeconds != nil && info.metadata != nil && !info.metadata.EndTime.IsZero() { + elapsed := int(info.metadata.EndTime.Sub(info.metadata.StartTime).Seconds()) + remaining := *params.MaxDurationInSeconds - elapsed + if remaining < 1 { + remaining = 1 + } + params.MaxDurationInSeconds = &remaining + log.Info("adjusted max duration for new segment", "id", info.id, "elapsed_s", elapsed, "remaining_s", remaining) + } + + if params.MaxSizeInMB != nil && params.OutputDir != nil { + segmentPath := filepath.Join(*params.OutputDir, info.id+".mp4") + if fi, err := os.Stat(segmentPath); err == nil { + consumedMB := int(fi.Size() / (1024 * 1024)) + remaining := *params.MaxSizeInMB - consumedMB + if remaining < 1 { + remaining = 1 + } + params.MaxSizeInMB = &remaining + log.Info("adjusted max size for new segment", "id", info.id, "consumed_mb", consumedMB, "remaining_mb", remaining) + } + } + + return params +} + // startNewRecordingSegments creates and starts a new recording segment for // each previously-stopped recorder. Each new segment gets a unique suffixed // ID so the old (stopped) recorder and its finalized file remain accessible // in the manager. +// +// Duration and size limits are adjusted to account for what the previous +// segment already consumed, so the cumulative totals stay within the +// originally requested bounds. func (s *ApiService) startNewRecordingSegments(ctx context.Context, stopped []stoppedRecordingInfo) { log := logger.FromContext(ctx) for _, info := range stopped { newID := fmt.Sprintf("%s-%d", info.id, time.Now().UnixMilli()) - rec, err := s.factory(newID, info.params) + params := adjustParamsForRemainingBudget(log, info) + + rec, err := s.factory(newID, params) if err != nil { log.Error("failed to create recorder for new segment", "old_id", info.id, "new_id", newID, "error", err) continue diff --git a/server/cmd/api/api/display_test.go b/server/cmd/api/api/display_test.go index 72780ea8..401ef575 100644 --- a/server/cmd/api/api/display_test.go +++ b/server/cmd/api/api/display_test.go @@ -2,6 +2,8 @@ package api import ( "context" + "log/slog" + "os" "path/filepath" "testing" "time" @@ -55,6 +57,8 @@ func TestStopActiveRecordings(t *testing.T) { require.Len(t, stopped, 1) assert.Equal(t, "test-rec", stopped[0].id) assert.NotNil(t, stopped[0].params.FrameRate) + require.NotNil(t, stopped[0].metadata, "metadata should be captured") + assert.False(t, stopped[0].metadata.StartTime.IsZero(), "start time should be set") oldRec, exists := mgr.GetRecorder("test-rec") assert.True(t, exists, "old recorder should remain registered") @@ -245,3 +249,139 @@ func TestStopAndStartNewSegment_RoundTrip(t *testing.T) { _ = newRec.Stop(ctx) } + +func TestAdjustParamsForRemainingBudget(t *testing.T) { + log := slog.Default() + + t.Run("reduces MaxDurationInSeconds by elapsed time", func(t *testing.T) { + maxDur := 60 + fr := 5 + disp := 0 + size := 500 + dir := t.TempDir() + + info := stoppedRecordingInfo{ + id: "dur-test", + params: recorder.FFmpegRecordingParams{ + FrameRate: &fr, + DisplayNum: &disp, + MaxSizeInMB: &size, + MaxDurationInSeconds: &maxDur, + OutputDir: &dir, + }, + metadata: &recorder.RecordingMetadata{ + StartTime: time.Now().Add(-25 * time.Second), + EndTime: time.Now(), + }, + } + + adjusted := adjustParamsForRemainingBudget(log, info) + require.NotNil(t, adjusted.MaxDurationInSeconds) + assert.InDelta(t, 35, *adjusted.MaxDurationInSeconds, 2, "remaining duration should be ~35s") + assert.Equal(t, 60, maxDur, "original param should not be mutated") + }) + + t.Run("clamps remaining duration to 1 when budget exhausted", func(t *testing.T) { + maxDur := 10 + fr := 5 + disp := 0 + size := 500 + dir := t.TempDir() + + info := stoppedRecordingInfo{ + id: "exhausted-dur", + params: recorder.FFmpegRecordingParams{ + FrameRate: &fr, + DisplayNum: &disp, + MaxSizeInMB: &size, + MaxDurationInSeconds: &maxDur, + OutputDir: &dir, + }, + metadata: &recorder.RecordingMetadata{ + StartTime: time.Now().Add(-30 * time.Second), + EndTime: time.Now(), + }, + } + + adjusted := adjustParamsForRemainingBudget(log, info) + require.NotNil(t, adjusted.MaxDurationInSeconds) + assert.Equal(t, 1, *adjusted.MaxDurationInSeconds) + }) + + t.Run("reduces MaxSizeInMB by consumed file size", func(t *testing.T) { + maxSize := 10 + fr := 5 + disp := 0 + dir := t.TempDir() + + segmentFile := filepath.Join(dir, "size-test.mp4") + data := make([]byte, 3*1024*1024) // 3 MB + require.NoError(t, os.WriteFile(segmentFile, data, 0644)) + + info := stoppedRecordingInfo{ + id: "size-test", + params: recorder.FFmpegRecordingParams{ + FrameRate: &fr, + DisplayNum: &disp, + MaxSizeInMB: &maxSize, + OutputDir: &dir, + }, + metadata: &recorder.RecordingMetadata{}, + } + + adjusted := adjustParamsForRemainingBudget(log, info) + require.NotNil(t, adjusted.MaxSizeInMB) + assert.Equal(t, 7, *adjusted.MaxSizeInMB) + assert.Equal(t, 10, maxSize, "original param should not be mutated") + }) + + t.Run("clamps remaining size to 1 when budget exhausted", func(t *testing.T) { + maxSize := 2 + fr := 5 + disp := 0 + dir := t.TempDir() + + segmentFile := filepath.Join(dir, "big-test.mp4") + data := make([]byte, 5*1024*1024) // 5 MB > 2 MB limit + require.NoError(t, os.WriteFile(segmentFile, data, 0644)) + + info := stoppedRecordingInfo{ + id: "big-test", + params: recorder.FFmpegRecordingParams{ + FrameRate: &fr, + DisplayNum: &disp, + MaxSizeInMB: &maxSize, + OutputDir: &dir, + }, + metadata: &recorder.RecordingMetadata{}, + } + + adjusted := adjustParamsForRemainingBudget(log, info) + require.NotNil(t, adjusted.MaxSizeInMB) + assert.Equal(t, 1, *adjusted.MaxSizeInMB) + }) + + t.Run("no adjustment when limits are nil", func(t *testing.T) { + fr := 5 + disp := 0 + size := 500 + dir := t.TempDir() + + info := stoppedRecordingInfo{ + id: "no-limits", + params: recorder.FFmpegRecordingParams{ + FrameRate: &fr, + DisplayNum: &disp, + MaxSizeInMB: &size, + OutputDir: &dir, + }, + metadata: &recorder.RecordingMetadata{ + StartTime: time.Now().Add(-10 * time.Second), + EndTime: time.Now(), + }, + } + + adjusted := adjustParamsForRemainingBudget(log, info) + assert.Nil(t, adjusted.MaxDurationInSeconds, "should remain nil when not set") + }) +} From f4d331321e127292184bf6f8d97b83cd576f5e25 Mon Sep 17 00:00:00 2001 From: Hiro Tamada Date: Wed, 4 Mar 2026 11:18:51 -0500 Subject: [PATCH 6/7] fix: only restart recording segments after successful resize Move segment restart from a defer (which fires on all return paths) to an explicit call after the resize succeeds. This avoids restarting recordings at the old resolution when the stop partially fails or the resize itself errors out. Also round up fractional MB when computing consumed file size budget to prevent cumulative overestimation across multiple resize cycles. Made-with: Cursor --- server/cmd/api/api/display.go | 13 ++++++++----- server/cmd/api/api/display_test.go | 26 ++++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 5 deletions(-) diff --git a/server/cmd/api/api/display.go b/server/cmd/api/api/display.go index 0296adb8..a9200205 100644 --- a/server/cmd/api/api/display.go +++ b/server/cmd/api/api/display.go @@ -84,11 +84,9 @@ func (s *ApiService) PatchDisplay(ctx context.Context, req oapi.PatchDisplayRequ } // Gracefully stop active recordings so the resize can proceed. - // New recording segments (with unique IDs) will be started after the resize. + // New recording segments (with unique IDs) will be started only after a + // successful resize to avoid restarting at the old resolution on error paths. stopped, stopErr := s.stopActiveRecordings(ctx) - if len(stopped) > 0 { - defer s.startNewRecordingSegments(context.WithoutCancel(ctx), stopped) - } if stopErr != nil { log.Error("failed to stop recordings for resize", "error", stopErr) return oapi.PatchDisplay500JSONResponse{ @@ -135,6 +133,11 @@ func (s *ApiService) PatchDisplay(ctx context.Context, req oapi.PatchDisplayRequ }, nil } + // Resize succeeded — restart recording segments at the new resolution. + if len(stopped) > 0 { + go s.startNewRecordingSegments(context.WithoutCancel(ctx), stopped) + } + // Return success with the new dimensions return oapi.PatchDisplay200JSONResponse{ Width: &width, @@ -453,7 +456,7 @@ func adjustParamsForRemainingBudget(log *slog.Logger, info stoppedRecordingInfo) if params.MaxSizeInMB != nil && params.OutputDir != nil { segmentPath := filepath.Join(*params.OutputDir, info.id+".mp4") if fi, err := os.Stat(segmentPath); err == nil { - consumedMB := int(fi.Size() / (1024 * 1024)) + consumedMB := int((fi.Size() + 1024*1024 - 1) / (1024 * 1024)) remaining := *params.MaxSizeInMB - consumedMB if remaining < 1 { remaining = 1 diff --git a/server/cmd/api/api/display_test.go b/server/cmd/api/api/display_test.go index 401ef575..687d65fc 100644 --- a/server/cmd/api/api/display_test.go +++ b/server/cmd/api/api/display_test.go @@ -361,6 +361,32 @@ func TestAdjustParamsForRemainingBudget(t *testing.T) { assert.Equal(t, 1, *adjusted.MaxSizeInMB) }) + t.Run("rounds up fractional MB when computing consumed size", func(t *testing.T) { + maxSize := 10 + fr := 5 + disp := 0 + dir := t.TempDir() + + segmentFile := filepath.Join(dir, "frac-test.mp4") + data := make([]byte, 3*1024*1024+512*1024) // 3.5 MB → rounds up to 4 MB consumed + require.NoError(t, os.WriteFile(segmentFile, data, 0644)) + + info := stoppedRecordingInfo{ + id: "frac-test", + params: recorder.FFmpegRecordingParams{ + FrameRate: &fr, + DisplayNum: &disp, + MaxSizeInMB: &maxSize, + OutputDir: &dir, + }, + metadata: &recorder.RecordingMetadata{}, + } + + adjusted := adjustParamsForRemainingBudget(log, info) + require.NotNil(t, adjusted.MaxSizeInMB) + assert.Equal(t, 6, *adjusted.MaxSizeInMB) // 10 - 4 = 6 + }) + t.Run("no adjustment when limits are nil", func(t *testing.T) { fr := 5 disp := 0 From 3e47691c61d16d3a25764240156bbef335cd48bc Mon Sep 17 00:00:00 2001 From: Hiro Tamada Date: Wed, 4 Mar 2026 12:58:31 -0500 Subject: [PATCH 7/7] fix: always restart stopped recordings to prevent data loss Move segment restart back to a defer so recordings are restarted regardless of whether the resize succeeds. If the resize fails the display is still at the old resolution, so restarting there is correct. Losing recording data is worse than a brief gap. The defer is placed after the stopErr check so it only fires when all stops succeeded cleanly. Made-with: Cursor --- server/cmd/api/api/display.go | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/server/cmd/api/api/display.go b/server/cmd/api/api/display.go index a9200205..20fe337c 100644 --- a/server/cmd/api/api/display.go +++ b/server/cmd/api/api/display.go @@ -84,8 +84,10 @@ func (s *ApiService) PatchDisplay(ctx context.Context, req oapi.PatchDisplayRequ } // Gracefully stop active recordings so the resize can proceed. - // New recording segments (with unique IDs) will be started only after a - // successful resize to avoid restarting at the old resolution on error paths. + // Recordings are always restarted (via defer) regardless of whether the + // resize succeeds — losing recording data is worse than a brief gap. If + // the resize fails the display is still at the old resolution, so + // restarting at the "old" resolution is correct. stopped, stopErr := s.stopActiveRecordings(ctx) if stopErr != nil { log.Error("failed to stop recordings for resize", "error", stopErr) @@ -95,6 +97,11 @@ func (s *ApiService) PatchDisplay(ctx context.Context, req oapi.PatchDisplayRequ }, }, nil } + if len(stopped) > 0 { + defer func() { + go s.startNewRecordingSegments(context.WithoutCancel(ctx), stopped) + }() + } // Detect display mode (xorg or xvfb) displayMode := s.detectDisplayMode(ctx) @@ -133,11 +140,6 @@ func (s *ApiService) PatchDisplay(ctx context.Context, req oapi.PatchDisplayRequ }, nil } - // Resize succeeded — restart recording segments at the new resolution. - if len(stopped) > 0 { - go s.startNewRecordingSegments(context.WithoutCancel(ctx), stopped) - } - // Return success with the new dimensions return oapi.PatchDisplay200JSONResponse{ Width: &width,