perf(httpapi): GET /instances forks one git rev-list per active row for commits_behind #152

New issue

Open

opened 2026-07-13 00:07:26 +02:00 by dominik.polakovics · 0 comments

dominik.polakovics commented

2026-07-13 00:07:26 +02:00

Owner

Raised while landing PR #150 (issue #149). Not a bug today — filing for a maintainer call on whether the cost is acceptable as the fleet grows.

What

The commits_behind badge is computed per row on the instances dashboard:

// internal/httpapi/instances.go:104-105
run := runJSON(v.Run)
run.CommitsBehind = s.commitsBehind(r.Context(), v.Run, defaultBranch[v.Run.RepoID])

and Server.commitsBehind (internal/httpapi/runs.go:112-121) shells out to gitx.CommitsBehind → git rev-list --count. So GET /api/v1/instances forks one git subprocess per active instance row, every time it is called.

The accompanying comment says:

a single Repos() call mapped by id keeps this O(1) queries, not N+1

That is true of the SQL — and the comment is a fair description of what it set out to fix — but it reads as though the endpoint is O(1) overall, which it is not: the git subprocesses are genuinely per-row. Whatever is decided here, that comment should be corrected, because it currently vouches for a property the endpoint does not have.

Why it might matter

/instances is a hot endpoint: the SPA refetches it on every run.changed (which #149 itself newly publishes from crmerge on every CR merge) and on debounced run.messages.changed. So the fork count is active_instances × refetch_rate, and #149 added a new event source driving that rate.

Today active instances are few and rev-list --count on a bare repo is milliseconds, so this is comfortably fine. It gets less comfortable with a larger fleet or a chattier event stream.

Options (for triage)

Accept as-is and just fix the misleading comment.
Cache the count per run with a short TTL, the way the command catalog is memoized (commandsCacheTTL) — the count only changes when a fetch advances a base ref, so it is very cacheable.
Recompute on the fetch cadence instead of in the request path: store the count when the periodic fetch updates the refs, and serve it from the store. This matches the existing "freshness rides the fetch cadence" contract that commitsBehind already documents, and takes git out of the request path entirely.

No action needed if the answer is "fine at this scale" — but the comment fix should happen either way.

Raised while landing PR #150 (issue #149). Not a bug today — filing for a maintainer call on whether the cost is acceptable as the fleet grows. ## What The `commits_behind` badge is computed per row on the instances dashboard: ```go // internal/httpapi/instances.go:104-105 run := runJSON(v.Run) run.CommitsBehind = s.commitsBehind(r.Context(), v.Run, defaultBranch[v.Run.RepoID]) ``` and `Server.commitsBehind` (`internal/httpapi/runs.go:112-121`) shells out to `gitx.CommitsBehind` → `git rev-list --count`. So **`GET /api/v1/instances` forks one `git` subprocess per active instance row**, every time it is called. The accompanying comment says: > a single `Repos()` call mapped by id keeps this O(1) queries, not N+1 That is true of the **SQL** — and the comment is a fair description of what it set out to fix — but it reads as though the endpoint is O(1) overall, which it is not: the git subprocesses are genuinely per-row. Whatever is decided here, that comment should be corrected, because it currently vouches for a property the endpoint does not have. ## Why it might matter `/instances` is a hot endpoint: the SPA refetches it on every `run.changed` (which #149 itself newly publishes from crmerge on *every* CR merge) and on debounced `run.messages.changed`. So the fork count is `active_instances × refetch_rate`, and #149 added a new event source driving that rate. Today active instances are few and `rev-list --count` on a bare repo is milliseconds, so this is comfortably fine. It gets less comfortable with a larger fleet or a chattier event stream. ## Options (for triage) - **Accept as-is** and just fix the misleading comment. - **Cache** the count per run with a short TTL, the way the command catalog is memoized (`commandsCacheTTL`) — the count only changes when a fetch advances a base ref, so it is very cacheable. - **Recompute on the fetch cadence** instead of in the request path: store the count when the periodic fetch updates the refs, and serve it from the store. This matches the existing "freshness rides the fetch cadence" contract that `commitsBehind` already documents, and takes git out of the request path entirely. No action needed if the answer is "fine at this scale" — but the comment fix should happen either way.

dominik.polakovics added the

needs-triage

label

2026-07-13 00:07:26 +02:00

dominik.polakovics referenced this issue

2026-07-19 00:01:15 +02:00

tracker: full-history PR walks (reaper done-signal, labctl pr list) grow unboundedly and hammer the forge #176