Skip to content

feat: purge cached bytes when their upload is deleted - #154

Merged
Nic-dorman merged 2 commits into
masterfrom
feat/download-cache-purge
Aug 5, 2026
Merged

feat: purge cached bytes when their upload is deleted#154
Nic-dorman merged 2 commits into
masterfrom
feat/download-cache-purge

Conversation

@Nic-dorman

@Nic-dorman Nic-dorman commented Aug 5, 2026

Copy link
Copy Markdown
Member

What

The download cache holds decrypted plaintext on the instance's disk. Deleting an upload destroys the record (and, for private uploads, the DataMap — the only thing that makes the network copy recoverable), but until now a cached copy was only ever removed by later eviction. This PR makes the delete honest on the handling instance, at two strengths:

  • Strict, fails closed: any copy cached at delete time is unlinked before the row delete — if the unlink fails, the API returns 500 with the row intact, so a reported deletion always means the pre-existing plaintext is gone.
  • Best-effort race cleanup: a promotion racing the delete is taken back out by re-verification on both sides (below). In the doubly-degraded case — the promotion raced the delete and that final unlink failed — the leftover bytes are logged, are not HTTP-servable (the row gate 404s before the cache is consulted; regression-tested), and fall to eviction/inactivity. Durable purge retry for that residual arrives with the V2-873 fleet purge log.

The cache remains public-only, so today this is hygiene plus the recorded shred foundation; it becomes load-bearing only if private-content caching is built (V2-873, deliberately not part of this PR).

