From f199a718c8fc8238cf86150ca00650882f28aeaa Mon Sep 17 00:00:00 2001 From: Eli Mallon Date: Tue, 21 Jul 2026 17:24:51 -0700 Subject: [PATCH] media/rtcrec: give recording finalize a slow-uplink-sized window MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Greptile round two: the 30/40s finalize waits were shorter than the s3 per-op timeouts, so a slow-but-working upload could be abandoned by a worker exiting even though waiting would have saved the recording. At teardown up to ~128 MB of backpressured parts can still be uploading; give them 5 minutes — a post-stream worker lingering is cheap, a lost recording isn't. A genuinely stalled connection stays bounded by the per-op timeouts and is unrecoverable under any window; past the wait the recording is abandoned and logged, and bucket lifecycle rules should reap the dangling multipart. Co-Authored-By: Claude Fable 5 --- pkg/media/mkv_ingest.go | 9 ++++++++- pkg/rtcrec/recording_peerconnection.go | 9 +++++++-- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/pkg/media/mkv_ingest.go b/pkg/media/mkv_ingest.go index 2e9451a5..49cc5c63 100644 --- a/pkg/media/mkv_ingest.go +++ b/pkg/media/mkv_ingest.go @@ -125,7 +125,14 @@ func buildMKVIngestPipeline(ctx context.Context, input io.Reader, signerElem *gs // debugRecordingFlushTimeout bounds how long ingest teardown waits for a debug // recording to finalize — for S3 the commit only happens at Close, so an // unbounded wait could wedge teardown while an unwaited exit loses the object. -const debugRecordingFlushTimeout = 30 * time.Second +// Generous on purpose: at teardown there can be up to ~128 MB of backpressured +// parts still uploading (multipartUploadConcurrency × MultipartPartSize), and a +// slow-but-working uplink deserves the time to land them — a post-stream worker +// lingering is cheap, a lost recording isn't. A genuinely stalled connection is +// bounded separately by the s3 package's per-operation timeouts; past this +// window the recording is abandoned (logged by the dump goroutine when its op +// timeouts fire; bucket lifecycle rules should reap the dangling multipart). +const debugRecordingFlushTimeout = 5 * time.Minute // recordTee wires up a debug recording: everything read through the returned // reader is teed into an asynchronous dumpToFile. The returned finalize ends diff --git a/pkg/rtcrec/recording_peerconnection.go b/pkg/rtcrec/recording_peerconnection.go index 3e43f0dc..b7395638 100644 --- a/pkg/rtcrec/recording_peerconnection.go +++ b/pkg/rtcrec/recording_peerconnection.go @@ -99,8 +99,13 @@ func (pc *RecordingPeerConnection) FinalizeRecording(ctx context.Context) { } // recordingFinalizeTimeout bounds FinalizeRecording: the 10s straggler drain in -// finishRecording plus generous headroom for the S3 commit. -const recordingFinalizeTimeout = 40 * time.Second +// finishRecording plus generous headroom for the S3 commit — enough for a +// slow-but-working uplink to land any backpressured parts (a post-stream worker +// lingering is cheap, a lost recording isn't). A genuinely stalled connection +// is bounded separately by the s3 package's per-operation timeouts; past this +// window the recording is abandoned and the commit failure logged when those +// fire. +const recordingFinalizeTimeout = 5 * time.Minute func (pc *RecordingPeerConnection) CreateAnswer(options *webrtc.AnswerOptions) (webrtc.SessionDescription, error) { now := time.Now() -- 2.51.2