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
Plan (repo-manager). Re-checked against next at f149813; the issue still holds, at new places:
Case 1: internal/handlers/image.goparseImageRequest, the exp block. An exp that is present but not a whole number becomes a 400 naming exp and the value, as the route now does for q and fit (#134, #139). An empty exp= is present, so it is a 400 too; only an exp missing from the URL keeps today's behavior.
Case 2: internal/imgcache/storage.go, the .meta sidecar os.WriteFile. Case 3: internal/imgcache/cache.go, c.srcMetadata.Store. Case 4: cache.goStats, the two QueryRowContext scans. Each is logged at warn with the path or key and the error and stays non-fatal. Which tables Stats reads stays with #56.
Sweep: the two ExecContext results discarded in cache.go are logged the same way. The Close/Remove cleanups on paths already returning an error stay as they are; the PR says so in one line.
The coordination notes above are out of date: the PRs they name have landed.
Model: opus-5-5
Plan (repo-manager). Re-checked against `next` at `f149813`; the issue still holds, at new places:
- Case 1: `internal/handlers/image.go` `parseImageRequest`, the `exp` block. An `exp` that is present but not a whole number becomes a 400 naming `exp` and the value, as the route now does for `q` and `fit` (https://git.eeqj.de/sneak/pixa/issues/134, https://git.eeqj.de/sneak/pixa/issues/139). An empty `exp=` is present, so it is a 400 too; only an `exp` missing from the URL keeps today's behavior.
- Case 2: `internal/imgcache/storage.go`, the `.meta` sidecar `os.WriteFile`. Case 3: `internal/imgcache/cache.go`, `c.srcMetadata.Store`. Case 4: `cache.go` `Stats`, the two `QueryRowContext` scans. Each is logged at `warn` with the path or key and the error and stays non-fatal. Which tables `Stats` reads stays with https://git.eeqj.de/sneak/pixa/issues/56.
- Sweep: the two `ExecContext` results discarded in `cache.go` are logged the same way. The `Close`/`Remove` cleanups on paths already returning an error stay as they are; the PR says so in one line.
The coordination notes above are out of date: the PRs they name have landed.
Model: opus-5-5
clawbot
self-assigned this 2026-09-28 19:10:23 +02:00
Built in #141: an exp that is not a whole number, an empty exp= included, is now a 400 naming exp and the value; the discarded cache errors are logged at warn and stay non-fatal. Beyond the plan's list, the negative cache write in service.go is logged too.
Model: opus-5-5
Built in https://git.eeqj.de/sneak/pixa/pulls/141: an `exp` that is not a whole number, an empty `exp=` included, is now a 400 naming `exp` and the value; the discarded cache errors are logged at `warn` and stay non-fatal. Beyond the plan's list, the negative cache write in `service.go` is logged too.
Model: opus-5-5
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Verified against
mainat61f42e6. 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 unparseableexpquery parameter has itsstrconverror discarded, leavingreq.Expireszero. The request then fails asErrMissingExpiration, so the client gets 401 Unauthorized for what is a malformed request.exp=bananaand 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.metasidecar 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 toapplication/octet-streamon a later read.internal/imgcache/cache.go:169-172— the source metadata JSON write failure is likewise discarded with_ = errand no log.internal/imgcache/cache.go:312-313— twoQueryRowContextresults 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
expreturns 400 naming the parameter and the offending value, not 401. Failing test first.warnwith enough context to act on (path or key, and the error). Keep them non-fatal; the goal is visibility, not new failure modes._ = errin 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 checkgreen (errcheckis 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.
Plan (repo-manager). Re-checked against
nextatf149813; the issue still holds, at new places:internal/handlers/image.goparseImageRequest, theexpblock. Anexpthat is present but not a whole number becomes a 400 namingexpand the value, as the route now does forqandfit(#134, #139). An emptyexp=is present, so it is a 400 too; only anexpmissing from the URL keeps today's behavior.internal/imgcache/storage.go, the.metasidecaros.WriteFile. Case 3:internal/imgcache/cache.go,c.srcMetadata.Store. Case 4:cache.goStats, the twoQueryRowContextscans. Each is logged atwarnwith the path or key and the error and stays non-fatal. Which tablesStatsreads stays with #56.ExecContextresults discarded incache.goare logged the same way. TheClose/Removecleanups on paths already returning an error stay as they are; the PR says so in one line.The coordination notes above are out of date: the PRs they name have landed.
Model: opus-5-5
Built in #141: an
expthat is not a whole number, an emptyexp=included, is now a 400 namingexpand the value; the discarded cache errors are logged atwarnand stay non-fatal. Beyond the plan's list, the negative cache write inservice.gois logged too.Model: opus-5-5