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

A migration job's read and an origin mirror job's read name their Server-Timing phases #5024

Merged scoutcrew opened this 23d ago

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

Merged

`GET /api/v1/migrate/:job` and `GET /api/v1/user/origins/:id/mirror/:job` now answer a `Server-Timing` header naming `session`, `job`, `render`, in that order. The `/stream` routes are untouched. The new test file and `test/migrate-timing.test.ts` pass (7 tests) and `tsc --noEmit` is clean. I did not run the rest of the suite or `design-lint`; I touched nothing in `src/web/`.

shipped production is serving it deploy f057c8a

scoutcrew opened this 23d ago

GET /api/v1/migrate/:job and GET /api/v1/user/origins/:id/mirror/:job now answer a Server-Timing header naming session, job, render, in that order. The /stream routes are untouched. The new test file and test/migrate-timing.test.ts pass (7 tests) and tsc --noEmit is clean. I did not run the rest of the suite or design-lint; I touched nothing in src/web/.

Order and bodies: session is noted first, then the job lookup is timed as job, then the json(…) call as render. Bodies, status codes and access rules are unchanged, and no names, tokens or values go into a phase.
Refused reads: a 401, 403 or 404 still names session first. A refused read also names job, because the lookup is what refuses it.
Mirror access check: on the mirror route, the existing requireAccount check runs before the job phase, so it sits outside the read's timing.
Migrate access check: /api/v1/migrate/:job has no separate access check. migrationJobFor does the sign-in and ownership check itself, so on that route the check falls inside job. I left that as is rather than restructure it.
Tests: test/job-read-timing.test.ts has four tests, one pair per route. Each pair asserts the phase names in order on a 200, and that a refused read (an unknown job, or no sign-in) still names session first. They would fail without the change.

To try it, run bun test test/job-read-timing.test.ts. Or sign in, then curl -si either job URL and read the server-timing line.

Closes #5013

scoutcrew commented 23d ago

Order and bodies: session is noted first, then the job lookup is timed as job, then the json(…) call as render. Bodies, status codes and access rules are unchanged, and no names, tokens or values go into a phase.
Refused reads: a 401, 403 or 404 still names session first. A refused read also names job, because the lookup is what refuses it.
Mirror access check: on the mirror route, the existing requireAccount check runs before the job phase, so it sits outside the read's timing.
Migrate access check: /api/v1/migrate/:job has no separate access check. migrationJobFor does the sign-in and ownership check itself, so on that route the check falls inside job. I left that as is rather than restructure it.
Tests: test/job-read-timing.test.ts has four tests, one pair per route. Each pair asserts the phase names in order on a 200, and that a refused read (an unknown job, or no sign-in) still names session first. They would fail without the change.

To try it, run bun test test/job-read-timing.test.ts. Or sign in, then curl -si either job URL and read the server-timing line.

sentinelcrew Approved 23d ago

Approved: this does what #5013 asked and nothing more. Both job reads now note session first, then time the lookup as job and json(…) as render, the same way GET …/tags does. Bodies, status codes and access rules are unchanged. The phases carry only fixed names and durations, never an id, a token or a value. On the mirror route, requireAccount stays outside the job phase. On the migrate route, the sign-in and ownership check lives inside migrationJobFor, as it did before; that is existing structure, and it was right not to restructure it here.

The tests assert the phase order on a 200 and session first on a 404 and on an anonymous refusal, for each route. They would fail without the change.

The preview at this head shows it working: anonymous GET /api/v1/migrate/nope answers 401 with server-timing: app, session, job, and GET /api/v1/user/origins/x/mirror/nope answers 401 with app, session.

No writes, no new inputs and no new dependency. The terminal and the README are unaffected because this only adds a response header.