Skip to main content

max / makenotwork

Harden the OTA path and stop orphaning S3 on failed cleanup enqueue (ultra-fuzz Run 12 Storage) The OTA artifact path never got the presign->confirm discipline the rest of storage enforces, and three route-handler deletes warned-and-proceeded when their sole durable S3-cleanup enqueue failed. - OTA F1: create_artifact upserts on (release_id, target, arch) so re-uploading an artifact (fix a bad binary, retry a partial upload) overwrites in place instead of raising 23505 -> 500. - OTA F2: artifact_download HEAD-verifies the object exists before presigning, so an abandoned upload 404s cleanly instead of redirecting the updater to a URL that 404s at S3. - S-B: delete_project, delete_release, and delete_insertion now abort if the S3 deletion enqueue fails, rather than deleting the rows and orphaning every object. The reaper's is_s3_key_live guard makes enqueue-before-delete safe. - 23505 -> Conflict: a shared helpers::map_unique_violation turns raw 500s on legitimate retries into clean 409s at three create paths (custom domains, passkeys); the internal git auto-create now re-resolves on a concurrent-push unique violation, mirroring the smart-HTTP path. - confirm_insertion is idempotent: a replayed confirm returns the existing row instead of double-charging storage and inserting a duplicate. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Author: Max Johnson <me@maxj.phd> · 2026-07-01 14:14 UTC
Commit: 7a8c406dccf866b5de55e6b557049b3c835b5e52
Parent: 44dc82f
9 files changed, +111 insertions, -29 deletions
M server/src/db/content_insertions.rs +22
@@ -73,6 +73,28 @@ pub async fn get_insertion(
73 73 Ok(row)
74 74 }
75 75
76 + /// Get an insertion by its (user-scoped, deterministic) S3 storage key. Used to
77 + /// make `confirm_insertion` idempotent: the key is `{user}/insertions/{filename}`
78 + /// with no UUID and `storage_key` carries no UNIQUE, so a replayed confirm would
79 + /// otherwise re-charge storage and insert a duplicate row (ultra-fuzz Run 12
80 + /// Storage: confirm idempotency).
81 + #[tracing::instrument(skip_all)]
82 + pub async fn get_insertion_by_storage_key(
83 + pool: &PgPool,
84 + user_id: UserId,
85 + storage_key: &str,
86 + ) -> Result<Option<DbContentInsertion>> {
87 + let row = sqlx::query_as::<_, DbContentInsertion>(
88 + "SELECT * FROM content_insertions WHERE user_id = $1 AND storage_key = $2",
89 + )
90 + .bind(user_id)
91 + .bind(storage_key)
92 + .fetch_optional(pool)
93 + .await?;
94 +
95 + Ok(row)
96 + }
97 +
76 98 /// Rename an insertion clip. Returns true if the row was found and updated.
77 99 #[tracing::instrument(skip_all)]
78 100 pub async fn update_insertion_title(
M server/src/db/ota.rs +9 -1
@@ -175,7 +175,13 @@ pub async fn get_release_artifact_keys(
175 175
176 176 // ── Artifacts ──
177 177
178 - /// Create an artifact record for a release.
178 + /// Create or replace an artifact record for a release.
179 + ///
180 + /// The S3 key is deterministic (`ota/{app}/{version}/{target}/{arch}/artifact`) and
181 + /// the object overwrites in place, so re-uploading the same artifact (fixing a bad
182 + /// binary, retrying a half-finished upload) must succeed. Upsert on the
183 + /// `(release_id, target, arch)` unique key rather than raising 23505 -> 500 on the
184 + /// second upload (ultra-fuzz Run 12 Storage F1).
179 185 #[tracing::instrument(skip_all)]
180 186 pub async fn create_artifact(
181 187 pool: &PgPool,
@@ -189,6 +195,8 @@ pub async fn create_artifact(
189 195 r#"
190 196 INSERT INTO ota_artifacts (release_id, target, arch, s3_key, file_size)
191 197 VALUES ($1, $2, $3, $4, $5)
198 + ON CONFLICT (release_id, target, arch)
199 + DO UPDATE SET s3_key = EXCLUDED.s3_key, file_size = EXCLUDED.file_size
192 200 RETURNING *
193 201 "#,
194 202 )
M server/src/helpers.rs +14
@@ -49,6 +49,20 @@ pub fn is_unique_violation(e: &crate::error::AppError) -> bool {
49 49 )
50 50 }
51 51
52 + /// Map a Postgres unique-violation (23505) into a clean `AppError::Conflict(msg)`;
53 + /// pass any other error through unchanged. A create/insert on a deterministic or
54 + /// user-supplied unique key that legitimately re-runs (a retry, a duplicate the
55 + /// user can fix) should surface a 409 with a helpful message, not a raw 500
56 + /// (ultra-fuzz Run 12 Storage: unhandled 23505 on lower-traffic create paths).
57 + /// Use only where the INSERT has a single relevant unique constraint.
58 + pub fn map_unique_violation(e: crate::error::AppError, msg: &str) -> crate::error::AppError {
59 + if is_unique_violation(&e) {
60 + crate::error::AppError::Conflict(msg.to_string())
61 + } else {
62 + e
63 + }
64 + }
65 +
52 66 /// Insert a row whose slug must be unique under a per-parent `UNIQUE` index,
53 67 /// auto-suffixing the slug (`base`, `base-2`, `base-3`, ...) on collision.
54 68 ///
M server/src/routes/api/content_insertions.rs +23 -8
@@ -130,6 +130,22 @@ pub(super) async fn confirm_insertion(
130 130 ));
131 131 }
132 132
133 + // Idempotent replay: a retried confirm for the same (deterministic) key must not
134 + // re-charge storage or insert a duplicate row (`storage_key` has no UNIQUE).
135 + // Return the already-created insertion (ultra-fuzz Run 12 Storage: confirm
136 + // idempotency).
137 + if let Some(existing) =
138 + db::content_insertions::get_insertion_by_storage_key(&state.db, user.id, &req.s3_key).await?
139 + {
140 + return Ok(Json(InsertionResponse {
141 + id: existing.id,
142 + title: existing.title,
143 + media_type: existing.media_type,
144 + duration_ms: existing.duration_ms,
145 + file_size: existing.file_size,
146 + }));
147 + }
148 +
133 149 // Get real file size from S3 (never trust client-provided size)
134 150 let file_size_bytes = s3.object_size(&req.s3_key).await?.ok_or_else(|| {
135 151 AppError::BadRequest("Upload not found. Please try uploading again.".to_string())
@@ -285,18 +301,17 @@ pub(super) async fn delete_insertion(
285 301 let insertion = db::content_insertions::get_insertion(&state.db, id, user.id).await?;
286 302 let file_size = insertion.as_ref().map(|i| i.file_size).unwrap_or(0);
287 303
288 - // Enqueue as a durable safety net
289 - if let Some(ref ins) = insertion
290 - && let Err(e) = db::pending_s3_deletions::enqueue_deletions(
304 + // Enqueue as the sole durable deletion path, BEFORE the row delete. Abort on
305 + // enqueue failure rather than warn-and-proceed: deleting the row anyway would
306 + // orphan the clip's S3 object with no record (ultra-fuzz Run 12 Storage F3).
307 + // The reaper's is_s3_key_live guard makes the reverse case safe.
308 + if let Some(ref ins) = insertion {
309 + db::pending_s3_deletions::enqueue_deletions(
291 310 &state.db,
292 311 &[(ins.storage_key.clone(), "main".to_string())],
293 312 "insertion_delete",
294 - ).await
295 - {
296 - tracing::warn!(error = ?e, "failed to enqueue S3 deletion for insertion");
313 + ).await?;
297 314 }
298 - // The durable queue entry above is the sole deletion path; the queue worker
299 - // (the only sanctioned S3 deleter) performs the actual delete.
300 315
301 316 let deleted = db::content_insertions::delete_insertion(&state.db, id, user.id).await?;
302 317
M server/src/routes/api/domains.rs +5 -1
@@ -34,9 +34,13 @@ pub(super) async fn add_domain(
34 34
35 35 let verification_token = generate_verification_token();
36 36
37 + // The per-user 1-domain limit is pre-checked in `create_custom_domain`; the
38 + // durable `UNIQUE (domain)` is global, so a domain another user already holds
39 + // surfaces here as a clean 409, not a raw 500 (ultra-fuzz Run 12 Storage).
37 40 let _row =
38 41 db::custom_domains::create_custom_domain(&state.db, session_user.id, &domain, &verification_token)
39 - .await?;
42 + .await
43 + .map_err(|e| crate::helpers::map_unique_violation(e, "That domain is already registered"))?;
40 44
41 45 // Escape interpolated values even though `validate_domain` restricts the
42 46 // domain to `[a-z0-9-]` and the token is server-generated: this is the one
M server/src/routes/api/internal/git.rs +12 -1
@@ -150,7 +150,18 @@ pub(super) async fn git_authorize(
150 150 }
151 151
152 152 tracing::info!(owner = %req.owner, repo = %req.repo_name, "registering new repository");
153 - db::git_repos::create_repo(&state.db, owner_user.id, &req.repo_name).await?
153 + // Concurrent double-push can race two auto-registers; on the loser's
154 + // unique violation, re-resolve instead of 500ing the git client — the
155 + // same pattern the smart-HTTP path uses (ultra-fuzz Run 12 Storage).
156 + match db::git_repos::create_repo(&state.db, owner_user.id, &req.repo_name).await {
157 + Ok(r) => r,
158 + Err(e) => {
159 + tracing::debug!(owner = %req.owner, repo = %req.repo_name, error = ?e, "auto-register failed, retrying lookup");
160 + db::git_repos::get_repo_by_user_and_name(&state.db, owner_user.id, &req.repo_name)
161 + .await?
162 + .ok_or(AppError::NotFound)?
163 + }
164 + }
154 165 }
155 166 };
156 167
M server/src/routes/api/passkeys.rs +2 -1
@@ -120,7 +120,8 @@ pub(super) async fn register_finish(
120 120 let credential_id = passkey.cred_id().to_vec();
121 121
122 122 db::passkeys::create_passkey(&state.db, user.id, "Passkey", &credential_json, &credential_id)
123 - .await?;
123 + .await
124 + .map_err(|e| crate::helpers::map_unique_violation(e, "This passkey is already registered"))?;
124 125
125 126 tracing::info!(user_id = %user.id, event = "passkey_registered", "Passkey registered");
126 127
M server/src/routes/api/projects.rs +7 -10
@@ -382,16 +382,13 @@ pub(super) async fn delete_project(
382 382 all_keys.push((key, "main".to_string()));
383 383 }
384 384
385 - // Enqueue for durable S3 deletion (survives crashes)
386 - if let Err(e) = db::pending_s3_deletions::enqueue_deletions(
387 - &state.db,
388 - &all_keys,
389 - "project_delete",
390 - )
391 - .await
392 - {
393 - tracing::warn!(error = ?e, "failed to enqueue S3 deletions for project");
394 - }
385 + // Enqueue for durable S3 deletion (survives crashes) BEFORE the CASCADE delete.
386 + // Abort on failure rather than warn-and-proceed: enqueue is the sole durable
387 + // deletion path for the whole project's item/version/gallery/cover keys, so
388 + // deleting the rows anyway would orphan every object with no record (ultra-fuzz
389 + // Run 12 Storage F3 sibling). The reverse case (enqueue succeeds, delete fails)
390 + // is safe — the reaper's is_s3_key_live guard skips keys whose rows still exist.
391 + db::pending_s3_deletions::enqueue_deletions(&state.db, &all_keys, "project_delete").await?;
395 392
396 393 // Decrement storage before deleting rows
397 394 let storage_bytes = db::items::get_project_storage_bytes(&state.db, id).await?;
M server/src/routes/ota.rs +17 -7
@@ -248,14 +248,14 @@ async fn delete_release_handler(
248 248 .await?
249 249 .ok_or(AppError::NotFound)?;
250 250
251 - // Enqueue keys as a durable safety net
251 + // Enqueue keys as the sole durable deletion path, BEFORE the CASCADE delete.
252 + // Abort on enqueue failure rather than warn-and-proceed: deleting the rows
253 + // anyway would orphan every artifact object in the synckit bucket with no
254 + // record (ultra-fuzz Run 12 Storage F3). The queue worker is the only
255 + // sanctioned S3 deleter, and its is_s3_key_live guard makes the reverse case
256 + // (enqueue succeeds, delete fails) safe.
252 257 let enqueue_keys: Vec<(String, String)> = s3_keys.iter().map(|k| (k.clone(), "synckit".to_string())).collect();
253 - if let Err(e) = db::pending_s3_deletions::enqueue_deletions(&state.db, &enqueue_keys, "delete_release").await {
254 - tracing::warn!(error = ?e, "failed to enqueue S3 deletions for release artifacts");
255 - }
256 -
257 - // The durable queue entries above are the sole deletion path; the queue
258 - // worker (the only sanctioned S3 deleter) removes the artifacts.
258 + db::pending_s3_deletions::enqueue_deletions(&state.db, &enqueue_keys, "delete_release").await?;
259 259
260 260 db::ota::delete_release(&state.db, release_id).await?;
261 261
@@ -402,6 +402,16 @@ async fn artifact_download(
402 402
403 403 let synckit_s3 = state.require_synckit_s3()?;
404 404
405 + // The artifact row is written at presign time (before the client PUTs the
406 + // object), so an abandoned upload leaves a row pointing at an object that never
407 + // landed. Verify the object exists before handing out a presigned URL: 404
408 + // cleanly here rather than redirecting the updater to a URL that 404s at S3
409 + // (ultra-fuzz Run 12 Storage F2).
410 + if !synckit_s3.object_exists(&artifact.s3_key).await? {
411 + tracing::warn!(%release_id, %target, %arch, "OTA artifact row present but object missing (abandoned upload?)");
412 + return Err(AppError::NotFound);
413 + }
414 +
405 415 let download_url = synckit_s3
406 416 .presign_download(&crate::storage::S3Key::from_stored(&artifact.s3_key), Some(constants::OTA_PRESIGN_EXPIRY_SECS))
407 417 .await?;