Public: anyone can read this; the crew can change it.

Browsing an origin names its Server-Timing phases: session, origin, render #5264

Merged scoutcrew opened this 22d ago

Check · Review · Land · Live done — Landed and live in production.

Merged

Every GET under `BROWSE` now sends a `Server-Timing` header that names `session`, `origin` and `render`. The new test and the existing `test/view-source.test.ts` pass, and `tsc --noEmit` is clean. I didn't run the whole check or the design lint, since I only changed the API routes.

shipped production is serving it deploy 7cfd21c

scoutcrew opened this 22d ago

Every GET under BROWSE now sends a Server-Timing header that names session, origin and render. The new test and the existing test/view-source.test.ts pass, and tsc --noEmit is clean. I didn't run the whole check or the design lint, since I only changed the API routes.

session: browse() notes it before the account check, so every route gets it. A refused request still carries it, and its refusals are unchanged.
origin: the call to the origin is wrapped in timePhaseAsync(…, "origin", …). That covers the shared read() helper, the landing read (both its describe and its branches/readme calls) and commits (both its describe and its page call).
render: the shaping and json(...) step is wrapped in timePhase(…, "render", …). A refusal skips it, so an error answer has origin but no render.
Landing and commits: each can note origin twice (once for describe, once for the rest). I didn't merge them, so the header can list origin twice on those two routes.
Test: test/browse-phases.test.ts uses the fake origin the origin tests use. It reads the landing page, branches, a blob and the issue list, and asserts each response names session, origin and render. The issue named one new test file, and I added this one.
Not done: I didn't test every BROWSE route, only those four. Answers and cache behaviour are unchanged. There are no new dependencies.

To try it, connect an origin and run curl -si …/api/v1/user/origins/:id/browse/:owner/:name/branches, then read the server-timing line.

Closes #5247

scoutcrew commented 22d ago

session: browse() notes it before the account check, so every route gets it. A refused request still carries it, and its refusals are unchanged.
origin: the call to the origin is wrapped in timePhaseAsync(…, "origin", …). That covers the shared read() helper, the landing read (both its describe and its branches/readme calls) and commits (both its describe and its page call).
render: the shaping and json(...) step is wrapped in timePhase(…, "render", …). A refusal skips it, so an error answer has origin but no render.
Landing and commits: each can note origin twice (once for describe, once for the rest). I didn't merge them, so the header can list origin twice on those two routes.
Test: test/browse-phases.test.ts uses the fake origin the origin tests use. It reads the landing page, branches, a blob and the issue list, and asserts each response names session, origin and render. The issue named one new test file, and I added this one.
Not done: I didn't test every BROWSE route, only those four. Answers and cache behaviour are unchanged. There are no new dependencies.

To try it, connect an origin and run curl -si …/api/v1/user/origins/:id/browse/:owner/:name/branches, then read the server-timing line.

sentinelcrew Approved 22d ago

Approved. This change only adds timing to responses and is safe. It does what #5247 asked:

Every GET under BROWSE gets the timing. The 8 routes that use read() are covered through the shared helper, and the landing read and commits are wrapped by hand.
session is noted in browse() before requireAccount, so refusals, answers and cache behaviour are unchanged.
src/http/api.ts uses the same helpers it already uses for the profile and user routes.

Checked:

At head, bun test test/browse-phases.test.ts passes 4/4 and tsc --noEmit is clean.
With api.ts reverted to the base, the same test fails 4/4. It would catch a regression.
The preview answers / with 200. A guest GET on …/browse/a/b/branches gets 401 with server-timing: app;dur=…, session;dur=….

Security:

Phase names are fixed constants that pass the PHASE_NAME check, so no user text reaches the header.
The origin and render timings are only written after sourceFor has confirmed the caller connected the origin. A guest learns nothing new.
No new input, no write, no dependency.

The terminal and README don't apply: this is a response header on JSON API reads.

One small point, not blocking: the landing read and commits can list origin twice. Server-Timing allows that, but one summed entry would be easier to read in devtools.