diff --git a/.planning/phases/05-files/05-REVIEW-FIX.md b/.planning/phases/05-files/05-REVIEW-FIX.md new file mode 100644 index 0000000..7d036bc --- /dev/null +++ b/.planning/phases/05-files/05-REVIEW-FIX.md @@ -0,0 +1,59 @@ +--- +phase: 05-files +fixed_at: 2026-05-15T00:00:00Z +review_path: .planning/phases/05-files/05-REVIEW.md +iteration: 1 +findings_in_scope: 5 +fixed: 5 +skipped: 0 +status: all_fixed +--- + +# Phase 05: Code Review Fix Report + +**Fixed at:** 2026-05-15 +**Source review:** .planning/phases/05-files/05-REVIEW.md +**Iteration:** 1 + +**Summary:** +- Findings in scope: 5 (Critical + Warning; IN-01 and IN-02 excluded per fix scope) +- Fixed: 5 +- Skipped: 0 + +## Fixed Issues + +### CR-01: S3 object orphaned silently when DB insert fails after successful upload + +**Files modified:** `backend/internal/web/handlers_files.go` +**Commit:** 690ea2d +**Applied fix:** After `deps.Files.Upload` succeeds, if `deps.Queries.InsertTabloFile` fails, a background context with 10s timeout is created and `deps.Files.Delete` is called to clean up the S3 object. Both the DB error and any S3 cleanup error are logged. The 500 response is returned after the cleanup attempt. + +### WR-01: Global 15s WriteTimeout will abort large file uploads in flight + +**Files modified:** `backend/cmd/web/main.go` +**Commit:** 49e84c8 +**Applied fix:** Raised both `ReadTimeout` and `WriteTimeout` from 15s to 120s. A 25MB upload at ~256 KB/s takes ~100s; 120s provides comfortable headroom. Added inline comment explaining the rationale (MaxBytesReader bounds size not time; WriteTimeout covers the full request lifecycle). + +### WR-02: Empty `header.Filename` is accepted and stored to DB + +**Files modified:** `backend/internal/web/handlers_files.go` +**Commit:** 690ea2d +**Applied fix:** Added `filename := strings.TrimSpace(header.Filename)` immediately after `r.FormFile("file")`. If `filename` is empty, returns 400 before any S3 operation. `filename` (trimmed) is used in `InsertTabloFileParams` instead of `header.Filename`. `strings` import added. + +### WR-03: Dead variable `errMsg` left in production handler + +**Files modified:** `backend/internal/web/handlers_files.go` +**Commit:** 690ea2d +**Applied fix:** Removed the dead `errMsg := "File too large (max %d MB)."` declaration and the `_ = errMsg` silencer. The `UploadErrorFragment` call now uses a locally constructed `errMsg` string built with `strconv.Itoa` (see WR-04 below). + +### WR-04: Custom `itoa` reimplements `strconv.Itoa` without benefit + +**Files modified:** `backend/internal/web/handlers_files.go` +**Commit:** 690ea2d +**Applied fix:** Deleted the `itoa` and `formatMBError` helper functions entirely. Their single call site now builds the error string inline: `errMsg := "File too large (max " + strconv.Itoa(deps.MaxUploadMB) + " MB)."`. `strconv` import added. + +--- + +_Fixed: 2026-05-15_ +_Fixer: Claude (gsd-code-fixer)_ +_Iteration: 1_