Skip to content

feat: private-content caching opt-in with fleet-wide delete purge - #155

Merged
Nic-dorman merged 4 commits into
masterfrom
feat/download-cache-private
Aug 6, 2026
Merged

feat: private-content caching opt-in with fleet-wide delete purge#155
Nic-dorman merged 4 commits into
masterfrom
feat/download-cache-private

Conversation

@Nic-dorman

Copy link
Copy Markdown
Member

What

Private uploads can now be cached — behind an explicit operator opt-in — and deletes reach every instance's cache, not just the one that handled the API call. This extends the "bytes already on disk → serve from disk" story to private content, which was excluded until deletion could mean something fleet-wide.

Two halves:

  1. download_cache_private (default off, admin-UI toggle): private downloads promote read-through and private uploads seed, exactly like public — keyed on the serve path's derivation (the local DataMap). Off, behavior is byte-identical to today. Authorization is untouched either way: the cache is consulted only after the row/owner/visibility checks.
  2. Fleet purge propagation, on for everyone (it also tidies public entries):
    • uploads.cache_key — indexed serve-path key stamped at every terminal store status; Go-side backfill for pre-existing rows at writer boot (the digest can't be computed in SQL).
    • cache_purge_log — the delete appends the upload's cache keys (both derivations) before any row is removed, so a failure later can at worst cause a spurious purge and re-warm, never a missed purge.
    • The existing 1-minute cache sweep tick (all roles, including readers) consumes the log tail and unlinks local copies, never advancing its high-water mark past a key whose unlink failed — the durable retry the feat: purge cached bytes when their upload is deleted #154 panel asked about. Writer-role instances prune the log (7-day retention).
    • Boot reconciliation: each instance validates its entire cache directory against live rows via the cache_key index before consuming the log — covering deletes that happened while it was down, even beyond log retention. The log high-water mark is read before the cache snapshot, so nothing falls between reconciliation and the tail.

The purge-window contract (documented in the deployment guide): the instance handling a delete purges synchronously and fails the API rather than report a deletion with plaintext still readable (#154); every other instance purges within ~one sweep tick (60s). Replica lag would add to that window if read-replicas are ever introduced — called out in the docs.

Notes for review

  • The feat: purge cached bytes when their upload is deleted #154 machinery is reused visibility-blind: Drop's error contract is what makes the no-advance-past-failure loop sound, and the promote-site resurrection guards already close promote-vs-delete races on every instance against the shared DB.
  • Reader discipline (V2-514): the sweep tick's log read is the only new reader DB access; the two writes (log prune, cache_key backfill) are writer-role-gated.
  • Boot reconciliation uses unconditional Drop on non-live keys: the only concurrent promotion of a non-live key is a resurrection, which the promote-site guard is already unwinding.
  • MarkPublished (private→public flip) keeps data_map, and the serve path prefers it — so the stamped cache_key is stable across publishing.

Tests

All -race, full suite green; frontend builds.

  • Services: terminal statuses stamp cache_key (both derivations); delete appends both derivations to the purge log exactly once; legacy-row backfill is correct and idempotent; prune and liveness lookups.
  • Worker (fleet): a remote instance's sweep applies a delete within one tick (boot keeps the live entry, post-delete tick purges it); boot reconciliation drops orphans and keeps live entries, bytes verified gone; a failed unlink halts the high-water mark and the next tick retries to completion; writer prunes aged log rows.
  • Handlers: private uploads cache end-to-end when opted in (repeat download = 1 antd fetch); private delete purges the plaintext before returning; default-off behavior unchanged (existing private-never-cached assertions still pass).
  • Worker (seeding): private seeding respects the opt-in via the same seedDownloadCache gates.

🤖 Generated with Claude Code

Nic-dorman and others added 2 commits August 5, 2026 14:56
download_cache_private (default off) admits private uploads to the
download cache — read-through and seeding, keyed on the serve path's
DataMap derivation. Deletes now propagate to every instance: the delete
appends the upload's cache keys to cache_purge_log before any row is
removed, each instance's 1-minute sweep tick consumes the tail and
never advances past a failed unlink, and boot reconciliation validates
the whole cache directory against live rows (uploads.cache_key, stamped
at terminal statuses and backfilled at writer boot) for instances that
were down past log retention. Purge window: synchronous on the handling
instance, ~one tick everywhere else.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Boot reconciliation (correctly) purged the sweep tests' fixtures as
orphans before the eviction assertions ran — the entries stood in for
legitimately cached live content and now have matching rows, which is
also what production looks like.

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

dirvine commented Aug 5, 2026

Copy link
Copy Markdown
Member

Panel review — changes requested

Reviewed head: 699af6b36fe286af3544cf477c4f00eb287c4996

Consensus: request changes. Five of six substantive seats classified the private-cache lifecycle gaps below as merge blockers; one seat dissented on severity but independently found the opt-out retention gap. The ordinary implementation and tests are clean, but the advertised whole-fleet/durable purge contract is not yet true under failure and boot interleavings.

1. Boot reconciliation can permanently skip a concurrent delete

UploadService.Delete commits the purge-log insert and upload-row delete as separate autocommit statements (internal/services/upload.go:579-607). bootReconcile reads MaxPurgeLogID, then asks LiveCacheKeys, and finally assigns that earlier maximum to lastPurgeID (internal/worker/cache_sweep.go:233-265).

A valid ordering is:

  1. delete commits log entry N;
  2. boot reads max ≥ N;
  3. boot sees the not-yet-deleted upload row and keeps key K;
  4. boot records lastPurgeID ≥ N;
  5. delete commits the row removal;
  6. steady-state reads only IDs greater than the watermark, so N is skipped.

The remote instance retains deleted private plaintext until another restart or incidental eviction. I reproduced this ordering deterministically; the current tests cover boot-then-delete, not log-insert → reconcile → row-delete.

Required invariant/fix: make purge-log insertion, related-row cleanup and upload-row deletion one database transaction. Other connections must see either row-live/no-log or row-gone/log-present. Add a deterministic regression test around that visibility boundary.

2. Seven-day pruning can erase an affected instance's only retry record

On Store.Drop failure, propagatePurges returns without advancing lastPurgeID (cache_sweep.go:200-210). That correctly retries—but it also head-of-line blocks all later purge entries on that instance. Meanwhile the writer deletes every log row older than seven days without per-instance acknowledgement (cache_sweep.go:216-223, upload.go:668-680).

I reproduced this deterministically:

  1. make one cache file unremovable;
  2. process its purge and confirm the watermark stalls;
  3. prune the log row;
  4. restore filesystem permissions;
  5. run propagation again.

The plaintext and cache entry remain because the retry record is gone. Later deletes behind the failed key can also be delayed on that instance.

Required invariant/fix: pruning must not destroy unconfirmed erasure work. Viable shapes include durable per-instance acknowledgements, retaining the log until safe, or a periodic full liveness reconciliation on every instance as a guaranteed backstop. Also prevent one failed key from indefinitely starving later purges. Add tests for failed unlink → retention expiry/prune → recovery, and failed first entry followed by later deletions.

3. Turning private caching off does not remove existing private plaintext

download_cache_private=false only stops future lookup/admission (uploads.go:826-829). Existing private cache entries remain indexed and on disk. Boot reconciliation keeps them because their upload rows are still live; inactivity expiry defaults to zero, and under-budget caches may never evict them.

For an explicit private-data opt-in, “Disabled” should not silently mean “new caching disabled, old plaintext retained indefinitely.” Either implement true→false fleet purge plus policy-aware boot reconciliation, or explicitly redesign/document the control as admission-only with a required drain workflow. The former matches the rest of this PR's security contract.

Required tests: true→false purges online instances; an instance offline past log retention purges private entries at boot; no-op true→true/false→false updates do not create spurious work.

Non-blocking inherited issue

already_stored remains absent from both initial-schema status CHECK constraints and from UploadService.Delete's allowed statuses. This predates #155, but it means the status cannot be persisted on a fresh SQLite/Postgres database. Track separately with a migration and lifecycle tests.

What is sound

  • Owner/status checks remain before cache access; private-cache hits do not bypass authorization.
  • Key derivation and terminal-status cache_key stamping are consistent for the reachable completed paths.
  • Failed unlink retains store accounting; successful log processing advances only after Drop succeeds.
  • SQLite/Postgres migration shapes otherwise match, and reader instances perform no purge-log writes.

Verification

  • Exact head revalidated immediately before posting; mergeable, no unresolved review threads.
  • GitHub CI green for lint, SQLite/Postgres tests, frontend, Docker and smoke. PR-policy race/security jobs are skipped.
  • Local frontend build and 45/45 unit tests pass.
  • Full go test -race -count=1 ./..., go vet ./..., and git diff --check pass; affected packages also passed repeated race runs.
  • Both blocking purge-loss paths were reproduced with temporary deterministic tests and the working tree was restored clean.
  • Windows x86-64 worker/handler test binaries cross-compile; runtime filesystem semantics were not exercised on Windows.

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

Finding 1: the purge-log append and upload-row delete now commit in one
transaction — no interleaving lets a reconciling instance record a log
high-water mark, still see the live row, and miss the delete between.
A refused delete rolls its log rows back (regression-tested).

Finding 2: the full liveness reconciliation now re-runs every 12 hours
(and on demand) as the guaranteed backstop, so a purge whose log row
was pruned while its unlink was stuck is re-derived from live rows and
retried until the bytes are gone; a stuck key no longer delays later
purges — every entry is attempted per tick, only the high-water mark
waits for the contiguous prefix. Both panel reproductions are now
regression tests.

Finding 3: reconciliation is policy-aware — turning
download_cache_private off purges already-cached private plaintext
(next tick online, boot reconciliation for instances that were down)
instead of stranding it; no-op setting updates do no spurious work.

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

Copy link
Copy Markdown
Member Author

Thanks — all three blockers are fixed in 87c503e, each with the regression tests you specified.

1. Delete atomicity: UploadService.Delete is now a single transaction — identifier read, purge-log append, related-row cleanup, and the status-guarded row delete commit together (the wrapper's Tx carries the placeholder rebinding). Other connections see either row-live/no-log or row-gone/log-present; your interleaving (log N visible → reconcile keeps K and records the mark → row delete commits) is unobservable by construction. A refused delete rolls its log rows back — TestDeleteRefusedRollsBackPurgeLog pins the observable half.

2. Prune vs unconfirmed work + starvation: took the periodic-reconciliation shape. The full liveness reconciliation now re-runs every 12 hours (well inside the 7-day retention) and stays dirty — re-running every tick — while any of its unlinks fail, so erasure work never depends on the log row surviving. Your exact reproduction (stuck unlink → watermark stalls → prune → recovery) is TestPropagatePurges_PruneLossRecoveredByReconciliation, including the assertion that the tail alone would have lost it. Starvation: the tail now attempts every entry per tick and only the high-water mark waits for the contiguous successful prefix — TestPropagatePurges_FailedKeyDoesNotDelayLaterPurges shows a later delete applied in the same tick the first key is stuck.

3. Opt-out removes existing plaintext: reconciliation is policy-aware (CacheKeyVisibility replaces the bare liveness lookup): a private key is an orphan whenever download_cache_private is off. The sweeper detects the true→false transition and reconciles immediately — TestPropagatePurges_PrivateOptOutPurgesExisting (online flip purges private, keeps public, and a following no-op tick moves no counters) and TestPropagatePurges_BootPurgesPrivateWhenOptedOut (offline-past-retention instance applies the policy at boot). UI help text, USER-GUIDE, and the deployment guide now state the off-switch semantics and the reconciliation backstop.

The inherited already_stored issue was filed during the #154 review as V2-875 (migration + status-site sweep + lifecycle tests), queued next.

Full -race suite green (verified by direct exit code), frontend builds; CI running on the new head.

Completes the panel's no-op-update matrix — false-to-false was already
asserted; both are the same non-transition branch.

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

dirvine commented Aug 6, 2026

Copy link
Copy Markdown
Member

Re-review — changes since 699af6b (verified at bd4b3b5)

Re-checked the incremental 699af6b → bd4b3b5 diff against the three blockers from the prior panel. All three are addressed, each with the regression test you specified, and I re-ran the focused suites locally under -race on the current head.

1. Delete atomicity (boot race) — fixed. UploadService.Delete is now one transaction: identifier read → purge-log append → related-row cleanup → status-guarded row delete commit together, with the deferred tx.Rollback() discarding log rows on a refused delete. Your stated interleaving (log N visible → reconcile keeps key and records mark → row delete commits) is genuinely unobservable now, because the log row and row deletion share a commit: another connection sees either row-live/no-log or row-gone/log-present. TestDeleteRefusedRollsBackPurgeLog pins the observable half (refused delete leaves no log rows, row survives). Verified the transaction wrapper and that CacheKeys() derives from the same data_map/datamap_address read inside the tx.

2. Prune-loss + starvation — fixed. reconcile now re-runs at boot, every 12h, and on a private opt-out; it stays dirty (clean=false, lastReconcile not advanced) while any unlink fails, so erasure never depends on the log row surviving. The tail attempts every entry per tick (stalled flag) and only the high-water mark waits for the contiguous successful prefix. Both reproductions pass under -race locally:

  • TestPropagatePurges_PruneLossRecoveredByReconciliation — your exact stuck-unlink → watermark stall → prune → periodic-reconcile-recovery sequence, including the assertion that the tail alone loses it.
  • TestPropagatePurges_FailedKeyDoesNotDelayLaterPurges — later delete applied same tick while first key is stuck; HWM does not advance; retry tick clears it.

3. Opt-out removes existing plaintext — fixed. CacheKeyVisibility (replacing bare liveness lookup) returns each live key's visibility; reconcile treats a private key as an orphan whenever download_cache_private is off, and the sweeper detects the true→false transition (privateAllowed && !privateNow) and reconciles immediately. TestPropagatePurges_PrivateOptOutPurgesExisting (online flip purges private, keeps public, no-op tick moves no counters) and TestPropagatePurges_BootPurgesPrivateWhenOptedOut (offline-past-retention) both pass under -race. Visibility is DB-constrained to public/private, so the policy branch covers all states.

Verification performed: go build ./internal/... and go vet ./internal/{services,worker,database} clean; full internal/worker + internal/services suites green with -race; no stale references to removed LiveCacheKeys/bootReconcile. CI on bd4b3b5 is green (sqlite + postgres tests, lint, Docker, smoke).

Verdict: no remaining blockers from my side. This was a focused re-review of the requested changes rather than a fresh six-seat panel — appropriate given the targeted, well-tested fixes. Docs/UI copy accurately describe the off-switch semantics and the reconciliation backstop. The inherited already_stored item is correctly tracked as V2-875 (out of scope here).

No approval or merge action taken; that stays a human gate. Note the periodic reconcile is a full cache-directory scan every 12h — worth confirming the sweep cost is acceptable at your largest cache sizes before ship (operational consideration, not a blocker).

@Nic-dorman

Copy link
Copy Markdown
Member Author

Thanks — noted on the reconcile cost, with one precision: the periodic reconciliation is not a filesystem walk. It snapshots the store's in-memory index (Oldest over the entry map) and runs one indexed DB lookup per 500 keys — no disk I/O beyond the orphan unlinks themselves. At the design working set (wiki-scale, ~1GB / tens of thousands of entries) that is milliseconds every 12h; even a million-entry cache is a small in-memory sort plus ~2000 point-indexed queries. The per-tick eviction passes remain the bounded-batch scans they were. Will keep an eye on the download cache stats line as real deployments grow.

@Nic-dorman
Nic-dorman merged commit fc77d85 into master Aug 6, 2026
10 checks passed
@Nic-dorman
Nic-dorman deleted the feat/download-cache-private branch August 6, 2026 09:46
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