---
phase: 05-files
reviewed: 2026-05-15T00:00:00Z
depth: standard
files_reviewed: 11
files_reviewed_list:
  - backend/cmd/web/main.go
  - backend/compose.yaml
  - backend/go.mod
  - backend/internal/db/queries/files.sql
  - backend/internal/files/store.go
  - backend/internal/web/handlers_files.go
  - backend/internal/web/handlers_tablos.go
  - backend/internal/web/router.go
  - backend/migrations/0005_files.sql
  - backend/templates/files_helpers.go
  - backend/templates/files.templ
  - backend/templates/tablos.templ
findings:
  critical: 1
  warning: 4
  info: 2
  total: 7
status: issues_found
---

# Phase 05: Code Review Report

**Reviewed:** 2026-05-15
**Depth:** standard
**Files Reviewed:** 11
**Status:** issues_found

## Summary

Phase 5 implements file upload, listing, download (presigned URL), and delete for tablo owners. The ownership model is consistently applied via `loadOwnedTablo` / `loadOwnedTabloForFile`, content-type sniffing is server-side, and S3 keys are UUID-based with no user-controlled path component. These are solid foundations.

However, one critical data-integrity bug exists: when a successful S3 upload is followed by a failing DB insert, the uploaded object is silently orphaned with no logging, no cleanup, and no client error that accurately reflects what happened. Four warnings cover the global 15 s `WriteTimeout` killing large-file uploads, an un-validated empty filename stored to the DB, a dead code variable left in production, and a bespoke `itoa` helper that reimplements `strconv.Itoa`. Two informational notes cover the missing `UNIQUE` constraint on `s3_key` and the upload error path not checking `HX-Request` before writing a 422.

---

## Critical Issues

### CR-01: S3 object orphaned silently when DB insert fails after successful upload

**File:** `backend/internal/web/handlers_files.go:178-196`

**Issue:** `deps.Files.Upload` streams the object to S3 successfully (lines 178-183). If the subsequent `deps.Queries.InsertTabloFile` call fails (lines 185-196), the handler logs the DB error and returns HTTP 500 — but the S3 object is never deleted and no log entry records the orphaned key. The caller receives a 500 and believes the upload failed; the file persists in S3 with no DB record and no way to reconcile it (Phase 6 worker does not exist yet). This is a data-integrity bug: storage leaks on every DB error after a successful upload.

**Fix:** Delete the S3 object before returning on DB insert failure. The context may already be cancelled if the DB error was a timeout, so use a fresh background context with a short deadline:

```go
_, err = deps.Queries.InsertTabloFile(r.Context(), sqlc.InsertTabloFileParams{
    TabloID:     tablo.ID,
    S3Key:       s3Key,
    Filename:    header.Filename,
    ContentType: contentType,
    SizeBytes:   bytesWritten,
})
if err != nil {
    slog.Default().Error("files upload: InsertTabloFile failed", "tablo_id", tablo.ID, "s3_key", s3Key, "err", err)
    // Best-effort cleanup — orphan prevention until Phase 6 reconciler exists.
    cleanupCtx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
    defer cancel()
    if delErr := deps.Files.Delete(cleanupCtx, s3Key); delErr != nil {
        slog.Default().Error("files upload: S3 cleanup after DB failure", "s3_key", s3Key, "err", delErr)
    }
    http.Error(w, "internal server error", http.StatusInternalServerError)
    return
}
```

---

## Warnings

### WR-01: Global 15 s `WriteTimeout` will abort large file uploads in flight

**File:** `backend/cmd/web/main.go:122-123`

**Issue:** `http.Server.WriteTimeout` applies to the entire response write, including the request body read that happens during `ParseMultipartForm`. For a 25 MB file on a slow connection (e.g. 2 Mbit/s), reading the body alone takes ~100 s, which exceeds the 15 s timeout. The server will cut the connection mid-upload, leaving the client with a TCP reset and no meaningful error. The `MaxBytesReader` limit (lines 146-147 of `handlers_files.go`) only bounds _size_, not _time_.

**Fix:** Either increase `WriteTimeout` to a value that accommodates `MAX_UPLOAD_SIZE_MB` at a minimum expected upload speed, or (better) set a dedicated per-route timeout for upload endpoints using `http.TimeoutHandler` or by upgrading to `http.Server.WriteTimeout = 0` and relying on `ReadTimeout` + handler-level deadlines:

```go
// Option A: raise to accommodate MAX_UPLOAD_SIZE_MB at worst-case bandwidth.
// 25 MB at 256 KB/s = ~100 s; choose a generous multiple.
WriteTimeout: 120 * time.Second,

// Option B: keep the default low global timeout and wrap upload routes:
// r.With(http.TimeoutHandler(nil, 120*time.Second, "upload timeout")).
// Post("/tablos/{id}/files", FileUploadHandler(fileDeps))
```

---

### WR-02: Empty `header.Filename` is accepted and stored to DB

**File:** `backend/internal/web/handlers_files.go:168-173, 185-192`

