fix(api): set Cache-Control: max-age=60 on uploads so overwrites propagate#10
Conversation
R2 custom domains edge-cache cacheable objects (images) under a default max-age=14400, so overwriting a key at its stable URL kept serving the old body for up to 4h — breaking the GitHub-attachments promise that re-uploading the same key updates every embed. Two layers: - Set `Cache-Control: public, max-age=60` on every upload. Bounds staleness to ~60s everywhere, including bring-your-own-domain buckets. - Purge-on-overwrite for zones we control (the shared `storage.uploads.sh` domain): after writing, purge the exact URL via the Cloudflare API so replacements propagate immediately. Gated on a CF_PURGE_TOKEN secret plus CF_PURGE_ZONE_ID / CF_PURGE_HOSTS vars; unset or a non-matching host (BYO domains) => no-op, falling back to the short TTL. Best-effort: purge failures never fail the upload.
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (2)
🚫 Excluded labels (none allowed) (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…an-1986a5 # Conflicts: # apps/api/src/routes/files.ts
|
Reviewed this — solid, and it's targeting the right layer (source-side freshness, no Worker needed). The live repro of the default Edge purge ≠ Camo purge. Practical implication: the ceiling on "how stale can a GitHub thread be after a re-upload" is set by how Camo honors our Non-blocking: awaiting the purge adds a Cloudflare API round-trip to every core-zone upload. The ordering rationale (URL fresh before the caller references it) justifies awaiting over Nothing here needs to change to merge; the |
Per review: purging our Cloudflare edge doesn't evict GitHub Camo's cache, so for the GitHub-embed path the upload max-age is the operative freshness lever, not the purge. Corrects comments that implied purge made embeds fresh; no behavior change.
|
You're right, and it changes the framing (not the code's behavior). Split out what each layer buys:
🤖 Addressed by Claude Code |
|
Concrete evidence that Camo honors origin Takeaways for this PR:
Still worth the post-fix e2e check to confirm behavior for our specific origin (and whether GitHub floors very-low 🤖 Addressed by Claude Code |
The purge only evicts our Cloudflare edge, not GitHub's Camo/Fastly cache, so it did nothing for the GitHub-embed path (Camo honors the origin max-age, as the shields.io/Actions badges demonstrate). It only helped direct browser viewers — not worth the CF token setup + per-upload round-trip. Keep the max-age=60 upload header, which is the actual fix; purge can be revisited later if direct-view freshness ever matters.
Problem
The GitHub-attachments feature (#8) promises that re-uploading a file at the same key (
gh/<owner>/<repo>/pull/<num>/<name>) updates every embed. But the API never setCache-Controlon uploads, so R2's custom domain applied its defaultmax-age=14400(4h) to cacheable objects (images). Confirmed live against prod: the edge (and GitHub's Camo proxy) then served the old image for up to 4 hours after an overwrite.Fix
Set
Cache-Control: public, max-age=60on every upload (files-sdk's R2 adapter already supportsupload({ cacheControl })). This is the operative lever for the GitHub path: embeds are proxied through GitHub's Camo/Fastly cache, which honors the origin'sCache-Controland revalidates within the TTL — so an overwrite now propagates in ~60s instead of hours. Applies to every bucket, including bring-your-own-domain workspaces. No config or secrets required.Why not also purge the CF edge?
An earlier revision added a purge-on-overwrite call to the Cloudflare API. It was dropped: purging our edge does not evict Camo's cache, so it did nothing for the GitHub-embed path (the motivating case) — it only sped up viewing
storage.uploads.shdirectly in a browser, which isn't worth the CF API token setup + per-upload round-trip. Themax-ageis the real fix. Purge can be revisited later if direct-view freshness ever matters.Real-world confirmation that Camo honors the origin TTL: GitHub badges are the same mechanism (external image → Camo). shields.io ships
cache-control: max-age=300(tunable via?cacheSeconds=), GitHub's own Actions badges shipno-cache— both behave exactly per those headers.Verification
pnpm run check(oxlint + oxfmt) andpnpm run typecheck— pass.pnpm --filter @uploads/storage test— 19 passing.max-age=14400,cf-cache-status: HITserving a stale body after overwrite); probes cleaned up.Recommended follow-up (not in this PR): a post-fix end-to-end check — re-upload a visibly different image to the same key and confirm a real GitHub thread reflects it within ~a minute — to confirm Camo honors the short TTL for our origin. If it doesn't, the fallback is a cache-busting query param on the embed URL (CLI-controlled).