Skip to main content

max / makenotwork

server: make gallery confirm idempotent (STOR-S1) A replayed gallery_confirm (lost-response retry) inserted a second item_images/project_images row for the same s3_key and charged storage twice; recalculate_all_storage_batch SUMs the rows, so the overcharge was baked into reconciliation, not self-healing. Add an in-tx existence check by s3_key (race-safe under the existing gallery advisory lock) that short-circuits to the existing row, plus a UNIQUE index on s3_key (migration 147, de-duping any pre-existing rows first) as the durable backstop. Also clamp the StorageBreakdown total to a saturating sum. Ultra-fuzz Run #23, Storage axis. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Author: Max Johnson <me@maxj.phd> · 2026-06-18 21:31 UTC
Commit: 73fc8c6dea3a7bf6fe0241536315b40517f1c059
Parent: 8828d87
5 files changed, +148 insertions, -4 deletions
A server/migrations/147_gallery_image_s3_key_unique.sql +23
@@ -0,0 +1,23 @@
1 + -- Enforce one gallery row per S3 object (STOR-S1, ultra-fuzz Run #23).
2 + --
3 + -- `gallery_confirm` had no idempotency guard: a replayed confirm (the classic
4 + -- lost-response retry — first confirm committed, client never saw the 200) would
5 + -- insert a second `item_images`/`project_images` row for the SAME `s3_key` and
6 + -- charge storage a second time. The drift is not self-healing: the weekly
7 + -- `recalculate_all_storage_batch` SUMs `file_size_bytes` over the rows, so two
8 + -- rows for one object bake the overcharge into reconciliation.
9 + --
10 + -- `s3_key` already carries a per-image UUID under the entity's gallery prefix, so
11 + -- it is globally unique by construction — one object, one row. De-duplicate any
12 + -- rows that already share a key (keep the physically-earliest), then enforce it.
13 + -- The surviving row keeps the S3 object live (the `S3_KEY_REFS` registry still
14 + -- sees it), so dropping the duplicate orphans nothing and the next storage
15 + -- recalc self-corrects the prior double-charge.
16 +
17 + DELETE FROM item_images a USING item_images b
18 + WHERE a.s3_key = b.s3_key AND a.ctid > b.ctid;
19 + DELETE FROM project_images a USING project_images b
20 + WHERE a.s3_key = b.s3_key AND a.ctid > b.ctid;
21 +
22 + CREATE UNIQUE INDEX IF NOT EXISTS idx_item_images_s3_key ON item_images (s3_key);
23 + CREATE UNIQUE INDEX IF NOT EXISTS idx_project_images_s3_key ON project_images (s3_key);
M server/src/db/creator_tiers.rs +5 -1
@@ -440,7 +440,11 @@ pub async fn get_storage_breakdown(pool: &PgPool, user_id: UserId) -> Result<Sto
440 440 video_bytes: row.4,
441 441 media_bytes: row.5,
442 442 gallery_bytes: row.6,
443 - total_bytes: row.0 + row.1 + row.2 + row.3 + row.4 + row.5 + row.6,
443 + // Each per-category SUM is SQL-clamped to [0, i64::MAX]; sum them
444 + // saturating so seven near-max categories can't overflow the total.
445 + total_bytes: [row.0, row.1, row.2, row.3, row.4, row.5, row.6]
446 + .into_iter()
447 + .fold(0i64, i64::saturating_add),
444 448 })
445 449 }
446 450
M server/src/db/gallery_images.rs +40
@@ -85,6 +85,27 @@ pub async fn insert_for_item<'e>(
85 85 Ok(id)
86 86 }
87 87
88 + /// Look up an item gallery row by its `s3_key` (idempotency guard for confirm:
89 + /// a replayed upload-confirm finds the already-inserted row and short-circuits
90 + /// instead of double-inserting + double-charging storage). Takes an executor so
91 + /// it runs inside the confirm transaction under the gallery advisory lock.
92 + #[tracing::instrument(skip_all)]
93 + pub async fn find_for_item_by_key<'e>(
94 + executor: impl PgExecutor<'e>,
95 + item_id: ItemId,
96 + s3_key: &str,
97 + ) -> Result<Option<GalleryImage>> {
98 + let row = sqlx::query_as::<_, GalleryImage>(
99 + "SELECT id, s3_key, image_url, alt, position, file_size_bytes \
100 + FROM item_images WHERE item_id = $1 AND s3_key = $2",
101 + )
102 + .bind(item_id)
103 + .bind(s3_key)
104 + .fetch_optional(executor)
105 + .await?;
106 + Ok(row)
107 + }
108 +
88 109 /// Delete one item gallery image IF it belongs to an item owned by `user_id`.
89 110 /// Returns the deleted row (for storage decrement + S3 cleanup), or None if it
90 111 /// did not exist or the caller does not own it.
@@ -177,6 +198,25 @@ pub async fn insert_for_project<'e>(
177 198 Ok(id)
178 199 }
179 200
201 + /// Look up a project gallery row by its `s3_key` (confirm idempotency guard —
202 + /// see [`find_for_item_by_key`]).
203 + #[tracing::instrument(skip_all)]
204 + pub async fn find_for_project_by_key<'e>(
205 + executor: impl PgExecutor<'e>,
206 + project_id: ProjectId,
207 + s3_key: &str,
208 + ) -> Result<Option<GalleryImage>> {
209 + let row = sqlx::query_as::<_, GalleryImage>(
210 + "SELECT id, s3_key, image_url, alt, position, file_size_bytes \
211 + FROM project_images WHERE project_id = $1 AND s3_key = $2",
212 + )
213 + .bind(project_id)
214 + .bind(s3_key)
215 + .fetch_optional(executor)
216 + .await?;
217 + Ok(row)
218 + }
219 +
180 220 /// Delete one project gallery image IF the project is owned by `user_id`.
181 221 /// Returns the deleted row, or None if missing / not owned.
182 222 #[tracing::instrument(skip_all)]
M server/src/routes/storage/gallery.rs +41 -3
@@ -195,6 +195,14 @@ pub struct GalleryConfirmResponse {
195 195 pub alt: String,
196 196 }
197 197
198 + /// Outcome of the confirm transaction: a fresh insert, or the pre-existing row
199 + /// found on a replayed confirm (STOR-S1 idempotency — no second insert, no
200 + /// second storage charge).
201 + enum GalleryOutcome {
202 + Inserted(Uuid),
203 + Existing(db::gallery_images::GalleryImage),
204 + }
205 +
198 206 /// POST /api/gallery/confirm — finalize a gallery image upload.
199 207 #[tracing::instrument(skip_all, name = "storage::gallery_confirm", fields(user_id = %user.id))]
200 208 pub(super) async fn gallery_confirm(
@@ -281,7 +289,9 @@ pub(super) async fn gallery_confirm(
281 289
282 290 // Storage increment + row INSERT in ONE transaction (gallery is add-only, so
283 291 // a pure increment — no old-key replacement). A rollback restores the counter.
284 - let committed: Result<Uuid> = async {
292 + // Returns the existing row on a replayed confirm (idempotent, no re-charge)
293 + // or the freshly-inserted id otherwise.
294 + let committed: Result<GalleryOutcome> = async {
285 295 let mut tx = state.db.begin().await?;
286 296 // Serialize concurrent confirms for this gallery and re-count INSIDE the
287 297 // tx, so two inserts that both passed the best-effort pre-check above
@@ -292,6 +302,23 @@ pub(super) async fn gallery_confirm(
292 302 .bind(gallery_lock_obj)
293 303 .execute(&mut *tx)
294 304 .await?;
305 + // Idempotency guard (STOR-S1): a replayed confirm for an s3_key already
306 + // recorded must not insert a second row or charge storage again. The
307 + // advisory lock above serializes confirms for this gallery, so this
308 + // check-then-insert is race-safe; the UNIQUE index on s3_key (migration
309 + // 147) is the durable backstop against any cross-process gap.
310 + let existing = match target {
311 + GalleryTarget::Item => {
312 + db::gallery_images::find_for_item_by_key(&mut *tx, ItemId::from(req.target_id), &req.s3_key).await?
313 + }
314 + GalleryTarget::Project => {
315 + db::gallery_images::find_for_project_by_key(&mut *tx, ProjectId::from(req.target_id), &req.s3_key)
316 + .await?
317 + }
318 + };
319 + if let Some(img) = existing {
320 + return Ok(GalleryOutcome::Existing(img));
321 + }
295 322 let count_in_tx = match target {
296 323 GalleryTarget::Item => db::gallery_images::count_for_item(&mut *tx, ItemId::from(req.target_id)).await?,
297 324 GalleryTarget::Project => db::gallery_images::count_for_project(&mut *tx, ProjectId::from(req.target_id)).await?,
@@ -318,12 +345,23 @@ pub(super) async fn gallery_confirm(
318 345 }
319 346 };
320 347 tx.commit().await?;
321 - Ok(id)
348 + Ok(GalleryOutcome::Inserted(id))
322 349 }
323 350 .await;
324 351
325 352 let id = match committed {
326 - Ok(id) => id,
353 + // Replayed confirm: the object is already a live, recorded gallery row —
354 + // nothing was charged or inserted, so do NOT orphan-enqueue (that key is
355 + // in use). Return the existing row so the client sees the same success.
356 + Ok(GalleryOutcome::Existing(img)) => {
357 + return Ok(Json(GalleryConfirmResponse {
358 + success: true,
359 + id: img.id,
360 + image_url: img.image_url,
361 + alt: img.alt,
362 + }));
363 + }
364 + Ok(GalleryOutcome::Inserted(id)) => id,
327 365 Err(e) => {
328 366 super::enqueue_s3_orphan(&state.db, &req.s3_key, "gallery_image_insert_failed").await;
329 367 return Err(e);
M server/tests/workflows/gallery.rs +39
@@ -90,6 +90,45 @@ async fn gallery_confirm_inserts_row_and_charges_storage() {
90 90 }
91 91
92 92 #[tokio::test]
93 + async fn gallery_confirm_replay_is_idempotent() {
94 + // STOR-S1 (Run #23): a replayed confirm for an already-recorded s3_key (the
95 + // classic lost-response retry) must NOT insert a second row or charge storage
96 + // again — it returns the existing row.
97 + let mut h = TestHarness::with_storage().await;
98 + let (user_id, _, item_id) = setup_creator_with_item(&mut h).await;
99 +
100 + let (id, s3_key) = add_gallery_image(&mut h, "item", &item_id, "shot.png", vec![0u8; 1234]).await;
101 + assert_eq!(storage_used(&h, &user_id).await, 1234);
102 +
103 + // Replay the exact same confirm body.
104 + let resp = h
105 + .client
106 + .post_json(
107 + "/api/gallery/confirm",
108 + &json!({
109 + "target_type": "item",
110 + "target_id": item_id,
111 + "s3_key": s3_key,
112 + "alt": "a descriptive caption",
113 + })
114 + .to_string(),
115 + )
116 + .await;
117 + assert!(resp.status.is_success(), "replayed confirm should succeed idempotently: {}", resp.text);
118 + let data: Value = resp.json();
119 + assert_eq!(data["success"], true);
120 + assert_eq!(data["id"].as_str().unwrap(), id, "replay returns the same row id");
121 +
122 + let count: i64 = sqlx::query_scalar("SELECT COUNT(*) FROM item_images WHERE item_id = $1::uuid")
123 + .bind(&item_id)
124 + .fetch_one(&h.db)
125 + .await
126 + .unwrap();
127 + assert_eq!(count, 1, "replay does not insert a second row");
128 + assert_eq!(storage_used(&h, &user_id).await, 1234, "replay does not double-charge storage");
129 + }
130 +
131 + #[tokio::test]
93 132 async fn gallery_list_returns_in_insertion_order() {
94 133 let mut h = TestHarness::with_storage().await;
95 134 let (_, _, item_id) = setup_creator_with_item(&mut h).await;