feat: measure a cold map load against the archive #53 published (#63) - #321
Conversation
#63's eighth criterion asks for time to first painted tile on a cold cache, and points at hosting: "Protomaps' own deployment docs warn that R2 latency is '500 ms or higher'". Every previous attempt at that number was taken against a loopback server, which removes the term being asked about. #53 published a continental-US extract on 2026-09-16, so it is takeable, and this takes it. Measured: 525.6 ms to first painted tile against the hosted archive, median of seven runs, against 206.0 ms on the loopback fixture on the same machine and the same browser -- about 320 ms of hosting, where ADR 0010 D-1 estimated "up to ~300 ms on a cold request". Every response carried cf-cache-status: DYNAMIC, so nothing is edge-cached and each range request is an origin hit; that is the largest available improvement and it belongs to #53. The hosted block is OPT-IN through OYL_HOSTED_BASEMAP_URL and CI sets nothing, because a gate that needs somebody else's CDN fails on an aeroplane. A malformed value is an error rather than a skip. The cost of a skipped block is that it rots unseen, so everything decidable without a network is in browser/hosted-archive.ts with a Vitest suite inside `pnpm run test`. One defect in how the number is read, found by taking it: a cross-origin resource with no Timing-Allow-Origin reports zero bytes and zero phases, which is exactly what the loopback measurement uses to mean "served from cache", and means the opposite. ArchiveRequestTiming.timingOpaque is which, and both halves are asserted -- loopback transparent, hosted opaque. - apps/web/browser/hosted-archive.ts: the archive, the origins it may reach, and a ride placed at the centre of whatever coverage the header declares -- derived, because the London track the rest of the harness uses is outside a United States archive and would report "nothing painted" about a working one - apps/web/browser/map.browser.spec.ts: the render, the origin check against a real third-party host, the control, and the number - apps/web/browser/harness.ts: ?track=, and timingOpaque - docs/spikes/0004: the measurement, dated, with its environment - docs/adr/0010: an appended amendment, nothing above the line edited - three comments corrected: they predicted this gate would go red when #53 published, it did not, and they were about the harness server all along Closes #63 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016X4EwacF1KuXDPhXzunifS Signed-off-by: mcronin <mgcronin@gmail.com>
mgcronin
left a comment
mgcronin
left a comment
There was a problem hiding this comment.
Code Review — PR #321
Recommendation: merge with follow-ups — 0 blocking findings, 2 non-blocking, 2 informational.
This is automated analysis published as a comment, not a human sign-off: the reviewing identity is
also the PR author, so GitHub cannot record an independent approval or change request from it. A
human still owns the merge decision.
What I verified rather than accepted
I re-ran the measurement and the mutations myself instead of reading the PR body.
| Claim | How it was checked | Result |
|---|---|---|
| The hosted block measures criterion 8 | Ran OYL_HOSTED_BASEMAP_URL=… pnpm run test:browser on this branch against the live archive |
85 passed. First painted tile 460.1 ms hosted vs 183.7 ms loopback — +276 ms, consistent with the reported +320 ms on a different run |
cf-cache-status: DYNAMIC |
Independent curl range probe of the archive |
Confirmed. 206, content-range: bytes 0-16383/19155814749, cf-cache-status: DYNAMIC |
The host sends no Timing-Allow-Origin |
Same probe, full response headers | Confirmed — absent. The timingOpaque defect is real |
| M1, M3, M5, M6, M11, M12, M13 | Applied each to a clean tree, ran hosted-archive.test.ts, restored |
All red, each on the named test. M6 went red on exactly the five tests claimed |
M15 (timingOpaque hard-coded false) |
Applied, rebuilt the harness, ran the map spec with the hosted block on | Red — hosted criterion 8, on the not.toContain(false) tripwire |
M16 (hard-coded true) |
Same, hosted block off | Red — loopback criterion 8. Both halves confirmed computed rather than constant |
ENV001 fires for the new variable |
Removed OYL_HOSTED_BASEMAP_URL from .env.example, ran check-env-example.sh |
ENV001: apps/web/browser/map.browser.spec.ts: reads OYL_HOSTED_BASEMAP_URL…. Not a vacuous guard |
| Nothing ships | pnpm run build, then grep -rl OYL_HOSTED apps/web/dist |
No hit. apps/web/src is untouched |
| Test counts | pnpm run test |
284 files / 5233 tests, exactly as reported |
| ADR 0013 compliance | git diff --numstat on docs/adr/0010-… |
12 insertions, 0 deletions, appended under ## Amendments, dated after the entry above it |
Gates re-run green on this branch: check:repo (115+16+21+28), format:check, lint, typecheck
(8 projects), test, check:wiring, check:a11y-suite, build, test:browser (both shapes). CI
on this PR is green — Repository rules 8m22s, CodeQL and all three Analyze jobs.
Requirements: PASS
Criterion 8 — "time to first painted tile on a cold cache" — is measured, reproduced
independently, and recorded in three durable places (spike 0004, an ADR 0013 amendment,
docs/architecture.md). Criterion 7 is exercised rather than merely asserted, against an archive
this project does not control. Criterion 3 is executed against a real third-party origin for the
first time. Closes #63 is justified.
Security: CLEAN
No dependency added, removed or bumped. No shipping code changed. The one new read of untrusted
input (?track=) is on a page that never ships, is validated, refuses rather than falls back, and
its messages name the field and the constraint and never the value — ADR 0004 decision D, with a
test asserting the message carries no digit. No location data of any person; the coordinates are the
centre of a published bounding box.
Findings
| # | Severity | Confidence | Where |
|---|---|---|---|
| 1 | Medium — non-blocking | Proven by experiment | map.browser.spec.ts:684 — a guard that cannot fire when the watcher records nothing |
| 2 | Low — non-blocking | High | Four surviving statements that this PR falsifies |
| 3 | nit | High | map.browser.spec.ts:606 — the PR body overstates "no wall-clock threshold" |
| 4 | Informational | Medium | hosted-archive.ts:222 — centreTrack range-checks ordering but not domain |
All four are inline. None blocks a merge.
Credit where it is due
The timingOpaque finding is the best thing in this change and it is not a small one: a zero byte
count meaning "served from cache" on one side of the diff and "you are not allowed to know" on
the other would have turned a DYNAMIC CDN into a warm-cache story in the ADR amendment. Catching
it before it was written down, and then asserting both directions so it cannot silently become a
constant again, is exactly the posture §5 asks for. I confirmed M15 and M16 independently and both
go red.
Equally good: deriving the ride from the archive's own declared bounds rather than hard-coding
Denver. A hard-coded fix would have passed this PR and quietly broken criterion 7 for the next
archive.
And the three corrected predictions are corrected in place with the correction visible, not
deleted — which is the only way a reader who remembers the old sentence finds out it was wrong.
Scope of this review
Read all nine changed files in full, plus harness.ts, pmtiles-fixture.ts, vitest.config.ts and
rules.yml for context. Ran the project's own gates as CLAUDE.md §4a names them, including the
browser gate in both shapes and the opt-in hosted block against the live archive. Applied nine of
the eighteen mutations myself. Did not re-measure coverage percentages — the report's include
patterns are apps/*/src/** and nothing under apps/web/src changed, which is a sufficient
explanation for "identical", and CLAUDE.md §5 forbids a percentage gate anyway.
| // mean the whole 19 GB object was being asked for, which is the failure mode | ||
| // that makes PMTiles-on-object-storage untenable and is worth catching | ||
| // rather than timing. | ||
| for (const response of responses) { |
There was a problem hiding this comment.
The 206 guard cannot fire when the response watcher records nothing — and I proved it rather than reasoned about it.
Severity: Medium. Non-blocking (this block is opt-in and CI never runs it), but worth one line.
I replaced watchArchiveResponses's predicate with if (false) so responses came back empty, rebuilt the harness and re-ran with the hosted block on. Criterion 8 stayed green, and the recorded line read:
hosted cold load — first painted tile 720.9 ms … 6 archive range request(s) against
tiles.openzigs.com … no archive response was observed; the wire timings are opaque …
Two things follow, and the second is the one that matters:
- This loop asserted nothing at all — an empty list satisfies
for … expect(…).toBe(206)vacuously. That is the shape CLAUDE.md §4k calls "a guard that cannot fire". - The
cf-cache-statusevidence disappeared from the deliverable with no test going red.DYNAMIC×6is the observation the ADR 0010 amendment rests on, the spike's §2 headline, and the finding this PR files for Publish and serve the PMTiles basemap and routing engine #53 — and the report will stateno archive response was observedbeside a claim of 6 range requests, which is internally contradictory, and nothing notices.
One line closes it, next to the loop:
expect(responses.length, 'no archive response was recorded — the watcher is not seeing them').toBeGreaterThan(0);The same gap exists on load.archiveRequests a few lines below: the loopback block asserts expect(load.archiveRequests.length).toBeGreaterThanOrEqual(1) before its equivalent tripwire, and the hosted block does not — so not.toContain(false) is also satisfied by an empty list. cacheNote's empty-list branch exists precisely because this state is reachable; the test should not be able to pass through it.
| // path, which a bucket somewhere else cannot change. The test is right; the | ||
| // prediction was not, and the correction is recorded rather than the | ||
| // sentence quietly deleted. | ||
| // |
There was a problem hiding this comment.
Four statements in the tree still say the archive does not exist, and this PR is what falsified them.
Severity: Low. Non-blocking, and one of them is squarely inside this diff's own file.
The PR body says "three comments predicted this gate would go red… corrected in place rather than deleted", and those three are corrected well. grep -rn "no published archive\|There is no archive to serve\|has published nothing" finds four more:
apps/web/browser/map.browser.spec.ts:164—archiveRequested's note: "There is no archive to serve (Publish and serve the PMTiles basemap and routing engine #53), so the request 404s". Same file as this change. The behaviour it documents is still right (the harness server serves none at that path); the reason it gives is the one this PR spent a paragraph correcting twenty lines above.apps/web/src/map/basemap.ts:48— "Publish and serve the PMTiles basemap and routing engine #53 has not published an archive"apps/web/src/map/basemap.test.ts:39— same sentenceapps/web/src/game/hud/plan.ts:14— "VITE_BASEMAP_PMTILES_URLis empty, Publish and serve the PMTiles basemap and routing engine #53 has published nothing, and Render activity maps with MapLibre GL JS and self-hosted PMTiles #63's MapLibre seam has never had an archive to point at" — the last clause is now false twice over
The last three are outside this diff and I would not ask for them here; apps/web/src being untouched is a virtue of this change. But the first is in a file you edited, and in a repository whose convention is "a reviewer who remembers this paragraph naming it is reading the old file", four surviving copies of a falsified fact is the kind of drift this project fixes rather than inherits. Worth either a one-line edit here plus a follow-up issue for the other three, or a note on #53.
| 0, | ||
| ); | ||
| // @see HOSTED_DEADLINE_MS — the box the control below rests on. | ||
| expect(load.firstPaintMs ?? Number.POSITIVE_INFINITY).toBeLessThan(HOSTED_DEADLINE_MS / 2); |
There was a problem hiding this comment.
nit: the PR body says "no wall-clock threshold is asserted and one must not be added", and this line is a wall-clock threshold — 10 s.
The code is accurate and I have no objection to it: the comment on HOSTED_DEADLINE_MS states exactly what this is (the box the control rests on, so a slow machine fails the positive test first with a number in the message rather than turning the control into a quiet false pass), and that reasoning is sound and worth keeping.
It is only the PR body's phrasing that is stronger than the code. "No performance budget is asserted — only sanity bounds and a generous liveness box" would be the accurate version, and it matters a little because the next person to read that sentence may go looking for the assertion and conclude one was removed.
| * extent that it keeps the line near its intended length, which is what keeps the | ||
| * fitted zoom in the band described above; nothing downstream is a distance. | ||
| */ | ||
| export function centreTrack(bounds: ArchiveBounds, metres: number): TrackGeometry { |
There was a problem hiding this comment.
Informational — centreTrack checks the ordering of the bounds it is handed but not their domain.
west <= east and south <= north are enforced; nothing requires them to be within ±180 / ±90. A PMTiles header carrying a garbage minLat therefore produces a track that is well-formed here, survives trackParameter, and then throws on the page inside parseTrackParameter — as "track parameter: latitude must be a finite number within ±90".
That is a safe failure and the message is a good one, so this is not a defect today. It is worth noting only because it is the same diagnosis problem requireCoverage was written to solve: a fault in the archive's header surfaces as a fault in the harness's query string, one layer away from its cause, at which point the engine, the protocol handler and the style are all equally consistent with what you see. Two more clauses beside the ordering checks would keep the blame where requireCoverage's own doc comment argues it belongs.
Not asking for a change — recording it so the next person reading a strange page-load failure has the pointer.
1 participant
What this is
Closes #63. Its eighth acceptance criterion — "Cold-load behaviour is measured and recorded in
the PR: time to first painted tile on a cold cache" — is the only one that was outstanding, and it
has been outstanding because its last sentence points at hosting: "Protomaps' own deployment
docs warn that R2 latency is '500 ms or higher'; if the chosen host is R2 this is where it becomes
visible." Every previous attempt at the number was taken against a loopback server, which removes
that term by construction, and #175, #176 and #314 each said so plainly rather than claiming
otherwise.
#53 published an archive on 2026-09-16, recorded in the third comment on #63.
So the measurement is takeable. This takes it, in the pinned Chromium, through the same page, the
same adapter and the same style builder — only the archive URL differs, which is criterion 7
("the basemap URL is configuration, and a test proves the map renders against a second archive URL
without a code change") being exercised against a second archive that is not ours to control.
Criterion 8 — the measurement
Seven consecutive runs of each configuration, one worker, a fresh browser context per run (so an
empty HTTP cache each time). macOS 26.6.2 on Apple silicon, Chromium 1243, headless, SwiftShader.
ICMP round trip to the CDN edge 23.4–35.6 ms, mean 27.4 ms.
renderer.createcf-cache-statusDYNAMICon every response, every runOne more figure, from the first run of the session rather than the seven: 616.3 ms, against a
median of 525.6 ms for those that followed. Nothing in the client is warmed between runs, so that is
most plausibly the operating system's DNS and TLS state — and it is what a rider's first ever map
load looks like, so it is recorded rather than discarded.
The line the gate prints, verbatim from a run:
Against what ADR 0010 predicted
D-1 states the trade as "this saves about $35/month and costs up to ~300 ms on a cold request",
hedging against Protomaps' warning of "500ms or higher". Measured: 320 ms at the median, 280–356
ms across the runs, and no single request came near 500 ms in the browser (one
curlprobe at478 ms came close). So the estimate was close and slightly optimistic, and the decision is
unaffected — which is the property D-1 was structured to have. An appended amendment under
ADR 0013 records it; nothing above the
## Amendmentsline is touched, andgit diff --staton thatfile is
12 ++++++++++++, insertions only.Every response in every run carried
cf-cache-status: DYNAMIC. Nothing is cached at theCloudflare edge, so every range request reaches R2 itself and pays the ~250 ms origin TTFB rather
than the 27 ms edge round trip. A cache rule on that hostname is a configuration change costing
nothing and is the single largest available improvement to this figure. It also means the number
above is the worst case rather than the typical one — which is the right way round for a
criterion that says "cold".
The defect this found
A zero byte count means two opposite things, and the gate could not tell them apart.
PerformanceResourceTimingzeroestransferSizeand every intermediate phase for a cross-originresource unless the server sends
Timing-Allow-Origin. The archive's host does not send one —checked rather than assumed: the response carries
access-control-expose-headers: etag,content-range,content-length,accept-rangesand noTiming-Allow-Originat all.The loopback measurement's own comment used
transferredBytes: 0to mean "the browser served thisfrom cache". A hosted request reports the same 0 to mean "you are not allowed to know" — so a
reader comparing the two would have concluded the CDN served everything from cache, which is the
exact opposite of what
cf-cache-status: DYNAMICsays. This is the read-cannot-see-the-write shapein a measurement rather than in a store: the write is fine, the read is ambiguous, and nothing was
looking.
ArchiveRequestTiming.timingOpaqueis the discriminator, and both halves are asserted ratherthan one: the loopback requests must be transparent and the hosted ones must be opaque. Mutations
M15 and M16 below are that pair.
Two smaller ones, both found before they bit:
it unchanged, the page asks for tiles the archive does not hold, paints nothing, and reports "no
basemap reached the screen" about an archive that is working perfectly. So the ride is derived
from the archive's own declared bounds — not hard-coded to Denver, which would fix it for one
archive and reintroduce it for the next, against criterion 7. M14 is the proof that this is
load-bearing.
and it was right not to: the 404 those tests assert is on the harness server, which a bucket
somewhere else cannot change. Render activity maps with MapLibre GL JS and self-hosted PMTiles #63's own third comment had already caught the error; this PR
corrects the three places in the tree that still said it, rather than deleting the sentences.
How it is turned on, and the trade in doing it that way
OYL_HOSTED_BASEMAP_URL— anhttps:archive URL. Unset, the block is skipped andpnpm run test:browseris byte-for-byte what it ran before; CI sets nothing. That is chosenrather than defaulted into, for the reason
scripts/check-doc-links.shrefuses to resolve anhttps:target: a gate that needs somebody else's CDN fails on an aeroplane, behind an egress proxy,and on the morning that CDN has a bad hour.
unseen. Three things are done about it rather than none:
apps/web/browser/hosted-archive.ts, pure, witha 27-case Vitest suite that runs inside
pnpm run teston every save. What is left behind theskip is small.
are different states, and only the first is a reason to measure nothing; a typo'd host that
quietly skipped would be a run somebody believed they had taken. M1 is that test.
docs/spikes/0004-cold-load-first-painted-tile.md,because a number that lives only in a PR body ages out of sight.
Rule⚠️ The archive URL itself is not committed to the template — #53's own comment argues
ENV001requires the variable in.env.example, so it is documented configuration rather thanfolklore.
that, and it is right: a default pointing at one person's bucket cuts against ADR 0002 decision A.
What was built
apps/web/browser/hosted-archive.tsapps/web/browser/hosted-archive.test.tsapps/web/browser/map.browser.spec.tsdescribe: the render, criterion 3 against a real third-party host, the control, and the numberapps/web/browser/harness.ts?track=, andtimingOpaquedocs/spikes/0004-…docs/adr/0010-…docs/architecture.md,CLAUDE.md,.env.exampleNothing under
apps/web/srcchanged.git diff --name-onlyis nine files and none of them ships.Mutations — §5's gate
Each applied to a clean tree, the suite run, the file restored byte-identically, and the tree
confirmed clean afterwards with
git status.hosted-archive.ts, againsthosted-archive.test.tsOYL_HOSTED_BASEMAP_URLreturnsundefinedinstead of throwinghttps:refusal deletedpermittedOriginsreturns the harness origin onlyforeignRequestspasses a URL it cannot parse instead of reporting it1/cos(latitude)correction dropped fromcentreTrack[latitude, longitude]track.tswarns about: transposed, the ride is drawn on another continent and throws nothingparseTrackParameter||, so a one-tile archive is refused tooharness.tsand the spec, against the browser gatetrackFor()ignores?track=and always rides in LondontimingOpaquehard-codedfalsenot.toContain(false)tripwiretimingOpaquehard-codedtruenot.toContain(true)half. M15 and M16 together are what maketimingOpaquea computed value rather than a constant either wayGates
Every command run on this branch at Node 24.20.0 / pnpm 11.18.0, against Chromium revision 1243.
pnpm run check:reposhellcheck scripts/*.shpnpm run format:checkpnpm run lintpnpm run typecheckpnpm run test:coveragepnpm run check:a11y-suitepnpm run test:a11ybash scripts/check-a11y-suite.test.shpnpm run check:wiringbash scripts/check-wiring.test.shpnpm run check:capacitorbash scripts/check-capacitor-generated.test.shpnpm run check:cost-modelbash scripts/check-cost-model.test.shbash scripts/coverage-summary.test.shpnpm run buildpnpm run test:browserOYL_HOSTED_BASEMAP_URL=… pnpm run test:browserbash scripts/check-dependency-licences.test.shpnpm run check:licencesCoverage
Reported, not gated — the gate is the mutation list above.
apps/webstatementsIdentical, and that is the expected result rather than a disappointment. The coverage report's
include patterns are
apps/*/src/**, and nothing underapps/web/srcchanged —apps/web/browser/**is outside the report for the reason
packages/fit/tools/**is (#110): a harness's coverage does notbelong in a client's denominator. Measured both ways round rather than reasoned about: the before
figures come from a
git stash push -uof this branch and a re-run.Security
pnpm-lock.yamland everypackage.jsonareuntouched;
check:licencesre-run and green.apps/web/browser/, whichvite.browser.config.tskeeps out of the product by being a second config, or in documentation.grep OYL_HOSTED apps/web/distfinds nothing.?track=, on a page that never ships. Itis validated and bounded, and refuses rather than falls back.
field and the constraint and never the value — ADR 0004 decision D, which binds every layer that
formats a coordinate into a string, and there is a test asserting the message carries no digit.
a hosted render is on the page's origin or the archive's, and nothing else — no telemetry, no font
fallback, no tile API arriving in a dependency default. On a loopback-only render that assertion
could not distinguish "contacts nothing else" from "cannot reach anything"; M18 shows this one can.
pnpm audit --audit-level=moderatereports one moderate, pre-existing and untouched:GHSA-w5hq-g745-h8pqinuuid, reached throughapps/mobile > @capacitor/cli > xcode. Build-timeonly, under
apps/, in no distributed closure, and not introduced here. Reported rather than fixed— a Capacitor bump is
check:capacitor's business and its own change.cross-athlete read, no persistence, no file parsing of user input, no trainer control.
Deliberately not done, and one finding filed here rather than fixed
the existing
Repository rulesjob — never a second job, which reports under a different contextand could not block a merge.
absolute millisecond gate against somebody else's CDN is flaky, a flaky gate gets disabled, and a
disabled gate is how a performance claim survives with nobody re-running it. Only sanity bounds —
the page cannot paint before it began loading, the client's share cannot exceed the whole, and
every archive response must be a
206rather than a request for the whole 19 GB object.cf-cache-status: DYNAMICis Publish and serve the PMTiles basemap and routing engine #53's to act on, and a comment saying so ison this PR's spike rather than in a change to somebody else's bucket.
CLAUDE.md§4a and §4c do not listpnpm run check:cost-modelorbash scripts/check-cost-model.test.sh, and CI runs both. That is pre-existing drift from Build the cost model and the self-hosting story #54 /feat: model what "free to the end user" costs, and gate the arithmetic (#54) #315, not from this change, and §4c's own rule says the file gets fixed in the PR that adds the
step. Reported rather than fixed here, because editing another change's command list is a drive-by
that would conflict with whoever does fix it. Both were run on this branch and are in the table
above.
basemapStylestill emits noglyphsand nosprite, because both wouldbe third-party origins today. The basemap in this measurement carries no text at all — Publish and serve the PMTiles basemap and routing engine #53's
outstanding half, recorded in
apps/web/src/map/basemap.ts.🤖 Generated with Claude Code
https://claude.ai/code/session_016X4EwacF1KuXDPhXzunifS