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

GET a repository's branches compares only the branches it returns: `git` under 60 ms for a 30-branch page of trevato/forge #7054

Merged scoutcrew opened this 17d ago

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

Merged

`GET /api/v1/repos/:owner/:name/branches` now lists the refs, takes the page, and computes ahead/behind only for the rows it returns. The new 200-branch test did not fail on main, so I haven't shown the speedup. The type check and the branch tests I ran pass: `branches-many`, `branches`, `api-branches-timing`, `mcp-branches` and `branches-timing`. I did not run the full check.

shipped production is serving it deploy 4915f35

scoutcrew opened this 17d ago

GET /api/v1/repos/:owner/:name/branches now lists the refs, takes the page, and computes ahead/behind only for the rows it returns. The new 200-branch test did not fail on main, so I haven't shown the speedup. The type check and the branch tests I ran pass: branches-many, branches, api-branches-timing, mcp-branches and branches-timing. I did not run the full check.

What changed

src/git/branches.ts:

- listBranchRows returns names and tips in the same order as before, with no comparison. - compareBranches compares just the rows it is given. - listBranches now calls those two, so its other callers (the web page, MCP, TUI, flow alarm) behave as before. - The bulk for-each-ref %(ahead-behind:) call now names only the page's refs, and the per-branch rev-list fallback still works.

src/http/api.ts: the route finds the cursor in the cheap list, slices the page, then compares that slice. Rows, order, next_cursor and the invalid_cursor 422 are unchanged. The session, git and render timing phases are still named in that order.
test/branches-many.test.ts: the new test has 200 branches and limit=30. It asserts the git phase is under 60 ms, using the repo's expectBudget helper. It also checks that the page's rows, ahead/behind counts and next_cursor equal the first 30 of the full list, and that the next page starts at row 31.

The test passes on main too Git's bulk ahead-behind is already fast on this machine, so the git phase on main was inside 60 ms here. The 356 ms in the issue was probably from the larger trevato/forge repo or a slower git. The new test therefore protects the budget and the row equivalence, but it does not fail on main, so the third "done when" box is not met as written.

Left out

The 200-branch comparison cap is unchanged. Past 200 branches, ahead/behind are still null on every page. Now that comparison is per page, the cap could be lifted, but that would change the returned rows, so I did not.
I did not build #3376's batched compare. This reuses the existing bulk ahead-behind path.

Try it Run bun test test/branches-many.test.ts, or request /api/v1/repos/<owner>/<repo>/branches?limit=30 and read git;dur in the Server-Timing header.

Closes #6225

scoutcrew commented 17d ago

What changed

src/git/branches.ts:

src/http/api.ts: the route finds the cursor in the cheap list, slices the page, then compares that slice. Rows, order, next_cursor and the invalid_cursor 422 are unchanged. The session, git and render timing phases are still named in that order.
test/branches-many.test.ts: the new test has 200 branches and limit=30. It asserts the git phase is under 60 ms, using the repo's expectBudget helper. It also checks that the page's rows, ahead/behind counts and next_cursor equal the first 30 of the full list, and that the next page starts at row 31.

Left out

The 200-branch comparison cap is unchanged. Past 200 branches, ahead/behind are still null on every page. Now that comparison is per page, the cap could be lifted, but that would change the returned rows, so I did not.
I did not build #3376's batched compare. This reuses the existing bulk ahead-behind path.

Try it Run bun test test/branches-many.test.ts, or request /api/v1/repos/<owner>/<repo>/branches?limit=30 and read git;dur in the Server-Timing header.

sentinelcrew Changes requested 17d ago— before the latest push

The split is sound and safe. listBranchRows and compareBranches keep listBranches's rows and order for its other callers (web, MCP, TUI, flow). Every ref name passed to the bulk for-each-ref goes through isBranchName and comes after --. The base is an oid from git. argv is bounded by limit ≤ 200. The cursor/422 contract is unchanged. branches-many and api-branches-timing pass at this head (9/9).

What does not meet the bar: this pull changes no behaviour except speed, and no test pins the speed. The new test is green on main too, as the description says, so it would not fail without the change. The issue's third done-when (git under 60 ms for a 30-branch page of trevato/forge, where main measured 356 ms) is unproven. The preview has no repositories, so it cannot show this either.

Fix first: add a test that fails on main. A fixture of 200 branches each one commit off main costs git almost nothing, which is why 60 ms passes on both sides. Make the comparison expensive instead: put the branches deep in history, e.g. a few thousand commits on main with branches forked at spread-out points. Then assert that the git phase for limit=30 is well under the phase for limit=200 on the same repo. That ratio holds at this head and is about 1 on main, because main compares all 200 either way. Use best-of-three, as the fallback test in this file already does, so a loaded machine cannot flip it. Alternatively, assert the absolute 60 ms budget against a fixture that takes over 60 ms on main.

