Repository navigation
fix(images): version-scope the /images/full Cache API key (ARCHIVE_VERSION) - #50
Merged
Merged
Conversation
…RSION) The full-res click-in originals are served from R2 under stable, non-content- addressed URLs and cached hard (1yr immutable) on the convention that an original is never overwritten in place. The 2026-07 bulk re-encode of every HIF archive (sips 4:2:0 -> zenc q100 4:2:2) broke that convention on purpose, and Cloudflare's cache purge — both by-URL and Purge Everything — does not evict the worker's per-colo caches.default entries, so the overwritten originals kept serving the old 4:2:0 bytes (verified: x-photo-cache=hit persists 5+ min after a zone purge). Fold an ARCHIVE_VERSION token into the caches.default KEY (never the public URL): the versioned key misses the stale entries and falls through to fresh R2 bytes. Bumping the const busts every overwritten original at once; one Purge Everything then clears the CDN tier. Same idea as the retired THUMB_VERSION, scoped to the non-content-addressed originals. Public URLs stay clean; no new files. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
aadhar-sh | be65221 | Jul 21 2026, 03:07 AM |
Contributor
Understanding-first reviewThis is a prompt, not a score or merge gate. Before approving, reconstruct the change:
For docs and dependency changes, a compact answer is enough. For behavior or production changes, include the causal path and one concrete counterexample. The author’s claim card belongs in the PR description; this prompt is for the reviewer’s independent reconstruction. |
oddharsh
added a commit
that referenced
this pull request
Jul 27, 2026
…SOOC file (#128) /images/full/<stem>.jpg is the one image surface here with no AVIF tier and no <picture> around it: the worker streams it from R2 as bare image/jpeg, ~10MB, and that single file is the whole payload. Scan order is therefore the entire loading experience, and cameras write baseline. Of the 158 archive originals, the 112 from HIF sources already go up progressive because zenc sets ProgressiveScanMode unconditionally; the 46 straight-from-camera JPGs did not. phase 3 now runs those through jpegtran -progressive -copy all on the way to R2. jpegtran reorders existing DCT coefficients and never decodes to pixels, so this is not a re-encode and carries no generational loss. Measured on a 9.95MB Leica original: SOF0 -> SOF2, 3.05% smaller (progressive Huffman packs better), decoded pixels byte-identical, EXIF including Orientation and ISO preserved. The conversion writes to a copy under $TMP and the source folder is never opened for writing. The bytes on disk stay exactly what the camera wrote, which is the point of keeping a SOOC archive; only the serving copy is rearranged. Falls back to uploading the untouched original if jpegtran fails, so a bad file costs the optimisation rather than the upload. This only affects photos added from here on. The 46 already in R2 stay baseline until someone backfills them, which needs an ARCHIVE_VERSION bump and a Purge Everything to get past caches.default (PR #50) and is deliberately not bundled in. Prompted by Kornel Lesiński's perf.now() 2018 image-optimization talk, which makes the case that progressive scan order is worth more than it looks precisely when one large image is the payload. Co-authored-by: Aadharsh Pannirselvam <19518661+oddharsh@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
oddharsh
added a commit
that referenced
this pull request
Aug 12, 2026
* fix: close the tag-stripping gaps CodeQL keeps finding All 15 open code-scanning alerts are the same 15 that were triaged and dismissed months ago. They came back because #340 renamed holding/ to www/, lwe-pipeline/ to pipelines/lwe/, and moved build.mjs and contract-tests.mjs into scripts/. CodeQL tracks an alert by file path, so every dismissal whose file moved reset to open. The alerts on files that stayed put, in scripts/lib/html-to-md.mjs and check-page-contracts.mjs, are still dismissed, which is what confirms the cause. So this fixes the 11 where fixing is cheaper than re-dismissing, and leaves 4 that no code change should touch. The security half of every old dismissal still holds: lensMarkdown returns JSON that reaches the DOM through esc(), an excerpt is stored escaped, and contentOf feeds link extraction rather than rendering. What those dismissals conceded was a FIDELITY cost, and that part is real and now fixed. `</script >` is a legal end tag and `--!>` legally closes a comment, so a stripper spelled `</script>` or `-->` hands the body through: script source lands in a markdown render, and minified JS lands in a webmention excerpt a human reads. www/_worker.js/lens.js \s* on the closer, --!> on the comment, and \b so <head> stops swallowing a page that opens with <header> and runs to the next literal </head> www/_worker.js/webmention.js same closers in excerptAround www/_worker.js/webmention-send.js same closers in contentOf pipelines/lwe/generate.mjs replaceAll on the accent's #, and --!> on the console.log marker strip scripts/build.mjs escape the backslash and the rest of the metacharacters before turning * into .* lensStripTags loops, and the comment at it says plainly that one pass of /<[^>]+>/g is already a fixpoint: the match runs from a < to the next >, so a surviving < has no > after it and a second pass cannot match. The loop is for CodeQL, which cannot see that argument, and for whoever narrows the pattern later, where the proof stops holding. Better to write that down than to claim a fix the code does not make. The contract test fails if either closer regex goes back, verified by reverting one. Left open deliberately, because the right resolution is a dismissal: #51 www/garage/horizon.html the unsafe parse IS the demo, setHTMLUnsafe beside setHTML on the visitor's own textarea #48 #49 #50 contract-tests two of these are Array.prototype.includes on a list of extracted citations, so they are not string operations at all, let alone URL checks Suite 240 pass, build, pages:check, typecheck, and 130 routes all green. * fix: end tags carry attributes, and one strip pass can re-form what it removes CodeQL re-scanned the changed lines and found two things the first pass got wrong. Both are real, and the second is the interesting one. An end tag may carry attributes. `</script bar>` closes a script element as surely as `</script>` does, so `<\/script\s*>` was still handing the body through, just for a narrower set of inputs than before. Every closer in lensMarkdown, excerptAround and contentOf is `<\/tag\b[^>]*>` now. Removing a span can RE-FORM the thing being removed out of what is left on either side. `<!-` + `<!--x-->` + `-` collapses to `<!--` the moment the middle goes, so one pass leaves a live comment opener behind. The contract test asserts that collapse directly before asserting the fix, so it demonstrates the bug rather than asserting around it. What explains the split in the alerts is the REPLACEMENT, not the pattern. The webmention strippers replace with a space, which keeps the two sides from touching and makes them immune for free; that is why CodeQL never flagged them for this and flagged lens.js twice. lensMarkdown replaces with "" to preserve markdown spacing, so it pays for that with a fixpoint loop instead. The old lensStripTags is now a general untilStable(s, re, repl) covering all five strips, and the comment at it says which of the two problems each caller has. pipelines/lwe/generate.mjs builds its console.log label by allowlisting the wordy part of the marker rather than denylisting delimiters it has to remember. Output is byte-identical on all four call sites, checked against the old form. Suite 242 pass, build, pages:check, typecheck, 130 routes all green. * fix: keep the end-tag fix, drop the fixpoint loops that fixed nothing The loops in the previous commit were wrong and the measurement is why. Keeping the reasoning in the file so it is not re-argued every time this moves. CodeQL wants a fixpoint loop on each strip in lensMarkdown, because removing a span can re-form what was removed out of what is left either side. That really happens here: one pass of the comment strip over `<p>a<!-<!--x-->-</p>` leaves `<p>a<!--</p>`, since the match starts at the INNER opener. It is unreachable anyway, twice over. The re-formed opener is eaten by the generic `<[^>]+>` strip further down, because `<!--</p>` matches whole. And that strip is itself already a fixpoint: its match runs from a `<` to the next `>`, so any `<` surviving a pass has no `>` after it and a second pass cannot match. The bill for looping all three, measured on one 256KB fixture, node, same V8: 6.29ms against 5.94ms median. Roughly 10% of the CPU-bound half of a scan, on the exact path LENS_PARSE_CAP exists to keep under the 10ms Workers ceiling. Real money for a bug nothing can reach, so the loops are out and the four lens.js multi-character alerts want a dismissal instead. The contract test I wrote for the loops is out with them. It passed with the loops reverted, which made it decoration, and this repo has the scar tissue to know better. The two end-tag tests stay and both fail when their fix is reverted, verified. What survives from that commit is the part that was real. An end tag may carry attributes, so `</script bar>` closes a script element as surely as `</script>` does, and `<\/script\s*>` was still handing the body through. Every closer in lensMarkdown, excerptAround and contentOf is `<\/tag\b[^>]*>` now. Suite 241 pass, build, pages:check, typecheck, 130 routes all green. --------- Co-authored-by: Aadharsh Pannirselvam <19518661+oddharsh@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
Introduce an
ARCHIVE_VERSIONconstant and fold it into thecaches.defaultkey (never the public URL) used byservePhotoFromR2for/images/full/<stem>originals. Bumping the const orphans the worker's cached copies of overwritten originals so the next read falls through to fresh R2 bytes. 24 lines, no new files; the served URL stays/images/full/<stem>.jpg.Why
The full-res click-in originals are served from R2 under stable, non-content-addressed URLs (the SOOC filename names a slot, not its bytes) and cached hard as
max-age=31536000, immutable, riding the convention that an original is never overwritten in place.The 2026-07 bulk re-encode of every HIF archive (sips 4:2:0 → zenc q100 4:2:2) broke that convention on purpose: 111 existing R2 keys were overwritten with new bytes. Cloudflare's cache purge — both by-URL and Purge Everything — does not evict the worker's per-colo
caches.defaultentries, so the overwritten originals kept serving the old 4:2:0 bodies. Versioning the internal cache key is the reliable bust (same idea as the retiredTHUMB_VERSION, scoped to the non-content-addressed originals instead of the now-hashed thumbnails).Understanding check
cache.match/cache.putrun against a synthetic?av=ARCHIVE_VERSIONkey. Old entries live under the tokenless (or lower-version) key, so a bump guarantees a miss and a fresh R2 read; the client never sees the token./images/full/<stem>.jpgon a bare URL servesYCbCr4:2:2 / Progressivefor all re-encoded stems, andx-photo-cachereturns tomissthenhiton fresh bytes.x-photo-cache=hiton stale 4:2:0 persisted 5+ min after both purge modes. That is the whole reason for this change.node --checkon both files;wrangler deploy --dry-runbundles the import graph clean.wrangler dev).Validation
node --check holding/_worker.js/lib/const.jsandholding/_worker.js/photos.js: pass.npx wrangler deploy --dry-run -c wrangler.jsonc: bundles clean.