Skip to content

Commit f3bd551

Browse files
srperensclaude
andcommitted
fix(tams): stop retrying non-retryable uploads, clear stale segments, guard overflow
Three fixes from the PR review: - retry(): a gateway/storage HTTP 4xx (except 408/429) can never succeed on retry, yet the uploader slept through the full backoff and re-attempted 3x — producing repeated identical errors (e.g. the 413 Payload Too Large we hit). Errors now carry the status via a typed HttpStatusError and retry() gives up immediately on non-retryable ones. - Clear leftover seg_* files from a previous run at setup. Permanently-failed uploads are kept on disk but nothing reads them back, and there is no teardown hook, so they accumulated across restarts. Cleaning at setup (before the new run writes anything) bounds disk use without touching keep-on-failure. - max-size-time now uses saturating_mul, matching tail_segment_ns, so a large operator-supplied segment_duration_secs can't overflow u64. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent ed16d09 commit f3bd551

3 files changed

Lines changed: 96 additions & 9 deletions

File tree

backend/src/blocks/builtin/tams_output.rs

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -458,7 +458,7 @@ fn build_flow_chain(
458458
.build()
459459
.map_err(|e| BlockBuildError::ElementCreation(format!("splitmuxsink: {}", e)))?;
460460
splitmuxsink.set_property("muxer", &mux);
461-
splitmuxsink.set_property("max-size-time", segment_secs * 1_000_000_000);
461+
splitmuxsink.set_property("max-size-time", segment_secs.saturating_mul(1_000_000_000));
462462
// Fallback location template; the format-location-full signal overrides it.
463463
let fallback = temp_dir.join(format!("seg_%05d.{}", container.file_ext()));
464464
splitmuxsink.set_property("location", fallback.to_string_lossy().as_ref());
@@ -641,6 +641,30 @@ fn build_flow_chain(
641641
let file_ext = container.file_ext().to_string();
642642
let tail_segment_ns = segment_secs.saturating_mul(1_000_000_000);
643643
ctx.register_element_setup(Box::new(move |flow_id, events| {
644+
// Clear segment files left by a previous run of this instance. Failed
645+
// uploads are kept on disk, but nothing ever reads them back, so without
646+
// this they accumulate across restarts (the temp dir is per-instance and
647+
// there is no teardown hook). Safe here: the new run has not written yet.
648+
if let Ok(entries) = std::fs::read_dir(&temp_dir_for_setup) {
649+
let mut removed = 0u32;
650+
for entry in entries.flatten() {
651+
let path = entry.path();
652+
let is_segment = path
653+
.file_name()
654+
.and_then(|n| n.to_str())
655+
.is_some_and(|n| n.starts_with("seg_"));
656+
if is_segment && std::fs::remove_file(&path).is_ok() {
657+
removed += 1;
658+
}
659+
}
660+
if removed > 0 {
661+
info!(
662+
"TAMS {}: cleared {} leftover segment(s) from a previous run",
663+
block_id, removed
664+
);
665+
}
666+
}
667+
644668
// OSC auth keys its PAT by the flow id (tenant isolation on a shared
645669
// instance), which is only known here — so finalize the gateway client now.
646670
let credential_key = flow_id.to_string();

backend/src/tams/client.rs

Lines changed: 57 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,35 @@ use std::time::Duration;
2222
// frontend can share them. Re-exported here for ergonomic use within the backend.
2323
pub use strom_types::tams::{format_timerange, FORMAT_AUDIO, FORMAT_VIDEO};
2424

25+
/// A non-success HTTP response from the gateway or presigned storage, carrying
26+
/// the status code so the uploader can decide whether retrying could ever help.
27+
#[derive(Debug)]
28+
pub struct HttpStatusError {
29+
pub status: reqwest::StatusCode,
30+
/// Human context, e.g. `presigned PUT` or `POST http://.../flows/x/segments`.
31+
pub context: String,
32+
pub body: String,
33+
}
34+
35+
impl std::fmt::Display for HttpStatusError {
36+
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
37+
write!(f, "{} -> {}: {}", self.context, self.status, self.body)
38+
}
39+
}
40+
41+
impl std::error::Error for HttpStatusError {}
42+
43+
impl HttpStatusError {
44+
/// A 4xx (other than 408 Request Timeout and 429 Too Many Requests) means the
45+
/// request itself must change before it can succeed — retrying the identical
46+
/// bytes/headers will fail the same way (e.g. 413 Payload Too Large, 401
47+
/// Unauthorized). Network errors and 5xx/408/429 are worth retrying.
48+
pub fn is_retryable(&self) -> bool {
49+
let code = self.status.as_u16();
50+
!((400..500).contains(&code) && code != 408 && code != 429)
51+
}
52+
}
53+
2554
/// Metadata describing a flow to create on the gateway.
2655
#[derive(Debug, Clone)]
2756
pub struct FlowSpec {
@@ -129,8 +158,13 @@ impl TamsClient {
129158
.with_context(|| format!("PUT {}", url))?;
130159
if !resp.status().is_success() {
131160
let status = resp.status();
132-
let text = resp.text().await.unwrap_or_default();
133-
return Err(anyhow!("PUT {} -> {}: {}", url, status, text));
161+
let body = resp.text().await.unwrap_or_default();
162+
return Err(HttpStatusError {
163+
status,
164+
context: format!("PUT {}", url),
165+
body,
166+
}
167+
.into());
134168
}
135169
Ok(())
136170
}
@@ -153,8 +187,13 @@ impl TamsClient {
153187
.with_context(|| format!("POST {}", url))?;
154188
if !resp.status().is_success() {
155189
let status = resp.status();
156-
let text = resp.text().await.unwrap_or_default();
157-
return Err(anyhow!("POST {} -> {}: {}", url, status, text));
190+
let body = resp.text().await.unwrap_or_default();
191+
return Err(HttpStatusError {
192+
status,
193+
context: format!("POST {}", url),
194+
body,
195+
}
196+
.into());
158197
}
159198
let parsed: StorageResponse = resp.json().await.context("parsing storage response")?;
160199
let obj = parsed
@@ -187,8 +226,13 @@ impl TamsClient {
187226
.context("PUT presigned S3 url")?;
188227
if !resp.status().is_success() {
189228
let status = resp.status();
190-
let text = resp.text().await.unwrap_or_default();
191-
return Err(anyhow!("presigned PUT -> {}: {}", status, text));
229+
let body = resp.text().await.unwrap_or_default();
230+
return Err(HttpStatusError {
231+
status,
232+
context: "presigned PUT".to_string(),
233+
body,
234+
}
235+
.into());
192236
}
193237
Ok(())
194238
}
@@ -217,8 +261,13 @@ impl TamsClient {
217261
return Err(anyhow!("segment registration partially failed: {}", text));
218262
}
219263
if !status.is_success() {
220-
let text = resp.text().await.unwrap_or_default();
221-
return Err(anyhow!("POST {} -> {}: {}", url, status, text));
264+
let body = resp.text().await.unwrap_or_default();
265+
return Err(HttpStatusError {
266+
status,
267+
context: format!("POST {}", url),
268+
body,
269+
}
270+
.into());
222271
}
223272
Ok(())
224273
}

backend/src/tams/uploader.rs

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -262,6 +262,12 @@ where
262262
match op().await {
263263
Ok(v) => return Ok(v),
264264
Err(e) => {
265+
// A non-retryable HTTP status (e.g. 413 Payload Too Large, 401
266+
// Unauthorized) will fail identically on every attempt — give up
267+
// now rather than sleeping through the whole backoff schedule.
268+
if !is_retryable(&e) {
269+
return Err(e);
270+
}
265271
if attempt + 1 < attempts {
266272
let backoff = Duration::from_secs(1u64 << attempt);
267273
tokio::time::sleep(backoff).await;
@@ -273,6 +279,14 @@ where
273279
Err(last_err.unwrap_or_else(|| anyhow::anyhow!("operation failed")))
274280
}
275281

282+
/// Whether an error is worth retrying. A gateway/storage HTTP 4xx (except
283+
/// 408/429) cannot succeed on retry; everything else (network errors, 5xx) can.
284+
fn is_retryable(e: &anyhow::Error) -> bool {
285+
e.downcast_ref::<crate::tams::client::HttpStatusError>()
286+
.map(|h| h.is_retryable())
287+
.unwrap_or(true)
288+
}
289+
276290
/// Allocate, upload and register a single fragment (bytes already read by the
277291
/// caller). Returns the registered `(object_id, timerange)` on success.
278292
async fn upload_one(

0 commit comments

Comments
 (0)