One smaller point is inline: an empty page compares every branch.

scoutcrew commented 17d ago

Both review points are fixed and pushed. The type check and the branch tests I ran pass (29 across five files). I did not run the full check.

Test that fails on main: the new test/branches-deep.test.ts builds a repo with 3000 commits on main and 199 branches. Each branch has 150 commits of its own, forked at spread-out points, so every comparison costs a real walk. It asserts that the git phase for limit=30 is under 0.6× the phase for limit=200 on the same repo, best of three. I ran that assertion against main's route alone, with the new empty-page test left out because main has no compareBranches to import. It fails there: the 30-row page took 268 and 272 ms against limits of 164 and 153 ms. At this head it passes.
Why the fixture is built this way: a first try with only 3000 to 12000 commits on main and one commit per branch was dominated by the walk of main. It gave a ratio of about 0.8 and would not have separated the two. Giving each branch its own history made the per-branch cost dominate.
Empty page: aheadBehindAll in src/git/branches.ts now returns an empty map for an empty page without calling git, so it no longer compares the whole repo. A test pins that compareBranches with no rows returns no branches. It checks the return value only, not that git was never run.
Not met: the issue's literal "git under 60 ms for trevato/forge" is still not shown. Only the ratio is pinned, and the preview has no repositories to measure.

sentinelcrew Approved 17d ago— before the latest push

Approved. The read path stays behind the same gates: loadRepo runs before the route lists anything, and limit and cursor are checked as before. The refs passed to for-each-ref come from the repository's own branch list, they follow --, and each one must pass isBranchName first. If any name fails that check, the page falls back to per-branch rev-list, which gives the same answer more slowly. Rows, order, next_cursor and invalid_cursor are unchanged, and the new test checks them against the full list.

At head 40ed2f2, bun test test/branches-deep.test.ts test/branches-many.test.ts passes: 9 tests in 7.3 s. The deep fixture's ratio check is a real regression guard; the builder showed it fails on main's route.

The preview answers / and its security headers are in place. It has no repositories, so the branches route could only be checked for a 404 there. The issue's literal "under 60 ms on trevato/forge" is not shown, as the pull says itself. One non-blocking note is on the empty-page test.

trevatoowner Changes requested 17d ago— before the latest push

The test "…200 branches, limit=30, git under 60 ms" is a raw ms budget (#4429), the same rule that sank three batches at 15:02–15:09Z. Assert the work (git spawned once, compares only the 30 returned) or use expectBudget(), and take the ms number out of the title.

scoutcrew commented 17d ago

I pushed the changes to the branch. The type check and bun test test/branches-deep.test.ts test/branches-many.test.ts pass (9 tests).

Raw ms budget: I think the test the review quotes was already on expectBudget(), not a raw ms assertion. It judges the 60 ms against a /healthz baseline from the same run. What was wrong was the title, which carried the number. It now reads "the API returns the first 30 of 200 branches with the whole list's counts, and its git phase fits the budget". The work-based checks are unchanged: rows, order, ahead/behind and next_cursor equal the first 30 of the full list, and the second page starts at row 31. I did not add an assertion on how many times git is spawned.
Ratio test: I added a comment that the loop runs four times and drops the first as warm-up, so the next reader doesn't "fix" it to three.
Empty-page test: it is now named "an empty page compares nothing", which is all it checks. It checks the return value, not that git was never called.

sentinelcrew Approved 17d ago

Approved. The route lists the refs once, finds the cursor in that cheap list, slices the page, and compares only the slice. listBranches is now made of listBranchRows plus compareBranches, so the web page, MCP, TUI and flow alarm get the same rows as before.

Security: there is no new write surface, and reads still go through loadRepo. The ref names passed to for-each-ref come after --, and each is checked with isBranchName before git runs, so no user text reaches a shell. limit stays capped at 200 by eventsLimit, so the argv is bounded. The cap still checks the total branch count (total), not the page size, so a repo with more than 200 branches still returns null ahead/behind, as before.

I checked out head b5a7b781 on its own. The type check is clean. These five test files pass (29 tests): branches-deep, branches-many, branches, api-branches-timing and mcp-branches. I ran branches-deep three more times and it passed each time. The 30-row page is judged relative to the 200-row page on the same repo, not against a raw ms number, which meets trevato's #4429 point. The 60 ms check uses expectBudget. The preview at this head has no repositories, so the endpoint can only be exercised by the suites. Its health check is up, and an unknown repo answers 404 with the security headers.

One small point, which needs no new round: see the line comment on the empty-page test.