chadtmiller.com/knotless

Switch repository

Loading…

What changes

A pull's merge box answers from what the server holds. getMergeStatus decides with the same code as knotless merge --check, but reads the repository as its mirror last synced, both sides through pull_range, and the pull, its reviews and its checks from the index, instead of opening the repository, the pull's head and every runner's checks from their PDSes again.

landing::mergeable is split into gauge (method, conflicts, reviews, who may, which workflows a merge waits on) and decide (where the checks leave it, a draft, why someone may not merge). The CLI, MCP's merge check and the gateway's gate keep reading afresh and call both; serve gathers its own inputs and calls the same two.

forge::Facts is what those decisions read of a repository (URL, owner, key, record, private), so approvals::of, spent, head_here, may and org::needs_public take it rather than a whole Target.

Why

On prod every merge box took about 4 s: 428 of 449 calls over 2 s in three days. Traced with outgoing calls at debug, one check is ~25 PDS round trips of ~150 ms one after another (the repository's snapshot twice, its head repository again, five pages of the whole repository's getForge, each runner's PLC document and check pages), while the git work, merge-tree included, is ~50 ms. Serve already has all of it in its mirror and index.

Running the same reads in parallel was the other way, perhaps 1 to 1.5 s, but still reads the PDS for every page.

How it was checked

Benchmarked on prod against an open pull, 20 calls as the owner with a fresh service token each (token minting not timed): before, p50 4.19 s, p90 4.54 s, min 3.89 s. After it deploys, the same script runs again and the numbers go in a comment here.

The pages tests (landing_pulls_from_the_pages, private_flow) already ask for the merge status through each state (clean, pending, blocked, undelegated, a private repository's reader and owner); one more asks for a pull there is none of, which is read once from its author and answered 404. Left to CI.

Shortcomings

The box can now lag a review or a check written a moment ago, until the index hears of it (seconds); a merge still reads them afresh, so it never merges past one. A pull asked about that is not open answers 400, where it was 502. Comments written a moment ago by maintainers, which merge --check reads from their repos beside the index, are not read here.

Review

The design: crates/knotless/src/landing.rs (gauge and decide), then mergeable in crates/knotless-serve/src/web/landing.rs.

Changeset: none

🤖 Generated with Claude Code

This pull makes getMergeStatus answer from the server's mirror and index. It splits landing::mergeable into gauge and decide, so the CLI, MCP and gateway paths keep their fresh reads through the same two functions. forge::Facts replaces passing a whole Target to the decision helpers.

I found nothing that must change. The refactor keeps the order of the old logic: reviews refuse before checks, the draft state and the "may not merge" reason are applied last, and a conflict or a repository with no runners still skips the checks. Merge itself still reads reviews and checks afresh, so the lag in the box can't merge past a newer review or check. The new 404 test covers the missing-pull path, and docs/design/serve.md is updated.

Things to confirm before merging:

• The check and test runs were still running and docs was queued when I looked, so their results aren't in this review. Wait for all six to pass.
• The new 400 for a pull that isn't open (it was 502) changes the API's behaviour. If docs/reference/api.md lists the errors of getMergeStatus, update it to match.
• The speed-up is only benchmarked "before". Posting the after numbers, as the body promises, would confirm it.

The new commit fixes the failure that ejected the pull from the queue. The approvals::of call in knotless-run's batch review gate (queue.rs) still passed a Target. It now passes on.target.facts(). That matches the new signature and doesn't change behaviour. My earlier review found nothing else to change.

Still open from the first round:

• check and test are still running on this head. They must pass, since check is what failed before.
• Update docs/reference/api.md if it lists the errors of getMergeStatus, because a pull that isn't open now answers 400 instead of 502.
• Post the after-deploy benchmark numbers, as the body promises.

I found nothing new to change. The pull makes getMergeStatus read the server's mirror and index, and it splits landing::mergeable into gauge and decide.

• Docs: docs/reference/api.md line 574 now lists 400 for a pull that isn't open and 404 for none, so that earlier request is done.
• Queue-gate fix: the queue.rs fix from round 2 is still in.
• Checks: on the new head 2c9b94b, check and test are still running. On the previous head, test failed in its test step after about 267 s. I couldn't tell from here whether that was a timeout, a flake or a real failure, so make sure test passes on this head before it merges.
• Benchmark: the after-deploy numbers the body promises are still to post as a comment.

Benchmarked on prod after the deploy, the same script as before (20 calls to getMergeStatus on an open pull, as the owner, from a Mac):

A bare describeServer from the same Mac takes ~0.55 s, nearly all of it TCP and TLS, so the client numbers after are mostly the connection: the server's own time fell from ~4.1 s to ~135 ms, about 30 times faster. The first call after the deploy, 774 ms in serve, read the repository's mirror and index cold.