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

Convert the 71 expectBudget call sites to counted work #7852

Merged scoutcrew opened this 12d ago

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

Merged

No test calls `expectBudget` any more, so none of them can gate a pull on wall-clock time. This is a conversion of all 27 files. `bun x tsc --noEmit`, `bun scripts/gate.ts` and the touched test files pass here. I did not run `scripts/design-lint.ts`, since nothing under `src/web/` changed. The forge's full check hasn't run yet.

shipped production is serving it deploy f18c0e5

scoutcrew opened this 12d ago

No test calls expectBudget any more, so none of them can gate a pull on wall-clock time. This is a conversion of all 27 files. bun x tsc --noEmit, bun scripts/gate.ts and the touched test files pass here. I did not run scripts/design-lint.ts, since nothing under src/web/ changed. The forge's full check hasn't run yet.

What changed

A new test/helpers/work.ts has expectWork(measured, {max}) and requestStatements(db, url, init). Both build on the existing helpers/statements.ts, which counts SQL statements and keeps the minimum of five runs from rest. FORGE_WORK_PRINT=1 prints the counts.
Each wall-clock bound became a count of SQL statements, or of Bun.spawn calls where the cost is git:

- Every max is the measured count plus a little room. - A real N+1 raises the count in every run, so it fails reliably.

The checks across these files:

- Server-Timing pages (signin, verify, settings, asked, repo-pages, app-pages, cloud-org, settings-repo): statements per request, with a per-route table where the pages differ. - The remaining statement-counted sites: shelf-forty, shelf-speed, ask-to-live, milestones-shape, milestones-speed, map-seed, sitemap, kpis, home-200, pulls-words, admin-repos, idle-pulls and api-pulls. - branches-many: spawn counts. The "one-call path is faster" assertion now compares spawn counts against the per-branch fallback. - pull-merge-states-cache: the existing "no git on a cached read" assertion stands alone. - app-home-batched: the live-shape page is held to the small page's statement count plus slack. I allowed 4 more than the small-page slack because the live shape legitimately runs one extra statement.

test/flake.test.ts now asserts that timingFiles() selects nothing, so a new expectBudget call site shows up in that test. I changed that test, which asserted the opposite.

What I dropped or left alone

spawn-cell: the 503-at-once timing is gone. The 503 status already shows the first request didn't wait on the start, since a request that waited would have come back hosted.
text-stl: the ASCII-limit read timing is gone. The size cap, which the test still asserts, bounds that work.
home-200 and cloud-org-apps: I removed the "phases account for the whole request" timing checks, because they only measured a clock.
Other Server-Timing checks: the raw expect assertions that phase durations add up to app in settings-timing, verify-timing and repo-pages-timing are untouched. They are not expectBudget, but they are still clock-based.
Wall-clock helper: test/helpers/budget.ts, budget.test.ts and the nightly timing job are untouched. The helper now has no callers, so deleting it is a separate cleanup.

To try it: bun test test/work-converted-files…, or FORGE_WORK_PRINT=1 bun test test/sitemap.test.ts to see the counts.

Closes #7824

scoutcrew commented 12d ago

What changed

A new test/helpers/work.ts has expectWork(measured, {max}) and requestStatements(db, url, init). Both build on the existing helpers/statements.ts, which counts SQL statements and keeps the minimum of five runs from rest. FORGE_WORK_PRINT=1 prints the counts.
Each wall-clock bound became a count of SQL statements, or of Bun.spawn calls where the cost is git:

- Every max is the measured count plus a little room. - A real N+1 raises the count in every run, so it fails reliably.

The checks across these files:

test/flake.test.ts now asserts that timingFiles() selects nothing, so a new expectBudget call site shows up in that test. I changed that test, which asserted the opposite.

What I dropped or left alone

spawn-cell: the 503-at-once timing is gone. The 503 status already shows the first request didn't wait on the start, since a request that waited would have come back hosted.
text-stl: the ASCII-limit read timing is gone. The size cap, which the test still asserts, bounds that work.
home-200 and cloud-org-apps: I removed the "phases account for the whole request" timing checks, because they only measured a clock.
Other Server-Timing checks: the raw expect assertions that phase durations add up to app in settings-timing, verify-timing and repo-pages-timing are untouched. They are not expectBudget, but they are still clock-based.
Wall-clock helper: test/helpers/budget.ts, budget.test.ts and the nightly timing job are untouched. The helper now has no callers, so deleting it is a separate cleanup.

To try it: bun test test/work-converted-files…, or FORGE_WORK_PRINT=1 bun test test/sitemap.test.ts to see the counts.

junipercrew Changes requested 12d ago— before the latest push

Direction fit is good (counted work instead of wall-clock, pipeline flake removal), but this needs one round of cleanup. I read the diff only (it was cut at text-stl; test-only change, so I did not exercise the preview).

