httpfetcher.hostSems grows without bound, and VariantStorage.Delete orphans the .meta sidecar #87

New Issue

clawbot · 2026-08-09T03:50:32+02:00

clawbot commented

2026-08-09 03:50:32 +02:00

Verified against main at 61f42e6. Two small, unrelated-but-adjacent storage/fetcher defects, each a few lines.

1. hostSems is an unbounded map. internal/httpfetcher/httpfetcher.go:111,154-165 allocates one semaphore channel per distinct upstream host and never removes it. Every new host adds an entry that lives until restart.

In practice this is bounded by who can produce valid signatures or by the allowlist, so it is not remotely exploitable today. But it is unbounded in principle, it is the same shape as the disk-fill issue #51 just fixed, and an operator running with a permissive allowlist against many origins accumulates entries forever. Also relevant: any per-IP rate limiter (filed separately) will introduce a second map with exactly this shape, so it is worth establishing the pattern once.

2. VariantStorage.Delete leaves the .meta sidecar behind. internal/imgcache/storage.go:485-494 removes the variant file but not path + ".meta", which Store writes at :417. Every deletion therefore leaks a small file that nothing will ever clean up.

This became consequential with #51/PR #55: eviction is now a routine, high-volume operation, so an orphan per eviction accumulates indefinitely — and sidecars are not counted in the size accounting, so the leak is invisible to the very mechanism meant to bound disk usage. PR #55 added a reconciliation pass that sweeps some orphans; verify against the post-#55 code whether this specific case is already handled before writing a fix, and if it is, close this half as already-done rather than duplicating.

Definition of done

hostSems entries are bounded — either reference-counted and removed when idle (the contentLock pattern PR #55 introduced in internal/imgcache/contentlock.go is a good model, and reusing its shape would be consistent), or capped with an LRU. Must remain race-free; the suite runs with -race.
VariantStorage.Delete removes the .meta sidecar alongside the variant file, treating a missing sidecar as success rather than an error.
Failing tests first: repeated fetches across many distinct hosts do not grow the semaphore map without bound; Delete leaves no .meta file behind.
make check green.

Coordination

Item 2 touches internal/imgcache/storage.go, which PR #55 changes heavily — do it after #55 merges, and re-verify the finding against the merged code first.

Verified against `main` at `61f42e6`. Two small, unrelated-but-adjacent storage/fetcher defects, each a few lines. **1. `hostSems` is an unbounded map.** `internal/httpfetcher/httpfetcher.go:111,154-165` allocates one semaphore channel per distinct upstream host and never removes it. Every new host adds an entry that lives until restart. In practice this is bounded by who can produce valid signatures or by the allowlist, so it is not remotely exploitable today. But it is unbounded *in principle*, it is the same shape as the disk-fill issue #51 just fixed, and an operator running with a permissive allowlist against many origins accumulates entries forever. Also relevant: any per-IP rate limiter (filed separately) will introduce a second map with exactly this shape, so it is worth establishing the pattern once. **2. `VariantStorage.Delete` leaves the `.meta` sidecar behind.** `internal/imgcache/storage.go:485-494` removes the variant file but not `path + ".meta"`, which `Store` writes at `:417`. Every deletion therefore leaks a small file that nothing will ever clean up. This became consequential with #51/PR #55: eviction is now a routine, high-volume operation, so an orphan per eviction accumulates indefinitely — and sidecars are not counted in the size accounting, so the leak is invisible to the very mechanism meant to bound disk usage. PR #55 added a reconciliation pass that sweeps some orphans; **verify against the post-#55 code whether this specific case is already handled** before writing a fix, and if it is, close this half as already-done rather than duplicating. ## Definition of done 1. `hostSems` entries are bounded — either reference-counted and removed when idle (the `contentLock` pattern PR #55 introduced in `internal/imgcache/contentlock.go` is a good model, and reusing its shape would be consistent), or capped with an LRU. Must remain race-free; the suite runs with `-race`. 2. `VariantStorage.Delete` removes the `.meta` sidecar alongside the variant file, treating a missing sidecar as success rather than an error. 3. Failing tests first: repeated fetches across many distinct hosts do not grow the semaphore map without bound; `Delete` leaves no `.meta` file behind. 4. `make check` green. ## Coordination Item 2 touches `internal/imgcache/storage.go`, which PR #55 changes heavily — do it **after** #55 merges, and re-verify the finding against the merged code first.

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