Silently swallowed errors: unparseable exp yields 401 instead of 400, sidecar write failures logged nowhere #72

New Issue

clawbot · 2026-08-09T03:45:56+02:00

clawbot commented

2026-08-09 03:45:56 +02:00

Verified against main at 61f42e6. Repo rules forbid silently defaulting an invalid explicit value; these are the remaining instances in the request path.

internal/handlers/image.go:43-47 — an unparseable exp query parameter has its strconv error discarded, leaving req.Expires zero. The request then fails as ErrMissingExpiration, so the client gets 401 Unauthorized for what is a malformed request. exp=banana and a genuinely unsigned request are indistinguishable to the caller, which makes integration debugging needlessly hard. Should be a 400 naming the parameter.
internal/imgcache/storage.go:417-420 — the .meta sidecar write failure is discarded with a bare _ = err, no log line. The variant file is then on disk with no sidecar; content type falls back to application/octet-stream on a later read.
internal/imgcache/cache.go:169-172 — the source metadata JSON write failure is likewise discarded with _ = err and no log.
internal/imgcache/cache.go:312-313 — two QueryRowContext results discarded with _ =. The wrong-table problem is #56; noting the swallowing separately, because even after #56 fixes the tables, a failing query would still be invisible.

Cases 2 and 3 are defensible as non-fatal (the DB is authoritative), but "non-fatal" and "invisible" are different things: a disk filling up or a permissions problem currently produces no signal at all. This became more consequential with #51/PR #55, where reconciliation relies on sidecars to recover content types.

Definition of done

Case 1: unparseable exp returns 400 naming the parameter and the offending value, not 401. Failing test first.
Cases 2, 3, 4: every discarded error is logged at warn with enough context to act on (path or key, and the error). Keep them non-fatal; the goal is visibility, not new failure modes.
Sweep for any remaining bare _ = err in non-test code and either handle or log each; state in the PR what was found and why anything left alone is genuinely fine.
make check green (errcheck is already enforced by the canonical config landing in #54 — coordinate ordering, since that PR may touch some of these lines).

Coordination

Case 4 overlaps #56. Cases 2-3 touch files PR #55 changes heavily. Prefer doing this after both PRs land.

Verified against `main` at `61f42e6`. Repo rules forbid silently defaulting an invalid explicit value; these are the remaining instances in the request path. 1. **`internal/handlers/image.go:43-47`** — an unparseable `exp` query parameter has its `strconv` error discarded, leaving `req.Expires` zero. The request then fails as `ErrMissingExpiration`, so the client gets **401 Unauthorized for what is a malformed request**. `exp=banana` and a genuinely unsigned request are indistinguishable to the caller, which makes integration debugging needlessly hard. Should be a 400 naming the parameter. 2. **`internal/imgcache/storage.go:417-420`** — the `.meta` sidecar write failure is discarded with a bare `_ = err`, no log line. The variant file is then on disk with no sidecar; content type falls back to `application/octet-stream` on a later read. 3. **`internal/imgcache/cache.go:169-172`** — the source metadata JSON write failure is likewise discarded with `_ = err` and no log. 4. **`internal/imgcache/cache.go:312-313`** — two `QueryRowContext` results discarded with `_ =`. The wrong-table problem is #56; noting the *swallowing* separately, because even after #56 fixes the tables, a failing query would still be invisible. Cases 2 and 3 are defensible as non-fatal (the DB is authoritative), but "non-fatal" and "invisible" are different things: a disk filling up or a permissions problem currently produces no signal at all. This became more consequential with #51/PR #55, where reconciliation relies on sidecars to recover content types. ## Definition of done 1. Case 1: unparseable `exp` returns 400 naming the parameter and the offending value, not 401. Failing test first. 2. Cases 2, 3, 4: every discarded error is logged at `warn` with enough context to act on (path or key, and the error). Keep them non-fatal; the goal is visibility, not new failure modes. 3. Sweep for any remaining bare `_ = err` in non-test code and either handle or log each; state in the PR what was found and why anything left alone is genuinely fine. 4. `make check` green (`errcheck` is already enforced by the canonical config landing in #54 — coordinate ordering, since that PR may touch some of these lines). ## Coordination Case 4 overlaps #56. Cases 2-3 touch files PR #55 changes heavily. Prefer doing this **after** both PRs land.

clawbot added this to the 1.0.0 milestone 2026-08-09 03:45:56 +02:00

clawbot referenced this issue

2026-08-09 16:10:39 +02:00

Tracking: 1.0.0 sequencing and critical path #103

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/pixa#72