docs(media): specify Abort and Close race and prefer an observed encoder exit

commit: a6bc749edfbc8afbc66f0aafba237c7139a726b5
author: brice · 22d ago · 2026-09-30 08:34 UTC
committer: brice · 22d ago · 2026-09-30 14:48 UTC
parent: be238fd
signature: GPG VerifiedCD88F9E564E0102646FB6077A1A07C7C7C3E349D · arch-gpg Verified for the committer email

2 files · +66 -17

M internal/adapters/ffmpeg/ffmpeg.go +41 -17


@@ -373,15 +373,23 @@ func writeFull(dst io.Writer, p []byte) error {
373	373		// Close signals EOF, waits for the encoder to finalize the file (bounded, then
374	374		// kills it) and reports the outcome. The video is kept when the encoder exits
375	375		// cleanly after at least one frame, including after a cancelled context; it is
376		-	// removed otherwise. Repeated calls return the first result.
	376	+	// removed otherwise. Close and Abort are terminal and the first one wins:
	377	+	// later Close calls return the first Close result, and Close after an Abort
	378	+	// returns nil because nothing was kept. Close is only concurrent with Abort
	379	+	// (besides Write) as described on Abort.
377	380		func (w *Writer) Close() error { return w.terminate(false) }
378	381
379		-	// Abort discards the recording: it interrupts a pending Write, kills the
380		-	// encoder and removes the output file, even if frames were already written and
381		-	// the encoder was healthy. A Close that is still draining is escalated to a
382		-	// kill. Use Close to keep a video. It returns nil unless the encoder could not
383		-	// be reaped or the file could not be removed, is safe concurrently with Write
384		-	// and is idempotent.
	382	+	// Abort discards the recording. When it is the first terminal operation it
	383	+	// interrupts a pending Write, kills the encoder and removes the output file,
	384	+	// even if frames were already written and the encoder was healthy; use Close to
	385	+	// keep a video. It returns nil unless the encoder could not be reaped or the
	386	+	// file could not be removed. It is safe concurrently with Write and idempotent.
	387	+	//
	388	+	// If Abort arrives while a Close is still draining, it escalates that Close to
	389	+	// a kill and the output is removed, unless the encoder had already exited: an
	390	+	// exit observed first lets Close finish and keep the video. Close's result is
	391	+	// authoritative in that race, and the Abort call itself then returns nil. Abort
	392	+	// after Close has finished is a no-op.
385	393		func (w *Writer) Abort() error { return w.terminate(true) }
386	394
387	395		func (w *Writer) terminate(abort bool) error {

@@ -431,16 +439,7 @@ func (w *Writer) shutdown(abort bool) error {
431	439		if abort {
432	440		return w.discard()
433	441		}
434		-	timer := time.NewTimer(w.closeTimeout)
435		-	defer timer.Stop()
436		-	var err error
437		-	select {
438		-	case <-w.waitDone:
439		-	case <-timer.C:
440		-	err = fmt.Errorf("ffmpeg: encoder did not finish within %s; killed", w.closeTimeout)
441		-	case <-w.abortCh:
442		-	err = errors.New("ffmpeg: shutdown aborted; encoder killed")
443		-	}
	442	+	err := w.awaitExit()
444	443		if err != nil {
445	444		_ = w.cmd.Process.Kill() // ErrProcessDone is harmless
446	445		kt := time.NewTimer(killTimeout)

@@ -475,6 +474,31 @@ func (w *Writer) shutdown(abort bool) error {
475	474		return err
476	475		}
477	476
	477	+	// awaitExit waits for the encoder to exit after EOF. It returns nil if the
	478	+	// process has exited, else the reason the wait was cut short (timeout or
	479	+	// Abort). An exit that is already visible wins over a timeout or Abort that
	480	+	// became ready at the same time; the remaining race is inherent and documented
	481	+	// on Abort.
	482	+	func (w *Writer) awaitExit() error {
	483	+	timer := time.NewTimer(w.closeTimeout)
	484	+	defer timer.Stop()
	485	+	var err error
	486	+	select {
	487	+	case <-w.waitDone:
	488	+	return nil
	489	+	case <-timer.C:
	490	+	err = fmt.Errorf("ffmpeg: encoder did not finish within %s; killed", w.closeTimeout)
	491	+	case <-w.abortCh:
	492	+	err = errors.New("ffmpeg: shutdown aborted; encoder killed")
	493	+	}
	494	+	select {
	495	+	case <-w.waitDone:
	496	+	return nil
	497	+	default:
	498	+	return err
	499	+	}
	500	+	}
	501	+
478	502		// discard kills the encoder, waits for the reaper and removes the output.
479	503		func (w *Writer) discard() error {
480	504		_ = w.cmd.Process.Kill() // ErrProcessDone is harmless

 // Close signals EOF, waits for the encoder to finalize the file (bounded, then
 // kills it) and reports the outcome. The video is kept when the encoder exits
 // cleanly after at least one frame, including after a cancelled context; it is
-// removed otherwise. Repeated calls return the first result.
+// removed otherwise. Close and Abort are terminal and the first one wins:
+// later Close calls return the first Close result, and Close after an Abort
+// returns nil because nothing was kept. Close is only concurrent with Abort
+// (besides Write) as described on Abort.
 func (w *Writer) Close() error { return w.terminate(false) }
-// Abort discards the recording: it interrupts a pending Write, kills the
+// Abort discards the recording. When it is the first terminal operation it
-// encoder and removes the output file, even if frames were already written and
+// interrupts a pending Write, kills the encoder and removes the output file,
-// the encoder was healthy. A Close that is still draining is escalated to a
+// even if frames were already written and the encoder was healthy; use Close to
-// kill. Use Close to keep a video. It returns nil unless the encoder could not
+// keep a video. It returns nil unless the encoder could not be reaped or the
-// be reaped or the file could not be removed, is safe concurrently with Write
+// file could not be removed. It is safe concurrently with Write and idempotent.
-// and is idempotent.
+//
+// If Abort arrives while a Close is still draining, it escalates that Close to
+// a kill and the output is removed, unless the encoder had already exited: an
+// exit observed first lets Close finish and keep the video. Close's result is
+// authoritative in that race, and the Abort call itself then returns nil. Abort
+// after Close has finished is a no-op.
 func (w *Writer) Abort() error { return w.terminate(true) }
 func (w *Writer) terminate(abort bool) error {
 	if abort {
 		return w.discard()
 	}
-	timer := time.NewTimer(w.closeTimeout)
+	err := w.awaitExit()
-	defer timer.Stop()
-	var err error
-	select {
-	case <-w.waitDone:
-	case <-timer.C:
-		err = fmt.Errorf("ffmpeg: encoder did not finish within %s; killed", w.closeTimeout)
-	case <-w.abortCh:
-		err = errors.New("ffmpeg: shutdown aborted; encoder killed")
-	}
 	if err != nil {
 		_ = w.cmd.Process.Kill() // ErrProcessDone is harmless
 		kt := time.NewTimer(killTimeout)
 	return err
 }
+// awaitExit waits for the encoder to exit after EOF. It returns nil if the
+// process has exited, else the reason the wait was cut short (timeout or
+// Abort). An exit that is already visible wins over a timeout or Abort that
+// became ready at the same time; the remaining race is inherent and documented
+// on Abort.
+func (w *Writer) awaitExit() error {
+	timer := time.NewTimer(w.closeTimeout)
+	defer timer.Stop()
+	var err error
+	select {
+	case <-w.waitDone:
+		return nil
+	case <-timer.C:
+		err = fmt.Errorf("ffmpeg: encoder did not finish within %s; killed", w.closeTimeout)
+	case <-w.abortCh:
+		err = errors.New("ffmpeg: shutdown aborted; encoder killed")
+	}
+	select {
+	case <-w.waitDone:
+		return nil
+	default:
+		return err
+	}
+}
 // discard kills the encoder, waits for the reaper and removes the output.
 func (w *Writer) discard() error {
 	_ = w.cmd.Process.Kill() // ErrProcessDone is harmless

M internal/adapters/ffmpeg/ffmpeg_test.go +25 -0


@@ -921,3 +921,28 @@ func TestCloseKeepsVideoAfterCancelledWrite(t *testing.T) {
921	921		t.Fatalf("encoder output %q", got)
922	922		}
923	923		}
	924	+
	925	+	func TestAwaitExitPrefersObservedExit(t *testing.T) {
	926	+	for name, ready := range map[string]func(*Writer){
	927	+	"abort": func(w *Writer) { close(w.abortCh) },
	928	+	"timeout": func(w *Writer) { w.closeTimeout = 0 },
	929	+	} {
	930	+	t.Run(name, func(t *testing.T) {
	931	+	w := NewWithBinary("unused")
	932	+	w.waitDone = make(chan struct{})
	933	+	close(w.waitDone) // the encoder has already exited
	934	+	ready(w)
	935	+	if err := w.awaitExit(); err != nil {
	936	+	t.Fatalf("clean exit reported as %v", err)
	937	+	}
	938	+	})
	939	+	}
	940	+	t.Run("abort without exit", func(t *testing.T) {
	941	+	w := NewWithBinary("unused")
	942	+	w.waitDone = make(chan struct{})
	943	+	close(w.abortCh)
	944	+	if err := w.awaitExit(); err == nil {
	945	+	t.Fatal("abort before exit reported success")
	946	+	}
	947	+	})
	948	+	}


@@ -921,3 +921,28 @@ func TestCloseKeepsVideoAfterCancelledWrite(t *testing.T) {
921	t.Fatalf("encoder output %q", got)	921		t.Fatalf("encoder output %q", got)
922	}	922		}
923	}	923		}
		924	+
		925	+	func TestAwaitExitPrefersObservedExit(t *testing.T) {
		926	+	for name, ready := range map[string]func(*Writer){
		927	+	"abort": func(w *Writer) { close(w.abortCh) },
		928	+	"timeout": func(w *Writer) { w.closeTimeout = 0 },
		929	+	} {
		930	+	t.Run(name, func(t *testing.T) {
		931	+	w := NewWithBinary("unused")
		932	+	w.waitDone = make(chan struct{})
		933	+	close(w.waitDone) // the encoder has already exited
		934	+	ready(w)
		935	+	if err := w.awaitExit(); err != nil {
		936	+	t.Fatalf("clean exit reported as %v", err)
		937	+	}
		938	+	})
		939	+	}
		940	+	t.Run("abort without exit", func(t *testing.T) {
		941	+	w := NewWithBinary("unused")
		942	+	w.waitDone = make(chan struct{})
		943	+	close(w.abortCh)
		944	+	if err := w.awaitExit(); err == nil {
		945	+	t.Fatal("abort before exit reported success")
		946	+	}
		947	+	})
		948	+	}