How

  • Store.Drop(key) error is now the purge primitive (the binding contract from the feat: download cache disk budget — LRU eviction sweeper with uploads-pause precedence #150 panel re-review): unlink-first inside the promotion critical section, returns the unlink error instead of silently retaining the entry, removes on-disk bytes even when the key is not indexed (a failed boot scan leaves files present but unindexed), and an absent file counts as success.
  • DeleteUpload purges before the row delete — a failed purge fails the request with the row intact — and purges again (best-effort, logged) after the row is gone, covering both key derivations (local DataMap and network address, which differ).
  • Resurrection guard at both promote sites (read-through and upload seeding): a fetch that passed its DB check before a concurrent delete committed re-verifies the row after promoting and takes the bytes back out. Together with the delete path's post-commit purge, whichever side acts last observes the final state and holds the unlink.
  • purged counter in the cache stats line (deferred here from feat: download cache observability — lifecycle counters, stats emission, deployment guide #152) and the failed-unlink warning is rate-limited to once a minute per store (feat: download cache disk budget — LRU eviction sweeper with uploads-pause precedence #150 panel rider — it previously could fire per victim per sweep tick).
  • Docs: deployment-guide privacy posture updated to the new purge-on-delete reality, including the honest statement of the best-effort residual; telemetry table row for purged.

Tests

All -race, full suite green:

  • Store: Drop returns error and retains the entry on failed unlink, then succeeds and counts on retry; purges unindexed on-disk files; no-op drops don't count.
  • Handler: delete purges the warmed cache before returning (invariant test); unpurgeable cache ⇒ 500 with row, index, and bytes intact, clean retry after; mid-fetch delete resurrection test — a stub-antd hook deletes the row while the fetch is in flight, the completed download still serves, and the promotion is verified purged; residual/404 regression — orphaned cache bytes behind a deleted row are never served.
  • Worker: a seed whose row was deleted between completion and promotion is taken back out. (Seed test env now backs uploads with real DB rows, which the guard requires and which is closer to production.)

🤖 Generated with Claude Code

Deleting an upload now removes its cached plaintext from the handling
instance's disk before the delete API returns. Drop becomes the purge
primitive: it returns an actionable error, unlinks even unindexed files
(failed-boot-scan leftovers), and a failed unlink fails the delete with
the row intact — the API never reports a deletion while a cached copy
remains readable. Both promote sites (read-through and upload seeding)
re-verify the upload row after promoting and take the bytes back out if
a concurrent delete removed it, closing the delete/late-promote race.
Purged counter added to the cache stats line; the failed-unlink warning
is rate-limited to once a minute per store.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@dirvine

dirvine commented Aug 5, 2026

Copy link
Copy Markdown
Member

Panel review — no material blocker

Reviewed head: 2577e519bedcbb4edccf58c049d6ecc08c58deee

Consensus: 6/6 substantive reviewers found no material merge blocker. The core delete/purge mechanics are sound for the current public-only cache:

  • Store.Drop unlinks before mutating index/accounting, returns actionable errors, purges unindexed leftovers, and serialises correctly against same-key promotion.
  • The pre-delete purge fails closed: unlink failure returns 500 and leaves the upload row intact.
  • Read-through and seeded promotions re-check row existence, while the post-delete purge closes the complementary interleaving. Key derivation covers both DataMap and DatamapAddress.
  • Cache access remains behind the upload-row/owner/status checks, so bytes left after row deletion are not HTTP-servable.
  • Purged metrics and unlink-warning rate limiting are correctly wired.

Panel dissent / non-blocking tightening

A deterministic adversarial reproducer confirmed one narrower case: if a late promotion appears after the strict pre-purge, the row deletion commits, and the post-commit Drop then fails, uploads.go:1263-1275 logs the failure but still returns HTTP 200 while public plaintext remains on disk until eviction/inactivity. It is unreachable through the API because the row gate returns 404 before cache.Open, so the panel did not classify it as a current public-only security blocker.

However, this is weaker than the PR body's literal claims that the API reports success only once plaintext is gone and that no cache entry outlives its row. Please either:

  1. tighten the wording to distinguish the strict pre-delete guarantee from best-effort post-commit race cleanup, and add a regression test for the residual/404 path; or
  2. retain purge keys durably for retry if the stronger disk-level guarantee is intended.

This becomes load-bearing before any private-content caching is considered.

Separate inherited defect

Not introduced by #154: already_stored is referenced as a terminal/downloadable/cacheable status, but both SQLite and Postgres initial-schema CHECK constraints omit it, so the transition fails on a fresh database. UploadService.Delete also omits it from the deletable-status SQL. This should be tracked and fixed separately with a migration plus status/delete tests; it does not block this PR's current reachable behaviour.

Verification

  • Current exact head revalidated immediately before posting; mergeable, no unresolved review threads.
  • GitHub CI green for lint, SQLite/Postgres tests, frontend, Docker and smoke. Race/security jobs are skipped by PR workflow policy.
  • Local: frontend build and 45/45 unit tests pass; go test -race -count=1 ./..., go vet ./..., and git diff --check pass.
  • Additional stability: affected handler/service suites passed two more full race runs; downloadcache/handler/worker passed three repeated race runs; downloadcache/worker passed ten repeated race runs.
  • Windows x86-64 test binaries cross-compile successfully; permission/open-file runtime semantics were not executed on Windows.

No approval or merge action was taken; this is an advisory panel review.

…al 404 path

Panel follow-up: the post-commit purge and promote-side guard are race
cleanup, not a disk-level guarantee — if that final unlink fails, the
leftover bytes are unreachable (the row gate 404s before the cache is
consulted) and fall to eviction. Say so in the handler comment and the
deployment guide, and add a regression test that orphaned cache bytes
behind a deleted row are never served. Durable purge retry arrives with
the fleet purge log (V2-873).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@Nic-dorman

Copy link
Copy Markdown
Member Author

Thanks — both follow-ups are handled in cb7fab9.

Non-blocking tightening → option 1 taken, with the option-2 linkage recorded:

  • PR body, deployment guide, and the handler comment now state the two strengths explicitly: the pre-delete purge is strict and fails closed; the post-commit purge + promote-side guard are best-effort race cleanup, and in the doubly-degraded case the leftover bytes are non-servable and fall to eviction.
  • New regression test TestDeleteResidual_OrphanedCacheBytesNotServable pins the 404 path the risk assessment leans on: orphaned cache bytes behind a deleted row (bytes on disk, still indexed) are never served.
  • Option 2 (durable purge-key retention) is deliberately not duplicated here: it is exactly the cache_purge_log in the V2-873 fleet design — the delete transaction appends keys durably and the sweep tick retries failed drops without advancing its high-water mark. That ticket is now scoped fleet-wide and marked load-bearing-before-private-caching, matching the panel's condition.

Inherited already_stored defect → filed as V2-875 (schema CHECK omits the status in both dialects so dedup re-uploads fail on schema-built databases; also missing from Delete's status list; fix = migration + status-site sweep + tests).

Affected suites re-run green under -race on the new head.

@Nic-dorman
Nic-dorman merged commit 1890194 into master Aug 5, 2026
10 checks passed
@Nic-dorman
Nic-dorman deleted the feat/download-cache-purge branch August 5, 2026 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants