Add named markers to replay recordings as MP4 chapters#289
Open
rgarcia wants to merge 6 commits into
Open
Conversation
Contributor
Author
Independent review — replay markers as MP4 chaptersReviewed the full diff, read all changed files, rebuilt, ran unit + integration tests, and verified a few correctness concerns with real ffmpeg/ffprobe. Not blocking — the core mechanism is sound and well-tested. A few minor items below. Verified correct
Minor / nits
Build green, |
rgarcia
marked this pull request as ready for review
June 16, 2026 16:35
|
Firetiger deploy monitoring skipped This PR didn't match the auto-monitor filter configured on your GitHub connection:
Reason: PR is in the To monitor this PR anyway, reply with |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Marker limit counts bytes
- Marker validation now enforces the 200-character limit using Unicode rune count instead of UTF-8 byte length, with regression tests for multibyte names.
- ✅ Fixed: OpenAPI omits name length
- The MarkRecordingRequest schema now declares name minLength 1 and maxLength 200 to match server-side validation.
Or push these changes by commenting:
@cursor push 804fa5149b
Preview (804fa5149b)
diff --git a/server/lib/recorder/ffmpeg.go b/server/lib/recorder/ffmpeg.go
--- a/server/lib/recorder/ffmpeg.go
+++ b/server/lib/recorder/ffmpeg.go
@@ -15,6 +15,7 @@
"sync"
"syscall"
"time"
+ "unicode/utf8"
"github.com/kernel/kernel-images/server/lib/logger"
"github.com/kernel/kernel-images/server/lib/scaletozero"
@@ -497,7 +498,7 @@
}
name = strings.TrimSpace(name)
- if name == "" || len(name) > maxMarkerNameLen {
+ if name == "" || utf8.RuneCountInString(name) > maxMarkerNameLen {
return "", 0, ErrInvalidMarkerName
}
diff --git a/server/lib/recorder/markers_test.go b/server/lib/recorder/markers_test.go
--- a/server/lib/recorder/markers_test.go
+++ b/server/lib/recorder/markers_test.go
@@ -156,6 +156,12 @@
_, _, err = rec.Mark(strings.Repeat("x", maxMarkerNameLen+1))
assert.ErrorIs(t, err, ErrInvalidMarkerName)
+
+ _, _, err = rec.Mark(strings.Repeat("界", maxMarkerNameLen))
+ assert.NoError(t, err)
+
+ _, _, err = rec.Mark(strings.Repeat("界", maxMarkerNameLen+1))
+ assert.ErrorIs(t, err, ErrInvalidMarkerName)
}
func TestMark_ConcurrentCallsAreSafe(t *testing.T) {
diff --git a/server/openapi.yaml b/server/openapi.yaml
--- a/server/openapi.yaml
+++ b/server/openapi.yaml
@@ -3484,6 +3484,8 @@
name:
type: string
description: Name of the marker, used as the MP4 chapter title.
+ minLength: 1
+ maxLength: 200
id:
type: string
description: Identifier of the recorder to mark. Alphanumeric or hyphen.You can send follow-ups to the cloud agent here.
Add a Mark(name) API to the recorder and a POST /recording/mark endpoint that records named, timestamped markers during an active recording. At finalize the markers are written into the output MP4 as chapter markers via an ffmetadata input, so they can be scrubbed to in any tool that reads MP4 chapters (ffprobe, QuickTime, VLC, NLEs). The feature is additive and backwards-compatible: with no markers the remux command is byte-identical to before, and any marker/metadata failure falls back to the plain remux so a recording is never corrupted. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
buildChapterMetadata always received 0 for the origin shift, so remove the parameter, its drop-when-shifted branch, and the corresponding test. Chapter starts are now computed directly from each marker's offset. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Drop markers at/past the recording duration and clamp chapter ENDs so the ffmetadata file can never contain an END-before-START chapter, which ffmpeg rejects outright. If the chaptered remux still fails, retry once without the chapter input before caching the finalize error, so bad chapter data can never permanently poison finalize and cost the recording its faststart/duration remux.
rgarcia
force-pushed
the
hypeship/replay-markers
branch
from
July 24, 2026 15:11
81d787d to
85afd80
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Chapters misaligned with media timeline
- Recorder start time is now anchored immediately after ffmpeg successfully starts, so marker/chapter offsets and finalized duration are calculated from the media timeline instead of pre-start wall clock time.
Or push these changes by commenting:
@cursor push 37ce629079
Preview (37ce629079)
diff --git a/server/lib/recorder/ffmpeg.go b/server/lib/recorder/ffmpeg.go
--- a/server/lib/recorder/ffmpeg.go
+++ b/server/lib/recorder/ffmpeg.go
@@ -268,7 +268,6 @@
// ensure internal state
fr.ffmpegErr = nil
fr.exitCode = exitCodeInitValue
- fr.startTime = time.Now()
fr.exited = make(chan struct{})
args, err := ffmpegArgs(fr.params, fr.outputPath)
@@ -287,7 +286,6 @@
cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}
cmd.Stderr = os.Stderr
cmd.Stdout = os.Stdout
- fr.cmd = cmd
fr.mu.Unlock()
if err := cmd.Start(); err != nil {
@@ -299,6 +297,12 @@
fr.mu.Unlock()
return fmt.Errorf("failed to start ffmpeg process: %w", err)
}
+ fr.mu.Lock()
+ fr.cmd = cmd
+ // Anchor recording time to ffmpeg startup so marker/chapter offsets align
+ // with the media timeline, especially when audio inputs use input timestamps.
+ fr.startTime = time.Now()
+ fr.mu.Unlock()
// Launch background waiter to capture process completion.
go fr.waitForCommand(ctx)You can send follow-ups to the cloud agent here.
…dation Chapter offsets were measured from a startTime stamped before the ffmpeg process even spawned, so every chapter landed late by the spawn + input open + encoder init latency. Watch the output file for ffmpeg's first written bytes (the muxer only writes once the capture pipeline is producing media) and anchor marker offsets there, falling back to startTime if no bytes ever appear. Also count the 200-char marker name limit in characters (runes) to match the API error text instead of UTF-8 bytes, and declare minLength/maxLength on MarkRecordingRequest.name in the OpenAPI spec (oapi.go regenerated).
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Stale output anchors chapters
- watchCaptureStart now ignores pre-existing non-empty output files unless their mtime is at or after the current session start, preventing stale files from anchoring new recordings.
- ✅ Fixed: Early marks vanish from chapters
- buildChapterMetadata now clamps negative marker offsets to 0 instead of dropping them so marks accepted before captureAnchor is detected still become MP4 chapters.
Or push these changes by commenting:
@cursor push 89b9a10ed2
Preview (89b9a10ed2)
diff --git a/server/lib/recorder/ffmpeg.go b/server/lib/recorder/ffmpeg.go
--- a/server/lib/recorder/ffmpeg.go
+++ b/server/lib/recorder/ffmpeg.go
@@ -71,12 +71,12 @@ type FFmpegRecorder struct {
// chapter offsets to the media timeline; zero means never detected and
// callers fall back to startTime (stamped before the process spawned).
captureAnchor time.Time
- ffmpegErr error
- exitCode int
- exited chan struct{}
- deleted bool
- markers []Marker
- stz *scaletozero.Oncer
+ ffmpegErr error
+ exitCode int
+ exited chan struct{}
+ deleted bool
+ markers []Marker
+ stz *scaletozero.Oncer
// flight coordinates concurrent operations using different keys:
// - "stop": prevents multiple SIGINTs from being sent to ffmpeg
@@ -71,12 +71,12 @@ type FFmpegRecorder struct {
// chapter offsets to the media timeline; zero means never detected and
// callers fall back to startTime (stamped before the process spawned).
captureAnchor time.Time
- ffmpegErr error
- exitCode int
- exited chan struct{}
- deleted bool
- markers []Marker
- stz *scaletozero.Oncer
+ ffmpegErr error
+ exitCode int
+ exited chan struct{}
+ deleted bool
+ markers []Marker
+ stz *scaletozero.Oncer
// flight coordinates concurrent operations using different keys:
// - "stop": prevents multiple SIGINTs from being sent to ffmpeg
@@ -520,12 +520,16 @@ func (fr *FFmpegRecorder) Mark(name string) (string, int64, error) {
}
// watchCaptureStart stamps captureAnchor with the moment ffmpeg writes its
-// first output bytes. The muxer only writes them once the capture pipeline is
-// fully initialized (input opened, encoder ready, first frame in flight), so
-// this tracks media t=0 far more closely than startTime, which is stamped
-// before the process is even spawned. If no bytes ever appear the anchor
-// stays zero and callers fall back to startTime.
+// first output bytes for the current Start() session. The muxer only writes
+// them once the capture pipeline is fully initialized (input opened, encoder
+// ready, first frame in flight), so this tracks media t=0 far more closely
+// than startTime, which is stamped before the process is even spawned. If no
+// bytes ever appear the anchor stays zero and callers fall back to startTime.
func (fr *FFmpegRecorder) watchCaptureStart() {
+ fr.mu.Lock()
+ sessionStart := fr.startTime
+ fr.mu.Unlock()
+
ticker := time.NewTicker(10 * time.Millisecond)
defer ticker.Stop()
for {
@@ -520,12 +520,16 @@ func (fr *FFmpegRecorder) Mark(name string) (string, int64, error) {
}
// watchCaptureStart stamps captureAnchor with the moment ffmpeg writes its
-// first output bytes. The muxer only writes them once the capture pipeline is
-// fully initialized (input opened, encoder ready, first frame in flight), so
-// this tracks media t=0 far more closely than startTime, which is stamped
-// before the process is even spawned. If no bytes ever appear the anchor
-// stays zero and callers fall back to startTime.
+// first output bytes for the current Start() session. The muxer only writes
+// them once the capture pipeline is fully initialized (input opened, encoder
+// ready, first frame in flight), so this tracks media t=0 far more closely
+// than startTime, which is stamped before the process is even spawned. If no
+// bytes ever appear the anchor stays zero and callers fall back to startTime.
func (fr *FFmpegRecorder) watchCaptureStart() {
+ fr.mu.Lock()
+ sessionStart := fr.startTime
+ fr.mu.Unlock()
+
ticker := time.NewTicker(10 * time.Millisecond)
defer ticker.Stop()
for {
@@ -534,6 +538,12 @@ func (fr *FFmpegRecorder) watchCaptureStart() {
return
case <-ticker.C:
if info, err := os.Stat(fr.outputPath); err == nil && info.Size() > 0 {
+ // Ignore stale bytes from a previous recording session that used the
+ // same output path. We only anchor once this file has been updated
+ // after the current Start() timestamp.
+ if !sessionStart.IsZero() && info.ModTime().Before(sessionStart) {
+ continue
+ }
now := time.Now()
fr.mu.Lock()
fr.captureAnchor = now
@@ -534,6 +538,12 @@ func (fr *FFmpegRecorder) watchCaptureStart() {
return
case <-ticker.C:
if info, err := os.Stat(fr.outputPath); err == nil && info.Size() > 0 {
+ // Ignore stale bytes from a previous recording session that used the
+ // same output path. We only anchor once this file has been updated
+ // after the current Start() timestamp.
+ if !sessionStart.IsZero() && info.ModTime().Before(sessionStart) {
+ continue
+ }
now := time.Now()
fr.mu.Lock()
fr.captureAnchor = now
diff --git a/server/lib/recorder/markers.go b/server/lib/recorder/markers.go
--- a/server/lib/recorder/markers.go
+++ b/server/lib/recorder/markers.go
@@ -23,8 +23,9 @@ const sentinelChapterName = "_recording_start"
// buildChapterMetadata writes an ffmetadata file describing one MP4 chapter per
// marker and returns its path. Each marker's chapter starts at its offset from
-// startTime; markers before startTime or at/after durationMs are dropped.
-// durationMs is the recording length and becomes the END of the final chapter.
+// startTime; markers before startTime are clamped to 0 and markers at/after
+// durationMs are dropped. durationMs is the recording length and becomes the
+// END of the final chapter.
// Chapter ENDs are clamped to never precede their START, since ffmpeg rejects
// an END-before-START chapter outright and would fail the whole remux.
//
@@ -23,8 +23,9 @@ const sentinelChapterName = "_recording_start"
// buildChapterMetadata writes an ffmetadata file describing one MP4 chapter per
// marker and returns its path. Each marker's chapter starts at its offset from
-// startTime; markers before startTime or at/after durationMs are dropped.
-// durationMs is the recording length and becomes the END of the final chapter.
+// startTime; markers before startTime are clamped to 0 and markers at/after
+// durationMs are dropped. durationMs is the recording length and becomes the
+// END of the final chapter.
// Chapter ENDs are clamped to never precede their START, since ffmpeg rejects
// an END-before-START chapter outright and would fail the whole remux.
//
@@ -40,9 +41,12 @@ func buildChapterMetadata(outputPath string, markers []Marker, startTime time.Ti
chapters := make([]chapter, 0, len(markers))
for _, m := range markers {
startMs := m.At.Sub(startTime).Milliseconds()
- if startMs < 0 || startMs >= durationMs {
+ if startMs >= durationMs {
continue
}
+ if startMs < 0 {
+ startMs = 0
+ }
chapters = append(chapters, chapter{startMs: startMs, title: m.Name})
}
if len(chapters) == 0 {
@@ -40,9 +41,12 @@ func buildChapterMetadata(outputPath string, markers []Marker, startTime time.Ti
chapters := make([]chapter, 0, len(markers))
for _, m := range markers {
startMs := m.At.Sub(startTime).Milliseconds()
- if startMs < 0 || startMs >= durationMs {
+ if startMs >= durationMs {
continue
}
+ if startMs < 0 {
+ startMs = 0
+ }
chapters = append(chapters, chapter{startMs: startMs, title: m.Name})
}
if len(chapters) == 0 {
diff --git a/server/lib/recorder/markers_test.go b/server/lib/recorder/markers_test.go
--- a/server/lib/recorder/markers_test.go
+++ b/server/lib/recorder/markers_test.go
@@ -47,7 +47,7 @@ func TestBuildChapterMetadata_SentinelAndOrdering(t *testing.T) {
assert.Equal(t, expected, meta)
}
-func TestBuildChapterMetadata_DropsNegativeOffsets(t *testing.T) {
+func TestBuildChapterMetadata_ClampsNegativeOffsetsToZero(t *testing.T) {
start := time.Unix(1000, 0)
out := filepath.Join(t.TempDir(), "rec.mp4")
markers := []Marker{
@@ -47,7 +47,7 @@ func TestBuildChapterMetadata_SentinelAndOrdering(t *testing.T) {
assert.Equal(t, expected, meta)
}
-func TestBuildChapterMetadata_DropsNegativeOffsets(t *testing.T) {
+func TestBuildChapterMetadata_ClampsNegativeOffsetsToZero(t *testing.T) {
start := time.Unix(1000, 0)
out := filepath.Join(t.TempDir(), "rec.mp4")
markers := []Marker{
@@ -60,9 +60,9 @@ func TestBuildChapterMetadata_DropsNegativeOffsets(t *testing.T) {
require.True(t, ok)
meta := readMeta(t, path)
- assert.NotContains(t, meta, "before-start")
+ assert.Contains(t, meta, "START=0\nEND=4000\ntitle=before-start")
assert.Contains(t, meta, "title=kept")
- // Sentinel + one kept marker = two chapters.
+ // Two usable chapters, no sentinel needed because the first starts at 0.
assert.Equal(t, 2, strings.Count(meta, "[CHAPTER]"))
}
@@ -60,9 +60,9 @@ func TestBuildChapterMetadata_DropsNegativeOffsets(t *testing.T) {
require.True(t, ok)
meta := readMeta(t, path)
- assert.NotContains(t, meta, "before-start")
+ assert.Contains(t, meta, "START=0\nEND=4000\ntitle=before-start")
assert.Contains(t, meta, "title=kept")
- // Sentinel + one kept marker = two chapters.
+ // Two usable chapters, no sentinel needed because the first starts at 0.
assert.Equal(t, 2, strings.Count(meta, "[CHAPTER]"))
}
@@ -329,6 +329,38 @@ func TestWatchCaptureStart_AnchorsToFirstOutputBytes(t *testing.T) {
rec.mu.Unlock()
}
+func TestWatchCaptureStart_IgnoresPreexistingOutputUntilCurrentSessionWrite(t *testing.T) {
+ outputPath := filepath.Join(t.TempDir(), "anchor-stale.mp4")
+ require.NoError(t, os.WriteFile(outputPath, []byte("stale"), 0o644))
+ time.Sleep(20 * time.Millisecond) // ensure stale file mtime predates startTime
+
+ rec := &FFmpegRecorder{
+ id: "anchor-stale",
+ outputPath: outputPath,
+ startTime: time.Now(),
+ exited: make(chan struct{}),
+ }
+ defer close(rec.exited)
+ go rec.watchCaptureStart()
+
+ time.Sleep(50 * time.Millisecond)
+ rec.mu.Lock()
+ assert.True(t, rec.captureAnchor.IsZero(), "stale output must not anchor this session")
+ rec.mu.Unlock()
+
+ beforeRewrite := time.Now()
+ require.NoError(t, os.WriteFile(outputPath, []byte("fresh"), 0o644))
+ require.Eventually(t, func() bool {
+ rec.mu.Lock()
+ defer rec.mu.Unlock()
+ return !rec.captureAnchor.IsZero()
+ }, 2*time.Second, 5*time.Millisecond)
+
+ rec.mu.Lock()
+ assert.False(t, rec.captureAnchor.Before(beforeRewrite), "anchor stamped at/after fresh session write")
+ rec.mu.Unlock()
+}
+
func TestWatchCaptureStart_StopsOnExitWithoutOutput(t *testing.T) {
rec := &FFmpegRecorder{
id: "anchor-exit",
@@ -329,6 +329,38 @@ func TestWatchCaptureStart_AnchorsToFirstOutputBytes(t *testing.T) {
rec.mu.Unlock()
}
+func TestWatchCaptureStart_IgnoresPreexistingOutputUntilCurrentSessionWrite(t *testing.T) {
+ outputPath := filepath.Join(t.TempDir(), "anchor-stale.mp4")
+ require.NoError(t, os.WriteFile(outputPath, []byte("stale"), 0o644))
+ time.Sleep(20 * time.Millisecond) // ensure stale file mtime predates startTime
+
+ rec := &FFmpegRecorder{
+ id: "anchor-stale",
+ outputPath: outputPath,
+ startTime: time.Now(),
+ exited: make(chan struct{}),
+ }
+ defer close(rec.exited)
+ go rec.watchCaptureStart()
+
+ time.Sleep(50 * time.Millisecond)
+ rec.mu.Lock()
+ assert.True(t, rec.captureAnchor.IsZero(), "stale output must not anchor this session")
+ rec.mu.Unlock()
+
+ beforeRewrite := time.Now()
+ require.NoError(t, os.WriteFile(outputPath, []byte("fresh"), 0o644))
+ require.Eventually(t, func() bool {
+ rec.mu.Lock()
+ defer rec.mu.Unlock()
+ return !rec.captureAnchor.IsZero()
+ }, 2*time.Second, 5*time.Millisecond)
+
+ rec.mu.Lock()
+ assert.False(t, rec.captureAnchor.Before(beforeRewrite), "anchor stamped at/after fresh session write")
+ rec.mu.Unlock()
+}
+
func TestWatchCaptureStart_StopsOnExitWithoutOutput(t *testing.T) {
rec := &FFmpegRecorder{
id: "anchor-exit",You can send follow-ups to the cloud agent here.
Remove any leftover output file before spawning ffmpeg so watchCaptureStart can't stamp the anchor from a previous session's bytes, and clamp markers placed before the capture anchor to media t=0 instead of silently dropping them from the chapters (the API had already returned 201 for them).
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Late anchor breaks chapter duration
- watchCaptureStart now refuses to stamp captureAnchor after process exit and finalize defensively falls back to startTime when anchor is not before endTime so chapter duration remains valid.
- ✅ Fixed: Mark succeeds after capture stops
- Mark now requires the ffmpeg process to be truly running (including non-zombie on Linux) instead of relying only on the delayed exitCode sentinel, so post-exit marks are rejected.
Or push these changes by commenting:
@cursor push c97e219e94
Preview (c97e219e94)
diff --git a/server/lib/recorder/ffmpeg.go b/server/lib/recorder/ffmpeg.go
--- a/server/lib/recorder/ffmpeg.go
+++ b/server/lib/recorder/ffmpeg.go
@@ -71,12 +71,12 @@
// chapter offsets to the media timeline; zero means never detected and
// callers fall back to startTime (stamped before the process spawned).
captureAnchor time.Time
- ffmpegErr error
- exitCode int
- exited chan struct{}
- deleted bool
- markers []Marker
- stz *scaletozero.Oncer
+ ffmpegErr error
+ exitCode int
+ exited chan struct{}
+ deleted bool
+ markers []Marker
+ stz *scaletozero.Oncer
// flight coordinates concurrent operations using different keys:
// - "stop": prevents multiple SIGINTs from being sent to ffmpeg
@@ -426,6 +426,9 @@
binaryPath := fr.binaryPath
markers := fr.markers
anchor := fr.anchorLocked()
+ if !fr.endTime.IsZero() && !anchor.Before(fr.endTime) {
+ anchor = fr.startTime
+ }
durationMs := fr.endTime.Sub(anchor).Milliseconds()
fr.mu.Unlock()
@@ -511,7 +514,7 @@
fr.mu.Lock()
defer fr.mu.Unlock()
- if fr.cmd == nil || fr.exitCode >= exitCodeProcessDoneMinValue {
+ if !fr.canMarkLocked() {
return "", 0, ErrNotRecording
}
@@ -525,6 +528,45 @@
return name, now.Sub(fr.anchorLocked()).Milliseconds(), nil
}
+// canMarkLocked reports whether ffmpeg is still actively recording. It is
+// stricter than the exitCode sentinel check alone because the process can exit
+// before the wait goroutine updates exitCode/endTime.
+func (fr *FFmpegRecorder) canMarkLocked() bool {
+ if fr.cmd == nil || fr.cmd.Process == nil || fr.exitCode >= exitCodeProcessDoneMinValue {
+ return false
+ }
+ if fr.cmd.ProcessState != nil && fr.cmd.ProcessState.Exited() {
+ return false
+ }
+ return processRunning(fr.cmd.Process.Pid)
+}
+
+// processRunning reports whether pid is still alive. On Linux it treats zombie
+// tasks as not running so callers can reject work after capture has ended.
+func processRunning(pid int) bool {
+ if pid <= 0 {
+ return false
+ }
+ if runtime.GOOS == "linux" {
+ statPath := fmt.Sprintf("/proc/%d/stat", pid)
+ if b, err := os.ReadFile(statPath); err == nil {
+ stat := string(b)
+ if end := strings.LastIndexByte(stat, ')'); end != -1 && end+2 < len(stat) {
+ state := stat[end+2]
+ if state == 'Z' || state == 'X' || state == 'x' {
+ return false
+ }
+ return true
+ }
+ } else if os.IsNotExist(err) {
+ return false
+ }
+ }
+ // Fallback for non-Linux or /proc parsing failures.
+ err := syscall.Kill(pid, 0)
+ return err == nil || !errors.Is(err, syscall.ESRCH)
+}
+
// watchCaptureStart stamps captureAnchor with the moment ffmpeg writes its
// first output bytes. The muxer only writes them once the capture pipeline is
// fully initialized (input opened, encoder ready, first frame in flight), so
@@ -542,7 +584,15 @@
if info, err := os.Stat(fr.outputPath); err == nil && info.Size() > 0 {
now := time.Now()
fr.mu.Lock()
- fr.captureAnchor = now
+ // If ffmpeg already exited, avoid stamping a late anchor after
+ // endTime; finalize then falls back to startTime.
+ if fr.cmd != nil && fr.exitCode >= exitCodeProcessDoneMinValue {
+ fr.mu.Unlock()
+ return
+ }
+ if fr.captureAnchor.IsZero() {
+ fr.captureAnchor = now
+ }
fr.mu.Unlock()
return
}
diff --git a/server/lib/recorder/markers_test.go b/server/lib/recorder/markers_test.go
--- a/server/lib/recorder/markers_test.go
+++ b/server/lib/recorder/markers_test.go
@@ -5,6 +5,7 @@
"os"
"os/exec"
"path/filepath"
+ "runtime"
"strings"
"sync"
"testing"
@@ -171,6 +172,27 @@
assert.ErrorIs(t, err, ErrNotRecording)
}
+func TestMark_RejectsWhenProcessExitedBeforeWait(t *testing.T) {
+ if runtime.GOOS != "linux" {
+ t.Skip("requires Linux /proc state inspection")
+ }
+
+ cmd := exec.Command("sh", "-c", "exit 0")
+ require.NoError(t, cmd.Start())
+ t.Cleanup(func() { _ = cmd.Wait() })
+ require.Eventually(t, func() bool {
+ return !processRunning(cmd.Process.Pid)
+ }, 2*time.Second, 10*time.Millisecond, "process should have exited before mark attempt")
+
+ rec := &FFmpegRecorder{
+ id: "mark-exited",
+ cmd: cmd,
+ exitCode: exitCodeInitValue,
+ }
+ _, _, err := rec.Mark("late")
+ assert.ErrorIs(t, err, ErrNotRecording)
+}
+
func TestMark_AppendsAndReturnsOffset(t *testing.T) {
tempDir := t.TempDir()
rec := &FFmpegRecorder{
@@ -306,6 +328,38 @@
assert.True(t, os.IsNotExist(statErr), "metadata temp file should be removed")
}
+func TestFinalize_UsesStartAnchorWhenCaptureAnchorIsAfterEnd(t *testing.T) {
+ tempDir := t.TempDir()
+ outputPath := filepath.Join(tempDir, "rec.mp4")
+ require.NoError(t, os.WriteFile(outputPath, []byte("recording"), 0o644))
+
+ argsLog := filepath.Join(tempDir, "args.log")
+ t.Setenv("FAKE_FFMPEG_ARGS_LOG", argsLog)
+ script := "#!/usr/bin/env bash\n" +
+ "printf '%s\\n' \"$@\" > \"$FAKE_FFMPEG_ARGS_LOG\"\n" +
+ "printf remuxed > \"${@: -1}\"\n"
+ bin := filepath.Join(tempDir, "fake_ffmpeg.sh")
+ require.NoError(t, os.WriteFile(bin, []byte(script), 0o755))
+
+ start := time.Now()
+ rec := &FFmpegRecorder{
+ id: "anchor-after-end",
+ binaryPath: bin,
+ outputPath: outputPath,
+ startTime: start,
+ endTime: start.Add(4 * time.Second),
+ captureAnchor: start.Add(5 * time.Second), // late/stale anchor should be ignored at finalize.
+ exitCode: 0,
+ markers: []Marker{{Name: "m", At: start.Add(1 * time.Second)}},
+ stz: scaletozero.NewOncer(scaletozero.NewNoopController()),
+ }
+ require.NoError(t, rec.finalizeRecording(context.Background()))
+
+ args, err := os.ReadFile(argsLog)
+ require.NoError(t, err)
+ assert.Contains(t, string(args), "-map_chapters", "chapter metadata should still be injected")
+}
+
func TestMark_NameLimitCountsCharactersNotBytes(t *testing.T) {
tempDir := t.TempDir()
rec := &FFmpegRecorder{
@@ -373,7 +427,7 @@
start := time.Now()
rec := &FFmpegRecorder{
id: "anchor-offset",
- cmd: &exec.Cmd{},
+ cmd: &exec.Cmd{Process: &os.Process{Pid: os.Getpid()}},
exitCode: exitCodeInitValue,
startTime: start.Add(-10 * time.Second),
captureAnchor: start.Add(-1 * time.Second),You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit e77fa11. Configure here.
Guard the capture-anchor stamp on the process not having been reaped (exitCode and endTime are set together under fr.mu), so a late tick can never stamp an anchor past endTime and zero out the chapter duration. At finalize, clamp marker offsets into [0, durationMs] instead of dropping late ones: a marker accepted with 201 during the stop drain or the exit window now becomes a zero-length chapter at the end of the timeline rather than silently vanishing. Degenerate non-positive durations fall back to the plain no-chapter remux.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds named, timestamped markers to screen recordings. A marker records a point in time during an active recording; at finalize the markers are injected into the output MP4 as chapter markers, so users can scrub/crop to them in post (ffprobe
-show_chapters, QuickTime, VLC, and most NLEs all read MP4 chapters).The feature is purely additive and backwards-compatible.
Why
Recordings are currently opaque blobs — there's no way to flag "the interesting thing happened here" while a session runs. MP4 chapters are the standard, tool-agnostic way to annotate timeline positions, so a marker dropped during recording survives into the downloaded file with no extra sidecar.
API
New endpoint
POST /recording/mark:{ "name": string (required), "id"?: string }201{ "name": string, "offsetMs": integer }on success409when no recording is in progress400for an empty/oversized name or missing bodyoffsetMsis documented as provisional (measured against the recording start time at mark time); the authoritative offset is the chapter start computed at finalize.How
Mark(name)on the recorder appends a marker under the existing mutex and is safe under concurrent calls.TIMEBASE=1/1000chapters, special characters escaped) and passed as a second input with-map 0 -map_chapters 1. A sentinel chapter is prepended atSTART=0because ffmpeg forces the first chapter to start at 0 — without it the first real marker's timestamp would be clamped.lib/oapi/oapi.gowas regenerated viamake oapi-generate(openapi-down-convert + oapi-codegen), not hand-edited.Tests
START=0, integerSTART/ENDtiling, negative-offset markers dropped, ascending ordering, special-character escaping, and a nonzero origin-shift parameter.Mark(): 409 path when not recording, trim/validation, and concurrency (run under-race).-map_chaptersis injected only when markers are present.testsrc, no display needed) that records a synthetic clip, marks at known offsets, finalizes, and asserts viaffprobe -show_chaptersthat chapters land within ~1.5 frame intervals.go vet ./...,go build ./..., and the full non-e2e suite (go test -race) pass locally.Note
Medium Risk
Touches finalize/remux on the recording path; safeguards (fallback remux, stale file removal) limit corruption risk, but chapter injection is new behavior on stop/download.
Overview
Adds named markers during an active screen recording so they appear as MP4 chapters in the finalized file (scrub-friendly in players and NLEs).
API: New
POST /recording/markwith optional recorderidand requiredname; returns201with provisionaloffsetMs, or400/409for bad input or no active recording. OpenAPI and generatedoapiclient/server stubs are updated.Recorder:
Markon theRecorderinterface appends timestamped markers under the recorder mutex. Finalize builds an ffmetadata sidecar and remuxes with-map_chapterswhen markers exist; failures fall back to the previous plain remux (and retry without chapters if chaptered remux fails). With no markers, remux args stay byte-identical to before. A capture anchor (first ffmpeg output bytes) improves chapter timing vs pre-spawnstartTime; stale output is removed onStartso anchors are not wrong.Tests: API handler cases, chapter metadata builder, concurrency, backwards-compat remux args, fake-ffmpeg retry, and ffprobe integration when ffmpeg is on PATH.
Reviewed by Cursor Bugbot for commit 7643964. Bugbot is set up for automated code reviews on this repo. Configure here.