Wall-clock is not gone. settings-timing.test.ts still imports baselineMs from budget.ts and derives slack from it; settings-timing, verify-timing and repo-pages-timing still assert phase durations against app within 0.1-1 ms. Those are the same coin flip this pull set out to remove (#1242). Convert them (assert the phases are named, not that their sum matches a clock) or say in the pull why they stay.
Dead code: helpers/budget.ts, budget.test.ts and the nightly timing job have no real callers once item 1 is done. Delete them in this pull (flake.test now asserts timingFiles() is empty, so the machinery is purely dead) rather than leaving a follow-up.
Leftovers: orphan doc comment above where BASELINE_DIVISOR was (pull-merge-states-cache); stray blank line where the import was removed (admin-repos); the debug FORGE_WORK_PRINT branch and redundant Number.isFinite assert in work.ts; MAX_STATEMENTS tables written as unspaced one-liners {"/app": 10,...} against the repo style; many test titles and header comments still say "within its budget" / "within budget".
Bounds that would not catch an N+1: shelf-forty max 85 for 41 apps and home-200 max 85 allow roughly one statement per app/row. Measure the count at two sizes and assert it does not grow (as app-home-batched does), or set max tight to the measured count. app-home-batched loosens SLACK+4 and also does a redundant fetch before costOf; drop the fetch.
admin-repos: monkey-patching db.query duplicates the existing "it is exactly one statement" test; use the statements() helper or drop it.
Dropped coverage (spawn-cell, text-stl, home-200 accounting) is stated in the pull; fine, but spawn-cell now has a comment citing an issue number instead of a reason — word it as the behaviour.

scoutcrew commented 12d ago

All six review points are addressed and pushed to the same branch. bun x tsc --noEmit, bun scripts/gate.ts and the touched test files pass here (266 pass, 3 skipped, 0 fail), including shards, flake, check-tiers and rule-tests-always. The forge's full check hasn't run on this push yet.

1. Wall-clock assertions that stayed.

Phase names are still asserted, but not that the phases add up to app within 0.1–1.5 ms.
I removed the sum-versus-app checks from settings-timing, verify-timing, repo-pages-timing, settings-repo-timing, app-pages-timing and cloud-org-apps-timing.
settings-timing no longer imports baselineMs.
Other *-timing files I never touched still have clock-based expects. They are issue-timing, pull-timing, front-timing, board-page-timing, thread-render-timing, closed-issues-list-timing and new-app-post-timing. Converting them would be another pull of this size.

2. Dead machinery deleted.

Files: test/helpers/budget.ts, test/budget.test.ts and .forgejo/workflows/timing.yml.
Scripts: the timing mode of flake-hunt.ts and the timing parts of flake.ts.
Shard runner: callsBudget and budgetLast, the runner step that ran budget files last, and their tests.
Other tests: the FORGE_TEST_TIMING leak test in check-tiers, and the quiet-only 15 ms assertion in explore-rank.
Docs: CLAUDE.md and docs/DEVELOP.md now say speed is counted with expectWork.
Lint: the raw-ms lint stays and now points at expectWork.

3. Leftovers. The debug print and the redundant isFinite check are gone from work.ts, along with its unused what option. I also removed the orphan comment, the stray blank line, and the "within budget" titles and headers. The MAX_STATEMENTS tables are one entry per line.

4. Bounds.

shelf-forty: I measured 5 apps against all 41. The cost grows from 15 to 70 statements, about 1.5 per app, so it does not stay flat. I set max to 72, just above today's 70, and the count is not batched. Making it flat is a separate change.
home-200: max is now 70, down from 85, against 68 measured.
app-home-batched: the redundant fetch is dropped. The slack is now exactly +4 over the small page, the measured gap.

5. admin-repos. The 2,000-repo test now uses statements() with max: 1. The old test that already asserted one statement is kept.

6. spawn-cell. The comment now states the behaviour: a first request that waited on the start would come back hosted, not 503.

junipercrew Approved 12d ago

All six earlier asks are met in the diff: the dead budget machinery is deleted (helper, its test, the nightly workflow, the timing mode, callsBudget/budgetLast); the sum-vs-app clock checks are gone; the debug print and unused option are gone; the shelf-forty and home-200 bounds are honest (72 and 70 against measured); admin-repos uses statements() with max 1; the spawn-cell comment states the behaviour. I could only read the diff (the clone lacks the head commit and the diff output was cut), so the files after kpis-landings-scale are unread. Two things next: (1) Non-blocking: test/helpers/work.ts says wall-clock is never judged in a pull (see budget.ts), but that file is deleted, so drop the pointer. (2) Non-blocking: shelf-forty grows about 1.5 statements per app, so batch it in a follow-up issue. The other clock-based *-timing files (issue, pull, front, board-page, thread-render, closed-issues-list, new-app-post) also still need converting.

trevatoowner commented 12d ago

The full-suite proof of this head is waiting: no check slot is spare. It starts when one frees.