**Issue:** `r.FormFile("file")` returns a `*multipart.FileHeader` whose `Filename` field can be an empty string when a client submits a multipart upload without a `filename` parameter in the `Content-Disposition` header (e.g. raw `curl` calls). `header.Filename` is stored directly in `InsertTabloFileParams.Filename` (line 188) without any validation. A DB row is written with an empty `filename`, which the template then renders as a blank filename in the file list, degrading the UI and making the file unidentifiable.

**Fix:** Validate `header.Filename` before the S3 upload:

```go
filename := strings.TrimSpace(header.Filename)
if filename == "" {
    http.Error(w, "bad request: file must have a filename", http.StatusBadRequest)
    return
}
// Use filename (not header.Filename) in InsertTabloFileParams.
```

---

### WR-03: Dead variable `errMsg` left in production handler

**File:** `backend/internal/web/handlers_files.go:159-161`

**Issue:** Lines 159 and 161 declare `errMsg` as a format string and then immediately discard it with `_ = errMsg`. The actual error message is constructed by `formatMBError` on line 160 and passed correctly to `UploadErrorFragment`. The `errMsg` variable is leftover from a draft or earlier approach and is never used. In Go, an unused local variable that is explicitly silenced with `_ =` is dead code and misleading to readers — it suggests the variable serves a purpose.

**Fix:** Delete lines 159 and 161:

```go
// Before:
errMsg := "File too large (max %d MB)."
_ = templates.UploadErrorFragment(tablo, fileList, csrf.Token(r), formatMBError(deps.MaxUploadMB)).Render(r.Context(), w)
_ = errMsg

// After:
_ = templates.UploadErrorFragment(tablo, fileList, csrf.Token(r), formatMBError(deps.MaxUploadMB)).Render(r.Context(), w)
```

---

### WR-04: Custom `itoa` reimplements `strconv.Itoa` without benefit

**File:** `backend/internal/web/handlers_files.go:223-242`

**Issue:** The `itoa` helper (lines 223-242) is a hand-rolled integer-to-string converter. The comment on line 223 states it exists "without importing strconv in this file." `strconv` is a standard library package with zero external dependencies; there is no valid reason to avoid it. The custom implementation introduces a subtle bug: it silently produces an incorrect result for `math.MinInt` (negating `math.MinInt` overflows to itself in two's-complement arithmetic), though this edge case is unreachable given `maxMB` is validated as positive. More importantly, the bespoke code is maintenance surface with no upside.

**Fix:** Remove `itoa` and `formatMBError`, replace with a `strconv.Itoa` call directly in the one call site:

```go
// In FileUploadHandler, at the UploadErrorFragment call:
errMsg := "File too large (max " + strconv.Itoa(deps.MaxUploadMB) + " MB)."
_ = templates.UploadErrorFragment(tablo, fileList, csrf.Token(r), errMsg).Render(r.Context(), w)
```

---

## Info

### IN-01: No `UNIQUE` constraint on `s3_key` in `tablo_files`

**File:** `backend/migrations/0005_files.sql:9`

**Issue:** `s3_key` is declared `NOT NULL` but has no `UNIQUE` constraint. The S3 key is constructed from a `uuid.New()` per upload, making collisions astronomically unlikely in practice. However, the DB provides no hard guarantee. If a duplicate key were somehow inserted (e.g. via a test fixture or a future migration that replays records), `Delete` would remove the S3 object but two DB rows would still reference it, or a re-upload to the same key could silently overwrite existing data.

**Fix:** Add the constraint to the migration:

```sql
s3_key text NOT NULL UNIQUE,
```

---

### IN-02: Upload error path (file-too-large) does not differentiate HTMX vs non-HTMX

**File:** `backend/internal/web/handlers_files.go:151-162`

**Issue:** When `ParseMultipartForm` fails with `*http.MaxBytesError`, the handler always renders the `UploadErrorFragment` HTML with a 422 status, regardless of whether the request came from HTMX or a plain form submit. For a non-HTMX browser submit, this renders a bare fragment (no surrounding layout) into the full page response, producing a broken page. The success path at line 207 correctly branches on `HX-Request`, but the error path at line 151 does not.

**Fix:** Mirror the HTMX branch check used elsewhere:

```go
if errors.As(err, &mbErr) {
    fileList, _ := deps.Queries.ListFilesByTablo(r.Context(), tablo.ID)
    if fileList == nil {
        fileList = []sqlc.TabloFile{}
    }
    errMsg := formatMBError(deps.MaxUploadMB)
    w.Header().Set("Content-Type", "text/html; charset=utf-8")
    w.WriteHeader(http.StatusUnprocessableEntity)
    if r.Header.Get("HX-Request") == "true" {
        _ = templates.UploadErrorFragment(tablo, fileList, csrf.Token(r), errMsg).Render(r.Context(), w)
        return
    }
    // Non-HTMX: render full page with error embedded.
    // (requires passing user through; consider extracting from loadOwnedTablo return)
    http.Redirect(w, r, "/tablos/"+tablo.ID.String()+"/files?err=too_large", http.StatusSeeOther)
    return
}
```

---

_Reviewed: 2026-05-15_
_Reviewer: Claude (gsd-code-reviewer)_
_Depth: standard_
