Skip to content

fix(api): set Cache-Control: max-age=60 on uploads so overwrites propagate#10

Merged
Zach Dunn (zachdunn) merged 4 commits into
mainfrom
claude/goofy-lichterman-1986a5
Jul 7, 2026
Merged

fix(api): set Cache-Control: max-age=60 on uploads so overwrites propagate#10
Zach Dunn (zachdunn) merged 4 commits into
mainfrom
claude/goofy-lichterman-1986a5

Conversation

@zachdunn

@zachdunn Zach Dunn (zachdunn) commented Jul 7, 2026

Copy link
Copy Markdown
Member

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 set Cache-Control on uploads, so R2's custom domain applied its default max-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=60 on every upload (files-sdk's R2 adapter already supports upload({ cacheControl })). This is the operative lever for the GitHub path: embeds are proxied through GitHub's Camo/Fastly cache, which honors the origin's Cache-Control and 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.sh directly in a browser, which isn't worth the CF API token setup + per-upload round-trip. The max-age is 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 ship no-cache — both behave exactly per those headers.

Verification

  • pnpm run check (oxlint + oxfmt) and pnpm run typecheck — pass.
  • pnpm --filter @uploads/storage test — 19 passing.
  • Bug reproduced live against prod pre-fix (default max-age=14400, cf-cache-status: HIT serving 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).

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.
@coderabbitai

coderabbitai Bot commented Jul 7, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are limited based on label configuration.

🏷️ Required labels (at least one) (2)
  • coderabbit:review
  • review
🚫 Excluded labels (none allowed) (1)
  • wip

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 98a42647-1ab9-452f-9485-cdc1158e9e64

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/goofy-lichterman-1986a5

Comment @coderabbitai help to get the list of available commands.

…an-1986a5

# Conflicts:
#	apps/api/src/routes/files.ts
@zachdunn

Copy link
Copy Markdown
Member Author

Reviewed this — solid, and it's targeting the right layer (source-side freshness, no Worker needed). The live repro of the default max-age=14400 behavior is the useful bit. One nuance worth capturing before we lean on this for the GitHub-attachments path:

Edge purge ≠ Camo purge. purgeUrls invalidates our Cloudflare edge cache, but GitHub proxies every embedded image through Camo/Fastly, which keeps its own cache in front of our origin. Purging our edge does not evict Camo's copy. So for the GitHub path (the stated motivation), the operative lever is the max-age=60 on upload — that's what caps how long Camo caches before it revalidates against our now-fresh edge. The purge mainly benefits people viewing storage.uploads.sh directly in a browser.

Practical implication: the ceiling on "how stale can a GitHub thread be after a re-upload" is set by how Camo honors our max-age, not by the purge. The verification confirms the bug live pre-fix; the one thing I'd add is a post-fix end-to-end check — re-upload a visibly different image to the same key and confirm the real GitHub thread reflects it within ~a minute — since that depends on Camo actually respecting the short TTL. If it doesn't propagate that fast, that's a Camo behavior finding, not a bug in this PR.

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 waitUntil, so I'd keep it — just noting the latency cost is real.

Nothing here needs to change to merge; the max-age=60 backstop is the correctness floor and it's sound.

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.
@zachdunn

Copy link
Copy Markdown
Member Author

You're right, and it changes the framing (not the code's behavior). Split out what each layer buys:

  • Edge purge ≠ Camo purge — confirmed. The purge evicts our Cloudflare edge only; GitHub's Camo/Fastly cache in front of the origin is untouched. So for the GitHub-embed path the operative lever is max-age=60, and the purge mainly helps direct storage.uploads.sh viewers (plus it means Camo gets fresh bytes the moment it revalidates). Updated the files.ts / purge.ts comments that implied otherwise — commit 00dffc2, no behavior change — and added an "edge purge ≠ Camo purge" section to the PR description.
  • Keeping the awaited purge as you suggested; noted the per-upload round-trip is real and justified by the ordering.
  • Post-fix Camo e2e check — agree it's the thing worth adding, since the whole feature now hinges on Camo honoring the short TTL. I didn't run it: minting a Camo URL requires a public GitHub write (issue/comment), which got correctly blocked as out of scope. Happy to run it if you want to greenlight a throwaway issue/comment — otherwise it's a good manual smoke test. If Camo ignores the short TTL, the fallback is a cache-busting query param on the embed URL (CLI-controlled), not a change to this PR.

🤖 Addressed by Claude Code

@zachdunn

Copy link
Copy Markdown
Member Author

Concrete evidence that Camo honors origin Cache-Control — GitHub badges are the same mechanism (external image in markdown, proxied through Camo), and they stay current precisely because of their cache headers. Live from just now:

# shields.io npm version badge
cache-control: max-age=300, s-maxage=300      # tunable: ?cacheSeconds=3600 -> max-age=3600

# GitHub Actions "passing" badge
cache-control: no-cache

Takeaways for this PR:

  • Camo caches (nothing on our origin can force-refresh a badge — same reason the purge can't evict Camo), and it respects the origin TTL: max-age=300 badges refresh within ~5 min, no-cache badges refresh on ~every render. That's exactly the lever our upload max-age=60 pulls, so the whole shields.io ecosystem is real-world proof the approach propagates within the TTL.
  • It also frames the freshness/load dial: shields picks max-age=300 to shield upstream APIs from Camo fan-out; GitHub's own Actions badge picks no-cache because it doesn't care. Our max-age=60 sits between. If we ever want near-instant GitHub-embed refresh, no-cache is the knob — at the cost of Camo hitting R2 on every render.

Still worth the post-fix e2e check to confirm behavior for our specific origin (and whether GitHub floors very-low max-age), but this makes me confident max-age=60 behaves as intended.

🤖 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.
@zachdunn Zach Dunn (zachdunn) changed the title fix(api): keep replaced uploads fresh at the edge fix(api): set Cache-Control: max-age=60 on uploads so overwrites propagate Jul 7, 2026
@zachdunn Zach Dunn (zachdunn) merged commit 1d5265f into main Jul 7, 2026
3 checks passed
@zachdunn Zach Dunn (zachdunn) deleted the claude/goofy-lichterman-1986a5 branch July 7, 2026 23:45
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.

1 participant