From d34583771397ba9bcaa5c932c34547025bbdffa7 Mon Sep 17 00:00:00 2001 From: Alice Alexandra Moore <86723305+3mdistal@users.noreply.github.com> Date: Fri, 21 Aug 2026 09:01:38 -0400 Subject: [PATCH 01/11] fix(clips): reclaim stale upload retries --- templates/clips/desktop/src-tauri/Cargo.lock | 23 ++ templates/clips/desktop/src-tauri/Cargo.toml | 2 +- templates/clips/desktop/src-tauri/src/lib.rs | 1 + .../desktop/src-tauri/src/native_screen.rs | 239 +++++++++++++++--- templates/clips/desktop/src/app.tsx | 86 ++++++- templates/clips/desktop/src/lib/recorder.ts | 94 +++++-- .../desktop/src/lib/upload-recovery.test.ts | 65 ++++- .../clips/desktop/src/lib/upload-recovery.ts | 22 +- .../uploads/[recordingId]/chunk.post.test.ts | 98 ++++++- .../api/uploads/[recordingId]/chunk.post.ts | 86 +++++-- .../[recordingId]/reset-chunks.post.test.ts | 28 +- .../[recordingId]/reset-chunks.post.ts | 24 +- .../uploads/[recordingId]/resume.get.test.ts | 137 +++++++++- .../api/uploads/[recordingId]/resume.get.ts | 111 ++++++-- 14 files changed, 875 insertions(+), 141 deletions(-) diff --git a/templates/clips/desktop/src-tauri/Cargo.lock b/templates/clips/desktop/src-tauri/Cargo.lock index 907774909f..19efb91774 100644 --- a/templates/clips/desktop/src-tauri/Cargo.lock +++ b/templates/clips/desktop/src-tauri/Cargo.lock @@ -5303,6 +5303,17 @@ dependencies = [ "unicode-ident", ] +[[package]] +name = "syn" +version = "3.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f2fac314a64dc9a36e61a9eb4261a5e9bbfbc922b27e518af97bc32b926cf967" +dependencies = [ + "proc-macro2", + "quote", + "unicode-ident", +] + [[package]] name = "sync_wrapper" version = "1.0.2" @@ -6002,9 +6013,21 @@ dependencies = [ "pin-project-lite", "signal-hook-registry", "socket2 0.6.3", + "tokio-macros", "windows-sys 0.61.2", ] +[[package]] +name = "tokio-macros" +version = "2.7.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "78773a2a397f451582ce068015985c33193cf6dea8b74d2a639fe457b2f07b0e" +dependencies = [ + "proc-macro2", + "quote", + "syn 3.0.0", +] + [[package]] name = "tokio-native-tls" version = "0.3.1" diff --git a/templates/clips/desktop/src-tauri/Cargo.toml b/templates/clips/desktop/src-tauri/Cargo.toml index fe76297b25..ae954b534d 100644 --- a/templates/clips/desktop/src-tauri/Cargo.toml +++ b/templates/clips/desktop/src-tauri/Cargo.toml @@ -67,7 +67,7 @@ reqwest = { version = "0.12", default-features = false, features = [ "rustls-tls", "json", ] } -tokio = { version = "1", features = ["rt", "rt-multi-thread", "time", "sync"] } +tokio = { version = "1", features = ["rt", "rt-multi-thread", "time", "sync", "macros"] } chrono = { version = "0.4", features = ["serde"] } # Verify the integrity of the Whisper model we download from HuggingFace. sha2 = "0.10" diff --git a/templates/clips/desktop/src-tauri/src/lib.rs b/templates/clips/desktop/src-tauri/src/lib.rs index 85d6578fc9..9c5a8c90c2 100644 --- a/templates/clips/desktop/src-tauri/src/lib.rs +++ b/templates/clips/desktop/src-tauri/src/lib.rs @@ -161,6 +161,7 @@ pub fn run() { native_screen::native_fullscreen_pending_uploads, native_screen::native_fullscreen_recover_orphaned_uploads, native_screen::native_fullscreen_recording_retry_upload, + native_screen::native_fullscreen_recording_cancel_retry, native_screen::native_fullscreen_recording_mark_upload_error, native_screen::native_fullscreen_recording_clear_upload, native_screen::native_fullscreen_recording_dismiss_upload, diff --git a/templates/clips/desktop/src-tauri/src/native_screen.rs b/templates/clips/desktop/src-tauri/src/native_screen.rs index 505d5330b5..14dcd53e83 100644 --- a/templates/clips/desktop/src-tauri/src/native_screen.rs +++ b/templates/clips/desktop/src-tauri/src/native_screen.rs @@ -211,6 +211,8 @@ struct NativeUploadResumeResponse { next_chunk_index: Option, attempt_id: Option, upload_generation_id: Option, + reason: Option, + retry_after_ms: Option, } #[derive(Debug, Deserialize)] @@ -956,6 +958,8 @@ fn clear_recording_active(app: &AppHandle) { static LAST_NATIVE_UPLOAD_FINISHED: OnceLock>> = OnceLock::new(); static CLAIMED_NATIVE_UPLOAD_OPEN: OnceLock>> = OnceLock::new(); +static CANCELLED_NATIVE_UPLOAD_RETRIES: OnceLock>> = OnceLock::new(); +const NATIVE_UPLOAD_RETRY_CANCELLED: &str = "native recording upload retry cancelled"; fn last_native_upload_finished() -> &'static Mutex> { LAST_NATIVE_UPLOAD_FINISHED.get_or_init(|| Mutex::new(None)) @@ -965,6 +969,29 @@ fn claimed_native_upload_open() -> &'static Mutex> { CLAIMED_NATIVE_UPLOAD_OPEN.get_or_init(|| Mutex::new(None)) } +fn cancelled_native_upload_retries() -> &'static Mutex> { + CANCELLED_NATIVE_UPLOAD_RETRIES.get_or_init(|| Mutex::new(BTreeSet::new())) +} + +fn native_upload_retry_cancelled(recording_id: &str) -> bool { + cancelled_native_upload_retries() + .lock() + .map(|cancelled| cancelled.contains(recording_id)) + .unwrap_or(true) +} + +fn clear_native_upload_retry_cancelled(recording_id: &str) { + if let Ok(mut cancelled) = cancelled_native_upload_retries().lock() { + cancelled.remove(recording_id); + } +} + +async fn wait_for_native_upload_retry_cancel(recording_id: &str) { + while !native_upload_retry_cancelled(recording_id) { + tokio::time::sleep(Duration::from_millis(100)).await; + } +} + fn reset_native_upload_completion_state() { if let Ok(mut last) = last_native_upload_finished().lock() { *last = None; @@ -2130,6 +2157,7 @@ pub async fn native_fullscreen_recording_stop_and_upload( match result { Ok(result) => { + clear_native_upload_retry_cancelled(&recording_id); if !result.verification_pending { clear_saved_recording_after_success(&app, &saved); } @@ -3636,6 +3664,7 @@ pub async fn native_fullscreen_recording_retry_upload( auth_token: Option, cookie: Option, ) -> Result { + clear_native_upload_retry_cancelled(&recording_id); let mut saved = read_saved_recording_metadata(&app, &recording_id)?; saved.server_url = server_url.trim_end_matches('/').to_string(); saved.last_attempt_at = Some(now_iso()); @@ -3658,6 +3687,7 @@ pub async fn native_fullscreen_recording_retry_upload( // second click cannot steal an upload session already owned by this // local recording. let retry_plan = match get_native_retry_upload_plan( + &app, &saved.server_url, &saved.recording_id, prepared.bytes, @@ -3670,16 +3700,18 @@ pub async fn native_fullscreen_recording_retry_upload( { Ok(plan) => plan, Err(err) => { - interrupt_native_retry_upload( - &saved.server_url, - &saved.recording_id, - &err, - Some(&claimed_attempt_id), - None, - &auth_token, - &cookie, - ) - .await; + if err != NATIVE_UPLOAD_RETRY_CANCELLED { + interrupt_native_retry_upload( + &saved.server_url, + &saved.recording_id, + &err, + Some(&claimed_attempt_id), + None, + &auth_token, + &cookie, + ) + .await; + } cleanup_prepared_saved_recording_files(&prepared, retry_combined_path); return Err(err); } @@ -3863,6 +3895,11 @@ pub async fn native_fullscreen_recording_retry_upload( Ok(result) } Err(err) => { + clear_native_upload_retry_cancelled(&recording_id); + if err == NATIVE_UPLOAD_RETRY_CANCELLED { + emit_native_upload_progress(&app, "paused", "Retry cancelled", None, None); + return Err(err); + } if is_moov_corrupt_error(&err) { saved.corrupt = true; } @@ -3878,6 +3915,15 @@ pub async fn native_fullscreen_recording_retry_upload( } } +#[tauri::command] +pub fn native_fullscreen_recording_cancel_retry(recording_id: String) -> Result<(), String> { + cancelled_native_upload_retries() + .lock() + .map_err(|_| "native upload retry cancellation state is unavailable".to_string())? + .insert(recording_id); + Ok(()) +} + #[tauri::command] pub async fn native_fullscreen_recording_mark_upload_error( app: AppHandle, @@ -5708,6 +5754,7 @@ async fn upload_prepared_recording_file( } async fn get_native_retry_upload_plan( + app: &AppHandle, server_url: &str, recording_id: &str, local_bytes: u64, @@ -5735,30 +5782,103 @@ async fn get_native_retry_upload_plan( if !cookie.trim().is_empty() { request = request.header("Cookie", cookie.trim()); } - let response = request - .send() - .await - .map_err(|e| format!("native recording resume check failed: {e}"))?; - let status = response.status(); - let body = response.text().await.unwrap_or_default(); - if !status.is_success() { - return Err(format!( - "native recording resume check returned {status}: {}", - body.chars().take(400).collect::() + let deadline = tokio::time::Instant::now() + Duration::from_secs(5 * 60); + loop { + if native_upload_retry_cancelled(recording_id) { + return Err(NATIVE_UPLOAD_RETRY_CANCELLED.to_string()); + } + let response = tokio::select! { + response = request + .try_clone() + .ok_or_else(|| "native recording resume request could not be retried".to_string())? + .send() => response.map_err(|e| format!("native recording resume check failed: {e}"))?, + _ = wait_for_native_upload_retry_cancel(recording_id) => { + return Err(NATIVE_UPLOAD_RETRY_CANCELLED.to_string()); + } + }; + let status = response.status(); + let body = response.text().await.unwrap_or_default(); + let parsed = serde_json::from_str::(&body); + if !status.is_success() { + if let Ok(conflict) = &parsed { + if let Some(delay) = native_retry_conflict_delay(conflict) { + if tokio::time::Instant::now() + delay <= deadline { + emit_native_upload_progress( + app, + "uploading", + "Waiting for prior retry", + None, + None, + ); + tokio::select! { + _ = tokio::time::sleep(delay) => {} + _ = wait_for_native_upload_retry_cancel(recording_id) => { + return Err(NATIVE_UPLOAD_RETRY_CANCELLED.to_string()); + } + } + continue; + } + return Err( + "Another upload retry is still active. Wait a moment and try again" + .to_string(), + ); + } + if conflict.reason.as_deref() == Some("retry_claim_liveness_unavailable") { + return Err( + "Clips could not verify whether another retry is active".to_string() + ); + } + } + return Err(format!("native recording resume check failed ({status})")); + } + let response = parsed.map_err(|_| { + "native recording resume check returned an unreadable response".to_string() + })?; + if response.resumable && response.attempt_id.as_deref() != Some(claimed_attempt_id) { + return Err( + "native recording resume check did not acknowledge its attempt claim".to_string(), + ); + } + let recovery_enabled = response.recovery_enabled; + let rollback_generation_id = response.upload_generation_id.clone(); + return Ok(preserve_native_retry_fence_during_rollback( + plan_native_retry_upload(response, local_bytes, exact_local_stream), + recovery_enabled, + claimed_attempt_id, + rollback_generation_id, )); } - let response: NativeUploadResumeResponse = serde_json::from_str(&body) - .map_err(|_| "native recording resume check returned an unreadable response".to_string())?; - if response.resumable && response.attempt_id.as_deref() != Some(claimed_attempt_id) { - return Err( - "native recording resume check did not acknowledge its attempt claim".to_string(), - ); +} + +fn preserve_native_retry_fence_during_rollback( + mut plan: NativeRetryUploadPlan, + recovery_enabled: bool, + claimed_attempt_id: &str, + upload_generation_id: Option, +) -> NativeRetryUploadPlan { + if !recovery_enabled { + if let NativeRetryUploadPlan::Restart { + attempt_id, + upload_generation_id: planned_generation_id, + } = &mut plan + { + *attempt_id = Some(claimed_attempt_id.to_string()); + *planned_generation_id = upload_generation_id; + } } - Ok(plan_native_retry_upload( - response, - local_bytes, - exact_local_stream, - )) + plan +} + +fn native_retry_conflict_delay(response: &NativeUploadResumeResponse) -> Option { + if response.resumable + || !response.recovery_enabled + || response.reason.as_deref() != Some("retry_already_active") + { + return None; + } + response + .retry_after_ms + .map(|delay| Duration::from_millis(delay.clamp(250, 30_000))) } #[derive(Deserialize)] @@ -5945,8 +6065,9 @@ fn native_retry_interruption_payload( mod native_retry_upload_plan_tests { use super::{ is_native_upload_restart_required, is_native_upload_unfenced_restart_required, - native_replay_attempt_id, native_retry_attempt_id, native_retry_interruption_payload, - plan_native_retry_upload, saved_native_retry_attempt_id, upload_url, + native_replay_attempt_id, native_retry_attempt_id, native_retry_conflict_delay, + native_retry_interruption_payload, plan_native_retry_upload, + preserve_native_retry_fence_during_rollback, saved_native_retry_attempt_id, upload_url, NativeFullscreenUploadResult, NativeRetryUploadPlan, NativeUploadResumeResponse, NATIVE_UPLOAD_RESTART_REQUIRED, NATIVE_UPLOAD_UNFENCED_RESTART_REQUIRED, UPLOAD_CHUNK_BYTES, @@ -5962,6 +6083,8 @@ mod native_retry_upload_plan_tests { next_chunk_index: Some(next_chunk_index), attempt_id: Some("attempt-1".to_string()), upload_generation_id: Some("generation-1".to_string()), + reason: None, + retry_after_ms: None, } } @@ -6013,6 +6136,8 @@ mod native_retry_upload_plan_tests { next_chunk_index: None, attempt_id: Some("ignored-attempt".to_string()), upload_generation_id: Some("ignored-generation".to_string()), + reason: Some("feature_disabled".to_string()), + retry_after_ms: None, }, UPLOAD_CHUNK_BYTES as u64, true, @@ -6027,6 +6152,26 @@ mod native_retry_upload_plan_tests { )); } + #[test] + fn preserves_an_existing_fence_when_resumable_retry_is_disabled() { + let plan = preserve_native_retry_fence_during_rollback( + NativeRetryUploadPlan::Restart { + attempt_id: None, + upload_generation_id: None, + }, + false, + "attempt-1", + Some("generation-1".to_string()), + ); + assert!(matches!( + plan, + NativeRetryUploadPlan::Restart { + attempt_id: Some(attempt_id), + upload_generation_id: Some(generation_id), + } if attempt_id == "attempt-1" && generation_id == "generation-1" + )); + } + #[test] fn reconciles_terminal_resume_without_an_attempt_echo() { let terminal = plan_native_retry_upload( @@ -6039,6 +6184,8 @@ mod native_retry_upload_plan_tests { next_chunk_index: None, attempt_id: None, upload_generation_id: None, + reason: None, + retry_after_ms: None, }, UPLOAD_CHUNK_BYTES as u64, true, @@ -6064,6 +6211,8 @@ mod native_retry_upload_plan_tests { next_chunk_index: Some(0), attempt_id: Some(claimed_attempt_id.clone()), upload_generation_id: Some("generation-1".to_string()), + reason: None, + retry_after_ms: None, }, UPLOAD_CHUNK_BYTES as u64, true, @@ -6108,6 +6257,30 @@ mod native_retry_upload_plan_tests { assert_eq!(saved_attempt_id.as_deref(), Some(first.as_str())); } + #[test] + fn waits_only_for_a_typed_bounded_retry_conflict() { + let conflict = NativeUploadResumeResponse { + resumable: false, + recovery_enabled: true, + status: Some("uploading".to_string()), + upload_mode: None, + bytes_received: None, + next_chunk_index: None, + attempt_id: None, + upload_generation_id: None, + reason: Some("retry_already_active".to_string()), + retry_after_ms: Some(60_000), + }; + assert_eq!( + native_retry_conflict_delay(&conflict), + Some(std::time::Duration::from_secs(30)) + ); + + let mut untyped = conflict; + untyped.retry_after_ms = None; + assert_eq!(native_retry_conflict_delay(&untyped), None); + } + #[test] fn reports_retry_interruptions_with_or_without_a_fencing_claim() { let unfenced = native_retry_interruption_payload("upload failed", None, None); diff --git a/templates/clips/desktop/src/app.tsx b/templates/clips/desktop/src/app.tsx index d308b67d19..c7c0f9aec8 100644 --- a/templates/clips/desktop/src/app.tsx +++ b/templates/clips/desktop/src/app.tsx @@ -160,6 +160,10 @@ interface PendingNativeUpload { type PendingDesktopUpload = PendingNativeUpload | PendingBrowserRecordingUpload; +type NativeUploadProgress = { + message?: string; +}; + type PopoverView = | "recorder" | "memory" @@ -967,6 +971,26 @@ export function App() { const [retryingUploadStatus, setRetryingUploadStatus] = useState< string | null >(null); + const retryUploadAbortRef = useRef(null); + const retryingUploadKindRef = useRef( + null, + ); + useEffect(() => { + if (!retryingUploadId) return; + let disposed = false; + let unlisten: (() => void) | null = null; + listen("clips:native-upload-progress", (event) => { + const message = event.payload?.message?.trim(); + if (message) setRetryingUploadStatus(message); + }).then((cleanup) => { + if (disposed) cleanup(); + else unlisten = cleanup; + }); + return () => { + disposed = true; + unlisten?.(); + }; + }, [retryingUploadId]); const [exportingUploadId, setExportingUploadId] = useState( null, ); @@ -2873,6 +2897,9 @@ export function App() { const targetServerUrl = serverUrlForPendingUpload(upload, serverUrl); setRecError(null); setRetryingUploadId(upload.recordingId); + const abortController = new AbortController(); + retryUploadAbortRef.current = abortController; + retryingUploadKindRef.current = upload.kind; try { const authToken = loadDesktopAuthToken(targetServerUrl); if (upload.kind === "native") { @@ -2898,13 +2925,16 @@ export function App() { recordingId: upload.recordingId, serverUrl: targetServerUrl, authToken, + signal: abortController.signal, onRecoveryDecision: ({ action, progress }) => { setRetryingUploadStatus( action === "resume" ? `Resuming ยท ${Math.round(progress * 100)}% already uploaded` - : action === "restart" - ? "Restarting upload" - : "Finishing upload", + : action === "wait" + ? "Waiting for prior retry" + : action === "restart" + ? "Restarting upload" + : "Finishing upload", ); }, }); @@ -2918,6 +2948,14 @@ export function App() { emit("clips:popover-visible", false).catch(() => {}); } catch (err) { const message = err instanceof Error ? err.message : String(err); + if ( + abortController.signal.aborted || + (err instanceof DOMException && err.name === "AbortError") || + message === "native recording upload retry cancelled" + ) { + await loadPendingUploads(); + return; + } console.error("[clips-tray] retry saved upload failed:", err); setRecError( isStorageSetupFailureMessage(message) @@ -2926,11 +2964,28 @@ export function App() { ); await loadPendingUploads(); } finally { + if (retryUploadAbortRef.current === abortController) { + retryUploadAbortRef.current = null; + retryingUploadKindRef.current = null; + } setRetryingUploadId(null); setRetryingUploadStatus(null); } } + function cancelPendingUploadRetry(upload: PendingDesktopUpload) { + if (retryingUploadId !== upload.recordingId) return; + retryUploadAbortRef.current?.abort(); + if (retryingUploadKindRef.current === "native") { + invoke("native_fullscreen_recording_cancel_retry", { + recordingId: upload.recordingId, + }).catch((err) => { + console.error("[clips-tray] cancel saved upload retry failed:", err); + }); + } + setRetryingUploadStatus("Cancelling retry"); + } + async function exportPendingUpload(upload: PendingDesktopUpload) { if (retryingUploadId || exportingUploadId || dismissingUploadId) return; setRecError(null); @@ -3648,6 +3703,7 @@ export function App() { dismissingUploadId={dismissingUploadId} onExport={exportPendingUpload} onRetry={retryPendingUpload} + onCancelRetry={cancelPendingUploadRetry} onDismiss={dismissPendingUpload} onOpenFolder={openPendingUploadFolder} onConnectStorage={(upload) => openVideoStorageSetup(upload.serverUrl)} @@ -4352,6 +4408,7 @@ function PendingUploadBanner({ dismissingUploadId, onExport, onRetry, + onCancelRetry, onDismiss, onOpenFolder, onConnectStorage, @@ -4363,6 +4420,7 @@ function PendingUploadBanner({ dismissingUploadId: string | null; onExport: (upload: PendingDesktopUpload) => void; onRetry: (upload: PendingDesktopUpload) => void; + onCancelRetry: (upload: PendingDesktopUpload) => void; onDismiss: (upload: PendingDesktopUpload) => void; onOpenFolder: (upload: PendingDesktopUpload) => void; onConnectStorage: (upload: PendingDesktopUpload) => void; @@ -4371,6 +4429,8 @@ function PendingUploadBanner({ if (!latest) return null; const retrying = retryingUploadId === latest.recordingId; + const retryWaiting = + retrying && retryingUploadStatus === "Waiting for prior retry"; const storageSetupFailure = isStorageSetupFailureMessage(latest.lastError); const canOpenFolder = latest.kind === "native" && !!latest.folderPath; @@ -4477,12 +4537,24 @@ function PendingUploadBanner({ )} diff --git a/templates/clips/desktop/src/lib/recorder.ts b/templates/clips/desktop/src/lib/recorder.ts index fe3bc77028..7341f7ae6e 100644 --- a/templates/clips/desktop/src/lib/recorder.ts +++ b/templates/clips/desktop/src/lib/recorder.ts @@ -104,6 +104,7 @@ import { planStreamingRecovery, retryAttemptIdAfterRestartSignal, retryAttemptIdAfterResumeResponse, + retryConflictDelay, type UploadResumeResponse, } from "./upload-recovery"; import { @@ -1147,32 +1148,55 @@ async function getBrowserRecordingUploadResume( meta: BrowserRecordingBackupMeta, attemptId: string, authToken?: string, + onWaiting?: (delayMs: number) => void, + signal?: AbortSignal, ): Promise { const resumeUrl = new URL( `${meta.serverUrl.replace(/\/+$/, "")}/api/uploads/${meta.recordingId}/resume`, ); resumeUrl.searchParams.set("attemptId", attemptId); - const res = await fetch(resumeUrl, { - method: "GET", - headers: buildRetryHeaders("application/json", authToken), - credentials: "include", - }); - const body = await res.text().catch(() => ""); - if (!res.ok) { - throw new Error( - `Upload resume check failed (${res.status}): ${body.slice(0, 200)}`, - ); - } - let parsed: UploadResumeResponse; - try { - parsed = JSON.parse(body) as UploadResumeResponse; - } catch { - throw new Error("Upload resume check returned an unreadable response"); - } - if (parsed.resumable && parsed.attemptId !== attemptId) { - throw new Error("Upload resume check returned a mismatched retry token"); + const deadline = Date.now() + 5 * 60_000; + for (;;) { + const res = await fetch(resumeUrl, { + method: "GET", + headers: buildRetryHeaders("application/json", authToken), + credentials: "include", + signal, + }); + const body = await res.text().catch(() => ""); + let parsed: UploadResumeResponse; + try { + parsed = JSON.parse(body) as UploadResumeResponse; + } catch { + throw new Error("Upload resume check returned an unreadable response"); + } + if (!res.ok) { + const delayMs = retryConflictDelay(parsed); + if (delayMs !== null && Date.now() + delayMs <= deadline) { + onWaiting?.(delayMs); + await abortableWait(delayMs, signal); + continue; + } + if (!parsed.resumable && parsed.reason === "retry_already_active") { + throw new Error( + "Another upload retry is still active. Wait a moment and try again.", + ); + } + if ( + !parsed.resumable && + parsed.reason === "retry_claim_liveness_unavailable" + ) { + throw new Error( + "Clips could not verify whether another retry is active. Your local clip is safe; try again.", + ); + } + throw new Error(`Upload resume check failed (${res.status})`); + } + if (parsed.resumable && parsed.attemptId !== attemptId) { + throw new Error("Upload resume check returned a mismatched retry token"); + } + return parsed; } - return parsed; } async function replayBrowserBackupToResumableSession( @@ -1250,8 +1274,9 @@ export async function retryBrowserRecordingBackup(input: { recordingId: string; serverUrl?: string; authToken?: string; + signal?: AbortSignal; onRecoveryDecision?: (decision: { - action: "resume" | "restart" | "reconcile"; + action: "wait" | "resume" | "restart" | "reconcile"; progress: number; }) => void; }): Promise<{ recordingId: string; viewUrl: string }> { @@ -1284,6 +1309,8 @@ export async function retryBrowserRecordingBackup(input: { meta, activeAttemptId, input.authToken, + () => input.onRecoveryDecision?.({ action: "wait", progress: 0 }), + input.signal, ); const recoveryPlan = planStreamingRecovery({ response: resumeResponse, @@ -1294,7 +1321,7 @@ export async function retryBrowserRecordingBackup(input: { activeAttemptId, resumeResponse, ); - activeUploadGenerationId = resumeResponse.resumable + activeUploadGenerationId = activeAttemptId ? resumeResponse.uploadGenerationId : undefined; if (recoveryPlan.action === "reconcile") { @@ -1498,6 +1525,12 @@ export async function retryBrowserRecordingBackup(input: { await deleteBrowserRecordingBackup(meta.recordingId); return { recordingId: meta.recordingId, viewUrl: `/r/${meta.recordingId}` }; } catch (err) { + if ( + input.signal?.aborted || + (err instanceof DOMException && err.name === "AbortError") + ) { + throw err; + } const message = err instanceof Error ? err.message : String(err); if ( await recoverAcceptedRecordingAfterFinalizeError({ @@ -1638,6 +1671,23 @@ function wait(ms: number): Promise { return new Promise((resolve) => window.setTimeout(resolve, ms)); } +function abortableWait(ms: number, signal?: AbortSignal): Promise { + if (!signal) return wait(ms); + if (signal.aborted) + return Promise.reject(new DOMException("Aborted", "AbortError")); + return new Promise((resolve, reject) => { + const timer = window.setTimeout(() => { + signal.removeEventListener("abort", onAbort); + resolve(); + }, ms); + const onAbort = () => { + window.clearTimeout(timer); + reject(new DOMException("Aborted", "AbortError")); + }; + signal.addEventListener("abort", onAbort, { once: true }); + }); +} + interface NativeFullscreenUploadResult { recordingId: string; durationMs: number; diff --git a/templates/clips/desktop/src/lib/upload-recovery.test.ts b/templates/clips/desktop/src/lib/upload-recovery.test.ts index eb9dd3d301..3ba0925ad7 100644 --- a/templates/clips/desktop/src/lib/upload-recovery.test.ts +++ b/templates/clips/desktop/src/lib/upload-recovery.test.ts @@ -3,17 +3,61 @@ import { describe, expect, it } from "vitest"; import { buildStreamingReplayPlan, planStreamingRecovery, + retryConflictDelay, retryAttemptIdAfterRestartSignal, retryAttemptIdAfterResumeResponse, } from "./upload-recovery"; const CHUNK_BYTES = 3_932_160; -describe("retryAttemptIdAfterRestartSignal", () => { - it("drops the retry claim only when the server disables recovery", () => { +describe("retryConflictDelay", () => { + it("accepts only a typed, bounded active-retry delay", () => { expect( - retryAttemptIdAfterRestartSignal("attempt-1", false), - ).toBeUndefined(); + retryConflictDelay({ + resumable: false, + recoveryEnabled: true, + status: "uploading", + reason: "retry_already_active", + retryAfterMs: 1_500, + }), + ).toBe(1_500); + expect( + retryConflictDelay({ + resumable: false, + recoveryEnabled: true, + status: "uploading", + reason: "retry_already_active", + retryAfterMs: 60_000, + }), + ).toBe(30_000); + }); + + it("rejects untyped and unreadable conflict delays", () => { + expect( + retryConflictDelay({ + resumable: false, + recoveryEnabled: true, + status: "uploading", + reason: "retry_already_active", + }), + ).toBeNull(); + expect( + retryConflictDelay({ + resumable: false, + recoveryEnabled: true, + status: "uploading", + reason: "upload_state_changed", + retryAfterMs: 1_000, + }), + ).toBeNull(); + }); +}); + +describe("retryAttemptIdAfterRestartSignal", () => { + it("preserves an existing retry claim through a flag rollback", () => { + expect(retryAttemptIdAfterRestartSignal("attempt-1", false)).toBe( + "attempt-1", + ); expect(retryAttemptIdAfterRestartSignal("attempt-1", true)).toBe( "attempt-1", ); @@ -48,6 +92,19 @@ describe("retryAttemptIdAfterResumeResponse", () => { }), ).toBeUndefined(); }); + + it("preserves a local claim while the flag is rolled back", () => { + expect( + retryAttemptIdAfterResumeResponse("attempt-1", { + resumable: false, + recoveryEnabled: false, + status: "uploading", + reason: "feature_disabled", + attemptId: "attempt-1", + uploadGenerationId: "generation-1", + }), + ).toBe("attempt-1"); + }); }); describe("planStreamingRecovery", () => { diff --git a/templates/clips/desktop/src/lib/upload-recovery.ts b/templates/clips/desktop/src/lib/upload-recovery.ts index a8f160ee82..65ee3c083e 100644 --- a/templates/clips/desktop/src/lib/upload-recovery.ts +++ b/templates/clips/desktop/src/lib/upload-recovery.ts @@ -17,8 +17,25 @@ export type UploadResumeResponse = videoUrl?: string | null; reason?: string; attemptId?: string; + uploadGenerationId?: string; + retryAfterMs?: number; }; +export function retryConflictDelay( + response: UploadResumeResponse, +): number | null { + if ( + response.resumable || + response.recoveryEnabled !== true || + response.reason !== "retry_already_active" || + !Number.isSafeInteger(response.retryAfterMs) || + (response.retryAfterMs ?? 0) <= 0 + ) { + return null; + } + return Math.min(Math.max(response.retryAfterMs!, 250), 30_000); +} + export type StreamingRecoveryPlan = | { action: "resume"; @@ -38,15 +55,16 @@ export interface StreamingReplayRequest { export function retryAttemptIdAfterRestartSignal( attemptId: string | undefined, - recoveryEnabled: unknown, + _recoveryEnabled: unknown, ): string | undefined { - return recoveryEnabled === false ? undefined : attemptId; + return attemptId; } export function retryAttemptIdAfterResumeResponse( attemptId: string | undefined, response: UploadResumeResponse, ): string | undefined { + if (response.recoveryEnabled === false) return attemptId; return response.resumable && response.attemptId === attemptId ? attemptId : undefined; diff --git a/templates/clips/server/routes/api/uploads/[recordingId]/chunk.post.test.ts b/templates/clips/server/routes/api/uploads/[recordingId]/chunk.post.test.ts index bb52b73d0c..c61878bdbb 100644 --- a/templates/clips/server/routes/api/uploads/[recordingId]/chunk.post.test.ts +++ b/templates/clips/server/routes/api/uploads/[recordingId]/chunk.post.test.ts @@ -311,7 +311,7 @@ describe("/api/uploads/:recordingId/chunk route", () => { expect(mockWriteAppState).not.toHaveBeenCalled(); }); - it("forces a full restart when the retry flag switches off between chunks", async () => { + it("preserves a fenced retry when the retry flag switches off between chunks", async () => { mockGetResumableSession.mockResolvedValue({ providerId: "s3", sessionId: "sess-1", @@ -349,15 +349,95 @@ describe("/api/uploads/:recordingId/chunk route", () => { body: new Uint8Array([4, 5, 6]), }); await expect(handler({} as any)).resolves.toEqual({ - ok: false, - error: "Resumable upload retry is disabled.", - restartRequired: true, - recoveryEnabled: false, + ok: true, + finalized: false, + index: 1, + bytes: 3, }); - expect(mockSetResponseStatus).toHaveBeenLastCalledWith({}, 409); - expect(mockReadRawBody).toHaveBeenCalledOnce(); - expect(mockRelayChunk).toHaveBeenCalledOnce(); - expect(mockRenewUploadLease).toHaveBeenCalledTimes(3); + expect(mockReadRawBody).toHaveBeenCalledTimes(2); + expect(mockRelayChunk).toHaveBeenCalledTimes(2); + expect(mockRenewUploadLease).toHaveBeenCalledTimes(6); + }); + + it("heartbeats a fenced retry while a provider relay is still in flight", async () => { + vi.useFakeTimers(); + try { + mockGetResumableSession.mockResolvedValue({ + providerId: "s3", + sessionId: "sess-1", + meta: { objectKey: "clips/rec-1.webm" }, + bytesUploaded: 0, + lastCommittedIndex: -1, + }); + let finishRelay!: (value: { ok: boolean; status: number }) => void; + mockRelayChunk.mockImplementationOnce( + () => + new Promise((resolve) => { + finishRelay = resolve; + }), + ); + setRequest({ + query: { + index: "0", + mimeType: "video/webm", + attemptId: "retry-attempt", + }, + body: new Uint8Array([1]), + }); + + const pending = handler({} as any); + await vi.advanceTimersByTimeAsync(10_000); + expect(mockRenewUploadLease).toHaveBeenCalledTimes(3); + finishRelay({ ok: true, status: 308 }); + await expect(pending).resolves.toEqual( + expect.objectContaining({ ok: true, finalized: false }), + ); + const renewalsAfterRelay = mockRenewUploadLease.mock.calls.length; + await vi.advanceTimersByTimeAsync(30_000); + expect(mockRenewUploadLease).toHaveBeenCalledTimes(renewalsAfterRelay); + } finally { + vi.useRealTimers(); + } + }); + + it("fails loudly when a provider relay loses its fenced retry claim", async () => { + mockGetResumableSession.mockResolvedValue({ + providerId: "s3", + sessionId: "sess-1", + meta: { objectKey: "clips/rec-1.webm" }, + bytesUploaded: 0, + lastCommittedIndex: -1, + }); + mockRenewUploadLease + .mockResolvedValueOnce({ held: true }) + .mockResolvedValueOnce({ held: true }) + .mockResolvedValueOnce({ held: false, staleAttempt: true }); + let finishRelay!: (value: { ok: boolean; status: number }) => void; + mockRelayChunk.mockImplementationOnce( + () => + new Promise((resolve) => { + finishRelay = resolve; + }), + ); + vi.useFakeTimers(); + try { + setRequest({ + query: { + index: "0", + mimeType: "video/webm", + attemptId: "retry-attempt", + }, + body: new Uint8Array([1]), + }); + const pending = handler({} as any); + await vi.advanceTimersByTimeAsync(10_000); + finishRelay({ ok: true, status: 308 }); + await expect(pending).resolves.toEqual( + expect.objectContaining({ staleAttempt: true }), + ); + } finally { + vi.useRealTimers(); + } }); it("stores in-order chunks and advances upload progress state", async () => { diff --git a/templates/clips/server/routes/api/uploads/[recordingId]/chunk.post.ts b/templates/clips/server/routes/api/uploads/[recordingId]/chunk.post.ts index bedb82fe34..573e673f3d 100644 --- a/templates/clips/server/routes/api/uploads/[recordingId]/chunk.post.ts +++ b/templates/clips/server/routes/api/uploads/[recordingId]/chunk.post.ts @@ -17,7 +17,6 @@ import { readAppState, writeAppState, } from "@agent-native/core/application-state"; -import { isFeatureFlagEnabled } from "@agent-native/core/feature-flags"; import { runWithRequestContext } from "@agent-native/core/server"; import { track } from "@agent-native/core/tracking"; import { normalizeChunkUploadNumber } from "@shared/recording-core.js"; @@ -35,7 +34,6 @@ import { } from "h3"; import finalizeRecording from "../../../../../actions/finalize-recording.js"; -import { UPLOAD_RETRY_RESUME_FLAG } from "../../../../../shared/feature-flags.js"; import { getDb, schema } from "../../../../db/index.js"; import { debugLog } from "../../../../lib/debug.js"; import { @@ -55,7 +53,10 @@ import { import { abortResumableUploadSession } from "../../../../lib/resumable-upload-cleanup.js"; import { resolveResumableUploadProvider } from "../../../../lib/resumable-upload-provider.js"; import { isStreamingUploadDisabled } from "../../../../lib/streaming-upload-mode.js"; -import { renewUploadLease } from "../../../../lib/upload-lease.js"; +import { + renewUploadLease, + type UploadLeaseResult, +} from "../../../../lib/upload-lease.js"; import { allowsSqlRecordingChunkScratch, shouldRejectVideoUploadWithoutStorage, @@ -68,6 +69,42 @@ const RECORDING_TOO_LARGE_REASON = `Recording exceeds the ${Math.round(MAX_RECOR // are base64 encoded by the gateway and effectively cap out around 4.5 MB. // Keep our own cap lower so dev/local failures match production. const MAX_CHUNK_BYTES = 4 * 1024 * 1024; +const RETRY_OWNERSHIP_HEARTBEAT_MS = 10 * 1000; + +async function relayWithRetryOwnershipHeartbeat( + recordingId: string, + attemptId: string | null, + generationId: string | null, + relay: () => Promise, +): Promise<{ result: T; ownershipFailure: UploadLeaseResult | Error | null }> { + if (attemptId === null) + return { result: await relay(), ownershipFailure: null }; + let ownershipFailure: UploadLeaseResult | Error | null = null; + let pending: Promise | null = null; + const heartbeat = () => { + if (pending || ownershipFailure) return; + pending = renewUploadLease(recordingId, { attemptId, generationId }) + .then((lease) => { + if (!lease.held) ownershipFailure = lease; + }) + .catch((error) => { + ownershipFailure = + error instanceof Error ? error : new Error(String(error)); + }) + .finally(() => { + pending = null; + }); + }; + const timer = setInterval(heartbeat, RETRY_OWNERSHIP_HEARTBEAT_MS); + let result: T; + try { + result = await relay(); + } finally { + clearInterval(timer); + await pending; + } + return { result: result!, ownershipFailure }; +} const ALLOWED_RECORDING_MIME_TYPES = new Set([ "video/webm", @@ -239,23 +276,6 @@ export default defineEventHandler(async (event: H3Event) => { } debugLog("[chunk] resolved owner:", ownerEmail); - if ( - attemptId !== null && - !(await isFeatureFlagEnabled(UPLOAD_RETRY_RESUME_FLAG, { - userEmail: ownerEmail, - userKey: ownerEmail, - orgId, - })) - ) { - setResponseStatus(event, 409); - return { - ok: false, - error: "Resumable upload retry is disabled.", - restartRequired: true, - recoveryEnabled: false, - }; - } - return runWithRequestContext({ userEmail: ownerEmail, orgId }, async () => { const db = getDb(); @@ -1002,12 +1022,28 @@ async function handleResumableChunk( const putT0 = Date.now(); let putResult; try { - putResult = await uploadProvider.resumable.relayChunk( - { sessionId: session.sessionId, meta: session.meta }, - contentRange, - bytes, - { mimeType: mimeType.split(";")[0].trim() }, + const relayed = await relayWithRetryOwnershipHeartbeat( + recordingId, + attemptId, + uploadGenerationId, + () => + uploadProvider.resumable!.relayChunk( + { sessionId: session.sessionId, meta: session.meta }, + contentRange, + bytes, + { mimeType: mimeType.split(";")[0].trim() }, + ), ); + if (relayed.ownershipFailure) { + setResponseStatus(event, 409); + return { + ok: false, + error: + "Upload retry ownership was lost while the provider was responding.", + staleAttempt: true, + }; + } + putResult = relayed.result; } catch (error) { if (isFinal) { const cleanupFailed = await cleanupFailedFinalSession(); diff --git a/templates/clips/server/routes/api/uploads/[recordingId]/reset-chunks.post.test.ts b/templates/clips/server/routes/api/uploads/[recordingId]/reset-chunks.post.test.ts index 6b18398f0d..83fd7b422e 100644 --- a/templates/clips/server/routes/api/uploads/[recordingId]/reset-chunks.post.test.ts +++ b/templates/clips/server/routes/api/uploads/[recordingId]/reset-chunks.post.test.ts @@ -263,7 +263,7 @@ describe("/api/uploads/:recordingId/reset-chunks route", () => { expect(mockDeleteResumableSession).not.toHaveBeenCalled(); }); - it("clears a recovery claim when the flag is disabled mid-retry", async () => { + it("does not clear a recovery claim when the flag is disabled mid-retry", async () => { mockIsFeatureFlagEnabled.mockResolvedValue(false); mockExistingRecording.current.uploadAttemptId = "old-attempt"; mockReadBody.mockResolvedValue({ @@ -273,14 +273,32 @@ describe("/api/uploads/:recordingId/reset-chunks route", () => { }); await expect(handler({} as any)).resolves.toEqual( - expect.objectContaining({ ok: true, uploadGenerationId: null }), + expect.objectContaining({ staleAttempt: true }), ); - expect(mockUpdateSets).toContainEqual( + expect(mockUpdateSets).toHaveLength(0); + }); + + it("preserves a fenced retry through flag disable when the client echoes its claim", async () => { + mockIsFeatureFlagEnabled.mockResolvedValue(false); + mockExistingRecording.current.uploadAttemptId = "old-attempt"; + mockExistingRecording.current.uploadGenerationId = "generation-old"; + mockReadBody.mockResolvedValue({ + attemptId: "old-attempt", + uploadGenerationId: "generation-old", + }); + + await expect(handler({} as any)).resolves.toEqual( expect.objectContaining({ - uploadAttemptId: null, - uploadGenerationId: null, + ok: true, + uploadGenerationId: expect.any(String), }), ); + expect(mockUpdateSets).toContainEqual( + expect.objectContaining({ uploadGenerationId: expect.any(String) }), + ); + expect(mockUpdateSets.some((set) => set.uploadAttemptId === null)).toBe( + false, + ); }); it("recreates a resumable session for a browser backup retry", async () => { diff --git a/templates/clips/server/routes/api/uploads/[recordingId]/reset-chunks.post.ts b/templates/clips/server/routes/api/uploads/[recordingId]/reset-chunks.post.ts index a4ee5a736c..ae8c131809 100644 --- a/templates/clips/server/routes/api/uploads/[recordingId]/reset-chunks.post.ts +++ b/templates/clips/server/routes/api/uploads/[recordingId]/reset-chunks.post.ts @@ -238,7 +238,10 @@ export default defineEventHandler(async (event: H3Event) => { : null; const existingAttemptId = existing.uploadAttemptId ?? null; const existingGenerationId = existing.uploadGenerationId ?? null; - if (recoveryEnabled && existingAttemptId !== requestedAttemptId) { + if ( + existingAttemptId !== null && + existingAttemptId !== requestedAttemptId + ) { setResponseStatus(event, 409); return { error: "A newer upload retry is already active.", @@ -251,7 +254,10 @@ export default defineEventHandler(async (event: H3Event) => { body.uploadGenerationId.length <= 128 ? body.uploadGenerationId : null; - if (recoveryEnabled && existingGenerationId !== requestedGenerationId) { + if ( + existingGenerationId !== null && + existingGenerationId !== requestedGenerationId + ) { setResponseStatus(event, 409); return { error: "A newer upload generation is already active.", @@ -281,10 +287,12 @@ export default defineEventHandler(async (event: H3Event) => { // fence. Retry claims always opt in; legacy reset callers keep the null // generation wire contract until they are upgraded. const useGenerationFence = - recoveryEnabled && - (requestedAttemptId !== null || - requestedGenerationId !== null || - body?.useGenerationFence === true); + existingAttemptId !== null || + existingGenerationId !== null || + (recoveryEnabled && + (requestedAttemptId !== null || + requestedGenerationId !== null || + body?.useGenerationFence === true)); const nextGenerationId = useGenerationFence ? randomUUID() : null; const uploadStateKey = `recording-upload-${recordingId}`; const uploadStateSnapshot = await readAppState(uploadStateKey); @@ -314,7 +322,9 @@ export default defineEventHandler(async (event: H3Event) => { failureReason: null, uploadProgress: 0, uploadGenerationId: nextGenerationId, - ...(!recoveryEnabled ? { uploadAttemptId: null } : {}), + ...(!recoveryEnabled && existingAttemptId === null + ? { uploadAttemptId: null } + : {}), uploadLeaseExpiresAt: uploadLeaseExpiry(), updatedAt: now, }) diff --git a/templates/clips/server/routes/api/uploads/[recordingId]/resume.get.test.ts b/templates/clips/server/routes/api/uploads/[recordingId]/resume.get.test.ts index 7505613df7..28484e6117 100644 --- a/templates/clips/server/routes/api/uploads/[recordingId]/resume.get.test.ts +++ b/templates/clips/server/routes/api/uploads/[recordingId]/resume.get.test.ts @@ -52,6 +52,7 @@ vi.mock("drizzle-orm", () => ({ and: vi.fn(() => "and"), eq: vi.fn(() => "eq"), isNull: vi.fn(() => "is-null"), + lte: vi.fn(() => "lte"), })); vi.mock("h3", () => ({ @@ -115,21 +116,44 @@ describe("/api/uploads/:recordingId/resume route", () => { mockIsFeatureFlagEnabled.mockResolvedValue(true); }); - it("leaves upload state untouched when resumable retry is disabled", async () => { + it("leaves legacy upload state untouched when resumable retry is disabled", async () => { mockIsFeatureFlagEnabled.mockResolvedValue(false); await expect(handler({} as any)).resolves.toEqual({ recoveryEnabled: false, resumable: false, recordingId: "rec-1", - status: null, + status: "uploading", reason: "feature_disabled", }); - expect(mockDb.select).not.toHaveBeenCalled(); + expect(mockDb.select).toHaveBeenCalledOnce(); expect(mockDb.update).not.toHaveBeenCalled(); expect(mockWriteAppState).not.toHaveBeenCalled(); }); + it("returns an existing fence when the flag is disabled", async () => { + mockIsFeatureFlagEnabled.mockResolvedValue(false); + mockSelectRows.rows = [ + { + id: "rec-1", + status: "uploading", + uploadAttemptId: "client-attempt-0001", + uploadGenerationId: "generation-1", + }, + ]; + + await expect(handler({} as any)).resolves.toEqual({ + recoveryEnabled: false, + resumable: false, + recordingId: "rec-1", + status: "uploading", + reason: "feature_disabled", + attemptId: "client-attempt-0001", + uploadGenerationId: "generation-1", + }); + expect(mockDb.update).not.toHaveBeenCalled(); + }); + it("reports the provider's committed offset for a streaming upload", async () => { mockGetResumableSession.mockResolvedValue({ bytesUploaded: 4_194_304, @@ -223,6 +247,22 @@ describe("/api/uploads/:recordingId/resume route", () => { expect(mockDb.update).toHaveBeenCalledOnce(); }); + it("accepts an interruption with its detailed retryable reason", async () => { + mockSelectRows.rows = [ + { + id: "rec-1", + status: "failed", + failureReason: + "Upload was interrupted. The local recording is safe; retry from the Clips desktop app. Last error: network changed", + }, + ]; + + await expect(handler({} as any)).resolves.toEqual( + expect.objectContaining({ resumable: true, status: "uploading" }), + ); + expect(mockDb.update).toHaveBeenCalledOnce(); + }); + it("claims a restart token when the prior provider session is gone", async () => { mockSelectRows.rows = [ { @@ -254,6 +294,7 @@ describe("/api/uploads/:recordingId/resume route", () => { recordingId: "rec-1", status: "uploading", reason: "retry_already_active", + retryAfterMs: 250, }); expect(mockSetResponseStatus).toHaveBeenCalledWith({}, 409); expect(mockWriteAppState).not.toHaveBeenCalled(); @@ -321,12 +362,41 @@ describe("/api/uploads/:recordingId/resume route", () => { }); }); - it("does not let a different claim steal an active retry", async () => { + it("returns a bounded typed conflict for a live different retry claim", async () => { + vi.useFakeTimers(); + vi.setSystemTime(new Date("2026-08-21T12:00:00.000Z")); + try { + mockSelectRows.rows = [ + { + id: "rec-1", + status: "uploading", + uploadAttemptId: "active-attempt-0001", + updatedAt: "2026-08-21T11:59:59.000Z", + }, + ]; + + await expect(handler({} as any)).resolves.toEqual({ + resumable: false, + recoveryEnabled: true, + recordingId: "rec-1", + status: "uploading", + reason: "retry_already_active", + retryAfterMs: 29_000, + }); + expect(mockSetResponseStatus).toHaveBeenCalledWith({}, 409); + expect(mockDb.update).not.toHaveBeenCalled(); + } finally { + vi.useRealTimers(); + } + }); + + it("fails loudly when a different retry claim has unreadable liveness", async () => { mockSelectRows.rows = [ { id: "rec-1", status: "uploading", uploadAttemptId: "active-attempt-0001", + updatedAt: "not-a-timestamp", }, ]; @@ -335,12 +405,69 @@ describe("/api/uploads/:recordingId/resume route", () => { recoveryEnabled: true, recordingId: "rec-1", status: "uploading", - reason: "retry_already_active", + reason: "retry_claim_liveness_unavailable", }); expect(mockSetResponseStatus).toHaveBeenCalledWith({}, 409); expect(mockDb.update).not.toHaveBeenCalled(); }); + it("atomically reclaims an expired different retry claim without changing its generation or provider offset", async () => { + mockSelectRows.rows = [ + { + id: "rec-1", + status: "uploading", + uploadAttemptId: "stale-attempt-0001", + uploadGenerationId: "generation-1", + updatedAt: "2000-01-01T00:00:00.000Z", + }, + ]; + mockGetResumableSession.mockResolvedValue({ + bytesUploaded: 7_864_320, + lastCommittedIndex: 1, + }); + + await expect(handler({} as any)).resolves.toEqual( + expect.objectContaining({ + resumable: true, + attemptId: "client-attempt-0001", + uploadGenerationId: "generation-1", + bytesReceived: 7_864_320, + nextChunkIndex: 2, + }), + ); + expect(mockCompareAndSetAppState).toHaveBeenCalledWith( + "recording-upload-rec-1", + expect.anything(), + expect.objectContaining({ + uploadAttemptId: "client-attempt-0001", + uploadGenerationId: "generation-1", + bytesReceived: 7_864_320, + }), + ); + }); + + it("loses a stale-claim takeover race without publishing resume state", async () => { + mockSelectRows.rows = [ + { + id: "rec-1", + status: "uploading", + uploadAttemptId: "stale-attempt-0001", + updatedAt: "2000-01-01T00:00:00.000Z", + }, + ]; + mockUpdateRows.rows = []; + + await expect(handler({} as any)).resolves.toEqual({ + resumable: false, + recoveryEnabled: true, + recordingId: "rec-1", + status: "uploading", + reason: "retry_already_active", + retryAfterMs: 250, + }); + expect(mockWriteAppState).not.toHaveBeenCalled(); + }); + it("lets the same claim re-read its offset after a lost response", async () => { mockSelectRows.rows = [ { diff --git a/templates/clips/server/routes/api/uploads/[recordingId]/resume.get.ts b/templates/clips/server/routes/api/uploads/[recordingId]/resume.get.ts index de3244b895..fa77034085 100644 --- a/templates/clips/server/routes/api/uploads/[recordingId]/resume.get.ts +++ b/templates/clips/server/routes/api/uploads/[recordingId]/resume.get.ts @@ -18,7 +18,7 @@ import { } from "@agent-native/core/application-state"; import { isFeatureFlagEnabled } from "@agent-native/core/feature-flags"; import { runWithRequestContext } from "@agent-native/core/server"; -import { and, eq, isNull } from "drizzle-orm"; +import { and, eq, isNull, lte } from "drizzle-orm"; import { createError, defineEventHandler, @@ -41,12 +41,22 @@ import { ownerEmailMatches, } from "../../../../lib/recordings.js"; import { getResumableSession } from "../../../../lib/resumable-session.js"; -import { - isRetryableUploadInterruption, - RETRYABLE_UPLOAD_INTERRUPTION_REASON, -} from "../../../../lib/upload-interruption.js"; +import { isRetryableUploadInterruption } from "../../../../lib/upload-interruption.js"; import { uploadLeaseExpiry } from "../../../../lib/upload-lease.js"; +const RETRY_CLAIM_LIVENESS_MS = 30 * 1000; +const RETRY_CLAIM_RETRY_AFTER_MIN_MS = 250; + +function retryClaimRetryAfterMs(updatedAtMs: number, nowMs: number): number { + return Math.max( + RETRY_CLAIM_RETRY_AFTER_MIN_MS, + Math.min( + RETRY_CLAIM_LIVENESS_MS, + updatedAtMs + RETRY_CLAIM_LIVENESS_MS - nowMs, + ), + ); +} + export default defineEventHandler(async (event: H3Event) => { setResponseHeader(event, "Cache-Control", "private, max-age=0, no-store"); const recordingId = getRouterParam(event, "recordingId"); @@ -85,13 +95,51 @@ export default defineEventHandler(async (event: H3Event) => { orgId, }); if (!recoveryEnabled) { - return { - recoveryEnabled: false, - resumable: false, - recordingId, - status: null, - reason: "feature_disabled", - }; + return runWithRequestContext({ userEmail: ownerEmail, orgId }, async () => { + const [recording] = await getDb() + .select({ + status: schema.recordings.status, + uploadAttemptId: schema.recordings.uploadAttemptId, + uploadGenerationId: schema.recordings.uploadGenerationId, + }) + .from(schema.recordings) + .where( + and( + eq(schema.recordings.id, recordingId), + ownerEmailMatches(schema.recordings.ownerEmail, ownerEmail), + ), + ); + if (!recording) { + setResponseStatus(event, 404); + return { error: "Recording not found" }; + } + if ( + recording.uploadAttemptId && + recording.uploadAttemptId !== requestedAttemptId + ) { + setResponseStatus(event, 409); + return { + recoveryEnabled: false, + resumable: false, + recordingId, + status: recording.status, + reason: "retry_already_active", + }; + } + return { + recoveryEnabled: false, + resumable: false, + recordingId, + status: recording.status ?? null, + reason: "feature_disabled", + ...(recording.uploadAttemptId + ? { attemptId: recording.uploadAttemptId } + : {}), + ...(recording.uploadGenerationId + ? { uploadGenerationId: recording.uploadGenerationId } + : {}), + }; + }); } return runWithRequestContext({ userEmail: ownerEmail, orgId }, async () => { @@ -104,6 +152,7 @@ export default defineEventHandler(async (event: H3Event) => { uploadProgress: schema.recordings.uploadProgress, uploadAttemptId: schema.recordings.uploadAttemptId, uploadGenerationId: schema.recordings.uploadGenerationId, + updatedAt: schema.recordings.updatedAt, }) .from(schema.recordings) .where( @@ -129,11 +178,29 @@ export default defineEventHandler(async (event: H3Event) => { recording.status === "failed" && isRetryableUploadInterruption(recording.failureReason); const existingAttemptId = recording.uploadAttemptId ?? null; - if ( + const nowMs = Date.now(); + const now = new Date(nowMs).toISOString(); + const staleThreshold = new Date( + nowMs - RETRY_CLAIM_LIVENESS_MS, + ).toISOString(); + const claimUpdatedAtMs = Date.parse(recording.updatedAt); + const differentRetryClaim = recording.status === "uploading" && existingAttemptId !== null && - existingAttemptId !== requestedAttemptId - ) { + existingAttemptId !== requestedAttemptId; + if (differentRetryClaim && !Number.isFinite(claimUpdatedAtMs)) { + setResponseStatus(event, 409); + return { + resumable: false, + recoveryEnabled: true, + recordingId, + status: "uploading", + reason: "retry_claim_liveness_unavailable", + }; + } + const differentLiveRetryClaim = + differentRetryClaim && claimUpdatedAtMs > nowMs - RETRY_CLAIM_LIVENESS_MS; + if (differentLiveRetryClaim) { setResponseStatus(event, 409); return { resumable: false, @@ -141,6 +208,7 @@ export default defineEventHandler(async (event: H3Event) => { recordingId, status: "uploading", reason: "retry_already_active", + retryAfterMs: retryClaimRetryAfterMs(claimUpdatedAtMs, nowMs), }; } if (recording.status !== "uploading" && !retryableFailure) { @@ -158,7 +226,7 @@ export default defineEventHandler(async (event: H3Event) => { const uploadStateRaw = await readAppState(uploadStateKey); const uploadState = uploadStateRaw ?? {}; const attemptId = requestedAttemptId; - const now = new Date().toISOString(); + const takingOverStaleRetryClaim = differentRetryClaim; const claimed = await getDb() .update(schema.recordings) .set({ @@ -166,7 +234,7 @@ export default defineEventHandler(async (event: H3Event) => { failureReason: null, uploadAttemptId: attemptId, ...(generationId ? { uploadGenerationId: generationId } : {}), - uploadLeaseExpiresAt: uploadLeaseExpiry(), + uploadLeaseExpiresAt: uploadLeaseExpiry(nowMs), updatedAt: now, }) .where( @@ -177,10 +245,7 @@ export default defineEventHandler(async (event: H3Event) => { ? eq(schema.recordings.status, "failed") : eq(schema.recordings.status, "uploading"), retryableFailure - ? eq( - schema.recordings.failureReason, - RETRYABLE_UPLOAD_INTERRUPTION_REASON, - ) + ? eq(schema.recordings.failureReason, recording.failureReason!) : undefined, existingAttemptId === null ? isNull(schema.recordings.uploadAttemptId) @@ -188,6 +253,9 @@ export default defineEventHandler(async (event: H3Event) => { existingGenerationId === null ? isNull(schema.recordings.uploadGenerationId) : eq(schema.recordings.uploadGenerationId, existingGenerationId), + takingOverStaleRetryClaim + ? lte(schema.recordings.updatedAt, staleThreshold) + : undefined, ), ) .returning({ id: schema.recordings.id }); @@ -200,6 +268,7 @@ export default defineEventHandler(async (event: H3Event) => { recordingId, status: "uploading", reason: "retry_already_active", + retryAfterMs: 250, }; } From c137952bbca3ed223a49b3d5bd2bb5fd6966d156 Mon Sep 17 00:00:00 2001 From: Alice Alexandra Moore <86723305+3mdistal@users.noreply.github.com> Date: Fri, 21 Aug 2026 09:07:15 -0400 Subject: [PATCH 02/11] fix(clips): fail loudly on unreadable retry response --- templates/clips/desktop/src/lib/recorder.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/templates/clips/desktop/src/lib/recorder.ts b/templates/clips/desktop/src/lib/recorder.ts index 7341f7ae6e..9cc5825cb6 100644 --- a/templates/clips/desktop/src/lib/recorder.ts +++ b/templates/clips/desktop/src/lib/recorder.ts @@ -1163,7 +1163,12 @@ async function getBrowserRecordingUploadResume( credentials: "include", signal, }); - const body = await res.text().catch(() => ""); + let body: string; + try { + body = await res.text(); + } catch { + throw new Error("Upload resume check response could not be read"); + } let parsed: UploadResumeResponse; try { parsed = JSON.parse(body) as UploadResumeResponse; From af92e84cbaebd7dfc3158476e0db79327c5b4e43 Mon Sep 17 00:00:00 2001 From: Alice Alexandra Moore <86723305+3mdistal@users.noreply.github.com> Date: Fri, 21 Aug 2026 09:55:02 -0400 Subject: [PATCH 03/11] fix(clips): address retry fencing review --- .../desktop/src-tauri/src/native_screen.rs | 24 ++++++++++++++- templates/clips/desktop/src/lib/recorder.ts | 15 ++++++++-- .../desktop/src/lib/upload-recovery.test.ts | 13 ++++++++- .../clips/desktop/src/lib/upload-recovery.ts | 2 +- .../api/uploads/[recordingId]/chunk.post.ts | 24 ++++++++++++--- .../[recordingId]/reset-chunks.post.ts | 8 ++--- .../uploads/[recordingId]/resume.get.test.ts | 9 +++--- .../api/uploads/[recordingId]/resume.get.ts | 29 ++++++++++++------- 8 files changed, 95 insertions(+), 29 deletions(-) diff --git a/templates/clips/desktop/src-tauri/src/native_screen.rs b/templates/clips/desktop/src-tauri/src/native_screen.rs index 14dcd53e83..0b44f8d640 100644 --- a/templates/clips/desktop/src-tauri/src/native_screen.rs +++ b/templates/clips/desktop/src-tauri/src/native_screen.rs @@ -5862,7 +5862,9 @@ fn preserve_native_retry_fence_during_rollback( upload_generation_id: planned_generation_id, } = &mut plan { - *attempt_id = Some(claimed_attempt_id.to_string()); + *attempt_id = upload_generation_id + .as_ref() + .map(|_| claimed_attempt_id.to_string()); *planned_generation_id = upload_generation_id; } } @@ -6172,6 +6174,26 @@ mod native_retry_upload_plan_tests { )); } + #[test] + fn keeps_a_legacy_restart_unfenced_when_resumable_retry_is_disabled() { + let plan = preserve_native_retry_fence_during_rollback( + NativeRetryUploadPlan::Restart { + attempt_id: None, + upload_generation_id: None, + }, + false, + "attempt-1", + None, + ); + assert!(matches!( + plan, + NativeRetryUploadPlan::Restart { + attempt_id: None, + upload_generation_id: None, + } + )); + } + #[test] fn reconciles_terminal_resume_without_an_attempt_echo() { let terminal = plan_native_retry_upload( diff --git a/templates/clips/desktop/src/lib/recorder.ts b/templates/clips/desktop/src/lib/recorder.ts index 9cc5825cb6..ca723f86a9 100644 --- a/templates/clips/desktop/src/lib/recorder.ts +++ b/templates/clips/desktop/src/lib/recorder.ts @@ -1062,6 +1062,7 @@ async function postBackupChunk( url: string, blob: Blob, authToken?: string, + signal?: AbortSignal, ): Promise { const res = await fetch(url, { method: "POST", @@ -1071,6 +1072,7 @@ async function postBackupChunk( ), credentials: "include", body: blob, + signal, }); const body = await res.text().catch(() => ""); if (!res.ok) { @@ -1104,6 +1106,7 @@ async function resetBrowserRecordingBackupUpload( authToken?: string, attemptId?: string, uploadGenerationId?: string, + signal?: AbortSignal, ): Promise<{ uploadMode: UploadMode; uploadGenerationId?: string }> { const res = await fetch( `${meta.serverUrl.replace(/\/+$/, "")}/api/uploads/${meta.recordingId}/reset-chunks`, @@ -1121,6 +1124,7 @@ async function resetBrowserRecordingBackupUpload( ...(attemptId ? { attemptId } : {}), ...(uploadGenerationId ? { uploadGenerationId } : {}), }), + signal, }, ); if (!res.ok) { @@ -1214,6 +1218,7 @@ async function replayBrowserBackupToResumableSession( bytesReceived: 0, nextChunkIndex: 0, }, + signal?: AbortSignal, ): Promise { // The backup is stored in raw MediaRecorder blobs, which have arbitrary // boundaries. A resumable provider needs every non-final request aligned, @@ -1246,6 +1251,7 @@ async function replayBrowserBackupToResumableSession( }), body, authToken, + signal, ); } @@ -1272,6 +1278,7 @@ async function replayBrowserBackupToResumableSession( }), finalBody, authToken, + signal, ); } @@ -1375,6 +1382,7 @@ export async function retryBrowserRecordingBackup(input: { input.authToken, activeAttemptId, activeUploadGenerationId, + input.signal, ); uploadMode = reset.uploadMode; activeUploadGenerationId = reset.uploadGenerationId; @@ -1390,6 +1398,7 @@ export async function retryBrowserRecordingBackup(input: { activeAttemptId, activeUploadGenerationId, resumeFrom, + input.signal, ); } catch (err) { if (err instanceof UploadRestartRequiredError) { @@ -1397,9 +1406,6 @@ export async function retryBrowserRecordingBackup(input: { activeAttemptId, err.recoveryEnabled, ); - if (err.recoveryEnabled === false) { - activeUploadGenerationId = undefined; - } input.onRecoveryDecision?.({ action: "restart", progress: 0 }); console.info("[clips-recorder] restarting expired upload session", { recordingId: meta.recordingId, @@ -1410,6 +1416,7 @@ export async function retryBrowserRecordingBackup(input: { input.authToken, activeAttemptId, activeUploadGenerationId, + input.signal, ); uploadMode = reset.uploadMode; activeUploadGenerationId = reset.uploadGenerationId; @@ -1420,6 +1427,8 @@ export async function retryBrowserRecordingBackup(input: { input.authToken, activeAttemptId, activeUploadGenerationId, + undefined, + input.signal, ); } } else if ( diff --git a/templates/clips/desktop/src/lib/upload-recovery.test.ts b/templates/clips/desktop/src/lib/upload-recovery.test.ts index 3ba0925ad7..d7bdba6fc0 100644 --- a/templates/clips/desktop/src/lib/upload-recovery.test.ts +++ b/templates/clips/desktop/src/lib/upload-recovery.test.ts @@ -93,7 +93,7 @@ describe("retryAttemptIdAfterResumeResponse", () => { ).toBeUndefined(); }); - it("preserves a local claim while the flag is rolled back", () => { + it("preserves a server-acknowledged claim while the flag is rolled back", () => { expect( retryAttemptIdAfterResumeResponse("attempt-1", { resumable: false, @@ -105,6 +105,17 @@ describe("retryAttemptIdAfterResumeResponse", () => { }), ).toBe("attempt-1"); }); + + it("drops an unacknowledged legacy claim while the flag is rolled back", () => { + expect( + retryAttemptIdAfterResumeResponse("attempt-1", { + resumable: false, + recoveryEnabled: false, + status: "uploading", + reason: "feature_disabled", + }), + ).toBeUndefined(); + }); }); describe("planStreamingRecovery", () => { diff --git a/templates/clips/desktop/src/lib/upload-recovery.ts b/templates/clips/desktop/src/lib/upload-recovery.ts index 65ee3c083e..a10f476bb0 100644 --- a/templates/clips/desktop/src/lib/upload-recovery.ts +++ b/templates/clips/desktop/src/lib/upload-recovery.ts @@ -64,7 +64,7 @@ export function retryAttemptIdAfterResumeResponse( attemptId: string | undefined, response: UploadResumeResponse, ): string | undefined { - if (response.recoveryEnabled === false) return attemptId; + if (response.recoveryEnabled === false) return response.attemptId; return response.resumable && response.attemptId === attemptId ? attemptId : undefined; diff --git a/templates/clips/server/routes/api/uploads/[recordingId]/chunk.post.ts b/templates/clips/server/routes/api/uploads/[recordingId]/chunk.post.ts index 573e673f3d..336aa0f6ae 100644 --- a/templates/clips/server/routes/api/uploads/[recordingId]/chunk.post.ts +++ b/templates/clips/server/routes/api/uploads/[recordingId]/chunk.post.ts @@ -924,11 +924,27 @@ async function handleResumableChunk( // to close the session before handing off to finalize-recording. let closeRes; try { - closeRes = await uploadProvider.resumable.relayChunk( - { sessionId: session.sessionId, meta: session.meta }, - `bytes */${session.bytesUploaded}`, - new Uint8Array(0), + const relayed = await relayWithRetryOwnershipHeartbeat( + recordingId, + attemptId, + uploadGenerationId, + () => + uploadProvider.resumable!.relayChunk( + { sessionId: session.sessionId, meta: session.meta }, + `bytes */${session.bytesUploaded}`, + new Uint8Array(0), + ), ); + if (relayed.ownershipFailure) { + setResponseStatus(event, 409); + return { + ok: false, + error: + "Upload retry ownership was lost while the provider was responding.", + staleAttempt: true, + }; + } + closeRes = relayed.result; } catch (error) { const cleanupFailed = await cleanupFailedFinalSession(); const detail = error instanceof Error ? error.message : String(error); diff --git a/templates/clips/server/routes/api/uploads/[recordingId]/reset-chunks.post.ts b/templates/clips/server/routes/api/uploads/[recordingId]/reset-chunks.post.ts index ae8c131809..4cf5834e8a 100644 --- a/templates/clips/server/routes/api/uploads/[recordingId]/reset-chunks.post.ts +++ b/templates/clips/server/routes/api/uploads/[recordingId]/reset-chunks.post.ts @@ -513,8 +513,9 @@ export default defineEventHandler(async (event: H3Event) => { } } + const preservedAttemptId = existingAttemptId; const resetLease = await renewUploadLease(recordingId, { - attemptId: recoveryEnabled ? existingAttemptId : null, + attemptId: preservedAttemptId, generationId: nextGenerationId, }); if (!resetLease.held) { @@ -538,7 +539,7 @@ export default defineEventHandler(async (event: H3Event) => { progress: 0, chunksReceived: 0, bytesReceived: 0, - uploadAttemptId: recoveryEnabled ? existingAttemptId : null, + uploadAttemptId: preservedAttemptId, uploadGenerationId: nextGenerationId, maxBytes: MAX_RECORDING_UPLOAD_BYTES, updatedAt: now, @@ -560,8 +561,7 @@ export default defineEventHandler(async (event: H3Event) => { ); if ( current?.status !== "uploading" || - (current.uploadAttemptId ?? null) !== - (recoveryEnabled ? existingAttemptId : null) || + (current.uploadAttemptId ?? null) !== preservedAttemptId || (current.uploadGenerationId ?? null) !== nextGenerationId ) { setResponseStatus(event, 409); diff --git a/templates/clips/server/routes/api/uploads/[recordingId]/resume.get.test.ts b/templates/clips/server/routes/api/uploads/[recordingId]/resume.get.test.ts index 28484e6117..14c0177c62 100644 --- a/templates/clips/server/routes/api/uploads/[recordingId]/resume.get.test.ts +++ b/templates/clips/server/routes/api/uploads/[recordingId]/resume.get.test.ts @@ -94,6 +94,7 @@ vi.mock("../../../../lib/resumable-session.js", () => ({ })); vi.mock("../../../../lib/upload-lease.js", () => ({ + UPLOAD_LEASE_MS: 60 * 60 * 1000, renewUploadLease: (...args: unknown[]) => mockRenewUploadLease(...args), uploadLeaseExpiry: () => "2099-01-01T00:00:00.000Z", })); @@ -371,7 +372,7 @@ describe("/api/uploads/:recordingId/resume route", () => { id: "rec-1", status: "uploading", uploadAttemptId: "active-attempt-0001", - updatedAt: "2026-08-21T11:59:59.000Z", + uploadLeaseExpiresAt: "2026-08-21T12:59:59.000Z", }, ]; @@ -396,7 +397,7 @@ describe("/api/uploads/:recordingId/resume route", () => { id: "rec-1", status: "uploading", uploadAttemptId: "active-attempt-0001", - updatedAt: "not-a-timestamp", + uploadLeaseExpiresAt: "not-a-timestamp", }, ]; @@ -418,7 +419,7 @@ describe("/api/uploads/:recordingId/resume route", () => { status: "uploading", uploadAttemptId: "stale-attempt-0001", uploadGenerationId: "generation-1", - updatedAt: "2000-01-01T00:00:00.000Z", + uploadLeaseExpiresAt: "2000-01-01T00:00:00.000Z", }, ]; mockGetResumableSession.mockResolvedValue({ @@ -452,7 +453,7 @@ describe("/api/uploads/:recordingId/resume route", () => { id: "rec-1", status: "uploading", uploadAttemptId: "stale-attempt-0001", - updatedAt: "2000-01-01T00:00:00.000Z", + uploadLeaseExpiresAt: "2000-01-01T00:00:00.000Z", }, ]; mockUpdateRows.rows = []; diff --git a/templates/clips/server/routes/api/uploads/[recordingId]/resume.get.ts b/templates/clips/server/routes/api/uploads/[recordingId]/resume.get.ts index fa77034085..aec59ba984 100644 --- a/templates/clips/server/routes/api/uploads/[recordingId]/resume.get.ts +++ b/templates/clips/server/routes/api/uploads/[recordingId]/resume.get.ts @@ -42,17 +42,23 @@ import { } from "../../../../lib/recordings.js"; import { getResumableSession } from "../../../../lib/resumable-session.js"; import { isRetryableUploadInterruption } from "../../../../lib/upload-interruption.js"; -import { uploadLeaseExpiry } from "../../../../lib/upload-lease.js"; +import { + UPLOAD_LEASE_MS, + uploadLeaseExpiry, +} from "../../../../lib/upload-lease.js"; const RETRY_CLAIM_LIVENESS_MS = 30 * 1000; const RETRY_CLAIM_RETRY_AFTER_MIN_MS = 250; -function retryClaimRetryAfterMs(updatedAtMs: number, nowMs: number): number { +function retryClaimRetryAfterMs( + lastHeartbeatMs: number, + nowMs: number, +): number { return Math.max( RETRY_CLAIM_RETRY_AFTER_MIN_MS, Math.min( RETRY_CLAIM_LIVENESS_MS, - updatedAtMs + RETRY_CLAIM_LIVENESS_MS - nowMs, + lastHeartbeatMs + RETRY_CLAIM_LIVENESS_MS - nowMs, ), ); } @@ -152,7 +158,7 @@ export default defineEventHandler(async (event: H3Event) => { uploadProgress: schema.recordings.uploadProgress, uploadAttemptId: schema.recordings.uploadAttemptId, uploadGenerationId: schema.recordings.uploadGenerationId, - updatedAt: schema.recordings.updatedAt, + uploadLeaseExpiresAt: schema.recordings.uploadLeaseExpiresAt, }) .from(schema.recordings) .where( @@ -180,15 +186,16 @@ export default defineEventHandler(async (event: H3Event) => { const existingAttemptId = recording.uploadAttemptId ?? null; const nowMs = Date.now(); const now = new Date(nowMs).toISOString(); - const staleThreshold = new Date( - nowMs - RETRY_CLAIM_LIVENESS_MS, + const staleLeaseThreshold = new Date( + nowMs + UPLOAD_LEASE_MS - RETRY_CLAIM_LIVENESS_MS, ).toISOString(); - const claimUpdatedAtMs = Date.parse(recording.updatedAt); + const claimLeaseExpiryMs = Date.parse(recording.uploadLeaseExpiresAt ?? ""); + const claimHeartbeatMs = claimLeaseExpiryMs - UPLOAD_LEASE_MS; const differentRetryClaim = recording.status === "uploading" && existingAttemptId !== null && existingAttemptId !== requestedAttemptId; - if (differentRetryClaim && !Number.isFinite(claimUpdatedAtMs)) { + if (differentRetryClaim && !Number.isFinite(claimHeartbeatMs)) { setResponseStatus(event, 409); return { resumable: false, @@ -199,7 +206,7 @@ export default defineEventHandler(async (event: H3Event) => { }; } const differentLiveRetryClaim = - differentRetryClaim && claimUpdatedAtMs > nowMs - RETRY_CLAIM_LIVENESS_MS; + differentRetryClaim && claimHeartbeatMs > nowMs - RETRY_CLAIM_LIVENESS_MS; if (differentLiveRetryClaim) { setResponseStatus(event, 409); return { @@ -208,7 +215,7 @@ export default defineEventHandler(async (event: H3Event) => { recordingId, status: "uploading", reason: "retry_already_active", - retryAfterMs: retryClaimRetryAfterMs(claimUpdatedAtMs, nowMs), + retryAfterMs: retryClaimRetryAfterMs(claimHeartbeatMs, nowMs), }; } if (recording.status !== "uploading" && !retryableFailure) { @@ -254,7 +261,7 @@ export default defineEventHandler(async (event: H3Event) => { ? isNull(schema.recordings.uploadGenerationId) : eq(schema.recordings.uploadGenerationId, existingGenerationId), takingOverStaleRetryClaim - ? lte(schema.recordings.updatedAt, staleThreshold) + ? lte(schema.recordings.uploadLeaseExpiresAt, staleLeaseThreshold) : undefined, ), ) From 8f60bd2730690eaada28d7cf3f72d39451fe7124 Mon Sep 17 00:00:00 2001 From: Alice Alexandra Moore <86723305+3mdistal@users.noreply.github.com> Date: Fri, 21 Aug 2026 10:08:37 -0400 Subject: [PATCH 04/11] fix(clips): finish retry cancellation paths --- .../desktop/src-tauri/src/native_screen.rs | 90 ++++++++++++------- templates/clips/desktop/src/lib/recorder.ts | 2 + .../api/uploads/[recordingId]/chunk.post.ts | 26 ++++++ 3 files changed, 84 insertions(+), 34 deletions(-) diff --git a/templates/clips/desktop/src-tauri/src/native_screen.rs b/templates/clips/desktop/src-tauri/src/native_screen.rs index 0b44f8d640..a5c0abccbf 100644 --- a/templates/clips/desktop/src-tauri/src/native_screen.rs +++ b/templates/clips/desktop/src-tauri/src/native_screen.rs @@ -3772,17 +3772,20 @@ pub async fn native_fullscreen_recording_retry_upload( None, Some(0.0), ); - let reset = match reset_upload_chunks( - &saved.server_url, - &saved.recording_id, - &prepared.mime_type, - attempt_id.as_deref(), - upload_generation_id.as_deref(), - &auth_token, - &cookie, - ) - .await - { + let reset = match tokio::select! { + reset = reset_upload_chunks( + &saved.server_url, + &saved.recording_id, + &prepared.mime_type, + attempt_id.as_deref(), + upload_generation_id.as_deref(), + &auth_token, + &cookie, + ) => reset, + _ = wait_for_native_upload_retry_cancel(&saved.recording_id) => { + Err(NATIVE_UPLOAD_RETRY_CANCELLED.to_string()) + } + } { Ok(reset) => reset, Err(err) => { interrupt_native_retry_upload( @@ -3833,17 +3836,20 @@ pub async fn native_fullscreen_recording_retry_upload( eprintln!( "[clips-tray] native retry replaying from byte zero after the provider requested a restart" ); - match reset_upload_chunks( - &saved.server_url, - &saved.recording_id, - &prepared.mime_type, - replay_attempt_id.as_deref(), - replay_upload_generation_id.as_deref(), - &auth_token, - &cookie, - ) - .await - { + match tokio::select! { + reset = reset_upload_chunks( + &saved.server_url, + &saved.recording_id, + &prepared.mime_type, + replay_attempt_id.as_deref(), + replay_upload_generation_id.as_deref(), + &auth_token, + &cookie, + ) => reset, + _ = wait_for_native_upload_retry_cancel(&saved.recording_id) => { + Err(NATIVE_UPLOAD_RETRY_CANCELLED.to_string()) + } + } { Ok(reset) => { interruption_upload_generation_id = reset.upload_generation_id.clone(); upload_prepared_recording_file( @@ -5600,7 +5606,8 @@ async fn upload_prepared_recording_file( let mut buffer = vec![0_u8; UPLOAD_CHUNK_BYTES]; file.read_exact(&mut buffer) .map_err(|e| format!("native recording read failed: {e}"))?; - send_upload_post_with_attempt( + tokio::select! { + result = send_upload_post_with_attempt( &client, &server_url, &recording_id, @@ -5621,8 +5628,11 @@ async fn upload_prepared_recording_file( upload_attempt_id, upload_generation_id, buffer, - ) - .await?; + ) => result, + _ = wait_for_native_upload_retry_cancel(&recording_id) => { + Err(NATIVE_UPLOAD_RETRY_CANCELLED.to_string()) + } + }?; emit_native_upload_progress( app, "uploading", @@ -5645,7 +5655,8 @@ async fn upload_prepared_recording_file( None, Some(streaming_full_chunks as f32 / total_posts as f32), ); - verification_pending = send_upload_post_with_attempt( + verification_pending = tokio::select! { + result = send_upload_post_with_attempt( &client, &server_url, &recording_id, @@ -5666,8 +5677,11 @@ async fn upload_prepared_recording_file( upload_attempt_id, upload_generation_id, final_body, - ) - .await?; + ) => result, + _ = wait_for_native_upload_retry_cancel(&recording_id) => { + Err(NATIVE_UPLOAD_RETRY_CANCELLED.to_string()) + } + }?; } else { for index in 0..total_chunks { let mut buffer = vec![0_u8; UPLOAD_CHUNK_BYTES]; @@ -5678,7 +5692,8 @@ async fn upload_prepared_recording_file( return Err("Native recording ended before all chunks were read.".into()); } buffer.truncate(read); - send_upload_post_with_attempt( + tokio::select! { + result = send_upload_post_with_attempt( &client, &server_url, &recording_id, @@ -5699,8 +5714,11 @@ async fn upload_prepared_recording_file( upload_attempt_id, upload_generation_id, buffer, - ) - .await?; + ) => result, + _ = wait_for_native_upload_retry_cancel(&recording_id) => { + Err(NATIVE_UPLOAD_RETRY_CANCELLED.to_string()) + } + }?; emit_native_upload_progress( app, "uploading", @@ -5717,7 +5735,8 @@ async fn upload_prepared_recording_file( None, Some(total_chunks as f32 / total_posts as f32), ); - verification_pending = send_upload_post_with_attempt( + verification_pending = tokio::select! { + result = send_upload_post_with_attempt( &client, &server_url, &recording_id, @@ -5738,8 +5757,11 @@ async fn upload_prepared_recording_file( upload_attempt_id, upload_generation_id, Vec::new(), - ) - .await?; + ) => result, + _ = wait_for_native_upload_retry_cancel(&recording_id) => { + Err(NATIVE_UPLOAD_RETRY_CANCELLED.to_string()) + } + }?; } emit_native_upload_progress(app, "opening", "Uploading clip", None, Some(1.0)); diff --git a/templates/clips/desktop/src/lib/recorder.ts b/templates/clips/desktop/src/lib/recorder.ts index ca723f86a9..8db1162dfe 100644 --- a/templates/clips/desktop/src/lib/recorder.ts +++ b/templates/clips/desktop/src/lib/recorder.ts @@ -1480,6 +1480,7 @@ export async function retryBrowserRecordingBackup(input: { }), chunk.blob, input.authToken, + input.signal, ); } @@ -1507,6 +1508,7 @@ export async function retryBrowserRecordingBackup(input: { finalChunkUrl, new Blob([], { type: meta.mimeType }), input.authToken, + input.signal, ); const receiptStatus = verifyFinalizeReceipt(receipt, meta); if (receiptStatus === "processing") { diff --git a/templates/clips/server/routes/api/uploads/[recordingId]/chunk.post.ts b/templates/clips/server/routes/api/uploads/[recordingId]/chunk.post.ts index 336aa0f6ae..568b3215f2 100644 --- a/templates/clips/server/routes/api/uploads/[recordingId]/chunk.post.ts +++ b/templates/clips/server/routes/api/uploads/[recordingId]/chunk.post.ts @@ -946,6 +946,19 @@ async function handleResumableChunk( } closeRes = relayed.result; } catch (error) { + const failedCloseLease = await renewUploadLease(recordingId, { + attemptId, + generationId: uploadGenerationId, + }); + if (!failedCloseLease.held) { + setResponseStatus(event, 409); + return { + ok: false, + error: + "Upload retry ownership was lost while the provider was responding.", + staleAttempt: true, + }; + } const cleanupFailed = await cleanupFailedFinalSession(); const detail = error instanceof Error ? error.message : String(error); console.error( @@ -1062,6 +1075,19 @@ async function handleResumableChunk( putResult = relayed.result; } catch (error) { if (isFinal) { + const failedFinalLease = await renewUploadLease(recordingId, { + attemptId, + generationId: uploadGenerationId, + }); + if (!failedFinalLease.held) { + setResponseStatus(event, 409); + return { + ok: false, + error: + "Upload retry ownership was lost while the provider was responding.", + staleAttempt: true, + }; + } const cleanupFailed = await cleanupFailedFinalSession(); const detail = error instanceof Error ? error.message : String(error); console.error( From 2e17343ffe5e374d95a1b4eb2b2738a5f7b0ff59 Mon Sep 17 00:00:00 2001 From: Alice Alexandra Moore <86723305+3mdistal@users.noreply.github.com> Date: Fri, 21 Aug 2026 10:51:28 -0400 Subject: [PATCH 05/11] fix(clips): fence stale provider takeover --- .../desktop/src-tauri/src/native_screen.rs | 135 ++++++++++-------- .../uploads/[recordingId]/resume.get.test.ts | 24 +++- .../api/uploads/[recordingId]/resume.get.ts | 26 +++- 3 files changed, 123 insertions(+), 62 deletions(-) diff --git a/templates/clips/desktop/src-tauri/src/native_screen.rs b/templates/clips/desktop/src-tauri/src/native_screen.rs index a5c0abccbf..4340e3f449 100644 --- a/templates/clips/desktop/src-tauri/src/native_screen.rs +++ b/templates/clips/desktop/src-tauri/src/native_screen.rs @@ -3772,35 +3772,37 @@ pub async fn native_fullscreen_recording_retry_upload( None, Some(0.0), ); - let reset = match tokio::select! { - reset = reset_upload_chunks( - &saved.server_url, - &saved.recording_id, - &prepared.mime_type, - attempt_id.as_deref(), - upload_generation_id.as_deref(), - &auth_token, - &cookie, - ) => reset, - _ = wait_for_native_upload_retry_cancel(&saved.recording_id) => { - Err(NATIVE_UPLOAD_RETRY_CANCELLED.to_string()) - } - } { + let reset = match reset_upload_chunks( + &saved.server_url, + &saved.recording_id, + &prepared.mime_type, + attempt_id.as_deref(), + upload_generation_id.as_deref(), + &auth_token, + &cookie, + ) + .await + { Ok(reset) => reset, Err(err) => { - interrupt_native_retry_upload( - &saved.server_url, - &saved.recording_id, - &err, - active_attempt_id.as_deref(), - active_upload_generation_id.as_deref(), - &auth_token, - &cookie, - ) - .await; + if err != NATIVE_UPLOAD_RETRY_CANCELLED { + interrupt_native_retry_upload( + &saved.server_url, + &saved.recording_id, + &err, + active_attempt_id.as_deref(), + active_upload_generation_id.as_deref(), + &auth_token, + &cookie, + ) + .await; + } return Err(err); } }; + if native_upload_retry_cancelled(&saved.recording_id) { + return Err(NATIVE_UPLOAD_RETRY_CANCELLED.to_string()); + } (reset.mode(), None, reset.upload_generation_id) } NativeRetryUploadPlan::Reconcile => unreachable!("handled above"), @@ -3836,22 +3838,22 @@ pub async fn native_fullscreen_recording_retry_upload( eprintln!( "[clips-tray] native retry replaying from byte zero after the provider requested a restart" ); - match tokio::select! { - reset = reset_upload_chunks( - &saved.server_url, - &saved.recording_id, - &prepared.mime_type, - replay_attempt_id.as_deref(), - replay_upload_generation_id.as_deref(), - &auth_token, - &cookie, - ) => reset, - _ = wait_for_native_upload_retry_cancel(&saved.recording_id) => { - Err(NATIVE_UPLOAD_RETRY_CANCELLED.to_string()) - } - } { + match reset_upload_chunks( + &saved.server_url, + &saved.recording_id, + &prepared.mime_type, + replay_attempt_id.as_deref(), + replay_upload_generation_id.as_deref(), + &auth_token, + &cookie, + ) + .await + { Ok(reset) => { interruption_upload_generation_id = reset.upload_generation_id.clone(); + if native_upload_retry_cancelled(&saved.recording_id) { + return Err(NATIVE_UPLOAD_RETRY_CANCELLED.to_string()); + } upload_prepared_recording_file( &app, &prepared, @@ -3877,16 +3879,18 @@ pub async fn native_fullscreen_recording_retry_upload( upload_result }; if let Err(err) = &upload_result { - interrupt_native_retry_upload( - &saved.server_url, - &saved.recording_id, - err, - replay_attempt_id.as_deref(), - interruption_upload_generation_id.as_deref(), - &auth_token, - &cookie, - ) - .await; + if err != NATIVE_UPLOAD_RETRY_CANCELLED { + interrupt_native_retry_upload( + &saved.server_url, + &saved.recording_id, + err, + replay_attempt_id.as_deref(), + interruption_upload_generation_id.as_deref(), + &auth_token, + &cookie, + ) + .await; + } } cleanup_prepared_saved_recording_files(&prepared, retry_combined_path); upload_result @@ -5862,11 +5866,12 @@ async fn get_native_retry_upload_plan( ); } let recovery_enabled = response.recovery_enabled; + let rollback_attempt_id = response.attempt_id.clone(); let rollback_generation_id = response.upload_generation_id.clone(); return Ok(preserve_native_retry_fence_during_rollback( plan_native_retry_upload(response, local_bytes, exact_local_stream), recovery_enabled, - claimed_attempt_id, + rollback_attempt_id, rollback_generation_id, )); } @@ -5875,7 +5880,7 @@ async fn get_native_retry_upload_plan( fn preserve_native_retry_fence_during_rollback( mut plan: NativeRetryUploadPlan, recovery_enabled: bool, - claimed_attempt_id: &str, + acknowledged_attempt_id: Option, upload_generation_id: Option, ) -> NativeRetryUploadPlan { if !recovery_enabled { @@ -5884,9 +5889,7 @@ fn preserve_native_retry_fence_during_rollback( upload_generation_id: planned_generation_id, } = &mut plan { - *attempt_id = upload_generation_id - .as_ref() - .map(|_| claimed_attempt_id.to_string()); + *attempt_id = acknowledged_attempt_id; *planned_generation_id = upload_generation_id; } } @@ -6184,7 +6187,7 @@ mod native_retry_upload_plan_tests { upload_generation_id: None, }, false, - "attempt-1", + Some("attempt-1".to_string()), Some("generation-1".to_string()), ); assert!(matches!( @@ -6197,14 +6200,14 @@ mod native_retry_upload_plan_tests { } #[test] - fn keeps_a_legacy_restart_unfenced_when_resumable_retry_is_disabled() { + fn keeps_an_unacknowledged_legacy_restart_unfenced_when_resumable_retry_is_disabled() { let plan = preserve_native_retry_fence_during_rollback( NativeRetryUploadPlan::Restart { attempt_id: None, upload_generation_id: None, }, false, - "attempt-1", + None, None, ); assert!(matches!( @@ -6216,6 +6219,26 @@ mod native_retry_upload_plan_tests { )); } + #[test] + fn preserves_an_acknowledged_legacy_attempt_without_a_generation() { + let plan = preserve_native_retry_fence_during_rollback( + NativeRetryUploadPlan::Restart { + attempt_id: None, + upload_generation_id: None, + }, + false, + Some("attempt-1".to_string()), + None, + ); + assert!(matches!( + plan, + NativeRetryUploadPlan::Restart { + attempt_id: Some(attempt_id), + upload_generation_id: None, + } if attempt_id == "attempt-1" + )); + } + #[test] fn reconciles_terminal_resume_without_an_attempt_echo() { let terminal = plan_native_retry_upload( diff --git a/templates/clips/server/routes/api/uploads/[recordingId]/resume.get.test.ts b/templates/clips/server/routes/api/uploads/[recordingId]/resume.get.test.ts index 14c0177c62..2ebaa02c29 100644 --- a/templates/clips/server/routes/api/uploads/[recordingId]/resume.get.test.ts +++ b/templates/clips/server/routes/api/uploads/[recordingId]/resume.get.test.ts @@ -2,6 +2,8 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; const mockRenewUploadLease = vi.hoisted(() => vi.fn()); const mockGetResumableSession = vi.hoisted(() => vi.fn()); +const mockDeleteResumableSession = vi.hoisted(() => vi.fn()); +const mockAbortResumableUploadSession = vi.hoisted(() => vi.fn()); const mockListRecordingChunkKeys = vi.hoisted(() => vi.fn()); const mockSumRecordingChunkBytes = vi.hoisted(() => vi.fn()); const mockReadAppState = vi.hoisted(() => vi.fn()); @@ -90,9 +92,16 @@ vi.mock("../../../../lib/recording-upload-state.js", () => ({ })); vi.mock("../../../../lib/resumable-session.js", () => ({ + deleteResumableSession: (...args: unknown[]) => + mockDeleteResumableSession(...args), getResumableSession: (...args: unknown[]) => mockGetResumableSession(...args), })); +vi.mock("../../../../lib/resumable-upload-cleanup.js", () => ({ + abortResumableUploadSession: (...args: unknown[]) => + mockAbortResumableUploadSession(...args), +})); + vi.mock("../../../../lib/upload-lease.js", () => ({ UPLOAD_LEASE_MS: 60 * 60 * 1000, renewUploadLease: (...args: unknown[]) => mockRenewUploadLease(...args), @@ -108,6 +117,8 @@ describe("/api/uploads/:recordingId/resume route", () => { mockGetQuery.mockReturnValue({ attemptId: "client-attempt-0001" }); mockRenewUploadLease.mockResolvedValue({ held: true }); mockGetResumableSession.mockResolvedValue(null); + mockAbortResumableUploadSession.mockResolvedValue(true); + mockDeleteResumableSession.mockResolvedValue(undefined); mockListRecordingChunkKeys.mockResolvedValue([]); mockSumRecordingChunkBytes.mockResolvedValue(0); mockReadAppState.mockResolvedValue({ progress: 50 }); @@ -412,7 +423,7 @@ describe("/api/uploads/:recordingId/resume route", () => { expect(mockDb.update).not.toHaveBeenCalled(); }); - it("atomically reclaims an expired different retry claim without changing its generation or provider offset", async () => { + it("invalidates an expired claim's provider session before restarting it", async () => { mockSelectRows.rows = [ { id: "rec-1", @@ -432,8 +443,9 @@ describe("/api/uploads/:recordingId/resume route", () => { resumable: true, attemptId: "client-attempt-0001", uploadGenerationId: "generation-1", - bytesReceived: 7_864_320, - nextChunkIndex: 2, + uploadMode: "buffered", + bytesReceived: 0, + nextChunkIndex: 0, }), ); expect(mockCompareAndSetAppState).toHaveBeenCalledWith( @@ -442,9 +454,13 @@ describe("/api/uploads/:recordingId/resume route", () => { expect.objectContaining({ uploadAttemptId: "client-attempt-0001", uploadGenerationId: "generation-1", - bytesReceived: 7_864_320, }), ); + expect(mockAbortResumableUploadSession).toHaveBeenCalledOnce(); + expect(mockDeleteResumableSession).toHaveBeenCalledWith( + "rec-1", + "generation-1", + ); }); it("loses a stale-claim takeover race without publishing resume state", async () => { diff --git a/templates/clips/server/routes/api/uploads/[recordingId]/resume.get.ts b/templates/clips/server/routes/api/uploads/[recordingId]/resume.get.ts index aec59ba984..a6a94cf66a 100644 --- a/templates/clips/server/routes/api/uploads/[recordingId]/resume.get.ts +++ b/templates/clips/server/routes/api/uploads/[recordingId]/resume.get.ts @@ -40,7 +40,11 @@ import { getEventOwnerContext, ownerEmailMatches, } from "../../../../lib/recordings.js"; -import { getResumableSession } from "../../../../lib/resumable-session.js"; +import { + deleteResumableSession, + getResumableSession, +} from "../../../../lib/resumable-session.js"; +import { abortResumableUploadSession } from "../../../../lib/resumable-upload-cleanup.js"; import { isRetryableUploadInterruption } from "../../../../lib/upload-interruption.js"; import { UPLOAD_LEASE_MS, @@ -177,7 +181,7 @@ export default defineEventHandler(async (event: H3Event) => { // upgrades them by installing a fresh generation before it deletes data. const existingGenerationId = recording.uploadGenerationId ?? null; const generationId = existingGenerationId; - const session = generationId + let session = generationId ? await getResumableSession(recordingId, generationId) : await getResumableSession(recordingId); const retryableFailure = @@ -279,6 +283,24 @@ export default defineEventHandler(async (event: H3Event) => { }; } + if (takingOverStaleRetryClaim && session) { + const invalidated = await abortResumableUploadSession(session, { + label: `upload-resume-takeover-${recordingId}`, + }); + if (!invalidated) { + setResponseStatus(event, 409); + return { + resumable: false, + recoveryEnabled: true, + recordingId, + status: "uploading", + reason: "stale_provider_session_invalidation_failed", + }; + } + await deleteResumableSession(recordingId, generationId); + session = null; + } + const uploadStateUpdated = await compareAndSetAppState( uploadStateKey, uploadStateRaw, From 94e657039753f20d3a0626386201b9dacd593651 Mon Sep 17 00:00:00 2001 From: Alice Alexandra Moore <86723305+3mdistal@users.noreply.github.com> Date: Fri, 21 Aug 2026 11:13:46 -0400 Subject: [PATCH 06/11] fix(clips): close retry review edges --- templates/clips/desktop/src/app.tsx | 18 ++++---- .../uploads/[recordingId]/resume.get.test.ts | 41 ++++++++++++++++++- .../api/uploads/[recordingId]/resume.get.ts | 22 +++++++++- 3 files changed, 69 insertions(+), 12 deletions(-) diff --git a/templates/clips/desktop/src/app.tsx b/templates/clips/desktop/src/app.tsx index c7c0f9aec8..3ce738d48c 100644 --- a/templates/clips/desktop/src/app.tsx +++ b/templates/clips/desktop/src/app.tsx @@ -4429,8 +4429,8 @@ function PendingUploadBanner({ if (!latest) return null; const retrying = retryingUploadId === latest.recordingId; - const retryWaiting = - retrying && retryingUploadStatus === "Waiting for prior retry"; + const retryCancelling = + retrying && retryingUploadStatus === "Cancelling retry"; const storageSetupFailure = isStorageSetupFailureMessage(latest.lastError); const canOpenFolder = latest.kind === "native" && !!latest.folderPath; @@ -4537,23 +4537,21 @@ function PendingUploadBanner({ diff --git a/templates/clips/server/routes/api/uploads/[recordingId]/resume.get.test.ts b/templates/clips/server/routes/api/uploads/[recordingId]/resume.get.test.ts index 2ebaa02c29..9d4d5984cf 100644 --- a/templates/clips/server/routes/api/uploads/[recordingId]/resume.get.test.ts +++ b/templates/clips/server/routes/api/uploads/[recordingId]/resume.get.test.ts @@ -13,6 +13,7 @@ const mockSetResponseStatus = vi.hoisted(() => vi.fn()); const mockGetQuery = vi.hoisted(() => vi.fn()); const mockIsFeatureFlagEnabled = vi.hoisted(() => vi.fn()); const mockUpdateRows = vi.hoisted(() => ({ rows: [{ id: "rec-1" }] })); +const mockUpdateSets = vi.hoisted(() => [] as Array>); const mockSelectRows = vi.hoisted(() => ({ rows: [] as Array>, })); @@ -26,7 +27,10 @@ const mockDb = vi.hoisted(() => ({ }), update: vi.fn(() => { const builder = { - set: vi.fn(() => builder), + set: vi.fn((values: Record) => { + mockUpdateSets.push(values); + return builder; + }), where: vi.fn(() => builder), returning: vi.fn(async () => mockUpdateRows.rows), }; @@ -125,6 +129,7 @@ describe("/api/uploads/:recordingId/resume route", () => { mockWriteAppState.mockResolvedValue(undefined); mockCompareAndSetAppState.mockResolvedValue(true); mockUpdateRows.rows = [{ id: "rec-1" }]; + mockUpdateSets.length = 0; mockIsFeatureFlagEnabled.mockResolvedValue(true); }); @@ -463,6 +468,40 @@ describe("/api/uploads/:recordingId/resume route", () => { ); }); + it("restores an expired claim when its provider session cannot be invalidated", async () => { + mockSelectRows.rows = [ + { + id: "rec-1", + status: "uploading", + uploadAttemptId: "stale-attempt-0001", + uploadGenerationId: "generation-1", + uploadLeaseExpiresAt: "2000-01-01T00:00:00.000Z", + }, + ]; + mockGetResumableSession.mockResolvedValue({ + bytesUploaded: 7_864_320, + lastCommittedIndex: 1, + }); + mockAbortResumableUploadSession.mockResolvedValue(false); + + await expect(handler({} as any)).resolves.toEqual({ + resumable: false, + recoveryEnabled: true, + recordingId: "rec-1", + status: "uploading", + reason: "stale_provider_session_invalidation_failed", + }); + expect(mockUpdateSets).toHaveLength(2); + expect(mockUpdateSets[1]).toEqual( + expect.objectContaining({ + uploadAttemptId: "stale-attempt-0001", + uploadLeaseExpiresAt: "2000-01-01T00:00:00.000Z", + }), + ); + expect(mockDeleteResumableSession).not.toHaveBeenCalled(); + expect(mockCompareAndSetAppState).not.toHaveBeenCalled(); + }); + it("loses a stale-claim takeover race without publishing resume state", async () => { mockSelectRows.rows = [ { diff --git a/templates/clips/server/routes/api/uploads/[recordingId]/resume.get.ts b/templates/clips/server/routes/api/uploads/[recordingId]/resume.get.ts index a6a94cf66a..b07c2a3bc8 100644 --- a/templates/clips/server/routes/api/uploads/[recordingId]/resume.get.ts +++ b/templates/clips/server/routes/api/uploads/[recordingId]/resume.get.ts @@ -238,6 +238,7 @@ export default defineEventHandler(async (event: H3Event) => { const uploadState = uploadStateRaw ?? {}; const attemptId = requestedAttemptId; const takingOverStaleRetryClaim = differentRetryClaim; + const claimedLeaseExpiry = uploadLeaseExpiry(nowMs); const claimed = await getDb() .update(schema.recordings) .set({ @@ -245,7 +246,7 @@ export default defineEventHandler(async (event: H3Event) => { failureReason: null, uploadAttemptId: attemptId, ...(generationId ? { uploadGenerationId: generationId } : {}), - uploadLeaseExpiresAt: uploadLeaseExpiry(nowMs), + uploadLeaseExpiresAt: claimedLeaseExpiry, updatedAt: now, }) .where( @@ -288,6 +289,25 @@ export default defineEventHandler(async (event: H3Event) => { label: `upload-resume-takeover-${recordingId}`, }); if (!invalidated) { + await getDb() + .update(schema.recordings) + .set({ + uploadAttemptId: existingAttemptId, + uploadLeaseExpiresAt: recording.uploadLeaseExpiresAt, + updatedAt: new Date(claimHeartbeatMs).toISOString(), + }) + .where( + and( + eq(schema.recordings.id, recordingId), + ownerEmailMatches(schema.recordings.ownerEmail, ownerEmail), + eq(schema.recordings.status, "uploading"), + eq(schema.recordings.uploadAttemptId, attemptId), + generationId === null + ? isNull(schema.recordings.uploadGenerationId) + : eq(schema.recordings.uploadGenerationId, generationId), + eq(schema.recordings.uploadLeaseExpiresAt, claimedLeaseExpiry), + ), + ); setResponseStatus(event, 409); return { resumable: false, From bb2cc7ac36c4457bdd16e42153663b43d6b20e65 Mon Sep 17 00:00:00 2001 From: Alice Alexandra Moore <86723305+3mdistal@users.noreply.github.com> Date: Fri, 21 Aug 2026 15:56:13 -0400 Subject: [PATCH 07/11] fix(clips): settle provider retry effects safely --- ...hape-pr3345-provider-side-effect-repair.md | 411 ++++++++++++++++++ .../desktop/src-tauri/src/native_screen.rs | 67 ++- .../server/lib/resumable-session.test.ts | 50 +++ .../clips/server/lib/resumable-session.ts | 15 + .../uploads/[recordingId]/chunk.post.test.ts | 331 +++++++++++++- .../api/uploads/[recordingId]/chunk.post.ts | 303 +++++++------ 6 files changed, 1017 insertions(+), 160 deletions(-) create mode 100644 plans/shape-pr3345-provider-side-effect-repair.md create mode 100644 templates/clips/server/lib/resumable-session.test.ts diff --git a/plans/shape-pr3345-provider-side-effect-repair.md b/plans/shape-pr3345-provider-side-effect-repair.md new file mode 100644 index 0000000000..fd33414c2f --- /dev/null +++ b/plans/shape-pr3345-provider-side-effect-repair.md @@ -0,0 +1,411 @@ +# PR #3345 provider-side-effect repair Shape + +Status: Work implemented and locally verified; PR refresh and exact-head CI remain pending. Merge and production mutation are not authorized. + +## Summary + +PR [#3345](https://github.com/BuilderIO/agent-native/pull/3345) is still open at exact head +`94e657039753f20d3a0626386201b9dacd593651`. Its ordinary GitHub, targeted test, +desktop-platform, security, build, and preview checks completed without failure. Builder's +exact-head review opened two blocking threads in the resumable chunk route: + +1. a delayed failed provider response may abort or delete a session after the request has + lost the exact attempt/generation lease; and +2. a provider-accepted chunk may be returned as stale before its accepted offset and metadata + are durably reconciled, leaving a later retry able to replay the old range. + +The smallest coherent repair keeps the shared provider contract unchanged. Clips must treat a +provider call as a side effect whose result must be settled exactly once: destructive cleanup +belongs only to a path that has first retired the session's generation, while an accepted result +must be monotonically reconciled into the exact stored session before ownership loss is returned +to the caller. + +## Current evidence + +### Direct facts + +- Exact PR head: `94e657039753f20d3a0626386201b9dacd593651`. +- Current open Builder threads are only: + - `PRRT_kwDORlS_j86bNdZl` / comment `3831642188`, failed-response cleanup without a + current ownership fence; and + - `PRRT_kwDORlS_j86bNdZo` / comment `3831642190`, accepted provider state stranded after + heartbeat ownership loss. +- `handleResumableChunk()` currently performs provider relay under a lease heartbeat, then: + - returns `409` immediately when the heartbeat observed ownership loss; + - advances `bytesUploaded`, `lastCommittedIndex`, and `updatedMeta` only after another lease + renewal succeeds; and + - calls `cleanupFailedFinalSession()` on non-OK final responses without first checking the + exact attempt/generation lease. The same helper aborts the provider session and deletes its + local handle. +- The reset route already owns the safer cleanup seam: it first compare-and-set rotates the + recording to a new generation, claims cleanup of the retired session in application state, + aborts that old provider session, and only then retires the old local handle. +- The shared `FileUploadProvider.resumable` contract returns `ok`, `status`, and optional + `updatedMeta`; it has no cross-provider authoritative-offset query. Builder/GCS returns an + accepted status but no offset metadata, while S3 returns provider metadata required for later + multipart completion. +- Stored resumable session state is already generation-scoped and contains `sessionId`, `meta`, + `bytesUploaded`, and `lastCommittedIndex`. Core application state already provides exact + compare-and-set. +- The older committed-reset cancellation thread `PRRT_kwDORlS_j86bMP7d` is resolved. Current + native code awaits the reset response, retains the returned generation for subsequent work, + observes cancellation only after that response, and excludes the cancellation sentinel from + the generic interrupt path. Builder's latest summary explicitly did not repost it. + +### Inferences + +- A provider's accepted response is the best available cross-provider proof of the exact call's + side effect. Querying the provider later is not a portable first-slice solution. +- An application-state compare-and-set from the exact pre-relay session snapshot to its monotonic + accepted successor can preserve the accepted offset without overwriting a newer session. +- Destructive provider cleanup is safe only after the affected session is no longer a live session + any writer may use. Merely checking a renewable lease immediately before a potentially slow + abort leaves another time-of-check/time-of-use window; rotating the generation first closes it. + +## Causal model and ownership boundary + +There are two authorities, and neither substitutes for the other: + +- The recording row's `(uploadAttemptId, uploadGenerationId, lease)` decides which request may + initiate another provider write or move recording lifecycle state. +- The provider response decides whether the provider side effect from an already-issued request + was accepted. + +Today those authorities are evaluated in the wrong order after relay. If the lease is lost, the +handler returns stale before recording an accepted provider effect. Conversely, if the provider +returns failure, the handler may destroy a session even though that request no longer owns it. + +The repaired boundary is: + +1. **Before dispatch:** require the exact attempt/generation lease. +2. **During dispatch:** heartbeat the same fence, including close/final provider calls. +3. **After an accepted response:** settle the provider effect monotonically against the exact + pre-dispatch session snapshot. This settlement is reconciliation, not permission for more work; + it must occur before returning an observed ownership loss. +4. **After a failed or ambiguous response:** do not destructively abort or delete a still-live + session from the chunk request. Return a typed failure/restart result and let reset rotate the + generation and claim cleanup of the retired session. +5. **After settlement:** only a request that still holds the lease may continue to finalization or + dispatch another provider operation. A stale request stops with a typed `409` after leaving + durable provider/session truth monotonic. + +## Frozen invariants + +### Provider session cleanup + +- A chunk request never aborts or deletes a provider session that remains attached to a live + generation. +- Provider abort and local-handle deletion occur only through an explicit cleanup claim for a + generation already retired by an exact recording compare-and-set. +- Cleanup failure remains loud and recoverable; it cannot be coerced into successful restart or + absence. +- Cleanup of generation A can neither target nor delete generation B's local handle or provider + session. + +### Accepted offset persistence and reconciliation + +- Every accepted data-chunk response advances stored session truth exactly once from the exact + pre-dispatch snapshot, including provider `updatedMeta`. +- Accepted state is monotonic: `bytesUploaded` and `lastCommittedIndex` never move backward, and + a stale settlement never overwrites a different session or a successor already beyond it. +- If the exact compare-and-set loses, the handler rereads session state. It may classify the + effect as already reconciled only when the same session is at or beyond the accepted byte/index + boundary with compatible metadata. Missing, regressed, or contradictory state fails loudly and + forces the safe retired-generation restart path; it is never returned as a normal stale retry. +- Ownership loss prevents further work, but does not discard an accepted provider result. + +### Takeover and retry + +- A different live claim continues to return the typed bounded conflict. +- A stale takeover never reuses an offset from a session with an unsettled or contradictory + provider effect. It retires that session/generation and restarts from a new safe generation. +- Same-claim response-loss retry resumes only from the reconciled committed offset and index. +- Duplicate chunks at or below the reconciled committed index remain acknowledgements without a + second provider write. + +### Cancellation + +- Cancellation stops local replay/upload work and preserves the local backup. +- Cancellation never routes through generic interruption merely because it races a reset response. +- If reset commits before cancellation is observed, the client consumes the authoritative reset + response first. The preserved local attempt can immediately re-enter that server fence on the + next retry; no pre-reset fence is used to interrupt it. +- The resolved committed-reset behavior receives regression coverage in the same verification + packet, but no new native behavior is in the first slice unless that coverage disproves the + current invariant. + +### Finalization + +- Accepted provider state is settled before finalization. +- A stale request never finalizes, aborts, or deletes after ownership loss. +- Ambiguous provider completion remains distinguishable from failure, restart-required, stale + ownership, and ready recording reconciliation. + +## Recommended Work slice + +Keep the repair inside Clips and avoid a shared provider-contract expansion: + +1. Add a generation-scoped resumable-session compare-and-set helper using the existing core + application-state CAS primitive. +2. Refactor the chunk route's provider-result settlement into one boundary used by data chunks, + final data chunks, and the zero-byte close sentinel: + - reconcile accepted offset/index/meta first; + - then stop with typed stale ownership when the heartbeat or post-response lease was lost; and + - continue/finalize only while the exact lease remains held. +3. Remove inline destructive cleanup from failed provider-response paths. Route restart-required + and final-session retirement through the existing reset generation-rotation and cleanup-claim + path. +4. Reuse the same typed stale/restart vocabulary already consumed by browser and native retry + clients. Change client code only if an existing response branch cannot express the safe reset. +5. Add focused deterministic server races and a regression assertion for the already-resolved + native committed-reset cancellation behavior. + +This is one coherent Work slice because all changes enforce one rule: provider effects are settled +against an exact session incarnation, while destruction occurs only after that incarnation is +retired. + +## Compatibility and rollback + +- Keep the existing default-off `uploadRetryResume` feature flag and Alice-only production target. +- Flag-off legacy null-fence uploads remain unfenced and keep their existing full-restart behavior. +- Existing non-null attempt and optional generation fences remain preserved when the flag changes. +- Buffered uploads, non-resumable providers, ordinary first uploads, share URLs, recording schema, + and provider credentials are unchanged. +- No schema migration and no new shared provider method are required. +- Disabling the flag remains the operational rollback for new retries. It does not erase an + already-stored fence or accepted resumable-session state. +- A failed cleanup or contradictory settlement fails closed with the local backup retained; it + does not delete media or pretend the upload restarted. + +## Acceptance story + +### Automated assertions + +Focused deterministic tests must prove: + +1. A non-OK final data response that loses its exact lease performs no provider abort and no local + session deletion, and returns typed stale ownership. +2. A non-OK close-sentinel response under the same race has the same result. +3. A provider failure while ownership remains held returns the existing loud failure/restart + result but defers destruction to reset. +4. Reset first rotates generation, then claims and cleans only the retired session; a concurrent + successor generation is untouched. +5. A provider-accepted ordinary chunk plus heartbeat ownership loss CAS-persists its advanced + bytes, index, and metadata before returning stale. +6. The analogous accepted final-data and close-sentinel paths settle metadata before stale exit + and never finalize after ownership loss. +7. A lost settlement CAS rereads: already-advanced same-session state is accepted as reconciled; + a different/newer session is untouched; regressed or contradictory same-session state fails + loudly and forces safe restart. +8. A same-claim retry resumes at the reconciled byte/index boundary and duplicate replay does not + call the provider. +9. A stale different-attempt takeover cannot consume unsettled state and uses the retired-session + cleanup/reset path. +10. Cancellation after a committed native reset waits for the reset response, skips generic + interruption, preserves the local backup/attempt, and allows immediate same-claim retry. +11. Flag-off legacy null-fence and acknowledged-attempt-with-null-generation cases remain + unchanged. + +### Exact-head verification + +After implementation, bind all evidence to the new exact PR head: + +- focused resume, chunk, reset, abort, upload-lease, browser recovery, and native retry-plan tests; +- Clips desktop TypeScript; +- Rust 1.88 native tests and `cargo fmt`; +- `oxfmt` for every modified TypeScript/TSX file and `git diff --check`; +- repository targeted-workspace tests, lint/format, typecheck, security/static guards, general + build, and all three Clips desktop platform builds; +- a fresh Builder review with zero open actionable threads, plus signed replies/resolutions only + after Work is authorized and fixes exist; +- the required human technical approval on the exact final head. + +### Real-interface and canary evidence + +The concurrency invariants are principally automated; a manual desktop run cannot reliably force +the provider-response/lease interleavings. Real-interface acceptance is still required for the +successful-user story because the product bug is a production desktop retry after network loss. + +- Before merge: a local desktop smoke is preferred, same-context allowed, verifying Retry remains + cancellable and the local backup remains visible. It is not a substitute for the race tests. +- After merge/deploy: repeat the Alice-only production Wi-Fi-interruption canary under + `uploadRetryResume`. The prior canary predates this repair and cannot prove the new exact artifact. + Verify interruption leaves the backup, Retry completes without byte-zero replay or raw conflict, + Cancel retry remains available during active work, and a subsequent retry still succeeds. +- Do not broaden flag targeting until that post-merge canary is recorded against the deployed + revision. + +Acceptance policy: real-interface, independence preferred, same-context allowed, through the signed +Clips desktop application and Alice-only production flag target. Independent technical review is +required because this is provider-side concurrency and destructive-cleanup logic. + +## Explicit non-goals + +- No shared `FileUploadProvider` offset-query API or provider-specific status protocol. +- No schema change, background reconciliation service, upload queue, or new distributed lock. +- No changes to recording/share content, local cache format, authentication, storage credentials, + or production data. +- No broad feature-flag rollout. +- No redesign of the retry banner or cancellation UX. +- No merge, deployment, production mutation, review reply, or thread resolution during Shape. + +## Work evidence + +- Alice explicitly invoked Work against this artifact on 2026-08-21. +- Clips persists accepted data, final-data, and close-sentinel provider effects with an exact generation-scoped session CAS before returning stale ownership. +- Provider failures no longer abort or delete a live resumable session. Ambiguous transport outcomes return the existing typed restart signal so reset retires the generation before replay. +- The resolved native committed-reset cancellation ordering is covered without changing the user-facing flow. +- Focused Vitest: 112 passed. Full Clips Vitest: 1,399 passed. Rust 1.88: 210 passed, 1 intentionally ignored. TypeScript, `oxfmt`, `cargo fmt`, and `git diff --check` pass. +- Independent Terra review found ambiguous data and close response-loss paths plus missing final-data race coverage. All findings were repaired; the bounded one-follow-up ceiling prevented a third ceremonial review turn after the final close-path repair. + +## Architecture grounding and fit + +Grounding is required because this repair crosses the recording lease, persisted session, provider, +reset cleanup, and native cancellation seams. + +- **Demonstrated caller:** signed Clips desktop retrying one locally saved recording after a failed + or interrupted resumable upload. +- **Existing primitives:** exact upload lease CAS, generation-scoped resumable session state, core + application-state CAS, relay heartbeat, reset generation rotation, reset cleanup claim, typed + retry/restart responses, and native cancellation sentinel. +- **Ownership boundaries:** recording row owns write permission; provider response owns knowledge of + an issued side effect; generation-scoped application state owns resumable offset/meta; reset owns + retirement and destructive cleanup; clients own local cancellation and backup custody. +- **Legacy contracts:** flag-off behavior, buffered upload fallback, provider portability, local + backup retention, same-claim lost-response retry, and ready-recording reconciliation remain + unchanged. +- **Smallest compatible delta:** use existing Clips/core CAS and reset cleanup primitives to settle + provider results and retire sessions safely; do not expand the provider interface. +- **Deferred capabilities:** provider offset introspection, generic framework resumable-operation + journals, and cross-template upload orchestration. +- **Reversibility:** bounded source changes behind the existing flag, no migration, and operational + rollback by disabling the flag for new retries. +- **Unresolved owner questions:** none. Current code and review evidence establish the local Clips + boundary without changing a public/shared contract. + +## Architecture fingerprint and lifecycle authority + +```yaml +authoritySchemaVersion: 3 +stage: shape +authority-source: >- + Alice delegated PR #3345 back to Shape for diagnosis only; no implementation, + push, review mutation, production mutation, or merge. +authorized-scope: + repositories: [BuilderIO/agent-native] + product-surfaces: [Clips resumable retry recovery] + outcome: >- + Freeze the smallest repair that settles accepted provider effects and prevents stale + destructive cleanup while preserving retry, cancellation, and feature-flag compatibility. +allowed-mutations: [artifact-write] +write-targets: + artifacts: [plans/shape-pr3345-provider-side-effect-repair.md] +governing-artifact: + path: plans/shape-pr3345-provider-side-effect-repair.md + revision: shape-pr3345-provider-side-effect-repair-r1 +architecture-fingerprint: + outcome: >- + A failed or accepted provider response is reconciled against its exact resumable session; + stale requests cannot destroy live sessions, and retries never replay a provider-accepted range. + shipping-surfaces: + - id: clips-resumable-retry-repair + repository: BuilderIO/agent-native + product-surface: signed Clips desktop resumable retry and hosted Clips upload routes + constituency: Clips users with a locally saved recording whose upload failed or was interrupted + durable-destination: BuilderIO/agent-native main lineage and deployed Clips production + integration-action: merge + governing-architecture: >- + Recording attempt/generation lease gates new work, generation-scoped CAS settles provider + results monotonically, and reset owns cleanup only after retiring the affected generation; + the shared provider contract remains unchanged. + acceptance-story: + id: clips-pr3345-provider-side-effect-repair + summary: >- + After network loss or stale retry ownership, accepted bytes remain resumable, stale requests + cannot abort a successor session, cancellation preserves the local backup, and the Alice-only + production retry completes safely. + required-assertions: + - both current Builder races are deterministically covered and fixed on the exact PR head + - accepted bytes, chunk index, and provider metadata reconcile monotonically before stale exit + - destructive cleanup occurs only after exact generation retirement and cannot touch a successor + - same-claim retry resumes without replay while different-claim takeover safely restarts + - committed-reset cancellation preserves the authoritative fence and local backup + - flag-off legacy and buffered/non-resumable behavior remain unchanged + - focused and full exact-head CI are green with zero actionable review threads + - Alice-only post-merge production Wi-Fi canary succeeds before broader rollout + acceptance-policy: + modality: real-interface + independence: preferred + custody: same-context-allowed + interface: signed Clips desktop plus Alice-only production uploadRetryResume target + rationale: >- + Deterministic tests prove concurrency; the real desktop canary proves the user journey. + Same-context custody is sufficient, while exact-head independent technical review remains required. + risk-strategy: + kind: feature-flagged + production-validation-after-merge: true +architecture-grounding: + applicability: required + reason: Provider side effects cross lease, session, reset-cleanup, and native-client boundaries. + status: grounded + demonstrated-callers: + - signed Clips desktop retrying a locally saved interrupted recording + existing-primitives: + - exact attempt/generation upload lease CAS and heartbeat + - generation-scoped resumable session application state + - core application-state compare-and-set + - reset generation rotation and resumable cleanup claim + - typed browser/native retry, restart, and cancellation flows + ownership-boundaries: + - recording lease authorizes new provider work + - provider response proves the result of already-issued work + - session CAS owns monotonic accepted offset and metadata + - reset owns retired-generation destructive cleanup + - desktop owns local cancellation and backup custody + legacy-contracts: + - flag-off legacy uploads and preserved fences + - buffered and non-resumable uploads + - same-claim response-loss retry and duplicate acknowledgement + - local backup retention and ready-recording reconciliation + shared-vocabulary: + - provider-effect settlement + - retired-generation cleanup + - accepted-offset reconciliation + smallest-compatible-delta: >- + Add a Clips resumable-session CAS settlement helper, use it after provider relay, and route + destructive failure cleanup through existing reset retirement. + deferred-capabilities: + - provider offset query API + - generic framework operation journal + - background reconciliation worker + reversibility: Existing default-off flag, no migration, no shared contract expansion. + direct-evidence: + - PR head 94e657039753f20d3a0626386201b9dacd593651 + - Builder threads PRRT_kwDORlS_j86bNdZl and PRRT_kwDORlS_j86bNdZo + - templates/clips/server/routes/api/uploads/[recordingId]/chunk.post.ts + - templates/clips/server/routes/api/uploads/[recordingId]/reset-chunks.post.ts + - templates/clips/server/lib/resumable-session.ts + - packages/core/src/file-upload/types.ts + - templates/clips/desktop/src-tauri/src/native_screen.rs + inferences: + - exact session CAS can portably settle accepted provider results without a provider query method + unresolved-owner-questions: [] +delegation-ceiling: [read-only] +acceptance-state: + status: pending + summary: >- + Work is not authorized. Current PR head has two open blocking Builder threads; exact-head repair, + technical review, CI, and post-merge Alice-only canary remain required. + blockers: + - explicit Alice /work approval for this exact fingerprint + - implementation and exact-head verification of the frozen assertions + - fresh technical approval and post-merge production canary +ledger-revision: shape-pr3345-provider-side-effect-repair-r1 +status: return-to-shape +``` + +## Approval boundary + +Approval of `/work plans/shape-pr3345-provider-side-effect-repair.md` authorizes only the +bounded Clips repair, focused/full verification, PR push, and review-thread handling described +above. It does not authorize merge, deployment, production mutation, or broader flag rollout. diff --git a/templates/clips/desktop/src-tauri/src/native_screen.rs b/templates/clips/desktop/src-tauri/src/native_screen.rs index 4340e3f449..08a56d13e7 100644 --- a/templates/clips/desktop/src-tauri/src/native_screen.rs +++ b/templates/clips/desktop/src-tauri/src/native_screen.rs @@ -215,7 +215,7 @@ struct NativeUploadResumeResponse { retry_after_ms: Option, } -#[derive(Debug, Deserialize)] +#[derive(Debug, Deserialize, PartialEq, Eq)] #[serde(rename_all = "camelCase")] struct NativeUploadResetResponse { upload_mode: Option, @@ -228,6 +228,16 @@ impl NativeUploadResetResponse { } } +fn accept_native_retry_reset( + reset: NativeUploadResetResponse, + cancelled: bool, +) -> Result { + if cancelled { + return Err(NATIVE_UPLOAD_RETRY_CANCELLED.to_string()); + } + Ok(reset) +} + impl NativeUploadMode { pub(crate) fn from_option(value: Option) -> Self { match value.as_deref() { @@ -3800,9 +3810,10 @@ pub async fn native_fullscreen_recording_retry_upload( return Err(err); } }; - if native_upload_retry_cancelled(&saved.recording_id) { - return Err(NATIVE_UPLOAD_RETRY_CANCELLED.to_string()); - } + let reset = accept_native_retry_reset( + reset, + native_upload_retry_cancelled(&saved.recording_id), + )?; (reset.mode(), None, reset.upload_generation_id) } NativeRetryUploadPlan::Reconcile => unreachable!("handled above"), @@ -3851,9 +3862,10 @@ pub async fn native_fullscreen_recording_retry_upload( { Ok(reset) => { interruption_upload_generation_id = reset.upload_generation_id.clone(); - if native_upload_retry_cancelled(&saved.recording_id) { - return Err(NATIVE_UPLOAD_RETRY_CANCELLED.to_string()); - } + let reset = accept_native_retry_reset( + reset, + native_upload_retry_cancelled(&saved.recording_id), + )?; upload_prepared_recording_file( &app, &prepared, @@ -6091,15 +6103,42 @@ fn native_retry_interruption_payload( #[cfg(test)] mod native_retry_upload_plan_tests { use super::{ - is_native_upload_restart_required, is_native_upload_unfenced_restart_required, - native_replay_attempt_id, native_retry_attempt_id, native_retry_conflict_delay, - native_retry_interruption_payload, plan_native_retry_upload, - preserve_native_retry_fence_during_rollback, saved_native_retry_attempt_id, upload_url, - NativeFullscreenUploadResult, NativeRetryUploadPlan, NativeUploadResumeResponse, - NATIVE_UPLOAD_RESTART_REQUIRED, NATIVE_UPLOAD_UNFENCED_RESTART_REQUIRED, - UPLOAD_CHUNK_BYTES, + accept_native_retry_reset, is_native_upload_restart_required, + is_native_upload_unfenced_restart_required, native_replay_attempt_id, + native_retry_attempt_id, native_retry_conflict_delay, native_retry_interruption_payload, + plan_native_retry_upload, preserve_native_retry_fence_during_rollback, + saved_native_retry_attempt_id, upload_url, NativeFullscreenUploadResult, + NativeRetryUploadPlan, NativeUploadResetResponse, NativeUploadResumeResponse, + NATIVE_UPLOAD_RESTART_REQUIRED, NATIVE_UPLOAD_RETRY_CANCELLED, + NATIVE_UPLOAD_UNFENCED_RESTART_REQUIRED, UPLOAD_CHUNK_BYTES, }; + #[test] + fn cancellation_after_a_committed_reset_preserves_the_authoritative_response() { + let reset = NativeUploadResetResponse { + upload_mode: Some("streaming".to_string()), + upload_generation_id: Some("generation-after-reset".to_string()), + }; + + assert_eq!( + accept_native_retry_reset(reset, true), + Err(NATIVE_UPLOAD_RETRY_CANCELLED.to_string()) + ); + assert_eq!( + accept_native_retry_reset( + NativeUploadResetResponse { + upload_mode: Some("streaming".to_string()), + upload_generation_id: Some("generation-after-reset".to_string()), + }, + false, + ) + .expect("retry may re-enter the committed reset fence") + .upload_generation_id + .as_deref(), + Some("generation-after-reset") + ); + } + fn response(bytes_received: u64, next_chunk_index: u64) -> NativeUploadResumeResponse { NativeUploadResumeResponse { resumable: true, diff --git a/templates/clips/server/lib/resumable-session.test.ts b/templates/clips/server/lib/resumable-session.test.ts new file mode 100644 index 0000000000..f255b31d4f --- /dev/null +++ b/templates/clips/server/lib/resumable-session.test.ts @@ -0,0 +1,50 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +const mockCompareAndSetAppState = vi.hoisted(() => vi.fn()); + +vi.mock("@agent-native/core/application-state", () => ({ + compareAndSetAppState: (...args: unknown[]) => + mockCompareAndSetAppState(...args), + deleteAppState: vi.fn(), + readAppState: vi.fn(), + writeAppState: vi.fn(), +})); + +import { compareAndSetResumableSession } from "./resumable-session"; + +describe("compareAndSetResumableSession", () => { + beforeEach(() => { + vi.clearAllMocks(); + mockCompareAndSetAppState.mockResolvedValue(true); + }); + + it("fences settlement to the exact recording generation and session snapshot", async () => { + const expected = { + providerId: "s3", + sessionId: "session-a", + meta: { uploadId: "upload-a" }, + bytesUploaded: 100, + lastCommittedIndex: 2, + }; + const next = { + ...expected, + meta: { ...expected.meta, completedPart: 3 }, + bytesUploaded: 125, + lastCommittedIndex: 3, + }; + + await expect( + compareAndSetResumableSession( + "recording-a", + expected, + next, + "generation-a", + ), + ).resolves.toBe(true); + expect(mockCompareAndSetAppState).toHaveBeenCalledWith( + "resumable-session-recording-a-generation-a", + expected, + next, + ); + }); +}); diff --git a/templates/clips/server/lib/resumable-session.ts b/templates/clips/server/lib/resumable-session.ts index 06b41ff7c3..28590ea452 100644 --- a/templates/clips/server/lib/resumable-session.ts +++ b/templates/clips/server/lib/resumable-session.ts @@ -1,4 +1,5 @@ import { + compareAndSetAppState, deleteAppState, readAppState, writeAppState, @@ -10,6 +11,20 @@ export interface StoredResumableSession { meta: Record; bytesUploaded: number; lastCommittedIndex?: number; + providerClosed?: boolean; +} + +export async function compareAndSetResumableSession( + recordingId: string, + expected: StoredResumableSession, + next: StoredResumableSession, + generationId?: string | null, +): Promise { + return compareAndSetAppState( + key(recordingId, generationId), + expected as unknown as Record, + next as unknown as Record, + ); } const key = (recordingId: string, generationId?: string | null) => diff --git a/templates/clips/server/routes/api/uploads/[recordingId]/chunk.post.test.ts b/templates/clips/server/routes/api/uploads/[recordingId]/chunk.post.test.ts index c61878bdbb..3449ca81c3 100644 --- a/templates/clips/server/routes/api/uploads/[recordingId]/chunk.post.test.ts +++ b/templates/clips/server/routes/api/uploads/[recordingId]/chunk.post.test.ts @@ -21,6 +21,7 @@ const mockSumRecordingChunkBytes = vi.hoisted(() => vi.fn()); const mockGetResumableSession = vi.hoisted(() => vi.fn()); const mockDeleteResumableSession = vi.hoisted(() => vi.fn()); const mockSetResumableSession = vi.hoisted(() => vi.fn()); +const mockCompareAndSetResumableSession = vi.hoisted(() => vi.fn()); const mockRelayChunk = vi.hoisted(() => vi.fn()); const mockAbortSession = vi.hoisted(() => vi.fn()); const mockResolveResumableUploadProvider = vi.hoisted(() => vi.fn()); @@ -122,6 +123,8 @@ vi.mock("../../../../lib/recordings.js", () => ({ })); vi.mock("../../../../lib/resumable-session.js", () => ({ + compareAndSetResumableSession: (...args: unknown[]) => + mockCompareAndSetResumableSession(...args), deleteResumableSession: (...args: unknown[]) => mockDeleteResumableSession(...args), getResumableSession: (...args: unknown[]) => mockGetResumableSession(...args), @@ -192,6 +195,7 @@ describe("/api/uploads/:recordingId/chunk route", () => { mockGetResumableSession.mockResolvedValue(null); mockDeleteResumableSession.mockResolvedValue(undefined); mockSetResumableSession.mockResolvedValue(undefined); + mockCompareAndSetResumableSession.mockResolvedValue(true); mockIsStreamingUploadDisabled.mockReturnValue(false); mockShouldRejectVideoUploadWithoutStorage.mockResolvedValue(false); mockAllowsSqlRecordingChunkScratch.mockReturnValue(true); @@ -435,6 +439,12 @@ describe("/api/uploads/:recordingId/chunk route", () => { await expect(pending).resolves.toEqual( expect.objectContaining({ staleAttempt: true }), ); + expect(mockCompareAndSetResumableSession).toHaveBeenCalledWith( + "rec-1", + expect.objectContaining({ bytesUploaded: 0 }), + expect.objectContaining({ bytesUploaded: 1, lastCommittedIndex: 0 }), + null, + ); } finally { vi.useRealTimers(); } @@ -1009,8 +1019,15 @@ describe("/api/uploads/:recordingId/chunk route", () => { bytes, { mimeType: "video/webm" }, ); - expect(mockSetResumableSession).toHaveBeenCalledWith( + expect(mockCompareAndSetResumableSession).toHaveBeenCalledWith( "rec-1", + { + providerId: "s3", + sessionId: "sess-1", + meta: { objectKey: "clips/rec-1.webm" }, + bytesUploaded: 100, + lastCommittedIndex: 2, + }, { providerId: "s3", sessionId: "sess-1", @@ -1023,7 +1040,7 @@ describe("/api/uploads/:recordingId/chunk route", () => { expect(mockFinalizeRun).not.toHaveBeenCalled(); }); - it("aborts and surfaces a provider error on the final resumable chunk", async () => { + it("defers destructive cleanup when a final provider call throws", async () => { mockGetResumableSession.mockResolvedValue({ providerId: "s3", sessionId: "sess-final", @@ -1050,20 +1067,50 @@ describe("/api/uploads/:recordingId/chunk route", () => { try { await expect(handler({} as any)).resolves.toEqual({ ok: false, - error: "Final chunk upload failed: S3 staging object read failed (500)", + error: + "Chunk upload outcome is unknown: S3 staging object read failed (500)", + restartRequired: true, }); } finally { consoleError.mockRestore(); } - expect(mockAbortSession).toHaveBeenCalledWith({ - sessionId: "sess-final", - meta: { objectKey: "clips/rec-1.webm" }, - }); - expect(mockDeleteResumableSession).toHaveBeenCalledWith("rec-1", null); + expect(mockAbortSession).not.toHaveBeenCalled(); + expect(mockDeleteResumableSession).not.toHaveBeenCalled(); expect(mockFinalizeRun).not.toHaveBeenCalled(); }); + it("forces a retired-generation restart for an ambiguous ordinary chunk", async () => { + mockGetResumableSession.mockResolvedValue({ + providerId: "s3", + sessionId: "sess-ordinary", + meta: {}, + bytesUploaded: 100, + lastCommittedIndex: 2, + }); + mockRelayChunk.mockRejectedValueOnce(new Error("connection reset")); + setRequest({ + query: { index: "3", mimeType: "video/webm" }, + body: new Uint8Array([1]), + }); + const consoleError = vi + .spyOn(console, "error") + .mockImplementation(() => undefined); + + try { + await expect(handler({} as any)).resolves.toEqual({ + ok: false, + error: "Chunk upload outcome is unknown: connection reset", + restartRequired: true, + }); + } finally { + consoleError.mockRestore(); + } + expect(mockSetResponseStatus).toHaveBeenCalledWith({}, 409); + expect(mockAbortSession).not.toHaveBeenCalled(); + expect(mockDeleteResumableSession).not.toHaveBeenCalled(); + }); + it("acks a replayed resumable chunk without re-uploading to the provider", async () => { mockGetResumableSession.mockResolvedValue({ providerId: "s3", @@ -1101,7 +1148,7 @@ describe("/api/uploads/:recordingId/chunk route", () => { expect(mockFinalizeRun).not.toHaveBeenCalled(); }); - it("retires an expired provider session so the desktop can restart safely", async () => { + it("reports an expired provider session without destroying its live generation", async () => { mockGetResumableSession.mockResolvedValue({ providerId: "s3", sessionId: "expired-session", @@ -1121,10 +1168,251 @@ describe("/api/uploads/:recordingId/chunk route", () => { restartRequired: true, }); expect(mockSetResponseStatus).toHaveBeenCalledWith({}, 409); - expect(mockDeleteResumableSession).toHaveBeenCalledWith("rec-1", null); + expect(mockDeleteResumableSession).not.toHaveBeenCalled(); expect(mockSetResumableSession).not.toHaveBeenCalled(); }); + it("returns stale without cleanup when a failed provider response loses ownership", async () => { + mockGetResumableSession.mockResolvedValue({ + providerId: "s3", + sessionId: "sess-final", + meta: {}, + bytesUploaded: 100, + lastCommittedIndex: 2, + }); + mockRenewUploadLease + .mockResolvedValueOnce({ held: true }) + .mockResolvedValueOnce({ held: true }) + .mockResolvedValueOnce({ held: false, staleAttempt: true }); + mockRelayChunk.mockResolvedValueOnce({ ok: false, status: 500 }); + setRequest({ + query: { + index: "3", + isFinal: "1", + mimeType: "video/webm", + attemptId: "attempt-a", + }, + body: new Uint8Array([1]), + }); + + await expect(handler({} as any)).resolves.toEqual( + expect.objectContaining({ staleAttempt: true }), + ); + expect(mockAbortSession).not.toHaveBeenCalled(); + expect(mockDeleteResumableSession).not.toHaveBeenCalled(); + }); + + it("settles an accepted close sentinel before returning stale ownership", async () => { + vi.useFakeTimers(); + try { + mockGetResumableSession.mockResolvedValue({ + providerId: "s3", + sessionId: "sess-close", + meta: { uploadId: "upload-1" }, + bytesUploaded: 100, + lastCommittedIndex: 2, + }); + mockRenewUploadLease + .mockResolvedValueOnce({ held: true }) + .mockResolvedValueOnce({ held: true }) + .mockResolvedValueOnce({ held: false, staleAttempt: true }); + let finishRelay!: (value: { + ok: boolean; + status: number; + updatedMeta: Record; + }) => void; + mockRelayChunk.mockImplementationOnce( + () => + new Promise((resolve) => { + finishRelay = resolve; + }), + ); + setRequest({ + query: { + index: "3", + isFinal: "1", + mimeType: "video/webm", + attemptId: "attempt-a", + }, + }); + + const pending = handler({} as any); + await vi.advanceTimersByTimeAsync(10_000); + finishRelay({ + ok: true, + status: 200, + updatedMeta: { completedPart: 3 }, + }); + await expect(pending).resolves.toEqual( + expect.objectContaining({ staleAttempt: true }), + ); + expect(mockCompareAndSetResumableSession).toHaveBeenCalledWith( + "rec-1", + expect.objectContaining({ sessionId: "sess-close" }), + expect.objectContaining({ + providerClosed: true, + meta: { uploadId: "upload-1", completedPart: 3 }, + }), + null, + ); + expect(mockFinalizeRun).not.toHaveBeenCalled(); + } finally { + vi.useRealTimers(); + } + }); + + it("forces a retired-generation restart for an ambiguous close sentinel", async () => { + mockGetResumableSession.mockResolvedValue({ + providerId: "s3", + sessionId: "sess-close", + meta: {}, + bytesUploaded: 100, + lastCommittedIndex: 2, + }); + mockRelayChunk.mockRejectedValueOnce( + new Error("response connection closed"), + ); + setRequest({ + query: { index: "3", isFinal: "1", mimeType: "video/webm" }, + }); + const consoleError = vi + .spyOn(console, "error") + .mockImplementation(() => undefined); + + try { + await expect(handler({} as any)).resolves.toEqual({ + ok: false, + error: + "Resumable session close outcome is unknown: response connection closed", + restartRequired: true, + }); + } finally { + consoleError.mockRestore(); + } + expect(mockSetResponseStatus).toHaveBeenCalledWith({}, 409); + expect(mockAbortSession).not.toHaveBeenCalled(); + expect(mockDeleteResumableSession).not.toHaveBeenCalled(); + }); + + it("settles accepted final data before returning stale ownership", async () => { + vi.useFakeTimers(); + try { + mockGetResumableSession.mockResolvedValue({ + providerId: "s3", + sessionId: "sess-final", + meta: { uploadId: "upload-1" }, + bytesUploaded: 100, + lastCommittedIndex: 2, + }); + mockRenewUploadLease + .mockResolvedValueOnce({ held: true }) + .mockResolvedValueOnce({ held: true }) + .mockResolvedValueOnce({ held: false, staleAttempt: true }); + let finishRelay!: (value: { + ok: boolean; + status: number; + updatedMeta: Record; + }) => void; + mockRelayChunk.mockImplementationOnce( + () => + new Promise((resolve) => { + finishRelay = resolve; + }), + ); + setRequest({ + query: { + index: "3", + isFinal: "1", + mimeType: "video/webm", + attemptId: "attempt-a", + }, + body: new Uint8Array([1, 2, 3]), + }); + + const pending = handler({} as any); + await vi.advanceTimersByTimeAsync(10_000); + finishRelay({ + ok: true, + status: 200, + updatedMeta: { completedPart: 3 }, + }); + await expect(pending).resolves.toEqual( + expect.objectContaining({ staleAttempt: true }), + ); + expect(mockCompareAndSetResumableSession).toHaveBeenCalledWith( + "rec-1", + expect.objectContaining({ bytesUploaded: 100 }), + expect.objectContaining({ + bytesUploaded: 103, + lastCommittedIndex: 3, + meta: { uploadId: "upload-1", completedPart: 3 }, + }), + null, + ); + expect(mockFinalizeRun).not.toHaveBeenCalled(); + } finally { + vi.useRealTimers(); + } + }); + + it("accepts an already-reconciled CAS loss for the same session", async () => { + const initial = { + providerId: "s3", + sessionId: "sess-1", + meta: { uploadId: "upload-1" }, + bytesUploaded: 100, + lastCommittedIndex: 2, + }; + mockGetResumableSession + .mockResolvedValueOnce(initial) + .mockResolvedValueOnce({ + ...initial, + meta: { uploadId: "upload-1", completedPart: 3 }, + bytesUploaded: 101, + lastCommittedIndex: 3, + }); + mockCompareAndSetResumableSession.mockResolvedValueOnce(false); + mockRelayChunk.mockResolvedValueOnce({ + ok: true, + status: 308, + updatedMeta: { completedPart: 3 }, + }); + setRequest({ + query: { index: "3", mimeType: "video/webm" }, + body: new Uint8Array([1]), + }); + + await expect(handler({} as any)).resolves.toEqual( + expect.objectContaining({ ok: true, finalized: false }), + ); + }); + + it("forces restart when accepted state contradicts the same stored session", async () => { + const initial = { + providerId: "s3", + sessionId: "sess-1", + meta: { uploadId: "upload-1" }, + bytesUploaded: 100, + lastCommittedIndex: 2, + }; + mockGetResumableSession + .mockResolvedValueOnce(initial) + .mockResolvedValueOnce(initial); + mockCompareAndSetResumableSession.mockResolvedValueOnce(false); + mockRelayChunk.mockResolvedValueOnce({ ok: true, status: 308 }); + setRequest({ + query: { index: "3", mimeType: "video/webm" }, + body: new Uint8Array([1]), + }); + + await expect(handler({} as any)).resolves.toEqual({ + ok: false, + error: "Accepted provider state could not be reconciled safely.", + restartRequired: true, + }); + expect(mockFinalizeRun).not.toHaveBeenCalled(); + }); + it("keeps replacement-generation scratch when a stale writer loses its lease", async () => { (mockSelectRows.rows[0] as Record).uploadGenerationId = "generation-a"; @@ -1196,17 +1484,14 @@ describe("/api/uploads/:recordingId/chunk route", () => { await expect(handler({} as any)).resolves.toEqual( expect.objectContaining({ restartRequired: true }), ); - expect(mockDeleteResumableSession).toHaveBeenCalledWith( - "rec-1", - "generation-a", - ); + expect(mockDeleteResumableSession).not.toHaveBeenCalled(); expect(mockDeleteResumableSession).not.toHaveBeenCalledWith( "rec-1", "generation-b", ); }); - it("does not restore a replacement session after a delayed old provider success", async () => { + it("settles a delayed provider success before returning stale ownership", async () => { (mockSelectRows.rows[0] as Record).uploadGenerationId = "generation-a"; mockGetResumableSession.mockResolvedValue({ @@ -1239,11 +1524,19 @@ describe("/api/uploads/:recordingId/chunk route", () => { await expect(handler({} as any)).resolves.toEqual( expect.objectContaining({ ok: false }), ); - expect(mockSetResumableSession).not.toHaveBeenCalled(); - expect(mockSetResumableSession).not.toHaveBeenCalledWith( + expect(mockCompareAndSetResumableSession).toHaveBeenCalledWith( "rec-1", - expect.anything(), - "generation-b", + expect.objectContaining({ + sessionId: "old-session", + bytesUploaded: 100, + lastCommittedIndex: 0, + }), + expect.objectContaining({ + sessionId: "old-session", + bytesUploaded: 101, + lastCommittedIndex: 1, + }), + "generation-a", ); }); diff --git a/templates/clips/server/routes/api/uploads/[recordingId]/chunk.post.ts b/templates/clips/server/routes/api/uploads/[recordingId]/chunk.post.ts index 568b3215f2..83fe833cad 100644 --- a/templates/clips/server/routes/api/uploads/[recordingId]/chunk.post.ts +++ b/templates/clips/server/routes/api/uploads/[recordingId]/chunk.post.ts @@ -45,12 +45,10 @@ import { ownerEmailMatches, } from "../../../../lib/recordings.js"; import { - deleteResumableSession, + compareAndSetResumableSession, getResumableSession, - setResumableSession, type StoredResumableSession, } from "../../../../lib/resumable-session.js"; -import { abortResumableUploadSession } from "../../../../lib/resumable-upload-cleanup.js"; import { resolveResumableUploadProvider } from "../../../../lib/resumable-upload-provider.js"; import { isStreamingUploadDisabled } from "../../../../lib/streaming-upload-mode.js"; import { @@ -875,21 +873,55 @@ async function handleResumableChunk( `[resumable-chunk-${recordingId}] resumable session exists - bytesUploaded=${session.bytesUploaded} index=${index} isFinal=${isFinal}`, ); - const cleanupFailedFinalSession = async () => { - const cleaned = await abortResumableUploadSession(session, { - provider: uploadProvider, - label: `resumable-final-${recordingId}`, - }); - if (cleaned) { - await deleteResumableSession(recordingId, uploadGenerationId).catch( - (error) => - console.warn( - `[resumable-chunk-${recordingId}] failed to retire aborted session:`, - error, - ), - ); + const settleAcceptedProviderEffect = async ( + next: StoredResumableSession, + ): Promise<"settled" | "superseded" | "contradictory"> => { + if ( + await compareAndSetResumableSession( + recordingId, + session, + next, + uploadGenerationId, + ) + ) { + session = next; + return "settled"; + } + + const current = await getResumableSession(recordingId, uploadGenerationId); + if (!current || current.sessionId !== session.sessionId) { + return "superseded"; } - return !cleaned; + const metaSettled = Object.entries(next.meta).every( + ([key, value]) => + JSON.stringify(current.meta[key]) === JSON.stringify(value), + ); + if ( + current.bytesUploaded >= next.bytesUploaded && + (current.lastCommittedIndex ?? -1) >= (next.lastCommittedIndex ?? -1) && + (!next.providerClosed || current.providerClosed === true) && + metaSettled + ) { + session = current; + return "settled"; + } + return "contradictory"; + }; + + const settlementFailure = (outcome: "superseded" | "contradictory") => { + setResponseStatus(event, 409); + return outcome === "superseded" + ? { + ok: false, + error: + "Upload retry ownership was lost while the provider was responding.", + staleAttempt: true, + } + : { + ok: false, + error: "Accepted provider state could not be reconciled safely.", + restartRequired: true, + }; }; const raw = await readRawBody(event, false); @@ -922,69 +954,95 @@ async function handleResumableChunk( // 0-byte sentinel from the recorder after stop(). All data chunks have // already been PUT to the provider; send Content-Range: bytes */ // to close the session before handing off to finalize-recording. - let closeRes; - try { - const relayed = await relayWithRetryOwnershipHeartbeat( - recordingId, - attemptId, - uploadGenerationId, - () => - uploadProvider.resumable!.relayChunk( - { sessionId: session.sessionId, meta: session.meta }, - `bytes */${session.bytesUploaded}`, - new Uint8Array(0), - ), - ); - if (relayed.ownershipFailure) { + if (session.providerClosed) { + // A prior close response was accepted but its caller lost ownership. + // The durable marker makes replay a no-op before idempotent finalization. + } else { + let closeRes; + try { + const relayed = await relayWithRetryOwnershipHeartbeat( + recordingId, + attemptId, + uploadGenerationId, + () => + uploadProvider.resumable!.relayChunk( + { sessionId: session.sessionId, meta: session.meta }, + `bytes */${session.bytesUploaded}`, + new Uint8Array(0), + ), + ); + closeRes = relayed.result; + if (closeRes.ok && closeRes.status !== 308) { + const settlement = await settleAcceptedProviderEffect({ + ...session, + ...(closeRes.updatedMeta + ? { meta: { ...session.meta, ...closeRes.updatedMeta } } + : {}), + providerClosed: true, + }); + if (settlement !== "settled") return settlementFailure(settlement); + } + if (relayed.ownershipFailure) { + setResponseStatus(event, 409); + return { + ok: false, + error: + "Upload retry ownership was lost while the provider was responding.", + staleAttempt: true, + }; + } + } catch (error) { + const failedCloseLease = await renewUploadLease(recordingId, { + attemptId, + generationId: uploadGenerationId, + }); + if (!failedCloseLease.held) { + setResponseStatus(event, 409); + return { + ok: false, + error: + "Upload retry ownership was lost while the provider was responding.", + staleAttempt: true, + }; + } + const detail = error instanceof Error ? error.message : String(error); + console.error( + `[resumable-chunk-${recordingId}] session close threw:`, + error, + ); setResponseStatus(event, 409); return { ok: false, - error: - "Upload retry ownership was lost while the provider was responding.", - staleAttempt: true, + error: `Resumable session close outcome is unknown: ${detail}`, + restartRequired: true, }; } - closeRes = relayed.result; - } catch (error) { - const failedCloseLease = await renewUploadLease(recordingId, { - attemptId, - generationId: uploadGenerationId, - }); - if (!failedCloseLease.held) { - setResponseStatus(event, 409); + if (!closeRes.ok || closeRes.status === 308) { + const failedCloseLease = await renewUploadLease(recordingId, { + attemptId, + generationId: uploadGenerationId, + }); + if (!failedCloseLease.held) { + setResponseStatus(event, 409); + return { + ok: false, + error: + "Upload retry ownership was lost while the provider was responding.", + staleAttempt: true, + }; + } + console.error( + `[resumable-chunk-${recordingId}] session close failed (${closeRes.status})`, + ); + const restartRequired = + closeRes.status === 404 || closeRes.status === 410; + setResponseStatus(event, restartRequired ? 409 : 502); return { ok: false, - error: - "Upload retry ownership was lost while the provider was responding.", - staleAttempt: true, + error: `Resumable session close failed (${closeRes.status})`, + ...(restartRequired ? { restartRequired: true } : {}), }; } - const cleanupFailed = await cleanupFailedFinalSession(); - const detail = error instanceof Error ? error.message : String(error); - console.error( - `[resumable-chunk-${recordingId}] session close threw:`, - error, - ); - setResponseStatus(event, 502); - return { - ok: false, - error: `Resumable session close failed: ${detail}`, - ...(cleanupFailed ? { cleanupFailed: true } : {}), - }; - } - if (!closeRes.ok || closeRes.status === 308) { - console.error( - `[resumable-chunk-${recordingId}] session close failed (${closeRes.status})`, - ); - const cleanupFailed = await cleanupFailedFinalSession(); - setResponseStatus(event, 502); - return { - ok: false, - error: `Resumable session close failed (${closeRes.status})`, - ...(cleanupFailed ? { cleanupFailed: true } : {}), - }; - } - if (closeRes.updatedMeta) { const postCloseLease = await renewUploadLease(recordingId, { attemptId, generationId: uploadGenerationId, @@ -996,16 +1054,9 @@ async function handleResumableChunk( error: postCloseLease.failureReason ?? "Recording upload has already failed.", + staleAttempt: true, }; } - await setResumableSession( - recordingId, - { - ...session, - meta: { ...session.meta, ...closeRes.updatedMeta }, - }, - uploadGenerationId, - ); } } else { // Idempotent replay guard: a client retry (after a lost response) can @@ -1063,6 +1114,19 @@ async function handleResumableChunk( { mimeType: mimeType.split(";")[0].trim() }, ), ); + putResult = relayed.result; + if (isFinal ? putResult.ok && putResult.status !== 308 : putResult.ok) { + const settlement = await settleAcceptedProviderEffect({ + ...session, + ...(putResult.updatedMeta + ? { meta: { ...session.meta, ...putResult.updatedMeta } } + : {}), + bytesUploaded: start + bytes.byteLength, + lastCommittedIndex: index, + }); + if (settlement !== "settled") return settlementFailure(settlement); + finalizedSourceSizeBytes = start + bytes.byteLength; + } if (relayed.ownershipFailure) { setResponseStatus(event, 409); return { @@ -1072,36 +1136,31 @@ async function handleResumableChunk( staleAttempt: true, }; } - putResult = relayed.result; } catch (error) { - if (isFinal) { - const failedFinalLease = await renewUploadLease(recordingId, { - attemptId, - generationId: uploadGenerationId, - }); - if (!failedFinalLease.held) { - setResponseStatus(event, 409); - return { - ok: false, - error: - "Upload retry ownership was lost while the provider was responding.", - staleAttempt: true, - }; - } - const cleanupFailed = await cleanupFailedFinalSession(); - const detail = error instanceof Error ? error.message : String(error); - console.error( - `[resumable-chunk-${recordingId}] final chunk upload threw:`, - error, - ); - setResponseStatus(event, 502); + const failedUploadLease = await renewUploadLease(recordingId, { + attemptId, + generationId: uploadGenerationId, + }); + if (!failedUploadLease.held) { + setResponseStatus(event, 409); return { ok: false, - error: `Final chunk upload failed: ${detail}`, - ...(cleanupFailed ? { cleanupFailed: true } : {}), + error: + "Upload retry ownership was lost while the provider was responding.", + staleAttempt: true, }; } - throw error; + const detail = error instanceof Error ? error.message : String(error); + console.error( + `[resumable-chunk-${recordingId}] provider response was ambiguous:`, + error, + ); + setResponseStatus(event, 409); + return { + ok: false, + error: `Chunk upload outcome is unknown: ${detail}`, + restartRequired: true, + }; } console.log( `[resumable-chunk-${recordingId}] PUT ${Date.now() - putT0}ms status=${putResult.status} range="${contentRange}"`, @@ -1111,22 +1170,26 @@ async function handleResumableChunk( ? putResult.ok && putResult.status !== 308 : putResult.ok; if (!resultOk) { + const failedUploadLease = await renewUploadLease(recordingId, { + attemptId, + generationId: uploadGenerationId, + }); + if (!failedUploadLease.held) { + setResponseStatus(event, 409); + return { + ok: false, + error: + "Upload retry ownership was lost while the provider was responding.", + staleAttempt: true, + }; + } const restartRequired = putResult.status === 404 || putResult.status === 410; - const cleanupFailed = isFinal - ? await cleanupFailedFinalSession() - : false; - if (restartRequired && !cleanupFailed) { - await deleteResumableSession(recordingId, uploadGenerationId).catch( - () => {}, - ); - } setResponseStatus(event, restartRequired ? 409 : 502); return { ok: false, error: `Chunk upload failed (${putResult.status})`, ...(restartRequired ? { restartRequired: true } : {}), - ...(cleanupFailed ? { cleanupFailed: true } : {}), }; } @@ -1143,20 +1206,6 @@ async function handleResumableChunk( "Recording upload has already failed.", }; } - await setResumableSession( - recordingId, - { - ...session, - ...(putResult.updatedMeta - ? { meta: { ...session.meta, ...putResult.updatedMeta } } - : {}), - bytesUploaded: start + bytes.byteLength, - lastCommittedIndex: index, - }, - uploadGenerationId, - ); - finalizedSourceSizeBytes = start + bytes.byteLength; - if (!isFinal) { return { ok: true, finalized: false, index, bytes: bytes.byteLength }; } From 1e679b30aa2d7bfa799d655e321058bcb6a36b4f Mon Sep 17 00:00:00 2001 From: Alice Alexandra Moore <86723305+3mdistal@users.noreply.github.com> Date: Fri, 21 Aug 2026 16:01:00 -0400 Subject: [PATCH 08/11] chore(clips): format native dependencies --- templates/clips/desktop/src-tauri/Cargo.toml | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/templates/clips/desktop/src-tauri/Cargo.toml b/templates/clips/desktop/src-tauri/Cargo.toml index ae954b534d..b114eb7000 100644 --- a/templates/clips/desktop/src-tauri/Cargo.toml +++ b/templates/clips/desktop/src-tauri/Cargo.toml @@ -67,7 +67,13 @@ reqwest = { version = "0.12", default-features = false, features = [ "rustls-tls", "json", ] } -tokio = { version = "1", features = ["rt", "rt-multi-thread", "time", "sync", "macros"] } +tokio = { version = "1", features = [ + "rt", + "rt-multi-thread", + "time", + "sync", + "macros", +] } chrono = { version = "0.4", features = ["serde"] } # Verify the integrity of the Whisper model we download from HuggingFace. sha2 = "0.10" From bfff842d0762f331bd8c2f85639f1380c1156107 Mon Sep 17 00:00:00 2001 From: Alice Alexandra Moore <86723305+3mdistal@users.noreply.github.com> Date: Mon, 24 Aug 2026 10:47:19 -0400 Subject: [PATCH 09/11] chore: remove clips work plan --- ...hape-pr3345-provider-side-effect-repair.md | 411 ------------------ 1 file changed, 411 deletions(-) delete mode 100644 plans/shape-pr3345-provider-side-effect-repair.md diff --git a/plans/shape-pr3345-provider-side-effect-repair.md b/plans/shape-pr3345-provider-side-effect-repair.md deleted file mode 100644 index fd33414c2f..0000000000 --- a/plans/shape-pr3345-provider-side-effect-repair.md +++ /dev/null @@ -1,411 +0,0 @@ -# PR #3345 provider-side-effect repair Shape - -Status: Work implemented and locally verified; PR refresh and exact-head CI remain pending. Merge and production mutation are not authorized. - -## Summary - -PR [#3345](https://github.com/BuilderIO/agent-native/pull/3345) is still open at exact head -`94e657039753f20d3a0626386201b9dacd593651`. Its ordinary GitHub, targeted test, -desktop-platform, security, build, and preview checks completed without failure. Builder's -exact-head review opened two blocking threads in the resumable chunk route: - -1. a delayed failed provider response may abort or delete a session after the request has - lost the exact attempt/generation lease; and -2. a provider-accepted chunk may be returned as stale before its accepted offset and metadata - are durably reconciled, leaving a later retry able to replay the old range. - -The smallest coherent repair keeps the shared provider contract unchanged. Clips must treat a -provider call as a side effect whose result must be settled exactly once: destructive cleanup -belongs only to a path that has first retired the session's generation, while an accepted result -must be monotonically reconciled into the exact stored session before ownership loss is returned -to the caller. - -## Current evidence - -### Direct facts - -- Exact PR head: `94e657039753f20d3a0626386201b9dacd593651`. -- Current open Builder threads are only: - - `PRRT_kwDORlS_j86bNdZl` / comment `3831642188`, failed-response cleanup without a - current ownership fence; and - - `PRRT_kwDORlS_j86bNdZo` / comment `3831642190`, accepted provider state stranded after - heartbeat ownership loss. -- `handleResumableChunk()` currently performs provider relay under a lease heartbeat, then: - - returns `409` immediately when the heartbeat observed ownership loss; - - advances `bytesUploaded`, `lastCommittedIndex`, and `updatedMeta` only after another lease - renewal succeeds; and - - calls `cleanupFailedFinalSession()` on non-OK final responses without first checking the - exact attempt/generation lease. The same helper aborts the provider session and deletes its - local handle. -- The reset route already owns the safer cleanup seam: it first compare-and-set rotates the - recording to a new generation, claims cleanup of the retired session in application state, - aborts that old provider session, and only then retires the old local handle. -- The shared `FileUploadProvider.resumable` contract returns `ok`, `status`, and optional - `updatedMeta`; it has no cross-provider authoritative-offset query. Builder/GCS returns an - accepted status but no offset metadata, while S3 returns provider metadata required for later - multipart completion. -- Stored resumable session state is already generation-scoped and contains `sessionId`, `meta`, - `bytesUploaded`, and `lastCommittedIndex`. Core application state already provides exact - compare-and-set. -- The older committed-reset cancellation thread `PRRT_kwDORlS_j86bMP7d` is resolved. Current - native code awaits the reset response, retains the returned generation for subsequent work, - observes cancellation only after that response, and excludes the cancellation sentinel from - the generic interrupt path. Builder's latest summary explicitly did not repost it. - -### Inferences - -- A provider's accepted response is the best available cross-provider proof of the exact call's - side effect. Querying the provider later is not a portable first-slice solution. -- An application-state compare-and-set from the exact pre-relay session snapshot to its monotonic - accepted successor can preserve the accepted offset without overwriting a newer session. -- Destructive provider cleanup is safe only after the affected session is no longer a live session - any writer may use. Merely checking a renewable lease immediately before a potentially slow - abort leaves another time-of-check/time-of-use window; rotating the generation first closes it. - -## Causal model and ownership boundary - -There are two authorities, and neither substitutes for the other: - -- The recording row's `(uploadAttemptId, uploadGenerationId, lease)` decides which request may - initiate another provider write or move recording lifecycle state. -- The provider response decides whether the provider side effect from an already-issued request - was accepted. - -Today those authorities are evaluated in the wrong order after relay. If the lease is lost, the -handler returns stale before recording an accepted provider effect. Conversely, if the provider -returns failure, the handler may destroy a session even though that request no longer owns it. - -The repaired boundary is: - -1. **Before dispatch:** require the exact attempt/generation lease. -2. **During dispatch:** heartbeat the same fence, including close/final provider calls. -3. **After an accepted response:** settle the provider effect monotonically against the exact - pre-dispatch session snapshot. This settlement is reconciliation, not permission for more work; - it must occur before returning an observed ownership loss. -4. **After a failed or ambiguous response:** do not destructively abort or delete a still-live - session from the chunk request. Return a typed failure/restart result and let reset rotate the - generation and claim cleanup of the retired session. -5. **After settlement:** only a request that still holds the lease may continue to finalization or - dispatch another provider operation. A stale request stops with a typed `409` after leaving - durable provider/session truth monotonic. - -## Frozen invariants - -### Provider session cleanup - -- A chunk request never aborts or deletes a provider session that remains attached to a live - generation. -- Provider abort and local-handle deletion occur only through an explicit cleanup claim for a - generation already retired by an exact recording compare-and-set. -- Cleanup failure remains loud and recoverable; it cannot be coerced into successful restart or - absence. -- Cleanup of generation A can neither target nor delete generation B's local handle or provider - session. - -### Accepted offset persistence and reconciliation - -- Every accepted data-chunk response advances stored session truth exactly once from the exact - pre-dispatch snapshot, including provider `updatedMeta`. -- Accepted state is monotonic: `bytesUploaded` and `lastCommittedIndex` never move backward, and - a stale settlement never overwrites a different session or a successor already beyond it. -- If the exact compare-and-set loses, the handler rereads session state. It may classify the - effect as already reconciled only when the same session is at or beyond the accepted byte/index - boundary with compatible metadata. Missing, regressed, or contradictory state fails loudly and - forces the safe retired-generation restart path; it is never returned as a normal stale retry. -- Ownership loss prevents further work, but does not discard an accepted provider result. - -### Takeover and retry - -- A different live claim continues to return the typed bounded conflict. -- A stale takeover never reuses an offset from a session with an unsettled or contradictory - provider effect. It retires that session/generation and restarts from a new safe generation. -- Same-claim response-loss retry resumes only from the reconciled committed offset and index. -- Duplicate chunks at or below the reconciled committed index remain acknowledgements without a - second provider write. - -### Cancellation - -- Cancellation stops local replay/upload work and preserves the local backup. -- Cancellation never routes through generic interruption merely because it races a reset response. -- If reset commits before cancellation is observed, the client consumes the authoritative reset - response first. The preserved local attempt can immediately re-enter that server fence on the - next retry; no pre-reset fence is used to interrupt it. -- The resolved committed-reset behavior receives regression coverage in the same verification - packet, but no new native behavior is in the first slice unless that coverage disproves the - current invariant. - -### Finalization - -- Accepted provider state is settled before finalization. -- A stale request never finalizes, aborts, or deletes after ownership loss. -- Ambiguous provider completion remains distinguishable from failure, restart-required, stale - ownership, and ready recording reconciliation. - -## Recommended Work slice - -Keep the repair inside Clips and avoid a shared provider-contract expansion: - -1. Add a generation-scoped resumable-session compare-and-set helper using the existing core - application-state CAS primitive. -2. Refactor the chunk route's provider-result settlement into one boundary used by data chunks, - final data chunks, and the zero-byte close sentinel: - - reconcile accepted offset/index/meta first; - - then stop with typed stale ownership when the heartbeat or post-response lease was lost; and - - continue/finalize only while the exact lease remains held. -3. Remove inline destructive cleanup from failed provider-response paths. Route restart-required - and final-session retirement through the existing reset generation-rotation and cleanup-claim - path. -4. Reuse the same typed stale/restart vocabulary already consumed by browser and native retry - clients. Change client code only if an existing response branch cannot express the safe reset. -5. Add focused deterministic server races and a regression assertion for the already-resolved - native committed-reset cancellation behavior. - -This is one coherent Work slice because all changes enforce one rule: provider effects are settled -against an exact session incarnation, while destruction occurs only after that incarnation is -retired. - -## Compatibility and rollback - -- Keep the existing default-off `uploadRetryResume` feature flag and Alice-only production target. -- Flag-off legacy null-fence uploads remain unfenced and keep their existing full-restart behavior. -- Existing non-null attempt and optional generation fences remain preserved when the flag changes. -- Buffered uploads, non-resumable providers, ordinary first uploads, share URLs, recording schema, - and provider credentials are unchanged. -- No schema migration and no new shared provider method are required. -- Disabling the flag remains the operational rollback for new retries. It does not erase an - already-stored fence or accepted resumable-session state. -- A failed cleanup or contradictory settlement fails closed with the local backup retained; it - does not delete media or pretend the upload restarted. - -## Acceptance story - -### Automated assertions - -Focused deterministic tests must prove: - -1. A non-OK final data response that loses its exact lease performs no provider abort and no local - session deletion, and returns typed stale ownership. -2. A non-OK close-sentinel response under the same race has the same result. -3. A provider failure while ownership remains held returns the existing loud failure/restart - result but defers destruction to reset. -4. Reset first rotates generation, then claims and cleans only the retired session; a concurrent - successor generation is untouched. -5. A provider-accepted ordinary chunk plus heartbeat ownership loss CAS-persists its advanced - bytes, index, and metadata before returning stale. -6. The analogous accepted final-data and close-sentinel paths settle metadata before stale exit - and never finalize after ownership loss. -7. A lost settlement CAS rereads: already-advanced same-session state is accepted as reconciled; - a different/newer session is untouched; regressed or contradictory same-session state fails - loudly and forces safe restart. -8. A same-claim retry resumes at the reconciled byte/index boundary and duplicate replay does not - call the provider. -9. A stale different-attempt takeover cannot consume unsettled state and uses the retired-session - cleanup/reset path. -10. Cancellation after a committed native reset waits for the reset response, skips generic - interruption, preserves the local backup/attempt, and allows immediate same-claim retry. -11. Flag-off legacy null-fence and acknowledged-attempt-with-null-generation cases remain - unchanged. - -### Exact-head verification - -After implementation, bind all evidence to the new exact PR head: - -- focused resume, chunk, reset, abort, upload-lease, browser recovery, and native retry-plan tests; -- Clips desktop TypeScript; -- Rust 1.88 native tests and `cargo fmt`; -- `oxfmt` for every modified TypeScript/TSX file and `git diff --check`; -- repository targeted-workspace tests, lint/format, typecheck, security/static guards, general - build, and all three Clips desktop platform builds; -- a fresh Builder review with zero open actionable threads, plus signed replies/resolutions only - after Work is authorized and fixes exist; -- the required human technical approval on the exact final head. - -### Real-interface and canary evidence - -The concurrency invariants are principally automated; a manual desktop run cannot reliably force -the provider-response/lease interleavings. Real-interface acceptance is still required for the -successful-user story because the product bug is a production desktop retry after network loss. - -- Before merge: a local desktop smoke is preferred, same-context allowed, verifying Retry remains - cancellable and the local backup remains visible. It is not a substitute for the race tests. -- After merge/deploy: repeat the Alice-only production Wi-Fi-interruption canary under - `uploadRetryResume`. The prior canary predates this repair and cannot prove the new exact artifact. - Verify interruption leaves the backup, Retry completes without byte-zero replay or raw conflict, - Cancel retry remains available during active work, and a subsequent retry still succeeds. -- Do not broaden flag targeting until that post-merge canary is recorded against the deployed - revision. - -Acceptance policy: real-interface, independence preferred, same-context allowed, through the signed -Clips desktop application and Alice-only production flag target. Independent technical review is -required because this is provider-side concurrency and destructive-cleanup logic. - -## Explicit non-goals - -- No shared `FileUploadProvider` offset-query API or provider-specific status protocol. -- No schema change, background reconciliation service, upload queue, or new distributed lock. -- No changes to recording/share content, local cache format, authentication, storage credentials, - or production data. -- No broad feature-flag rollout. -- No redesign of the retry banner or cancellation UX. -- No merge, deployment, production mutation, review reply, or thread resolution during Shape. - -## Work evidence - -- Alice explicitly invoked Work against this artifact on 2026-08-21. -- Clips persists accepted data, final-data, and close-sentinel provider effects with an exact generation-scoped session CAS before returning stale ownership. -- Provider failures no longer abort or delete a live resumable session. Ambiguous transport outcomes return the existing typed restart signal so reset retires the generation before replay. -- The resolved native committed-reset cancellation ordering is covered without changing the user-facing flow. -- Focused Vitest: 112 passed. Full Clips Vitest: 1,399 passed. Rust 1.88: 210 passed, 1 intentionally ignored. TypeScript, `oxfmt`, `cargo fmt`, and `git diff --check` pass. -- Independent Terra review found ambiguous data and close response-loss paths plus missing final-data race coverage. All findings were repaired; the bounded one-follow-up ceiling prevented a third ceremonial review turn after the final close-path repair. - -## Architecture grounding and fit - -Grounding is required because this repair crosses the recording lease, persisted session, provider, -reset cleanup, and native cancellation seams. - -- **Demonstrated caller:** signed Clips desktop retrying one locally saved recording after a failed - or interrupted resumable upload. -- **Existing primitives:** exact upload lease CAS, generation-scoped resumable session state, core - application-state CAS, relay heartbeat, reset generation rotation, reset cleanup claim, typed - retry/restart responses, and native cancellation sentinel. -- **Ownership boundaries:** recording row owns write permission; provider response owns knowledge of - an issued side effect; generation-scoped application state owns resumable offset/meta; reset owns - retirement and destructive cleanup; clients own local cancellation and backup custody. -- **Legacy contracts:** flag-off behavior, buffered upload fallback, provider portability, local - backup retention, same-claim lost-response retry, and ready-recording reconciliation remain - unchanged. -- **Smallest compatible delta:** use existing Clips/core CAS and reset cleanup primitives to settle - provider results and retire sessions safely; do not expand the provider interface. -- **Deferred capabilities:** provider offset introspection, generic framework resumable-operation - journals, and cross-template upload orchestration. -- **Reversibility:** bounded source changes behind the existing flag, no migration, and operational - rollback by disabling the flag for new retries. -- **Unresolved owner questions:** none. Current code and review evidence establish the local Clips - boundary without changing a public/shared contract. - -## Architecture fingerprint and lifecycle authority - -```yaml -authoritySchemaVersion: 3 -stage: shape -authority-source: >- - Alice delegated PR #3345 back to Shape for diagnosis only; no implementation, - push, review mutation, production mutation, or merge. -authorized-scope: - repositories: [BuilderIO/agent-native] - product-surfaces: [Clips resumable retry recovery] - outcome: >- - Freeze the smallest repair that settles accepted provider effects and prevents stale - destructive cleanup while preserving retry, cancellation, and feature-flag compatibility. -allowed-mutations: [artifact-write] -write-targets: - artifacts: [plans/shape-pr3345-provider-side-effect-repair.md] -governing-artifact: - path: plans/shape-pr3345-provider-side-effect-repair.md - revision: shape-pr3345-provider-side-effect-repair-r1 -architecture-fingerprint: - outcome: >- - A failed or accepted provider response is reconciled against its exact resumable session; - stale requests cannot destroy live sessions, and retries never replay a provider-accepted range. - shipping-surfaces: - - id: clips-resumable-retry-repair - repository: BuilderIO/agent-native - product-surface: signed Clips desktop resumable retry and hosted Clips upload routes - constituency: Clips users with a locally saved recording whose upload failed or was interrupted - durable-destination: BuilderIO/agent-native main lineage and deployed Clips production - integration-action: merge - governing-architecture: >- - Recording attempt/generation lease gates new work, generation-scoped CAS settles provider - results monotonically, and reset owns cleanup only after retiring the affected generation; - the shared provider contract remains unchanged. - acceptance-story: - id: clips-pr3345-provider-side-effect-repair - summary: >- - After network loss or stale retry ownership, accepted bytes remain resumable, stale requests - cannot abort a successor session, cancellation preserves the local backup, and the Alice-only - production retry completes safely. - required-assertions: - - both current Builder races are deterministically covered and fixed on the exact PR head - - accepted bytes, chunk index, and provider metadata reconcile monotonically before stale exit - - destructive cleanup occurs only after exact generation retirement and cannot touch a successor - - same-claim retry resumes without replay while different-claim takeover safely restarts - - committed-reset cancellation preserves the authoritative fence and local backup - - flag-off legacy and buffered/non-resumable behavior remain unchanged - - focused and full exact-head CI are green with zero actionable review threads - - Alice-only post-merge production Wi-Fi canary succeeds before broader rollout - acceptance-policy: - modality: real-interface - independence: preferred - custody: same-context-allowed - interface: signed Clips desktop plus Alice-only production uploadRetryResume target - rationale: >- - Deterministic tests prove concurrency; the real desktop canary proves the user journey. - Same-context custody is sufficient, while exact-head independent technical review remains required. - risk-strategy: - kind: feature-flagged - production-validation-after-merge: true -architecture-grounding: - applicability: required - reason: Provider side effects cross lease, session, reset-cleanup, and native-client boundaries. - status: grounded - demonstrated-callers: - - signed Clips desktop retrying a locally saved interrupted recording - existing-primitives: - - exact attempt/generation upload lease CAS and heartbeat - - generation-scoped resumable session application state - - core application-state compare-and-set - - reset generation rotation and resumable cleanup claim - - typed browser/native retry, restart, and cancellation flows - ownership-boundaries: - - recording lease authorizes new provider work - - provider response proves the result of already-issued work - - session CAS owns monotonic accepted offset and metadata - - reset owns retired-generation destructive cleanup - - desktop owns local cancellation and backup custody - legacy-contracts: - - flag-off legacy uploads and preserved fences - - buffered and non-resumable uploads - - same-claim response-loss retry and duplicate acknowledgement - - local backup retention and ready-recording reconciliation - shared-vocabulary: - - provider-effect settlement - - retired-generation cleanup - - accepted-offset reconciliation - smallest-compatible-delta: >- - Add a Clips resumable-session CAS settlement helper, use it after provider relay, and route - destructive failure cleanup through existing reset retirement. - deferred-capabilities: - - provider offset query API - - generic framework operation journal - - background reconciliation worker - reversibility: Existing default-off flag, no migration, no shared contract expansion. - direct-evidence: - - PR head 94e657039753f20d3a0626386201b9dacd593651 - - Builder threads PRRT_kwDORlS_j86bNdZl and PRRT_kwDORlS_j86bNdZo - - templates/clips/server/routes/api/uploads/[recordingId]/chunk.post.ts - - templates/clips/server/routes/api/uploads/[recordingId]/reset-chunks.post.ts - - templates/clips/server/lib/resumable-session.ts - - packages/core/src/file-upload/types.ts - - templates/clips/desktop/src-tauri/src/native_screen.rs - inferences: - - exact session CAS can portably settle accepted provider results without a provider query method - unresolved-owner-questions: [] -delegation-ceiling: [read-only] -acceptance-state: - status: pending - summary: >- - Work is not authorized. Current PR head has two open blocking Builder threads; exact-head repair, - technical review, CI, and post-merge Alice-only canary remain required. - blockers: - - explicit Alice /work approval for this exact fingerprint - - implementation and exact-head verification of the frozen assertions - - fresh technical approval and post-merge production canary -ledger-revision: shape-pr3345-provider-side-effect-repair-r1 -status: return-to-shape -``` - -## Approval boundary - -Approval of `/work plans/shape-pr3345-provider-side-effect-repair.md` authorizes only the -bounded Clips repair, focused/full verification, PR push, and review-thread handling described -above. It does not authorize merge, deployment, production mutation, or broader flag rollout. From aa151a8346f8a45c8cb5d63dced2565dde445d4e Mon Sep 17 00:00:00 2001 From: Alice Alexandra Moore <86723305+3mdistal@users.noreply.github.com> Date: Mon, 24 Aug 2026 10:52:21 -0400 Subject: [PATCH 10/11] fix(clips): retire stale retry generations safely --- .../desktop/src-tauri/src/native_screen.rs | 33 ++++++++++++++++--- .../uploads/[recordingId]/resume.get.test.ts | 18 ++++++---- .../api/uploads/[recordingId]/resume.get.ts | 31 ++++++----------- 3 files changed, 49 insertions(+), 33 deletions(-) diff --git a/templates/clips/desktop/src-tauri/src/native_screen.rs b/templates/clips/desktop/src-tauri/src/native_screen.rs index 08a56d13e7..e227c0e4c5 100644 --- a/templates/clips/desktop/src-tauri/src/native_screen.rs +++ b/templates/clips/desktop/src-tauri/src/native_screen.rs @@ -996,6 +996,13 @@ fn clear_native_upload_retry_cancelled(recording_id: &str) { } } +fn take_native_upload_retry_cancelled(recording_id: &str) -> bool { + cancelled_native_upload_retries() + .lock() + .map(|mut cancelled| cancelled.remove(recording_id)) + .unwrap_or(true) +} + async fn wait_for_native_upload_retry_cancel(recording_id: &str) { while !native_upload_retry_cancelled(recording_id) { tokio::time::sleep(Duration::from_millis(100)).await; @@ -3674,7 +3681,10 @@ pub async fn native_fullscreen_recording_retry_upload( auth_token: Option, cookie: Option, ) -> Result { - clear_native_upload_retry_cancelled(&recording_id); + if take_native_upload_retry_cancelled(&recording_id) { + emit_native_upload_progress(&app, "paused", "Retry cancelled", None, None); + return Err(NATIVE_UPLOAD_RETRY_CANCELLED.to_string()); + } let mut saved = read_saved_recording_metadata(&app, &recording_id)?; saved.server_url = server_url.trim_end_matches('/').to_string(); saved.last_attempt_at = Some(now_iso()); @@ -6104,15 +6114,28 @@ fn native_retry_interruption_payload( mod native_retry_upload_plan_tests { use super::{ accept_native_retry_reset, is_native_upload_restart_required, - is_native_upload_unfenced_restart_required, native_replay_attempt_id, - native_retry_attempt_id, native_retry_conflict_delay, native_retry_interruption_payload, - plan_native_retry_upload, preserve_native_retry_fence_during_rollback, - saved_native_retry_attempt_id, upload_url, NativeFullscreenUploadResult, + is_native_upload_unfenced_restart_required, native_fullscreen_recording_cancel_retry, + native_replay_attempt_id, native_retry_attempt_id, native_retry_conflict_delay, + native_retry_interruption_payload, native_upload_retry_cancelled, plan_native_retry_upload, + preserve_native_retry_fence_during_rollback, saved_native_retry_attempt_id, + take_native_upload_retry_cancelled, upload_url, NativeFullscreenUploadResult, NativeRetryUploadPlan, NativeUploadResetResponse, NativeUploadResumeResponse, NATIVE_UPLOAD_RESTART_REQUIRED, NATIVE_UPLOAD_RETRY_CANCELLED, NATIVE_UPLOAD_UNFENCED_RESTART_REQUIRED, UPLOAD_CHUNK_BYTES, }; + #[test] + fn consumes_a_cancellation_that_arrives_before_retry_startup() { + let recording_id = "pre-start-cancel-recording".to_string(); + assert!(!take_native_upload_retry_cancelled(&recording_id)); + + native_fullscreen_recording_cancel_retry(recording_id.clone()) + .expect("record pre-start cancellation"); + assert!(native_upload_retry_cancelled(&recording_id)); + assert!(take_native_upload_retry_cancelled(&recording_id)); + assert!(!native_upload_retry_cancelled(&recording_id)); + } + #[test] fn cancellation_after_a_committed_reset_preserves_the_authoritative_response() { let reset = NativeUploadResetResponse { diff --git a/templates/clips/server/routes/api/uploads/[recordingId]/resume.get.test.ts b/templates/clips/server/routes/api/uploads/[recordingId]/resume.get.test.ts index 9d4d5984cf..e70f7003fd 100644 --- a/templates/clips/server/routes/api/uploads/[recordingId]/resume.get.test.ts +++ b/templates/clips/server/routes/api/uploads/[recordingId]/resume.get.test.ts @@ -54,6 +54,10 @@ vi.mock("@agent-native/core/server", () => ({ runWithRequestContext: (_ctx: unknown, fn: () => unknown) => fn(), })); +vi.mock("node:crypto", () => ({ + randomUUID: () => "generation-2", +})); + vi.mock("drizzle-orm", () => ({ and: vi.fn(() => "and"), eq: vi.fn(() => "eq"), @@ -447,7 +451,7 @@ describe("/api/uploads/:recordingId/resume route", () => { expect.objectContaining({ resumable: true, attemptId: "client-attempt-0001", - uploadGenerationId: "generation-1", + uploadGenerationId: "generation-2", uploadMode: "buffered", bytesReceived: 0, nextChunkIndex: 0, @@ -458,7 +462,7 @@ describe("/api/uploads/:recordingId/resume route", () => { expect.anything(), expect.objectContaining({ uploadAttemptId: "client-attempt-0001", - uploadGenerationId: "generation-1", + uploadGenerationId: "generation-2", }), ); expect(mockAbortResumableUploadSession).toHaveBeenCalledOnce(); @@ -468,7 +472,7 @@ describe("/api/uploads/:recordingId/resume route", () => { ); }); - it("restores an expired claim when its provider session cannot be invalidated", async () => { + it("keeps the replacement generation fenced when retired-session cleanup fails", async () => { mockSelectRows.rows = [ { id: "rec-1", @@ -491,11 +495,11 @@ describe("/api/uploads/:recordingId/resume route", () => { status: "uploading", reason: "stale_provider_session_invalidation_failed", }); - expect(mockUpdateSets).toHaveLength(2); - expect(mockUpdateSets[1]).toEqual( + expect(mockUpdateSets).toHaveLength(1); + expect(mockUpdateSets[0]).toEqual( expect.objectContaining({ - uploadAttemptId: "stale-attempt-0001", - uploadLeaseExpiresAt: "2000-01-01T00:00:00.000Z", + uploadAttemptId: "client-attempt-0001", + uploadGenerationId: "generation-2", }), ); expect(mockDeleteResumableSession).not.toHaveBeenCalled(); diff --git a/templates/clips/server/routes/api/uploads/[recordingId]/resume.get.ts b/templates/clips/server/routes/api/uploads/[recordingId]/resume.get.ts index b07c2a3bc8..eb5949eca4 100644 --- a/templates/clips/server/routes/api/uploads/[recordingId]/resume.get.ts +++ b/templates/clips/server/routes/api/uploads/[recordingId]/resume.get.ts @@ -11,6 +11,8 @@ * Route: GET /api/uploads/:recordingId/resume */ +import { randomUUID } from "node:crypto"; + import { compareAndSetAppState, readAppState, @@ -180,7 +182,7 @@ export default defineEventHandler(async (event: H3Event) => { // Legacy rows keep their null generation and unscoped scratch. A reset // upgrades them by installing a fresh generation before it deletes data. const existingGenerationId = recording.uploadGenerationId ?? null; - const generationId = existingGenerationId; + let generationId = existingGenerationId; let session = generationId ? await getResumableSession(recordingId, generationId) : await getResumableSession(recordingId); @@ -238,6 +240,9 @@ export default defineEventHandler(async (event: H3Event) => { const uploadState = uploadStateRaw ?? {}; const attemptId = requestedAttemptId; const takingOverStaleRetryClaim = differentRetryClaim; + const claimedGenerationId = takingOverStaleRetryClaim + ? randomUUID() + : generationId; const claimedLeaseExpiry = uploadLeaseExpiry(nowMs); const claimed = await getDb() .update(schema.recordings) @@ -245,7 +250,9 @@ export default defineEventHandler(async (event: H3Event) => { status: "uploading", failureReason: null, uploadAttemptId: attemptId, - ...(generationId ? { uploadGenerationId: generationId } : {}), + ...(claimedGenerationId + ? { uploadGenerationId: claimedGenerationId } + : {}), uploadLeaseExpiresAt: claimedLeaseExpiry, updatedAt: now, }) @@ -289,25 +296,6 @@ export default defineEventHandler(async (event: H3Event) => { label: `upload-resume-takeover-${recordingId}`, }); if (!invalidated) { - await getDb() - .update(schema.recordings) - .set({ - uploadAttemptId: existingAttemptId, - uploadLeaseExpiresAt: recording.uploadLeaseExpiresAt, - updatedAt: new Date(claimHeartbeatMs).toISOString(), - }) - .where( - and( - eq(schema.recordings.id, recordingId), - ownerEmailMatches(schema.recordings.ownerEmail, ownerEmail), - eq(schema.recordings.status, "uploading"), - eq(schema.recordings.uploadAttemptId, attemptId), - generationId === null - ? isNull(schema.recordings.uploadGenerationId) - : eq(schema.recordings.uploadGenerationId, generationId), - eq(schema.recordings.uploadLeaseExpiresAt, claimedLeaseExpiry), - ), - ); setResponseStatus(event, 409); return { resumable: false, @@ -320,6 +308,7 @@ export default defineEventHandler(async (event: H3Event) => { await deleteResumableSession(recordingId, generationId); session = null; } + generationId = claimedGenerationId; const uploadStateUpdated = await compareAndSetAppState( uploadStateKey, From 26f49d8589c5e1154e68ba38cca22fa2993e24c4 Mon Sep 17 00:00:00 2001 From: Alice Alexandra Moore <86723305+3mdistal@users.noreply.github.com> Date: Mon, 24 Aug 2026 10:54:34 -0400 Subject: [PATCH 11/11] fix(clips): reconcile authenticated retry banner --- templates/clips/desktop/src/app.tsx | 37 ++++++++++++++++------------- 1 file changed, 20 insertions(+), 17 deletions(-) diff --git a/templates/clips/desktop/src/app.tsx b/templates/clips/desktop/src/app.tsx index 3ce738d48c..b6535aca5e 100644 --- a/templates/clips/desktop/src/app.tsx +++ b/templates/clips/desktop/src/app.tsx @@ -3692,23 +3692,26 @@ export function App() { const showCameraRow = mode !== "screen"; // screen-only has no camera const showSourceRow = mode !== "camera"; // camera-only has no screen source - const pendingUploadBanner = recordingStopFinalizing ? ( - - ) : pendingUploads.length > 0 ? ( - openVideoStorageSetup(upload.serverUrl)} - /> - ) : null; + const pendingUploadBanner = + authStatus === "authed" ? ( + recordingStopFinalizing ? ( + + ) : pendingUploads.length > 0 ? ( + openVideoStorageSetup(upload.serverUrl)} + /> + ) : null + ) : null; async function copyRewindAgentPrompt() { try {