Remove dead abstractions in imgcache: unimplemented Purge and three interfaces with no implementers #73

New Issue

clawbot · 2026-08-09T03:46:08+02:00

clawbot commented

2026-08-09 03:46:08 +02:00

Verified against main at 61f42e6.

1. Purge is declared but not implemented. internal/imgcache/imgcache.go:137-138 puts Purge in the ImageCache interface; internal/imgcache/service.go:376-379 is:

// TODO: Implement purge
return errors.New("purge not implemented")

This is the only TODO/FIXME/not implemented in non-test code in the entire repo. No route exposes it, so it is unreachable in production — it is interface surface promising a capability that does not exist.

2. Three interfaces have no implementers and do not match the real types. internal/imgcache/imgcache.go:158-182:

Allowlist.IsAllowlisted — the real type is allowlist.HostAllowList.IsAllowed; names do not match, so nothing satisfies the interface.
Storage — its methods take ctx and string; the real ContentStorage/VariantStorage take neither.
SignatureValidator — no implementer.

These are leftovers from an earlier design. They cost nothing at runtime but they actively mislead: someone reading imgcache.go for the package's shape sees three abstractions that describe nothing in the codebase.

Definition of done

Purge: either implement it (delete variant file + .meta sidecar + accounting row, transactionally consistent with #51's eviction path, and expose it on a route with authentication) or remove it from the interface and delete the stub. Recommend removing — nothing needs it today, #51 handles reclamation, and an unauthenticated-by-accident purge endpoint is a liability. If removed, note in the PR that it can be reintroduced deliberately when a use case exists.
Delete Allowlist, Storage, and SignatureValidator from imgcache.go unless a concrete implementer is added in the same change.
No behavior change; this should be a pure deletion plus, if Purge is removed, one interface-method removal.
make check green.

Coordination

Overlaps #39 (splitting internal/imgcache into focused packages) — the dead interfaces are exactly the kind of thing that should not survive into the split. Doing this before #39's urlparser extraction keeps that diff smaller.
If Purge is implemented rather than removed, it must be consistent with PR #55's eviction and sidecar handling — do it after #55 merges.

Verified against `main` at `61f42e6`. **1. `Purge` is declared but not implemented.** `internal/imgcache/imgcache.go:137-138` puts `Purge` in the `ImageCache` interface; `internal/imgcache/service.go:376-379` is: ```go // TODO: Implement purge return errors.New("purge not implemented") ``` This is the only `TODO`/`FIXME`/`not implemented` in non-test code in the entire repo. No route exposes it, so it is unreachable in production — it is interface surface promising a capability that does not exist. **2. Three interfaces have no implementers and do not match the real types.** `internal/imgcache/imgcache.go:158-182`: - `Allowlist.IsAllowlisted` — the real type is `allowlist.HostAllowList.IsAllowed`; names do not match, so nothing satisfies the interface. - `Storage` — its methods take `ctx` and `string`; the real `ContentStorage`/`VariantStorage` take neither. - `SignatureValidator` — no implementer. These are leftovers from an earlier design. They cost nothing at runtime but they actively mislead: someone reading `imgcache.go` for the package's shape sees three abstractions that describe nothing in the codebase. ## Definition of done 1. `Purge`: either implement it (delete variant file + `.meta` sidecar + accounting row, transactionally consistent with #51's eviction path, and expose it on a route with authentication) **or** remove it from the interface and delete the stub. Recommend **removing** — nothing needs it today, #51 handles reclamation, and an unauthenticated-by-accident purge endpoint is a liability. If removed, note in the PR that it can be reintroduced deliberately when a use case exists. 2. Delete `Allowlist`, `Storage`, and `SignatureValidator` from `imgcache.go` unless a concrete implementer is added in the same change. 3. No behavior change; this should be a pure deletion plus, if `Purge` is removed, one interface-method removal. 4. `make check` green. ## Coordination - Overlaps #39 (splitting `internal/imgcache` into focused packages) — the dead interfaces are exactly the kind of thing that should not survive into the split. Doing this **before** #39's `urlparser` extraction keeps that diff smaller. - If `Purge` is implemented rather than removed, it must be consistent with PR #55's eviction and sidecar handling — do it after #55 merges.

clawbot added this to the 1.0.0 milestone 2026-08-09 03:46:08 +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#73